diff --git a/e2e/test/scenarios/actions/actions-in-object-detail-view.cy.spec.js b/e2e/test/scenarios/actions/actions-in-object-detail-view.cy.spec.js new file mode 100644 index 0000000000000000000000000000000000000000..4bd654a14aadf034210ed28d2a2d2383a2431da4 --- /dev/null +++ b/e2e/test/scenarios/actions/actions-in-object-detail-view.cy.spec.js @@ -0,0 +1,387 @@ +import moment from "moment-timezone"; + +import { WRITABLE_DB_ID } from "e2e/support/cypress_data"; +import { + modal, + popover, + resetTestTable, + restore, + resyncDatabase, + undoToast, + visitDashboard, + visitModel, +} from "e2e/support/helpers"; + +const PG_DB_ID = 2; +const PG_SCOREBOARD_TABLE_ID = 9; +const WRITABLE_TEST_TABLE = "scoreboard_actions"; +const FIRST_SCORE_ROW_ID = 11; +const SECOND_SCORE_ROW_ID = 12; +const UPDATED_SCORE = 987654321; +const UPDATED_SCORE_FORMATTED = "987,654,321"; + +const SCORES_MODEL = { + name: "Scores model", + dataset: true, + display: "table", + database: PG_DB_ID, + query: { + "source-table": PG_SCOREBOARD_TABLE_ID, + }, +}; + +const DASHBOARD = { + name: "Test dashboard", + database: PG_DB_ID, +}; + +describe("scenarios > actions > actions-in-object-detail-view", () => { + beforeEach(() => { + cy.intercept("POST", "/api/action").as("createBasicActions"); + cy.intercept("GET", "/api/action?model-id=*").as("getModelActions"); + cy.intercept("GET", "/api/action/*/execute?parameters=*").as( + "prefetchValues", + ); + + resetTestTable({ type: "postgres", table: WRITABLE_TEST_TABLE }); + restore("postgres-writable"); + }); + + describe("in dashboard", () => { + beforeEach(() => { + asAdmin(() => { + resyncDatabase({ + dbId: WRITABLE_DB_ID, + tableName: WRITABLE_TEST_TABLE, + }); + + cy.createQuestion(SCORES_MODEL, { wrapId: true, idAlias: "modelId" }); + + cy.get("@modelId").then(modelId => { + createBasicModelActions(modelId); + + cy.createQuestionAndDashboard({ + questionDetails: { + name: "Score detail", + display: "object", + database: PG_DB_ID, + query: { + "source-table": `card__${modelId}`, + }, + }, + dashboardDetails: DASHBOARD, + }).then(({ body: { card_id, dashboard_id } }) => { + cy.wrap(card_id).as("modelId"); + cy.wrap(dashboard_id).as("dashboardId"); + }); + }); + }); + }); + + it("does not show model actions in model visualization on a dashboard", () => { + asAdmin(() => { + cy.get("@dashboardId").then(dashboardId => { + visitDashboard(dashboardId); + }); + + cy.findByTestId("dashcard").within(() => { + assertActionsDropdownNotExists(); + }); + }); + }); + }); + + describe("in modal", () => { + beforeEach(() => { + asAdmin(() => { + resyncDatabase({ + dbId: WRITABLE_DB_ID, + tableName: WRITABLE_TEST_TABLE, + }); + cy.createQuestion(SCORES_MODEL, { wrapId: true, idAlias: "modelId" }); + }); + }); + + it("should be able to run update and delete actions when enabled", () => { + cy.get("@modelId").then(modelId => { + asNormalUser(() => { + cy.log("As normal user: verify database actions are enabled"); + visitModelDetail(modelId); + assertActionsTabExists(); + + cy.log("As normal user: verify there are no model actions to run"); + visitObjectDetail(modelId, FIRST_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownNotExists(); + }); + }); + + asAdmin(() => { + cy.log("As admin: verify database actions are enabled"); + visitModelDetail(modelId); + assertActionsTabExists(); + + cy.log("As admin: Verify that there are no model actions to run"); + visitObjectDetail(modelId, FIRST_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownNotExists(); + }); + + cy.log("As admin: create basic model actions"); + createBasicModelActions(modelId); + + cy.log("As admin: verify there are model actions to run"); + visitObjectDetail(modelId, FIRST_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownExists(); + }); + }); + + asNormalUser(() => { + cy.log("As normal user: verify there are model actions to run (1)"); + visitObjectDetail(modelId, FIRST_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownExists(); + }); + + cy.log("As normal user: verify update form gets prefilled"); + openUpdateObjectModal(); + actionExecuteModal().within(() => { + cy.wait("@prefetchValues").then(request => { + const firstScoreRow = request.response.body; + + actionForm().within(() => { + assertScoreFormPrefilled(firstScoreRow); + }); + }); + + cy.icon("close").click(); + }); + objectDetailModal().icon("close").click(); + + cy.log("As normal user: verify there are model actions to run (2)"); + visitObjectDetail(modelId, SECOND_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownExists(); + }); + + cy.log( + "As normal user: verify form gets prefilled with values for another entity and run update action", + ); + openUpdateObjectModal(); + actionExecuteModal().within(() => { + cy.wait("@prefetchValues").then(request => { + const secondScoreRow = request.response.body; + + actionForm().within(() => { + assertScoreFormPrefilled(secondScoreRow); + + cy.findByLabelText("Score").clear().type(UPDATED_SCORE); + cy.findByText("Update").click(); + }); + }); + }); + objectDetailModal().icon("close").click(); + assertSuccessfullUpdateToast(); + assertUpdatedScoreInTable(); + + cy.log("As normal user: run delete action"); + visitObjectDetail(modelId, SECOND_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownExists(); + }); + openDeleteObjectModal(); + deleteObjectModal().findByText("Delete forever").click(); + assertSuccessfullDeleteToast(); + assertUpdatedScoreNotInTable(); + }); + + asAdmin(() => { + cy.log("As admin: verify database actions are enabled"); + visitModelDetail(modelId); + assertActionsTabExists(); + + cy.log("As admin: disable basic model actions"); + disableBasicModelActions(modelId); + + cy.log("As admin user: verify there are no model actions to run"); + visitObjectDetail(modelId, FIRST_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownNotExists(); + }); + + cy.log("As admin: disable database actions"); + disableDatabaseActions(WRITABLE_DB_ID); + + cy.log("As admin: verify database actions are disabled"); + visitModelDetail(modelId); + assertActionsTabNotExists(); + }); + + asNormalUser(() => { + cy.log("As normal user: verify database actions are disabled"); + visitModelDetail(modelId); + assertActionsTabNotExists(); + + cy.log("As normal user: verify there are no model actions to run"); + visitObjectDetail(modelId, FIRST_SCORE_ROW_ID); + objectDetailModal().within(() => { + assertActionsDropdownNotExists(); + }); + }); + }); + }); + }); +}); + +function asAdmin(callback) { + cy.signInAsAdmin(); + callback(); + cy.signOut(); +} + +function asNormalUser(callback) { + cy.signInAsNormalUser(); + callback(); + cy.signOut(); +} + +function disableDatabaseActions(databaseId) { + cy.visit(`/admin/databases/${databaseId}`); + const actionsToggle = cy.findByLabelText("Model actions"); + + cy.log("actions should be enabled in model page"); + actionsToggle.should("be.checked"); + + actionsToggle.click(); + + cy.log("actions should be disabled in model page"); + actionsToggle.should("not.be.checked"); +} + +function createBasicModelActions(modelId) { + visitModelDetailActions(modelId); + cy.findByText("Create basic actions").click(); + cy.wait("@createBasicActions"); +} + +function disableBasicModelActions(modelId) { + visitModelDetailActions(modelId); + cy.findByLabelText("Actions menu").click(); + popover().findByText("Disable basic actions").click(); + modal().findByText("Disable").click(); + cy.wait("@getModelActions"); +} + +function visitObjectDetail(modelId, objectId) { + visitModel(modelId); + cy.findAllByText(objectId).first().click(); +} + +function visitModelDetail(modelId) { + visitModel(modelId); + cy.icon("info").click(); + cy.findByTestId("sidebar-right").findByText("Model details").click(); +} + +function visitModelDetailActions(modelId) { + visitModelDetail(modelId); + cy.findByText("Actions").click(); +} + +function openUpdateObjectModal() { + cy.findByTestId("actions-menu").click(); + popover().findByText("Update").click(); +} + +function openDeleteObjectModal() { + cy.findByTestId("actions-menu").click(); + popover().findByText("Delete").click(); +} + +function assertActionsDropdownExists() { + cy.log("actions dropdown should be shown in object detail view"); + cy.findByTestId("actions-menu").should("exist"); +} + +function assertActionsDropdownNotExists() { + cy.log("actions dropdown should not be shown in object detail view"); + cy.findByTestId("actions-menu").should("not.exist"); +} + +function assertActionsTabExists() { + cy.log("actions tab should be shown in model detail page"); + cy.findByText("Actions").should("exist"); +} + +function assertActionsTabNotExists() { + cy.log("actions tab should not be shown in model detail page"); + cy.findByText("Actions").should("not.exist"); +} + +function assertScoreFormPrefilled(object) { + assertInputValue("ID", object.id); + assertInputValue("Team Name", object.team_name); + assertInputValue("Score", object.score); + assertInputValue("Status", object.status); + assertDateInputValue("Created At", object.created_at); + assertDateInputValue("Updated At", object.updated_at); +} + +function assertInputValue(labelText, value) { + const expectedValue = value || ""; + + cy.log(`input for "${labelText}" should have value "${expectedValue}"`); + cy.findByLabelText(labelText).should("have.value", expectedValue); +} + +function assertDateInputValue(labelText, value) { + const expectedValue = moment(value) + .format() + .replace(/-\d\d:\d\d$/, ""); + + cy.log(`input for "${labelText}" should have value "${expectedValue}"`); + cy.findByLabelText(labelText).should("have.value", expectedValue); +} + +function assertUpdatedScoreInTable() { + cy.log("updated quantity should be present in the table"); + cy.findByTestId("TableInteractive-root") + .findByText(UPDATED_SCORE_FORMATTED) + .should("exist"); +} + +function assertUpdatedScoreNotInTable() { + cy.log("updated quantity should not be present in the table"); + cy.findByTestId("TableInteractive-root") + .findByText(UPDATED_SCORE_FORMATTED) + .should("not.exist"); +} + +function assertSuccessfullUpdateToast() { + cy.log("it shows a toast informing the update was successful"); + undoToast().should("have.attr", "color", "success"); + undoToast().findByText("Successfully updated").should("be.visible"); +} + +function assertSuccessfullDeleteToast() { + cy.log("it shows a toast informing the delete was successful"); + undoToast().should("have.attr", "color", "success"); + undoToast().findByText("Successfully deleted").should("be.visible"); +} + +function actionForm() { + return cy.findByTestId("action-form"); +} + +function objectDetailModal() { + return cy.findByTestId("object-detail"); +} + +function actionExecuteModal() { + return cy.findByTestId("action-execute-modal"); +} + +function deleteObjectModal() { + return cy.findByTestId("delete-object-modal"); +} diff --git a/frontend/src/metabase-types/api/actions.ts b/frontend/src/metabase-types/api/actions.ts index 6874cd74971a37afeacbab7c61a2f07b3217ca6f..aeeaab9525aa0dbcdde4b9b3c80da7828348e831 100644 --- a/frontend/src/metabase-types/api/actions.ts +++ b/frontend/src/metabase-types/api/actions.ts @@ -185,3 +185,7 @@ export interface ActionDashboardCard actionDisplayType?: ActionDisplayType; }; } + +export interface WritebackActionListQuery { + "model-id"?: CardId; +} diff --git a/frontend/src/metabase/actions/components/ActionForm/ActionForm.tsx b/frontend/src/metabase/actions/components/ActionForm/ActionForm.tsx index c917d0d44a7ebea4966351c3f7a4828b62fd5138..2a6929f2ce53cfc7f289dae5220e96322a9ac480 100644 --- a/frontend/src/metabase/actions/components/ActionForm/ActionForm.tsx +++ b/frontend/src/metabase/actions/components/ActionForm/ActionForm.tsx @@ -28,6 +28,7 @@ import { ActionFormButtonContainer } from "./ActionForm.styled"; interface ActionFormProps { action: WritebackAction; initialValues?: ActionFormInitialValues; + prefetchesInitialValues?: boolean; // Parameters that shouldn't be displayed in the form // Can be used to "lock" certain parameter values. @@ -46,6 +47,7 @@ interface ActionFormProps { function ActionForm({ action, initialValues: rawInitialValues = {}, + prefetchesInitialValues, hiddenFields = [], onSubmit, onClose, @@ -54,6 +56,7 @@ function ActionForm({ useActionForm({ action, initialValues: rawInitialValues, + prefetchesInitialValues, }); const editableFields = useMemo( diff --git a/frontend/src/metabase/actions/components/ActionViz/Action.tsx b/frontend/src/metabase/actions/components/ActionViz/Action.tsx index 5f5ce47d717afd7a48c43b4d08e621d6e8cb0335..64d1771c97138db6b0af0d37c8f91b89bd60efb5 100644 --- a/frontend/src/metabase/actions/components/ActionViz/Action.tsx +++ b/frontend/src/metabase/actions/components/ActionViz/Action.tsx @@ -2,7 +2,10 @@ import { useCallback, useMemo } from "react"; import { t } from "ttag"; import { connect } from "react-redux"; import Tooltip from "metabase/core/components/Tooltip"; -import { executeRowAction } from "metabase/dashboard/actions"; +import { + executeRowAction, + reloadDashboardCards, +} from "metabase/dashboard/actions"; import { getEditingDashcardId } from "metabase/dashboard/selectors"; import type { VisualizationProps } from "metabase/visualizations/types"; import type { @@ -94,14 +97,21 @@ const ActionComponent = ({ const canWrite = model?.canWriteActions(); const onSubmit = useCallback( - (parameters: ParametersForActionExecution) => - executeRowAction({ + async (parameters: ParametersForActionExecution) => { + const result = await executeRowAction({ dashboard, dashcard, parameters, dispatch, shouldToast: shouldDisplayButton, - }), + }); + + if (result.success) { + dispatch(reloadDashboardCards()); + } + + return result; + }, [dashboard, dashcard, dispatch, shouldDisplayButton], ); diff --git a/frontend/src/metabase/actions/components/ActionViz/ActionVizForm.tsx b/frontend/src/metabase/actions/components/ActionViz/ActionVizForm.tsx index 0ac13aa53dd01acf87a0496a05c5c3883485d453..c1f15af2cd9530ef835b3372b20a6a30f40ba404 100644 --- a/frontend/src/metabase/actions/components/ActionViz/ActionVizForm.tsx +++ b/frontend/src/metabase/actions/components/ActionViz/ActionVizForm.tsx @@ -1,6 +1,6 @@ -import { useState } from "react"; +import { useCallback, useState } from "react"; -import { getFormTitle } from "metabase/actions/utils"; +import { getFormTitle, isImplicitUpdateAction } from "metabase/actions/utils"; import type { ActionDashboardCard, @@ -16,12 +16,14 @@ import ActionCreator from "metabase/actions/containers/ActionCreator/ActionCreat import Modal from "metabase/components/Modal"; import ActionParametersInputForm, { ActionParametersInputModal, -} from "../../containers/ActionParametersInputForm"; +} from "metabase/actions/containers/ActionParametersInputForm"; +import { getDashboardType } from "metabase/dashboard/utils"; +import { ActionsApi, PublicApi } from "metabase/services"; + import ActionButtonView from "./ActionButtonView"; +import { FormTitle, FormWrapper } from "./ActionForm.styled"; import { shouldShowConfirmation } from "./utils"; -import { FormWrapper, FormTitle } from "./ActionForm.styled"; - interface ActionFormProps { action: WritebackAction; dashcard: ActionDashboardCard; @@ -83,6 +85,27 @@ function ActionVizForm({ setShowEditModal(false); }; + const fetchInitialValues = useCallback(async () => { + const prefetchDashcardValues = + getDashboardType(dashboard.id) === "public" + ? PublicApi.prefetchDashcardValues + : ActionsApi.prefetchDashcardValues; + + const canPrefetch = Object.keys(dashcardParamValues).length > 0; + + if (!canPrefetch) { + return {}; + } + + return prefetchDashcardValues({ + dashboardId: dashboard.id, + dashcardId: dashcard.id, + parameters: JSON.stringify(dashcardParamValues), + }); + }, [dashboard.id, dashcard.id, dashcardParamValues]); + + const shouldPrefetch = isImplicitUpdateAction(action); + if (shouldDisplayButton) { return ( <> @@ -95,10 +118,10 @@ function ActionVizForm({ {showFormModal && ( <ActionParametersInputModal action={action} - dashboard={dashboard} - dashcard={dashcard} mappedParameters={mappedParameters} - dashcardParamValues={dashcardParamValues} + initialValues={dashcardParamValues} + fetchInitialValues={fetchInitialValues} + shouldPrefetch={shouldPrefetch} title={title} showConfirmMessage={showConfirmMessage} confirmMessage={action.visualization_settings?.confirmMessage} @@ -134,10 +157,10 @@ function ActionVizForm({ <FormTitle>{title}</FormTitle> <ActionParametersInputForm action={action} - dashboard={dashboard} - dashcard={dashcard} mappedParameters={mappedParameters} - dashcardParamValues={dashcardParamValues} + initialValues={dashcardParamValues} + fetchInitialValues={fetchInitialValues} + shouldPrefetch={shouldPrefetch} onSubmit={onSubmit} /> </FormWrapper> diff --git a/frontend/src/metabase/actions/components/ActionViz/utils.ts b/frontend/src/metabase/actions/components/ActionViz/utils.ts index 2ea81e31b62103a75f534e7a960a1f9dd0b6c402..6aca4124fa246ef3469e4e3b9024f862d34e6c84 100644 --- a/frontend/src/metabase/actions/components/ActionViz/utils.ts +++ b/frontend/src/metabase/actions/components/ActionViz/utils.ts @@ -13,6 +13,7 @@ import type { WritebackAction, WritebackParameter, } from "metabase-types/api"; +import { isImplicitDeleteAction } from "metabase/actions/utils"; type ActionParameterTuple = [ParameterId, ActionParameterValue]; @@ -100,9 +101,7 @@ export const shouldShowConfirmation = (action?: WritebackAction) => { } const hasConfirmationMessage = !!action.visualization_settings?.confirmMessage; - const isImplicitDelete = - action.type === "implicit" && action.kind === "row/delete"; - return hasConfirmationMessage || isImplicitDelete; + return hasConfirmationMessage || isImplicitDeleteAction(action); }; export const isParameterHidden = ( diff --git a/frontend/src/metabase/actions/containers/ActionExecuteModal/ActionExecuteModal.tsx b/frontend/src/metabase/actions/containers/ActionExecuteModal/ActionExecuteModal.tsx index 2072976bfcaef439622adef3dba3c018d0fa78a6..d6fa84f850b2436ecb13b57ce3672aa4500f0175 100644 --- a/frontend/src/metabase/actions/containers/ActionExecuteModal/ActionExecuteModal.tsx +++ b/frontend/src/metabase/actions/containers/ActionExecuteModal/ActionExecuteModal.tsx @@ -1,4 +1,3 @@ -import { useCallback } from "react"; import { connect } from "react-redux"; import _ from "underscore"; import Actions from "metabase/entities/actions"; @@ -15,8 +14,12 @@ import ActionParametersInputForm from "../ActionParametersInputForm"; interface OwnProps { actionId: WritebackActionId; + initialValues?: ParametersForActionExecution; + fetchInitialValues?: () => Promise<ParametersForActionExecution>; + shouldPrefetch?: boolean; onSubmit: (opts: ExecuteActionOpts) => Promise<ActionFormSubmitResult>; onClose?: () => void; + onSuccess?: () => void; } interface ActionLoaderProps { @@ -31,23 +34,36 @@ const mapDispatchToProps = { const ActionExecuteModal = ({ action, + initialValues, + fetchInitialValues, + shouldPrefetch, onSubmit, onClose, + onSuccess, }: ActionExecuteModalProps) => { - const handleSubmit = useCallback( - (parameters: ParametersForActionExecution) => { - return onSubmit({ action, parameters }); - }, - [action, onSubmit], - ); + const handleSubmit = (parameters: ParametersForActionExecution) => { + return onSubmit({ action, parameters }); + }; + + const handleSubmitSuccess = () => { + onClose?.(); + onSuccess?.(); + }; return ( - <ModalContent title={action.name} onClose={onClose}> + <ModalContent + data-testid="action-execute-modal" + title={action.name} + onClose={onClose} + > <ActionParametersInputForm action={action} + initialValues={initialValues} + fetchInitialValues={fetchInitialValues} + shouldPrefetch={shouldPrefetch} onCancel={onClose} onSubmit={handleSubmit} - onSubmitSuccess={onClose} + onSubmitSuccess={handleSubmitSuccess} /> </ModalContent> ); diff --git a/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.tsx b/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.tsx index ef1dbf770f1128f0502aa8150626ec31ca32ed2e..e77bbea86db850ef231754ea8cb5cfabfbb6ed1a 100644 --- a/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.tsx +++ b/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.tsx @@ -1,43 +1,34 @@ -import { useCallback, useMemo, useState, useEffect } from "react"; +import { useCallback, useEffect, useMemo, useState } from "react"; import { t } from "ttag"; -import _ from "underscore"; import EmptyState from "metabase/components/EmptyState"; -import { ActionsApi, PublicApi } from "metabase/services"; - import ActionForm from "metabase/actions/components/ActionForm"; -import { getDashboardType } from "metabase/dashboard/utils"; import type { - WritebackParameter, OnSubmitActionForm, - Dashboard, - ActionDashboardCard, ParametersForActionExecution, WritebackAction, + WritebackParameter, } from "metabase-types/api"; export interface ActionParametersInputFormProps { action: WritebackAction; - dashboard?: Dashboard; - dashcard?: ActionDashboardCard; mappedParameters?: WritebackParameter[]; - dashcardParamValues?: ParametersForActionExecution; + initialValues?: ParametersForActionExecution; + fetchInitialValues?: () => Promise<ParametersForActionExecution>; + shouldPrefetch?: boolean; onSubmit: OnSubmitActionForm; onSubmitSuccess?: () => void; onCancel?: () => void; } -const shouldPrefetchValues = (action: WritebackAction) => - action.type === "implicit" && action.kind === "row/update"; - function ActionParametersInputForm({ action, mappedParameters = [], - dashcardParamValues = {}, - dashboard, - dashcard, + initialValues = {}, + fetchInitialValues, + shouldPrefetch, onCancel, onSubmit, onSubmitSuccess, @@ -46,17 +37,10 @@ function ActionParametersInputForm({ useState<ParametersForActionExecution>({}); const hasPrefetchedValues = Object.keys(prefetchedValues).length > 0; - const shouldPrefetch = useMemo( - () => shouldPrefetchValues(action) && dashboard && dashcard, - [action, dashboard, dashcard], - ); - const initialValues = useMemo( - () => ({ - ...prefetchedValues, - ...dashcardParamValues, - }), - [prefetchedValues, dashcardParamValues], + const values = useMemo( + () => ({ ...prefetchedValues, ...initialValues }), + [prefetchedValues, initialValues], ); const hiddenFields = useMemo(() => { @@ -71,39 +55,26 @@ function ActionParametersInputForm({ .concat(hiddenFieldIds); }, [mappedParameters, action.visualization_settings?.fields]); - const fetchInitialValues = useCallback(async () => { - const prefetchEndpoint = - getDashboardType(dashboard?.id) === "public" - ? PublicApi.prefetchValues - : ActionsApi.prefetchValues; - - const fetchedValues = await prefetchEndpoint({ - dashboardId: dashboard?.id, - dashcardId: dashcard?.id, - parameters: JSON.stringify(dashcardParamValues), - }).catch(_.noop); + const prefetchValues = useCallback(async () => { + if (!fetchInitialValues) { + return; + } - if (fetchedValues) { + try { + const fetchedValues = await fetchInitialValues(); setPrefetchedValues(fetchedValues); + } catch (error) { + // do not show user this error + console.error(error); } - }, [dashboard?.id, dashcard?.id, dashcardParamValues]); + }, [fetchInitialValues]); useEffect(() => { - const hasValueFromDashboard = Object.keys(dashcardParamValues).length > 0; - const canPrefetch = hasValueFromDashboard && dashboard && dashcard; - if (shouldPrefetch && !hasPrefetchedValues) { setPrefetchedValues({}); - canPrefetch && fetchInitialValues(); + prefetchValues(); } - }, [ - shouldPrefetch, - hasPrefetchedValues, - dashboard, - dashcard, - dashcardParamValues, - fetchInitialValues, - ]); + }, [shouldPrefetch, hasPrefetchedValues, prefetchValues]); const handleSubmit = useCallback( async (parameters, actions) => { @@ -112,12 +83,17 @@ function ActionParametersInputForm({ if (success) { actions.setErrors({}); onSubmitSuccess?.(); - shouldPrefetch ? fetchInitialValues() : actions.resetForm(); + + if (shouldPrefetch) { + prefetchValues(); + } else { + actions.resetForm(); + } } else { throw new Error(error); } }, - [shouldPrefetch, onSubmit, onSubmitSuccess, fetchInitialValues], + [prefetchValues, shouldPrefetch, onSubmit, onSubmitSuccess], ); if (shouldPrefetch && !hasPrefetchedValues) { @@ -127,7 +103,8 @@ function ActionParametersInputForm({ return ( <ActionForm action={action} - initialValues={initialValues} + initialValues={values} + prefetchesInitialValues={Boolean(fetchInitialValues)} hiddenFields={hiddenFields} onSubmit={handleSubmit} onClose={onCancel} diff --git a/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.unit.spec.tsx b/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.unit.spec.tsx index 4c2d556b7aae04f399e05be3d2f0216337b7e2a5..ccf5dbdb012c5113165a1c156febbc8ed6b7d3bf 100644 --- a/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.unit.spec.tsx +++ b/frontend/src/metabase/actions/containers/ActionParametersInputForm/ActionParametersInputForm.unit.spec.tsx @@ -1,18 +1,19 @@ -import _ from "underscore"; -import fetchMock from "fetch-mock"; -import userEvent from "@testing-library/user-event"; import { waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import fetchMock from "fetch-mock"; +import _ from "underscore"; import { getIcon, render, screen } from "__support__/ui"; import { createMockActionDashboardCard, createMockActionParameter, + createMockDashboard, createMockFieldSettings, - createMockQueryAction, createMockImplicitQueryAction, - createMockDashboard, + createMockQueryAction, } from "metabase-types/api/mocks"; +import { ActionsApi } from "metabase/services"; import ActionParametersInputForm, { ActionParametersInputFormProps, @@ -49,17 +50,28 @@ const mockAction = createMockQueryAction({ }, }); +const dashboard = createMockDashboard({ id: 123 }); + +const dashcard = createMockActionDashboardCard({ id: 456, action: mockAction }); + const defaultProps: ActionParametersInputFormProps = { action: mockAction, mappedParameters: [], - dashboard: createMockDashboard({ id: 123 }), - dashcard: createMockActionDashboardCard({ id: 456, action: mockAction }), - dashcardParamValues: {}, + fetchInitialValues: undefined, + shouldPrefetch: false, + initialValues: {}, onCancel: _.noop, onSubmitSuccess: _.noop, onSubmit: jest.fn().mockResolvedValue({ success: true }), }; +const fetchInitialValues = () => + ActionsApi.prefetchDashcardValues({ + dashboardId: dashboard.id, + dashcardId: dashcard.id, + parameters: JSON.stringify({}), + }).catch(_.noop); + function setup(options?: Partial<ActionParametersInputModalProps>) { render(<ActionParametersInputForm {...defaultProps} {...options} />); } @@ -174,9 +186,11 @@ describe("Actions > ActionParametersInputForm", () => { parameters: [idParameter, parameter1, parameter2], }), mappedParameters: [idParameter], - dashcardParamValues: { + initialValues: { id: 888, }, + fetchInitialValues, + shouldPrefetch: true, }); await waitFor(async () => { @@ -194,7 +208,8 @@ describe("Actions > ActionParametersInputForm", () => { type: "implicit", kind: "row/update", }), - dashcardParamValues: {}, + initialValues: {}, + shouldPrefetch: true, }); expect(screen.getByText(/Choose a record to update/i)).toBeInTheDocument(); @@ -220,9 +235,11 @@ describe("Actions > ActionParametersInputForm", () => { type: "implicit", kind: "row/update", }), - dashcardParamValues: { + initialValues: { id: 888, }, + fetchInitialValues, + shouldPrefetch: true, }); expect( diff --git a/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.ts b/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.ts index 5cb53826e128085251ec68bfb1cf50abd0cd37ed..1182314a08b1a9276c77d39cd3d65e91851e632c 100644 --- a/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.ts +++ b/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.ts @@ -19,9 +19,16 @@ import { type Opts = { action: WritebackAction; initialValues?: ActionFormInitialValues; + prefetchesInitialValues?: boolean; }; -function useActionForm({ action, initialValues = {} }: Opts) { +const INITIAL_VALUES = {}; + +function useActionForm({ + action, + initialValues = INITIAL_VALUES, + prefetchesInitialValues, +}: Opts) { const fieldSettings = useMemo(() => { return getOrGenerateFieldSettings( action.parameters, @@ -54,17 +61,19 @@ function useActionForm({ action, initialValues = {} }: Opts) { const allValues = { ...cleanedInitialValues, ...values }; const formatted = formatSubmitValues(allValues, fieldSettings); - const isImplicitUpdate = - action.type === "implicit" && action.kind === "row/update"; - - // For implicit update actions, we sometimes prefetch selected row values, - // and pass them as initial values to prefill the form. - // In that case, we want to return only changed values - return isImplicitUpdate + // For some actions (e.g. implicit update actions), we prefetch + // selected row values, and pass them as initial values to prefill + // the form. In that case, we want to return only changed values. + return prefetchesInitialValues ? getChangedValues(formatted, initialValues) : formatted; }, - [action, initialValues, cleanedInitialValues, fieldSettings], + [ + initialValues, + cleanedInitialValues, + fieldSettings, + prefetchesInitialValues, + ], ); return { diff --git a/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.unit.spec.ts b/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.unit.spec.ts index 5bf53c6087ada048b970098f72748da55a4125dd..552529847be9457548a0e121c65e5e17738ab13d 100644 --- a/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.unit.spec.ts +++ b/frontend/src/metabase/actions/hooks/use-action-form/use-action-form.unit.spec.ts @@ -132,7 +132,7 @@ describe("useActionForm", () => { }); }); - it("should filter out unchanged values for implicit update actions", () => { + it("should filter out unchanged values when prefetching initial values", () => { const parameter = createMockActionParameter({ id: "param1", type: "string", @@ -145,6 +145,7 @@ describe("useActionForm", () => { useActionForm({ action, initialValues: { param1: "some value" }, + prefetchesInitialValues: true, }), ); expect(result.current.getCleanValues({ param1: "some value" })).toEqual( diff --git a/frontend/src/metabase/actions/utils.ts b/frontend/src/metabase/actions/utils.ts index 3af461a4c64baa95e4ed6567013cf72336626149..084197888c1b76a93b3807b3bbc8c1281efa9bb9 100644 --- a/frontend/src/metabase/actions/utils.ts +++ b/frontend/src/metabase/actions/utils.ts @@ -303,7 +303,7 @@ export const getFormValidationSchema = ( }; export const getSubmitButtonColor = (action: WritebackAction): string => { - if (action.type === "implicit" && action.kind === "row/delete") { + if (isImplicitDeleteAction(action)) { return "danger"; } return action.visualization_settings?.submitButtonColor ?? "primary"; @@ -334,3 +334,9 @@ export const getSubmitButtonLabel = (action: WritebackAction): string => { export const isActionPublic = (action: Partial<WritebackAction>) => { return action.public_uuid != null; }; + +export const isImplicitDeleteAction = (action: WritebackAction): boolean => + action.type === "implicit" && action.kind === "row/delete"; + +export const isImplicitUpdateAction = (action: WritebackAction): boolean => + action.type === "implicit" && action.kind === "row/update"; diff --git a/frontend/src/metabase/common/hooks/index.ts b/frontend/src/metabase/common/hooks/index.ts index 1079b6f02ad8065ec7b39ddc756e4dfcb95444dd..508afe37ece833b0126ed70673bb58027334230c 100644 --- a/frontend/src/metabase/common/hooks/index.ts +++ b/frontend/src/metabase/common/hooks/index.ts @@ -1,3 +1,4 @@ +export * from "./use-action-list-query"; export * from "./use-collection-query"; export * from "./use-collection-list-query"; export * from "./use-dashboard-query"; diff --git a/frontend/src/metabase/common/hooks/use-action-list-query/index.ts b/frontend/src/metabase/common/hooks/use-action-list-query/index.ts new file mode 100644 index 0000000000000000000000000000000000000000..7f4299c9532b56ae365754af9b8c0cd7185b24a3 --- /dev/null +++ b/frontend/src/metabase/common/hooks/use-action-list-query/index.ts @@ -0,0 +1 @@ +export * from "./use-action-list-query"; diff --git a/frontend/src/metabase/common/hooks/use-action-list-query/use-action-list-query.ts b/frontend/src/metabase/common/hooks/use-action-list-query/use-action-list-query.ts new file mode 100644 index 0000000000000000000000000000000000000000..5392ce15911353565e7ba64840c1d04198b96a50 --- /dev/null +++ b/frontend/src/metabase/common/hooks/use-action-list-query/use-action-list-query.ts @@ -0,0 +1,20 @@ +import { WritebackAction, WritebackActionListQuery } from "metabase-types/api"; +import { + useEntityListQuery, + UseEntityListQueryProps, + UseEntityListQueryResult, +} from "metabase/common/hooks/use-entity-list-query"; +import Actions from "metabase/entities/actions"; + +export const useActionListQuery = ( + props: UseEntityListQueryProps<WritebackActionListQuery> = {}, +): UseEntityListQueryResult<WritebackAction> => { + return useEntityListQuery(props, { + fetchList: Actions.actions.fetchList, + getList: Actions.selectors.getList, + getLoading: Actions.selectors.getLoading, + getLoaded: Actions.selectors.getLoaded, + getError: Actions.selectors.getError, + getListMetadata: Actions.selectors.getListMetadata, + }); +}; diff --git a/frontend/src/metabase/common/hooks/use-action-list-query/use-action-list-query.unit.spec.tsx b/frontend/src/metabase/common/hooks/use-action-list-query/use-action-list-query.unit.spec.tsx new file mode 100644 index 0000000000000000000000000000000000000000..eafe08295c1528657b8ea64fcc8786c6425ce25a --- /dev/null +++ b/frontend/src/metabase/common/hooks/use-action-list-query/use-action-list-query.unit.spec.tsx @@ -0,0 +1,46 @@ +import { setupActionsEndpoints } from "__support__/server-mocks"; +import { + renderWithProviders, + screen, + waitForElementToBeRemoved, +} from "__support__/ui"; +import { createMockImplicitQueryAction } from "metabase-types/api/mocks"; +import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper"; + +import { useActionListQuery } from "./use-action-list-query"; + +const IMPLICIT_ACTION = createMockImplicitQueryAction(); + +const TestComponent = () => { + const { data = [], isLoading, error } = useActionListQuery(); + + if (isLoading || error) { + return <LoadingAndErrorWrapper loading={isLoading} error={error} />; + } + + return ( + <div> + {data.map(action => ( + <div key={action.id}>{action.name}</div> + ))} + </div> + ); +}; + +const setup = () => { + setupActionsEndpoints([IMPLICIT_ACTION]); + renderWithProviders(<TestComponent />); +}; + +describe("useActionListQuery", () => { + it("should be initially loading", () => { + setup(); + expect(screen.getByText("Loading...")).toBeInTheDocument(); + }); + + it("should show data from the response", async () => { + setup(); + await waitForElementToBeRemoved(() => screen.queryByText("Loading...")); + expect(screen.getByText(IMPLICIT_ACTION.name)).toBeInTheDocument(); + }); +}); diff --git a/frontend/src/metabase/components/EntityMenu/EntityMenu.jsx b/frontend/src/metabase/components/EntityMenu/EntityMenu.jsx index f0379ea04b9cc9faa4f4a6e75dd508c6572b4c23..af604c3c3dcc74df7db8059a2a52e21953ae0f29 100644 --- a/frontend/src/metabase/components/EntityMenu/EntityMenu.jsx +++ b/frontend/src/metabase/components/EntityMenu/EntityMenu.jsx @@ -21,6 +21,10 @@ class EntityMenu extends Component { menuItemContent: null, }; + static defaultProps = { + horizontalAttachments: ["left", "right"], + }; + constructor(props, context) { super(props, context); @@ -54,6 +58,7 @@ class EntityMenu extends Component { className, openClassNames, closedClassNames, + horizontalAttachments, minWidth, tooltip, trigger, @@ -89,7 +94,7 @@ class EntityMenu extends Component { onClose={this.toggleMenu} hasArrow={false} hasBackground={false} - horizontalAttachments={["left", "right"]} + horizontalAttachments={horizontalAttachments} targetOffsetY={targetOffsetY || 0} ignoreTrigger > diff --git a/frontend/src/metabase/components/ModalContent/ModalContent.tsx b/frontend/src/metabase/components/ModalContent/ModalContent.tsx index f65d024f07b2bc918e1394a8a95457691cbf3e0a..00c6026ca189843bf7108d2b03ca51abf12ed25b 100644 --- a/frontend/src/metabase/components/ModalContent/ModalContent.tsx +++ b/frontend/src/metabase/components/ModalContent/ModalContent.tsx @@ -9,6 +9,7 @@ import { } from "./ModalContent.styled"; export interface ModalContentProps extends CommonModalProps { + "data-testid"?: string; id?: string; title: string; footer?: ReactNode; @@ -31,6 +32,7 @@ interface CommonModalProps { // eslint-disable-next-line import/no-default-export -- deprecated usage export default class ModalContent extends Component<ModalContentProps> { static propTypes = { + "data-testid": PropTypes.string, id: PropTypes.string, title: PropTypes.string, centeredTitle: PropTypes.bool, @@ -57,6 +59,7 @@ export default class ModalContent extends Component<ModalContentProps> { render() { const { + "data-testid": dataTestId, title, centeredTitle, footer, @@ -78,6 +81,7 @@ export default class ModalContent extends Component<ModalContentProps> { // add bottom padding if this is a standard "form modal" with no footer { pb4: formModal && !footer }, )} + data-testid={dataTestId} > {title && ( <ModalHeader diff --git a/frontend/src/metabase/dashboard/actions/actions.ts b/frontend/src/metabase/dashboard/actions/actions.ts index d17a61b633a87ab9bc9cc200e72b399468dd91ae..0d44846e751bff8ca64b145039e79d7c519a756b 100644 --- a/frontend/src/metabase/dashboard/actions/actions.ts +++ b/frontend/src/metabase/dashboard/actions/actions.ts @@ -19,7 +19,6 @@ import type { Dispatch } from "metabase-types/store"; import { getDashboardType } from "../utils"; import { setDashCardAttributes } from "./core"; -import { reloadDashboardCards } from "./data-fetching"; import { closeSidebar, setSidebar } from "./ui"; interface DashboardAttributes { @@ -71,7 +70,6 @@ export const executeRowAction = async ({ parameters, }); - dispatch(reloadDashboardCards()); const message = getActionExecutionMessage( dashcard.action as WritebackAction, result, diff --git a/frontend/src/metabase/models/components/ModelDetailPage/ModelActionDetails/ModelActionDetails.tsx b/frontend/src/metabase/models/components/ModelDetailPage/ModelActionDetails/ModelActionDetails.tsx index 6d32877c946643434c9d3bbd4e72689c21fd5c96..3f5aa5452d588b839cc8d5462bea0f919bce842b 100644 --- a/frontend/src/metabase/models/components/ModelDetailPage/ModelActionDetails/ModelActionDetails.tsx +++ b/frontend/src/metabase/models/components/ModelDetailPage/ModelActionDetails/ModelActionDetails.tsx @@ -39,7 +39,6 @@ import { interface OwnProps { model: Question; - canRunActions: boolean; } interface DispatchProps { diff --git a/frontend/src/metabase/models/components/ModelDetailPage/ModelDetailPage.tsx b/frontend/src/metabase/models/components/ModelDetailPage/ModelDetailPage.tsx index 305d5be444e5d5909473dd50531f13a1eb20a789..a3b1ab0a28948b74f4cbfc44d4c26bb3c96ab688 100644 --- a/frontend/src/metabase/models/components/ModelDetailPage/ModelDetailPage.tsx +++ b/frontend/src/metabase/models/components/ModelDetailPage/ModelDetailPage.tsx @@ -28,7 +28,6 @@ interface Props { tab: string; hasDataPermissions: boolean; hasActionsTab: boolean; - canRunActions: boolean; onChangeName: (name?: string) => void; onChangeDescription: (description?: string | null) => void; onChangeCollection: (collection: Collection) => void; @@ -40,7 +39,6 @@ function ModelDetailPage({ mainTable, hasDataPermissions, hasActionsTab, - canRunActions, onChangeName, onChangeDescription, onChangeCollection, @@ -92,10 +90,7 @@ function ModelDetailPage({ {hasActionsTab && ( <TabPanel value="actions"> <TabPanelContent> - <ModelActionDetails - model={model} - canRunActions={canRunActions} - /> + <ModelActionDetails model={model} /> </TabPanelContent> </TabPanel> )} diff --git a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.tsx b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.tsx index 14cbd470e83c1042e17a061817d76b41d99fcf87..22e7f75ddf1354fb5883cfb18032f842ce1c4bee 100644 --- a/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.tsx +++ b/frontend/src/metabase/models/containers/ModelDetailPage/ModelDetailPage.tsx @@ -90,7 +90,6 @@ function ModelDetailPage({ const hasActions = actions.length > 0; const hasActionsEnabled = database != null && database.hasActionsEnabled(); const hasActionsTab = hasActions || hasActionsEnabled; - const canRunActions = hasActionsEnabled && hasDataPermissions; const mainTable = useMemo( () => (model.isStructured() ? model.query().sourceTable() : null), @@ -175,7 +174,6 @@ function ModelDetailPage({ mainTable={mainTable} tab={tab} hasDataPermissions={hasDataPermissions} - canRunActions={canRunActions} hasActionsTab={hasActionsTab} onChangeName={handleNameChange} onChangeDescription={handleDescriptionChange} diff --git a/frontend/src/metabase/services.js b/frontend/src/metabase/services.js index 030e2dcb4a62b15bbe941cf93c9634f0e74fc5d6..73ca1894bde58999af0ad41caed6eb04c51fb983 100644 --- a/frontend/src/metabase/services.js +++ b/frontend/src/metabase/services.js @@ -248,7 +248,7 @@ export const PublicApi = { dashboardCardQueryPivot: GET( PIVOT_PUBLIC_PREFIX + "dashboard/:uuid/dashcard/:dashcardId/card/:cardId", ), - prefetchValues: GET( + prefetchDashcardValues: GET( "/api/public/dashboard/:dashboardId/dashcard/:dashcardId/execute", ), }; @@ -579,7 +579,8 @@ export const ActionsApi = { create: POST("/api/action"), update: PUT("/api/action/:id"), execute: POST("/api/action/:id/execute"), - prefetchValues: GET( + prefetchValues: GET("/api/action/:id/execute"), + prefetchDashcardValues: GET( "/api/dashboard/:dashboardId/dashcard/:dashcardId/execute", ), executeDashcardAction: POST( diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/DeleteObjectModal.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/DeleteObjectModal.tsx new file mode 100644 index 0000000000000000000000000000000000000000..48dcf90cd87406cc32ddf4237f2f480672fa07b8 --- /dev/null +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/DeleteObjectModal.tsx @@ -0,0 +1,70 @@ +import { FunctionComponent } from "react"; +import { t } from "ttag"; + +import { WritebackActionId } from "metabase-types/api"; +import { getActionErrorMessage } from "metabase/actions/utils"; +import ModalContent from "metabase/components/ModalContent"; +import Button from "metabase/core/components/Button"; +import { useDispatch } from "metabase/lib/redux"; +import { addUndo } from "metabase/redux/undo"; +import { ActionsApi } from "metabase/services"; + +import { ObjectId } from "./types"; + +interface Props { + actionId: WritebackActionId | undefined; + objectId: ObjectId | undefined; + onClose: () => void; + onSuccess: () => void; +} + +export const DeleteObjectModal: FunctionComponent<Props> = ({ + actionId, + objectId, + onClose, + onSuccess, +}) => { + const dispatch = useDispatch(); + + const handleSubmit = async () => { + try { + await ActionsApi.execute({ + id: actionId, + parameters: { + id: typeof objectId === "string" ? parseInt(objectId, 10) : objectId, + }, + }); + + const message = t`Successfully deleted`; + dispatch(addUndo({ message, toastColor: "success" })); + onClose(); + onSuccess(); + } catch (error) { + const message = getActionErrorMessage(error); + dispatch(addUndo({ icon: "warning", toastColor: "error", message })); + } + }; + + return ( + <ModalContent + data-testid="delete-object-modal" + title={t`Are you sure you want to delete this row?`} + footer={[ + <Button key="cancel" onClick={onClose}>{t`Cancel`}</Button>, + <Button + key="delete" + danger + disabled={ + typeof actionId === "undefined" || + typeof objectId === "undefined" || + objectId === null + } + onClick={handleSubmit} + >{t`Delete forever`}</Button>, + ]} + onClose={onClose} + > + {t`This will permanently delete the row. There’s no undoing this, so please be sure.`} + </ModalContent> + ); +}; diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx index ff6185b51eb5c63d4df948ac9e3721fe6342419f..529e214720bfc615e6f8ceb771e37ae0cea6fdd0 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.tsx @@ -2,24 +2,34 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { connect } from "react-redux"; import _ from "underscore"; import { t } from "ttag"; - import { useMount, usePrevious } from "react-use"; + import { State } from "metabase-types/store"; import type { ConcreteTableId, DatasetData, VisualizationSettings, + WritebackActionId, } from "metabase-types/api"; +import ActionExecuteModal from "metabase/actions/containers/ActionExecuteModal"; +import { + useActionListQuery, + useDatabaseListQuery, +} from "metabase/common/hooks"; import Button from "metabase/core/components/Button"; import { NotFound } from "metabase/containers/ErrorPages"; +import EntityMenu from "metabase/components/EntityMenu"; import LoadingSpinner from "metabase/components/LoadingSpinner"; +import Modal from "metabase/components/Modal"; +import { Flex } from "metabase/ui/components"; import Tables from "metabase/entities/tables"; import { closeObjectDetail, followForeignKey, loadObjectDetailFKReferences, + runQuestionQuery, viewNextObjectDetail, viewPreviousObjectDetail, } from "metabase/query_builder/actions"; @@ -35,7 +45,8 @@ import { } from "metabase/query_builder/selectors"; import { getUser } from "metabase/selectors/user"; -import { MetabaseApi } from "metabase/services"; +import { useDispatch } from "metabase/lib/redux"; +import { ActionsApi, MetabaseApi } from "metabase/services"; import { ObjectDetailWrapper } from "metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper"; import { isVirtualCardId } from "metabase-lib/metadata/utils/saved-questions"; import { isPK } from "metabase-lib/types/utils/isa"; @@ -46,7 +57,9 @@ import type { OnVisualizationClickType, } from "./types"; +import { DeleteObjectModal } from "./DeleteObjectModal"; import { + getActionItems, getDisplayId, getIdValue, getObjectName, @@ -129,7 +142,7 @@ export function ObjectDetailView({ canZoom, canZoomPreviousRow, canZoomNextRow, - showActions = true, + showControls = true, showRelations = true, showHeader, onVisualizationClick, @@ -148,6 +161,20 @@ export function ObjectDetailView({ const prevData = usePrevious(passedData); const prevTableForeignKeys = usePrevious(tableForeignKeys); const [data, setData] = useState<DatasetData>(passedData); + const [actionId, setActionId] = useState<WritebackActionId>(); + const [deleteActionId, setDeleteActionId] = useState<WritebackActionId>(); + + const isActionExecuteModalOpen = typeof actionId === "number"; + const isDeleteModalOpen = typeof deleteActionId === "number"; + const isModalOpen = isActionExecuteModalOpen || isDeleteModalOpen; + + const handleExecuteModalClose = () => { + setActionId(undefined); + }; + + const handleDeleteModalClose = () => { + setDeleteActionId(undefined); + }; const pkIndex = useMemo( () => getSinglePKIndex(passedData?.cols), @@ -198,13 +225,10 @@ export function ObjectDetailView({ Escape: closeObjectDetail, }; - if (capturedKeys[event.key]) { + if (capturedKeys[event.key] && !isModalOpen) { event.preventDefault(); capturedKeys[event.key](); } - if (event.key === "Escape") { - closeObjectDetail(); - } }; window.addEventListener("keydown", onKeyDown, true); @@ -214,6 +238,7 @@ export function ObjectDetailView({ viewPreviousObjectDetail, viewNextObjectDetail, closeObjectDetail, + isModalOpen, ]); useEffect(() => { @@ -279,6 +304,54 @@ export function ObjectDetailView({ [zoomedRowID, followForeignKey], ); + const areImplicitActionsEnabled = + question && + question.canWrite() && + question.isDataset() && + question.supportsImplicitActions(); + + const { data: actions = [] } = useActionListQuery({ + enabled: areImplicitActionsEnabled, + query: { "model-id": question?.id() }, + }); + + const { data: databases = [] } = useDatabaseListQuery({ + enabled: areImplicitActionsEnabled, + }); + + const actionItems = areImplicitActionsEnabled + ? getActionItems({ + actions, + databases, + onDelete: action => setDeleteActionId(action.id), + onUpdate: action => setActionId(action.id), + }) + : []; + + const fetchInitialValues = useCallback(async () => { + if (typeof actionId !== "number") { + return {}; + } + + return ActionsApi.prefetchValues({ + id: actionId, + parameters: JSON.stringify({ id: String(zoomedRowID) }), + }); + }, [actionId, zoomedRowID]); + + const initialValues = useMemo(() => ({ id: zoomedRowID }), [zoomedRowID]); + + const dispatch = useDispatch(); + + const handleActionSuccess = useCallback(() => { + dispatch(runQuestionQuery()); + }, [dispatch]); + + const handleDeleteSuccess = useCallback(() => { + handleActionSuccess(); + closeObjectDetail(); + }, [closeObjectDetail, handleActionSuccess]); + if (!data) { return null; } @@ -302,72 +375,104 @@ export function ObjectDetailView({ showRelations && !_.isEmpty(tableForeignKeys) && hasPk; return ( - <ObjectDetailContainer wide={hasRelationships} className={className}> - {maybeLoading ? ( - <ErrorWrapper> - <LoadingSpinner /> - </ErrorWrapper> - ) : hasNotFoundError ? ( - <ErrorWrapper> - <NotFound message={t`We couldn't find that record`} /> - </ErrorWrapper> - ) : ( - <ObjectDetailWrapperDiv - className="ObjectDetail" - data-testid="object-detail" - > - {showHeader && ( - <ObjectDetailHeader - canZoom={Boolean( - canZoom && (canZoomNextRow || canZoomPreviousRow), - )} + <> + <ObjectDetailContainer wide={hasRelationships} className={className}> + {maybeLoading ? ( + <ErrorWrapper> + <LoadingSpinner /> + </ErrorWrapper> + ) : hasNotFoundError ? ( + <ErrorWrapper> + <NotFound message={t`We couldn't find that record`} /> + </ErrorWrapper> + ) : ( + <ObjectDetailWrapperDiv + className="ObjectDetail" + data-testid="object-detail" + > + {showHeader && ( + <ObjectDetailHeader + actionItems={actionItems} + canZoom={Boolean( + canZoom && (canZoomNextRow || canZoomPreviousRow), + )} + objectName={objectName} + objectId={displayId} + canZoomPreviousRow={!!canZoomPreviousRow} + canZoomNextRow={canZoomNextRow} + showControls={showControls} + viewPreviousObjectDetail={viewPreviousObjectDetail} + viewNextObjectDetail={viewNextObjectDetail} + closeObjectDetail={closeObjectDetail} + /> + )} + <ObjectDetailBody + data={data} objectName={objectName} - objectId={displayId} - canZoomPreviousRow={!!canZoomPreviousRow} - canZoomNextRow={canZoomNextRow} - showActions={showActions} - viewPreviousObjectDetail={viewPreviousObjectDetail} - viewNextObjectDetail={viewNextObjectDetail} - closeObjectDetail={closeObjectDetail} + zoomedRow={zoomedRow ?? []} + settings={settings} + hasRelationships={hasRelationships} + onVisualizationClick={onVisualizationClick} + visualizationIsClickable={visualizationIsClickable} + tableForeignKeys={tableForeignKeys} + tableForeignKeyReferences={tableForeignKeyReferences} + followForeignKey={onFollowForeignKey} /> - )} - <ObjectDetailBody - data={data} - objectName={objectName} - zoomedRow={zoomedRow ?? []} - settings={settings} - hasRelationships={hasRelationships} - onVisualizationClick={onVisualizationClick} - visualizationIsClickable={visualizationIsClickable} - tableForeignKeys={tableForeignKeys} - tableForeignKeyReferences={tableForeignKeyReferences} - followForeignKey={onFollowForeignKey} - /> - </ObjectDetailWrapperDiv> - )} - </ObjectDetailContainer> + </ObjectDetailWrapperDiv> + )} + </ObjectDetailContainer> + + <Modal + isOpen={isActionExecuteModalOpen} + onClose={handleExecuteModalClose} + > + <ActionExecuteModal + actionId={actionId} + initialValues={initialValues} + fetchInitialValues={fetchInitialValues} + shouldPrefetch + onClose={handleExecuteModalClose} + onSuccess={handleActionSuccess} + /> + </Modal> + + <Modal isOpen={isDeleteModalOpen} onClose={handleDeleteModalClose}> + <DeleteObjectModal + actionId={deleteActionId} + objectId={zoomedRowID} + onClose={handleDeleteModalClose} + onSuccess={handleDeleteSuccess} + /> + </Modal> + </> ); } export interface ObjectDetailHeaderProps { + actionItems: { + title: string; + icon: string; + action: () => void; + }[]; canZoom: boolean; objectName: string; objectId: ObjectId | null | unknown; canZoomPreviousRow: boolean; canZoomNextRow?: boolean; - showActions?: boolean; + showControls?: boolean; viewPreviousObjectDetail: () => void; viewNextObjectDetail: () => void; closeObjectDetail: () => void; } export function ObjectDetailHeader({ + actionItems, canZoom, objectName, objectId, canZoomPreviousRow, canZoomNextRow, - showActions = true, + showControls = true, viewPreviousObjectDetail, viewNextObjectDetail, closeObjectDetail, @@ -380,41 +485,51 @@ export function ObjectDetailHeader({ {objectId !== null && <ObjectIdLabel> {objectId}</ObjectIdLabel>} </h2> </div> - {showActions && ( - <div className="flex align-center"> - <div className="flex p2"> - {!!canZoom && ( - <> - <Button - data-testid="view-previous-object-detail" - onlyIcon - borderless - className="mr1" - disabled={!canZoomPreviousRow} - onClick={viewPreviousObjectDetail} - icon="chevronup" - /> - <Button - data-testid="view-next-object-detail" - onlyIcon - borderless - disabled={!canZoomNextRow} - onClick={viewNextObjectDetail} - icon="chevrondown" - /> - </> - )} - <CloseButton> + + {showControls && ( + <Flex align="center" gap="0.5rem" p="1rem"> + {canZoom && ( + <> <Button - data-testid="object-detail-close-button" + data-testid="view-previous-object-detail" onlyIcon borderless - onClick={closeObjectDetail} - icon="close" + disabled={!canZoomPreviousRow} + onClick={viewPreviousObjectDetail} + icon="chevronup" /> - </CloseButton> - </div> - </div> + <Button + data-testid="view-next-object-detail" + onlyIcon + borderless + disabled={!canZoomNextRow} + onClick={viewNextObjectDetail} + icon="chevrondown" + /> + </> + )} + + {actionItems.length > 0 && ( + <EntityMenu + horizontalAttachments={["right", "left"]} + items={actionItems} + triggerIcon="ellipsis" + triggerProps={{ + "data-testid": "actions-menu", + }} + /> + )} + + <CloseButton> + <Button + data-testid="object-detail-close-button" + onlyIcon + borderless + onClick={closeObjectDetail} + icon="close" + /> + </CloseButton> + </Flex> )} </ObjectDetailHeaderWrapper> ); diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx index 06ef62efac6d7d0a9553ea5c4e8dacef0324a5d5..3c0287115fd7e3b2f60e29f4c2c8cd3bb47a9533 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetail.unit.spec.tsx @@ -1,50 +1,249 @@ -import { render, screen } from "@testing-library/react"; +import { render, screen, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { createMockMetadata } from "__support__/metadata"; +import { + setupActionsEndpoints, + setupCardDataset, + setupDatabasesEndpoints, +} from "__support__/server-mocks"; import { testDataset } from "__support__/testDataset"; -import { setupCardDataset } from "__support__/server-mocks"; -import { createMockCard, createMockTable } from "metabase-types/api/mocks"; - +import { renderWithProviders } from "__support__/ui"; +import { getNextId } from "__support__/utils"; +import { WritebackAction } from "metabase-types/api"; import { - createMockQueryBuilderState, - createMockState, -} from "metabase-types/store/mocks"; -import { createMockEntitiesState } from "__support__/store"; -import { getMetadata } from "metabase/selectors/metadata"; + createMockCard, + createMockDatabase, + createMockField, + createMockImplicitQueryAction, + createMockQueryAction, + createMockTable, +} from "metabase-types/api/mocks"; +import { + PEOPLE, + PEOPLE_ID, + createSampleDatabase, +} from "metabase-types/api/mocks/presets"; import { checkNotNull } from "metabase/core/utils/types"; -import type { ObjectDetailProps } from "./types"; import { - ObjectDetailView, - ObjectDetailHeader, ObjectDetailBody, + ObjectDetailHeader, + ObjectDetailView, } from "./ObjectDetail"; +import type { ObjectDetailProps } from "./types"; -const MOCK_CARD = createMockCard({ +const mockCard = createMockCard({ + id: getNextId(), name: "Product", }); -const MOCK_TABLE = createMockTable({ - name: "Product", - display_name: "Product", +const mockTable = createMockTable({ + id: getNextId(), +}); + +const mockTableNoPk = createMockTable({ + id: getNextId(), + fields: [], +}); + +const mockTableMultiplePks = createMockTable({ + id: getNextId(), + fields: [ + createMockField({ semantic_type: "type/PK" }), + createMockField({ semantic_type: "type/PK" }), + ], +}); + +const databaseWithActionsEnabled = createMockDatabase({ + id: getNextId(), + settings: { "database-enable-actions": true }, +}); + +const databaseWithActionsDisabled = createMockDatabase({ + id: getNextId(), + settings: { "database-enable-actions": false }, +}); + +const mockDatasetCard = createMockCard({ + id: getNextId(), + dataset: true, + dataset_query: { + type: "query", + database: databaseWithActionsEnabled.id, + query: { + "source-table": PEOPLE_ID, + }, + }, +}); + +const mockDatasetNoPkCard = createMockCard({ + id: getNextId(), + dataset: true, + dataset_query: { + type: "query", + database: databaseWithActionsEnabled.id, + query: { + "source-table": mockTableNoPk.id, + }, + }, +}); + +const mockDatasetMultiplePksCard = createMockCard({ + id: getNextId(), + dataset: true, + dataset_query: { + type: "query", + database: databaseWithActionsEnabled.id, + query: { + "source-table": mockTableMultiplePks.id, + }, + }, +}); + +const mockDatasetWithClausesCard = createMockCard({ + id: getNextId(), + dataset: true, + dataset_query: { + type: "query", + database: databaseWithActionsEnabled.id, + query: { + "source-table": PEOPLE_ID, + filter: [ + "contains", + ["field", PEOPLE.NAME, null], + "Macy", + { "case-sensitive": false }, + ], + }, + }, +}); + +const mockDatasetNoWritePermissionCard = createMockCard({ + id: getNextId(), + can_write: false, + dataset: true, + dataset_query: { + type: "query", + database: databaseWithActionsEnabled.id, + query: { + "source-table": PEOPLE_ID, + }, + }, }); -function setup(options?: Partial<ObjectDetailProps>) { - const state = createMockState({ - entities: createMockEntitiesState({ - questions: [MOCK_CARD], - tables: [MOCK_TABLE], +const metadata = createMockMetadata({ + databases: [ + createSampleDatabase({ + id: databaseWithActionsEnabled.id, + settings: { "database-enable-actions": true }, }), - qb: createMockQueryBuilderState({ card: MOCK_CARD }), - }); - const metadata = getMetadata(state); + ], + tables: [mockTable, mockTableMultiplePks, mockTableNoPk], + questions: [ + mockCard, + mockDatasetCard, + mockDatasetNoPkCard, + mockDatasetMultiplePksCard, + mockDatasetWithClausesCard, + mockDatasetNoWritePermissionCard, + ], +}); + +const mockQuestion = checkNotNull(metadata.question(mockCard.id)); + +const mockDataset = checkNotNull(metadata.question(mockDatasetCard.id)); + +const mockDatasetNoPk = checkNotNull(metadata.question(mockDatasetNoPkCard.id)); + +const mockDatasetMultiplePks = checkNotNull( + metadata.question(mockDatasetMultiplePksCard.id), +); + +const mockDatasetWithClauses = checkNotNull( + metadata.question(mockDatasetWithClausesCard.id), +); + +const mockDatasetNoWritePermission = checkNotNull( + metadata.question(mockDatasetNoWritePermissionCard.id), +); + +const implicitCreateAction = createMockImplicitQueryAction({ + id: getNextId(), + database_id: databaseWithActionsEnabled.id, + name: "Create", + kind: "row/create", +}); + +const implicitDeleteAction = createMockImplicitQueryAction({ + id: getNextId(), + database_id: databaseWithActionsEnabled.id, + name: "Delete", + kind: "row/delete", +}); + +const implicitUpdateAction = createMockImplicitQueryAction({ + id: getNextId(), + database_id: databaseWithActionsEnabled.id, + name: "Update", + kind: "row/update", +}); - const question = checkNotNull(metadata.question(MOCK_CARD.id)); - const table = checkNotNull(metadata.table(MOCK_TABLE.id)); +const implicitPublicUpdateAction = { + ...implicitUpdateAction, + id: getNextId(), + name: "Public Update", + public_uuid: "mock-uuid", +}; - render( +const implicitPublicDeleteAction = { + ...implicitDeleteAction, + id: getNextId(), + name: "Public Delete", + public_uuid: "mock-uuid", +}; + +const implicitArchivedUpdateAction = { + ...implicitUpdateAction, + id: getNextId(), + name: "Archived Implicit Update", + archived: true, +}; + +const implicitArchivedDeleteAction = { + ...implicitDeleteAction, + id: getNextId(), + name: "Archived Implicit Delete", + archived: true, +}; + +const queryAction = createMockQueryAction({ + id: getNextId(), + name: "Query action", +}); + +const actions = [ + implicitCreateAction, + implicitDeleteAction, + implicitUpdateAction, + implicitPublicUpdateAction, + implicitPublicDeleteAction, + implicitArchivedUpdateAction, + implicitArchivedDeleteAction, + queryAction, +]; + +const actionsFromDatabaseWithDisabledActions = actions.map(action => ({ + ...action, + database_id: databaseWithActionsDisabled.id, +})); + +function setup( + options: Partial<ObjectDetailProps> & + Required<Pick<ObjectDetailProps, "question">>, +) { + renderWithProviders( <ObjectDetailView data={testDataset} - question={question} - table={table} zoomedRow={testDataset.rows[0]} zoomedRowID={0} tableForeignKeys={[]} @@ -73,6 +272,7 @@ describe("Object Detail", () => { it("renders an object detail header", () => { render( <ObjectDetailHeader + actionItems={[]} canZoom={false} objectName="Large Sandstone Socks" objectId={778} @@ -90,6 +290,7 @@ describe("Object Detail", () => { it("renders an object detail header with enabled next object button and disabled previous object button", () => { render( <ObjectDetailHeader + actionItems={[]} canZoom={true} objectName="Large Sandstone Socks" objectId={778} @@ -135,7 +336,7 @@ describe("Object Detail", () => { }); it("renders an object detail component", () => { - setup(); + setup({ question: mockQuestion }); expect(screen.getByText(/Product/i)).toBeInTheDocument(); expect( @@ -168,7 +369,7 @@ describe("Object Detail", () => { }); // because this row is not in the test dataset, it should trigger a fetch - setup({ zoomedRowID: "101", zoomedRow: undefined }); + setup({ question: mockQuestion, zoomedRowID: "101", zoomedRow: undefined }); expect(screen.getByTestId("loading-spinner")).toBeInTheDocument(); expect( @@ -180,9 +381,195 @@ describe("Object Detail", () => { setupCardDataset({ data: { rows: [] } }); // because this row is not in the test dataset, it should trigger a fetch - setup({ zoomedRowID: "102", zoomedRow: undefined }); + setup({ question: mockQuestion, zoomedRowID: "102", zoomedRow: undefined }); expect(screen.getByTestId("loading-spinner")).toBeInTheDocument(); expect(await screen.findByText(/we're a little lost/i)).toBeInTheDocument(); }); + + describe("renders actions menu", () => { + beforeEach(() => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDataset }); + }); + + it("should not show implicit create action", async () => { + const action = await findActionInActionMenu(implicitCreateAction); + expect(action).not.toBeInTheDocument(); + }); + + it("should show implicit update action", async () => { + const action = await findActionInActionMenu(implicitUpdateAction); + expect(action).toBeInTheDocument(); + }); + + it("should show implicit delete action", async () => { + const action = await findActionInActionMenu(implicitDeleteAction); + expect(action).toBeInTheDocument(); + }); + + it("should not show implicit public update action", async () => { + const action = await findActionInActionMenu(implicitPublicUpdateAction); + expect(action).not.toBeInTheDocument(); + }); + + it("should not show implicit public delete action", async () => { + const action = await findActionInActionMenu(implicitPublicDeleteAction); + expect(action).not.toBeInTheDocument(); + }); + + it("should not show implicit archived update action", async () => { + const action = await findActionInActionMenu(implicitArchivedUpdateAction); + expect(action).not.toBeInTheDocument(); + }); + + it("should not show implicit archived delete action", async () => { + const action = await findActionInActionMenu(implicitArchivedDeleteAction); + expect(action).not.toBeInTheDocument(); + }); + + it("should not show query action", async () => { + const action = await findActionInActionMenu(queryAction); + expect(action).not.toBeInTheDocument(); + }); + }); + + it("should not render actions menu for models based on database with actions disabled", async () => { + setupDatabasesEndpoints([databaseWithActionsDisabled]); + setupActionsEndpoints(actionsFromDatabaseWithDisabledActions); + setup({ question: mockDataset }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + it("should not render actions menu for non-model questions", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockQuestion }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + it(`should not render actions menu when "showControls" is "false"`, async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDataset, showControls: false }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + it("should render actions menu when user has write permission", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDataset }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeInTheDocument(); + }); + + it("should not render actions menu when user has no write permission", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDatasetNoWritePermission }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + /** + * This is an exotic case. It's not possible to enable implicit actions + * for a model with clauses (joins, expressions, filters, etc.). + * Implicit actions are supported only in very simple models. + */ + it("should not render actions menu when model's query has clauses", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDatasetWithClauses }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + it("should not render actions menu when model's source table does not have a primary key", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDatasetNoPk }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + it("should not render actions menu when model's source table has multiple primary keys", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDatasetMultiplePks }); + + const actionsMenu = await findActionsMenu(); + expect(actionsMenu).toBeUndefined(); + }); + + it("should show update object modal on update action click", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDataset }); + + expect( + screen.queryByTestId("action-execute-modal"), + ).not.toBeInTheDocument(); + + const action = await findActionInActionMenu(implicitUpdateAction); + expect(action).toBeInTheDocument(); + action?.click(); + + const modal = await screen.findByTestId("action-execute-modal"); + expect(modal).toBeInTheDocument(); + + expect(within(modal).getByTestId("modal-header")).toHaveTextContent( + "Update", + ); + }); + + it("should show delete object modal on delete action click", async () => { + setupDatabasesEndpoints([databaseWithActionsEnabled]); + setupActionsEndpoints(actions); + setup({ question: mockDataset }); + + expect(screen.queryByTestId("delete-object-modal")).not.toBeInTheDocument(); + + const action = await findActionInActionMenu(implicitDeleteAction); + expect(action).toBeInTheDocument(); + action?.click(); + + const modal = await screen.findByTestId("delete-object-modal"); + expect(modal).toBeInTheDocument(); + + expect(within(modal).getByTestId("modal-header")).toHaveTextContent( + "Are you sure you want to delete this row?", + ); + }); }); + +async function findActionInActionMenu({ name }: Pick<WritebackAction, "name">) { + const actionsMenu = await screen.findByTestId("actions-menu"); + userEvent.click(actionsMenu); + const popover = await screen.findByTestId("popover"); + const action = within(popover).queryByText(name); + return action; +} + +/** + * There is no loading state for useActionListQuery & useDatabaseListQuery + * in ObjectDetail component, so there is no easy way to wait for relevant + * API requests to finish. This function relies on DOM changes instead. + */ +async function findActionsMenu() { + try { + return await screen.findByTestId("actions-menu"); + } catch (error) { + return undefined; + } +} diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper.tsx b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper.tsx index 61b336930ddf9eb6ba1ff6cabd44be2e155dca07..b2953d82d81e8438578a6bdb9a2a38477f259f07 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper.tsx +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/ObjectDetailWrapper.tsx @@ -50,7 +50,7 @@ export function ObjectDetailWrapper({ data={data} question={question} showHeader={props.settings["detail.showHeader"]} - showActions={false} + showControls={false} showRelations={false} closeObjectDetail={closeObjectDetail} isDataApp={isDataApp} diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/types.ts b/frontend/src/metabase/visualizations/components/ObjectDetail/types.ts index 68d4c8ad44c6ba93dee14d5cdf68d060a1374f52..e39aa623b540fb976ce14a30cf13adbacd3628bc 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/types.ts +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/types.ts @@ -40,7 +40,7 @@ export interface ObjectDetailProps { canZoomPreviousRow?: boolean; canZoomNextRow?: boolean; isDataApp?: boolean; - showActions?: boolean; + showControls?: boolean; showRelations?: boolean; showHeader?: boolean; onVisualizationClick: OnVisualizationClickType; diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts index 81bf9fb17d78b918872a0016333fc757871d05cc..d09324daa101bf506edf42c7f9c4f5f503987700 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.ts @@ -2,11 +2,16 @@ import { t } from "ttag"; import { singularize, formatValue } from "metabase/lib/formatting"; +import { + isImplicitDeleteAction, + isImplicitUpdateAction, +} from "metabase/actions/utils"; import type { DatasetColumn, DatasetData, TableId, VisualizationSettings, + WritebackAction, } from "metabase-types/api"; import { @@ -14,8 +19,10 @@ import { isEntityName, isPK, } from "metabase-lib/types/utils/isa"; -import Question from "metabase-lib/Question"; +import { canRunAction } from "metabase-lib/actions/utils"; +import Database from "metabase-lib/metadata/Database"; import Table from "metabase-lib/metadata/Table"; +import Question from "metabase-lib/Question"; import { ObjectId } from "./types"; @@ -127,3 +134,45 @@ export const getSinglePKIndex = (cols: DatasetColumn[]) => { return index === -1 ? undefined : index; }; + +export const getActionItems = ({ + actions, + databases, + onDelete, + onUpdate, +}: { + actions: WritebackAction[]; + databases: Database[]; + onDelete: (action: WritebackAction) => void; + onUpdate: (action: WritebackAction) => void; +}) => { + const actionItems = []; + /** + * Public actions require an additional endpoint which is out of scope + * of Milestone 1 in #32320 epic. + * + * @see https://github.com/metabase/metabase/issues/32320 + * @see https://metaboat.slack.com/archives/C057T1QTB3L/p1689845931726009?thread_ts=1689665950.493399&cid=C057T1QTB3L + */ + const privateActions = actions.filter(action => !action.public_uuid); + const deleteAction = privateActions.find(isValidImplicitDeleteAction); + const updateAction = privateActions.find(isValidImplicitUpdateAction); + + if (updateAction && canRunAction(updateAction, databases)) { + const action = () => onUpdate(updateAction); + actionItems.push({ title: t`Update`, icon: "pencil", action }); + } + + if (deleteAction && canRunAction(deleteAction, databases)) { + const action = () => onDelete(deleteAction); + actionItems.push({ title: t`Delete`, icon: "trash", action }); + } + + return actionItems; +}; + +export const isValidImplicitDeleteAction = (action: WritebackAction): boolean => + isImplicitDeleteAction(action) && !action.archived; + +export const isValidImplicitUpdateAction = (action: WritebackAction): boolean => + isImplicitUpdateAction(action) && !action.archived; diff --git a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts index d974184ca734c4329716e5fd183e90c504679fc0..e9719237a94ae95f77c9e1678a17c141a5352b0d 100644 --- a/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts +++ b/frontend/src/metabase/visualizations/components/ObjectDetail/utils.unit.spec.ts @@ -3,6 +3,8 @@ import { createMockMetadata } from "__support__/metadata"; import { createMockColumn, createMockDatasetData, + createMockImplicitQueryAction, + createMockNativeDatasetQuery, } from "metabase-types/api/mocks"; import { ORDERS_ID, @@ -13,21 +15,64 @@ import { import Question from "metabase-lib/Question"; import { - getObjectName, + getActionItems, getDisplayId, getIdValue, + getObjectName, getSinglePKIndex, + isValidImplicitDeleteAction, + isValidImplicitUpdateAction, } from "./utils"; +const ACTIONS_ENABLED_DB_ID = 10; + +const ACTIONS_DISABLED_DB_ID = 11; + const card = createSavedStructuredCard({ name: "Special Order", }); +const database = createSampleDatabase(); + const metadata = createMockMetadata({ - databases: [createSampleDatabase()], + databases: [ + database, + createSampleDatabase({ + id: ACTIONS_ENABLED_DB_ID, + settings: { "database-enable-actions": true }, + }), + createSampleDatabase({ + id: ACTIONS_DISABLED_DB_ID, + settings: { "database-enable-actions": false }, + }), + ], questions: [card], }); +// eslint-disable-next-line @typescript-eslint/no-non-null-assertion +const databaseWithEnabledActions = metadata.database(ACTIONS_ENABLED_DB_ID)!; + +// eslint-disable-next-line @typescript-eslint/no-non-null-assertion +const databaseWithDisabledActions = metadata.database(ACTIONS_DISABLED_DB_ID)!; + +const implicitCreateAction = createMockImplicitQueryAction({ + database_id: ACTIONS_ENABLED_DB_ID, + name: "Create", + kind: "row/create", +}); + +const implicitDeleteAction = createMockImplicitQueryAction({ + database_id: ACTIONS_ENABLED_DB_ID, + name: "Delete", + kind: "row/delete", +}); + +const implicitUpdateAction = createMockImplicitQueryAction({ + database_id: ACTIONS_ENABLED_DB_ID, + name: "Update", + kind: "row/update", +}); + describe("ObjectDetail utils", () => { const productIdCol = createMockColumn({ name: "product_id", @@ -218,4 +263,115 @@ describe("ObjectDetail utils", () => { expect(getSinglePKIndex([qtyCol, nameCol])).toBe(undefined); }); }); + + describe("getActionItems", () => { + const onDelete = jest.fn(); + const onUpdate = jest.fn(); + const actions = [ + implicitDeleteAction, + implicitUpdateAction, + implicitCreateAction, + ]; + + it("should return delete and update action items", () => { + expect( + getActionItems({ + actions, + databases: [databaseWithEnabledActions], + onDelete, + onUpdate, + }), + ).toMatchObject([ + { title: "Update", icon: "pencil" }, + { title: "Delete", icon: "trash" }, + ]); + }); + + it("should not return any items when database actions are disabled", () => { + expect( + getActionItems({ + actions, + databases: [databaseWithDisabledActions], + onDelete, + onUpdate, + }), + ).toEqual([]); + }); + + it("should not return any items when there are no databases", () => { + expect( + getActionItems({ + actions, + databases: [], + onDelete, + onUpdate, + }), + ).toEqual([]); + }); + + it("should not return any items when there are no actions", () => { + expect( + getActionItems({ + actions: [], + databases: [databaseWithDisabledActions, databaseWithEnabledActions], + onDelete, + onUpdate, + }), + ).toEqual([]); + }); + }); + + describe("isValidImplicitDeleteAction", () => { + it("should detect implicit delete action", () => { + expect(isValidImplicitDeleteAction(implicitCreateAction)).toBe(false); + expect(isValidImplicitDeleteAction(implicitDeleteAction)).toBe(true); + expect(isValidImplicitDeleteAction(implicitUpdateAction)).toBe(false); + }); + + it("should ignore archived action", () => { + expect( + isValidImplicitDeleteAction({ + ...implicitDeleteAction, + archived: true, + }), + ).toBe(false); + }); + + it("should ignore non-implicit action", () => { + expect( + isValidImplicitDeleteAction({ + ...implicitDeleteAction, + type: "query", + dataset_query: createMockNativeDatasetQuery(), + }), + ).toBe(false); + }); + }); + + describe("isValidImplicitUpdateAction", () => { + it("should detect implicit update action", () => { + expect(isValidImplicitUpdateAction(implicitCreateAction)).toBe(false); + expect(isValidImplicitUpdateAction(implicitDeleteAction)).toBe(false); + expect(isValidImplicitUpdateAction(implicitUpdateAction)).toBe(true); + }); + + it("should ignore archived action", () => { + expect( + isValidImplicitUpdateAction({ + ...implicitUpdateAction, + archived: true, + }), + ).toBe(false); + }); + + it("should ignore non-implicit action", () => { + expect( + isValidImplicitUpdateAction({ + ...implicitUpdateAction, + type: "query", + dataset_query: createMockNativeDatasetQuery(), + }), + ).toBe(false); + }); + }); }); diff --git a/frontend/test/__support__/utils.ts b/frontend/test/__support__/utils.ts new file mode 100644 index 0000000000000000000000000000000000000000..6ce60de4d9590f00a81c7c9e5a0ed8ce51e4481a --- /dev/null +++ b/frontend/test/__support__/utils.ts @@ -0,0 +1,4 @@ +export const getNextId = (() => { + let id = 0; + return () => ++id; +})(); diff --git a/src/metabase/actions.clj b/src/metabase/actions.clj index 18a5cd8da1f654995284d1946b8b208fd88e18e8..d6fb3105765626a20968e92cc80af43ac6727750 100644 --- a/src/metabase/actions.clj +++ b/src/metabase/actions.clj @@ -141,7 +141,7 @@ "Throws an appropriate error if actions are unsupported or disabled for the database of the action's model, otherwise returns nil." [action-or-id] - (check-actions-enabled-for-database! (database-for-action action-or-id))) + (check-actions-enabled-for-database! (api/check-404 (database-for-action action-or-id)))) (defn perform-action! "Perform an `action`. Invoke this function for performing actions, e.g. in API endpoints; diff --git a/src/metabase/api/action.clj b/src/metabase/api/action.clj index 9e60256dc8a7faf94de967ad2afe80233b5f5ef4..a4e613792a8fbef19d46922a4af5c4ab755430cf 100644 --- a/src/metabase/api/action.clj +++ b/src/metabase/api/action.clj @@ -1,6 +1,7 @@ (ns metabase.api.action "`/api/action/` endpoints." (:require + [cheshire.core :as json] [compojure.core :as compojure :refer [POST]] [metabase.actions :as actions] [metabase.actions.execution :as actions.execution] @@ -195,6 +196,16 @@ (t2/update! Action id {:public_uuid nil, :made_public_by_id nil}) {:status 204, :body nil}) +(api/defendpoint GET "/:action-id/execute" + "Fetches the values for filling in execution parameters. Pass PK parameters and values to select." + [action-id parameters] + {action-id ms/PositiveInt + parameters ms/JSONString} + (actions/check-actions-enabled! action-id) + (-> (action/select-action :id action-id :archived false) + api/read-check + (actions.execution/fetch-values (json/parse-string parameters)))) + (api/defendpoint POST "/:id/execute" "Execute the Action. diff --git a/test/metabase/api/action_test.clj b/test/metabase/api/action_test.clj index a28ca55235f3013064f4576249a764b6a6e9e142..f0c4acb5c78d89221c300ccaae9e22b439da19cd 100644 --- a/test/metabase/api/action_test.clj +++ b/test/metabase/api/action_test.clj @@ -1,10 +1,12 @@ (ns metabase.api.action-test (:require + [cheshire.core :as json] [clojure.set :as set] [clojure.test :refer :all] [metabase.analytics.snowplow-test :as snowplow-test] [metabase.api.action :as api.action] [metabase.models :refer [Action Card Database]] + [metabase.models.collection :as collection] [metabase.models.user :as user] [metabase.test :as mt] [metabase.util :as u] @@ -588,3 +590,45 @@ (is (partial= {:message "No destination parameter found for #{\"name\"}. Found: #{\"last_login\" \"id\"}"} (mt/user-http-request :crowberto :post 400 (format "action/%s/execute" action-id) {:parameters {:name "Darth Vader"}}))))))))) + +(deftest fetch-implicit-action-default-values-test + (mt/test-drivers (mt/normal-drivers-with-feature :actions) + (mt/with-actions-enabled + (mt/with-actions [_ {:dataset true :dataset_query (mt/mbql-query venues {:fields [$id $name]}) + :collection_id (:id (collection/user->personal-collection (mt/user->id :crowberto)))} + {create-action-id :action-id} {:type :implicit :kind "row/create"} + {update-action-id :action-id} {:type :implicit :kind "row/update"} + {delete-action-id :action-id} {:type :implicit :kind "row/delete"} + {http-action-id :action-id} {:type :http} + {query-action-id :action-id} {:type :query}] + (testing "403 if user does not have permission to view the action" + (is (= "You don't have permissions to do that." + (mt/user-http-request :rasta :get 403 (format "action/%d/execute" update-action-id) :parameters (json/encode {:id 1}))))) + + (testing "404 if id does not exist" + (is (= "Not found." + (mt/user-http-request :rasta :get 404 (format "action/%d/execute" Integer/MAX_VALUE) :parameters (json/encode {:id 1}))))) + + (testing "returns empty map for actions that are not implicit" + (is (= {} + (mt/user-http-request :crowberto :get 200 (format "action/%d/execute" http-action-id) :parameters (json/encode {:id 1})))) + + (is (= {} + (mt/user-http-request :crowberto :get 200 (format "action/%d/execute" query-action-id) :parameters (json/encode {:id 1}))))) + + (testing "Can't fetch for create action" + (is (= "Values can only be fetched for actions that require a Primary Key." + (mt/user-http-request :crowberto :get 400 (format "action/%d/execute" create-action-id) :parameters (json/encode {:id 1}))))) + + (testing "fetch for update action return name and id" + (is (= {:id 1 :name "Red Medicine"} + (mt/user-http-request :crowberto :get 200 (format "action/%d/execute" update-action-id) :parameters (json/encode {:id 1}))))) + + (testing "fetch for delete action returns the id only" + (is (= {:id 1} + (mt/user-http-request :crowberto :get 200 (format "action/%d/execute" delete-action-id) :parameters (json/encode {:id 1}))))) + + (mt/with-actions-disabled + (testing "error if actions is disabled" + (is (= "Actions are not enabled." + (:message (mt/user-http-request :crowberto :get 400 (format "action/%d/execute" delete-action-id) :parameters (json/encode {:id 1})))))))))))