diff --git a/frontend/src/metabase/lib/urls.js b/frontend/src/metabase/lib/urls.js index fda9cc211f68c771b7b1296abe263d2c20d3cdc8..b563d606bc18949efd6e6ba29e0f11183d077bd2 100644 --- a/frontend/src/metabase/lib/urls.js +++ b/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}`; } diff --git a/frontend/test/metabase/lib/urls.unit.spec.js b/frontend/test/metabase/lib/urls.unit.spec.js index 852d08b346d4a947edc3228c211c31f19229f6a0..6f5400827614751578942b59f0ea0baadba89d0b 100644 --- a/frontend/test/metabase/lib/urls.unit.spec.js +++ b/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", () => { diff --git a/frontend/test/metabase/scenarios/onboarding/home/activity-page.cy.spec.js b/frontend/test/metabase/scenarios/onboarding/home/activity-page.cy.spec.js index 68ffa3349a99921fc10297b259b11d3ef94631db..d7a25ce59add6f9f22ee73e2aed91dda6cf5f659 100644 --- a/frontend/test/metabase/scenarios/onboarding/home/activity-page.cy.spec.js +++ b/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");