Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Embedded dashboard parameters parse runtime error #41545

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Fix Embedded dashboard parameters parse runtime error #41545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d609d77
Fix parameters parse
deniskaber Apr 17, 2024
d0f06e6
Add e2e test
deniskaber Apr 18, 2024
2955a3d
Add e2e test
deniskaber Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions e2e/test/scenarios/sharing/public-dashboard.cy.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,4 +245,16 @@ describe("scenarios > public > dashboard", () => {

assertDashboardFullWidth();
});

it("should render when a filter passed with value starting from '0' (metabase#41483)", () => {
cy.get("@dashboardId").then(id => {
visitPublicDashboard(id, {
params: { text: "002" },
});
});

cy.url().should("include", "text=002");

filterWidget().findByText("002").should("be.visible");
});
});
4 changes: 3 additions & 1 deletion frontend/src/metabase/lib/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import querystring from "querystring";

import { safeJsonParse } from "metabase/lib/utils";

function parseQueryStringOptions(s) {
const options = querystring.parse(s);

for (const name in options) {
if (options[name] === "") {
options[name] = true;
} else if (/^(true|false|-?\d+(\.\d+)?)$/.test(options[name])) {
options[name] = JSON.parse(options[name]);
options[name] = safeJsonParse(options[name]);
}
}

Expand Down
13 changes: 13 additions & 0 deletions frontend/src/metabase/lib/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,16 @@ export function compareVersions(
}

export const isEEBuild = () => PLUGIN_IS_EE_BUILD.isEEBuild();

export const safeJsonParse = (value: string | null | undefined) => {
if (!value) {
return null;
}

try {
return JSON.parse(value);
} catch (e) {
console.error("Unable to parse JSON: ", value, e);
return null;
}
};
10 changes: 10 additions & 0 deletions frontend/src/metabase/public/containers/PublicDashboard/PublicDashboard.unit.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@ describe("PublicDashboard", () => {

expect(firstTab).toHaveAttribute("aria-selected", "true");
});

it("should render when a filter passed with value starting from '0' (metabase#41483)", async () => {
// note: as all slugs this is ignored and we only use the id
await setup({
queryString: "?my-filter-value=01",
});

// should not throw runtime error and render dashboard content
expect(screen.getByText(DASHBOARD_TITLE)).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only verifies that the dashboard is rendered not crashed. But it doesn't assert that the filter value is retained.

I've tried a few iterations to make this unit test work by passing dashcards and parameters but couldn't do that. Maybe you could try or create an E2E if that's easier. I think it's important to capture that 01 would still be present and used as a filter.

At first I thought this code wasn't fixing the issue because there's also no filter value assertion in the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behaviour customers expect is that the filter will have value 1, should we make an expect on that too?
I think we'll need to so some more mocking to make the filter show up in the UI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get 01 in the URL, customer needs to enter 01 in the filter. If the initial result shows as 01 then I'd expect that he might expect it to still show 01 (like what it initially was before refreshing the page)
image

});
});

async function setup({
Expand Down