diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js b/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js
index b920b72b1ab..65e2fca64bb 100644
--- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js
+++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/hashtag-autocomplete.js
@@ -14,11 +14,7 @@ function addHashtag(buffer, matches, state) {
// slug lookup.
const result =
hashtagLookup &&
- hashtagLookup(
- slug,
- options.currentUser,
- options.hashtagTypesInPriorityOrder
- );
+ hashtagLookup(slug, options.userId, options.hashtagTypesInPriorityOrder);
// NOTE: When changing the HTML structure here, you must also change
// it in the placeholder HTML code inside lib/hashtag-autocomplete, and vice-versa.
diff --git a/app/models/category.rb b/app/models/category.rb
index 7e7bd947b02..477744f9879 100644
--- a/app/models/category.rb
+++ b/app/models/category.rb
@@ -202,6 +202,10 @@ class Category < ActiveRecord::Base
Category.clear_subcategory_ids
end
+ def top_level?
+ self.parent_category_id.nil?
+ end
+
def self.scoped_to_permissions(guardian, permission_types)
if guardian.try(:is_admin?)
all
diff --git a/app/models/concerns/category_hashtag.rb b/app/models/concerns/category_hashtag.rb
index 959216baf30..db28efe789f 100644
--- a/app/models/concerns/category_hashtag.rb
+++ b/app/models/concerns/category_hashtag.rb
@@ -12,9 +12,7 @@ module CategoryHashtag
slug_path = category_slug.split(SEPARATOR)
return nil if slug_path.empty? || slug_path.size > 2
- if SiteSetting.slug_generation_method == "encoded"
- slug_path.map! { |slug| CGI.escape(slug) }
- end
+ slug_path.map! { |slug| CGI.escape(slug) } if SiteSetting.slug_generation_method == "encoded"
parent_slug, child_slug = slug_path.last(2)
categories = Category.where(slug: parent_slug)
@@ -31,9 +29,8 @@ module CategoryHashtag
# depth supported).
#
# @param {Array} category_slugs - Slug strings to look up, can also be in the parent:child format
- # @param {Array} cached_categories - An array of Hashes representing categories, Site.categories
- # should be used here since it is scoped to the Guardian.
- def query_from_cached_categories(category_slugs, cached_categories)
+ # @param {Array} categories - An array of Category models scoped to the user's guardian permissions.
+ def query_loaded_from_slugs(category_slugs, categories)
category_slugs
.map(&:downcase)
.map do |slug|
@@ -51,18 +48,19 @@ module CategoryHashtag
# by its slug then find the child by its slug and its parent's
# ID to make sure they match.
if child_slug.present?
- parent_category = cached_categories.find { |cat| cat[:slug].downcase == parent_slug }
+ parent_category = categories.find { |cat| cat.slug.casecmp?(parent_slug) }
if parent_category.present?
- cached_categories.find do |cat|
- cat[:slug].downcase == child_slug && cat[:parent_category_id] == parent_category[:id]
+ categories.find do |cat|
+ cat.slug.downcase == child_slug && cat.parent_category_id == parent_category.id
end
end
else
- cached_categories.find do |cat|
- cat[:slug].downcase == parent_slug && cat[:parent_category_id].nil?
+ categories.find do |cat|
+ cat.slug.downcase == parent_slug && cat.top_level?
end
end
- end.compact
+ end
+ .compact
end
end
end
diff --git a/app/services/category_hashtag_data_source.rb b/app/services/category_hashtag_data_source.rb
index 3edab5c017c..ab912bf1c4f 100644
--- a/app/services/category_hashtag_data_source.rb
+++ b/app/services/category_hashtag_data_source.rb
@@ -8,9 +8,7 @@ class CategoryHashtagDataSource
"folder"
end
- def self.category_to_hashtag_item(parent_category, category)
- category = Category.new(category.slice(:id, :slug, :name, :parent_category_id, :description))
-
+ def self.category_to_hashtag_item(category)
HashtagAutocompleteService::HashtagItem.new.tap do |item|
item.text = category.name
item.slug = category.slug
@@ -22,7 +20,7 @@ class CategoryHashtagDataSource
# categories here.
item.ref =
if category.parent_category_id
- !parent_category ? category.slug : "#{parent_category[:slug]}:#{category.slug}"
+ "#{category.parent_category.slug}:#{category.slug}"
else
category.slug
end
@@ -30,31 +28,19 @@ class CategoryHashtagDataSource
end
def self.lookup(guardian, slugs)
- # We use Site here because it caches all the categories the
- # user has access to.
- guardian_categories = Site.new(guardian).categories
+ user_categories = Category.secured(guardian).includes(:parent_category)
Category
- .query_from_cached_categories(slugs, guardian_categories)
- .map do |category|
- parent_category =
- guardian_categories.find { |cat| cat[:id] == category[:parent_category_id] }
- category_to_hashtag_item(parent_category, category)
- end
+ .query_loaded_from_slugs(slugs, user_categories)
+ .map { |category| category_to_hashtag_item(category) }
end
def self.search(guardian, term, limit)
- guardian_categories = Site.new(guardian).categories
-
- guardian_categories
- .select do |category|
- category[:name].downcase.include?(term) || category[:slug].downcase.include?(term)
- end
+ Category
+ .secured(guardian)
+ .includes(:parent_category)
+ .where("LOWER(name) LIKE :term OR LOWER(slug) LIKE :term", term: "%#{term}%")
.take(limit)
- .map do |category|
- parent_category =
- guardian_categories.find { |cat| cat[:id] == category[:parent_category_id] }
- category_to_hashtag_item(parent_category, category)
- end
+ .map { |category| category_to_hashtag_item(category) }
end
def self.search_sort(search_results, term)
@@ -79,6 +65,6 @@ class CategoryHashtagDataSource
)
.order(topic_count: :desc)
.take(limit)
- .map { |category| category_to_hashtag_item(category.parent_category, category) }
+ .map { |category| category_to_hashtag_item(category) }
end
end
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index d8cb0cf94dd..632dcabd915 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -225,6 +225,12 @@ module PrettyText
if opts[:user_id]
buffer << "__optInput.userId = #{opts[:user_id].to_i};\n"
+
+ # NOTE: If using this for server-side cooking you will end up
+ # with a Hash once it is passed to a PrettyText::Helper. If
+ # you use that hash to instanciate a User model, you will want to do
+ # user.reload before accessing data on this parsed User, otherwise
+ # AR relations will not be loaded.
buffer << "__optInput.currentUser = #{User.find(opts[:user_id]).to_json}\n"
end
diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb
index 97d2a795585..cb6a5faa16c 100644
--- a/lib/pretty_text/helpers.rb
+++ b/lib/pretty_text/helpers.rb
@@ -110,15 +110,15 @@ module PrettyText
end
end
- def hashtag_lookup(slug, cooking_user, types_in_priority_order)
+ def hashtag_lookup(slug, cooking_user_id, types_in_priority_order)
# This is _somewhat_ expected since we need to be able to cook posts
# etc. without a user sometimes, but it is still an edge case.
- if cooking_user.blank?
+ if cooking_user_id.blank?
cooking_user = Discourse.system_user
+ else
+ cooking_user = User.find(cooking_user_id)
end
- cooking_user = User.new(cooking_user) if cooking_user.is_a?(Hash)
-
result = HashtagAutocompleteService.new(
Guardian.new(cooking_user)
).lookup([slug], types_in_priority_order)
diff --git a/lib/pretty_text/shims.js b/lib/pretty_text/shims.js
index 70c6ea56494..1c54183c922 100644
--- a/lib/pretty_text/shims.js
+++ b/lib/pretty_text/shims.js
@@ -113,8 +113,8 @@ function __categoryLookup(c) {
return __helpers.category_tag_hashtag_lookup(c);
}
-function __hashtagLookup(slug, cookingUser, typesInPriorityOrder) {
- return __helpers.hashtag_lookup(slug, cookingUser, typesInPriorityOrder);
+function __hashtagLookup(slug, cookingUserId, typesInPriorityOrder) {
+ return __helpers.hashtag_lookup(slug, cookingUserId, typesInPriorityOrder);
}
function __lookupAvatar(p) {
diff --git a/spec/lib/pretty_text/helpers_spec.rb b/spec/lib/pretty_text/helpers_spec.rb
index 62ef9593327..4e95adc8449 100644
--- a/spec/lib/pretty_text/helpers_spec.rb
+++ b/spec/lib/pretty_text/helpers_spec.rb
@@ -55,7 +55,7 @@ RSpec.describe PrettyText::Helpers do
fab!(:user) { Fabricate(:user) }
it "handles tags and categories based on slug with type suffix" do
- expect(PrettyText::Helpers.hashtag_lookup("somecooltag::tag", user, %w[category tag])).to eq(
+ expect(PrettyText::Helpers.hashtag_lookup("somecooltag::tag", user.id, %w[category tag])).to eq(
{
relative_url: tag.url,
text: "somecooltag",
@@ -66,7 +66,7 @@ RSpec.describe PrettyText::Helpers do
type: "tag",
},
)
- expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory::category", user, %w[category tag])).to eq(
+ expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory::category", user.id, %w[category tag])).to eq(
{
relative_url: category.url,
text: "Some Awesome Category",
@@ -81,7 +81,7 @@ RSpec.describe PrettyText::Helpers do
it "handles categories based on slug" do
expect(
- PrettyText::Helpers.hashtag_lookup("someawesomecategory", user, %w[category tag]),
+ PrettyText::Helpers.hashtag_lookup("someawesomecategory", user.id, %w[category tag]),
).to eq(
{
relative_url: category.url,
@@ -96,7 +96,7 @@ RSpec.describe PrettyText::Helpers do
end
it "handles tags and categories based on slug without type suffix" do
- expect(PrettyText::Helpers.hashtag_lookup("somecooltag", user, %w[category tag])).to eq(
+ expect(PrettyText::Helpers.hashtag_lookup("somecooltag", user.id, %w[category tag])).to eq(
{
relative_url: tag.url,
text: "somecooltag",
@@ -107,7 +107,7 @@ RSpec.describe PrettyText::Helpers do
type: "tag",
},
)
- expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory", user, %w[category tag])).to eq(
+ expect(PrettyText::Helpers.hashtag_lookup("someawesomecategory", user.id, %w[category tag])).to eq(
{
relative_url: category.url,
text: "Some Awesome Category",
@@ -123,10 +123,10 @@ RSpec.describe PrettyText::Helpers do
it "does not include categories the cooking user does not have access to" do
group = Fabricate(:group)
private_category = Fabricate(:private_category, slug: "secretcategory", name: "Manager Hideout", group: group)
- expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user, %w[category tag])).to eq(nil)
+ expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user.id, %w[category tag])).to eq(nil)
GroupUser.create(group: group, user: user)
- expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user, %w[category tag])).to eq(
+ expect(PrettyText::Helpers.hashtag_lookup("secretcategory", user.id, %w[category tag])).to eq(
{
relative_url: private_category.url,
text: "Manager Hideout",
@@ -140,7 +140,7 @@ RSpec.describe PrettyText::Helpers do
end
it "returns nil when no tag or category that matches exists" do
- expect(PrettyText::Helpers.hashtag_lookup("blah", user, %w[category tag])).to eq(nil)
+ expect(PrettyText::Helpers.hashtag_lookup("blah", user.id, %w[category tag])).to eq(nil)
end
it "uses the system user if the cooking_user is nil" do
diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb
index 8168d4b8161..b953ce1353e 100644
--- a/spec/lib/pretty_text_spec.rb
+++ b/spec/lib/pretty_text_spec.rb
@@ -1456,16 +1456,24 @@ RSpec.describe PrettyText do
SiteSetting.enable_experimental_hashtag_autocomplete = true
user = Fabricate(:user)
- category = Fabricate(:category, name: 'testing')
- category2 = Fabricate(:category, name: 'known')
+ category = Fabricate(:category, name: 'testing', slug: 'testing')
+ category2 = Fabricate(:category, name: 'known', slug: 'known')
+ group = Fabricate(:group)
+ private_category = Fabricate(:private_category, name: 'secret', group: group, slug: 'secret')
Fabricate(:topic, tags: [Fabricate(:tag, name: 'known')])
- cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing", user_id: user.id)
+ cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing #secret", user_id: user.id)
expect(cooked).to include("#unknown::tag")
expect(cooked).to include("known")
expect(cooked).to include("known")
expect(cooked).to include("testing")
+ expect(cooked).to include("#secret")
+
+ # If the user hash access to the private category it should be cooked with the details + icon
+ group.add(user)
+ cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing #secret", user_id: user.id)
+ expect(cooked).to include("secret")
cooked = PrettyText.cook("[`a` #known::tag here](http://example.com)", user_id: user.id)
diff --git a/spec/services/category_hashtag_data_source_spec.rb b/spec/services/category_hashtag_data_source_spec.rb
index a0a411628d6..7e0a88c9f7a 100644
--- a/spec/services/category_hashtag_data_source_spec.rb
+++ b/spec/services/category_hashtag_data_source_spec.rb
@@ -1,8 +1,11 @@
# frozen_string_literal: true
RSpec.describe CategoryHashtagDataSource do
- fab!(:category1) { Fabricate(:category, slug: "random", topic_count: 12) }
- fab!(:category2) { Fabricate(:category, slug: "books", topic_count: 566) }
+ fab!(:parent_category) { Fabricate(:category, slug: "fun") }
+ fab!(:category1) do
+ Fabricate(:category, slug: "random", topic_count: 12, parent_category: parent_category)
+ end
+ fab!(:category2) { Fabricate(:category, name: "Book Section", slug: "books", topic_count: 566) }
fab!(:category3) { Fabricate(:category, slug: "movies", topic_count: 245) }
fab!(:group) { Fabricate(:group) }
fab!(:category4) { Fabricate(:private_category, slug: "secret", group: group, topic_count: 40) }
@@ -11,6 +14,60 @@ RSpec.describe CategoryHashtagDataSource do
let(:guardian) { Guardian.new(user) }
let(:uncategorized_category) { Category.find(SiteSetting.uncategorized_category_id) }
+ describe "#lookup" do
+ it "finds categories using their slug, downcasing for matches" do
+ result = described_class.lookup(guardian, ["movies"]).first
+ expect(result.ref).to eq("movies")
+ expect(result.slug).to eq("movies")
+
+ result = described_class.lookup(guardian, ["BoOKs"]).first
+ expect(result.ref).to eq("books")
+ expect(result.slug).to eq("books")
+ end
+
+ it "finds categories using the parent:child slug format" do
+ result = described_class.lookup(guardian, ["fun:random"]).first
+ expect(result.ref).to eq("fun:random")
+ expect(result.slug).to eq("random")
+ end
+
+ it "does not find child categories by their standalone slug" do
+ expect(described_class.lookup(guardian, ["random"]).first).to eq(nil)
+ end
+
+ it "does not find categories the user cannot access" do
+ expect(described_class.lookup(guardian, ["secret"]).first).to eq(nil)
+ group.add(user)
+ expect(described_class.lookup(Guardian.new(user), ["secret"]).first).not_to eq(nil)
+ end
+ end
+
+ describe "#search" do
+ it "finds categories by partial name" do
+ result = described_class.search(guardian, "mov", 5).first
+ expect(result.ref).to eq("movies")
+ expect(result.slug).to eq("movies")
+ end
+
+ it "finds categories by partial slug" do
+ result = described_class.search(guardian, "ook sec", 5).first
+ expect(result.ref).to eq("books")
+ expect(result.slug).to eq("books")
+ end
+
+ it "does not find categories the user cannot access" do
+ expect(described_class.search(guardian, "secret", 5).first).to eq(nil)
+ group.add(user)
+ expect(described_class.search(Guardian.new(user), "secret", 5).first).not_to eq(nil)
+ end
+
+ it "uses the correct ref format for a parent:child category that is found" do
+ result = described_class.search(guardian, "random", 5).first
+ expect(result.ref).to eq("fun:random")
+ expect(result.slug).to eq("random")
+ end
+ end
+
describe "#search_without_term" do
it "returns distinct categories ordered by topic_count" do
expect(described_class.search_without_term(guardian, 5).map(&:slug)).to eq(
diff --git a/spec/services/hashtag_autocomplete_service_spec.rb b/spec/services/hashtag_autocomplete_service_spec.rb
index 797ac062ba7..45a625e0c26 100644
--- a/spec/services/hashtag_autocomplete_service_spec.rb
+++ b/spec/services/hashtag_autocomplete_service_spec.rb
@@ -98,9 +98,8 @@ RSpec.describe HashtagAutocompleteService do
it "does not search other data sources if the limit is reached by earlier type data sources" do
# only expected once to try get the exact matches first
- site_guardian_categories = Site.new(guardian).categories
- Site.any_instance.expects(:categories).once.returns(site_guardian_categories)
- subject.search("book", %w[tag category], limit: 1)
+ DiscourseTagging.expects(:filter_allowed_tags).never
+ subject.search("book", %w[category tag], limit: 1)
end
it "includes the tag count" do