Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

DEV: Merge category and tag hashtags code paths (#10216) 

Category and tag hashtags used to be handled differently even though
most of the code was very similar. This design was the root cause of
multiple issues related to hashtags.

This commit reduces the number of requests (just one and debounced
better), removes the use of CSS classes which marked resolved hashtags,
simplifies a lot of the code as there is a single source of truth and
previous race condition fixes are now useless.

It also includes a very minor security fix which let unauthorized users
to guess hidden tags.
This commit is contained in:
Dan Ungureanu 2020-07-13 19:13:17 +03:00 committed by GitHub
parent c5da813ff5
commit cf02c518b9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 190 additions and 300 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

55
app/assets/javascripts/discourse/app/components/composer-editor.js
View File

@ -12,13 +12,9 @@ import {
fetchUnseenMentions
} from "discourse/lib/link-mentions";
import {
linkSeenCategoryHashtags,
fetchUnseenCategoryHashtags
} from "discourse/lib/link-category-hashtags";
import {
linkSeenTagHashtags,
fetchUnseenTagHashtags
} from "discourse/lib/link-tag-hashtag";
linkSeenHashtags,
fetchUnseenHashtags
} from "discourse/lib/link-hashtags";
import Composer from "discourse/models/composer";
import { load, LOADING_ONEBOX_CSS_CLASS } from "pretty-text/oneboxer";
import { applyInlineOneboxes } from "pretty-text/inline-oneboxer";
@ -520,16 +516,13 @@ export default Component.extend({
});
},
_renderUnseenCategoryHashtags($preview, unseen) {
fetchUnseenCategoryHashtags(unseen).then(() => {
linkSeenCategoryHashtags($preview);
});
},
_renderUnseenTagHashtags($preview, unseen) {
fetchUnseenTagHashtags(unseen).then(() => {
linkSeenTagHashtags($preview);
});
_renderUnseenHashtags($preview) {
const unseen = linkSeenHashtags($preview);
if (unseen.length > 0) {
fetchUnseenHashtags(unseen).then(() => {
linkSeenHashtags($preview);
});
}
},
_loadInlineOneboxes(inline) {
@ -927,30 +920,10 @@ export default Component.extend({
this._warnMentionedGroups($preview);
this._warnCannotSeeMention($preview);
// Paint category hashtags
const unseenCategoryHashtags = linkSeenCategoryHashtags($preview);
if (unseenCategoryHashtags.length) {
debounce(
this,
this._renderUnseenCategoryHashtags,
$preview,
unseenCategoryHashtags,
450
);
}
// Paint tag hashtags
if (this.siteSettings.tagging_enabled) {
const unseenTagHashtags = linkSeenTagHashtags($preview);
if (unseenTagHashtags.length) {
debounce(
this,
this._renderUnseenTagHashtags,
$preview,
unseenTagHashtags,
450
);
}
// Paint category and tag hashtags
const unseenHashtags = linkSeenHashtags($preview);
if (unseenHashtags.length > 0) {
debounce(this, this._renderUnseenHashtags, $preview, 450);
}
// Paint oneboxes

61
app/assets/javascripts/discourse/app/lib/link-category-hashtags.js
View File

@ -1,61 +0,0 @@
import { schedule } from "@ember/runloop";
import { ajax } from "discourse/lib/ajax";
import { replaceSpan } from "discourse/lib/category-hashtags";
const validCategoryHashtags = {};
const checkedCategoryHashtags = [];
const testedClass = `hashtag-category-tested`;
function updateFound($hashtags, categorySlugs) {
schedule("afterRender", () => {
$hashtags.each((index, hashtag) => {
const categorySlug = categorySlugs[index];
const link = validCategoryHashtags[categorySlug];
const $hashtag = $(hashtag);
if (link) {
if ($hashtag.data("type") !== "tag") {
replaceSpan($hashtag, categorySlug, link, "category");
}
} else if (checkedCategoryHashtags.indexOf(categorySlug) !== -1) {
if (hashtag.tagName !== "A") {
$hashtag.addClass(testedClass);
}
}
});
});
}
export function linkSeenCategoryHashtags($elem) {
const $hashtags = $(`span.hashtag:not(.${testedClass}), a.hashtag`, $elem);
const unseen = [];
if ($hashtags.length) {
const categorySlugs = $hashtags.map((_, hashtag) =>
$(hashtag)
.text()
.substr(1)
);
if (categorySlugs.length) {
_.uniq(categorySlugs).forEach(categorySlug => {
if (checkedCategoryHashtags.indexOf(categorySlug) === -1) {
unseen.push(categorySlug);
}
});
}
updateFound($hashtags, categorySlugs);
}
return unseen;
}
export function fetchUnseenCategoryHashtags(categorySlugs) {
return ajax("/category_hashtags/check", {
data: { category_slugs: categorySlugs }
}).then(response => {
response.valid.forEach(category => {
validCategoryHashtags[category.slug] = category.url;
});
checkedCategoryHashtags.push.apply(checkedCategoryHashtags, categorySlugs);
});
}

51
app/assets/javascripts/discourse/app/lib/link-hashtags.js Normal file
View File

@ -0,0 +1,51 @@
import { schedule } from "@ember/runloop";
import { ajax } from "discourse/lib/ajax";
import { replaceSpan } from "discourse/lib/category-hashtags";
import { TAG_HASHTAG_POSTFIX } from "discourse/lib/tag-hashtags";
const categoryHashtags = {};
const tagHashtags = {};
const checkedHashtags = new Set();
export function linkSeenHashtags($elem) {
const $hashtags = $elem.find("span.hashtag");
if ($hashtags.length === 0) {
return [];
}
const slugs = [...$hashtags.map((_, hashtag) => hashtag.innerText.substr(1))];
schedule("afterRender", () => {
$hashtags.each((index, hashtag) => {
let slug = slugs[index];
const hasTagSuffix = slug.endsWith(TAG_HASHTAG_POSTFIX);
if (hasTagSuffix) {
slug = slug.substr(0, slug.length - TAG_HASHTAG_POSTFIX.length);
}
if (categoryHashtags[slug] && !hasTagSuffix) {
replaceSpan($(hashtag), slug, categoryHashtags[slug]);
} else if (tagHashtags[slug]) {
replaceSpan($(hashtag), slug, tagHashtags[slug]);
}
});
});
return slugs.uniq().filter(slug => !checkedHashtags.has(slug));
}
export function fetchUnseenHashtags(slugs) {
return ajax("/hashtags", {
data: { slugs }
}).then(response => {
Object.keys(response.categories).forEach(slug => {
categoryHashtags[slug] = response.categories[slug];
});
Object.keys(response.tags).forEach(slug => {
tagHashtags[slug] = response.tags[slug];
});
slugs.forEach(checkedHashtags.add, checkedHashtags);
});
}

62
app/assets/javascripts/discourse/app/lib/link-tag-hashtag.js
View File

@ -1,62 +0,0 @@
import { schedule } from "@ember/runloop";
import { ajax } from "discourse/lib/ajax";
import { replaceSpan } from "discourse/lib/category-hashtags";
import { TAG_HASHTAG_POSTFIX } from "discourse/lib/tag-hashtags";
const validTagHashtags = {};
const checkedTagHashtags = [];
const testedClass = "hashtag-tag-tested";
function updateFound($hashtags, tagValues) {
schedule("afterRender", () => {
$hashtags.each((index, hashtag) => {
const tagValue = tagValues[index];
const link = validTagHashtags[tagValue];
const $hashtag = $(hashtag);
if (link) {
if (!$hashtag.data("type") || $hashtag.data("type") === "tag") {
replaceSpan($hashtag, tagValue, link, $hashtag.data("type"));
}
} else if (checkedTagHashtags.indexOf(tagValue) !== -1) {
$hashtag.addClass(testedClass);
}
});
});
}
export function linkSeenTagHashtags($elem) {
const $hashtags = $(`span.hashtag:not(.${testedClass})`, $elem);
const unseen = [];
if ($hashtags.length) {
const tagValues = $hashtags.map((_, hashtag) => {
let text = $(hashtag).text();
if (text.endsWith(TAG_HASHTAG_POSTFIX)) {
text = text.slice(0, -TAG_HASHTAG_POSTFIX.length);
$(hashtag).data("type", "tag");
}
return text.substr(1);
});
if (tagValues.length) {
_.uniq(tagValues).forEach(tagValue => {
if (checkedTagHashtags.indexOf(tagValue) === -1) unseen.push(tagValue);
});
}
updateFound($hashtags, tagValues);
}
return unseen;
}
export function fetchUnseenTagHashtags(tagValues) {
return ajax("/tags/check", { data: { tag_values: tagValues } }).then(
response => {
response.valid.forEach(tag => {
validTagHashtags[tag.value] = tag.url;
});
checkedTagHashtags.push.apply(checkedTagHashtags, tagValues);
}
);
}

21
app/controllers/category_hashtags_controller.rb
View File

@ -1,21 +0,0 @@
# frozen_string_literal: true
class CategoryHashtagsController < ApplicationController
requires_login
def check
category_slugs = params[:category_slugs]
ids = category_slugs.map { |category_slug| Category.query_from_hashtag_slug(category_slug).try(:id) }
slugs_and_urls = {}
Category.secured(guardian).where(id: ids).each do |category|
slugs_and_urls[category.slug_path.last(2).join(':')] ||= category.url
end
render json: {
valid: slugs_and_urls.map { |slug, url| { slug: slug, url: url } }
}
end
end

48
app/controllers/hashtags_controller.rb Normal file
View File

@ -0,0 +1,48 @@
# frozen_string_literal: true
class HashtagsController < ApplicationController
requires_login
HASHTAGS_PER_REQUEST = 20
def show
raise Discourse::InvalidParameters.new(:slugs) if !params[:slugs].is_a?(Array)
all_slugs = []
tag_slugs = []
params[:slugs][0..HASHTAGS_PER_REQUEST].each do |slug|
if slug.end_with?(PrettyText::Helpers::TAG_HASHTAG_POSTFIX)
tag_slugs << slug.chomp(PrettyText::Helpers::TAG_HASHTAG_POSTFIX)
else
all_slugs << slug
end
end
# Try to resolve hashtags as categories first
category_slugs_and_ids = all_slugs.map { |slug| [slug, Category.query_from_hashtag_slug(slug)&.id] }.to_h
category_ids_and_urls = Category
.secured(guardian)
.select(:id, :slug, :parent_category_id) # fields required for generating category URL
.where(id: category_slugs_and_ids.values)
.map { |c| [c.id, c.url] }
.to_h
categories_hashtags = {}
category_slugs_and_ids.each do |slug, id|
if category_url = category_ids_and_urls[id]
categories_hashtags[slug] = category_url
end
end
# Resolve remaining hashtags as tags
tag_hashtags = {}
if SiteSetting.tagging_enabled
tag_slugs += (all_slugs - categories_hashtags.keys)
DiscourseTagging.filter_visible(Tag.where_name(tag_slugs), guardian).each do |tag|
tag_hashtags[tag.name] = tag.full_url
end
end
render json: { categories: categories_hashtags, tags: tag_hashtags }
end
end

9
app/controllers/tags_controller.rb
View File

@ -12,7 +12,6 @@ class TagsController < ::ApplicationController
:show,
:tag_feed,
:search,
:check_hashtag,
:info,
Discourse.anonymous_filters.map { |f| :"show_#{f}" }
].flatten
@ -283,14 +282,6 @@ class TagsController < ::ApplicationController
render json: { notification_level: level, tag_id: tag.id }
end
def check_hashtag
valid_tags = Tag.where_name(params[:tag_values]).map do |tag|
{ value: tag.name, url: tag.full_url }
end.compact
render json: { valid: valid_tags }
end
def personal_messages
guardian.ensure_can_tag_pms!
allowed_user = fetch_user_from_params

3
config/routes.rb
View File

@ -692,7 +692,7 @@ Discourse::Application.routes.draw do
get "/" => "list#category_default", as: "category_default"
end
get "category_hashtags/check" => "category_hashtags#check"
get "hashtags" => "hashtags#show"
TopTopic.periods.each do |period|
get "top/#{period}.rss" => "list#top_#{period}_feed", format: :rss
@ -887,7 +887,6 @@ Discourse::Application.routes.draw do
get '/' => 'tags#index'
get '/filter/list' => 'tags#index'
get '/filter/search' => 'tags#search'
get '/check' => 'tags#check_hashtag'
get '/personal_messages/:username' => 'tags#personal_messages'
post '/upload' => 'tags#upload'
get '/unused' => 'tags#list_unused'

7
lib/pretty_text/helpers.rb
View File

@ -4,6 +4,8 @@ module PrettyText
module Helpers
extend self
TAG_HASHTAG_POSTFIX = "::tag"
# functions here are available to v8
def t(key, opts)
key = "js." + key
@ -102,13 +104,12 @@ module PrettyText
end
def category_tag_hashtag_lookup(text)
tag_postfix = '::tag'
is_tag = text =~ /#{tag_postfix}$/
is_tag = text =~ /#{TAG_HASHTAG_POSTFIX}$/
if !is_tag && category = Category.query_from_hashtag_slug(text)
[category.url, text]
elsif (!is_tag && tag = Tag.find_by(name: text)) ||
(is_tag && tag = Tag.find_by(name: text.gsub!("#{tag_postfix}", '')))
(is_tag && tag = Tag.find_by(name: text.gsub!(TAG_HASHTAG_POSTFIX, '')))
["#{Discourse.base_url}/tag/#{tag.name}", text]
else
nil

53
spec/requests/category_hashtags_controller_spec.rb → spec/requests/hashtags_controller_spec.rb
View File

@ -2,11 +2,20 @@
require "rails_helper"
describe CategoryHashtagsController do
describe HashtagsController do
fab!(:category) { Fabricate(:category) }
fab!(:tag) { Fabricate(:tag) }
let(:group) { Fabricate(:group) }
let(:private_category) { Fabricate(:private_category, group: group) }
fab!(:group) { Fabricate(:group) }
fab!(:private_category) { Fabricate(:private_category, group: group) }
fab!(:hidden_tag) { Fabricate(:tag, name: "hidden") }
let(:tag_group) { Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [hidden_tag.name]) }
before do
SiteSetting.tagging_enabled = true
tag_group
end
describe "#check" do
context "when logged in" do
@ -15,20 +24,21 @@ describe CategoryHashtagsController do
sign_in(Fabricate(:user))
end
it "returns only valid categories" do
get "/category_hashtags/check.json", params: { category_slugs: [category.slug, private_category.slug, "none"] }
it "returns only valid categories and tags" do
get "/hashtags.json", params: { slugs: [category.slug, private_category.slug, "none", tag.name, hidden_tag.name] }
expect(response.status).to eq(200)
expect(response.parsed_body).to eq(
"valid" => [{ "slug" => category.slug, "url" => category.url }]
"categories" => { category.slug => category.url },
"tags" => { tag.name => tag.full_url }
)
end
it "does not return restricted categories" do
get "/category_hashtags/check.json", params: { category_slugs: [private_category.slug] }
it "does not return restricted categories or hidden tags" do
get "/hashtags.json", params: { slugs: [private_category.slug, hidden_tag.name] }
expect(response.status).to eq(200)
expect(response.parsed_body).to eq("valid" => [])
expect(response.parsed_body).to eq("categories" => {}, "tags" => {})
end
end
@ -39,14 +49,15 @@ describe CategoryHashtagsController do
sign_in(admin)
end
it "returns restricted categories" do
it "returns restricted categories and hidden tagss" do
group.add(admin)
get "/category_hashtags/check.json", params: { category_slugs: [private_category.slug] }
get "/hashtags.json", params: { slugs: [private_category.slug, hidden_tag.name] }
expect(response.status).to eq(200)
expect(response.parsed_body).to eq(
"valid" => [{ "slug" => private_category.slug, "url" => private_category.url }]
"categories" => { private_category.slug => private_category.url },
"tags" => { hidden_tag.name => hidden_tag.full_url }
)
end
end
@ -66,8 +77,8 @@ describe CategoryHashtagsController do
quxbar = Fabricate(:category_with_definition, slug: "bar", parent_category_id: qux.id)
quxbarbaz = Fabricate(:category_with_definition, slug: "baz", parent_category_id: quxbar.id)
get "/category_hashtags/check.json", params: {
category_slugs: [
get "/hashtags.json", params: {
slugs: [
":", # should not work
"foo",
"bar", # should not work
@ -82,12 +93,12 @@ describe CategoryHashtagsController do
}
expect(response.status).to eq(200)
expect(response.parsed_body["valid"]).to contain_exactly(
{ "slug" => "foo", "url" => foo.url },
{ "slug" => "foo:bar", "url" => foobar.url },
{ "slug" => "bar:baz", "url" => foobarbaz.id < quxbarbaz.id ? foobarbaz.url : quxbarbaz.url },
{ "slug" => "qux", "url" => qux.url },
{ "slug" => "qux:bar", "url" => quxbar.url }
expect(response.parsed_body["categories"]).to eq(
"foo" => foo.url,
"foo:bar" => foobar.url,
"bar:baz" => foobarbaz.id < quxbarbaz.id ? foobarbaz.url : quxbarbaz.url,
"qux" => qux.url,
"qux:bar" => quxbar.url
)
end
end
@ -95,7 +106,7 @@ describe CategoryHashtagsController do
context "when not logged in" do
it "returns invalid access" do
get "/category_hashtags/check.json", params: { category_slugs: [] }
get "/hashtags.json", params: { slugs: [] }
expect(response.status).to eq(403)
end
end

13
spec/requests/tags_controller_spec.rb
View File

@ -328,19 +328,6 @@ describe TagsController do
end
end
describe '#check_hashtag' do
fab!(:tag) { Fabricate(:tag) }
it "should return the right response" do
get "/tags/check.json", params: { tag_values: [tag.name] }
expect(response.status).to eq(200)
response_tag = response.parsed_body["valid"].first
expect(response_tag["value"]).to eq(tag.name)
end
end
describe "#update" do
fab!(:tag) { Fabricate(:tag) }

18
test/javascripts/acceptance/category-hashtag-test.js
View File

@ -1,18 +0,0 @@
import { acceptance } from "helpers/qunit-helpers";
acceptance("Category hashtag", { loggedIn: true });
QUnit.test("category hashtag is cooked properly", async assert => {
await visit("/t/internationalization-localization/280");
await click("#topic-footer-buttons .btn.create");
await fillIn(".d-editor-input", "this is a category hashtag #bug");
// TODO: Test that the autocomplete shows
assert.equal(
find(".d-editor-preview:visible")
.html()
.trim(),
'<p>this is a category hashtag <a href="/c/bugs" class="hashtag" data-type="category">#<span>bug</span></a></p>'
);
});

40
test/javascripts/acceptance/hashtags-test.js Normal file
View File

@ -0,0 +1,40 @@
import { acceptance } from "helpers/qunit-helpers";
acceptance("Category and Tag Hashtags", {
loggedIn: true,
settings: { tagging_enabled: true },
pretend(server, helper) {
server.get("/hashtags", () => {
return helper.response({
categories: { bug: "/c/bugs" },
tags: {
monkey: "/tag/monkey",
bug: "/tag/bug"
}
});
});
}
});
QUnit.test("hashtags are cooked properly", async assert => {
await visit("/t/internationalization-localization/280");
await click("#topic-footer-buttons .btn.create");
await fillIn(
".d-editor-input",
`this is a category hashtag #bug
this is a tag hashtag #monkey
category vs tag: #bug vs #bug::tag`
);
assert.equal(
find(".d-editor-preview:visible")
.html()
.trim(),
`<p>this is a category hashtag <a href="/c/bugs" class="hashtag">#<span>bug</span></a></p>
<p>this is a tag hashtag <a href="/tag/monkey" class="hashtag">#<span>monkey</span></a></p>
<p>category vs tag: <a href="/c/bugs" class="hashtag">#<span>bug</span></a> vs <a href="/tag/bug" class="hashtag">#<span>bug</span></a></p>`
);
});

45
test/javascripts/acceptance/tag-hashtag-test.js
View File

@ -1,45 +0,0 @@
import { acceptance } from "helpers/qunit-helpers";
acceptance("Tag Hashtag", {
loggedIn: true,
settings: { tagging_enabled: true },
pretend(server, helper) {
server.get("/tags/check", () => {
return helper.response({
valid: [
{ value: "monkey", url: "/tag/monkey" },
{ value: "bug", url: "/tag/bug" }
]
});
});
}
});
QUnit.test("tag is cooked properly", async assert => {
await visit("/t/internationalization-localization/280");
await click("#topic-footer-buttons .btn.create");
await fillIn(".d-editor-input", "this is a tag hashtag #monkey");
assert.equal(
find(".d-editor-preview:visible")
.html()
.trim(),
'<p>this is a tag hashtag <a href="/tag/monkey" class="hashtag">#<span>monkey</span></a></p>'
);
});
QUnit.test(
"tags and categories with same name are cooked properly",
async assert => {
await visit("/t/internationalization-localization/280");
await click("#topic-footer-buttons .btn.create");
await fillIn(".d-editor-input", "#bug vs #bug::tag");
assert.equal(
find(".d-editor-preview:visible")
.html()
.trim(),
'<p><a href="/c/bugs" class="hashtag" data-type="category">#<span>bug</span></a> vs <a href="/tag/bug" class="hashtag" data-type="tag">#<span>bug</span></a></p>'
);
}
);

4
test/javascripts/helpers/create-pretender.js
View File

@ -300,10 +300,6 @@ export function applyDefaultHandlers(pretender) {
return response({ post_reply_histories: [{ id: 1234, cooked: "wat" }] });
});
pretender.get("/category_hashtags/check", () => {
return response({ valid: [{ slug: "bug", url: "/c/bugs" }] });
});
pretender.get("/categories_and_latest", () =>
response(fixturesByUrl["/categories_and_latest.json"])
);