From 2aae0cdfedeeecc8d15ff9800eb4632d4ee0a039 Mon Sep 17 00:00:00 2001 From: Ryan Laurie <30528226+iethree@users.noreply.github.com> Date: Tue, 5 Dec 2023 09:02:09 -0700 Subject: [PATCH 01/19] Handle Release Notes for Release Candidates (#36378) --- .github/workflows/release.yml | 7 ++++++- release/README.md | 8 ++++++++ release/release-offline.ts | 15 ++++++++++++++- release/src/github.ts | 12 ++++++++---- release/src/release-notes.ts | 26 ++++++++++++++++++++++++++ 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index eea803ecc0f72..5914e1072ed2b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -596,10 +596,15 @@ jobs: uses: actions/github-script@v6 with: script: | # js - const { closeMilestone, openNextMilestones } = require('${{ github.workspace }}/release/dist/index.cjs'); + const { closeMilestone, openNextMilestones, isRCVersion } = require('${{ github.workspace }}/release/dist/index.cjs'); const version = '${{ inputs.version }}'; + if (isRCVersion(version)) { + console.log("This is a release candidate, skipping milestone management"); + return; + } + await closeMilestone({ github, owner: context.repo.owner, diff --git a/release/README.md b/release/README.md index 7968b2c9d2cc7..b3ed32e74642b 100644 --- a/release/README.md +++ b/release/README.md @@ -52,3 +52,11 @@ yarn release-offline v0.77.77 1234567890abcdef1234567890abcdef12345678 --publish ``` The order of the arguments matters, the script will yell at you if you put the flags in the wrong order. We could make this more robust, but it's probably not worth the effort since hopefully we won't ever have to use this :crossed_fingers:. + +## Utilities + +In case you want to preview release notes generation, or re-generate them after a release has been built, you can use this command. (the hash doesn't matter, it's just a placeholder) + +```sh +yarn release-offline v0.77.0 1234567890abcdef1234567890abcdef12345678 --changelog > changelog.log +``` diff --git a/release/release-offline.ts b/release/release-offline.ts index 37ebc6c8f2a6a..11abbe4fd490f 100644 --- a/release/release-offline.ts +++ b/release/release-offline.ts @@ -11,7 +11,6 @@ import "zx/globals"; import { $ } from "zx"; $.verbose = false; -import { latest } from "immer/dist/internal"; import { isValidVersionString, hasBeenReleased, @@ -24,6 +23,7 @@ import { closeMilestone, openNextMilestones, versionRequirements, + getChangelog, } from "./src"; const { @@ -469,6 +469,19 @@ async function updateMilestones() { await releaseNotes(); } + if (step === "changelog") { + // changelog preview only, doesn't publish anything + const { GITHUB_OWNER, GITHUB_REPO } = getGithubCredentials(); + const notes = await getChangelog({ + version, github, + owner: GITHUB_OWNER, + repo: GITHUB_REPO + }); + // eslint-disable-next-line no-console -- allows piping to a file + console.log(notes); + return; + } + if (step === "update-milestones") { await updateMilestones(); } diff --git a/release/src/github.ts b/release/src/github.ts index 0ac1146b910f3..e9a220d66a945 100644 --- a/release/src/github.ts +++ b/release/src/github.ts @@ -20,8 +20,11 @@ const findMilestone = async ({ state: "open", }); - // our milestones don't have the v prefix - const expectedMilestoneName = getOSSVersion(version).replace(/^v/, ""); + // our milestones don't have the v prefix or a .0 suffix + const expectedMilestoneName = getOSSVersion(version) + .replace(/^v/, "") + .replace(/-rc\d+$/i, "") // RC versions use the major version milestone + .replace(/\.0$/, ''); return milestones.data.find( (milestone: { title: string; number: number }) => @@ -83,14 +86,15 @@ export const getMilestoneIssues = async ({ return []; } - const milestone = await github.rest.issues.listForRepo({ + // we have to use paginate function or the issues will be truncated to 100 + const issues = await github.paginate(github.rest.issues.listForRepo, { owner, repo, milestone: milestoneId.toString(), state: "closed", }); - return (milestone?.data ?? []) as Issue[]; + return (issues ?? []) as Issue[]; }; // latest for the purposes of github release notes diff --git a/release/src/release-notes.ts b/release/src/release-notes.ts index d95619b94f719..215eaa424930d 100644 --- a/release/src/release-notes.ts +++ b/release/src/release-notes.ts @@ -110,3 +110,29 @@ export async function publishRelease({ return github.rest.repos.createRelease(payload); } + +export async function getChangelog({ + version, + owner, + repo, + github, +}: ReleaseProps) { + if (!isValidVersionString(version)) { + throw new Error(`Invalid version string: ${version}`); + } + const issues = await getMilestoneIssues({ version, github, owner, repo }); + + const bugFixes = issues.filter(isBugIssue); + const enhancements = issues.filter(issue => !isBugIssue(issue)); + + const notes = ` +## Enhancements +${enhancements?.map(formatIssue).join("\n") ?? ""} + + +## Bug fixes +${bugFixes?.map(formatIssue).join("\n") ?? ""} +`; + + return notes; +} From 2af32bbc73f25b5f53c4e92a49f92787965cf72d Mon Sep 17 00:00:00 2001 From: Oleg Gromov Date: Tue, 5 Dec 2023 16:27:49 +0000 Subject: [PATCH 02/19] Fix card filter connected to a field not causing an update (#36206) * Rename DashboardHeaderView to match filename * Fix non updating cards on filter mapping to a field * Clean up DashboardApp tests a bit * Clean up Dashboard.jsx cruft * Add Dashboard componentDidUpdate "unit tests" * Simplify setup code as per feedback * Add test for changing dashboard prop in componentDidMount * Change constant names * Fix optional chaining in Dashboard --- .../components/Dashboard/Dashboard.jsx | 175 +++++++---------- .../Dashboard/Dashboard.unit.spec.tsx | 183 ++++++++++++++++++ .../DashboardHeader/DashboardHeader.jsx | 4 +- .../DashboardHeader/DashboardHeaderView.tsx | 2 +- .../DashboardApp/DashboardApp.unit.spec.tsx | 46 ++--- 5 files changed, 274 insertions(+), 136 deletions(-) create mode 100644 frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx diff --git a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx index 0680d4a7bb9e1..fdb3c9605c419 100644 --- a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx +++ b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.jsx @@ -42,10 +42,6 @@ class DashboardInner extends Component { isSharing: false, }; - constructor(props) { - super(props); - } - static getDerivedStateFromProps({ parameters }, { parametersListLength }) { const visibleParameters = getVisibleParameters(parameters); return visibleParameters.length !== parametersListLength @@ -54,14 +50,14 @@ class DashboardInner extends Component { } async componentDidMount() { - await this.loadDashboard(this.props.dashboardId); + await this.loadDashboard(); } async componentDidUpdate(prevProps) { updateParametersWidgetStickiness(this); if (prevProps.dashboardId !== this.props.dashboardId) { - await this.loadDashboard(this.props.dashboardId); + await this.loadDashboard(); return; } @@ -73,6 +69,7 @@ class DashboardInner extends Component { if ( !_.isEqual(prevProps.parameterValues, this.props.parameterValues) || + !_.isEqual(prevProps.parameters, this.props.parameters) || (!prevProps.dashboard && this.props.dashboard) ) { this.props.fetchDashboardCardData({ reload: false, clearCache: true }); @@ -83,51 +80,39 @@ class DashboardInner extends Component { this.props.cancelFetchDashboardCardData(); } - async loadDashboard(dashboardId) { - const { - editingOnLoad, - addCardOnLoad, - addCardToDashboard, - fetchDashboard, - initialize, - loadDashboardParams, - location, - setErrorPage, - isNavigatingBackToDashboard, - } = this.props; - - initialize({ clearCache: !isNavigatingBackToDashboard }); - - loadDashboardParams(); - - const result = await fetchDashboard({ - dashId: dashboardId, - queryParams: location.query, + async loadDashboard() { + const p = this.props; + p.initialize({ clearCache: !p.isNavigatingBackToDashboard }); + p.loadDashboardParams(); + + const result = await p.fetchDashboard({ + dashId: p.dashboardId, + queryParams: p.location.query, options: { - clearCache: !isNavigatingBackToDashboard, - preserveParameters: isNavigatingBackToDashboard, + clearCache: !p.isNavigatingBackToDashboard, + preserveParameters: p.isNavigatingBackToDashboard, }, }); if (result.error) { - setErrorPage(result.payload); + p.setErrorPage(result.payload); return; } try { - if (editingOnLoad) { + if (p.editingOnLoad) { this.setEditing(this.props.dashboard); } - if (addCardOnLoad != null) { - addCardToDashboard({ - dashId: dashboardId, - cardId: addCardOnLoad, - tabId: this.props.dashboard.tabs[0]?.id ?? null, + if (p.addCardOnLoad != null) { + p.addCardToDashboard({ + dashId: p.dashboardId, + cardId: p.addCardOnLoad, + tabId: p.dashboard?.tabs[0]?.id ?? null, }); } } catch (error) { if (error.status === 404) { - setErrorPage({ ...error, context: "dashboard" }); + p.setErrorPage({ ...error, context: "dashboard" }); } else { console.error(error); this.setState({ error }); @@ -156,133 +141,113 @@ class DashboardInner extends Component { }; onAddQuestion = () => { - const { dashboard } = this.props; - this.setEditing(dashboard); + this.setEditing(this.props.dashboard); this.props.toggleSidebar(SIDEBAR_NAME.addQuestion); }; - renderContent = () => { - const { dashboard, selectedTabId, isNightMode, isFullscreen } = this.props; - - const canWrite = dashboard?.can_write ?? false; - - const dashboardHasCards = dashboard?.dashcards.length > 0 ?? false; + shouldRenderAsNightMode() { + return this.props.isNightMode && this.props.isFullscreen; + } + renderContent = () => { + const p = this.props; + const canWrite = p.dashboard?.can_write ?? false; + const dashboardHasCards = p.dashboard?.dashcards.length > 0 ?? false; const tabHasCards = - dashboard?.dashcards.filter( + p.dashboard?.dashcards.filter( c => - selectedTabId !== undefined && c.dashboard_tab_id === selectedTabId, + p.selectedTabId !== undefined && + c.dashboard_tab_id === p.selectedTabId, ).length > 0 ?? false; - const shouldRenderAsNightMode = isNightMode && isFullscreen; - if (!dashboardHasCards && !canWrite) { return ( ); } + if (!dashboardHasCards) { return ( ); } + if (dashboardHasCards && !tabHasCards) { return ( ); } + return ( ); }; render() { - const { - addParameter, - dashboard, - isEditing, - isEditingParameter, - isFullscreen, - isNightMode, - isSharing, - parameters, - parameterValues, - draftParameterValues, - editingParameter, - setParameterValue, - setParameterIndex, - setEditingParameter, - isHeaderVisible, - isAutoApplyFilters, - } = this.props; - - const { error, isParametersWidgetSticky } = this.state; - - const shouldRenderAsNightMode = isNightMode && isFullscreen; - - const visibleParameters = getVisibleParameters(parameters); + const p = this.props; + const visibleParameters = getVisibleParameters(p.parameters); const parametersWidget = ( ); const shouldRenderParametersWidgetInViewMode = - !isEditing && !isFullscreen && visibleParameters.length > 0; + !p.isEditing && !p.isFullscreen && visibleParameters.length > 0; const shouldRenderParametersWidgetInEditMode = - isEditing && visibleParameters.length > 0; + p.isEditing && visibleParameters.length > 0; const cardsContainerShouldHaveMarginTop = !shouldRenderParametersWidgetInViewMode && - (!isEditing || isEditingParameter); + (!p.isEditing || p.isEditingParameter); return ( {() => ( - {isHeaderVisible && ( + {p.isHeaderVisible && ( @@ -290,7 +255,7 @@ class DashboardInner extends Component { {shouldRenderParametersWidgetInEditMode && ( {parametersWidget} @@ -298,17 +263,17 @@ class DashboardInner extends Component { )} - + {shouldRenderParametersWidgetInViewMode && ( {parametersWidget} diff --git a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx new file mode 100644 index 0000000000000..5c609950f61e4 --- /dev/null +++ b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.unit.spec.tsx @@ -0,0 +1,183 @@ +import { renderWithProviders, waitForLoaderToBeRemoved } from "__support__/ui"; +import { + createMockActionDashboardCard as _createMockActionDashboardCard, + createMockActionParameter, + createMockDashboard, +} from "metabase-types/api/mocks"; +import type { Dashboard, Parameter } from "metabase-types/api"; +import { Dashboard as DashboardComponent } from "./Dashboard"; + +type SetupOpts = { + dashboardId: number | null; + dashboard: Dashboard | null; + selectedTabId: number | null; // when there's no tabs, is null + parameters: Parameter[]; // when empty, is an empty array + parameterValues?: Record; // when empty, is undefined + skipLoader?: boolean; +}; + +async function setup(overrides: Partial = {}) { + const mockDashboard = createMockDashboard({ id: 10 }); // value is irrelevant + + const opts: SetupOpts = { + dashboardId: 10, + dashboard: mockDashboard, + selectedTabId: null, + parameters: [], + parameterValues: undefined, + ...overrides, + }; + const mockLoadDashboardParams = jest.fn(); + const mockFetchDashboard = jest.fn(() => mockDashboard); + const mockFetchDashboardCardData = jest.fn(); + const mockFetchDashboardCardMetadata = jest.fn(); + + function TestComponent(props: SetupOpts) { + return ( + + ); + } + + const { rerender } = renderWithProviders( + , + ); + + if (!opts.skipLoader) { + await waitForLoaderToBeRemoved(); + } + + return { + rerender: (overrides: Partial = {}) => + rerender(), + mockLoadDashboardParams, + mockFetchDashboard, + mockFetchDashboardCardData, + mockFetchDashboardCardMetadata, + }; +} + +describe("Dashboard data fetching", () => { + afterEach(() => jest.clearAllMocks()); + + it("should fetch dashboard on first load", async () => { + const mocks = await setup(); + expect(mocks.mockFetchDashboard).toHaveBeenCalledTimes(1); + }); + + it("should not fetch anything on re-render", async () => { + const mocks = await setup(); + jest.clearAllMocks(); + mocks.rerender(); + expect(mocks.mockFetchDashboard).toHaveBeenCalledTimes(0); + expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(0); + expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(0); + }); + + it("should fetch dashboard on dashboard id change", async () => { + const mocks = await setup(); + mocks.rerender({ dashboardId: 20 }); + expect(mocks.mockFetchDashboard).toHaveBeenCalledTimes(2); + }); + + it("should fetch card data and metadata when tab id changes", async () => { + const mocks = await setup(); + mocks.rerender({ selectedTabId: 1 }); + expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1); + expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(1); + }); + + it("should fetch card data when parameters change", async () => { + const mocks = await setup(); + jest.clearAllMocks(); + mocks.rerender({ + parameters: [createMockActionParameter()], + }); + expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(0); + expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1); + }); + + it("should fetch card data when parameter properties change", async () => { + const mocks = await setup({ + parameters: [createMockActionParameter()], + }); + jest.clearAllMocks(); + mocks.rerender({ + parameters: [createMockActionParameter({ id: "another" })], + }); + expect(mocks.mockFetchDashboardCardMetadata).toHaveBeenCalledTimes(0); + expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1); + }); + + it("should fetch card data when dashboard changes to non-empty", async () => { + const mocks = await setup({ + dashboardId: null, + dashboard: null, + skipLoader: true, + }); + expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(0); + + mocks.rerender({ + dashboardId: null, + dashboard: createMockDashboard({ id: 20 }), + }); + expect(mocks.mockFetchDashboardCardData).toHaveBeenCalledTimes(1); + }); +}); diff --git a/frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeader.jsx b/frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeader.jsx index 2bcb2f211a091..e222419ba85d7 100644 --- a/frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeader.jsx +++ b/frontend/src/metabase/dashboard/components/DashboardHeader/DashboardHeader.jsx @@ -46,7 +46,7 @@ import Link from "metabase/core/components/Link/Link"; import Collections from "metabase/entities/collections/collections"; import { isInstanceAnalyticsCollection } from "metabase/collections/utils"; import { SIDEBAR_NAME } from "../../constants"; -import { DashboardHeaderComponent } from "./DashboardHeaderView"; +import { DashboardHeaderView } from "./DashboardHeaderView"; import { DashboardHeaderButton, DashboardHeaderActionDivider, @@ -565,7 +565,7 @@ class DashboardHeaderContainer extends Component { return ( <> - null; } -export function DashboardHeaderComponent({ +export function DashboardHeaderView({ editingTitle = "", editingSubtitle = "", editingButtons = [], diff --git a/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx b/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx index c19daee0af0ab..41a436afa2d44 100644 --- a/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx +++ b/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.tsx @@ -34,21 +34,6 @@ import { import { createMockEntitiesState } from "__support__/store"; import { callMockEvent } from "__support__/events"; -const TEST_COLLECTION = createMockCollection(); - -const TEST_DATABASE_WITH_ACTIONS = createMockDatabase({ - settings: { "database-enable-actions": true }, -}); - -const TEST_COLLECTION_ITEM = createMockCollectionItem({ - collection: TEST_COLLECTION, - model: "dataset", -}); - -const TEST_CARD = createMockCard(); - -const TEST_TABLE = createMockTable(); - const TestHome = () =>
; interface Options { @@ -56,22 +41,33 @@ interface Options { } async function setup({ dashboard }: Options = {}) { + const testCollection = createMockCollection(); + + const testDatabaseWithActions = createMockDatabase({ + settings: { "database-enable-actions": true }, + }); + const mockDashboard = createMockDashboard(dashboard); const dashboardId = mockDashboard.id; const channelData = { channels: {} }; fetchMock.get("path:/api/pulse/form_input", channelData); - setupDatabasesEndpoints([TEST_DATABASE_WITH_ACTIONS]); + setupDatabasesEndpoints([testDatabaseWithActions]); setupDashboardEndpoints(mockDashboard); setupCollectionsEndpoints({ collections: [] }); setupCollectionItemsEndpoint({ - collection: TEST_COLLECTION, + collection: testCollection, collectionItems: [], }); - setupSearchEndpoints([TEST_COLLECTION_ITEM]); - setupCardsEndpoints([TEST_CARD]); - setupTableEndpoints(TEST_TABLE); + setupSearchEndpoints([ + createMockCollectionItem({ + collection: testCollection, + model: "dataset", + }), + ]); + setupCardsEndpoints([createMockCard()]); + setupTableEndpoints(createMockTable()); setupBookmarksEndpoints([]); setupActionsEndpoints([]); @@ -99,7 +95,7 @@ async function setup({ dashboard }: Options = {}) { storeInitialState: { dashboard: createMockDashboardState(), entities: createMockEntitiesState({ - databases: [TEST_DATABASE_WITH_ACTIONS], + databases: [testDatabaseWithActions], }), }, }, @@ -115,15 +111,9 @@ async function setup({ dashboard }: Options = {}) { } describe("DashboardApp", function () { - afterEach(() => { - jest.clearAllMocks(); - }); + afterEach(() => jest.clearAllMocks()); describe("beforeunload events", () => { - afterEach(() => { - jest.clearAllMocks(); - }); - it("should have a beforeunload event when the user tries to leave a dirty dashboard", async function () { const { mockEventListener } = await setup(); From e1c4687d325b3fb71de721a6d6f503b5d782b65e Mon Sep 17 00:00:00 2001 From: Nick Fitzpatrick Date: Tue, 5 Dec 2023 13:51:40 -0400 Subject: [PATCH 03/19] Adding pagination to All Personal Collections page (#36255) * Adding pagination to All Personal Collections page * adding unit test * test adjustments --- .../personal-collections.cy.spec.js | 6 +- e2e/test/scenarios/onboarding/urls.cy.spec.js | 12 --- frontend/src/metabase-types/api/mocks/user.ts | 1 + frontend/src/metabase-types/api/user.ts | 10 +- .../use-user-list-query.ts | 8 +- .../use-user-list-query.unit.spec.tsx | 4 +- .../BrowserCrumbs/BrowserCrumbs.jsx | 2 +- .../containers/UserCollectionList.jsx | 84 --------------- .../containers/UserCollectionList.styled.tsx | 5 + .../containers/UserCollectionList.tsx | 102 ++++++++++++++++++ .../UserCollectionList.unit.spec.tsx | 42 ++++++++ .../MainNavbarContainer/MainNavbarView.tsx | 1 + frontend/src/metabase/routes.jsx | 2 +- 13 files changed, 172 insertions(+), 107 deletions(-) delete mode 100644 frontend/src/metabase/containers/UserCollectionList.jsx create mode 100644 frontend/src/metabase/containers/UserCollectionList.tsx create mode 100644 frontend/src/metabase/containers/UserCollectionList.unit.spec.tsx diff --git a/e2e/test/scenarios/collections/personal-collections.cy.spec.js b/e2e/test/scenarios/collections/personal-collections.cy.spec.js index a8eb51b67f014..ec942c587319d 100644 --- a/e2e/test/scenarios/collections/personal-collections.cy.spec.js +++ b/e2e/test/scenarios/collections/personal-collections.cy.spec.js @@ -62,15 +62,13 @@ describe("personal collections", () => { }); cy.visit("/collection/root"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText("Your personal collection"); + cy.findByRole("tree").findByText("Your personal collection"); navigationSidebar().within(() => { cy.icon("ellipsis").click(); }); popover().findByText("Other users' personal collections").click(); cy.location("pathname").should("eq", "/collection/users"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText(/All personal collections/i); + cy.findByTestId("browsercrumbs").findByText(/All personal collections/i); Object.values(USERS).forEach(user => { const FULL_NAME = `${user.first_name} ${user.last_name}`; cy.findByText(FULL_NAME); diff --git a/e2e/test/scenarios/onboarding/urls.cy.spec.js b/e2e/test/scenarios/onboarding/urls.cy.spec.js index f59140e0d188b..c6b361a9a13aa 100644 --- a/e2e/test/scenarios/onboarding/urls.cy.spec.js +++ b/e2e/test/scenarios/onboarding/urls.cy.spec.js @@ -106,18 +106,6 @@ describe("URLs", () => { cy.location("pathname").should("eq", "/collection/users"); }); - it("should slugify users' personal collection URLs", () => { - cy.visit("/collection/users"); - // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage - cy.findByText(getFullName(normal)).click(); - cy.location("pathname").should( - "eq", - `/collection/${NORMAL_PERSONAL_COLLECTION_ID}-${getUsersPersonalCollectionSlug( - normal, - )}`, - ); - }); - it("should open slugified URLs correctly", () => { cy.visit(`/collection/${FIRST_COLLECTION_ID}-first-collection`); cy.findByTestId("collection-name-heading").should( diff --git a/frontend/src/metabase-types/api/mocks/user.ts b/frontend/src/metabase-types/api/mocks/user.ts index f53b4ad47dcf0..9e1e18718b604 100644 --- a/frontend/src/metabase-types/api/mocks/user.ts +++ b/frontend/src/metabase-types/api/mocks/user.ts @@ -31,6 +31,7 @@ export const createMockUserListResult = ( last_name: "Tableton", common_name: "Testy Tableton", email: "user@metabase.test", + personal_collection_id: 2, ...opts, }); diff --git a/frontend/src/metabase-types/api/user.ts b/frontend/src/metabase-types/api/user.ts index df56088204058..f8d712de1babc 100644 --- a/frontend/src/metabase-types/api/user.ts +++ b/frontend/src/metabase-types/api/user.ts @@ -1,3 +1,4 @@ +import type { CollectionId } from "./collection"; import type { DashboardId } from "./dashboard"; export type UserId = number; @@ -40,6 +41,11 @@ export interface UserListResult { last_name: string | null; common_name: string; email: string; + personal_collection_id: CollectionId; +} + +export interface UserListMetadata { + total: number; } // Used when hydrating `creator` property @@ -57,5 +63,7 @@ export type UserInfo = Pick< >; export type UserListQuery = { - recipients: boolean; + recipients?: boolean; + limit?: number; + offset?: number; }; diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts index 80f455373db45..c4b29ce25740c 100644 --- a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts +++ b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.ts @@ -4,11 +4,15 @@ import type { UseEntityListQueryResult, } from "metabase/common/hooks/use-entity-list-query"; import { useEntityListQuery } from "metabase/common/hooks/use-entity-list-query"; -import type { UserListQuery, UserListResult } from "metabase-types/api"; +import type { + UserListQuery, + UserListResult, + UserListMetadata, +} from "metabase-types/api"; export const useUserListQuery = ( props: UseEntityListQueryProps = {}, -): UseEntityListQueryResult => { +): UseEntityListQueryResult => { return useEntityListQuery(props, { fetchList: Users.actions.fetchList, getList: Users.selectors.getList, diff --git a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx index a7a785d77182e..04e54d7a1d6c4 100644 --- a/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx +++ b/frontend/src/metabase/common/hooks/use-user-list-query/use-user-list-query.unit.spec.tsx @@ -9,12 +9,12 @@ import { waitForLoaderToBeRemoved, within, } from "__support__/ui"; -import { createMockUserInfo } from "metabase-types/api/mocks"; +import { createMockUserListResult } from "metabase-types/api/mocks"; import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper/LoadingAndErrorWrapper"; import { useUserListQuery } from "./use-user-list-query"; -const TEST_USER = createMockUserInfo(); +const TEST_USER = createMockUserListResult(); type TestComponentProps = { getRecipients?: boolean }; diff --git a/frontend/src/metabase/components/BrowserCrumbs/BrowserCrumbs.jsx b/frontend/src/metabase/components/BrowserCrumbs/BrowserCrumbs.jsx index a76049c8a28b1..7255df6b9baed 100644 --- a/frontend/src/metabase/components/BrowserCrumbs/BrowserCrumbs.jsx +++ b/frontend/src/metabase/components/BrowserCrumbs/BrowserCrumbs.jsx @@ -15,7 +15,7 @@ const Crumb = ({ children }) => ( ); const BrowserCrumbs = ({ crumbs, analyticsContext }) => ( - + {crumbs .filter(c => c) .map((crumb, index, crumbs) => ( diff --git a/frontend/src/metabase/containers/UserCollectionList.jsx b/frontend/src/metabase/containers/UserCollectionList.jsx deleted file mode 100644 index 679ee2a811c92..0000000000000 --- a/frontend/src/metabase/containers/UserCollectionList.jsx +++ /dev/null @@ -1,84 +0,0 @@ -/* eslint-disable react/prop-types */ -import { connect } from "react-redux"; - -import * as Urls from "metabase/lib/urls"; -import { color } from "metabase/lib/colors"; - -import Card from "metabase/components/Card"; -import { Icon } from "metabase/core/components/Icon"; -import { Grid } from "metabase/components/Grid"; -import Link from "metabase/core/components/Link"; -import BrowserCrumbs from "metabase/components/BrowserCrumbs"; - -import User from "metabase/entities/users"; -import Collection, { - ROOT_COLLECTION, - PERSONAL_COLLECTIONS, -} from "metabase/entities/collections"; -import { - CardContent, - ListGridItem, - ListHeader, - ListRoot, -} from "./UserCollectionList.styled"; - -function mapStateToProps(state) { - return { - collectionsById: state.entities.collections, - }; -} - -const UserCollectionList = ({ collectionsById }) => ( - - - - - - {({ list }) => { - return ( -
- - { - // map through all users that have logged in at least once - // which gives them a personal collection ID - list.map( - user => - user.personal_collection_id && ( - - - - - -

{user.common_name}

- - - - - ), - ) - } - -
- ); - }} - - -); - -export default Collection.loadList()( - connect(mapStateToProps)(UserCollectionList), -); diff --git a/frontend/src/metabase/containers/UserCollectionList.styled.tsx b/frontend/src/metabase/containers/UserCollectionList.styled.tsx index fd46eea28b296..eb0efb8609aa7 100644 --- a/frontend/src/metabase/containers/UserCollectionList.styled.tsx +++ b/frontend/src/metabase/containers/UserCollectionList.styled.tsx @@ -1,5 +1,6 @@ import styled from "@emotion/styled"; import { GridItem } from "metabase/components/Grid"; +import { color } from "metabase/lib/colors"; export const ListRoot = styled.div` padding: 0 4rem; @@ -11,6 +12,10 @@ export const ListHeader = styled.div` export const ListGridItem = styled(GridItem)` width: 33.33%; + + &:hover { + color: ${color("brand")}; + } `; export const CardContent = styled.div` diff --git a/frontend/src/metabase/containers/UserCollectionList.tsx b/frontend/src/metabase/containers/UserCollectionList.tsx new file mode 100644 index 0000000000000..e3abbb6dc12b9 --- /dev/null +++ b/frontend/src/metabase/containers/UserCollectionList.tsx @@ -0,0 +1,102 @@ +import * as Urls from "metabase/lib/urls"; +import { color } from "metabase/lib/colors"; + +import Card from "metabase/components/Card"; +import { Icon } from "metabase/core/components/Icon"; +import { Grid } from "metabase/components/Grid"; +import Link from "metabase/core/components/Link"; +import BrowserCrumbs from "metabase/components/BrowserCrumbs"; + +import { useUserListQuery } from "metabase/common/hooks/use-user-list-query"; +import PaginationControls from "metabase/components/PaginationControls"; + +import { Box, Flex, Loader } from "metabase/ui"; +import { + ROOT_COLLECTION, + PERSONAL_COLLECTIONS, +} from "metabase/entities/collections"; + +import { usePeopleQuery } from "metabase/admin/people/hooks/use-people-query"; +import { + CardContent, + ListGridItem, + ListHeader, +} from "./UserCollectionList.styled"; + +const PAGE_SIZE = 27; + +export const UserCollectionList = () => { + const { query, handleNextPage, handlePreviousPage } = + usePeopleQuery(PAGE_SIZE); + + const { + data: users = [], + isLoading, + metadata, + } = useUserListQuery({ + query: { + limit: query.pageSize, + offset: query.pageSize * query.page, + }, + }); + + return ( + + + + + + {isLoading ? ( + + + + ) : ( + + {users.map( + user => + user.personal_collection_id && ( + + + + + +

{user.common_name}

+ + + + + ), + )} + + )} + + + + + + ); +}; diff --git a/frontend/src/metabase/containers/UserCollectionList.unit.spec.tsx b/frontend/src/metabase/containers/UserCollectionList.unit.spec.tsx new file mode 100644 index 0000000000000..4bb04e9455f7d --- /dev/null +++ b/frontend/src/metabase/containers/UserCollectionList.unit.spec.tsx @@ -0,0 +1,42 @@ +import fetchMock from "fetch-mock"; +import userEvent from "@testing-library/user-event"; +import { renderWithProviders, screen } from "__support__/ui"; +import { createMockUser } from "metabase-types/api/mocks"; +import { UserCollectionList } from "./UserCollectionList"; + +const MockUsers = new Array(100).fill(0).map((_, index) => + createMockUser({ + id: index, + common_name: `big boi ${index}`, + personal_collection_id: index + 2, + }), +); + +const setup = () => { + fetchMock.get("path:/api/user", (_: any, __: any, request: Request) => { + const params = new URL(request.url).searchParams; + const limit = parseInt(params.get("limit") ?? "0"); + const offset = parseInt(params.get("offset") ?? "0"); + return MockUsers.slice(offset, offset + limit); + }); + renderWithProviders(); +}; + +describe("UserCollectionList", () => { + it("should show pages of users", async () => { + setup(); + + expect(await screen.findByText("big boi 0")).toBeInTheDocument(); + expect(await screen.findAllByRole("list-item")).toHaveLength(27); + + expect(await screen.findByText("1 - 27")).toBeInTheDocument(); + + expect(await screen.findByTestId("previous-page-btn")).toBeDisabled(); + userEvent.click(await screen.findByTestId("next-page-btn")); + + expect(await screen.findByText("28 - 54")).toBeInTheDocument(); + + expect(await screen.findByText("big boi 29")).toBeInTheDocument(); + expect(await screen.findByTestId("previous-page-btn")).toBeEnabled(); + }); +}); diff --git a/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarView.tsx b/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarView.tsx index eeaa8e3b2e41b..f42cedb7a74fe 100644 --- a/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarView.tsx +++ b/frontend/src/metabase/nav/containers/MainNavbar/MainNavbarContainer/MainNavbarView.tsx @@ -127,6 +127,7 @@ function MainNavbarView({ onSelect={onItemSelect} TreeNode={SidebarCollectionLink} role="tree" + aria-label="collection-tree" /> diff --git a/frontend/src/metabase/routes.jsx b/frontend/src/metabase/routes.jsx index 193d77d25394d..b1d43a4657d8d 100644 --- a/frontend/src/metabase/routes.jsx +++ b/frontend/src/metabase/routes.jsx @@ -33,7 +33,7 @@ import QueryBuilder from "metabase/query_builder/containers/QueryBuilder"; import MoveCollectionModal from "metabase/collections/containers/MoveCollectionModal"; import ArchiveCollectionModal from "metabase/components/ArchiveCollectionModal"; import CollectionPermissionsModal from "metabase/admin/permissions/components/CollectionPermissionsModal/CollectionPermissionsModal"; -import UserCollectionList from "metabase/containers/UserCollectionList"; +import { UserCollectionList } from "metabase/containers/UserCollectionList"; import PulseEditApp from "metabase/pulse/containers/PulseEditApp"; import { Setup } from "metabase/setup/components/Setup"; From a0c3e90a04d94f1abc02cec5c2667ff29aa80898 Mon Sep 17 00:00:00 2001 From: Braden Shepherdson Date: Tue, 5 Dec 2023 13:32:15 -0500 Subject: [PATCH 04/19] [MLv2] Underlying records: show drills on clicks with no `:column` (#36319) That fixes "chart legend" clicks and "pivot cell" clicks, which should show underlying records and automatic insights drills. Fixes #35343 and #35394. --- .../lib/drill_thru/underlying_records.cljc | 18 +++++++++--- src/metabase/lib/js.cljs | 12 +++++--- src/metabase/lib/schema/drill_thru.cljc | 4 +-- .../drill_thru/underlying_records_test.cljc | 29 +++++++++++++++++++ 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/src/metabase/lib/drill_thru/underlying_records.cljc b/src/metabase/lib/drill_thru/underlying_records.cljc index 57276aa4a03f3..57ae382c64ad2 100644 --- a/src/metabase/lib/drill_thru/underlying_records.cljc +++ b/src/metabase/lib/drill_thru/underlying_records.cljc @@ -19,7 +19,10 @@ (mu/defn underlying-records-drill :- [:maybe ::lib.schema.drill-thru/drill-thru.underlying-records] "When clicking on a particular broken-out group, offer a look at the details of all the rows that went into this bucket. Eg. distribution of People by State, then click New York and see the table of all People filtered by - `STATE = 'New York'`." + `STATE = 'New York'`. + + There is another quite different case: clicking the legend of a chart with multiple bars or lines broken out by + category. Then `column` is nil!" [query :- ::lib.schema/query stage-number :- :int {:keys [column column-ref dimensions value]} :- ::lib.schema.drill-thru/context] @@ -31,11 +34,18 @@ ;; - value: the aggregated value (the count, the sum, etc.) ;; So dimensions is exactly what we want. ;; It returns the table name and row count, since that's used for pluralization of the name. + + ;; Clicking on a chart legend for eg. COUNT(Orders) by Products.CATEGORY and Orders.CREATED_AT has a context like: + ;; - column is nil + ;; - value is nil + ;; - dimensions holds only the legend's column, eg. Products.CATEGORY. (when (and (lib.drill-thru.common/mbql-stage? query stage-number) - column - (some? value) (not-empty dimensions) - (not (lib.types.isa/structured? column))) + ;; Either we need both column and value (cell/map/data point click) or neither (chart legend click). + (or (and column (some? value)) + (and (nil? column) (nil? value))) + ;; If the column exists, it must not be a structured column like JSON. + (not (and column (lib.types.isa/structured? column)))) {:lib/type :metabase.lib.drill-thru/drill-thru :type :drill-thru/underlying-records ;; TODO: This is a bit confused for non-COUNT aggregations. Perhaps it should just always be 10 or something? diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs index be43d8461f7c0..a6985e40e35b1 100644 --- a/src/metabase/lib/js.cljs +++ b/src/metabase/lib/js.cljs @@ -961,15 +961,19 @@ (defn ^:export available-drill-thrus "Return an array (possibly empty) of drill-thrus given: - - Required column + - Nullable column - Nullable value - Nullable data row (the array of `{col, value}` pairs from `clicked.data`) - - Nullable dimensions list (`{column, value}` pairs from `clicked.dimensions`)" + - Nullable dimensions list (`{column, value}` pairs from `clicked.dimensions`) + + Column can be nil for a \"chart legend\" click, eg. clicking a category in the legend explaining the colours in a + multiple bar or line chart. Underlying records drills apply in that case!" [a-query stage-number column value row dimensions] (lib.convert/with-aggregation-list (lib.core/aggregations a-query stage-number) - (let [column-ref (when-let [a-ref (.-field_ref ^js column)] + (let [column-ref (when-let [a-ref (and column (.-field_ref ^js column))] (legacy-ref->pMBQL a-ref))] - (->> (merge {:column (fix-column-with-ref column-ref (js.metadata/parse-column column)) + (->> (merge {:column (when column + (fix-column-with-ref column-ref (js.metadata/parse-column column))) :column-ref column-ref :value (cond (undefined? value) nil ; Missing a value, ie. a column click diff --git a/src/metabase/lib/schema/drill_thru.cljc b/src/metabase/lib/schema/drill_thru.cljc index 8ff49b55768d3..01dfc29c5b0fd 100644 --- a/src/metabase/lib/schema/drill_thru.cljc +++ b/src/metabase/lib/schema/drill_thru.cljc @@ -289,8 +289,8 @@ (mr/def ::context [:map - [:column [:ref ::lib.schema.metadata/column]] - [:column-ref [:ref ::lib.schema.ref/ref]] + [:column [:maybe [:ref ::lib.schema.metadata/column]]] + [:column-ref [:maybe [:ref ::lib.schema.ref/ref]]] [:value [:maybe :any]] [:row {:optional true} [:ref ::context.row]] [:dimensions {:optional true} [:maybe [:ref ::context.row]]]]) diff --git a/test/metabase/lib/drill_thru/underlying_records_test.cljc b/test/metabase/lib/drill_thru/underlying_records_test.cljc index 428309ab73586..e00017bc106aa 100644 --- a/test/metabase/lib/drill_thru/underlying_records_test.cljc +++ b/test/metabase/lib/drill_thru/underlying_records_test.cljc @@ -268,3 +268,32 @@ (meta/id :orders :quantity)] 30.0]]}]} query'))))) + +(deftest ^:parallel chart-legend-click-test + (testing "chart legend clicks have no `column` set, but should still work (#35343)" + (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders)) + (lib/aggregate (lib/count)) + (lib/breakout (meta/field-metadata :products :category)) + (lib/breakout (meta/field-metadata :orders :created-at))) + columns (lib/returned-columns query) + category (m/find-first #(= (:name %) "CATEGORY") columns) + context {:column nil + :column-ref nil + :value nil + :dimensions [{:column category + :column-ref (lib/ref category) + :value "Gadget"}]} + drills (lib.drill-thru/available-drill-thrus query context) + drill (m/find-first #(= (:type %) :drill-thru/underlying-records) + drills)] + (is (=? {:type :drill-thru/underlying-records + :row-count 2 + :table-name "Orders" + :dimensions [{}]} + drill)) + (is (=? {:lib/type :mbql/query + :stages [{:filters [[:= {} [:field {} (meta/id :products :category)] "Gadget"]] + :aggregation (symbol "nil #_\"key is not present.\"") + :breakout (symbol "nil #_\"key is not present.\"") + :fields (symbol "nil #_\"key is not present.\"")}]} + (lib.drill-thru/drill-thru query -1 drill)))))) From f4448046ee30b2b448760bcc54eb05de49182b72 Mon Sep 17 00:00:00 2001 From: Jerry Huang <34140255+qwef@users.noreply.github.com> Date: Tue, 5 Dec 2023 12:32:36 -0600 Subject: [PATCH 05/19] Filter values leaked in API request on Embedding (#36152) --- .clj-kondo/config.edn | 3 +- .../pulse_test.clj | 6 +- src/metabase/api/embed.clj | 62 ++++++++++++--- src/metabase/email/messages.clj | 8 +- src/metabase/models/params.clj | 16 ++++ src/metabase/pulse.clj | 14 ++-- test/metabase/api/embed_test.clj | 77 ++++++++++++++++++- test/metabase/models/params_test.clj | 32 ++++++++ test/metabase/pulse/parameters_test.clj | 14 ++-- 9 files changed, 198 insertions(+), 34 deletions(-) diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index f133202c105da..2529a62f80395 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -316,6 +316,7 @@ metabase.models.interface mi metabase.models.moderation-review moderation-review metabase.models.native-query-snippet native-query-snippet + metabase.models.params params metabase.models.permissions perms metabase.models.permissions-group perms-group metabase.models.permissions-group-membership perms-group-membership @@ -338,7 +339,7 @@ metabase.pulse.render.common common metabase.pulse.render.style style metabase.pulse.test-util pulse.test-util - metabase.pulse.parameters params + metabase.pulse.parameters pulse-params metabase.query-processor-test qp.test metabase.query-processor.context qp.context metabase.query-processor.error-type qp.error-type diff --git a/enterprise/backend/test/metabase_enterprise/dashboard_subscription_filters/pulse_test.clj b/enterprise/backend/test/metabase_enterprise/dashboard_subscription_filters/pulse_test.clj index 9f3cc19c11ab4..c262681db03d8 100644 --- a/enterprise/backend/test/metabase_enterprise/dashboard_subscription_filters/pulse_test.clj +++ b/enterprise/backend/test/metabase_enterprise/dashboard_subscription_filters/pulse_test.clj @@ -2,7 +2,7 @@ (:require [clojure.test :refer :all] [metabase.public-settings.premium-features-test :as premium-features-test] - [metabase.pulse.parameters :as params])) + [metabase.pulse.parameters :as pulse-params])) (deftest parameters-test (testing "Get params from both pulse and dashboard if :dashboard-subscription-filters feature is enabled" @@ -10,7 +10,7 @@ (is (= [{:id "1" :v "a"} {:id "2" :v "b"} {:id "3" :v "yes"}] - (params/the-parameters + (pulse-params/the-parameters {:parameters [{:id "1" :v "a"} {:id "2" :v "b"}]} {:parameters [{:id "1" :v "no, since it's trumped by the pulse"} {:id "3" :v "yes"}]}))))) @@ -18,6 +18,6 @@ (premium-features-test/with-premium-features #{} (is (= [{:id "1" :v "no, since it's trumped by the pulse"} {:id "3" :v "yes"}] - (params/the-parameters + (pulse-params/the-parameters {:parameters [{:id "1" :v "a"} {:id "2" :v "b"}]} {:parameters [{:id "1" :v "no, since it's trumped by the pulse"} {:id "3" :v "yes"}]})))))) diff --git a/src/metabase/api/embed.clj b/src/metabase/api/embed.clj index 5a95b4e8e7f4d..84b25aae10dc1 100644 --- a/src/metabase/api/embed.clj +++ b/src/metabase/api/embed.clj @@ -28,7 +28,8 @@ [metabase.driver.common.parameters.operators :as params.ops] [metabase.models.card :as card :refer [Card]] [metabase.models.dashboard :refer [Dashboard]] - [metabase.pulse.parameters :as params] + [metabase.models.params :as params] + [metabase.pulse.parameters :as pulse-params] [metabase.query-processor :as qp] [metabase.query-processor.middleware.constraints :as qp.constraints] [metabase.query-processor.pivot :as qp.pivot] @@ -113,18 +114,23 @@ :when (not (contains? params-to-remove (keyword (:slug param))))] param)) +(defn- get-params-to-remove + "Gets the params in both the provided embedding-params and dashboard-or-card object that we should remove." + [dashboard-or-card embedding-params] + (set (concat (for [[param status] embedding-params + :when (not= status "enabled")] + param) + (for [{slug :slug} (:parameters dashboard-or-card) + :let [param (keyword slug)] + :when (not (contains? embedding-params param))] + param)))) + (mu/defn ^:private remove-locked-and-disabled-params "Remove the `:parameters` for `dashboard-or-card` that listed as `disabled` or `locked` in the `embedding-params` whitelist, or not present in the whitelist. This is done so the frontend doesn't display widgets for params the user can't set." [dashboard-or-card embedding-params :- ms/EmbeddingParams] - (let [params-to-remove (set (concat (for [[param status] embedding-params - :when (not= status "enabled")] - param) - (for [{slug :slug} (:parameters dashboard-or-card) - :let [param (keyword slug)] - :when (not (contains? embedding-params param))] - param)))] + (let [params-to-remove (get-params-to-remove dashboard-or-card embedding-params)] (update dashboard-or-card :parameters remove-params-in-set params-to-remove))) (defn- remove-token-parameters @@ -152,7 +158,7 @@ (map (fn [card] (if (-> card :visualization_settings :virtual_card) - (params/process-virtual-dashcard card params-with-values) + (pulse-params/process-virtual-dashcard card params-with-values) card)) dashcards)))) @@ -251,18 +257,52 @@ ;;; -------------------------- Dashboard Fns used by both /api/embed and /api/preview_embed -------------------------- +(defn- remove-linked-filters-param-values [dashboard] + (let [param-ids (set (map :id (:parameters dashboard))) + param-ids-to-remove (set (for [{param-id :id + filtering-parameters :filteringParameters} (:parameters dashboard) + filtering-parameter-id filtering-parameters + :when (not (contains? param-ids filtering-parameter-id))] + param-id)) + linked-field-ids (set (mapcat (params/get-linked-field-ids (:dashcards dashboard)) param-ids-to-remove))] + (update dashboard :param_values #(->> % + (map (fn [[param-id param]] + {param-id (cond-> param + (contains? linked-field-ids param-id) ;; is param linked? + (assoc :values []))})) + (into {}))))) + +(defn- remove-locked-parameters [dashboard embedding-params] + (let [params-to-remove (get-params-to-remove dashboard embedding-params) + param-ids-to-remove (set (for [parameter (:parameters dashboard) + :when (contains? params-to-remove (keyword (:slug parameter)))] + (:id parameter))) + linked-field-ids (set (mapcat (params/get-linked-field-ids (:dashcards dashboard)) param-ids-to-remove)) + remove-parameters (fn [dashcard] + (update dashcard :parameter_mappings + (fn [param-mappings] + (remove (fn [{:keys [parameter_id]}] + (contains? param-ids-to-remove parameter_id)) param-mappings))))] + (-> dashboard + (update :dashcards #(map remove-parameters %)) + (update :param_fields #(apply dissoc % linked-field-ids)) + (update :param_values #(apply dissoc % linked-field-ids))))) + (defn dashboard-for-unsigned-token "Return the info needed for embedding about Dashboard specified in `token`. Additional `constraints` can be passed to the `public-dashboard` function that fetches the Dashboard." [unsigned-token & {:keys [embedding-params constraints]}] {:pre [((some-fn empty? sequential?) constraints) (even? (count constraints))]} (let [dashboard-id (embed/get-in-unsigned-token-or-throw unsigned-token [:resource :dashboard]) + embedding-params (or embedding-params + (t2/select-one-fn :embedding_params Dashboard, :id dashboard-id)) token-params (embed/get-in-unsigned-token-or-throw unsigned-token [:params])] (-> (apply api.public/public-dashboard :id dashboard-id, constraints) (substitute-token-parameters-in-text token-params) + (remove-locked-parameters embedding-params) (remove-token-parameters token-params) - (remove-locked-and-disabled-params (or embedding-params - (t2/select-one-fn :embedding_params Dashboard, :id dashboard-id)))))) + (remove-locked-and-disabled-params embedding-params) + (remove-linked-filters-param-values)))) (defn dashcard-results-async "Return results for running the query belonging to a DashboardCard. Returns a `StreamingResponse`." diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj index 89333532e07a4..9d454529410da 100644 --- a/src/metabase/email/messages.clj +++ b/src/metabase/email/messages.clj @@ -20,7 +20,7 @@ [metabase.public-settings :as public-settings] [metabase.public-settings.premium-features :as premium-features] [metabase.pulse.markdown :as markdown] - [metabase.pulse.parameters :as params] + [metabase.pulse.parameters :as pulse-params] [metabase.pulse.render :as render] [metabase.pulse.render.body :as body] [metabase.pulse.render.image-bundle :as image-bundle] @@ -318,7 +318,7 @@ (merge (common-context) {:emailType "pulse" :title (:name pulse) - :titleUrl (params/dashboard-url dashboard-id (params/parameters pulse dashboard)) + :titleUrl (pulse-params/dashboard-url dashboard-id (pulse-params/parameters pulse dashboard)) :dashboardDescription (:description dashboard) ;; There are legacy pulses that exist without being tied to a dashboard :dashboardHasTabs (when dashboard-id @@ -449,7 +449,7 @@ (defn- render-filters [notification dashboard] - (let [filters (params/parameters notification dashboard) + (let [filters (pulse-params/parameters notification dashboard) cells (map (fn [filter] [:td {:class "filter-cell" @@ -474,7 +474,7 @@ :width "50%" :padding "4px 16px 4px 8px" :vertical-align "baseline"})} - (params/value-string filter)]]]]) + (pulse-params/value-string filter)]]]]) filters) rows (partition 2 2 nil cells)] (html diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj index c7848d5e312e5..46d34b88f896e 100644 --- a/src/metabase/models/params.clj +++ b/src/metabase/models/params.clj @@ -255,6 +255,22 @@ id)) (cards->card-param-field-ids (map :card dashcards)))) +(defn get-linked-field-ids + "Retrieve a map relating paramater ids to field ids." + [dashcards] + (letfn [(targets [params card] + (into {} + (for [param params + :let [clause (param-target->field-clause (:target param) + card) + ids (mbql.u/match clause + [:field (id :guard integer?) _] + id)] + :when (seq ids)] + [(:parameter_id param) (set ids)])))] + (->> dashcards + (mapv (fn [{params :parameter_mappings card :card}] (targets params card))) + (apply merge-with into {})))) (defn- dashboard->param-field-values "Return a map of Field ID to FieldValues (if any) for any Fields referenced by Cards in `dashboard`, diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj index 78fb379b65b91..a32d6c9eb1d71 100644 --- a/src/metabase/pulse.clj +++ b/src/metabase/pulse.clj @@ -17,7 +17,7 @@ [metabase.models.setting :as setting :refer [defsetting]] [metabase.public-settings :as public-settings] [metabase.pulse.markdown :as markdown] - [metabase.pulse.parameters :as params] + [metabase.pulse.parameters :as pulse-params] [metabase.pulse.render :as render] [metabase.pulse.util :as pu] [metabase.query-processor :as qp] @@ -155,7 +155,7 @@ (assert api/*current-user-id* "Makes sure you wrapped this with a `with-current-user`.") (cond (:card_id dashcard) - (let [parameters (merge-default-values (params/parameters pulse dashboard))] + (let [parameters (merge-default-values (pulse-params/parameters pulse dashboard))] (execute-dashboard-subscription-card dashboard dashcard (:card_id dashcard) parameters)) ;; actions @@ -169,9 +169,9 @@ ;; text cards have existed for a while and I'm not sure if all existing text cards ;; will have virtual_card.display = "text", so assume everything else is a text card :else - (let [parameters (merge-default-values (params/parameters pulse dashboard))] + (let [parameters (merge-default-values (pulse-params/parameters pulse dashboard))] (-> dashcard - (params/process-virtual-dashcard parameters) + (pulse-params/process-virtual-dashcard parameters) escape-heading-markdown :visualization_settings (assoc :type :text))))) @@ -281,7 +281,7 @@ (defn- filter-text [filter] (truncate-mrkdwn - (format "*%s*\n%s" (:name filter) (params/value-string filter)) + (format "*%s*\n%s" (:name filter) (pulse-params/value-string filter)) attachment-text-length-limit)) (defn- slack-dashboard-header @@ -295,7 +295,7 @@ creator-section {:type "section" :fields [{:type "mrkdwn" :text (str "Sent by " (-> pulse :creator :common_name))}]} - filters (params/parameters pulse dashboard) + filters (pulse-params/parameters pulse dashboard) filter-fields (for [filter filters] {:type "mrkdwn" :text (filter-text filter)}) @@ -313,7 +313,7 @@ [{:type "divider"} {:type "context" :elements [{:type "mrkdwn" - :text (str "<" (params/dashboard-url (u/the-id dashboard) (params/parameters pulse dashboard)) "|" + :text (str "<" (pulse-params/dashboard-url (u/the-id dashboard) (pulse-params/parameters pulse dashboard)) "|" "*Sent from " (public-settings/site-name) "*>")}]}]}) (def slack-width diff --git a/test/metabase/api/embed_test.clj b/test/metabase/api/embed_test.clj index 084bdfa703b58..6bbaa3e125fa7 100644 --- a/test/metabase/api/embed_test.clj +++ b/test/metabase/api/embed_test.clj @@ -151,7 +151,7 @@ (def successful-dashboard-info {:auto_apply_filters true, :description nil, :parameters [], :dashcards [], :tabs [], - :param_values nil, :param_fields nil}) + :param_values {}, :param_fields nil}) (def ^:private yesterday (time/minus (time/now) (time/days 1))) @@ -559,6 +559,81 @@ :visualization_settings :text))))))) +(deftest locked-params-removes-values-fields-and-mappings-test + (testing "check that locked params are removed in parameter mappings, param_values, and param_fields" + (with-embedding-enabled-and-new-secret-key + (t2.with-temp/with-temp [Dashboard dashboard {:enable_embedding true + :embedding_params {:venue_name "locked"} + :name "Test Dashboard" + :parameters [{:name "venue_name" + :slug "venue_name" + :id "foo" + :type :string/= + :sectionId "string"}]} + Card {card-id :id} {:name "Dashboard Test Card"} + DashboardCard {_ :id} {:dashboard_id (:id dashboard) + :card_id card-id + :parameter_mappings [{:card_id card-id + :slug "venue_name" + :parameter_id "foo" + :target [:dimension + [:field (mt/id :venues :name) nil]]} + {:card_id card-id + :parameter_id "bar" + :target [:dimension + [:field (mt/id :categories :name) nil]]}]}] + (let [embedding-dashboard (client/client :get 200 (dashboard-url dashboard {:params {:foo "BCD Tofu House"}}))] + (is (= nil + (-> embedding-dashboard + :param_values + (get (mt/id :venues :name))))) + (is (= nil + (-> embedding-dashboard + :param_fields + (get (mt/id :venues :name))))) + (is (= 1 + (-> embedding-dashboard + :dashcards + first + :parameter_mappings + count)))))))) + +(deftest linked-param-to-locked-removes-param-values-test + (testing "Check that a linked parameter to a locked params we remove the param_values." + (with-embedding-enabled-and-new-secret-key + (t2.with-temp/with-temp [Dashboard dashboard {:enable_embedding true + :embedding_params {:venue_name "locked" :category_name "enabled"} + :name "Test Dashboard" + :parameters [{:name "venue_name" + :slug "venue_name" + :id "foo" + :type :string/= + :sectionId "string"} + {:name "category_name" + :filteringParameters ["foo"] + :slug "category_name" + :id "bar" + :type :string/= + :sectionId "string"}]} + Card {card-id :id} {:name "Dashboard Test Card"} + DashboardCard {_ :id} {:dashboard_id (:id dashboard) + :card_id card-id + :parameter_mappings [{:card_id card-id + :slug "venue_name" + :parameter_id "foo" + :target [:dimension + [:field (mt/id :venues :name) nil]]} + {:card_id card-id + :slug "category_name" + :parameter_id "bar" + :target [:dimension + [:field (mt/id :categories :name) nil]]}]}] + (let [embedding-dashboard (client/client :get 200 (dashboard-url dashboard {:params {:foo "BCD Tofu House"}}))] + (is (= [] + (-> embedding-dashboard + :param_values + (get (mt/id :categories :name)) + :values)))))))) ;;; ---------------------- GET /api/embed/dashboard/:token/dashcard/:dashcard-id/card/:card-id ----------------------- diff --git a/test/metabase/models/params_test.clj b/test/metabase/models/params_test.clj index 257725f3d4614..a64c199678bb5 100644 --- a/test/metabase/models/params_test.clj +++ b/test/metabase/models/params_test.clj @@ -127,3 +127,35 @@ (testing "card->template-tag-field-ids" (is (= #{(mt/id :venues :id)} (params/card->template-tag-field-ids card)))))) + +(deftest get-linked-field-ids-test + (testing "get-linked-field-ids basic test" + (is (= {"foo" #{256} + "bar" #{267}} + (params/get-linked-field-ids + [{:parameter_mappings + [{:parameter_id "foo" :target [:dimension [:field 256 nil]]} + {:parameter_id "bar" :target [:dimension [:field 267 nil]]}]}])))) + (testing "get-linked-field-ids multiple fields to one param test" + (is (= {"foo" #{256 10} + "bar" #{267}} + (params/get-linked-field-ids + [{:parameter_mappings + [{:parameter_id "foo" :target [:dimension [:field 256 nil]]} + {:parameter_id "bar" :target [:dimension [:field 267 nil]]}]} + {:parameter_mappings + [{:parameter_id "foo" :target [:dimension [:field 10 nil]]}]}])))) + (testing "get-linked-field-ids-test misc fields" + (is (= {"1" #{1} "2" #{2} "3" #{3} "4" #{4} "5" #{5}} + (params/get-linked-field-ids + [{:parameter_mappings + [{:parameter_id "1" :target [:dimension [:field 1 {}]]} + {:parameter_id "2" :target [:dimension [:field 2 {:x true}]]} + {:parameter_id "wow" :target [:dimension [:field "wow" {:base-type :type/Integer}]]} + {:parameter_id "3" :target [:dimension [:field 3 {:source-field 1}]]} + {:parameter_id "4" :target [:dimension [:field 4 {:binning {:strategy :num-bins, :num-bins 1}}]]} + {:parameter_id "5" :target [:dimension [:field 5]]}]}])))) + (testing "get-linked-field-ids-test no fields" + (is (= {} + (params/get-linked-field-ids + [{:parameter_mappings []}]))))) diff --git a/test/metabase/pulse/parameters_test.clj b/test/metabase/pulse/parameters_test.clj index 1346367507175..4400c5230be9e 100644 --- a/test/metabase/pulse/parameters_test.clj +++ b/test/metabase/pulse/parameters_test.clj @@ -1,35 +1,35 @@ (ns ^:mb/once metabase.pulse.parameters-test (:require [clojure.test :refer :all] - [metabase.pulse.parameters :as params] + [metabase.pulse.parameters :as pulse-params] [metabase.pulse.test-util :refer [test-dashboard]] [metabase.test :as mt])) (deftest ^:parallel value-string-test (testing "If a filter has multiple values, they are concatenated into a comma-separated string" (is (= "CA, NY, and NJ" - (params/value-string (-> test-dashboard :parameters first))))) + (pulse-params/value-string (-> test-dashboard :parameters first))))) (testing "If a filter has a single default value, it is formatted appropriately" (is (= "Q1, 2021" - (params/value-string (-> test-dashboard :parameters second)))))) + (pulse-params/value-string (-> test-dashboard :parameters second)))))) (deftest dashboard-url-test (mt/with-temporary-setting-values [site-url "https://metabase.com"] (testing "A valid dashboard URL can be generated with filters included" (is (= "https://metabase.com/dashboard/1?state=CA&state=NY&state=NJ&quarter_and_year=Q1-2021" - (params/dashboard-url 1 (:parameters test-dashboard))))) + (pulse-params/dashboard-url 1 (:parameters test-dashboard))))) (testing "If no filters are set, the base dashboard url is returned" (is (= "https://metabase.com/dashboard/1" - (params/dashboard-url 1 {})))) + (pulse-params/dashboard-url 1 {})))) (testing "Filters slugs and values are encoded properly for the URL" (is (= "https://metabase.com/dashboard/1?%26=contains%3F" - (params/dashboard-url 1 [{:value "contains?", :slug "&"}])))))) + (pulse-params/dashboard-url 1 [{:value "contains?", :slug "&"}])))))) (deftest param-val-or-default-test - (let [param-val-or-default #'params/param-val-or-default] + (let [param-val-or-default #'pulse-params/param-val-or-default] (testing "When the parameter’s :value key is missing, fallback to the :default key" (is (= "my default value" (param-val-or-default {:default "my default value"})))) From 37d701680bd174249df5f7757b57bc303c07232c Mon Sep 17 00:00:00 2001 From: Nick Fitzpatrick Date: Tue, 5 Dec 2023 15:53:56 -0400 Subject: [PATCH 06/19] Adding markdown support to search result descriptions (#36360) * Adding markdown support to search result descriptions * adding e2e test --- .../onboarding/search/search.cy.spec.js | 33 +++++++++++++++++++ .../SearchResult/SearchResult.styled.tsx | 11 +++++++ .../components/SearchResult/SearchResult.tsx | 18 +++++----- 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/e2e/test/scenarios/onboarding/search/search.cy.spec.js b/e2e/test/scenarios/onboarding/search/search.cy.spec.js index 8fb3b0a5aeecf..edf6a4155f96a 100644 --- a/e2e/test/scenarios/onboarding/search/search.cy.spec.js +++ b/e2e/test/scenarios/onboarding/search/search.cy.spec.js @@ -12,6 +12,7 @@ import { setActionsEnabledForDB, setTokenFeatures, summarize, + assertIsEllipsified, } from "e2e/support/helpers"; import { ADMIN_USER_ID, @@ -174,6 +175,38 @@ describe("scenarios > search", () => { cy.get("@search.all").should("have.length", 1); }); + + it("should render a preview of markdown descriptions", () => { + cy.createQuestion({ + name: "Description Test", + query: { "source-table": ORDERS_ID }, + description: `![alt](https://upload.wikimedia.org/wikipedia/commons/a/a2/Cat_outside.jpg) + + Lorem ipsum dolor sit amet. + + ---- + + ## Heading 1 + + This is a [link](https://upload.wikimedia.org/wikipedia/commons/a/a2/Cat_outside.jpg). + + Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. `, + }).then(() => { + cy.signInAsNormalUser(); + cy.visit("/"); + getSearchBar().type("Test"); + }); + + //Enseure that text is ellipsified + cy.findByTestId("result-description") + .findByText(/Lorem ipsum dolor sit amet./) + .then(el => assertIsEllipsified(el[0])); + + //Ensure that images are not being rendered in the descriptions + cy.findByTestId("result-description") + .findByRole("img") + .should("not.exist"); + }); }); describe("accessing full page search with `Enter`", () => { it("should not render full page search if user has not entered a text query", () => { diff --git a/frontend/src/metabase/search/components/SearchResult/SearchResult.styled.tsx b/frontend/src/metabase/search/components/SearchResult/SearchResult.styled.tsx index a9172f01edb57..e2737cc962a9f 100644 --- a/frontend/src/metabase/search/components/SearchResult/SearchResult.styled.tsx +++ b/frontend/src/metabase/search/components/SearchResult/SearchResult.styled.tsx @@ -5,6 +5,7 @@ import type { AnchorHTMLAttributes, HTMLAttributes, RefObject } from "react"; import { PLUGIN_MODERATION } from "metabase/plugins"; import type { AnchorProps, BoxProps, ButtonProps } from "metabase/ui"; import { Box, Divider, Stack, Anchor, Button } from "metabase/ui"; +import Markdown from "metabase/core/components/Markdown"; const { ModerationStatusIcon } = PLUGIN_MODERATION; @@ -111,3 +112,13 @@ export const DescriptionSection = styled(Box)` export const DescriptionDivider = styled(Divider)` border-radius: ${({ theme }) => theme.radius.xs}; `; + +export const SearchResultDescription = styled(Markdown)` + display: -webkit-box; + -webkit-line-clamp: 2; + -webkit-box-orient: vertical; + overflow: hidden; + overflow-wrap: break-word; + white-space: pre-line; + font-size: 0.75rem; +`; diff --git a/frontend/src/metabase/search/components/SearchResult/SearchResult.tsx b/frontend/src/metabase/search/components/SearchResult/SearchResult.tsx index 6d35c979c25b1..e7a0bbaa5b74a 100644 --- a/frontend/src/metabase/search/components/SearchResult/SearchResult.tsx +++ b/frontend/src/metabase/search/components/SearchResult/SearchResult.tsx @@ -4,7 +4,7 @@ import { useCallback } from "react"; import { push } from "react-router-redux"; import { useDispatch } from "metabase/lib/redux"; -import { Group, Text, Loader } from "metabase/ui"; +import { Group, Loader } from "metabase/ui"; import { isSyncCompleted } from "metabase/lib/syncing"; import type { WrappedResult } from "metabase/search/types"; @@ -20,6 +20,7 @@ import { ResultNameSection, ResultTitle, SearchResultContainer, + SearchResultDescription, XRayButton, XRaySection, } from "./SearchResult.styled"; @@ -124,21 +125,20 @@ export function SearchResult({ )} {description && showDescription && ( - + - {description} - + )} From 19f8bc1d25b412b8c2c78c81e8fd714371aced47 Mon Sep 17 00:00:00 2001 From: Jeff Bruemmer Date: Tue, 5 Dec 2023 14:58:36 -0500 Subject: [PATCH 07/19] docs - hide when empty (#36427) --- docs/dashboards/introduction.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/dashboards/introduction.md b/docs/dashboards/introduction.md index ad933c16cb3cc..6c8923cf5c034 100644 --- a/docs/dashboards/introduction.md +++ b/docs/dashboards/introduction.md @@ -132,6 +132,17 @@ Click on the **pencil** icon to enter dashboard edit mode, hover over the questi ![Visualization settings](images/visualization-settings.png) +### Hiding a card when it doesn't return results + +One neat thing to call out: if you have a question card that rarely returns results, but you still want to include it in your dashboard because you want to know when it _does_ return results, you can tell Metabase to hide the card unless it returns at least one row of data. + +When in dashboard edit mode, click on the **Visualization settings** for the card. + +- If the card displays a table, the option is in the **Columns** tab. +- If the card displays a chart, the option is in the **Display** tab. + +Toggle the option **Hide this card if there are no results**. When you turn on this option, the query will still run in the background, but the dashboard won't display the card. If the query returns results, the dashboard will display the card, moving the other cards around to make room for it according to how you've arranged the cards in dashboard edit mode. + ## Resetting a card's visualization settings If you want to revert a dashboard card to its original visualization settings (i.e., the settings on the question when it was _first_ saved to your dashboard): From 0521497a7c525b894b390be26cbdcbd8b4238bfb Mon Sep 17 00:00:00 2001 From: Mark Bastian Date: Tue, 5 Dec 2023 13:38:03 -0700 Subject: [PATCH 08/19] Updating default `number-formatter` for percents (#36431) * Updating default `number-formatter` for percents The number formatter used in some static viz rendering (e.g. tables) in `metabase.pulse.render.common/number-formatter` had logic to render only integer-valued percents for values between 1 and 100 and render two significant digits otherwise. The FE default formatter uses 2 significant digits consistently in `frontend/src/metabase/lib/formatting/numbers.tsx`: ```js const PRECISION_NUMBER_FORMATTER = d3.format(".2f"); ``` This change modifies `number-formatter` to consistenly use 2 significant digits for percentage types. Note that this does NOT apply to fields with custom formatting. Presently, custom column formatting does not appear to be applied for HTML tables at all for BE rendering while it is applied on the FE. This is a separate issue. This PR just brings the baseline behaviors based on typing into alignment. * Fixing formatting test --- src/metabase/pulse/render/common.clj | 5 +---- test/metabase/pulse/pulse_integration_test.clj | 15 ++++++++------- test/metabase/pulse/render/common_test.clj | 6 +++++- test/metabase/pulse/render_test.clj | 2 +- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/metabase/pulse/render/common.clj b/src/metabase/pulse/render/common.clj index 8b19b43bb0cef..c75c4b5ab3bfb 100644 --- a/src/metabase/pulse/render/common.clj +++ b/src/metabase/pulse/render/common.clj @@ -107,15 +107,12 @@ (fn [value] (if (number? value) (let [scaled-value (* value (or scale 1)) - percent-scaled-value (* 100 scaled-value) decimals-in-value (digits-after-decimal (if percent? (* 100 scaled-value) scaled-value)) decimal-digits (cond decimals decimals ;; if user ever specifies # of decimals, use that integral? 0 currency? (get-in currency/currency [(keyword (or currency "USD")) :decimal_digits]) - (and percent? (> percent-scaled-value 100)) (min 2 decimals-in-value) ;; 5.5432 -> %554.32 - ;; This needs configuration. I don't see why we don't keep more significant digits. - (and percent? (> 100 percent-scaled-value 1)) (min 0 decimals-in-value) ;; 0.2555 -> %26 + percent? (min 2 decimals-in-value) ;; 5.5432 -> %554.32 :else (if (>= scaled-value 1) (min 2 decimals-in-value) ;; values greater than 1 round to 2 decimal places (let [n-figs (sig-figs-after-decimal scaled-value decimal)] diff --git a/test/metabase/pulse/pulse_integration_test.clj b/test/metabase/pulse/pulse_integration_test.clj index 487c9f373cd8c..4ed443f5acb93 100644 --- a/test/metabase/pulse/pulse_integration_test.clj +++ b/test/metabase/pulse/pulse_integration_test.clj @@ -4,7 +4,6 @@ These tests should build content then mock out distrubution by usual channels (e.g. email) and check the results of the distributed content for correctness." (:require - [clojure.string :as str] [clojure.test :refer :all] [hickory.core :as hik] [hickory.select :as hik.s] @@ -77,6 +76,10 @@ (map (comp first :content)))) data-tables)))) +(def ^:private all-pct-2d? + "Is every element in the sequence percent formatted with 2 significant digits?" + (partial every? (partial re-matches #"\d+\.\d{2}%"))) + (deftest result-metadata-preservation-in-html-static-viz-for-dashboard-test (testing "In a dashboard, results metadata applied to a model or query based on a model should be used in the HTML rendering of the pulse email." #_{:clj-kondo/ignore [:unresolved-symbol]} @@ -115,9 +118,9 @@ (testing "The data from the first question is just numbers." (is (every? (partial re-matches #"\d+\.\d+") base-data-row))) (testing "The data from the second question (a model) is percent formatted" - (is (every? #(str/ends-with? % "%") model-data-row))) + (is (all-pct-2d? model-data-row))) (testing "The data from the last question (based on the above model) is percent formatted" - (is (every? #(str/ends-with? % "%") question-data-row)))))))) + (is (all-pct-2d? question-data-row)))))))) (deftest simple-model-with-metadata-no-dashboard-in-html-static-viz-test (testing "Cards with metadata sent as pulses should preserve metadata in html formatting (#36323)" @@ -148,8 +151,7 @@ :enabled true} PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id :user_id (mt/user->id :rasta)}] - (is (every? #(str/ends-with? % "%") - (first (run-pulse-and-return-last-data-columns pulse)))))) + (is (all-pct-2d? (first (run-pulse-and-return-last-data-columns pulse)))))) (testing "The data from the last question (based ona a model) is percent formatted" (mt/with-temp [Pulse {pulse-id :id :as pulse} {:name "Test Pulse"} @@ -160,5 +162,4 @@ :enabled true} PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id :user_id (mt/user->id :rasta)}] - (is (every? #(str/ends-with? % "%") - (first (run-pulse-and-return-last-data-columns pulse))))))))) + (is (all-pct-2d? (first (run-pulse-and-return-last-data-columns pulse))))))))) diff --git a/test/metabase/pulse/render/common_test.clj b/test/metabase/pulse/render/common_test.clj index c6af577a29b62..f15142a3853d0 100644 --- a/test/metabase/pulse/render/common_test.clj +++ b/test/metabase/pulse/render/common_test.clj @@ -64,7 +64,11 @@ ::mb.viz/number-separators ",."}))) (is (= "10%" (format 0.1 {::mb.viz/number-style "percent"}))) (is (= "1%" (format 0.01 {::mb.viz/number-style "percent"}))) - (is (= "0.00001%" (format 0.0000001 {::mb.viz/number-style "percent"})))) + (is (= "0%" (format 0.000000 {::mb.viz/number-style "percent"}))) + ;; This is not zero, so should show decimal places + (is (= "0.00%" (format 0.0000001 {::mb.viz/number-style "percent"}))) + (is (= "0.00001%" (format 0.0000001 {::mb.viz/number-style "percent" + ::mb.viz/decimals 5})))) (testing "Match UI 'natural formatting' behavior for decimal values with no column formatting present" ;; basically, for numbers greater than 1, round to 2 decimal places, ;; and do not display decimals if they end up as zeroes diff --git a/test/metabase/pulse/render_test.clj b/test/metabase/pulse/render_test.clj index c05350710ca50..702bdf80f7466 100644 --- a/test/metabase/pulse/render_test.clj +++ b/test/metabase/pulse/render_test.clj @@ -200,7 +200,7 @@ ;; the significant digits in the formatter, we'll need to modify this test as well. (letfn [(create-comparison-results [query-results card] (let [expected (mapv (fn [row] - (format "%s%%" (Math/round ^float (* 100 (peek row))))) + (format "%.2f%%" (* 100 (peek row)))) (get-in query-results [:data :rows])) rendered-card (render/render-pulse-card :inline (pulse/defaulted-timezone card) card nil query-results) table (-> rendered-card From 562b09483de9bf3674e5c6005e9dc1581e0e9d7e Mon Sep 17 00:00:00 2001 From: Alexander Polyankin Date: Tue, 5 Dec 2023 23:34:17 +0200 Subject: [PATCH 09/19] Fix scrolling in filter picker (#36375) --- .../FilterPicker/FilterColumnPicker/FilterColumnPicker.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/metabase/querying/components/FilterPicker/FilterColumnPicker/FilterColumnPicker.tsx b/frontend/src/metabase/querying/components/FilterPicker/FilterColumnPicker/FilterColumnPicker.tsx index ddbf74986a389..4069c4a4359c1 100644 --- a/frontend/src/metabase/querying/components/FilterPicker/FilterColumnPicker/FilterColumnPicker.tsx +++ b/frontend/src/metabase/querying/components/FilterPicker/FilterColumnPicker/FilterColumnPicker.tsx @@ -110,6 +110,9 @@ export function FilterColumnPicker({ renderItemName={renderItemName} renderItemDescription={omitItemDescription} renderItemIcon={renderItemIcon} + // disable scrollbars inside the list + style={{ overflow: "visible" }} + maxHeight={Infinity} // Compat with E2E tests around MLv1-based components // Prefer using a11y role selectors itemTestId="dimension-list-item" From 117fee84eada327ab49449cd0b1105ae5b897364 Mon Sep 17 00:00:00 2001 From: Cam Saul <1455846+camsaul@users.noreply.github.com> Date: Tue, 5 Dec 2023 15:37:45 -0800 Subject: [PATCH 10/19] Return `:pk` drills for non-PK values in rows with multiple PKs (#36440) --- src/metabase/lib/drill_thru/common.cljc | 6 ++ src/metabase/lib/drill_thru/fk_details.cljc | 26 ++++++- .../lib/drill_thru/object_details.cljc | 61 +++------------ src/metabase/lib/drill_thru/pk.cljc | 74 +++++++++++++++++-- src/metabase/lib/drill_thru/zoom.cljc | 43 ++++++++++- src/metabase/lib/schema/drill_thru.cljc | 50 +++++++++---- test/metabase/lib/drill_thru/pk_test.cljc | 61 ++++++++++++++- .../drill_thru/underlying_records_test.cljc | 6 +- test/metabase/lib/drill_thru/zoom_test.cljc | 17 +++-- 9 files changed, 258 insertions(+), 86 deletions(-) diff --git a/src/metabase/lib/drill_thru/common.cljc b/src/metabase/lib/drill_thru/common.cljc index f93fdb22528e8..aff0be6b0b116 100644 --- a/src/metabase/lib/drill_thru/common.cljc +++ b/src/metabase/lib/drill_thru/common.cljc @@ -1,6 +1,7 @@ (ns metabase.lib.drill-thru.common (:require [metabase.lib.hierarchy :as lib.hierarchy] + [metabase.lib.metadata.calculation :as lib.metadata.calculation] [metabase.lib.util :as lib.util])) (defn mbql-stage? @@ -35,3 +36,8 @@ ;; Several drill-thrus are rendered as a fixed label for that type, with no reference to the column or value, ;; so the default is simply the drill-thru type. (select-keys drill-thru [:type :display-name])) + +(defn many-pks? + "Does the source table for this `query` have more than one primary key?" + [query] + (> (count (lib.metadata.calculation/primary-keys query)) 1)) diff --git a/src/metabase/lib/drill_thru/fk_details.cljc b/src/metabase/lib/drill_thru/fk_details.cljc index def998c992b3b..b02ee3773745d 100644 --- a/src/metabase/lib/drill_thru/fk_details.cljc +++ b/src/metabase/lib/drill_thru/fk_details.cljc @@ -2,13 +2,33 @@ "An FK details drill is one where you click a foreign key value in a table view e.g. ORDERS.USER_ID and choose the 'View details' option, then it shows you the PEOPLE record in question (e.g. Person 5 if USER_ID was 5). - [[metabase.lib.drill-thru.object-details/object-detail-drill]] has the logic for determining whether to return this - drill as an option or not." + We will only possibly return one of the 'object details' + drills ([[metabase.lib.drill-thru.pk]], [[metabase.lib.drill-thru.fk-details]], + or [[metabase.lib.drill-thru.zoom]]); see [[metabase.lib.drill-thru.object-details]] for the high-level logic that + calls out to the individual implementations." (:require [metabase.lib.drill-thru.common :as lib.drill-thru.common] [metabase.lib.filter :as lib.filter] [metabase.lib.metadata :as lib.metadata] - [metabase.lib.query :as lib.query])) + [metabase.lib.query :as lib.query] + [metabase.lib.schema :as lib.schema] + [metabase.lib.schema.drill-thru :as lib.schema.drill-thru] + [metabase.lib.types.isa :as lib.types.isa] + [metabase.util.malli :as mu])) + +(mu/defn fk-details-drill :- [:maybe ::lib.schema.drill-thru/drill-thru.fk-details] + "Return an `:fk-details` 'View details' drill when clicking on the value of a FK column." + [query :- ::lib.schema/query + _stage-number :- :int + {:keys [column value] :as _context} :- ::lib.schema.drill-thru/context] + (when (and (lib.types.isa/foreign-key? column) + (some? value) + (not= value :null)) + {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/fk-details + :column column + :object-id value + :many-pks? (lib.drill-thru.common/many-pks? query)})) (defmethod lib.drill-thru.common/drill-thru-info-method :drill-thru/fk-details [_query _stage-number drill-thru] diff --git a/src/metabase/lib/drill_thru/object_details.cljc b/src/metabase/lib/drill_thru/object_details.cljc index 1579ee724d779..e3585a1a2942f 100644 --- a/src/metabase/lib/drill_thru/object_details.cljc +++ b/src/metabase/lib/drill_thru/object_details.cljc @@ -1,52 +1,12 @@ (ns metabase.lib.drill-thru.object-details (:require - [medley.core :as m] - [metabase.lib.aggregation :as lib.aggregation] - [metabase.lib.drill-thru.common :as lib.drill-thru.common] - [metabase.lib.metadata.calculation :as lib.metadata.calculation] + [metabase.lib.drill-thru.fk-details :as lib.drill-thru.fk-details] + [metabase.lib.drill-thru.pk :as lib.drill-thru.pk] + [metabase.lib.drill-thru.zoom :as lib.drill-thru.zoom] [metabase.lib.schema :as lib.schema] [metabase.lib.schema.drill-thru :as lib.schema.drill-thru] - [metabase.lib.types.isa :as lib.types.isa] [metabase.util.malli :as mu])) -(defn- object-detail-drill-for [query stage-number {:keys [column row value] :as context} many-pks?] - (let [base {:lib/type :metabase.lib.drill-thru/drill-thru - :type :drill-thru/pk - :column column - :object-id value - :many-pks? many-pks?} - mbql-stage? (lib.drill-thru.common/mbql-stage? query stage-number)] - (cond - (and (lib.types.isa/primary-key? column) - many-pks? - mbql-stage? - (not= value :null)) - (assoc base :type :drill-thru/pk) - - ;; TODO: Figure out clicked.extraData and the dashboard flow. - (and (lib.types.isa/primary-key? column) - (not= value :null)) - (assoc base :type :drill-thru/zoom) - - (and (lib.types.isa/foreign-key? column) - (not= value :null)) - (assoc base :type :drill-thru/fk-details) - - (and (not many-pks?) - (not-empty row) - (empty? (lib.aggregation/aggregations query stage-number))) - (let [[pk-column] (lib.metadata.calculation/primary-keys query) ; Already know there's only one. - pk-value (->> row - (m/find-first #(-> % :column :name (= (:name pk-column)))) - :value)] - (when (and pk-value - ;; Only recurse if this is a different column - otherwise it's an infinite loop. - (not= (:name column) (:name pk-column))) - (object-detail-drill-for query - stage-number - (assoc context :column pk-column :value pk-value) - many-pks?)))))) - (mu/defn object-detail-drill :- [:maybe [:or ::lib.schema.drill-thru/drill-thru.pk ::lib.schema.drill-thru/drill-thru.zoom @@ -55,10 +15,11 @@ Contrast [[metabase.lib.drill-thru.fk-filter/fk-filter-drill]], which filters this query to only those rows with a specific value for a FK column." - [query :- ::lib.schema/query - stage-number :- :int - {:keys [column value] :as context} :- ::lib.schema.drill-thru/context] - (when (and column - (some? value)) - (object-detail-drill-for query stage-number context - (> (count (lib.metadata.calculation/primary-keys query)) 1)))) + [query :- ::lib.schema/query + stage-number :- :int + context :- ::lib.schema.drill-thru/context] + (some (fn [f] + (f query stage-number context)) + [lib.drill-thru.fk-details/fk-details-drill + lib.drill-thru.pk/pk-drill + lib.drill-thru.zoom/zoom-drill])) diff --git a/src/metabase/lib/drill_thru/pk.cljc b/src/metabase/lib/drill_thru/pk.cljc index accb9e2d058a4..0e719265bb10e 100644 --- a/src/metabase/lib/drill_thru/pk.cljc +++ b/src/metabase/lib/drill_thru/pk.cljc @@ -1,15 +1,75 @@ (ns metabase.lib.drill-thru.pk - "[[metabase.lib.drill-thru.object-details/object-detail-drill]] has the logic for determining whether to return this - drill as an option or not." + "A `:pk` drill is a 'View details' (AKA object details) drill that adds filter(s) for the value(s) of a PK(s). It is + only presented for Tables that have multiple PKs; for Tables with a single PK you'd instead + see [[metabase.lib.drill-thru.zoom]]. + + We will only possibly return one of the 'object details' + drills ([[metabase.lib.drill-thru.pk]], [[metabase.lib.drill-thru.fk-details]], + or [[metabase.lib.drill-thru.zoom]]); see [[metabase.lib.drill-thru.object-details]] for the high-level logic that + calls out to the individual implementations." (:require + [medley.core :as m] [metabase.lib.drill-thru.common :as lib.drill-thru.common] - [metabase.lib.filter :as lib.filter])) + [metabase.lib.filter :as lib.filter] + [metabase.lib.metadata.calculation :as lib.metadata.calculation] + [metabase.lib.schema :as lib.schema] + [metabase.lib.schema.drill-thru :as lib.schema.drill-thru] + [metabase.lib.types.isa :as lib.types.isa] + [metabase.util.malli :as mu])) + +(mu/defn pk-drill :- [:maybe ::lib.schema.drill-thru/drill-thru.pk] + "'View details' drill when you click on a value in a table that has MULTIPLE PKs. There are two subtypes of PK + drills: + + 1) if you click on a PK column value, then we return a drill that will add a filter for that PK column/value + + 2) if you click a non-PK column value, then we return a drill that will add filters for the PK columns/values in the + row. This is never returned for FK columns; we return [[metabase.lib.drill-thru.fk-details]] drills instead." + [query :- ::lib.schema/query + stage-number :- :int + {:keys [column value row] :as _context} :- ::lib.schema.drill-thru/context] + (when (and + ;; ignore column header clicks (value = nil). NULL values (value = :null) are ok if this is a click on a + ;; non-PK column. + (some? value) + (lib.drill-thru.common/mbql-stage? query stage-number) + ;; `:pk` drills are only for Tables with multiple PKs. For Tables with one PK, we do + ;; a [[metabase.lib.drill-thru.zoom]] drill instead. + (lib.drill-thru.common/many-pks? query) + ;; if this is an FK column we should return an [[metabase.lib.drill-thru.fk-details]] drill instead. + (not (lib.types.isa/foreign-key? column))) + (if (lib.types.isa/primary-key? column) + ;; 1) we clicked on a PK column: return a drill thru for that PK column + value. Ignore `nil` values. + (when (and (some? value) + (not= value :null)) + {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/pk + :dimensions [{:column column + :value value}]}) + ;; 2) we clicked on a non-PK column: return a drill for ALL of the PK columns + values. Ignore any + ;; `nil` (`:null`) values. + (let [pk-columns (lib.metadata.calculation/primary-keys query) + dimensions (for [pk-column pk-columns + :let [value (->> row + (m/find-first #(-> % :column :name (= (:name pk-column)))) + :value)] + ;; ignore any PKs that don't have a value in this row. + :when value] + {:column pk-column, :value value})] + (when (seq dimensions) + {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/pk + ;; return the dimensions sorted by column ID so the return value is determinate. + :dimensions (vec (sort-by #(get-in % [:column :id]) dimensions))}))))) (defmethod lib.drill-thru.common/drill-thru-info-method :drill-thru/pk [_query _stage-number drill-thru] - (select-keys drill-thru [:many-pks? :object-id :type])) + (select-keys drill-thru [:type :dimensions])) (defmethod lib.drill-thru.common/drill-thru-method :drill-thru/pk - [query stage-number {:keys [column object-id]} & _] - ;; This type is only used when there are multiple PKs and one was selected - [= pk x] filter. - (lib.filter/filter query stage-number (lib.filter/= column object-id))) + [query stage-number {:keys [dimensions], :as _pk-drill}] + (reduce + (fn [query {:keys [column value], :as _dimension}] + (lib.filter/filter query stage-number (lib.filter/= column value))) + query + dimensions)) diff --git a/src/metabase/lib/drill_thru/zoom.cljc b/src/metabase/lib/drill_thru/zoom.cljc index cd5d62349342e..43da553289986 100644 --- a/src/metabase/lib/drill_thru/zoom.cljc +++ b/src/metabase/lib/drill_thru/zoom.cljc @@ -1,12 +1,51 @@ (ns metabase.lib.drill-thru.zoom - "[[metabase.lib.drill-thru.object-details/object-detail-drill]] has the logic for determining whether to return this - drill as an option or not." + "A `:zoom` drill is a 'View details' drill when you click on the value of a PK column in a Table that has EXACTLY ONE + PK column. In MLv2, it is a no-op; in the frontend it changes the URL to take you to the 'object details' view for + the row in question. For Tables with multiple PK columns, a [[metabase.lib.drill-thru.pk]] drill is returned + instead. + + We will only possibly return one of the 'object details' + drills ([[metabase.lib.drill-thru.pk]], [[metabase.lib.drill-thru.fk-details]], + or [[metabase.lib.drill-thru.zoom]]); see [[metabase.lib.drill-thru.object-details]] for the high-level logic that + calls out to the individual implementations." (:require + [medley.core :as m] [metabase.lib.drill-thru.common :as lib.drill-thru.common] + [metabase.lib.metadata.calculation :as lib.metadata.calculation] [metabase.lib.schema :as lib.schema] [metabase.lib.schema.drill-thru :as lib.schema.drill-thru] + [metabase.lib.types.isa :as lib.types.isa] [metabase.util.malli :as mu])) +(defn- zoom-drill* [column value] + {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/zoom + :column column + :object-id value + :many-pks? false}) + +(mu/defn zoom-drill :- [:maybe ::lib.schema.drill-thru/drill-thru.zoom] + "Return a `:zoom` drill when clicking on the value of a PK column in a Table that has only one PK column." + [query :- ::lib.schema/query + _stage-number :- :int + {:keys [column value row] :as _context} :- ::lib.schema.drill-thru/context] + + (when (and + ;; ignore clicks on headers (value = nil rather than :null) + (some? value) + ;; if this table has more than one PK we should be returning a [[metabase.lib.drill-thru.pk]] instead. + (not (lib.drill-thru.common/many-pks? query))) + (if (lib.types.isa/primary-key? column) + ;; PK column was clicked. Ignore NULL values. + (when-not (= value :null) + (zoom-drill* column value)) + ;; some other column was clicked. Find the PK column and create a filter for its value. + (let [[pk-column] (lib.metadata.calculation/primary-keys query)] + (when-let [pk-value (->> row + (m/find-first #(-> % :column :name (= (:name pk-column)))) + :value)] + (zoom-drill* pk-column pk-value)))))) + (defmethod lib.drill-thru.common/drill-thru-info-method :drill-thru/zoom [_query _stage-number drill-thru] (select-keys drill-thru [:many-pks? :object-id :type])) diff --git a/src/metabase/lib/schema/drill_thru.cljc b/src/metabase/lib/schema/drill_thru.cljc index 01dfc29c5b0fd..f57205524e67f 100644 --- a/src/metabase/lib/schema/drill_thru.cljc +++ b/src/metabase/lib/schema/drill_thru.cljc @@ -39,18 +39,30 @@ [:map [:column [:ref ::lib.schema.metadata/column]]]]) -(mr/def ::drill-thru.object-details - [:merge - ::drill-thru.common.with-column - [:map - [:object-id :any] - [:many-pks? :boolean]]]) +;;; there are three "object details" drills: `:pk`, `:fk-details`, and `:zoom`. Originally, all three had `:column` +;;; and `:object-id` (value), but since we want `:pk` to handle multiple PKs (thus multiple columns and values) we +;;; changed it to instead have a list of `:dimensions` (similar in shape to `::context.row`, but without requiring +;;; `:column-ref`). I didn't change the other ones so as to avoid unintentionally breaking something in the middle of +;;; the drills epic. We should revisit these shapes in the future. See +;;; https://metaboat.slack.com/archives/C04CYTEL9N2/p1701803047600169 for more information. -- Cam + +(mr/def ::drill-thru.object-details.dimension + [:map + [:column [:ref ::lib.schema.metadata/column]] + ;; we should ignore NULL values for PKs and FKs -- do not add filters on them. + [:value [:and + :some + [:fn {:error/message "Non-NULL value"} #(not= % :null)]]]]) + +(mr/def ::drill-thru.object-details.dimensions + [:sequential {:min 1} [:ref ::drill-thru.object-details.dimension]]) (mr/def ::drill-thru.pk [:merge - ::drill-thru.object-details + ::drill-thru.common [:map - [:type [:= :drill-thru/pk]]]]) + [:type [:= :drill-thru/pk]] + [:dimensions [:ref ::drill-thru.object-details.dimensions]]]]) (mr/def ::drill-thru.fk-details.fk-column [:merge @@ -60,16 +72,22 @@ (mr/def ::drill-thru.fk-details [:merge - ::drill-thru.object-details + ::drill-thru.common.with-column [:map - [:type [:= :drill-thru/fk-details]] - [:column [:ref ::drill-thru.fk-details.fk-column]]]]) + [:type [:= :drill-thru/fk-details]] + [:column [:ref ::drill-thru.fk-details.fk-column]] + [:object-id :any] + [:many-pks? :boolean]]]) (mr/def ::drill-thru.zoom [:merge - ::drill-thru.object-details + ::drill-thru.common.with-column [:map - [:type [:= :drill-thru/zoom]]]]) + [:type [:= :drill-thru/zoom]] + [:object-id :any] + ;; TODO -- I don't think we really need this because there is no situation in which this isn't `false`, if it were + ;; true we'd return a `::drill-thru.pk` drill instead. See if we can remove this key without breaking the FE. + [:many-pks? [:= false]]]]) (mr/def ::drill-thru.quick-filter.operator [:map @@ -144,6 +162,8 @@ [:query [:ref ::lib.schema/query]] [:stage-number number?]]]) +;;; TODO FIXME -- it seems like underlying records drills also include `:dimensions` and `:column-ref`... +;;; see [[metabase.lib.drill-thru.underlying-records/underlying-records-drill]]... this should be part of the schema (mr/def ::drill-thru.underlying-records [:merge ::drill-thru.common @@ -279,7 +299,9 @@ [:map [:column [:ref ::lib.schema.metadata/column]] [:column-ref [:ref ::lib.schema.ref/ref]] - [:value :any]]) + [:value [:fn + {:error/message ":null should not be used in context row values, only for top-level context value"} + #(not= % :null)]]]) ;;; Sequence of maps with keys `:column`, `:column-ref`, and `:value` ;;; diff --git a/test/metabase/lib/drill_thru/pk_test.cljc b/test/metabase/lib/drill_thru/pk_test.cljc index ae9577e42cfde..76f3f06c50cf3 100644 --- a/test/metabase/lib/drill_thru/pk_test.cljc +++ b/test/metabase/lib/drill_thru/pk_test.cljc @@ -4,7 +4,8 @@ [medley.core :as m] [metabase.lib.core :as lib] [metabase.lib.test-metadata :as meta] - [metabase.lib.test-util :as lib.tu])) + [metabase.lib.test-util :as lib.tu] + #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal])))) (deftest ^:parallel do-not-return-pk-for-nil-test (testing "do not return pk drills for nil cell values (#36126)" @@ -22,3 +23,61 @@ :value nil}]}] (is (not (m/find-first #(= (:type %) :drill-thru/pk) (lib/available-drill-thrus query -1 context))))))) + +(deftest ^:parallel return-pk-drill-for-query-with-multiple-pks-on-non-pk-columns-click-test + (testing "should drill thru a non-PK and non-FK cell when there are multiple PK columns (#35618)" + ;; simulate a table with multiple PK columns: mark orders.product-id as a PK column + (let [metadata-provider (lib.tu/merged-mock-metadata-provider + meta/metadata-provider + {:fields [{:id (meta/id :orders :product-id) + :semantic-type :type/PK}]}) + query (lib/query metadata-provider (meta/table-metadata :orders)) + context-with-values (fn [column-value pk-1-value pk-2-value] + {:column (meta/field-metadata :orders :total) + :value column-value + :column-ref (lib/ref (meta/field-metadata :orders :total)) + :row [{:column (meta/field-metadata :orders :id) + :column-ref (lib/ref (meta/field-metadata :orders :id)) + :value pk-1-value} + {:column (meta/field-metadata :orders :product-id) + :column-ref (lib/ref (meta/field-metadata :orders :product-id)) + :value pk-2-value} + {:column (meta/field-metadata :orders :total) + :column-ref (lib/ref (meta/field-metadata :orders :total)) + :value (when-not (= column-value :null) + column-value)}]}) + find-drill (fn [query context] + (m/find-first #(= (:type %) :drill-thru/pk) + (lib/available-drill-thrus query -1 context)))] + (testing "both PKs have values" + (doseq [column-value [10 :null]] + (let [context (context-with-values column-value 100 200) + drill (find-drill query context)] + (is (=? {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/pk + :dimensions [{:column {:name "ID"} + :value 100} + {:column {:name "PRODUCT_ID"} + :value 200}]} + drill)) + (testing "Should add one filter for each PK column (#36426)" + (is (=? {:stages [{:filters [[:= {} [:field {} (meta/id :orders :id)] 100] + [:= {} [:field {} (meta/id :orders :product-id)] 200]]}]} + (lib/drill-thru query -1 drill))))))) + (testing "only one PK has a value: ignore the PK without a value" + (let [context (context-with-values 10 100 nil) + drill (find-drill query context)] + (is (=? {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/pk + :dimensions [{:column {:name "ID"} + :value 100}]} + drill)) + (testing "\nShould add one filter for each PK column (#36426)" + (is (=? {:stages [{:filters [[:= {} [:field {} (meta/id :orders :id)] 100]]}]} + (lib/drill-thru query -1 drill)))))) + (testing "neither PK has a value: don't return a drill at all" + (let [context (context-with-values 10 nil nil)] + (is (nil? (find-drill query context))))) + (testing "ignore header clicks (column value = nil, NOT :null)" + (let [context (context-with-values nil 100 200)] + (is (nil? (find-drill query context)))))))) diff --git a/test/metabase/lib/drill_thru/underlying_records_test.cljc b/test/metabase/lib/drill_thru/underlying_records_test.cljc index e00017bc106aa..d4e7895fca3e5 100644 --- a/test/metabase/lib/drill_thru/underlying_records_test.cljc +++ b/test/metabase/lib/drill_thru/underlying_records_test.cljc @@ -96,7 +96,11 @@ :column-ref (lib/ref breakout) :value value}) context (merge agg-dim - {:row (cons agg-dim breakout-dims) + ;; rows aren't supposed to use `:null`, so change them to `nil` instead. + {:row (for [value (cons agg-dim breakout-dims)] + (update value :value (fn [v] + (when-not (= v :null) + v)))) :dimensions breakout-dims})] (is (=? {:lib/type :mbql/query :stages [{:filters (exp-filters-fn agg-dim breakout-dims) diff --git a/test/metabase/lib/drill_thru/zoom_test.cljc b/test/metabase/lib/drill_thru/zoom_test.cljc index e39134ffaf98d..7f27a459f59fd 100644 --- a/test/metabase/lib/drill_thru/zoom_test.cljc +++ b/test/metabase/lib/drill_thru/zoom_test.cljc @@ -17,14 +17,15 @@ :many-pks? false}})) (deftest ^:parallel returns-zoom-test-2 - (lib.drill-thru.tu/test-returns-drill - {:drill-type :drill-thru/zoom - :click-type :cell - :query-type :unaggregated - :column-name "TAX" - :expected {:type :drill-thru/zoom - :object-id (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "ID"]) - :many-pks? false}})) + (testing ":zoom drill should get returned when you click on a non-PK column in a Table with one PK" + (lib.drill-thru.tu/test-returns-drill + {:drill-type :drill-thru/zoom + :click-type :cell + :query-type :unaggregated + :column-name "TAX" + :expected {:type :drill-thru/zoom + :object-id (get-in lib.drill-thru.tu/test-queries ["ORDERS" :unaggregated :row "ID"]) + :many-pks? false}}))) (deftest ^:parallel returns-zoom-test-3 (lib.drill-thru.tu/test-returns-drill From 51757c4a01f7985f429e727075083dd346f67136 Mon Sep 17 00:00:00 2001 From: Cam Saul <1455846+camsaul@users.noreply.github.com> Date: Tue, 5 Dec 2023 18:11:20 -0800 Subject: [PATCH 11/19] Return row count: 2 instead of negative row count (#36389) --- .../lib/drill_thru/underlying_records.cljc | 11 +++-- .../drill_thru/underlying_records_test.cljc | 48 +++++++++++++++++-- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/metabase/lib/drill_thru/underlying_records.cljc b/src/metabase/lib/drill_thru/underlying_records.cljc index 57ae382c64ad2..a0e810002c405 100644 --- a/src/metabase/lib/drill_thru/underlying_records.cljc +++ b/src/metabase/lib/drill_thru/underlying_records.cljc @@ -23,9 +23,9 @@ There is another quite different case: clicking the legend of a chart with multiple bars or lines broken out by category. Then `column` is nil!" - [query :- ::lib.schema/query - stage-number :- :int - {:keys [column column-ref dimensions value]} :- ::lib.schema.drill-thru/context] + [query :- ::lib.schema/query + stage-number :- :int + {:keys [column column-ref dimensions value], :as _context} :- ::lib.schema.drill-thru/context] ;; Clicking on breakouts is weird. Clicking on Count(People) by State: Minnesota yields a FE `clicked` with: ;; - column is COUNT ;; - row[0] has col: STATE, value: "Minnesota" @@ -50,7 +50,10 @@ :type :drill-thru/underlying-records ;; TODO: This is a bit confused for non-COUNT aggregations. Perhaps it should just always be 10 or something? ;; Note that some languages have different plurals for exactly 2, or for 1, 2-5, and 6+. - :row-count (if (number? value) value 2) + :row-count (if (and (number? value) + (not (neg? value))) + value + 2) :table-name (when-let [table-or-card (or (some->> query lib.util/source-table-id (lib.metadata/table query)) (some->> query lib.util/source-card-id (lib.metadata/card query)))] (lib.metadata.calculation/display-name query stage-number table-or-card)) diff --git a/test/metabase/lib/drill_thru/underlying_records_test.cljc b/test/metabase/lib/drill_thru/underlying_records_test.cljc index d4e7895fca3e5..9cdf51572e77b 100644 --- a/test/metabase/lib/drill_thru/underlying_records_test.cljc +++ b/test/metabase/lib/drill_thru/underlying_records_test.cljc @@ -12,7 +12,10 @@ [metabase.lib.test-util.metadata-providers.mock :as providers.mock] [metabase.util :as u] #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]) - :clj ([java-time.api :as jt])))) + :clj ([java-time.api :as jt] + [metabase.util.malli.fn :as mu.fn])))) + +#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me)) (deftest ^:parallel returns-underlying-records-test-1 (lib.drill-thru.tu/test-returns-drill @@ -68,9 +71,7 @@ drills)))))))))) -#?(:cljs (comment metabase.test-runner.assert-exprs.approximately-equal/keep-me)) - -(def last-month +(def ^:private last-month #?(:cljs (let [now (js/Date.) year (.getFullYear now) month (.getMonth now)] @@ -301,3 +302,42 @@ :breakout (symbol "nil #_\"key is not present.\"") :fields (symbol "nil #_\"key is not present.\"")}]} (lib.drill-thru/drill-thru query -1 drill)))))) + +(deftest ^:parallel negative-aggregation-values-display-info-test + (testing "should use the default row count for aggregations with negative values (#36143)" + (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders)) + (lib/aggregate (lib/count)) + (lib/breakout (lib.metadata/field lib.tu/metadata-provider-with-mock-cards + (meta/id :orders :created-at)))) + count-col (m/find-first #(= (:name %) "count") + (lib/returned-columns query)) + _ (is (some? count-col)) + context {:column count-col + :column-ref (lib/ref count-col) + :value -10, + :row [{:column (meta/field-metadata :orders :created-at) + :column-ref (lib/ref (meta/field-metadata :orders :created-at)) + :value "2020-01-01"} + {:column count-col + :column-ref (lib/ref count-col) + :value -10}] + :dimensions [{:column (meta/field-metadata :orders :created-at) + :column-ref (lib/ref (meta/field-metadata :orders :created-at)), + :value "2020-01-01"}]} + drill (m/find-first #(= (:type %) :drill-thru/underlying-records) + (lib/available-drill-thrus query -1 context))] + (is (=? {:lib/type :metabase.lib.drill-thru/drill-thru + :type :drill-thru/underlying-records + :row-count 2 + :table-name "Orders" + :dimensions [{:column {:name "CREATED_AT"} + :column-ref [:field {} (meta/id :orders :created-at)] + :value "2020-01-01"}] + :column-ref [:aggregation {} string?]} + drill)) + ;; display info currently doesn't include a `display-name` for drill thrus... we can fix this later. + (binding #?(:clj [mu.fn/*enforce* false] :cljs []) + (is (=? {:type :drill-thru/underlying-records + :row-count 2 + :table-name "Orders"} + (lib/display-info query -1 drill))))))) From d4f85c244ca0c48fbc61356b5660c08cce0a0fd6 Mon Sep 17 00:00:00 2001 From: "Mahatthana (Kelvin) Nomsawadi" Date: Wed, 6 Dec 2023 12:40:08 +0700 Subject: [PATCH 12/19] Fix click behavior link to dashboards/questions not using client router (#35975) * Fix click behavior link to dashboards/questions not using client router * Make the code cleaner * Fix unit tests * Make API clearer * Address review: Handle case insensitive subpath * Add E2E tests * Fix failed E2E tests * Fix another fail E2E test * Add a test to ensure invalid URL won't break navigation * Add tests + ensure multiple level subpath works --- .../dashboard-cards/click-behavior.cy.spec.js | 75 +++++++ frontend/src/metabase/lib/dom.js | 56 +++++- .../src/metabase/visualizations/lib/action.js | 2 +- .../visualizations/lib/action.unit.spec.ts | 186 +++++++++++++++++- 4 files changed, 310 insertions(+), 9 deletions(-) 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 814e713ac8b0c..ecb0271064075 100644 --- a/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js +++ b/e2e/test/scenarios/dashboard-cards/click-behavior.cy.spec.js @@ -2,6 +2,7 @@ import { USER_GROUPS } from "e2e/support/cypress_data"; import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database"; import { addOrUpdateDashboardCard, + dashboardHeader, editDashboard, getActionCardDetails, getDashboardCard, @@ -203,6 +204,13 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { saveDashboard(); + cy.intercept( + "GET", + "/api/collection/root", + cy.spy().as("rootCollection"), + ); + cy.intercept("GET", "/api/collection", cy.spy().as("collections")); + clickLineChartPoint(); cy.get("@targetDashboardId").then(targetDashboardId => { cy.location().should(({ pathname, search }) => { @@ -210,6 +218,13 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { expect(search).to.equal(""); }); }); + + cy.log("Should navigate to question using router (metabase#33379)"); + dashboardHeader().findByText(TARGET_DASHBOARD.name).should("be.visible"); + // If the page was reloaded, many API request would have been made and theses + // calls are 2 of those. + cy.get("@rootCollection").should("not.have.been.called"); + cy.get("@collections").should("not.have.been.called"); }); it("allows setting dashboard with single parameter as custom destination", () => { @@ -577,6 +592,13 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { saveDashboard(); + cy.intercept( + "GET", + "/api/collection/root", + cy.spy().as("rootCollection"), + ); + cy.intercept("GET", "/api/collection", cy.spy().as("collections")); + clickLineChartPoint(); cy.location().should(({ hash, pathname }) => { expect(pathname).to.equal("/question"); @@ -586,6 +608,13 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { expect(card.dataset_query.query).to.deep.equal(TARGET_QUESTION.query); }); + cy.log("Should navigate to question using router (metabase#33379)"); + cy.findByTestId("view-footer").should("contain", "Showing 5 rows"); + // If the page was reloaded, many API request would have been made and theses + // calls are 2 of those. + cy.get("@rootCollection").should("not.have.been.called"); + cy.get("@collections").should("not.have.been.called"); + cy.go("back"); testChangingBackToDefaultBehavior(); }); @@ -1433,6 +1462,52 @@ describe("scenarios > dashboard > dashboard cards > click behavior", () => { clickLineChartPoint(); }); + it("allows opening custom URL destination that is not a Metabase instance URL using link (metabase#33379)", () => { + cy.request("PUT", "/api/setting/site-url", { + value: "https://localhost:4000/subpath", + }); + const dashboardDetails = { + enable_embedding: true, + }; + + const metabaseInstanceUrl = "http://localhost:4000"; + cy.createQuestionAndDashboard({ + questionDetails, + dashboardDetails, + }).then(({ body: card }) => { + addOrUpdateDashboardCard({ + dashboard_id: card.dashboard_id, + card_id: card.card_id, + card: { + id: card.id, + visualization_settings: { + click_behavior: { + type: "link", + linkType: "url", + linkTemplate: `${metabaseInstanceUrl}/404`, + }, + }, + }, + }); + + visitEmbeddedPage({ + resource: { dashboard: card.dashboard_id }, + params: {}, + }); + cy.wait("@dashboard"); + cy.wait("@cardQuery"); + }); + + clickLineChartPoint(); + + cy.log( + "This is app 404 page, the embed 404 page will have different copy", + ); + cy.findByRole("main") + .findByText("The page you asked for couldn't be found.") + .should("be.visible"); + }); + it("allows updating multiple dashboard filters", () => { const dashboardDetails = { parameters: [DASHBOARD_FILTER_TEXT, DASHBOARD_FILTER_TIME], diff --git a/frontend/src/metabase/lib/dom.js b/frontend/src/metabase/lib/dom.js index c01cd2b962ba1..4b40c7b70f442 100644 --- a/frontend/src/metabase/lib/dom.js +++ b/frontend/src/metabase/lib/dom.js @@ -220,7 +220,25 @@ export function constrainToScreen(element, direction, padding) { return false; } -const isAbsoluteUrl = url => url.startsWith("/"); +function getSitePath() { + return new URL(MetabaseSettings.get("site-url")).pathname.toLowerCase(); +} + +function isMetabaseUrl(url) { + const urlPath = new URL(url, window.location.origin).pathname.toLowerCase(); + + if (!isAbsoluteUrl(url)) { + return true; + } + + return isSameOrSiteUrlOrigin(url) && urlPath.startsWith(getSitePath()); +} + +function isAbsoluteUrl(url) { + return ["/", "http:", "https:", "mailto:"].some(prefix => + url.startsWith(prefix), + ); +} function getWithSiteUrl(url) { const siteUrl = MetabaseSettings.get("site-url"); @@ -272,21 +290,20 @@ export function open( // custom function for opening in new window openInBlankWindow = url => clickLink(url, true), // custom function for opening in same app instance - openInSameOrigin = openInSameWindow, + openInSameOrigin, ignoreSiteUrl = false, ...options } = {}, ) { - const isOriginalUrlAbsolute = isAbsoluteUrl(url); // this does not check real "absolute" url, but if a url should be resolved from the root URL url = ignoreSiteUrl ? url : getWithSiteUrl(url); if (shouldOpenInBlankWindow(url, options)) { openInBlankWindow(url); } else if (isSameOrigin(url)) { - if (isOriginalUrlAbsolute) { + if (!isMetabaseUrl(url)) { clickLink(url, false); } else { - openInSameOrigin(url, getLocation(url)); + openInSameOrigin(getLocation(url)); } } else { openInSameWindow(url); @@ -350,12 +367,39 @@ const getLocation = url => { try { const { pathname, search, hash } = new URL(url, window.location.origin); const query = querystring.parse(search.substring(1)); - return { pathname, search, query, hash }; + return { + pathname: getPathnameWithoutSubPath(pathname), + search, + query, + hash, + }; } catch { return {}; } }; +function getPathnameWithoutSubPath(pathname) { + const pathnameSections = pathname.split("/"); + const sitePathSections = getSitePath().split("/"); + + return isPathnameContainSitePath(pathnameSections, sitePathSections) + ? "/" + pathnameSections.slice(sitePathSections.length).join("/") + : pathname; +} + +function isPathnameContainSitePath(pathnameSections, sitePathSections) { + for (let index = 0; index < sitePathSections.length; index++) { + const sitePathSection = sitePathSections[index].toLowerCase(); + const pathnameSection = pathnameSections[index].toLowerCase(); + + if (sitePathSection !== pathnameSection) { + return false; + } + } + + return true; +} + export function isSameOrigin(url) { const origin = getOrigin(url); return origin == null || origin === window.location.origin; diff --git a/frontend/src/metabase/visualizations/lib/action.js b/frontend/src/metabase/visualizations/lib/action.js index 4d211e9106089..2770ba52012f0 100644 --- a/frontend/src/metabase/visualizations/lib/action.js +++ b/frontend/src/metabase/visualizations/lib/action.js @@ -17,7 +17,7 @@ export function performAction(action, { dispatch, onChangeCardAndRun }) { const ignoreSiteUrl = action.ignoreSiteUrl; if (url) { open(url, { - openInSameOrigin: (url, location) => { + openInSameOrigin: location => { dispatch(push(location)); dispatch(setParameterValuesFromQueryParams(location.query)); }, diff --git a/frontend/src/metabase/visualizations/lib/action.unit.spec.ts b/frontend/src/metabase/visualizations/lib/action.unit.spec.ts index 20c6e35403800..1f246d3d8adae 100644 --- a/frontend/src/metabase/visualizations/lib/action.unit.spec.ts +++ b/frontend/src/metabase/visualizations/lib/action.unit.spec.ts @@ -1,13 +1,15 @@ +import MetabaseSettings from "metabase/lib/settings"; import type { UrlClickAction } from "metabase/visualizations/types"; import { performAction } from "./action"; describe("performAction", () => { it('should redirect using router if a "relative" url has been passed', () => { + MetabaseSettings.set("site-url", "http://localhost"); const action: UrlClickAction = { buttonType: "horizontal", name: "automatic-insights", section: "auto", - url: jest.fn(() => "auto/dashboard/adhoc/123"), + url: jest.fn(() => "auto/dashboard/adhoc/123Abc"), }; const extraProps = { @@ -25,7 +27,7 @@ describe("performAction", () => { args: [ { hash: "", - pathname: "/auto/dashboard/adhoc/123", + pathname: "/auto/dashboard/adhoc/123Abc", query: {}, search: "", }, @@ -35,4 +37,184 @@ describe("performAction", () => { type: "@@router/CALL_HISTORY_METHOD", }); }); + + describe.each([ + { + sitePath: "http://localhost", + name: "without subpath without trailing slash", + }, + { + sitePath: "http://localhost/", + name: "without subpath with trailing slash", + }, + { + sitePath: "http://localhost/Metabase", + name: "with subpath without trailing slash", + }, + { + sitePath: "http://localhost/Metabase/", + name: "with subpath with trailing slash", + }, + { + sitePath: "http://localhost/Metabase/path", + name: "with 2 levels deep subpath without trailing slash", + }, + { + sitePath: "http://localhost/Metabase/path/", + name: "with 2 levels deep subpath with trailing slash", + }, + ])(`when site URL is $name`, ({ sitePath }) => { + it.each(["/", "/question/1", "/question/1/"])( + `should redirect using router when using a relative URL with leading slash: "%s"`, + url => { + MetabaseSettings.set("site-url", sitePath); + const action: UrlClickAction = { + buttonType: "horizontal", + name: "automatic-insights", + section: "auto", + url: jest.fn(() => url), + }; + + const extraProps = { + dispatch: jest.fn(), + onChangeCardAndRun: jest.fn(), + }; + + expect(performAction(action, extraProps)).toBe(true); + + expect(action.url).toHaveBeenCalledTimes(1); + + expect(extraProps.dispatch).toHaveBeenCalledTimes(2); + expect(extraProps.dispatch).toHaveBeenCalledWith({ + payload: { + args: [ + { + hash: "", + pathname: url, + query: {}, + search: "", + }, + ], + method: "push", + }, + type: "@@router/CALL_HISTORY_METHOD", + }); + }, + ); + + it.each(["auto/dashboard/adhoc/123Abc", "auto/dashboard/adhoc/123Abc/"])( + `should redirect using router when using a relative URL: "%s"`, + url => { + MetabaseSettings.set("site-url", sitePath); + const action: UrlClickAction = { + buttonType: "horizontal", + name: "automatic-insights", + section: "auto", + url: jest.fn(() => url), + }; + + const extraProps = { + dispatch: jest.fn(), + onChangeCardAndRun: jest.fn(), + }; + + expect(performAction(action, extraProps)).toBe(true); + + expect(action.url).toHaveBeenCalledTimes(1); + + expect(extraProps.dispatch).toHaveBeenCalledTimes(2); + expect(extraProps.dispatch).toHaveBeenCalledWith({ + payload: { + args: [ + { + hash: "", + pathname: "/" + url, + query: {}, + search: "", + }, + ], + method: "push", + }, + type: "@@router/CALL_HISTORY_METHOD", + }); + }, + ); + + it("should redirect using router when using an absolute URL containing site URL", () => { + MetabaseSettings.set("site-url", sitePath); + const action: UrlClickAction = { + buttonType: "horizontal", + name: "automatic-insights", + section: "auto", + url: jest.fn( + // This ensures that even if the subpath has different cases, it will still work + () => `${sitePath.toLowerCase()}/auto/dashboard/adhoc/123Abc`, + ), + }; + + const extraProps = { + dispatch: jest.fn(), + onChangeCardAndRun: jest.fn(), + }; + + expect(performAction(action, extraProps)).toBe(true); + + expect(action.url).toHaveBeenCalledTimes(1); + + expect(extraProps.dispatch).toHaveBeenCalledTimes(2); + expect(extraProps.dispatch).toHaveBeenCalledWith({ + payload: { + args: [ + { + hash: "", + pathname: "/auto/dashboard/adhoc/123Abc", + query: {}, + search: "", + }, + ], + method: "push", + }, + type: "@@router/CALL_HISTORY_METHOD", + }); + }); + + it("should redirect with invalid URL", () => { + MetabaseSettings.set("site-url", sitePath); + const action: UrlClickAction = { + buttonType: "horizontal", + name: "automatic-insights", + section: "auto", + url: jest.fn( + () => + "invalid_protocol://localhost/metabase/auto/dashboard/adhoc/123Abc", + ), + }; + + const extraProps = { + dispatch: jest.fn(), + onChangeCardAndRun: jest.fn(), + }; + + expect(performAction(action, extraProps)).toBe(true); + + expect(action.url).toHaveBeenCalledTimes(1); + + expect(extraProps.dispatch).toHaveBeenCalledTimes(2); + expect(extraProps.dispatch).toHaveBeenCalledWith({ + payload: { + args: [ + { + hash: "", + pathname: + "/invalid_protocol://localhost/metabase/auto/dashboard/adhoc/123Abc", + query: {}, + search: "", + }, + ], + method: "push", + }, + type: "@@router/CALL_HISTORY_METHOD", + }); + }); + }); }); From 18742132b608d15e3298d8a5dae2377a3f574a66 Mon Sep 17 00:00:00 2001 From: Levente Kurusa <849140+levex@users.noreply.github.com> Date: Wed, 6 Dec 2023 07:00:09 +0100 Subject: [PATCH 13/19] team: add levex to BEC (#36434) --- .github/team.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/team.json b/.github/team.json index 30e4c20c8b933..1c4b0f3ea8624 100644 --- a/.github/team.json +++ b/.github/team.json @@ -27,7 +27,8 @@ "tsmacdonald", "qnkhuat", "calherries", - "piranha" + "piranha", + "levex" ] }, { From 2ffee750cb9ccc3bcae02632a976d6893a296e23 Mon Sep 17 00:00:00 2001 From: Denis Berezin Date: Wed, 6 Dec 2023 10:35:51 +0300 Subject: [PATCH 14/19] Set embedding visual settings before initial render (#36095) * Move embedding settings prep before rendering * Revert not needed change * Add dynamic embedding settings for preview mode, add e2e tests for embedding settings * Refactor test, remove not needed code * Updates for e2e test based on code review --- e2e/support/helpers/e2e-embedding-helpers.js | 14 +++++ ...6988-embedding-dynamic-settings.cy.spec.js | 56 +++++++++++++++++++ .../pivot_tables.cy.spec.js | 11 +--- frontend/src/metabase/app.js | 4 +- frontend/src/metabase/lib/embed.js | 9 +-- .../components/widgets/DisplayOptionsPane.jsx | 10 +++- frontend/src/metabase/public/lib/types.js | 0 frontend/src/metabase/redux/embed.ts | 11 +++- 8 files changed, 92 insertions(+), 23 deletions(-) create mode 100644 e2e/test/scenarios/sharing/reproductions/26988-embedding-dynamic-settings.cy.spec.js delete mode 100644 frontend/src/metabase/public/lib/types.js diff --git a/e2e/support/helpers/e2e-embedding-helpers.js b/e2e/support/helpers/e2e-embedding-helpers.js index 2e81e8d34cc1c..7e3e25d91044b 100644 --- a/e2e/support/helpers/e2e-embedding-helpers.js +++ b/e2e/support/helpers/e2e-embedding-helpers.js @@ -114,3 +114,17 @@ export function visitIframe() { cy.visit(iframe.src); }); } + +/** + * Get page iframe body wrapped in `cy` helper + * @param {string} [selector] + */ +export function getIframeBody(selector = "iframe") { + return cy + .get(selector) + .its("0.contentDocument") + .should("exist") + .its("body") + .should("not.be.null") + .then(cy.wrap); +} diff --git a/e2e/test/scenarios/sharing/reproductions/26988-embedding-dynamic-settings.cy.spec.js b/e2e/test/scenarios/sharing/reproductions/26988-embedding-dynamic-settings.cy.spec.js new file mode 100644 index 0000000000000..8623797414054 --- /dev/null +++ b/e2e/test/scenarios/sharing/reproductions/26988-embedding-dynamic-settings.cy.spec.js @@ -0,0 +1,56 @@ +import { + describeEE, + getIframeBody, + popover, + restore, + setTokenFeatures, + visitDashboard, +} from "e2e/support/helpers"; +import { ORDERS_ID } from "metabase-types/api/mocks/presets"; + +describeEE("issue 26988", () => { + beforeEach(() => { + restore(); + cy.intercept("GET", "/api/embed/dashboard/*").as("dashboard"); + + cy.signInAsAdmin(); + setTokenFeatures("all"); + }); + + it("should apply embedding settings passed in URL on load", () => { + cy.createQuestionAndDashboard({ + questionDetails: { + name: "Q1", + query: { + "source-table": ORDERS_ID, + limit: 3, + }, + }, + dashboardDetails: { + enable_embedding: true, + }, + }).then(({ body: card }) => { + visitDashboard(card.dashboard_id); + }); + + cy.icon("share").click(); + cy.findByRole("button", { name: "Set up" }).click(); + cy.wait("@dashboard"); + + getIframeBody().should("have.css", "font-family", `Lato, sans-serif`); + + cy.findByTestId("embedding-settings").findByLabelText("Font").click(); + popover().findByText("Oswald").click(); + cy.wait("@dashboard"); + getIframeBody().should("have.css", "font-family", `Oswald, sans-serif`); + + cy.findByTestId("embedding-settings").findByLabelText("Font").click(); + popover().findByText("Slabo 27px").click(); + cy.wait("@dashboard"); + getIframeBody().should( + "have.css", + "font-family", + `"Slabo 27px", sans-serif`, + ); + }); +}); diff --git a/e2e/test/scenarios/visualizations-tabular/pivot_tables.cy.spec.js b/e2e/test/scenarios/visualizations-tabular/pivot_tables.cy.spec.js index dead17f4071a2..e9fa3aee70173 100644 --- a/e2e/test/scenarios/visualizations-tabular/pivot_tables.cy.spec.js +++ b/e2e/test/scenarios/visualizations-tabular/pivot_tables.cy.spec.js @@ -10,6 +10,7 @@ import { leftSidebar, main, modal, + getIframeBody, } from "e2e/support/helpers"; import { SAMPLE_DB_ID } from "e2e/support/cypress_data"; @@ -1241,16 +1242,6 @@ function dragColumnHeader(el, xDistance = 50) { }); } -function getIframeBody(selector = "iframe") { - return cy - .get(selector) - .its("0.contentDocument") - .should("exist") - .its("body") - .should("not.be.null") - .then(cy.wrap); -} - function openColumnSettings(columnName) { sidebar() .findByText(columnName) diff --git a/frontend/src/metabase/app.js b/frontend/src/metabase/app.js index 3d548bd4b4945..57a4b7131dae5 100644 --- a/frontend/src/metabase/app.js +++ b/frontend/src/metabase/app.js @@ -73,6 +73,8 @@ function _init(reducers, getRoutes, callback) { createTracker(store); + initializeEmbedding(store); + ReactDOM.render( (root = ref)}> @@ -89,8 +91,6 @@ function _init(reducers, getRoutes, callback) { registerVisualizations(); - initializeEmbedding(store); - store.dispatch(refreshSiteSettings()); PLUGIN_APP_INIT_FUCTIONS.forEach(init => init({ root })); diff --git a/frontend/src/metabase/lib/embed.js b/frontend/src/metabase/lib/embed.js index 0a35fa8b6825d..ee18ac03c2621 100644 --- a/frontend/src/metabase/lib/embed.js +++ b/frontend/src/metabase/lib/embed.js @@ -1,6 +1,5 @@ import { push } from "react-router-redux"; import _ from "underscore"; -import { parseSearchOptions, parseHashOptions } from "metabase/lib/browser"; import { isWithinIframe, IFRAMED_IN_SELF } from "metabase/lib/dom"; import { setOptions } from "metabase/redux/embed"; import { isFitViewportMode } from "metabase/hoc/FitViewPort"; @@ -41,12 +40,8 @@ export function initializeEmbedding(store) { } } }); - store.dispatch( - setOptions({ - ...parseSearchOptions(window.location.search), - ...parseHashOptions(window.location.hash), - }), - ); + + store.dispatch(setOptions(window.location)); } } diff --git a/frontend/src/metabase/public/components/widgets/DisplayOptionsPane.jsx b/frontend/src/metabase/public/components/widgets/DisplayOptionsPane.jsx index bbaa853e1f79c..1f5e7c2734e90 100644 --- a/frontend/src/metabase/public/components/widgets/DisplayOptionsPane.jsx +++ b/frontend/src/metabase/public/components/widgets/DisplayOptionsPane.jsx @@ -37,6 +37,7 @@ const DisplayOptionsPane = ({ showDownloadDataButtonVisibilityToggle, }) => { const toggleId = useUniqueId("show-download-data-button"); + const fontControlLabelId = useUniqueId("display-option"); return (
@@ -78,7 +79,7 @@ const DisplayOptionsPane = ({ {canWhitelabel && ( <> - +