From 7991679b35b118c226e242e98093ef7155c242e4 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:13:27 -0400 Subject: [PATCH 01/10] wip Removing database prompt banner --- .../DatabasePromptBanner.styled.ts | 60 ----- .../DatabasePromptBanner.tsx | 64 ----- .../DatabasePromptBanner.unit.spec.tsx | 249 ------------------ .../components/DatabasePromptBanner/index.ts | 1 - 4 files changed, 374 deletions(-) delete mode 100644 frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.styled.ts delete mode 100644 frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.tsx delete mode 100644 frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.unit.spec.tsx delete mode 100644 frontend/src/metabase/nav/components/DatabasePromptBanner/index.ts diff --git a/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.styled.ts b/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.styled.ts deleted file mode 100644 index 357168da63320..0000000000000 --- a/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.styled.ts +++ /dev/null @@ -1,60 +0,0 @@ -import styled from "@emotion/styled"; - -import Button from "metabase/core/components/Button/Button"; -import ExternalLink from "metabase/core/components/ExternalLink"; -import { color } from "metabase/lib/colors"; -import { breakpointMinSmall } from "metabase/styled-components/theme"; - -// This color is only used here, so I'm reluctant to add it to the color palette. -const VIBRANT_BLUE = "#1888EC"; - -export const DatabasePromptBannerRoot = styled.div` - background: ${VIBRANT_BLUE}; - color: ${color("white")}; - display: flex; - align-items: center; - flex-wrap: wrap; - - ${breakpointMinSmall} { - flex-wrap: nowrap; - } -`; - -export const Prompt = styled.div` - margin: 1rem 1.5rem 0.625rem; - width: 100%; - - ${breakpointMinSmall} { - width: auto; - margin-bottom: 1rem; - } -`; - -export const CallToActions = styled.div` - margin-left: auto; - margin-right: auto; - margin-bottom: 1rem; - display: flex; - align-items: center; - white-space: nowrap; - - ${breakpointMinSmall} { - margin-right: 0; - margin-bottom: 0; - } -`; - -export const GetHelpButton = styled(ExternalLink)` - font-size: 12px; - margin-right: 1.125rem; - text-decoration: underline; -`; - -export const ConnectDatabaseButton = styled(Button)` - background-color: ${color("white")}; - color: ${VIBRANT_BLUE}; - - ${breakpointMinSmall} { - margin-right: 0.5625rem; - } -`; diff --git a/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.tsx b/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.tsx deleted file mode 100644 index 2284b336f8204..0000000000000 --- a/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.tsx +++ /dev/null @@ -1,64 +0,0 @@ -import type { Location } from "history"; -import { t } from "ttag"; - -import { useSetting } from "metabase/common/hooks"; -import Link from "metabase/core/components/Link/Link"; -import { trackDatabasePromptBannerClicked } from "metabase/nav/analytics"; -import { useShouldShowDatabasePromptBanner } from "metabase/nav/hooks"; - -import { - ConnectDatabaseButton, - CallToActions, - DatabasePromptBannerRoot, - Prompt, - GetHelpButton, -} from "./DatabasePromptBanner.styled"; - -interface DatabasePromptBannerProps { - location: Location; -} - -export function DatabasePromptBanner({ location }: DatabasePromptBannerProps) { - const adminEmail = useSetting("admin-email"); - const siteUrl = useSetting("site-url"); - - const helpUrl = new URL("https://metabase.com/help/connect"); - helpUrl.searchParams.set("email", adminEmail || ""); - helpUrl.searchParams.set("site_url", siteUrl || ""); - - const shouldShowDatabasePromptBanner = useShouldShowDatabasePromptBanner(); - if (!shouldShowDatabasePromptBanner) { - return null; - } - - const isOnAdminAddDatabasePage = location.pathname.startsWith( - "/admin/databases/create", - ); - - return ( - - {/* eslint-disable-next-line no-literal-metabase-strings -- Only shows for admins*/} - {t`Connect to your database to get the most from Metabase.`} - - { - trackDatabasePromptBannerClicked("help"); - }} - >{t`Get help connecting`} - {!isOnAdminAddDatabasePage && ( - { - trackDatabasePromptBannerClicked("nav"); - }} - > - - {t`Connect your database`} - - - )} - - - ); -} diff --git a/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.unit.spec.tsx b/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.unit.spec.tsx deleted file mode 100644 index 7d2a4b49969ab..0000000000000 --- a/frontend/src/metabase/nav/components/DatabasePromptBanner/DatabasePromptBanner.unit.spec.tsx +++ /dev/null @@ -1,249 +0,0 @@ -import fetchMock from "fetch-mock"; -import { Route } from "react-router"; - -import { setupEnterpriseTest } from "__support__/enterprise"; -import { setupDatabasesEndpoints } from "__support__/server-mocks"; -import { mockSettings } from "__support__/settings"; -import { renderWithProviders, screen, waitFor } from "__support__/ui"; -import { - createMockDatabase, - createMockTokenStatus, - createMockUser, -} from "metabase-types/api/mocks"; -import { createSampleDatabase } from "metabase-types/api/mocks/presets"; -import { createMockState } from "metabase-types/store/mocks"; - -import { DatabasePromptBanner } from "./DatabasePromptBanner"; - -interface setupOpts { - isAdmin?: boolean; - isPaidPlan?: boolean; - onlyHaveSampleDatabase?: boolean; - isWhiteLabeling?: boolean; - isOnAdminAddDatabasePage?: boolean; -} -const TEST_DB = createSampleDatabase(); - -const DATA_WAREHOUSE_DB = createMockDatabase({ id: 2 }); - -async function setup({ - isAdmin = false, - isPaidPlan = false, - onlyHaveSampleDatabase = false, - isWhiteLabeling = false, - isOnAdminAddDatabasePage = false, -}: setupOpts = {}) { - if (onlyHaveSampleDatabase) { - setupDatabasesEndpoints([TEST_DB]); - } else { - setupDatabasesEndpoints([TEST_DB, DATA_WAREHOUSE_DB]); - } - - const state = createMockState({ - currentUser: createMockUser({ is_superuser: isAdmin }), - settings: mockSettings({ - "token-status": createMockTokenStatus({ valid: isPaidPlan }), - "application-name": isWhiteLabeling ? "Acme Corp." : "Metabase", - }), - }); - - renderWithProviders( - isOnAdminAddDatabasePage ? ( - - ) : ( - - ), - { - initialRoute: isOnAdminAddDatabasePage ? "/admin/databases/create" : "/", - storeInitialState: state, - withRouter: true, - }, - ); - - // This check ensures the conditions for database prompt banner are all available. - // Then we could safely assert that the banner is not rendered. - // If we don't wait for this API call to finish, the banner could have rendered, - // and the test would still pass. - if (isAdmin && isPaidPlan && !isWhiteLabeling) { - await waitFor(() => { - expect(fetchMock.called("path:/api/database")).toBe(true); - }); - } -} - -describe("DatabasePromptBanner", () => { - it("should not render for non-admin users without paid plan without connected databases", async () => { - await setup({ - isAdmin: false, - isPaidPlan: false, - onlyHaveSampleDatabase: false, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should not render for admin users without paid plan or connected databases", async () => { - await setup({ - isAdmin: true, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should not render for paid-plan instance without being an admin or connected databases", async () => { - await setup({ - isPaidPlan: true, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should not render for instance with only sample database without being an admin or without paid plan", async () => { - await setup({ - onlyHaveSampleDatabase: true, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should not render for admin users with paid plan but with connected databases", async () => { - await setup({ - isAdmin: true, - isPaidPlan: true, - onlyHaveSampleDatabase: false, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should not render for admin users without connected databases but without paid plan", async () => { - await setup({ - isAdmin: true, - isPaidPlan: false, - onlyHaveSampleDatabase: true, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should not render for instance with paid plan and without connected databases but without being an admin user", async () => { - await setup({ - isAdmin: false, - isPaidPlan: true, - onlyHaveSampleDatabase: true, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - - it("should render for admin users with paid plan without connected databases", async () => { - await setup({ - isAdmin: true, - isPaidPlan: true, - onlyHaveSampleDatabase: true, - }); - - expect( - await screen.findByText( - "Connect to your database to get the most from Metabase.", - ), - ).toBeInTheDocument(); - - const getHelpLink = screen.getByRole("link", { - name: "Get help connecting", - }); - expect(getHelpLink).toBeInTheDocument(); - expect(getHelpLink).toHaveAttribute( - "href", - "https://metabase.com/help/connect?email=admin%40metabase.test&site_url=http%3A%2F%2Flocalhost%3A3000", - ); - - const connectDatabaseLink = screen.getByRole("link", { - name: "Connect your database", - }); - expect(connectDatabaseLink).toBeInTheDocument(); - expect(connectDatabaseLink).toHaveAttribute( - "href", - "/admin/databases/create", - ); - }); - - it("should render for admin users with paid plan without connected databases, but without connect your database button if users is on admin add database page", async () => { - await setup({ - isAdmin: true, - isPaidPlan: true, - onlyHaveSampleDatabase: true, - isOnAdminAddDatabasePage: true, - }); - - expect( - await screen.findByText( - "Connect to your database to get the most from Metabase.", - ), - ).toBeInTheDocument(); - - const getHelpLink = screen.getByRole("link", { - name: "Get help connecting", - }); - expect(getHelpLink).toBeInTheDocument(); - expect(getHelpLink).toHaveAttribute( - "href", - "https://metabase.com/help/connect?email=admin%40metabase.test&site_url=http%3A%2F%2Flocalhost%3A3000", - ); - - expect( - screen.queryByRole("link", { - name: "Connect your database", - }), - ).not.toBeInTheDocument(); - }); - - describe("EE", () => { - beforeEach(() => { - setupEnterpriseTest(); - }); - - it("should not render for admin users with paid plan without connected databases, but is white labeling", async () => { - await setup({ - isAdmin: true, - isPaidPlan: true, - onlyHaveSampleDatabase: true, - isWhiteLabeling: true, - }); - - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }); - }); -}); diff --git a/frontend/src/metabase/nav/components/DatabasePromptBanner/index.ts b/frontend/src/metabase/nav/components/DatabasePromptBanner/index.ts deleted file mode 100644 index 084e779621302..0000000000000 --- a/frontend/src/metabase/nav/components/DatabasePromptBanner/index.ts +++ /dev/null @@ -1 +0,0 @@ -export { DatabasePromptBanner } from "./DatabasePromptBanner"; From f059092a8a2c6826878f4676f4aae5c5a130ab6f Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:25:20 -0400 Subject: [PATCH 02/10] wip Removing database prompt banner --- frontend/src/metabase/App.tsx | 3 +-- .../src/metabase/components/AppBanner/AppBanner.tsx | 12 ++---------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/frontend/src/metabase/App.tsx b/frontend/src/metabase/App.tsx index 999ea36bb50bc..b5c1dc4e4bfd3 100644 --- a/frontend/src/metabase/App.tsx +++ b/frontend/src/metabase/App.tsx @@ -90,7 +90,6 @@ function App({ isNavBarEnabled, children, onError, - location, }: AppProps) { const [viewportElement, setViewportElement] = useState(); @@ -104,7 +103,7 @@ function App({ - + {isAppBarVisible && } {isNavBarEnabled && } diff --git a/frontend/src/metabase/components/AppBanner/AppBanner.tsx b/frontend/src/metabase/components/AppBanner/AppBanner.tsx index 460048688708a..c4bf64f72d62c 100644 --- a/frontend/src/metabase/components/AppBanner/AppBanner.tsx +++ b/frontend/src/metabase/components/AppBanner/AppBanner.tsx @@ -1,24 +1,16 @@ -import type { Location } from "history"; - import { useSetting } from "metabase/common/hooks"; import { useSelector } from "metabase/lib/redux"; -import { DatabasePromptBanner } from "metabase/nav/components/DatabasePromptBanner"; import { PaymentBanner, shouldRenderPaymentBanner, } from "metabase/nav/components/PaymentBanner/PaymentBanner"; import { getUserIsAdmin } from "metabase/selectors/user"; -interface AppBannerProps { - location: Location; -} - -export const AppBanner = ({ location }: AppBannerProps) => { +export const AppBanner = () => { const isAdmin = useSelector(getUserIsAdmin); const tokenStatus = useSetting("token-status"); if (tokenStatus && shouldRenderPaymentBanner({ isAdmin, tokenStatus })) { return ; } - - return ; + return null; }; From 093218ec9443b1f871b0b816287ea912add6c8a5 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:27:26 -0400 Subject: [PATCH 03/10] Remove e2e test --- .../database-prompt-banner.cy.spec.js | 172 ------------------ 1 file changed, 172 deletions(-) delete mode 100644 e2e/test/scenarios/admin/databases/database-prompt-banner.cy.spec.js diff --git a/e2e/test/scenarios/admin/databases/database-prompt-banner.cy.spec.js b/e2e/test/scenarios/admin/databases/database-prompt-banner.cy.spec.js deleted file mode 100644 index a4e5d56b1a322..0000000000000 --- a/e2e/test/scenarios/admin/databases/database-prompt-banner.cy.spec.js +++ /dev/null @@ -1,172 +0,0 @@ -import { ORDERS_DASHBOARD_ID } from "e2e/support/cypress_sample_instance_data"; -import { - appBar, - describeWithSnowplow, - enableTracking, - expectGoodSnowplowEvents, - expectNoBadSnowplowEvents, - resetSnowplow, - restore, - rightSidebar, - visitDashboard, - describeEE, - setTokenFeatures, -} from "e2e/support/helpers"; - -describeEE("database prompt banner", () => { - beforeEach(() => { - restore(); - cy.signInAsAdmin(); - setTokenFeatures("all"); - }); - - it("should show a database prompt banner when logged in as an admin, an instance is on a paid plan, only have a single sample dataset, and is not white labelled", () => { - cy.visit("/"); - cy.findByRole("main").findByText("Loading...").should("not.exist"); - - cy.findAllByRole("banner") - .first() - .within(() => { - cy.findByText( - "Connect to your database to get the most from Metabase.", - ).should("exist"); - - cy.findByRole("link", { name: "Get help connecting" }) - .should("have.attr", "href") - .and( - "eq", - "https://metabase.com/help/connect?email=admin%40metabase.test&site_url=http%3A%2F%2Flocalhost%3A4000", - ); - - cy.findByRole("link", { name: "Connect your database" }).click(); - cy.url().should("include", "/admin/databases/create"); - }); - - // Assert that database form is rendered - cy.findByRole("main").within(() => { - cy.findByText("Add Database").should("exist"); - cy.findByLabelText("Database type").should("exist"); - cy.findByLabelText("Database name").should("exist"); - }); - }); - - it("should not show a database prompt banner when logged in as an admin, an instance is on a paid plan, and only have a single sample dataset, but is white labelled", () => { - cy.request("PUT", "/api/setting/application-name", { value: "Acme Corp." }); - cy.visit("/"); - cy.findByRole("main").findByText("Loading...").should("not.exist"); - - cy.findAllByRole("banner") - .first() - .within(() => { - cy.findByText( - "Connect to your database to get the most from Metabase.", - ).should("not.exist"); - }); - }); - - // Until we enable multi-browser support, this repro will be skipped by Cypress in CI - // Issue was specific to Firefox only - it is still possible to test it locally - it( - "should show info sidebar correctly on Firefox", - { browser: "firefox" }, - function () { - visitDashboard(ORDERS_DASHBOARD_ID); - cy.findByRole("main").findByText("Loading...").should("not.exist"); - cy.findByRole("main").icon("info").click(); - - rightSidebar().within(() => { - cy.findByRole("heading", { name: "About" }).should("be.visible"); - cy.findByRole("heading", { name: "History" }).should("be.visible"); - }); - }, - ); - - describe("embeddings", () => { - // Public and signed embeds are tested in `PublicQuestion.unit.spec.tsx` - - describe("full-app embeddings", () => { - it("should render database prompt banner when logged in as an admin, an instance is on a paid plan, only have a single sample dataset, and is not white labeling", () => { - visitFullAppEmbeddingUrl({ - url: "/", - qs: { side_nav: false, logo: false }, - }); - - cy.findByRole("link", { name: "Metabase tips" }).should("exist"); - - // Test that we're in full-app embedding since parameters are working. - appBar().should("not.exist"); - - cy.findAllByRole("banner") - .first() - .within(() => { - cy.findByText( - "Connect to your database to get the most from Metabase.", - ).should("exist"); - }); - }); - - it("should not render for any other condition", () => { - // Adding a second database should prevent the database prompt - cy.addSQLiteDatabase(); - - visitFullAppEmbeddingUrl({ - url: "/", - qs: { side_nav: false, logo: false }, - }); - - cy.findByRole("link", { name: "Metabase tips" }).should("exist"); - - // Test that we're in full-app embedding since parameters are working. - appBar().should("not.exist"); - - // Since there wouldn't be a database prompt banner, we can assert that there is no banner at all - cy.findAllByRole("banner").should("not.exist"); - }); - }); - }); -}); - -describeWithSnowplow("database prompt banner", () => { - const PAGE_VIEW_EVENT = 1; - - beforeEach(() => { - restore(); - resetSnowplow(); - cy.signInAsAdmin(); - setTokenFeatures("all"); - enableTracking(); - cy.visit("/"); - cy.findByRole("main").findByText("Loading...").should("not.exist"); - }); - - afterEach(() => { - expectNoBadSnowplowEvents(); - }); - - it("should send snowplow events when clicking on links in the database prompt banner", () => { - expectNoBadSnowplowEvents(); - expectGoodSnowplowEvents(PAGE_VIEW_EVENT); - cy.findAllByRole("banner") - .first() - .within(() => { - cy.findByRole("link", { name: "Get help connecting" }).click(); - expectGoodSnowplowEvents(PAGE_VIEW_EVENT + 1); - - cy.findByRole("link", { name: "Connect your database" }).click(); - // clicking this link also brings us to the admin page causing a new page_view event - expectGoodSnowplowEvents(2 * PAGE_VIEW_EVENT + 2); - }); - }); -}); - -const visitFullAppEmbeddingUrl = ({ url, qs }) => { - cy.visit({ - url, - qs, - onBeforeLoad(window) { - // cypress runs all tests in an iframe and the app uses this property to avoid embedding mode for all tests - // by removing the property the app would work in embedding mode - window.Cypress = undefined; - }, - }); -}; From b695ca140887d3ab3e3708aa0f3e9472ec9017c6 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:31:13 -0400 Subject: [PATCH 04/10] wip Removing database prompt banner --- .../AppBanner/AppBanner.unit.spec.tsx | 133 +++---------- frontend/src/metabase/nav/hooks.ts | 27 --- frontend/src/metabase/nav/hooks.unit.spec.tsx | 188 ------------------ .../PublicQuestion.unit.spec.tsx | 14 +- 4 files changed, 27 insertions(+), 335 deletions(-) delete mode 100644 frontend/src/metabase/nav/hooks.ts delete mode 100644 frontend/src/metabase/nav/hooks.unit.spec.tsx diff --git a/frontend/src/metabase/components/AppBanner/AppBanner.unit.spec.tsx b/frontend/src/metabase/components/AppBanner/AppBanner.unit.spec.tsx index 78a55d2f580b5..0bff5413e41b0 100644 --- a/frontend/src/metabase/components/AppBanner/AppBanner.unit.spec.tsx +++ b/frontend/src/metabase/components/AppBanner/AppBanner.unit.spec.tsx @@ -17,29 +17,20 @@ import { AppBanner } from "./AppBanner"; interface SetupOpts { isAdmin: boolean; tokenStatusStatus: TokenStatusStatus; - shouldShowDatabasePromptBanner: boolean; } const TEST_DB = createSampleDatabase(); const DATA_WAREHOUSE_DB = createMockDatabase({ id: 2 }); -function setup({ - isAdmin, - tokenStatusStatus, - shouldShowDatabasePromptBanner, -}: SetupOpts) { - if (shouldShowDatabasePromptBanner) { - setupDatabasesEndpoints([TEST_DB]); - } else { - setupDatabasesEndpoints([TEST_DB, DATA_WAREHOUSE_DB]); - } +function setup({ isAdmin, tokenStatusStatus }: SetupOpts) { + setupDatabasesEndpoints([TEST_DB, DATA_WAREHOUSE_DB]); const state = createMockState({ currentUser: createMockUser({ is_superuser: isAdmin }), settings: mockSettings({ "token-status": createMockTokenStatus({ status: tokenStatusStatus, - valid: shouldShowDatabasePromptBanner, + valid: false, }), }), }); @@ -52,97 +43,42 @@ function setup({ } describe("AppBanner", () => { - it.each([ - { - shouldShowDatabasePromptBanner: true, - }, - { - shouldShowDatabasePromptBanner: false, - }, - ])( - "should render past-due banner for admin user with tokenStatusStatus: past-due, shouldShowDatabasePromptBanner: $shouldShowDatabasePromptBanner", - ({ shouldShowDatabasePromptBanner }) => { - setup({ - isAdmin: true, - tokenStatusStatus: "past-due", - shouldShowDatabasePromptBanner, - }); - - expect( - screen.getByText(/We couldn't process payment for your account\./), - ).toBeInTheDocument(); - expect( - screen.queryByText( - /Pro features won’t work right now due to lack of payment\./, - ), - ).not.toBeInTheDocument(); - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }, - ); - - it.each([ - { - shouldShowDatabasePromptBanner: true, - }, - { - shouldShowDatabasePromptBanner: false, - }, - ])( - "should render unpaid banner for admin user with tokenStatusStatus: unpaid, shouldShowDatabasePromptBanner: $shouldShowDatabasePromptBanner", - ({ shouldShowDatabasePromptBanner }) => { - setup({ - isAdmin: true, - tokenStatusStatus: "unpaid", - shouldShowDatabasePromptBanner, - }); - - expect( - screen.queryByText(/We couldn't process payment for your account\./), - ).not.toBeInTheDocument(); - expect( - screen.getByText( - /Pro features won’t work right now due to lack of payment\./, - ), - ).toBeInTheDocument(); - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); - }, - ); - - it("should render database prompt banner for admin user with tokenStatusStatus: something-else, shouldShowDatabasePromptBanner: true", async () => { + it("should render past-due banner for admin user with tokenStatusStatus: past-due", () => { setup({ isAdmin: true, - tokenStatusStatus: "something-else", - shouldShowDatabasePromptBanner: true, + tokenStatusStatus: "past-due", }); expect( - screen.queryByText(/We couldn't process payment for your account\./), - ).not.toBeInTheDocument(); + screen.getByText(/We couldn't process payment for your account\./), + ).toBeInTheDocument(); expect( screen.queryByText( /Pro features won’t work right now due to lack of payment\./, ), ).not.toBeInTheDocument(); + }); + + it("should render unpaid banner for admin user with tokenStatusStatus: unpaid", () => { + setup({ + isAdmin: true, + tokenStatusStatus: "unpaid", + }); + + expect( + screen.queryByText(/We couldn't process payment for your account\./), + ).not.toBeInTheDocument(); expect( - await screen.findByText( - "Connect to your database to get the most from Metabase.", + screen.getByText( + /Pro features won’t work right now due to lack of payment\./, ), ).toBeInTheDocument(); }); - it("should not render for admin user with tokenStatusStatus: something-else, shouldShowDatabasePromptBanner: false", () => { + it("should not render for admin user with tokenStatusStatus: something-else", () => { setup({ isAdmin: true, tokenStatusStatus: "something-else", - shouldShowDatabasePromptBanner: false, }); expect( @@ -153,35 +89,21 @@ describe("AppBanner", () => { /Pro features won’t work right now due to lack of payment\./, ), ).not.toBeInTheDocument(); - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); }); it.each([ - { tokenStatusStatus: "past-due", shouldShowDatabasePromptBanner: true }, - { tokenStatusStatus: "past-due", shouldShowDatabasePromptBanner: false }, - { tokenStatusStatus: "unpaid", shouldShowDatabasePromptBanner: true }, - { tokenStatusStatus: "unpaid", shouldShowDatabasePromptBanner: false }, + { tokenStatusStatus: "past-due" }, + { tokenStatusStatus: "unpaid" }, { tokenStatusStatus: "something-else", - shouldShowDatabasePromptBanner: true, - }, - { - tokenStatusStatus: "something-else", - shouldShowDatabasePromptBanner: false, }, ] as const)( - "should not render for non admin user with tokenStatusStatus: $tokenStatusStatus, shouldShowDatabasePromptBanner: $shouldShowDatabasePromptBanner", - ({ tokenStatusStatus, shouldShowDatabasePromptBanner }) => { + "should not render for non admin user with tokenStatusStatus: $tokenStatusStatus", + ({ tokenStatusStatus }) => { setup({ isAdmin: false, tokenStatusStatus, - shouldShowDatabasePromptBanner, }); - expect( screen.queryByText(/We couldn't process payment for your account\./), ).not.toBeInTheDocument(); @@ -190,11 +112,6 @@ describe("AppBanner", () => { /Pro features won’t work right now due to lack of payment\./, ), ).not.toBeInTheDocument(); - expect( - screen.queryByText( - "Connect to your database to get the most from Metabase.", - ), - ).not.toBeInTheDocument(); }, ); }); diff --git a/frontend/src/metabase/nav/hooks.ts b/frontend/src/metabase/nav/hooks.ts deleted file mode 100644 index c39fb29a254bc..0000000000000 --- a/frontend/src/metabase/nav/hooks.ts +++ /dev/null @@ -1,27 +0,0 @@ -import { useDatabaseListQuery } from "metabase/common/hooks"; -import { useSelector } from "metabase/lib/redux"; -import { getIsPaidPlan } from "metabase/selectors/settings"; -import { getUserIsAdmin } from "metabase/selectors/user"; -import { getIsWhiteLabeling } from "metabase/selectors/whitelabel"; - -export function useShouldShowDatabasePromptBanner(): boolean | undefined { - const isAdmin = useSelector(getUserIsAdmin); - const isPaidPlan = useSelector(getIsPaidPlan); - const isWhiteLabeling = useSelector(getIsWhiteLabeling); - const isEligibleForDatabasePromptBanner = - isAdmin && isPaidPlan && !isWhiteLabeling; - - const { data: databases } = useDatabaseListQuery({ - enabled: isEligibleForDatabasePromptBanner, - }); - - if (!isEligibleForDatabasePromptBanner) { - return false; - } - - if (databases === undefined) { - return undefined; - } - - return databases.every(database => database.is_sample); -} diff --git a/frontend/src/metabase/nav/hooks.unit.spec.tsx b/frontend/src/metabase/nav/hooks.unit.spec.tsx deleted file mode 100644 index 0e1c7c422c7f6..0000000000000 --- a/frontend/src/metabase/nav/hooks.unit.spec.tsx +++ /dev/null @@ -1,188 +0,0 @@ -import { setupEnterpriseTest } from "__support__/enterprise"; -import { setupDatabasesEndpoints } from "__support__/server-mocks"; -import { mockSettings } from "__support__/settings"; -import { renderWithProviders, screen } from "__support__/ui"; -import { - createMockDatabase, - createMockTokenStatus, - createMockUser, -} from "metabase-types/api/mocks"; -import { createSampleDatabase } from "metabase-types/api/mocks/presets"; -import { createMockState } from "metabase-types/store/mocks"; - -import { useShouldShowDatabasePromptBanner } from "./hooks"; - -interface SetupOpts { - isAdmin?: boolean; - isPaidPlan?: boolean; - onlyHaveSampleDatabase?: boolean; - isOnAdminAddDatabasePage?: boolean; - isWhiteLabeling?: boolean; -} -const TEST_DB = createSampleDatabase(); - -const DATA_WAREHOUSE_DB = createMockDatabase({ id: 2 }); - -function setup({ - isAdmin = false, - isPaidPlan = false, - isWhiteLabeling = false, - onlyHaveSampleDatabase = false, -}: SetupOpts = {}) { - if (onlyHaveSampleDatabase) { - setupDatabasesEndpoints([TEST_DB]); - } else { - setupDatabasesEndpoints([TEST_DB, DATA_WAREHOUSE_DB]); - } - - const state = createMockState({ - currentUser: createMockUser({ is_superuser: isAdmin }), - settings: mockSettings({ - "token-status": createMockTokenStatus({ valid: isPaidPlan }), - "application-name": isWhiteLabeling ? "Acme Corp." : "Metabase", - }), - }); - - function TestComponent() { - const shouldShowDatabasePromptBanner = useShouldShowDatabasePromptBanner(); - if (shouldShowDatabasePromptBanner == null) { - return <>Mock loading state; - } - - return shouldShowDatabasePromptBanner ? ( - <>showing database prompt banner - ) : ( - <>hiding database prompt banner - ); - } - - renderWithProviders(, { - storeInitialState: state, - }); -} - -describe("useShouldShowDatabasePromptBanner", () => { - beforeEach(() => { - setupEnterpriseTest(); - }); - - it("should render for admin user with paid plan and has only sample database, but not white-labeled", async () => { - setup({ - isAdmin: true, - isPaidPlan: true, - onlyHaveSampleDatabase: true, - isWhiteLabeling: false, - }); - - expect( - await screen.findByText("showing database prompt banner"), - ).toBeVisible(); - }); - - it.each([ - { - isPaidPlan: true, - onlyHaveSampleDatabase: true, - isWhiteLabeling: true, - }, - { - isPaidPlan: true, - onlyHaveSampleDatabase: false, - isWhiteLabeling: true, - }, - { - isPaidPlan: true, - onlyHaveSampleDatabase: false, - isWhiteLabeling: false, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: true, - isWhiteLabeling: true, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: true, - isWhiteLabeling: false, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: false, - isWhiteLabeling: true, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: false, - isWhiteLabeling: false, - }, - ] as const)( - "should not render for admin users when isPaidPlan: $isPaidPlan, onlyHaveSampleDatabase: $onlyHaveSampleDatabase, isWhiteLabeling: $isWhiteLabeling", - async ({ isPaidPlan, onlyHaveSampleDatabase, isWhiteLabeling }) => { - setup({ - isAdmin: true, - isPaidPlan, - onlyHaveSampleDatabase, - isWhiteLabeling, - }); - - expect( - await screen.findByText("hiding database prompt banner"), - ).toBeVisible(); - }, - ); - - it.each([ - { - isPaidPlan: true, - onlyHaveSampleDatabase: true, - isWhiteLabeling: true, - }, - { - isPaidPlan: true, - onlyHaveSampleDatabase: true, - isWhiteLabeling: false, - }, - { - isPaidPlan: true, - onlyHaveSampleDatabase: false, - isWhiteLabeling: true, - }, - { - isPaidPlan: true, - onlyHaveSampleDatabase: false, - isWhiteLabeling: false, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: true, - isWhiteLabeling: true, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: true, - isWhiteLabeling: false, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: false, - isWhiteLabeling: true, - }, - { - isPaidPlan: false, - onlyHaveSampleDatabase: false, - isWhiteLabeling: false, - }, - ] as const)( - "should not render for non-admin users when isPaidPlan: $isPaidPlan, onlyHaveSampleDatabase: $onlyHaveSampleDatabase, isWhiteLabeling: $isWhiteLabeling", - async ({ isPaidPlan, onlyHaveSampleDatabase, isWhiteLabeling }) => { - setup({ - isAdmin: false, - isPaidPlan, - onlyHaveSampleDatabase, - isWhiteLabeling, - }); - - expect(screen.getByText("hiding database prompt banner")).toBeVisible(); - }, - ); -}); diff --git a/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx b/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx index e4a3c444a6dbd..6987aa506d20f 100644 --- a/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx +++ b/frontend/src/metabase/public/containers/PublicQuestion/PublicQuestion.unit.spec.tsx @@ -5,12 +5,12 @@ import { setupPublicCardQueryEndpoints, setupPublicQuestionEndpoints, } from "__support__/server-mocks"; -import { renderWithProviders, screen, within } from "__support__/ui"; +import { renderWithProviders, screen } from "__support__/ui"; import registerVisualizations from "metabase/visualizations/register"; import type { VisualizationProps } from "metabase/visualizations/types"; import { - createMockPublicCard, createMockEmbedDataset, + createMockPublicCard, } from "metabase-types/api/mocks"; import { createMockState } from "metabase-types/store/mocks"; @@ -84,16 +84,6 @@ describe("PublicQuestion", () => { expect(screen.getByText("John W.")).toBeInTheDocument(); }); - it("should not database prompt banner", async () => { - await setup(); - - // Since database prompt banner render as a banner. If we only find one banner - // that is showing the question name, we know that the database prompt banner is not showing. - expect( - within(screen.getByRole("banner")).getByText(QUESTION_NAME), - ).toBeInTheDocument(); - }); - it("should update card settings when visualization component changes them (metabase#37429)", async () => { await setup(); From 7b0ce23299e4979052d8fe12a88651796bd63c7f Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:32:01 -0400 Subject: [PATCH 05/10] wip Removing database prompt banner --- frontend/src/metabase/nav/analytics.ts | 11 ----------- 1 file changed, 11 deletions(-) delete mode 100644 frontend/src/metabase/nav/analytics.ts diff --git a/frontend/src/metabase/nav/analytics.ts b/frontend/src/metabase/nav/analytics.ts deleted file mode 100644 index db9f53a8ae2aa..0000000000000 --- a/frontend/src/metabase/nav/analytics.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { trackSchemaEvent } from "metabase/lib/analytics"; - -type LinkType = "nav" | "help"; - -export const trackDatabasePromptBannerClicked = (link: LinkType) => { - trackSchemaEvent("account", "1-0-1", { - event: "db_connection_banner_click", - version: "1.0.0", - link, - }); -}; From 2e40a28a58975fa46304d2af6dfb66f55c79aad6 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:34:40 -0400 Subject: [PATCH 06/10] Correct a comment --- e2e/test/scenarios/dashboard/dashboard.cy.spec.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/e2e/test/scenarios/dashboard/dashboard.cy.spec.js b/e2e/test/scenarios/dashboard/dashboard.cy.spec.js index 09c5cae60d352..6080a828eb789 100644 --- a/e2e/test/scenarios/dashboard/dashboard.cy.spec.js +++ b/e2e/test/scenarios/dashboard/dashboard.cy.spec.js @@ -1159,8 +1159,7 @@ function checkOptionsForFilter(filter) { .and("not.contain", "Dashboard filters"); // Get rid of the open popover to be able to select another filter - // Uses force: true because the popover is covering this text. This happens - // after we introduce the database prompt banner. + // Uses force: true because the popover is covering this text. cy.findByText("Pick one or more filters to update").click({ force: true }); } From 0ff7015f3a5719f1439cad304ba7ddc3d963eb68 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Tue, 26 Mar 2024 16:45:11 -0400 Subject: [PATCH 07/10] wip Removing database prompt banner --- .../schemas/com.metabase/account/jsonschema/1-0-1 | 1 - 1 file changed, 1 deletion(-) diff --git a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 index 1b1fc0455d2da..b570b99036923 100644 --- a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 +++ b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 @@ -15,7 +15,6 @@ "enum": [ "new_user_created", "new_instance_created", - "db_connection_banner_click" ], "maxLength": 1024 }, From 3ff9d09392489aba45375a87d1fc685f25edb628 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Wed, 27 Mar 2024 09:40:37 -0400 Subject: [PATCH 08/10] Restore jsonschema --- .../schemas/com.metabase/account/jsonschema/1-0-1 | 1 + 1 file changed, 1 insertion(+) diff --git a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 index b570b99036923..1b1fc0455d2da 100644 --- a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 +++ b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 @@ -15,6 +15,7 @@ "enum": [ "new_user_created", "new_instance_created", + "db_connection_banner_click" ], "maxLength": 1024 }, From f74dfed6f9df6650bf3f853ba919c0c183d21909 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Wed, 27 Mar 2024 09:42:05 -0400 Subject: [PATCH 09/10] update jsonschema properly --- .../schemas/com.metabase/account/jsonschema/{1-0-1 => 1-0-2} | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) rename snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/{1-0-1 => 1-0-2} (94%) diff --git a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-2 similarity index 94% rename from snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 rename to snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-2 index 1b1fc0455d2da..0276a42921b8c 100644 --- a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 +++ b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-2 @@ -5,7 +5,7 @@ "vendor": "com.metabase", "name": "account", "format": "jsonschema", - "version": "1-0-1" + "version": "1-0-2" }, "type": "object", "properties": { @@ -15,7 +15,6 @@ "enum": [ "new_user_created", "new_instance_created", - "db_connection_banner_click" ], "maxLength": 1024 }, From 1e5d4abb9cfa60933561253a7fd4a4b7c298cfd1 Mon Sep 17 00:00:00 2001 From: Raphael Krut-Landau Date: Wed, 27 Mar 2024 10:43:02 -0400 Subject: [PATCH 10/10] Include previous jsonschema version --- .../com.metabase/account/jsonschema/1-0-1 | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 diff --git a/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 new file mode 100644 index 0000000000000..1b1fc0455d2da --- /dev/null +++ b/snowplow/iglu-client-embedded/schemas/com.metabase/account/jsonschema/1-0-1 @@ -0,0 +1,47 @@ +{ + "$schema": "http://iglucentral.com/schemas/com.snowplowanalytics.self-desc/schema/jsonschema/1-0-0#", + "description": "Account creation", + "self": { + "vendor": "com.metabase", + "name": "account", + "format": "jsonschema", + "version": "1-0-1" + }, + "type": "object", + "properties": { + "event": { + "description": "Event name", + "type": "string", + "enum": [ + "new_user_created", + "new_instance_created", + "db_connection_banner_click" + ], + "maxLength": 1024 + }, + "version": { + "description": "String describing the version of database connection banner we're on", + "type": [ + "string", + "null" + ], + "maxLength": 1024 + }, + "link": { + "description": "String identifying the link where the user clicked", + "type": [ + "string", + "null" + ], + "enum": [ + "nav", + "help" + ], + "maxLength": 1024 + } + }, + "required": [ + "event" + ], + "additionalProperties": true +}