Rework parameter mapping comparing to MLv2 (#38545)

* Do not show Unknown field in parameter mappings if there is mapping

* Rework to MLv2

* handle virtual cards separately

* WIP

* fixup

* Rework the approach

* cleanup

* Fix unit test

* fixup! Fix unit test

* keep only one -1

* Add a test

* Split tests into groups

* Improve tests

* Move code to utils

* Make types more flexible

* Test

* Test

* Normalize target

* Add more tests

frontend/src/metabase-types/api/parameters.ts

@@ -74,10 +74,15 @@ export type ParameterTarget =
   | ParameterDimensionTarget
   | ParameterTextTarget;
 
-type DimensionTarget = LocalFieldReference;
-export type ParameterDimensionTarget = [
+export type ParameterDimensionTarget =
+  | NativeParameterDimensionTarget
+  | StructuredParameterDimensionTarget;
+
+export type NativeParameterDimensionTarget = ["dimension", VariableTarget];
+
+export type StructuredParameterDimensionTarget = [
   "dimension",
-  DimensionTarget | VariableTarget,
+  LocalFieldReference,
 ];
 
 export type ParameterValueOrArray = string | number | Array<any>;

frontend/src/metabase/dashboard/components/DashCard/DashCardParameterMapper/DashCardCardParameterMapper.jsx

@@ -29,7 +29,6 @@ import * as Lib from "metabase-lib";
 import { isVariableTarget } from "metabase-lib/parameters/utils/targets";
 import { isDateParameter } from "metabase-lib/parameters/utils/parameter-type";
 
-import { normalize } from "metabase-lib/queries/utils/normalize";
 import {
   getEditingParameter,
   getDashcardParameterMappingOptions,
@@ -38,6 +37,7 @@ import {
 } from "../../../selectors";
 import { setParameterMapping } from "../../../actions";
 
+import { getMappingOptionByTarget } from "../utils";
 import {
   Container,
   CardLabel,
@@ -98,9 +98,6 @@ export function DashCardCardParameterMapper({
 
   const hasSeries = dashcard.series && dashcard.series.length > 0;
   const isDisabled = mappingOptions.length === 0 || isActionDashCard(dashcard);
-  const selectedMappingOption = _.find(mappingOptions, option =>
-    _.isEqual(normalize(option.target), normalize(target)),
-  );
 
   const handleChangeTarget = useCallback(
     target => {
@@ -113,6 +110,13 @@ export function DashCardCardParameterMapper({
   const virtualCardType = getVirtualCardType(dashcard);
   const isNative = isNativeDashCard(dashcard);
 
+  const selectedMappingOption = getMappingOptionByTarget(
+    mappingOptions,
+    dashcard,
+    target,
+    question,
+  );
+
   const hasPermissionsToMap = useMemo(() => {
     if (isVirtual) {
       return true;

frontend/src/metabase/dashboard/components/DashCard/DashCardParameterMapper/DashCardCardParameterMapper.unit.spec.jsx

@@ -11,6 +11,9 @@ import {
   createMockStructuredDatasetQuery,
   createMockNativeDatasetQuery,
   createMockNativeQuery,
+  createMockLinkDashboardCard,
+  createMockVirtualCard,
+  createMockVirtualDashCard,
 } from "metabase-types/api/mocks";
 
 import { getMetadata } from "metabase/selectors/metadata";
@@ -63,77 +66,100 @@ describe("DashCardParameterMapper", () => {
     ).toBeInTheDocument();
   });
 
-  it("should render an informative error state for action cards", () => {
-    setup({
-      dashcard: createMockActionDashboardCard(),
+  describe("Action cards", () => {
+    it("should render an informative error state for action cards", () => {
+      setup({
+        dashcard: createMockActionDashboardCard(),
+      });
+      expect(getIcon("info")).toBeInTheDocument();
+      expect(
+        screen.getByLabelText(/action settings to connect variables/i),
+      ).toBeInTheDocument();
     });
-    expect(getIcon("info")).toBeInTheDocument();
-    expect(
-      screen.getByLabelText(/action settings to connect variables/i),
-    ).toBeInTheDocument();
   });
 
-  it("should render an informative error state for link cards", () => {
-    const linkCard = createMockCard({ dataset_query: {}, display: "link" });
-    setup({
-      card: linkCard,
-      dashcard: createMockDashboardCard({
-        visualization_settings: {
-          virtual_card: linkCard,
-        },
-      }),
+  describe("Virtual cards", () => {
+    it("should render an informative error state for link cards", () => {
+      const dashcard = createMockLinkDashboardCard();
+
+      setup({
+        card: dashcard.card,
+        dashcard,
+      });
+
+      expect(getIcon("info")).toBeInTheDocument();
+      expect(
+        screen.getByLabelText(/cannot connect variables to link cards/i),
+      ).toBeInTheDocument();
     });
-    expect(getIcon("info")).toBeInTheDocument();
-    expect(
-      screen.getByLabelText(/cannot connect variables to link cards/i),
-    ).toBeInTheDocument();
-  });
 
-  it("should render an informative parameter mapping state for text cards without variables", () => {
-    const textCard = createMockTextDashboardCard({ size_x: 3, size_y: 3 });
-    setup({
-      dashcard: textCard,
+    it("should render an informative parameter mapping state for text cards without variables", () => {
+      const textCard = createMockTextDashboardCard({ size_y: 3 });
+      setup({
+        dashcard: textCard,
+      });
+      expect(getIcon("info")).toBeInTheDocument();
+      expect(
+        screen.getByText(
+          "You can connect widgets to {{variables}} in text cards.",
+        ),
+      ).toBeInTheDocument();
     });
-    expect(getIcon("info")).toBeInTheDocument();
-    expect(
-      screen.getByText(
-        "You can connect widgets to {{variables}} in text cards.",
-      ),
-    ).toBeInTheDocument();
-  });
 
-  it("should render an informative parameter mapping state for heading cards without variables", () => {
-    const headingCard = createMockHeadingDashboardCard({
-      size_x: 3,
-      size_y: 3,
+    it("should render an informative parameter mapping state for heading cards without variables", () => {
+      const headingCard = createMockHeadingDashboardCard({
+        size_y: 3,
+      });
+      setup({
+        dashcard: headingCard,
+        card: headingCard.card,
+      });
+      expect(getIcon("info")).toBeInTheDocument();
+      expect(
+        screen.getByText(
+          "You can connect widgets to {{variables}} in heading cards.",
+        ),
+      ).toBeInTheDocument();
     });
-    setup({
-      dashcard: headingCard,
+
+    it("should render a different header for virtual cards", () => {
+      const textCard = createMockVirtualCard({ display: "text" });
+
+      setup({
+        card: textCard,
+        dashcard: createMockVirtualDashCard({
+          card: textCard,
+          size_y: 3,
+        }),
+        mappingOptions: ["foo", "bar"],
+      });
+
+      expect(screen.getByText(/Variable to map to/i)).toBeInTheDocument();
     });
-    expect(getIcon("info")).toBeInTheDocument();
-    expect(
-      screen.getByText(
-        "You can connect widgets to {{variables}} in heading cards.",
-      ),
-    ).toBeInTheDocument();
   });
 
-  it("should render a different header for virtual cards", () => {
-    const textCard = createMockCard({ dataset_query: {}, display: "text" });
-    setup({
-      card: textCard,
-      dashcard: createMockDashboardCard({
-        card: textCard,
-        size_y: 3,
-        visualization_settings: {
-          text: "{{foo}} {{bar}}",
-          virtual_card: textCard,
+  it("should render mapping for a non-native, non-virtual, non-action card", () => {
+    const card = createMockCard({
+      dataset_query: createMockStructuredDatasetQuery({
+        query: {
+          "source-table": 1,
         },
       }),
-      mappingOptions: ["foo", "bar"],
     });
 
-    expect(screen.getByText(/Variable to map to/i)).toBeInTheDocument();
+    setup({
+      card,
+      mappingOptions: [
+        {
+          target: ["dimension", ["field", 1]],
+          sectionName: "Section",
+          name: "Name",
+        },
+      ],
+      target: ["dimension", ["field", 1]],
+    });
+
+    expect(screen.getByText("Section.Name")).toBeInTheDocument();
   });
 
   it("should render an error state when a field is not present in the list of options", () => {
@@ -150,7 +176,7 @@ describe("DashCardParameterMapper", () => {
       dashcard: createMockDashboardCard({
         card,
       }),
-      mappingOptions: [["dimension", ["field", 1]]],
+      mappingOptions: [{ target: ["dimension", ["field", 1]] }],
       target: ["dimension", ["field", 2]],
       isMobile: true,
     });
@@ -189,56 +215,58 @@ describe("DashCardParameterMapper", () => {
     expect(screen.queryByText(/Column to filter on/i)).not.toBeInTheDocument();
   });
 
-  it("should show native question variable warning if a native question variable is used", () => {
-    const card = createMockCard({
-      dataset_query: createMockNativeDatasetQuery({
-        dataset_query: {
-          native: createMockNativeQuery({
-            query: "SELECT * FROM ACCOUNTS WHERE source = {{ source }}",
-            "template-tags": [createMockTemplateTag({ name: "source" })],
-          }),
-        },
-      }),
-    });
-    setup({
-      card,
-      dashcard: createMockDashboardCard({ card }),
-      target: ["variable", ["template-tag", "source"]],
+  describe("Native question", () => {
+    it("should show native question variable warning if a native question variable is used", () => {
+      const card = createMockCard({
+        dataset_query: createMockNativeDatasetQuery({
+          dataset_query: {
+            native: createMockNativeQuery({
+              query: "SELECT * FROM ACCOUNTS WHERE source = {{ source }}",
+              "template-tags": [createMockTemplateTag({ name: "source" })],
+            }),
+          },
+        }),
+      });
+      setup({
+        card,
+        dashcard: createMockDashboardCard({ card }),
+        target: ["variable", ["template-tag", "source"]],
+      });
+      expect(
+        screen.getByText(
+          /Native question variables only accept a single value\. They do not support dropdown lists/i,
+        ),
+      ).toBeInTheDocument();
     });
-    expect(
-      screen.getByText(
-        /Native question variables only accept a single value\. They do not support dropdown lists/i,
-      ),
-    ).toBeInTheDocument();
-  });
 
-  it("should show native question variable warning without single value explanation if parameter is date type", () => {
-    const card = createMockCard({
-      dataset_query: createMockNativeDatasetQuery({
-        dataset_query: {
-          native: createMockNativeQuery({
-            query: "SELECT * FROM ORDERS WHERE created_at = {{ created_at }}",
-            "template-tags": [
-              createMockTemplateTag({
-                name: "created_at",
-                type: "date/month-year",
-              }),
-            ],
-          }),
-        },
-      }),
-    });
-    setup({
-      card,
-      dashcard: createMockDashboardCard({ card }),
-      target: ["variable", ["template-tag", "created_at"]],
-      editingParameter: createMockParameter({ type: "date/month-year" }),
+    it("should show native question variable warning without single value explanation if parameter is date type", () => {
+      const card = createMockCard({
+        dataset_query: createMockNativeDatasetQuery({
+          dataset_query: {
+            native: createMockNativeQuery({
+              query: "SELECT * FROM ORDERS WHERE created_at = {{ created_at }}",
+              "template-tags": [
+                createMockTemplateTag({
+                  name: "created_at",
+                  type: "date/month-year",
+                }),
+              ],
+            }),
+          },
+        }),
+      });
+      setup({
+        card,
+        dashcard: createMockDashboardCard({ card }),
+        target: ["variable", ["template-tag", "created_at"]],
+        editingParameter: createMockParameter({ type: "date/month-year" }),
+      });
+      expect(
+        screen.getByText(
+          /Native question variables do not support dropdown lists/i,
+        ),
+      ).toBeInTheDocument();
     });
-    expect(
-      screen.getByText(
-        /Native question variables do not support dropdown lists/i,
-      ),
-    ).toBeInTheDocument();
   });
 
   describe("mobile", () => {
@@ -260,15 +288,13 @@ describe("DashCardParameterMapper", () => {
     });
 
     it("should hide header content when card is less than 3 units high", () => {
-      const textCard = createMockCard({ dataset_query: {}, display: "text" });
+      const textCard = createMockVirtualCard({ display: "text" });
+
       setup({
         card: textCard,
-        dashcard: createMockDashboardCard({
+        dashcard: createMockVirtualCard({
           card: textCard,
           size_y: 3,
-          visualization_settings: {
-            virtual_card: textCard,
-          },
         }),
         mappingOptions: ["foo", "bar"],
         isMobile: true,

frontend/src/metabase/dashboard/components/DashCard/utils.ts

-import type { BaseDashboardCard } from "metabase-types/api";
-import { getVirtualCardType } from "metabase/dashboard/utils";
+import _ from "underscore";
+import type {
+  BaseDashboardCard,
+  DashboardCard,
+  ParameterTarget,
+  QuestionDashboardCard,
+} from "metabase-types/api";
+import {
+  getVirtualCardType,
+  isActionDashCard,
+  isNativeDashCard,
+  isQuestionDashCard,
+  isVirtualDashCard,
+} from "metabase/dashboard/utils";
+import * as Lib from "metabase-lib";
+import { normalize } from "metabase-lib/queries/utils/normalize";
+import type Question from "metabase-lib/Question";
 
 const VIZ_WITH_CUSTOM_MAPPING_UI = ["placeholder", "link"];
 
@@ -16,3 +31,75 @@ export function shouldShowParameterMapper({
     !(display && VIZ_WITH_CUSTOM_MAPPING_UI.includes(display))
   );
 }
+
+// TODO: @uladzimirdev fix type definition in https://github.com/metabase/metabase/pull/38596
+export type MappingOption = {
+  name: string;
+  icon: string;
+  isForeign: boolean;
+  target: ParameterTarget;
+};
+
+export function getMappingOptionByTarget<T extends DashboardCard>(
+  mappingOptions: MappingOption[],
+  dashcard: T,
+  target: ParameterTarget,
+  question?: T extends QuestionDashboardCard ? Question : undefined,
+): MappingOption | undefined {
+  if (!target) {
+    return;
+  }
+
+  const isAction = isActionDashCard(dashcard);
+
+  // action has it's own settings, no need to get mapping options
+  if (isAction) {
+    return;
+  }
+
+  const isVirtual = isVirtualDashCard(dashcard);
+  const isNative = isQuestionDashCard(dashcard)
+    ? isNativeDashCard(dashcard)
+    : false;
+
+  if (isVirtual || isAction || isNative) {
+    const normalizedTarget = normalize(target);
+
+    return mappingOptions.find(mappingOption =>
+      _.isEqual(normalize(mappingOption.target), normalizedTarget),
+    );
+  }
+
+  if (!question) {
+    return;
+  }
+
+  const stageIndex = -1;
+  const columns = Lib.visibleColumns(question.query(), stageIndex);
+  const normalizedTarget = normalize(target[1]);
+
+  const [columnByTargetIndex] = Lib.findColumnIndexesFromLegacyRefs(
+    question.query(),
+    stageIndex,
+    columns,
+    [normalizedTarget],
+  );
+
+  // target not found - no need to look further
+  if (columnByTargetIndex === -1) {
+    return;
+  }
+
+  const mappingColumnIndexes = Lib.findColumnIndexesFromLegacyRefs(
+    question.query(),
+    stageIndex,
+    columns,
+    mappingOptions.map(({ target }) => normalize(target[1])),
+  );
+
+  const mappingIndex = mappingColumnIndexes.indexOf(columnByTargetIndex);
+
+  if (mappingIndex >= 0) {
+    return mappingOptions[mappingIndex];
+  }
+}

frontend/src/metabase/dashboard/components/DashCard/utils.unit.spec.ts

+import {
+  createMockActionDashboardCard,
+  createMockCard,
+  createMockDashboardCard,
+  createMockHeadingDashboardCard,
+  createMockNativeDatasetQuery,
+  createMockNativeQuery,
+  createMockStructuredDatasetQuery,
+  createMockTemplateTag,
+} from "metabase-types/api/mocks";
+import type {
+  ParameterTextTarget,
+  ParameterVariableTarget,
+  QuestionDashboardCard,
+} from "metabase-types/api";
+import { createSampleDatabase } from "metabase-types/api/mocks/presets";
+import { createMockMetadata } from "__support__/metadata";
+import Question from "metabase-lib/Question";
+import type { MappingOption } from "./utils";
+import { getMappingOptionByTarget } from "./utils";
+
+describe("dashcard utils", () => {
+  describe("getMappingOptionByTarget", () => {
+    describe("virtual dashcard", () => {
+      it("should find mapping option", () => {
+        const headingCard = createMockHeadingDashboardCard();
+        const mappingOption: MappingOption = {
+          name: "param",
+          icon: "string",
+          isForeign: false,
+          target: ["text-tag", "param"],
+        };
+        const target: ParameterTextTarget = ["text-tag", "param"];
+
+        expect(
+          getMappingOptionByTarget([mappingOption], headingCard, target),
+        ).toBe(mappingOption);
+      });
+
+      it("should return undefined if option is not found", () => {
+        const headingCard = createMockHeadingDashboardCard();
+        const mappingOption: MappingOption = {
+          name: "param",
+          icon: "string",
+          isForeign: false,
+          target: ["text-tag", "param"],
+        };
+        const target: ParameterTextTarget = ["text-tag", "param2"];
+
+        expect(
+          getMappingOptionByTarget([mappingOption], headingCard, target),
+        ).toBe(undefined);
+      });
+    });
+
+    describe("action dashcard", () => {
+      it("should return nothing as action has it's own settings", () => {
+        const actionDashcard = createMockActionDashboardCard();
+        const mappingOptions: MappingOption[] = [
+          {
+            icon: "variable",
+            isForeign: false,
+            name: "Param1",
+            // @ts-expect-error @uladzimirdev https://github.com/metabase/metabase/pull/38596
+            id: "a8ab6fee-7974-4f51-833c-35177b446467",
+            type: "string/=",
+            target: ["variable", ["template-tag", "param1"]],
+            slug: "param1",
+            hasVariableTemplateTagTarget: true,
+          },
+        ];
+        const target: ParameterVariableTarget = [
+          "variable",
+          ["template-tag", "param1"],
+        ];
+
+        expect(
+          getMappingOptionByTarget(mappingOptions, actionDashcard, target),
+        ).toBe(undefined);
+      });
+    });
+    describe("native dashcard", () => {
+      it("should find mapping option", () => {
+        const card = createMockCard({
+          dataset_query: createMockNativeDatasetQuery({
+            native: createMockNativeQuery({
+              query: "SELECT * FROM ACCOUNTS WHERE source = {{ source }}",
+              "template-tags": {
+                source: createMockTemplateTag({ name: "source" }),
+              },
+            }),
+          }),
+        });
+        const dashcard = createMockDashboardCard({ card });
+
+        const mappingOption = {
+          name: "Source",
+          icon: "string",
+          isForeign: false,
+          target: ["variable", ["template-tag", "source"]],
+        };
+        const target = ["variable", ["template-tag", "source"]];
+
+        expect(
+          // @ts-expect-error @uladzimirdev https://github.com/metabase/metabase/pull/38596
+          getMappingOptionByTarget([mappingOption], dashcard, target),
+        ).toBe(mappingOption);
+      });
+      it("should return undefined if option is not found", () => {
+        const card = createMockCard({
+          dataset_query: createMockNativeDatasetQuery({
+            native: createMockNativeQuery({
+              query: "SELECT * FROM ACCOUNTS WHERE source = {{ source }}",
+              "template-tags": {
+                source: createMockTemplateTag({ name: "source" }),
+              },
+            }),
+          }),
+        });
+        const dashcard = createMockDashboardCard({ card });
+
+        const mappingOption = {
+          name: "Source",
+          icon: "string",
+          isForeign: false,
+          target: ["variable", ["template-tag", "source"]],
+        };
+        const target = ["variable", ["template-tag", "source1"]];
+
+        expect(
+          // @ts-expect-error @uladzimirdev https://github.com/metabase/metabase/pull/38596
+          getMappingOptionByTarget([mappingOption], dashcard, target),
+        ).toBe(undefined);
+      });
+    });
+    describe("structured dashcard", () => {
+      let question: Question;
+      let dashcard: QuestionDashboardCard;
+
+      beforeEach(() => {
+        const card = createMockCard({
+          dataset_query: createMockStructuredDatasetQuery({
+            query: {
+              "source-table": 2,
+            },
+          }),
+        });
+        const database = createSampleDatabase();
+        const metadata = createMockMetadata({
+          questions: [card],
+          databases: [database],
+        });
+
+        question = new Question(card, metadata);
+        dashcard = createMockDashboardCard({ card });
+      });
+      it("should find mapping option", () => {
+        const mappingOption = {
+          sectionName: "User",
+          name: "Name",
+          icon: "string",
+          target: [
+            "dimension",
+            [
+              "field",
+              1,
+              {
+                "base-type": "type/Text",
+              },
+            ],
+          ],
+          isForeign: true,
+        };
+
+        const target = [
+          "dimension",
+          [
+            "field",
+            1,
+            {
+              "base-type": "type/Text",
+            },
+          ],
+        ];
+
+        expect(
+          // @ts-expect-error @uladzimirdev https://github.com/metabase/metabase/pull/38596
+          getMappingOptionByTarget([mappingOption], dashcard, target, question),
+        ).toBe(mappingOption);
+      });
+      it("should return undefined if option is not found", () => {
+        const card = createMockCard({
+          dataset_query: createMockStructuredDatasetQuery({
+            query: {
+              "source-table": 2,
+            },
+          }),
+        });
+        const database = createSampleDatabase();
+        const metadata = createMockMetadata({
+          questions: [card],
+          databases: [database],
+        });
+
+        const question = new Question(card, metadata);
+        const dashcard = createMockDashboardCard({ card });
+
+        const mappingOption = {
+          sectionName: "User",
+          name: "Name",
+          icon: "string",
+          target: [
+            "dimension",
+            [
+              "field",
+              1,
+              {
+                "base-type": "type/Text",
+              },
+            ],
+          ],
+          isForeign: true,
+        };
+
+        const target = [
+          "dimension",
+          [
+            "field",
+            2,
+            {
+              "base-type": "type/Text",
+            },
+          ],
+        ];
+
+        expect(
+          // @ts-expect-error @uladzimirdev https://github.com/metabase/metabase/pull/38596
+          getMappingOptionByTarget([mappingOption], dashcard, target, question),
+        ).toBe(undefined);
+      });
+    });
+  });
+});