fix(web): install button and confirmation dialog fixes (#1717)

## Problems 1. The "Install" button is shown during the installation progress :disappointed_relieved: Thanks @imobachgs! 2. The install confirmation dialog could be accepted by mistake because of 'Continue' button receiving the focus by default https://trello.com/c/8ZWINXjs (internal/private link) ## Solutions 1. Do not show the "Install" button when it does not make sense 2. Set 'Cancel' button as focused by default via [autofocus](https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/autofocus) attribute (a.k.a. `autoFocus` prop) ## Testing - New behavior covered by unit tests. ## Notes Constants for router paths were moved to a single _routes/paths_ file in order to avoid circular dependencies that made test suite fail after fe18300
agama-project · Nov 4, 2024 · 7ab5690 · 7ab5690
2 parents b0d9d1b + 29068f1
commit 7ab5690
Show file tree

Hide file tree

Showing 30 changed files with 249 additions and 163 deletions.
diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes
@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Wed Oct 30 22:26:29 UTC 2024 - David Diaz <[email protected]>
+
+- Render Install button only where it makes sense and prevent the
+  user starting the installation by mistake
+  (gh#agama-project/agama#1717).
+
 -------------------------------------------------------------------
 Mon Oct 28 19:26:29 UTC 2024 - David Diaz <[email protected]>
 

diff --git a/web/src/App.tsx b/web/src/App.tsx
@@ -32,8 +32,7 @@ import { useL10nConfigChanges } from "~/queries/l10n";
 import { useIssuesChanges } from "~/queries/issues";
 import { useInstallerStatus, useInstallerStatusChanges } from "~/queries/status";
 import { useDeprecatedChanges } from "~/queries/storage";
-import { PATHS as PRODUCT_PATHS } from "~/routes/products";
-import { PATHS as ROOT_PATHS } from "~/router";
+import { ROOT, PRODUCT } from "~/routes/paths";
 import { InstallationPhase } from "~/types/status";
 
 /**
@@ -60,11 +59,11 @@ function App() {
       );
 
     if (phase === InstallationPhase.Install && isBusy) {
-      return <Navigate to={ROOT_PATHS.installationProgress} />;
+      return <Navigate to={ROOT.installationProgress} />;
     }
 
     if (phase === InstallationPhase.Install && !isBusy) {
-      return <Navigate to={ROOT_PATHS.installationFinished} />;
+      return <Navigate to={ROOT.installationFinished} />;
     }
 
     if (!products || !connected) return <Loading />;
@@ -77,16 +76,12 @@ function App() {
       );
     }
 
-    if (selectedProduct === undefined && location.pathname !== PRODUCT_PATHS.root) {
-      return <Navigate to={PRODUCT_PATHS.root} />;
+    if (selectedProduct === undefined && location.pathname !== PRODUCT.root) {
+      return <Navigate to={PRODUCT.root} />;
     }
 
-    if (
-      phase === InstallationPhase.Config &&
-      isBusy &&
-      location.pathname !== PRODUCT_PATHS.progress
-    ) {
-      return <Navigate to={PRODUCT_PATHS.progress} />;
+    if (phase === InstallationPhase.Config && isBusy && location.pathname !== PRODUCT.progress) {
+      return <Navigate to={PRODUCT.progress} />;
     }
 
     return <Outlet />;

diff --git a/web/src/components/core/ChangeProductLink.test.tsx b/web/src/components/core/ChangeProductLink.test.tsx
@@ -23,7 +23,7 @@
 import React from "react";
 import { screen } from "@testing-library/react";
 import { installerRender } from "~/test-utils";
-import { PATHS } from "~/routes/products";
+import { PRODUCT as PATHS } from "~/routes/paths";
 import { Product } from "~/types/software";
 import ChangeProductLink from "./ChangeProductLink";
 

diff --git a/web/src/components/core/ChangeProductLink.tsx b/web/src/components/core/ChangeProductLink.tsx
@@ -23,7 +23,7 @@
 import React from "react";
 import { Link, LinkProps } from "react-router-dom";
 import { useProduct } from "~/queries/software";
-import { PATHS } from "~/routes/products";
+import { PRODUCT as PATHS } from "~/routes/paths";
 import { _ } from "~/i18n";
 
 /**

diff --git a/web/src/components/core/InstallButton.test.tsx b/web/src/components/core/InstallButton.test.tsx
@@ -21,11 +21,11 @@
  */
 
 import React from "react";
-import { screen, waitFor } from "@testing-library/react";
+import { screen, waitFor, within } from "@testing-library/react";
 import { installerRender, mockRoutes } from "~/test-utils";
 import { InstallButton } from "~/components/core";
 import { IssuesList } from "~/types/issues";
-import { PATHS as PRODUCT_PATHS } from "~/routes/products";
+import { PRODUCT, ROOT } from "~/routes/paths";
 
 const mockStartInstallationFn = jest.fn();
 let mockIssuesList: IssuesList;
@@ -40,82 +40,119 @@ jest.mock("~/queries/issues", () => ({
   useAllIssues: () => mockIssuesList,
 }));
 
-describe("when there are installation issues", () => {
-  beforeEach(() => {
-    mockIssuesList = new IssuesList(
-      [
-        {
-          description: "Fake Issue",
-          source: 0,
-          severity: 0,
-          details: "Fake Issue details",
-        },
-      ],
-      [],
-      [],
-      [],
-    );
-  });
+const clickInstallButton = async () => {
+  const { user } = installerRender(<InstallButton />);
+  const button = await screen.findByRole("button", { name: "Install" });
+  await user.click(button);
 
-  it("renders nothing", () => {
-    const { container } = installerRender(<InstallButton />);
-    expect(container).toBeEmptyDOMElement();
-  });
-});
+  const dialog = screen.getByRole("dialog", { name: "Confirm Installation" });
+  return { user, dialog };
+};
 
-describe("when there are not installation issues", () => {
-  beforeEach(() => {
-    mockIssuesList = new IssuesList([], [], [], []);
-  });
+describe("InstallButton", () => {
+  describe("when there are installation issues", () => {
+    beforeEach(() => {
+      mockIssuesList = new IssuesList(
+        [
+          {
+            description: "Fake Issue",
+            source: 0,
+            severity: 0,
+            details: "Fake Issue details",
+          },
+        ],
+        [],
+        [],
+        [],
+      );
+    });
 
-  it("renders an Install button", () => {
-    installerRender(<InstallButton />);
-    screen.getByRole("button", { name: "Install" });
+    it("renders nothing", () => {
+      const { container } = installerRender(<InstallButton />);
+      expect(container).toBeEmptyDOMElement();
+    });
   });
 
-  it("starts the installation after user clicks on it and accept the confirmation", async () => {
-    const { user } = installerRender(<InstallButton />);
-    const button = await screen.findByRole("button", { name: "Install" });
-    await user.click(button);
+  describe("when there are not installation issues", () => {
+    beforeEach(() => {
+      mockIssuesList = new IssuesList([], [], [], []);
+    });
 
-    const continueButton = await screen.findByRole("button", { name: "Continue" });
-    await user.click(continueButton);
-    expect(mockStartInstallationFn).toHaveBeenCalled();
-  });
+    it("renders an Install button", () => {
+      installerRender(<InstallButton />);
+      screen.getByRole("button", { name: "Install" });
+    });
 
-  it("does not start the installation if the user clicks on it but cancels the confirmation", async () => {
-    const { user } = installerRender(<InstallButton />);
-    const button = await screen.findByRole("button", { name: "Install" });
-    await user.click(button);
+    it("renders a confirmation dialog when clicked", async () => {
+      const { user } = installerRender(<InstallButton />);
+      const button = await screen.findByRole("button", { name: "Install" });
+      await user.click(button);
 
-    const cancelButton = await screen.findByRole("button", { name: "Cancel" });
-    await user.click(cancelButton);
-    expect(mockStartInstallationFn).not.toHaveBeenCalled();
+      screen.getByRole("dialog", { name: "Confirm Installation" });
+    });
 
-    await waitFor(() => {
-      expect(screen.queryByRole("button", { name: "Continue" })).not.toBeInTheDocument();
+    describe.each([
+      ["login", ROOT.login],
+      ["product selection", PRODUCT.changeProduct],
+      ["product selection progress", PRODUCT.progress],
+      ["installation progress", ROOT.installationProgress],
+      ["installation finished", ROOT.installationFinished],
+    ])(`but the installer is rendering the %s screen`, (_, path) => {
+      beforeEach(() => {
+        mockRoutes(path);
+      });
+
+      it("renders nothing", () => {
+        const { container } = installerRender(<InstallButton />);
+        expect(container).toBeEmptyDOMElement();
+      });
     });
   });
+});
 
-  describe("but installer is rendering the product selection", () => {
-    beforeEach(() => {
-      mockRoutes(PRODUCT_PATHS.changeProduct);
+describe("InstallConfirmationPopup", () => {
+  it("closes the dialog without triggering installation if user press {enter} before 'Continue' gets the focus", async () => {
+    const { user, dialog } = await clickInstallButton();
+    const continueButton = within(dialog).getByRole("button", { name: "Continue" });
+    expect(continueButton).not.toHaveFocus();
+    await user.keyboard("{enter}");
+    expect(mockStartInstallationFn).not.toHaveBeenCalled();
+    await waitFor(() => {
+      expect(dialog).not.toBeInTheDocument();
     });
+  });
 
-    it("renders nothing", () => {
-      const { container } = installerRender(<InstallButton />);
-      expect(container).toBeEmptyDOMElement();
+  it("closes the dialog and triggers installation if user {enter} when 'Continue' has the focus", async () => {
+    const { user, dialog } = await clickInstallButton();
+    const continueButton = within(dialog).getByRole("button", { name: "Continue" });
+    expect(continueButton).not.toHaveFocus();
+    await user.keyboard("{tab}");
+    expect(continueButton).toHaveFocus();
+    await user.keyboard("{enter}");
+    expect(mockStartInstallationFn).toHaveBeenCalled();
+    await waitFor(() => {
+      expect(dialog).not.toBeInTheDocument();
     });
   });
 
-  describe("but installer is configuring a product", () => {
-    beforeEach(() => {
-      mockRoutes(PRODUCT_PATHS.progress);
+  it("closes the dialog and triggers installation if user clicks on 'Continue'", async () => {
+    const { user, dialog } = await clickInstallButton();
+    const continueButton = within(dialog).getByRole("button", { name: "Continue" });
+    await user.click(continueButton);
+    expect(mockStartInstallationFn).toHaveBeenCalled();
+    await waitFor(() => {
+      expect(dialog).not.toBeInTheDocument();
     });
+  });
 
-    it("renders nothing", () => {
-      const { container } = installerRender(<InstallButton />);
-      expect(container).toBeEmptyDOMElement();
+  it("closes the dialog without triggering installation if the user clicks on 'Cancel'", async () => {
+    const { user, dialog } = await clickInstallButton();
+    const cancelButton = within(dialog).getByRole("button", { name: "Cancel" });
+    await user.click(cancelButton);
+    expect(mockStartInstallationFn).not.toHaveBeenCalled();
+
+    await waitFor(() => {
+      expect(dialog).not.toBeInTheDocument();
     });
   });
 });
diff --git a/web/src/components/core/InstallButton.tsx b/web/src/components/core/InstallButton.tsx
@@ -29,7 +29,22 @@ import { _ } from "~/i18n";
 import { startInstallation } from "~/api/manager";
 import { useAllIssues } from "~/queries/issues";
 import { useLocation } from "react-router-dom";
-import { PATHS as PRODUCT_PATHS } from "~/routes/products";
+import { PRODUCT, ROOT } from "~/routes/paths";
+
+/**
+ * List of paths where the InstallButton must not be shown.
+ *
+ * Apart from obvious login and installation paths, it does not make sense to
+ * show the button neither, when the user is about to change the product nor
+ * when the installer is setting the chosen product.
+ * */
+const EXCLUDED_FROM = [
+  ROOT.login,
+  PRODUCT.changeProduct,
+  PRODUCT.progress,
+  ROOT.installationProgress,
+  ROOT.installationFinished,
+];
 
 const InstallConfirmationPopup = ({ onAccept, onClose }) => {
   return (
@@ -48,7 +63,7 @@ according to the provided installation settings.",
           {/* TRANSLATORS: button label */}
           {_("Continue")}
         </Popup.Confirm>
-        <Popup.Cancel onClick={onClose}>
+        <Popup.Cancel autoFocus onClick={onClose}>
           {/* TRANSLATORS: button label */}
           {_("Cancel")}
         </Popup.Cancel>
@@ -60,21 +75,26 @@ according to the provided installation settings.",
 /**
  * Installation button
  *
- * It starts the installation after asking for confirmation.
+ * It will be shown only if there aren't installation issues and the current
+ * path is not in the EXCLUDED_FROM list.
  *
+ * When clicked, it will ask for a confirmation before triggering the request
+ * for starting the installation.
  */
 const InstallButton = (props: Omit<ButtonProps, "onClick">) => {
   const issues = useAllIssues();
   const [isOpen, setIsOpen] = useState(false);
   const location = useLocation();
 
   if (!issues.isEmpty) return;
-  // Do not show the button if the user is about to change the product or the
-  // installer is configuring a product.
-  if ([PRODUCT_PATHS.changeProduct, PRODUCT_PATHS.progress].includes(location.pathname)) return;
+  if (EXCLUDED_FROM.includes(location.pathname)) return;
 
   const open = async () => setIsOpen(true);
   const close = () => setIsOpen(false);
+  const onAccept = () => {
+    close();
+    startInstallation();
+  };
 
   return (
     <>
@@ -83,7 +103,7 @@ const InstallButton = (props: Omit<ButtonProps, "onClick">) => {
         {_("Install")}
       </Button>
 
-      {isOpen && <InstallConfirmationPopup onAccept={startInstallation} onClose={close} />}
+      {isOpen && <InstallConfirmationPopup onAccept={onAccept} onClose={close} />}
     </>
   );
 };

diff --git a/web/src/components/core/InstallationFinished.tsx b/web/src/components/core/InstallationFinished.tsx
@@ -46,7 +46,7 @@ import { useProposalResult } from "~/queries/storage";
 import { finishInstallation } from "~/api/manager";
 import { InstallationPhase } from "~/types/status";
 import { Navigate } from "react-router-dom";
-import { PATHS } from "~/router";
+import { ROOT as PATHS } from "~/routes/paths";
 
 const TpmHint = () => {
   const [isExpanded, setIsExpanded] = useState(false);

diff --git a/web/src/components/core/InstallationProgress.tsx b/web/src/components/core/InstallationProgress.tsx
@@ -24,7 +24,7 @@ import React from "react";
 import { _ } from "~/i18n";
 import ProgressReport from "./ProgressReport";
 import { InstallationPhase } from "~/types/status";
-import { PATHS } from "~/router";
+import { ROOT as PATHS } from "~/routes/paths";
 import { Navigate } from "react-router-dom";
 import { useInstallerStatus } from "~/queries/status";
 

diff --git a/web/src/components/core/IssuesLink.test.tsx b/web/src/components/core/IssuesLink.test.tsx
@@ -25,7 +25,7 @@ import { screen } from "@testing-library/react";
 import { installerRender, mockRoutes } from "~/test-utils";
 import { IssuesLink } from "~/components/core";
 import { IssuesList } from "~/types/issues";
-import { PATHS as PRODUCT_PATHS } from "~/routes/products";
+import { PRODUCT as PATHS } from "~/routes/paths";
 
 const mockStartInstallationFn = jest.fn();
 let mockIssuesList: IssuesList;
@@ -64,7 +64,7 @@ describe("when there are installation issues", () => {
 
   describe("but installer is rendering the product selection", () => {
     beforeEach(() => {
-      mockRoutes(PRODUCT_PATHS.changeProduct);
+      mockRoutes(PATHS.changeProduct);
     });
 
     it("renders nothing", () => {
@@ -75,7 +75,7 @@ describe("when there are installation issues", () => {
 
   describe("but installer is configuring the product", () => {
     beforeEach(() => {
-      mockRoutes(PRODUCT_PATHS.progress);
+      mockRoutes(PATHS.progress);
     });
 
     it("renders nothing", () => {