Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Bianca Nenciu 2023-12-08 12:01:08 +02:00 committed by GitHub
parent d9a422cf61
commit dcd81d56c0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 265 additions and 143 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
app/assets/javascripts/discourse/app/components/user-preferences/categories.hbs
View File

@ -9,8 +9,8 @@
}}</a>
{{/if}}
<CategorySelector
@categories={{@model.watchedCategories}}
@blockedCategories={{@selectedCategories}}
@categoryIds={{@model.watched_category_ids}}
@blockedCategoryIds={{@selectedCategoryIds}}
@onChange={{action (mut @model.watchedCategories)}}
/>
</div>
@ -26,8 +26,8 @@
}}</a>
{{/if}}
<CategorySelector
@categories={{@model.trackedCategories}}
@blockedCategories={{@selectedCategories}}
@categoryIds={{@model.tracked_category_ids}}
@blockedCategoryIds={{@selectedCategoryIds}}
@onChange={{action (mut @model.trackedCategories)}}
/>
</div>
@ -41,8 +41,8 @@
<label>{{d-icon "d-watching-first"}}
{{i18n "user.watched_first_post_categories"}}</label>
<CategorySelector
@categories={{@model.watchedFirstPostCategories}}
@blockedCategories={{@selectedCategories}}
@categoryIds={{@model.watched_first_post_category_ids}}
@blockedCategoryIds={{@selectedCategoryIds}}
@onChange={{action (mut @model.watchedFirstPostCategories)}}
/>
</div>
@ -56,8 +56,8 @@
>
<label>{{d-icon "d-regular"}} {{i18n "user.regular_categories"}}</label>
<CategorySelector
@categories={{@model.regularCategories}}
@blockedCategories={{@selectedCategories}}
@categoryIds={{@model.regular_category_ids}}
@blockedCategoryIds={{@selectedCategoryIds}}
@onChange={{action (mut @model.regularCategories)}}
/>
</div>
@ -75,8 +75,8 @@
{{/if}}
<CategorySelector
@categories={{@model.mutedCategories}}
@blockedCategories={{@selectedCategories}}
@categoryIds={{@model.muted_category_ids}}
@blockedCategoryIds={{@selectedCategoryIds}}
@onChange={{action (mut @model.mutedCategories)}}
/>
</div>

16
app/assets/javascripts/discourse/app/controllers/group-manage-categories.js
View File

@ -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);
},
});

27
app/assets/javascripts/discourse/app/controllers/preferences/categories.js
View File

@ -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

30
app/assets/javascripts/discourse/app/controllers/preferences/tracking.js
View File

@ -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")

34
app/assets/javascripts/discourse/app/models/category.js
View File

@ -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) {

20
app/assets/javascripts/discourse/app/templates/group/manage/categories.hbs
View File

@ -11,8 +11,8 @@
{{i18n "groups.notifications.watching.title"}}</label>
<CategorySelector
@categories={{this.model.watchingCategories}}
@blockedCategories={{this.selectedCategories}}
@categoryIds={{this.model.watching_category_ids}}
@blockedCategoryIds={{this.selectedCategoryIds}}
@onChange={{action (mut this.model.watchingCategories)}}
/>
@ -26,8 +26,8 @@
{{i18n "groups.notifications.tracking.title"}}</label>
<CategorySelector
@categories={{this.model.trackingCategories}}
@blockedCategories={{this.selectedCategories}}
@categoryIds={{this.model.tracking_category_ids}}
@blockedCategoryIds={{this.selectedCategoryIds}}
@onChange={{action (mut this.model.trackingCategories)}}
/>
@ -41,8 +41,8 @@
{{i18n "groups.notifications.watching_first_post.title"}}</label>
<CategorySelector
@categories={{this.model.watchingFirstPostCategories}}
@blockedCategories={{this.selectedCategories}}
@categoryIds={{this.model.watching_first_post_category_ids}}
@blockedCategoryIds={{this.selectedCategoryIds}}
@onChange={{action (mut this.model.watchingFirstPostCategories)}}
/>
@ -58,8 +58,8 @@
{{i18n "groups.notifications.regular.title"}}</label>
<CategorySelector
@categories={{this.model.regularCategories}}
@blockedCategories={{this.selectedCategories}}
@categoryIds={{this.model.regular_category_ids}}
@blockedCategoryIds={{this.selectedCategoryIds}}
@onChange={{action (mut this.model.regularCategories)}}
/>
@ -73,8 +73,8 @@
{{i18n "groups.notifications.muted.title"}}</label>
<CategorySelector
@categories={{this.model.mutedCategories}}
@blockedCategories={{this.selectedCategories}}
@categoryIds={{this.model.muted_category_ids}}
@blockedCategoryIds={{this.selectedCategoryIds}}
@onChange={{action (mut this.model.mutedCategories)}}
/>

2
app/assets/javascripts/discourse/app/templates/preferences/categories.hbs
View File

@ -1,7 +1,7 @@
<UserPreferences::Categories
@canSee={{this.canSee}}
@model={{this.model}}
@selectedCategories={{this.selectedCategories}}
@selectedCategoryIds={{this.selectedCategoryIds}}
@hideMutedTags={{this.hideMutedTags}}
@save={{action "save"}}
@siteSettings={{this.siteSettings}}

2
app/assets/javascripts/discourse/app/templates/preferences/tracking.hbs
View File

@ -57,7 +57,7 @@
<UserPreferences::Categories
@canSee={{this.canSee}}
@model={{this.model}}
@selectedCategories={{this.selectedCategories}}
@selectedCategoryIds={{this.selectedCategoryIds}}
@hideMutedTags={{this.hideMutedTags}}
@siteSettings={{this.siteSettings}}
/>

13
app/assets/javascripts/select-kit/addon/components/category-chooser.js
View File

@ -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";
},

67
app/assets/javascripts/select-kit/addon/components/category-selector.js
View File

@ -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:

33
app/controllers/categories_controller.rb
View File

@ -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

49
app/models/category.rb
View File

@ -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?

2
app/serializers/basic_category_serializer.rb
View File

@ -19,7 +19,7 @@ class BasicCategorySerializer < ApplicationSerializer
:notification_level,
:can_edit,
:topic_template,
:has_children?,
:has_children,
:sort_order,
:sort_ascending,
:show_subcategory_list,

2
spec/models/category_spec.rb
View File

@ -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)

5
spec/requests/api/schemas/json/category_create_response.json
View File

@ -79,7 +79,10 @@
"items": {}
},
"has_children": {
"type": "boolean"
"type": [
"boolean",
"null"
]
},
"sort_order": {
"type": [

7
spec/requests/api/schemas/json/category_update_response.json
View File

@ -61,7 +61,7 @@
},
"permission": {
"type": [
"string",
"integer",
"null"
]
},
@ -82,7 +82,10 @@
"items": {}
},
"has_children": {
"type": "boolean"
"type": [
"boolean",
"null"
]
},
"sort_order": {
"type": [

79
spec/requests/categories_controller_spec.rb
View File

@ -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