Fix #18547 - Wrong id applied to the link to the question added to the...

Fix #18547 - Wrong id applied to the link to the question added to the dashboard [Activity log] (#18578)

* Fix wrong id applied when constructing a link to the question added to the dashboard

Closes #18547

* Unskip repro

* Add unit tests for question ids

* Guard against the dashboard questions with no names

frontend/src/metabase/lib/urls.js

@@ -28,24 +28,47 @@ export function question(card, hash = "", query = "") {
   if (hash && typeof hash === "object") {
     hash = serializeCardForUrl(hash);
   }
+
   if (query && typeof query === "object") {
     query = extractQueryParams(query)
       .map(kv => kv.map(encodeURIComponent).join("="))
       .join("&");
   }
+
   if (hash && hash.charAt(0) !== "#") {
     hash = "#" + hash;
   }
+
   if (query && query.charAt(0) !== "?") {
     query = "?" + query;
   }
+
   if (!card || !card.id) {
     return `/question${query}${hash}`;
   }
-  if (!card.name) {
-    return `/question/${card.id}${query}${hash}`;
+
+  const { card_id, id, name } = card;
+
+  /**
+   * If the question has been added to the dashboard we're reading the dashCard's properties.
+   * In that case `card_id` is the actual question's id, while `id` corresponds with the dashCard itself.
+   *
+   * There can be multiple instances of the same question in a dashboard, hence this distinction.
+   */
+  const questionId = card_id || id;
+
+  /**
+   * Although it's not possible to intentionally save a question without a name,
+   * it is possible that the `name` is not recognized if it contains symbols.
+   *
+   * Please see: https://github.com/metabase/metabase/pull/15989#pullrequestreview-656646149
+   */
+  if (!name) {
+    return `/question/${questionId}${query}${hash}`;
   }
-  const path = appendSlug(`/question/${card.id}`, slugg(card.name));
+
+  const path = appendSlug(`/question/${questionId}`, slugg(name));
+
   return `${path}${query}${hash}`;
 }
 

frontend/test/metabase/lib/urls.unit.spec.js

@@ -35,6 +35,37 @@ describe("urls", () => {
         );
       });
     });
+
+    describe("question ids", () => {
+      it("returns the correct url", () => {
+        expect(question(null)).toEqual("/question");
+        expect(question({ id: 1 })).toEqual("/question/1");
+
+        /**
+         * If we're dealing with the question in a dashboard, we're reading the dashCard properties.
+         * Make sure `card_id` gets assigned to the question url.
+         */
+
+        expect(question({ id: 1, card_id: 42, name: "Foo" })).toEqual(
+          "/question/42-foo",
+        );
+
+        /** Symbol-based languages are unsupported. Such names result in the `name` being dropped.
+         * Please see: https://github.com/metabase/metabase/pull/15989#pullrequestreview-656646149
+         *
+         * * We still have to make sure links to questions in a dashboard render correctly.
+         */
+
+        expect(question({ id: 1, card_id: 42 })).toEqual("/question/42");
+
+        expect(question({ id: 1, card_id: 42, name: "ベーコン" })).toEqual(
+          "/question/42",
+        );
+        expect(question({ id: 1, card_id: 42, name: "培根" })).toEqual(
+          "/question/42",
+        );
+      });
+    });
   });
 
   describe("query", () => {

frontend/test/metabase/scenarios/onboarding/home/activity-page.cy.spec.js

@@ -55,7 +55,7 @@ describe("metabase > scenarios > home > activity-page", () => {
     cy.findByText("Products, Filtered by Rating");
   });
 
-  it.skip("should respect the (added to dashboard) card id in the link (metabase#18547)", () => {
+  it("should respect the (added to dashboard) card id in the link (metabase#18547)", () => {
     cy.intercept("GET", `/api/dashboard/1`).as("dashboard");
 
     cy.visit("/dashboard/1");