sts-sponsors team mailing list archive

[Peter Makowski <mp+440024@xxxxxxxxxxxxxxxxxx>]

Peter Makowski has proposed merging ~petermakowski/maas-site-manager:enrollment-add-action-bar-MAASENG-1523 into maas-site-manager:main.

Commit message:
add enrollment action bar MAASENG-1523


Requested reviews:
  MAAS Committers (maas-committers)

For more details, see:
https://code.launchpad.net/~petermakowski/maas-site-manager/+git/site-manager/+merge/440024

This merge proposal adds functionality to the enrollment requests list view for handling Accept and Deny actions, as well as displaying error notifications when there is an issue processing enrollment requests.

QA Steps
- go to /requests
- select a few items on the list
- Click "Accept"
- open browser developer tools and go to network tab
- verify the network request has been made with correct parameters and a success notification is displayed
- right click on the "requests" and select an option to block the request URL 
- click "Accept" again
- Verify an error message has been displayed
-- 
Your team MAAS Committers is requested to review the proposed merge of ~petermakowski/maas-site-manager:enrollment-add-action-bar-MAASENG-1523 into maas-site-manager:main.

diff --git a/frontend/src/App.scss b/frontend/src/App.scss
index 9b245c7..5ff98f6 100644
--- a/frontend/src/App.scss
+++ b/frontend/src/App.scss
@@ -21,6 +21,7 @@
 @include vf-p-strip;
 @include vf-p-contextual-menu;
 @include vf-p-form-validation;
+@include vf-p-notification;
 
 // icons
 @include vf-p-icons;
diff --git a/frontend/src/api/handlers.test.ts b/frontend/src/api/handlers.test.ts
index 13381e2..0362c04 100644
--- a/frontend/src/api/handlers.test.ts
+++ b/frontend/src/api/handlers.test.ts
@@ -1,10 +1,13 @@
-import { postTokens } from "./handlers";
+import { setupServer } from "msw/node";
 
-import urls from "@/api/urls";
-import { createMockTokensResolver } from "@/mocks/resolvers";
-import { createMockPostServer } from "@/mocks/server";
+import { postEnrollmentRequests, postTokens } from "./handlers";
 
