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

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

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 <template>
+        <@field.Input @type="number" name={{@info.identifier}} />
+      </template>;
+    case "boolean":
+      return <template><@field.Checkbox name={{@info.identifier}} /></template>;
+    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 <template><@field.Input name={{@info.identifier}} /></template>;
+  }
+}
+
+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;
+  }
+
+  <template>
+    <div class="query-params">
+      <Form
+        @data={{this.data}}
+        @onRegisterApi={{this.onRegisterApi}}
+        @onSubmit={{this.onSubmit}}
+        class="params-form"
+        as |form|
+      >
+        {{#each this.paramInfo as |info|}}
+          <div class="param">
+            <form.Field
+              @name={{info.identifier}}
+              @title={{info.identifier}}
+              @validation={{info.validation}}
+              @validate={{info.validate}}
+              as |field|
+            >
+              <info.component @field={{field}} @info={{info}} />
+            </form.Field>
+          </div>
+        {{/each}}
+      </Form>
+    </div>
+  </template>
+}

assets/javascripts/discourse/components/param-input.hbs

@@ -1,83 +0,0 @@
-<div class="param {{if this.valid 'valid' 'invalid'}}">
-  {{#if (eq this.type "boolean")}}
-    {{#if @info.nullable}}
-      <ComboBox
-        @valueAttribute="id"
-        @value={{this.nullableBoolValue}}
-        @nameProperty="name"
-        @content={{this.boolTypes}}
-        @onChange={{this.updateNullableBoolValue}}
-        name={{@info.identifier}}
-      />
-    {{else}}
-      <Input
-        @type="checkbox"
-        @checked={{this.boolvalue}}
-        {{on "change" this.updateBoolValue}}
-        name={{@info.identifier}}
-      />
-    {{/if}}
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else if (eq this.type "int")}}
-    <Input
-      @type="number"
-      @value={{this.value}}
-      {{on "change" this.updateValue}}
-      name={{@info.identifier}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else if (eq this.type "string")}}
-    <TextField
-      @value={{this.value}}
-      @type="text"
-      @onChange={{this.updateValue}}
-      name={{@info.identifier}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else if (eq this.type "user_id")}}
-    <EmailGroupUserChooser
-      @value={{this.value}}
-      @options={{hash maximum=1}}
-      @onChange={{this.updateValue}}
-      name={{@info.identifier}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else if (eq this.type "group_list")}}
-    <GroupChooser
-      @content={{this.allGroups}}
-      @value={{this.value}}
-      @labelProperty="name"
-      @valueProperty="name"
-      @onChange={{this.updateGroupValue}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else if (eq this.type "user_list")}}
-    <EmailGroupUserChooser
-      @value={{this.value}}
-      @onChange={{this.updateValue}}
-      name={{@info.identifier}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else if (eq this.type "category_id")}}
-    <CategoryChooser
-      @value={{this.value}}
-      @onChange={{this.updateValue}}
-      name={{@info.identifier}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-
-  {{else}}
-    <TextField
-      @value={{this.value}}
-      @onChange={{this.updateValue}}
-      name={{@info.identifier}}
-    />
-    <span class="param-name">{{@info.identifier}}</span>
-  {{/if}}
-</div>

assets/javascripts/discourse/components/param-input.js

@@ -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;
-  }
-}

assets/javascripts/discourse/components/param-input/boolean-three.hbs

@@ -0,0 +1,11 @@
+<@field.Select name={{@info.identifier}} as |select|>
+  <select.Option @value="Y">
+    {{i18n "explorer.types.bool.true"}}
+  </select.Option>
+  <select.Option @value="N">
+    {{i18n "explorer.types.bool.false"}}
+  </select.Option>
+  <select.Option @value="#null">
+    {{i18n "explorer.types.bool.null_"}}
+  </select.Option>
+</@field.Select>

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);
+  }
+
+  <template>
+    <@field.Custom id={{@field.id}}>
+      <CategoryChooser
+        @value={{this.value}}
+        @onChange={{this.update}}
+        name={{@info.identifier}}
+      />
+    </@field.Custom>
+  </template>
+}

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");
+  }
+
+  <template>
+    <@field.Custom id={{@field.id}}>
+      <GroupChooser
+        @content={{this.allGroups}}
+        @value={{@field.value}}
+        @labelProperty="name"
+        @valueProperty="name"
+        @onChange={{@field.set}}
+      />
+    </@field.Custom>
+  </template>
+}

assets/javascripts/discourse/components/param-input/user-id-input.hbs

@@ -0,0 +1,8 @@
+<@field.Custom id={{@field.id}}>
+  <EmailGroupUserChooser
+    @value={{@field.value}}
+    @options={{hash maximum=1}}
+    @onChange={{@field.set}}
+    name={{@info.identifier}}
+  />
+</@field.Custom>

