tech: Refactor and improve `AppBanner` (#49273)

* Remove unused component

* Remove extra space

* Remove cross-component dependency on the `Banner`

* Use named export

* Convert the Banner to TS

* Make `ReadOnlyBanner` use `Banner` under the hood

* Add test id to the banner

* Simplify render method and tests

* Add the missing test for ReadOnly banner

* Avoid using special ASCII characters for the common apostrophe

* Remove E2E spec

* Use `Banner` for `DatabaseInfoField`

* Use colors defined in the theme

* Remove the dependency on the `Banner` component again!

* Add a comment

* Use pattern matching

e2e/test/scenarios/admin/subscription.cy.spec.js

-import { mockSessionProperty, restore } from "e2e/support/helpers";
-
-describe("banner", () => {
-  beforeEach(() => {
-    restore();
-    cy.signInAsAdmin();
-  });
-
-  it("Show a banner when the subscription payment status is `past-due`", () => {
-    mockSessionProperty("token-status", {
-      status: "past-due",
-      valid: false,
-      trial: false,
-      features: [],
-    });
-
-    cy.visit("/");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.contains("We couldn't process payment for your account.");
-    cy.visit("/admin/");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.contains("We couldn't process payment for your account.");
-
-    cy.signInAsNormalUser();
-    cy.visit("/");
-    // Wait for page to load
-    cy.get("header");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.contains("We couldn't process payment for your account.").should(
-      "not.exist",
-    );
-  });
-
-  it("Show a banner when the subscription payment status is `unpaid`", () => {
-    mockSessionProperty("token-status", {
-      status: "unpaid",
-      valid: false,
-      trial: false,
-      features: [],
-    });
-
-    cy.visit("/");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.contains("Pro features won’t work right now due to lack of payment.");
-    cy.visit("/admin/");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.contains("Pro features won’t work right now due to lack of payment.");
-
-    cy.signInAsNormalUser();
-    cy.visit("/");
-    // Wait for page to load
-    cy.get("header");
-    // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
-    cy.contains(
-      "Pro features won’t work right now due to lack of payment.",
-    ).should("not.exist");
-  });
-});

frontend/src/metabase/collections/components/EmptyPinnedItemsBanner/EmptyPinnedItemsBanner.styled.tsx

-import styled from "@emotion/styled";
-
-import Banner from "metabase/components/Banner";
-import { lighten } from "metabase/lib/colors";
-import { Icon } from "metabase/ui";
-
-export const EmptyBanner = styled(Banner)`
-  border: 1px solid var(--mb-color-brand);
-  background-color: ${() => lighten("brand", 0.6)};
-  display: flex;
-  align-items: center;
-  color: var(--mb-color-text-dark);
-  font-weight: bold;
-  gap: 0.5rem;
-  padding: 1rem;
-`;
-
-export const ColoredIcon = styled(Icon)`
-  color: var(--mb-color-brand);
-`;

frontend/src/metabase/collections/components/EmptyPinnedItemsBanner/EmptyPinnedItemsBanner.tsx

-import { t } from "ttag";
-
-import { ColoredIcon, EmptyBanner } from "./EmptyPinnedItemsBanner.styled";
-
-function EmptyPinnedItemsBanner() {
-  return (
-    <EmptyBanner className={undefined}>
-      <ColoredIcon name="pin" />
-      {t`Save your questions, dashboards, and models in collections — and pin them to feature them at the top.`}
-    </EmptyBanner>
-  );
-}
-
-// eslint-disable-next-line import/no-default-export -- deprecated usage
-export default EmptyPinnedItemsBanner;

frontend/src/metabase/components/AppBanner/AppBanner.tsx

 import { useSetting } from "metabase/common/hooks";
 import { useSelector } from "metabase/lib/redux";
-import {
-  PaymentBanner,
-  shouldRenderPaymentBanner,
-} from "metabase/nav/components/PaymentBanner/PaymentBanner";
+import { PaymentBanner } from "metabase/nav/components/PaymentBanner/PaymentBanner";
 import { ReadOnlyBanner } from "metabase/nav/components/ReadOnlyBanner";
 import { getUserIsAdmin } from "metabase/selectors/user";
 
@@ -12,12 +9,25 @@ export const AppBanner = () => {
   const tokenStatus = useSetting("token-status");
   const readOnly = useSetting("read-only-mode");
 
-  if (tokenStatus && shouldRenderPaymentBanner({ isAdmin, tokenStatus })) {
-    return <PaymentBanner isAdmin={isAdmin} tokenStatus={tokenStatus} />;
+  const paymentStatuses = ["past-due", "unpaid", "invalid"];
+  const shouldRenderPaymentBanner =
+    tokenStatus && paymentStatuses.includes(tokenStatus?.status ?? "");
+
+  // Even though both the `tokenStatus` and `readOnly` settings
+  // are visible only to admins (and will be `undefined` otherwise),
+  // we still need to explicitly prevent rendering the banner for non-admins.
+  if (!isAdmin) {
+    return null;
   }
 
   if (readOnly) {
     return <ReadOnlyBanner />;
   }
+
+  if (shouldRenderPaymentBanner) {
+    return <PaymentBanner tokenStatus={tokenStatus} />;
+  }
+
+  // Do not render to admins if the specific conditions haven't been met
   return null;
 };

frontend/src/metabase/components/AppBanner/AppBanner.unit.spec.tsx

@@ -16,21 +16,30 @@ import { AppBanner } from "./AppBanner";
 
 interface SetupOpts {
   isAdmin: boolean;
-  tokenStatusStatus: TokenStatusStatus;
+  isReadOnly?: boolean;
+  tokenStatusStatus?: TokenStatusStatus;
+  tokenError?: string;
 }
 const TEST_DB = createSampleDatabase();
 
 const DATA_WAREHOUSE_DB = createMockDatabase({ id: 2 });
 
-function setup({ isAdmin, tokenStatusStatus }: SetupOpts) {
+function setup({
+  isAdmin,
+  tokenStatusStatus,
+  tokenError,
+  isReadOnly = false,
+}: SetupOpts) {
   setupDatabasesEndpoints([TEST_DB, DATA_WAREHOUSE_DB]);
 
   const state = createMockState({
     currentUser: createMockUser({ is_superuser: isAdmin }),
     settings: mockSettings({
+      "read-only-mode": isReadOnly,
       "token-status": createMockTokenStatus({
         status: tokenStatusStatus,
         valid: false,
+        "error-details": tokenError,
       }),
     }),
   });
@@ -43,75 +52,79 @@ function setup({ isAdmin, tokenStatusStatus }: SetupOpts) {
 }
 
 describe("AppBanner", () => {
-  it("should render past-due banner for admin user with tokenStatusStatus: past-due", () => {
-    setup({
-      isAdmin: true,
-      tokenStatusStatus: "past-due",
-    });
+  it("should not render for non admins", () => {
+    setup({ isAdmin: false });
 
-    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.queryByTestId("app-banner")).not.toBeInTheDocument();
   });
 
-  it("should render unpaid banner for admin user with tokenStatusStatus: unpaid", () => {
-    setup({
-      isAdmin: true,
-      tokenStatusStatus: "unpaid",
+  describe("PaymentBanner", () => {
+    it("should render past-due banner for admin user with tokenStatusStatus: past-due", () => {
+      setup({
+        isAdmin: true,
+        tokenStatusStatus: "past-due",
+      });
+
+      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(/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();
-  });
+    it("should render unpaid banner for admin user with tokenStatusStatus: unpaid", () => {
+      setup({
+        isAdmin: true,
+        tokenStatusStatus: "unpaid",
+      });
 
-  it("should not render for admin user with tokenStatusStatus: something-else", () => {
-    setup({
-      isAdmin: true,
-      tokenStatusStatus: "something-else",
+      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(/We couldn't process payment for your account\./),
-    ).not.toBeInTheDocument();
-    expect(
-      screen.queryByText(
-        /Pro features won’t work right now due to lack of payment\./,
-      ),
-    ).not.toBeInTheDocument();
+    it("should render an error with details when the token is `invalid`", () => {
+      setup({
+        isAdmin: true,
+        tokenStatusStatus: "invalid",
+        tokenError: "This is a critical damage.",
+      });
+
+      expect(
+        screen.getByText(/This is a critical damage\./),
+      ).toBeInTheDocument();
+    });
+
+    it("should not render for admin user with tokenStatusStatus: something-else", () => {
+      setup({
+        isAdmin: true,
+        tokenStatusStatus: "something-else",
+      });
+
+      expect(screen.queryByTestId("app-banner")).not.toBeInTheDocument();
+    });
   });
 
-  it.each([
-    { tokenStatusStatus: "past-due" },
-    { tokenStatusStatus: "unpaid" },
-    {
-      tokenStatusStatus: "something-else",
-    },
-  ] as const)(
-    "should not render for non admin user with tokenStatusStatus: $tokenStatusStatus",
-    ({ tokenStatusStatus }) => {
+  describe("ReadOnlyBanner", () => {
+    it("should render if Metabase is in read-only mode", () => {
       setup({
-        isAdmin: false,
-        tokenStatusStatus,
+        isAdmin: true,
+        isReadOnly: true,
       });
+
       expect(
-        screen.queryByText(/We couldn't process payment for your account\./),
-      ).not.toBeInTheDocument();
-      expect(
-        screen.queryByText(
-          /Pro features won’t work right now due to lack of payment\./,
+        screen.getByText(
+          "Metabase is under maintenance and is operating in read-only mode. It should only take up to 30 minutes.",
         ),
-      ).not.toBeInTheDocument();
-    },
-  );
+      ).toBeInTheDocument();
+    });
+  });
 });

frontend/src/metabase/components/Banner/Banner.styled.tsx

-import styled from "@emotion/styled";
-
-export const BannerRoot = styled.div`
-  padding: 0.75rem;
-  border-radius: 6px;
-  color: var(--mb-color-text-medium);
-  background-color: var(--mb-color-bg-light);
-`;

frontend/src/metabase/components/Banner/Banner.jsx → frontend/src/metabase/components/Banner/Banner.tsx

-import PropTypes from "prop-types";
-
-import { BannerRoot } from "metabase/components/Banner/Banner.styled";
 import Markdown from "metabase/core/components/Markdown";
+import { Flex, type FlexProps } from "metabase/ui";
 
-const propTypes = {
-  className: PropTypes.string,
-  children: PropTypes.node,
-};
-
-const Banner = ({ className, children }) => {
+export const Banner = ({ children, ...props }: FlexProps) => {
   const content =
     typeof children === "string" ? <Markdown>{children}</Markdown> : children;
 
-  return <BannerRoot className={className}>{content}</BannerRoot>;
+  return (
+    <Flex
+      bg="bg-light"
+      c="text-medium"
+      data-testid="app-banner"
+      p="0.75rem"
+      {...props}
+    >
+      {content}
+    </Flex>
+  );
 };
-
-Banner.propTypes = propTypes;
-
-export default Banner;

frontend/src/metabase/components/Banner/Banner.unit.spec.js → frontend/src/metabase/components/Banner/Banner.unit.spec.tsx

 import { render, screen } from "@testing-library/react";
 
-import Banner from "./Banner";
+import { Banner } from "./Banner";
 
 describe("Banner", () => {
   it("should render banner with content", () => {
     render(<Banner>Content</Banner>);
 
+    expect(screen.getByTestId("app-banner")).toBeInTheDocument();
     expect(screen.getByText("Content")).toBeInTheDocument();
   });
 });

frontend/src/metabase/components/Banner/index.js

-export { default } from "./Banner";

frontend/src/metabase/components/Banner/index.ts

+export { Banner } from "./Banner";

frontend/src/metabase/databases/components/DatabaseInfoField/DatabaseInfoField.styled.tsx

-import styled from "@emotion/styled";
-
-import Banner from "metabase/components/Banner";
-
-export const InfoBanner = styled(Banner)`
-  margin-bottom: 0.5rem;
-`;

frontend/src/metabase/databases/components/DatabaseInfoField/DatabaseInfoField.tsx

-import { InfoBanner } from "./DatabaseInfoField.styled";
+import Markdown from "metabase/core/components/Markdown";
+import { Flex } from "metabase/ui";
 
 export interface DatabaseInfoFieldProps {
   placeholder?: string;
@@ -6,8 +7,19 @@ export interface DatabaseInfoFieldProps {
 
 const DatabaseInfoField = ({
   placeholder,
-}: DatabaseInfoFieldProps): JSX.Element => {
-  return <InfoBanner>{placeholder}</InfoBanner>;
+}: DatabaseInfoFieldProps): JSX.Element | null => {
+  return placeholder ? (
+    <Flex
+      bg="bg-light"
+      c="text-medium"
+      data-testid="app-banner"
+      mb="sm"
+      p="0.75rem"
+      style={{ borderRadius: "0.375rem" }}
+    >
+      <Markdown>{placeholder}</Markdown>
+    </Flex>
+  ) : null;
 };
 
 // eslint-disable-next-line import/no-default-export -- deprecated usage

frontend/src/metabase/nav/components/PaymentBanner/PaymentBanner.tsx

+import { match } from "ts-pattern";
 import { jt, t } from "ttag";
 
-import Banner from "metabase/components/Banner";
+import { Banner } from "metabase/components/Banner";
 import ExternalLink from "metabase/core/components/ExternalLink";
 import CS from "metabase/css/core/index.css";
 import { getStoreUrl } from "metabase/selectors/settings";
 import type { TokenStatus } from "metabase-types/api";
 
 interface PaymentBannerProps {
-  isAdmin: boolean;
   tokenStatus: TokenStatus;
 }
 
-export const PaymentBanner = ({ isAdmin, tokenStatus }: PaymentBannerProps) => {
-  if (isAdmin && tokenStatus.status === "past-due") {
-    return (
+export const PaymentBanner = ({ tokenStatus }: PaymentBannerProps) => {
+  return match(tokenStatus.status)
+    .with("past-due", () => (
       <Banner>
         {jt`⚠️ We couldn't process payment for your account. Please ${(
           <ExternalLink
@@ -25,11 +25,10 @@ export const PaymentBanner = ({ isAdmin, tokenStatus }: PaymentBannerProps) => {
           </ExternalLink>
         )} to avoid service interruptions.`}
       </Banner>
-    );
-  } else if (isAdmin && tokenStatus.status === "unpaid") {
-    return (
+    ))
+    .with("unpaid", () => (
       <Banner>
-        {jt`⚠️ Pro features won’t work right now due to lack of payment. ${(
+        {jt`⚠️ Pro features won't work right now due to lack of payment. ${(
           <ExternalLink
             key="payment-unpaid"
             className={CS.link}
@@ -39,26 +38,11 @@ export const PaymentBanner = ({ isAdmin, tokenStatus }: PaymentBannerProps) => {
           </ExternalLink>
         )} to restore Pro functionality.`}
       </Banner>
-    );
-  } else if (isAdmin && tokenStatus.status === "invalid") {
-    return (
+    ))
+    .with("invalid", () => (
       <Banner>
         {jt`⚠️ Pro features error. ` + (tokenStatus["error-details"] || "")}
       </Banner>
-    );
-  }
-
-  return null;
+    ))
+    .otherwise(() => null);
 };
-
-export function shouldRenderPaymentBanner({
-  isAdmin,
-  tokenStatus,
-}: PaymentBannerProps) {
-  const shouldRenderStatuses: (string | undefined)[] = [
-    "past-due",
-    "unpaid",
-    "invalid",
-  ];
-  return isAdmin && shouldRenderStatuses.includes(tokenStatus?.status);
-}

frontend/src/metabase/nav/components/PaymentBanner/PaymentBanner.unit.spec.tsx

-/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectBannerToBeHidden"] }] */
-import { render, screen } from "__support__/ui";
-import type { TokenStatus } from "metabase-types/api";
-
-import { PaymentBanner, shouldRenderPaymentBanner } from "./PaymentBanner";
-
-interface SetupOpts {
-  isAdmin: boolean;
-  tokenStatus: TokenStatus;
-}
-
-function setup({ isAdmin, tokenStatus }: SetupOpts) {
-  render(<PaymentBanner isAdmin={isAdmin} tokenStatus={tokenStatus} />);
-}
-
-const PAST_DUE_BANNER_REGEX =
-  /We couldn't process payment for your account. Please/;
-
-const UNPAID_BANNER_REGEX =
-  /Pro features won’t work right now due to lack of payment\./;
-
-describe("PaymentBanner", () => {
-  describe("admin users", () => {
-    it("should render past-due banner when token status status is `past-due`", () => {
-      setup({
-        isAdmin: true,
-        tokenStatus: { status: "past-due", valid: false, trial: false },
-      });
-
-      expect(screen.getByText(PAST_DUE_BANNER_REGEX)).toBeInTheDocument();
-    });
-
-    it("should render unpaid banner when token status status is `unpaid`", () => {
-      setup({
-        isAdmin: true,
-        tokenStatus: { status: "unpaid", valid: false, trial: false },
-      });
-
-      expect(screen.getByText(UNPAID_BANNER_REGEX)).toBeInTheDocument();
-    });
-
-    it("should render an error with details when the token is `invalid`", () => {
-      setup({
-        isAdmin: true,
-        tokenStatus: {
-          status: "invalid",
-          valid: false,
-          trial: false,
-          "error-details": "This is critical damage.",
-        },
-      });
-
-      expect(screen.getByText(/This is critical damage\./)).toBeInTheDocument();
-    });
-
-    it("should not render any banner when token status status is not `past-due` or `unpaid`", () => {
-      setup({
-        isAdmin: true,
-        tokenStatus: { status: "something-else", valid: false, trial: false },
-      });
-
-      expectBannerToBeHidden();
-    });
-  });
-
-  describe("non-admin users", () => {
-    it.each([
-      {
-        tokenStatus: { status: "past-due", valid: false, trial: false },
-      },
-      {
-        tokenStatus: { status: "unpaid", valid: false, trial: false },
-      },
-      {
-        tokenStatus: { status: "something-else", valid: false, trial: false },
-      },
-    ])(
-      "should not render any banner when tokenStatus.status is `${tokenStatus.status}`",
-      ({ tokenStatus }) => {
-        setup({ isAdmin: false, tokenStatus });
-
-        expectBannerToBeHidden();
-      },
-    );
-  });
-
-  describe("shouldRenderPaymentBanner", () => {
-    it.each([
-      {
-        isAdmin: true,
-        tokenStatus: { status: "past-due", valid: false, trial: false },
-        hasBanner: true,
-      },
-      {
-        isAdmin: true,
-        tokenStatus: { status: "unpaid", valid: false, trial: false },
-        hasBanner: true,
-      },
-      {
-        isAdmin: true,
-        tokenStatus: { status: "invalid", valid: false, trial: false },
-        hasBanner: true,
-      },
-      {
-        isAdmin: true,
-        tokenStatus: { status: "something-else", valid: false, trial: false },
-        hasBanner: false,
-      },
-      {
-        isAdmin: false,
-        tokenStatus: { status: "past-due", valid: false, trial: false },
-        hasBanner: false,
-      },
-      {
-        isAdmin: false,
-        tokenStatus: { status: "unpaid", valid: false, trial: false },
-        hasBanner: false,
-      },
-      {
-        isAdmin: false,
-        tokenStatus: { status: "invalid", valid: false, trial: false },
-        hasBanner: false,
-      },
-      {
-        isAdmin: false,
-        tokenStatus: { status: "something-else", valid: false, trial: false },
-        hasBanner: false,
-      },
-    ])(
-      "should return `${shouldRenderPaymentBanner} when isAdmin: $isAdmin, and tokenStatus.status: ${tokenStatus.status}`",
-      ({ isAdmin, tokenStatus, hasBanner }) => {
-        expect(shouldRenderPaymentBanner({ isAdmin, tokenStatus })).toEqual(
-          hasBanner,
-        );
-      },
-    );
-  });
-});
-
-function expectBannerToBeHidden() {
-  expect(screen.queryByText(PAST_DUE_BANNER_REGEX)).not.toBeInTheDocument();
-  expect(screen.queryByText(UNPAID_BANNER_REGEX)).not.toBeInTheDocument();
-}

frontend/src/metabase/nav/components/ReadOnlyBanner/ReadOnlyBanner.tsx

 import { t } from "ttag";
 
-import { Flex, Icon, Text, useMantineTheme } from "metabase/ui";
+import { Banner } from "metabase/components/Banner";
+import { Icon, Text, useMantineTheme } from "metabase/ui";
 
 export const ReadOnlyBanner = () => {
   const theme = useMantineTheme();
   return (
-    <Flex
+    <Banner
       py="0.75rem"
       px="1rem"
       bg={theme.fn.themeColor("accent4")}
@@ -15,8 +16,8 @@ export const ReadOnlyBanner = () => {
       <Icon color="text-dark" name="info_filled" />
       <Text fw="bold" color="text-dark">
         {/* eslint-disable-next-line no-literal-metabase-strings -- correct usage */}
-        {t`Metabase is under maintenance and is operating in read-only mode.  It should only take up to 30 minutes.`}
+        {t`Metabase is under maintenance and is operating in read-only mode. It should only take up to 30 minutes.`}
       </Text>
-    </Flex>
+    </Banner>
   );
 };