-const mockServer = createMockPostServer(urls.tokens, createMockTokensResolver());
+import {
+  postTokens as postTokensResolver,
+  postEnrollmentRequests as postEnrollmentRequestsResolver,
+} from "@/mocks/resolvers";
+
+const mockServer = setupServer(postTokensResolver, postEnrollmentRequestsResolver);
 
 beforeAll(() => {
   mockServer.listen();
@@ -20,5 +23,19 @@ describe("postTokens handler", () => {
   it("requires name, amount and expiration time", async () => {
     // @ts-expect-error
     await expect(postTokens({})).rejects.toThrowError();
+    await expect(postTokens({ amount: 1, expires: "P0Y0M7DT0H0M0S" })).resolves.toEqual(
+      expect.objectContaining({
+        items: expect.any(Array),
+      }),
+    );
+  });
+});
+
+describe("postEnrollmentRequests handler", () => {
+  it("requires ids and accept values", async () => {
+    // @ts-expect-error
+    await expect(postEnrollmentRequests({})).rejects.toThrowError();
+    await expect(postEnrollmentRequests({ ids: [], accept: false })).resolves.toEqual("");
+    await expect(postEnrollmentRequests({ ids: [], accept: true })).resolves.toEqual("");
   });
 });
diff --git a/frontend/src/api/handlers.ts b/frontend/src/api/handlers.ts
index 5a33c6f..93f2607 100644
--- a/frontend/src/api/handlers.ts
+++ b/frontend/src/api/handlers.ts
@@ -62,3 +62,19 @@ export const getEnrollmentRequests = async (params: GetEnrollmentRequestsQueryPa
     console.error(error);
   }
 };
+
+export type PostEnrollmentRequestsData = {
+  ids: string[];
+  accept: boolean;
+};
+export const postEnrollmentRequests = async (data: PostEnrollmentRequestsData) => {
+  if (!data?.ids || typeof data?.accept !== "boolean") {
+    throw Error("Missing required fields");
+  }
+  try {
+    const response = await api.post(urls.enrollmentRequests, data);
+    return response.data;
+  } catch (error) {
+    throw error;
+  }
+};
diff --git a/frontend/src/components/EnrollmentActions/EnrollmentActions.test.tsx b/frontend/src/components/EnrollmentActions/EnrollmentActions.test.tsx
new file mode 100644
index 0000000..fafd39a
--- /dev/null
+++ b/frontend/src/components/EnrollmentActions/EnrollmentActions.test.tsx
@@ -0,0 +1,30 @@
+import { vi } from "vitest";
+
+import EnrollmentActions from "./EnrollmentActions";
+
+import type * as apiHooks from "@/hooks/api";
+import { render, screen, within } from "@/test-utils";
+
+const enrollmentRequestsMutationMock = vi.fn();
+
+it("displays enrollment action buttons", () => {
+  render(<EnrollmentActions />);
+
+  expect(screen.getByRole("button", { name: /Deny/i })).toBeInTheDocument();
+  expect(screen.getByRole("button", { name: /Accept/i })).toBeInTheDocument();
+});
+
+it("can display an error message on request error", () => {
+  vi.mock("@/hooks/api", async (importOriginal) => {
+    const original: typeof apiHooks = await importOriginal();
+    return {
+      ...original,
+      useEnrollmentRequestsMutation: () => ({ mutate: enrollmentRequestsMutationMock, isError: true }),
+    };
+  });
+  render(<EnrollmentActions />);
+
+  expect(
+    within(screen.getByRole("alert")).getByText(/There was an error processing enrolment request/i),
+  ).toBeInTheDocument();
+});
diff --git a/frontend/src/components/EnrollmentActions/EnrollmentActions.tsx b/frontend/src/components/EnrollmentActions/EnrollmentActions.tsx
new file mode 100644
index 0000000..812ddad
--- /dev/null
+++ b/frontend/src/components/EnrollmentActions/EnrollmentActions.tsx
@@ -0,0 +1,48 @@
+import { Button, Notification } from "@canonical/react-components";
+
+import EnrollmentNotification from "./EnrollmentNotification";
+
+import { useAppContext } from "@/context";
+import { useEnrollmentRequestsMutation } from "@/hooks/api";
+
+const EnrollmentActions: React.FC = () => {
+  const { rowSelection, setRowSelection } = useAppContext();
+  const selectedIds = Object.keys(rowSelection).map((id) => id);
+  const enrollmentRequestsMutation = useEnrollmentRequestsMutation({ onSuccess: () => setRowSelection({}) });
+  const isActionDisabled = Object.keys(rowSelection).length === 0 || enrollmentRequestsMutation.isLoading;
+  const handleAccept = () =>
+    enrollmentRequestsMutation.mutate({
+      accept: true,
+      ids: selectedIds,
+    });
+  const handleDeny = () =>
+    enrollmentRequestsMutation.mutate({
+      accept: false,
+      ids: selectedIds,
+    });
+
+  return (
+    <>
+      <div className="u-fixed-width">
+        {enrollmentRequestsMutation.isSuccess ? (
+          <EnrollmentNotification {...enrollmentRequestsMutation.variables} />
+        ) : null}
+        {enrollmentRequestsMutation.isError ? (
+          <Notification role="alert" severity="negative">
+            There was an error processing enrolment request(s).
+          </Notification>
+        ) : null}
+        <div className="u-flex u-flex--justify-end">
+          <Button appearance="negative" disabled={isActionDisabled} onClick={handleDeny} type="button">
+            Deny
+          </Button>
+          <Button appearance="positive" disabled={isActionDisabled} onClick={handleAccept} type="button">
+            Accept
+          </Button>
+        </div>
+      </div>
+    </>
+  );
+};
+
+export default EnrollmentActions;
diff --git a/frontend/src/components/EnrollmentActions/EnrollmentNotification/EnrollmentNotification.tsx b/frontend/src/components/EnrollmentActions/EnrollmentNotification/EnrollmentNotification.tsx
new file mode 100644
index 0000000..afe4489
--- /dev/null
+++ b/frontend/src/components/EnrollmentActions/EnrollmentNotification/EnrollmentNotification.tsx
@@ -0,0 +1,21 @@
+import { Notification } from "@canonical/react-components";
+import { useNavigate } from "react-router-dom";
+
+import type { PostEnrollmentRequestsData } from "@/api/handlers";
+
+const EnrollmentNotification = ({ accept }: Partial<PostEnrollmentRequestsData>) => {
+  const navigate = useNavigate();
+  return (
+    <Notification
+      actions={[{ label: "Go to Regions", onClick: () => navigate("/sites") }]}
+      role="alert"
+      severity="information"
+      title={accept ? "Accepted" : "Denied"}
+    >
+      {accept ? "Accepted" : "Denied"} enrolment request for maas-example-region. See more data of this region in the
+      Regions page.
+    </Notification>
+  );
+};
+
+export default EnrollmentNotification;
diff --git a/frontend/src/components/EnrollmentActions/EnrollmentNotification/index.ts b/frontend/src/components/EnrollmentActions/EnrollmentNotification/index.ts
new file mode 100644
index 0000000..610c2dd
--- /dev/null
+++ b/frontend/src/components/EnrollmentActions/EnrollmentNotification/index.ts
@@ -0,0 +1 @@
+export { default } from "./EnrollmentNotification";
diff --git a/frontend/src/components/EnrollmentActions/index.ts b/frontend/src/components/EnrollmentActions/index.ts
new file mode 100644
index 0000000..f119ff3
--- /dev/null
+++ b/frontend/src/components/EnrollmentActions/index.ts
@@ -0,0 +1 @@
+export { default } from "./EnrollmentActions";
diff --git a/frontend/src/components/Notification/index.ts b/frontend/src/components/Notification/index.ts
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/frontend/src/components/Notification/index.ts
diff --git a/frontend/src/components/RequestsList/RequestsList.test.tsx b/frontend/src/components/RequestsList/RequestsList.test.tsx
new file mode 100644
index 0000000..9ad50bc
--- /dev/null
+++ b/frontend/src/components/RequestsList/RequestsList.test.tsx
@@ -0,0 +1,74 @@
+import { rest } from "msw";
+import { setupServer } from "msw/node";
+
+import RequestsList from "./RequestsList";
+
+import urls from "@/api/urls";
+import { enrollmentRequestFactory } from "@/mocks/factories";
+import { createMockGetEnrollmentRequestsResolver, postEnrollmentRequests } from "@/mocks/resolvers";
+import { renderWithMemoryRouter, screen, userEvent, waitFor, within } from "@/test-utils";
+
+const enrollmentRequest = enrollmentRequestFactory.build({ name: "new-maas-site" });
+const enrollmentRequests = [enrollmentRequest, ...enrollmentRequestFactory.buildList(2)];
+
+const mockServer = setupServer(
+  rest.get(urls.enrollmentRequests, createMockGetEnrollmentRequestsResolver(enrollmentRequests)),
+  postEnrollmentRequests,
+);
+
+beforeAll(() => {
+  mockServer.listen();
+});
+afterEach(() => {
+  mockServer.resetHandlers();
+});
+afterAll(() => {
+  mockServer.close();
+});
+
+it("displays a loading text", async () => {
+  const { rerender } = renderWithMemoryRouter(<RequestsList />);
+
+  const table = screen.getByRole("table", { name: /enrollment requests/i });
+  expect(table).toBeInTheDocument();
+  expect(within(table).getByText(/Loading/i)).toBeInTheDocument();
+
+  rerender(<RequestsList />);
+
+  await waitFor(() => expect(within(table).queryByText(/Loading/i)).not.toBeInTheDocument());
+});
+
+it("action buttons are disabled if no row is selected", async () => {
+  renderWithMemoryRouter(<RequestsList />);
+  expect(screen.getByRole("button", { name: /Accept/i })).toBeDisabled();
+  expect(screen.getByRole("button", { name: /Deny/i })).toBeDisabled();
+});
+
+it("action buttons are enabled if some rows are selected", async () => {
+  renderWithMemoryRouter(<RequestsList />);
+  await userEvent.click(screen.getByRole("checkbox", { name: /select all/i }));
+  await waitFor(() => expect(screen.getByRole("button", { name: /Accept/i })).toBeEnabled());
+  expect(screen.getByRole("button", { name: /Deny/i })).toBeEnabled();
+});
+
+it("displays a notification and clears selection if a region has been accepted", async () => {
+  renderWithMemoryRouter(<RequestsList />);
+  expect(screen.queryByRole("alert")).not.toBeInTheDocument();
+  const requestCheckbox = screen.getByRole("checkbox", { name: `select ${enrollmentRequest.name}` });
+  await userEvent.click(requestCheckbox);
+  expect(requestCheckbox).toBeChecked();
+  await userEvent.click(screen.getByRole("button", { name: /Accept/i }));
+  expect(await screen.findByRole("alert")).toBeInTheDocument();
+  expect(requestCheckbox).not.toBeChecked();
+});
+
+it("displays a notification and clears selection if a region has been denied", async () => {
+  renderWithMemoryRouter(<RequestsList />);
+  expect(screen.queryByRole("alert")).not.toBeInTheDocument();
+  const requestCheckbox = screen.getByRole("checkbox", { name: `select ${enrollmentRequest.name}` });
+  await userEvent.click(requestCheckbox);
+  expect(requestCheckbox).toBeChecked();
+  await userEvent.click(screen.getByRole("button", { name: /Deny/i }));
+  expect(await screen.findByRole("alert")).toBeInTheDocument();
+  expect(requestCheckbox).not.toBeChecked();
+});
diff --git a/frontend/src/components/RequestsList/RequestsList.tsx b/frontend/src/components/RequestsList/RequestsList.tsx
new file mode 100644
index 0000000..da895ed
--- /dev/null
+++ b/frontend/src/components/RequestsList/RequestsList.tsx
@@ -0,0 +1,27 @@
+import { useState } from "react";
+
+import { Col, Row } from "@canonical/react-components";
+
+import EnrollmentActions from "@/components/EnrollmentActions";
+import RequestsTable from "@/components/RequestsTable";
+import { useRequestsQuery } from "@/hooks/api";
+
+const Requests: React.FC = () => {
+  // TODO: update page and size when pagination is implemented
+  // https://warthogs.atlassian.net/browse/MAASENG-1525
+  const [page] = useState<number>(0);
+  const [size] = useState<number>(50);
+  const { data, isLoading, isFetchedAfterMount } = useRequestsQuery({ page: `${page}`, size: `${size}` });
+  return (
+    <section>
+      <EnrollmentActions />
+      <Row>
+        <Col size={12}>
+          <RequestsTable data={data} isFetchedAfterMount={isFetchedAfterMount} isLoading={isLoading} />
+        </Col>
+      </Row>
+    </section>
+  );
+};
+
+export default Requests;
diff --git a/frontend/src/components/RequestsList/index.ts b/frontend/src/components/RequestsList/index.ts
new file mode 100644
index 0000000..12f3486
--- /dev/null
+++ b/frontend/src/components/RequestsList/index.ts
@@ -0,0 +1 @@
+export { default } from "./RequestsList";
diff --git a/frontend/src/components/SitesList/SitesList.test.tsx b/frontend/src/components/SitesList/SitesList.test.tsx
index 97abb01..477c817 100644
--- a/frontend/src/components/SitesList/SitesList.test.tsx
+++ b/frontend/src/components/SitesList/SitesList.test.tsx
@@ -43,7 +43,7 @@ it("remove button is disabled if no row is selected", async () => {
   expect(screen.getByRole("button", { name: /Remove/i })).toBeDisabled();
 });
 
-it("remove button is enabled if a row is selected", async () => {
+it("remove button is enabled if some rows are selected", async () => {
   renderWithMemoryRouter(<SitesList />);
   await userEvent.click(screen.getByRole("checkbox", { name: /select all/i }));
   await waitFor(() => expect(screen.getByRole("button", { name: /Remove/i })).toBeEnabled());
diff --git a/frontend/src/hooks/api.ts b/frontend/src/hooks/api.ts
index 0448ea5..9627dee 100644
--- a/frontend/src/hooks/api.ts
+++ b/frontend/src/hooks/api.ts
@@ -1,7 +1,13 @@
+import type { UseMutationOptions } from "@tanstack/react-query";
 import { useMutation, useQuery } from "@tanstack/react-query";
 
-import type { GetEnrollmentRequestsQueryParams, GetSitesQueryParams, GetTokensQueryParams } from "@/api/handlers";
-import { getEnrollmentRequests, postTokens, getSites, getTokens } from "@/api/handlers";
+import type {
+  GetEnrollmentRequestsQueryParams,
+  GetSitesQueryParams,
+  GetTokensQueryParams,
+  PostEnrollmentRequestsData,
+} from "@/api/handlers";
+import { postEnrollmentRequests, getEnrollmentRequests, postTokens, getSites, getTokens } from "@/api/handlers";
 import type { SitesQueryResult, PostTokensResult, EnrollmentRequestsQueryResult } from "@/api/types";
 
 export type UseSitesQueryResult = ReturnType<typeof useSitesQuery>;
@@ -34,3 +40,7 @@ export const useRequestsQuery = ({ page, size }: GetEnrollmentRequestsQueryParam
     keepPreviousData: true,
     refetchInterval: defaultRefetchInterval,
   });
+
+export const useEnrollmentRequestsMutation = (
+  options: UseMutationOptions<unknown, unknown, PostEnrollmentRequestsData, unknown>,
+) => useMutation(postEnrollmentRequests, options);
diff --git a/frontend/src/mocks/browser.ts b/frontend/src/mocks/browser.ts
index 200b1ae..47fcf77 100644
--- a/frontend/src/mocks/browser.ts
+++ b/frontend/src/mocks/browser.ts
@@ -1,5 +1,5 @@
 import { setupWorker } from "msw";
 
-import { getSites, getTokens, getEnrollmentRequests, postTokens } from "./resolvers";
+import { getSites, getTokens, getEnrollmentRequests, postEnrollmentRequests, postTokens } from "./resolvers";
 
-export const worker = setupWorker(getSites, postTokens, getEnrollmentRequests, getTokens);
+export const worker = setupWorker(getSites, postTokens, getEnrollmentRequests, postEnrollmentRequests, getTokens);
diff --git a/frontend/src/mocks/resolvers.ts b/frontend/src/mocks/resolvers.ts
index 2a0b3fb..f1c14ec 100644
--- a/frontend/src/mocks/resolvers.ts
+++ b/frontend/src/mocks/resolvers.ts
@@ -80,7 +80,19 @@ export const createMockGetEnrollmentRequestsResolver =
     return res(ctx.json(response));
   };
 
+type PostEnrollmentRequestsResponseResolver = ResponseResolver<RestRequest<PostTokensData>, typeof restContext>;
+export const createMockPostEnrollmentRequestsResolver =
+  (): PostEnrollmentRequestsResponseResolver => async (req, res, ctx) => {
+    const { ids, accept } = await req.json();
+    if (ids && typeof accept === "boolean") {
+      return res(ctx.status(204));
+    } else {
+      return res(ctx.status(400));
+    }
+  };
+
 export const getSites = rest.get(urls.sites, createMockSitesResolver());
 export const postTokens = rest.post(urls.tokens, createMockTokensResolver());
 export const getTokens = rest.get(urls.tokens, createMockGetTokensResolver());
 export const getEnrollmentRequests = rest.get(urls.enrollmentRequests, createMockGetEnrollmentRequestsResolver());
+export const postEnrollmentRequests = rest.post(urls.enrollmentRequests, createMockPostEnrollmentRequestsResolver());
diff --git a/frontend/src/pages/requests.tsx b/frontend/src/pages/requests.tsx
index 1b54444..d75f28d 100644
--- a/frontend/src/pages/requests.tsx
+++ b/frontend/src/pages/requests.tsx
@@ -1,30 +1,5 @@
-import { useState } from "react";
+import RequestsList from "@/components/RequestsList";
 
-import { Col, Row } from "@canonical/react-components";
-
-import RequestsTable from "@/components/RequestsTable";
-import { useRequestsQuery } from "@/hooks/api";
-
-const Requests: React.FC = () => {
-  // TODO: update page and size when pagination is implemented
-  // https://warthogs.atlassian.net/browse/MAASENG-1525
-  const [page] = useState<number>(0);
-  const [size] = useState<number>(50);
-  const { data, isLoading, isFetchedAfterMount } = useRequestsQuery({ page: `${page}`, size: `${size}` });
-  return (
-    <section>
-      <Row>
-        <Col size={2}>
-          <h2 className="p-heading--4">Requests</h2>
-        </Col>
-      </Row>
-      <Row>
-        <Col size={12}>
-          <RequestsTable data={data} isFetchedAfterMount={isFetchedAfterMount} isLoading={isLoading} />
-        </Col>
-      </Row>
-    </section>
-  );
-};
+const Requests: React.FC = () => <RequestsList />;
 
 export default Requests;
diff --git a/frontend/tests/requests.spec.ts b/frontend/tests/requests.spec.ts
new file mode 100644
index 0000000..9b59012
--- /dev/null
+++ b/frontend/tests/requests.spec.ts
@@ -0,0 +1,13 @@
+import { test, expect } from "@playwright/test";
+
+test.beforeEach(async ({ page }) => {
+  await page.goto("/requests");
+});
+
+test("goes to the regions page if the user clicks on the regions link", async ({ page }) => {
+  await expect(page.getByRole("heading", { name: /regions/i })).toBeHidden();
+  await page.getByRole("checkbox", { name: "select all" }).click({ force: true });
+  await page.getByRole("button", { name: /Accept/i }).click();
+  await page.getByRole("button", { name: /Go to Regions/i }).click();
+  await expect(page.getByRole("heading", { name: /regions/i })).toBeVisible();
+});