UX: Use GroupChooser in `group_id` param input (#315)

This commit uses GroupChooser as the input for param input of type
group_id. Meanwhile, it improves invalid group validation and semantic
error prompts.

assets/javascripts/discourse/components/param-input-form.gjs

@@ -9,7 +9,7 @@ import Category from "discourse/models/category";
 import I18n from "I18n";
 import BooleanThree from "./param-input/boolean-three";
 import CategoryIdInput from "./param-input/category-id-input";
-import GroupListInput from "./param-input/group-list-input";
+import GroupInput from "./param-input/group-input";
 import UserIdInput from "./param-input/user-id-input";
 import UserListInput from "./param-input/user-list-input";
 
@@ -28,7 +28,7 @@ const layoutMap = {
   post_id: "string",
   topic_id: "generic",
   category_id: "category_id",
-  group_id: "generic",
+  group_id: "group_list",
   badge_id: "generic",
   int_list: "generic",
   string_list: "generic",
@@ -36,7 +36,7 @@ const layoutMap = {
   group_list: "group_list",
 };
 
-const ERRORS = {
+export const ERRORS = {
   REQUIRED: I18n.t("form_kit.errors.required"),
   NOT_AN_INTEGER: I18n.t("form_kit.errors.not_an_integer"),
   NOT_A_NUMBER: I18n.t("form_kit.errors.not_a_number"),
@@ -66,31 +66,6 @@ function digitalizeCategoryId(value) {
   return value;
 }
 
-function normalizeValue(info, value) {
-  switch (info.type) {
-    case "category_id":
-      return digitalizeCategoryId(value);
-    case "boolean":
-      if (value == null) {
-        return info.nullable ? "#null" : false;
-      }
-      return value;
-    case "group_list":
-    case "user_list":
-      if (Array.isArray(value)) {
-        return value || null;
-      }
-      return value?.split(",") || null;
-    case "user_id":
-      if (Array.isArray(value)) {
-        return value[0];
-      }
-      return value;
-    default:
-      return value;
-  }
-}
-
 function serializeValue(type, value) {
   switch (type) {
     case "string":
@@ -101,6 +76,8 @@ function serializeValue(type, value) {
     case "group_list":
     case "user_list":
       return value?.join(",");
+    case "group_id":
+      return value[0];
     default:
       return value?.toString();
   }
@@ -141,8 +118,7 @@ function componentOf(info) {
     case "user_list":
       return UserListInput;
     case "group_list":
-      return GroupListInput;
+      return GroupInput;
-
     case "bigint":
     case "string":
     default:
@@ -158,6 +134,9 @@ export default class ParamInputForm extends Component {
   form = null;
 
   promiseNormalizations = [];
+  formLoaded = new Promise((res) => {
+    this.__form_load_callback = res;
+  });
 
   constructor() {
     super(...arguments);
@@ -196,10 +175,53 @@ export default class ParamInputForm extends Component {
     });
   }
 
+  @action
+  async addError(identifier, message) {
+    await this.formLoaded;
+    this.form.addError(identifier, {
+      title: identifier,
+      message,
+    });
+  }
+
+  @action
+  normalizeValue(info, value) {
+    switch (info.type) {
+      case "category_id":
+        return digitalizeCategoryId(value);
+      case "boolean":
+        if (value == null) {
+          return info.nullable ? "#null" : false;
+        }
+        return value;
+      case "group_id":
+      case "group_list":
+        const normalized = this.normalizeGroups(value);
+        if (normalized.errorMsg) {
+          this.addError(info.identifier, normalized.errorMsg);
+        }
+        return info.type === "group_id"
+          ? normalized.value.slice(0, 1)
+          : normalized.value;
+      case "user_list":
+        if (Array.isArray(value)) {
+          return value || null;
+        }
+        return value?.split(",") || null;
+      case "user_id":
+        if (Array.isArray(value)) {
+          return value[0];
+        }
+        return value;
+      default:
+        return value;
+    }
+  }
+
   getNormalizedValue(info) {
     const initialValues = this.args.initialValues;
     const identifier = info.identifier;
-    return normalizeValue(
+    return this.normalizeValue(
       info,
       initialValues && identifier in initialValues
         ? initialValues[identifier]
@@ -214,15 +236,44 @@ export default class ParamInputForm extends Component {
 
     promise
       .then((res) => this.form.set(pinfo.identifier, res))
-      .catch((err) =>
+      .catch((err) => this.addError(pinfo.identifier, err.message))
-        this.form.addError(pinfo.identifier, {
-          title: pinfo.identifier,
-          message: err.message,
-        })
-      )
       .finally(() => pinfo.set("loading", false));
   }
 
+  @action
+  normalizeGroups(values) {
+    values ||= [];
+    if (typeof values === "string") {
+      values = values.split(",");
+    }
+
+    const GroupNames = new Set(this.site.get("groups").map((g) => g.name));
+    const GroupNameOf = Object.fromEntries(
+      this.site.get("groups").map((g) => [g.id, g.name])
+    );
+
+    const valid_groups = [];
+    const invalid_groups = [];
+
+    for (const val of values) {
+      if (GroupNames.has(val)) {
+        valid_groups.push(val);
+      } else if (GroupNameOf[Number(val)]) {
+        valid_groups.push(GroupNameOf[Number(val)]);
+      } else {
+        invalid_groups.push(String(val));
+      }
+    }
+
+    return {
+      value: valid_groups,
+      errorMsg:
+        invalid_groups.length !== 0
+          ? `${ERRORS.NO_SUCH_GROUP}: ${invalid_groups.join(", ")}`
+          : null,
+    };
+  }
+
   getErrorFn(info) {
     const isPositiveInt = (value) => /^\d+$/.test(value);
     const VALIDATORS = {
@@ -277,18 +328,11 @@ export default class ParamInputForm extends Component {
           ? null
           : ERRORS.NO_SUCH_CATEGORY;
       },
+      group_list: (value) => {
+        return this.normalizeGroups(value).errorMsg;
+      },
       group_id: (value) => {
-        const groups = this.site.get("groups");
+        return this.normalizeGroups(value).errorMsg;
-        if (isPositiveInt(value)) {
-          const intVal = parseInt(value, 10);
-          return groups.find((g) => g.id === intVal)
-            ? null
-            : ERRORS.NO_SUCH_GROUP;
-        } else {
-          return groups.find((g) => g.name === value)
-            ? null
-            : ERRORS.NO_SUCH_GROUP;
-        }
       },
     };
     return VALIDATORS[info.type] ?? (() => null);
@@ -324,6 +368,7 @@ export default class ParamInputForm extends Component {
 
   @action
   onRegisterApi(form) {
+    this.__form_load_callback();
     this.form = form;
   }
 

assets/javascripts/discourse/components/param-input/group-input.gjs

@@ -2,13 +2,21 @@ import Component from "@glimmer/component";
 import { inject as service } from "@ember/service";
 import GroupChooser from "select-kit/components/group-chooser";
 
-export default class GroupListInput extends Component {
+export default class GroupInput extends Component {
   @service site;
 
   get allGroups() {
     return this.site.get("groups");
   }
 
+  get groupChooserOption() {
+    return this.args.info.type === "group_id"
+      ? {
+          maximum: 1,
+        }
+      : {};
+  }
+
   <template>
     <@field.Custom id={{@field.id}}>
       <GroupChooser
@@ -17,6 +25,8 @@ export default class GroupListInput extends Component {
         @labelProperty="name"
         @valueProperty="name"
         @onChange={{@field.set}}
+        @options={{this.groupChooserOption}}
+        name={{@info.identifier}}
       />
     </@field.Custom>
   </template>

spec/system/param_input_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe "Param input", type: :system, js: true do
     -- string_list  :string_list
     -- category_id  :category_id
     -- group_id     :group_id
+    -- group_list   :group_list
     -- user_list    :mul_users
     -- int          :int_with_default = 3
     -- bigint       :bigint_with_default = 12345678912345
@@ -38,6 +39,7 @@ RSpec.describe "Param input", type: :system, js: true do
     -- string_list  :string_list_with_default = a,b,c
     -- category_id  :category_id_with_default = general
     -- group_id     :group_id_with_default = staff
+    -- group_list   :group_list_with_default = trust_level_0,trust_level_1
     -- user_list    :mul_users_with_default = system,discobot
     SELECT 1
   SQL

test/javascripts/components/param-input-test.js

@@ -4,16 +4,7 @@ import { module, test } from "qunit";
 import { setupRenderingTest } from "discourse/tests/helpers/component-test";
 import formKit from "discourse/tests/helpers/form-kit-helper";
 import selectKit from "discourse/tests/helpers/select-kit-helper";
-import I18n from "I18n";
+import { ERRORS } from "discourse/plugins/discourse-data-explorer/discourse/components/param-input-form";
-
-const ERRORS = {
-  REQUIRED: I18n.t("form_kit.errors.required"),
-  NOT_AN_INTEGER: I18n.t("form_kit.errors.not_an_integer"),
-  NOT_A_NUMBER: I18n.t("form_kit.errors.not_a_number"),
-  OVERFLOW_HIGH: I18n.t("form_kit.errors.too_high", { count: 2147484647 }),
-  OVERFLOW_LOW: I18n.t("form_kit.errors.too_low", { count: -2147484648 }),
-  INVALID: I18n.t("explorer.form.errors.invalid"),
-};
 
 const InputTestCases = [
   {
@@ -74,6 +65,38 @@ const InputTestCases = [
       },
     ],
   },
+  {
+    type: "group_id",
+    default: "trust_level_1",
+    initial: "trust_level_3",
+    tests: [
+      {
+        input: null,
+        data_null: undefined,
+        error: ERRORS.REQUIRED,
+      },
+      {
+        input: async () => {
+          const groupChooser = selectKit(".group-chooser");
+          await groupChooser.expand();
+          await groupChooser.selectRowByValue("trust_level_2");
+        },
+        data: "trust_level_2",
+      },
+    ],
+  },
+  {
+    type: "group_list",
+    default: "trust_level_1",
+    initial: "trust_level_3,trust_level_4",
+    tests: [
+      {
+        input: null,
+        data_null: "",
+        error: ERRORS.REQUIRED,
+      },
+    ],
+  },
 ];
 
 module("Data Explorer Plugin | Component | param-input", function (hooks) {
@@ -234,4 +257,31 @@ module("Data Explorer Plugin | Component | param-input", function (hooks) {
       assert.strictEqual(res.category_id, "1003");
     });
   });
+
+  test("show error message when default value is invalid", async function (assert) {
+    this.setProperties({
+      param_info: [
+        {
+          identifier: "group_id",
+          type: "group_id",
+          default: "invalid_group_name",
+          nullable: false,
+        },
+      ],
+      initialValues: {},
+      onRegisterApi: () => {},
+    });
+
+    await render(hbs`
+    <ParamInputForm
+      @initialValues={{this.initialValues}}
+      @paramInfo={{this.param_info}}
+      @onRegisterApi={{this.onRegisterApi}}
+    />`);
+
+    assert
+      .form()
+      .field("group_id")
+      .hasError(`${ERRORS.NO_SUCH_GROUP}: invalid_group_name`);
+  });
 });