Stop native parameters defaults changing chart type after save or load (#33211)

e2e/support/helpers/e2e-misc-helpers.js

@@ -272,6 +272,16 @@ export function saveQuestion(
   });
 }
 
+export function saveSavedQuestion() {
+  cy.intercept("PUT", "/api/card/**").as("updateQuestion");
+  cy.findByText("Save").click();
+
+  modal().within(() => {
+    cy.button("Save").click();
+  });
+  cy.wait("@updateQuestion");
+}
+
 export function visitPublicQuestion(id) {
   cy.request("POST", `/api/card/${id}/public_link`).then(
     ({ body: { uuid } }) => {

e2e/test/scenarios/visualizations/reproductions/33208-auto-chart-type-after-save.cy.spec.js

+import {
+  restore,
+  runNativeQuery,
+  saveSavedQuestion,
+} from "e2e/support/helpers";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+
+const { PRODUCTS } = SAMPLE_DATABASE;
+
+describe("issue 33208", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+    cy.createNativeQuestion(
+      {
+        native: {
+          query:
+            "select distinct category from products where {{category}} order by category",
+          "template-tags": {
+            category: {
+              type: "dimension",
+              name: "category",
+              id: "82e3e985-5bd8-4503-a628-15201bad321b",
+              "display-name": "Category",
+              required: true,
+              default: ["Doohickey", "Gizmo"],
+              dimension: ["field", PRODUCTS.CATEGORY, null],
+              "widget-type": "string/=",
+            },
+          },
+        },
+        display: "scalar",
+      },
+      { visitQuestion: true },
+    );
+  });
+
+  it("should not auto-select chart type when opening a saved native question with parameters that have default values (metabase#33208)", () => {
+    // The default value for the category parameter is ["Doohickey","Gizmo"], which means the query results should have two rows, meaning
+    // scalar is not a sensible chart type. Normally the chart type would be automatically changed to table, but this shouldn't happen.
+    cy.findByTestId("scalar-value").should("be.visible");
+  });
+
+  it("should not auto-select chart type when saving a native question with parameters that have default values", () => {
+    cy.findByTestId("query-builder-main").findByText("Open Editor").click();
+    cy.get(".ace_editor").type(" ");
+    saveSavedQuestion("top category");
+    runNativeQuery({ wait: false });
+    cy.findByTestId("scalar-value").should("be.visible");
+  });
+});

frontend/src/metabase/dashboard/actions/data-fetching.js

