From dcd81d56c075c66c975320ff083e66419572ffeb Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Fri, 8 Dec 2023 12:01:08 +0200 Subject: [PATCH] FIX: category selectors for lazy loaded categories (#24533) A lot of work has been put in the select kits used for selecting categories: CategorySelector, CategoryChooser, CategoryDrop, however they still do not work as expected when these selectors already have values set, because the category were still looked up in the list of categories stored on the client-side Categrories.list(). This PR fixes that by looking up the categories when the selector is initialized. This required altering the /categories/find.json endpoint to accept a list of IDs that need to be looked up. The API is called using Category.asyncFindByIds on the client-side. CategorySelector was also updated to receive a list of category IDs as attribute, instead of the list of categories, because the list of categories may have not been loaded. During this development, I noticed that SiteCategorySerializer did not serializer all fields (such as permission and notification_level) which are not a property of category, but a property of the relationship between users and categories. To make this more efficient, the preload_user_fields! method was implemented that can be used to preload these attributes for a user and a list of categories. --- .../user-preferences/categories.hbs | 20 ++--- .../controllers/group-manage-categories.js | 16 ++-- .../app/controllers/preferences/categories.js | 27 +++---- .../app/controllers/preferences/tracking.js | 30 ++++--- .../discourse/app/models/category.js | 34 +++++--- .../app/templates/group/manage/categories.hbs | 20 ++--- .../app/templates/preferences/categories.hbs | 2 +- .../app/templates/preferences/tracking.hbs | 2 +- .../addon/components/category-chooser.js | 13 +++ .../addon/components/category-selector.js | 67 +++++++++++----- app/controllers/categories_controller.rb | 33 ++++---- app/models/category.rb | 49 +++++++----- app/serializers/basic_category_serializer.rb | 2 +- spec/models/category_spec.rb | 2 - .../json/category_create_response.json | 5 +- .../json/category_update_response.json | 7 +- spec/requests/categories_controller_spec.rb | 79 +++++++++++++++---- 17 files changed, 265 insertions(+), 143 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/user-preferences/categories.hbs b/app/assets/javascripts/discourse/app/components/user-preferences/categories.hbs index 74160c9b62c..b5a2bb12dbe 100644 --- a/app/assets/javascripts/discourse/app/components/user-preferences/categories.hbs +++ b/app/assets/javascripts/discourse/app/components/user-preferences/categories.hbs @@ -9,8 +9,8 @@ }} {{/if}} @@ -26,8 +26,8 @@ }} {{/if}} @@ -41,8 +41,8 @@ @@ -56,8 +56,8 @@ > @@ -75,8 +75,8 @@ {{/if}} diff --git a/app/assets/javascripts/discourse/app/controllers/group-manage-categories.js b/app/assets/javascripts/discourse/app/controllers/group-manage-categories.js index 239a2eab233..9c056f91be1 100644 --- a/app/assets/javascripts/discourse/app/controllers/group-manage-categories.js +++ b/app/assets/javascripts/discourse/app/controllers/group-manage-categories.js @@ -3,15 +3,13 @@ import discourseComputed from "discourse-common/utils/decorators"; export default Controller.extend({ @discourseComputed( - "model.watchingCategories.[]", - "model.watchingFirstPostCategories.[]", - "model.trackingCategories.[]", - "model.regularCategories.[]", - "model.mutedCategories.[]" + "model.watching_category_ids.[]", + "model.watching_first_post_category_ids.[]", + "model.tracking_category_ids.[]", + "model.regular_category_ids.[]", + "model.muted_category_ids.[]" ) - selectedCategories(watching, watchingFirst, tracking, regular, muted) { - return [] - .concat(watching, watchingFirst, tracking, regular, muted) - .filter((t) => t); + selectedCategoryIds(watching, watchingFirst, tracking, regular, muted) { + return [].concat(watching, watchingFirst, tracking, regular, muted); }, }); diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/categories.js b/app/assets/javascripts/discourse/app/controllers/preferences/categories.js index d22c1509c51..3368c5f327b 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/categories.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/categories.js @@ -17,28 +17,27 @@ export default Controller.extend({ }, @discourseComputed( - "siteSettings.mute_all_categories_by_default", - "model.watchedCategories", - "model.watchedFirstPostCategories", - "model.trackedCategories", - "model.mutedCategories", - "model.regularCategories" + "model.watched_categoriy_ids", + "model.watched_first_post_categoriy_ids", + "model.tracked_categoriy_ids", + "model.muted_categoriy_ids", + "model.regular_category_ids", + "siteSettings.mute_all_categories_by_default" ) - selectedCategories( - muteAllCategoriesByDefault, + selectedCategoryIds( watched, watchedFirst, tracked, muted, - regular + regular, + muteAllCategoriesByDefault ) { - let categories = [].concat(watched, watchedFirst, tracked); - - categories = categories.concat( + return [].concat( + watched, + watchedFirst, + tracked, muteAllCategoriesByDefault ? regular : muted ); - - return categories.filter((t) => t); }, @discourseComputed diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js b/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js index 0277c02c616..7522182373e 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js +++ b/app/assets/javascripts/discourse/app/controllers/preferences/tracking.js @@ -122,24 +122,22 @@ export default class extends Controller { } @computed( - "model.watchedCategories", - "model.watchedFirstPostCategories", - "model.trackedCategories", - "model.mutedCategories", - "model.regularCategories", + "model.watched_category_ids", + "model.watched_first_post_category_ids", + "model.tracked_category_ids", + "model.muted_category_ids", + "model.regular_category_ids", "siteSettings.mute_all_categories_by_default" ) - get selectedCategories() { - return [] - .concat( - this.model.watchedCategories, - this.model.watchedFirstPostCategories, - this.model.trackedCategories, - this.siteSettings.mute_all_categories_by_default - ? this.model.regularCategories - : this.model.mutedCategories - ) - .filter((t) => t); + get selectedCategoryIds() { + return [].concat( + this.model.watched_category_ids, + this.model.watched_first_post_category_ids, + this.model.tracked_category_ids, + this.siteSettings.mute_all_categories_by_default + ? this.model.regular_category_ids + : this.model.muted_category_ids + ); } @computed("siteSettings.remove_muted_tags_from_latest") diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index a72f9ff4537..1308c539f42 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -462,6 +462,26 @@ Category.reopenClass({ return categories; }, + hasAsyncFoundAll(ids) { + const loadedCategoryIds = Site.current().loadedCategoryIds || new Set(); + return ids.every((id) => loadedCategoryIds.has(id)); + }, + + async asyncFindByIds(ids = []) { + const result = await ajax("/categories/find", { data: { ids } }); + + const categories = result["categories"].map((category) => + Site.current().updateCategory(category) + ); + + // Update loadedCategoryIds list + const loadedCategoryIds = Site.current().loadedCategoryIds || new Set(); + ids.forEach((id) => loadedCategoryIds.add(id)); + Site.current().set("loadedCategoryIds", loadedCategoryIds); + + return categories; + }, + findBySlugAndParent(slug, parentCategory) { if (this.slugEncoded()) { slug = encodeURI(slug); @@ -490,18 +510,14 @@ Category.reopenClass({ async asyncFindBySlugPathWithID(slugPathWithID) { const result = await ajax("/categories/find", { - data: { - category_slug_path_with_id: slugPathWithID, - }, + data: { slug_path_with_id: slugPathWithID }, }); - if (result["ancestors"]) { - result["ancestors"].map((category) => - Site.current().updateCategory(category) - ); - } + const categories = result["categories"].map((category) => + Site.current().updateCategory(category) + ); - return Site.current().updateCategory(result.category); + return categories[categories.length - 1]; }, findBySlugPathWithID(slugPathWithID) { diff --git a/app/assets/javascripts/discourse/app/templates/group/manage/categories.hbs b/app/assets/javascripts/discourse/app/templates/group/manage/categories.hbs index 351e7acfa82..7dd6411dcb7 100644 --- a/app/assets/javascripts/discourse/app/templates/group/manage/categories.hbs +++ b/app/assets/javascripts/discourse/app/templates/group/manage/categories.hbs @@ -11,8 +11,8 @@ {{i18n "groups.notifications.watching.title"}} @@ -26,8 +26,8 @@ {{i18n "groups.notifications.tracking.title"}} @@ -41,8 +41,8 @@ {{i18n "groups.notifications.watching_first_post.title"}} @@ -58,8 +58,8 @@ {{i18n "groups.notifications.regular.title"}} @@ -73,8 +73,8 @@ {{i18n "groups.notifications.muted.title"}} diff --git a/app/assets/javascripts/discourse/app/templates/preferences/categories.hbs b/app/assets/javascripts/discourse/app/templates/preferences/categories.hbs index c9a5517f73f..15e7e0a8732 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences/categories.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences/categories.hbs @@ -1,7 +1,7 @@ diff --git a/app/assets/javascripts/select-kit/addon/components/category-chooser.js b/app/assets/javascripts/select-kit/addon/components/category-chooser.js index c2fd30649d1..d6a5e56b0e7 100644 --- a/app/assets/javascripts/select-kit/addon/components/category-chooser.js +++ b/app/assets/javascripts/select-kit/addon/components/category-chooser.js @@ -24,6 +24,19 @@ export default ComboBoxComponent.extend({ prioritizedCategoryId: null, }, + init() { + this._super(...arguments); + + if ( + this.siteSettings.lazy_load_categories && + !Category.hasAsyncFoundAll([this.value]) + ) { + Category.asyncFindByIds([this.value]).then(() => { + this.notifyPropertyChange("value"); + }); + } + }, + modifyComponentForRow() { return "category-row"; }, 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 9f3f5221c4d..1f3722f097f 100644 --- a/app/assets/javascripts/select-kit/addon/components/category-selector.js +++ b/app/assets/javascripts/select-kit/addon/components/category-selector.js @@ -1,5 +1,4 @@ -import { computed } from "@ember/object"; -import { mapBy } from "@ember/object/computed"; +import { computed, defineProperty } from "@ember/object"; import Category from "discourse/models/category"; import { makeArray } from "discourse-common/lib/helpers"; import MultiSelectComponent from "select-kit/components/multi-select"; @@ -21,16 +20,47 @@ export default MultiSelectComponent.extend({ init() { this._super(...arguments); - if (!this.categories) { - this.set("categories", []); + if (this.categories && !this.categoryIds) { + defineProperty( + this, + "categoryIds", + computed("categories.[]", function () { + return this.categories.map((c) => c.id); + }) + ); } - if (!this.blockedCategories) { - this.set("blockedCategories", []); + + if (this.blockedCategories && !this.blockedCategoryIds) { + defineProperty( + this, + "blockedCategoryIds", + computed("blockedCategories.[]", function () { + return this.blockedCategories.map((c) => c.id); + }) + ); + } else if (!this.blockedCategoryIds) { + this.set("blockedCategoryIds", []); + } + + if (this.siteSettings.lazy_load_categories) { + const allCategoryIds = [ + ...new Set([...this.categoryIds, ...this.blockedCategoryIds]), + ]; + + if (!Category.hasAsyncFoundAll(allCategoryIds)) { + Category.asyncFindByIds(allCategoryIds).then(() => { + this.notifyPropertyChange("categoryIds"); + this.notifyPropertyChange("blockedCategoryIds"); + }); + } } }, - content: computed("categories.[]", "blockedCategories.[]", function () { - const blockedCategories = makeArray(this.blockedCategories); + content: computed("categoryIds.[]", "blockedCategoryIds.[]", function () { + if (this.siteSettings.lazy_load_categories) { + return Category.findByIds(this.categoryIds); + } + return Category.list().filter((category) => { if (category.isUncategorizedCategory) { if (this.options?.allowUncategorized !== undefined) { @@ -41,13 +71,15 @@ export default MultiSelectComponent.extend({ } return ( - this.categories.includes(category) || - !blockedCategories.includes(category) + this.categoryIds.includes(category.id) || + !this.blockedCategoryIds.includes(category.id) ); }); }), - value: mapBy("categories", "id"), + value: computed("categoryIds.[]", function () { + return this.categoryIds; + }), modifyComponentForRow() { return "category-row"; @@ -58,15 +90,10 @@ export default MultiSelectComponent.extend({ return this._super(filter); } - const rejectCategoryIds = new Set(); - // Reject selected options - if (this.categories) { - this.categories.forEach((c) => rejectCategoryIds.add(c.id)); - } - // Reject blocked categories - if (this.blockedCategories) { - this.blockedCategories.forEach((c) => rejectCategoryIds.add(c.id)); - } + const rejectCategoryIds = new Set([ + ...(this.categoryIds || []), + ...(this.blockedCategoryIds || []), + ]); return await Category.asyncSearch(filter, { includeUncategorized: diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 855491f93a2..c0f94046b6b 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -301,21 +301,24 @@ class CategoriesController < ApplicationController end def find - category = Category.find_by_slug_path_with_id(params[:category_slug_path_with_id]) - raise Discourse::NotFound if category.blank? - guardian.ensure_can_see!(category) + categories = [] - ancestors = Category.secured(guardian).with_ancestors(category.id).where.not(id: category.id) + if params[:ids].present? + categories = Category.secured(guardian).where(id: params[:ids]) + elsif params[:slug_path_with_id].present? + category = Category.find_by_slug_path_with_id(params[:slug_path_with_id]) + raise Discourse::NotFound if category.blank? + guardian.ensure_can_see!(category) - render json: { - category: SiteCategorySerializer.new(category, scope: guardian, root: nil), - ancestors: - ActiveModel::ArraySerializer.new( - ancestors, - scope: guardian, - each_serializer: SiteCategorySerializer, - ), - } + ancestors = Category.secured(guardian).with_ancestors(category.id).where.not(id: category.id) + categories = [*ancestors, category] + end + + raise Discourse::NotFound if categories.blank? + + Category.preload_user_fields!(guardian, categories) + + render_serialized(categories, SiteCategorySerializer, root: :categories, scope: guardian) end def search @@ -382,7 +385,9 @@ class CategoriesController < ApplicationController categories = categories.order(:id) - render json: categories, each_serializer: SiteCategorySerializer + Category.preload_user_fields!(guardian, categories) + + render_serialized(categories, SiteCategorySerializer, root: :categories, scope: guardian) end private diff --git a/app/models/category.rb b/app/models/category.rb index 69d1cf35078..ae393d7f93c 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -109,7 +109,6 @@ class Category < ActiveRecord::Base after_save :reset_topic_ids_cache after_save :clear_subcategory_ids - after_save :clear_parent_ids after_save :clear_url_cache after_save :update_reviewables after_save :publish_discourse_stylesheet @@ -130,7 +129,6 @@ class Category < ActiveRecord::Base after_destroy :reset_topic_ids_cache after_destroy :clear_subcategory_ids - after_destroy :clear_parent_ids after_destroy :publish_category_deletion after_destroy :remove_site_settings @@ -227,6 +225,34 @@ class Category < ActiveRecord::Base # Allows us to skip creating the category definition topic in tests. attr_accessor :skip_category_definition + def self.preload_user_fields!(guardian, categories) + category_ids = categories.map(&:id) + + # Load notification levels + notification_levels = CategoryUser.notification_levels_for(guardian.user) + notification_levels.default = CategoryUser.default_notification_level + + # Load permissions + allowed_topic_create_ids = + if !guardian.is_admin? && !guardian.is_anonymous? + Category.topic_create_allowed(guardian).where(id: category_ids).pluck(:id).to_set + end + + # Categories with children + with_children = + Category.where(parent_category_id: category_ids).pluck(:parent_category_id).to_set + + # Update category attributes + categories.each do |category| + category.notification_level = notification_levels[category[:id]] + + 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]) + end + end + def self.topic_id_cache @topic_id_cache ||= DistributedCache.new("category_topic_ids") end @@ -859,24 +885,9 @@ class Category < ActiveRecord::Base self.where("string_to_array(email_in, '|') @> ARRAY[?]", Email.downcase(email)).first end - @@has_children = DistributedCache.new("has_children") - - def self.has_children?(category_id) - @@has_children.defer_get_set(category_id.to_s) do - Category.where(parent_category_id: category_id).exists? - end - end - def has_children? - !!id && Category.has_children?(id) - end - - def self.clear_parent_ids - @@has_children.clear - end - - def clear_parent_ids - Category.clear_parent_ids + @has_children ||= (id && Category.where(parent_category_id: id).exists?) ? :true : :false + @has_children == :true end def uncategorized? diff --git a/app/serializers/basic_category_serializer.rb b/app/serializers/basic_category_serializer.rb index 2bd0bcf0c38..2ec27126888 100644 --- a/app/serializers/basic_category_serializer.rb +++ b/app/serializers/basic_category_serializer.rb @@ -19,7 +19,7 @@ class BasicCategorySerializer < ApplicationSerializer :notification_level, :can_edit, :topic_template, - :has_children?, + :has_children, :sort_order, :sort_ascending, :show_subcategory_list, diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 7944182bd96..4617cd45349 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -1298,8 +1298,6 @@ RSpec.describe Category do let(:guardian) { Guardian.new(admin) } fab!(:category) - before { Category.clear_parent_ids } - describe "when category is uncategorized" do it "should return the reason" do category = Category.find(SiteSetting.uncategorized_category_id) diff --git a/spec/requests/api/schemas/json/category_create_response.json b/spec/requests/api/schemas/json/category_create_response.json index d7c7e400a0d..e48af843f66 100644 --- a/spec/requests/api/schemas/json/category_create_response.json +++ b/spec/requests/api/schemas/json/category_create_response.json @@ -79,7 +79,10 @@ "items": {} }, "has_children": { - "type": "boolean" + "type": [ + "boolean", + "null" + ] }, "sort_order": { "type": [ diff --git a/spec/requests/api/schemas/json/category_update_response.json b/spec/requests/api/schemas/json/category_update_response.json index 04bccf8f889..08a6611e02d 100644 --- a/spec/requests/api/schemas/json/category_update_response.json +++ b/spec/requests/api/schemas/json/category_update_response.json @@ -61,7 +61,7 @@ }, "permission": { "type": [ - "string", + "integer", "null" ] }, @@ -82,7 +82,10 @@ "items": {} }, "has_children": { - "type": "boolean" + "type": [ + "boolean", + "null" + ] }, "sort_order": { "type": [ diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 2c2484b4f1e..aebd307ee54 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -1044,24 +1044,64 @@ RSpec.describe CategoriesController do fab!(:category) { Fabricate(:category, name: "Foo") } fab!(:subcategory) { Fabricate(:category, name: "Foobar", parent_category: category) } - it "returns the category" do - get "/categories/find.json", - params: { - category_slug_path_with_id: "#{category.slug}/#{category.id}", - } + context "with ids" do + it "returns the categories" do + get "/categories/find.json", params: { ids: [subcategory.id] } - expect(response.parsed_body["category"]["id"]).to eq(category.id) - expect(response.parsed_body["ancestors"]).to eq([]) + expect(response.parsed_body["categories"].map { |c| c["id"] }).to eq([subcategory.id]) + end + + it "does not return hidden category" do + category.update!(read_restricted: true) + + get "/categories/find.json", params: { ids: [123_456_789] } + + expect(response.status).to eq(404) + end end - it "returns the subcategory and ancestors" do - get "/categories/find.json", - params: { - category_slug_path_with_id: "#{subcategory.slug}/#{subcategory.id}", - } + context "with slug path" do + it "returns the category" do + get "/categories/find.json", + params: { + slug_path_with_id: "#{category.slug}/#{category.id}", + } - expect(response.parsed_body["category"]["id"]).to eq(subcategory.id) - expect(response.parsed_body["ancestors"].map { |c| c["id"] }).to eq([category.id]) + expect(response.parsed_body["categories"].map { |c| c["id"] }).to eq([category.id]) + end + + it "returns the subcategory and ancestors" do + get "/categories/find.json", + params: { + slug_path_with_id: "#{subcategory.slug}/#{subcategory.id}", + } + + expect(response.parsed_body["categories"].map { |c| c["id"] }).to eq( + [category.id, subcategory.id], + ) + end + + it "does not return hidden category" do + category.update!(read_restricted: true) + + get "/categories/find.json", + params: { + slug_path_with_id: "#{category.slug}/#{category.id}", + } + + expect(response.status).to eq(403) + end + end + + it "returns user fields" do + sign_in(admin) + + get "/categories/find.json", params: { slug_path_with_id: "#{category.slug}/#{category.id}" } + + category = response.parsed_body["categories"].first + 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) end end @@ -1197,5 +1237,16 @@ RSpec.describe CategoriesController do expect(response.parsed_body["categories"].size).to eq(2) end end + + it "returns user fields" do + sign_in(admin) + + get "/categories/search.json", params: { select_category_ids: [category.id] } + + category = response.parsed_body["categories"].first + 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) + end end end