DEV: Change tag type to tags type for theme object schema (#26315)

Why this change?

While working on the tag selector for the theme object editor, I
realised that there is an extremely high possibility that users might want to select
more than one tag. By supporting the ability to select more than one
tag, it also means that we get support for a single tag for free as
well.

What does this change do?

1. Change `type: tag` to `type: tags` and support `min` and `max`
   validations for `type: tags`.

2. Fix the `<SchemaThemeSetting::Types::Tags>` component to support the
   `min` and `max` validations

app/assets/javascripts/admin/addon/components/schema-theme-setting/field.gjs

@@ -8,10 +8,12 @@ import FloatField from "./types/float";
 import GroupField from "./types/group";
 import IntegerField from "./types/integer";
 import StringField from "./types/string";
-import TagField from "./types/tag";
+import TagsField from "./types/tags";
 
 export default class SchemaThemeSettingField extends Component {
   get component() {
+    const type = this.args.spec.type;
+
     switch (this.args.spec.type) {
       case "string":
         return StringField;
@@ -25,12 +27,12 @@ export default class SchemaThemeSettingField extends Component {
         return EnumField;
       case "category":
         return CategoryField;
-      case "tag":
-        return TagField;
+      case "tags":
+        return TagsField;
       case "group":
         return GroupField;
       default:
-        throw new Error("unknown type");
+        throw new Error(`unknown type ${type}`);
     }
   }
 

app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tag.gjs

@@ -1,26 +0,0 @@
-import Component from "@glimmer/component";
-import { tracked } from "@glimmer/tracking";
-import { hash } from "@ember/helper";
-import { action } from "@ember/object";
-import FieldInputDescription from "admin/components/schema-theme-setting/field-input-description";
-import TagChooser from "select-kit/components/tag-chooser";
-
-export default class SchemaThemeSettingTypeTag extends Component {
-  @tracked value = this.args.value;
-
-  @action
-  onInput(newVal) {
-    this.value = newVal;
-    this.args.onChange(newVal);
-  }
-
-  <template>
-    <TagChooser
-      @tags={{this.value}}
-      @onChange={{this.onInput}}
-      @options={{hash allowAny=false}}
-    />
-
-    <FieldInputDescription @description={{@description}} />
-  </template>
-}

app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tags.gjs

@@ -0,0 +1,65 @@
+import Component from "@glimmer/component";
+import { tracked } from "@glimmer/tracking";
+import { action } from "@ember/object";
+import { and, not } from "truth-helpers";
+import I18n from "discourse-i18n";
+import FieldInputDescription from "admin/components/schema-theme-setting/field-input-description";
+import TagChooser from "select-kit/components/tag-chooser";
+
+export default class SchemaThemeSettingTypeTags extends Component {
+  @tracked touched = false;
+  @tracked value = this.args.value;
+  required = this.args.spec.required;
+  min = this.args.spec.validations?.min;
+  max = this.args.spec.validations?.max;
+
+  @action
+  onInput(newVal) {
+    this.touched = true;
+    this.value = newVal;
+    this.args.onChange(newVal);
+  }
+
+  get tagChooserOption() {
+    return {
+      allowAny: false,
+      maximum: this.max,
+    };
+  }
+
+  get validationErrorMessage() {
+    if (!this.touched) {
+      return;
+    }
+
+    if (
+      (this.min && this.value.length < this.min) ||
+      (this.required && (!this.value || this.value.length === 0))
+    ) {
+      return I18n.t("admin.customize.theme.schema.fields.tags.at_least_tag", {
+        count: this.min,
+      });
+    }
+  }
+
+  <template>
+    <TagChooser
+      @tags={{this.value}}
+      @onChange={{this.onInput}}
+      @options={{this.tagChooserOption}}
+      class={{if this.validationErrorMessage "--invalid"}}
+    />
+
+    <div class="schema-field__input-supporting-text">
+      {{#if (and @description (not this.validationErrorMessage))}}
+        <FieldInputDescription @description={{@description}} />
+      {{/if}}
+
+      {{#if this.validationErrorMessage}}
+        <div class="schema-field__input-error">
+          {{this.validationErrorMessage}}
+        </div>
+      {{/if}}
+    </div>
+  </template>
+}

app/assets/javascripts/discourse/tests/fixtures/theme-setting-schema-data.js

@@ -189,8 +189,8 @@ export default function schemaAndData(version = 1) {
         group_field: {
           type: "group",
         },
-        tag_field: {
-          type: "tag",
+        tags_field: {
+          type: "tags",
         }
       },
     };

app/assets/javascripts/discourse/tests/integration/components/admin-schema-theme-setting/editor-test.gjs

@@ -726,34 +726,76 @@ module(
       assert.dom(categorySelector.clearButton()).exists("is clearable");
     });
 
-    test("input fields of type tag", async function (assert) {
-      const setting = schemaAndData(3);
+    test("input fields of type tags which is required", async function (assert) {
+      const setting = ThemeSettings.create({
+        setting: "objects_setting",
+        objects_schema: {
+          name: "something",
+          identifier: "id",
+          properties: {
+            required_tags: {
+              type: "tags",
+              required: true,
+              validations: {
+                min: 2,
+                max: 3,
+              },
+            },
+          },
+        },
+        value: [
+          {
+            required_tags: ["gazelle", "cat"],
+          },
+        ],
+      });
 
       await render(<template>
         <AdminSchemaThemeSettingEditor @themeId="1" @setting={{setting}} />
       </template>);
 
       const inputFields = new InputFieldsFromDOM();
+
       const tagSelector = selectKit(
-        `${inputFields.fields.tag_field.selector} .select-kit`
+        `${inputFields.fields.required_tags.selector} .select-kit`
       );
 
+      assert.strictEqual(tagSelector.header().value(), "gazelle,cat");
+
+      await tagSelector.expand();
+      await tagSelector.selectRowByIndex(2);
+      await tagSelector.collapse();
+
+      assert.strictEqual(tagSelector.header().value(), "gazelle,cat,dog");
+
+      await tagSelector.expand();
+      await tagSelector.deselectItemByName("gazelle");
+      await tagSelector.deselectItemByName("cat");
+      await tagSelector.deselectItemByName("dog");
+      await tagSelector.collapse();
+
       assert.strictEqual(tagSelector.header().value(), null);
 
+      inputFields.refresh();
+
+      assert.dom(inputFields.fields.required_tags.errorElement).hasText(
+        I18n.t("admin.customize.theme.schema.fields.tags.at_least_tag", {
+          count: 2,
+        })
+      );
+
       await tagSelector.expand();
       await tagSelector.selectRowByIndex(1);
-      await tagSelector.selectRowByIndex(3);
 
-      assert.strictEqual(tagSelector.header().value(), "gazelle,cat");
+      assert.strictEqual(tagSelector.header().value(), "gazelle");
 
-      const tree = new TreeFromDOM();
-      await click(tree.nodes[1].element);
-      assert.strictEqual(tagSelector.header().value(), null);
+      inputFields.refresh();
 
-      tree.refresh();
-
-      await click(tree.nodes[0].element);
-      assert.strictEqual(tagSelector.header().value(), "gazelle,cat");
+      assert.dom(inputFields.fields.required_tags.errorElement).hasText(
+        I18n.t("admin.customize.theme.schema.fields.tags.at_least_tag", {
+          count: 2,
+        })
+      );
     });
 
     test("input fields of type group", async function (assert) {

app/assets/stylesheets/common/admin/schema_field.scss

@@ -30,6 +30,12 @@
 
     .select-kit {
       width: 100%;
+
+      &.--invalid {
+        summary {
+          border-color: var(--danger);
+        }
+      }
     }
 
     .schema-field__input-description {

config/locales/client.en.yml

@@ -5650,6 +5650,10 @@ en:
             back_button: "Back to %{name}"
             fields:
               required: "*required"
+              tags:
+                at_least_tag:
+                  one: "at least %{count} tag is required"
+                  other: "at least %{count} tags are required"
               string:
                 too_short: "must be at least %{count} characters"
               number:

config/locales/server.en.yml

@@ -159,8 +159,12 @@ en:
         not_valid_post_value: "must be a valid post id"
         humanize_not_valid_group_value: "The property at JSON Pointer '%{property_json_pointer}' must be a valid group id."
         not_valid_group_value: "must be a valid group id"
-        humanize_not_valid_tag_value: "The property at JSON Pointer '%{property_json_pointer}' must be a valid tag name."
-        not_valid_tag_value: "must be a valid tag name"
+        humanize_not_valid_tags_value: "The property at JSON Pointer '%{property_json_pointer}' must be an array of valid tag names."
+        not_valid_tags_value: "must be an array of valid tag names"
+        humanize_tags_value_not_valid_min: "The property at JSON Pointer '%{property_json_pointer}' must have at least %{min} tag names."
+        tags_value_not_valid_min: "must have at least %{min} tag names"
+        humanize_tags_value_not_valid_max: "The property at JSON Pointer '%{property_json_pointer}' must have at most %{max} tag names."
+        tags_value_not_valid_max: "must have at most %{max} tag names"
         humanize_not_valid_upload_value: "The property at JSON Pointer '%{property_json_pointer}' must be a valid upload id."
         not_valid_upload_value: "must be a valid upload id"
         humanize_string_value_not_valid_min: "The property at JSON Pointer '%{property_json_pointer}' must be at least %{min} characters long."

lib/theme_settings_object_validator.rb

@@ -118,7 +118,7 @@ class ThemeSettingsObjectValidator
 
     is_value_valid =
       case type
-      when "string", "tag"
+      when "string"
         value.is_a?(String)
       when "integer", "category", "topic", "post", "group", "upload"
         value.is_a?(Integer)
@@ -128,6 +128,8 @@ class ThemeSettingsObjectValidator
         [true, false].include?(value)
       when "enum"
         property_attributes[:choices].include?(value)
+      when "tags"
+        value.is_a?(Array) && value.all? { |tag| tag.is_a?(String) }
       else
         add_error(property_name, :invalid_type, type:)
         return false
@@ -149,11 +151,26 @@ class ThemeSettingsObjectValidator
     return true if value.nil?
 
     case type
-    when "topic", "category", "upload", "post", "group", "tag"
+    when "topic", "category", "upload", "post", "group"
       if !valid_ids(type).include?(value)
         add_error(property_name, :"not_valid_#{type}_value")
         return false
       end
+    when "tags"
+      if !Array(value).to_set.subset?(valid_ids(type))
+        add_error(property_name, :"not_valid_#{type}_value")
+        return false
+      end
+
+      if (min = validations&.dig(:min)) && value.length < min
+        add_error(property_name, :tags_value_not_valid_min, min:)
+        return false
+      end
+
+      if (max = validations&.dig(:max)) && value.length > max
+        add_error(property_name, :tags_value_not_valid_max, max:)
+        return false
+      end
     when "string"
       if (min = validations&.dig(:min_length)) && value.length < min
         add_error(property_name, :string_value_not_valid_min, min:)
@@ -223,7 +240,7 @@ class ThemeSettingsObjectValidator
     "upload" => {
       klass: Upload,
     },
-    "tag" => {
+    "tags" => {
       klass: Tag,
       column: :name,
     },
@@ -247,7 +264,7 @@ class ThemeSettingsObjectValidator
 
     properties.each do |property_name, property_attributes|
       if property_attributes[:type] == type
-        values << object[property_name]
+        values.merge(Array(object[property_name]))
       elsif property_attributes[:type] == "objects"
         object[property_name]&.each do |child_object|
           values.merge(

spec/lib/theme_settings_object_validator_spec.rb

@@ -708,53 +708,107 @@ RSpec.describe ThemeSettingsObjectValidator do
     end
 
     context "for tag properties" do
-      it "should not return any error message when the value of the property is a valid name of a tag record" do
-        tag = Fabricate(:tag)
+      fab!(:tag_1) { Fabricate(:tag) }
+      fab!(:tag_2) { Fabricate(:tag) }
+      fab!(:tag_3) { Fabricate(:tag) }
 
-        schema = { name: "section", properties: { tag_property: { type: "tag" } } }
+      it "should not return any error message when the value of the property is an array of valid tag names" do
+        schema = { name: "section", properties: { tags_property: { type: "tags" } } }
 
         expect(
-          described_class.new(schema: schema, object: { tag_property: tag.name }).validate,
+          described_class.new(
+            schema: schema,
+            object: {
+              tags_property: [tag_1.name, tag_2.name],
+            },
+          ).validate,
         ).to eq({})
       end
 
       it "should not return any error messages when the value is not present and it's not required in the schema" do
-        schema = { name: "section", properties: { tag_property: { type: "tag" } } }
+        schema = { name: "section", properties: { tags_property: { type: "tags" } } }
         expect(described_class.new(schema: schema, object: {}).validate).to eq({})
       end
 
       it "should return the right hash of error messages when value of property is not present and it's required" do
-        schema = { name: "section", properties: { tag_property: { type: "tag", required: true } } }
-        errors = described_class.new(schema: schema, object: {}).validate
-
-        expect(errors.keys).to eq(["/tag_property"])
-        expect(errors["/tag_property"].full_messages).to contain_exactly("must be present")
-      end
-
-      it "should return the right hash of error messages when value of property is not a valid tag name" do
-        schema = { name: "section", properties: { tag_property: { type: "tag" } } }
-
-        errors = described_class.new(schema: schema, object: { tag_property: "string" }).validate
-
-        expect(errors.keys).to eq(["/tag_property"])
-
-        expect(errors["/tag_property"].full_messages).to contain_exactly("must be a valid tag name")
-      end
-
-      it "should return the right hash of error messages when value of property is not a valid name of a tag record" do
         schema = {
           name: "section",
           properties: {
-            tag_property: {
-              type: "tag",
+            tags_property: {
+              type: "tags",
+              required: true,
+            },
+          },
+        }
+        errors = described_class.new(schema: schema, object: {}).validate
+
+        expect(errors.keys).to eq(["/tags_property"])
+        expect(errors["/tags_property"].full_messages).to contain_exactly("must be present")
+      end
+
+      it "should return the right hash of error messages when value of property is not an array of tag names" do
+        schema = { name: "section", properties: { tags_property: { type: "tags" } } }
+
+        errors = described_class.new(schema: schema, object: { tags_property: "string" }).validate
+
+        expect(errors.keys).to eq(["/tags_property"])
+
+        expect(errors["/tags_property"].full_messages).to contain_exactly(
+          "must be an array of valid tag names",
+        )
+      end
+
+      it "should return the right hash of error messages when number of tag names does not satisfy min or max validations" do
+        schema = {
+          name: "section",
+          properties: {
+            tags_property: {
+              type: "tags",
+              validations: {
+                min: 1,
+                max: 2,
+              },
+            },
+          },
+        }
+
+        errors = described_class.new(schema: schema, object: { tags_property: [] }).validate
+
+        expect(errors.keys).to eq(["/tags_property"])
+
+        expect(errors["/tags_property"].full_messages).to contain_exactly(
+          "must have at least 1 tag names",
+        )
+
+        errors =
+          described_class.new(
+            schema: schema,
+            object: {
+              tags_property: [tag_1.name, tag_2.name, tag_3.name],
+            },
+          ).validate
+
+        expect(errors.keys).to eq(["/tags_property"])
+
+        expect(errors["/tags_property"].full_messages).to contain_exactly(
+          "must have at most 2 tag names",
+        )
+      end
+
+      it "should return the right hash of error messages when value of property contain tag names which are invalid" do
+        schema = {
+          name: "section",
+          properties: {
+            tags_property: {
+              type: "tags",
             },
             child_tags: {
               type: "objects",
               schema: {
                 name: "child_tag",
                 properties: {
-                  tag_property_2: {
-                    type: "tag",
+                  tags_property_2: {
+                    type: "tags",
                   },
                 },
               },
@@ -762,25 +816,27 @@ RSpec.describe ThemeSettingsObjectValidator do
           },
         }
 
+        tag_1
+
         queries =
           track_sql_queries do
             errors =
               described_class.new(
                 schema:,
                 object: {
-                  tag_property: "some random tag name",
-                  child_tags: [{ tag_property_2: "some random tag name" }],
+                  tags_property: ["some random tag name", tag_1.name],
+                  child_tags: [{ tags_property_2: ["some random tag name", tag_1.name, "abcdef"] }],
                 },
               ).validate
 
-            expect(errors.keys).to eq(%w[/tag_property /child_tags/0/tag_property_2])
+            expect(errors.keys).to eq(%w[/tags_property /child_tags/0/tags_property_2])
 
-            expect(errors["/tag_property"].full_messages).to contain_exactly(
-              "must be a valid tag name",
+            expect(errors["/tags_property"].full_messages).to contain_exactly(
+              "must be an array of valid tag names",
             )
 
-            expect(errors["/child_tags/0/tag_property_2"].full_messages).to contain_exactly(
-              "must be a valid tag name",
+            expect(errors["/child_tags/0/tags_property_2"].full_messages).to contain_exactly(
+              "must be an array of valid tag names",
             )
           end