diff --git a/e2e/support/helpers/e2e-downloads-helpers.js b/e2e/support/helpers/e2e-downloads-helpers.js
index 4aa251c521bdff60c4e49a374d1f2ff1eda4cb81..402b13989aa71c4b97d589c8243412896415a82d 100644
--- a/e2e/support/helpers/e2e-downloads-helpers.js
+++ b/e2e/support/helpers/e2e-downloads-helpers.js
@@ -29,6 +29,7 @@ export function downloadAndAssert(
downloadUrl,
isEmbed,
isDashboard,
+ enableFormatting = true,
} = {},
callback,
) {
@@ -76,7 +77,7 @@ export function downloadAndAssert(
cy.findByTestId("download-button").click();
}
// Initiate the file download
- popover().findByText(`.${fileType}`).click();
+ popover().findByText(`.${fileType}`).click({ altKey: !enableFormatting });
cy.wait("@fileDownload")
.its("request")
@@ -110,15 +111,15 @@ export function assertSheetRowsCount(expectedCount) {
function getEndpoint(fileType, questionId, publicUid, dashcardId, dashboardId) {
if (dashcardId != null && dashboardId != null) {
- return `api/dashboard/${dashboardId}/dashcard/${dashcardId}/card/${questionId}/query/${fileType}`;
+ return `api/dashboard/${dashboardId}/dashcard/${dashcardId}/card/${questionId}/query/${fileType}**`;
}
if (publicUid) {
return `/public/question/${publicUid}.${fileType}**`;
}
- const questionEndpoint = `/api/card/${questionId}/query/${fileType}`;
- const queryEndpoint = `/api/dataset/${fileType}`;
+ const questionEndpoint = `/api/card/${questionId}/query/${fileType}**`;
+ const queryEndpoint = `/api/dataset/${fileType}**`;
return questionId ? questionEndpoint : queryEndpoint;
}
diff --git a/e2e/test/scenarios/sharing/downloads/downloads.cy.spec.js b/e2e/test/scenarios/sharing/downloads/downloads.cy.spec.js
index 03f4631a99340a6ab52f1b2e54517ad28722e675..659ed34c4bd221c612097bb04b8d3a68d518eb71 100644
--- a/e2e/test/scenarios/sharing/downloads/downloads.cy.spec.js
+++ b/e2e/test/scenarios/sharing/downloads/downloads.cy.spec.js
@@ -21,6 +21,8 @@ import {
resetSnowplow,
enableTracking,
addOrUpdateDashboardCard,
+ createQuestion,
+ queryBuilderMain,
} from "e2e/support/helpers";
const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
@@ -75,6 +77,60 @@ describe("scenarios > question > download", () => {
});
});
+ it("should allow downloading unformatted CSV data", () => {
+ const fieldRef = ["field", ORDERS.TOTAL, null];
+ const columnKey = `["ref",${JSON.stringify(fieldRef)}]`;
+
+ createQuestion(
+ {
+ query: {
+ "source-table": ORDERS_ID,
+ fields: [fieldRef],
+ },
+ visualization_settings: {
+ column_settings: {
+ [columnKey]: {
+ currency: "USD",
+ currency_in_header: false,
+ currency_style: "code",
+ number_style: "currency",
+ },
+ },
+ },
+ },
+ { visitQuestion: true, wrapId: true },
+ );
+
+ queryBuilderMain().findByText("USD 39.72").should("exist");
+
+ cy.get("@questionId").then(questionId => {
+ const opts = { questionId, fileType: "csv" };
+
+ downloadAndAssert(
+ {
+ ...opts,
+ enableFormatting: true,
+ },
+ sheet => {
+ expect(sheet["A1"].v).to.eq("Total");
+ expect(sheet["A2"].v).to.eq("USD 39.72");
+ },
+ );
+
+ downloadAndAssert(
+ {
+ ...opts,
+ enableFormatting: false,
+ },
+ sheet => {
+ expect(sheet["A1"].v).to.eq("Total");
+ expect(sheet["A2"].v).to.eq(39.718145389078366);
+ expect(sheet["A2"].w).to.eq("39.718145389078366");
+ },
+ );
+ });
+ });
+
describe("from dashboards", () => {
it("should allow downloading card data", () => {
cy.intercept("GET", "/api/dashboard/**").as("dashboard");
diff --git a/frontend/src/metabase/dashboard/components/DashCard/DashCardMenu/DashCardMenu.tsx b/frontend/src/metabase/dashboard/components/DashCard/DashCardMenu/DashCardMenu.tsx
index 47e06df7d833128488769f651f50876e4a9feba2..1553fe057121c282672284b5d9c033ef52c5819a 100644
--- a/frontend/src/metabase/dashboard/components/DashCard/DashCardMenu/DashCardMenu.tsx
+++ b/frontend/src/metabase/dashboard/components/DashCard/DashCardMenu/DashCardMenu.tsx
@@ -64,9 +64,9 @@ const DashCardMenu = ({
onDownloadResults,
}: DashCardMenuProps) => {
const [{ loading }, handleDownload] = useAsyncFn(
- async (type: string) => {
+ async (opts: { type: string; enableFormatting: boolean }) => {
await onDownloadResults({
- type,
+ ...opts,
question,
result,
dashboardId,
@@ -84,9 +84,9 @@ const DashCardMenu = ({
<QueryDownloadPopover
question={question}
result={result}
- onDownload={type => {
+ onDownload={opts => {
toggleMenu();
- handleDownload(type);
+ handleDownload(opts);
}}
/>
),
diff --git a/frontend/src/metabase/query_builder/actions/downloading.ts b/frontend/src/metabase/query_builder/actions/downloading.ts
index 7dd7c66f46abbae1a428bdcc6cdee29a977cc70b..8c9bb43bb5d89021a96dd4c21067852a7d2db410 100644
--- a/frontend/src/metabase/query_builder/actions/downloading.ts
+++ b/frontend/src/metabase/query_builder/actions/downloading.ts
@@ -17,6 +17,7 @@ export interface DownloadQueryResultsOpts {
type: string;
question: Question;
result: Dataset;
+ enableFormatting?: boolean;
dashboardId?: DashboardId;
dashcardId?: DashCardId;
uuid?: string;
@@ -28,7 +29,8 @@ export interface DownloadQueryResultsOpts {
interface DownloadQueryResultsParams {
method: string;
url: string;
- params: URLSearchParams;
+ body?: Record<string, unknown>;
+ params?: URLSearchParams;
}
export const downloadQueryResults =
@@ -59,6 +61,7 @@ const getDatasetParams = ({
question,
dashboardId,
dashcardId,
+ enableFormatting,
uuid,
token,
params = {},
@@ -67,11 +70,15 @@ const getDatasetParams = ({
}: DownloadQueryResultsOpts): DownloadQueryResultsParams => {
const cardId = question.id();
const isSecureDashboardEmbedding = dashcardId != null && token != null;
+
+ // Formatting is always enabled for Excel
+ const format_rows = enableFormatting && type !== "xlsx" ? "true" : "false";
+
if (isSecureDashboardEmbedding) {
return {
method: "GET",
url: `/api/embed/dashboard/${token}/dashcard/${dashcardId}/card/${cardId}/${type}`,
- params: Urls.getEncodedUrlSearchParams(params),
+ params: Urls.getEncodedUrlSearchParams({ ...params, format_rows }),
};
}
@@ -80,9 +87,10 @@ const getDatasetParams = ({
return {
method: "POST",
url: `/api/dashboard/${dashboardId}/dashcard/${dashcardId}/card/${cardId}/query/${type}`,
- params: new URLSearchParams({
- parameters: JSON.stringify(result?.json_query?.parameters ?? []),
- }),
+ params: new URLSearchParams({ format_rows }),
+ body: {
+ parameters: result?.json_query?.parameters ?? [],
+ },
};
}
@@ -93,6 +101,7 @@ const getDatasetParams = ({
url: Urls.publicQuestion({ uuid, type, includeSiteUrl: false }),
params: new URLSearchParams({
parameters: JSON.stringify(result?.json_query?.parameters ?? []),
+ format_rows,
}),
};
}
@@ -101,10 +110,12 @@ const getDatasetParams = ({
if (isEmbeddedQuestion) {
// 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", format_rows);
return {
method: "GET",
url: Urls.embedCard(token, type),
- params: new URLSearchParams(window.location.search),
+ params,
};
}
@@ -113,41 +124,53 @@ const getDatasetParams = ({
return {
method: "POST",
url: `/api/card/${cardId}/query/${type}`,
- params: new URLSearchParams({
- parameters: JSON.stringify(result?.json_query?.parameters ?? []),
- }),
+ params: new URLSearchParams({ format_rows }),
+ body: {
+ parameters: result?.json_query?.parameters ?? [],
+ },
};
}
return {
- url: `/api/dataset/${type}`,
method: "POST",
- params: new URLSearchParams({
- query: JSON.stringify(_.omit(result?.json_query ?? {}, "constraints")),
- visualization_settings: JSON.stringify(visualizationSettings ?? {}),
- }),
+ url: `/api/dataset/${type}`,
+ params: new URLSearchParams({ format_rows }),
+ body: {
+ query: _.omit(result?.json_query ?? {}, "constraints"),
+ visualization_settings: visualizationSettings ?? {},
+ },
};
};
-export function getDatasetDownloadUrl(url: string) {
- // make url relative if it's not
- url = url.replace(api.basename, "");
- const requestUrl = new URL(api.basename + url, location.origin);
-
+export function getDatasetDownloadUrl(url: string, params?: URLSearchParams) {
+ url = url.replace(api.basename, ""); // make url relative if it's not
+ url = api.basename + url;
+ if (params) {
+ url += `?${params.toString()}`;
+ }
+ const requestUrl = new URL(url, location.origin);
return requestUrl.href;
}
const getDatasetResponse = ({
url,
method,
+ body,
params,
}: DownloadQueryResultsParams) => {
- const requestUrl = getDatasetDownloadUrl(url);
+ const requestUrl = getDatasetDownloadUrl(url, params);
if (method === "POST") {
- return fetch(requestUrl, { method, body: params });
+ // BE expects the body to be form-encoded :(
+ const formattedBody = new URLSearchParams();
+ if (body != null) {
+ for (const key in body) {
+ formattedBody.append(key, JSON.stringify(body[key]));
+ }
+ }
+ return fetch(requestUrl, { method, body: formattedBody });
} else {
- return fetch(`${requestUrl}?${params}`);
+ return fetch(requestUrl);
}
};
diff --git a/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.styled.tsx b/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.styled.tsx
index a7cf1a771677433f3047511561b9ff60b3c44817..63cc524e7a56f9a977f80552d603ec3c6f41e30a 100644
--- a/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.styled.tsx
+++ b/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.styled.tsx
@@ -12,6 +12,10 @@ export const DownloadPopoverRoot = styled.div<DownloadPopoverRootProps>`
`;
export const DownloadPopoverHeader = styled.div`
+ display: flex;
+ flex-direction: row;
+ justify-content: space-between;
+ align-items: center;
padding: 0.5rem;
`;
@@ -22,7 +26,7 @@ export const DownloadPopoverMessage = styled.div`
export const DownloadButtonRoot = styled.button`
display: flex;
align-items: center;
- gap: 0.5rem;
+ justify-content: space-between;
width: 100%;
margin: 0.5rem 0;
padding: 0.5rem;
@@ -42,3 +46,12 @@ export const DownloadButtonText = styled.div`
color: ${color("white")};
}
`;
+
+export const DownloadButtonSecondaryText = styled.div`
+ color: ${color("text-light")};
+ font-weight: bold;
+
+ ${DownloadButtonRoot}:hover & {
+ color: ${color("white")};
+ }
+`;
diff --git a/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.tsx b/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.tsx
index 6b513e15911d3b81c6fe4d8fc1a06f8d5bfb6666..5b27b095894b73e9f0eaed2f1fd34fa0fa582257 100644
--- a/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.tsx
+++ b/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.tsx
@@ -1,9 +1,12 @@
-import { useCallback } from "react";
+import { useState } from "react";
import { connect } from "react-redux";
+import { useEvent } from "react-use";
import { t } from "ttag";
+import { isMac } from "metabase/lib/browser";
import { exportFormats, exportFormatPng } from "metabase/lib/urls";
import { PLUGIN_FEATURE_LEVEL_PERMISSIONS } from "metabase/plugins";
+import { Icon, Tooltip, useHover } from "metabase/ui";
import { canSavePng } from "metabase/visualizations";
import type Question from "metabase-lib/v1/Question";
import type { Dataset } from "metabase-types/api";
@@ -11,6 +14,7 @@ import type { State } from "metabase-types/store";
import {
DownloadButtonRoot,
+ DownloadButtonSecondaryText,
DownloadButtonText,
DownloadPopoverHeader,
DownloadPopoverMessage,
@@ -20,7 +24,7 @@ import {
interface OwnProps {
question: Question;
result: Dataset;
- onDownload: (format: string) => void;
+ onDownload: (opts: { type: string; enableFormatting: boolean }) => void;
}
interface StateProps {
@@ -43,20 +47,53 @@ const mapStateToProps = (
t`The maximum download size is 1 million rows.`,
});
+// Excel and images always use formatting
+const checkCanManageFormatting = (format: string) =>
+ format !== "xlsx" && format !== "png";
+
const QueryDownloadPopover = ({
canDownloadPng,
hasTruncatedResults,
limitedDownloadSizeText,
onDownload,
}: QueryDownloadPopoverProps) => {
+ const [isHoldingAltKey, setHoldingAltKey] = useState(false);
+
+ useEvent("keydown", event => {
+ if (event.key === "Alt") {
+ setHoldingAltKey(true);
+ }
+ });
+
+ useEvent("keyup", event => {
+ if (event.key === "Alt") {
+ setHoldingAltKey(false);
+ }
+ });
+
const formats = canDownloadPng
? [...exportFormats, exportFormatPng]
: exportFormats;
+ const handleDownload = (type: string, enableFormatting: boolean) => {
+ const canManageFormatting = checkCanManageFormatting(type);
+ onDownload({
+ type,
+ enableFormatting: canManageFormatting ? enableFormatting : true,
+ });
+ };
+
+ const formattingInfoTooltipLabel = isMac()
+ ? t`Option click to download without formatting applied`
+ : t`Alt click to download without formatting applied`;
+
return (
<DownloadPopoverRoot isExpanded={hasTruncatedResults}>
<DownloadPopoverHeader>
<h4>{t`Download full results`}</h4>
+ <Tooltip label={formattingInfoTooltipLabel}>
+ <Icon name="info_filled" />
+ </Tooltip>
</DownloadPopoverHeader>
{hasTruncatedResults && (
<DownloadPopoverMessage>
@@ -65,7 +102,13 @@ const QueryDownloadPopover = ({
</DownloadPopoverMessage>
)}
{formats.map(format => (
- <DownloadButton key={format} format={format} onDownload={onDownload} />
+ <DownloadButton
+ key={format}
+ format={format}
+ hasUnformattedOption={checkCanManageFormatting(format)}
+ isHoldingAltKey={isHoldingAltKey}
+ onDownload={handleDownload}
+ />
))}
</DownloadPopoverRoot>
);
@@ -73,17 +116,28 @@ const QueryDownloadPopover = ({
interface DownloadButtonProps {
format: string;
- onDownload: (format: string) => void;
+ hasUnformattedOption: boolean;
+ isHoldingAltKey: boolean;
+ onDownload: (format: string, enableFormatting: boolean) => void;
}
-const DownloadButton = ({ format, onDownload }: DownloadButtonProps) => {
- const handleClick = useCallback(() => {
- onDownload(format);
- }, [format, onDownload]);
+const DownloadButton = ({
+ format,
+ hasUnformattedOption,
+ isHoldingAltKey,
+ onDownload,
+}: DownloadButtonProps) => {
+ const { hovered, ref } = useHover<HTMLButtonElement>();
return (
- <DownloadButtonRoot onClick={handleClick}>
+ <DownloadButtonRoot
+ onClick={event => onDownload(format, !event.altKey)}
+ ref={ref}
+ >
<DownloadButtonText>.{format}</DownloadButtonText>
+ {hasUnformattedOption && isHoldingAltKey && hovered && (
+ <DownloadButtonSecondaryText>{t`(Unformatted)`}</DownloadButtonSecondaryText>
+ )}
</DownloadButtonRoot>
);
};
diff --git a/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.unit.spec.tsx b/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.unit.spec.tsx
new file mode 100644
index 0000000000000000000000000000000000000000..0a098dadd2e67b39a9383700ab5c2438cc79ebf2
--- /dev/null
+++ b/frontend/src/metabase/query_builder/components/QueryDownloadPopover/QueryDownloadPopover.unit.spec.tsx
@@ -0,0 +1,129 @@
+import userEvent from "@testing-library/user-event";
+
+import { setupCardQueryDownloadEndpoint } from "__support__/server-mocks";
+import { createMockEntitiesState } from "__support__/store";
+import { act, fireEvent, renderWithProviders, screen } from "__support__/ui";
+import { checkNotNull } from "metabase/lib/types";
+import { getMetadata } from "metabase/selectors/metadata";
+import registerVisualizations from "metabase/visualizations/register";
+import type { Card, Dataset } from "metabase-types/api";
+import {
+ createMockCard,
+ createMockDataset,
+ createMockStructuredDatasetQuery,
+} from "metabase-types/api/mocks";
+import { ORDERS_ID, SAMPLE_DB_ID } from "metabase-types/api/mocks/presets";
+import { createMockState } from "metabase-types/store/mocks";
+
+import QueryDownloadPopover from "./QueryDownloadPopover";
+
+registerVisualizations();
+
+const TEST_CARD = createMockCard({
+ dataset_query: createMockStructuredDatasetQuery({
+ database: SAMPLE_DB_ID,
+ query: {
+ "source-table": ORDERS_ID,
+ },
+ }),
+});
+
+const TEST_RESULT = createMockDataset();
+
+interface SetupOpts {
+ card?: Card;
+ result?: Dataset;
+}
+
+const setup = ({ card = TEST_CARD, result = TEST_RESULT }: SetupOpts = {}) => {
+ const onDownload = jest.fn();
+
+ const state = createMockState({
+ entities: createMockEntitiesState({
+ questions: [card],
+ }),
+ });
+
+ const metadata = getMetadata(state);
+ const question = checkNotNull(metadata.question(card.id));
+
+ setupCardQueryDownloadEndpoint(card, "json");
+
+ renderWithProviders(
+ <QueryDownloadPopover
+ question={question}
+ result={result}
+ onDownload={onDownload}
+ />,
+ );
+
+ return { onDownload };
+};
+
+describe("QueryDownloadPopover", () => {
+ it("should display download options", async () => {
+ setup();
+
+ expect(screen.getByText(/csv/)).toBeInTheDocument();
+ expect(screen.getByText(/xlsx/)).toBeInTheDocument();
+ expect(screen.getByText(/json/)).toBeInTheDocument();
+ });
+
+ it("should suggest PNG downloads for compatible visualizations", async () => {
+ setup({ card: { ...TEST_CARD, display: "line" } });
+ expect(screen.getByText(/png/)).toBeInTheDocument();
+ });
+
+ it("should not suggest PNG downloads for incompatible visualizations", async () => {
+ setup({ card: { ...TEST_CARD, display: "table" } });
+ expect(screen.queryByText(/png/)).not.toBeInTheDocument();
+ });
+
+ it("should trigger download on click", async () => {
+ const { onDownload } = setup();
+ act(() => userEvent.click(screen.getByText(/csv/)));
+ expect(onDownload).toHaveBeenCalledWith({
+ type: "csv",
+ enableFormatting: true,
+ });
+ });
+
+ it.each(["csv", "json"])(
+ "should trigger unformatted download for %s format",
+ async format => {
+ const { onDownload } = setup();
+
+ expect(screen.queryByText(/Unformatted/i)).not.toBeInTheDocument();
+ await fireEvent.keyDown(screen.getByText(/csv/), { key: "Alt" });
+ act(() => userEvent.hover(screen.getByText(/csv/)));
+ expect(await screen.findByText(/Unformatted/i)).toBeInTheDocument();
+
+ act(() =>
+ userEvent.click(screen.getByText(new RegExp(format)), { altKey: true }),
+ );
+
+ expect(onDownload).toHaveBeenCalledWith({
+ type: format,
+ enableFormatting: false,
+ });
+ },
+ );
+
+ it.each(["xlsx", "png"])(
+ "should not trigger unformatted download for %s format",
+ async format => {
+ const { onDownload } = setup({ card: { ...TEST_CARD, display: "line" } });
+
+ expect(screen.queryByText(/Unformatted/i)).not.toBeInTheDocument();
+ await fireEvent.keyDown(screen.getByText(/csv/), { key: "Alt" });
+ expect(screen.queryByText(/Unformatted/i)).not.toBeInTheDocument();
+
+ act(() => userEvent.click(screen.getByText(new RegExp(format))));
+
+ expect(onDownload).toHaveBeenCalledWith({
+ type: format,
+ enableFormatting: true,
+ });
+ },
+ );
+});
diff --git a/frontend/src/metabase/query_builder/components/QueryDownloadWidget/QueryDownloadWidget.tsx b/frontend/src/metabase/query_builder/components/QueryDownloadWidget/QueryDownloadWidget.tsx
index 97940800ac19f5bb97854f406973829a86a45d59..d4c31cdfb8c6c7c4c6d68986c2469a6b5720ab11 100644
--- a/frontend/src/metabase/query_builder/components/QueryDownloadWidget/QueryDownloadWidget.tsx
+++ b/frontend/src/metabase/query_builder/components/QueryDownloadWidget/QueryDownloadWidget.tsx
@@ -50,9 +50,9 @@ const QueryDownloadWidget = ({
const [isPopoverOpen, setIsPopoverOpen] = useState(false);
const [{ loading }, handleDownload] = useAsyncFn(
- async (type: string) => {
+ async (opts: { type: string; enableFormatting: boolean }) => {
await onDownload({
- type,
+ ...opts,
question,
result,
dashboardId,
@@ -89,9 +89,9 @@ const QueryDownloadWidget = ({
<QueryDownloadPopover
question={question}
result={result}
- onDownload={type => {
+ onDownload={opts => {
setIsPopoverOpen(false);
- handleDownload(type);
+ handleDownload(opts);
}}
/>
</Popover.Dropdown>
diff --git a/frontend/src/metabase/sharing/components/EmailAttachmentPicker.jsx b/frontend/src/metabase/sharing/components/EmailAttachmentPicker.jsx
index 9f48f0189bd7facc078d6cc9ecbfb776a3d457be..c3fa1d04e3c00b92f98a4b6635038e7a118fe0ac 100644
--- a/frontend/src/metabase/sharing/components/EmailAttachmentPicker.jsx
+++ b/frontend/src/metabase/sharing/components/EmailAttachmentPicker.jsx
@@ -16,6 +16,7 @@ export default class EmailAttachmentPicker extends Component {
state = {
isEnabled: false,
+ isFormattingEnabled: true,
selectedAttachmentType: this.DEFAULT_ATTACHMENT_TYPE,
selectedCardIds: new Set(),
};
@@ -54,6 +55,7 @@ export default class EmailAttachmentPicker extends Component {
selectedAttachmentType:
this.attachmentTypeFor(selectedCards) || this.DEFAULT_ATTACHMENT_TYPE,
selectedCardIds: new Set(selectedCards.map(card => card.id)),
+ isFormattingEnabled: getInitialFormattingState(selectedCards),
};
}
@@ -71,6 +73,8 @@ export default class EmailAttachmentPicker extends Component {
*/
updatePulseCards(attachmentType, selectedCardIds) {
const { pulse, setPulse } = this.props;
+ const { isFormattingEnabled } = this.state;
+
const isXls = attachmentType === "xls",
isCsv = attachmentType === "csv";
@@ -81,6 +85,7 @@ export default class EmailAttachmentPicker extends Component {
cards: pulse.cards.map(card => {
card.include_csv = selectedCardIds.has(card.id) && isCsv;
card.include_xls = selectedCardIds.has(card.id) && isXls;
+ card.format_rows = isCsv && isFormattingEnabled; // Excel always uses formatting
return card;
}),
});
@@ -165,6 +170,21 @@ export default class EmailAttachmentPicker extends Component {
});
};
+ onToggleFormatting = () => {
+ this.setState(
+ prevState => ({
+ ...prevState,
+ isFormattingEnabled: !prevState.isFormattingEnabled,
+ }),
+ () => {
+ this.updatePulseCards(
+ this.state.selectedAttachmentType,
+ this.state.selectedCardIds,
+ );
+ },
+ );
+ };
+
disableAllCards() {
const selectedCardIds = new Set();
this.updatePulseCards(this.state.selectedAttachmentType, selectedCardIds);
@@ -181,7 +201,12 @@ export default class EmailAttachmentPicker extends Component {
render() {
const { cards } = this.props;
- const { isEnabled, selectedAttachmentType, selectedCardIds } = this.state;
+ const {
+ isEnabled,
+ isFormattingEnabled,
+ selectedAttachmentType,
+ selectedCardIds,
+ } = this.state;
return (
<div>
@@ -205,6 +230,15 @@ export default class EmailAttachmentPicker extends Component {
fullWidth
/>
</div>
+ {selectedAttachmentType === "csv" && (
+ <div className={cx(CS.mt2, CS.mb3)}>
+ <CheckBox
+ checked={!isFormattingEnabled}
+ label={t`Use unformatted values in attachments`}
+ onChange={this.onToggleFormatting}
+ />
+ </div>
+ )}
<div
className={cx(
CS.textBold,
@@ -264,3 +298,10 @@ export default class EmailAttachmentPicker extends Component {
);
}
}
+
+function getInitialFormattingState(cards) {
+ if (cards.length > 0) {
+ return cards.some(card => !!card.format_rows);
+ }
+ return true;
+}
diff --git a/frontend/src/metabase/sharing/components/SharingSidebar/SharingSidebar.jsx b/frontend/src/metabase/sharing/components/SharingSidebar/SharingSidebar.jsx
index 230cc868f6b17098f118a3d6b01e9841f7877463..f415880162c40a85014149e69d62016daaaf594c 100644
--- a/frontend/src/metabase/sharing/components/SharingSidebar/SharingSidebar.jsx
+++ b/frontend/src/metabase/sharing/components/SharingSidebar/SharingSidebar.jsx
@@ -77,6 +77,7 @@ const cardsToPulseCards = (cards, pulseCards) => {
const pulseCard = pulseCards.find(pc => pc.id === card.id) || card;
return {
...card,
+ format_rows: pulseCard.format_rows,
include_csv: pulseCard.include_csv,
include_xls: pulseCard.include_xls,
};
diff --git a/frontend/src/metabase/ui/index.ts b/frontend/src/metabase/ui/index.ts
index 33b227c6fbd27f6f3f9e83cf46799ede4ec11e81..49e195bf4d0beaf54861fa2b235ce1130b1ccf28 100644
--- a/frontend/src/metabase/ui/index.ts
+++ b/frontend/src/metabase/ui/index.ts
@@ -1,2 +1,3 @@
export { rem } from "@mantine/core";
+export { useHover } from "@mantine/hooks";
export * from "./components";
diff --git a/resources/migrations/001_update_migrations.yaml b/resources/migrations/001_update_migrations.yaml
index b2ffa389095608cbcb165f57c54733d920ba96be..58d7a515c9a7d349d30965ac065e8ddd144c8691 100644
--- a/resources/migrations/001_update_migrations.yaml
+++ b/resources/migrations/001_update_migrations.yaml
@@ -5529,6 +5529,20 @@ databaseChangeLog:
sql: INSERT INTO setting ("KEY", "VALUE") VALUES ('enable-public-sharing', 'false');
rollback: # not needed
+ - changeSet:
+ id: v49.2024-03-26T10:23:12
+ author: adam-james
+ comment: Add pulse_card.format_rows
+ changes:
+ - addColumn:
+ tableName: pulse_card
+ columns:
+ - column:
+ name: format_rows
+ type: ${boolean.type}
+ defaultValue: true
+ remarks: Whether or not to apply formatting to the rows of the export
+
- changeSet:
id: v49.2024-03-26T20:27:58
author: noahmoss
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index 4205ba7e8538f56dcce65c1b3321d86cb26b961d..741514fd9718c8585e0927abfbacea91cde786de 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -672,9 +672,10 @@
`parameters` should be passed as query parameter encoded as a serialized JSON string (this is because this endpoint
is normally used to power 'Download Results' buttons that use HTML `form` actions)."
- [card-id export-format :as {{:keys [parameters]} :params}]
+ [card-id export-format :as {{:keys [parameters format_rows]} :params}]
{card-id ms/PositiveInt
parameters [:maybe ms/JSONString]
+ format_rows [:maybe :boolean]
export-format (into [:enum] api.dataset/export-formats)}
(qp.card/process-query-for-card
card-id export-format
@@ -684,7 +685,7 @@
:middleware {:process-viz-settings? true
:skip-results-metadata? true
:ignore-cached-results? true
- :format-rows? false
+ :format-rows? format_rows
:js-int-to-string? false}))
;;; ----------------------------------------------- Sharing is Caring ------------------------------------------------
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index 7b66fd4bcdf98e4bbe2b6bf6459457ba193db9d0..fc987517da9f1e07e5345d1de7411f859db85648 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -1160,12 +1160,13 @@
`parameters` should be passed as query parameter encoded as a serialized JSON string (this is because this endpoint
is normally used to power 'Download Results' buttons that use HTML `form` actions)."
- [dashboard-id dashcard-id card-id export-format :as {{:keys [parameters], :as request-parameters} :params}]
+ [dashboard-id dashcard-id card-id export-format :as {{:keys [parameters format_rows], :as request-parameters} :params}]
{dashboard-id ms/PositiveInt
dashcard-id ms/PositiveInt
card-id ms/PositiveInt
parameters [:maybe ms/JSONString]
- export-format api.dataset/ExportFormat}
+ export-format api.dataset/ExportFormat
+ format_rows [:maybe :boolean]}
(m/mapply qp.dashboard/process-query-for-dashcard
(merge
request-parameters
@@ -1182,7 +1183,7 @@
:middleware {:process-viz-settings? true
:skip-results-metadata? true
:ignore-cached-results? true
- :format-rows? false
+ :format-rows? format_rows
:js-int-to-string? false}})))
(api/defendpoint POST "/pivot/:dashboard-id/dashcard/:dashcard-id/card/:card-id/query"
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index 43f5e05fef435df3140624ba356a81c6eab64fdb..3cfcb37d942382500eeeabaaa68ffef843c65607 100644
--- a/src/metabase/api/dataset.clj
+++ b/src/metabase/api/dataset.clj
@@ -125,9 +125,11 @@
(api/defendpoint POST ["/:export-format", :export-format export-format-regex]
"Execute a query and download the result data as a file in the specified format."
- [export-format :as {{:keys [query visualization_settings] :or {visualization_settings "{}"}} :params}]
+ [export-format :as {{:keys [query visualization_settings format_rows]
+ :or {visualization_settings "{}"}} :params}]
{query ms/JSONString
visualization_settings ms/JSONString
+ format_rows [:maybe :boolean]
export-format (into [:enum] export-formats)}
(let [query (json/parse-string query keyword)
viz-settings (-> (json/parse-string visualization_settings viz-setting-key-fn)
@@ -140,7 +142,7 @@
(dissoc :add-default-userland-constraints? :js-int-to-string?)
(assoc :process-viz-settings? true
:skip-results-metadata? true
- :format-rows? false))))]
+ :format-rows? format_rows))))]
(run-streaming-query
(qp/userland-query query)
:export-format export-format
diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj
index 4ec632ec9b5155984e530ce0e081df228d584d1a..532cfb0df2cff77822e7b66188bc2ca590e3eb78 100644
--- a/src/metabase/api/embed.clj
+++ b/src/metabase/api/embed.clj
@@ -406,8 +406,9 @@
(api/defendpoint GET ["/card/:token/query/:export-format", :export-format api.dataset/export-format-regex]
"Like `GET /api/embed/card/query`, but returns the results as a file in the specified format."
- [token export-format :as {:keys [query-params]}]
- {export-format (into [:enum] api.dataset/export-formats)}
+ [token export-format :as {:keys [query-params format_rows]}]
+ {export-format (into [:enum] api.dataset/export-formats)
+ format_rows [:maybe :boolean]}
(run-query-for-unsigned-token-async
(embed/unsign token)
export-format
@@ -415,7 +416,7 @@
:constraints nil
:middleware {:process-viz-settings? true
:js-int-to-string? false
- :format-rows? false}))
+ :format-rows? format_rows}))
;;; ----------------------------------------- /api/embed/dashboard endpoints -----------------------------------------
@@ -459,7 +460,7 @@
:card-id card-id
:embedding-params (t2/select-one-fn :embedding_params Dashboard :id dashboard-id)
:token-params (embed/get-in-unsigned-token-or-throw unsigned-token [:params])
- :query-params query-params
+ :query-params (dissoc query-params :format_rows)
:constraints constraints
:qp qp
:middleware middleware)))
@@ -556,9 +557,10 @@
:export-format api.dataset/export-format-regex]
"Fetch the results of running a Card belonging to a Dashboard using a JSON Web Token signed with the
`embedding-secret-key` return the data in one of the export formats"
- [token export-format dashcard-id card-id, :as {:keys [query-params]}]
+ [token export-format dashcard-id card-id format_rows, :as {:keys [query-params]}]
{dashcard-id ms/PositiveInt
card-id ms/PositiveInt
+ format_rows [:maybe :boolean]
export-format (into [:enum] api.dataset/export-formats)}
(process-query-for-dashcard-with-signed-token token
dashcard-id
@@ -568,7 +570,7 @@
:constraints nil
:middleware {:process-viz-settings? true
:js-int-to-string? false
- :format-rows? false}))
+ :format-rows? format_rows}))
;;; ----------------------------------------------- Param values -------------------------------------------------
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index d3193d55fa8bc4c7510f6d4d45b8f9e66dda8234..e2d56da2770769b1ffa9ae3d2363588149a5c4d8 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -175,9 +175,10 @@
(api/defendpoint GET "/card/:uuid/query/:export-format"
"Fetch a publicly-accessible Card and return query results in the specified format. Does not require auth
credentials. Public sharing must be enabled."
- [uuid export-format :as {{:keys [parameters]} :params}]
+ [uuid export-format :as {{:keys [parameters format_rows]} :params}]
{uuid ms/UUIDString
export-format api.dataset/ExportFormat
+ format_rows [:maybe :boolean]
parameters [:maybe ms/JSONString]}
(process-query-for-card-with-public-uuid
uuid
@@ -186,7 +187,7 @@
:constraints nil
:middleware {:process-viz-settings? true
:js-int-to-string? false
- :format-rows? false}))
+ :format-rows? format_rows}))
;;; ----------------------------------------------- Public Dashboards ------------------------------------------------
diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj
index 44760486694d87a783d2c84f49e89087e154f09b..74a9f8fb999c738b4917e2f3b138a64160a8ff12 100644
--- a/src/metabase/email/messages.clj
+++ b/src/metabase/email/messages.clj
@@ -30,7 +30,6 @@
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.streaming :as qp.streaming]
[metabase.query-processor.streaming.interface :as qp.si]
- [metabase.query-processor.streaming.xlsx :as qp.xlsx]
[metabase.query-processor.timezone :as qp.timezone]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
@@ -403,39 +402,41 @@
point in the future; for now, this function is a stopgap.
Results are streamed synchronously. Caller is responsible for closing `os` when this call is complete."
- [export-format ^OutputStream os {{:keys [rows]} :data, database-id :database_id, :as results}]
+ [export-format format-rows? ^OutputStream os {{:keys [rows]} :data, database-id :database_id, :as results}]
;; make sure Database/driver info is available for the streaming results writers -- they might need this in order to
;; get timezone information when writing results
(driver/with-driver (driver.u/database->driver database-id)
(qp.store/with-metadata-provider database-id
- (binding [qp.xlsx/*parse-temporal-string-values* true]
- (let [w (qp.si/streaming-results-writer export-format os)
- cols (-> results :data :cols)
- viz-settings (-> results :data :viz-settings)
- [ordered-cols output-order] (qp.streaming/order-cols cols viz-settings)
- viz-settings' (assoc viz-settings :output-order output-order)]
- (qp.si/begin! w
- (assoc-in results [:data :ordered-cols] ordered-cols)
- viz-settings')
- (dorun
- (map-indexed
- (fn [i row]
- (qp.si/write-row! w row i ordered-cols viz-settings'))
- rows))
- (qp.si/finish! w results))))))
+ (let [w (qp.si/streaming-results-writer export-format os)
+ cols (-> results :data :cols)
+ viz-settings (-> results :data :viz-settings)
+ [ordered-cols output-order] (qp.streaming/order-cols cols viz-settings)
+ viz-settings' (assoc viz-settings :output-order output-order)]
+ (qp.si/begin! w
+ (-> results
+ (assoc-in [:data :format-rows?] format-rows?)
+ (assoc-in [:data :ordered-cols] ordered-cols))
+ viz-settings')
+ (dorun
+ (map-indexed
+ (fn [i row]
+ (qp.si/write-row! w row i ordered-cols viz-settings'))
+ rows))
+ (qp.si/finish! w results)))))
(defn- result-attachment
- [{{card-name :name :as card} :card {{:keys [rows]} :data :as result} :result}]
+ [{{card-name :name format-rows :format_rows :as card} :card
+ {{:keys [rows]} :data :as result} :result}]
(when (seq rows)
[(when-let [temp-file (and (:include_csv card)
(create-temp-file-or-throw "csv"))]
(with-open [os (io/output-stream temp-file)]
- (stream-api-results-to-export-format :csv os result))
+ (stream-api-results-to-export-format :csv format-rows os result))
(create-result-attachment-map "csv" card-name temp-file))
(when-let [temp-file (and (:include_xls card)
(create-temp-file-or-throw "xlsx"))]
(with-open [os (io/output-stream temp-file)]
- (stream-api-results-to-export-format :xlsx os result))
+ (stream-api-results-to-export-format :xlsx format-rows os result))
(create-result-attachment-map "xlsx" card-name temp-file))]))
(defn- part-attachments [parts]
@@ -518,7 +519,7 @@
(for [{{result-card-id :id} :card :as result} results
:let [pulse-card (m/find-first #(= (:id %) result-card-id) (:cards pulse))]]
(if result-card-id
- (update result :card merge (select-keys pulse-card [:include_csv :include_xls]))
+ (update result :card merge (select-keys pulse-card [:include_csv :include_xls :format_rows]))
result)))
(defn render-pulse-email
diff --git a/src/metabase/legacy_mbql/schema.cljc b/src/metabase/legacy_mbql/schema.cljc
index 6bd5d6956deae00bb5062094df9ca92c231592d4..2fd071ce6f5bd51222f40084b5ab4ecea66fbbc2 100644
--- a/src/metabase/legacy_mbql/schema.cljc
+++ b/src/metabase/legacy_mbql/schema.cljc
@@ -1522,9 +1522,11 @@
;; should we skip adding results_metadata to query results after running the query? Used by
;; [[metabase.query-processor.middleware.results-metadata]]; default `false`
[:skip-results-metadata? {:optional true} :boolean]
- ;; should we skip converting datetime types to ISO-8601 strings with appropriate timezone when post-processing
- ;; results? Used by [[metabase.query-processor.middleware.format-rows]]; default `false`
- [:format-rows? {:optional true} :boolean]
+ ;; should we convert datetime types to ISO-8601 strings with appropriate timezone when post-processing results?
+ ;; additionally, should we apply formatting specified in column/viz settings to exports (csv/json)?
+ ;; See [[metabase.query-processor.streaming.csv]]
+ ;; Used by [[metabase.query-processor.middleware.format-rows]]; defaults to `true`
+ [:format-rows? {:optional true} [:maybe :boolean]]
;; disable the MBQL->native middleware. If you do this, the query will not work at all, so there are no cases where
;; you should set this yourself. This is only used by the [[metabase.query-processor.preprocess/preprocess]]
;; function to get the fully pre-processed query without attempting to convert it to native.
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 282e725073ba6206026ea1dcbd5084a782699362..de167bbc194328d28eb293b327ce61072136661e 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -180,6 +180,7 @@
[:map
[:include_csv ms/BooleanValue]
[:include_xls ms/BooleanValue]
+ [:format_rows {:optional true} [:maybe ms/BooleanValue]]
[:dashboard_card_id {:optional true} [:maybe ms/PositiveInt]]]
(deferred-tru "value must be a map with the keys `{0}`, `{1}`, and `{2}`." "include_csv" "include_xls" "dashboard_card_id")))
@@ -232,7 +233,7 @@
[pulse-ids]
(t2/select
:model/Card
- {:select [:c.id :c.name :c.description :c.collection_id :c.display :pc.include_csv :pc.include_xls
+ {:select [:c.id :c.name :c.description :c.collection_id :c.display :pc.include_csv :pc.include_xls :pc.format_rows
:pc.dashboard_card_id :dc.dashboard_id [nil :parameter_mappings] [:p.id :pulse_id]] ;; :dc.parameter_mappings - how do you select this?
:from [[:pulse :p]]
:join [[:pulse_card :pc] [:= :p.id :pc.pulse_id]
@@ -416,6 +417,7 @@
{:id (u/the-id card)
:include_csv (get card :include_csv false)
:include_xls (get card :include_xls false)
+ :format_rows (get card :format_rows true)
:dashboard_card_id (get card :dashboard_card_id nil)})
@@ -433,12 +435,13 @@
(t2/delete! PulseCard :pulse_id (u/the-id notification-or-id))
;; now just insert all of the cards that were given to us
(when (seq card-refs)
- (let [cards (map-indexed (fn [i {card-id :id :keys [include_csv include_xls dashboard_card_id]}]
+ (let [cards (map-indexed (fn [i {card-id :id :keys [include_csv include_xls format_rows dashboard_card_id]}]
{:pulse_id (u/the-id notification-or-id)
:card_id card-id
:position i
:include_csv include_csv
:include_xls include_xls
+ :format_rows format_rows
:dashboard_card_id dashboard_card_id})
card-refs)]
(t2/insert! PulseCard cards))))
diff --git a/src/metabase/models/pulse_card.clj b/src/metabase/models/pulse_card.clj
index ed0ae801abaf2776427a36177171bf71cb16ae61..2ce2866a0dbc4634745ca390129caff5ed9b6a6f 100644
--- a/src/metabase/models/pulse_card.clj
+++ b/src/metabase/models/pulse_card.clj
@@ -40,20 +40,22 @@
[:dashboard_card_id ms/PositiveInt]
[:position {:optional true} [:maybe ms/IntGreaterThanOrEqualToZero]]
[:include_csv {:optional true} [:maybe :boolean]]
- [:include_xls {:optional true} [:maybe :boolean]]])
+ [:include_xls {:optional true} [:maybe :boolean]]
+ [:format_rows {:optional true} [:maybe :boolean]]])
(mu/defn bulk-create!
"Creates new PulseCards, joining the given card, pulse, and dashboard card and setting appropriate defaults for other
values if they're not provided."
[new-pulse-cards :- [:sequential NewPulseCard]]
(t2/insert! PulseCard
- (for [{:keys [card_id pulse_id dashboard_card_id position include_csv include_xls]} new-pulse-cards]
+ (for [{:keys [card_id pulse_id dashboard_card_id position include_csv include_xls format_rows]} new-pulse-cards]
{:card_id card_id
:pulse_id pulse_id
:dashboard_card_id dashboard_card_id
:position (u/or-with some? position (next-position-for pulse_id))
:include_csv (boolean include_csv)
- :include_xls (boolean include_xls)})))
+ :include_xls (boolean include_xls)
+ :format_rows (boolean format_rows)})))
; ----------------------------------------------------- Serialization -------------------------------------------------
diff --git a/src/metabase/query_processor/middleware/format_rows.clj b/src/metabase/query_processor/middleware/format_rows.clj
index 8ab3a14c45c853990e82e66a1f9a43ffda7ef554..bc8abfb46a5e77e83d6ab7314f1925bd2df91a7b 100644
--- a/src/metabase/query_processor/middleware/format_rows.clj
+++ b/src/metabase/query_processor/middleware/format_rows.clj
@@ -76,7 +76,10 @@
(defn format-rows
"Format individual query result values as needed. Ex: format temporal values as ISO-8601 strings w/ timezone offset."
[{{:keys [format-rows?] :or {format-rows? true}} :middleware, :as _query} rff]
- (if format-rows?
- (fn format-rows-rff* [metadata]
- (format-rows-xform (rff metadata) metadata))
- rff))
+ (fn format-rows-rff* [metadata]
+ ;; always assoc `:format-rows?` into the metadata so that
+ ;; the `qp.si/streaming-results-writer` implmementations can apply/not-apply formatting based on the key's value
+ (let [metadata (assoc metadata :format-rows? format-rows?)]
+ (if format-rows?
+ (format-rows-xform (rff metadata) metadata)
+ (rff metadata)))))
diff --git a/src/metabase/query_processor/streaming/csv.clj b/src/metabase/query_processor/streaming/csv.clj
index 30ea9cf8260e6a1d6ba643a61fccff1b6bb25f4c..937d1158e9effcafce89308f8f4504c67c42c8a5 100644
--- a/src/metabase/query_processor/streaming/csv.clj
+++ b/src/metabase/query_processor/streaming/csv.clj
@@ -29,11 +29,13 @@
(let [writer (BufferedWriter. (OutputStreamWriter. os StandardCharsets/UTF_8))
ordered-formatters (volatile! nil)]
(reify qp.si/StreamingResultsWriter
- (begin! [_ {{:keys [ordered-cols results_timezone]} :data} viz-settings]
+ (begin! [_ {{:keys [ordered-cols results_timezone format-rows?]
+ :or {format-rows? true}} :data} viz-settings]
(let [col-names (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings))]
- (vreset! ordered-formatters (mapv (fn [col]
- (formatter/create-formatter results_timezone col viz-settings))
- ordered-cols))
+ (vreset! ordered-formatters
+ (if format-rows?
+ (mapv #(formatter/create-formatter results_timezone % viz-settings) ordered-cols)
+ (vec (repeat (count ordered-cols) identity))))
(csv/write-csv writer [col-names])
(.flush writer)))
diff --git a/src/metabase/query_processor/streaming/json.clj b/src/metabase/query_processor/streaming/json.clj
index 1a48839c06869c34b28579799a42bb80e3871b45..f505dcf629f5e0633f1013fe3a8f5c70af83d4ee 100644
--- a/src/metabase/query_processor/streaming/json.clj
+++ b/src/metabase/query_processor/streaming/json.clj
@@ -31,13 +31,15 @@
col-names (volatile! nil)
ordered-formatters (volatile! nil)]
(reify qp.si/StreamingResultsWriter
- (begin! [_ {{:keys [ordered-cols results_timezone]} :data} viz-settings]
+ (begin! [_ {{:keys [ordered-cols results_timezone format-rows?]
+ :or {format-rows? true}} :data} viz-settings]
;; TODO -- wouldn't it make more sense if the JSON downloads used `:name` preferentially? Seeing how JSON is
;; probably going to be parsed programmatically
(vreset! col-names (common/column-titles ordered-cols (::mb.viz/column-settings viz-settings)))
- (vreset! ordered-formatters (mapv (fn [col]
- (formatter/create-formatter results_timezone col viz-settings))
- ordered-cols))
+ (vreset! ordered-formatters
+ (if format-rows?
+ (mapv #(formatter/create-formatter results_timezone % viz-settings) ordered-cols)
+ (vec (repeat (count ordered-cols) identity))))
(.write writer "[\n"))
(write-row! [_ row row-num _ {:keys [output-order]}]
diff --git a/src/metabase/query_processor/streaming/xlsx.clj b/src/metabase/query_processor/streaming/xlsx.clj
index 15a5c465ae0b3100f6025b1892fe0d87784a6afd..f4a3afc1cc171b02fa0f4721b6a09dddc0501561 100644
--- a/src/metabase/query_processor/streaming/xlsx.clj
+++ b/src/metabase/query_processor/streaming/xlsx.clj
@@ -368,11 +368,6 @@
(defmethod set-cell! nil [^Cell cell _value _styles _typed-styles]
(.setBlank cell))
-(def ^:dynamic *parse-temporal-string-values*
- "When true, XLSX exports will attempt to parse string values into corresponding java.time classes so that
- formatting can be applied. This should be enabled for generation of pulse/dashboard subscription attachments."
- false)
-
(defn- maybe-parse-coordinate-value [value {:keys [semantic_type]}]
(when (isa? semantic_type :type/Coordinate)
(try (formatter/format-geographic-coordinates semantic_type value)
@@ -381,9 +376,15 @@
value))))
(defn- maybe-parse-temporal-value
+ "The format-rows qp middleware formats rows into strings, which circumvents the formatting done in this namespace.
+ To gain the formatting back, we parse the temporal strings back into their java.time objects.
+
+ TODO: find a way to avoid this java.time -> string -> java.time conversion by making sure the format-rows middlware
+ works effectively with the streaming-results-writer implementations for CSV, JSON, and XLSX.
+ A hint towards a better solution is to add into the format-rows middleware the use of
+ viz-settings/column-formatting that is used inside `metabase.formatter/create-formatter`."
[value col]
- (when (and *parse-temporal-string-values*
- (isa? (:effective_type col) :type/Temporal)
+ (when (and (isa? (or (:base_type col) (:effective_type col)) :type/Temporal)
(string? value))
(try (u.date/parse value)
;; Fallback to plain string value if it couldn't be parsed
diff --git a/test/metabase/api/alert_test.clj b/test/metabase/api/alert_test.clj
index 1a7e3443ce60594e781c4b5fc4f79ff0efe51ec0..61dd8ea95dcc115aeaeb4ef395981e6ceb8a99ba 100644
--- a/test/metabase/api/alert_test.clj
+++ b/test/metabase/api/alert_test.clj
@@ -34,7 +34,7 @@
(select-keys [:name :description :display])
(update :display name)
(update :collection_id boolean)
- (assoc :id true, :include_csv false, :include_xls false, :dashboard_card_id false,
+ (assoc :id true, :include_csv false, :include_xls false, :format_rows true, :dashboard_card_id false,
:dashboard_id false, :parameter_mappings nil)))
(defn- recipient-details [user-kwd]
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 625c4d1bfe8a8428bbfaeb323e87a0bed337e815..d8522320367b6e62300623c9b88274e6540fae8b 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -2,6 +2,7 @@
"Tests for /api/card endpoints."
(:require
[cheshire.core :as json]
+ [clojure.data.csv :as csv]
[clojure.set :as set]
[clojure.string :as str]
[clojure.test :refer :all]
@@ -3514,3 +3515,22 @@
:visualization_settings {}}]
(is (=? {:message #"Invalid Field Filter: Field \d+ \"VENUES\"\.\"NAME\" belongs to Database \d+ \"test-data\", but the query is against Database \d+ \"daily-bird-counts\""}
(mt/user-http-request :crowberto :post 400 "card" card-data)))))))
+
+(deftest ^:parallel format-export-middleware-test
+ (testing "The `:format-export?` query processor middleware has the intended effect on file exports."
+ (let [q {:database (mt/id)
+ :type :native
+ :native {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}}
+ output-helper {:csv (fn [output] (->> output csv/read-csv last))
+ :json (fn [output] (->> output (map (juxt :NUMBER :DATE)) last))}]
+ (t2.with-temp/with-temp [Card {card-id :id} {:display :table :dataset_query q}]
+ (doseq [[export-format apply-formatting? expected] [[:csv true ["2,000" "March 26, 2024"]]
+ [:csv false ["2000" "2024-03-26"]]
+ [:json true ["2,000" "March 26, 2024"]]
+ [:json false [2000 "2024-03-26"]]]]
+ (testing (format "export_format %s yields expected output for %s exports." apply-formatting? export-format)
+ (is (= expected
+ (->> (mt/user-http-request
+ :crowberto :post 200
+ (format "card/%s/query/%s?format_rows=%s" card-id (name export-format) apply-formatting?))
+ ((get output-helper export-format)))))))))))
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 7a7125f6f51403c9edf539a2c283cdcd5a11e019..d37ff3997c0a6e94283149fb4880aebc01f67871 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -2,6 +2,7 @@
"Tests for /api/dashboard endpoints."
(:require
[cheshire.core :as json]
+ [clojure.data.csv :as csv]
[clojure.set :as set]
[clojure.string :as str]
[clojure.test :refer :all]
@@ -4363,3 +4364,25 @@
(is (=? {:data {:rows [["1" "Red Medicine" "4" 10.0646 -165.374 3]
["2" "Stout Burgers & Beers" "11" 34.0996 -118.329 2]]}}
(mt/user-http-request :crowberto :post 202 (dashboard-card-query-url dashboard-id card-id dashcard-id)))))))))
+
+(deftest ^:parallel format-export-middleware-test
+ (testing "The `:format-export?` query processor middleware has the intended effect on file exports."
+ (let [q {:database (mt/id)
+ :type :native
+ :native {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}}
+ output-helper {:csv (fn [output] (->> output csv/read-csv last))
+ :json (fn [output] (->> output (map (juxt :NUMBER :DATE)) last))}]
+ (t2.with-temp/with-temp [Card {card-id :id} {:display :table :dataset_query q}
+ Dashboard {dashboard-id :id} {}
+ DashboardCard {dashcard-id :id} {:dashboard_id dashboard-id
+ :card_id card-id}]
+ (doseq [[export-format apply-formatting? expected] [[:csv true ["2,000" "March 26, 2024"]]
+ [:csv false ["2000" "2024-03-26"]]
+ [:json true ["2,000" "March 26, 2024"]]
+ [:json false [2000 "2024-03-26"]]]]
+ (testing (format "export_format %s yields expected output for %s exports." apply-formatting? export-format)
+ (is (= expected
+ (->> (mt/user-http-request
+ :crowberto :post 200
+ (format "/dashboard/%s/dashcard/%s/card/%s/query/%s?format_rows=%s" dashboard-id dashcard-id card-id (name export-format) apply-formatting?))
+ ((get output-helper export-format)))))))))))
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 435e85ee52274577d4bb16f2e9bd4148fd6e528f..faec1dafbe32a8a28811261f1786f8bf8103e979 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -634,3 +634,22 @@
(some->> s
(driver/prettify-native-form :h2)
str/split-lines))))))))))
+
+(deftest ^:parallel format-export-middleware-test
+ (testing "The `:format-export?` query processor middleware has the intended effect on file exports."
+ (let [q {:database (mt/id)
+ :type :native
+ :native {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}}
+ output-helper {:csv (fn [output] (->> output csv/read-csv last))
+ :json (fn [output] (->> output (map (juxt :NUMBER :DATE)) last))}]
+ (doseq [[export-format apply-formatting? expected] [[:csv true ["2,000" "March 26, 2024"]]
+ [:csv false ["2000" "2024-03-26"]]
+ [:json true ["2,000" "March 26, 2024"]]
+ [:json false [2000 "2024-03-26"]]]]
+ (testing (format "export_format %s yields expected output for %s exports." apply-formatting? export-format)
+ (is (= expected
+ (->> (mt/user-http-request
+ :crowberto :post 200
+ (format "dataset/%s?format_rows=%s" (name export-format) apply-formatting?)
+ :query (json/generate-string q))
+ ((get output-helper export-format))))))))))
diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj
index 5598019973a8974402d737ccb4d1eab3e25787ed..13466a46585bafc544b2c6cb7c8db75aa61a04a2 100644
--- a/test/metabase/api/embed_test.clj
+++ b/test/metabase/api/embed_test.clj
@@ -1599,3 +1599,28 @@
:embedding_params {:qty_locked "locked"}}]
(is (= [3443]
(mt/first-row (client/client :get 202 (card-query-url card "" {:params {:qty_locked 1}}))))))))))
+
+(deftest format-export-middleware-test
+ (testing "The `:format-export?` query processor middleware has the intended effect on file exports."
+ (let [q {:database (mt/id)
+ :type :native
+ :native {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}}
+ output-helper {:csv (fn [output] (->> output csv/read-csv last))
+ :json (fn [output] (->> output (map (juxt :NUMBER :DATE)) last))}]
+ (with-embedding-enabled-and-new-secret-key
+ (t2.with-temp/with-temp [Card {card-id :id} {:display :table :dataset_query q}
+ Dashboard {dashboard-id :id} {:enable_embedding true
+ :embedding_params {:name "enabled"}}
+ DashboardCard {dashcard-id :id} {:dashboard_id dashboard-id
+ :card_id card-id}]
+ (doseq [[export-format apply-formatting? expected] [[:csv true ["2,000" "March 26, 2024"]]
+ [:csv false ["2000" "2024-03-26"]]
+ [:json true ["2,000" "March 26, 2024"]]
+ [:json false [2000 "2024-03-26"]]]]
+ (testing (format "export_format %s yields expected output for %s exports." apply-formatting? export-format)
+ (is (= expected
+ (->> (mt/user-http-request
+ :crowberto :get 200
+ (format "embed/dashboard/%s/dashcard/%s/card/%s/%s?format_rows=%s"
+ (dash-token dashboard-id) dashcard-id card-id (name export-format) apply-formatting?))
+ ((get output-helper export-format))))))))))))
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index 25e15b988366483051a2d101ebf0a464476903bc..09aa1ca722778c913813a44d5e0ac4d03b511ced 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -2,6 +2,7 @@
"Tests for `api/public/` (public links) endpoints."
(:require
[cheshire.core :as json]
+ [clojure.data.csv :as csv]
[clojure.set :as set]
[clojure.string :as str]
[clojure.test :refer :all]
@@ -1736,3 +1737,23 @@
"type" "query"}
:user-id nil}
(last (snowplow-test/pop-event-data-and-user-id!))))))))))))
+
+(deftest format-export-middleware-test
+ (mt/with-temporary-setting-values [enable-public-sharing true]
+ (testing "The `:format-export?` query processor middleware has the intended effect on file exports."
+ (let [q {:database (mt/id)
+ :type :native
+ :native {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}}
+ output-helper {:csv (fn [output] (->> output csv/read-csv last))
+ :json (fn [output] (->> output (map (juxt :NUMBER :DATE)) last))}]
+ (with-temp-public-card [{uuid :public_uuid} {:display :table :dataset_query q}]
+ (doseq [[export-format apply-formatting? expected] [[:csv true ["2,000" "March 26, 2024"]]
+ [:csv false ["2000" "2024-03-26"]]
+ [:json true ["2,000" "March 26, 2024"]]
+ [:json false [2000 "2024-03-26"]]]]
+ (testing (format "export_format %s yields expected output for %s exports." apply-formatting? export-format)
+ (is (= expected
+ (->> (mt/user-http-request
+ :crowberto :get 200
+ (format "public/card/%s/query/%s?format_rows=%s" uuid (name export-format) apply-formatting?))
+ ((get output-helper export-format))))))))))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index ecf49c0af9f0d35ddb34050b2736d29b1874b82c..917cc830d1473c2e20a07a85851914b137855e14 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -43,7 +43,7 @@
(update :collection_id boolean)
;; why? these fields in this last assoc are from the PulseCard model and this function takes the Card model
;; because PulseCard is somewhat hidden behind the scenes
- (assoc :include_csv false, :include_xls false, :dashboard_card_id nil, :dashboard_id nil,
+ (assoc :include_csv false, :include_xls false, :dashboard_card_id nil, :dashboard_id nil, :format_rows true
:parameter_mappings nil)))
(defn- pulse-channel-details [channel]
@@ -329,10 +329,12 @@
:cards [{:id (u/the-id card-1)
:include_csv true
:include_xls true
+ :format_rows true
:dashboard_card_id nil}
{:id (u/the-id card-2)
:include_csv false
:include_xls false
+ :format_rows true
:dashboard_card_id nil}]
:channels [daily-email-channel]
:skip_if_empty false})
diff --git a/test/metabase/dashboard_subscription_test.clj b/test/metabase/dashboard_subscription_test.clj
index 7aa0e6a50c6d0bb5c553acccd35a487ae9c0f224..73a889e5acf0ce7541bf85c6c093c047e58ea6b6 100644
--- a/test/metabase/dashboard_subscription_test.clj
+++ b/test/metabase/dashboard_subscription_test.clj
@@ -891,7 +891,7 @@
(when (seq rows)
[(let [^java.io.ByteArrayOutputStream baos (java.io.ByteArrayOutputStream.)]
(with-open [os baos]
- (#'messages/stream-api-results-to-export-format :csv os result)
+ (#'messages/stream-api-results-to-export-format :csv true os result)
(let [output-string (.toString baos "UTF-8")]
{:type :attachment
:content-type :csv
diff --git a/test/metabase/models/pulse_test.clj b/test/metabase/models/pulse_test.clj
index 3ac74599963e1a74330c66ce9787f5a0680d984a..dbcf503eafeaa3c6d571264207480eea61789ad6 100644
--- a/test/metabase/models/pulse_test.clj
+++ b/test/metabase/models/pulse_test.clj
@@ -80,6 +80,7 @@
:display :table
:include_csv false
:include_xls false
+ :format_rows true
:dashboard_card_id nil
:dashboard_id nil
:parameter_mappings nil}]
@@ -166,6 +167,7 @@
:display :table
:include_csv false
:include_xls false
+ :format_rows true
:dashboard_card_id nil
:dashboard_id nil
:parameter_mappings nil}]})
@@ -256,6 +258,7 @@
:display :bar
:include_csv false
:include_xls false
+ :format_rows true
:dashboard_card_id nil
:dashboard_id nil
:parameter_mappings nil}
@@ -265,6 +268,7 @@
:display :table
:include_csv false
:include_xls false
+ :format_rows true
:dashboard_card_id nil
:dashboard_id nil
:parameter_mappings nil}]
diff --git a/test/metabase/query_processor/streaming/xlsx_test.clj b/test/metabase/query_processor/streaming/xlsx_test.clj
index 1f1584ef8dcbb50ebf0e7b2fd27b63d470ae02b9..ede84ea189ff7429e2cf23eb4b03df47bc7b06bc 100644
--- a/test/metabase/query_processor/streaming/xlsx_test.clj
+++ b/test/metabase/query_processor/streaming/xlsx_test.clj
@@ -579,17 +579,16 @@
(testing "LocalDateTime formatted as a string; should be parsed when *parse-temporal-string-values* is true"
(is (= ["2020-03-28T10:12:06.681"]
(second (xlsx-export [{:id 0, :name "Col"}] {} [["2020-03-28T10:12:06.681"]]))))
- (binding [qp.xlsx/*parse-temporal-string-values* true]
- (is (= [#inst "2020-03-28T10:12:06.681"]
- (second (xlsx-export [{:id 0, :name "Col" :effective_type :type/Temporal}]
- {}
- [["2020-03-28T10:12:06.681"]]))))
- (testing "Values that are parseable as dates are not when effective_type is not temporal (#29708)"
- (doseq [value ["0001" "4161" "02" "2020-03-28T10:12:06.681"]]
- (is (= [value]
- (second (xlsx-export [{:id 0, :name "Col" :effective_type :type/Text}]
- {}
- [[value]]))))))))
+ (is (= [#inst "2020-03-28T10:12:06.681"]
+ (second (xlsx-export [{:id 0, :name "Col" :effective_type :type/Temporal}]
+ {}
+ [["2020-03-28T10:12:06.681"]]))))
+ (testing "Values that are parseable as dates are not when effective_type is not temporal (#29708)"
+ (doseq [value ["0001" "4161" "02" "2020-03-28T10:12:06.681"]]
+ (is (= [value]
+ (second (xlsx-export [{:id 0, :name "Col" :effective_type :type/Text}]
+ {}
+ [[value]])))))))
(mt/with-metadata-provider (mt/id)
(binding [driver/*driver* :h2]
(testing "OffsetDateTime"
@@ -602,11 +601,10 @@
(is (= [#inst "2020-03-28T10:12:06.000-00:00"]
(second (xlsx-export [{:id 0, :name "Col"}] {} [[#t "2020-03-28T10:12:06Z"]])))))))
(testing "Strings representing country names/codes don't error when *parse-temporal-string-values* is true (#18724)"
- (binding [qp.xlsx/*parse-temporal-string-values* true]
- (is (= ["GB"]
- (second (xlsx-export [{:id 0, :name "Col"}] {} [["GB"]]))))
- (is (= ["Portugal"]
- (second (xlsx-export [{:id 0, :name "Col"}] {} [["Portugal"]]))))))
+ (is (= ["GB"]
+ (second (xlsx-export [{:id 0, :name "Col"}] {} [["GB"]]))))
+ (is (= ["Portugal"]
+ (second (xlsx-export [{:id 0, :name "Col"}] {} [["Portugal"]])))))
(testing "NaN and infinity values (#21343)"
;; These values apparently are represented as error codes, which are parsed here into keywords
(is (= [:NUM]