FIX: Duplicate hashtag lookup results based on permissions (#19337)

When looking up hashtags which were conflicting (e.g.
management::tag and management) where the user did
not have permission for one of them, we ended up returning
the one they did have permission to (e.g. the tag) twice
because of the way the lookup fallback code worked. This
fixes the issue, and another related one where the
::type was not added to the found item's .ref, and
so the hashtag replacement on the client was not working
correctly.

app/assets/javascripts/discourse/app/lib/hashtag-autocomplete.js

@@ -221,19 +221,13 @@ function _searchRequest(term, contextualHashtagConfiguration, resultFunc) {
 }
 
 function _findAndReplaceSeenHashtagPlaceholder(
-  slug,
+  slugRef,
   contextualHashtagConfiguration,
   hashtagSpan
 ) {
   contextualHashtagConfiguration.forEach((type) => {
-    // remove type suffixes
-    const typePostfix = `::${type}`;
-    if (slug.endsWith(typePostfix)) {
-      slug = slug.slice(0, slug.length - typePostfix.length);
-    }
-
     // replace raw span for the hashtag with a cooked one
-    const matchingSeenHashtag = seenHashtags[type]?.[slug];
+    const matchingSeenHashtag = seenHashtags[type]?.[slugRef];
     if (matchingSeenHashtag) {
       // NOTE: When changing the HTML structure here, you must also change
       // it in the hashtag-autocomplete markdown rule, and vice-versa.

app/services/hashtag_autocomplete_service.rb

@@ -122,7 +122,18 @@ class HashtagAutocompleteService
     # composer we want a slug without a suffix to be a category first, tag second.
     if slugs_without_suffixes.any?
       types_in_priority_order.each do |type|
-        found_from_slugs = execute_lookup!(lookup_results, type, guardian, slugs_without_suffixes)
+        # We do not want to continue fallback if there are conflicting slugs where
+        # one has a type and one does not, this may result in duplication. An
+        # example:
+        #
+        # A category with slug `management` is not found because of permissions
+        # and we also have a slug with suffix in the form of `management::tag`.
+        # There is a tag that exists with the `management` slug. The tag should
+        # not be found here but rather in the next lookup since it's got a more
+        # specific lookup with the type.
+        slugs_to_lookup =
+          slugs_without_suffixes.reject { |slug| slugs_with_suffixes.include?("#{slug}::#{type}") }
+        found_from_slugs = execute_lookup!(lookup_results, type, guardian, slugs_to_lookup)
 
         slugs_without_suffixes = slugs_without_suffixes - found_from_slugs.map(&:ref)
         break if slugs_without_suffixes.empty?
@@ -139,6 +150,11 @@ class HashtagAutocompleteService
             .map { |slug| slug.gsub("::#{type}", "") }
         next if slugs_for_type.empty?
         execute_lookup!(lookup_results, type, guardian, slugs_for_type)
+
+        # Make sure the refs are the same going out as they were going in.
+        lookup_results[type.to_sym].each do |item|
+          item.ref = "#{item.ref}::#{type}" if slugs_with_suffixes.include?("#{item.ref}::#{type}")
+        end
       end
     end
 

spec/lib/pretty_text/helpers_spec.rb

@@ -62,7 +62,7 @@ RSpec.describe PrettyText::Helpers do
           description: "Coolest things ever",
           icon: "tag",
           slug: "somecooltag",
-          ref: "somecooltag",
+          ref: "somecooltag::tag",
           type: "tag",
         },
       )
@@ -73,7 +73,7 @@ RSpec.describe PrettyText::Helpers do
           description: "Really great stuff here",
           icon: "folder",
           slug: "someawesomecategory",
-          ref: "someawesomecategory",
+          ref: "someawesomecategory::category",
           type: "category",
         },
       )

spec/lib/pretty_text_spec.rb

@@ -1462,14 +1462,10 @@ RSpec.describe PrettyText do
 
     cooked = PrettyText.cook(" #unknown::tag #known #known::tag #testing", user_id: user.id)
 
-    [
+    expect(cooked).to include("<span class=\"hashtag-raw\">#unknown::tag</span>")
-      "<span class=\"hashtag-raw\">#unknown::tag</span>",
+    expect(cooked).to include("<a class=\"hashtag-cooked\" href=\"#{category2.url}\" data-type=\"category\" data-slug=\"known\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>known</span></a>")
-      "<a class=\"hashtag-cooked\" href=\"#{category2.url}\" data-type=\"category\" data-slug=\"known\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>known</span></a>",
+    expect(cooked).to include("<a class=\"hashtag-cooked\" href=\"/tag/known\" data-type=\"tag\" data-slug=\"known\" data-ref=\"known::tag\"><svg class=\"fa d-icon d-icon-tag svg-icon svg-node\"><use href=\"#tag\"></use></svg><span>known</span></a>")
-      "<a class=\"hashtag-cooked\" href=\"/tag/known\" data-type=\"tag\" data-slug=\"known\"><svg class=\"fa d-icon d-icon-tag svg-icon svg-node\"><use href=\"#tag\"></use></svg><span>known</span></a>",
+    expect(cooked).to include("<a class=\"hashtag-cooked\" href=\"#{category.url}\" data-type=\"category\" data-slug=\"testing\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>testing</span></a>")
-      "<a class=\"hashtag-cooked\" href=\"#{category.url}\" data-type=\"category\" data-slug=\"testing\"><svg class=\"fa d-icon d-icon-folder svg-icon svg-node\"><use href=\"#folder\"></use></svg><span>testing</span></a>"
-    ].each do |element|
-      expect(cooked).to include(element)
-    end
 
     cooked = PrettyText.cook("[`a` #known::tag here](http://example.com)", user_id: user.id)
 
@@ -1485,7 +1481,7 @@ RSpec.describe PrettyText do
 
     cooked = PrettyText.cook("<A href='/a'>test</A> #known::tag", user_id: user.id)
     html = <<~HTML
-      <p><a href="/a">test</a> <a class="hashtag-cooked" href="/tag/known" data-type="tag" data-slug="known"><svg class="fa d-icon d-icon-tag svg-icon svg-node"><use href="#tag"></use></svg><span>known</span></a></p>
+      <p><a href="/a">test</a> <a class="hashtag-cooked" href="/tag/known" data-type="tag" data-slug="known" data-ref="known::tag"><svg class="fa d-icon d-icon-tag svg-icon svg-node"><use href="#tag"></use></svg><span>known</span></a></p>
     HTML
     expect(cooked).to eq(html.strip)
 

spec/services/hashtag_autocomplete_service_spec.rb

@@ -377,6 +377,14 @@ RSpec.describe HashtagAutocompleteService do
       expect(result[:bookmark].map(&:slug)).to eq(["coolrock"])
     end
 
+    it "handles type suffix lookups where there is another type with a conflicting slug that the user cannot access" do
+      category1.update!(read_restricted: true)
+      Fabricate(:tag, name: "book-club")
+      result = subject.lookup(%w[book-club::tag book-club], %w[category tag])
+      expect(result[:category].map(&:ref)).to eq([])
+      expect(result[:tag].map(&:ref)).to eq(["book-club::tag"])
+    end
+
     context "when not tagging_enabled" do
       before { SiteSetting.tagging_enabled = false }