use correct api endpoint when exporting dashcards (#28923)

e2e/support/helpers/e2e-downloads-helpers.js

@@ -12,11 +12,25 @@ const xlsx = require("xlsx");
  * @param {function} callback
  */
 export function downloadAndAssert(
-  { fileType, questionId, raw, logResults, publicUid } = {},
+  {
+    fileType,
+    questionId,
+    raw,
+    logResults,
+    publicUid,
+    dashcardId,
+    dashboardId,
+  } = {},
   callback,
 ) {
   const downloadClassName = `.Icon-${fileType}`;
-  const endpoint = getEndpoint(fileType, questionId, publicUid);
+  const endpoint = getEndpoint(
+    fileType,
+    questionId,
+    publicUid,
+    dashcardId,
+    dashboardId,
+  );
   const isPublicDownload = !!publicUid;
   const method = isPublicDownload ? "GET" : "POST";
 
@@ -78,7 +92,11 @@ export function assertSheetRowsCount(expectedCount) {
   };
 }
 
-function getEndpoint(fileType, questionId, publicUid) {
+function getEndpoint(fileType, questionId, publicUid, dashcardId, dashboardId) {
+  if (dashcardId != null && dashboardId != null) {
+    return `api/dashboard/${dashboardId}/dashcard/${dashcardId}/card/${questionId}/query/${fileType}`;
+  }
+
   if (publicUid) {
     return `/public/question/${publicUid}.${fileType}**`;
   }

e2e/test/scenarios/dashboard/dashboard.cy.spec.js

@@ -11,8 +11,6 @@ import {
   visitDashboard,
   appbar,
   rightSidebar,
-  downloadAndAssert,
-  assertSheetRowsCount,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -543,21 +541,6 @@ describe("scenarios > dashboard", () => {
 
     cy.findByText("Orders").should("be.visible");
   });
-
-  it("should allow downloading card data", () => {
-    visitDashboard(1);
-    cy.findByTestId("dashcard").within(() => {
-      cy.findByTestId("legend-caption").realHover();
-    });
-
-    downloadAndAssert({ fileType: "xlsx", questionId: 1 }, sheet => {
-      expect(sheet["A1"].v).to.eq("ID");
-      expect(sheet["A2"].v).to.eq(1);
-      expect(sheet["A3"].v).to.eq(2);
-
-      assertSheetRowsCount(18760)(sheet);
-    });
-  });
 });
 
 function checkOptionsForFilter(filter) {

e2e/test/scenarios/downloads/downloads.cy.spec.js

@@ -5,6 +5,9 @@ import {
   visualize,
   visitDashboard,
   popover,
+  assertSheetRowsCount,
+  filterWidget,
+  saveDashboard,
 } from "e2e/support/helpers";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 
@@ -34,6 +37,45 @@ describe("scenarios > question > download", () => {
     });
   });
 
+  describe("from dashboards", () => {
+    it("should allow downloading card data", () => {
+      cy.intercept("GET", "/api/dashboard/**").as("dashboard");
+      visitDashboard(1);
+      cy.findByTestId("dashcard").within(() => {
+        cy.findByTestId("legend-caption").realHover();
+      });
+
+      assertOrdersExport(18760);
+
+      cy.icon("pencil").click();
+
+      cy.icon("filter").click();
+
+      popover().within(() => {
+        cy.contains("ID").click();
+      });
+
+      cy.get(".DashCard").contains("Select…").click();
+      popover().contains("ID").eq(0).click();
+
+      saveDashboard();
+
+      filterWidget().contains("ID").click();
+
+      popover().find("input").type("1");
+
+      cy.findByText("Add filter").click();
+
+      cy.wait("@dashboard");
+
+      cy.findByTestId("dashcard").within(() => {
+        cy.findByTestId("legend-caption").realHover();
+      });
+
+      assertOrdersExport(1);
+    });
+  });
+
   describe("png images", () => {
     const canSavePngQuestion = {
       name: "Q1",
@@ -104,3 +146,22 @@ describe("scenarios > question > download", () => {
     });
   });
 });
