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.
2024-04-02 11:55:51 +08:00 · 2024-04-02 11:55:51 +08:00 · 91f0c71720
parent e58110a9a0
commit 91f0c71720
7 changed files with 243 additions and 174 deletions
--- 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));
  }

  <template>
    <div class="schema-theme-setting-editor">
-      <div class="schema-theme-setting-editor__navigation">
-        <Tree
-          @data={{this.activeData}}
-          @schema={{this.activeSchema}}
-          @onChildClick={{this.onChildClick}}
-          @clickBack={{this.clickBack}}
-          @backButtonText={{this.backButtonText}}
-          @activeIndex={{this.activeIndex}}
-          @updateIndex={{this.updateIndex}}
-          @addItem={{this.addItem}}
-          @addChildItem={{this.addChildItem}}
-          @generateSchemaTitle={{this.generateSchemaTitle}}
-          @registerInputFieldObserver={{this.registerInputFieldObserver}}
-          @unregisterInputFieldObserver={{this.unregisterInputFieldObserver}}
-        />
-
-        <div class="schema-theme-setting-editor__footer">
-          <DButton
-            @disabled={{this.saveButtonDisabled}}
-            @action={{this.saveChanges}}
-            @label="save"
-            class="btn-primary"
-          />
+      {{#if this.validationErrorMessage}}
+        <div class="schema-theme-setting-editor__errors">
+          <div class="alert alert-error">
+            {{this.validationErrorMessage}}
+          </div>
        </div>
-      </div>
+      {{/if}}

-      <div class="schema-theme-setting-editor__fields">
-        {{#each this.fields as |field|}}
-          <FieldInput
-            @name={{field.name}}
-            @value={{field.value}}
-            @spec={{field.spec}}
-            @onValueChange={{fn this.inputFieldChanged field}}
-            @description={{field.description}}
-            @label={{field.label}}
-            @setting={{@setting}}
+      <div class="schema-theme-setting-editor__wrapper">
+        <div class="schema-theme-setting-editor__navigation">
+          <Tree
+            @data={{this.activeData}}
+            @schema={{this.activeSchema}}
+            @onChildClick={{this.onChildClick}}
+            @clickBack={{this.clickBack}}
+            @backButtonText={{this.backButtonText}}
+            @activeIndex={{this.activeIndex}}
+            @updateIndex={{this.updateIndex}}
+            @addItem={{this.addItem}}
+            @addChildItem={{this.addChildItem}}
+            @generateSchemaTitle={{this.generateSchemaTitle}}
+            @registerInputFieldObserver={{this.registerInputFieldObserver}}
+            @unregisterInputFieldObserver={{this.unregisterInputFieldObserver}}
          />
-        {{/each}}

-        {{#if (gt this.fields.length 0)}}
-          <DButton
-            @action={{this.removeItem}}
-            @icon="trash-alt"
-            class="btn-danger schema-theme-setting-editor__remove-btn"
-          />
-        {{/if}}
+          <div class="schema-theme-setting-editor__footer">
+            <DButton
+              @disabled={{this.saveButtonDisabled}}
+              @action={{this.saveChanges}}
+              @label="save"
+              class="btn-primary"
+            />
+          </div>
+        </div>
+
+        <div class="schema-theme-setting-editor__fields">
+          {{#each this.fields as |field|}}
+            <FieldInput
+              @name={{field.name}}
+              @value={{field.value}}
+              @spec={{field.spec}}
+              @onValueChange={{fn this.inputFieldChanged field}}
+              @description={{field.description}}
+              @label={{field.label}}
+              @setting={{@setting}}
+            />
+          {{/each}}
+
+          {{#if (gt this.fields.length 0)}}
+            <DButton
+              @action={{this.removeItem}}
+              @icon="trash-alt"
+              class="btn-danger schema-theme-setting-editor__remove-btn"
+            />
+          {{/if}}
+        </div>
      </div>
    </div>
  </template>
--- 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(
--- 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;
+          }
        }
      }
    }
--- 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)
--- 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

--- 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}"

--- 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