Fix broken metric revision API call (#42634)

* Fix broken metric revision API call

Fixes #42633

* Add E2E repro for #42633

* Fix metrics revision history in the data reference

* Add repro for #42633 for the reference page

* Consolidate tests

* Do not use hard coded id

* Fix lint errors

e2e/test/scenarios/admin/datamodel/metrics.cy.spec.js

@@ -205,64 +205,75 @@ describe("scenarios > admin > datamodel > metrics", () => {
       cy.findByText("Preview");
     });
 
-    it("should update that metric", () => {
-      cy.visit("/admin");
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Table Metadata").click();
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Metrics").click();
+    it("should update that metric (metabase#42633)", () => {
+      cy.visit("/admin/datamodel/metrics");
 
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("orders < 100")
-        .parent()
-        .parent()
-        .parent()
-        .find(".Icon-ellipsis")
+      cy.get("td").filter(":contains(orders < 100)").should("be.visible");
+      cy.get("tbody tr").icon("ellipsis").click();
+
+      popover().contains("Edit Metric").click();
+
+      cy.log('Update the filter from "< 100" to "> 10"');
+      cy.location("pathname").should("eq", "/admin/datamodel/metric/1");
+      cy.get("label").contains("Edit Your Metric");
+      cy.findByTestId("gui-builder")
+        .findByText("Total is less than 100")
         .click();
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Edit Metric").click();
 
-      // update the filter from "< 100" to "> 10"
-      cy.url().should("match", /metric\/1$/);
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Edit Your Metric");
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains(/Total\s+is less than/).click();
-      popover().contains("Less than").click();
-      popover().contains("Greater than").click();
-      popover().find("input").type("{SelectAll}10");
-      popover().contains("Update filter").click();
+      cy.findByTestId("select-button").contains("Less than").click();
+      popover().last().findByText("Greater than").click();
+      popover().within(() => {
+        cy.findByDisplayValue("100").type("{backspace}");
+        cy.findByDisplayValue("10");
+        cy.button("Update filter").click();
+      });
 
-      // confirm that the preview updated
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Result: 18758");
+      cy.log("Confirm that the preview updated");
+      cy.findByTestId("gui-builder").should("contain", "Result: 18758");
 
-      // update name and description, set a revision note, and save the update
-      cy.get('[name="name"]').type("{selectall}orders > 10");
-      cy.get('[name="description"]').type(
-        "{selectall}Count of orders with a total over $10.",
+      cy.log(
+        "Update name and description, set a revision note, and save the update",
       );
+      cy.findByDisplayValue("orders < 100").clear().type("orders > 10");
+      cy.findByDisplayValue("Count of orders with a total under $100.")
+        .clear()
+        .type("Count of orders with a total over $10.");
+
       cy.get('[name="revision_message"]').type("time for a change");
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Save changes").click();
+      cy.button("Save changes").click();
 
       // get redirected to previous page and see the new metric name
-      cy.url().should("match", /datamodel\/metrics$/);
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("orders > 10");
+      cy.location("pathname").should("eq", "/admin/datamodel/metrics");
+      cy.get("td").filter(":contains(orders > 10)").should("be.visible");
+
+      cy.log("Make sure the revision history works (metabase#42633");
+      cy.get("tbody tr").icon("ellipsis").click();
+      popover().findByTextEnsureVisible("Revision History").click();
+      cy.location("pathname").should(
+        "eq",
+        "/admin/datamodel/metric/1/revisions",
+      );
+      cy.findByRole("heading").should(
+        "have.text",
+        'Revision History for "ORDERS"',
+      );
+      cy.findAllByRole("listitem").should("contain", "time for a change");
+      cy.go("back");
+      cy.location("pathname").should("eq", "/admin/datamodel/metrics");
+
+      cy.log("Clean up");
+      cy.get("tbody tr").icon("ellipsis").click();
+      popover().findByTextEnsureVisible("Retire Metric").click();
+      modal().within(() => {
+        cy.findByRole("heading").should("have.text", "Retire this metric?");
+        cy.get("textarea").type("delete it");
+        cy.button("Retire").click();
+      });
 
-      // clean up
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("orders > 10")
-        .parent()
-        .parent()
-        .parent()
-        .find(".Icon-ellipsis")
-        .click();
-      // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-      cy.contains("Retire Metric").click();
-      modal().find("textarea").type("delete it");
-      modal().contains("button", "Retire").click();
+      cy.get("main").should(
+        "contain",
+        "Create metrics to add them to the Summarize dropdown in the query builder",
+      );
     });
   });
 

e2e/test/scenarios/onboarding/reference/metrics.cy.spec.js

@@ -20,79 +20,74 @@ describe("scenarios > reference > metrics", () => {
       name: METRIC_NAME,
       description: METRIC_DESCRIPTION,
       table_id: ORDERS_ID,
+    }).then(({ body: { id } }) => {
+      cy.wrap(id).as("metricId");
     });
   });
 
