Remove duplicate field entries for parameters in actions (#28615)

frontend/src/metabase-types/api/mocks/actions.ts

@@ -4,6 +4,8 @@ import {
   WritebackParameter,
   WritebackQueryAction,
   WritebackImplicitQueryAction,
+  ActionFormSettings,
+  FieldSettings,
 } from "metabase-types/api";
 import { createMockNativeDatasetQuery } from "./query";
 import { createMockParameter } from "./parameters";
@@ -99,3 +101,24 @@ export const createMockPublicAction = (
   parameters: [],
   ...opts,
 });
+
+export const createMockActionFormSettings = (
+  opts?: Partial<ActionFormSettings>,
+): ActionFormSettings => ({ ...opts });
+
+export const createMockFieldSettings = (
+  opts?: Partial<FieldSettings>,
+): FieldSettings => ({
+  id: "",
+  name: "",
+  title: "",
+  description: "",
+  placeholder: "",
+  order: 0,
+  fieldType: "string",
+  inputType: "string",
+  required: true,
+  hidden: false,
+  width: "medium",
+  ...opts,
+});

frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.tsx

@@ -11,8 +11,7 @@ import MetabaseSettings from "metabase/lib/settings";
 import type { ActionFormSettings, Parameter } from "metabase-types/api";
 
 import { getDefaultFormSettings, sortActionParams } from "../../../utils";
-import { addMissingSettings } from "../utils";
-import { hasNewParams } from "./utils";
+import { syncFieldsWithParameters } from "../utils";
 
 import { EmptyFormPlaceholder } from "./EmptyFormPlaceholder";
 import { FormContainer, InfoText } from "./FormCreator.styled";
