From 8bc5b89fdfc47c78e2178ee9232915809cd59e36 Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Mon, 27 Feb 2023 16:31:28 +0200
Subject: [PATCH] Remove duplicate field entries for parameters in actions
(#28615)
---
.../src/metabase-types/api/mocks/actions.ts | 23 ++++++++
.../ActionCreator/FormCreator/FormCreator.tsx | 7 +--
.../ActionCreator/FormCreator/utils.ts | 15 +----
.../FormCreator/utils.unit.spec.ts | 46 +--------------
.../ActionCreator/tests/utils.unit.spec.ts | 50 ----------------
.../actions/containers/ActionCreator/utils.ts | 29 +++-------
.../ActionCreator/utils.unit.spec.ts | 58 +++++++++++++++++++
7 files changed, 95 insertions(+), 133 deletions(-)
create mode 100644 frontend/src/metabase/actions/containers/ActionCreator/utils.unit.spec.ts
diff --git a/frontend/src/metabase-types/api/mocks/actions.ts b/frontend/src/metabase-types/api/mocks/actions.ts
index 66d3996413c..efc366bb2aa 100644
--- a/frontend/src/metabase-types/api/mocks/actions.ts
+++ b/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,
+});
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.tsx b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.tsx
index 23d79cad9f0..7c9422d1b7e 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.tsx
+++ b/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]);
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.ts b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.ts
index 5d6305afab7..53ea4367820 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.ts
+++ b/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));
-};
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.unit.spec.ts b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.unit.spec.ts
index 2dd91ddbcfb..a76dfeb9fa3 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.unit.spec.ts
+++ b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/utils.unit.spec.ts
@@ -1,19 +1,7 @@
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);
- });
- });
});
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/tests/utils.unit.spec.ts b/frontend/src/metabase/actions/containers/ActionCreator/tests/utils.unit.spec.ts
index dc15d5e835c..e5c5e51a424 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/tests/utils.unit.spec.ts
+++ b/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({
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/utils.ts b/frontend/src/metabase/actions/containers/ActionCreator/utils.ts
index 14d9575737e..0a5262b32d7 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/utils.ts
+++ b/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,
};
};
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/utils.unit.spec.ts b/frontend/src/metabase/actions/containers/ActionCreator/utils.unit.spec.ts
new file mode 100644
index 00000000000..42cd193f9fe
--- /dev/null
+++ b/frontend/src/metabase/actions/containers/ActionCreator/utils.unit.spec.ts
@@ -0,0 +1,58 @@
+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" },
+ });
+ });
+});
--
GitLab