FIX: Category hashtags weren't always found for sub-sub-categories (#20156)
The algorithm failed to find the correct category by slug when there are multiple sub-sub-categories with the same child-category name and the first child doesn't have the correct grandchild.
So, searching for "child / grandchild" worked in the following case, it found (3):
- (1) parent 1
- (2) child
- (3) grandchild
- (4) parent 2
- (5) child
- (6) grandchild
But it failed to find the grandchild in the following case:
- (1) parent 1
- (2) child
- (4) parent 2
- (5) child
- (6) grandchild
And this also fixes a flaky spec by forcing categories to always order by by `parent_category_id` and `id`.
This makes it possible to partly revert 60990aab55
This commit is contained in:
parent
6c80e17b92
commit
e17c145e8d
|
@ -46,18 +46,20 @@ module CategoryHashtag
|
|||
# is no child then the "parent" part of the slug is just the
|
||||
# entire slug we look for.
|
||||
#
|
||||
# Otherwise if the child slug is present, we find the parent
|
||||
# by its slug then find the child by its slug and its parent's
|
||||
# ID to make sure they match.
|
||||
# Otherwise if the child slug is present, we find the child
|
||||
# by its slug then find the parent by its slug and the child's
|
||||
# parent ID to make sure they match.
|
||||
if child_slug.present?
|
||||
parent_category = categories.find { |cat| cat.slug.casecmp?(parent_slug) }
|
||||
if parent_category.present?
|
||||
categories.find do |cat|
|
||||
cat.slug.downcase == child_slug && cat.parent_category_id == parent_category.id
|
||||
categories.find do |cat|
|
||||
if cat.slug.casecmp?(child_slug) && cat.parent_category_id
|
||||
categories.find do |parent_category|
|
||||
parent_category.id == cat.parent_category_id &&
|
||||
parent_category.slug.casecmp?(parent_slug)
|
||||
end
|
||||
end
|
||||
end
|
||||
else
|
||||
categories.find { |cat| cat.slug.downcase == parent_slug && cat.top_level? }
|
||||
categories.find { |cat| cat.slug.casecmp?(parent_slug) && cat.top_level? }
|
||||
end
|
||||
end
|
||||
.compact
|
||||
|
|
|
@ -32,7 +32,11 @@ class CategoryHashtagDataSource
|
|||
end
|
||||
|
||||
def self.lookup(guardian, slugs)
|
||||
user_categories = Category.secured(guardian).includes(:parent_category)
|
||||
user_categories =
|
||||
Category
|
||||
.secured(guardian)
|
||||
.includes(:parent_category)
|
||||
.order("parent_category_id ASC NULLS FIRST, id ASC")
|
||||
Category
|
||||
.query_loaded_from_slugs(slugs, user_categories)
|
||||
.map { |category| category_to_hashtag_item(category) }
|
||||
|
|
|
@ -275,6 +275,8 @@ RSpec.describe HashtagsController do
|
|||
|
||||
qux = Fabricate(:category_with_definition, slug: "qux")
|
||||
quxbar = Fabricate(:category_with_definition, slug: "bar", parent_category_id: qux.id)
|
||||
quxbarbaz =
|
||||
Fabricate(:category_with_definition, slug: "baz", parent_category_id: quxbar.id)
|
||||
|
||||
invalid_slugs = [":"]
|
||||
child_slugs = %w[bar baz]
|
||||
|
|
|
@ -40,6 +40,47 @@ RSpec.describe CategoryHashtagDataSource do
|
|||
group.add(user)
|
||||
expect(described_class.lookup(Guardian.new(user), ["secret"]).first).not_to eq(nil)
|
||||
end
|
||||
|
||||
context "with sub-sub-categories" do
|
||||
before { SiteSetting.max_category_nesting = 3 }
|
||||
|
||||
it "returns the first matching grandchild category (ordered by IDs) when there are multiple categories with the same slug" do
|
||||
parent1 = Fabricate(:category, slug: "parent1")
|
||||
parent2 = Fabricate(:category, slug: "parent2")
|
||||
|
||||
parent1_child = Fabricate(:category, slug: "child", parent_category_id: parent1.id)
|
||||
parent1_child_grandchild =
|
||||
Fabricate(:category, slug: "grandchild", parent_category_id: parent1_child.id)
|
||||
|
||||
parent2_child = Fabricate(:category, slug: "child", parent_category_id: parent2.id)
|
||||
parent2_child_grandchild =
|
||||
Fabricate(:category, slug: "grandchild", parent_category_id: parent2_child.id)
|
||||
|
||||
result = described_class.lookup(guardian, ["child:grandchild"])
|
||||
expect(result.map(&:relative_url)).to eq([parent1_child_grandchild.url])
|
||||
|
||||
parent1_child.destroy
|
||||
parent1_child = Fabricate(:category, slug: "child", parent_category_id: parent1.id)
|
||||
|
||||
result = described_class.lookup(guardian, ["child:grandchild"])
|
||||
expect(result.map(&:relative_url)).to eq([parent2_child_grandchild.url])
|
||||
end
|
||||
|
||||
it "returns the correct grandchild category when there are multiple children with the same slug and only one of them has the correct grandchild" do
|
||||
parent1 = Fabricate(:category, slug: "parent1")
|
||||
parent1_child = Fabricate(:category, slug: "child", parent_category_id: parent1.id)
|
||||
parent1_child_grandchild =
|
||||
Fabricate(:category, slug: "another-grandchild", parent_category_id: parent1_child.id)
|
||||
|
||||
parent2 = Fabricate(:category, slug: "parent2")
|
||||
parent2_child = Fabricate(:category, slug: "child", parent_category_id: parent2.id)
|
||||
parent2_child_grandchild =
|
||||
Fabricate(:category, slug: "grandchild", parent_category_id: parent2_child.id)
|
||||
|
||||
result = described_class.lookup(guardian, ["child:grandchild"])
|
||||
expect(result.map(&:relative_url)).to eq([parent2_child_grandchild.url])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe "#search" do
|
||||
|
|
Loading…
Reference in New Issue