From f4f71ea7bdb7669bbabc06b6855ce599c8c9a7ba Mon Sep 17 00:00:00 2001 From: Anton Kulyk <kuliks.anton@gmail.com> Date: Thu, 18 Jul 2024 11:30:57 +0100 Subject: [PATCH] Clean up dashboard URL management (#44639) * Fix falsy tabSelected/tabRenamed checks * Update URL management (WIP) * Fix tests failing because of whitespace changes * Fix tests failing because of parameter order * Use one hook for parameters and tab URL management * Fix tests failing because of parameter order * Fix test failing because of whitespace changes * Revert * Fix DashboardTabs tests * Fix tests failing because of parameter order * Fix test * Fix test failing because of whitespace changes * Remove unit tests that should be e2e tests now * Fix tab navigation * Fix string encoding/decoding * Revert test changes * Consistent question search params * Update DashboardTabs tests * Fix `SyncedParametersList` * Fix e2e tests * Fix nav usage * Fix types * Remove `useSyncedQueryString` * Fix test * Move slug utils to dashboard utils * Fix nav * Revert nav change * Fix e2e tests * Fix public/embedded dashboards, handle site url * Add repro test * Bring back and fix `PublicOrEmbeddedDashboardPage` tests * Move `SyncedParametersList` to qb --- .eslintrc.js | 2 +- e2e/support/helpers/e2e-dashboard-helpers.js | 2 +- .../dashboard-cards/click-behavior.cy.spec.js | 91 ++++++++++-- .../dashboard-drill.cy.spec.js | 2 +- ...dashboard-filters-explicit-join.cy.spec.js | 2 +- ...dashboard-filters-text-category.cy.spec.js | 8 +- .../dashboard-filters/parameters.cy.spec.js | 8 +- .../embedding/embedding-dashboard.cy.spec.js | 2 +- .../embedding-linked-filters.cy.spec.js | 20 +-- ...dashboard-filters-reproductions.cy.spec.js | 4 +- .../DashboardParameterList.tsx | 20 +-- .../DashboardTabs/DashboardTabs.unit.spec.tsx | 30 +++- .../DashboardTabs/SyncedDashboardTabs.tsx | 31 ---- .../DashboardTabs/use-sync-url-slug.ts | 140 ------------------ .../use-sync-url-slug.unit.spec.ts | 46 ------ .../containers/AutomaticDashboardApp.jsx | 20 ++- .../containers/DashboardApp/DashboardApp.tsx | 15 +- .../src/metabase/dashboard/hooks/index.ts | 1 + .../hooks/use-dashboard-url-query.ts | 138 +++++++++++++++++ frontend/src/metabase/dashboard/selectors.ts | 68 ++++++++- frontend/src/metabase/dashboard/utils.ts | 23 +++ .../src/metabase/dashboard/utils.unit.spec.ts | 45 ++++++ .../metabase/hooks/use-synced-query-string.ts | 52 ------- frontend/src/metabase/lib/urls/utils.ts | 35 +---- .../ParametersList/SyncedParametersList.tsx | 55 ------- .../components/ParametersList/index.ts | 4 +- .../components/EmbedFrame/EmbedFrame.tsx | 27 +++- .../PublicOrEmbeddedDashboardPage.tsx | 8 +- ...ublicOrEmbeddedDashboardPage.unit.spec.tsx | 14 +- .../query_builder/actions/downloading.ts | 7 +- .../__mocks__/NativeQueryEditor.jsx | 2 +- .../ResponsiveParametersList.styled.tsx | 2 +- .../components/SyncedParametersList.tsx | 95 ++++++++++++ .../components/view/View.styled.tsx | 2 +- frontend/src/metabase/selectors/web-app.ts | 17 +++ frontend/test/__support__/ui.tsx | 2 +- 36 files changed, 595 insertions(+), 445 deletions(-) delete mode 100644 frontend/src/metabase/dashboard/components/DashboardTabs/SyncedDashboardTabs.tsx delete mode 100644 frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.ts delete mode 100644 frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.unit.spec.ts create mode 100644 frontend/src/metabase/dashboard/hooks/use-dashboard-url-query.ts delete mode 100644 frontend/src/metabase/hooks/use-synced-query-string.ts delete mode 100644 frontend/src/metabase/parameters/components/ParametersList/SyncedParametersList.tsx create mode 100644 frontend/src/metabase/query_builder/components/SyncedParametersList.tsx create mode 100644 frontend/src/metabase/selectors/web-app.ts diff --git a/.eslintrc.js b/.eslintrc.js index ba9a9b3a0fc..d68e2550835 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -78,7 +78,7 @@ module.exports = { "react/forbid-component-props": [2, { forbid: ["sx"] }], "react-hooks/exhaustive-deps": [ "warn", - { additionalHooks: "(useSyncedQueryString|useSafeAsyncFunction)" }, + { additionalHooks: "(useSafeAsyncFunction)" }, ], "prefer-const": [1, { destructuring: "all" }], "no-useless-escape": 0, diff --git a/e2e/support/helpers/e2e-dashboard-helpers.js b/e2e/support/helpers/e2e-dashboard-helpers.js index 3a23169b680..29b9ea67f4d 100644 --- a/e2e/support/helpers/e2e-dashboard-helpers.js +++ b/e2e/support/helpers/e2e-dashboard-helpers.js @@ -472,7 +472,7 @@ export function assertDashboardFullWidth() { } export function createDashboardWithTabs({ - dashcards, + dashcards = [], tabs, ...dashboardDetails }) { diff --git a/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js b/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js index eca135daefe..661205c20a4 100644 --- a/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js +++ b/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js @@ -453,11 +453,9 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { cy.get("@targetDashboardId").then(targetDashboardId => { cy.location().should(({ pathname, search }) => { expect(pathname).to.equal(`/dashboard/${targetDashboardId}`); - expect(search).to.equal( - `?tab=${TAB_SLUG_MAP[SECOND_TAB.name]}&${ - DASHBOARD_FILTER_TEXT.slug - }=${POINT_COUNT}`, - ); + const tabParam = `tab=${TAB_SLUG_MAP[SECOND_TAB.name]}`; + const textFilterParam = `${DASHBOARD_FILTER_TEXT.slug}=${POINT_COUNT}`; + expect(search).to.equal(`?${textFilterParam}&${tabParam}`); }); }); }); @@ -1324,10 +1322,11 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { cy.get("@targetDashboardId").then(targetDashboardId => { cy.location().should(({ pathname, search }) => { expect(pathname).to.equal(`/dashboard/${targetDashboardId}`); + const tabParam = `tab=${TAB_SLUG_MAP[SECOND_TAB.name]}`; + const textFilterParam = `${DASHBOARD_FILTER_TEXT.slug}=${POINT_COUNT}`; + const timeFilterParam = `${DASHBOARD_FILTER_TIME.slug}=`; expect(search).to.equal( - `?tab=${TAB_SLUG_MAP[SECOND_TAB.name]}&${ - DASHBOARD_FILTER_TEXT.slug - }=${POINT_COUNT}&${DASHBOARD_FILTER_TIME.slug}=`, + `?${textFilterParam}&${timeFilterParam}&${tabParam}`, ); }); }); @@ -1867,7 +1866,7 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { cy.location().should(({ pathname, search }) => { expect(pathname).to.equal(`/dashboard/${targetDashboardId}`); expect(search).to.equal( - `?tab=${TAB_SLUG_MAP[TAB_2.name]}&${DASHBOARD_FILTER_TEXT.slug}=${1}`, + `?${DASHBOARD_FILTER_TEXT.slug}=${1}&tab=${TAB_SLUG_MAP[TAB_2.name]}`, ); }); }); @@ -2110,6 +2109,80 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { }); }); }); + + it("should navigate to correct dashboard tab via custom destination click behavior (metabase#34447 metabase#44106)", () => { + createDashboardWithTabs({ + name: TARGET_DASHBOARD.name, + tabs: [ + { + id: -1, + name: "first-tab", + }, + { + id: -2, + name: "second-tab", + }, + ], + }).then(targetDashboard => { + const baseClickBehavior = { + type: "link", + linkType: "dashboard", + targetId: targetDashboard.id, + parameterMapping: {}, + }; + + const [firstTab, secondTab] = targetDashboard.tabs; + + cy.createDashboard({ + dashcards: [ + createMockDashboardCard({ + id: -1, + card_id: ORDERS_QUESTION_ID, + size_x: 12, + size_y: 6, + visualization_settings: { + click_behavior: { + ...baseClickBehavior, + tabId: firstTab.id, + }, + }, + }), + createMockDashboardCard({ + id: -2, + card_id: ORDERS_QUESTION_ID, + size_x: 12, + size_y: 6, + visualization_settings: { + click_behavior: { + ...baseClickBehavior, + tabId: secondTab.id, + }, + }, + }), + ], + }).then(({ body: dashboard }) => { + visitDashboard(dashboard.id); + + getDashboardCard(1).findByText("14").click(); + cy.location("pathname").should( + "eq", + `/dashboard/${targetDashboard.id}`, + ); + cy.location("search").should("eq", `?tab=${secondTab.id}-second-tab`); + + cy.go("back"); + cy.location("pathname").should("eq", `/dashboard/${dashboard.id}`); + cy.location("search").should("eq", ""); + + getDashboardCard(0).findByText("14").click(); + cy.location("pathname").should( + "eq", + `/dashboard/${targetDashboard.id}`, + ); + cy.location("search").should("eq", `?tab=${firstTab.id}-first-tab`); + }); + }); + }); }); /** diff --git a/e2e/test/scenarios/dashboard-cards/dashboard-drill.cy.spec.js b/e2e/test/scenarios/dashboard-cards/dashboard-drill.cy.spec.js index 5bab9ffa4c8..d847d58fff5 100644 --- a/e2e/test/scenarios/dashboard-cards/dashboard-drill.cy.spec.js +++ b/e2e/test/scenarios/dashboard-cards/dashboard-drill.cy.spec.js @@ -314,7 +314,7 @@ describe("scenarios > dashboard > dashboard drill", () => { cy.get("fieldset").should("contain", "Aaron Hand"); cy.location("pathname").should("eq", `/dashboard/${dashboardId}`); - cy.location("search").should("eq", "?my_param=Aaron%20Hand"); + cy.location("search").should("eq", "?my_param=Aaron+Hand"); }); }); diff --git a/e2e/test/scenarios/dashboard-filters/dashboard-filters-explicit-join.cy.spec.js b/e2e/test/scenarios/dashboard-filters/dashboard-filters-explicit-join.cy.spec.js index 60b23c1131f..58c7553f7ca 100644 --- a/e2e/test/scenarios/dashboard-filters/dashboard-filters-explicit-join.cy.spec.js +++ b/e2e/test/scenarios/dashboard-filters/dashboard-filters-explicit-join.cy.spec.js @@ -88,7 +88,7 @@ describe("scenarios > dashboard > filters", () => { cy.location("search").should( "eq", - "?text=Awesome%20Concrete%20Shoes&text=Awesome%20Iron%20Hat", + "?text=Awesome+Concrete+Shoes&text=Awesome+Iron+Hat", ); filterWidget().contains("2 selections"); diff --git a/e2e/test/scenarios/dashboard-filters/dashboard-filters-text-category.cy.spec.js b/e2e/test/scenarios/dashboard-filters/dashboard-filters-text-category.cy.spec.js index 90fb59fe612..2496e7560b4 100644 --- a/e2e/test/scenarios/dashboard-filters/dashboard-filters-text-category.cy.spec.js +++ b/e2e/test/scenarios/dashboard-filters/dashboard-filters-text-category.cy.spec.js @@ -167,7 +167,7 @@ describe("scenarios > dashboard > filters > text/category", () => { saveDashboard(); waitDashboardCardQuery(); - cy.location("search").should("eq", "?text=Organic&id="); + cy.location("search").should("eq", "?id=&text=Organic"); cy.findByTestId("dashcard").contains("39.58"); // This part reproduces metabase#13960 @@ -175,19 +175,19 @@ describe("scenarios > dashboard > filters > text/category", () => { cy.get("fieldset .Icon-close").click(); waitDashboardCardQuery(); - cy.location("search").should("eq", "?text=&id="); + cy.location("search").should("eq", "?id=&text="); filterWidget().contains("ID").click(); cy.findByPlaceholderText("Enter an ID").type("4{enter}").blur(); cy.button("Add filter").click(); waitDashboardCardQuery(); - cy.location("search").should("eq", "?text=&id=4"); + cy.location("search").should("eq", "?id=4&text="); cy.reload(); waitDashboardCardQuery(); - cy.location("search").should("eq", "?text=&id=4"); + cy.location("search").should("eq", "?id=4&text="); filterWidget().contains("Text"); filterWidget().contains("Arnold Adams"); }); diff --git a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js index 4a6c3d05bff..5dfdeca4ce5 100644 --- a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js +++ b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js @@ -205,7 +205,7 @@ describe("scenarios > dashboard > parameters", () => { cy.location("search").should( "eq", - `?${startsWith.slug}=G&${endsWith.slug}=`, + `?${endsWith.slug}=&${startsWith.slug}=G`, ); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("37.65").should("not.exist"); @@ -223,7 +223,7 @@ describe("scenarios > dashboard > parameters", () => { cy.location("search").should( "eq", - `?${startsWith.slug}=G&${endsWith.slug}=zmo`, + `?${endsWith.slug}=zmo&${startsWith.slug}=G`, ); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText("52.72").should("not.exist"); @@ -488,7 +488,7 @@ describe("scenarios > dashboard > parameters", () => { cy.location("search").should( "eq", - "?title=Awesome%20Concrete%20Shoes&category=Widget&vendor=McClure-Lockman", + "?category=Widget&title=Awesome+Concrete+Shoes&vendor=McClure-Lockman", ); cy.findAllByTestId("table-row").should("have.length", 1); @@ -501,7 +501,7 @@ describe("scenarios > dashboard > parameters", () => { cy.location("search").should( "eq", - "?title=Awesome%20Concrete%20Shoes&category=Widget&vendor=McClure-Lockman", + "?category=Widget&title=Awesome+Concrete+Shoes&vendor=McClure-Lockman", ); cy.findAllByTestId("table-row").should("have.length", 1); }); diff --git a/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js b/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js index bb220a0524d..1d81167e3e0 100644 --- a/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js +++ b/e2e/test/scenarios/embedding/embedding-dashboard.cy.spec.js @@ -253,7 +253,7 @@ describe("scenarios > embedding > dashboard parameters", () => { // Filter widget must be visible filterWidget().contains("Name"); // Its default value must be in the URL - cy.location("search").should("contain", "name=Ferne%20Rosenbaum"); + cy.location("search").should("contain", "name=Ferne+Rosenbaum"); // And the default should be applied giving us only 1 result cy.findByTestId("scalar-value").invoke("text").should("eq", "1"); }); diff --git a/e2e/test/scenarios/embedding/embedding-linked-filters.cy.spec.js b/e2e/test/scenarios/embedding/embedding-linked-filters.cy.spec.js index e6efb35f1f9..1544cff5017 100644 --- a/e2e/test/scenarios/embedding/embedding-linked-filters.cy.spec.js +++ b/e2e/test/scenarios/embedding/embedding-linked-filters.cy.spec.js @@ -76,7 +76,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Add filter").click(); }); - cy.location("search").should("eq", "?state=AK&city="); + cy.location("search").should("eq", "?city=&state=AK"); echartsContainer() .get("text") @@ -108,7 +108,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Add filter").click(); }); - cy.location("search").should("eq", "?state=AK&city=Anchorage"); + cy.location("search").should("eq", "?city=Anchorage&state=AK"); chartPathWithFillColor("#509EE3").should("have.length", 1).realHover(); @@ -155,7 +155,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Apply").should("be.visible").click(); cy.button("Apply").should("not.exist"); - cy.location("search").should("eq", "?state=AK&city="); + cy.location("search").should("eq", "?city=&state=AK"); echartsContainer() .get("text") @@ -190,7 +190,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Apply").should("be.visible").click(); cy.button("Apply").should("not.exist"); - cy.location("search").should("eq", "?state=AK&city=Anchorage"); + cy.location("search").should("eq", "?city=Anchorage&state=AK"); chartPathWithFillColor("#509EE3").should("have.length", 1).realHover(); @@ -239,7 +239,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Add filter").click(); }); - cy.location("search").should("eq", "?state=AK&city=Anchorage"); + cy.location("search").should("eq", "?city=Anchorage&state=AK"); chartPathWithFillColor("#509EE3").should("have.length", 1).realHover(); @@ -287,7 +287,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Add filter").click(); }); - cy.location("search").should("eq", "?state=AK&city=Anchorage"); + cy.location("search").should("eq", "?city=Anchorage&state=AK"); chartPathWithFillColor("#509EE3").should("have.length", 1).realHover(); @@ -367,7 +367,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me }); // ID filter already comes with the default value - cy.location("search").should("eq", "?id_filter=1&category="); + cy.location("search").should("eq", "?category=&id_filter=1"); // But it should still be editable, and that's why we see two filter widgets filterWidget().should("have.length", 2).contains("Category").click(); @@ -380,7 +380,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Add filter").click(); }); - cy.location("search").should("eq", "?id_filter=1&category=Gizmo"); + cy.location("search").should("eq", "?category=Gizmo&id_filter=1"); cy.findByTestId("table-row") .should("have.length", 1) @@ -410,7 +410,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.button("Add filter").click(); }); - cy.location("search").should("eq", "?id_filter=4&category=Doohickey"); + cy.location("search").should("eq", "?category=Doohickey&id_filter=4"); cy.findByTestId("table-row") .should("have.length", 1) @@ -419,7 +419,7 @@ describe("scenarios > embedding > dashboard > linked filters (metabase#13639, me cy.log("Make sure we can set multiple values"); cy.window().then( win => - (win.location.search = "?id_filter=4&id_filter=29&category=Widget"), + (win.location.search = "?category=Widget&id_filter=4&id_filter=29"), ); filterWidget() diff --git a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js index b07b2f681a3..f43ce195dba 100644 --- a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js +++ b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js @@ -555,7 +555,7 @@ describe("issues 15119 and 16112", () => { cy.button("Add filter").click(); cy.findByTestId("dashcard-container").should("contain", "adam"); - cy.location("search").should("eq", "?reviewer=adam&rating="); + cy.location("search").should("eq", "?rating=&reviewer=adam"); // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage cy.findByText(ratingFilter.name).click(); @@ -565,7 +565,7 @@ describe("issues 15119 and 16112", () => { cy.findByTestId("dashcard-container").should("contain", "adam"); cy.findByTestId("dashcard-container").should("contain", "5"); - cy.location("search").should("eq", "?reviewer=adam&rating=5"); + cy.location("search").should("eq", "?rating=5&reviewer=adam"); }); }); diff --git a/frontend/src/metabase/dashboard/components/DashboardParameterList/DashboardParameterList.tsx b/frontend/src/metabase/dashboard/components/DashboardParameterList/DashboardParameterList.tsx index 860d27628cb..208d4f1871a 100644 --- a/frontend/src/metabase/dashboard/components/DashboardParameterList/DashboardParameterList.tsx +++ b/frontend/src/metabase/dashboard/components/DashboardParameterList/DashboardParameterList.tsx @@ -6,19 +6,15 @@ import { } from "metabase/dashboard/actions"; import { getDashboardComplete, - getDraftParameterValues, getEditingParameter, getHiddenParameterSlugs, - getIsAutoApplyFilters, getIsEditing, getIsNightMode, - getParameters, - getParameterValues, + getValuePopulatedParameters, } from "metabase/dashboard/selectors"; import { useDispatch, useSelector } from "metabase/lib/redux"; -import { getValuePopulatedParameters } from "metabase-lib/v1/parameters/utils/parameter-values"; -import { SyncedParametersList } from "../../../parameters/components/ParametersList"; +import { ParametersList } from "../../../parameters/components/ParametersList"; interface DashboardParameterListProps { isFullscreen: boolean; @@ -28,23 +24,17 @@ export function DashboardParameterList({ isFullscreen, }: DashboardParameterListProps) { const dashboard = useSelector(getDashboardComplete); - const parameters = useSelector(getParameters); - const parameterValues = useSelector(getParameterValues); - const draftParameterValues = useSelector(getDraftParameterValues); + const parameters = useSelector(getValuePopulatedParameters); const editingParameter = useSelector(getEditingParameter); const hiddenParameterSlugs = useSelector(getHiddenParameterSlugs); const isEditing = useSelector(getIsEditing); - const isAutoApplyFilters = useSelector(getIsAutoApplyFilters); const isNightMode = useSelector(getIsNightMode); const shouldRenderAsNightMode = isNightMode && isFullscreen; const dispatch = useDispatch(); return ( - <SyncedParametersList - parameters={getValuePopulatedParameters({ - parameters, - values: isAutoApplyFilters ? parameterValues : draftParameterValues, - })} + <ParametersList + parameters={parameters} editingParameter={editingParameter} hideParameters={hiddenParameterSlugs} dashboard={dashboard} diff --git a/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx b/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx index 6c21d611992..0cd418c82d5 100644 --- a/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx +++ b/frontend/src/metabase/dashboard/components/DashboardTabs/DashboardTabs.unit.spec.tsx @@ -1,11 +1,13 @@ import userEvent from "@testing-library/user-event"; import type { Location } from "history"; -import { Link, Route, withRouter } from "react-router"; +import { type InjectedRouter, Link, Route, withRouter } from "react-router"; import { renderWithProviders, screen } from "__support__/ui"; import { INPUT_WRAPPER_TEST_ID } from "metabase/core/components/TabButton"; import { getDefaultTab, resetTempTabId } from "metabase/dashboard/actions"; +import { useDashboardUrlQuery } from "metabase/dashboard/hooks/use-dashboard-url-query"; import { getSelectedTabId } from "metabase/dashboard/selectors"; +import { createTabSlug } from "metabase/dashboard/utils"; import { useSelector } from "metabase/lib/redux"; import type { DashboardTab } from "metabase-types/api"; import type { DashboardState, State } from "metabase-types/store"; @@ -13,7 +15,6 @@ import type { DashboardState, State } from "metabase-types/store"; import { DashboardTabs } from "./DashboardTabs"; import { TEST_DASHBOARD_STATE } from "./test-utils"; import { useDashboardTabs } from "./use-dashboard-tabs"; -import { getSlug, useSyncURLSlug } from "./use-sync-url-slug"; function setup({ tabs, @@ -37,8 +38,7 @@ function setup({ const RoutedDashboardComponent = withRouter( ({ location }: { location: Location }) => { const { selectedTabId } = useDashboardTabs({ dashboardId: 1 }); - useSyncURLSlug({ location }); - + useDashboardUrlQuery(createMockRouter(), location); return ( <> <DashboardTabs dashboardId={1} isEditing={isEditing} /> @@ -141,7 +141,23 @@ async function duplicateTab(num: number) { } async function findSlug({ tabId, name }: { tabId: number; name: string }) { - return screen.findByText(new RegExp(getSlug({ tabId, name }))); + return screen.findByText(new RegExp(createTabSlug({ id: tabId, name }))); +} + +function createMockRouter(): InjectedRouter { + return { + push: jest.fn(), + replace: jest.fn(), + go: jest.fn(), + goBack: jest.fn(), + goForward: jest.fn(), + setRouteLeaveHook: jest.fn(), + createPath: jest.fn(), + createHref: jest.fn(), + isActive: jest.fn(), + // @ts-expect-error missing type definition + listen: jest.fn().mockReturnValue(jest.fn()), + }; } describe("DashboardTabs", () => { @@ -191,7 +207,7 @@ describe("DashboardTabs", () => { it("should automatically select the tab in the slug if valid", async () => { setup({ isEditing: false, - slug: getSlug({ tabId: 2, name: "Tab 2" }), + slug: createTabSlug({ id: 2, name: "Tab 2" }), }); expect(await selectTab(2)).toHaveAttribute("aria-selected", "true"); @@ -204,7 +220,7 @@ describe("DashboardTabs", () => { it("should automatically select the first tab if slug is invalid", async () => { setup({ isEditing: false, - slug: getSlug({ tabId: 99, name: "A bad slug" }), + slug: createTabSlug({ id: 99, name: "A bad slug" }), }); expect(queryTab(1)).toHaveAttribute("aria-selected", "true"); diff --git a/frontend/src/metabase/dashboard/components/DashboardTabs/SyncedDashboardTabs.tsx b/frontend/src/metabase/dashboard/components/DashboardTabs/SyncedDashboardTabs.tsx deleted file mode 100644 index b4e6ebb6288..00000000000 --- a/frontend/src/metabase/dashboard/components/DashboardTabs/SyncedDashboardTabs.tsx +++ /dev/null @@ -1,31 +0,0 @@ -import type { Location } from "history"; -import { useMount } from "react-use"; - -import { initTabs } from "metabase/dashboard/actions"; -import type { DashboardTabsProps } from "metabase/dashboard/components/DashboardTabs/DashboardTabs"; -import { DashboardTabs } from "metabase/dashboard/components/DashboardTabs/DashboardTabs"; -import { useDispatch } from "metabase/lib/redux"; - -import { parseSlug, useSyncURLSlug } from "./use-sync-url-slug"; - -export const SyncedDashboardTabs = ({ - dashboardId, - location, - isEditing = false, - className, -}: DashboardTabsProps & { - location: Location; -}) => { - const dispatch = useDispatch(); - - useSyncURLSlug({ location }); - useMount(() => dispatch(initTabs({ slug: parseSlug({ location }) }))); - - return ( - <DashboardTabs - dashboardId={dashboardId} - isEditing={isEditing} - className={className} - /> - ); -}; diff --git a/frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.ts b/frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.ts deleted file mode 100644 index 16bbbf1a120..00000000000 --- a/frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.ts +++ /dev/null @@ -1,140 +0,0 @@ -import type { Location } from "history"; -import { useEffect, useState } from "react"; -import { push, replace } from "react-router-redux"; -import { usePrevious } from "react-use"; -import _ from "underscore"; - -import { getIdFromSlug, initTabs, selectTab } from "metabase/dashboard/actions"; -import { getSelectedTabId, getTabs } from "metabase/dashboard/selectors"; -import { useDispatch, useSelector } from "metabase/lib/redux"; -import type { SelectedTabId } from "metabase-types/store"; - -export function parseSlug({ location }: { location: Location }) { - const slug = location.query["tab"]; - if (typeof slug === "string" && slug.length > 0) { - return slug; - } - return undefined; -} - -export function getSlug({ - tabId, - name, -}: { - tabId: SelectedTabId; - name: string | undefined; -}) { - if (tabId === null || tabId < 0 || !name) { - return ""; - } - return [tabId, ...name.toLowerCase().split(" ")].join("-"); -} - -function useUpdateURLSlug({ location }: { location: Location }) { - const dispatch = useDispatch(); - - return { - updateURLSlug: ({ - slug, - shouldReplace = false, - }: { - slug: string; - shouldReplace?: boolean; - }) => { - const updater = shouldReplace ? replace : push; - - const newQuery = slug - ? { ...location.query, tab: slug } - : _.omit(location.query, "tab"); - dispatch(updater({ ...location, query: newQuery })); - }, - }; -} - -export function useSyncURLSlug({ location }: { location: Location }) { - const [tabInitialized, setTabInitialized] = useState(false); - - const slug = parseSlug({ location }); - const tabs = useSelector(getTabs); - const selectedTabId = useSelector(getSelectedTabId); - - const prevSlug = usePrevious(slug); - const prevTabs = usePrevious(tabs); - const prevSelectedTabId = usePrevious(selectedTabId); - - const dispatch = useDispatch(); - const { updateURLSlug } = useUpdateURLSlug({ location }); - - useEffect(() => { - if (!tabs || tabs.length === 0) { - return; - } - - const tabFromSlug = tabs.find(t => t.id === getIdFromSlug(slug)); - - // If the tabs haven't been loaded before, the tab data from slug exists, - // and the current tab differs from the slug. - const isTabFromSlugLoaded = - !prevTabs?.length && tabFromSlug && selectedTabId !== tabFromSlug.id; - - // Selects the tab once the tab data has been loaded. - if (isTabFromSlugLoaded) { - dispatch(selectTab({ tabId: tabFromSlug.id })); - - updateURLSlug({ - slug: getSlug({ - tabId: tabFromSlug.id, - name: tabFromSlug.name, - }), - }); - return; - } - - const slugChanged = slug && slug !== prevSlug; - if (slugChanged) { - dispatch(initTabs({ slug })); - const slugId = getIdFromSlug(slug); - const hasTabs = tabs.length > 0; - const isValidSlug = !!tabs.find(t => t.id === slugId); - if (hasTabs && !isValidSlug) { - const [tab] = tabs; - updateURLSlug({ slug: getSlug({ tabId: tab.id, name: tab.name }) }); - } - return; - } - - const tabSelected = selectedTabId !== prevSelectedTabId; - const tabRenamed = - tabs.find(t => t.id === selectedTabId)?.name !== - prevTabs?.find(t => t.id === selectedTabId)?.name; - const penultimateTabDeleted = tabs.length === 1 && prevTabs?.length === 2; - - if (tabSelected || tabRenamed || penultimateTabDeleted) { - const newSlug = - tabs.length <= 1 - ? "" - : getSlug({ - tabId: selectedTabId, - name: tabs.find(t => t.id === selectedTabId)?.name, - }); - updateURLSlug({ - slug: newSlug, - shouldReplace: !tabInitialized, - }); - - if (newSlug) { - setTabInitialized(true); - } - } - }, [ - tabInitialized, - slug, - selectedTabId, - tabs, - prevSlug, - prevSelectedTabId, - prevTabs, - dispatch, - updateURLSlug, - ]); -} diff --git a/frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.unit.spec.ts b/frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.unit.spec.ts deleted file mode 100644 index 365961a32e2..00000000000 --- a/frontend/src/metabase/dashboard/components/DashboardTabs/use-sync-url-slug.unit.spec.ts +++ /dev/null @@ -1,46 +0,0 @@ -import type { Location } from "history"; - -import { createMockLocation } from "metabase-types/store/mocks"; - -import { getSlug, parseSlug } from "./use-sync-url-slug"; - -function getMockLocation(slug: Location["query"][string]) { - return createMockLocation({ query: { tab: slug } }); -} - -describe("parseSlug", () => { - it("should return the slug from the location object if valid", () => { - const slug = "1-tab-name"; - expect(parseSlug({ location: getMockLocation(slug) })).toBe(slug); - }); - - it("should return undefined if the slug is invalid", () => { - expect(parseSlug({ location: getMockLocation(null) })).toBe(undefined); - expect(parseSlug({ location: getMockLocation(undefined) })).toBe(undefined); - expect(parseSlug({ location: getMockLocation("") })).toBe(undefined); - expect( - parseSlug({ - location: getMockLocation(["1-tab-name", "2-another-tab-name"]), - }), - ).toBe(undefined); - expect(parseSlug({ location: { ...getMockLocation(""), query: {} } })).toBe( - undefined, - ); - }); -}); - -describe("getSlug", () => { - it("should return a lower-cased, hyphenated concatenation of the tabId and name", () => { - expect(getSlug({ tabId: 1, name: "SoMe-TaB-NaMe" })).toEqual( - "1-some-tab-name", - ); - }); - - it("should return an empty string when tabId or name is invalid", () => { - expect(getSlug({ tabId: null, name: "SoMe-TaB-NaMe" })).toEqual(""); - expect(getSlug({ tabId: -1, name: "SoMe-TaB-NaMe" })).toEqual(""); - - expect(getSlug({ tabId: 1, name: "" })).toEqual(""); - expect(getSlug({ tabId: 1, name: undefined })).toEqual(""); - }); -}); diff --git a/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx b/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx index 9119bd88d10..02459c67fd1 100644 --- a/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx +++ b/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx @@ -12,7 +12,7 @@ import Button from "metabase/core/components/Button"; import Link from "metabase/core/components/Link"; import Tooltip from "metabase/core/components/Tooltip"; import CS from "metabase/css/core/index.css"; -import { SyncedDashboardTabs } from "metabase/dashboard/components/DashboardTabs/SyncedDashboardTabs"; +import { DashboardTabs } from "metabase/dashboard/components/DashboardTabs"; import { Dashboard } from "metabase/dashboard/containers/Dashboard"; import { DashboardData } from "metabase/dashboard/hoc/DashboardData"; import { getIsHeaderVisible, getTabs } from "metabase/dashboard/selectors"; @@ -23,12 +23,13 @@ import withToast from "metabase/hoc/Toast"; import * as MetabaseAnalytics from "metabase/lib/analytics"; import { color } from "metabase/lib/colors"; import * as Urls from "metabase/lib/urls"; -import { SyncedParametersList } from "metabase/parameters/components/ParametersList"; +import { ParametersList } from "metabase/parameters/components/ParametersList"; import { getMetadata } from "metabase/selectors/metadata"; import { Icon } from "metabase/ui"; import { getValuePopulatedParameters } from "metabase-lib/v1/parameters/utils/parameter-values"; import { FixedWidthContainer } from "../components/Dashboard/Dashboard.styled"; +import { useDashboardUrlQuery } from "../hooks/use-dashboard-url-query"; import { ItemContent, @@ -121,6 +122,10 @@ class AutomaticDashboardAppInner extends Component { "AutomaticDashboard--withSidebar": hasSidebar, })} > + <AutomaticDashboardQueryParamsSync + router={this.props.router} + location={this.props.location} + /> <div className="" style={{ marginRight: hasSidebar ? 346 : undefined }}> {isHeaderVisible && ( <div @@ -154,7 +159,7 @@ class AutomaticDashboardAppInner extends Component { </div> {this.props.tabs.length > 1 && ( <div className={cx(CS.wrapper, CS.flex, CS.alignCenter)}> - <SyncedDashboardTabs location={this.props.location} /> + <DashboardTabs dashboardId={dashboard.id} /> </div> )} </FixedWidthContainer> @@ -169,7 +174,7 @@ class AutomaticDashboardAppInner extends Component { data-testid="fixed-width-filters" isFixedWidth={dashboard?.width === "fixed"} > - <SyncedParametersList + <ParametersList className={CS.mt1} parameters={getValuePopulatedParameters({ parameters, @@ -299,3 +304,10 @@ const SuggestionsSidebar = ({ related }) => ( <SuggestionsList suggestions={related} /> </SidebarRoot> ); + +// Workaround until AutomaticDashboardApp is refactored to be a function component +// (or even better, merged/generalized with DashboardApp) +const AutomaticDashboardQueryParamsSync = ({ router, location }) => { + useDashboardUrlQuery(router, location); + return null; +}; diff --git a/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.tsx b/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.tsx index 829bfa53d61..71c86127f36 100644 --- a/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.tsx +++ b/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.tsx @@ -12,9 +12,9 @@ import _ from "underscore"; import { LeaveConfirmationModal } from "metabase/components/LeaveConfirmationModal"; import CS from "metabase/css/core/index.css"; import { Dashboard } from "metabase/dashboard/components/Dashboard/Dashboard"; -import { useSyncURLSlug } from "metabase/dashboard/components/DashboardTabs/use-sync-url-slug"; import { useDashboardUrlParams, + useDashboardUrlQuery, useRefreshDashboard, } from "metabase/dashboard/hooks"; import favicon from "metabase/hoc/Favicon"; @@ -109,8 +109,15 @@ type ReduxProps = ConnectedProps<typeof connector>; type DashboardAppProps = OwnProps & ReduxProps & WithRouterProps; const DashboardApp = (props: DashboardAppProps) => { - const { dashboard, isRunning, isLoadingComplete, isEditing, isDirty, route } = - props; + const { + dashboard, + isRunning, + isLoadingComplete, + isEditing, + isDirty, + route, + router, + } = props; const { documentTitle: _documentTitle, @@ -205,7 +212,7 @@ const DashboardApp = (props: DashboardAppProps) => { onRefreshPeriodChange, } = useDashboardUrlParams({ location, onRefresh: refreshDashboard }); - useSyncURLSlug({ location }); + useDashboardUrlQuery(router, location); return ( <div className={cx(CS.shrinkBelowContentSize, CS.fullHeight)}> diff --git a/frontend/src/metabase/dashboard/hooks/index.ts b/frontend/src/metabase/dashboard/hooks/index.ts index 31c3422921a..a6e0b64df5b 100644 --- a/frontend/src/metabase/dashboard/hooks/index.ts +++ b/frontend/src/metabase/dashboard/hooks/index.ts @@ -3,5 +3,6 @@ export * from "./use-dashboard-fullscreen"; export * from "./use-dashboard-refresh-period"; export * from "./use-embed-theme"; export * from "./use-dashboard-url-params"; +export * from "./use-dashboard-url-query"; export * from "./use-refresh-dashboard"; export * from "./use-location-sync"; diff --git a/frontend/src/metabase/dashboard/hooks/use-dashboard-url-query.ts b/frontend/src/metabase/dashboard/hooks/use-dashboard-url-query.ts new file mode 100644 index 00000000000..783e4436914 --- /dev/null +++ b/frontend/src/metabase/dashboard/hooks/use-dashboard-url-query.ts @@ -0,0 +1,138 @@ +import type { Location } from "history"; +import { useEffect, useMemo } from "react"; +import type { InjectedRouter } from "react-router"; +import { push, replace } from "react-router-redux"; +import { usePrevious } from "react-use"; +import _ from "underscore"; + +import { useSetting } from "metabase/common/hooks"; +import { IS_EMBED_PREVIEW } from "metabase/lib/embed"; +import { useDispatch, useSelector } from "metabase/lib/redux"; +import * as Urls from "metabase/lib/urls"; +import { getParameterValuesBySlug } from "metabase-lib/v1/parameters/utils/parameter-values"; + +import { selectTab } from "../actions"; +import { + getDashboard, + getValuePopulatedParameters, + getSelectedTab, + getTabs, +} from "../selectors"; +import { createTabSlug } from "../utils"; + +export function useDashboardUrlQuery( + router: InjectedRouter, + location: Location, +) { + const dashboardId = useSelector(state => getDashboard(state)?.id); + const tabs = useSelector(getTabs); + const selectedTab = useSelector(getSelectedTab); + const parameters = useSelector(getValuePopulatedParameters); + const siteUrl = useSetting("site-url"); + + const dispatch = useDispatch(); + + const parameterValuesBySlug = useMemo( + () => getParameterValuesBySlug(parameters), + [parameters], + ); + + const queryParams = useMemo(() => { + const queryParams = { ...parameterValuesBySlug }; + + const hasRealSelectedTab = selectedTab && selectedTab.id > 0; + if (hasRealSelectedTab && tabs.length > 1) { + queryParams.tab = createTabSlug(selectedTab); + } + + return queryParams; + }, [parameterValuesBySlug, tabs, selectedTab]); + + const previousQueryParams = usePrevious(queryParams); + + useEffect(() => { + /** + * We don't want to sync the query string to the URL because when previewing, + * this changes the URL of the iframe by appending the query string to the src. + * This causes the iframe to reload when changing the preview hash from appearance + * settings because now the base URL (including the query string) is different. + */ + if (IS_EMBED_PREVIEW) { + return; + } + + const pathname = location.pathname.replace(siteUrl, ""); + const isDashboardUrl = pathname.startsWith("/dashboard/"); + if (isDashboardUrl) { + const dashboardSlug = pathname.replace("/dashboard/", ""); + const dashboardUrlId = Urls.extractEntityId(dashboardSlug); + const isNavigationInProgress = dashboardId !== dashboardUrlId; + if (isNavigationInProgress) { + return; + } + } + + if (_.isEqual(previousQueryParams, queryParams)) { + return; + } + + const currentQuery = location?.query ?? {}; + + const nextQueryParams = toLocationQuery(queryParams); + const currentQueryParams = _.omit(currentQuery, ...QUERY_PARAMS_ALLOW_LIST); + + if (!_.isEqual(nextQueryParams, currentQueryParams)) { + const otherQueryParams = _.pick(currentQuery, ...QUERY_PARAMS_ALLOW_LIST); + const nextQuery = { ...otherQueryParams, ...nextQueryParams }; + + const isDashboardTabChange = + queryParams && + previousQueryParams?.tab && + queryParams.tab !== previousQueryParams.tab; + + const action = isDashboardTabChange ? push : replace; + dispatch(action({ ...location, query: nextQuery })); + } + }, [ + dashboardId, + queryParams, + previousQueryParams, + location, + siteUrl, + dispatch, + ]); + + useEffect(() => { + // @ts-expect-error missing type declaration + const unsubscribe = router.listen(nextLocation => { + const isSamePath = nextLocation.pathname === location.pathname; + if (!isSamePath) { + return; + } + + const currentTabId = parseTabId(location); + const nextTabId = parseTabId(nextLocation); + + if (nextTabId && currentTabId !== nextTabId) { + dispatch(selectTab({ tabId: nextTabId })); + } + }); + + return () => unsubscribe(); + }, [router, location, selectedTab, dispatch]); +} + +const QUERY_PARAMS_ALLOW_LIST = ["objectId"]; + +function parseTabId(location: Location) { + const slug = location.query?.tab; + if (typeof slug === "string" && slug.length > 0) { + const id = parseInt(slug, 10); + return Number.isSafeInteger(id) ? id : null; + } + return null; +} + +function toLocationQuery(object: Record<string, any>) { + return _.mapObject(object, value => (value == null ? "" : value)); +} diff --git a/frontend/src/metabase/dashboard/selectors.ts b/frontend/src/metabase/dashboard/selectors.ts index c6fba988e5b..0155c316a12 100644 --- a/frontend/src/metabase/dashboard/selectors.ts +++ b/frontend/src/metabase/dashboard/selectors.ts @@ -8,14 +8,20 @@ import { SIDEBAR_NAME, } from "metabase/dashboard/constants"; import { LOAD_COMPLETE_FAVICON } from "metabase/hoc/Favicon"; +import * as Urls from "metabase/lib/urls"; import { getDashboardUiParameters } from "metabase/parameters/utils/dashboards"; import { getParameterMappingOptions as _getParameterMappingOptions } from "metabase/parameters/utils/mapping-options"; import type { EmbeddingParameterVisibility } from "metabase/public/lib/types"; import { getEmbedOptions, getIsEmbedded } from "metabase/selectors/embed"; import { getMetadata } from "metabase/selectors/metadata"; +import { getSetting } from "metabase/selectors/settings"; +import { getIsWebApp } from "metabase/selectors/web-app"; import { mergeSettings } from "metabase/visualizations/lib/settings"; import Question from "metabase-lib/v1/Question"; -import { getParameterValuesBySlug } from "metabase-lib/v1/parameters/utils/parameter-values"; +import { + getParameterValuesBySlug, + getValuePopulatedParameters as _getValuePopulatedParameters, +} from "metabase-lib/v1/parameters/utils/parameter-values"; import type { Card, CardId, @@ -420,6 +426,21 @@ export const getParameters = createSelector( }, ); +export const getValuePopulatedParameters = createSelector( + [ + getParameters, + getParameterValues, + getDraftParameterValues, + getIsAutoApplyFilters, + ], + (parameters, parameterValues, draftParameterValues, isAutoApplyFilters) => { + return _getValuePopulatedParameters({ + parameters, + values: isAutoApplyFilters ? parameterValues : draftParameterValues, + }); + }, +); + export const getMissingRequiredParameters = createSelector( [getParameters], parameters => @@ -485,17 +506,54 @@ export const getTabs = createSelector([getDashboard], dashboard => { }); export const getSelectedTabId = createSelector( - [getDashboard, state => state.dashboard.selectedTabId], - (dashboard, selectedTabId) => { + [ + getIsWebApp, + state => getSetting(state, "site-url"), + getDashboard, + state => state.dashboard.selectedTabId, + ], + (isWebApp, siteUrl, dashboard, selectedTabId) => { if (dashboard && selectedTabId === null) { - return getInitialSelectedTabId(dashboard); + return getInitialSelectedTabId(dashboard, siteUrl, isWebApp); } return selectedTabId; }, ); -export function getInitialSelectedTabId(dashboard: Dashboard | StoreDashboard) { +export const getSelectedTab = createSelector( + [getDashboard, getSelectedTabId], + (dashboard, selectedTabId) => { + if (!dashboard || selectedTabId === null) { + return null; + } + return dashboard.tabs?.find(tab => tab.id === selectedTabId) || null; + }, +); + +export function getInitialSelectedTabId( + dashboard: Dashboard | StoreDashboard, + siteUrl: string, + isWebApp: boolean, +) { + const pathname = window.location.pathname.replace(siteUrl, ""); + const isDashboardUrl = pathname.includes("/dashboard/"); + + if (isDashboardUrl) { + const dashboardSlug = pathname.replace("/dashboard/", ""); + const dashboardUrlId = Urls.extractEntityId(dashboardSlug); + const isNavigationInProgress = dashboardUrlId !== dashboard.id; + if (!isNavigationInProgress || !isWebApp) { + const searchParams = new URLSearchParams(window.location.search); + const tabParam = searchParams.get("tab"); + const tabId = tabParam ? parseInt(tabParam, 10) : null; + const hasTab = dashboard.tabs?.find?.(tab => tab.id === tabId); + if (hasTab) { + return tabId; + } + } + } + return dashboard.tabs?.[0]?.id || null; } diff --git a/frontend/src/metabase/dashboard/utils.ts b/frontend/src/metabase/dashboard/utils.ts index 65cbb385fef..01086bbe7d5 100644 --- a/frontend/src/metabase/dashboard/utils.ts +++ b/frontend/src/metabase/dashboard/utils.ts @@ -1,3 +1,4 @@ +import type { Location } from "history"; import _ from "underscore"; import { IS_EMBED_PREVIEW } from "metabase/lib/embed"; @@ -345,3 +346,25 @@ export function createVirtualCard(display: VirtualCardDisplay): VirtualCard { export const isDashboardCacheable = ( dashboard: Dashboard, ): dashboard is CacheableDashboard => typeof dashboard.id !== "string"; + +export function parseTabSlug(location: Location) { + const slug = location.query?.tab; + if (typeof slug === "string" && slug.length > 0) { + const id = parseInt(slug, 10); + return Number.isSafeInteger(id) ? id : null; + } + return null; +} + +export function createTabSlug({ + id, + name, +}: { + id: SelectedTabId; + name: string | undefined; +}) { + if (id === null || id < 0 || !name) { + return ""; + } + return [id, ...name.toLowerCase().split(" ")].join("-"); +} diff --git a/frontend/src/metabase/dashboard/utils.unit.spec.ts b/frontend/src/metabase/dashboard/utils.unit.spec.ts index f48d1da2ca9..a1959257c8f 100644 --- a/frontend/src/metabase/dashboard/utils.unit.spec.ts +++ b/frontend/src/metabase/dashboard/utils.unit.spec.ts @@ -1,3 +1,5 @@ +import type { Location } from "history"; + import { fetchDataOrError, getCurrentTabDashboardCards, @@ -6,6 +8,8 @@ import { hasDatabaseActionsEnabled, isDashcardLoading, syncParametersAndEmbeddingParams, + parseTabSlug, + createTabSlug, } from "metabase/dashboard/utils"; import { SERVER_ERROR_TYPES } from "metabase/lib/errors"; import { @@ -16,6 +20,7 @@ import { createMockDataset, createMockDatasetData, } from "metabase-types/api/mocks"; +import { createMockLocation } from "metabase-types/store/mocks"; const ENABLED_ACTIONS_DATABASE = createMockDatabase({ id: 1, @@ -27,6 +32,10 @@ const DISABLED_ACTIONS_DATABASE = createMockDatabase({ }); const NO_ACTIONS_DATABASE = createMockDatabase({ id: 3 }); +function getMockLocationWithTab(slug: Location["query"][string]) { + return createMockLocation({ query: { tab: slug } }); +} + describe("Dashboard utils", () => { describe("fetchDataOrError()", () => { it("should return data on successful fetch", async () => { @@ -329,4 +338,40 @@ describe("Dashboard utils", () => { ]); }); }); + + describe("parseTabSlug", () => { + it("should return the tab ID from the location object if valid", () => { + expect(parseTabSlug(getMockLocationWithTab("1-tab-name"))).toBe(1); + }); + + it("should return null if the slug is invalid", () => { + expect(parseTabSlug(getMockLocationWithTab(null))).toBe(null); + expect(parseTabSlug(getMockLocationWithTab(undefined))).toBe(null); + expect(parseTabSlug(getMockLocationWithTab(""))).toBe(null); + expect( + parseTabSlug( + getMockLocationWithTab(["1-tab-name", "2-another-tab-name"]), + ), + ).toBe(null); + expect(parseTabSlug({ ...getMockLocationWithTab(""), query: {} })).toBe( + null, + ); + }); + }); + + describe("createTabSlug", () => { + it("should return a lower-cased, hyphenated concatenation of the tabId and name", () => { + expect(createTabSlug({ id: 1, name: "SoMe-TaB-NaMe" })).toEqual( + "1-some-tab-name", + ); + }); + + it("should return an empty string when tabId or name is invalid", () => { + expect(createTabSlug({ id: null, name: "SoMe-TaB-NaMe" })).toEqual(""); + expect(createTabSlug({ id: -1, name: "SoMe-TaB-NaMe" })).toEqual(""); + + expect(createTabSlug({ id: 1, name: "" })).toEqual(""); + expect(createTabSlug({ id: 1, name: undefined })).toEqual(""); + }); + }); }); diff --git a/frontend/src/metabase/hooks/use-synced-query-string.ts b/frontend/src/metabase/hooks/use-synced-query-string.ts deleted file mode 100644 index f35378edfb2..00000000000 --- a/frontend/src/metabase/hooks/use-synced-query-string.ts +++ /dev/null @@ -1,52 +0,0 @@ -import { useEffect } from "react"; - -import { IS_EMBED_PREVIEW } from "metabase/lib/embed"; -import { buildSearchString } from "metabase/lib/urls"; - -export function useSyncedQueryString(object: Record<string, any>) { - useEffect(() => { - /** - * We don't want to sync the query string to the URL because when previewing, - * this changes the URL of the iframe by appending the query string to the src. - * This causes the iframe to reload when changing the preview hash from appearance - * settings because now the base URL (including the query string) is different. - */ - if (IS_EMBED_PREVIEW) { - return; - } - const searchString = buildSearchString({ - object, - filterFn: containsAllowedParams, - }); - - if (searchString !== window.location.search) { - history.replaceState( - null, - document.title, - window.location.pathname + searchString + window.location.hash, - ); - } - - return () => { - // Remove every previously-synced keys from the query string when the component is unmounted. - // This is a workaround to clear the parameter list state when [SyncedParametersList] unmounts. - const searchString = buildSearchString({ - filterFn: key => !(key in object), - }); - - if (searchString !== window.location.search) { - history.replaceState( - null, - document.title, - window.location.pathname + searchString + window.location.hash, - ); - } - }; - }, [object]); -} - -const QUERY_PARAMS_ALLOW_LIST = ["objectId", "tab"]; - -const containsAllowedParams = (objectKey: string) => { - return QUERY_PARAMS_ALLOW_LIST.includes(objectKey); -}; diff --git a/frontend/src/metabase/lib/urls/utils.ts b/frontend/src/metabase/lib/urls/utils.ts index 470db1c0451..6ed1efdb962 100644 --- a/frontend/src/metabase/lib/urls/utils.ts +++ b/frontend/src/metabase/lib/urls/utils.ts @@ -1,5 +1,3 @@ -import querystring from "querystring"; - export function appendSlug(path: string | number, slug?: string) { return slug ? `${path}-${slug}` : String(path); } @@ -21,33 +19,12 @@ export function extractQueryParams(query: Record<string, unknown>) { } export function getEncodedUrlSearchParams(query: Record<string, unknown>) { - return new URLSearchParams( - extractQueryParams(query).map(([key, value]) => { + return extractQueryParams(query) + .map(([key, value]) => { if (value == null) { - return [key, ""]; + return `${key}=`; } - return [key, value]; - }), - ); -} - -export function buildSearchString({ - object = {}, - filterFn, -}: { - object?: Record<string, any>; - filterFn: (objectKey: string) => boolean; -}) { - const currentSearchParams = querystring.parse( - window.location.search.replace("?", ""), - ); - const filteredSearchParams = Object.fromEntries( - Object.entries(currentSearchParams).filter(entry => filterFn(entry[0])), - ); - - const search = querystring.stringify({ - ...filteredSearchParams, - ...object, - }); - return search ? `?${search}` : ""; + return `${key}=${encodeURIComponent(value)}`; + }) + .join("&"); } diff --git a/frontend/src/metabase/parameters/components/ParametersList/SyncedParametersList.tsx b/frontend/src/metabase/parameters/components/ParametersList/SyncedParametersList.tsx deleted file mode 100644 index 2d1a66d4cf6..00000000000 --- a/frontend/src/metabase/parameters/components/ParametersList/SyncedParametersList.tsx +++ /dev/null @@ -1,55 +0,0 @@ -import { useMemo } from "react"; - -import { useSyncedQueryString } from "metabase/hooks/use-synced-query-string"; -import type { ParametersListProps } from "metabase/parameters/components/ParametersList/types"; -import { getParameterValuesBySlug } from "metabase-lib/v1/parameters/utils/parameter-values"; - -import { ParametersList } from "./ParametersList"; - -export const SyncedParametersList = ({ - parameters, - editingParameter, - question, - dashboard, - - className, - hideParameters, - - isFullscreen, - isNightMode, - isEditing, - commitImmediately, - - setParameterValue, - setParameterIndex, - setEditingParameter, - setParameterValueToDefault, - enableParameterRequiredBehavior, -}: ParametersListProps) => { - const queryParams = useMemo( - () => getParameterValuesBySlug(parameters), - [parameters], - ); - - useSyncedQueryString(queryParams); - - return ( - <ParametersList - className={className} - parameters={parameters} - question={question} - dashboard={dashboard} - editingParameter={editingParameter} - isFullscreen={isFullscreen} - isNightMode={isNightMode} - hideParameters={hideParameters} - isEditing={isEditing} - commitImmediately={commitImmediately} - setParameterValue={setParameterValue} - setParameterIndex={setParameterIndex} - setEditingParameter={setEditingParameter} - setParameterValueToDefault={setParameterValueToDefault} - enableParameterRequiredBehavior={enableParameterRequiredBehavior} - /> - ); -}; diff --git a/frontend/src/metabase/parameters/components/ParametersList/index.ts b/frontend/src/metabase/parameters/components/ParametersList/index.ts index c12a2e473c1..0ead2340df4 100644 --- a/frontend/src/metabase/parameters/components/ParametersList/index.ts +++ b/frontend/src/metabase/parameters/components/ParametersList/index.ts @@ -1,2 +1,2 @@ -export { SyncedParametersList } from "./SyncedParametersList"; -export { ParametersList } from "./ParametersList"; +export * from "./ParametersList"; +export * from "./types"; diff --git a/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx b/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx index 913a7c2c81f..0133fe44118 100644 --- a/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx +++ b/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx @@ -15,11 +15,9 @@ import { DASHBOARD_PDF_EXPORT_ROOT_ID } from "metabase/dashboard/constants"; import { initializeIframeResizer, isSmallScreen } from "metabase/lib/dom"; import { useSelector } from "metabase/lib/redux"; import { FilterApplyButton } from "metabase/parameters/components/FilterApplyButton"; -import { - ParametersList, - SyncedParametersList, -} from "metabase/parameters/components/ParametersList"; +import { ParametersList } from "metabase/parameters/components/ParametersList"; import { getVisibleParameters } from "metabase/parameters/utils/ui"; +import { SyncedParametersList } from "metabase/query_builder/components/SyncedParametersList"; import { getIsEmbeddingSdk } from "metabase/selectors/embed"; import { getSetting } from "metabase/selectors/settings"; import { Box, Button, Icon } from "metabase/ui"; @@ -124,9 +122,10 @@ export const EmbedFrame = ({ state => !getSetting(state, "hide-embed-branding?"), ); - const ParametersListComponent = isEmbeddingSdk - ? ParametersList - : SyncedParametersList; + const ParametersListComponent = getParametersListComponent({ + isEmbeddingSdk, + isDashboard: !!dashboard, + }); const [hasFrameScroll, setHasFrameScroll] = useState(!isEmbeddingSdk); @@ -322,3 +321,17 @@ function useIsFiltersSticky() { return [isSticky, intersectionObserverTargetRef] as const; } + +function getParametersListComponent({ + isEmbeddingSdk, + isDashboard, +}: { + isEmbeddingSdk: boolean; + isDashboard: boolean; +}) { + if (isDashboard) { + // Dashboards manage parameters themselves + return ParametersList; + } + return isEmbeddingSdk ? ParametersList : SyncedParametersList; +} diff --git a/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.tsx b/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.tsx index 28c39f4c794..665edd233b6 100644 --- a/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.tsx +++ b/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.tsx @@ -1,10 +1,10 @@ import type { WithRouterProps } from "react-router"; -import { useSyncURLSlug } from "metabase/dashboard/components/DashboardTabs/use-sync-url-slug"; import { useDashboardUrlParams, useRefreshDashboard, } from "metabase/dashboard/hooks"; +import { useDashboardUrlQuery } from "metabase/dashboard/hooks/use-dashboard-url-query"; import { getDashboardComplete } from "metabase/dashboard/selectors"; import { SetTitle } from "metabase/hoc/Title"; import { useSelector } from "metabase/lib/redux"; @@ -13,7 +13,7 @@ import { PublicOrEmbeddedDashboard } from "./PublicOrEmbeddedDashboard"; import { usePublicDashboardEndpoints } from "./WithPublicDashboardEndpoints"; export const PublicOrEmbeddedDashboardPage = (props: WithRouterProps) => { - const { location } = props; + const { location, router } = props; const parameterQueryParams = props.location.query; const { dashboardId } = usePublicDashboardEndpoints(props); @@ -23,6 +23,8 @@ export const PublicOrEmbeddedDashboardPage = (props: WithRouterProps) => { parameterQueryParams, }); + useDashboardUrlQuery(router, location); + const { background, bordered, @@ -41,8 +43,6 @@ export const PublicOrEmbeddedDashboardPage = (props: WithRouterProps) => { font, } = useDashboardUrlParams({ location, onRefresh: refreshDashboard }); - useSyncURLSlug({ location }); - const dashboard = useSelector(getDashboardComplete); return ( diff --git a/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.unit.spec.tsx b/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.unit.spec.tsx index 17e9dae9520..b53bf4e561d 100644 --- a/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.unit.spec.tsx +++ b/frontend/src/metabase/public/containers/PublicOrEmbeddedDashboard/PublicOrEmbeddedDashboardPage.unit.spec.tsx @@ -156,7 +156,7 @@ describe("PublicOrEmbeddedDashboardPage", () => { async function setup({ hash, - queryString, + queryString = "", numberOfTabs = 1, }: { hash?: string; queryString?: string; numberOfTabs?: number } = {}) { const tabs: DashboardTab[] = []; @@ -191,6 +191,14 @@ async function setup({ setupEmbedDashboardEndpoints(MOCK_TOKEN, dashboard, dashcards); + const pathname = `/embed/dashboard/${MOCK_TOKEN}`; + const hashString = hash ? `#${hash}` : ""; + const href = `${pathname}${queryString}${hashString}`; + + // Setting initial window.location state, + // so it can be used by getInitialSelectedTabId + window.history.replaceState({}, "", href); + const view = renderWithProviders( <Route path="embed/dashboard/:token" @@ -199,9 +207,7 @@ async function setup({ { storeInitialState: createMockState(), withRouter: true, - initialRoute: `embed/dashboard/${MOCK_TOKEN}${ - queryString ? `?` + queryString : "" - }${hash ? "#" + hash : ""}`, + initialRoute: href, }, ); diff --git a/frontend/src/metabase/query_builder/actions/downloading.ts b/frontend/src/metabase/query_builder/actions/downloading.ts index 6d34600f76c..ef4fa04e076 100644 --- a/frontend/src/metabase/query_builder/actions/downloading.ts +++ b/frontend/src/metabase/query_builder/actions/downloading.ts @@ -31,7 +31,7 @@ interface DownloadQueryResultsParams { method: string; url: string; body?: Record<string, unknown>; - params?: URLSearchParams; + params?: URLSearchParams | string; } export const downloadQueryResults = @@ -161,7 +161,10 @@ const getDatasetParams = ({ }; }; -export function getDatasetDownloadUrl(url: string, params?: URLSearchParams) { +export function getDatasetDownloadUrl( + url: string, + params?: URLSearchParams | string, +) { url = url.replace(api.basename, ""); // make url relative if it's not if (params) { url += `?${params.toString()}`; diff --git a/frontend/src/metabase/query_builder/components/NativeQueryEditor/__mocks__/NativeQueryEditor.jsx b/frontend/src/metabase/query_builder/components/NativeQueryEditor/__mocks__/NativeQueryEditor.jsx index 28deccc0e71..76f1d21a08d 100644 --- a/frontend/src/metabase/query_builder/components/NativeQueryEditor/__mocks__/NativeQueryEditor.jsx +++ b/frontend/src/metabase/query_builder/components/NativeQueryEditor/__mocks__/NativeQueryEditor.jsx @@ -1,8 +1,8 @@ /* eslint-disable react/prop-types */ import CS from "metabase/css/core/index.css"; -import { SyncedParametersList } from "metabase/parameters/components/ParametersList"; import DataSourceSelectors from "metabase/query_builder/components/NativeQueryEditor/DataSourceSelectors"; import { ACE_ELEMENT_ID } from "metabase/query_builder/components/NativeQueryEditor/constants"; +import { SyncedParametersList } from "metabase/query_builder/components/SyncedParametersList"; const MockNativeQueryEditor = ({ canChangeDatabase = true, diff --git a/frontend/src/metabase/query_builder/components/ResponsiveParametersList.styled.tsx b/frontend/src/metabase/query_builder/components/ResponsiveParametersList.styled.tsx index eec63b4c1e0..9bfc9f0b0fb 100644 --- a/frontend/src/metabase/query_builder/components/ResponsiveParametersList.styled.tsx +++ b/frontend/src/metabase/query_builder/components/ResponsiveParametersList.styled.tsx @@ -3,7 +3,7 @@ import styled from "@emotion/styled"; import Button from "metabase/core/components/Button"; -import { SyncedParametersList } from "../../parameters/components/ParametersList"; +import { SyncedParametersList } from "./SyncedParametersList"; export const FilterButton = styled(Button)` color: var(--mb-color-brand); diff --git a/frontend/src/metabase/query_builder/components/SyncedParametersList.tsx b/frontend/src/metabase/query_builder/components/SyncedParametersList.tsx new file mode 100644 index 00000000000..da2bdfbd49a --- /dev/null +++ b/frontend/src/metabase/query_builder/components/SyncedParametersList.tsx @@ -0,0 +1,95 @@ +import querystring from "querystring"; +import { useEffect, useMemo } from "react"; +import _ from "underscore"; + +import { IS_EMBED_PREVIEW } from "metabase/lib/embed"; +import { + ParametersList, + type ParametersListProps, +} from "metabase/parameters/components/ParametersList"; +import { getParameterValuesBySlug } from "metabase-lib/v1/parameters/utils/parameter-values"; + +export const SyncedParametersList = ({ + parameters, + editingParameter, + question, + dashboard, + + className, + hideParameters, + + isFullscreen, + isNightMode, + isEditing, + commitImmediately, + + setParameterValue, + setParameterIndex, + setEditingParameter, + setParameterValueToDefault, + enableParameterRequiredBehavior, +}: ParametersListProps) => { + const queryParams = useMemo( + () => getParameterValuesBySlug(parameters), + [parameters], + ); + + useEffect(() => { + /** + * We don't want to sync the query string to the URL because when previewing, + * this changes the URL of the iframe by appending the query string to the src. + * This causes the iframe to reload when changing the preview hash from appearance + * settings because now the base URL (including the query string) is different. + */ + if (IS_EMBED_PREVIEW) { + return; + } + const searchString = buildSearchString(queryParams); + if (searchString !== window.location.search) { + window.history.replaceState( + null, + document.title, + window.location.pathname + searchString + window.location.hash, + ); + } + }, [queryParams]); + + return ( + <ParametersList + className={className} + parameters={parameters} + question={question} + dashboard={dashboard} + editingParameter={editingParameter} + isFullscreen={isFullscreen} + isNightMode={isNightMode} + hideParameters={hideParameters} + isEditing={isEditing} + commitImmediately={commitImmediately} + setParameterValue={setParameterValue} + setParameterIndex={setParameterIndex} + setEditingParameter={setEditingParameter} + setParameterValueToDefault={setParameterValueToDefault} + enableParameterRequiredBehavior={enableParameterRequiredBehavior} + /> + ); +}; + +const QUERY_PARAMS_ALLOW_LIST = ["objectId"]; + +function buildSearchString(object: Record<string, any>) { + const currentSearchParams = querystring.parse( + window.location.search.replace("?", ""), + ); + const filteredSearchParams = Object.fromEntries( + Object.entries(currentSearchParams).filter(entry => + QUERY_PARAMS_ALLOW_LIST.includes(entry[0]), + ), + ); + + const search = querystring.stringify({ + ...filteredSearchParams, + ...object, + }); + return search ? `?${search}` : ""; +} diff --git a/frontend/src/metabase/query_builder/components/view/View.styled.tsx b/frontend/src/metabase/query_builder/components/view/View.styled.tsx index 24c6154361f..5163cb76505 100644 --- a/frontend/src/metabase/query_builder/components/view/View.styled.tsx +++ b/frontend/src/metabase/query_builder/components/view/View.styled.tsx @@ -2,7 +2,7 @@ import { css } from "@emotion/react"; import styled from "@emotion/styled"; import DebouncedFrame from "metabase/components/DebouncedFrame"; -import { SyncedParametersList } from "metabase/parameters/components/ParametersList"; +import { SyncedParametersList } from "metabase/query_builder/components/SyncedParametersList"; import { breakpointMaxSmall } from "metabase/styled-components/theme/media-queries"; import { ViewTitleHeader } from "./ViewHeader"; diff --git a/frontend/src/metabase/selectors/web-app.ts b/frontend/src/metabase/selectors/web-app.ts new file mode 100644 index 00000000000..7f1c21bec00 --- /dev/null +++ b/frontend/src/metabase/selectors/web-app.ts @@ -0,0 +1,17 @@ +import { createSelector } from "reselect"; + +import { getIsEmbedded, getIsEmbeddingSdk } from "./embed"; +import { getSetting } from "./settings"; + +export const getIsWebApp = createSelector( + [state => getSetting(state, "site-url"), getIsEmbedded, getIsEmbeddingSdk], + (siteUrl, isEmbedded, isEmbeddingSdk) => { + const pathname = window.location.pathname.replace(siteUrl, ""); + return ( + !isEmbedded && + !isEmbeddingSdk && + !pathname.startsWith("/public/") && + !pathname.startsWith("/embed/") + ); + }, +); diff --git a/frontend/test/__support__/ui.tsx b/frontend/test/__support__/ui.tsx index 6b12c458720..e24fbf36a52 100644 --- a/frontend/test/__support__/ui.tsx +++ b/frontend/test/__support__/ui.tsx @@ -91,7 +91,7 @@ export function renderWithProviders( } // We need to call `useRouterHistory` to ensure the history has a `query` object, - // since some components and hooks like `use-sync-url-slug` rely on it to read/write query params. + // since some components and hooks rely on it to read/write query params. // eslint-disable-next-line react-hooks/rules-of-hooks const browserHistory = useRouterHistory(createMemoryHistory)({ entries: [initialRoute], -- GitLab