Bugfixes for collection create/edit and record loading (#3051)

* Fixed CollectionForm bug where users couldn't create collections. Added unit tests. Also fixed lifecycle quirk on CollectionRecords where data did not load when switching between collections of the same name under different buckets.
* Added unit tests to check useEffect being hit correctly on CollectionRecords

src/components/collection/CollectionForm.tsx

@@ -6,7 +6,6 @@ import type {
 } from "../../types";
 
 import React, { useState } from "react";
-import { Link } from "react-router-dom";
 
 import { Check2 } from "react-bootstrap-icons";
 
@@ -18,6 +17,7 @@ import { canCreateCollection, canEditCollection } from "../../permission";
 import { validateSchema, validateUiSchema } from "../../utils";
 import DeleteForm from "./DeleteForm";
 import { FormInstructions } from "./FormInstructions";
+import { Link } from "react-router-dom";
 
 const defaultSchema = JSON.stringify(
   {
@@ -248,8 +248,8 @@ export default function CollectionForm({
 }: Props) {
   const [asJSON, setAsJSON] = useState(false);
   const allowEditing = propFormData
-    ? canCreateCollection(session, bucket)
-    : canEditCollection(session, bucket, collection);
+    ? canEditCollection(session, bucket, collection)
+    : canCreateCollection(session, bucket);
 
   const toggleJSON = event => {
     event.preventDefault();

src/components/collection/CollectionRecords.tsx

@@ -60,17 +60,13 @@ export default function CollectionRecords(props: Props) {
     [bid, cid, listRecords]
   );
 
-  const onCollectionRecordsEnter = useCallback(() => {
+  useEffect(() => {
     if (!session.authenticated) {
       return;
     }
     const { currentSort } = collection;
     listRecords(bid, cid, currentSort);
-  }, [bid, cid, collection, listRecords, session]);
-
-  useEffect(() => {
-    onCollectionRecordsEnter();
-  }, []);
+  }, [bid, cid]);
 
   const {
     busy,

test/components/BaseForm_test.js

@@ -39,10 +39,6 @@ const testUiSchema = {
 };
 
 describe("BaseForm component", () => {
-  beforeEach(() => {});
-
-  afterEach(() => {});
-
   it("Should render a rjsf form as expected", async () => {
     const result = render(
       <BaseForm

test/components/CollectionForm_test.js

@@ -0,0 +1,152 @@
+import React from "react";
+import { render } from "@testing-library/react";
+import CollectionForm from "../../src/components/collection/CollectionForm";
+
+const warningText =
+  "You don't have the required permission to edit this collection.";
+
+const testProps = {
+  cid: null,
+  bid: "default",
+  session: {
+    permissions: [],
+    buckets: [
+      {
+        id: "default",
+      },
+    ],
+  },
+  bucket: {
+    busy: false,
+    data: {
+      id: "default",
+    },
+    permissions: {},
+    groups: [],
+  },
+  collection: {
+    busy: false,
+    data: {},
+    permissions: {},
+    records: [],
+  },
+  deleteCollection: jest.fn(),
+  onSubmit: jest.fn(),
+  formData: undefined,
+};
+
+// to avoid rendering a router around everything, allows for easier testing
+jest.mock("react-router-dom", () => {
+  const originalModule = jest.requireActual("react-router-dom");
+  return {
+    __esModule: true,
+    ...originalModule,
+    Link: "a",
+  };
+});
+
+describe("CollectionForm component", () => {
+  beforeAll(() => {
+    // preventing code mirror errors
+    Range.prototype.getBoundingClientRect = () => ({
+      bottom: 0,
+      height: 0,
+      left: 0,
+      right: 0,
+      top: 0,
+      width: 0,
+    });
+    Range.prototype.getClientRects = () => ({
+      item: () => null,
+      length: 0,
+      [Symbol.iterator]: jest.fn(),
+    });
+  });
+
+  it("Should render an editable form for a user with permissions creating a new collection", async () => {
+    const localTestProps = JSON.parse(JSON.stringify(testProps));
+    localTestProps.session.permissions.push({
+      resource_name: "bucket",
+      permissions: [
+        "group:create",
+        "read",
+        "read:attributes",
+        "collection:create",
+      ],
+      bucket_id: "default",
+    });
+    const result = render(<CollectionForm {...localTestProps} />);
+
+    const warning = result.queryByText(warningText);
+    expect(warning).toBeNull();
+    const title = await result.findByLabelText("Collection id*");
+    expect(title.disabled).toBe(false);
+  });
+
+  it("Should render an editable form for a user with permissions editing a collection", async () => {
+    const localTestProps = JSON.parse(JSON.stringify(testProps));
+    localTestProps.session.permissions.push({
+      uri: "/buckets/default/test",
+      resource_name: "collection",
+      permissions: ["write"],
+      id: "test",
+      bucket_id: "default",
+      collection_id: "test",
+    });
+    localTestProps.cid = "test";
+    localTestProps.bid = "default";
+    localTestProps.collection.data.id = "test";
+    localTestProps.formData = {};
+
+    const result = render(<CollectionForm {...localTestProps} />);
+
+    const warning = result.queryByText(warningText);
+    expect(warning).toBeNull();
+    const title = await result.findByLabelText("Collection id*");
+    expect(title.disabled).toBe(false);
+  });
+
+  it("Should render an error for a user lacking permissions creating a collection", async () => {
+    const localTestProps = JSON.parse(JSON.stringify(testProps));
+    localTestProps.session.permissions = [
+      {
+        uri: "/buckets/default",
+        resource_name: "bucket",
+        permissions: ["group:create", "read", "read:attributes", "write"],
+        id: "default",
+        bucket_id: "default",
+      },
+    ];
+    const result = render(<CollectionForm {...localTestProps} />);
+
+    const warning = await result.queryByText(warningText);
+    expect(warning).toBeDefined();
+    const title = await result.findByLabelText("Collection id*");
+    expect(title.disabled).toBe(true);
+  });
+
+  it("Should render as read-only for a user lacking permissions editing a collection", async () => {
+    const localTestProps = JSON.parse(JSON.stringify(testProps));
+    localTestProps.collection = {
+      busy: false,
+      data: {
+        id: "test",
+      },
+      permissions: {
+        read: [],
+        "record:create": [],
+      },
+      records: [],
+    };
+    localTestProps.cid = "test";
+    localTestProps.bid = "default";
+    localTestProps.formData = {};
+
+    const result = render(<CollectionForm {...localTestProps} />);
+
+    const warning = await result.queryByText(warningText);
+    expect(warning).toBeDefined();
+    const title = await result.findByLabelText("Collection id*");
+    expect(title.disabled).toBe(true);
+  });
+});

test/components/CollectionRecords_test.js

@@ -1,21 +1,8 @@
-import { expect } from "chai";
-
-import { createSandbox, createComponent } from "../test_utils";
-
+import { createComponent, renderWithProvider } from "../test_utils";
 import CollectionRecords from "../../src/components/collection/CollectionRecords";
 import * as React from "react";
 
 describe("CollectionRecords component", () => {
-  let sandbox;
-
-  beforeEach(() => {
-    sandbox = createSandbox();
-  });
-
-  afterEach(() => {
-    sandbox.restore();
-  });
-
   const capabilities = {
     history: {},
   };
@@ -29,6 +16,69 @@ describe("CollectionRecords component", () => {
     },
   };
 
+  it("Calls listRecords every time bid or cid changes", async () => {
+    const listRecordsMock = jest.fn();
+
+    const props = {
+      session: { authenticated: true, permissions: ["foo"] },
+      bucket,
+      collection: {
+        busy: false,
+        data: {
+          schema: {
+            type: "object",
+            properties: {
+              foo: { type: "string" },
+            },
+          },
+          displayFields: ["foo"],
+        },
+        permissions: {
+          write: ["basicauth:plop"],
+        },
+        recordsLoaded: true,
+        records: [],
+      },
+      capabilities,
+      listRecords: listRecordsMock,
+    };
+    const result = renderWithProvider(
+      <CollectionRecords
+        {...props}
+        match={{ params: { bid: "bucket1", cid: "collection1" } }}
+      />
+    );
+
+    expect(listRecordsMock).toHaveBeenCalledTimes(1);
+
+    result.rerender(
+      <CollectionRecords
+        {...props}
+        match={{ params: { bid: "bucket1", cid: "collection2" } }}
+      />
+    );
+
+    expect(listRecordsMock).toHaveBeenCalledTimes(2);
+
+    result.rerender(
+      <CollectionRecords
+        {...props}
+        match={{ params: { bid: "bucket2", cid: "collection2" } }}
+      />
+    );
+
+    expect(listRecordsMock).toHaveBeenCalledTimes(3);
+
+    result.rerender(
+      <CollectionRecords
+        {...props}
+        match={{ params: { bid: "bucket2", cid: "collection1" } }}
+      />
+    );
+
+    expect(listRecordsMock).toHaveBeenCalledTimes(4);
+  });
+
   describe("Schema defined", () => {
     let node;
 
@@ -70,15 +120,15 @@ describe("CollectionRecords component", () => {
     });
 
     it("should render a table", () => {
-      expect(node.querySelector("table")).to.exist;
+      expect(node.querySelector("table")).toBeDefined();
     });
 
     it("should render record rows", () => {
       const rows = node.querySelectorAll("tbody tr");
 
-      expect(rows).to.have.a.lengthOf(2);
-      expect(rows[0].querySelectorAll("td")[0].textContent).eql("bar");
-      expect(rows[1].querySelectorAll("td")[0].textContent).eql("baz");
+      expect(rows).toHaveLength(2);
+      expect(rows[0].querySelectorAll("td")[0].textContent).toBe("bar");
+      expect(rows[1].querySelectorAll("td")[0].textContent).toBe("baz");
     });
   });
 
@@ -120,19 +170,19 @@ describe("CollectionRecords component", () => {
     });
 
     it("should render a table", () => {
-      expect(node.querySelector("table")).to.exist;
+      expect(node.querySelector("table")).toBeDefined();
     });
 
     it("should render record rows", () => {
       const rows = node.querySelectorAll("tbody tr");
 
-      expect(rows).to.have.a.lengthOf(2);
-      expect(rows[0].querySelectorAll("td")[0].textContent).eql("id1");
-      expect(rows[0].querySelectorAll("td")[1].textContent).eql(
+      expect(rows).toHaveLength(2);
+      expect(rows[0].querySelectorAll("td")[0].textContent).toBe("id1");
+      expect(rows[0].querySelectorAll("td")[1].textContent).toBe(
         JSON.stringify({ foo: "bar" })
       );
-      expect(rows[1].querySelectorAll("td")[0].textContent).eql("id2");
-      expect(rows[1].querySelectorAll("td")[1].textContent).eql(
+      expect(rows[1].querySelectorAll("td")[0].textContent).toBe("id2");
+      expect(rows[1].querySelectorAll("td")[1].textContent).toBe(
         JSON.stringify({ foo: "baz" })
       );
     });
@@ -166,7 +216,7 @@ describe("CollectionRecords component", () => {
     it("should show the total number of records", () => {
       expect(
         node.querySelector(".card-header-tabs .nav-link.active").textContent
-      ).to.eql("Records (18)");
+      ).toBe("Records (18)");
     });
   });
 
@@ -206,9 +256,12 @@ describe("CollectionRecords component", () => {
       });
 
       it("should render list actions", () => {
-        expect(node.querySelector(".list-actions .btn-record-add")).to.exist;
-        expect(node.querySelector(".list-actions .btn-record-bulk-add")).to
-          .exist;
+        expect(
+          node.querySelector(".list-actions .btn-record-add")
+        ).toBeDefined();
+        expect(
+          node.querySelector(".list-actions .btn-record-bulk-add")
+        ).toBeDefined();
       });
     });
 
@@ -229,8 +282,9 @@ describe("CollectionRecords component", () => {
       });
 
       it("should not render list actions", () => {
-        expect(node.querySelector(".list-actions .btn-record-bulk-add")).to.not
-          .exist;
+        expect(
+          node.querySelector(".list-actions .btn-record-bulk-add")
+        ).toBeNull();
       });
     });
   });