From d616baf693f9aa890eba1685b3a415589c3236cd Mon Sep 17 00:00:00 2001
From: "Mahatthana (Kelvin) Nomsawadi" <me@bboykelvin.dev>
Date: Fri, 25 Oct 2024 23:27:18 +0700
Subject: [PATCH] Fix static embed card download when param has accentuation
(#49157)
* Fix static embed card download when param has accentuation
* Fix E2E util type
* Add a dashboard E2E test
* test question download with params
* Fix some names regarding tests
* Fix failed E2E tests
---
.../api/createDashboardWithQuestions.ts | 15 +-
.../embed-resource-downloads.cy.spec.ts | 176 ++++++++++++++++++
frontend/src/metabase/redux/downloads.ts | 15 +-
3 files changed, 191 insertions(+), 15 deletions(-)
diff --git a/e2e/support/helpers/api/createDashboardWithQuestions.ts b/e2e/support/helpers/api/createDashboardWithQuestions.ts
index d2a269cd292..99b8d6d1955 100644
--- a/e2e/support/helpers/api/createDashboardWithQuestions.ts
+++ b/e2e/support/helpers/api/createDashboardWithQuestions.ts
@@ -1,4 +1,4 @@
-import type { Card, Dashboard } from "metabase-types/api";
+import type { Card, Dashboard, DashboardCard } from "metabase-types/api";
import { cypressWaitAll } from "../e2e-misc-helpers";
@@ -16,14 +16,11 @@ export const createDashboardWithQuestions = ({
dashboardName?: string;
dashboardDetails?: DashboardDetails;
questions: (NativeQuestionDetails | StructuredQuestionDetails)[];
- cards?: Partial<Card>[];
-}): Cypress.Chainable<
- Cypress.Response<{
- dashboard: Dashboard;
- questions: Card;
- }>
-> => {
- // @ts-expect-error - Cypress typings don't account for what happens in then() here
+ cards?: Partial<DashboardCard>[];
+}): Cypress.Chainable<{
+ dashboard: Dashboard;
+ questions: Card[];
+}> => {
return createDashboard({ name: dashboardName, ...dashboardDetails }).then(
({ body: dashboard }) => {
return cypressWaitAll(
diff --git a/e2e/test/scenarios/embedding/embed-resource-downloads.cy.spec.ts b/e2e/test/scenarios/embedding/embed-resource-downloads.cy.spec.ts
index 534b746e04f..cd5258f9e6f 100644
--- a/e2e/test/scenarios/embedding/embed-resource-downloads.cy.spec.ts
+++ b/e2e/test/scenarios/embedding/embed-resource-downloads.cy.spec.ts
@@ -1,8 +1,12 @@
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
import {
ORDERS_BY_YEAR_QUESTION_ID,
ORDERS_DASHBOARD_ID,
} from "e2e/support/cypress_sample_instance_data";
import {
+ addOrUpdateDashboardCard,
+ createDashboardWithQuestions,
+ createNativeQuestion,
describeWithSnowplowEE,
expectGoodSnowplowEvent,
expectNoBadSnowplowEvents,
@@ -16,6 +20,9 @@ import {
showDashboardCardActions,
visitEmbeddedPage,
} from "e2e/support/helpers";
+import { createMockParameter } from "metabase-types/api/mocks";
+
+const { PRODUCTS, PRODUCTS_ID } = SAMPLE_DATABASE;
/** These tests are about the `downloads` flag for static embeds, both dashboards and questions.
* Unless the product changes, these should test the same things as `public-resource-downloads.cy.spec.ts`
@@ -120,6 +127,89 @@ describeWithSnowplowEE(
export_type: "csv",
});
});
+
+ describe("with dashboard parameters", () => {
+ beforeEach(() => {
+ cy.signInAsAdmin();
+
+ setTokenFeatures("all");
+
+ // Test parameter with accentuation (metabase#49118)
+ const CATEGORY_FILTER = createMockParameter({
+ id: "5aefc725",
+ name: "usuário",
+ slug: "usu%C3%A1rio",
+ type: "string/=",
+ });
+ const questionDetails = {
+ name: "Products",
+ query: {
+ "source-table": PRODUCTS_ID,
+ },
+ };
+ createDashboardWithQuestions({
+ // Can't figure out the type if I extracted `dashboardDetails` to a variable.
+ dashboardDetails: {
+ name: "Dashboard with a parameter",
+ parameters: [CATEGORY_FILTER],
+ enable_embedding: true,
+ embedding_params: {
+ [CATEGORY_FILTER.slug]: "enabled",
+ },
+ },
+ questions: [questionDetails],
+ }).then(({ dashboard, questions }) => {
+ const dashboardId = dashboard.id;
+ cy.wrap(dashboardId).as("dashboardId");
+ const questionId = questions[0].id;
+ addOrUpdateDashboardCard({
+ dashboard_id: dashboardId,
+ card_id: questionId,
+ card: {
+ parameter_mappings: [
+ {
+ card_id: questionId,
+ parameter_id: CATEGORY_FILTER.id,
+ target: ["dimension", ["field", PRODUCTS.CATEGORY, null]],
+ },
+ ],
+ },
+ });
+ });
+
+ cy.signOut();
+ });
+
+ it("should be able to download a static embedded dashcard as CSV", () => {
+ cy.get("@dashboardId").then(dashboardId => {
+ visitEmbeddedPage(
+ {
+ resource: { dashboard: Number(dashboardId) },
+ params: {},
+ },
+ {
+ pageStyle: {
+ downloads: true,
+ },
+ },
+ );
+ });
+
+ waitLoading();
+
+ showDashboardCardActions();
+ getDashboardCardMenu().click();
+ exportFromDashcard(".csv");
+ cy.verifyDownload(".csv", { contains: true });
+
+ expectGoodSnowplowEvent({
+ event: "download_results_clicked",
+ resource_type: "dashcard",
+ accessed_via: "static-embed",
+ export_type: "csv",
+ });
+ });
+ });
});
describe("Static embed questions", () => {
@@ -216,6 +306,92 @@ describeWithSnowplowEE(
export_type: "csv",
});
});
+
+ describe("with native question parameters", () => {
+ beforeEach(() => {
+ cy.signInAsAdmin();
+
+ setTokenFeatures("all");
+
+ // Can't figure out the type if I extracted `questionDetails` to a variable.
+ createNativeQuestion(
+ {
+ name: "Native question with a parameter",
+ native: {
+ "template-tags": {
+ num: {
+ id: "f7672b4d-1e84-1fa8-bf02-b5e584cd4535",
+ name: "num",
+ "display-name": "Num",
+ type: "number",
+ default: null,
+ },
+ },
+ query: "select {{num}}",
+ },
+ parameters: [
+ {
+ id: "f7672b4d-1e84-1fa8-bf02-b5e584cd4535",
+ type: "number/=",
+ target: ["variable", ["template-tag", "num"]],
+ name: "Num",
+ slug: "num",
+ default: null,
+ },
+ ],
+ enable_embedding: true,
+ embedding_params: {
+ num: "enabled",
+ },
+ },
+ {
+ idAlias: "questionId",
+ wrapId: true,
+ },
+ );
+
+ cy.signOut();
+ });
+
+ it("should be able to download a static embedded dashcard as CSV", () => {
+ const value = 9999;
+ cy.get("@questionId").then(questionId => {
+ visitEmbeddedPage(
+ {
+ resource: { question: Number(questionId) },
+ params: {
+ num: value,
+ },
+ },
+ {
+ pageStyle: {
+ downloads: true,
+ },
+ },
+ );
+ });
+
+ waitLoading();
+
+ main().findByText(value).should("exist");
+
+ cy.findByTestId("download-button").click();
+
+ popover().within(() => {
+ cy.findByText(".csv").click();
+ cy.findByTestId("download-results-button").click();
+ });
+
+ cy.verifyDownload(".csv", { contains: true });
+
+ expectGoodSnowplowEvent({
+ event: "download_results_clicked",
+ resource_type: "question",
+ accessed_via: "static-embed",
+ export_type: "csv",
+ });
+ });
+ });
});
},
);
diff --git a/frontend/src/metabase/redux/downloads.ts b/frontend/src/metabase/redux/downloads.ts
index 76046612aaf..a4153974439 100644
--- a/frontend/src/metabase/redux/downloads.ts
+++ b/frontend/src/metabase/redux/downloads.ts
@@ -206,20 +206,23 @@ const getDatasetParams = ({
return {
method: "GET",
url: `/api/embed/dashboard/${token}/dashcard/${dashcardId}/card/${cardId}/${type}`,
- params: Urls.getEncodedUrlSearchParams({ ...params, ...exportParams }),
+ params: new URLSearchParams({
+ parameters: JSON.stringify(params),
+ ..._.mapObject(exportParams, value => String(value)),
+ }),
};
}
if (resource === "question" && token) {
- // For whatever wacky reason the /api/embed endpoint expect params like ?key=value instead
- // of like ?params=<json-encoded-params-array> like the other endpoints do.
const params = new URLSearchParams(window.location.search);
- params.set("format_rows", String(enableFormatting));
- params.set("pivot_results", String(enablePivot));
+
return {
method: "GET",
url: Urls.embedCard(token, type),
- params,
+ params: new URLSearchParams({
+ parameters: JSON.stringify(Object.fromEntries(params)),
+ ..._.mapObject(exportParams, value => String(value)),
+ }),
};
}
}
--
GitLab