FIX: display validation under custom sidebar fields (#20772)

Before, incorrectly filled fields were marked with red border. Now, additional information under the field is displayed to notify the user what is incorrect.

/t/93696

app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js

@@ -8,6 +8,7 @@ import I18n from "I18n";
 import { sanitize } from "discourse/lib/text";
 import { tracked } from "@glimmer/tracking";
 import { A } from "@ember/array";
+import { SIDEBAR_SECTION, SIDEBAR_URL } from "discourse/lib/constants";
 
 const FULL_RELOAD_LINKS_REGEX = [/^\/my\/[a-z_\-\/]+$/, /^\/safe-mode$/];
 
@@ -29,12 +30,34 @@ class Section {
   }
 
   get validTitle() {
-    return !isEmpty(this.title) && this.title.length <= 30;
+    return !this.#blankTitle && !this.#tooLongTitle;
+  }
+
+  get invalidTitleMessage() {
+    if (this.title === undefined) {
+      return;
+    }
+    if (this.#blankTitle) {
+      return I18n.t("sidebar.sections.custom.title.validation.blank");
+    }
+    if (this.#tooLongTitle) {
+      return I18n.t("sidebar.sections.custom.title.validation.maximum", {
+        count: SIDEBAR_SECTION.max_title_length,
+      });
+    }
   }
 
   get titleCssClass() {
     return this.title === undefined || this.validTitle ? "" : "warning";
   }
+
+  get #blankTitle() {
+    return isEmpty(this.title);
+  }
+
+  get #tooLongTitle() {
+    return this.title.length > SIDEBAR_SECTION.max_title_length;
+  }
 }
 
 class SectionLink {
@@ -62,21 +85,71 @@ class SectionLink {
   }
 
   get validIcon() {
-    return !isEmpty(this.icon) && this.icon.length <= 40;
+    return !this.#blankIcon && !this.#tooLongIcon;
+  }
+
+  get validName() {
+    return !this.#blankName && !this.#tooLongName;
+  }
+
+  get validValue() {
+    return !this.#blankValue && !this.#tooLongValue && !this.#invalidValue;
+  }
+
+  get invalidIconMessage() {
+    if (this.#blankIcon) {
+      return I18n.t("sidebar.sections.custom.links.icon.validation.blank");
+    }
+    if (this.#tooLongIcon) {
+      return I18n.t("sidebar.sections.custom.links.icon.validation.maximum", {
+        count: SIDEBAR_URL.max_icon_length,
+      });
+    }
+  }
+
+  get invalidNameMessage() {
+    if (this.name === undefined) {
+      return;
+    }
+    if (this.#blankName) {
+      return I18n.t("sidebar.sections.custom.links.name.validation.blank");
+    }
+    if (this.#tooLongName) {
+      return I18n.t("sidebar.sections.custom.links.name.validation.maximum", {
+        count: SIDEBAR_URL.max_name_length,
+      });
+    }
+  }
+
+  get invalidValueMessage() {
+    if (this.value === undefined) {
+      return;
+    }
+    if (this.#blankValue) {
+      return I18n.t("sidebar.sections.custom.links.value.validation.blank");
+    }
+    if (this.#tooLongValue) {
+      return I18n.t("sidebar.sections.custom.links.value.validation.maximum", {
+        count: SIDEBAR_URL.max_value_length,
+      });
+    }
+    if (this.#invalidValue) {
+      return I18n.t("sidebar.sections.custom.links.value.validation.invalid");
+    }
   }
 
   get iconCssClass() {
     return this.icon === undefined || this.validIcon ? "" : "warning";
   }
 
-  get validName() {
-    return !isEmpty(this.name) && this.name.length <= 80;
-  }
-
   get nameCssClass() {
     return this.name === undefined || this.validName ? "" : "warning";
   }
 
+  get valueCssClass() {
+    return this.value === undefined || this.validValue ? "" : "warning";
+  }
+
   get external() {
     return (
       this.value &&
@@ -88,6 +161,37 @@ class SectionLink {
     );
   }
 
+  get #blankIcon() {
+    return isEmpty(this.icon);
+  }
+
+  get #tooLongIcon() {
+    return this.icon.length > SIDEBAR_URL.max_icon_length;
+  }
+
+  get #blankName() {
+    return isEmpty(this.name);
+  }
+
+  get #tooLongName() {
+    return this.name.length > SIDEBAR_URL.max_name_length;
+  }
+
+  get #blankValue() {
+    return isEmpty(this.value);
+  }
+
+  get #tooLongValue() {
+    return this.value.length > SIDEBAR_URL.max_value_length;
+  }
+
+  get #invalidValue() {
+    return (
+      this.path &&
+      (this.external ? !this.#validExternal() : !this.#validInternal())
+    );
+  }
+
   #validExternal() {
     try {
       return new URL(this.value);
@@ -102,19 +206,6 @@ class SectionLink {
       FULL_RELOAD_LINKS_REGEX.some((regex) => this.path.match(regex))
     );
   }
