Clicking outside of the Actions Editor loses your work (#30697)

frontend/src/metabase/actions/containers/ActionPicker/ActionPicker.tsx

@@ -136,7 +136,7 @@ function ModelActionPicker({
         )}
       </ModelCollapseSection>
       {isActionCreatorOpen && (
-        <Modal wide onClose={closeModal}>
+        <Modal wide onClose={closeModal} closeOnClickOutside>
           <ActionCreator
             modelId={model.id}
             databaseId={model.database_id}

frontend/src/metabase/components/Modal/MaybeOnClickOutsideWrapper.tsx

+import React from "react";
+import OnClickOutsideWrapper from "metabase/components/OnClickOutsideWrapper";
+
+export function MaybeOnClickOutsideWrapper({
+  children,
+  closeOnClickOutside = false,
+  ...props
+}: {
+  children: React.ReactNode;
+  closeOnClickOutside?: boolean;
+} & React.ComponentProps<typeof OnClickOutsideWrapper>) {
+  return closeOnClickOutside ? (
+    <>{children}</>
+  ) : (
+    <OnClickOutsideWrapper {...props}>{children}</OnClickOutsideWrapper>
+  );
+}

frontend/src/metabase/components/Modal/Modal.jsx

@@ -10,8 +10,8 @@ import { getScrollX, getScrollY } from "metabase/lib/dom";
 
 import SandboxedPortal from "metabase/components/SandboxedPortal";
 import routeless from "metabase/hoc/Routeless";
-import OnClickOutsideWrapper from "metabase/components/OnClickOutsideWrapper";
 import ModalContent from "metabase/components/ModalContent";
+import { MaybeOnClickOutsideWrapper } from "metabase/components/Modal/MaybeOnClickOutsideWrapper";
 
 function getModalContent(props) {
   if (
@@ -30,6 +30,7 @@ export class WindowModal extends Component {
     isOpen: PropTypes.bool,
     enableMouseEvents: PropTypes.bool,
     enableTransition: PropTypes.bool,
+    closeOnClickOutside: PropTypes.bool,
   };
 
   static defaultProps = {
@@ -64,7 +65,8 @@ export class WindowModal extends Component {
         .map(type => `Modal--${type}`),
     );
     return (
-      <OnClickOutsideWrapper
+      <MaybeOnClickOutsideWrapper
+        closeOnClickOutside={this.props.closeOnClickOutside}
         backdropElement={this._modalElement}
         handleDismissal={this.handleDismissal}
       >
@@ -80,7 +82,7 @@ export class WindowModal extends Component {
             formModal: !!this.props.form || this.props.formModal,
           })}
         </div>
-      </OnClickOutsideWrapper>
+      </MaybeOnClickOutsideWrapper>
     );
   }
 
@@ -195,7 +197,10 @@ export class FullPageModal extends Component {
               occupies the entire screen. We do this to put this modal on top of
               the OnClickOutsideWrapper popover stack.  Otherwise, clicks within
               this modal might be seen as clicks outside another popover. */}
-              <OnClickOutsideWrapper handleDismissal={this.handleDismissal}>
+              <MaybeOnClickOutsideWrapper
+                closeOnClickOutside={this.props.closeOnClickOutside}
+                handleDismissal={this.handleDismissal}
+              >
                 <div
                   className="full-height relative scroll-y"
                   style={motionStyle}
@@ -207,7 +212,7 @@ export class FullPageModal extends Component {
                     onClose: this.handleDismissal,
                   })}
                 </div>
-              </OnClickOutsideWrapper>
+              </MaybeOnClickOutsideWrapper>
             </div>
           </SandboxedPortal>
         )}

frontend/src/metabase/components/NewItemMenu/NewItemMenu.tsx

