FIX: Make category-drop work with lazy_load_categories (#24187)

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.
2023-11-28 17:58:47 +02:00 · 2023-11-28 17:58:47 +02:00 · e85a81f33c
parent 21d614215b
commit e85a81f33c
17 changed files with 165 additions and 63 deletions
--- 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,
      };
    });
  },
--- 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")
--- 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) {
      })}
      </span>`;
  }
-  return `<${tagName} class="badge-category__wrapper ${extraClasses}" ${href}>${html}</${tagName}>${afterBadgeWrapper}`;
+
+  const style = categoryVariables(category);
+  const extraAttrs = style.string ? `style="${style}"` : "";
+
+  return `<${tagName} class="badge-category__wrapper ${extraClasses}" ${extraAttrs} ${href}>${html}</${tagName}>${afterBadgeWrapper}`;
 }
--- 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
--- 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;
    }
  },
--- 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(
+    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");
--- 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,
--- 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,7 +141,8 @@ export default ComboBoxComponent.extend({

    if (this.siteSettings.lazy_load_categories) {
      const results = await Category.asyncSearch(filter, { ...opts, limit: 5 });
-      return results.sort((a, b) => {
+      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) {
@ -146,7 +150,8 @@ export default ComboBoxComponent.extend({
          } else {
            return 0;
          }
-      });
+        })
+      );
    }

    if (filter) {
--- 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 @@ class CategoriesController < ApplicationController
    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 @@ class CategoriesController < ApplicationController
        ) 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 @@ class CategoriesController < ApplicationController
      END
    SQL

-    categories.order(:id)
+    categories = categories.order(:id)

    render json: categories, each_serializer: SiteCategorySerializer
  end
--- 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 @@ 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?
-    @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?
--- 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 @@ class Site
             )
            categories << category
          end
-
-          if SiteSetting.lazy_load_categories && categories.size >= Site::LAZY_LOAD_CATEGORIES_LIMIT
-            break
-          end
        end

        with_children = Set.new
@ -161,9 +157,13 @@ class Site

        self.class.categories_callbacks.each { |callback| callback.call(categories, @guardian) }

+        if SiteSetting.lazy_load_categories
+          categories[0...Site::LAZY_LOAD_CATEGORIES_LIMIT]
+        else
          categories
        end
      end
+  end

  def groups
    query =
--- 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,
--- a/config/routes.rb
+++ b/config/routes.rb
@ -1160,6 +1160,7 @@ Discourse::Application.routes.draw do

    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
--- a/spec/models/category_spec.rb
+++ b/spec/models/category_spec.rb
@ -1298,6 +1298,8 @@ 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)
--- 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": [
--- 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": [
--- a/spec/requests/categories_controller_spec.rb
+++ b/spec/requests/categories_controller_spec.rb
@ -1040,6 +1040,31 @@ RSpec.describe CategoriesController do
    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 @@ RSpec.describe CategoriesController do
      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