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/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 (!this.model.id) {
-          if (!model.id) {
+            this.model.setProperties({
-            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.site.updateCategory(this.model);
-            this.router.transitionTo("editCategory", Category.slugFor(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);
         });
     },
 

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

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

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) {
+        return result.store.createRecord("category", c);
-          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")]
-          );
-        }
       });
     }
 
@@ -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;
     }
   }

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

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

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%); }"
     );
   });
 });

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].setProperties({
-    siteCategories[0].id = -1000;
+      parent_category_id: -1001,
-    siteCategories[0].name = "Parent B Child A";
+      id: -1000,
-
+      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[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].setProperties({
-    siteCategories[0].id = -1000;
+      parent_category_id: -1001,
-    siteCategories[0].name = "Parent A Child A";
+      id: -1000,
-
+      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[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].setProperties({
-    siteCategories[0].id = -1000;
+      parent_category_id: -1001,
-    siteCategories[0].name = "Parent A Child A";
+      id: -1000,
-
+      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[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],
     });

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,

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"

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]);

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%); }"
     );
   });
 });