Skip to content

Commit

Permalink
FIX: Avoid using categories list in computed property
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nbianca committed Nov 27, 2023
1 parent 6f3b087 commit 8ab65a7
Show file tree
Hide file tree
Showing 17 changed files with 165 additions and 63 deletions.
17 changes: 2 additions & 15 deletions app/assets/javascripts/discourse/app/components/bread-crumbs.js
Expand Up @@ -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];
Expand All @@ -44,7 +31,7 @@ export default Component.extend({
options,
isSubcategory: !!parentCategory,
noSubcategories: !category && noSubcategories,
hasOptions: options.length !== 0,
hasOptions: !parentCategory || parentCategory.has_children,
};
});
},
Expand Down
18 changes: 13 additions & 5 deletions app/assets/javascripts/discourse/app/components/d-navigation.js
Expand Up @@ -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")
Expand Down
@@ -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";
Expand Down Expand Up @@ -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}`;
}
16 changes: 16 additions & 0 deletions app/assets/javascripts/discourse/app/models/category.js
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions app/assets/javascripts/discourse/app/models/site.js
Expand Up @@ -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;
}
},
Expand Down
Expand Up @@ -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;
Expand All @@ -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");
Expand Down
Expand Up @@ -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,
Expand All @@ -158,6 +159,7 @@ export default {
background_url: null,
show_subcategory_list: false,
default_view: "latest",
has_children: true,
},
{
id: 24,
Expand Down Expand Up @@ -309,6 +311,7 @@ export default {
notification_level: null,
show_subcategory_list: false,
default_view: "latest",
has_children: true,
},
{
id: 11,
Expand Down Expand Up @@ -463,6 +466,7 @@ export default {
default_view: "latest",
subcategory_list_style: "boxes",
default_list_filter: "all",
has_children: true,
},
{
id: 240,
Expand Down Expand Up @@ -516,6 +520,7 @@ export default {
parent_category_id: null,
notification_level: null,
background_url: null,
has_children: true,
},
{
id: 1002,
Expand All @@ -533,6 +538,7 @@ export default {
parent_category_id: 1001,
notification_level: null,
background_url: null,
has_children: true,
},
{
id: 1003,
Expand Down
31 changes: 18 additions & 13 deletions app/assets/javascripts/select-kit/addon/components/category-drop.js
Expand Up @@ -54,8 +54,7 @@ export default ComboBoxComponent.extend({
return this.options.subCategory || false;
}),

categoriesWithShortcuts: computed(
"categories.[]",
shortcuts: computed(
"value",
"selectKit.options.{subCategory,noSubcategories}",
function () {
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
26 changes: 21 additions & 5 deletions app/controllers/categories_controller.rb
Expand Up @@ -11,6 +11,7 @@ class CategoriesController < ApplicationController
redirect
find_by_slug
visible_groups
find
search
]

Expand Down Expand Up @@ -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?
Expand Down Expand Up @@ -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
Expand All @@ -358,7 +374,7 @@ def search
END
SQL

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

render json: categories, each_serializer: SiteCategorySerializer
end
Expand Down
39 changes: 35 additions & 4 deletions app/models/category.rb
Expand Up @@ -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? ||
Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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?
Expand Down
12 changes: 6 additions & 6 deletions app/models/site.rb
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down

0 comments on commit 8ab65a7

Please sign in to comment.