diff --git a/e2e/support/helpers/api/createQuestion.ts b/e2e/support/helpers/api/createQuestion.ts index 8775cd77a11b43409d2de1679d4bffca73baadae..a7a6751890612fe9cd2ec3dbe58fe1e1a195a784 100644 --- a/e2e/support/helpers/api/createQuestion.ts +++ b/e2e/support/helpers/api/createQuestion.ts @@ -6,6 +6,8 @@ import type { StructuredQuery, } from "metabase-types/api"; +import { visitMetric, visitModel, visitQuestion } from "../e2e-misc-helpers"; + export type QuestionDetails = { dataset_query: DatasetQuery; /** @@ -123,7 +125,7 @@ export const question = ( }: QuestionDetails, { loadMetadata = false, - visitQuestion = false, + visitQuestion: shouldVisitQuestion = false, wrapId = false, idAlias = "questionId", interceptAlias = "cardQuery", @@ -161,21 +163,17 @@ export const question = ( }); } - if (loadMetadata || visitQuestion) { + if (loadMetadata || shouldVisitQuestion) { if (type === "model") { - cy.intercept("POST", "/api/dataset").as("dataset"); - cy.visit(`/model/${body.id}`); - cy.wait("@dataset"); // Wait for `result_metadata` to load + visitModel(body.id); } 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 + visitMetric(body.id); } 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( interceptAlias, ); - cy.visit(`/question/${body.id}`); + visitQuestion(body.id); cy.wait("@" + interceptAlias); // Wait for `result_metadata` to load } } diff --git a/e2e/test/scenarios/metrics/metrics-question.cy.spec.js b/e2e/test/scenarios/metrics/metrics-question.cy.spec.js index a86e8deea8136bd319a6a6ec22b79d23a8fffb3f..0ec2507ea38f660831a5cca13391da926a160bf4 100644 --- a/e2e/test/scenarios/metrics/metrics-question.cy.spec.js +++ b/e2e/test/scenarios/metrics/metrics-question.cy.spec.js @@ -15,6 +15,7 @@ import { popover, queryBuilderHeader, restore, + summarize, undoToast, visitMetric, } from "e2e/support/helpers"; @@ -66,9 +67,7 @@ describe("scenarios > metrics > question", () => { }); it("should be able to move a metric to a different collection", () => { - createQuestion(ORDERS_SCALAR_METRIC).then(({ body: card }) => - visitMetric(card.id), - ); + createQuestion(ORDERS_SCALAR_METRIC, { visitQuestion: true }); openQuestionActions(); popover().findByText("Move").click(); modal().within(() => { @@ -83,9 +82,7 @@ describe("scenarios > metrics > question", () => { }); it("should be able to add a filter with an ad-hoc question", () => { - createQuestion(ORDERS_SCALAR_METRIC).then(({ body: card }) => - visitMetric(card.id), - ); + createQuestion(ORDERS_SCALAR_METRIC, { visitQuestion: true }); cy.findByTestId("qb-header-action-panel").button("Filter").click(); modal().within(() => { cy.findByText("Product").click(); @@ -98,9 +95,7 @@ describe("scenarios > metrics > question", () => { }); it("should be able to add a custom aggregation expression based on a metric", () => { - createQuestion(ORDERS_TIMESERIES_METRIC).then(({ body: card }) => - visitMetric(card.id), - ); + createQuestion(ORDERS_TIMESERIES_METRIC, { visitQuestion: true }); cy.findByTestId("qb-header-action-panel").button("Summarize").click(); cy.findByTestId("sidebar-content") .button(ORDERS_TIMESERIES_METRIC.name) @@ -114,18 +109,14 @@ describe("scenarios > metrics > question", () => { }); it("should be able to add a breakout with an ad-hoc question", () => { - createQuestion(ORDERS_TIMESERIES_METRIC).then(({ body: card }) => - visitMetric(card.id), - ); + createQuestion(ORDERS_TIMESERIES_METRIC, { visitQuestion: true }); cy.findByTestId("qb-header-action-panel").button("Summarize").click(); cy.findByTestId("sidebar-content").findByText("Category").click(); echartsContainer().findByText("Product → Category").should("be.visible"); }); it("should be able to change the temporal unit when consuming a timeseries metric", () => { - createQuestion(ORDERS_TIMESERIES_METRIC).then(({ body: card }) => - visitMetric(card.id), - ); + createQuestion(ORDERS_TIMESERIES_METRIC, { visitQuestion: true }); assertQueryBuilderRowCount(49); cy.findByTestId("qb-header-action-panel").button("Summarize").click(); cy.findByTestId("sidebar-content") @@ -138,10 +129,7 @@ describe("scenarios > metrics > question", () => { }); it("should be able to drill-thru with a metric", () => { - createQuestion(ORDERS_TIMESERIES_METRIC).then(({ body: card }) => { - visitMetric(card.id); - cy.wait("@dataset"); - }); + createQuestion(ORDERS_TIMESERIES_METRIC, { visitQuestion: true }); cartesianChartCircle() .eq(23) // random dot .click({ force: true }); @@ -155,10 +143,7 @@ describe("scenarios > metrics > question", () => { }); it("should be able to drill-thru with a metric without the aggregation clause", () => { - createQuestion(ORDERS_TIMESERIES_METRIC).then(({ body: card }) => { - visitMetric(card.id); - cy.wait("@dataset"); - }); + createQuestion(ORDERS_TIMESERIES_METRIC, { visitQuestion: true }); cartesianChartCircle() .eq(23) // random dot .click({ force: true }); @@ -221,4 +206,15 @@ describe("scenarios > metrics > question", () => { cy.button("Summarize").should("not.exist"); }); }); + + it("should not show 'Replace existing question' option when saving an edited ad-hoc question from a metric (metabase#48555)", () => { + cy.signInAsNormalUser(); + createQuestion(ORDERS_SCALAR_METRIC, { visitQuestion: true }); + + summarize(); + cy.button("Done").click(); + + queryBuilderHeader().button("Save").click(); + modal().findByText("Replace or save as new?").should("not.exist"); + }); }); diff --git a/e2e/test/scenarios/models/reproductions.cy.spec.js b/e2e/test/scenarios/models/reproductions.cy.spec.js index 9a273ce3708dca6cb694019d57a6d74da862206c..d53852dc8b8bd4a582b39cdf761604e05ef091aa 100644 --- a/e2e/test/scenarios/models/reproductions.cy.spec.js +++ b/e2e/test/scenarios/models/reproductions.cy.spec.js @@ -1587,6 +1587,7 @@ describe("issue 32963", () => { it("should pick sensible display for model based questions (metabase#32963)", () => { cy.findByTestId("qb-header").button("Summarize").click(); + cy.intercept("POST", "/api/dataset").as("dataset"); rightSidebar().within(() => { cy.findAllByText("Created At").eq(0).click(); diff --git a/frontend/src/metabase/components/SaveQuestionForm/context.tsx b/frontend/src/metabase/components/SaveQuestionForm/context.tsx index 803e2bb898af84485158b813a8d3a6b0419c10d9..77d6ad0f0e05d2c9d587299cc13fc98c86a60fdc 100644 --- a/frontend/src/metabase/components/SaveQuestionForm/context.tsx +++ b/frontend/src/metabase/components/SaveQuestionForm/context.tsx @@ -75,14 +75,14 @@ export const SaveQuestionProvider = ({ // we care only about the very first result as question can be changed before // the modal is closed const [isSavedQuestionInitiallyChanged] = useState( - isNotNull(originalQuestion) && - originalQuestion.type() !== "model" && - question.isDirtyComparedTo(originalQuestion), + isNotNull(originalQuestion) && question.isDirtyComparedTo(originalQuestion), ); const showSaveType = isSavedQuestionInitiallyChanged && originalQuestion != null && + originalQuestion.type() !== "model" && + originalQuestion.type() !== "metric" && originalQuestion.canWrite(); return (