sts-sponsors team mailing list archive

[Peter Makowski <mp+440258@xxxxxxxxxxxxxxxxxx>]

Looks good, just a few comments below that would be good to address.

Diff comments:

> diff --git a/frontend/src/components/NoRegions/NoRegions.test.tsx b/frontend/src/components/NoRegions/NoRegions.test.tsx
> new file mode 100644
> index 0000000..2824a99
> --- /dev/null
> +++ b/frontend/src/components/NoRegions/NoRegions.test.tsx
> @@ -0,0 +1,90 @@
> +import NoRegions from "./NoRegions";
> +
> +import urls from "@/api/urls";
> +import { enrollmentRequestFactory } from "@/mocks/factories";
> +import { createMockGetEnrollmentRequestsResolver } from "@/mocks/resolvers";
> +import { createMockGetServer } from "@/mocks/server";
> +import { renderWithMemoryRouter, screen, waitFor } from "@/test-utils";
> +
> +describe("open enrollment requests available", () => {

This is missing a test that the correct number of enrollment requests is displayed.

> +  const enrollmentRequests = enrollmentRequestFactory.buildList(2);
> +  const mockServer = createMockGetServer(
> +    urls.enrollmentRequests,
> +    createMockGetEnrollmentRequestsResolver(enrollmentRequests),
> +  );
> +
> +  beforeAll(() => {
> +    mockServer.listen();
> +  });
> +  afterEach(() => {
> +    mockServer.resetHandlers();
> +  });
> +  afterAll(() => {
> +    mockServer.close();
> +  });
> +
> +  it("should display 'no enrolled regions' text", () => {
> +    renderWithMemoryRouter(<NoRegions />);
> +
> +    expect(screen.getByText(/no enroled maas regions/i)).toBeInTheDocument();
> +  });
> +  it("should display link to enrollment docs", () => {
> +    renderWithMemoryRouter(<NoRegions />);
> +
> +    expect(
> +      screen.getByRole("link", { name: /learn more about the enrolment process in the documentation\./i }),
> +    ).toBeInTheDocument();
> +  });
> +
> +  it("should display a link to the request page if there are open requests", async () => {
> +    renderWithMemoryRouter(<NoRegions />);
> +
> +    await waitFor(() =>
> +      expect(
> +        screen.getByRole("link", {
> +          name: /go to requests page/i,
> +        }),
> +      ).toBeInTheDocument(),
> +    );
> +  });
> +});
> +
> +describe("no open enrollment requests available", () => {
> +  const enrollmentRequests = enrollmentRequestFactory.buildList(0);
> +  const mockServer = createMockGetServer(
> +    urls.enrollmentRequests,
> +    createMockGetEnrollmentRequestsResolver(enrollmentRequests),
> +  );
> +
> +  beforeAll(() => {
> +    mockServer.listen();
> +  });
> +  afterEach(() => {
> +    mockServer.resetHandlers();
> +  });
> +  afterAll(() => {
> +    mockServer.close();
> +  });
> +
> +  it("should display a link to the tokens page", async () => {
> +    renderWithMemoryRouter(<NoRegions />);
> +
> +    await waitFor(() =>
> +      expect(
> +        screen.getByRole("link", {
> +          name: /go to tokens page/i,
> +        }),
> +      ).toBeInTheDocument(),
> +    );
> +  });
> +
> +  it("should display a link to enrollment process docs", () => {
> +    renderWithMemoryRouter(<NoRegions />);
> +
> +    expect(
> +      screen.getByRole("link", {
> +        name: new RegExp("Learn more about the enrolment process in the documentation.", "i"),
> +      }),
> +    ).toBeInTheDocument();
> +  });
> +});
> diff --git a/frontend/src/components/NoRegions/NoRegions.tsx b/frontend/src/components/NoRegions/NoRegions.tsx
> new file mode 100644
> index 0000000..40e4741
> --- /dev/null
> +++ b/frontend/src/components/NoRegions/NoRegions.tsx
> @@ -0,0 +1,54 @@
> +import { useState } from "react";
> +
> +import { Link } from "react-router-dom";
> +
> +import ExternalLink from "../ExternalLink";
> +import TableCaption from "../TableCaption";
> +
> +import docsUrls from "@/base/docsUrls";
> +import { useRequestsQuery } from "@/hooks/api";
> +
> +const NoRegions = () => {
> +  const [size] = useState(50);
> +  const [page] = useState(0);

There's no need to use state here, these values will never change. Could you please inline values in useRequestsQuery instead.

You could also create a new hook named useRequestsCountQuery to abstract this away, it can be slightly unclear otherwise (optional).

> +  const { data, isLoading } = useRequestsQuery({ page: `${page}`, size: `${size}` });
> +
> +  return (
> +    <TableCaption>
> +      <TableCaption.Title>No enroled MAAS regions</TableCaption.Title>
> +      {!isLoading && data!.total > 0 ? (
> +        <>
> +          <TableCaption.Description>
> +            You have <strong>{data?.total} open enrolment requests, </strong>inspect them in the Requests page.
> +            <br />
> +            <ExternalLink to={docsUrls.enrollmentRequest}>
> +              Learn more about the enrolment process in the documentation.
> +            </ExternalLink>
> +          </TableCaption.Description>
> +          <TableCaption.Description>
> +            <Link className="p-button--positive" to="/requests">
> +              Go to Requests Page
> +            </Link>
> +          </TableCaption.Description>
> +        </>
> +      ) : (
> +        <>
> +          <TableCaption.Description>
> +            To enrol follow the steps in the Tokens page.
> +            <br />
> +            <ExternalLink to={docsUrls.enrollmentRequest}>
> +              Learn more about the enrolment process in the documentation.
> +            </ExternalLink>
> +          </TableCaption.Description>
> +          <TableCaption.Description>
> +            <Link className="p-button--positive" to="/tokens">
> +              Go to Tokens page
> +            </Link>
> +          </TableCaption.Description>
> +        </>
> +      )}
> +    </TableCaption>
> +  );
> +};
> +
> +export default NoRegions;


-- 
https://code.launchpad.net/~jonesogolo/maas-site-manager/+git/maas-site-manager/+merge/440258
Your team MAAS Committers is requested to review the proposed merge of ~jonesogolo/maas-site-manager:1526-add-empty-states-to-regions-table into maas-site-manager:main.