Refactor some modals so they can be tested semantically

This sets the stage for #36866
metabase · Dec 21, 2023 · 1c408bf · 1c408bf
1 parent 7a4ebb5
commit 1c408bf
Show file tree

Hide file tree

Showing 9 changed files with 63 additions and 28 deletions.
diff --git a/frontend/src/metabase/actions/components/ActionViz/Action.unit.spec.tsx b/frontend/src/metabase/actions/components/ActionViz/Action.unit.spec.tsx
@@ -212,7 +212,9 @@ describe("Actions > ActionViz > Action", () => {
       await setup();
 
       userEvent.click(screen.getByRole("button"));
-      expect(screen.getByRole("dialog")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "My Awesome Action" }),
+      ).toBeInTheDocument();
       expect(screen.getByTestId("action-form")).toBeInTheDocument();
       expect(screen.getByLabelText("Parameter 1")).toBeInTheDocument();
     });
@@ -229,7 +231,9 @@ describe("Actions > ActionViz > Action", () => {
       });
 
       userEvent.click(screen.getByRole("button"));
-      expect(screen.getByRole("dialog")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "Query Action Mock" }),
+      ).toBeInTheDocument();
       expect(screen.getByTestId("action-form")).toBeInTheDocument();
       expect(screen.queryByLabelText(/^Parameter/)).not.toBeInTheDocument();
     });
@@ -238,14 +242,18 @@ describe("Actions > ActionViz > Action", () => {
       await setup();
 
       userEvent.click(screen.getByRole("button"));
-      expect(screen.getByRole("dialog")).toHaveTextContent("My Awesome Action");
+      expect(
+        screen.getByRole("dialog", { name: "My Awesome Action" }),
+      ).toBeInTheDocument();
     });
 
     it("clicking the cancel button on the form should close the modal", async () => {
       await setup();
 
       userEvent.click(screen.getByRole("button"));
-      expect(screen.getByRole("dialog")).toHaveTextContent("My Awesome Action");
+      expect(
+        screen.getByRole("dialog", { name: "My Awesome Action" }),
+      ).toBeInTheDocument();
       userEvent.click(screen.getByRole("button", { name: "Cancel" }));
       expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
     });
@@ -291,10 +299,14 @@ describe("Actions > ActionViz > Action", () => {
       });
 
       userEvent.click(screen.getByRole("button", { name: "Click me" }));
-      expect(screen.getByRole("dialog")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "My Awesome Action" }),
+      ).toBeInTheDocument();
       expect(screen.getByTestId("action-form")).toBeInTheDocument();
       userEvent.click(
-        within(screen.getByRole("dialog")).getByRole("button", {
+        within(
+          screen.getByRole("dialog", { name: "My Awesome Action" }),
+        ).getByRole("button", {
           name: action.name,
         }),
       );
@@ -531,7 +543,9 @@ describe("Actions > ActionViz > Action", () => {
       });
 
       userEvent.click(screen.getByRole("button"));
-      expect(screen.getByRole("dialog")).toHaveTextContent(/cannot be undone/i);
+      expect(
+        screen.getByRole("dialog", { name: "My Delete Action" }),
+      ).toHaveTextContent(/cannot be undone/i);
       expect(
         screen.getByRole("button", { name: "Delete" }),
       ).toBeInTheDocument();

diff --git a/frontend/src/metabase/collections/components/CollectionHeader/CollectionUploadInfoModal.tsx b/frontend/src/metabase/collections/components/CollectionHeader/CollectionUploadInfoModal.tsx
@@ -23,7 +23,10 @@ export const UploadInfoModal = ({
   const applicationName = useSelector(getApplicationName);
   return (
     <Modal small>
-      <ModalContent title=" " onClose={onClose}>
+      <ModalContent
+        title={t`Uploads CSVs to ${applicationName}`}
+        onClose={onClose}
+      >
         <InfoModalContainer>
           <NewBadge>{t`New`}</NewBadge>
           <InfoModalTitle>{t`Uploads CSVs to ${applicationName}`}</InfoModalTitle>

diff --git a/frontend/src/metabase/collections/components/CollectionHeader/tests/common.unit.spec.tsx b/frontend/src/metabase/collections/components/CollectionHeader/tests/common.unit.spec.tsx
@@ -230,8 +230,9 @@ describe("CollectionHeader", () => {
       });
       userEvent.click(screen.getByLabelText("Upload data"));
 
-      expect(await screen.findByRole("dialog")).toBeInTheDocument();
-      expect(screen.getByText("Uploads CSVs to Metabase")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "Uploads CSVs to Metabase" }),
+      ).toBeInTheDocument();
     });
 
     it("should show an informational modal with a link to settings for admins", async () => {
@@ -243,7 +244,9 @@ describe("CollectionHeader", () => {
       });
       userEvent.click(screen.getByLabelText("Upload data"));
 
-      expect(await screen.findByRole("dialog")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "Uploads CSVs to Metabase" }),
+      ).toBeInTheDocument();
       expect(screen.getByText("Enable in settings")).toBeInTheDocument();
       expect(screen.getByRole("link")).toBeInTheDocument();
     });
