Add custom warning message when leaving creating/editing a DB connection (#34272)

* Use LeaveConfirmationModal in QueryBuilder

* Add isLocationAllowed prop to LeaveConfirmationModal

* Do not route-block running modified native question

* Fix typo

* Introduce explanatory variables

* Move updating URL during question creation to QueryBuilder

* Schedule navigation to after re-render so that LeaveConfirmationModal prop has a chance to update

* Add a test for unsaved changes warning

* Introduce useCallbackEffect

* Refactor QueryBuilder to use useCallbackEffect

* Refactor ActionCreator to use useCallbackEffect

* Fix useCallbackEffect typing

* Move onSubmit out of the scheduled callback to reduce probability of race conditions

* Add comments explaining useCallbackEffect usage

* Improve JSDocs

* Add a test for running modified question

* Update button query to more accessible one

* Add a test for saving a modified question

* Update test names

* Add a test for leaving with no changes

* Remove isDirty-related no-change assertions, as isDirty flag does not work reliably
- Question.prototype.isDirtyComparedTo would return true because edited "question"
  would have extra "description: null" attribute and no "name" compared to
  "originalQuestion" (see "getIsDirty")

* Move useCallbackEffect to its own directory

* Add test setup for useCallbackEffect

* Add a test case for scheduling

* Add a test for scheduling callback effect

* Do not disable navigation in models
- this is a temporary condition until #33793 is implemented

* Remove redundant _.noop

* Rename isScheduled to isCallbackScheduled

* Replace useBeforeUnload with LeaveConfirmationModal in DatabaseEditApp

* Refactor DatabaseEditApp.unit.spec.js to TypeScript

* Add initialRoute param, return history & add /home route

* Add a test case for showing the unsaved changes modal

* Add a test case for not showing the unsaved changes modal

* Format code

* Extend a test case to assert isDirty being properly computed

* Remove redundant route

* Remove redundant code

* Add a test for custom warning modal when saving edited question as a new one

* Redirect to databases page on database creation in component so that isDirty flag can re-compute on re-render

* Fix mocks setup

* Remove redundant withRouter usage

* Use waitFor + expect + not.toBeInDocument instead waitForElementToBeRemoved

* Add a test case for creating new database connection

* Remove redundant promise

frontend/src/metabase/actions/containers/ActionCreatorModal/ActionCreatorModal.unit.spec.tsx

@@ -2,12 +2,7 @@ import userEvent from "@testing-library/user-event";
 import fetchMock from "fetch-mock";
 import { Route } from "react-router";
 
-import {
-  renderWithProviders,
-  screen,
-  waitFor,
-  waitForElementToBeRemoved,
-} from "__support__/ui";
+import { renderWithProviders, screen, waitFor } from "__support__/ui";
 import {
   setupCardsEndpoints,
   setupDatabasesEndpoints,
@@ -76,6 +71,10 @@ async function setup({
     },
   );
 
+  await waitFor(() => {
+    expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument();
+  });
+
   return { history: checkNotNull(history) };
 }
 
@@ -97,10 +96,6 @@ describe("actions > containers > ActionCreatorModal", () => {
     const initialRoute = `/model/${MODEL.id}/detail/actions/${ACTION_NOT_FOUND_ID}`;
     const { history } = await setup({ initialRoute, action: null });
 
-    await waitForElementToBeRemoved(() =>
-      screen.queryAllByTestId("loading-spinner"),
-    );
-
     expect(await screen.findByTestId("mock-model-detail")).toBeInTheDocument();
     expect(screen.queryByTestId("action-creator")).not.toBeInTheDocument();
     expect(history.getCurrentLocation().pathname).toBe(
@@ -113,10 +108,6 @@ describe("actions > containers > ActionCreatorModal", () => {
     const initialRoute = `/model/${MODEL.id}/detail/actions/${action.id}`;
     const { history } = await setup({ initialRoute, action });
 
-    await waitForElementToBeRemoved(() =>
-      screen.queryAllByTestId("loading-spinner"),
-    );
-
     expect(await screen.findByTestId("mock-model-detail")).toBeInTheDocument();
     expect(screen.queryByTestId("action-creator")).not.toBeInTheDocument();
     expect(history.getCurrentLocation().pathname).toBe(

frontend/src/metabase/admin/databases/containers/DatabaseEditApp.tsx

 import type { ComponentType } from "react";
 import { useState } from "react";
 import { connect } from "react-redux";
+import { push } from "react-router-redux";
+import type { Route } from "react-router";
 
 import { t } from "ttag";
 import _ from "underscore";
 import { updateIn } from "icepick";
 
 import { useMount } from "react-use";
-import type { Location } from "history";
+import type { Location, LocationDescriptor } from "history";
 import title from "metabase/hoc/Title";
 
 import Breadcrumbs from "metabase/components/Breadcrumbs";
 import Sidebar from "metabase/admin/databases/components/DatabaseEditApp/Sidebar/Sidebar";
 import { getUserIsAdmin } from "metabase/selectors/user";
+import { useCallbackEffect } from "metabase/hooks/use-callback-effect";
 
 import { getSetting } from "metabase/selectors/settings";
 
+import { LeaveConfirmationModal } from "metabase/components/LeaveConfirmationModal";
 import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
 import { DatabaseForm } from "metabase/databases/components/DatabaseForm";
 import ErrorBoundary from "metabase/ErrorBoundary";
@@ -26,7 +30,6 @@ import type {
   DatabaseId,
 } from "metabase-types/api";
 import type { State } from "metabase-types/store";
-import useBeforeUnload from "metabase/hooks/use-before-unload";
 import Database from "metabase-lib/metadata/Database";
 
 import { getEditingDatabase, getInitializeError } from "../selectors";
@@ -73,6 +76,8 @@ interface DatabaseEditAppProps {
   isAdmin: boolean;
   isModelPersistenceEnabled: boolean;
   initializeError?: DatabaseEditErrorType;
+  route: Route;
+  onChangeLocation: (location: LocationDescriptor) => void;
 }
 
 const mapStateToProps = (state: State) => {
@@ -97,6 +102,7 @@ const mapDispatchToProps = {
   discardSavedFieldValues,
   deleteDatabase,
   selectEngine,
+  onChangeLocation: push,
 };
 
 type DatabaseEditErrorType = {
@@ -124,6 +130,8 @@ function DatabaseEditApp(props: DatabaseEditAppProps) {
     initializeDatabase,
     params,
     saveDatabase,
+    route,
+    onChangeLocation,
   } = props;
 
   const editingExistingDatabase = database?.id != null;
@@ -131,7 +139,11 @@ function DatabaseEditApp(props: DatabaseEditAppProps) {
 
   const [isDirty, setIsDirty] = useState(false);
 
-  useBeforeUnload(isDirty);
+  /**
+   * Navigation is scheduled so that LeaveConfirmationModal's isEnabled
+   * prop has a chance to re-compute on re-render
+   */
+  const [isCallbackScheduled, scheduleCallback] = useCallbackEffect();
 
   useMount(async () => {
     await reset();
@@ -145,6 +157,12 @@ function DatabaseEditApp(props: DatabaseEditAppProps) {
   const handleSubmit = async (database: DatabaseData) => {
     try {
       await saveDatabase(database);
+
+      if (addingNewDatabase) {
+        scheduleCallback(() => {
+          onChangeLocation("/admin/databases?created=true");
+        });
+      }
     } catch (error) {
       throw getSubmitError(error as DatabaseEditErrorType);
     }
@@ -195,6 +213,11 @@ function DatabaseEditApp(props: DatabaseEditAppProps) {
           />
         )}
       </DatabaseEditMain>
+
+      <LeaveConfirmationModal
+        isEnabled={isDirty && !isCallbackScheduled}
+        route={route}
+      />
     </DatabaseEditRoot>
   );
 }

frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js → frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.tsx

 import { IndexRoute, Route } from "react-router";
-
 import userEvent from "@testing-library/user-event";
+
 import {
   renderWithProviders,
   screen,
-  waitForElementToBeRemoved,
   waitFor,
+  waitForElementToBeRemoved,
 } from "__support__/ui";
 import { setupEnterpriseTest } from "__support__/enterprise";
 import { mockSettings } from "__support__/settings";
 
+import type { Engine } from "metabase-types/api";
 import {
   createMockDatabase,
+  createMockEngineSource,
   createMockTokenFeatures,
 } from "metabase-types/api/mocks";
-import { setupDatabaseEndpoints } from "__support__/server-mocks";
+import {
+  setupDatabaseEndpoints,
+  setupDatabasesEndpoints,
+} from "__support__/server-mocks";
 
+import { checkNotNull } from "metabase/core/utils/types";
 import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload";
 import { callMockEvent } from "__support__/events";
 import DatabaseEditApp from "./DatabaseEditApp";
 
-const ENGINES_MOCK = {
+const ENGINES_MOCK: Record<string, Engine> = {
   H2: {
     "details-fields": [
       { "display-name": "Connection String", name: "db", required: true },
@@ -28,6 +34,7 @@ const ENGINES_MOCK = {
     ],
     "driver-name": "H2",
     "superseded-by": null,
+    source: createMockEngineSource(),
   },
   sqlite: {
     "details-fields": [
@@ -36,12 +43,27 @@ const ENGINES_MOCK = {
     ],
     "driver-name": "SQLite",
     "superseded-by": null,
+    source: createMockEngineSource(),
   },
 };
 
-async function setup({ cachingEnabled = false, databaseIdParam = "" } = {}) {
+const MockComponent = () => <div />;
+
+interface SetupOpts {
+  cachingEnabled?: boolean;
+  databaseIdParam?: string;
+  initialRoute?: string;
+}
+
+async function setup({
+  cachingEnabled = false,
+  databaseIdParam = "",
+  initialRoute = `/${databaseIdParam}`,
+}: SetupOpts = {}) {
   const mockEventListener = jest.spyOn(window, "addEventListener");
 
+  setupDatabasesEndpoints([]);
+
   const settings = mockSettings({
     engines: ENGINES_MOCK,
     "token-features": createMockTokenFeatures({
@@ -50,23 +72,30 @@ async function setup({ cachingEnabled = false, databaseIdParam = "" } = {}) {
     "enable-query-caching": cachingEnabled,
   });
 
-  renderWithProviders(
+  const { history } = renderWithProviders(
     <Route path="/">
+      <Route path="/home" component={MockComponent} />
+      <Route path="/admin/databases" component={MockComponent} />
       <IndexRoute component={DatabaseEditApp} />
       <Route path=":databaseId" component={DatabaseEditApp} />
     </Route>,
     {
       withRouter: true,
-      initialRoute: `/${databaseIdParam}`,
+      initialRoute,
       storeInitialState: {
         settings,
       },
     },
   );
 
-  await waitForElementToBeRemoved(() => screen.queryByText("Loading..."));
+  await waitFor(() => {
+    expect(screen.queryByText("Loading...")).not.toBeInTheDocument();
+  });
 
-  return { mockEventListener };
+  return {
+    history: checkNotNull(history),
+    mockEventListener,
+  };
 }
 
 describe("DatabaseEditApp", () => {
@@ -95,9 +124,9 @@ describe("DatabaseEditApp", () => {
     it("should trigger beforeunload event when database connection is edited", async () => {
       const { mockEventListener } = await setup();
 
-      const databaseForm = await screen.findByLabelText("Display name");
+      const displayNameInput = await screen.findByLabelText("Display name");
 
-      userEvent.type(databaseForm, "Test database");
+      userEvent.type(displayNameInput, "Test database");
       const mockEvent = await waitFor(() => {
         return callMockEvent(mockEventListener, "beforeunload");
       });
@@ -112,6 +141,91 @@ describe("DatabaseEditApp", () => {
       expect(mockEvent.preventDefault).not.toHaveBeenCalled();
       expect(mockEvent.returnValue).toBe(undefined);
     });
+
+    it("does not show custom warning modal when leaving with no changes via SPA navigation", async () => {
+      const { history } = await setup({ initialRoute: "/home" });
+
+      history.push("/");
+
+      await waitForElementToBeRemoved(() =>
+        screen.queryAllByTestId("loading-spinner"),
+      );
+
+      const displayNameInput = await screen.findByLabelText("Display name");
+      userEvent.type(displayNameInput, "ab");
+      userEvent.type(displayNameInput, "{backspace}{backspace}");
+
+      history.goBack();
+
+      expect(
+        screen.queryByText("Changes were not saved"),
+      ).not.toBeInTheDocument();
+      expect(
+        screen.queryByText(
+          "Navigating away from here will cause you to lose any changes you have made.",
+        ),
+      ).not.toBeInTheDocument();
+    });
+
+    it("shows custom warning modal when leaving with unsaved changes via SPA navigation", async () => {
+      const { history } = await setup({ initialRoute: "/home" });
+
+      history.push("/");
+
+      await waitForElementToBeRemoved(() =>
+        screen.queryAllByTestId("loading-spinner"),
+      );
+      const displayNameInput = await screen.findByLabelText("Display name");
+      userEvent.type(displayNameInput, "Test database");
+
+      history.goBack();
+
+      expect(screen.getByText("Changes were not saved")).toBeInTheDocument();
+      expect(
+        screen.getByText(
+          "Navigating away from here will cause you to lose any changes you have made.",
+        ),
+      ).toBeInTheDocument();
+    });
+
+    it("does not show custom warning modal after creating new database connection", async () => {
+      const { history } = await setup({ initialRoute: "/home" });
+
+      history.push("/");
+
+      await waitForElementToBeRemoved(() =>
+        screen.queryAllByTestId("loading-spinner"),
+      );
+
+      const displayNameInput = await screen.findByLabelText("Display name");
+      userEvent.type(displayNameInput, "Test database");
+      const connectionStringInput = await screen.findByLabelText(
+        "Connection String",
+      );
+      userEvent.type(
+        connectionStringInput,
+        "file:/sample-database.db;USER=GUEST;PASSWORD=guest",
+      );
+
+      userEvent.click(await screen.findByText("Save"));
+
+      await waitFor(() => {
+        expect(history.getCurrentLocation().pathname).toEqual(
+          "/admin/databases",
+        );
+      });
+
+      expect(history.getCurrentLocation().search).toEqual("?created=true");
+
+      expect(
+        screen.queryByText("Changes were not saved"),
+      ).not.toBeInTheDocument();
+      expect(
+        screen.queryByText(
+          "Navigating away from here will cause you to lose any changes you have made.",
+        ),
+      ).not.toBeInTheDocument();
+    });
   });
 
   describe("Cache TTL field", () => {

frontend/src/metabase/admin/databases/database.js

@@ -166,7 +166,6 @@ export const createDatabase = function (database) {
       );
 
       dispatch({ type: CREATE_DATABASE });
-      dispatch(push("/admin/databases?created=true"));
     } catch (error) {
       console.error("error creating a database", error);
       MetabaseAnalytics.trackStructEvent(

frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx

@@ -243,11 +243,9 @@ const setup = async ({
     },
   );
 
-  if (initialRoute !== "/home") {
-    await waitForElementToBeRemoved(() =>
-      screen.queryByTestId("loading-spinner"),
-    );
-  }
+  await waitFor(() => {
+    expect(screen.queryByTestId("loading-spinner")).not.toBeInTheDocument();
+  });
 
   return {
     history: checkNotNull(history),

frontend/test/__support__/server-mocks/database.ts

@@ -39,6 +39,10 @@ export function setupDatabasesEndpoints(
     hasSavedQuestions ? [...dbs, SAVED_QUESTIONS_DATABASE] : dbs,
   );
   fetchMock.get({ url: "path:/api/database", overwriteRoutes: false }, dbs);
+  fetchMock.post("path:/api/database", async url => {
+    const lastCall = fetchMock.lastCall(url);
+    return await lastCall?.request?.json();
+  });
 
   dbs.forEach(db => setupDatabaseEndpoints(db));
 }