From 8ab65a7543e64e62280c9ffa3bd45c48608b8e08 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 31 Oct 2023 23:51:54 +0200 Subject: [PATCH] FIX: Avoid using categories list in computed property The category drop was rerendered after every category async change because it updated the categories list. This is not necessary and categories can be referenced indirectly by ID instead. --- .../discourse/app/components/bread-crumbs.js | 17 +------- .../discourse/app/components/d-navigation.js | 18 ++++++--- .../discourse/app/helpers/category-link.js | 7 +++- .../discourse/app/models/category.js | 16 ++++++++ .../javascripts/discourse/app/models/site.js | 4 ++ .../app/routes/build-category-route.js | 9 +++-- .../discourse/tests/fixtures/site-fixtures.js | 6 +++ .../addon/components/category-drop.js | 31 ++++++++------- app/controllers/categories_controller.rb | 26 ++++++++++--- app/models/category.rb | 39 +++++++++++++++++-- app/models/site.rb | 12 +++--- app/serializers/basic_category_serializer.rb | 2 +- config/routes.rb | 1 + spec/models/category_spec.rb | 2 + .../json/category_create_response.json | 5 +-- .../json/category_update_response.json | 5 +-- spec/requests/categories_controller_spec.rb | 28 ++++++++++++- 17 files changed, 165 insertions(+), 63 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/bread-crumbs.js b/app/assets/javascripts/discourse/app/components/bread-crumbs.js index 86dfefb3b3302..3c5e92e7e26be 100644 --- a/app/assets/javascripts/discourse/app/components/bread-crumbs.js +++ b/app/assets/javascripts/discourse/app/components/bread-crumbs.js @@ -10,20 +10,7 @@ export default Component.extend({ editingCategory: false, editingCategoryTab: null, - @discourseComputed("categories") - filteredCategories(categories) { - return categories.filter( - (category) => - this.siteSettings.allow_uncategorized_topics || - category.id !== this.site.uncategorized_category_id - ); - }, - - @discourseComputed( - "category.ancestors", - "filteredCategories", - "noSubcategories" - ) + @discourseComputed("category.ancestors", "categories", "noSubcategories") categoryBreadcrumbs(categoryAncestors, filteredCategories, noSubcategories) { categoryAncestors = categoryAncestors || []; const parentCategories = [undefined, ...categoryAncestors]; @@ -44,7 +31,7 @@ export default Component.extend({ options, isSubcategory: !!parentCategory, noSubcategories: !category && noSubcategories, - hasOptions: options.length !== 0, + hasOptions: !parentCategory || parentCategory.has_children, }; }); }, diff --git a/app/assets/javascripts/discourse/app/components/d-navigation.js b/app/assets/javascripts/discourse/app/components/d-navigation.js index 96df798ec3b99..fbc7426b783a3 100644 --- a/app/assets/javascripts/discourse/app/components/d-navigation.js +++ b/app/assets/javascripts/discourse/app/components/d-navigation.js @@ -22,16 +22,24 @@ export default Component.extend({ // Should be a `readOnly` instead but some themes/plugins still pass // the `categories` property into this component - @discourseComputed("site.categoriesList") - categories(categoriesList) { + @discourseComputed() + categories() { + let categories = this.site.categoriesList; + + if (!this.siteSettings.allow_uncategorized_topics) { + categories = categories.filter( + (category) => category.id !== this.site.uncategorized_category_id + ); + } + if (this.currentUser?.indirectly_muted_category_ids) { - return categoriesList.filter( + categories = categories.filter( (category) => !this.currentUser.indirectly_muted_category_ids.includes(category.id) ); - } else { - return categoriesList; } + + return categories; }, @discourseComputed("category") diff --git a/app/assets/javascripts/discourse/app/helpers/category-link.js b/app/assets/javascripts/discourse/app/helpers/category-link.js index 0082706ddfffb..28029ea571f74 100644 --- a/app/assets/javascripts/discourse/app/helpers/category-link.js +++ b/app/assets/javascripts/discourse/app/helpers/category-link.js @@ -1,5 +1,6 @@ import { get } from "@ember/object"; import { htmlSafe } from "@ember/template"; +import categoryVariables from "discourse/helpers/category-variables"; import { isRTL } from "discourse/lib/text-direction"; import { escapeExpression } from "discourse/lib/utilities"; import Category from "discourse/models/category"; @@ -180,5 +181,9 @@ export function defaultCategoryLinkRenderer(category, opts) { })} `; } - return `<${tagName} class="badge-category__wrapper ${extraClasses}" ${href}>${html}${afterBadgeWrapper}`; + + const style = categoryVariables(category); + const extraAttrs = style.string ? `style="${style}"` : ""; + + return `<${tagName} class="badge-category__wrapper ${extraClasses}" ${extraAttrs} ${href}>${html}${afterBadgeWrapper}`; } diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index 28cfd7c6fc6df..a72f9ff4537df 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -488,6 +488,22 @@ Category.reopenClass({ return category; }, + async asyncFindBySlugPathWithID(slugPathWithID) { + const result = await ajax("/categories/find", { + data: { + category_slug_path_with_id: slugPathWithID, + }, + }); + + if (result["ancestors"]) { + result["ancestors"].map((category) => + Site.current().updateCategory(category) + ); + } + + return Site.current().updateCategory(result.category); + }, + findBySlugPathWithID(slugPathWithID) { let parts = slugPathWithID.split("/").filter(Boolean); // slugs found by star/glob pathing in ember do not automatically url decode - ensure that these are decoded diff --git a/app/assets/javascripts/discourse/app/models/site.js b/app/assets/javascripts/discourse/app/models/site.js index eb2d345fc37d9..9d071d1c64d1c 100644 --- a/app/assets/javascripts/discourse/app/models/site.js +++ b/app/assets/javascripts/discourse/app/models/site.js @@ -124,6 +124,10 @@ const Site = RestModel.extend({ newCategory = this.store.createRecord("category", newCategory); categories.pushObject(newCategory); this.categoriesById[categoryId] = newCategory; + newCategory.set( + "parentCategory", + this.categoriesById[newCategory.parent_category_id] + ); return newCategory; } }, diff --git a/app/assets/javascripts/discourse/app/routes/build-category-route.js b/app/assets/javascripts/discourse/app/routes/build-category-route.js index eb1b7f93b436b..26d13c3443bec 100644 --- a/app/assets/javascripts/discourse/app/routes/build-category-route.js +++ b/app/assets/javascripts/discourse/app/routes/build-category-route.js @@ -18,6 +18,7 @@ import I18n from "discourse-i18n"; class AbstractCategoryRoute extends DiscourseRoute { @service composer; @service router; + @service siteSettings; @service store; @service topicTrackingState; @service("search") searchService; @@ -29,9 +30,11 @@ class AbstractCategoryRoute extends DiscourseRoute { controllerName = "discovery/list"; async model(params, transition) { - const category = Category.findBySlugPathWithID( - params.category_slug_path_with_id - ); + const category = this.siteSettings.lazy_load_categories + ? await Category.asyncFindBySlugPathWithID( + params.category_slug_path_with_id + ) + : Category.findBySlugPathWithID(params.category_slug_path_with_id); if (!category) { this.router.replaceWith("/404"); diff --git a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js index 2f419f2610f1a..37213c52ba636 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/site-fixtures.js @@ -138,6 +138,7 @@ export default { show_subcategory_list: true, default_view: "latest", subcategory_list_style: "boxes_with_featured_topics", + has_children: true, }, { id: 6, @@ -158,6 +159,7 @@ export default { background_url: null, show_subcategory_list: false, default_view: "latest", + has_children: true, }, { id: 24, @@ -309,6 +311,7 @@ export default { notification_level: null, show_subcategory_list: false, default_view: "latest", + has_children: true, }, { id: 11, @@ -463,6 +466,7 @@ export default { default_view: "latest", subcategory_list_style: "boxes", default_list_filter: "all", + has_children: true, }, { id: 240, @@ -516,6 +520,7 @@ export default { parent_category_id: null, notification_level: null, background_url: null, + has_children: true, }, { id: 1002, @@ -533,6 +538,7 @@ export default { parent_category_id: 1001, notification_level: null, background_url: null, + has_children: true, }, { id: 1003, diff --git a/app/assets/javascripts/select-kit/addon/components/category-drop.js b/app/assets/javascripts/select-kit/addon/components/category-drop.js index 246544a3d819e..fc4b0069e4ab9 100644 --- a/app/assets/javascripts/select-kit/addon/components/category-drop.js +++ b/app/assets/javascripts/select-kit/addon/components/category-drop.js @@ -54,8 +54,7 @@ export default ComboBoxComponent.extend({ return this.options.subCategory || false; }), - categoriesWithShortcuts: computed( - "categories.[]", + shortcuts: computed( "value", "selectKit.options.{subCategory,noSubcategories}", function () { @@ -82,11 +81,15 @@ export default ComboBoxComponent.extend({ }); } - const results = this._filterUncategorized(this.categories || []); - return shortcuts.concat(results); + return shortcuts; } ), + categoriesWithShortcuts: computed("categories.[]", "shortcuts", function () { + const results = this._filterUncategorized(this.categories || []); + return this.shortcuts.concat(results); + }), + modifyNoSelection() { if (this.selectKit.options.noSubcategories) { return this.defaultItem(NO_CATEGORIES_ID, this.noCategoriesLabel); @@ -138,15 +141,17 @@ export default ComboBoxComponent.extend({ if (this.siteSettings.lazy_load_categories) { const results = await Category.asyncSearch(filter, { ...opts, limit: 5 }); - return results.sort((a, b) => { - if (a.parent_category_id && !b.parent_category_id) { - return 1; - } else if (!a.parent_category_id && b.parent_category_id) { - return -1; - } else { - return 0; - } - }); + return this.shortcuts.concat( + results.sort((a, b) => { + if (a.parent_category_id && !b.parent_category_id) { + return 1; + } else if (!a.parent_category_id && b.parent_category_id) { + return -1; + } else { + return 0; + } + }) + ); } if (filter) { diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index 6622fb784113f..dd08851c3e8ce 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -11,6 +11,7 @@ class CategoriesController < ApplicationController redirect find_by_slug visible_groups + find search ] @@ -299,6 +300,24 @@ def visible_groups render json: success_json.merge(groups: groups || []) 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) + + ancestors = Category.secured(guardian).with_ancestors(category.id).where.not(id: category.id) + + render json: { + category: SiteCategorySerializer.new(category, scope: guardian, root: nil), + ancestors: + ActiveModel::ArraySerializer.new( + ancestors, + scope: guardian, + each_serializer: SiteCategorySerializer, + ), + } + end + def search term = params[:term].to_s.strip parent_category_id = params[:parent_category_id].to_i if params[:parent_category_id].present? @@ -334,10 +353,7 @@ def search ) if term.present? categories = - categories.where( - "id = :id OR parent_category_id = :id", - id: parent_category_id, - ) if parent_category_id.present? + categories.where(parent_category_id: parent_category_id) if parent_category_id.present? categories = categories.where.not(id: SiteSetting.uncategorized_category_id) if !include_uncategorized @@ -358,7 +374,7 @@ def search END SQL - categories.order(:id) + categories = categories.order(:id) render json: categories, each_serializer: SiteCategorySerializer end diff --git a/app/models/category.rb b/app/models/category.rb index 687d0ad56e4ee..0c13d560e025f 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -107,12 +107,13 @@ class Category < ActiveRecord::Base before_save :downcase_name before_save :ensure_category_setting - after_save :publish_discourse_stylesheet - after_save :publish_category 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 + after_save :publish_category after_save do if saved_change_to_uploaded_logo_id? || saved_change_to_uploaded_logo_dark_id? || @@ -128,6 +129,8 @@ class Category < ActiveRecord::Base end 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 @@ -197,6 +200,19 @@ class Category < ActiveRecord::Base scope :post_create_allowed, ->(guardian) { scoped_to_permissions(guardian, POST_CREATION_PERMISSIONS) } + scope :with_ancestors, ->(id) { where(<<~SQL, id) } + id IN ( + WITH RECURSIVE ancestors(category_id) AS ( + SELECT ? + UNION + SELECT parent_category_id + FROM categories, ancestors + WHERE id = ancestors.category_id + ) + SELECT category_id FROM ancestors + ) + SQL + delegate :post_template, to: "self.class" # permission is just used by serialization @@ -843,9 +859,24 @@ def self.find_by_email(email) 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? - @has_children ||= (id && Category.where(parent_category_id: id).exists?) ? :true : :false - @has_children == :true + !!id && Category.has_children?(id) + end + + def self.clear_parent_ids + @@has_children.clear + end + + def clear_parent_ids + Category.clear_parent_ids end def uncategorized? diff --git a/app/models/site.rb b/app/models/site.rb index 526a5fc99885d..6cc00e7cdb6b7 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -5,7 +5,7 @@ class Site include ActiveModel::Serialization # Number of categories preloaded when lazy_load_categories is enabled - LAZY_LOAD_CATEGORIES_LIMIT = 50 + LAZY_LOAD_CATEGORIES_LIMIT = 10 cattr_accessor :preloaded_category_custom_fields @@ -120,10 +120,6 @@ def categories ) categories << category end - - if SiteSetting.lazy_load_categories && categories.size >= Site::LAZY_LOAD_CATEGORIES_LIMIT - break - end end with_children = Set.new @@ -161,7 +157,11 @@ def categories self.class.categories_callbacks.each { |callback| callback.call(categories, @guardian) } - categories + if SiteSetting.lazy_load_categories + categories[0...Site::LAZY_LOAD_CATEGORIES_LIMIT] + else + categories + end end end diff --git a/app/serializers/basic_category_serializer.rb b/app/serializers/basic_category_serializer.rb index 2ec271268884c..2bd0bcf0c3866 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/config/routes.rb b/config/routes.rb index 663a8f8d60419..9ac25194af4c1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1160,6 +1160,7 @@ def patch(*) resources :categories, except: %i[show new edit] post "categories/reorder" => "categories#reorder" + get "categories/find" => "categories#find" get "categories/search" => "categories#search" scope path: "category/:category_id" do diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 66332fbc3cfcb..eb2f017e32d9b 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -1298,6 +1298,8 @@ 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 1e91ec3c3314d..d7c7e400a0ddf 100644 --- a/spec/requests/api/schemas/json/category_create_response.json +++ b/spec/requests/api/schemas/json/category_create_response.json @@ -79,10 +79,7 @@ "items": {} }, "has_children": { - "type": [ - "string", - "null" - ] + "type": "boolean" }, "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 550382ef901a0..04bccf8f88901 100644 --- a/spec/requests/api/schemas/json/category_update_response.json +++ b/spec/requests/api/schemas/json/category_update_response.json @@ -82,10 +82,7 @@ "items": {} }, "has_children": { - "type": [ - "string", - "null" - ] + "type": "boolean" }, "sort_order": { "type": [ diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index ffa6248356f1c..ff2bbb038c819 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/spec/requests/categories_controller_spec.rb @@ -1040,6 +1040,31 @@ end end + describe "#find" 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}", + } + + expect(response.parsed_body["category"]["id"]).to eq(category.id) + expect(response.parsed_body["ancestors"]).to eq([]) + end + + it "returns the subcategory and ancestors" do + get "/categories/find.json", + params: { + category_slug_path_with_id: "#{subcategory.slug}/#{subcategory.id}", + } + + expect(response.parsed_body["category"]["id"]).to eq(subcategory.id) + expect(response.parsed_body["ancestors"].map { |c| c["id"] }).to eq([category.id]) + end + end + describe "#search" do fab!(:category) { Fabricate(:category, name: "Foo") } fab!(:subcategory) { Fabricate(:category, name: "Foobar", parent_category: category) } @@ -1066,9 +1091,8 @@ it "returns categories" do get "/categories/search.json", params: { parent_category_id: category.id } - expect(response.parsed_body["categories"].size).to eq(2) + expect(response.parsed_body["categories"].size).to eq(1) expect(response.parsed_body["categories"].map { |c| c["name"] }).to contain_exactly( - "Foo", "Foobar", ) end