From 86204fa4f001cc2cfe09405a48ce5be787845a4b Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 Apr 2023 09:03:55 +1000 Subject: [PATCH] FIX: Hashtag subcategory ref incorrect when not highest-ranked type (#21163) This commit fixes the following scenario: 1. The user is searching for hashtags in chat, where the subcategory type is not highest-ranked in priority order. 2. There can, but doesn't have to be, a higher-ranked matching chat channel that has the same slug as the subcategory. 3. Since it is not the highest-ranked type, the subcategory, which normally has a ref of parent:child, has its ref changed to child::category, which does not work This was happening because whenever a hashtag type was not highest ranked, if _any_ other hashtag results conflicted slugs, we would append the ::type suffix. Now, we only append this suffix if a higher-ranked type conflicts with the hashtag, and we use the current ref to build the new typed ref to preserve this parent:child format as well, it's more accurate. --- app/services/hashtag_autocomplete_service.rb | 21 +++++--- .../hashtag_autocomplete_service_spec.rb | 50 +++++++++++++++++++ 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/app/services/hashtag_autocomplete_service.rb b/app/services/hashtag_autocomplete_service.rb index b13940382b5..6e92b2ee272 100644 --- a/app/services/hashtag_autocomplete_service.rb +++ b/app/services/hashtag_autocomplete_service.rb @@ -273,7 +273,7 @@ class HashtagAutocompleteService # Any items that are _not_ the top-ranked type (which could possibly not be # the same as the first item in the types_in_priority_order if there was # no data for that type) that have conflicting slugs with other items for - # other types need to have a ::type suffix added to their ref. + # other higher-ranked types need to have a ::type suffix added to their ref. # # This will be used for the lookup method above if one of these items is # chosen in the UI, otherwise there is no way to determine whether a hashtag is @@ -281,7 +281,7 @@ class HashtagAutocompleteService # # For example, if there is a category with the slug #general and a tag # with the slug #general, then the tag will have its ref changed to #general::tag - append_types_to_conflicts(limited_results, top_ranked_type, limit) + append_types_to_conflicts(limited_results, top_ranked_type, types_in_priority_order, limit) end # TODO (martin) Remove this once plugins are not relying on the old lookup @@ -425,13 +425,22 @@ class HashtagAutocompleteService ) end - def append_types_to_conflicts(limited_results, top_ranked_type, limit) + def append_types_to_conflicts(limited_results, top_ranked_type, types_in_priority_order, limit) limited_results.each do |hashtag_item| next if hashtag_item.type == top_ranked_type - other_slugs = limited_results.reject { |r| r.type === hashtag_item.type }.map(&:slug) - if other_slugs.include?(hashtag_item.slug) - hashtag_item.ref = "#{hashtag_item.slug}::#{hashtag_item.type}" + # We only need to change the ref to include the type if there is a + # higher-ranked hashtag slug that conflicts with this one. + higher_ranked_types = + types_in_priority_order.slice(0, types_in_priority_order.index(hashtag_item.type)) + higher_ranked_slugs = + limited_results + .reject { |r| r.type === hashtag_item.type } + .select { |r| higher_ranked_types.include?(r.type) } + .map(&:slug) + + if higher_ranked_slugs.include?(hashtag_item.slug) + hashtag_item.ref = "#{hashtag_item.ref}::#{hashtag_item.type}" end end diff --git a/spec/services/hashtag_autocomplete_service_spec.rb b/spec/services/hashtag_autocomplete_service_spec.rb index eeb4dd74b8c..9c2382816ba 100644 --- a/spec/services/hashtag_autocomplete_service_spec.rb +++ b/spec/services/hashtag_autocomplete_service_spec.rb @@ -140,6 +140,44 @@ RSpec.describe HashtagAutocompleteService do ) end + it "does not add a type suffix where + 1. a subcategory name conflicts with an existing tag name and + 2. the category is not the top ranked type" do + parent = Fabricate(:category, name: "Hobbies", slug: "hobbies") + category1.update!(parent_category: parent) + Fabricate(:tag, name: "the-book-club") + + Fabricate(:bookmark, user: user, name: "book club") + guardian.user.reload + + DiscoursePluginRegistry.register_hashtag_autocomplete_data_source( + FakeBookmarkHashtagDataSource, + stub(enabled?: true), + ) + + expect(subject.search("book", %w[bookmark category tag]).map(&:ref)).to eq( + %w[book-club hobbies:the-book-club great-books the-book-club::tag], + ) + end + + it "handles the type suffix where the top ranked type conflicts with a subcategory" do + parent = Fabricate(:category, name: "Hobbies", slug: "hobbies") + category1.update!(parent_category: parent) + Fabricate(:tag, name: "the-book-club") + + Fabricate(:bookmark, user: user, name: "the book club") + guardian.user.reload + + DiscoursePluginRegistry.register_hashtag_autocomplete_data_source( + FakeBookmarkHashtagDataSource, + stub(enabled?: true), + ) + + expect(subject.search("book", %w[bookmark category tag]).map(&:ref)).to eq( + %w[the-book-club hobbies:the-book-club::category great-books the-book-club::tag], + ) + end + it "orders results by (with type ordering within each section): 1. exact match on slug (ignoring parent/child distinction for categories) 2. slugs that start with the term @@ -342,6 +380,18 @@ RSpec.describe HashtagAutocompleteService do category1.update!(parent_category: nil) end + it "handles parent:child category lookups with type suffix" do + parent_category = Fabricate(:category, name: "Media", slug: "media") + category1.update!(parent_category: parent_category) + result = subject.lookup(%w[media:the-book-club::category], %w[category tag]) + expect(result[:category].map(&:slug)).to eq(["the-book-club"]) + expect(result[:category].map(&:ref)).to eq(["media:the-book-club::category"]) + expect(result[:category].map(&:relative_url)).to eq( + ["/c/media/the-book-club/#{category1.id}"], + ) + category1.update!(parent_category: nil) + end + it "does not return the category if the parent does not match the child" do parent_category = Fabricate(:category, name: "Media", slug: "media") category1.update!(parent_category: parent_category)