@@ -238,14 +238,9 @@ export const fetchDashboard = createThunkAction(
 
       const parameterValuesById = preserveParameters
         ? getParameterValues(getState())
-        : getParameterValuesByIdFromQueryParams(
-            parameters,
-            queryParams,
-            metadata,
-            {
-              forcefullyUnsetDefaultedParametersWithEmptyStringValue: true,
-            },
-          );
+        : getParameterValuesByIdFromQueryParams(parameters, queryParams, {
+            forcefullyUnsetDefaultedParametersWithEmptyStringValue: true,
+          });
 
       entities = entities ?? normalize(result, dashboard).entities;
 

frontend/src/metabase/dashboard/actions/parameters.js

@@ -12,7 +12,6 @@ import {
 import { getParameterValuesByIdFromQueryParams } from "metabase/parameters/utils/parameter-values";
 import { SIDEBAR_NAME } from "metabase/dashboard/constants";
 
-import { getMetadata } from "metabase/selectors/metadata";
 import { isActionDashCard } from "metabase/actions/utils";
 import { updateDashboard } from "metabase/dashboard/actions/save";
 import {
@@ -330,11 +329,9 @@ export const setOrUnsetParameterValues =
 export const setParameterValuesFromQueryParams =
   queryParams => (dispatch, getState) => {
     const parameters = getParameters(getState());
-    const metadata = getMetadata(getState());
     const parameterValues = getParameterValuesByIdFromQueryParams(
       parameters,
       queryParams,
-      metadata,
       { forcefullyUnsetDefaultedParametersWithEmptyStringValue: true },
     );
 

frontend/src/metabase/parameters/utils/parameter-values.js

 import { getParameterType } from "metabase-lib/parameters/utils/parameter-type";
 import { hasParameterValue } from "metabase-lib/parameters/utils/parameter-values";
 
-export function getParameterValueFromQueryParams(
-  parameter,
-  queryParams,
-  metadata,
-) {
+export function getParameterValueFromQueryParams(parameter, queryParams) {
   queryParams = queryParams || {};
 
   const maybeParameterValue = queryParams[parameter.slug || parameter.id];
@@ -111,12 +107,11 @@ function removeUndefaultedEmptyStringParameters(pairs) {
 export function getParameterValuesByIdFromQueryParams(
   parameters,
   queryParams,
-  metadata,
   { forcefullyUnsetDefaultedParametersWithEmptyStringValue } = {},
 ) {
   const parameterValuePairs = parameters.map(parameter => [
     parameter,
-    getParameterValueFromQueryParams(parameter, queryParams, metadata),
+    getParameterValueFromQueryParams(parameter, queryParams),
   ]);
 
   const transformedPairs =

frontend/src/metabase/parameters/utils/parameter-values.unit.spec.js

@@ -8,7 +8,6 @@ describe("parameters/utils/parameter-values", () => {
   let field2;
   let field3;
   let field4;
-  let metadata;
   let parameter1;
   let parameter2;
   let parameter3;
@@ -45,26 +44,6 @@ describe("parameters/utils/parameter-values", () => {
       isBoolean: () => false,
     };
 
-    metadata = {
-      field(id) {
-        return this.fields[id];
-      },
-      fields: {
-        [field1.id]: field1,
-        [field2.id]: field2,
-        [field3.id]: field3,
-        [field4.id]: field4,
-      },
-      table(id) {
-        return this.tables[id];
-      },
-      tables: {
-        1: {
-          id: 1,
-        },
-      },
-    };
-
     // found in queryParams and not defaulted
     parameter1 = {
       id: 111,
@@ -100,44 +79,40 @@ describe("parameters/utils/parameter-values", () => {
 
   describe("getParameterValueFromQueryParams", () => {
     it("should return undefined when given an undefined queryParams arg", () => {
-      expect(
-        getParameterValueFromQueryParams(parameter1, undefined, metadata),
-      ).toBe(undefined);
+      expect(getParameterValueFromQueryParams(parameter1, undefined)).toBe(
+        undefined,
+      );
     });
 
     it("should return the parameter's default value when given an undefined queryParams arg", () => {
-      expect(
-        getParameterValueFromQueryParams(parameter2, undefined, metadata),
-      ).toBe("parameter2 default value");
+      expect(getParameterValueFromQueryParams(parameter2, undefined)).toBe(
+        "parameter2 default value",
+      );
     });
 
     it("should return the parameter's default value when the parameter value is not found in queryParams", () => {
-      expect(
-        getParameterValueFromQueryParams(parameter3, queryParams, metadata),
-      ).toBe("parameter3 default value");
+      expect(getParameterValueFromQueryParams(parameter3, queryParams)).toBe(
+        "parameter3 default value",
+      );
     });
 
     it("should return the parameter value found in the queryParams object", () => {
-      expect(
-        getParameterValueFromQueryParams(parameter1, queryParams, metadata),
-      ).toEqual(["parameter1 queryParam value"]);
+      expect(getParameterValueFromQueryParams(parameter1, queryParams)).toEqual(
+        ["parameter1 queryParam value"],
+      );
     });
 
     it("should ignore the parameter's default value when the parameter value is found in queryParams", () => {
-      expect(
-        getParameterValueFromQueryParams(parameter2, queryParams, metadata),
-      ).toEqual(["parameter2 queryParam value"]);
+      expect(getParameterValueFromQueryParams(parameter2, queryParams)).toEqual(
+        ["parameter2 queryParam value"],
+      );
     });
 
     it("should return an empty string as the value for a defaulted parameter because we handle that special case elsewhere", () => {
       expect(
-        getParameterValueFromQueryParams(
-          parameter2,
-          {
-            [parameter2.slug]: "",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter2, {
+          [parameter2.slug]: "",
+        }),
       ).toBe("");
     });
 
@@ -149,23 +124,15 @@ describe("parameters/utils/parameter-values", () => {
       field4.isDate = () => false;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "123.456",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "123.456",
+        }),
       ).toEqual([123.456]);
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "",
+        }),
       ).toBe("");
     });
 
@@ -177,13 +144,9 @@ describe("parameters/utils/parameter-values", () => {
       field4.isDate = () => false;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "123.456",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "123.456",
+        }),
       ).toEqual(["123.456"]);
     });
 
@@ -192,43 +155,27 @@ describe("parameters/utils/parameter-values", () => {
       field4.isBoolean = () => true;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "true",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "true",
+        }),
       ).toEqual([true]);
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "false",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "false",
+        }),
       ).toEqual([false]);
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "",
+        }),
       ).toBe("");
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "foo",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "foo",
+        }),
       ).toEqual(["foo"]);
     });
 
