DEV: Migrate `categories` setting to new objects setting type (#51)

Since https://github.com/discourse/discourse/commit/a440e15, we have started to support objects typed theme setting so we are switching this theme component to use it instead as it provides a much better UX for configuring the settings required for the theme component.

.discourse-compatibility

@@ -1,3 +1,4 @@
+< 3.3.0.beta2-dev: 7da1900e78ba47886f44f9ab09036c3dd6b66f56
 < 3.3.0.beta1-dev: c46f2905bde50b23f9008560c747d635f5a73455
 < 3.2.0.beta2: 1398eeeff550c1da186a47b6d3878d3230f62340
 3.1.999: e7b1e68708abaad7a815d69228075cf57b532f11

javascripts/discourse/components/category-banner.js

@@ -55,20 +55,6 @@ export default class DiscourseCategoryBanners extends Component {
     return settings.show_description && this.category.description?.length > 0;
   }
 
-  #parseCategories(categoriesStr) {
-    const categories = {};
-    categoriesStr.split("|").forEach((item) => {
-      item = item.split(":");
-
-      if (item[0]) {
-        categories[item[0].toLowerCase()] = item[1]
-          ? item[1].toLowerCase()
-          : "all";
-      }
-    });
-    return categories;
-  }
-
   #parseExceptions(exceptionsStr) {
     return exceptionsStr
       .split("|")
@@ -76,20 +62,41 @@ export default class DiscourseCategoryBanners extends Component {
       .map((value) => value.toLowerCase());
   }
 
-  #checkTargetCategory(categories) {
-    const currentCategoryName = this.category?.name.toLowerCase();
-    const parentCategoryName = this.category?.parentCategory
-      ? this.category.parentCategory.name.toLowerCase()
-      : null;
+  #checkTargetCategory() {
+    if (settings.categories.length === 0) {
+      return true;
+    }
 
-    return (
-      Object.keys(categories).length === 0 ||
-      categories[currentCategoryName] === "all" ||
-      categories[currentCategoryName] === "no_sub" ||
-      (this.category?.parentCategory &&
-        (categories[parentCategoryName] === "all" ||
-          categories[parentCategoryName] === "only_sub"))
-    );
+    const currentCategoryId = this.category?.id;
+
+    const activeCategory = settings.categories.find((category) => {
+      return category.category_id[0] === currentCategoryId;
+    });
+
+    if (
+      activeCategory &&
+      (activeCategory.target === "all" || activeCategory.target === "no_sub")
+    ) {
+      return true;
+    }
+
+    const parentCategoryId = this.category?.parentCategory?.id;
+
+    if (parentCategoryId) {
+      const activeParentCategory = settings.categories.find((category) => {
+        return category.category_id[0] === parentCategoryId;
+      });
+
+      if (
+        activeParentCategory &&
+        (activeParentCategory.target === "all" ||
+          activeParentCategory.target === "only_sub")
+      ) {
+        return true;
+      }
+    }
+
+    return false;
   }
 
   @action
@@ -117,10 +124,9 @@ export default class DiscourseCategoryBanners extends Component {
       }
     }
 
-    const categories = this.#parseCategories(settings.categories);
     const exceptions = this.#parseExceptions(settings.exceptions);
     const isException = exceptions.includes(this.category?.name.toLowerCase());
-    const isTarget = this.#checkTargetCategory(categories);
+    const isTarget = this.#checkTargetCategory();
     const hideMobile = this.site.mobileView && !settings.show_mobile;
     const hideSubCategory =
       this.category?.parentCategory && !settings.show_subcategory;

locales/en.yml

@@ -6,15 +6,20 @@ en:
       show_subcategory: 'Show banners for subcategories'
       hide_if_no_description: 'Hide banners if category description is not set'
       show_category_logo: 'Show category logo'
-      exceptions: "Banner will not show for these category NAMES."
-      categories: |
-        Banner will only show for these categories. Format as <code>name:target</code> (e.g., <code>staff:all</code>).
-        Accepted targets:
-        <ul>
-        <li> all - named category and subcategories (default);
-        <li> no_sub - only the named category;
-        <li> only_sub - only subcategories of the named category.
-        </ul>
-      plugin_outlet: "Changes the position of the banner on the page."
+      exceptions: "Banner will not show for these category NAMES"
+      categories:
+        description: Categories to show a banner for
+        schema:
+          properties:
+            category_id:
+              label: Category
+              description: "Category to show a banner for"
+            target:
+              label: Target
+              description: |
+                all - Banner will show for selected category and its subcategories
+                no_sub - Banner will only show for the selected category
+                only_sub - Banner will only show for the subcategories of the selected category
+      plugin_outlet: "Changes the position of the banner on the page"
       show_category_icon: Show category icon from the <a href="https://meta.discourse.org/t/category-icons/104683" target="_blank">Discourse Category Icons component</a>
       override_category_icon_color: When the category icons are used, enabling this will make the icon match the banner text color

migrations/settings/0001-migrate-categories-setting.js

