From 91f0c717209ade996b9c83b26024c3522e1dd5c3 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 2 Apr 2024 11:55:51 +0800 Subject: [PATCH] UX: Improve validation error message when saving theme objects setting (#26455) Why this change? Before this change, the validation error message shown to the user when saving a theme objects setting is very cryptic. This commit changes the validation error messages to be displayed on top of the editor instead. Note that I don't think this way of displaying is the ideal state we want to get to but given the time we have this will do for now. --- .../schema-theme-setting/editor.gjs | 103 ++++---- .../schema-theme-setting/types/models.gjs | 8 +- .../admin/schema_theme_setting_editor.scss | 242 +++++++++--------- app/controllers/admin/themes_controller.rb | 8 +- spec/requests/admin/themes_controller_spec.rb | 27 ++ ...diting_objects_typed_theme_setting_spec.rb | 21 +- .../admin_objects_theme_setting_editor.rb | 8 +- 7 files changed, 243 insertions(+), 174 deletions(-) diff --git a/app/assets/javascripts/admin/addon/components/schema-theme-setting/editor.gjs b/app/assets/javascripts/admin/addon/components/schema-theme-setting/editor.gjs index 1684ad5cbec..5c6d17ce2c5 100644 --- a/app/assets/javascripts/admin/addon/components/schema-theme-setting/editor.gjs +++ b/app/assets/javascripts/admin/addon/components/schema-theme-setting/editor.gjs @@ -19,6 +19,7 @@ export default class SchemaThemeSettingNewEditor extends Component { @tracked activeDataPaths = []; @tracked activeSchemaPaths = []; @tracked saveButtonDisabled = false; + @tracked validationErrorMessage; inputFieldObserver = new Map(); data = cloneJSON(this.args.setting.value); @@ -230,58 +231,74 @@ export default class SchemaThemeSettingNewEditor extends Component { this.args.themeId ); }) - .catch(popupAjaxError) + .catch((e) => { + if (e.jqXHR.responseJSON && e.jqXHR.responseJSON.errors) { + this.validationErrorMessage = e.jqXHR.responseJSON.errors[0]; + } else { + popupAjaxError(e); + } + }) .finally(() => (this.saveButtonDisabled = false)); } diff --git a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/models.gjs b/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/models.gjs index 2fe0bb61737..6bc3958eb1d 100644 --- a/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/models.gjs +++ b/app/assets/javascripts/admin/addon/components/schema-theme-setting/types/models.gjs @@ -5,7 +5,6 @@ import { isBlank } from "@ember/utils"; import I18n from "discourse-i18n"; export default class SchemaThemeSettingTypeModels extends Component { - @tracked touched = false; @tracked value = this.args.value; required = this.args.spec.required; @@ -15,7 +14,6 @@ export default class SchemaThemeSettingTypeModels extends Component { @action onInput(newValue) { - this.touched = true; this.value = newValue; this.args.onChange(this.onChange(newValue)); } @@ -25,10 +23,6 @@ export default class SchemaThemeSettingTypeModels extends Component { } get validationErrorMessage() { - if (!this.touched) { - return; - } - const isValueBlank = isBlank(this.value); if (!this.required && isValueBlank) { @@ -36,7 +30,7 @@ export default class SchemaThemeSettingTypeModels extends Component { } if ( - (this.min && this.value.length < this.min) || + (this.min && this.value && this.value.length < this.min) || (this.required && isValueBlank) ) { return I18n.t( diff --git a/app/assets/stylesheets/common/admin/schema_theme_setting_editor.scss b/app/assets/stylesheets/common/admin/schema_theme_setting_editor.scss index 38ea4913234..b49d887716a 100644 --- a/app/assets/stylesheets/common/admin/schema_theme_setting_editor.scss +++ b/app/assets/stylesheets/common/admin/schema_theme_setting_editor.scss @@ -1,156 +1,158 @@ .schema-theme-setting-editor { - --schema-space: 0.5em; - display: grid; - grid-template-columns: minmax(10em, 0.3fr) 1fr; - grid-template-rows: auto 1fr; - gap: 0 5vw; - @include breakpoint(mobile-extra-large) { - --schema-space: 0.33em; - gap: 0 1em; - } - - &__navigation { - overflow: hidden; - align-self: start; - - ul { - list-style: none; + .schema-theme-setting-editor__wrapper { + --schema-space: 0.5em; + display: grid; + grid-template-columns: minmax(10em, 0.3fr) 1fr; + grid-template-rows: auto 1fr; + gap: 0 5vw; + @include breakpoint(mobile-extra-large) { + --schema-space: 0.33em; + gap: 0 1em; } - .schema-theme-setting-editor__tree { - border: 1px solid var(--primary-low); - overflow: auto; - margin: 0 0 2em 0; + .schema-theme-setting-editor__navigation { + overflow: hidden; + align-self: start; ul { - padding: 0 calc(var(--schema-space) * 2) 0 var(--schema-space); - margin: 0 0 1em; - li:hover { - background: var(--primary-very-low); - } - - &.--is-hidden { - display: none; - } + list-style: none; } - .--heading { - display: flex; - align-items: center; - padding: var(--schema-space) calc(var(--schema-space) * 3) - var(--schema-space) calc(var(--schema-space) * 2); - gap: 0.5em; - .d-icon { - font-size: var(--font-down-3); - margin-left: auto; - } - } + .schema-theme-setting-editor__tree { + border: 1px solid var(--primary-low); + overflow: auto; + margin: 0 0 2em 0; - .schema-theme-setting-editor__tree-node.--back-btn { - cursor: pointer; - width: 100%; - border-bottom: 1px solid var(--primary-low); - color: var(--primary-700); - &:hover { - color: var(--primary-800); - background: var(--primary-very-low); - } + ul { + padding: 0 calc(var(--schema-space) * 2) 0 var(--schema-space); + margin: 0 0 1em; + li:hover { + background: var(--primary-very-low); + } - .schema-theme-setting-editor__tree-node-text { - color: currentColor; - .d-icon { - color: currentColor; - margin-left: 0; - margin-right: var(--schema-space); + &.--is-hidden { + display: none; } } - } - .schema-theme-setting-editor__tree-node-text { - padding: var(--schema-space); - color: var(--primary); - display: flex; - align-items: center; - - span { - @include ellipsis; + .--heading { + display: flex; + align-items: center; + padding: var(--schema-space) calc(var(--schema-space) * 3) + var(--schema-space) calc(var(--schema-space) * 2); + gap: 0.5em; + .d-icon { + font-size: var(--font-down-3); + margin-left: auto; + } } - .d-icon { - margin-left: auto; - font-size: var(--font-down-3); - color: var(--primary-500); - } - } - - .schema-theme-setting-editor__tree-node { - cursor: pointer; - - &.--active { - > .schema-theme-setting-editor__tree-node-text { - background-color: var(--tertiary); - color: var(--secondary); + .schema-theme-setting-editor__tree-node.--back-btn { + cursor: pointer; + width: 100%; + border-bottom: 1px solid var(--primary-low); + color: var(--primary-700); + &:hover { + color: var(--primary-800); + background: var(--primary-very-low); + } + .schema-theme-setting-editor__tree-node-text { + color: currentColor; .d-icon { - color: var(--secondary); + color: currentColor; + margin-left: 0; + margin-right: var(--schema-space); } } } - &.--parent { - border: 2px solid transparent; + .schema-theme-setting-editor__tree-node-text { + padding: var(--schema-space); + color: var(--primary); + display: flex; + align-items: center; - &:not(:has(ul)) { + span { + @include ellipsis; + } + + .d-icon { + margin-left: auto; + font-size: var(--font-down-3); + color: var(--primary-500); + } + } + + .schema-theme-setting-editor__tree-node { + cursor: pointer; + + &.--active { + > .schema-theme-setting-editor__tree-node-text { + background-color: var(--tertiary); + color: var(--secondary); + + .d-icon { + color: var(--secondary); + } + } + } + + &.--parent { + border: 2px solid transparent; + + &:not(:has(ul)) { + &:hover { + background: var(--primary-very-low); + } + } + + &.--active { + border: 2px solid var(--tertiary); + } + + &.--add-button { + border-top: 1px solid var(--primary-low); + } + } + + &.--heading { + .d-icon { + color: var(--primary-700); + } &:hover { background: var(--primary-very-low); } } - &.--active { - border: 2px solid var(--tertiary); - } - - &.--add-button { - border-top: 1px solid var(--primary-low); - } - } - - &.--heading { - .d-icon { - color: var(--primary-700); - } - &:hover { - background: var(--primary-very-low); - } - } - - &.--child { - margin-left: var(--schema-space); - border-left: 1px solid var(--primary-200); - .schema-theme-setting-editor__tree-node-text { - color: var(--primary-800); + &.--child { + margin-left: var(--schema-space); + border-left: 1px solid var(--primary-200); + .schema-theme-setting-editor__tree-node-text { + color: var(--primary-800); + } } } } - } - .schema-theme-setting-editor__tree-add-button { - color: var(--tertiary); - width: 100%; - line-height: 1.4; // match li height - justify-content: start; - padding: var(--schema-space); + .schema-theme-setting-editor__tree-add-button { + color: var(--tertiary); + width: 100%; + line-height: 1.4; // match li height + justify-content: start; + padding: var(--schema-space); - .d-icon { - color: currentColor; - font-size: var(--font-down-1); - } - - &:hover { - background: var(--primary-very-low); - color: var(--tertiary-hover); .d-icon { color: currentColor; + font-size: var(--font-down-1); + } + + &:hover { + background: var(--primary-very-low); + color: var(--tertiary-hover); + .d-icon { + color: currentColor; + } } } } diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index f43f9d6f908..b2318ed4e6a 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -338,7 +338,13 @@ class Admin::ThemesController < Admin::AdminController new_value = params[:value] || nil previous_value = @theme.cached_settings[setting_name] - @theme.update_setting(setting_name, new_value) + + begin + @theme.update_setting(setting_name, new_value) + rescue Discourse::InvalidParameters => e + return render_json_error e.message + end + @theme.save log_theme_setting_change(setting_name, previous_value, new_value) diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index aec99fcf05b..fbe848a9b3a 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -1063,6 +1063,33 @@ RSpec.describe Admin::ThemesController do expect(user_history.action).to eq(UserHistory.actions[:change_theme_setting]) end + it "should return the right error when value used to update a theme setting of `objects` typed is invalid" do + SiteSetting.experimental_objects_type_for_theme_settings = true + + field = + theme.set_field( + target: :settings, + name: "yaml", + value: File.read("#{Rails.root}/spec/fixtures/theme_settings/objects_settings.yaml"), + ) + + theme.save! + + put "/admin/themes/#{theme.id}/setting.json", + params: { + name: "objects_setting", + value: [ + { name: "new_section", links: [{ name: "a" * 21, url: "https://some.url.com" }] }, + ].to_json, + } + + expect(response.status).to eq(422) + + expect(response.parsed_body["errors"]).to eq( + ["The property at JSON Pointer '/0/links/0/name' must be at most 20 characters long."], + ) + end + it "should be able to update a theme setting of `objects` typed" do SiteSetting.experimental_objects_type_for_theme_settings = true diff --git a/spec/system/admin_editing_objects_typed_theme_setting_spec.rb b/spec/system/admin_editing_objects_typed_theme_setting_spec.rb index 679c568b557..7bed8a48d44 100644 --- a/spec/system/admin_editing_objects_typed_theme_setting_spec.rb +++ b/spec/system/admin_editing_objects_typed_theme_setting_spec.rb @@ -46,7 +46,7 @@ RSpec.describe "Admin editing objects type theme setting", type: :system do expect(admin_objects_theme_setting_editor_page).to have_setting_field_label("name", "Name") - admin_objects_theme_setting_editor_page.click_link("link 1") + admin_objects_theme_setting_editor_page.click_child_link("link 1") expect(admin_objects_theme_setting_editor_page).to have_setting_field_description( "name", @@ -99,6 +99,25 @@ RSpec.describe "Admin editing objects type theme setting", type: :system do expect(admin_objects_theme_setting_editor).to have_setting_field("name", "section 1") end + it "displays the validation errors when an admin tries to save the settting with an invalid value" do + visit("/admin/customize/themes/#{theme.id}") + + admin_objects_theme_setting_editor = + admin_customize_themes_page.click_edit_objects_theme_setting_button("objects_setting") + + admin_objects_theme_setting_editor + .fill_in_field("name", "") + .click_link("section 2") + .fill_in_field("name", "") + .click_child_link("link 1") + .fill_in_field("name", "") + .save + + expect(find(".schema-theme-setting-editor__errors")).to have_text( + "The property at JSON Pointer '/0/name' must be present. The property at JSON Pointer '/1/name' must be present. The property at JSON Pointer '/1/links/0/name' must be present.", + ) + end + it "allows an admin to edit a theme setting of objects type via the settings editor" do visit "/admin/customize/themes/#{theme.id}" diff --git a/spec/system/page_objects/pages/admin_objects_theme_setting_editor.rb b/spec/system/page_objects/pages/admin_objects_theme_setting_editor.rb index c6cad9859af..f571d1ca8cd 100644 --- a/spec/system/page_objects/pages/admin_objects_theme_setting_editor.rb +++ b/spec/system/page_objects/pages/admin_objects_theme_setting_editor.rb @@ -20,15 +20,19 @@ module PageObjects expect(input_field_label(field_name)).to have_text(label) end - def click_link(name) + def click_link(name, child: false) find( - ".schema-theme-setting-editor__navigation .schema-theme-setting-editor__tree-node.--child", + ".schema-theme-setting-editor__navigation .schema-theme-setting-editor__tree-node#{child ? ".--child" : ".--parent"}", text: name, ).click self end + def click_child_link(name) + click_link(name, child: true) + end + def fill_in_field(field_name, value) input_field(field_name).fill_in(with: value) self