Fix x-ray dashcard drilldown (#19436)

frontend/src/metabase-lib/lib/Question.ts

@@ -228,6 +228,21 @@ export default class Question {
     );
   }
 
+  omitTransientCardIds() {
+    let question = this;
+
+    const card = question.card();
+    const { id, original_card_id } = card;
+    if (isTransientId(id)) {
+      question = question.setCard(_.omit(question.card(), "id"));
+    }
+    if (isTransientId(original_card_id)) {
+      question = question.setCard(_.omit(question.card(), "original_card_id"));
+    }
+
+    return question;
+  }
+
   /**
    * A question contains either a:
    * - StructuredQuery for queries written in MBQL
@@ -900,13 +915,15 @@ export default class Question {
     includeDisplayIsLocked?: boolean;
     creationType: string;
   } = {}): string {
+    const question = this.omitTransientCardIds();
+
     if (
-      !this.id() ||
-      (originalQuestion && this.isDirtyComparedTo(originalQuestion))
+      !question.id() ||
+      (originalQuestion && question.isDirtyComparedTo(originalQuestion))
     ) {
       return Urls.question(
         null,
-        this._serializeForUrl({
+        question._serializeForUrl({
           clean,
           includeDisplayIsLocked,
           creationType,
@@ -914,7 +931,7 @@ export default class Question {
         query,
       );
     } else {
-      return Urls.question(this.card(), "", query);
+      return Urls.question(question.card(), "", query);
     }
   }
 

frontend/test/metabase-lib/lib/Question.unit.spec.js

@@ -709,6 +709,9 @@ describe("Question", () => {
   });
 
   describe("URLs", () => {
+    const adhocUrl =
+      "/question#eyJkYXRhc2V0X3F1ZXJ5Ijp7ImRhdGFiYXNlIjoxLCJxdWVyeSI6eyJzb3VyY2UtdGFibGUiOjF9LCJ0eXBlIjoicXVlcnkifSwiZGlzcGxheSI6InRhYmxlIiwibmFtZSI6IlJhdyBvcmRlcnMgZGF0YSIsInZpc3VhbGl6YXRpb25fc2V0dGluZ3MiOnt9fQ==";
+
     // Covered a lot in query_builder/actions.spec.js, just very basic cases here
     // (currently getUrl has logic that is strongly tied to the logic query builder Redux actions)
     describe("getUrl(originalQuestion?)", () => {
@@ -721,11 +724,18 @@ describe("Question", () => {
       });
       it("returns a URL with hash for an unsaved question", () => {
         const question = new Question(dissoc(orders_raw_card, "id"), metadata);
-        expect(question.getUrl()).toBe(
-          "/question#eyJkYXRhc2V0X3F1ZXJ5Ijp7ImRhdGFiYXNlIjoxLCJxdWVyeSI6eyJzb3VyY2UtdGFibGUiOjF9LCJ0eXBlIjoicXVlcnkifSwiZGlzcGxheSI6InRhYmxlIiwibmFtZSI6IlJhdyBvcmRlcnMgZGF0YSIsInZpc3VhbGl6YXRpb25fc2V0dGluZ3MiOnt9fQ==",
-        );
+        expect(question.getUrl()).toBe(adhocUrl);
       });
     });
+
+    it("should avoid generating URLs with transient IDs", () => {
+      const question = new Question(
+        assoc(orders_raw_card, "id", "foo"),
+        metadata,
+      );
+
+      expect(question.getUrl()).toBe(adhocUrl);
+    });
   });
 
   describe("Question.prototype._syncNativeQuerySettings", () => {
@@ -1336,6 +1346,47 @@ describe("Question", () => {
       });
     });
   });
+
+  describe("Question.prototype.omitTransientCardIds", () => {
+    it("should return a question without a transient ids", () => {
+      const cardWithTransientId = {
+        ...card,
+        id: "foo",
+        original_card_id: 123,
+      };
+
+      const question = new Question(cardWithTransientId, metadata);
+      const newQuestion = question.omitTransientCardIds();
+      expect(newQuestion.id()).toBeUndefined();
+      expect(newQuestion.card().original_card_id).toBe(123);
+    });
+
+    it("should return a question without a transient original_card_id", () => {
+      const cardWithTransientId = {
+        ...card,
+        id: 123,
+        original_card_id: "bar",
+      };
+
+      const question = new Question(cardWithTransientId, metadata);
+      const newQuestion = question.omitTransientCardIds();
+      expect(newQuestion.card().original_card_id).toBeUndefined();
+      expect(newQuestion.id()).toBe(123);
+    });
+
+    it("should do nothing if id and original_card_id are both not transient", () => {
+      const cardWithoutTransientId = {
+        ...card,
+        id: 123,
+        original_card_id: undefined,
+      };
+
+      const question = new Question(cardWithoutTransientId, metadata);
+      const newQuestion = question.omitTransientCardIds();
+
+      expect(newQuestion).toBe(question);
+    });
+  });
 });
 
 function parseUrl(url) {

frontend/test/metabase/scenarios/dashboard/x-rays.cy.spec.js

-import { restore, visitQuestionAdhoc } from "__support__/e2e/cypress";
+import { restore, visitQuestionAdhoc, popover } from "__support__/e2e/cypress";
 import { SAMPLE_DATASET } from "__support__/e2e/cypress_sample_dataset";
 
 const {
@@ -153,4 +153,36 @@ describe("scenarios > x-rays", () => {
     cy.get(".Card").contains("18,760");
     cy.findByText("How these transactions are distributed");
   });
+
+  it("should be able to click the title of an x-ray dashcard to see it in the query builder", () => {
+    const timeout = { timeout: 10000 };
+
+    cy.visit("/");
+    cy.contains("A look at your Orders table").click();
+
+    // confirm results of "Total transactions" card are present
+    cy.findByText("18,760", timeout);
+    cy.findByText("Total transactions").click();
+
+    // confirm we're in the query builder with the same results
+    cy.url().should("contain", "/question");
+    cy.findByText("18,760");
+
+    cy.go("back");
+
+    // add a parameter filter to the auto dashboard
+    cy.findByText("State", timeout).click();
+    popover().within(() => {
+      cy.get("input").type("GA{enter}");
+      cy.findByText("Add filter").click();
+    });
+
+    // confirm results of "Total transactions" card were updated
+    cy.findByText("463", timeout);
+    cy.findByText("Total transactions").click();
+
+    // confirm parameter filter is applied as filter in query builder
+    cy.findByText("State is GA");
+    cy.findByText("463");
+  });
 });