From e2ced85757faab3b1ab56c2eb3e54f5b80ddd1e2 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 9 Apr 2024 11:26:24 +0800 Subject: [PATCH] DEV: Allow enum typed theme objects property to be optional (#26571) This commit changes enum typed theme objects property to be optional. Previously, an enum typed property is always required but we have found that this might not be ideal so we want to change it. --- .../schema-theme-setting/types/enum.gjs | 4 ++- .../editor-test.gjs | 30 +++++++++++++------ lib/theme_settings_object_validator.rb | 2 +- .../theme_settings_object_validator_spec.rb | 19 +++++++----- 4 files changed, 37 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/enum.gjs b/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/enum.gjs index 642dee97602..78827948c52 100644 --- a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/enum.gjs +++ b/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/enum.gjs @@ -5,7 +5,9 @@ import FieldInputDescription from "admin/components/schema-theme-setting/field-i import ComboBox from "select-kit/components/combo-box"; export default class SchemaThemeSettingTypeEnum extends Component { - @tracked value = this.args.value || this.args.spec.default; + @tracked + value = + this.args.value || (this.args.spec.required && this.args.spec.default); get content() { return this.args.spec.choices.map((choice) => { 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 d1d7a64f326..7f655fb9c68 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 @@ -651,14 +651,20 @@ module( default: "awesome", choices: ["nice", "cool", "awesome"], }, + required_enum_field: { + type: "enum", + default: "awesome", + required: true, + choices: ["nice", "cool", "awesome"], + }, }, }, value: [ { - enum_field: "awesome", + required_enum_field: "awesome", }, { - enum_field: "cool", + required_enum_field: "cool", }, ], }); @@ -673,25 +679,31 @@ module( `${inputFields.fields.enum_field.selector} .select-kit` ); - assert.strictEqual(enumSelector.header().value(), "awesome"); + assert.strictEqual(enumSelector.header().value(), null); - await enumSelector.expand(); - await enumSelector.selectRowByValue("nice"); + const requiredEnumSelector = selectKit( + `${inputFields.fields.required_enum_field.selector} .select-kit` + ); - assert.strictEqual(enumSelector.header().value(), "nice"); + assert.strictEqual(requiredEnumSelector.header().value(), "awesome"); + + await requiredEnumSelector.expand(); + await requiredEnumSelector.selectRowByValue("nice"); + + assert.strictEqual(requiredEnumSelector.header().value(), "nice"); const tree = new TreeFromDOM(); await click(tree.nodes[1].element); - assert.strictEqual(enumSelector.header().value(), "cool"); + assert.strictEqual(requiredEnumSelector.header().value(), "cool"); tree.refresh(); await click(tree.nodes[0].element); - assert.strictEqual(enumSelector.header().value(), "nice"); + assert.strictEqual(requiredEnumSelector.header().value(), "nice"); await click(TOP_LEVEL_ADD_BTN); - assert.strictEqual(enumSelector.header().value(), "awesome"); + assert.strictEqual(requiredEnumSelector.header().value(), "awesome"); }); test("input fields of type categories that is not required with min and max validations", async function (assert) { diff --git a/lib/theme_settings_object_validator.rb b/lib/theme_settings_object_validator.rb index b074d5504c0..68ca804d2d5 100644 --- a/lib/theme_settings_object_validator.rb +++ b/lib/theme_settings_object_validator.rb @@ -118,7 +118,7 @@ class ThemeSettingsObjectValidator value = @object[property_name] type = property_attributes[:type] - return true if (value.nil? && type != "enum") + return true if value.nil? is_value_valid = case type diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index 1f8c557a9f0..356eb395727 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/spec/lib/theme_settings_object_validator_spec.rb @@ -155,8 +155,8 @@ RSpec.describe ThemeSettingsObjectValidator do end context "for enum properties" do - let(:schema) do - { + def schema(required: false) + property = { name: "section", properties: { enum_property: { @@ -165,6 +165,9 @@ RSpec.describe ThemeSettingsObjectValidator do }, }, } + + property[:properties][:enum_property][:required] = true if required + property end it "should not return any error messages when the value of the property is in the enum" do @@ -184,14 +187,16 @@ RSpec.describe ThemeSettingsObjectValidator do ) end - it "should return the right hash of error messages when enum property is not present" do - errors = described_class.new(schema: schema, object: {}).validate + it "should not return any error messages when enum property is not present but is not required" do + expect(described_class.new(schema: schema(required: false), object: {}).validate).to eq({}) + end + + it "should return the right hash of error messages when enum property is not present and is required" do + errors = described_class.new(schema: schema(required: true), object: {}).validate expect(errors.keys).to eq(["/enum_property"]) - expect(errors["/enum_property"].full_messages).to contain_exactly( - "must be one of the following: [\"choice 1\", 2, false]", - ) + expect(errors["/enum_property"].full_messages).to contain_exactly("must be present") end end