From bf49ab3568ea18f9c669b6df1fff032b324d5b4e Mon Sep 17 00:00:00 2001 From: Oisin Coveney <oisin@metabase.com> Date: Fri, 19 May 2023 13:12:59 +0300 Subject: [PATCH] Warn users before they lose new changes (#30632) --- e2e/support/helpers/e2e-dashboard-helpers.js | 2 +- frontend/src/metabase-types/api/dataset.ts | 9 +- .../src/metabase-types/api/mocks/dataset.ts | 23 +- .../src/metabase-types/api/mocks/group.ts | 10 + .../metabase-types/api/mocks/permissions.ts | 38 +++ .../src/metabase-types/api/permissions.ts | 5 +- frontend/src/metabase-types/store/admin.ts | 1 + .../ActionContext/ActionContext.ts | 2 + .../ImplicitActionContextProvider.tsx | 5 +- .../QueryActionContextProvider.tsx | 12 +- .../ActionCreator/ActionCreator.tsx | 4 + .../FormCreator/FormCreator.styled.tsx | 3 +- .../tests/ActionCreator-Common.unit.spec.tsx | 50 ++++ .../databases/containers/DatabaseEditApp.jsx | 187 --------------- .../databases/containers/DatabaseEditApp.tsx | 214 +++++++++++++++++ .../containers/DatabaseEditApp.unit.spec.js | 35 +++ .../src/metabase/admin/databases/database.js | 1 - .../components/FormInput/FormInput.tsx | 1 + .../components/MetricForm/MetricForm.tsx | 16 +- .../MetricForm/MetricForm.unit.spec.tsx | 51 ++++ .../components/PartialQueryBuilder.jsx | 2 +- .../components/SegmentForm/SegmentForm.tsx | 16 +- .../SegmentForm/SegmentForm.unit.spec.tsx | 51 ++++ ...geLayout.jsx => PermissionsPageLayout.tsx} | 101 ++++---- .../ToolbarUpsell/ToolbarUpsell.tsx | 7 +- ...ssionsPage.jsx => DataPermissionsPage.tsx} | 84 ++++--- .../DatabasesPermissionsPage.unit.spec.tsx | 109 +++++++++ .../GroupsPermissionsPage.unit.spec.tsx | 114 +++++++++ .../AddToDashSelectDashModal.unit.spec.tsx | 4 +- .../ItemPicker/ItemPicker.unit.spec.js | 4 +- .../metabase/dashboard/actions/parameters.js | 2 +- .../containers/DashboardApp.unit.spec.jsx | 100 ++++++++ .../containers/DashboardApp/DashboardApp.jsx | 47 ++-- .../dashboard/containers/DashboardHeader.jsx | 12 +- .../dashboard/containers/selectors.jsx | 0 .../components/DatabaseForm/DatabaseForm.tsx | 13 +- .../metabase/hooks/use-before-unload/index.ts | 2 + .../use-before-unload/use-before-unload.ts | 15 ++ .../__mocks__/NativeQueryEditor.jsx | 29 ++- .../query_builder/containers/QueryBuilder.jsx | 18 ++ .../containers/QueryBuilder.unit.spec.tsx | 220 ++++++++++++++++-- .../components/ChartCaption.unit.spec.tsx | 12 +- .../__support__/server-mocks/collection.ts | 33 ++- .../test/__support__/server-mocks/group.ts | 6 + .../__support__/server-mocks/permissions.ts | 8 + 45 files changed, 1306 insertions(+), 372 deletions(-) create mode 100644 frontend/src/metabase-types/api/mocks/group.ts create mode 100644 frontend/src/metabase-types/api/mocks/permissions.ts delete mode 100644 frontend/src/metabase/admin/databases/containers/DatabaseEditApp.jsx create mode 100644 frontend/src/metabase/admin/databases/containers/DatabaseEditApp.tsx create mode 100644 frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.unit.spec.tsx create mode 100644 frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.unit.spec.tsx rename frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/{PermissionsPageLayout.jsx => PermissionsPageLayout.tsx} (59%) rename frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/{DataPermissionsPage.jsx => DataPermissionsPage.tsx} (55%) create mode 100644 frontend/src/metabase/admin/permissions/test/DatabasesPermissionsPage/DatabasesPermissionsPage.unit.spec.tsx create mode 100644 frontend/src/metabase/admin/permissions/test/GroupsPermissionsPage/GroupsPermissionsPage.unit.spec.tsx create mode 100644 frontend/src/metabase/dashboard/containers/DashboardApp.unit.spec.jsx delete mode 100644 frontend/src/metabase/dashboard/containers/selectors.jsx create mode 100644 frontend/src/metabase/hooks/use-before-unload/index.ts create mode 100644 frontend/src/metabase/hooks/use-before-unload/use-before-unload.ts create mode 100644 frontend/test/__support__/server-mocks/group.ts create mode 100644 frontend/test/__support__/server-mocks/permissions.ts diff --git a/e2e/support/helpers/e2e-dashboard-helpers.js b/e2e/support/helpers/e2e-dashboard-helpers.js index 8317c93c88e..5b04f84f218 100644 --- a/e2e/support/helpers/e2e-dashboard-helpers.js +++ b/e2e/support/helpers/e2e-dashboard-helpers.js @@ -102,5 +102,5 @@ export function addTextBox(string, options = {}) { } export function openQuestionsSidebar() { - cy.findByLabelText("add questions").click(); + cy.findByLabelText("Add questions").click(); } diff --git a/frontend/src/metabase-types/api/dataset.ts b/frontend/src/metabase-types/api/dataset.ts index 329a29e6c52..07ee9ea44ab 100644 --- a/frontend/src/metabase-types/api/dataset.ts +++ b/frontend/src/metabase-types/api/dataset.ts @@ -14,7 +14,7 @@ export interface DatasetColumn { name: string; display_name: string; description: string | null; - source: string; + source?: string; coercion_strategy: string | null; visibility_type: FieldVisibilityType; table_id: TableId; @@ -35,6 +35,10 @@ export interface DatasetColumn { fingerprint: FieldFingerprint | null; } +export interface ResultsMetadata { + columns: DatasetColumn[]; +} + export interface DatasetData { rows: RowValues[]; cols: DatasetColumn[]; @@ -42,6 +46,7 @@ export interface DatasetData { requested_timezone?: string; results_timezone?: string; download_perms?: DownloadPermission; + results_metadata: ResultsMetadata; } export type JsonQuery = DatasetQuery & { @@ -55,6 +60,8 @@ export interface Dataset { running_time: number; json_query?: JsonQuery; error?: string; + context?: string; + status?: string; } export interface NativeQueryForm { diff --git a/frontend/src/metabase-types/api/mocks/dataset.ts b/frontend/src/metabase-types/api/mocks/dataset.ts index 811b0ebabb5..d995b75052e 100644 --- a/frontend/src/metabase-types/api/mocks/dataset.ts +++ b/frontend/src/metabase-types/api/mocks/dataset.ts @@ -2,11 +2,12 @@ import { Dataset, DatasetColumn, DatasetData, + ResultsMetadata, TemplateTag, } from "metabase-types/api/dataset"; export const createMockColumn = ( - data?: Partial<DatasetColumn>, + data: Partial<DatasetColumn> = {}, ): DatasetColumn => { return { id: 1, @@ -22,18 +23,20 @@ export const createMockColumn = ( }; }; -export const createMockDatasetData = ( - opts?: Partial<DatasetData>, -): DatasetData => ({ - rows: [], - cols: [ +export const createMockDatasetData = ({ + cols = [ createMockColumn({ display_name: "NAME", source: "native", name: "NAME", }), ], + ...opts +}: Partial<DatasetData>): DatasetData => ({ + rows: [], + cols, rows_truncated: 0, + results_metadata: createMockResultsMetadata(cols), ...opts, }); @@ -61,3 +64,11 @@ export const createMockTemplateTag = ( type: "text", ...opts, }); + +export const createMockResultsMetadata = ( + columns: DatasetColumn[] = [createMockColumn()], + opts?: Partial<ResultsMetadata>, +): ResultsMetadata => ({ + columns, + ...opts, +}); diff --git a/frontend/src/metabase-types/api/mocks/group.ts b/frontend/src/metabase-types/api/mocks/group.ts new file mode 100644 index 00000000000..b96fab009ef --- /dev/null +++ b/frontend/src/metabase-types/api/mocks/group.ts @@ -0,0 +1,10 @@ +import { Group } from "metabase-types/api"; + +export const createMockGroup = ( + opts?: Partial<Group>, +): Omit<Group, "members"> => ({ + id: 1, + name: "All Users", + member_count: 1, + ...opts, +}); diff --git a/frontend/src/metabase-types/api/mocks/permissions.ts b/frontend/src/metabase-types/api/mocks/permissions.ts new file mode 100644 index 00000000000..6594f4e19ea --- /dev/null +++ b/frontend/src/metabase-types/api/mocks/permissions.ts @@ -0,0 +1,38 @@ +import { + Database, + Group, + GroupsPermissions, + PermissionsGraph, +} from "metabase-types/api"; + +export const createMockPermissionsGraph = ({ + groups, + databases, +}: { + groups: Omit<Group, "members">[]; + databases: Database[]; +}): PermissionsGraph => { + const permissionGroups: GroupsPermissions = {}; + + for (const group of groups) { + for (const database of databases) { + permissionGroups[group.id] = { + [database.id]: { + data: { + native: "write", + schemas: "all", + }, + download: { + native: "full", + schemas: "full", + }, + }, + }; + } + } + + return { + groups: permissionGroups, + revision: 1, + }; +}; diff --git a/frontend/src/metabase-types/api/permissions.ts b/frontend/src/metabase-types/api/permissions.ts index 7ed0ffbeb98..4ec937ad3af 100644 --- a/frontend/src/metabase-types/api/permissions.ts +++ b/frontend/src/metabase-types/api/permissions.ts @@ -18,6 +18,7 @@ export type GroupPermissions = { export type DownloadPermission = "full" | "limited" | "none"; export type DownloadAccessPermission = { + native?: DownloadSchemasPermission; schemas: DownloadSchemasPermission; }; @@ -37,9 +38,9 @@ export type DownloadTablePermission = export type DatabasePermissions = { data: DatabaseAccessPermissions; - "data-model": DataModelPermissions; + "data-model"?: DataModelPermissions; download: DownloadAccessPermission; - details: DetailsPermissions; + details?: DetailsPermissions; }; export type DataModelPermissions = { diff --git a/frontend/src/metabase-types/store/admin.ts b/frontend/src/metabase-types/store/admin.ts index 04db324d88f..35784c7619d 100644 --- a/frontend/src/metabase-types/store/admin.ts +++ b/frontend/src/metabase-types/store/admin.ts @@ -21,6 +21,7 @@ export interface AdminState { permissions: { dataPermissions: GroupsPermissions; originalDataPermissions: GroupsPermissions; + saveError?: string; }; settings: { settings: SettingDefinition[]; diff --git a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ActionContext.ts b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ActionContext.ts index 972ce77a33b..5ecaf5a7ba9 100644 --- a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ActionContext.ts +++ b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ActionContext.ts @@ -13,6 +13,7 @@ export type ActionContextType = { formSettings: ActionFormSettings; canSave: boolean; isNew: boolean; + isDirty: boolean; ui: ActionCreatorUIProps; handleActionChange: (action: EditableActionParams) => void; handleFormSettingsChange: (formSettings: ActionFormSettings) => void; @@ -24,6 +25,7 @@ export const ActionContext = createContext<ActionContextType>({ formSettings: getDefaultFormSettings(), canSave: false, isNew: true, + isDirty: false, ui: { canRename: true, canChangeFieldSettings: true, diff --git a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ImplicitActionContextProvider/ImplicitActionContextProvider.tsx b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ImplicitActionContextProvider/ImplicitActionContextProvider.tsx index 5d8540e9534..0d534f65aa7 100644 --- a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ImplicitActionContextProvider/ImplicitActionContextProvider.tsx +++ b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/ImplicitActionContextProvider/ImplicitActionContextProvider.tsx @@ -11,7 +11,7 @@ import type { import { getDefaultFormSettings } from "../../../../utils"; import type { ActionContextProviderProps } from "../types"; -import { ActionContext } from "../ActionContext"; +import { ActionContext, ActionContextType } from "../ActionContext"; import { EditorBodyRoot, EditorTitle, @@ -64,11 +64,12 @@ function ImplicitActionContextProvider({ }, [formSettings, initialAction?.visualization_settings]); const value = useMemo( - () => ({ + (): ActionContextType => ({ action: initialAction, formSettings, isNew: false, canSave, + isDirty: canSave, ui: { canRename: false, canChangeFieldSettings: false, diff --git a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx index 0c0ff40934a..b206a438e7e 100644 --- a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx +++ b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx @@ -1,5 +1,6 @@ import React, { useCallback, useEffect, useMemo, useState } from "react"; +import _ from "underscore"; import { CreateQueryActionParams } from "metabase/entities/actions"; import type { @@ -19,7 +20,7 @@ import { getTemplateTagParametersFromCard } from "metabase-lib/parameters/utils/ import { getDefaultFormSettings } from "../../../../utils"; -import { ActionContext } from "../ActionContext"; +import { ActionContext, ActionContextType } from "../ActionContext"; import type { ActionContextProviderProps, EditorBodyProps } from "../types"; import { @@ -185,12 +186,17 @@ function QueryActionContextProvider({ [query, handleQueryChange], ); + const isDirty = useMemo(() => { + return canSave && !_.isEqual(action, initialAction); + }, [action, canSave, initialAction]); + const value = useMemo( - () => ({ + (): ActionContextType => ({ action, formSettings, isNew, canSave, + isDirty, ui: { canRename: true, canChangeFieldSettings: true, @@ -204,8 +210,8 @@ function QueryActionContextProvider({ formSettings, isNew, canSave, + isDirty, handleActionChange, - setFormSettings, renderEditorBody, ], ); diff --git a/frontend/src/metabase/actions/containers/ActionCreator/ActionCreator.tsx b/frontend/src/metabase/actions/containers/ActionCreator/ActionCreator.tsx index 19f7f1ff894..598c8d60d31 100644 --- a/frontend/src/metabase/actions/containers/ActionCreator/ActionCreator.tsx +++ b/frontend/src/metabase/actions/containers/ActionCreator/ActionCreator.tsx @@ -22,6 +22,7 @@ import type { } from "metabase-types/api"; import type { State } from "metabase-types/store"; +import useBeforeUnload from "metabase/hooks/use-before-unload"; import Question from "metabase-lib/Question"; import type Metadata from "metabase-lib/metadata/Metadata"; @@ -87,12 +88,15 @@ function ActionCreator({ formSettings, isNew, canSave, + isDirty, ui: UIProps, handleActionChange, handleFormSettingsChange, renderEditorBody, } = useActionContext(); + useBeforeUnload(isDirty); + const [isSaveModalShown, setShowSaveModal] = useState(false); const isEditable = isNew || (model != null && model.canWriteActions()); diff --git a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.styled.tsx b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.styled.tsx index 6349ec51bca..7868d04d46c 100644 --- a/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.styled.tsx +++ b/frontend/src/metabase/actions/containers/ActionCreator/FormCreator/FormCreator.styled.tsx @@ -40,7 +40,7 @@ export const ExplainerTitle = styled.h3` margin-bottom: ${space(1)}; `; -export const ExplainerText = styled.p` +export const ExplainerText = styled.div` font-weight: 400; line-height: 1.5rem; color: ${color("text-medium")}; @@ -64,6 +64,7 @@ export const ExplainerLink = styled(ExternalLink)` margin-top: ${space(2)}; color: ${color("brand")}; + &:hover { color: ${lighten("brand", 0.1)}; } diff --git a/frontend/src/metabase/actions/containers/ActionCreator/tests/ActionCreator-Common.unit.spec.tsx b/frontend/src/metabase/actions/containers/ActionCreator/tests/ActionCreator-Common.unit.spec.tsx index 18417dbca5b..2b1a1344edb 100644 --- a/frontend/src/metabase/actions/containers/ActionCreator/tests/ActionCreator-Common.unit.spec.tsx +++ b/frontend/src/metabase/actions/containers/ActionCreator/tests/ActionCreator-Common.unit.spec.tsx @@ -7,6 +7,9 @@ import { createMockQueryAction, } from "metabase-types/api/mocks"; +import { callMockEvent } from "__support__/events"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; +import { getDefaultFormSettings } from "metabase/actions/utils"; import { setup as baseSetup, SetupOpts } from "./common"; async function setup({ @@ -23,6 +26,10 @@ describe("ActionCreator > Common", () => { ["implicit", createMockImplicitQueryAction], ])(`%s actions`, (_, getAction) => { describe("with write permissions", () => { + afterEach(() => { + jest.resetAllMocks(); + }); + it("should show action settings button", async () => { await setup({ action: getAction(), canWrite: true }); const button = screen.getByRole("button", { name: "Action settings" }); @@ -44,6 +51,49 @@ describe("ActionCreator > Common", () => { screen.getByRole("textbox", { name: "Success message" }), ).toHaveValue("Thanks!"); }); + + it("should warn the user before leaving an edited action", async () => { + const mockEventListener = jest.spyOn(window, "addEventListener"); + await setup({ + action: getAction({ + visualization_settings: getDefaultFormSettings(), + }), + canWrite: true, + }); + + userEvent.click( + screen.getByRole("button", { name: "Action settings" }), + ); + + userEvent.type( + screen.getByRole("textbox", { name: "Success message" }), + `Thanks!`, + ); + + userEvent.tab(); + + expect( + screen.getByRole("textbox", { name: "Success message" }), + ).toHaveValue("Thanks!"); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.returnValue).toEqual(BEFORE_UNLOAD_UNSAVED_MESSAGE); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + }); + + it("should not warn user when leaving unedited action", async () => { + const mockEventListener = jest.spyOn(window, "addEventListener"); + await setup({ + action: getAction({ + visualization_settings: getDefaultFormSettings(), + }), + canWrite: true, + }); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.returnValue).toBeUndefined(); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + }); }); describe("with read-only permissions", () => { diff --git a/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.jsx b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.jsx deleted file mode 100644 index 4c0d6b943ca..00000000000 --- a/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.jsx +++ /dev/null @@ -1,187 +0,0 @@ -/* eslint-disable react/prop-types */ -import React, { Component } from "react"; -import PropTypes from "prop-types"; -import { connect } from "react-redux"; - -import { t } from "ttag"; -import _ from "underscore"; -import { updateIn } from "icepick"; - -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 { getSetting } from "metabase/selectors/settings"; - -import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper"; -import DatabaseForm from "metabase/databases/containers/DatabaseForm"; -import ErrorBoundary from "metabase/ErrorBoundary"; -import { GenericError } from "metabase/containers/ErrorPages"; -import Database from "metabase-lib/metadata/Database"; - -import { getEditingDatabase, getInitializeError } from "../selectors"; - -import { - reset, - initializeDatabase, - saveDatabase, - updateDatabase, - syncDatabaseSchema, - dismissSyncSpinner, - rescanDatabaseFields, - discardSavedFieldValues, - deleteDatabase, - selectEngine, -} from "../database"; -import { - DatabaseEditContent, - DatabaseEditForm, - DatabaseEditHelp, - DatabaseEditMain, - DatabaseEditRoot, -} from "./DatabaseEditApp.styled"; - -const mapStateToProps = state => { - const database = getEditingDatabase(state); - - return { - database: database ? new Database(database) : undefined, - initializeError: getInitializeError(state), - isAdmin: getUserIsAdmin(state), - isModelPersistenceEnabled: getSetting(state, "persisted-models-enabled"), - }; -}; - -const mapDispatchToProps = { - reset, - initializeDatabase, - saveDatabase, - updateDatabase, - syncDatabaseSchema, - dismissSyncSpinner, - rescanDatabaseFields, - discardSavedFieldValues, - deleteDatabase, - selectEngine, -}; - -class DatabaseEditApp extends Component { - constructor(props, context) { - super(props, context); - } - - static propTypes = { - database: PropTypes.object, - metadata: PropTypes.object, - params: PropTypes.object.isRequired, - reset: PropTypes.func.isRequired, - initializeDatabase: PropTypes.func.isRequired, - syncDatabaseSchema: PropTypes.func.isRequired, - dismissSyncSpinner: PropTypes.func.isRequired, - rescanDatabaseFields: PropTypes.func.isRequired, - discardSavedFieldValues: PropTypes.func.isRequired, - deleteDatabase: PropTypes.func.isRequired, - saveDatabase: PropTypes.func.isRequired, - updateDatabase: PropTypes.func.isRequired, - selectEngine: PropTypes.func.isRequired, - location: PropTypes.object, - isAdmin: PropTypes.bool, - isModelPersistenceEnabled: PropTypes.bool, - }; - - async componentDidMount() { - await this.props.reset(); - await this.props.initializeDatabase(this.props.params.databaseId); - } - - render() { - const { - database, - deleteDatabase, - updateDatabase, - discardSavedFieldValues, - initializeError, - rescanDatabaseFields, - syncDatabaseSchema, - dismissSyncSpinner, - isAdmin, - isModelPersistenceEnabled, - } = this.props; - const editingExistingDatabase = database?.id != null; - const addingNewDatabase = !editingExistingDatabase; - - const crumbs = [ - [t`Databases`, "/admin/databases"], - [addingNewDatabase ? t`Add Database` : database.name], - ]; - - const handleSubmit = async database => { - try { - await this.props.saveDatabase(database); - } catch (error) { - throw getSubmitError(error); - } - }; - - return ( - <DatabaseEditRoot> - <Breadcrumbs className="py4" crumbs={crumbs} /> - - <DatabaseEditMain> - <ErrorBoundary errorComponent={GenericError}> - <div> - <div className="pt0"> - <LoadingAndErrorWrapper - loading={!database} - error={initializeError} - > - <DatabaseEditContent> - <DatabaseEditForm> - <DatabaseForm - initialValues={database} - isAdvanced - onSubmit={handleSubmit} - /> - </DatabaseEditForm> - <div>{addingNewDatabase && <DatabaseEditHelp />}</div> - </DatabaseEditContent> - </LoadingAndErrorWrapper> - </div> - </div> - </ErrorBoundary> - - {editingExistingDatabase && ( - <Sidebar - database={database} - isAdmin={isAdmin} - isModelPersistenceEnabled={isModelPersistenceEnabled} - updateDatabase={updateDatabase} - deleteDatabase={deleteDatabase} - discardSavedFieldValues={discardSavedFieldValues} - rescanDatabaseFields={rescanDatabaseFields} - syncDatabaseSchema={syncDatabaseSchema} - dismissSyncSpinner={dismissSyncSpinner} - /> - )} - </DatabaseEditMain> - </DatabaseEditRoot> - ); - } -} - -const getSubmitError = error => { - if (_.isObject(error?.data?.errors)) { - return updateIn(error, ["data", "errors"], errors => ({ - details: errors, - })); - } - - return error; -}; - -export default _.compose( - connect(mapStateToProps, mapDispatchToProps), - title(({ database }) => database && database.name), -)(DatabaseEditApp); diff --git a/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.tsx b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.tsx new file mode 100644 index 00000000000..a7514a79b7c --- /dev/null +++ b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.tsx @@ -0,0 +1,214 @@ +import React, { ComponentType, useState } from "react"; +import { connect } from "react-redux"; + +import { t } from "ttag"; +import _ from "underscore"; +import { updateIn } from "icepick"; + +import { useMount } from "react-use"; +import type { Location } 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 { getSetting } from "metabase/selectors/settings"; + +import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper"; +import DatabaseForm from "metabase/databases/containers/DatabaseForm"; +import ErrorBoundary from "metabase/ErrorBoundary"; +import { GenericError } from "metabase/containers/ErrorPages"; +import { + Database as DatabaseType, + DatabaseData, + DatabaseId, +} from "metabase-types/api"; +import { 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"; + +import { + reset, + initializeDatabase, + saveDatabase, + updateDatabase, + syncDatabaseSchema, + dismissSyncSpinner, + rescanDatabaseFields, + discardSavedFieldValues, + deleteDatabase, + selectEngine, +} from "../database"; +import { + DatabaseEditContent, + DatabaseEditForm, + DatabaseEditHelp, + DatabaseEditMain, + DatabaseEditRoot, +} from "./DatabaseEditApp.styled"; + +interface DatabaseEditAppProps { + database: Database; + params: { databaseId: DatabaseId }; + reset: () => void; + initializeDatabase: (databaseId: DatabaseId) => void; + syncDatabaseSchema: (databaseId: DatabaseId) => Promise<void>; + dismissSyncSpinner: (databaseId: DatabaseId) => Promise<void>; + rescanDatabaseFields: (databaseId: DatabaseId) => Promise<void>; + discardSavedFieldValues: (databaseId: DatabaseId) => Promise<void>; + deleteDatabase: ( + databaseId: DatabaseId, + isDetailView: boolean, + ) => Promise<void>; + saveDatabase: (database: DatabaseData) => void; + updateDatabase: ( + database: { id: DatabaseId } & Partial<DatabaseType>, + ) => Promise<void>; + selectEngine: (engine: string) => void; + location: Location; + isAdmin: boolean; + isModelPersistenceEnabled: boolean; + initializeError?: DatabaseEditErrorType; +} + +const mapStateToProps = (state: State) => { + const database = getEditingDatabase(state); + + return { + database: database ? new Database(database) : undefined, + initializeError: getInitializeError(state), + isAdmin: getUserIsAdmin(state), + isModelPersistenceEnabled: getSetting(state, "persisted-models-enabled"), + }; +}; + +const mapDispatchToProps = { + reset, + initializeDatabase, + saveDatabase, + updateDatabase, + syncDatabaseSchema, + dismissSyncSpinner, + rescanDatabaseFields, + discardSavedFieldValues, + deleteDatabase, + selectEngine, +}; + +type DatabaseEditErrorType = { + data: { + message: string; + errors: { [key: string]: string }; + }; + statusText: string; + message: string; +}; + +function DatabaseEditApp(props: DatabaseEditAppProps) { + const { + database, + deleteDatabase, + updateDatabase, + discardSavedFieldValues, + initializeError, + rescanDatabaseFields, + syncDatabaseSchema, + dismissSyncSpinner, + isAdmin, + isModelPersistenceEnabled, + reset, + initializeDatabase, + params, + saveDatabase, + } = props; + + const editingExistingDatabase = database?.id != null; + const addingNewDatabase = !editingExistingDatabase; + + const [isDirty, setIsDirty] = useState(false); + + useBeforeUnload(isDirty); + + useMount(async () => { + await reset(); + await initializeDatabase(params.databaseId); + }); + + const crumbs = [ + [t`Databases`, "/admin/databases"], + [addingNewDatabase ? t`Add Database` : database.name], + ]; + const handleSubmit = async (database: DatabaseData) => { + try { + await saveDatabase(database); + } catch (error) { + throw getSubmitError(error as DatabaseEditErrorType); + } + }; + + return ( + <DatabaseEditRoot> + <Breadcrumbs className="py4" crumbs={crumbs} /> + + <DatabaseEditMain> + <ErrorBoundary errorComponent={GenericError as ComponentType}> + <div> + <div className="pt0"> + <LoadingAndErrorWrapper + loading={!database} + error={initializeError} + > + <DatabaseEditContent> + <DatabaseEditForm> + <DatabaseForm + initialValues={database} + isAdvanced + onSubmit={handleSubmit} + setIsDirty={setIsDirty} + /> + </DatabaseEditForm> + <div>{addingNewDatabase && <DatabaseEditHelp />}</div> + </DatabaseEditContent> + </LoadingAndErrorWrapper> + </div> + </div> + </ErrorBoundary> + + {editingExistingDatabase && ( + <Sidebar + database={database} + isAdmin={isAdmin} + isModelPersistenceEnabled={isModelPersistenceEnabled} + updateDatabase={updateDatabase} + deleteDatabase={deleteDatabase} + discardSavedFieldValues={discardSavedFieldValues} + rescanDatabaseFields={rescanDatabaseFields} + syncDatabaseSchema={syncDatabaseSchema} + dismissSyncSpinner={dismissSyncSpinner} + /> + )} + </DatabaseEditMain> + </DatabaseEditRoot> + ); +} + +const getSubmitError = (error: DatabaseEditErrorType) => { + if (_.isObject(error?.data?.errors)) { + return updateIn(error, ["data", "errors"], errors => ({ + details: errors, + })); + } + + return error; +}; + +// eslint-disable-next-line import/no-default-export -- deprecated usage +export default _.compose( + connect(mapStateToProps, mapDispatchToProps), + title( + ({ database }: { database: DatabaseData }) => database && database.name, + ), +)(DatabaseEditApp); diff --git a/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js index 71f55814ea4..841e6d12c29 100644 --- a/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js +++ b/frontend/src/metabase/admin/databases/containers/DatabaseEditApp.unit.spec.js @@ -1,16 +1,20 @@ import React from "react"; import { Route } from "react-router"; +import userEvent from "@testing-library/user-event"; import { renderWithProviders, screen, waitForElementToBeRemoved, + waitFor, } from "__support__/ui"; import { setupEnterpriseTest } from "__support__/enterprise"; import { mockSettings } from "__support__/settings"; import { createMockTokenFeatures } from "metabase-types/api/mocks"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; +import { callMockEvent } from "__support__/events"; import DatabaseEditApp from "./DatabaseEditApp"; const ENGINES_MOCK = { @@ -39,6 +43,8 @@ jest.mock( ); async function setup({ cachingEnabled = false } = {}) { + const mockEventListener = jest.spyOn(window, "addEventListener"); + const settings = mockSettings({ engines: ENGINES_MOCK, "token-features": createMockTokenFeatures({ advanced_config: true }), @@ -53,9 +59,38 @@ async function setup({ cachingEnabled = false } = {}) { }); await waitForElementToBeRemoved(() => screen.queryByText("Loading...")); + + return { mockEventListener }; } describe("DatabaseEditApp", () => { + describe("Database connections", () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it("should trigger beforeunload event when database connection is edited", async () => { + const { mockEventListener } = await setup(); + + const databaseForm = await screen.findByLabelText("Display name"); + + userEvent.type(databaseForm, "Test database"); + const mockEvent = await waitFor(() => { + return callMockEvent(mockEventListener, "beforeunload"); + }); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not trigger beforeunload event when database connection is unchanged", async () => { + const { mockEventListener } = await setup(); + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); + }); + describe("Cache TTL field", () => { describe("OSS", () => { it("is invisible", async () => { diff --git a/frontend/src/metabase/admin/databases/database.js b/frontend/src/metabase/admin/databases/database.js index c4675ead400..a1b2a05167d 100644 --- a/frontend/src/metabase/admin/databases/database.js +++ b/frontend/src/metabase/admin/databases/database.js @@ -316,7 +316,6 @@ export const closeSyncingModal = createThunkAction( ); // reducers - const editingDatabase = handleActions( { [RESET]: () => null, diff --git a/frontend/src/metabase/admin/datamodel/components/FormInput/FormInput.tsx b/frontend/src/metabase/admin/datamodel/components/FormInput/FormInput.tsx index 4ba9b43d9e6..6b4d6db4b77 100644 --- a/frontend/src/metabase/admin/datamodel/components/FormInput/FormInput.tsx +++ b/frontend/src/metabase/admin/datamodel/components/FormInput/FormInput.tsx @@ -14,6 +14,7 @@ const FormInput = forwardRef(function FormInput( return ( <FormInputRoot {...props} + value={props.value ?? ""} ref={ref} className={cx("input", className)} type="text" diff --git a/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.tsx b/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.tsx index 5375cf5f5da..4dd7c5ec2dd 100644 --- a/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.tsx +++ b/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.tsx @@ -7,6 +7,7 @@ import { formatValue } from "metabase/lib/formatting"; import Button from "metabase/core/components/Button"; import FieldSet from "metabase/components/FieldSet"; import { Metric, StructuredQuery } from "metabase-types/api"; +import useBeforeUnload from "metabase/hooks/use-before-unload"; import * as Q from "metabase-lib/queries/utils/query"; import FormInput from "../FormInput"; import FormLabel from "../FormLabel"; @@ -42,12 +43,15 @@ const MetricForm = ({ }: MetricFormProps): JSX.Element => { const isNew = metric == null; - const { isValid, getFieldProps, getFieldMeta, handleSubmit } = useFormik({ - initialValues: metric ?? {}, - isInitialValid: false, - validate: getFormErrors, - onSubmit, - }); + const { isValid, getFieldProps, getFieldMeta, handleSubmit, dirty } = + useFormik({ + initialValues: metric ?? {}, + isInitialValid: false, + validate: getFormErrors, + onSubmit, + }); + + useBeforeUnload(dirty); return ( <FormRoot onSubmit={handleSubmit}> diff --git a/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.unit.spec.tsx b/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.unit.spec.tsx new file mode 100644 index 00000000000..bdaa89a8918 --- /dev/null +++ b/frontend/src/metabase/admin/datamodel/components/MetricForm/MetricForm.unit.spec.tsx @@ -0,0 +1,51 @@ +import React from "react"; +import userEvent from "@testing-library/user-event"; +import { renderWithProviders, screen, waitFor } from "__support__/ui"; +import MetricApp from "metabase/admin/datamodel/containers/MetricApp"; +import { Route } from "metabase/hoc/Title"; +import { callMockEvent } from "__support__/events"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; + +const setup = () => { + renderWithProviders( + <Route path="/admin/datamodel/metric/create" component={MetricApp} />, + { + initialRoute: "/admin/datamodel/metric/create", + withRouter: true, + }, + ); + + const mockEventListener = jest.spyOn(window, "addEventListener"); + + return { mockEventListener }; +}; + +describe("MetricForm", () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it("should have beforeunload event when user makes edits to a metric", async () => { + const { mockEventListener } = setup(); + + const descriptionInput = screen.getByPlaceholderText( + "Something descriptive but not too long", + ); + userEvent.type(descriptionInput, "01189998819991197253"); + + const mockEvent = await waitFor(() => { + return callMockEvent(mockEventListener, "beforeunload"); + }); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not have an beforeunload event when metric is unedited", async () => { + const { mockEventListener } = setup(); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); +}); diff --git a/frontend/src/metabase/admin/datamodel/components/PartialQueryBuilder.jsx b/frontend/src/metabase/admin/datamodel/components/PartialQueryBuilder.jsx index df19bd285e1..6c445ad7ae6 100644 --- a/frontend/src/metabase/admin/datamodel/components/PartialQueryBuilder.jsx +++ b/frontend/src/metabase/admin/datamodel/components/PartialQueryBuilder.jsx @@ -21,7 +21,7 @@ import withTableMetadataLoaded from "../hoc/withTableMetadataLoaded"; class PartialQueryBuilder extends Component { static propTypes = { onChange: PropTypes.func.isRequired, - table: PropTypes.object.isRequired, + table: PropTypes.object, updatePreviewSummary: PropTypes.func.isRequired, previewSummary: PropTypes.string, }; diff --git a/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.tsx b/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.tsx index 37c7f16ef41..230773be33d 100644 --- a/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.tsx +++ b/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.tsx @@ -7,6 +7,7 @@ import { formatValue } from "metabase/lib/formatting"; import Button from "metabase/core/components/Button/Button"; import FieldSet from "metabase/components/FieldSet"; import { Segment, StructuredQuery } from "metabase-types/api"; +import useBeforeUnload from "metabase/hooks/use-before-unload"; import * as Q from "metabase-lib/queries/utils/query"; import FormInput from "../FormInput"; import FormLabel from "../FormLabel"; @@ -41,12 +42,15 @@ const SegmentForm = ({ }: SegmentFormProps): JSX.Element => { const isNew = segment == null; - const { isValid, getFieldProps, getFieldMeta, handleSubmit } = useFormik({ - initialValues: segment ?? {}, - isInitialValid: false, - validate: getFormErrors, - onSubmit, - }); + const { isValid, getFieldProps, getFieldMeta, handleSubmit, dirty } = + useFormik({ + initialValues: segment ?? {}, + isInitialValid: false, + validate: getFormErrors, + onSubmit, + }); + + useBeforeUnload(dirty); return ( <FormRoot onSubmit={handleSubmit}> diff --git a/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.unit.spec.tsx b/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.unit.spec.tsx new file mode 100644 index 00000000000..b3bde40a14f --- /dev/null +++ b/frontend/src/metabase/admin/datamodel/components/SegmentForm/SegmentForm.unit.spec.tsx @@ -0,0 +1,51 @@ +import React from "react"; +import userEvent from "@testing-library/user-event"; +import { renderWithProviders, screen, waitFor } from "__support__/ui"; +import { Route } from "metabase/hoc/Title"; +import { callMockEvent } from "__support__/events"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; +import SegmentApp from "metabase/admin/datamodel/containers/SegmentApp"; + +const setup = () => { + renderWithProviders( + <Route path="/admin/datamodel/segment/create" component={SegmentApp} />, + { + initialRoute: "/admin/datamodel/segment/create", + withRouter: true, + }, + ); + + const mockEventListener = jest.spyOn(window, "addEventListener"); + + return { mockEventListener }; +}; + +describe("SegmentForm", () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it("should have beforeunload event when user makes edits to a segment", async () => { + const { mockEventListener } = setup(); + + const descriptionInput = screen.getByPlaceholderText( + "Something descriptive but not too long", + ); + userEvent.type(descriptionInput, "01189998819991197253"); + + const mockEvent = await waitFor(() => { + return callMockEvent(mockEventListener, "beforeunload"); + }); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not have an beforeunload event when metric is segment", async () => { + const { mockEventListener } = setup(); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); +}); diff --git a/frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.jsx b/frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.tsx similarity index 59% rename from frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.jsx rename to frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.tsx index d365da7305d..ffc54c7dd6d 100644 --- a/frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.jsx +++ b/frontend/src/metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.tsx @@ -1,20 +1,18 @@ -import React, { useState } from "react"; -import PropTypes from "prop-types"; +import React, { ReactNode, useState } from "react"; import _ from "underscore"; import { t } from "ttag"; -import { connect } from "react-redux"; import { push } from "react-router-redux"; -import { withRouter } from "react-router"; +import { Route, Router, withRouter } from "react-router"; +import { Location } from "history"; import Button from "metabase/core/components/Button"; import fitViewport from "metabase/hoc/FitViewPort"; import Modal from "metabase/components/Modal"; import ModalContent from "metabase/components/ModalContent"; -import { useLeaveConfirmation } from "../../hooks/use-leave-confirmation"; -import { clearSaveError } from "../../permissions"; -import { ToolbarButton } from "../ToolbarButton"; -import { PermissionsTabs } from "./PermissionsTabs"; +import { PermissionsGraph } from "metabase-types/api"; +import useBeforeUnload from "metabase/hooks/use-before-unload"; +import { useDispatch, useSelector } from "metabase/lib/redux"; import { FullHeightContainer, TabsContainer, @@ -23,38 +21,40 @@ import { PermissionPageSidebar, CloseSidebarButton, ToolbarButtonsContainer, -} from "./PermissionsPageLayout.styled"; -import { PermissionsEditBar } from "./PermissionsEditBar"; +} from "metabase/admin/permissions/components/PermissionsPageLayout/PermissionsPageLayout.styled"; +import { useLeaveConfirmation } from "../../hooks/use-leave-confirmation"; +import { clearSaveError as clearPermissionsSaveError } from "../../permissions"; +import { ToolbarButton } from "../ToolbarButton"; +import { PermissionsTabs } from "./PermissionsTabs"; -const mapDispatchToProps = { - navigateToTab: tab => push(`/admin/permissions/${tab}`), - navigateToLocation: location => push(location.pathname, location.state), - clearSaveError, -}; +import { PermissionsEditBar } from "./PermissionsEditBar"; -const mapStateToProps = (state, _props) => { - return { - saveError: state.admin.permissions.saveError, - }; +type PermissionsPageTab = "data" | "collections"; +type PermissionsPageLayoutProps = { + children: ReactNode; + tab: PermissionsPageTab; + confirmBar?: ReactNode; + diff?: PermissionsGraph; + isDirty: boolean; + onSave: () => void; + onLoad: () => void; + saveError?: string; + clearSaveError: () => void; + navigateToLocation: (location: string) => void; + router: typeof Router; + route: typeof Route; + navigateToTab: (tab: string) => void; + helpContent?: ReactNode; + toolbarRightContent?: ReactNode; }; -const propTypes = { - children: PropTypes.node.isRequired, - tab: PropTypes.oneOf(["data", "collections"]).isRequired, - confirmBar: PropTypes.node, - diff: PropTypes.object, - isDirty: PropTypes.bool, - onSave: PropTypes.func.isRequired, - onLoad: PropTypes.func.isRequired, - saveError: PropTypes.string, - clearSaveError: PropTypes.func.isRequired, - navigateToLocation: PropTypes.func.isRequired, - router: PropTypes.object, - route: PropTypes.object, - navigateToTab: PropTypes.func.isRequired, - helpContent: PropTypes.node, - toolbarRightContent: PropTypes.node, -}; +const CloseSidebarButtonWithDefault = ({ + name = "close", + ...props +}: { + name?: string; + [key: string]: unknown; +}) => <CloseSidebarButton name={name} {...props} />; function PermissionsPageLayout({ children, @@ -63,22 +63,30 @@ function PermissionsPageLayout({ isDirty, onSave, onLoad, - saveError, - clearSaveError, router, route, - navigateToLocation, - navigateToTab, toolbarRightContent, helpContent, -}) { +}: PermissionsPageLayoutProps) { + const saveError = useSelector(state => state.admin.permissions.saveError); + + const dispatch = useDispatch(); + + const navigateToTab = (tab: PermissionsPageTab) => + dispatch(push(`/admin/permissions/${tab}`)); + const navigateToLocation = (location: Location) => + dispatch(push(location.pathname)); + const clearSaveError = () => dispatch(clearPermissionsSaveError()); + const [shouldShowHelp, setShouldShowHelp] = useState(false); + const beforeLeaveConfirmation = useLeaveConfirmation({ router, route, onConfirm: navigateToLocation, isEnabled: isDirty, }); + useBeforeUnload(isDirty); return ( <PermissionPageRoot> @@ -126,7 +134,7 @@ function PermissionsPageLayout({ {shouldShowHelp && ( <PermissionPageSidebar> - <CloseSidebarButton + <CloseSidebarButtonWithDefault size={20} onClick={() => setShouldShowHelp(prev => !prev)} /> @@ -137,10 +145,5 @@ function PermissionsPageLayout({ ); } -PermissionsPageLayout.propTypes = propTypes; - -export default _.compose( - connect(mapStateToProps, mapDispatchToProps), - fitViewport, - withRouter, -)(PermissionsPageLayout); +// eslint-disable-next-line import/no-default-export -- deprecated usage +export default _.compose(fitViewport, withRouter)(PermissionsPageLayout); diff --git a/frontend/src/metabase/admin/permissions/components/ToolbarUpsell/ToolbarUpsell.tsx b/frontend/src/metabase/admin/permissions/components/ToolbarUpsell/ToolbarUpsell.tsx index d26afe06370..77f4aedf56f 100644 --- a/frontend/src/metabase/admin/permissions/components/ToolbarUpsell/ToolbarUpsell.tsx +++ b/frontend/src/metabase/admin/permissions/components/ToolbarUpsell/ToolbarUpsell.tsx @@ -29,11 +29,14 @@ const ToolbarUpsell = ({ upgradeUrl }: ToolbarUpsellProps) => { > <UpsellContent> {jt`${( - <ExternalLink href={upgradeUrl}> + <ExternalLink key="upsell-cta-link" href={upgradeUrl}> {t`Upgrade to Pro or Enterprise`} </ExternalLink> )} and disable download results, control access to the data model, promote group managers, ${( - <ExternalLink href={MetabaseSettings.docsUrl("permissions/start")}> + <ExternalLink + key="upsell-more-link" + href={MetabaseSettings.docsUrl("permissions/start")} + > {t`and more`} </ExternalLink> )}.`} diff --git a/frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage.jsx b/frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage.tsx similarity index 55% rename from frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage.jsx rename to frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage.tsx index 45cedb1b2d3..81463513b44 100644 --- a/frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage.jsx +++ b/frontend/src/metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage.tsx @@ -1,12 +1,14 @@ -import React, { useEffect } from "react"; -import PropTypes from "prop-types"; +import React, { useEffect, useCallback, ReactNode } from "react"; import _ from "underscore"; -import { connect } from "react-redux"; +import { Route } from "react-router"; import Tables from "metabase/entities/tables"; import Groups from "metabase/entities/groups"; import Databases from "metabase/entities/databases"; +import { DatabaseId, Group } from "metabase-types/api"; +import { useDispatch, useSelector } from "metabase/lib/redux"; +import Database from "metabase-lib/metadata/Database"; import { getIsDirty, getDiff } from "../../selectors/data-permissions/diff"; import { saveDataPermissions, @@ -17,51 +19,49 @@ import PermissionsPageLayout from "../../components/PermissionsPageLayout/Permis import { DataPermissionsHelp } from "../../components/DataPermissionsHelp"; import ToolbarUpsell from "../../components/ToolbarUpsell"; +type DataPermissionsPageProps = { + children: ReactNode; + route: typeof Route; + params: { + databaseId: DatabaseId; + }; + databases: Database[]; + groups: Group[]; +}; + export const DATA_PERMISSIONS_TOOLBAR_CONTENT = [ <ToolbarUpsell key="upsell" />, ]; -const mapDispatchToProps = { - loadPermissions: loadDataPermissions, - savePermissions: saveDataPermissions, - initialize: initializeDataPermissions, - fetchTables: dbId => - Tables.actions.fetchList({ - dbId, - include_hidden: true, - }), -}; - -const mapStateToProps = (state, props) => ({ - isDirty: getIsDirty(state, props), - diff: getDiff(state, props), -}); - -const propTypes = { - children: PropTypes.node.isRequired, - isDirty: PropTypes.bool, - diff: PropTypes.object, - savePermissions: PropTypes.func.isRequired, - loadPermissions: PropTypes.func.isRequired, - initialize: PropTypes.func.isRequired, - route: PropTypes.object, - params: PropTypes.shape({ - databaseId: PropTypes.string, - }), - fetchTables: PropTypes.func, -}; - function DataPermissionsPage({ children, - isDirty, - diff, - savePermissions, - loadPermissions, route, params, - initialize, - fetchTables, -}) { + databases, + groups, +}: DataPermissionsPageProps) { + const isDirty = useSelector(getIsDirty); + const diff = useSelector(state => getDiff(state, { databases, groups })); + + const dispatch = useDispatch(); + + const loadPermissions = () => dispatch(loadDataPermissions()); + const savePermissions = () => dispatch(saveDataPermissions()); + const initialize = useCallback( + () => dispatch(initializeDataPermissions()), + [dispatch], + ); + const fetchTables = useCallback( + (dbId: DatabaseId) => + dispatch( + Tables.actions.fetchList({ + dbId, + include_hidden: true, + }), + ), + [dispatch], + ); + useEffect(() => { initialize(); }, [initialize]); @@ -89,12 +89,10 @@ function DataPermissionsPage({ ); } -DataPermissionsPage.propTypes = propTypes; - +// eslint-disable-next-line import/no-default-export -- deprecated usage export default _.compose( Groups.loadList(), Databases.loadList({ selectorName: "getListUnfiltered", }), - connect(mapStateToProps, mapDispatchToProps), )(DataPermissionsPage); diff --git a/frontend/src/metabase/admin/permissions/test/DatabasesPermissionsPage/DatabasesPermissionsPage.unit.spec.tsx b/frontend/src/metabase/admin/permissions/test/DatabasesPermissionsPage/DatabasesPermissionsPage.unit.spec.tsx new file mode 100644 index 00000000000..dbf78ee6b26 --- /dev/null +++ b/frontend/src/metabase/admin/permissions/test/DatabasesPermissionsPage/DatabasesPermissionsPage.unit.spec.tsx @@ -0,0 +1,109 @@ +import React from "react"; +import { Route } from "react-router"; +import fetchMock from "fetch-mock"; +import { + renderWithProviders, + waitForElementToBeRemoved, + screen, + fireEvent, +} from "__support__/ui"; +import DataPermissionsPage from "metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage"; +import { createSampleDatabase } from "metabase-types/api/mocks/presets"; +import { createMockPermissionsGraph } from "metabase-types/api/mocks/permissions"; +import { createMockGroup } from "metabase-types/api/mocks/group"; +import { setupDatabasesEndpoints } from "__support__/server-mocks"; +import { setupPermissionsGraphEndpoint } from "__support__/server-mocks/permissions"; +import { setupGroupsEndpoint } from "__support__/server-mocks/group"; +import DatabasesPermissionsPage from "metabase/admin/permissions/pages/DatabasePermissionsPage/DatabasesPermissionsPage"; +import { PLUGIN_ADMIN_PERMISSIONS_TABLE_GROUP_ROUTES } from "metabase/plugins"; +import { delay } from "metabase/lib/promise"; +import { callMockEvent } from "__support__/events"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; + +const TEST_DATABASE = createSampleDatabase(); + +const TEST_GROUPS = [createMockGroup()]; + +const TEST_PERMISSIONS_GRAPH = createMockPermissionsGraph({ + groups: TEST_GROUPS, + databases: [TEST_DATABASE], +}); + +const setup = async () => { + setupDatabasesEndpoints([TEST_DATABASE]); + setupPermissionsGraphEndpoint(TEST_PERMISSIONS_GRAPH); + setupGroupsEndpoint(TEST_GROUPS); + + fetchMock.get( + `path:/api/database/${TEST_DATABASE.id}/metadata`, + TEST_DATABASE, + ); + + const mockEventListener = jest.spyOn(window, "addEventListener"); + + renderWithProviders( + <Route path="/admin/permissions/data" component={DataPermissionsPage}> + <Route + path="database(/:databaseId)(/schema/:schemaName)(/table/:tableId)" + component={DatabasesPermissionsPage} + > + {PLUGIN_ADMIN_PERMISSIONS_TABLE_GROUP_ROUTES} + </Route> + </Route>, + { + withRouter: true, + initialRoute: `/admin/permissions/data/database/${TEST_DATABASE.id}`, + }, + ); + + await waitForElementToBeRemoved(() => + screen.queryByTestId("loading-spinner"), + ); + + return { mockEventListener }; +}; + +const editDatabasePermission = async () => { + const permissionsSelectElem = screen.getAllByTestId("permissions-select")[0]; + fireEvent.click(permissionsSelectElem); + + const clickElement = screen.getByLabelText("eye icon"); + fireEvent.click(clickElement); + + await delay(0); +}; + +describe("DatabasesPermissionsPage", function () { + afterEach(() => { + jest.restoreAllMocks(); + }); + describe("rendering", () => { + it("should show 'Cancel' and 'Save Changes' when user makes changes to permissions", async () => { + await setup(); + + await editDatabasePermission(); + expect(screen.getByText("Cancel")).toBeInTheDocument(); + expect(screen.getByText("Save changes")).toBeInTheDocument(); + }); + }); + + describe("triggering beforeunload events", () => { + it("should generate beforeunload event when user edits database permissions", async () => { + const { mockEventListener } = await setup(); + + await editDatabasePermission(); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not have beforeunload event when permissions are unedited", async function () { + const { mockEventListener } = await setup(); + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); + }); +}); diff --git a/frontend/src/metabase/admin/permissions/test/GroupsPermissionsPage/GroupsPermissionsPage.unit.spec.tsx b/frontend/src/metabase/admin/permissions/test/GroupsPermissionsPage/GroupsPermissionsPage.unit.spec.tsx new file mode 100644 index 00000000000..bb1e35029d7 --- /dev/null +++ b/frontend/src/metabase/admin/permissions/test/GroupsPermissionsPage/GroupsPermissionsPage.unit.spec.tsx @@ -0,0 +1,114 @@ +import React from "react"; +import { Route } from "react-router"; +import fetchMock from "fetch-mock"; +import { + renderWithProviders, + waitForElementToBeRemoved, + screen, + fireEvent, +} from "__support__/ui"; +import DataPermissionsPage from "metabase/admin/permissions/pages/DataPermissionsPage/DataPermissionsPage"; +import { createSampleDatabase } from "metabase-types/api/mocks/presets"; +import { createMockPermissionsGraph } from "metabase-types/api/mocks/permissions"; +import { createMockGroup } from "metabase-types/api/mocks/group"; +import { setupDatabasesEndpoints } from "__support__/server-mocks"; +import { setupPermissionsGraphEndpoint } from "__support__/server-mocks/permissions"; +import { setupGroupsEndpoint } from "__support__/server-mocks/group"; +import { PLUGIN_ADMIN_PERMISSIONS_TABLE_ROUTES } from "metabase/plugins"; +import GroupsPermissionsPage from "metabase/admin/permissions/pages/GroupDataPermissionsPage/GroupsPermissionsPage"; +import { delay } from "metabase/lib/promise"; +import { callMockEvent } from "__support__/events"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; + +const TEST_DATABASE = createSampleDatabase(); + +const TEST_GROUPS = [ + createMockGroup({ id: 2, name: "Administrators" }), + createMockGroup({ name: "All Users" }), +]; + +const TEST_PERMISSIONS_GRAPH = createMockPermissionsGraph({ + groups: TEST_GROUPS, + databases: [TEST_DATABASE], +}); + +const setup = async () => { + setupDatabasesEndpoints([TEST_DATABASE]); + setupPermissionsGraphEndpoint(TEST_PERMISSIONS_GRAPH); + setupGroupsEndpoint(TEST_GROUPS); + + fetchMock.get( + `path:/api/database/${TEST_DATABASE.id}/metadata`, + TEST_DATABASE, + ); + + const mockEventListener = jest.spyOn(window, "addEventListener"); + + renderWithProviders( + <Route path="/admin/permissions/data" component={DataPermissionsPage}> + <Route + path="group(/:groupId)(/database/:databaseId)(/schema/:schemaName)" + component={GroupsPermissionsPage} + > + {PLUGIN_ADMIN_PERMISSIONS_TABLE_ROUTES} + </Route> + </Route>, + { + withRouter: true, + initialRoute: `/admin/permissions/data/group/${TEST_DATABASE.id}`, + }, + ); + + await waitForElementToBeRemoved(() => + screen.queryByTestId("loading-spinner"), + ); + + return { mockEventListener }; +}; + +const editDatabasePermission = async () => { + const permissionsSelectElem = screen.getAllByTestId("permissions-select")[0]; + fireEvent.click(permissionsSelectElem); + + const clickElement = screen.getByLabelText("eye icon"); + fireEvent.click(clickElement); + + await delay(0); +}; + +describe("GroupsPermissionsPage", function () { + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe("rendering", () => { + it("should show 'Cancel' and 'Save Changes' when user makes changes to permissions", async () => { + await setup(); + + await editDatabasePermission(); + + expect(screen.getByText("Cancel")).toBeInTheDocument(); + expect(screen.getByText("Save changes")).toBeInTheDocument(); + }); + }); + + describe("triggering beforeunload events", () => { + it("should generate beforeunload event when user edits database permissions", async () => { + const { mockEventListener } = await setup(); + + await editDatabasePermission(); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not have beforeunload event when permissions are unedited", async function () { + const { mockEventListener } = await setup(); + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); + }); +}); diff --git a/frontend/src/metabase/containers/AddToDashSelectDashModal.unit.spec.tsx b/frontend/src/metabase/containers/AddToDashSelectDashModal.unit.spec.tsx index bbfeecc51f8..43fb0be2eee 100644 --- a/frontend/src/metabase/containers/AddToDashSelectDashModal.unit.spec.tsx +++ b/frontend/src/metabase/containers/AddToDashSelectDashModal.unit.spec.tsx @@ -6,7 +6,7 @@ import { setupCollectionsEndpoints, setupSearchEndpoints, setupCollectionByIdEndpoint, - setupCollectionItemsEndpoint, + setupDashboardCollectionItemsEndpoint, } from "__support__/server-mocks"; import { renderWithProviders, @@ -97,7 +97,7 @@ const setup = async ({ }: SetupOpts = {}) => { setupSearchEndpoints([]); setupCollectionsEndpoints(collections); - setupCollectionItemsEndpoint([dashboard]); + setupDashboardCollectionItemsEndpoint([dashboard]); setupCollectionByIdEndpoint({ collections, error }); setupMostRecentlyViewedDashboard(noRecentDashboard ? undefined : dashboard); diff --git a/frontend/src/metabase/containers/ItemPicker/ItemPicker.unit.spec.js b/frontend/src/metabase/containers/ItemPicker/ItemPicker.unit.spec.js index 57c3d0c2f9f..4179c1bc62d 100644 --- a/frontend/src/metabase/containers/ItemPicker/ItemPicker.unit.spec.js +++ b/frontend/src/metabase/containers/ItemPicker/ItemPicker.unit.spec.js @@ -2,7 +2,7 @@ import React from "react"; import userEvent from "@testing-library/user-event"; import { setupCollectionsEndpoints, - setupCollectionItemsEndpoint, + setupDashboardCollectionItemsEndpoint, } from "__support__/server-mocks"; import { renderWithProviders, @@ -93,7 +93,7 @@ async function setup({ extraCollections = [], ...props } = {}) { - setupCollectionItemsEndpoint(Object.values(DASHBOARD)); + setupDashboardCollectionItemsEndpoint(Object.values(DASHBOARD)); setupCollectionsEndpoints(Object.values(COLLECTION).concat(extraCollections)); const onChange = jest.fn(); diff --git a/frontend/src/metabase/dashboard/actions/parameters.js b/frontend/src/metabase/dashboard/actions/parameters.js index 81291769de3..e941c2ecefb 100644 --- a/frontend/src/metabase/dashboard/actions/parameters.js +++ b/frontend/src/metabase/dashboard/actions/parameters.js @@ -369,7 +369,7 @@ export const showAutoApplyFiltersToast = createThunkAction( ); export const CLOSE_AUTO_APPLY_FILTERS_TOAST = - "metabase/dashboard/SHOW_AUTO_APPLY_FILTERS_TOAST"; + "metabase/dashboard/CLOSE_AUTO_APPLY_FILTERS_TOAST"; export const closeAutoApplyFiltersToast = createThunkAction( CLOSE_AUTO_APPLY_FILTERS_TOAST, () => (dispatch, getState) => { diff --git a/frontend/src/metabase/dashboard/containers/DashboardApp.unit.spec.jsx b/frontend/src/metabase/dashboard/containers/DashboardApp.unit.spec.jsx new file mode 100644 index 00000000000..47e2e6e56c7 --- /dev/null +++ b/frontend/src/metabase/dashboard/containers/DashboardApp.unit.spec.jsx @@ -0,0 +1,100 @@ +import React from "react"; +import { Route } from "react-router"; +import userEvent from "@testing-library/user-event"; +import fetchMock from "fetch-mock"; +import { + screen, + renderWithProviders, + waitForElementToBeRemoved, +} from "__support__/ui"; +import DashboardApp from "metabase/dashboard/containers/DashboardApp"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; +import { + createMockCollection, + createMockDashboard, + createMockUser, +} from "metabase-types/api/mocks"; +import { createMockDashboardState } from "metabase-types/store/mocks"; +import { + setupCollectionItemsEndpoint, + setupCollectionsEndpoints, + setupDashboardEndpoints, +} from "__support__/server-mocks"; +import { callMockEvent } from "__support__/events"; + +const TEST_DASHBOARD = createMockDashboard({ + id: 1, + name: "Example", +}); + +const TEST_COLLECTION = createMockCollection({ + id: "root", +}); + +async function setup({ user = createMockUser() }) { + setupDashboardEndpoints(createMockDashboard(TEST_DASHBOARD)); + setupCollectionsEndpoints([]); + setupCollectionItemsEndpoint(TEST_COLLECTION); + + fetchMock.get("path:/api/bookmark", []); + + const mockEventListener = jest.spyOn(window, "addEventListener"); + + const DashboardAppContainer = props => { + return ( + <main> + <link rel="icon" /> + <DashboardApp {...props} /> + </main> + ); + }; + + renderWithProviders( + <Route path="/dashboard/:slug" component={DashboardAppContainer} />, + { + initialRoute: `/dashboard/${TEST_DASHBOARD.id}`, + currentUser: user, + withRouter: true, + storeInitialState: { + dashboard: createMockDashboardState(), + }, + }, + ); + + await waitForElementToBeRemoved(() => + screen.queryAllByTestId("loading-spinner"), + ); + + return { mockEventListener }; +} + +describe("DashboardApp", function () { + afterEach(() => { + jest.clearAllMocks(); + }); + + it("should have a beforeunload event when the user tries to leave a dirty dashboard", async function () { + const { mockEventListener } = await setup({}); + + 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(); + + 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({}); + + userEvent.click(screen.getByLabelText("Edit dashboard")); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); +}); diff --git a/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.jsx b/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.jsx index 7e7d8fa8dfd..afc29be5ebd 100644 --- a/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.jsx +++ b/frontend/src/metabase/dashboard/containers/DashboardApp/DashboardApp.jsx @@ -4,8 +4,8 @@ import { connect } from "react-redux"; import { push } from "react-router-redux"; import _ from "underscore"; import { useUnmount } from "react-use"; - import { t } from "ttag"; +import useBeforeUnload from "metabase/hooks/use-before-unload"; import title from "metabase/hoc/Title"; import favicon from "metabase/hoc/Favicon"; @@ -17,12 +17,12 @@ import { useLoadingTimer } from "metabase/hooks/use-loading-timer"; import { useWebNotification } from "metabase/hooks/use-web-notification"; import { fetchDatabaseMetadata } from "metabase/redux/metadata"; -import { getIsNavbarOpen, closeNavbar, setErrorPage } from "metabase/redux/app"; +import { closeNavbar, getIsNavbarOpen, setErrorPage } from "metabase/redux/app"; import { getMetadata } from "metabase/selectors/metadata"; import { - getUserIsAdmin, canManageSubscriptions, + getUserIsAdmin, } from "metabase/selectors/user"; import { getEmbedOptions } from "metabase/selectors/embed"; @@ -37,28 +37,28 @@ import { addUndo, dismissUndo } from "metabase/redux/undo"; import { useUniqueId } from "metabase/hooks/use-unique-id"; import * as dashboardActions from "../../actions"; import { - getIsEditing, - getIsSharing, + getCardData, + getClickBehaviorSidebarDashcard, getDashboardBeforeEditing, - getIsEditingParameter, - getIsDirty, getDashboardComplete, - getCardData, - getSlowCards, + getDocumentTitle, getEditingParameter, + getFavicon, + getIsAdditionalInfoVisible, + getIsAddParameterPopoverOpen, + getIsDirty, + getIsEditing, + getIsEditingParameter, + getIsHeaderVisible, + getIsLoadingComplete, + getIsRunning, + getIsSharing, + getLoadingStartTime, getParameters, getParameterValues, getDraftParameterValues, - getLoadingStartTime, - getClickBehaviorSidebarDashcard, - getIsAddParameterPopoverOpen, getSidebar, - getFavicon, - getDocumentTitle, - getIsRunning, - getIsLoadingComplete, - getIsHeaderVisible, - getIsAdditionalInfoVisible, + getSlowCards, getIsAutoApplyFilters, getSelectedTabId, } from "../../selectors"; @@ -119,7 +119,14 @@ const mapDispatchToProps = { // NOTE: should use DashboardControls and DashboardData HoCs here? const DashboardApp = props => { - const { isRunning, isLoadingComplete, dashboard, closeDashboard } = props; + const { + isRunning, + isLoadingComplete, + dashboard, + closeDashboard, + isEditing, + isDirty, + } = props; const options = parseHashOptions(window.location.hash); const editingOnLoad = options.edit; @@ -132,6 +139,8 @@ const DashboardApp = props => { useUnmount(props.reset); const slowToastId = useUniqueId(); + useBeforeUnload(isEditing && isDirty); + useEffect(() => { if (isLoadingComplete) { if ( diff --git a/frontend/src/metabase/dashboard/containers/DashboardHeader.jsx b/frontend/src/metabase/dashboard/containers/DashboardHeader.jsx index e77defb44d2..49150cc1ae7 100644 --- a/frontend/src/metabase/dashboard/containers/DashboardHeader.jsx +++ b/frontend/src/metabase/dashboard/containers/DashboardHeader.jsx @@ -1,5 +1,5 @@ /* eslint-disable react/prop-types */ -import React, { Component } from "react"; +import React, { Component, Fragment } from "react"; import { connect } from "react-redux"; import { push } from "react-router-redux"; import PropTypes from "prop-types"; @@ -248,13 +248,13 @@ class DashboardHeader extends Component { : t`Add questions`; buttons.push( - <Tooltip tooltip={addQuestionButtonHint}> + <Tooltip key="add-question-element" tooltip={addQuestionButtonHint}> <DashboardHeaderButton icon="add" isActive={activeSidebarName === SIDEBAR_NAME.addQuestion} onClick={() => toggleSidebar(SIDEBAR_NAME.addQuestion)} + aria-label={t`Add questions`} data-metabase-event="Dashboard;Add Card Sidebar" - aria-label="add questions" /> </Tooltip>, ); @@ -265,6 +265,7 @@ class DashboardHeader extends Component { <a data-metabase-event="Dashboard;Add Text Box" key="add-text" + aria-label={t`Add a text box`} className="text-brand-hover cursor-pointer" onClick={() => this.onAddTextBox()} > @@ -320,7 +321,7 @@ class DashboardHeader extends Component { if (canEdit && hasModelActionsEnabled) { buttons.push( - <> + <Fragment key="add-action-element"> <DashboardHeaderActionDivider /> <Tooltip key="add-action-button" tooltip={t`Add action button`}> <DashboardHeaderButton @@ -331,7 +332,7 @@ class DashboardHeader extends Component { <Icon name="click" size={18} /> </DashboardHeaderButton> </Tooltip> - </>, + </Fragment>, ); } @@ -348,6 +349,7 @@ class DashboardHeader extends Component { <Tooltip key="edit-dashboard" tooltip={t`Edit dashboard`}> <DashboardHeaderButton key="edit" + aria-label={t`Edit dashboard`} data-metabase-event="Dashboard;Edit" icon="pencil" className="text-brand-hover cursor-pointer" diff --git a/frontend/src/metabase/dashboard/containers/selectors.jsx b/frontend/src/metabase/dashboard/containers/selectors.jsx deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/frontend/src/metabase/databases/components/DatabaseForm/DatabaseForm.tsx b/frontend/src/metabase/databases/components/DatabaseForm/DatabaseForm.tsx index 684a3faa10f..c5384184369 100644 --- a/frontend/src/metabase/databases/components/DatabaseForm/DatabaseForm.tsx +++ b/frontend/src/metabase/databases/components/DatabaseForm/DatabaseForm.tsx @@ -1,4 +1,4 @@ -import React, { useCallback, useMemo, useState } from "react"; +import React, { useCallback, useEffect, useMemo, useState } from "react"; import { useFormikContext } from "formik"; import { t } from "ttag"; import Button from "metabase/core/components/Button"; @@ -30,6 +30,7 @@ export interface DatabaseFormProps { onSubmit?: (values: DatabaseData) => void; onEngineChange?: (engineKey: string | undefined) => void; onCancel?: () => void; + setIsDirty?: (isDirty: boolean) => void; } const DatabaseForm = ({ @@ -41,6 +42,7 @@ const DatabaseForm = ({ onSubmit, onCancel, onEngineChange, + setIsDirty, }: DatabaseFormProps): JSX.Element => { const initialEngineKey = getEngineKey(engines, initialData, isAdvanced); const [engineKey, setEngineKey] = useState(initialEngineKey); @@ -88,6 +90,7 @@ const DatabaseForm = ({ isCachingEnabled={isCachingEnabled} onEngineChange={handleEngineChange} onCancel={onCancel} + setIsDirty={setIsDirty} /> </FormProvider> ); @@ -102,6 +105,7 @@ interface DatabaseFormBodyProps { isCachingEnabled: boolean; onEngineChange: (engineKey: string | undefined) => void; onCancel?: () => void; + setIsDirty?: (isDirty: boolean) => void; } const DatabaseFormBody = ({ @@ -113,8 +117,13 @@ const DatabaseFormBody = ({ isCachingEnabled, onEngineChange, onCancel, + setIsDirty, }: DatabaseFormBodyProps): JSX.Element => { - const { values } = useFormikContext<DatabaseData>(); + const { values, dirty } = useFormikContext<DatabaseData>(); + + useEffect(() => { + setIsDirty?.(dirty); + }, [dirty, setIsDirty]); const fields = useMemo(() => { return engine ? getVisibleFields(engine, values, isAdvanced) : []; diff --git a/frontend/src/metabase/hooks/use-before-unload/index.ts b/frontend/src/metabase/hooks/use-before-unload/index.ts new file mode 100644 index 00000000000..d9a9641c0cf --- /dev/null +++ b/frontend/src/metabase/hooks/use-before-unload/index.ts @@ -0,0 +1,2 @@ +// eslint-disable-next-line import/no-default-export -- deprecated usage +export { default, BEFORE_UNLOAD_UNSAVED_MESSAGE } from "./use-before-unload"; diff --git a/frontend/src/metabase/hooks/use-before-unload/use-before-unload.ts b/frontend/src/metabase/hooks/use-before-unload/use-before-unload.ts new file mode 100644 index 00000000000..4f0c0e74aa2 --- /dev/null +++ b/frontend/src/metabase/hooks/use-before-unload/use-before-unload.ts @@ -0,0 +1,15 @@ +import { useBeforeUnload as useBeforeUnloadHook } from "react-use"; +import { t } from "ttag"; + +// most browsers don't use a custom message with beforeunload anymore, just putting here to retain compatibility +// https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event#compatibility_notes +export const BEFORE_UNLOAD_UNSAVED_MESSAGE = t`You have unsaved changes.`; + +const useBeforeUnload = ( + condition: Parameters<typeof useBeforeUnloadHook>[0], +) => { + return useBeforeUnloadHook(condition, BEFORE_UNLOAD_UNSAVED_MESSAGE); +}; + +// eslint-disable-next-line import/no-default-export -- deprecated usage +export default useBeforeUnload; diff --git a/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx b/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx index 282eecf2326..7b136984a1a 100644 --- a/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx +++ b/frontend/src/metabase/query_builder/components/__mocks__/NativeQueryEditor.jsx @@ -3,15 +3,24 @@ import React from "react"; import SyncedParametersList from "metabase/parameters/components/SyncedParametersList/SyncedParametersList"; -const MockNativeQueryEditor = ({ query, setParameterValue }) => ( - <div data-testid="mock-native-query-editor"> - <SyncedParametersList - className="mt1" - parameters={query.question().parameters()} - setParameterValue={setParameterValue} - commitImmediately - /> - </div> -); +const MockNativeQueryEditor = ({ query, setParameterValue, ...props }) => { + const onChange = evt => { + props.setDatasetQuery(query.setQueryText(evt.target.value)); + }; + + return ( + <div data-testid="mock-native-query-editor"> + {query.queryText && ( + <textarea value={query.queryText()} onChange={onChange} /> + )} + <SyncedParametersList + className="mt1" + parameters={query.question().parameters()} + setParameterValue={setParameterValue} + commitImmediately + /> + </div> + ); +}; export default MockNativeQueryEditor; diff --git a/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx b/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx index 20a2d817986..e3d71e17710 100644 --- a/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx +++ b/frontend/src/metabase/query_builder/containers/QueryBuilder.jsx @@ -35,6 +35,7 @@ import title from "metabase/hoc/Title"; import titleWithLoadingTime from "metabase/hoc/TitleWithLoadingTime"; import favicon from "metabase/hoc/Favicon"; +import useBeforeUnload from "metabase/hooks/use-before-unload"; import View from "../components/view/View"; import { @@ -90,6 +91,7 @@ import { getIsAdditionalInfoVisible, getAutocompleteResultsFn, getCardAutocompleteResultsFn, + isResultsMetadataDirty, } from "../selectors"; import * as actions from "../actions"; import { VISUALIZATION_SLOW_TIMEOUT } from "../constants"; @@ -160,6 +162,7 @@ const mapStateToProps = (state, props) => { isRunnable: getIsRunnable(state), isResultDirty: getIsResultDirty(state), + isMetadataDirty: isResultsMetadataDirty(state), questionAlerts: getQuestionAlerts(state), visualizationSettings: getVisualizationSettings(state), @@ -218,7 +221,10 @@ function QueryBuilder(props) { showTimelinesForCollection, card, isLoadingComplete, + isDirty: isModelQueryDirty, + isMetadataDirty, closeQB, + isNew, } = props; const forceUpdate = useForceUpdate(); @@ -300,6 +306,18 @@ function QueryBuilder(props) { return () => window.removeEventListener("resize", forceUpdateDebounced); }, []); + const isExistingModelDirty = useMemo( + () => isModelQueryDirty || isMetadataDirty, + [isMetadataDirty, isModelQueryDirty], + ); + + const isExistingSqlQueryDirty = useMemo( + () => isModelQueryDirty && isNativeEditorOpen, + [isModelQueryDirty, isNativeEditorOpen], + ); + + useBeforeUnload(!isNew && (isExistingModelDirty || isExistingSqlQueryDirty)); + useUnmount(() => { cancelQuery(); closeModal(); diff --git a/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx b/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx index 37f0ee6cc2c..4767c808074 100644 --- a/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx +++ b/frontend/src/metabase/query_builder/containers/QueryBuilder.unit.spec.tsx @@ -1,9 +1,17 @@ import React, { ComponentPropsWithoutRef } from "react"; import { IndexRoute, Route } from "react-router"; -import { Card } from "metabase-types/api"; -import { createMockCard, createMockDataset } from "metabase-types/api/mocks"; +import userEvent from "@testing-library/user-event"; +import { Card, Dataset } from "metabase-types/api"; +import { + createMockCard, + createMockColumn, + createMockDataset, + createMockNativeDatasetQuery, +} from "metabase-types/api/mocks"; + import { createSampleDatabase, + ORDERS, ORDERS_ID, SAMPLE_DB_ID, } from "metabase-types/api/mocks/presets"; @@ -19,8 +27,12 @@ import { import { renderWithProviders, screen, + waitFor, waitForElementToBeRemoved, + within, } from "__support__/ui"; +import { callMockEvent } from "__support__/events"; +import { BEFORE_UNLOAD_UNSAVED_MESSAGE } from "metabase/hooks/use-before-unload"; import QueryBuilder from "./QueryBuilder"; const TEST_DB = createSampleDatabase(); @@ -35,7 +47,45 @@ const TEST_CARD = createMockCard({ }, }); -const TEST_DATASET = createMockDataset(); +const TEST_MODEL_CARD = createMockCard({ + dataset_query: { + database: SAMPLE_DB_ID, + type: "query", + query: { + "source-table": ORDERS_ID, + limit: 1, + }, + }, + dataset: true, + display: "scalar", + description: "Test description", +}); + +const TEST_NATIVE_CARD = createMockCard({ + dataset_query: createMockNativeDatasetQuery({ + database: SAMPLE_DB_ID, + }), +}); + +const TEST_MODEL_DATASET_COLUMN = createMockColumn({ + name: "ID", + source: "fields", + display_name: "ID", + description: "test", + field_ref: ["field", ORDERS.ID, null], +}); +const TEST_MODEL_DATASET = createMockDataset({ + data: { + rows: [["1"]], + cols: [TEST_MODEL_DATASET_COLUMN], + }, + database_id: SAMPLE_DB_ID, + + status: "completed", + context: "question", + row_count: 1, + running_time: 35, +}); const TestQueryBuilder = ( props: ComponentPropsWithoutRef<typeof QueryBuilder>, @@ -50,27 +100,37 @@ const TestQueryBuilder = ( interface SetupOpts { card?: Card; + dataset?: Dataset; initialRoute?: string; } const setup = async ({ card = TEST_CARD, + dataset = createMockDataset(), initialRoute = `/question/${card.id}`, }: SetupOpts = {}) => { setupDatabasesEndpoints([TEST_DB]); setupCardEndpoints(card); - setupCardQueryEndpoints(card, TEST_DATASET); + setupCardQueryEndpoints(card, dataset); setupSearchEndpoints([]); setupAlertsEndpoints(card, []); setupBookmarksEndpoints([]); setupTimelinesEndpoints([]); + const mockEventListener = jest.spyOn(window, "addEventListener"); + renderWithProviders( - <Route path="/question"> + <Route> <IndexRoute component={TestQueryBuilder} /> - <Route path="notebook" component={TestQueryBuilder} /> - <Route path=":slug" component={TestQueryBuilder} /> - <Route path=":slug/notebook" component={TestQueryBuilder} /> + <Route path="/model"> + <Route path=":slug/query" component={TestQueryBuilder} /> + <Route path=":slug/metadata" component={TestQueryBuilder} /> + </Route> + <Route path="/question"> + <Route path="notebook" component={TestQueryBuilder} /> + <Route path=":slug" component={TestQueryBuilder} /> + <Route path=":slug/notebook" component={TestQueryBuilder} /> + </Route> </Route>, { withRouter: true, @@ -78,21 +138,149 @@ const setup = async ({ }, ); - await waitForElementToBeRemoved(() => screen.queryByText(/Loading/)); + await waitForElementToBeRemoved(() => + screen.queryByTestId("loading-spinner"), + ); + + return { mockEventListener }; }; describe("QueryBuilder", () => { - it("renders a structured question in the simple mode", async () => { - await setup(); + describe("renders structured queries", () => { + it("renders a structured question in the simple mode", async () => { + await setup(); + + expect(screen.getByDisplayValue(TEST_CARD.name)).toBeInTheDocument(); + }); + + it("renders a structured question in the notebook mode", async () => { + await setup({ + initialRoute: `/question/${TEST_CARD.id}/notebook`, + }); + + expect(screen.getByDisplayValue(TEST_CARD.name)).toBeInTheDocument(); + }); + }); - expect(screen.getByDisplayValue(TEST_CARD.name)).toBeInTheDocument(); + describe("editing models", () => { + describe("editing queries", () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it("should trigger beforeunload event when leaving edited query", async () => { + const { mockEventListener } = await setup({ + card: TEST_MODEL_CARD, + initialRoute: `/model/${TEST_MODEL_CARD.id}/query`, + }); + + const rowLimitInput = await within( + screen.getByTestId("step-limit-0-0"), + ).findByPlaceholderText("Enter a limit"); + + userEvent.click(rowLimitInput); + userEvent.type(rowLimitInput, "0"); + + await waitFor(() => { + expect(rowLimitInput).toHaveValue(10); + }); + + userEvent.tab(); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not trigger beforeunload event when leaving unedited query", async () => { + const { mockEventListener } = await setup({ + card: TEST_MODEL_CARD, + initialRoute: `/model/${TEST_MODEL_CARD.id}/query`, + }); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); + }); + + describe("editing metadata", () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it("should trigger beforeunload event when leaving edited metadata", async () => { + const { mockEventListener } = await setup({ + card: TEST_MODEL_CARD, + dataset: TEST_MODEL_DATASET, + initialRoute: `/model/${TEST_MODEL_CARD.id}/metadata`, + }); + + const columnDisplayName = await screen.findByTitle("Display name"); + + userEvent.click(columnDisplayName); + userEvent.type(columnDisplayName, "X"); + + await waitFor(() => { + expect(columnDisplayName).toHaveValue( + `${TEST_MODEL_DATASET_COLUMN.display_name}X`, + ); + }); + + userEvent.tab(); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); + + it("should not trigger beforeunload event when model metadata is unedited", async () => { + const { mockEventListener } = await setup({ + card: TEST_MODEL_CARD, + dataset: TEST_MODEL_DATASET, + initialRoute: `/model/${TEST_MODEL_CARD.id}/metadata`, + }); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).toBe(undefined); + }); + }); }); - it("renders a structured question in the notebook mode", async () => { - await setup({ - initialRoute: `/question/${TEST_CARD.id}/notebook`, + describe("beforeunload events in native queries", () => { + afterEach(() => { + jest.restoreAllMocks(); }); - expect(screen.getByDisplayValue(TEST_CARD.name)).toBeInTheDocument(); + it("should not trigger beforeunload event when user tries to leave an ad-hoc native query", async () => { + const { mockEventListener } = await setup({ + card: TEST_NATIVE_CARD, + initialRoute: `/question/${TEST_NATIVE_CARD.id}`, + }); + + const inputArea = within( + screen.getByTestId("mock-native-query-editor"), + ).getByRole("textbox"); + + userEvent.click(inputArea); + userEvent.type(inputArea, "0"); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).not.toEqual(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); + + it("should not trigger beforeunload event when query is unedited", async () => { + const { mockEventListener } = await setup({ + card: TEST_NATIVE_CARD, + initialRoute: `/question/${TEST_NATIVE_CARD.id}`, + }); + + const mockEvent = callMockEvent(mockEventListener, "beforeunload"); + expect(mockEvent.preventDefault).not.toHaveBeenCalled(); + expect(mockEvent.returnValue).not.toEqual(BEFORE_UNLOAD_UNSAVED_MESSAGE); + }); }); }); diff --git a/frontend/src/metabase/visualizations/components/ChartCaption.unit.spec.tsx b/frontend/src/metabase/visualizations/components/ChartCaption.unit.spec.tsx index 119522f278c..ac6a927567b 100644 --- a/frontend/src/metabase/visualizations/components/ChartCaption.unit.spec.tsx +++ b/frontend/src/metabase/visualizations/components/ChartCaption.unit.spec.tsx @@ -4,7 +4,11 @@ import userEvent from "@testing-library/user-event"; import { render, screen, getIcon } from "__support__/ui"; import { Card, Series } from "metabase-types/api"; -import { createMockCard, createMockColumn } from "metabase-types/api/mocks"; +import { + createMockCard, + createMockColumn, + createMockDatasetData, +} from "metabase-types/api/mocks"; import ChartCaption from "./ChartCaption"; type Props = ComponentPropsWithoutRef<typeof ChartCaption>; @@ -25,7 +29,11 @@ const getSeries = ({ card }: { card?: Card } = {}): Series => { const series: Series = [ { card: card ?? createMockCard({ name: "" }), - data: { rows: [["foo", 1]], cols, rows_truncated: 0 }, + data: createMockDatasetData({ + cols, + rows: [["foo", 1]], + rows_truncated: 0, + }), }, ]; diff --git a/frontend/test/__support__/server-mocks/collection.ts b/frontend/test/__support__/server-mocks/collection.ts index 831ff129cb0..319fdc7d23c 100644 --- a/frontend/test/__support__/server-mocks/collection.ts +++ b/frontend/test/__support__/server-mocks/collection.ts @@ -1,7 +1,12 @@ import fetchMock from "fetch-mock"; import _ from "underscore"; import { ROOT_COLLECTION } from "metabase/entities/collections"; -import { Card, Collection, Dashboard } from "metabase-types/api"; +import { + Card, + Collection, + CollectionItem, + Dashboard, +} from "metabase-types/api"; import { convertSavedQuestionToVirtualTable, getCollectionVirtualSchemaName, @@ -60,6 +65,30 @@ export function setupCollectionVirtualSchemaEndpoints( fetchMock.get(urls.models, modelVirtualTables); } +export function setupCollectionItemsEndpoint( + collection: Collection, + collectionItems: CollectionItem[] = [], +) { + fetchMock.get(`path:/api/collection/${collection.id}/items`, uri => { + const url = new URL(uri); + const models = url.searchParams.getAll("models"); + const matchedItems = collectionItems.filter(({ model }) => + models.includes(model), + ); + + const limit = Number(url.searchParams.get("limit")) || matchedItems.length; + const offset = Number(url.searchParams.get("offset")) || 0; + + return { + data: matchedItems.slice(offset, offset + limit), + total: matchedItems.length, + models, + limit, + offset, + }; + }); +} + export function setupUnauthorizedCollectionEndpoints(collection: Collection) { fetchMock.get(`path:/api/collection/${collection.id}`, { status: 403, @@ -110,7 +139,7 @@ function setupCollectionWithErrorById({ }); } -export function setupCollectionItemsEndpoint(dashboards: Dashboard[]) { +export function setupDashboardCollectionItemsEndpoint(dashboards: Dashboard[]) { fetchMock.get(/api\/collection\/(\d+|root)\/items/, url => { const collectionIdParam = url.split("/")[5]; const collectionId = diff --git a/frontend/test/__support__/server-mocks/group.ts b/frontend/test/__support__/server-mocks/group.ts new file mode 100644 index 00000000000..f2bd0125089 --- /dev/null +++ b/frontend/test/__support__/server-mocks/group.ts @@ -0,0 +1,6 @@ +import fetchMock from "fetch-mock"; +import { Group } from "metabase-types/api"; + +export const setupGroupsEndpoint = (groups: Omit<Group, "members">[]) => { + fetchMock.get("path:/api/permissions/group", groups); +}; diff --git a/frontend/test/__support__/server-mocks/permissions.ts b/frontend/test/__support__/server-mocks/permissions.ts new file mode 100644 index 00000000000..7f9cf2161b6 --- /dev/null +++ b/frontend/test/__support__/server-mocks/permissions.ts @@ -0,0 +1,8 @@ +import fetchMock from "fetch-mock"; +import { PermissionsGraph } from "metabase-types/api"; + +export const setupPermissionsGraphEndpoint = ( + permissionsGraph: PermissionsGraph, +) => { + fetchMock.get("path:/api/permissions/graph", permissionsGraph); +}; -- GitLab