From 86b2e3aa3e8be30a308f1bff3664d76c5d56057a Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 22 Mar 2024 15:32:00 +0800 Subject: [PATCH] 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 `` component to support the `min` and `max` validations --- .../components/schema-theme-setting/field.gjs | 10 +- .../schema-theme-setting/types/tag.gjs | 26 ---- .../schema-theme-setting/types/tags.gjs | 65 +++++++++ .../fixtures/theme-setting-schema-data.js | 4 +- .../editor-test.gjs | 66 ++++++++-- .../common/admin/schema_field.scss | 6 + config/locales/client.en.yml | 4 + config/locales/server.en.yml | 8 +- lib/theme_settings_object_validator.rb | 25 +++- .../theme_settings_object_validator_spec.rb | 124 +++++++++++++----- 10 files changed, 254 insertions(+), 84 deletions(-) delete mode 100644 app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tag.gjs create mode 100644 app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tags.gjs diff --git a/app/assets/javascripts/admin/addon/components/schema-theme-setting/field.gjs b/app/assets/javascripts/admin/addon/components/schema-theme-setting/field.gjs index f1244071d03..a8c7715e29a 100644 --- a/app/assets/javascripts/admin/addon/components/schema-theme-setting/field.gjs +++ b/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}`); } } diff --git a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tag.gjs b/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tag.gjs deleted file mode 100644 index 1fb8a7a8d9d..00000000000 --- a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tag.gjs +++ /dev/null @@ -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); - } - - -} diff --git a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tags.gjs b/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/tags.gjs new file mode 100644 index 00000000000..5ccc04b009d --- /dev/null +++ b/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, + }); + } + } + + +} diff --git a/app/assets/javascripts/discourse/tests/fixtures/theme-setting-schema-data.js b/app/assets/javascripts/discourse/tests/fixtures/theme-setting-schema-data.js index 77dc0d0025a..de743c9620d 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/theme-setting-schema-data.js +++ b/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", } }, }; diff --git a/app/assets/javascripts/discourse/tests/integration/components/admin-schema-theme-setting/editor-test.gjs b/app/assets/javascripts/discourse/tests/integration/components/admin-schema-theme-setting/editor-test.gjs index 2a694fbd25f..9ae8eee39ca 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/admin-schema-theme-setting/editor-test.gjs +++ b/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(); 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) { diff --git a/app/assets/stylesheets/common/admin/schema_field.scss b/app/assets/stylesheets/common/admin/schema_field.scss index 1660c2eba24..e9b37171d43 100644 --- a/app/assets/stylesheets/common/admin/schema_field.scss +++ b/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 { diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 92a04329ff6..71134fa16e2 100644 --- a/config/locales/client.en.yml +++ b/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: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 9a8b707c83b..5b9f189500f 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/lib/theme_settings_object_validator.rb b/lib/theme_settings_object_validator.rb index 039120088c2..a57d5ce8451 100644 --- a/lib/theme_settings_object_validator.rb +++ b/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( diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index 368de4012c3..d1459ac8dbd 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/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