Remove "Replace or save question" UI when saving and edited question from a metric (#48669)

* Remove 'Save as new question' toggle when saving questions from editing a metrics' ad-hoc question

* Allow visitQuestion to work for metrics

* Use visitQuestion in metric test

* Add test for #48555 (showing the 'Save as new?' ui for metrics)

* Add issue info to repro

* Use visitMetric, visitModel and visitQuestion helpers in createQuestion

* Move check to make it more clear what the save ui does

* Use cy.button for Done button

* Remove unnecessary .within

* Sign in as normal user

* Add missing dataset alias

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
         }
       }

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");
+  });
 });

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();

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 (