Skip to content

Commit

Permalink
Fix null values being applied to filters with click behavior custom U…
Browse files Browse the repository at this point in the history
…RL links (#40023)

* do not pass null values to link url

* update null check to include query builder nulls replaced with "(empty")

* add test to verify behavior

* replace magic string with constant

* fix breakout in cypress spec
  • Loading branch information
JesseSDevaney authored and Metabase bot committed Mar 19, 2024
1 parent c6d10cb commit 75ceaa9
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 0 deletions.
78 changes: 78 additions & 0 deletions e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2018,6 +2018,84 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => {
});
});
});

it("should not pass through null values to filters in custom url click behavior (metabase#25203)", () => {
const DASHBOARD_NAME = "Click Behavior Custom URL Dashboard";
const questionDetails = {
name: "Orders",
query: {
"source-table": ORDERS_ID,
aggregation: [
["sum", ["field", ORDERS.TOTAL, null]],
["sum", ["field", ORDERS.DISCOUNT, null]],
],
breakout: [["field", ORDERS.CREATED_AT, { "temporal-unit": "year" }]],
filter: ["=", ["field", ORDERS.USER_ID, null], 1],
},
display: "bar",
};

cy.createQuestionAndDashboard({
questionDetails,
dashboardDetails: {
name: DASHBOARD_NAME,
},
cardDetails: {
size_x: 16,
size_y: 8,
},
}).then(({ body: { dashboard_id } }) => {
cy.wrap(dashboard_id).as("targetDashboardId");
visitDashboard(dashboard_id);
});

editDashboard();

getDashboardCard().realHover().icon("click").click();
addUrlDestination();

modal().within(() => {
const urlInput = cy.findAllByRole("textbox").eq(0);

cy.get("@targetDashboardId").then(targetDashboardId => {
urlInput.type(
`http://localhost:4000/dashboard/${targetDashboardId}?discount={{sum_2}}&total={{sum}}`,
{
parseSpecialCharSequences: false,
},
);
});
cy.button("Done").click();
});

cy.get("aside").button("Done").click();

saveDashboard();

// test that normal values still work properly
getDashboardCard().within(() => {
cy.get(".bar").eq(2).click();
});
cy.get("@targetDashboardId").then(targetDashboardId => {
cy.location().should(({ pathname, search }) => {
expect(pathname).to.equal(`/dashboard/${targetDashboardId}`);
expect(search).to.equal(
"?discount=15.070632139056723&total=298.9195210424866",
);
});
});

// test that null and "empty"s do not get passed through
getDashboardCard().within(() => {
cy.get(".bar").eq(1).click();
});
cy.get("@targetDashboardId").then(targetDashboardId => {
cy.location().should(({ pathname, search }) => {
expect(pathname).to.equal(`/dashboard/${targetDashboardId}`);
expect(search).to.equal("?discount=&total=420.3189231596888");
});
});
});
});

/**
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/metabase/lib/formatting/link.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { isDate } from "metabase-lib/types/utils/isa";
import type { ParameterValueOrArray } from "metabase-types/api";
import type { DatasetColumn, RowValue } from "metabase-types/api/dataset";

import { NULL_DISPLAY_VALUE } from "../constants";

import { formatDateTimeForParameter } from "./date";

type Value = ParameterValueOrArray | RowValue | undefined;
Expand Down Expand Up @@ -48,6 +50,11 @@ export function renderLinkURLForClick(
data,
({ value, column }: TemplateForClickFormatFunctionParamsType) => {
const valueForLinkTemplate = formatValueForLinkTemplate(value, column);

if ([null, NULL_DISPLAY_VALUE].includes(valueForLinkTemplate)) {
return "";
}

return encodeURIComponent(valueForLinkTemplate);
},
);
Expand Down

0 comments on commit 75ceaa9

Please sign in to comment.