From c172fe43ee4d4ecd97ffafc8dabed6da945cad34 Mon Sep 17 00:00:00 2001
From: Romeo Van Snick <romeo@romeovansnick.be>
Date: Mon, 14 Oct 2024 16:18:27 +0200
Subject: [PATCH] Do not save metric viz settings to metric (#48609)
* Add repro for #44171
* Do not save viz settings for metrics
* Lowercase description
Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>
* Rename variable to be more descriptive
* Use visitDashboard
* Fix typo in excludeVisualisationSettings
* Check the request does not contain visualization_settings too
* Allow visitQuestion to work for metrics
---------
Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>
---
e2e/support/helpers/api/createQuestion.ts | 4 +
.../metrics-reproductions.cy.spec.ts | 93 +++++++++++++++++++
.../query_builder/actions/core/core.js | 15 ++-
3 files changed, 108 insertions(+), 4 deletions(-)
diff --git a/e2e/support/helpers/api/createQuestion.ts b/e2e/support/helpers/api/createQuestion.ts
index f39936ce993..8775cd77a11 100644
--- a/e2e/support/helpers/api/createQuestion.ts
+++ b/e2e/support/helpers/api/createQuestion.ts
@@ -166,6 +166,10 @@ export const question = (
cy.intercept("POST", "/api/dataset").as("dataset");
cy.visit(`/model/${body.id}`);
cy.wait("@dataset"); // Wait for `result_metadata` to load
+ } else if (type === "metric") {
+ cy.intercept("POST", "/api/dataset").as("dataset");
+ cy.visit(`/metric/${body.id}`);
+ cy.wait("@dataset"); // Wait for `result_metadata` to load
} else {
// We need to use the wildcard because endpoint for pivot tables has the following format: `/api/card/pivot/${id}/query`
cy.intercept("POST", `/api/card/**/${body.id}/query`).as(
diff --git a/e2e/test/scenarios/metrics/reproductions/metrics-reproductions.cy.spec.ts b/e2e/test/scenarios/metrics/reproductions/metrics-reproductions.cy.spec.ts
index fdbb86f1f70..ecddcf3f366 100644
--- a/e2e/test/scenarios/metrics/reproductions/metrics-reproductions.cy.spec.ts
+++ b/e2e/test/scenarios/metrics/reproductions/metrics-reproductions.cy.spec.ts
@@ -1,9 +1,19 @@
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
import {
+ type StructuredQuestionDetails,
+ createDashboard,
createQuestion,
+ editDashboard,
+ getDashboardCard,
getNotebookStep,
main,
+ modal,
+ openQuestionActions,
+ popover,
restore,
+ showDashboardCardActions,
+ sidebar,
+ visitDashboard,
} from "e2e/support/helpers";
const { ORDERS_ID, ORDERS } = SAMPLE_DATABASE;
@@ -61,3 +71,86 @@ describe("issue 47058", () => {
});
});
});
+
+describe("issue 44171", () => {
+ const METRIC_A: StructuredQuestionDetails = {
+ name: "Metric 44171-A",
+ type: "metric",
+ display: "line",
+ query: {
+ "source-table": ORDERS_ID,
+ aggregation: [["count"]],
+ breakout: [
+ [
+ "field",
+ ORDERS.CREATED_AT,
+ { "temporal-unit": "month", "base-type": "type/DateTime" },
+ ],
+ ],
+ },
+ };
+
+ const METRIC_B: StructuredQuestionDetails = {
+ name: "Metric 44171-B",
+ type: "metric",
+ display: "line",
+ query: {
+ "source-table": ORDERS_ID,
+ aggregation: [["count"]],
+ breakout: [
+ [
+ "field",
+ ORDERS.CREATED_AT,
+ { "temporal-unit": "month", "base-type": "type/DateTime" },
+ ],
+ ],
+ },
+ };
+
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+
+ createQuestion(METRIC_A);
+ createQuestion(METRIC_B, { visitQuestion: true });
+ createDashboard(
+ {
+ name: "Dashboard 44171",
+ dashcards: [],
+ },
+ { wrapId: true },
+ );
+ });
+
+ it("should not save viz settings on metrics", () => {
+ cy.intercept("PUT", "/api/card/*").as("saveCard");
+
+ openQuestionActions();
+ popover().findByText("Edit metric definition").click();
+ getNotebookStep("summarize").button("Count").click();
+ popover().within(() => {
+ cy.findByText("Sum of ...").click();
+ cy.findByText("Total").click();
+ });
+ cy.button("Save changes").click();
+ cy.get<number>("@dashboardId").then(id => {
+ visitDashboard(id);
+ });
+
+ cy.get("@saveCard")
+ .its("request.body")
+ .its("visualization_settings")
+ .should("not.exist");
+
+ editDashboard();
+ cy.findByTestId("dashboard-header")
+ .findByLabelText("Add questions")
+ .click();
+
+ sidebar().findByText("Metric 44171-A").click();
+
+ showDashboardCardActions(0);
+ getDashboardCard(0).findByLabelText("Add series").click();
+ modal().findByText("Metric 44171-B").should("be.visible");
+ });
+});
diff --git a/frontend/src/metabase/query_builder/actions/core/core.js b/frontend/src/metabase/query_builder/actions/core/core.js
index a1e34508ed7..1fc2852f6c1 100644
--- a/frontend/src/metabase/query_builder/actions/core/core.js
+++ b/frontend/src/metabase/query_builder/actions/core/core.js
@@ -235,6 +235,7 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
const isResultDirty = getIsResultDirty(getState());
const isModel = question.type() === "model";
+ const isMetric = question.type() === "metric";
const { isNative } = Lib.queryDisplayInfo(question.query());
@@ -255,6 +256,7 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
question,
originalQuestion,
),
+ excludeVisualisationSettings: isMetric,
},
);
@@ -336,12 +338,17 @@ async function reduxCreateQuestion(question, dispatch) {
async function reduxUpdateQuestion(
question,
dispatch,
- { excludeDatasetQuery = false },
+ { excludeDatasetQuery = false, excludeVisualisationSettings = false },
) {
const fullCard = question.card();
- const card = excludeDatasetQuery
- ? _.omit(fullCard, "dataset_query")
- : fullCard;
+
+ const keysToOmit = [
+ excludeDatasetQuery ? "dataset_query" : null,
+ excludeVisualisationSettings ? "visualization_settings" : null,
+ ].filter(Boolean);
+
+ const card = _.omit(fullCard, ...keysToOmit);
+
const action = await dispatch(
Questions.actions.update({ id: question.id() }, card),
);
--
GitLab