From 5abdb50660c7b978627bed3884205a52520ad565 Mon Sep 17 00:00:00 2001 From: Vladimir Cucu <108150922+vladimir-cucu@users.noreply.github.com> Date: Wed, 3 Jul 2024 10:48:59 +0300 Subject: [PATCH] WD-11664 - refactor: remove nested components in ActionsPanel (#1786) --- src/panels/ActionsPanel/ActionsPanel.test.tsx | 59 +++---- src/panels/ActionsPanel/ActionsPanel.tsx | 99 +++--------- .../ConfirmationDialog.test.tsx | 144 ++++++++++++++++++ .../ConfirmationDialog/ConfirmationDialog.tsx | 90 +++++++++++ .../ActionsPanel/ConfirmationDialog/index.ts | 2 + .../ActionsPanel/ConfirmationDialog/types.ts | 5 + src/panels/ActionsPanel/types.ts | 8 +- 7 files changed, 284 insertions(+), 123 deletions(-) create mode 100644 src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.test.tsx create mode 100644 src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.tsx create mode 100644 src/panels/ActionsPanel/ConfirmationDialog/index.ts create mode 100644 src/panels/ActionsPanel/ConfirmationDialog/types.ts diff --git a/src/panels/ActionsPanel/ActionsPanel.test.tsx b/src/panels/ActionsPanel/ActionsPanel.test.tsx index 957d73574..d004a9971 100644 --- a/src/panels/ActionsPanel/ActionsPanel.test.tsx +++ b/src/panels/ActionsPanel/ActionsPanel.test.tsx @@ -19,7 +19,8 @@ import { import { renderComponent } from "testing/utils"; import ActionsPanel from "./ActionsPanel"; -import { Label } from "./types"; +import { ConfirmationDialogLabel } from "./ConfirmationDialog"; +import { Label as ActionsPanelLabel } from "./types"; const mockResponse = applicationsCharmActionsResultsFactory.build({ results: [ @@ -120,11 +121,13 @@ describe("ActionsPanel", () => { state, }); expect(await screen.findByRole("heading")).toHaveTextContent( - Label.NO_UNITS_SELECTED, + ActionsPanelLabel.NO_UNITS_SELECTED, ); expect( await screen.findByTestId("actions-panel-unit-list"), - ).toHaveTextContent(`Run action on: ${Label.NO_UNITS_SELECTED}`); + ).toHaveTextContent( + `Run action on: ${ActionsPanelLabel.NO_UNITS_SELECTED}`, + ); }); it("disables the submit button if no units are selected", async () => { @@ -262,7 +265,9 @@ describe("ActionsPanel", () => { await screen.findByRole("button", { name: "Run action" }), ); await userEvent.click( - await screen.findByRole("button", { name: Label.CONFIRM_BUTTON }), + await screen.findByRole("button", { + name: ConfirmationDialogLabel.CONFIRM_BUTTON, + }), ); const call = executeActionOnUnitsSpy.mock.calls[0]; expect(call[0]).toEqual(["ceph/0", "ceph/1"]); @@ -293,7 +298,9 @@ describe("ActionsPanel", () => { await screen.findByRole("button", { name: "Run action" }), ); await userEvent.click( - await screen.findByRole("button", { name: Label.CONFIRM_BUTTON }), + await screen.findByRole("button", { + name: ConfirmationDialogLabel.CONFIRM_BUTTON, + }), ); const call = executeActionOnUnitsSpy.mock.calls[0]; expect(call[0]).toEqual(["ceph/0", "ceph/1"]); @@ -305,32 +312,6 @@ describe("ActionsPanel", () => { executeActionOnUnitsSpy.mockRestore(); }); - it("should cancel the run selected action confirmation modal", async () => { - renderComponent(, { path, url, state }); - await userEvent.click( - await screen.findByRole("radio", { name: "add-disk" }), - ); - await userEvent.type( - await screen.findByRole("textbox", { name: "osd-devices" }), - "new device", - ); - expect( - await screen.findByRole("button", { name: "Run action" }), - ).not.toBeDisabled(); - await userEvent.click( - await screen.findByRole("button", { name: "Run action" }), - ); - expect( - screen.queryByRole("dialog", { name: "Run add-disk?" }), - ).toBeInTheDocument(); - await userEvent.click( - await screen.findByRole("button", { name: Label.CANCEL_BUTTON }), - ); - expect( - screen.queryByRole("dialog", { name: "Run add-disk?" }), - ).not.toBeInTheDocument(); - }); - it("should display error when trying to get actions for application", async () => { const getActionsForApplicationSpy = vi .fn() @@ -348,12 +329,12 @@ describe("ActionsPanel", () => { expect(getActionsForApplicationSpy).toHaveBeenCalledTimes(1), ); expect(console.error).toHaveBeenCalledWith( - Label.GET_ACTIONS_ERROR, + ActionsPanelLabel.GET_ACTIONS_ERROR, new Error("Error while trying to get actions for application!"), ); await waitFor(() => expect( - screen.getByText(Label.GET_ACTIONS_ERROR, { exact: false }), + screen.getByText(ActionsPanelLabel.GET_ACTIONS_ERROR, { exact: false }), ).toBeInTheDocument(), ); await userEvent.click( @@ -387,18 +368,18 @@ describe("ActionsPanel", () => { await screen.findByRole("button", { name: "Run action" }), ); await userEvent.click( - await screen.findByRole("button", { name: Label.CONFIRM_BUTTON }), + await screen.findByRole("button", { + name: ConfirmationDialogLabel.CONFIRM_BUTTON, + }), ); await waitFor(() => expect(executeActionOnUnitsSpy).toHaveBeenCalledTimes(1), ); - expect(console.error).toHaveBeenCalledWith( - Label.EXECUTE_ACTION_ERROR, - new Error("Error while trying to execute action on units!"), - ); await waitFor(() => expect( - screen.getByText(Label.EXECUTE_ACTION_ERROR, { exact: false }), + screen.getByText(ConfirmationDialogLabel.EXECUTE_ACTION_ERROR, { + exact: false, + }), ).toBeInTheDocument(), ); }); diff --git a/src/panels/ActionsPanel/ActionsPanel.tsx b/src/panels/ActionsPanel/ActionsPanel.tsx index 5461386aa..7a8534475 100644 --- a/src/panels/ActionsPanel/ActionsPanel.tsx +++ b/src/panels/ActionsPanel/ActionsPanel.tsx @@ -1,12 +1,7 @@ -import { - ActionButton, - Button, - ConfirmationModal, -} from "@canonical/react-components"; +import { ActionButton, Button } from "@canonical/react-components"; import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useSelector } from "react-redux"; import { useParams } from "react-router-dom"; -import usePortal from "react-useportal"; import CharmIcon from "components/CharmIcon"; import LoadingHandler from "components/LoadingHandler"; @@ -14,10 +9,7 @@ import Panel from "components/Panel"; import RadioInputBox from "components/RadioInputBox"; import type { EntityDetailsRoute } from "components/Routes"; import useInlineErrors from "hooks/useInlineErrors"; -import { - useExecuteActionOnUnits, - useGetActionsForApplication, -} from "juju/api-hooks"; +import { useGetActionsForApplication } from "juju/api-hooks"; import PanelInlineErrors from "panels/PanelInlineErrors"; import { usePanelQueryParams } from "panels/hooks"; import { ConfirmType, type ConfirmTypes } from "panels/types"; @@ -27,12 +19,13 @@ import type { RootState } from "store/store"; import { useAppStore } from "store/store"; import ActionOptions from "./ActionOptions"; +import ConfirmationDialog from "./ConfirmationDialog"; import type { ActionData, ActionOptionValue, ActionOptionValues, } from "./types"; -import { Label, TestId } from "./types"; +import { InlineErrors, Label, TestId } from "./types"; import { enableSubmit, onValuesChange } from "./utils"; type SetSelectedAction = (actionName: string) => void; @@ -42,11 +35,6 @@ type ActionsQueryParams = { units?: string[]; }; -enum InlineErrors { - GET_ACTION = "get-action", - EXECUTE_ACTION = "execute-action", -} - export default function ActionsPanel(): JSX.Element { const appStore = useAppStore(); const appState = appStore.getState(); @@ -66,7 +54,6 @@ export default function ActionsPanel(): JSX.Element { userName, modelName, ); - const executeActionOnUnits = useExecuteActionOnUnits(userName, modelName); const [selectedAction, setSelectedAction]: [ string | undefined, SetSelectedAction, @@ -89,7 +76,6 @@ export default function ActionsPanel(): JSX.Element { }); const [isExecutingAction, setIsExecutingAction] = useState(false); const scrollArea = useRef(null); - const { Portal } = usePortal(); const actionOptionsValues = useRef({}); @@ -152,16 +138,6 @@ export default function ActionsPanel(): JSX.Element { ); }; - const executeAction = async () => { - // You shouldn't be able to get this far without this defined but jic. - if (!selectedAction || !modelUUID) return; - await executeActionOnUnits( - selectedUnits, - selectedAction, - actionOptionsValues.current[selectedAction], - ); - }; - const handleSubmit = () => { setConfirmType(ConfirmType.SUBMIT); }; @@ -194,58 +170,6 @@ export default function ActionsPanel(): JSX.Element { [actionData, selectedUnits], ); - const generateConfirmationModal = () => { - if (confirmType && selectedAction) { - // Allow for adding more confirmation types, like for cancel - // if inputs have been changed. - if (confirmType === ConfirmType.SUBMIT) { - const unitNames = selectedUnits.reduce((acc, unitName) => { - return `${acc}, ${unitName.split("/")[1]}`; - }); - // Render the submit confirmation modal. - return ( - - { - setConfirmType(null); - setIsExecutingAction(true); - executeAction() - .then(() => { - handleRemovePanelQueryParams(); - return; - }) - .catch((error) => { - setInlineErrors( - InlineErrors.EXECUTE_ACTION, - Label.EXECUTE_ACTION_ERROR, - ); - setIsExecutingAction(false); - console.error(Label.EXECUTE_ACTION_ERROR, error); - }); - }} - close={() => setConfirmType(null)} - > -

- UNIT COUNT -

-

- {selectedUnits.length} -

-

- UNIT NAME -

-

{unitNames}

- - - ); - } - } - }; - const data = Object.keys(actionData).length > 0 ? actionData : null; return ( @@ -298,7 +222,20 @@ export default function ActionsPanel(): JSX.Element { ))} - {generateConfirmationModal()} + {selectedAction && confirmType ? ( + + ) : null} ); } diff --git a/src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.test.tsx b/src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.test.tsx new file mode 100644 index 000000000..8c6991ebe --- /dev/null +++ b/src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.test.tsx @@ -0,0 +1,144 @@ +import { screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { vi } from "vitest"; + +import * as actionsHooks from "juju/api-hooks/actions"; +import { ConfirmType } from "panels/types"; +import { renderComponent } from "testing/utils"; + +import { InlineErrors } from "../types"; + +import ConfirmationDialog from "./ConfirmationDialog"; +import { Label } from "./types"; + +describe("ConfirmationDialog", () => { + const path = "/models/:userName/:modelName/app/:appName"; + const url = + "/models/user-eggman@external/group-test/app/kubernetes-master?panel=execute-action&units=ceph%2F0,ceph%2F1"; + + afterEach(() => { + vi.resetModules(); + vi.restoreAllMocks(); + }); + + it("should return null if confirm type is not submit", async () => { + const { + result: { container }, + } = renderComponent( + , + ); + expect(container.children.length).toBe(1); + expect(container.firstChild).toBeEmptyDOMElement(); + }); + + it("should display submit confirmation dialog and can cancel running selected action", async () => { + const mockSetConfirmType = vi.fn(); + renderComponent( + , + { path, url }, + ); + expect( + screen.getByRole("dialog", { name: "Run pause?" }), + ).toBeInTheDocument(); + const cancelButton = screen.getByRole("button", { + name: Label.CANCEL_BUTTON, + }); + expect(cancelButton).toBeInTheDocument(); + expect(screen.getByText("2")).toBeInTheDocument(); + expect(screen.getByText("ceph/0, 1")).toBeInTheDocument(); + await userEvent.click(cancelButton); + expect(mockSetConfirmType).toHaveBeenCalledWith(null); + }); + + it("should run selected action and remove panel query params", async () => { + const mockSetConfirmType = vi.fn(); + const mockOnRemovePanelQueryParams = vi.fn(); + const mockSelectedActionOptionValue = { + bucket: "", + "osd-devices": "new device", + }; + const executeActionOnUnitsSpy = vi + .fn() + .mockImplementation(() => Promise.resolve()); + vi.spyOn(actionsHooks, "useExecuteActionOnUnits").mockImplementation( + () => executeActionOnUnitsSpy, + ); + renderComponent( + , + { path, url }, + ); + await userEvent.click( + screen.getByRole("button", { name: Label.CONFIRM_BUTTON }), + ); + expect(mockSetConfirmType).toHaveBeenCalledWith(null); + const call = executeActionOnUnitsSpy.mock.calls[0]; + expect(call[0]).toEqual(["ceph/0", "ceph/1"]); + expect(call[1]).toBe("pause"); + expect(call[2]).toEqual(mockSelectedActionOptionValue); + expect(mockOnRemovePanelQueryParams).toHaveBeenCalledOnce(); + }); + + it("should set inline error if execute action fails", async () => { + const consoleError = console.error; + console.error = vi.fn(); + const mockSetInlineErrors = vi.fn(); + const executeActionOnUnitsSpy = vi + .fn() + .mockImplementation(() => Promise.reject(new Error("mock error"))); + vi.spyOn(actionsHooks, "useExecuteActionOnUnits").mockImplementation( + () => executeActionOnUnitsSpy, + ); + renderComponent( + , + { path, url }, + ); + await userEvent.click( + screen.getByRole("button", { name: Label.CONFIRM_BUTTON }), + ); + expect(mockSetInlineErrors).toHaveBeenCalledWith( + InlineErrors.EXECUTE_ACTION, + Label.EXECUTE_ACTION_ERROR, + ); + expect(console.error).toHaveBeenCalledWith( + Label.EXECUTE_ACTION_ERROR, + new Error("mock error"), + ); + console.error = consoleError; + }); +}); diff --git a/src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.tsx b/src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.tsx new file mode 100644 index 000000000..c72debfeb --- /dev/null +++ b/src/panels/ActionsPanel/ConfirmationDialog/ConfirmationDialog.tsx @@ -0,0 +1,90 @@ +import { ConfirmationModal } from "@canonical/react-components"; +import { useParams } from "react-router-dom"; +import usePortal from "react-useportal"; + +import type { EntityDetailsRoute } from "components/Routes"; +import { type SetError } from "hooks/useInlineErrors"; +import { useExecuteActionOnUnits } from "juju/api-hooks"; +import type { ConfirmTypes } from "panels/types"; +import { ConfirmType } from "panels/types"; + +import { InlineErrors, type ActionOptionValue } from "../types"; + +import { Label } from "./types"; + +type Props = { + confirmType: ConfirmType; + selectedAction: string; + selectedUnits: string[]; + setConfirmType: React.Dispatch>; + setIsExecutingAction: React.Dispatch>; + selectedActionOptionValue: ActionOptionValue; + handleRemovePanelQueryParams: () => void; + setInlineErrors: SetError; +}; + +const ConfirmationDialog = ({ + confirmType, + selectedAction, + selectedUnits, + setConfirmType, + setIsExecutingAction, + selectedActionOptionValue, + handleRemovePanelQueryParams, + setInlineErrors, +}: Props): JSX.Element | null => { + const { Portal } = usePortal(); + const { modelName, userName } = useParams(); + const executeActionOnUnits = useExecuteActionOnUnits(userName, modelName); + + if (confirmType === ConfirmType.SUBMIT) { + const unitNames = selectedUnits.reduce((acc, unitName) => { + return `${acc}, ${unitName.split("/")[1]}`; + }); + // Render the submit confirmation modal. + return ( + + { + setConfirmType(null); + setIsExecutingAction(true); + executeActionOnUnits( + selectedUnits, + selectedAction, + selectedActionOptionValue, + ) + .then(() => { + handleRemovePanelQueryParams(); + return; + }) + .catch((error) => { + setInlineErrors( + InlineErrors.EXECUTE_ACTION, + Label.EXECUTE_ACTION_ERROR, + ); + setIsExecutingAction(false); + console.error(Label.EXECUTE_ACTION_ERROR, error); + }); + }} + close={() => setConfirmType(null)} + > +

UNIT COUNT

+

+ {selectedUnits.length} +

+

+ UNIT NAME +

+

{unitNames}

+ + + ); + } + return null; +}; + +export default ConfirmationDialog; diff --git a/src/panels/ActionsPanel/ConfirmationDialog/index.ts b/src/panels/ActionsPanel/ConfirmationDialog/index.ts new file mode 100644 index 000000000..dcb77806f --- /dev/null +++ b/src/panels/ActionsPanel/ConfirmationDialog/index.ts @@ -0,0 +1,2 @@ +export { default } from "./ConfirmationDialog"; +export { Label as ConfirmationDialogLabel } from "./types"; diff --git a/src/panels/ActionsPanel/ConfirmationDialog/types.ts b/src/panels/ActionsPanel/ConfirmationDialog/types.ts new file mode 100644 index 000000000..2ea7dd6dc --- /dev/null +++ b/src/panels/ActionsPanel/ConfirmationDialog/types.ts @@ -0,0 +1,5 @@ +export enum Label { + CANCEL_BUTTON = "Cancel", + CONFIRM_BUTTON = "Confirm", + EXECUTE_ACTION_ERROR = "Couldn't start the action.", +} diff --git a/src/panels/ActionsPanel/types.ts b/src/panels/ActionsPanel/types.ts index 5aac3c20d..0eeaafdc1 100644 --- a/src/panels/ActionsPanel/types.ts +++ b/src/panels/ActionsPanel/types.ts @@ -1,12 +1,9 @@ import type { ActionSpec } from "@canonical/jujulib/dist/api/facades/action/ActionV7"; export enum Label { - CANCEL_BUTTON = "Cancel", - CONFIRM_BUTTON = "Confirm", NO_UNITS_SELECTED = "0 units selected", NO_ACTIONS_PROVIDED = "This charm has not provided any actions.", GET_ACTIONS_ERROR = "Unable to get actions for application.", - EXECUTE_ACTION_ERROR = "Couldn't start the action.", } export enum TestId { @@ -53,3 +50,8 @@ export type OnValuesChange = ( actionName: string, options: ActionOptionValue, ) => void; + +export enum InlineErrors { + GET_ACTION = "get-action", + EXECUTE_ACTION = "execute-action", +}