From dbe923d26e996bafba85f9b53ec57b23cb311b82 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Thu, 25 Apr 2024 16:47:45 +0300 Subject: [PATCH] FIX: Fetch categories for "+subcategories" option (#26622) Selecting the +subcategories option does not work sometimes when "lazy load categories" is enabled because the subcategories may not be fetched. This ensures that subcategories are loaded by requesting them before being used. --- .../discourse/app/models/category.js | 1 + .../discourse/tests/fixtures/site-fixtures.js | 1 + .../addon/components/category-row.gjs | 4 +- .../addon/components/category-selector.js | 59 ++++++++++--------- app/controllers/categories_controller.rb | 13 +++- app/models/category.rb | 17 +++--- app/serializers/basic_category_serializer.rb | 1 + .../json/category_create_response.json | 7 +++ .../schemas/json/category_list_response.json | 7 +++ .../json/category_update_response.json | 7 +++ .../api/schemas/json/site_response.json | 7 +++ spec/requests/categories_controller_spec.rb | 5 ++ 12 files changed, 89 insertions(+), 40 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index 1abe3ec1a50..0efffb11d67 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -399,6 +399,7 @@ export default class Category extends RestModel { include_ancestors: opts.includeAncestors, prioritized_category_id: opts.prioritizedCategoryId, limit: opts.limit, + page: opts.page, }; const result = (CATEGORY_ASYNC_SEARCH_CACHE[JSON.stringify(data)] ||= diff --git a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js index 37213c52ba6..d11a3655f3e 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js @@ -521,6 +521,7 @@ export default { notification_level: null, background_url: null, has_children: true, + subcategory_count: 2, }, { id: 1002, diff --git a/app/assets/javascripts/select-kit/addon/components/category-row.gjs b/app/assets/javascripts/select-kit/addon/components/category-row.gjs index 45e047c4d19..1b183f0f30e 100644 --- a/app/assets/javascripts/select-kit/addon/components/category-row.gjs +++ b/app/assets/javascripts/select-kit/addon/components/category-row.gjs @@ -111,8 +111,8 @@ export default class CategoryRow extends Component { this.allowUncategorizedTopics || this.allowUncategorized, hideParent: !!this.parentCategory, topicCount: this.topicCount, - subcategoryCount: this.args.item?.categories - ? this.args.item.categories.length - 1 + subcategoryCount: this.args.item?.category + ? this.category.subcategory_count : 0, }) ); diff --git a/app/assets/javascripts/select-kit/addon/components/category-selector.js b/app/assets/javascripts/select-kit/addon/components/category-selector.js index 896cbed0068..eb096a7cd79 100644 --- a/app/assets/javascripts/select-kit/addon/components/category-selector.js +++ b/app/assets/javascripts/select-kit/addon/components/category-selector.js @@ -72,40 +72,45 @@ export default MultiSelectComponent.extend({ // If there is a single match or an exact match and it has subcategories, // add a row for selecting all subcategories if ( - categories.length === 1 || - (categories.length > 0 && categories[0].name.localeCompare(filter) === 0) + (categories.length === 1 || + (categories.length > 0 && + categories[0].name.localeCompare(filter) === 0)) && + categories[0].subcategory_count > 0 ) { - // Descendants may not be loaded if lazy loading is enabled. Search for - // subcategories will make sure these are loaded - if (this.site.lazy_load_categories) { - await Category.asyncSearch("", { - parentCategoryId: categories[0].id, - }); - } - - if (categories[0].descendants.length > 1) { - categories.splice( - 1, - 0, - EmberObject.create({ - // This is just a hack to ensure the IDs are unique, but ensure - // that parseInt still returns a valid ID in order to generate the - // label - id: `${categories[0].id}+subcategories`, - categories: categories[0].descendants, - }) - ); - } + categories.splice( + 1, + 0, + EmberObject.create({ + // This is just a hack to ensure the IDs are unique, but ensure + // that parseInt still returns a valid ID in order to generate the + // label + id: `${categories[0].id}+subcategories`, + category: categories[0], + }) + ); } return categories; }, - select(value, item) { - if (item.categories) { + async select(value, item) { + // item is usually a category, but if the "category" property is set, then + // it is the special row for selecting all subcategories + if (item.category) { + if (this.site.lazy_load_categories) { + // Descendants may not be loaded if lazy loading is enabled. Searching + // for subcategories will make sure these are loaded + for (let page = 1, categories = [null]; categories.length > 0; page++) { + categories = await Category.asyncSearch("", { + parentCategoryId: item.category.id, + page, + }); + } + } + this.selectKit.change( - makeArray(this.value).concat(item.categories.mapBy("id")), - makeArray(this.selectedContent).concat(item.categories) + makeArray(this.value).concat(item.category.descendants.mapBy("id")), + makeArray(this.selectedContent).concat(item.category.descendants) ); } else { this._super(value, item); diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index e8c7098099c..075c6b60e3e 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -362,7 +362,15 @@ class CategoriesController < ApplicationController prioritized_category_id = params[:prioritized_category_id].to_i if params[ :prioritized_category_id ].present? - limit = params[:limit].to_i.clamp(1, MAX_CATEGORIES_LIMIT) if params[:limit].present? + limit = + ( + if params[:limit].present? + params[:limit].to_i.clamp(1, MAX_CATEGORIES_LIMIT) + else + MAX_CATEGORIES_LIMIT + end + ) + page = [1, params[:page].to_i].max categories = Category.secured(guardian) @@ -404,7 +412,8 @@ class CategoriesController < ApplicationController ) .joins("LEFT JOIN topics t on t.id = categories.topic_id") .select("categories.*, t.slug topic_slug") - .limit(limit || MAX_CATEGORIES_LIMIT) + .limit(limit) + .offset((page - 1) * limit) if Site.preloaded_category_custom_fields.present? Category.preload_custom_fields(categories, Site.preloaded_category_custom_fields) diff --git a/app/models/category.rb b/app/models/category.rb index 442da2653b2..879a5a4def2 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -226,7 +226,8 @@ class Category < ActiveRecord::Base :subcategory_ids, :subcategory_list, :notification_level, - :has_children + :has_children, + :subcategory_count # Allows us to skip creating the category definition topic in tests. attr_accessor :skip_category_definition @@ -244,13 +245,9 @@ class Category < ActiveRecord::Base Category.topic_create_allowed(guardian).where(id: category_ids).pluck(:id).to_set end - # Categories with children - with_children = - Category - .secured(guardian) - .where(parent_category_id: category_ids) - .pluck(:parent_category_id) - .to_set + # Load subcategory counts (used to fill has_children property) + subcategory_count = + Category.secured(guardian).where.not(parent_category_id: nil).group(:parent_category_id).count # Update category attributes categories.each do |category| @@ -259,7 +256,9 @@ class Category < ActiveRecord::Base category.permission = CategoryGroup.permission_types[:full] if guardian.is_admin? || allowed_topic_create_ids&.include?(category[:id]) - category.has_children = with_children.include?(category[:id]) + category.has_children = subcategory_count.key?(category[:id]) + + category.subcategory_count = subcategory_count[category[:id]] if category.has_children end end diff --git a/app/serializers/basic_category_serializer.rb b/app/serializers/basic_category_serializer.rb index 2ec27126888..ff76518de29 100644 --- a/app/serializers/basic_category_serializer.rb +++ b/app/serializers/basic_category_serializer.rb @@ -20,6 +20,7 @@ class BasicCategorySerializer < ApplicationSerializer :can_edit, :topic_template, :has_children, + :subcategory_count, :sort_order, :sort_ascending, :show_subcategory_list, diff --git a/spec/requests/api/schemas/json/category_create_response.json b/spec/requests/api/schemas/json/category_create_response.json index e48af843f66..114b563df73 100644 --- a/spec/requests/api/schemas/json/category_create_response.json +++ b/spec/requests/api/schemas/json/category_create_response.json @@ -84,6 +84,12 @@ "null" ] }, + "subcategory_count": { + "type": [ + "integer", + "null" + ] + }, "sort_order": { "type": [ "string", @@ -287,6 +293,7 @@ "can_edit", "topic_template", "has_children", + "subcategory_count", "sort_order", "sort_ascending", "show_subcategory_list", diff --git a/spec/requests/api/schemas/json/category_list_response.json b/spec/requests/api/schemas/json/category_list_response.json index 0896cc5d10b..656bdb84325 100644 --- a/spec/requests/api/schemas/json/category_list_response.json +++ b/spec/requests/api/schemas/json/category_list_response.json @@ -78,6 +78,12 @@ "has_children": { "type": "boolean" }, + "subcategory_count": { + "type": [ + "integer", + "null" + ] + }, "sort_order": { "type": [ "string", @@ -194,6 +200,7 @@ "can_edit", "topic_template", "has_children", + "subcategory_count", "sort_order", "sort_ascending", "show_subcategory_list", diff --git a/spec/requests/api/schemas/json/category_update_response.json b/spec/requests/api/schemas/json/category_update_response.json index 08a6611e02d..531a20673d6 100644 --- a/spec/requests/api/schemas/json/category_update_response.json +++ b/spec/requests/api/schemas/json/category_update_response.json @@ -87,6 +87,12 @@ "null" ] }, + "subcategory_count": { + "type": [ + "integer", + "null" + ] + }, "sort_order": { "type": [ "string", @@ -291,6 +297,7 @@ "topic_template", "form_template_ids", "has_children", + "subcategory_count", "sort_order", "sort_ascending", "show_subcategory_list", diff --git a/spec/requests/api/schemas/json/site_response.json b/spec/requests/api/schemas/json/site_response.json index a88c2b4c763..487955e158d 100644 --- a/spec/requests/api/schemas/json/site_response.json +++ b/spec/requests/api/schemas/json/site_response.json @@ -603,6 +603,12 @@ "has_children": { "type": "boolean" }, + "subcategory_count": { + "type": [ + "integer", + "null" + ] + }, "sort_order": { "type": [ "string", @@ -740,6 +746,7 @@ "notification_level", "topic_template", "has_children", + "subcategory_count", "sort_order", "sort_ascending", "show_subcategory_list", diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 3ec01bf0381..735330957cc 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -1115,6 +1115,7 @@ RSpec.describe CategoriesController do expect(serialized["notification_level"]).to eq(CategoryUser.default_notification_level) expect(serialized["permission"]).to eq(nil) expect(serialized["has_children"]).to eq(false) + expect(serialized["subcategory_count"]).to eq(nil) end it "does not return hidden category" do @@ -1168,6 +1169,7 @@ RSpec.describe CategoriesController do expect(category["notification_level"]).to eq(NotificationLevels.all[:regular]) expect(category["permission"]).to eq(CategoryGroup.permission_types[:full]) expect(category["has_children"]).to eq(true) + expect(category["subcategory_count"]).to eq(1) end context "with a read restricted child category" do @@ -1179,6 +1181,7 @@ RSpec.describe CategoriesController do get "/categories/find.json", params: { ids: [category.id] } category = response.parsed_body["categories"].first expect(category["has_children"]).to eq(true) + expect(category["subcategory_count"]).to eq(1) end it "indicates to a normal user that the category has no child" do @@ -1187,6 +1190,7 @@ RSpec.describe CategoriesController do get "/categories/find.json", params: { ids: [category.id] } category = response.parsed_body["categories"].first expect(category["has_children"]).to eq(false) + expect(category["subcategory_count"]).to eq(nil) end end end @@ -1415,6 +1419,7 @@ RSpec.describe CategoriesController do expect(category["notification_level"]).to eq(NotificationLevels.all[:regular]) expect(category["permission"]).to eq(CategoryGroup.permission_types[:full]) expect(category["has_children"]).to eq(true) + expect(category["subcategory_count"]).to eq(1) end end end