@@ -257,7 +260,9 @@ describe("CollectionHeader", () => {
       });
       userEvent.click(screen.getByLabelText("Upload data"));
 
-      expect(await screen.findByRole("dialog")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "Uploads CSVs to Metabase" }),
+      ).toBeInTheDocument();
       expect(screen.getByText(/ask your admin to enable/i)).toBeInTheDocument();
     });
 
@@ -270,7 +275,9 @@ describe("CollectionHeader", () => {
       });
       userEvent.click(screen.getByLabelText("Upload data"));
 
-      expect(await screen.findByRole("dialog")).toBeInTheDocument();
+      expect(
+        screen.getByRole("dialog", { name: "Uploads CSVs to Metabase" }),
+      ).toBeInTheDocument();
       userEvent.click(getIcon("close"));
       expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
     });
@@ -284,7 +291,9 @@ describe("CollectionHeader", () => {
       });
       userEvent.click(screen.getByLabelText("Upload data"));
 
-      expect(await screen.findByRole("dialog")).toBeInTheDocument();
+      expect(
+        await screen.findByRole("dialog", { name: "Uploads CSVs to Metabase" }),
+      ).toBeInTheDocument();
       userEvent.click(screen.getByText("Got it"));
       expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
     });

diff --git a/frontend/src/metabase/components/Modal/WindowModal.tsx b/frontend/src/metabase/components/Modal/WindowModal.tsx
@@ -73,10 +73,7 @@ export class WindowModal extends Component<WindowModalProps> {
         closeOnClickOutside={this.props.closeOnClickOutside}
       >
         <FocusTrap active={this.props.trapFocus}>
-          <div
-            className={cx(className, "relative bg-white rounded")}
-            role="dialog"
-          >
+          <div className={cx(className, "relative bg-white rounded")}>
             {getModalContent({
               ...this.props,
               fullPageModal: false,

diff --git a/frontend/src/metabase/components/ModalContent/ModalContent.tsx b/frontend/src/metabase/components/ModalContent/ModalContent.tsx
@@ -82,6 +82,9 @@ export default class ModalContent extends Component<ModalContentProps> {
           // add bottom padding if this is a standard "form modal" with no footer
           { pb4: formModal && !footer },
         )}
+        role="dialog"
+        aria-label={title}
+        aria-modal="true"
         data-testid={dataTestId}
       >
         {title && (

diff --git a/frontend/src/metabase/containers/NewItemMenu/NewItemMenu.unit.spec.tsx b/frontend/src/metabase/containers/NewItemMenu/NewItemMenu.unit.spec.tsx
@@ -70,11 +70,8 @@ describe("NewItemMenu", () => {
   describe("New Action", () => {
     it("should open action editor on click", async () => {
       setup();
-
       userEvent.click(await screen.findByText("Action"));
-      const modal = screen.getByRole("dialog");
-
-      expect(modal).toBeVisible();
+      expect(screen.getByTestId("mock-action-editor")).toBeVisible();
     });
 
     it("should not be visible if there are no databases with actions enabled", () => {

diff --git a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.unit.spec.tsx
@@ -364,7 +364,9 @@ describe("ModelDetailPage", () => {
         userEvent.click(getIcon("ellipsis"));
         userEvent.click(screen.getByText("Archive"));
 
-        expect(screen.getByRole("dialog")).toBeInTheDocument();
+        expect(
+          screen.getByRole("dialog", { name: "Archive this model?" }),
+        ).toBeInTheDocument();
         userEvent.click(screen.getByRole("button", { name: "Archive" }));
 
         await waitFor(() => {
@@ -385,7 +387,11 @@ describe("ModelDetailPage", () => {
         userEvent.click(getIcon("ellipsis"));
         userEvent.click(screen.getByText("Move"));
 
-        expect(screen.getByRole("dialog")).toBeInTheDocument();
+        expect(
+          screen.getByRole("dialog", {
+            name: "Which collection should this be in?",
+          }),
+        ).toBeInTheDocument();
         userEvent.click(await screen.findByText(COLLECTION_2.name));
         userEvent.click(screen.getByRole("button", { name: "Move" }));
 
@@ -648,7 +654,9 @@ describe("ModelDetailPage", () => {
         userEvent.click(within(listItem).getByLabelText("ellipsis icon"));
         userEvent.click(screen.getByText("Archive"));
 
-        const modal = screen.getByRole("dialog");
+        const modal = screen.getByRole("dialog", {
+          name: "Archive Query Action Mock?",
+        });
         userEvent.click(within(modal).getByRole("button", { name: "Archive" }));
 
         expect(screen.queryByRole("dialog")).not.toBeInTheDocument();

diff --git a/frontend/src/metabase/query_builder/components/AlertModals.jsx b/frontend/src/metabase/query_builder/components/AlertModals.jsx
@@ -324,7 +324,7 @@ class UpdateAlertModalContentInner extends Component {
 
     // TODO: Remove PulseEdit css hack
     return (
-      <ModalContent onClose={onCancel}>
+      <ModalContent onClose={onCancel} title={title}>
         <div
           className="PulseEdit ml-auto mr-auto mb4"
           style={{ maxWidth: "550px" }}
@@ -393,6 +393,7 @@ export class DeleteAlertSection extends Component {
   render() {
     const { onDeleteAlert } = this.props;
 
+    const title = `Delete this alert?`;
     return (
       <DangerZone className="DangerZone mt4 pt4 mb2 p3 rounded bordered relative">
         <h3
@@ -408,10 +409,11 @@ export class DeleteAlertSection extends Component {
               as={Button}
               triggerClasses="Button--danger flex-align-right flex-no-shrink align-self-end"
               triggerElement={t`Delete this alert`}
+              title={title}
             >
               <DeleteModalWithConfirm
                 objectType="alert"
-                title={t`Delete this alert?`}
+                title={title}
                 confirmItems={this.getConfirmItems()}
                 onClose={() => this.deleteModal.close()}
                 onDelete={onDeleteAlert}

diff --git a/frontend/src/metabase/status/components/FileUploadStatus/FileUploadStatus.unit.spec.tsx b/frontend/src/metabase/status/components/FileUploadStatus/FileUploadStatus.unit.spec.tsx
@@ -182,7 +182,9 @@ describe("FileUploadStatus", () => {
 
     userEvent.click(await screen.findByText("Show error details"));
 
-    expect(await screen.findByRole("dialog")).toBeInTheDocument();
+    expect(
+      await screen.findByRole("dialog", { name: "Upload error details" }),
+    ).toBeInTheDocument();
 
     expect(await screen.findByText("Something went wrong")).toBeInTheDocument();
   });