-  it("should see the listing", () => {
-    cy.visit("/reference/metrics");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(METRIC_NAME);
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(METRIC_DESCRIPTION);
-  });
+  it("should let an admin edit details about the metric", () => {
+    cy.get("@metricId").then(id => {
+      cy.log("Should see the listing");
+      cy.visit("/reference/metrics");
+      cy.location("pathname").should("eq", "/reference/metrics");
 
-  it("should let the user navigate to details", () => {
-    cy.visit("/reference/metrics");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(METRIC_NAME).click();
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Why this metric is interesting");
-  });
+      cy.findByText(METRIC_DESCRIPTION);
 
-  it("should let an admin edit details about the metric", () => {
-    cy.visit("/reference/metrics");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(METRIC_NAME).click();
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Edit").click();
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Description")
-      .parent()
-      .parent()
-      .find("textarea")
-      .clear()
-      .type("Count of orders under $100");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Save").click();
-
-    modal().find("textarea").type("Renaming the description");
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Save changes").click();
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Count of orders under $100");
+      cy.log("Should let the user navigate to details");
+
+      cy.findByText(METRIC_NAME).click();
+      cy.location("pathname").should("eq", `/reference/metrics/${id}`);
+
+      cy.findByText("Edit").click();
+      cy.location("pathname").should("eq", `/reference/metrics/${id}/edit`);
+
+      cy.findByText("Description")
+        .parent()
+        .parent()
+        .find("textarea")
+        .clear()
+        .type("Count of orders under $100");
+
+      cy.findByText("Save").click();
+
+      modal().find("textarea").type("Renaming the description");
+
+      cy.findByText("Save changes").click();
+
+      cy.findByText("Count of orders under $100");
+
+      cy.log(
+        "Make sure this is reflected in the revision history (metabase#42633)",
+      );
+      cy.findAllByRole("listitem")
+        .filter(':contains("Revision history for orders < 100")')
+        .click();
+
+      cy.location("pathname").should(
+        "eq",
+        `/reference/metrics/${id}/revisions`,
+      );
+      cy.findAllByRole("listitem").should(
+        "contain",
+        "Renaming the description",
+      );
+    });
   });
 
   it("should let an admin start to edit and cancel without saving", () => {
-    cy.visit("/reference/metrics");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(METRIC_NAME).click();
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Edit").click();
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Why this metric is interesting")
-      .parent()
-      .parent()
-      .find("textarea")
-      .type("Because it's very nice");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Cancel").click();
-
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Because it's very nice").should("have.length", 0);
-  });
+    cy.get("@metricId").then(id => {
+      cy.visit(`/reference/metrics/${id}/edit`);
 
-  it("should have different URI while editing the metric", () => {
-    cy.visit("/reference/metrics");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText(METRIC_NAME).click();
+      cy.findByText("Why this metric is interesting")
+        .parent()
+        .parent()
+        .find("textarea")
+        .type("Because it's very nice");
 
-    cy.url().should("match", /\/reference\/metrics\/\d+$/);
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.findByText("Edit").click();
-    cy.url().should("match", /\/reference\/metrics\/\d+\/edit$/);
+      cy.findByText("Cancel").click();
+
+      cy.findByText("Because it's very nice").should("not.exist");
+      cy.location("pathname").should("eq", `/reference/metrics/${id}`);
+    });
   });
 });

frontend/src/metabase/admin/datamodel/containers/RevisionHistoryApp.jsx

@@ -21,7 +21,10 @@ const mapDispatchToProps = { fetchRevisions };
 class RevisionHistoryApp extends Component {
   componentDidMount() {
     const { id, objectType } = this.props;
-    this.props.fetchRevisions({ entity: objectType, id });
+    this.props.fetchRevisions({
+      entity: objectType === "metric" ? "legacy-metric" : objectType,
+      id,
+    });
   }
 
   render() {

frontend/src/metabase/redux/metadata.js

@@ -141,7 +141,10 @@ export const fetchRevisions = createThunkAction(
         return {
           type,
           id,
-          revisions: await RevisionsApi.get({ id, entity: type }),
+          revisions: await RevisionsApi.get({
+            id,
+            entity: type === "metric" ? "legacy-metric" : type,
+          }),
         };
       };