@@ -0,0 +1,31 @@
+const VALID_TARGETS = ["all", "no_sub", "only_sub"];
+
+export default function migrate(settings, helpers) {
+  if (settings.has("categories")) {
+    const categories = settings.get("categories");
+    const newCategories = [];
+
+    categories.split("|").forEach((item) => {
+      const [categoryName, target] = item.split(":");
+      const categoryId = helpers.getCategoryIdByName(categoryName);
+
+      if (categoryId) {
+        const category = {
+          category_id: [categoryId],
+        };
+
+        if (VALID_TARGETS.includes(target)) {
+          category.target = target;
+        } else {
+          category.target = "all";
+        }
+
+        newCategories.push(category);
+      }
+    });
+
+    settings.set("categories", newCategories);
+  }
+
+  return settings;
+}

settings.yml

@@ -27,16 +27,24 @@ exceptions:
   list_type: simple
 
 categories:
-  default: ""
-  type: list
-  description: |
-    Banner will only show for these categories. Format as <code>name:target</code> (e.g., <code>staff:all</code>).
-    Accepted targets:
-    <ul>
-     <li> all - named category and subcategories (default);
-     <li> no_sub - only the named category;
-     <li> only_sub - only subcategories of the named category.
-    </ul>
+  type: objects
+  default: []
+  schema:
+    name: category
+    identifier: category_id
+    properties:
+      category_id:
+        type: categories
+        required: true
+        validations:
+          max: 1
+      target:
+        type: enum
+        default: all
+        choices:
+          - all
+          - no_sub
+          - only_sub
 
 plugin_outlet:
   default: "below-site-header"

spec/migrations/0001_migrate_categories_setting_spec.rb

@@ -0,0 +1,62 @@
+# frozen_string_literal: true
+
+RSpec.describe "0001-migrate-categories-setting migration" do
+  let!(:theme) { upload_theme_component }
+
+  fab!(:category_1) { Fabricate(:category) }
+  fab!(:category_2) { Fabricate(:category) }
+  fab!(:category_3) { Fabricate(:category) }
+  fab!(:category_4) { Fabricate(:category) }
+
+  it "sets target to `all` if previous target property is not one of `all`, `no_sub` or  `only_sub`" do
+    theme.theme_settings.create!(
+      name: "categories",
+      theme:,
+      data_type: ThemeSetting.types[:string],
+      value: "#{category_1.name}:something|#{category_2.name}:something_else",
+    )
+
+    run_theme_migration(theme, "0001-migrate-categories-setting")
+
+    expect(theme.settings[:categories].value).to eq(
+      [
+        { "category_id" => [category_1.id], "target" => "all" },
+        { "category_id" => [category_2.id], "target" => "all" },
+      ],
+    )
+  end
+
+  it "should not migrate settings where a relevant category is not found for the given category names in the database" do
+    theme.theme_settings.create!(
+      name: "categories",
+      theme:,
+      data_type: ThemeSetting.types[:string],
+      value: "non_existent_category:something",
+    )
+
+    run_theme_migration(theme, "0001-migrate-categories-setting")
+
+    expect(theme.settings[:categories].value).to eq([])
+  end
+
+  it "should migrate settings where a relevant category is found for the given category names in the database" do
+    theme.theme_settings.create!(
+      name: "categories",
+      theme:,
+      data_type: ThemeSetting.types[:string],
+      value:
+        "#{category_1.name}:no_sub|#{category_2.name}:only_sub|invalid:something_else|#{category_3.name}:all|#{category_4.name}",
+    )
+
+    run_theme_migration(theme, "0001-migrate-categories-setting")
+
+    expect(theme.settings[:categories].value).to eq(
+      [
+        { "category_id" => [category_1.id], "target" => "no_sub" },
+        { "category_id" => [category_2.id], "target" => "only_sub" },
+        { "category_id" => [category_3.id], "target" => "all" },
+        { "category_id" => [category_4.id], "target" => "all" },
+      ],
+    )
+  end
+end

spec/system/category_banners_spec.rb

@@ -26,7 +26,7 @@ RSpec.describe "Category Banners", type: :system do
 
   context "when `categories` setting has been set" do
     it "displays category banner for category and its subcategory when target is set to `all`" do
-      theme.update_setting(:categories, "#{category.name}:all")
+      theme.update_setting(:categories, [{ category_id: [category.id], target: "all" }])
       theme.save!
 
       visit(category.url)
@@ -39,7 +39,7 @@ RSpec.describe "Category Banners", type: :system do
     end
 
     it "displays category banner only for root category when target is set to `no_sub`" do
-      theme.update_setting(:categories, "#{category.name}:no_sub")
+      theme.update_setting(:categories, [{ category_id: [category.id], target: "no_sub" }])
       theme.save!
 
       visit(category.url)
@@ -52,7 +52,7 @@ RSpec.describe "Category Banners", type: :system do
     end
 
     it "displays category banner only for sub categories when target is set to `only_sub`" do
-      theme.update_setting(:categories, "#{category.name}:only_sub")
+      theme.update_setting(:categories, [{ category_id: [category.id], target: "only_sub" }])
       theme.save!
 
       visit(category.url)