assets/javascripts/discourse/components/param-input/user-list-input.hbs

@@ -0,0 +1,7 @@
+<@field.Custom id={{@field.id}}>
+  <EmailGroupUserChooser
+    @value={{@field.value}}
+    @onChange={{@field.set}}
+    name={{@info.identifier}}
+  />
+</@field.Custom>

assets/javascripts/discourse/components/param-inputs-wrapper.hbs

@@ -1,12 +0,0 @@
-{{#if @hasParams}}
-  <div class="query-params">
-    {{#each @paramInfo as |pinfo|}}
-      <ParamInput
-        @params={{@params}}
-        @initialValues={{@initialValues}}
-        @info={{pinfo}}
-        @updateParams={{@updateParams}}
-      />
-    {{/each}}
-  </div>
-{{/if}}

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,
       },
     })

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;
+  }
 }

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() {

assets/javascripts/discourse/templates/admin/plugins-explorer.hbs

@@ -233,13 +233,13 @@
       </div>
 
       <form class="query-run" {{on "submit" this.run}}>
-        <ParamInputsWrapper
+        {{#if this.selectedItem.hasParams}}
-          @hasParams={{this.selectedItem.hasParams}}
+          <ParamInputForm
-          @params={{this.selectedItem.params}}
+            @initialValues={{this.parsedParams}}
-          @initialValues={{this.parsedParams}}
+            @paramInfo={{this.selectedItem.param_info}}
-          @paramInfo={{this.selectedItem.param_info}}
+            @onRegisterApi={{this.onRegisterApi}}
-          @updateParams={{this.updateParams}}
+          />
-        />
+        {{/if}}
 
         {{#if this.runDisabled}}
           {{#if this.saveDisabled}}

assets/javascripts/discourse/templates/group-reports-show.hbs

@@ -3,13 +3,13 @@
   <p>{{this.model.description}}</p>
 
   <form class="query-run" {{on "submit" this.run}}>
-    <ParamInputsWrapper
+    {{#if this.hasParams}}
-      @hasParams={{this.hasParams}}
+      <ParamInputForm
-      @params={{this.model.params}}
+        @initialValues={{this.parsedParams}}
-      @initialValues={{this.parsedParams}}
+        @paramInfo={{this.model.param_info}}
-      @paramInfo={{this.model.param_info}}
+        @onRegisterApi={{this.onRegisterApi}}
-      @updateParams={{this.updateParams}}
+      />
-    />
+    {{/if}}
 
     <DButton
       @action={{this.run}}

assets/stylesheets/explorer.scss

@@ -241,14 +241,18 @@ table.group-reports {
 
 .query-params {
   border: 1px solid var(--header_primary-medium);
-  display: flex;
+  .params-form {
-  align-items: center;
+    margin: 5px;
-  flex-wrap: wrap;
+    html.desktop-view & {
+      flex-direction: row;
+      flex-wrap: wrap;
+    }
+  }
   .param > 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,

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
-

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 [name=\"#{param.identifier}\"]")
-          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
       end
   end
 end

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 = {};
+const ERRORS = {
-function updateParams(identifier, value) {
+  REQUIRED: I18n.t("form_kit.errors.required"),
-  values[identifier] = value;
+  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) {
+  for (const testcase of InputTestCases) {
-    this.setProperties({
+    for (const config of [
-      info: {
+      { default: testcase.default },
-        identifier: "category_id",
+      { nullable: false, initial: testcase.initial },
-        type: "category_id",
+      { nullable: false, default: testcase.default, initial: testcase.initial },
-        default: null,
+      { nullable: true },
-        nullable: false,
+    ]) {
-      },
+      const testName = ["type"];
-      initialValues: {},
+      if (config.nullable) {
-      params: {},
+        testName.push("nullable");
-      updateParams,
+      }
-    });
+      testName.push(testcase.type);
+      if (config.initial) {
+        testName.push("with initial value");
+      }
+      if (config.initial) {
+        testName.push("with default");
+      }
 
-    await render(hbs`<ParamInput
+      test(testName.join(" "), async function (assert) {
-      @params={{this.params}}
+        this.setProperties({
-      @initialValues={{this.initialValues}}
+          param_info: [
-      @info={{this.info}}
+            {
-      @updateParams={{this.updateParams}}
+              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`
+        <ParamInputForm
+          @hasParams=true
+          @initialValues={{this.initialValues}}
+          @paramInfo={{this.param_info}}
+          @onRegisterApi={{this.onRegisterApi}}
+        />`);
 
-    await categoryChooser.expand();
+        if (config.initial || config.default) {
-    await categoryChooser.selectRowByValue(2);
+          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}"`
+            );
+          }
+        }
+      });
+    }
+  }
 });