From 437ef1721413a5a4f5cfd3d12fc7953fa5d41371 Mon Sep 17 00:00:00 2001 From: Lhc_fl Date: Wed, 14 Aug 2024 21:15:32 +0800 Subject: [PATCH] UX: Rewrite param-input using FormKit What does this PR do? ===================== This PR refactors param-input to use FormKit. FormKit is a structured form tool in the core. After the rewrite, we will be able to gradually abandon custom validators, get semantic parameter error prompts, etc. meta link: https://meta.discourse.org/t/wishlist-param-dropdown-for-data-explorer-query/253883/28?u=lhc_fl --- .../query_serializer.rb | 2 +- .../discourse/components/param-input.hbs | 138 +++++------ .../discourse/components/param-input.js | 229 +++++++++++------- .../components/param-inputs-wrapper.hbs | 19 +- .../controllers/admin-plugins-explorer.js | 11 + .../controllers/group-reports-show.js | 8 +- assets/javascripts/discourse/models/query.js | 12 +- .../templates/admin/plugins-explorer.hbs | 1 + .../templates/group-reports-show.hbs | 1 + assets/stylesheets/explorer.scss | 10 +- config/locales/client.en.yml | 6 +- .../components/param-input-test.js | 16 +- 12 files changed, 265 insertions(+), 188 deletions(-) diff --git a/app/serializers/discourse_data_explorer/query_serializer.rb b/app/serializers/discourse_data_explorer/query_serializer.rb index 01fb471c..2a87ad4c 100644 --- a/app/serializers/discourse_data_explorer/query_serializer.rb +++ b/app/serializers/discourse_data_explorer/query_serializer.rb @@ -15,7 +15,7 @@ class QuerySerializer < ActiveModel::Serializer :user_id def param_info - object&.params&.map(&:to_hash) + object&.params&.uniq { |p| p.identifier }&.map(&:to_hash) end def username diff --git a/assets/javascripts/discourse/components/param-input.hbs b/assets/javascripts/discourse/components/param-input.hbs index 5e9448fe..7df1f6c9 100644 --- a/assets/javascripts/discourse/components/param-input.hbs +++ b/assets/javascripts/discourse/components/param-input.hbs @@ -1,83 +1,63 @@
- {{#if (eq this.type "boolean")}} - {{#if @info.nullable}} - + + {{#if (eq this.type "boolean")}} + {{#if @info.nullable}} + + {{#each this.boolTypes as |t|}} + {{t.name}} + {{/each}} + + {{else}} + + {{/if}} + {{else if (eq this.type "int")}} + + {{else if (eq this.type "string")}} + + {{else if (eq this.type "user_id")}} + + + + {{else if (eq this.type "group_list")}} + + + + {{else if (eq this.type "user_list")}} + + + + {{else if (eq this.type "category_id")}} + + + {{else}} - + {{/if}} - {{@info.identifier}} - - {{else if (eq this.type "int")}} - - {{@info.identifier}} - - {{else if (eq this.type "string")}} - - {{@info.identifier}} - - {{else if (eq this.type "user_id")}} - - {{@info.identifier}} - - {{else if (eq this.type "group_list")}} - - {{@info.identifier}} - - {{else if (eq this.type "user_list")}} - - {{@info.identifier}} - - {{else if (eq this.type "category_id")}} - - {{@info.identifier}} - - {{else}} - - {{@info.identifier}} - {{/if}} +
\ No newline at end of file diff --git a/assets/javascripts/discourse/components/param-input.js b/assets/javascripts/discourse/components/param-input.js index a00f0b79..6ee6dae1 100644 --- a/assets/javascripts/discourse/components/param-input.js +++ b/assets/javascripts/discourse/components/param-input.js @@ -9,9 +9,9 @@ import I18n from "I18n"; const layoutMap = { int: "int", - bigint: "int", + bigint: "string", boolean: "boolean", - string: "generic", + string: "string", time: "generic", date: "generic", datetime: "generic", @@ -28,12 +28,24 @@ const layoutMap = { group_list: "group_list", }; +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"), + NO_SUCH_CATEGORY: I18n.t("explorer.form.errors.no_such_category"), + NO_SUCH_GROUP: I18n.t("explorer.form.errors.no_such_group"), +}; + export default class ParamInput extends Component { @service site; + /** + * @type {string | string[] | boolean | null} + */ @tracked value; - @tracked boolValue; - @tracked nullableBoolValue; boolTypes = [ { name: I18n.t("explorer.types.bool.true"), id: "Y" }, @@ -47,42 +59,72 @@ export default class ParamInput extends Component { const identifier = this.args.info.identifier; const initialValues = this.args.initialValues; - // access parsed params if present to update values to previously ran values - if (initialValues && identifier in initialValues) { - const initialValue = initialValues[identifier]; - if (this.type === "boolean") { - if (this.args.info.nullable) { - this.nullableBoolValue = initialValue; - this.args.updateParams( - this.args.info.identifier, - this.nullableBoolValue - ); - } else { - this.boolValue = initialValue !== "false"; - this.args.updateParams(this.args.info.identifier, this.boolValue); - } - } else { + // defet it to next tick to prevent infinite looping. + setTimeout(() => { + // access parsed params if present to update values to previously ran values + if (initialValues && identifier in initialValues) { + const initialValue = initialValues[identifier]; this.value = this.normalizeValue(initialValue); - this.args.updateParams(this.args.info.identifier, this.value); + this.args.updateParams(this.args.info.identifier, this.provideValue); + this.form.set(identifier, this.value); + } else { + // if no parsed params then get and set default values + const defaultValue = this.args.info.default; + this.value = this.normalizeValue(defaultValue); + if (this.value != null) { + this.args.updateParams(this.args.info.identifier, this.provideValue); + this.form.set(identifier, this.value); + } } - } else { - // if no parsed params then get and set default values - const defaultValue = this.args.info.default; - this.value = this.normalizeValue(defaultValue); - this.boolValue = defaultValue !== "false"; - this.nullableBoolValue = defaultValue; - } + }, 0); } + /** The value we will store in this.value */ normalizeValue(value) { switch (this.args.info.type) { case "category_id": return this.digitalizeCategoryId(value); + case "boolean": + if (value == null) { + return this.args.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; } } + /** The value we submitted when updatingParams */ + get provideValue() { + switch (this.type) { + case "string": + case "int": + return this.value != null ? String(this.value) : ""; + case "boolean": + return String(this.value); + case "group_list": + case "user_list": + return this.value.join(","); + default: + return this.value; + } + } + + get form() { + return this.args.form; + } + get type() { const type = this.args.info.type; if ((type === "time" || type === "date") && !allowsInputTypeTime()) { @@ -91,71 +133,96 @@ export default class ParamInput extends Component { return layoutMap[type] || "generic"; } - get valid() { + /** @returns {null | string} */ + getError() { const nullable = this.args.info.nullable; + if (isEmpty(this.value)) { + return nullable ? null : ERRORS.REQUIRED; + } + // intentionally use 'this.args' here instead of 'this.type' // to get the original key instead of the translated value from the layoutMap const type = this.args.info.type; - let value; - - if (type === "boolean") { - value = nullable ? this.nullableBoolValue : this.boolValue; - } else { - value = this.value; - } - - if (isEmpty(value)) { - return nullable; - } + const value = String(this.value); const intVal = parseInt(value, 10); - const intValid = - !isNaN(intVal) && intVal < 2147483648 && intVal > -2147483649; const isPositiveInt = /^\d+$/.test(value); switch (type) { case "int": - return /^-?\d+$/.test(value) && intValid; + if (intVal >= 2147483648) { + return ERRORS.OVERFLOW_HIGH; + } + if (intVal <= -2147483649) { + return ERRORS.OVERFLOW_LOW; + } + return null; case "bigint": - return /^-?\d+$/.test(value) && !isNaN(intVal); + if (isNaN(intVal)) { + return ERRORS.NOT_A_NUMBER; + } + return /^-?\d+$/.test(value) ? null : ERRORS.NOT_AN_INTEGER; case "boolean": - return /^Y|N|#null|true|false/.test(value); + return /^Y|N|#null|true|false/.test(value) ? null : ERRORS.INVALID; case "double": - return ( - !isNaN(parseFloat(value)) || - /^(-?)Inf(inity)?$/i.test(value) || - /^(-?)NaN$/i.test(value) - ); + if (isNaN(parseFloat(value))) { + if (/^(-?)Inf(inity)?$/i.test(value) || /^(-?)NaN$/i.test(value)) { + return null; + } + return ERRORS.NOT_A_NUMBER; + } + return null; case "int_list": - return value.split(",").every((i) => /^(-?\d+|null)$/.test(i.trim())); + return value.split(",").every((i) => /^(-?\d+|null)$/.test(i.trim())) + ? null + : ERRORS.INVALID; case "post_id": - return ( - isPositiveInt || + return isPositiveInt || /\d+\/\d+(\?u=.*)?$/.test(value) || /\/t\/[^/]+\/(\d+)(\?u=.*)?/.test(value) - ); + ? null + : ERRORS.INVALID; case "topic_id": - return isPositiveInt || /\/t\/[^/]+\/(\d+)/.test(value); + return isPositiveInt || /\/t\/[^/]+\/(\d+)/.test(value) + ? null + : ERRORS.INVALID; case "category_id": if (isPositiveInt) { - return !!this.site.categories.find((c) => c.id === intVal); + return this.site.categories.find((c) => c.id === intVal) + ? null + : ERRORS.NO_SUCH_CATEGORY; } else { - return false; + return ERRORS.REQUIRED; } case "group_id": const groups = this.site.get("groups"); if (isPositiveInt) { - return !!groups.find((g) => g.id === intVal); + return groups.find((g) => g.id === intVal) + ? null + : ERRORS.NO_SUCH_GROUP; } else { - return !!groups.find((g) => g.name === value); + return groups.find((g) => g.name === value) + ? null + : ERRORS.NO_SUCH_GROUP; } } - return true; + return null; + } + + get valid() { + return this.getError() == null; } get allGroups() { return this.site.get("groups"); } + get validation() { + if (this.type === "boolean") { + return ""; + } + return this.args.info.nullable ? "" : "required"; + } + digitalizeCategoryId(value) { value = String(value || ""); const isPositiveInt = /^\d+$/.test(value); @@ -178,37 +245,27 @@ export default class ParamInput extends Component { } @action - updateValue(input) { - // handle selectKit inputs as well as traditional inputs - const value = input.target ? input.target.value : input; - if (value.length) { - this.value = this.normalizeValue(value.toString()); - } else { - this.value = this.normalizeValue(value); + validate(name, value, { addError }) { + const message = this.getError(); + if (message != null) { + // skip require validation for we have used them in @validation + if (message === ERRORS.REQUIRED) { + return; + } + addError(name, { + title: this.args.info.identifier, + message, + }); } - - this.args.updateParams(this.args.info.identifier, this.value); } @action - updateBoolValue(input) { - this.boolValue = input.target.checked; - this.args.updateParams( - this.args.info.identifier, - this.boolValue.toString() - ); - } - - @action - updateNullableBoolValue(input) { - this.nullableBoolValue = input; - this.args.updateParams(this.args.info.identifier, this.nullableBoolValue); - } - - @action - updateGroupValue(input) { - this.value = input; - this.args.updateParams(this.args.info.identifier, this.value.join(",")); + updateValue(input) { + // handle selectKit inputs as well as traditional inputs + const value = input?.target ? input.target.value : input; + this.value = this.normalizeValue(value); + this.args.updateParams(this.args.info.identifier, this.provideValue); + this.form.set(this.args.info.identifier, value); } } diff --git a/assets/javascripts/discourse/components/param-inputs-wrapper.hbs b/assets/javascripts/discourse/components/param-inputs-wrapper.hbs index 64707c64..f84b3aab 100644 --- a/assets/javascripts/discourse/components/param-inputs-wrapper.hbs +++ b/assets/javascripts/discourse/components/param-inputs-wrapper.hbs @@ -1,12 +1,15 @@ {{#if @hasParams}}
- {{#each @paramInfo as |pinfo|}} - - {{/each}} +
+ {{#each @paramInfo as |pinfo|}} + + {{/each}} +
{{/if}} \ No newline at end of file diff --git a/assets/javascripts/discourse/controllers/admin-plugins-explorer.js b/assets/javascripts/discourse/controllers/admin-plugins-explorer.js index 5b6cf99c..1cbf48e9 100644 --- a/assets/javascripts/discourse/controllers/admin-plugins-explorer.js +++ b/assets/javascripts/discourse/controllers/admin-plugins-explorer.js @@ -37,6 +37,7 @@ export default class PluginsExplorerController extends Controller { explain = false; acceptedImportFileTypes = ["application/json"]; order = null; + form = null; get validQueryPresent() { return !!this.selectedItem.id; @@ -352,6 +353,11 @@ export default class PluginsExplorerController extends Controller { } } + @action + onRegisterApi(form) { + this.form = form; + } + @action updateParams(identifier, value) { this.selectedItem.set(`params.${identifier}`, value); @@ -379,6 +385,11 @@ export default class PluginsExplorerController extends Controller { @action run() { + // For now, this is just a form validation. + // defer the validation to load params + setTimeout(() => { + this.form?.submit(); + }, 10); this.setProperties({ loading: true, showResults: false, diff --git a/assets/javascripts/discourse/controllers/group-reports-show.js b/assets/javascripts/discourse/controllers/group-reports-show.js index 8949ed91..92660c30 100644 --- a/assets/javascripts/discourse/controllers/group-reports-show.js +++ b/assets/javascripts/discourse/controllers/group-reports-show.js @@ -23,7 +23,7 @@ export default class GroupReportsShowController extends Controller { @tracked queryGroupBookmark = this.queryGroup?.bookmark; queryParams = ["params"]; - + form = null; explain = false; get parsedParams() { @@ -55,6 +55,7 @@ export default class GroupReportsShowController extends Controller { @bind async run() { + this.form?.submit(); this.loading = true; this.showResults = false; @@ -129,4 +130,9 @@ export default class GroupReportsShowController extends Controller { updateParams(identifier, value) { this.set(`model.params.${identifier}`, value); } + + @action + onRegisterApi(form) { + this.form = form; + } } diff --git a/assets/javascripts/discourse/models/query.js b/assets/javascripts/discourse/models/query.js index 7d7407d4..716cd7e1 100644 --- a/assets/javascripts/discourse/models/query.js +++ b/assets/javascripts/discourse/models/query.js @@ -24,9 +24,17 @@ export default class Query extends RestModel { return getURL(`/admin/plugins/explorer/queries/${this.id}.json?export=1`); } - @computed("param_info") + @computed("param_info", "updateing") get hasParams() { - return this.param_info.length; + // When saving, we need to refresh the param-input component to clean up the old key + return this.param_info.length && !this.updateing; + } + + beforeUpdate() { + this.set("updateing", true); + } + afterUpdate() { + this.set("updateing", false); } resetParams() { diff --git a/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs b/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs index 61a0f419..07c03da4 100644 --- a/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs +++ b/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs @@ -239,6 +239,7 @@ @initialValues={{this.parsedParams}} @paramInfo={{this.selectedItem.param_info}} @updateParams={{this.updateParams}} + @onRegisterApi={{this.onRegisterApi}} /> {{#if this.runDisabled}} diff --git a/assets/javascripts/discourse/templates/group-reports-show.hbs b/assets/javascripts/discourse/templates/group-reports-show.hbs index 7a4ebb92..f6a12bc7 100644 --- a/assets/javascripts/discourse/templates/group-reports-show.hbs +++ b/assets/javascripts/discourse/templates/group-reports-show.hbs @@ -9,6 +9,7 @@ @initialValues={{this.parsedParams}} @paramInfo={{this.model.param_info}} @updateParams={{this.updateParams}} + @onRegisterApi={{this.onRegisterApi}} /> input, .param > .select-kit { margin: 9px; } - .invalid > input { + .invalid input { background-color: var(--danger-low); } .invalid .ac-wrap { @@ -270,9 +275,6 @@ table.group-reports { max-width: 250px; } } - .param-name { - margin-right: 1em; - } } .query-list, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 3e63947c..d5f3a43e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -89,6 +89,11 @@ en: reset_params: "Reset" search_placeholder: "Search..." no_search_results: "Sorry, we couldn't find any results matching your text." + form: + errors: + invalid: "Invalid" + no_such_category: "No such category" + no_such_group: "No such group" group: reports: "Reports" admin: @@ -109,4 +114,3 @@ en: label: Data Explorer Query parameters skip_empty: label: Skip sending PM if there are no results - diff --git a/test/javascripts/components/param-input-test.js b/test/javascripts/components/param-input-test.js index 72033883..bb1ac5dc 100644 --- a/test/javascripts/components/param-input-test.js +++ b/test/javascripts/components/param-input-test.js @@ -25,12 +25,16 @@ module("Data Explorer Plugin | Component | param-input", function (hooks) { updateParams, }); - await render(hbs``); + await render(hbs` +
+ + `); const categoryChooser = selectKit(".category-chooser");