@@ -237,13 +184,9 @@ describe("parameters/utils/parameter-values", () => {
       parameter1.hasVariableTemplateTagTarget = false;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "123",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "123",
+        }),
       ).toEqual("123");
     });
 
@@ -252,13 +195,9 @@ describe("parameters/utils/parameter-values", () => {
       parameter1.hasVariableTemplateTagTarget = true;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "foo",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "foo",
+        }),
       ).toEqual(["foo"]);
     });
 
@@ -267,13 +206,9 @@ describe("parameters/utils/parameter-values", () => {
       parameter1.hasVariableTemplateTagTarget = false;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "",
+        }),
       ).toBe("");
     });
 
@@ -282,23 +217,15 @@ describe("parameters/utils/parameter-values", () => {
       parameter1.hasVariableTemplateTagTarget = false;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: "foo",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: "foo",
+        }),
       ).toEqual(["foo"]);
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter1,
-          {
-            [parameter1.slug]: ["foo", "bar"],
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter1, {
+          [parameter1.slug]: ["foo", "bar"],
+        }),
       ).toEqual(["foo", "bar"]);
     });
 
@@ -306,25 +233,17 @@ describe("parameters/utils/parameter-values", () => {
       field3.isBoolean = () => true;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter3,
-          {
-            [parameter3.slug]: "true",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter3, {
+          [parameter3.slug]: "true",
+        }),
       ).toEqual([true]);
     });
 
     it("should not try to parse parameters without fields", () => {
       expect(
-        getParameterValueFromQueryParams(
-          parameter4,
-          {
-            [parameter4.slug]: "true",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter4, {
+          [parameter4.slug]: "true",
+        }),
       ).toEqual(["true"]);
     });
 
@@ -333,16 +252,12 @@ describe("parameters/utils/parameter-values", () => {
       field2.isDate = () => false;
 
       expect(
-        getParameterValueFromQueryParams(
-          parameter2,
-          {
-            [parameter2.slug]: "parameter2 default value",
-          },
-          metadata,
-        ),
+        getParameterValueFromQueryParams(parameter2, {
+          [parameter2.slug]: "parameter2 default value",
+        }),
       ).toEqual([NaN]);
 
-      expect(getParameterValueFromQueryParams(parameter2, {}, metadata)).toBe(
+      expect(getParameterValueFromQueryParams(parameter2, {})).toBe(
         "parameter2 default value",
       );
     });
@@ -355,13 +270,9 @@ describe("parameters/utils/parameter-values", () => {
       };
 
       const runGetParameterValueFromQueryParams = value =>
-        getParameterValueFromQueryParams(
-          numberParameter,
-          {
-            [numberParameter.slug]: value,
-          },
-          metadata,
-        );
+        getParameterValueFromQueryParams(numberParameter, {
+          [numberParameter.slug]: value,
+        });
 
       it("should parse the parameter value as a float when it is a number parameter without fields", () => {
         expect(runGetParameterValueFromQueryParams("123.456")).toEqual([
@@ -401,11 +312,7 @@ describe("parameters/utils/parameter-values", () => {
     describe("`forcefullyUnsetDefaultedParametersWithEmptyStringValue` === false", () => {
       it("should generate a map of parameter values found in the queryParams or with default values", () => {
         expect(
-          getParameterValuesByIdFromQueryParams(
-            parameters,
-            queryParams,
-            metadata,
-          ),
+          getParameterValuesByIdFromQueryParams(parameters, queryParams),
         ).toEqual({
           [parameter1.id]: ["parameter1 queryParam value"],
           [parameter2.id]: ["parameter2 queryParam value"],
@@ -415,11 +322,7 @@ describe("parameters/utils/parameter-values", () => {
 
       it("should handle an undefined queryParams", () => {
         expect(
-          getParameterValuesByIdFromQueryParams(
-            parameters,
-            undefined,
-            metadata,
-          ),
+          getParameterValuesByIdFromQueryParams(parameters, undefined),
         ).toEqual({
           [parameter2.id]: "parameter2 default value",
           [parameter3.id]: "parameter3 default value",
@@ -437,7 +340,6 @@ describe("parameters/utils/parameter-values", () => {
           getParameterValuesByIdFromQueryParams(
             parameters,
             queryParamsWithSpecialCase,
-            metadata,
             { forcefullyUnsetDefaultedParametersWithEmptyStringValue: false },
           ),
         ).toEqual({
@@ -449,13 +351,11 @@ describe("parameters/utils/parameter-values", () => {
           getParameterValuesByIdFromQueryParams(
             parameters,
             queryParamsWithSpecialCase,
-            metadata,
           ),
         ).toEqual(
           getParameterValuesByIdFromQueryParams(
             parameters,
             queryParamsWithSpecialCase,
-            metadata,
             { forcefullyUnsetDefaultedParametersWithEmptyStringValue: false },
           ),
         );
@@ -475,7 +375,6 @@ describe("parameters/utils/parameter-values", () => {
               [parameter2.slug]: "parameter2 foo value",
               [parameter3.slug]: "false",
             },
-            metadata,
             { forcefullyUnsetDefaultedParametersWithEmptyStringValue: false },
           ),
         ).toEqual({
@@ -498,7 +397,6 @@ describe("parameters/utils/parameter-values", () => {
           getParameterValuesByIdFromQueryParams(
             parameters,
             queryParamsWithSpecialCase,
-            metadata,
             { forcefullyUnsetDefaultedParametersWithEmptyStringValue: true },
           ),
         ).toEqual({
@@ -521,7 +419,6 @@ describe("parameters/utils/parameter-values", () => {
             [parameter2.slug]: "",
             [parameter3.slug]: "false",
           },
-          metadata,
           { forcefullyUnsetDefaultedParametersWithEmptyStringValue: true },
         ),
       ).toEqual({

frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.jsx

@@ -92,7 +92,6 @@ class PublicQuestionInner extends Component {
       const parameterValuesById = getParameterValuesByIdFromQueryParams(
         parameters,
         query,
-        this.props.metadata,
       );
 
       this.setState(

frontend/src/metabase/query_builder/actions/core/parameterUtils.ts

@@ -90,11 +90,7 @@ export function getParameterValuesForQuestion({
   metadata: Metadata;
 }) {
   const parameters = getCardUiParameters(card, metadata);
-  return getParameterValuesByIdFromQueryParams(
-    parameters,
-    queryParams,
-    metadata,
-  );
+  return getParameterValuesByIdFromQueryParams(parameters, queryParams);
 }
 
 /**

frontend/src/metabase/query_builder/actions/querying.js

@@ -23,6 +23,7 @@ import {
   getQuestion,
   getTimeoutId,
   getIsResultDirty,
+  getOriginalQuestionWithParameterValues,
 } from "../selectors";
 
 import { updateUrl } from "./navigation";
@@ -176,7 +177,7 @@ export const queryCompleted = (question, queryResults) => {
   return async (dispatch, getState) => {
     const [{ data }] = queryResults;
     const [{ data: prevData }] = getQueryResults(getState()) || [{}];
-    const originalQuestion = getOriginalQuestion(getState());
+    const originalQuestion = getOriginalQuestionWithParameterValues(getState());
     const isDirty =
       question.query().isEditable() &&
       question.isDirtyComparedTo(originalQuestion);

frontend/src/metabase/query_builder/selectors.js

@@ -306,6 +306,12 @@ export const getOriginalQuestion = createSelector(
   (metadata, card) => metadata && card && new Question(card, metadata),
 );
 
+export const getOriginalQuestionWithParameterValues = createSelector(
+  [getMetadata, getOriginalCard, getParameterValues],
+  (metadata, card, parameterValues) =>
+    metadata && card && new Question(card, metadata, parameterValues),
+);
+
 export const getLastRunQuestion = createSelector(
   [getMetadata, getLastRunCard, getParameterValues],
   (metadata, card, parameterValues) =>