Responds to feedback on PR

NYPL-Simplified · Feb 16, 2022 · e017159 · e017159
1 parent 01ea9bd
commit e017159
Show file tree

Hide file tree

Showing 7 changed files with 161 additions and 139 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,7 +5,7 @@
 #### Updated
 
 - Removed the router from the testUtils render function, which was causing testing issues with the RTL/Jest tests.
-- Created a new CatalogLink component that doesn't rely on the ContextProvider (related to the issue above).
+- Created a new CatalogLink component that doesn't rely on the ContextProvider (related to the issue above). This component is currently only used in the List Manager.
 - Wrote tests using RTL for two main components in the List Manager, CustomListEditorHeader and CustomListEditorBody.
 
 ### v0.5.15

diff --git a/src/components/CustomListEntries.tsx b/src/components/CustomListEntries.tsx
@@ -75,11 +75,7 @@ export default function CustomListEntries({
     }
   };
   return (
-    <div
-      role="region"
-      aria-label="List Entries"
-      className="custom-list-entries"
-    >
+    <section aria-label="List Entries" className="custom-list-entries">
       <div className="droppable-header">
         {showSaveError && (
           <p className="save-list-error">
@@ -136,6 +132,6 @@ export default function CustomListEntries({
           loadMore={onLoadMore}
         />
       )}
-    </div>
+    </section>
   );
 }
diff --git a/src/components/CustomListInfo.tsx b/src/components/CustomListInfo.tsx
@@ -73,7 +73,6 @@ export default function CustomListInfo({
           <Link
             to={"/admin/web/lists/" + library + "/edit/" + list.id}
             className="btn left-align small"
-            role="button"
           >
             <span>
               Edit

diff --git a/src/components/__tests_jest__/CustomListEditorBody.test.js b/src/components/__tests_jest__/CustomListEditorBody.test.js
@@ -1,5 +1,5 @@
 import React from "react";
-import { within } from "@testing-library/react";
+import { within, waitFor } from "@testing-library/react";
 import { render } from "../../testUtils/testUtils";
 import CustomListEditorBody from "../CustomListEditorBody";
 import { DragDropContext } from "react-beautiful-dnd";
@@ -99,86 +99,105 @@ const setDraftCollections = jest.fn();
 const onDragEnd = jest.fn();
 let utils;
 
-beforeEach(() => {
-  utils = render(
-    <DragDropContext onDragEnd={onDragEnd}>
-      <CustomListEditorBody
-        entries={listData.books}
-        draftTitle={listData.title}
-        isFetchingMoreCustomListEntries={false}
-        isFetchingMoreSearchResults={false}
-        languages={languages}
-        library={library}
-        listId={listData.id}
-        search={search}
-        setDraftCollections={setDraftCollections}
-        saveFormData={saveFormData}
-        loadMoreEntries={loadMoreEntries}
-        loadMoreSearchResults={loadMoreSearchResults}
-        totalListEntries={2}
-      />
-    </DragDropContext>
-  );
-});
-
-test("if there are entries passed, they render on the page", () => {
-  const entriesHeader = utils.getByRole("heading", {
-    level: 4,
-    name: "Displaying 1 - 2 of 2 Books",
+describe("CustomListBodyEditor", () => {
+  beforeEach(() => {
+    utils = render(
+      <DragDropContext onDragEnd={onDragEnd}>
+        <CustomListEditorBody
+          entries={listData.books}
+          draftTitle={listData.title}
+          isFetchingMoreCustomListEntries={false}
+          isFetchingMoreSearchResults={false}
+          languages={languages}
+          library={library}
+          listId={listData.id}
+          search={search}
+          setDraftCollections={setDraftCollections}
+          saveFormData={saveFormData}
+          loadMoreEntries={loadMoreEntries}
+          loadMoreSearchResults={loadMoreSearchResults}
+          totalListEntries={2}
+        />
+      </DragDropContext>
+    );
   });
-  expect(entriesHeader).toBeInTheDocument();
 
-  const entries = utils.getAllByRole("list")[1];
+  test("if there are entries passed, they render on the page", () => {
+    const entriesHeader = utils.getByRole("heading", {
+      level: 4,
+      name: "Displaying 1 - 2 of 2 Books",
+    });
+    expect(entriesHeader).toBeInTheDocument();
 
-  const entryA = within(entries).getByText("entry A");
-  const entryB = within(entries).getByText("entry B");
+    const lists = utils.queryAllByRole("list");
 
-  expect(entryA).toBeInTheDocument();
-  expect(entryB).toBeInTheDocument();
-});
+    expect(lists).toHaveLength(2);
+
+    const entries = utils.getAllByRole("list")[1];
+
+    const entryA = within(entries).queryAllByText("entry A");
+    const entryB = within(entries).queryAllByText("entry B");
 
-test("if there are no entries, then none render", () => {
-  utils = render(
-    <DragDropContext onDragEnd={onDragEnd}>
-      <CustomListEditorBody
-        entries={[]}
-        draftTitle={""}
-        isFetchingMoreCustomListEntries={false}
-        isFetchingMoreSearchResults={false}
-        languages={languages}
-        library={library}
-        search={search}
-        setDraftCollections={setDraftCollections}
-        saveFormData={saveFormData}
-        loadMoreEntries={loadMoreEntries}
-        loadMoreSearchResults={loadMoreSearchResults}
-      />
-    </DragDropContext>
-  );
-
-  const entriesHeader = utils.getByRole("heading", {
-    level: 4,
-    name: "No books in this list",
+    expect(entryA).toHaveLength(1);
+    expect(entryB).toHaveLength(1);
   });
-  expect(entriesHeader).toBeInTheDocument();
-});
 
-test("there is a container to display search results", () => {
-  const searchResultsContainer = utils.getByRole("region", {
-    name: "Search Results",
+  test("if there are no entries, then none render", () => {
+    const { rerender } = utils;
+    rerender(
+      <DragDropContext onDragEnd={onDragEnd}>
+        <CustomListEditorBody
+          entries={[]}
+          draftTitle={""}
+          isFetchingMoreCustomListEntries={false}
+          isFetchingMoreSearchResults={false}
+          languages={languages}
+          library={library}
+          search={search}
+          setDraftCollections={setDraftCollections}
+          saveFormData={saveFormData}
+          loadMoreEntries={loadMoreEntries}
+          loadMoreSearchResults={loadMoreSearchResults}
+        />
+      </DragDropContext>
+    );
+
+    const lists = utils.queryAllByRole("list");
+
+    expect(lists).toHaveLength(2);
+
+    const entries = utils.getAllByRole("list")[1];
+
+    const entryA = within(entries).queryAllByText("entry A");
+    const entryB = within(entries).queryAllByText("entry B");
+
+    expect(entryA).toHaveLength(0);
+    expect(entryB).toHaveLength(0);
+
+    const entriesHeader = utils.getByRole("heading", {
+      level: 4,
+      name: "No books in this list",
+    });
+    expect(entriesHeader).toBeInTheDocument();
   });
 
-  const searchResultsHeader = within(
-    searchResultsContainer
-  ).getByRole("heading", { level: 4 });
+  test("there is a container to display search results", () => {
+    const searchResultsContainer = utils.getByRole("region", {
+      name: "Search Results",
+    });
 
-  expect(searchResultsHeader).toHaveTextContent("Search Results");
+    const searchResultsHeader = within(
+      searchResultsContainer
+    ).getByRole("heading", { level: 4 });
 
-  const searchResultsDragInstructions = within(
-    searchResultsContainer
-  ).getByText("remove them from", {
-    exact: false,
-  });
+    expect(searchResultsHeader).toHaveTextContent("Search Results");
 
-  expect(searchResultsDragInstructions).toBeInTheDocument();
+    const searchResultsDragInstructions = within(
+      searchResultsContainer
+    ).getByText("remove them from", {
+      exact: false,
+    });
+
+    expect(searchResultsDragInstructions).toBeInTheDocument();
+  });
 });
diff --git a/src/components/__tests_jest__/CustomListEditorHeader.test.js b/src/components/__tests_jest__/CustomListEditorHeader.test.js
@@ -8,80 +8,85 @@ const editCustomList = jest.fn();
 
 let utils;
 
-beforeAll(() => {
-  utils = render(
-    <CustomListEditorHeader
-      draftTitle="title"
-      listId={1}
-      setDraftTitle={setDraftTitle}
-      editCustomList={editCustomList}
-    />
-  );
-});
+describe("CustomListEditorHeader", () => {
+  beforeAll(() => {
+    utils = render(
+      <CustomListEditorHeader
+        draftTitle="title"
+        listId={1}
+        setDraftTitle={setDraftTitle}
+        editCustomList={editCustomList}
+      />
+    );
+  });
 
-test("if there is a list passed, its title and id renders on the page", () => {
-  const title = utils.getByRole("heading", { level: 3 });
-  expect(title).toHaveTextContent("title");
+  test("if there is a list passed, its title and id renders on the page", () => {
+    const title = utils.getByRole("heading", { level: 3 });
 
-  const id = utils.getByRole("heading", { level: 4 });
-  expect(id).toHaveTextContent("ID-1");
+    expect(title).toHaveTextContent("title");
 
-  const titleButton = utils.getByRole("button", {
-    name: "Edit list title",
-  });
+    const id = utils.getByRole("heading", { level: 4 });
 
-  expect(titleButton).toBeInTheDocument();
-});
+    // The UI prefixes the id with "ID-".
+    expect(id).toHaveTextContent("ID-1");
 
-test("if there is no list passed, the create form is rendered", () => {
-  utils = render(
-    <CustomListEditorHeader
-      draftTitle=""
-      setDraftTitle={setDraftTitle}
-      editCustomList={editCustomList}
-    />
-  );
-
-  const titleInput = utils.getByRole("textbox", {
-    name: "Enter a title for this list",
+    const titleButton = utils.getByRole("button", {
+      name: "Edit list title",
+    });
+
+    expect(titleButton).toBeInTheDocument();
   });
 
-  expect(titleInput).toHaveValue("");
+  test("if there is no list passed, the create form is rendered", () => {
+    utils = render(
+      <CustomListEditorHeader
+        draftTitle=""
+        setDraftTitle={setDraftTitle}
+        editCustomList={editCustomList}
+      />
+    );
 
-  const titleButton = utils.getByRole("button", {
-    name: "Save list title",
-  });
+    const titleInput = utils.getByRole("textbox", {
+      name: "Enter a title for this list",
+    });
 
-  expect(titleButton).toBeInTheDocument();
-});
+    expect(titleInput).toHaveValue("");
 
-test("a user can update the title", () => {
-  utils = render(
-    <CustomListEditorHeader
-      draftTitle=""
-      setDraftTitle={setDraftTitle}
-      editCustomList={editCustomList}
-    />
-  );
-
-  const titleInput = utils.getByRole("textbox", {
-    name: "Enter a title for this list",
+    const titleButton = utils.getByRole("button", {
+      name: "Save list title",
+    });
+
+    expect(titleButton).toBeInTheDocument();
   });
 
-  expect(titleInput).toHaveValue("");
+  test("a user can update the title", () => {
+    utils = render(
+      <CustomListEditorHeader
+        draftTitle=""
+        setDraftTitle={setDraftTitle}
+        editCustomList={editCustomList}
+      />
+    );
 
-  let titleButton = utils.getByRole("button", {
-    name: "Save list title",
-  });
+    const titleInput = utils.getByRole("textbox", {
+      name: "Enter a title for this list",
+    });
 
-  fireEvent.change(titleInput, { target: { value: "new list" } });
-  fireEvent.click(titleButton);
+    expect(titleInput).toHaveValue("");
 
-  expect(titleInput).toHaveValue("new list");
+    let titleButton = utils.getByRole("button", {
+      name: "Save list title",
+    });
 
-  titleButton = utils.getByRole("button", {
-    name: "Edit list title",
-  });
+    fireEvent.change(titleInput, { target: { value: "new list" } });
+    fireEvent.click(titleButton);
 
-  expect(titleButton).toBeInTheDocument();
+    expect(titleInput).toHaveValue("new list");
+
+    titleButton = utils.getByRole("button", {
+      name: "Edit list title",
+    });
+
+    expect(titleButton).toBeInTheDocument();
+  });
 });
diff --git a/src/components/__tests_jest__/CustomListEntries.test.js b/src/components/__tests_jest__/CustomListEntries.test.js
@@ -3,7 +3,7 @@
  * */
 
 import React from "react";
-import { screen, waitFor } from "@testing-library/react";
+import { screen } from "@testing-library/react";
 import { render } from "../../testUtils/testUtils";
 import { stub } from "sinon";
 import CustomListEntries from "../CustomListEntries";

diff --git a/src/components/__tests_jest__/CustomLists.test.js b/src/components/__tests_jest__/CustomLists.test.js
@@ -107,6 +107,9 @@ beforeEach(() => {
   utils = render(<CustomLists {...customListsProps} />);
 });
 
+// Skipping this test for now. Tests for this component fail because the store cannot be found.
+// However, the store should be created in the context, which I wrapped around all test components
+// in testUtils. This will need further investigation to resolve.
 test.skip("calls fetchCustomLists and fetchCustomListDetails when a list's edit button is clicked", () => {
   const editButtonListA = waitFor(
     () => getAllByRole("button", { name: "Edit" })[0]
-Original file line number
+Diff line change
@@ Expand Up / @@ -73,7 +73,6 @@ export default function CustomListInfo({ @@
               <Link
                 to={"/admin/web/lists/" + library + "/edit/" + list.id}
                 className="btn left-align small"
-                role="button"
               >
                 <span>
                   Edit
@@ Expand Down @@