@@ -38,8 +37,8 @@ function FormCreator({
 
   useEffect(() => {
     // add default settings for new parameters
-    if (formSettings && params && hasNewParams(params, formSettings)) {
-      setFormSettings(addMissingSettings(formSettings, params));
+    if (formSettings && params) {
+      setFormSettings(syncFieldsWithParameters(formSettings, params));
     }
   }, [params, formSettings]);
 

frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.ts

@@ -3,12 +3,7 @@ import _ from "underscore";
 
 import { moveElement } from "metabase/core/utils/arrays";
 
-import type {
-  ActionFormSettings,
-  WritebackAction,
-  FieldSettingsMap,
-  Parameter,
-} from "metabase-types/api";
+import type { WritebackAction, FieldSettingsMap } from "metabase-types/api";
 
 export const getSubmitButtonColor = (action: WritebackAction): string => {
   if (action.type === "implicit" && action.kind === "row/delete") {
@@ -62,11 +57,3 @@ export const reorderFields = (
 
   return _.indexBy(fieldsWithUpdatedOrderProperty, "id");
 };
-
-export const hasNewParams = (
-  parameters: Parameter[],
-  formSettings: ActionFormSettings,
-) => {
-  const fieldIds = Object.keys(formSettings.fields || {});
-  return parameters.some(parameter => !fieldIds.includes(parameter.id));
-};

frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.unit.spec.ts

 import type { FieldSettingsMap } from "metabase-types/api";
 
-import {
-  getDefaultFieldSettings,
-  getDefaultFormSettings,
-} from "../../../utils";
-import { reorderFields, hasNewParams } from "./utils";
-
-const createParameter = (options?: any) => {
-  return {
-    id: "test_parameter",
-    name: "Test Parameter",
-    type: "type/Text",
-    ...options,
-  };
-};
+import { getDefaultFieldSettings } from "../../../utils";
+import { reorderFields } from "./utils";
 
 describe("actions > ActionCreator > FormCreator > utils", () => {
   describe("reorderFields", () => {
@@ -30,34 +18,4 @@ describe("actions > ActionCreator > FormCreator > utils", () => {
       expect(reorderedFields.c.order).toEqual(2);
     });
   });
-
-  describe("hasNewParams", () => {
-    const formSettings = getDefaultFormSettings({
-      fields: {
-        a: getDefaultFieldSettings({ order: 0 }),
-        b: getDefaultFieldSettings({ order: 1 }),
-        c: getDefaultFieldSettings({ order: 2 }),
-      },
-    });
-
-    it("should return true if there are new params", () => {
-      const params = [
-        createParameter({ id: "a" }),
-        createParameter({ id: "b" }),
-        createParameter({ id: "new" }),
-      ];
-
-      expect(hasNewParams(params, formSettings)).toBe(true);
-    });
-
-    it("should return false if there are no new params", () => {
-      const params = [
-        createParameter({ id: "a" }),
-        createParameter({ id: "b" }),
-        createParameter({ id: "c" }),
-      ];
-
-      expect(hasNewParams(params, formSettings)).toBe(false);
-    });
-  });
 });

frontend/src/metabase/actions/containers/ActionCreator/tests/utils.unit.spec.ts

@@ -9,7 +9,6 @@ import type { TemplateTagType } from "metabase-types/types/Query";
 import { getUnsavedNativeQuestion } from "metabase-lib/mocks";
 
 import {
-  removeOrphanSettings,
   setParameterTypesFromFieldSettings,
   setTemplateTagTypesFromFieldSettings,
 } from "../utils";
@@ -41,55 +40,6 @@ const createQuestionWithTemplateTags = (tagType: TemplateTagType) =>
   });
 
 describe("entities > actions > utils", () => {
-  describe("removeOrphanSettings", () => {
-    it("should remove orphan settings", () => {
-      const formSettings = getDefaultFormSettings({
-        name: "test form",
-        fields: {
-          aaa: getDefaultFieldSettings(),
-          bbb: getDefaultFieldSettings(),
-          ccc: getDefaultFieldSettings(),
-        },
-      });
-
-      const parameters = [
-        { id: "aaa", name: "foo" },
-        { id: "ccc", name: "bar" },
-      ] as Parameter[];
-
-      const result = removeOrphanSettings(formSettings, parameters);
-
-      expect(result.name).toEqual("test form");
-      expect(result.fields).toHaveProperty("aaa");
-      expect(result.fields).toHaveProperty("ccc");
-      expect(result.fields).not.toHaveProperty("bbb");
-    });
-
-    it("should leave non-orphan settings intact", () => {
-      const formSettings = getDefaultFormSettings({
-        name: "test form",
-        fields: {
-          aaa: getDefaultFieldSettings(),
-          bbb: getDefaultFieldSettings(),
-          ccc: getDefaultFieldSettings(),
-        },
-      });
-
-      const parameters = [
-        { id: "aaa", name: "foo" },
-        { id: "bbb", name: "foo" },
-        { id: "ccc", name: "bar" },
-      ] as Parameter[];
-
-      const result = removeOrphanSettings(formSettings, parameters);
-
-      expect(result.name).toEqual("test form");
-      expect(result.fields).toHaveProperty("aaa");
-      expect(result.fields).toHaveProperty("bbb");
-      expect(result.fields).toHaveProperty("ccc");
-    });
-  });
-
   describe("setParameterTypesFromFieldSettings", () => {
     it("should set string parameter types", () => {
       const formSettings = getDefaultFormSettings({

frontend/src/metabase/actions/containers/ActionCreator/utils.ts

@@ -117,35 +117,25 @@ export const setParameterTypesFromFieldSettings = (
   });
 };
 
-export const removeOrphanSettings = (
-  formSettings: ActionFormSettings,
-  parameters: Parameter[],
-): ActionFormSettings => {
-  const parameterIds = parameters.map(parameter => parameter.id);
-  return {
-    ...formSettings,
-    fields: _.pick(formSettings.fields || {}, parameterIds),
-  };
-};
-
-export const addMissingSettings = (
+export const syncFieldsWithParameters = (
   settings: ActionFormSettings,
   parameters: Parameter[],
 ): ActionFormSettings => {
   const parameterIds = parameters.map(parameter => parameter.id);
   const fieldIds = Object.keys(settings.fields || {});
-  const missingIds = _.difference(parameterIds, fieldIds);
+  const addedIds = _.difference(parameterIds, fieldIds);
+  const removedIds = _.difference(fieldIds, parameterIds);
 
-  if (!missingIds.length) {
+  if (!addedIds.length && !removedIds.length) {
     return settings;
   }
 
   return {
     ...settings,
     fields: {
-      ...settings.fields,
+      ..._.omit(settings.fields, removedIds),
       ...Object.fromEntries(
-        missingIds.map(id => [id, getDefaultFieldSettings({ id })]),
+        addedIds.map(id => [id, getDefaultFieldSettings({ id })]),
       ),
     },
   };
@@ -163,10 +153,7 @@ export const convertQuestionToAction = (
     formSettings,
     cleanQuestion.parameters(),
   );
-  const visualization_settings = removeOrphanSettings(
-    addMissingSettings(formSettings, parameters),
-    parameters,
-  );
+
   return {
     id: question.id(),
     name: question.displayName() as string,
@@ -174,7 +161,7 @@ export const convertQuestionToAction = (
     dataset_query: question.datasetQuery() as NativeDatasetQuery,
     database_id: question.databaseId() as DatabaseId,
     parameters: parameters as WritebackParameter[],
-    visualization_settings,
+    visualization_settings: formSettings,
   };
 };
 

frontend/src/metabase/actions/containers/ActionCreator/utils.unit.spec.ts

+import {
+  createMockActionFormSettings,
+  createMockFieldSettings,
+  createMockParameter,
+} from "metabase-types/api/mocks";
+import { syncFieldsWithParameters } from "./utils";
+
+describe("syncFieldsWithParameters", () => {
+  it("should not modify settings if there are no changed parameters", () => {
+    const settings = createMockActionFormSettings({
+      fields: {
+        u1: createMockFieldSettings({ id: "u1" }),
+        u2: createMockFieldSettings({ id: "u2" }),
+      },
+    });
+    const parameters = [
+      createMockParameter({
+        id: "u1",
+      }),
+      createMockParameter({
+        id: "u2",
+      }),
+    ];
+
+    const newSettings = syncFieldsWithParameters(settings, parameters);
+
+    expect(newSettings).toBe(settings);
+  });
+
+  it("should add new fields and remove non-existing ones", () => {
+    const settings = createMockActionFormSettings({
+      fields: {
+        u1: createMockFieldSettings({ id: "u1" }),
+        u2: createMockFieldSettings({ id: "u2", fieldType: "number" }),
+      },
+    });
+    const parameters = [
+      createMockParameter({
+        id: "u2",
+      }),
+      createMockParameter({
+        id: "u3",
+      }),
+    ];
+
+    const newSettings = syncFieldsWithParameters(settings, parameters);
+
+    expect(newSettings).toMatchObject({
+      fields: {
+        u2: { id: "u2", fieldType: "number" },
+        u3: { id: "u3", fieldType: "string" },
+      },
+    });
+    expect(newSettings).not.toMatchObject({
+      u1: { id: "u1" },
+    });
+  });
+});