FIX: Hashtag subcategory ref incorrect when not highest-ranked type ()

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.
This commit is contained in:
Martin Brennan 2023-04-20 09:03:55 +10:00 committed by GitHub
parent a3693fec58
commit 86204fa4f0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 65 additions and 6 deletions

View File

@ -273,7 +273,7 @@ class HashtagAutocompleteService
# Any items that are _not_ the top-ranked type (which could possibly not be # 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 # 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 # 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 # 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 # 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 # 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 # 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 end
# TODO (martin) Remove this once plugins are not relying on the old lookup # TODO (martin) Remove this once plugins are not relying on the old lookup
@ -425,13 +425,22 @@ class HashtagAutocompleteService
) )
end 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| limited_results.each do |hashtag_item|
next if hashtag_item.type == top_ranked_type next if hashtag_item.type == top_ranked_type
other_slugs = limited_results.reject { |r| r.type === hashtag_item.type }.map(&:slug) # We only need to change the ref to include the type if there is a
if other_slugs.include?(hashtag_item.slug) # higher-ranked hashtag slug that conflicts with this one.
hashtag_item.ref = "#{hashtag_item.slug}::#{hashtag_item.type}" 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
end end

View File

@ -140,6 +140,44 @@ RSpec.describe HashtagAutocompleteService do
) )
end 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): it "orders results by (with type ordering within each section):
1. exact match on slug (ignoring parent/child distinction for categories) 1. exact match on slug (ignoring parent/child distinction for categories)
2. slugs that start with the term 2. slugs that start with the term
@ -342,6 +380,18 @@ RSpec.describe HashtagAutocompleteService do
category1.update!(parent_category: nil) category1.update!(parent_category: nil)
end 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 it "does not return the category if the parent does not match the child" do
parent_category = Fabricate(:category, name: "Media", slug: "media") parent_category = Fabricate(:category, name: "Media", slug: "media")
category1.update!(parent_category: parent_category) category1.update!(parent_category: parent_category)