From 2190c9b95795ce172cc099ce0b2f31e105c1d0a6 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Wed, 3 Apr 2024 17:34:28 +0300 Subject: [PATCH] DEV: Make category object more Ember friendly (#26342) Some of the properties, like 'categoriesById', 'parentCategory' and 'subcategories', were updated manually when categories were loaded. This was not ideal because it required a lot of code to keep the objects in sync and some of the properties were not updated correctly. --- .../app/controllers/edit-category-tabs.js | 23 +-- .../discourse/app/models/category-list.js | 20 -- .../discourse/app/models/category.js | 27 ++- .../javascripts/discourse/app/models/site.js | 56 +---- .../javascripts/discourse/app/models/topic.js | 2 +- .../tests/acceptance/d-styles-test.js | 3 +- .../acceptance/hashtag-css-generator-test.js | 4 +- .../sidebar-user-categories-section-test.js | 195 ++++++++++-------- .../components/reorder-categories-test.gjs | 2 - .../tests/unit/models/category-test.js | 32 ++- .../discourse/tests/unit/models/site-test.js | 14 -- .../acceptance/hashtag-css-generator-test.js | 4 +- 12 files changed, 191 insertions(+), 191 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/edit-category-tabs.js b/app/assets/javascripts/discourse/app/controllers/edit-category-tabs.js index b0556e126e9..b76bb9e26ef 100644 --- a/app/assets/javascripts/discourse/app/controllers/edit-category-tabs.js +++ b/app/assets/javascripts/discourse/app/controllers/edit-category-tabs.js @@ -89,34 +89,33 @@ export default Controller.extend({ if (this.validators.some((validator) => validator())) { return; } - const model = this.model; - const parentCategory = Category.findById(model.parent_category_id); this.set("saving", true); - const previousParentCategory = model.get("parentCategory"); - model.set("parentCategory", parentCategory); - model + this.model .save() .then((result) => { - this.set("saving", false); - if (!model.id) { - model.setProperties({ + if (!this.model.id) { + this.model.setProperties({ slug: result.category.slug, id: result.category.id, can_edit: result.category.can_edit, permission: PermissionType.FULL, notification_level: NotificationLevels.REGULAR, }); - this.site.updateCategory(model); - this.router.transitionTo("editCategory", Category.slugFor(model)); + this.site.updateCategory(this.model); + this.router.transitionTo( + "editCategory", + Category.slugFor(this.model) + ); } }) .catch((error) => { popupAjaxError(error); + this.model.set("parent_category_id", undefined); + }) + .finally(() => { this.set("saving", false); - model.set("parent_category_id", undefined); - model.set("parentCategory", previousParentCategory); }); }, diff --git a/app/assets/javascripts/discourse/app/models/category-list.js b/app/assets/javascripts/discourse/app/models/category-list.js index eefc465af89..d5464d40c74 100644 --- a/app/assets/javascripts/discourse/app/models/category-list.js +++ b/app/assets/javascripts/discourse/app/models/category-list.js @@ -2,7 +2,6 @@ import ArrayProxy from "@ember/array/proxy"; import { ajax } from "discourse/lib/ajax"; import { number } from "discourse/lib/formatter"; import PreloadStore from "discourse/lib/preload-store"; -import Category from "discourse/models/category"; import Site from "discourse/models/site"; import Topic from "discourse/models/topic"; import { bind } from "discourse-common/utils/decorators"; @@ -40,25 +39,6 @@ export default class CategoryList extends ArrayProxy { } static _buildCategoryResult(c, statPeriod) { - if (c.parent_category_id) { - c.parentCategory = Category.findById(c.parent_category_id); - } - - if (c.subcategory_list) { - c.subcategories = c.subcategory_list.map((subCategory) => - this._buildCategoryResult(subCategory, statPeriod) - ); - } else if (c.subcategory_ids) { - c.subcategories = c.subcategory_ids.map((subCategoryId) => - Category.findById(parseInt(subCategoryId, 10)) - ); - } - - if (c.subcategories) { - // TODO: Not all subcategory_ids have been loaded - c.subcategories = c.subcategories?.filter(Boolean); - } - if (c.topics) { c.topics = c.topics.map((t) => Topic.create(t)); } diff --git a/app/assets/javascripts/discourse/app/models/category.js b/app/assets/javascripts/discourse/app/models/category.js index b4f79de8b78..17a1c35bf43 100644 --- a/app/assets/javascripts/discourse/app/models/category.js +++ b/app/assets/javascripts/discourse/app/models/category.js @@ -1,5 +1,5 @@ import { warn } from "@ember/debug"; -import { get } from "@ember/object"; +import { computed, get } from "@ember/object"; import { on } from "@ember-decorators/object"; import { ajax } from "discourse/lib/ajax"; import { NotificationLevels } from "discourse/lib/notification-levels"; @@ -102,7 +102,14 @@ export default class Category extends RestModel { if (!id) { return; } - return Category._idMap()[id]; + + if (typeof id === "string") { + // eslint-disable-next-line no-console + console.warn("Category.findById called with a string ID"); + id = parseInt(id, 10); + } + + return Category._idMap().get(id); } static findByIds(ids = []) { @@ -432,6 +439,22 @@ export default class Category extends RestModel { } } + @computed("parent_category_id", "site.categories.[]") + get parentCategory() { + if (this.parent_category_id) { + return Category.findById(this.parent_category_id); + } + } + + set parentCategory(newParentCategory) { + this.set("parent_category_id", newParentCategory?.id); + } + + @computed("site.categories.[]") + get subcategories() { + return this.site.categories.filterBy("parent_category_id", this.id); + } + @discourseComputed("required_tag_groups", "minimum_required_tags") minimumRequiredTags() { if (this.required_tag_groups?.length > 0) { diff --git a/app/assets/javascripts/discourse/app/models/site.js b/app/assets/javascripts/discourse/app/models/site.js index 08468b58e9b..c5628d57d4d 100644 --- a/app/assets/javascripts/discourse/app/models/site.js +++ b/app/assets/javascripts/discourse/app/models/site.js @@ -1,5 +1,5 @@ import { tracked } from "@glimmer/tracking"; -import EmberObject, { get } from "@ember/object"; +import EmberObject, { computed, get } from "@ember/object"; import { alias, sort } from "@ember/object/computed"; import { htmlSafe } from "@ember/template"; import { isEmpty } from "@ember/utils"; @@ -25,39 +25,10 @@ export default class Site extends RestModel.extend().reopenClass(Singleton) { static create() { const result = super.create.apply(this, arguments); - const store = result.store; if (result.categories) { - let subcatMap = {}; - - result.categoriesById = new Map(); result.categories = result.categories.map((c) => { - if (c.parent_category_id) { - subcatMap[c.parent_category_id] = - subcatMap[c.parent_category_id] || []; - subcatMap[c.parent_category_id].push(c.id); - } - return (result.categoriesById[c.id] = store.createRecord( - "category", - c - )); - }); - - // Associate the categories with their parents - result.categories.forEach((c) => { - let subcategoryIds = subcatMap[c.get("id")]; - if (subcategoryIds) { - c.set( - "subcategories", - subcategoryIds.map((id) => result.categoriesById[id]) - ); - } - if (c.get("parent_category_id")) { - c.set( - "parentCategory", - result.categoriesById[c.get("parent_category_id")] - ); - } + return result.store.createRecord("category", c); }); } @@ -110,6 +81,13 @@ export default class Site extends RestModel.extend().reopenClass(Singleton) { @sort("categories", "topicCountDesc") categoriesByCount; + @computed("categories.[]") + get categoriesById() { + const map = new Map(); + this.categories.forEach((c) => map.set(c.id, c)); + return map; + } + init() { super.init(...arguments); @@ -195,7 +173,6 @@ export default class Site extends RestModel.extend().reopenClass(Singleton) { const existingCategory = categories.findBy("id", id); if (existingCategory) { categories.removeObject(existingCategory); - delete this.categoriesById.categoryId; } } @@ -216,21 +193,6 @@ export default class Site extends RestModel.extend().reopenClass(Singleton) { // TODO insert in right order? newCategory = this.store.createRecord("category", newCategory); categories.pushObject(newCategory); - this.categoriesById[categoryId] = newCategory; - newCategory.set( - "parentCategory", - this.categoriesById[newCategory.parent_category_id] - ); - newCategory.set( - "subcategories", - this.categories.filterBy("parent_category_id", categoryId) - ); - if (newCategory.parentCategory) { - if (!newCategory.parentCategory.subcategories) { - newCategory.parentCategory.set("subcategories", []); - } - newCategory.parentCategory.subcategories.pushObject(newCategory); - } return newCategory; } } diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index f82a439aa3c..4e735e86a23 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -466,7 +466,7 @@ export default class Topic extends RestModel { return { type: "topic", id }; } - @computed("category_id") + @computed("category_id", "site.categoriesById.[]") get category() { return Category.findById(this.category_id); } diff --git a/app/assets/javascripts/discourse/tests/acceptance/d-styles-test.js b/app/assets/javascripts/discourse/tests/acceptance/d-styles-test.js index a495e76cbf7..76ef8e5734c 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/d-styles-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/d-styles-test.js @@ -139,7 +139,7 @@ acceptance("DStyles - category badges", function (needs) { id: 4, color: "2B81AF", text_color: "ffffff", - parentCategory: { id: 1 }, + parent_category_id: 1, name: "category3", }, ], @@ -162,6 +162,7 @@ acceptance("DStyles - category badges", function (needs) { const css = '.badge-category[data-category-id="1"] { --category-badge-color: var(--category-1-color); --category-badge-text-color: #ffffff; }\n' + + '.badge-category[data-parent-category-id="1"] { --parent-category-badge-color: var(--category-1-color); }\n' + '.badge-category[data-category-id="2"] { --category-badge-color: var(--category-2-color); --category-badge-text-color: #ffffff; }\n' + '.badge-category[data-category-id="4"] { --category-badge-color: var(--category-4-color); --category-badge-text-color: #ffffff; }'; assert.ok(document.querySelector("style#d-styles").innerHTML.includes(css)); diff --git a/app/assets/javascripts/discourse/tests/acceptance/hashtag-css-generator-test.js b/app/assets/javascripts/discourse/tests/acceptance/hashtag-css-generator-test.js index e1ba52f49a8..e240381560c 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/hashtag-css-generator-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/hashtag-css-generator-test.js @@ -13,7 +13,7 @@ acceptance("Hashtag CSS Generator", function (needs) { id: 4, color: "2B81AF", text_color: "ffffff", - parentCategory: { id: 1 }, + parent_category_id: 1, name: "category3", }, ], @@ -27,7 +27,7 @@ acceptance("Hashtag CSS Generator", function (needs) { ".hashtag-category-badge { background-color: var(--primary-medium); }\n" + ".hashtag-color--category-1 { background-color: #ff0000; }\n" + ".hashtag-color--category-2 { background-color: #333; }\n" + - ".hashtag-color--category-4 { background-color: #2B81AF; }" + ".hashtag-color--category-4 { background: linear-gradient(-90deg, #2B81AF 50%, #ff0000 50%); }" ); }); }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js index 13b734a5dbd..438fc1a4ad1 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js @@ -207,36 +207,45 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) { const site = Site.current(); const siteCategories = site.categories; - siteCategories[0].parent_category_id = -1001; - siteCategories[0].id = -1000; - siteCategories[0].name = "Parent B Child A"; - - siteCategories[1].parent_category_id = null; - siteCategories[1].id = -1001; - siteCategories[1].name = "Parent B"; - - siteCategories[2].parent_category_id = null; - siteCategories[2].id = -1002; - siteCategories[2].name = "Parent A"; - - siteCategories[3].parent_category_id = -1001; - siteCategories[3].id = -1003; - siteCategories[3].name = "Parent B Child B"; - - siteCategories[4].parent_category_id = -1002; - siteCategories[4].id = -1004; - siteCategories[4].name = "Parent A Child A"; - - siteCategories[5].parent_category_id = -1000; - siteCategories[5].id = -1005; - siteCategories[5].name = "Parent B Child A Child A"; - - site.categoriesById.clear(); - - siteCategories.forEach((category) => { - site.categoriesById[category.id] = category; + siteCategories[0].setProperties({ + parent_category_id: -1001, + id: -1000, + name: "Parent B Child A", }); + siteCategories[1].setProperties({ + parent_category_id: null, + id: -1001, + name: "Parent B", + }); + + siteCategories[2].setProperties({ + parent_category_id: null, + id: -1002, + name: "Parent A", + }); + + siteCategories[3].setProperties({ + parent_category_id: -1001, + id: -1003, + name: "Parent B Child B", + }); + + siteCategories[4].setProperties({ + parent_category_id: -1002, + id: -1004, + name: "Parent A Child A", + }); + + siteCategories[5].setProperties({ + parent_category_id: -1000, + id: -1005, + name: "Parent B Child A Child A", + }); + + // Changes to ID are not normally expected, let's force a change + site.notifyPropertyChange("categories"); + updateCurrentUser({ sidebar_category_ids: [-1005, -1004, -1003, -1002, -1000], }); @@ -270,36 +279,45 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) { const site = Site.current(); const siteCategories = site.categories; - siteCategories[0].parent_category_id = -1001; - siteCategories[0].id = -1000; - siteCategories[0].name = "Parent A Child A"; - - siteCategories[1].parent_category_id = null; - siteCategories[1].id = -1001; - siteCategories[1].name = "Parent A"; - - siteCategories[2].parent_category_id = null; - siteCategories[2].id = -1002; - siteCategories[2].name = "Parent B"; - - siteCategories[3].parent_category_id = -1001; - siteCategories[3].id = -1003; - siteCategories[3].name = "Parent A Child B"; - - siteCategories[4].parent_category_id = -1002; - siteCategories[4].id = -1004; - siteCategories[4].name = "Parent B Child A"; - - siteCategories[5].parent_category_id = -1000; - siteCategories[5].id = -1005; - siteCategories[5].name = "Parent A Child A Child A"; - - site.categoriesById.clear(); - - siteCategories.forEach((category) => { - site.categoriesById[category.id] = category; + siteCategories[0].setProperties({ + parent_category_id: -1001, + id: -1000, + name: "Parent A Child A", }); + siteCategories[1].setProperties({ + parent_category_id: null, + id: -1001, + name: "Parent A", + }); + + siteCategories[2].setProperties({ + parent_category_id: null, + id: -1002, + name: "Parent B", + }); + + siteCategories[3].setProperties({ + parent_category_id: -1001, + id: -1003, + name: "Parent A Child B", + }); + + siteCategories[4].setProperties({ + parent_category_id: -1002, + id: -1004, + name: "Parent B Child A", + }); + + siteCategories[5].setProperties({ + parent_category_id: -1000, + id: -1005, + name: "Parent A Child A Child A", + }); + + // Changes to ID are not normally expected, let's force a change + site.notifyPropertyChange("categories"); + updateCurrentUser({ sidebar_category_ids: [-1005, -1004, -1003, -1002, -1000], }); @@ -333,36 +351,45 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) { const site = Site.current(); const siteCategories = site.categories; - siteCategories[0].parent_category_id = -1001; - siteCategories[0].id = -1000; - siteCategories[0].name = "Parent A Child A"; - - siteCategories[1].parent_category_id = null; - siteCategories[1].id = -1001; - siteCategories[1].name = "Parent A"; - - siteCategories[2].parent_category_id = null; - siteCategories[2].id = -1002; - siteCategories[2].name = "Parent B"; - - siteCategories[3].parent_category_id = -1001; - siteCategories[3].id = -1003; - siteCategories[3].name = "Parent A Child B"; - - siteCategories[4].parent_category_id = -1002; - siteCategories[4].id = -1004; - siteCategories[4].name = "Parent B Child A"; - - siteCategories[5].parent_category_id = -1000; - siteCategories[5].id = -1005; - siteCategories[5].name = "Parent A Child A Child A"; - - site.categoriesById.clear(); - - siteCategories.forEach((category) => { - site.categoriesById[category.id] = category; + siteCategories[0].setProperties({ + parent_category_id: -1001, + id: -1000, + name: "Parent A Child A", }); + siteCategories[1].setProperties({ + parent_category_id: null, + id: -1001, + name: "Parent A", + }); + + siteCategories[2].setProperties({ + parent_category_id: null, + id: -1002, + name: "Parent B", + }); + + siteCategories[3].setProperties({ + parent_category_id: -1001, + id: -1003, + name: "Parent A Child B", + }); + + siteCategories[4].setProperties({ + parent_category_id: -1002, + id: -1004, + name: "Parent B Child A", + }); + + siteCategories[5].setProperties({ + parent_category_id: -1000, + id: -1005, + name: "Parent A Child A Child A", + }); + + // Changes to ID are not normally expected, let's force a change + site.notifyPropertyChange("categories"); + updateCurrentUser({ sidebar_category_ids: [-1005, -1004, -1003, -1002, -1000], }); diff --git a/app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs b/app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs index 826564e437e..a1dd706c795 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs +++ b/app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs @@ -136,13 +136,11 @@ module("Integration | Component | ReorderCategories", function (hooks) { position: 1, name: "foo-child", parent_category_id: 101, - subcategories: [fooChildChild], }); const foo = store.createRecord("category", { id: 101, position: 0, name: "foo", - subcategories: [fooChild], }); const bar = store.createRecord("category", { id: 102, diff --git a/app/assets/javascripts/discourse/tests/unit/models/category-test.js b/app/assets/javascripts/discourse/tests/unit/models/category-test.js index c5e3154888b..f41b0dc912e 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/category-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/category-test.js @@ -3,11 +3,35 @@ import { setupTest } from "ember-qunit"; import { module, test } from "qunit"; import sinon from "sinon"; import Category from "discourse/models/category"; +import Site from "discourse/models/site"; import pretender, { response } from "discourse/tests/helpers/create-pretender"; module("Unit | Model | category", function (hooks) { setupTest(hooks); + test("parentCategory and subcategories", function (assert) { + const foo = Site.current().updateCategory({ + id: 12345, + slug: "foo", + }); + + const bar = Site.current().updateCategory({ + id: 12346, + slug: "bar", + parent_category_id: 12345, + }); + + const baz = Site.current().updateCategory({ + id: 12347, + slug: "baz", + parent_category_id: 12345, + }); + + assert.deepEqual(foo.subcategories, [bar, baz]); + assert.equal(bar.parentCategory, foo); + assert.equal(baz.parentCategory, foo); + }); + test("slugFor", function (assert) { const store = getOwner(this).lookup("service:store"); @@ -36,14 +60,14 @@ module("Unit | Model | category", function (hooks) { "It can be non english characters" ); - const parentCategory = store.createRecord("category", { + const parentCategory = Site.current().updateCategory({ id: 345, slug: "darth", }); slugFor( store.createRecord("category", { slug: "luke", - parentCategory, + parent_category_id: parentCategory.id, }), "darth/luke", "it uses the parent slug before the child" @@ -52,7 +76,7 @@ module("Unit | Model | category", function (hooks) { slugFor( store.createRecord("category", { id: 555, - parentCategory, + parent_category_id: parentCategory.id, }), "darth/555-category", "it uses the parent slug before the child and then uses id" @@ -62,7 +86,7 @@ module("Unit | Model | category", function (hooks) { slugFor( store.createRecord("category", { id: 555, - parentCategory, + parent_category_id: parentCategory.id, }), "345-category/555-category", "it uses the parent before the child and uses ids for both" diff --git a/app/assets/javascripts/discourse/tests/unit/models/site-test.js b/app/assets/javascripts/discourse/tests/unit/models/site-test.js index 902fa531007..156f7655430 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/site-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/site-test.js @@ -42,20 +42,6 @@ module("Unit | Model | site", function (hooks) { "Test Subcategory", ]); - const parent = site.categories.findBy("id", 1234); - assert.present(parent, "it loaded the parent category"); - assert.blank(parent.parentCategory, "it has no parent category"); - - assert.strictEqual(parent.subcategories.length, 1); - - const subcategory = site.categories.findBy("id", 3456); - assert.present(subcategory, "it loaded the subcategory"); - assert.strictEqual( - subcategory.parentCategory, - parent, - "it has associated the child with the parent" - ); - // remove invalid category and child site.categories.removeObject(site.categories[2]); site.categories.removeObject(site.categories[0]); diff --git a/plugins/chat/test/javascripts/acceptance/hashtag-css-generator-test.js b/plugins/chat/test/javascripts/acceptance/hashtag-css-generator-test.js index 24615133d25..0355cf578a3 100644 --- a/plugins/chat/test/javascripts/acceptance/hashtag-css-generator-test.js +++ b/plugins/chat/test/javascripts/acceptance/hashtag-css-generator-test.js @@ -8,7 +8,7 @@ acceptance("Chat | Hashtag CSS Generator", function (needs) { const category3 = { id: 4, color: "2B81AF", - parentCategory: { id: 1 }, + parent_category_id: 1, name: "category3", }; @@ -71,7 +71,7 @@ acceptance("Chat | Hashtag CSS Generator", function (needs) { ".hashtag-category-badge { background-color: var(--primary-medium); }\n" + ".hashtag-color--category-1 { background-color: #ff0000; }\n" + ".hashtag-color--category-2 { background-color: #333; }\n" + - ".hashtag-color--category-4 { background-color: #2B81AF; }" + ".hashtag-color--category-4 { background: linear-gradient(-90deg, #2B81AF 50%, #ff0000 50%); }" ); }); });