@@ -160,7 +160,12 @@ const NewItemMenu = ({
               />
             </Modal>
           ) : modal === "new-action" ? (
-            <Modal wide enableTransition={false} onClose={handleModalClose}>
+            <Modal
+              wide
+              enableTransition={false}
+              onClose={handleModalClose}
+              closeOnClickOutside
+            >
               <ActionCreator
                 onSubmit={handleActionCreated}
                 onClose={handleModalClose}

frontend/src/metabase/dashboard/components/ActionSidebar/ActionSidebar.tsx

@@ -117,6 +117,7 @@ export function ActionSidebarFn({
           ref={actionSettingsModalRef}
           fit
           enableMouseEvents
+          closeOnClickOutside
           triggerElement={
             !dashcard.action ? (
               <Button primary={!dashcard.action} fullWidth>

frontend/src/metabase/dashboard/components/ActionSidebar/ActionSidebar.unit.spec.tsx

 import React from "react";
-import { screen, waitFor } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
-import { renderWithProviders } from "__support__/ui";
+import { screen, waitFor, renderWithProviders } from "__support__/ui";
 
 import {
   createMockDashboard,
   createMockActionDashboardCard,
   createMockDashboardOrderedCard,
   createMockQueryAction,
+  createMockCard,
+  createMockDatabase,
+  createMockCollectionItem,
 } from "metabase-types/api/mocks";
 
+import {
+  setupActionsEndpoints,
+  setupCardsEndpoints,
+  setupDatabasesEndpoints,
+  setupSearchEndpoints,
+} from "__support__/server-mocks";
 import { ActionSidebarFn } from "./ActionSidebar";
 
 const dashcard = createMockDashboardOrderedCard();
@@ -19,6 +27,13 @@ const actionDashcardWithAction = createMockActionDashboardCard({
   action: createMockQueryAction(),
 });
 
+const collectionItem = createMockCollectionItem({
+  model: "dataset",
+});
+const modelCard = createMockCard();
+const actionsDatabase = createMockDatabase({
+  settings: { "database-enable-actions": true },
+});
 const dashboard = createMockDashboard({
   ordered_cards: [dashcard, actionDashcard, actionDashcardWithAction],
 });
@@ -26,6 +41,11 @@ const dashboard = createMockDashboard({
 const setup = (
   options?: Partial<React.ComponentProps<typeof ActionSidebarFn>>,
 ) => {
+  setupDatabasesEndpoints([actionsDatabase]);
+  setupSearchEndpoints([collectionItem]);
+  setupActionsEndpoints([]);
+  setupCardsEndpoints([modelCard]);
+
   const vizUpdateSpy = jest.fn();
   const closeSpy = jest.fn();
 
@@ -42,6 +62,19 @@ const setup = (
   return { vizUpdateSpy, closeSpy };
 };
 
+const navigateToActionCreatorModal = async () => {
+  userEvent.click(screen.getByText("Pick an action"));
+  await waitFor(() => {
+    expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument();
+  });
+  userEvent.click(screen.getByText(collectionItem.name));
+  userEvent.click(screen.getByText("Create new action"));
+
+  await waitFor(() => {
+    expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument();
+  });
+};
+
 describe("Dashboard > ActionSidebar", () => {
   it("shows an action sidebar with text and variant form fields", () => {
     setup();
@@ -97,4 +130,32 @@ describe("Dashboard > ActionSidebar", () => {
 
     expect(screen.getByText("Change action")).toBeInTheDocument();
   });
+
+  describe("ActionCreator Modal", () => {
+    it("should not close modal on outside click", async () => {
+      setup();
+      await navigateToActionCreatorModal();
+
+      userEvent.click(document.body);
+
+      const mockNativeQueryEditor = screen.getByTestId(
+        "mock-native-query-editor",
+      );
+
+      expect(mockNativeQueryEditor).toBeInTheDocument();
+    });
+
+    it("should close modal when clicking 'Cancel'", async () => {
+      setup();
+      await navigateToActionCreatorModal();
+
+      const cancelButton = screen.getByRole("button", { name: "Cancel" });
+
+      userEvent.click(cancelButton);
+
+      expect(
+        screen.queryByTestId("mock-native-query-editor"),
+      ).not.toBeInTheDocument();
+    });
+  });
 });

frontend/src/metabase/dashboard/components/DashCard/DashCardActionButtons/ActionSettingsButton.tsx

-import React from "react";
+import React, { useEffect } from "react";
 import { t } from "ttag";
 import { connect } from "react-redux";
 
@@ -19,9 +19,11 @@ interface Props {
 }
 
 function ActionSettingsButton({ dashcard, setEditingDashcardId }: Props) {
-  if (dashcard.justAdded) {
-    setEditingDashcardId(dashcard.id);
-  }
+  useEffect(() => {
+    if (dashcard.justAdded) {
+      setEditingDashcardId(dashcard.id);
+    }
+  }, [dashcard.id, dashcard.justAdded, setEditingDashcardId]);
 
   return (
     <DashCardActionButton

frontend/src/metabase/dashboard/components/DashboardHeaderView.tsx

@@ -4,12 +4,11 @@ import React, {
   useRef,
   useMemo,
   useCallback,
+  useEffect,
 } from "react";
 import { t } from "ttag";
 import cx from "classnames";
 
-import { useMount } from "react-use";
-
 import { getScrollY } from "metabase/lib/dom";
 import { Dashboard } from "metabase-types/api";
 
@@ -105,12 +104,12 @@ function DashboardHeaderView({
     [setDashboardAttribute, onSave, isEditing],
   );
 
-  useMount(() => {
+  useEffect(() => {
     const timerId = setTimeout(() => {
       setShowSubHeader(false);
     }, 4000);
     return () => clearTimeout(timerId);
-  });
+  }, []);
 
   return (
     <div>

frontend/src/metabase/dashboard/containers/DashboardApp.unit.spec.jsx → frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.unit.spec.jsx

 import React from "react";
 import { Route } from "react-router";
 import userEvent from "@testing-library/user-event";
-import fetchMock from "fetch-mock";
 import {
   screen,
   renderWithProviders,
@@ -10,34 +9,58 @@ import {
 import DashboardApp from "metabase/dashboard/containers/DashboardApp";
 import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload";
 import {
+  createMockCard,
   createMockCollection,
+  createMockCollectionItem,
   createMockDashboard,
+  createMockDatabase,
+  createMockTable,
   createMockUser,
 } from "metabase-types/api/mocks";
 import { createMockDashboardState } from "metabase-types/store/mocks";
 import {
+  setupActionsEndpoints,
+  setupBookmarksEndpoints,
+  setupCardsEndpoints,
   setupCollectionItemsEndpoint,
   setupCollectionsEndpoints,
   setupDashboardEndpoints,
+  setupDatabasesEndpoints,
+  setupSearchEndpoints,
+  setupTableEndpoints,
 } from "__support__/server-mocks";
+import { createMockEntitiesState } from "__support__/store";
 import { callMockEvent } from "__support__/events";
 
-const TEST_DASHBOARD = createMockDashboard({
-  id: 1,
-  name: "Example",
+const TEST_DASHBOARD = createMockDashboard();
+const TEST_COLLECTION = createMockCollection();
+
+const TEST_DATABASE_WITH_ACTIONS = createMockDatabase({
+  settings: { "database-enable-actions": true },
 });
 
-const TEST_COLLECTION = createMockCollection({
-  id: "root",
+const TEST_COLLECTION_ITEM = createMockCollectionItem({
+  collection: TEST_COLLECTION,
+  model: "dataset",
 });
 
+const TEST_CARD = createMockCard();
+
+const TEST_TABLE = createMockTable();
+
 async function setup({ user = createMockUser() }) {
+  setupDatabasesEndpoints([TEST_DATABASE_WITH_ACTIONS]);
   setupDashboardEndpoints(createMockDashboard(TEST_DASHBOARD));
   setupCollectionsEndpoints([]);
   setupCollectionItemsEndpoint(TEST_COLLECTION);
+  setupSearchEndpoints([TEST_COLLECTION_ITEM]);
+  setupCardsEndpoints([TEST_CARD]);
+  setupTableEndpoints([TEST_TABLE]);
 
-  fetchMock.get("path:/api/bookmark", []);
+  setupBookmarksEndpoints([]);
+  setupActionsEndpoints([]);
 
+  window.HTMLElement.prototype.scrollIntoView = function () {};
   const mockEventListener = jest.spyOn(window, "addEventListener");
 
   const DashboardAppContainer = props => {
@@ -57,6 +80,9 @@ async function setup({ user = createMockUser() }) {
       withRouter: true,
       storeInitialState: {
         dashboard: createMockDashboardState(),
+        entities: createMockEntitiesState({
+          databases: [TEST_DATABASE_WITH_ACTIONS],
+        }),
       },
     },
   );
@@ -73,28 +99,34 @@ describe("DashboardApp", function () {
     jest.clearAllMocks();
   });
 
-  it("should have a beforeunload event when the user tries to leave a dirty dashboard", async function () {
-    const { mockEventListener } = await setup({});
+  describe("beforeunload events", () => {
+    afterEach(() => {
+      jest.clearAllMocks();
+    });
 
-    userEvent.click(screen.getByLabelText("Edit dashboard"));
-    userEvent.click(screen.getByTestId("dashboard-name-heading"));
-    userEvent.type(screen.getByTestId("dashboard-name-heading"), "a");
-    // need to click away from the input to trigger the isDirty flag
-    userEvent.tab();
+    it("should have a beforeunload event when the user tries to leave a dirty dashboard", async function () {
+      const { mockEventListener } = await setup({});
 
-    const mockEvent = callMockEvent(mockEventListener, "beforeunload");
+      userEvent.click(screen.getByLabelText("Edit dashboard"));
+      userEvent.click(screen.getByTestId("dashboard-name-heading"));
+      userEvent.type(screen.getByTestId("dashboard-name-heading"), "a");
+      // need to click away from the input to trigger the isDirty flag
+      userEvent.tab();
 
-    expect(mockEvent.preventDefault).toHaveBeenCalled();
-    expect(mockEvent.returnValue).toEqual(BEFORE_UNLOAD_UNSAVED_MESSAGE);
-  });
+      const mockEvent = callMockEvent(mockEventListener, "beforeunload");
+
+      expect(mockEvent.preventDefault).toHaveBeenCalled();
+      expect(mockEvent.returnValue).toEqual(BEFORE_UNLOAD_UNSAVED_MESSAGE);
+    });
 
-  it("should not have a beforeunload event when the dashboard is unedited", async function () {
-    const { mockEventListener } = await setup({});
+    it("should not have a beforeunload event when the dashboard is unedited", async function () {
+      const { mockEventListener } = await setup({});
 
-    userEvent.click(screen.getByLabelText("Edit dashboard"));
+      userEvent.click(screen.getByLabelText("Edit dashboard"));
 
-    const mockEvent = callMockEvent(mockEventListener, "beforeunload");
-    expect(mockEvent.preventDefault).not.toHaveBeenCalled();
-    expect(mockEvent.returnValue).toBe(undefined);
+      const mockEvent = callMockEvent(mockEventListener, "beforeunload");
+      expect(mockEvent.preventDefault).not.toHaveBeenCalled();
+      expect(mockEvent.returnValue).toBe(undefined);
+    });
   });
 });

frontend/src/metabase/models/components/ModelDetailPage/ModelActionDetails/ModelActionDetails.unit.spec.tsx

+import userEvent from "@testing-library/user-event";
+import {
+  createMockDatabase,
+  createMockQueryAction,
+} from "metabase-types/api/mocks";
+import {
+  createOrdersTable,
+  createStructuredModelCard,
+} from "metabase-types/api/mocks/presets";
+import {
+  setupCardsEndpoints,
+  setupDatabasesEndpoints,
+  setupModelActionsEndpoints,
+  setupTableEndpoints,
+} from "__support__/server-mocks";
+import {
+  renderWithProviders,
+  waitForElementToBeRemoved,
+  screen,
+} from "__support__/ui";
+
+import { getRoutes as getModelRoutes } from "metabase/models/routes";
+import {
+  Card,
+  Database,
+  StructuredDatasetQuery,
+  WritebackQueryAction,
+} from "metabase-types/api";
+
+const TEST_DATABASE_WITH_ACTIONS = createMockDatabase({
+  settings: { "database-enable-actions": true },
+});
+
+const TEST_MODEL = createStructuredModelCard();
+
+const TEST_ACTION = createMockQueryAction({ model_id: TEST_MODEL.id });
+
+const TEST_TABLE = createOrdersTable();
+
+async function setup({
+  model = TEST_MODEL,
+  actions = [TEST_ACTION],
+  databases = [TEST_DATABASE_WITH_ACTIONS],
+  initialRoute = `/model/${TEST_MODEL.id}/detail/actions/${TEST_ACTION.id}`,
+}: {
+  model?: Card<StructuredDatasetQuery>;
+  actions?: WritebackQueryAction[];
+  databases?: Database[];
+  initialRoute?: string;
+}) {
+  setupDatabasesEndpoints(databases);
+  setupCardsEndpoints([model]);
+  setupModelActionsEndpoints(actions, model.id);
+  setupTableEndpoints(TEST_TABLE);
+
+  renderWithProviders(getModelRoutes(), {
+    withRouter: true,
+    initialRoute,
+  });
+
+  await waitForElementToBeRemoved(() => screen.queryAllByText(/Loading/i));
+}
+
+describe("ModelActionDetails", () => {
+  it("should not leave ActionCreatorModal when clicking outside modal", async () => {
+    await setup({});
+
+    userEvent.click(document.body);
+
+    const mockQueryEditor = await screen.findByTestId(
+      "mock-native-query-editor",
+    );
+
+    expect(mockQueryEditor).toBeInTheDocument();
+  });
+
+  it("should leave ActionCreatorModal when clicking 'Cancel'", async () => {
+    await setup({});
+
+    (await screen.findByText("Cancel")).click();
+
+    expect(
+      screen.queryByTestId("mock-native-query-editor"),
+    ).not.toBeInTheDocument();
+  });
+});

frontend/src/metabase/models/routes.tsx

+import React from "react";
+import { IndexRedirect, Redirect } from "react-router";
+import { Route } from "metabase/hoc/Title";
+import ModelDetailPage from "metabase/models/containers/ModelDetailPage/ModelDetailPage";
+import { ModalRoute } from "metabase/hoc/ModalRoute";
+import ActionCreatorModal from "metabase/actions/containers/ActionCreatorModal/ActionCreatorModal";
+
+export const getRoutes = () => (
+  <Route path="/model/:slug/detail">
+    <IndexRedirect to="usage" />
+    <Route path="usage" component={ModelDetailPage} />
+    <Route path="schema" component={ModelDetailPage} />
+    <Route path="actions" component={ModelDetailPage}>
+      <ModalRoute
+        path="new"
+        modal={ActionCreatorModal}
+        modalProps={{
+          wide: true,
+          enableTransition: false,
+          closeOnClickOutside: true,
+        }}
+      />
+      <ModalRoute
+        path=":actionId"
+        modal={ActionCreatorModal}
+        modalProps={{
+          wide: true,
+          enableTransition: false,
+          closeOnClickOutside: true,
+        }}
+      />
+    </Route>
+    <Redirect from="*" to="usage" />
+  </Route>
+);

frontend/src/metabase/query_builder/components/NativeQueryEditor/DataSourceSelectors/DataSourceSelectors.jsx

@@ -83,6 +83,7 @@ const DataSourceSelectors = ({
 
   return (
     <PopulatedDataSourceSelectors
+      isNativeEditorOpen={isNativeEditorOpen}
       database={database}
       databases={databases}
       query={query}

frontend/src/metabase/routes.jsx

 import React from "react";
-import { Redirect, IndexRedirect, IndexRoute } from "react-router";
+import { IndexRedirect, IndexRoute, Redirect } from "react-router";
 import { t } from "ttag";
 
 import { Route } from "metabase/hoc/Title";
@@ -71,6 +71,7 @@ import FieldDetailContainer from "metabase/reference/databases/FieldDetailContai
 import getAccountRoutes from "metabase/account/routes";
 import getAdminRoutes from "metabase/admin/routes";
 import getCollectionTimelineRoutes from "metabase/timelines/collections/routes";
+import { getRoutes as getModelRoutes } from "metabase/models/routes";
 
 import PublicQuestion from "metabase/public/containers/PublicQuestion";
 import PublicDashboard from "metabase/public/containers/PublicDashboard";
@@ -85,15 +86,12 @@ import CollectionLanding from "metabase/collections/components/CollectionLanding
 import ArchiveApp from "metabase/home/containers/ArchiveApp";
 import SearchApp from "metabase/home/containers/SearchApp";
 import { trackPageView } from "metabase/lib/analytics";
-
-import ActionCreatorModal from "metabase/actions/containers/ActionCreatorModal";
-import ModelDetailPage from "metabase/models/containers/ModelDetailPage";
 import {
   CanAccessMetabot,
-  IsNotAuthenticated,
-  IsAuthenticated,
-  IsAdmin,
   CanAccessSettings,
+  IsAdmin,
+  IsAuthenticated,
+  IsNotAuthenticated,
 } from "./route-guards";
 
 export const getRoutes = store => (
@@ -191,30 +189,14 @@ export const getRoutes = store => (
           <Route path=":slug/:objectId" component={QueryBuilder} />
         </Route>
 
-        <Route path="/model/:slug/detail">
-          <IndexRedirect to="usage" />
-          <Route path="usage" component={ModelDetailPage} />
-          <Route path="schema" component={ModelDetailPage} />
-          <Route path="actions" component={ModelDetailPage}>
-            <ModalRoute
-              path="new"
-              modal={ActionCreatorModal}
-              modalProps={{ wide: true, enableTransition: false }}
-            />
-            <ModalRoute
-              path=":actionId"
-              modal={ActionCreatorModal}
-              modalProps={{ wide: true, enableTransition: false }}
-            />
-          </Route>
-          <Redirect from="*" to="usage" />
-        </Route>
-
         <Route path="/metabot" component={CanAccessMetabot}>
           <Route path="database/:databaseId" component={DatabaseMetabotApp} />
           <Route path="model/:slug" component={ModelMetabotApp} />
         </Route>
 
+        {/* MODELS */}
+        {getModelRoutes()}
+
         <Route path="/model">
           <IndexRoute component={QueryBuilder} />
           <Route path="new" title={t`New Model`} component={NewModelOptions} />