From 24bd4ba0994a81e1fb3326342400529b0883195f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=94=A6=E5=BF=83?= <41134017+Lhcfl@users.noreply.github.com> Date: Thu, 22 Aug 2024 17:46:12 +0800 Subject: [PATCH] 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. --- .../discourse/components/param-input-form.gjs | 141 ++++++++++++------ .../{group-list-input.gjs => group-input.gjs} | 12 +- spec/system/param_input_spec.rb | 2 + .../components/param-input-test.js | 70 +++++++-- 4 files changed, 166 insertions(+), 59 deletions(-) rename assets/javascripts/discourse/components/param-input/{group-list-input.gjs => group-input.gjs} (66%) diff --git a/assets/javascripts/discourse/components/param-input-form.gjs b/assets/javascripts/discourse/components/param-input-form.gjs index 926cc3c..fef4b90 100644 --- a/assets/javascripts/discourse/components/param-input-form.gjs +++ b/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) => - this.form.addError(pinfo.identifier, { - title: pinfo.identifier, - message: err.message, - }) - ) + .catch((err) => this.addError(pinfo.identifier, 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"); - 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 this.normalizeGroups(value).errorMsg; }, }; 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; } diff --git a/assets/javascripts/discourse/components/param-input/group-list-input.gjs b/assets/javascripts/discourse/components/param-input/group-input.gjs similarity index 66% rename from assets/javascripts/discourse/components/param-input/group-list-input.gjs rename to assets/javascripts/discourse/components/param-input/group-input.gjs index 762d23a..47fd9d0 100644 --- a/assets/javascripts/discourse/components/param-input/group-list-input.gjs +++ b/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, + } + : {}; + } + diff --git a/spec/system/param_input_spec.rb b/spec/system/param_input_spec.rb index 5910230..0284ea0 100644 --- a/spec/system/param_input_spec.rb +++ b/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 diff --git a/test/javascripts/components/param-input-test.js b/test/javascripts/components/param-input-test.js index b9e9e10..09f7b70 100644 --- a/test/javascripts/components/param-input-test.js +++ b/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"; - -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"), -}; +import { ERRORS } from "discourse/plugins/discourse-data-explorer/discourse/components/param-input-form"; 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` + `); + + assert + .form() + .field("group_id") + .hasError(`${ERRORS.NO_SUCH_GROUP}: invalid_group_name`); + }); });