Adjustments from feedback

src/components/collection/CollectionRecords.tsx

@@ -142,6 +142,10 @@ export default function CollectionRecords(props: Props) {
             onClick={() => {
               setUseSimpleReview(true);
             }}
+            style={{
+              float: "right",
+              marginTop: "1.5em",
+            }}
           >
             <Shuffle className="icon" /> Switch to Default Review UI
           </AdminLink>

src/components/signoff/SimpleReview/SimpleReviewButtons.tsx

@@ -35,11 +35,14 @@ export default function SimpleReviewButtons({
   return (
     <>
       <div className="simple-review-buttons">
-        {status === "to-review" && (
+        {status === "to-review" && canReview && (
           <>
             <button
               className="btn btn-success"
-              onClick={() => approveChanges()}
+              onClick={() => {
+                setShowSpinner(true);
+                approveChanges();
+              }}
             >
               <Check2 className="icon" /> Approve...
             </button>{" "}
@@ -61,7 +64,7 @@ export default function SimpleReviewButtons({
             <ChatLeft className="icon" /> Request review...
           </button>
         )}{" "}
-        {canReview &&
+        {canRequestReview &&
           ["work-in-progress", "to-review"].includes(status) &&
           !searchParams.hideRollback && (
             <button

src/components/signoff/SimpleReview/SimpleReviewHeader.tsx

@@ -7,6 +7,8 @@ export default function SimpleReviewHeader({
   lastReviewRequestBy,
   lastReviewerComment,
   children,
+  lastEditDate,
+  lastReviewDate,
 }: SignoffSourceInfo & { children?: React.ReactElement }) {
   return (
     <div className="simple-review-header">
@@ -20,7 +22,9 @@ export default function SimpleReviewHeader({
         {status === "work-in-progress" && (
           <div className="card-header">
             Status is <code>{status}</code>.{" "}
-            {lastReviewerComment && "Most recent reviewer comment was:"}
+            {lastReviewerComment &&
+              lastEditDate < lastReviewDate &&
+              "Most recent reviewer comment was:"}
           </div>
         )}
         <div className="card-body">
@@ -29,7 +33,7 @@ export default function SimpleReviewHeader({
               {lastEditorComment || "(No comment was left by the author)"}
             </p>
           )}
-          {status === "work-in-progress" && (
+          {status === "work-in-progress" && lastEditDate < lastReviewDate && (
             <p className="card-text">
               {lastReviewerComment || "(No comment was left by a reviewer)"}
             </p>

src/components/signoff/SimpleReview/index.tsx

@@ -17,7 +17,7 @@ import { isReviewer } from "../SignoffToolBar";
 import Spinner from "../../Spinner";
 import CollectionTabs from "../../collection/CollectionTabs";
 import { storageKeys, useLocalStorage } from "../../../hooks/storage";
-import { Redirect } from "react-router-dom";
+import { Redirect, useHistory } from "react-router-dom";
 import { Shuffle } from "react-bootstrap-icons";
 import { canEditCollection } from "../../../permission";
 import { isMember } from "../utils";
@@ -57,6 +57,7 @@ export default function SimpleReview({
     storageKeys.useSimpleReview,
     true
   );
+  const history = useHistory();
   const signoffSource = signoff?.collectionsInfo?.source;
   const sourceBid = signoffSource?.bid;
   const sourceCid = signoffSource?.cid;
@@ -65,7 +66,10 @@ export default function SimpleReview({
   const destBid = signoffDest?.bid;
   const destCid = signoffDest?.cid;
 
-  const canReview = signoffSource ? isReviewer(signoffSource, session) : false;
+  const canReview = signoffSource
+    ? isReviewer(signoffSource, session) &&
+      session.serverInfo?.user?.id !== signoffSource.lastReviewRequestBy
+    : false;
 
   const [records, setRecords] = useState<{
     loading: boolean;
@@ -117,22 +121,31 @@ export default function SimpleReview({
   }
 
   let message = "";
-  if (session.authenticating) {
-    return <Spinner />;
-  } else if (!session.authenticated) {
+  if (!session.authenticated) {
     message = "Not authenticated";
+  } else if (
+    session.authenticating ||
+    session.busy ||
+    (records.loading && signoffSource && signoffDest)
+  ) {
+    return <Spinner />;
   } else if (!signoffSource || !signoffSource?.status) {
-    // TODO: use this to show/hide
-    // also: {["to-review", "work-in-progress"].includes(signoffSource.status) && (
     message = "This is not a collection that supports reviews.";
   }
 
   if (message) {
-    return <div className="simple-review-blocked-message p-4">{message}</div>;
+    return (
+      <div className="simple-review-blocked-message list-page">{message}</div>
+    );
   }
 
+  const handleRollback = (text: string) => {
+    rollbackChanges(text);
+    history.push(`/buckets/${bid}/collections/${cid}/records`);
+  };
+
   return (
-    <div className="p-4">
+    <div className="list-page">
       <h1>
         Review{" "}
         <b>
@@ -154,25 +167,26 @@ export default function SimpleReview({
               approveChanges={approveChanges}
               declineChanges={declineChanges}
               requestReview={requestReview}
-              rollbackChanges={rollbackChanges}
+              rollbackChanges={handleRollback}
               canReview={canReview}
               canRequestReview={canRequestReview}
             />
           </SimpleReviewHeader>
         )}
-        {(records.loading && <Spinner />) || (
-          <PerRecordDiffView
-            oldRecords={records.oldRecords}
-            newRecords={records.newRecords}
-            collectionData={signoffSource}
-          />
-        )}
+        <PerRecordDiffView
+          oldRecords={records.oldRecords}
+          newRecords={records.newRecords}
+          collectionData={signoffSource}
+        />
         <button
           type="button"
           className="btn btn-secondary"
           onClick={() => {
             setUseSimpleReview(false);
           }}
+          style={{
+            float: "right",
+          }}
         >
           <Shuffle className="icon" /> Switch to Legacy Review UI
         </button>

test/components/signoff/SimpleReview/SimpleReviewButtons_test.js

@@ -22,18 +22,26 @@ function renderButtons(props = null) {
 }
 
 describe("SimpleReviewHeader component", () => {
-  it("should call approveChanges when approve button is clicked", () => {
-    const { approveChanges, node } = renderButtons({ status: "to-review" });
+  it("should call approveChanges when approve button is clicked", async () => {
+    const { approveChanges, node } = renderButtons({
+      status: "to-review",
+      canReview: true,
+    });
 
     fireEvent.click(node.getByText(/Approve/));
     expect(approveChanges).toHaveBeenCalled();
+    expect(await node.findByTestId("spinner")).toBeDefined();
   });
 
   it("should open CommentDialog when reject is clicked call declineChanges from modal", async () => {
-    const { declineChanges, node } = renderButtons({ status: "to-review" });
+    const { declineChanges, node } = renderButtons({
+      status: "to-review",
+      canReview: true,
+    });
     fireEvent.click(node.getByText(/Decline/));
     fireEvent.click(await node.findByText("Reject changes"));
     expect(declineChanges).toHaveBeenCalled();
+    expect(await node.findByTestId("spinner")).toBeDefined();
   });
 
   it("should open CommentDialog when request review is clicked and call requestReview from modal", async () => {
@@ -43,10 +51,11 @@ describe("SimpleReviewHeader component", () => {
   it("should display rollback button when status is wip and call rollbackChanges from modal", async () => {
     const { rollbackChanges, node } = renderButtons({
       status: "work-in-progress",
-      canReview: true,
+      canRequestReview: true,
     });
     fireEvent.click(node.getByText(/Rollback changes/));
     fireEvent.click(await node.findByText("Rollback"));
     expect(rollbackChanges).toHaveBeenCalled();
+    expect(await node.findByTestId("spinner")).toBeDefined();
   });
 });

test/components/signoff/SimpleReview/SimpleReviewHeader_test.js

@@ -1,6 +1,4 @@
-import { expect } from "chai";
 import { createComponent } from "../../../test_utils";
-
 import SimpleReviewHeader from "../../../../src/components/signoff/SimpleReview/SimpleReviewHeader";
 import * as React from "react";
 
@@ -22,24 +20,25 @@ const wipProps = {
 describe("SimpleReviewHeader component", () => {
   it("should render title when component is to-review", () => {
     const node = createComponent(<SimpleReviewHeader {...toReviewProps} />);
-    expect(node.querySelector(".card-header").textContent).to.equal(
+    expect(node.querySelector(".card-header").textContent).toBe(
       "Review requested by ana:"
     );
   });
   it("should render an editor comment when component is to-review", () => {
     const node = createComponent(<SimpleReviewHeader {...toReviewProps} />);
-    expect(node.querySelector(".card-text").textContent).to.equal(
-      "please review"
-    );
+    expect(node.querySelector(".card-text").textContent).toBe("please review");
   });
-  it("should render a wip header", () => {
-    const node = createComponent(<SimpleReviewHeader {...wipProps} />);
-    expect(node.querySelector(".card-header").textContent).to.equal(
+  it("should render a reviewer comments as expected when component is wip", () => {
+    const node = createComponent(
+      <SimpleReviewHeader
+        {...wipProps}
+        lastEditDate={123}
+        lastReviewDate={124}
+      />
+    );
+    expect(node.querySelector(".card-text").textContent).toBe("no thanks");
+    expect(node.querySelector(".card-header").textContent).toBe(
       "Status is work-in-progress. Most recent reviewer comment was:"
     );
   });
-  it("should render a reviewer comment when component is wip", () => {
-    const node = createComponent(<SimpleReviewHeader {...wipProps} />);
-    expect(node.querySelector(".card-text").textContent).to.equal("no thanks");
-  });
 });

test/components/signoff/SimpleReview/SimpleReview_test.js

@@ -1,9 +1,9 @@
 import * as React from "react";
-import { waitForElementToBeRemoved } from "@testing-library/react";
+import { fireEvent, waitForElementToBeRemoved } from "@testing-library/react";
 import { renderWithProvider, sessionFactory } from "../../../test_utils";
 import SimpleReview from "../../../../src/components/signoff/SimpleReview";
 import { useLocalStorage } from "../../../../src/hooks/storage";
-import { Redirect } from "react-router-dom";
+import { Redirect, useHistory } from "react-router-dom";
 
 jest.mock("../../../../src/hooks/storage", () => {
   const originalModule = jest.requireActual("../../../../src/hooks/storage");
@@ -14,6 +14,38 @@ jest.mock("../../../../src/hooks/storage", () => {
   };
 });
 
+jest.mock("react-router-dom", () => {
+  const originalModule = jest.requireActual("react-router-dom");
+  const pushMock = jest.fn();
+  const useHistory = () => {
+    return {
+      push: pushMock,
+    };
+  };
+  return {
+    __esModule: true,
+    ...originalModule,
+    useHistory,
+    Redirect: jest.fn().mockReturnValue(<div>foo</div>),
+  };
+});
+
+jest.mock("../../../../src/permission", () => {
+  return {
+    canEditCollection: () => {
+      return true;
+    },
+  };
+});
+
+jest.mock("../../../../src/components/signoff/utils", () => {
+  return {
+    isMember: () => {
+      return true;
+    },
+  };
+});
+
 function signoffFactory() {
   return {
     collectionsInfo: {
@@ -26,7 +58,7 @@ function signoffFactory() {
         lastEditDate: 1622816256864,
         lastEditorComment: undefined,
         lastReviewBy: undefined,
-        lastReviewDate: NaN,
+        lastReviewDate: 1622816256865,
         lastReviewRequestBy: undefined,
         lastReviewRequestDate: NaN,
         lastReviewerComment: undefined,
@@ -68,6 +100,7 @@ function renderSimpleReview(props = null) {
       return [];
     },
     ...props,
+    rollbackChanges: jest.fn(),
   };
   return renderWithProvider(<SimpleReview {...mergedProps} />);
 }
@@ -78,18 +111,6 @@ const fakeLocation = {
   hash: "",
 };
 
-jest.mock("react-router", () => {
-  const originalModule = jest.requireActual("react-router");
-  return {
-    __esModule: true,
-    ...originalModule,
-    useLocation: () => {
-      return fakeLocation;
-    },
-    Redirect: jest.fn().mockReturnValue(<div>foo</div>),
-  };
-});
-
 describe("SimpleTest component", () => {
   it("should render spinner when authenticating", async () => {
     const node = renderSimpleReview({
@@ -126,15 +147,21 @@ describe("SimpleTest component", () => {
     expect(node.getByText("Show all lines")).toBeDefined();
   });
 
-  it("should render a review component after records are fetched", async () => {
+  it("should render a review component after records are fetched and allow for a rollback", async () => {
     let node = renderSimpleReview({
       async fetchRecords() {
         return [];
       },
     });
+    await waitForElementToBeRemoved(() => node.queryByTestId("spinner"));
 
     expect(node.queryByText("Rollback")).toBeDefined();
     expect(node.container.querySelector(".simple-review-header")).toBeDefined();
+
+    // also check rollback calls history.push while we're here
+    fireEvent.click(node.queryByText(/Rollback changes/));
+    fireEvent.click(node.queryByText("Rollback"));
+    expect(useHistory().push).toHaveBeenCalled();
   });
 
   it("should hide the rollback button if the hideRollback query parameter is provided", async () => {
@@ -144,6 +171,7 @@ describe("SimpleTest component", () => {
         return [];
       },
     });
+    await waitForElementToBeRemoved(() => node.queryByTestId("spinner"));
 
     expect(node.queryByText("Rollback")).toBeNull();
     fakeLocation.search = "";
@@ -153,10 +181,9 @@ describe("SimpleTest component", () => {
     useLocalStorage.mockReturnValue([false, jest.fn()]);
     const session = sessionFactory();
     session.serverInfo.user.principals = [];
-    const node = renderSimpleReview({
+    renderSimpleReview({
       session,
     });
-    console.log(node.container.innerHTML);
     expect(Redirect).toHaveBeenCalled();
   });
 });