From 5080ce9b1f56f6566820cecaf763843e39fb861c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=94=A6=E5=BF=83?= <41134017+Lhcfl@users.noreply.github.com> Date: Tue, 20 Aug 2024 09:42:50 +0800 Subject: [PATCH] UX: Rewrite param-input using FormKit (#307) 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 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-form.gjs | 330 ++++++++++++++++++ .../discourse/components/param-input.hbs | 83 ----- .../discourse/components/param-input.js | 224 ------------ .../components/param-input/boolean-three.hbs | 11 + .../param-input/category-id-input.gjs | 28 ++ .../param-input/group-list-input.gjs | 23 ++ .../components/param-input/user-id-input.hbs | 8 + .../param-input/user-list-input.hbs | 7 + .../components/param-inputs-wrapper.hbs | 12 - .../controllers/admin-plugins-explorer.js | 26 +- .../controllers/group-reports-show.js | 26 +- assets/javascripts/discourse/models/query.js | 12 +- .../templates/admin/plugins-explorer.hbs | 14 +- .../templates/group-reports-show.hbs | 14 +- assets/stylesheets/explorer.scss | 15 +- config/locales/client.en.yml | 6 +- spec/system/param_input_spec.rb | 6 +- .../components/param-input-test.js | 186 ++++++++-- 19 files changed, 646 insertions(+), 387 deletions(-) create mode 100644 assets/javascripts/discourse/components/param-input-form.gjs delete mode 100644 assets/javascripts/discourse/components/param-input.hbs delete mode 100644 assets/javascripts/discourse/components/param-input.js create mode 100644 assets/javascripts/discourse/components/param-input/boolean-three.hbs create mode 100644 assets/javascripts/discourse/components/param-input/category-id-input.gjs create mode 100644 assets/javascripts/discourse/components/param-input/group-list-input.gjs create mode 100644 assets/javascripts/discourse/components/param-input/user-id-input.hbs create mode 100644 assets/javascripts/discourse/components/param-input/user-list-input.hbs delete mode 100644 assets/javascripts/discourse/components/param-inputs-wrapper.hbs diff --git a/app/serializers/discourse_data_explorer/query_serializer.rb b/app/serializers/discourse_data_explorer/query_serializer.rb index 01fb471..2a87ad4 100644 --- a/app/serializers/discourse_data_explorer/query_serializer.rb +++ b/app/serializers/discourse_data_explorer/query_serializer.rb @@ -15,7 +15,7 @@ module ::DiscourseDataExplorer :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-form.gjs b/assets/javascripts/discourse/components/param-input-form.gjs new file mode 100644 index 0000000..21da0cb --- /dev/null +++ b/assets/javascripts/discourse/components/param-input-form.gjs @@ -0,0 +1,330 @@ +import Component from "@glimmer/component"; +import { action } from "@ember/object"; +import { inject as service } from "@ember/service"; +import { dasherize } from "@ember/string"; +import { isEmpty } from "@ember/utils"; +import Form from "discourse/components/form"; +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 UserIdInput from "./param-input/user-id-input"; +import UserListInput from "./param-input/user-list-input"; + +export class ParamValidationError extends Error {} + +const layoutMap = { + int: "int", + bigint: "string", + boolean: "boolean", + string: "string", + time: "generic", + date: "generic", + datetime: "generic", + double: "string", + user_id: "user_id", + post_id: "string", + topic_id: "generic", + category_id: "category_id", + group_id: "generic", + badge_id: "generic", + int_list: "generic", + string_list: "generic", + user_list: "user_list", + 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"), +}; + +function digitalizeCategoryId(value) { + value = String(value || ""); + const isPositiveInt = /^\d+$/.test(value); + if (!isPositiveInt) { + if (/\//.test(value)) { + const match = /(.*)\/(.*)/.exec(value); + if (!match) { + value = null; + } else { + value = Category.findBySlug( + dasherize(match[2]), + dasherize(match[1]) + )?.id; + } + } else { + value = Category.findBySlug(dasherize(value))?.id; + } + } + return value?.toString(); +} + +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": + case "int": + return value != null ? String(value) : ""; + case "boolean": + return String(value); + case "group_list": + case "user_list": + return value?.join(","); + default: + return value?.toString(); + } +} + +function validationOf(info) { + switch (layoutMap[info.type]) { + case "boolean": + return info.nullable ? "required" : ""; + case "string": + case "string_list": + case "generic": + return info.nullable ? "" : "required:trim"; + default: + return info.nullable ? "" : "required"; + } +} + +function componentOf(info) { + let type = layoutMap[info.type] || "generic"; + if (info.nullable && type === "boolean") { + type = "boolean_three"; + } + switch (type) { + case "int": + return ; + case "boolean": + return ; + case "boolean_three": + return BooleanThree; + case "category_id": + // TODO + return CategoryIdInput; + case "user_id": + return UserIdInput; + case "user_list": + return UserListInput; + case "group_list": + return GroupListInput; + + case "bigint": + case "string": + default: + return ; + } +} + +export default class ParamInputForm extends Component { + @service site; + data = {}; + paramInfo = []; + infoOf = {}; + form = null; + + constructor() { + super(...arguments); + + const initialValues = this.args.initialValues; + for (const info of this.args.paramInfo) { + const identifier = info.identifier; + + // access parsed params if present to update values to previously ran values + let initialValue; + if (initialValues && identifier in initialValues) { + initialValue = initialValues[identifier]; + } else { + // if no parsed params then get and set default values + initialValue = info.default; + } + this.data[identifier] = normalizeValue(info, initialValue); + this.paramInfo.push({ + ...info, + validation: validationOf(info), + validate: this.validatorOf(info), + component: componentOf(info), + }); + this.infoOf[identifier] = info; + } + + this.args.onRegisterApi?.({ + submit: this.submit, + }); + } + + getErrorFn(info) { + const isPositiveInt = (value) => /^\d+$/.test(value); + const VALIDATORS = { + int: (value) => { + if (value >= 2147483648) { + return ERRORS.OVERFLOW_HIGH; + } + if (value <= -2147483649) { + return ERRORS.OVERFLOW_LOW; + } + return null; + }, + bigint: (value) => { + if (isNaN(parseInt(value, 10))) { + return ERRORS.NOT_A_NUMBER; + } + return /^-?\d+$/.test(value) ? null : ERRORS.NOT_AN_INTEGER; + }, + boolean: (value) => { + return /^Y|N|#null|true|false/.test(String(value)) + ? null + : ERRORS.INVALID; + }, + double: (value) => { + if (isNaN(parseFloat(value))) { + if (/^(-?)Inf(inity)?$/i.test(value) || /^(-?)NaN$/i.test(value)) { + return null; + } + return ERRORS.NOT_A_NUMBER; + } + return null; + }, + int_list: (value) => { + return value.split(",").every((i) => /^(-?\d+|null)$/.test(i.trim())) + ? null + : ERRORS.INVALID; + }, + post_id: (value) => { + return isPositiveInt(value) || + /\d+\/\d+(\?u=.*)?$/.test(value) || + /\/t\/[^/]+\/(\d+)(\?u=.*)?/.test(value) + ? null + : ERRORS.INVALID; + }, + topic_id: (value) => { + return isPositiveInt(value) || /\/t\/[^/]+\/(\d+)/.test(value) + ? null + : ERRORS.INVALID; + }, + category_id: (value) => { + return this.site.categoriesById.get(Number(value)) + ? null + : ERRORS.NO_SUCH_CATEGORY; + }, + 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 VALIDATORS[info.type] ?? (() => null); + } + + validatorOf(info) { + const getError = this.getErrorFn(info); + return (name, value, { addError }) => { + // skip require validation for we have used them in @validation + if (isEmpty(value) || value == null) { + return; + } + const message = getError(value); + if (message != null) { + addError(name, { title: info.identifier, message }); + } + }; + } + + @action + async submit() { + if (this.form == null) { + throw "No form"; + } + await this.form.submit(); + if (this.serializedData == null) { + throw new ParamValidationError("validation_failed"); + } else { + return this.serializedData; + } + } + + @action + onRegisterApi(form) { + this.form = form; + } + + @action + onSubmit(data) { + this.serializedData = null; + const serializedData = {}; + for (const [id, val] of Object.entries(data)) { + serializedData[id] = + serializeValue(this.infoOf[id].type, val) ?? undefined; + } + this.serializedData = serializedData; + } + + +} diff --git a/assets/javascripts/discourse/components/param-input.hbs b/assets/javascripts/discourse/components/param-input.hbs deleted file mode 100644 index 5e9448f..0000000 --- a/assets/javascripts/discourse/components/param-input.hbs +++ /dev/null @@ -1,83 +0,0 @@ -
- {{#if (eq this.type "boolean")}} - {{#if @info.nullable}} - - {{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 deleted file mode 100644 index a00f0b7..0000000 --- a/assets/javascripts/discourse/components/param-input.js +++ /dev/null @@ -1,224 +0,0 @@ -import Component from "@glimmer/component"; -import { tracked } from "@glimmer/tracking"; -import { action } from "@ember/object"; -import { inject as service } from "@ember/service"; -import { dasherize } from "@ember/string"; -import { isEmpty } from "@ember/utils"; -import Category from "discourse/models/category"; -import I18n from "I18n"; - -const layoutMap = { - int: "int", - bigint: "int", - boolean: "boolean", - string: "generic", - time: "generic", - date: "generic", - datetime: "generic", - double: "string", - user_id: "user_id", - post_id: "string", - topic_id: "generic", - category_id: "category_id", - group_id: "generic", - badge_id: "generic", - int_list: "generic", - string_list: "generic", - user_list: "user_list", - group_list: "group_list", -}; - -export default class ParamInput extends Component { - @service site; - - @tracked value; - @tracked boolValue; - @tracked nullableBoolValue; - - boolTypes = [ - { name: I18n.t("explorer.types.bool.true"), id: "Y" }, - { name: I18n.t("explorer.types.bool.false"), id: "N" }, - { name: I18n.t("explorer.types.bool.null_"), id: "#null" }, - ]; - - constructor() { - super(...arguments); - - 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 { - this.value = this.normalizeValue(initialValue); - this.args.updateParams(this.args.info.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; - } - } - - normalizeValue(value) { - switch (this.args.info.type) { - case "category_id": - return this.digitalizeCategoryId(value); - default: - return value; - } - } - - get type() { - const type = this.args.info.type; - if ((type === "time" || type === "date") && !allowsInputTypeTime()) { - return "string"; - } - return layoutMap[type] || "generic"; - } - - get valid() { - const nullable = this.args.info.nullable; - // 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 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; - case "bigint": - return /^-?\d+$/.test(value) && !isNaN(intVal); - case "boolean": - return /^Y|N|#null|true|false/.test(value); - case "double": - return ( - !isNaN(parseFloat(value)) || - /^(-?)Inf(inity)?$/i.test(value) || - /^(-?)NaN$/i.test(value) - ); - case "int_list": - return value.split(",").every((i) => /^(-?\d+|null)$/.test(i.trim())); - case "post_id": - return ( - isPositiveInt || - /\d+\/\d+(\?u=.*)?$/.test(value) || - /\/t\/[^/]+\/(\d+)(\?u=.*)?/.test(value) - ); - case "topic_id": - return isPositiveInt || /\/t\/[^/]+\/(\d+)/.test(value); - case "category_id": - if (isPositiveInt) { - return !!this.site.categories.find((c) => c.id === intVal); - } else { - return false; - } - case "group_id": - const groups = this.site.get("groups"); - if (isPositiveInt) { - return !!groups.find((g) => g.id === intVal); - } else { - return !!groups.find((g) => g.name === value); - } - } - return true; - } - - get allGroups() { - return this.site.get("groups"); - } - - digitalizeCategoryId(value) { - value = String(value || ""); - const isPositiveInt = /^\d+$/.test(value); - if (!isPositiveInt) { - if (/\//.test(value)) { - const match = /(.*)\/(.*)/.exec(value); - if (!match) { - value = null; - } else { - value = Category.findBySlug( - dasherize(match[2]), - dasherize(match[1]) - )?.id; - } - } else { - value = Category.findBySlug(dasherize(value))?.id; - } - } - return value?.toString(); - } - - @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); - } - - 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(",")); - } -} - -function allowsInputTypeTime() { - try { - const input = document.createElement("input"); - input.attributes.type = "time"; - input.attributes.type = "date"; - return true; - } catch (e) { - return false; - } -} diff --git a/assets/javascripts/discourse/components/param-input/boolean-three.hbs b/assets/javascripts/discourse/components/param-input/boolean-three.hbs new file mode 100644 index 0000000..b9d0c1f --- /dev/null +++ b/assets/javascripts/discourse/components/param-input/boolean-three.hbs @@ -0,0 +1,11 @@ +<@field.Select name={{@info.identifier}} as |select|> + + {{i18n "explorer.types.bool.true"}} + + + {{i18n "explorer.types.bool.false"}} + + + {{i18n "explorer.types.bool.null_"}} + + \ No newline at end of file diff --git a/assets/javascripts/discourse/components/param-input/category-id-input.gjs b/assets/javascripts/discourse/components/param-input/category-id-input.gjs new file mode 100644 index 0000000..2222eb4 --- /dev/null +++ b/assets/javascripts/discourse/components/param-input/category-id-input.gjs @@ -0,0 +1,28 @@ +import Component from "@glimmer/component"; +import { tracked } from "@glimmer/tracking"; +import { action } from "@ember/object"; +import CategoryChooser from "select-kit/components/category-chooser"; + +export default class GroupListInput extends Component { + @tracked value; + constructor() { + super(...arguments); + this.value = this.args.field.value; + } + + @action + update(id) { + this.value = id; + this.args.field.set(id); + } + + +} diff --git a/assets/javascripts/discourse/components/param-input/group-list-input.gjs b/assets/javascripts/discourse/components/param-input/group-list-input.gjs new file mode 100644 index 0000000..762d23a --- /dev/null +++ b/assets/javascripts/discourse/components/param-input/group-list-input.gjs @@ -0,0 +1,23 @@ +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 { + @service site; + + get allGroups() { + return this.site.get("groups"); + } + + +} diff --git a/assets/javascripts/discourse/components/param-input/user-id-input.hbs b/assets/javascripts/discourse/components/param-input/user-id-input.hbs new file mode 100644 index 0000000..c7e410c --- /dev/null +++ b/assets/javascripts/discourse/components/param-input/user-id-input.hbs @@ -0,0 +1,8 @@ +<@field.Custom id={{@field.id}}> + + \ No newline at end of file diff --git a/assets/javascripts/discourse/components/param-input/user-list-input.hbs b/assets/javascripts/discourse/components/param-input/user-list-input.hbs new file mode 100644 index 0000000..4ac5f92 --- /dev/null +++ b/assets/javascripts/discourse/components/param-input/user-list-input.hbs @@ -0,0 +1,7 @@ +<@field.Custom id={{@field.id}}> + + \ No newline at end of file diff --git a/assets/javascripts/discourse/components/param-inputs-wrapper.hbs b/assets/javascripts/discourse/components/param-inputs-wrapper.hbs deleted file mode 100644 index 64707c6..0000000 --- a/assets/javascripts/discourse/components/param-inputs-wrapper.hbs +++ /dev/null @@ -1,12 +0,0 @@ -{{#if @hasParams}} -
- {{#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 5b6cf99..8b14341 100644 --- a/assets/javascripts/discourse/controllers/admin-plugins-explorer.js +++ b/assets/javascripts/discourse/controllers/admin-plugins-explorer.js @@ -8,6 +8,7 @@ import { popupAjaxError } from "discourse/lib/ajax-error"; import { bind } from "discourse-common/utils/decorators"; import I18n from "I18n"; import QueryHelp from "discourse/plugins/discourse-data-explorer/discourse/components/modal/query-help"; +import { ParamValidationError } from "discourse/plugins/discourse-data-explorer/discourse/components/param-input-form"; import Query from "discourse/plugins/discourse-data-explorer/discourse/models/query"; const NoQuery = Query.create({ name: "No queries", fake: true, group_ids: [] }); @@ -37,6 +38,7 @@ export default class PluginsExplorerController extends Controller { explain = false; acceptedImportFileTypes = ["application/json"]; order = null; + form = null; get validQueryPresent() { return !!this.selectedItem.id; @@ -352,6 +354,11 @@ export default class PluginsExplorerController extends Controller { } } + @action + onRegisterApi(form) { + this.form = form; + } + @action updateParams(identifier, value) { this.selectedItem.set(`params.${identifier}`, value); @@ -378,17 +385,30 @@ export default class PluginsExplorerController extends Controller { } @action - run() { + async run() { + let params = null; + if (this.selectedItem.hasParams) { + try { + params = await this.form?.submit(); + } catch (err) { + if (err instanceof ParamValidationError) { + return; + } + } + if (params == null) { + return; + } + } this.setProperties({ loading: true, showResults: false, - params: JSON.stringify(this.selectedItem.params), + params: JSON.stringify(params), }); ajax("/admin/plugins/explorer/queries/" + this.selectedItem.id + "/run", { type: "POST", data: { - params: JSON.stringify(this.selectedItem.params), + params: JSON.stringify(params), explain: this.explain, }, }) diff --git a/assets/javascripts/discourse/controllers/group-reports-show.js b/assets/javascripts/discourse/controllers/group-reports-show.js index 8949ed9..5f222c7 100644 --- a/assets/javascripts/discourse/controllers/group-reports-show.js +++ b/assets/javascripts/discourse/controllers/group-reports-show.js @@ -11,6 +11,7 @@ import { WITH_REMINDER_ICON, } from "discourse/models/bookmark"; import { bind } from "discourse-common/utils/decorators"; +import { ParamValidationError } from "discourse/plugins/discourse-data-explorer/discourse/components/param-input-form"; export default class GroupReportsShowController extends Controller { @service currentUser; @@ -23,7 +24,7 @@ export default class GroupReportsShowController extends Controller { @tracked queryGroupBookmark = this.queryGroup?.bookmark; queryParams = ["params"]; - + form = null; explain = false; get parsedParams() { @@ -55,14 +56,20 @@ export default class GroupReportsShowController extends Controller { @bind async run() { - this.loading = true; - this.showResults = false; - try { - const stringifiedParams = JSON.stringify(this.model.params); + let params = null; + if (this.hasParams) { + params = await this.form.submit(); + if (params == null) { + return; + } + } + this.loading = true; + this.showResults = false; + const stringifiedParams = JSON.stringify(params); this.router.transitionTo({ queryParams: { - params: this.model.params ? stringifiedParams : null, + params: params ? stringifiedParams : null, }, }); const response = await ajax( @@ -84,7 +91,7 @@ export default class GroupReportsShowController extends Controller { } catch (error) { if (error.jqXHR?.status === 422 && error.jqXHR.responseJSON) { this.results = error.jqXHR.responseJSON; - } else { + } else if (error instanceof ParamValidationError) { popupAjaxError(error); } } finally { @@ -129,4 +136,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 7d7407d..716cd7e 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 61a0f41..d3f5fe2 100644 --- a/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs +++ b/assets/javascripts/discourse/templates/admin/plugins-explorer.hbs @@ -233,13 +233,13 @@
- + {{#if this.selectedItem.hasParams}} + + {{/if}} {{#if this.runDisabled}} {{#if this.saveDisabled}} diff --git a/assets/javascripts/discourse/templates/group-reports-show.hbs b/assets/javascripts/discourse/templates/group-reports-show.hbs index 7a4ebb9..8820544 100644 --- a/assets/javascripts/discourse/templates/group-reports-show.hbs +++ b/assets/javascripts/discourse/templates/group-reports-show.hbs @@ -3,13 +3,13 @@

{{this.model.description}}

- + {{#if this.hasParams}} + + {{/if}} input, .param > .select-kit { margin: 9px; } - .invalid > input { + .invalid input { background-color: var(--danger-low); } .invalid .ac-wrap { @@ -270,9 +274,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 3e63947..d5f3a43 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/spec/system/param_input_spec.rb b/spec/system/param_input_spec.rb index 8902aa6..5910230 100644 --- a/spec/system/param_input_spec.rb +++ b/spec/system/param_input_spec.rb @@ -64,11 +64,7 @@ RSpec.describe "Param input", type: :system, js: true do ::DiscourseDataExplorer::Parameter .create_from_sql(ALL_PARAMS_SQL) .each do |param| - if !param.nullable && param.type != :boolean && param.default.nil? - expect(page).to have_css(".query-params .param.invalid [name=\"#{param.identifier}\"]") - else - expect(page).to have_css(".query-params .param.valid [name=\"#{param.identifier}\"]") - end + expect(page).to have_css(".query-params .param [name=\"#{param.identifier}\"]") end end end diff --git a/test/javascripts/components/param-input-test.js b/test/javascripts/components/param-input-test.js index 7203388..2937708 100644 --- a/test/javascripts/components/param-input-test.js +++ b/test/javascripts/components/param-input-test.js @@ -1,42 +1,172 @@ -import { render } from "@ember/test-helpers"; +import { fillIn, render } from "@ember/test-helpers"; import hbs from "htmlbars-inline-precompile"; 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 values = {}; -function updateParams(identifier, value) { - values[identifier] = value; -} +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 = [ + { + type: "string", + default: "foo", + initial: "bar", + tests: [ + { input: "", data_null: "", error: ERRORS.REQUIRED }, + { input: " ", data_null: " ", error: ERRORS.REQUIRED }, + { input: "str", data: "str" }, + ], + }, + { + type: "int", + default: "123", + initial: "456", + tests: [ + { input: "", data_null: "", error: ERRORS.REQUIRED }, + { input: "1234", data: "1234" }, + { input: "0", data: "0" }, + { input: "-2147483648", data: "-2147483648" }, + { input: "2147483649", error: ERRORS.OVERFLOW_HIGH }, + { input: "-2147483649", error: ERRORS.OVERFLOW_LOW }, + ], + }, + { + type: "bigint", + default: "123", + initial: "456", + tests: [ + { input: "", data_null: undefined, error: ERRORS.REQUIRED }, + { input: "123", data: "123" }, + { input: "0", data: "0" }, + { input: "-2147483649", data: "-2147483649" }, + { input: "2147483649", data: "2147483649" }, + { input: "abcd", error: ERRORS.NOT_A_NUMBER }, + { input: "114.514", error: ERRORS.NOT_AN_INTEGER }, + ], + }, + { + type: "category_id", + default: "4", + initial: "3", + tests: [ + { + input: null, + data_null: undefined, + error: ERRORS.REQUIRED, + }, + { + input: async () => { + const categoryChooser = selectKit(".category-chooser"); + + await categoryChooser.expand(); + await categoryChooser.selectRowByValue(2); + }, + data: "2", + }, + ], + }, +]; module("Data Explorer Plugin | Component | param-input", function (hooks) { setupRenderingTest(hooks); - test("Renders the categroy_id type correctly", async function (assert) { - this.setProperties({ - info: { - identifier: "category_id", - type: "category_id", - default: null, - nullable: false, - }, - initialValues: {}, - params: {}, - updateParams, - }); + for (const testcase of InputTestCases) { + for (const config of [ + { default: testcase.default }, + { nullable: false, initial: testcase.initial }, + { nullable: false, default: testcase.default, initial: testcase.initial }, + { nullable: true }, + ]) { + const testName = ["type"]; + if (config.nullable) { + testName.push("nullable"); + } + testName.push(testcase.type); + if (config.initial) { + testName.push("with initial value"); + } + if (config.initial) { + testName.push("with default"); + } - await render(hbs``); + test(testName.join(" "), async function (assert) { + this.setProperties({ + param_info: [ + { + identifier: testcase.type, + type: testcase.type, + default: config.default ?? null, + nullable: config.nullable, + }, + ], + initialValues: config.initial + ? { [testcase.type]: config.initial } + : {}, + onRegisterApi: ({ submit }) => { + this.submit = submit; + }, + }); - const categoryChooser = selectKit(".category-chooser"); + await render(hbs` + `); - await categoryChooser.expand(); - await categoryChooser.selectRowByValue(2); + if (config.initial || config.default) { + const data = await this.submit(); + const val = config.initial || config.default; + assert.strictEqual( + data[testcase.type], + val, + `has initial/default value "${val}"` + ); + } - assert.strictEqual(values.category_id, "2"); - }); + for (const t of testcase.tests) { + if (t.input == null && (config.initial || config.default)) { + continue; + } + await formKit().reset(); + if (t.input != null) { + if (typeof t.input === "function") { + await t.input(); + } else { + await fillIn(`[name="${testcase.type}"]`, t.input); + } + } + + if (config.nullable && "data_null" in t) { + const data = await this.submit(); + assert.strictEqual( + data[testcase.type], + t.data_null, + `should have null data` + ); + } else if (t.error) { + await formKit().submit(); + assert.form().field(testcase.type).hasError(t.error); + } else { + const data = await this.submit(); + assert.strictEqual( + data[testcase.type], + t.data, + `data should be "${t.data}"` + ); + } + } + }); + } + } });