+
+function assertOrdersExport(length) {
+  downloadAndAssert(
+    {
+      fileType: "xlsx",
+      questionId: 1,
+      dashcardId: 1,
+      dashboardId: 1,
+    },
+    sheet => {
+      expect(sheet["A1"].v).to.eq("ID");
+      expect(sheet["A2"].v).to.eq(1);
+      expect(sheet["B1"].v).to.eq("User ID");
+      expect(sheet["B2"].v).to.eq(1);
+
+      assertSheetRowsCount(length)(sheet);
+    },
+  );
+}

frontend/src/metabase-types/api/dataset.ts

@@ -34,11 +34,16 @@ export interface DatasetData {
   download_perms?: DownloadPermission;
 }
 
+export type JsonQuery = {
+  parameters: unknown[];
+};
+
 export interface Dataset {
   data: DatasetData;
   database_id: DatabaseId;
   row_count: number;
   running_time: number;
+  json_query?: JsonQuery;
 }
 
 export interface NativeQueryForm {

frontend/src/metabase/core/types.ts

+export type PartialBy<T, K extends keyof T> = Omit<T, K> & Partial<Pick<T, K>>;

frontend/src/metabase/dashboard/components/DashCard/DashCardVisualization.tsx

@@ -203,12 +203,21 @@ function DashCardVisualization({
         result={mainSeries}
         params={parameterValuesBySlug}
         dashcardId={dashcard.id}
+        dashboardId={dashboard.id}
         token={isEmbed ? dashcard.dashboard_id : undefined}
         icon="ellipsis"
         iconSize={17}
       />
     );
-  }, [series, isEmbed, isPublic, isEditing, dashcard, parameterValuesBySlug]);
+  }, [
+    series,
+    isEmbed,
+    isPublic,
+    isEditing,
+    dashcard,
+    parameterValuesBySlug,
+    dashboard,
+  ]);
 
   return (
     <WrappedVisualization

frontend/src/metabase/query_builder/components/QueryDownloadWidget.jsx → frontend/src/metabase/query_builder/components/QueryDownloadWidget/QueryDownloadWidget.jsx

@@ -3,10 +3,7 @@ import React, { useState } from "react";
 import PropTypes from "prop-types";
 
 import { t } from "ttag";
-import { parse as urlParse } from "url";
-import querystring from "querystring";
 
-import _ from "underscore";
 import cx from "classnames";
 import { canSavePng } from "metabase/visualizations";
 import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
@@ -18,8 +15,8 @@ import {
 } from "metabase/components/DownloadButton";
 import Tooltip from "metabase/core/components/Tooltip";
 import { PLUGIN_FEATURE_LEVEL_PERMISSIONS } from "metabase/plugins";
-
 import * as Urls from "metabase/lib/urls";
+import { getDownloadButtonParams } from "./utils";
 
 import {
   WidgetFormat,
@@ -42,6 +39,7 @@ const QueryDownloadWidget = ({
   uuid,
   token,
   dashcardId,
+  dashboardId,
   icon,
   iconSize = 20,
   params,
@@ -73,74 +71,28 @@ const QueryDownloadWidget = ({
             <>
               {EXPORT_FORMATS.map(type => (
                 <WidgetFormat key={type}>
-                  {dashcardId && token ? (
-                    <DashboardEmbedQueryButton
-                      key={type}
-                      type={type}
-                      dashcardId={dashcardId}
-                      token={token}
-                      card={card}
-                      params={params}
-                      onDownloadStart={() => {
-                        setStatus("pending");
-                        closePopover();
-                      }}
-                      onDownloadResolved={() => setStatus("resolved")}
-                      onDownloadRejected={() => setStatus("rejected")}
-                    />
-                  ) : uuid ? (
-                    <PublicQueryButton
-                      key={type}
-                      type={type}
-                      uuid={uuid}
-                      result={result}
-                      onDownloadStart={() => {
-                        setStatus("pending");
-                        closePopover();
-                      }}
-                      onDownloadResolved={() => setStatus("resolved")}
-                      onDownloadRejected={() => setStatus("rejected")}
-                    />
-                  ) : token ? (
-                    <EmbedQueryButton
-                      key={type}
-                      type={type}
-                      token={token}
-                      onDownloadStart={() => {
-                        setStatus("pending");
-                        closePopover();
-                      }}
-                      onDownloadResolved={() => setStatus("resolved")}
-                      onDownloadRejected={() => setStatus("rejected")}
-                    />
-                  ) : card && card.id ? (
-                    <SavedQueryButton
-                      key={type}
-                      type={type}
-                      card={card}
-                      result={result}
-                      disabled={status === "pending"}
-                      onDownloadStart={() => {
-                        setStatus("pending");
-                        closePopover();
-                      }}
-                      onDownloadResolved={() => setStatus("resolved")}
-                      onDownloadRejected={() => setStatus("rejected")}
-                    />
-                  ) : card && !card.id ? (
-                    <UnsavedQueryButton
-                      key={type}
-                      type={type}
-                      result={result}
-                      visualizationSettings={visualizationSettings}
-                      onDownloadStart={() => {
-                        setStatus("pending");
-                        closePopover();
-                      }}
-                      onDownloadResolved={() => setStatus("resolved")}
-                      onDownloadRejected={() => setStatus("rejected")}
-                    />
-                  ) : null}
+                  <DownloadButton
+                    {...getDownloadButtonParams({
+                      type,
+                      params,
+                      card,
+                      visualizationSettings,
+                      result,
+                      uuid,
+                      token,
+                      dashcardId,
+                      dashboardId,
+                    })}
+                    extensions={[type]}
+                    onDownloadStart={() => {
+                      setStatus("pending");
+                      closePopover();
+                    }}
+                    onDownloadResolved={() => setStatus("resolved")}
+                    onDownloadRejected={() => setStatus("rejected")}
+                  >
+                    {type}
+                  </DownloadButton>
                 </WidgetFormat>
               ))}
               {canSavePng(card.display) ? (
@@ -154,122 +106,6 @@ const QueryDownloadWidget = ({
   );
 };
 
-const UnsavedQueryButton = ({
-  type,
-  result: { json_query = {} },
-  visualizationSettings,
-  onDownloadStart,
-  onDownloadResolved,
-  onDownloadRejected,
-}) => (
-  <DownloadButton
-    url={`api/dataset/${type}`}
-    params={{
-      query: JSON.stringify(_.omit(json_query, "constraints")),
-      visualization_settings: JSON.stringify(visualizationSettings),
-    }}
-    extensions={[type]}
-    onDownloadStart={onDownloadStart}
-    onDownloadResolved={onDownloadResolved}
-    onDownloadRejected={onDownloadRejected}
-  >
-    {type}
-  </DownloadButton>
-);
-
-const SavedQueryButton = ({
-  type,
-  result: { json_query = {} },
-  card,
-  onDownloadStart,
-  onDownloadResolved,
-  onDownloadRejected,
-}) => (
-  <DownloadButton
-    url={`api/card/${card.id}/query/${type}`}
-    params={{ parameters: JSON.stringify(json_query.parameters) }}
-    extensions={[type]}
-    onDownloadStart={onDownloadStart}
-    onDownloadResolved={onDownloadResolved}
-    onDownloadRejected={onDownloadRejected}
-  >
-    {type}
-  </DownloadButton>
-);
-
-const PublicQueryButton = ({
-  type,
-  uuid,
-  result: { json_query = {} },
-  onDownloadStart,
-  onDownloadResolved,
-  onDownloadRejected,
-}) => (
-  <DownloadButton
-    method="GET"
-    url={Urls.publicQuestion(uuid, type)}
-    params={{ parameters: JSON.stringify(json_query.parameters) }}
-    extensions={[type]}
-    onDownloadStart={onDownloadStart}
-    onDownloadResolved={onDownloadResolved}
-    onDownloadRejected={onDownloadRejected}
-  >
-    {type}
-  </DownloadButton>
-);
-
-const EmbedQueryButton = ({
-  type,
-  token,
-  onDownloadStart,
-  onDownloadResolved,
-  onDownloadRejected,
-}) => {
-  // Parse the query string part of the URL (e.g. the `?key=value` part) into an object. We need to pass them this
-  // way to the `DownloadButton` because it's a form which means we need to insert a hidden `<input>` for each param
-  // we want to pass along. 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 query = urlParse(window.location.href).query; // get the part of the URL that looks like key=value
-  const params = query && querystring.parse(query); // expand them out into a map
-
-  return (
-    <DownloadButton
-      method="GET"
-      url={Urls.embedCard(token, type)}
-      params={params}
-      extensions={[type]}
-      onDownloadStart={onDownloadStart}
-      onDownloadResolved={onDownloadResolved}
-      onDownloadRejected={onDownloadRejected}
-    >
-      {type}
-    </DownloadButton>
-  );
-};
-
-const DashboardEmbedQueryButton = ({
-  type,
-  dashcardId,
-  token,
-  card,
-  params,
-  onDownloadStart,
-  onDownloadResolved,
-  onDownloadRejected,
-}) => (
-  <DownloadButton
-    method="GET"
-    url={`api/embed/dashboard/${token}/dashcard/${dashcardId}/card/${card.id}/${type}`}
-    extensions={[type]}
-    params={params}
-    onDownloadStart={onDownloadStart}
-    onDownloadResolved={onDownloadResolved}
-    onDownloadRejected={onDownloadRejected}
-  >
-    {type}
-  </DownloadButton>
-);
-
 const LOADER_SCALE_FACTOR = 0.9;
 
 const renderIcon = ({ icon, status, iconSize }) => {

frontend/src/metabase/query_builder/components/QueryDownloadWidget/index.ts

+export { default } from "./QueryDownloadWidget";

frontend/src/metabase/query_builder/components/QueryDownloadWidget/utils.ts

+import _ from "underscore";
+import { parse as urlParse } from "url";
+import querystring from "querystring";
+import { Card, Dataset, VisualizationSettings } from "metabase-types/api";
+import * as Urls from "metabase/lib/urls";
+import { PartialBy } from "metabase/core/types";
+
+interface GetDownloadButtonParamsInput {
+  type: string;
+  params: Record<string, unknown>;
+  card: PartialBy<Card, "id">;
+  visualizationSettings?: VisualizationSettings;
+  result?: Dataset;
+  uuid?: string;
+  token?: string;
+  dashcardId?: number;
+  dashboardId?: number;
+}
+
+export const getDownloadButtonParams = ({
+  type,
+  params,
+  card,
+  visualizationSettings,
+  result,
+  uuid,
+  token,
+  dashcardId,
+  dashboardId,
+}: GetDownloadButtonParamsInput) => {
+  const isSecureDashboardEmbedding = dashcardId != null && token != null;
+  if (isSecureDashboardEmbedding) {
+    return {
+      method: "GET",
+      url: `api/embed/dashboard/${token}/dashcard/${dashcardId}/card/${card.id}/${type}`,
+      params,
+    };
+  }
+
+  const isDashboard = dashboardId != null && dashcardId != null;
+  if (isDashboard) {
+    return {
+      method: "POST",
+      url: `api/dashboard/${dashboardId}/dashcard/${dashcardId}/card/${card.id}/query/${type}`,
+      params: { parameters: JSON.stringify(result?.json_query?.parameters) },
+    };
+  }
+
+  const isPublicQuestion = uuid != null;
+  if (isPublicQuestion) {
+    return {
+      method: "GET",
+      url: Urls.publicQuestion(uuid, type),
+      params: { parameters: JSON.stringify(result?.json_query?.parameters) },
+    };
+  }
+
+  const isEmbeddedQuestion = token != null;
+  if (isEmbeddedQuestion) {
+    // Parse the query string part of the URL (e.g. the `?key=value` part) into an object. We need to pass them this
+    // way to the `DownloadButton` because it's a form which means we need to insert a hidden `<input>` for each param
+    // we want to pass along. 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 query = urlParse(window.location.href).query; // get the part of the URL that looks like key=value
+    const params = query && querystring.parse(query); // expand them out into a map
+
+    return {
+      method: "GET",
+      url: Urls.embedCard(token, type),
+      params: params,
+    };
+  }
+
+  const isSavedQuery = card?.id != null;
+  if (isSavedQuery) {
+    return {
+      url: `api/card/${card.id}/query/${type}`,
+      params: { parameters: JSON.stringify(result?.json_query?.parameters) },
+    };
+  }
+
+  const isUnsavedQuery = card && !card.id;
+  if (isUnsavedQuery) {
+    return {
+      url: `api/dataset/${type}`,
+      params: {
+        query: JSON.stringify(_.omit(result?.json_query, "constraints")),
+        visualization_settings: JSON.stringify(visualizationSettings),
+      },
+    };
+  }
+
+  return null;
+};

frontend/src/metabase/query_builder/components/QueryDownloadWidget/utils.unit.spec.ts

+import _ from "underscore";
+import {
+  createMockCard,
+  createMockDataset,
+  createMockVisualizationSettings,
+} from "metabase-types/api/mocks";
+import MetabaseSettings from "metabase/lib/settings";
+import { getDownloadButtonParams } from "./utils";
+
+const type = "csv";
+const params = { id: 1 };
+const card = createMockCard();
+const visualizationSettings = createMockVisualizationSettings();
+const result = createMockDataset({
+  json_query: {
+    parameters: [],
+  },
+});
+const uuid = "uuid";
+const token = "token";
+const dashcardId = 10;
+const dashboardId = 100;
+
+describe("getDownloadButtonParams", () => {
+  let siteUrl: any;
+
+  beforeEach(() => {
+    siteUrl = MetabaseSettings.get("site-url");
+    MetabaseSettings.set("site-url", "http://metabase.com");
+  });
+
+  afterEach(() => {
+    MetabaseSettings.set("site-url", siteUrl);
+  });
+
+  it("returns params for a embedding on an embedded dashboard", () => {
+    expect(
+      getDownloadButtonParams({
+        params,
+        type,
+        card,
+        dashcardId,
+        dashboardId,
+        token,
+      }),
+    ).toStrictEqual({
+      method: "GET",
+      params: {
+        id: 1,
+      },
+      url: "api/embed/dashboard/token/dashcard/10/card/1/csv",
+    });
+  });
+
+  it("returns params for a dashcard", () => {
+    expect(
+      getDownloadButtonParams({
+        params,
+        type,
+        card,
+        dashcardId,
+        dashboardId,
+        result,
+      }),
+    ).toStrictEqual({
+      method: "POST",
+      params: { parameters: "[]" },
+      url: "api/dashboard/100/dashcard/10/card/1/query/csv",
+    });
+  });
+
+  it("returns params for a public question", () => {
+    expect(
+      getDownloadButtonParams({
+        params,
+        type,
+        card,
+        uuid,
+        result,
+      }),
+    ).toStrictEqual({
+      method: "GET",
+      params: { parameters: "[]" },
+      url: "http://metabase.com/public/question/uuid.csv",
+    });
+  });
+
+  it("returns params for an embedded question", () => {
+    expect(
+      getDownloadButtonParams({
+        params,
+        type,
+        token,
+        card,
+      }),
+    ).toStrictEqual({
+      method: "GET",
+      params: null,
+      url: "http://metabase.com/embed/question/token.csv",
+    });
+  });
+
+  it("returns params for a saved question", () => {
+    expect(
+      getDownloadButtonParams({
+        params,
+        type,
+        card,
+        result,
+      }),
+    ).toStrictEqual({
+      params: { parameters: "[]" },
+      url: "api/card/1/query/csv",
+    });
+  });
+
+  it("returns params for an unsaved question", () => {
+    expect(
+      getDownloadButtonParams({
+        params,
+        type,
+        visualizationSettings,
+        card: _.omit(card, "id"),
+        result,
+      }),
+    ).toStrictEqual({
+      url: "api/dataset/csv",
+      params: {
+        query: JSON.stringify({ parameters: [] }),
+        visualization_settings: "{}",
+      },
+    });
+  });
+});