-
-  get validValue() {
-    return (
-      !isEmpty(this.value) &&
-      this.value.length <= 200 &&
-      this.path &&
-      (this.external ? this.#validExternal() : this.#validInternal())
-    );
-  }
-
-  get valueCssClass() {
-    return this.value === undefined || this.validValue ? "" : "warning";
-  }
 }
 
 export default Controller.extend(ModalFunctionality, {

app/assets/javascripts/discourse/app/lib/constants.js

@@ -11,3 +11,13 @@ export const SEARCH_PRIORITIES = {
 };
 
 export const SEARCH_PHRASE_REGEXP = '"([^"]+)"';
+
+export const SIDEBAR_URL = {
+  max_icon_length: 40,
+  max_name_length: 80,
+  max_value_length: 200,
+};
+
+export const SIDEBAR_SECTION = {
+  max_title_length: 30,
+};

app/assets/javascripts/discourse/app/templates/modal/sidebar-section-form.hbs

@@ -6,7 +6,9 @@
 <DModalBody @title={{this.header}}>
   <form class="form-horizontal">
     <div class="input-group">
-      <label for="section-name">{{i18n "sidebar.sections.custom.name"}}</label>
+      <label for="section-name">{{i18n
+          "sidebar.sections.custom.title.label"
+        }}</label>
       <Input
         name="section-name"
         @type="text"
@@ -14,12 +16,17 @@
         class={{this.model.titleCssClass}}
         {{on "input" (action (mut this.model.title) value="target.value")}}
       />
+      {{#if this.model.invalidTitleMessage}}
+        <div class="title warning">
+          {{this.model.invalidTitleMessage}}
+        </div>
+      {{/if}}
     </div>
     {{#each this.activeLinks as |link|}}
       <div class="row-wrapper">
         <div class="input-group">
           <label for="link-name">{{i18n
-              "sidebar.sections.custom.links.icon"
+              "sidebar.sections.custom.links.icon.label"
             }}</label>
           <IconPicker
             @name="icon"
@@ -29,10 +36,15 @@
             @onlyAvailable={{true}}
             @onChange={{action (mut link.icon)}}
           />
+          {{#if link.invalidIconMessage}}
+            <div class="icon warning">
+              {{link.invalidIconMessage}}
+            </div>
+          {{/if}}
         </div>
         <div class="input-group">
           <label for="link-name">{{i18n
-              "sidebar.sections.custom.links.name"
+              "sidebar.sections.custom.links.name.label"
             }}</label>
           <Input
             name="link-name"
@@ -41,10 +53,15 @@
             class={{link.nameCssClass}}
             {{on "input" (action (mut link.name) value="target.value")}}
           />
+          {{#if link.invalidNameMessage}}
+            <div class="name warning">
+              {{link.invalidNameMessage}}
+            </div>
+          {{/if}}
         </div>
         <div class="input-group">
           <label for="link-url">{{i18n
-              "sidebar.sections.custom.links.value"
+              "sidebar.sections.custom.links.value.label"
             }}</label>
           <Input
             name="link-url"
@@ -53,6 +70,11 @@
             class={{link.valueCssClass}}
             {{on "input" (action (mut link.value) value="target.value")}}
           />
+          {{#if link.invalidValueMessage}}
+            <div class="value warning">
+              {{link.invalidValueMessage}}
+            </div>
+          {{/if}}
         </div>
         <DButton
           @icon="trash-alt"

app/models/sidebar_section.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class SidebarSection < ActiveRecord::Base
+  MAX_TITLE_LENGTH = 30
+
   belongs_to :user
   has_many :sidebar_section_links, -> { order("position") }, dependent: :destroy
   has_many :sidebar_urls,
@@ -10,7 +12,14 @@ class SidebarSection < ActiveRecord::Base
 
   accepts_nested_attributes_for :sidebar_urls, allow_destroy: true
 
-  validates :title, presence: true, uniqueness: { scope: %i[user_id] }, length: { maximum: 30 }
+  validates :title,
+            presence: true,
+            uniqueness: {
+              scope: %i[user_id],
+            },
+            length: {
+              maximum: MAX_TITLE_LENGTH,
+            }
 end
 
 # == Schema Information

app/models/sidebar_url.rb

@@ -2,10 +2,13 @@
 
 class SidebarUrl < ActiveRecord::Base
   FULL_RELOAD_LINKS_REGEX = [%r{\A/my/[a-z_\-/]+\z}, %r{\A/safe-mode\z}]
+  MAX_ICON_LENGTH = 40
+  MAX_NAME_LENGTH = 80
+  MAX_VALUE_LENGTH = 200
 
-  validates :icon, presence: true, length: { maximum: 40 }
-  validates :name, presence: true, length: { maximum: 80 }
-  validates :value, presence: true, length: { maximum: 200 }
+  validates :icon, presence: true, length: { maximum: MAX_ICON_LENGTH }
+  validates :name, presence: true, length: { maximum: MAX_NAME_LENGTH }
+  validates :value, presence: true, length: { maximum: MAX_VALUE_LENGTH }
 
   validate :path_validator
 

config/locales/client.en.yml

@@ -4389,17 +4389,34 @@ en:
         custom:
           add: "Add custom section"
           edit: "Edit custom section"
-          name: "Section title"
           save: "Save"
           delete: "Delete"
           delete_confirm: "Are you sure you want to delete this section?"
           public: "Make this section public and visible to everyone"
           links:
-            icon: "Icon"
-            name: "Name"
-            value: "Link"
             add: "Add another link"
             delete: "Delete link"
+            icon:
+              label: "Icon"
+              validation:
+                blank: "Icon cannot be blank"
+                maximum: "Icon must be shorter than %{count} characters"
+            name: 
+              label: "Name"
+              validation:
+                blank: "Name cannot be blank"
+                maximum: "Name must be shorter than %{count} characters"
+            value: 
+              label: "Link"
+              validation:
+                blank: "Link cannot be blank"
+                maximum: "Link must be shorter than %{count} characters"
+                invalid: "Format is invalid"
+          title:
+            label: "Section title"
+            validation:
+              blank: "Title cannot be blank"
+              maximum: "Title must be shorter than %{count} characters"
         about:
           header_link_text: "About"
         messages:

lib/tasks/javascript.rake

@@ -163,6 +163,16 @@ task "javascript:update_constants" => :environment do
     export const SEARCH_PRIORITIES = #{Searchable::PRIORITIES.to_json};
 
     export const SEARCH_PHRASE_REGEXP = '#{Search::PHRASE_MATCH_REGEXP_PATTERN}';
+
+    export const SIDEBAR_URL = {
+      max_icon_length: #{SidebarUrl::MAX_ICON_LENGTH},
+      max_name_length: #{SidebarUrl::MAX_NAME_LENGTH},
+      max_value_length: #{SidebarUrl::MAX_VALUE_LENGTH}
+    }
+
+    export const SIDEBAR_SECTION = {
+      max_title_length: #{SidebarSection::MAX_TITLE_LENGTH},
+    }
   JS
 
   pretty_notifications = Notification.types.map { |n| "  #{n[0]}: #{n[1]}," }.join("\n")

spec/system/custom_sidebar_sections_spec.rb

@@ -189,4 +189,24 @@ describe "Custom sidebar sections", type: :system, js: true do
 
     expect(page).not_to have_button("Edited public section")
   end
+
+  it "validates custom section fields" do
+    visit("/latest")
+    sidebar.open_new_custom_section
+
+    section_modal.fill_name("A" * (SidebarSection::MAX_TITLE_LENGTH + 1))
+    section_modal.fill_link("B" * (SidebarUrl::MAX_NAME_LENGTH + 1), "/wrong-url")
+
+    expect(page.find(".title.warning")).to have_content("Title must be shorter than 30 characters")
+    expect(page.find(".name.warning")).to have_content("Name must be shorter than 80 characters")
+    expect(page.find(".value.warning")).to have_content("Format is invalid")
+
+    section_modal.fill_name("")
+    section_modal.fill_link("", "")
+    expect(page.find(".title.warning")).to have_content("Title cannot be blank")
+    expect(page.find(".name.warning")).to have_content("Name cannot be blank")
+    expect(page.find(".value.warning")).to have_content("Link cannot be blank")
+
+    expect(section_modal).to have_disabled_save
+  end
 end