From cdba86459877987f52def4bcfeba5c7bef7de407 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Fri, 15 Mar 2024 07:47:42 +0800
Subject: [PATCH] DEV: Support description for properties in objects schema
 (#26172)

Why this change?

When editing a objects typed theme setting, the input fields which are
rendered should include a description so that the user knows the purpose
of the field which they are changing.

What does this change do?

This change adds support for adding description to each property in the
schema for an object by following a given convention in the locale file.

For a schema like this:

```
objects_setting:
  type: objects
  schema:
    name: section
    properties:
      name:
        type: string
        required: true
      links:
        type: objects
        schema:
          name: link
          properties:
            name:
              type: string
              required: true
              validations:
                max_length: 20
            url:
              type: string
```

Description for each property in the object can be added like so:

```
en:
  theme_metadata:
    settings:
      objects_setting:
        description: <description> for the setting
        schema:
          properties:
            name: <description for the name property>
            links:
              name: <description for the name property in link>
              url: <description for the url property in link>
```

If the a description is not present, the input field will simply not
have an description.

Also note that a description for a theme setting can now be added like
so:

```
en:
  theme_metadata:
    settings:
      some_other_setting: <This will be used as the description>
      objects_setting:
        description: <This will also be used as the description>
```
---
 .../schema-theme-setting/editor.gjs           | 36 ++++++++--
 .../components/schema-theme-setting/field.gjs | 22 +++++-
 .../fixtures/theme-setting-schema-data.js     |  1 +
 .../editor-test.gjs                           | 67 ++++++++++++++++++-
 .../stylesheets/common/admin/admin_base.scss  |  1 +
 .../common/admin/schema_field.scss            | 16 +++++
 app/serializers/theme_settings_serializer.rb  | 27 ++++++--
 .../theme_locales/objects_settings/en.yaml    | 11 +++
 .../theme_settings/objects_settings.yaml      |  2 +-
 .../theme_settings_serializer_spec.rb         | 55 ++++++++++++---
 ...diting_objects_typed_theme_setting_spec.rb | 29 ++++++++
 .../admin_objects_theme_setting_editor.rb     | 20 +++++-
 12 files changed, 263 insertions(+), 24 deletions(-)
 create mode 100644 app/assets/stylesheets/common/admin/schema_field.scss
 create mode 100644 spec/fixtures/theme_locales/objects_settings/en.yaml

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 f3cf1a3ace2..c7660b23d90 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
@@ -18,13 +18,15 @@ class Node {
   schema;
   index;
   active = false;
+  parentTree;
   trees = [];
 
-  constructor({ text, index, object, schema }) {
+  constructor({ text, index, object, schema, parentTree }) {
     this.text = text;
     this.index = index;
     this.object = object;
     this.schema = schema;
+    this.parentTree = parentTree;
   }
 }
 
@@ -47,20 +49,21 @@ export default class SchemaThemeSettingEditor extends Component {
   get tree() {
     let schema = this.schema;
     let data = this.data;
+    let tree = new Tree();
 
     for (const point of this.history) {
+      tree.propertyName = point.propertyName;
       data = data[point.node.index][point.propertyName];
       schema = schema.properties[point.propertyName].schema;
     }
 
-    const tree = new Tree();
-
     data.forEach((object, index) => {
       const node = new Node({
         index,
         schema,
         object,
-        text: object[schema.identifier],
+        text: object[schema.identifier] || `${schema.name} ${index + 1}`,
+        parentTree: tree,
       });
 
       if (index === this.activeIndex) {
@@ -78,10 +81,13 @@ export default class SchemaThemeSettingEditor extends Component {
             (childObj, childIndex) => {
               subtree.nodes.push(
                 new Node({
-                  text: childObj[childObjectsProperty.schema.identifier],
+                  text:
+                    childObj[childObjectsProperty.schema.identifier] ||
+                    `${childObjectsProperty.schema.name} ${childIndex + 1}`,
                   index: childIndex,
                   object: childObj,
                   schema: childObjectsProperty.schema,
+                  parentTree: subtree,
                 })
               );
             }
@@ -117,6 +123,7 @@ export default class SchemaThemeSettingEditor extends Component {
         name,
         spec,
         value: node.object[name],
+        description: this.fieldDescription(name),
       });
     }
 
@@ -198,6 +205,24 @@ export default class SchemaThemeSettingEditor extends Component {
     this.activeNode.object[field.name] = newVal;
   }
 
+  fieldDescription(fieldName) {
+    const descriptions = this.args.setting.objects_schema_property_descriptions;
+
+    if (!descriptions) {
+      return;
+    }
+
+    let key;
+
+    if (this.activeNode.parentTree.propertyName) {
+      key = `${this.activeNode.parentTree.propertyName}.${fieldName}`;
+    } else {
+      key = `${fieldName}`;
+    }
+
+    return descriptions[key];
+  }
+
   <template>
     <div class="schema-editor-navigation">
       {{#if this.backButtonText}}
@@ -243,6 +268,7 @@ export default class SchemaThemeSettingEditor extends Component {
           @value={{field.value}}
           @spec={{field.spec}}
           @onValueChange={{fn this.inputFieldChanged field}}
+          @description={{field.description}}
         />
       {{/each}}
     </div>
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 ef07640da05..96f29021fc7 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
@@ -1,4 +1,6 @@
 import Component from "@glimmer/component";
+import { cached } from "@glimmer/tracking";
+import htmlSafe from "discourse-common/helpers/html-safe";
 import BooleanField from "./types/boolean";
 import CategoryField from "./types/category";
 import EnumField from "./types/enum";
@@ -32,16 +34,32 @@ export default class SchemaThemeSettingField extends Component {
     }
   }
 
+  @cached
+  get description() {
+    return this.args.description.trim().replace(/\n/g, "<br>");
+  }
+
+  get hasDescription() {
+    return this.args.description?.length > 0;
+  }
+
   <template>
     <div class="schema-field" data-name={{@name}}>
-      <label>{{@name}}</label>
-      <div class="input">
+      <label class="schema-field__label">{{@name}}</label>
+
+      <div class="schema-field__input">
         <this.component
           @value={{@value}}
           @spec={{@spec}}
           @onChange={{@onValueChange}}
         />
       </div>
+
+      {{#if this.hasDescription}}
+        <div class="schema-field__description">
+          {{htmlSafe this.description}}
+        </div>
+      {{/if}}
     </div>
   </template>
 }
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 55253b000ed..77dc0d0025a 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
@@ -2,6 +2,7 @@ import ThemeSettings from "admin/models/theme-settings";
 
 export default function schemaAndData(version = 1) {
   let schema, data;
+
   if (version === 1) {
     schema = {
       name: "level1",
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 affff6c7741..b7ea4182d31 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
@@ -48,8 +48,8 @@ class InputFieldsFromDOM {
     [...queryAll(".schema-field")].forEach((field) => {
       this.count += 1;
       this.fields[field.dataset.name] = {
-        labelElement: field.querySelector("label"),
-        inputElement: field.querySelector(".input").children[0],
+        labelElement: field.querySelector(".schema-field__label"),
+        inputElement: field.querySelector(".schema-field__input").children[0],
         selector: `.schema-field[data-name="${field.dataset.name}"]`,
       };
     });
@@ -606,6 +606,69 @@ module(
       assert.strictEqual(groupSelector.header().value(), "74");
     });
 
+    test("generic identifier is used when identifier is not specified in the schema", async function (assert) {
+      const setting = ThemeSettings.create({
+        setting: "objects_setting",
+        objects_schema: {
+          name: "section",
+          properties: {
+            name: {
+              type: "string",
+            },
+            links: {
+              type: "objects",
+              schema: {
+                name: "link",
+                properties: {
+                  title: {
+                    type: "string",
+                  },
+                },
+              },
+            },
+          },
+        },
+        value: [
+          {
+            name: "some section",
+            links: [
+              {
+                title: "some title",
+              },
+              {
+                title: "some other title",
+              },
+            ],
+          },
+          {
+            name: "some section 2",
+            links: [
+              {
+                title: "some title 3",
+              },
+            ],
+          },
+        ],
+      });
+
+      await render(<template>
+        <AdminSchemaThemeSettingEditor @themeId="1" @setting={{setting}} />
+      </template>);
+
+      const tree = new TreeFromDOM();
+
+      assert.dom(tree.nodes[0].element).hasText("section 1");
+      assert.dom(tree.nodes[0].children[0].element).hasText("link 1");
+      assert.dom(tree.nodes[0].children[1].element).hasText("link 2");
+      assert.dom(tree.nodes[1].element).hasText("section 2");
+
+      await click(tree.nodes[1].element);
+
+      tree.refresh();
+
+      assert.dom(tree.nodes[1].children[0].element).hasText("link 1");
+    });
+
     test("identifier field instantly updates in the navigation tree when the input field is changed", async function (assert) {
       const setting = schemaAndData(2);
 
diff --git a/app/assets/stylesheets/common/admin/admin_base.scss b/app/assets/stylesheets/common/admin/admin_base.scss
index cfb6590c33f..441e88c6e16 100644
--- a/app/assets/stylesheets/common/admin/admin_base.scss
+++ b/app/assets/stylesheets/common/admin/admin_base.scss
@@ -1050,6 +1050,7 @@ a.inline-editable-field {
 @import "common/admin/badges";
 @import "common/admin/emails";
 @import "common/admin/json_schema_editor";
+@import "common/admin/schema_field";
 @import "common/admin/staff_logs";
 @import "common/admin/customize";
 @import "common/admin/customize-install-theme";
diff --git a/app/assets/stylesheets/common/admin/schema_field.scss b/app/assets/stylesheets/common/admin/schema_field.scss
new file mode 100644
index 00000000000..cc3623e1f1f
--- /dev/null
+++ b/app/assets/stylesheets/common/admin/schema_field.scss
@@ -0,0 +1,16 @@
+.schema-field {
+  margin-bottom: 1em;
+
+  .schema-field__input {
+    input {
+      margin-bottom: 0;
+    }
+
+    margin-bottom: 0.3em;
+  }
+
+  .schema-field__description {
+    font-size: var(--font-down-1);
+    color: var(--primary-medium);
+  }
+}
diff --git a/app/serializers/theme_settings_serializer.rb b/app/serializers/theme_settings_serializer.rb
index 7ab1d54df42..80739e5bd3c 100644
--- a/app/serializers/theme_settings_serializer.rb
+++ b/app/serializers/theme_settings_serializer.rb
@@ -6,6 +6,7 @@ class ThemeSettingsSerializer < ApplicationSerializer
              :default,
              :value,
              :description,
+             :objects_schema_property_descriptions,
              :valid_values,
              :list_type,
              :textarea,
@@ -29,15 +30,31 @@ class ThemeSettingsSerializer < ApplicationSerializer
   end
 
   def description
+    description_regexp = /^theme_metadata\.settings\.#{setting}(\.description)?$/
+
     locale_file_description =
-      object
-        .theme
-        .internal_translations
-        .find { |t| t.key == "theme_metadata.settings.#{setting}" }
-        &.value
+      object.theme.internal_translations.find { |t| t.key.match?(description_regexp) }&.value
+
     locale_file_description || object.description
   end
 
+  def objects_schema_property_descriptions
+    locales = {}
+    key = "theme_metadata.settings.#{setting}.schema.properties."
+
+    object.theme.internal_translations.each do |internal_translation|
+      if internal_translation.key.start_with?(key)
+        locales[internal_translation.key.delete_prefix(key)] = internal_translation.value
+      end
+    end
+
+    locales
+  end
+
+  def include_objects_schema_property_descriptions?
+    include_objects_schema?
+  end
+
   def valid_values
     object.choices
   end
diff --git a/spec/fixtures/theme_locales/objects_settings/en.yaml b/spec/fixtures/theme_locales/objects_settings/en.yaml
new file mode 100644
index 00000000000..aecfb0cd5e7
--- /dev/null
+++ b/spec/fixtures/theme_locales/objects_settings/en.yaml
@@ -0,0 +1,11 @@
+en:
+  theme_metadata:
+    settings:
+      objects_setting:
+        description: "This is a description for objects setting"
+        schema:
+          properties:
+            name: "Section Name"
+            links:
+              name: "Name of the link"
+              url: "URL of the link"
diff --git a/spec/fixtures/theme_settings/objects_settings.yaml b/spec/fixtures/theme_settings/objects_settings.yaml
index 39c38538f58..1f8bdfeda0b 100644
--- a/spec/fixtures/theme_settings/objects_settings.yaml
+++ b/spec/fixtures/theme_settings/objects_settings.yaml
@@ -16,7 +16,7 @@ objects_setting:
         - name: "link 4"
           url: "http://example.com"
   schema:
-    name: sections
+    name: section
     properties:
       name:
         type: string
diff --git a/spec/serializers/theme_settings_serializer_spec.rb b/spec/serializers/theme_settings_serializer_spec.rb
index 69860527ed0..bb2c903eab2 100644
--- a/spec/serializers/theme_settings_serializer_spec.rb
+++ b/spec/serializers/theme_settings_serializer_spec.rb
@@ -3,20 +3,59 @@
 RSpec.describe ThemeSettingsSerializer do
   fab!(:theme)
 
-  describe "#objects_schema" do
-    let(:objects_setting) do
-      yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/objects_settings.yaml")
-      theme.set_field(target: :settings, name: "yaml", value: yaml)
-      theme.save!
-      theme.settings[:objects_setting]
-    end
+  let(:objects_setting) do
+    yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/objects_settings.yaml")
+    theme.set_field(target: :settings, name: "yaml", value: yaml)
+    theme.save!
+    theme.settings[:objects_setting]
+  end
 
+  describe "#objects_schema" do
     before { SiteSetting.experimental_objects_type_for_theme_settings = true }
 
     it "should include the attribute when theme setting is typed objects" do
       payload = ThemeSettingsSerializer.new(objects_setting).as_json
 
-      expect(payload[:theme_settings][:objects_schema][:name]).to eq("sections")
+      expect(payload[:theme_settings][:objects_schema][:name]).to eq("section")
+    end
+  end
+
+  describe "#objects_schema_property_descriptions" do
+    let(:objects_setting_locale) do
+      theme.set_field(
+        target: :translations,
+        name: "en",
+        value: File.read("#{Rails.root}/spec/fixtures/theme_locales/objects_settings/en.yaml"),
+      )
+
+      theme.save!
+    end
+
+    before { SiteSetting.experimental_objects_type_for_theme_settings = true }
+
+    it "should not include the attribute when theme setting is not typed objects" do
+      yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml")
+      theme.set_field(target: :settings, name: "yaml", value: yaml)
+      theme.save!
+
+      payload = ThemeSettingsSerializer.new(theme.settings[:string_setting]).as_json
+
+      expect(payload[:theme_settings][:objects_schema_property_descriptions]).to be_nil
+    end
+
+    it "should include the attribute when theme setting is of typed objects" do
+      objects_setting_locale
+      objects_setting
+
+      payload = ThemeSettingsSerializer.new(objects_setting).as_json
+
+      expect(payload[:theme_settings][:objects_schema_property_descriptions]).to eq(
+        {
+          "links.name" => "Name of the link",
+          "links.url" => "URL of the link",
+          "name" => "Section Name",
+        },
+      )
     end
   end
 end
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 32afbfab3a4..31b896abca6 100644
--- a/spec/system/admin_editing_objects_typed_theme_setting_spec.rb
+++ b/spec/system/admin_editing_objects_typed_theme_setting_spec.rb
@@ -28,6 +28,35 @@ RSpec.describe "Admin editing objects type theme setting", type: :system do
   end
 
   describe "when editing a theme setting of objects type" do
+    it "should display description for each property if the description has been configured in a locale file" do
+      theme.set_field(
+        target: :translations,
+        name: "en",
+        value: File.read("#{Rails.root}/spec/fixtures/theme_locales/objects_settings/en.yaml"),
+      )
+
+      theme.save!
+
+      admin_objects_theme_setting_editor_page.visit(theme, "objects_setting")
+
+      expect(admin_objects_theme_setting_editor_page).to have_setting_field_description(
+        "name",
+        "Section Name",
+      )
+
+      admin_objects_theme_setting_editor_page.click_link("link 1")
+
+      expect(admin_objects_theme_setting_editor_page).to have_setting_field_description(
+        "name",
+        "Name of the link",
+      )
+
+      expect(admin_objects_theme_setting_editor_page).to have_setting_field_description(
+        "url",
+        "URL of the link",
+      )
+    end
+
     it "should allow admin to edit the theme setting of objects type" 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 48e7308fdb9..7aef609d92c 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
@@ -3,10 +3,24 @@
 module PageObjects
   module Pages
     class AdminObjectsThemeSettingEditor < PageObjects::Pages::Base
+      def visit(theme, setting_name)
+        page.visit "/admin/customize/themes/#{theme.id}/schema/#{setting_name}"
+        self
+      end
+
       def has_setting_field?(field_name, value)
         expect(input_field(field_name).value).to eq(value)
       end
 
+      def has_setting_field_description?(field_name, description)
+        expect(input_field_description(field_name)).to have_text(description)
+      end
+
+      def click_link(name)
+        find(".schema-editor-navigation .node", text: name).click
+        self
+      end
+
       def fill_in_field(field_name, value)
         input_field(field_name).fill_in(with: value)
         self
@@ -20,7 +34,11 @@ module PageObjects
       private
 
       def input_field(field_name)
-        page.find(".schema-field[data-name=\"#{field_name}\"] input")
+        page.find(".schema-field[data-name=\"#{field_name}\"] .schema-field__input input")
+      end
+
+      def input_field_description(field_name)
+        page.find(".schema-field[data-name=\"#{field_name}\"] .schema-field__description")
       end
     end
   end