Merge pull request #4815 from minrk/admin-test

admin: don't use state change to update offset
jupyterhub · May 16, 2024 · 282cc02 · 282cc02
2 parents 6912a5a + cedf237
commit 282cc02
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 58 deletions.
diff --git a/jsx/src/components/Groups/Groups.jsx b/jsx/src/components/Groups/Groups.jsx
@@ -14,8 +14,7 @@ const Groups = (props) => {
   const dispatch = useDispatch();
   const navigate = useNavigate();
 
-  const { setOffset, offset, handleLimit, limit, setPagination } =
-    usePaginationParams();
+  const { offset, handleLimit, limit, setPagination } = usePaginationParams();
 
   const total = groups_page ? groups_page.total : undefined;
 
@@ -32,11 +31,22 @@ const Groups = (props) => {
     });
   };
 
+  // single callback to reload the page
+  // uses current state, or params can be specified if state
+  // should be updated _after_ load, e.g. offset
+  const loadPageData = (params) => {
+    params = params || {};
+    return updateGroups(
+      params.offset === undefined ? offset : params.offset,
+      params.limit === undefined ? limit : params.limit,
+    )
+      .then((data) => dispatchPageUpdate(data.items, data._pagination))
+      .catch((err) => setErrorAlert("Failed to update group list."));
+  };
+
   useEffect(() => {
-    updateGroups(offset, limit).then((data) =>
-      dispatchPageUpdate(data.items, data._pagination),
-    );
-  }, [offset, limit]);
+    loadPageData();
+  }, [limit]);
 
   if (!groups_data || !groups_page) {
     return <div data-testid="no-show"></div>;
@@ -72,8 +82,10 @@ const Groups = (props) => {
             limit={limit}
             visible={groups_data.length}
             total={total}
-            next={() => setOffset(offset + limit)}
-            prev={() => setOffset(offset - limit)}
+            next={() => loadPageData({ offset: offset + limit })}
+            prev={() =>
+              loadPageData({ offset: limit > offset ? 0 : offset - limit })
+            }
             handleLimit={handleLimit}
           />
         </Card.Body>

diff --git a/jsx/src/components/Groups/Groups.test.js b/jsx/src/components/Groups/Groups.test.js
@@ -112,8 +112,8 @@ test("Renders nothing if required data is not available", async () => {
   expect(noShow).toBeVisible();
 });
 
-test("Interacting with PaginationFooter causes state update and refresh via useEffect call", async () => {
-  let upgradeGroupsSpy = mockAsync();
+test("Interacting with PaginationFooter causes page refresh", async () => {
+  let updateGroupsSpy = mockAsync();
   let setSearchParamsSpy = mockAsync();
   let searchParams = new URLSearchParams({ limit: "2" });
   useSearchParams.mockImplementation(() => [
@@ -125,11 +125,11 @@ test("Interacting with PaginationFooter causes state update and refresh via useE
   ]);
   let _, setSearchParams;
   await act(async () => {
-    render(groupsJsx(upgradeGroupsSpy));
+    render(groupsJsx(updateGroupsSpy));
     [_, setSearchParams] = useSearchParams();
   });
 
-  expect(upgradeGroupsSpy).toBeCalledWith(0, 2);
+  expect(updateGroupsSpy).toBeCalledWith(0, 2);
 
   var lastState =
     mockReducers.mock.results[mockReducers.mock.results.length - 1].value;
@@ -140,9 +140,7 @@ test("Interacting with PaginationFooter causes state update and refresh via useE
   await act(async () => {
     fireEvent.click(next);
   });
-  expect(setSearchParamsSpy).toBeCalledWith("limit=2&offset=2");
-
-  // FIXME: mocked useSelector, state seem to prevent updateGroups from being called
-  // making the test environment not representative
-  // expect(callbackSpy).toHaveBeenCalledWith(2, 2);
+  expect(updateGroupsSpy).toBeCalledWith(2, 2);
+  // mocked updateGroups means callback after load doesn't fire
+  // expect(setSearchParamsSpy).toBeCalledWith("limit=2&offset=2");
 });
diff --git a/jsx/src/components/ServerDashboard/ServerDashboard.jsx b/jsx/src/components/ServerDashboard/ServerDashboard.jsx
@@ -41,7 +41,7 @@ const ServerDashboard = (props) => {
   let user_data = useSelector((state) => state.user_data);
   const user_page = useSelector((state) => state.user_page);
 
-  const { setOffset, offset, setLimit, handleLimit, limit, setPagination } =
+  const { offset, setLimit, handleLimit, limit, setPagination } =
     usePaginationParams();
 
   const name_filter = searchParams.get("name_filter") || "";
@@ -123,26 +123,39 @@ const ServerDashboard = (props) => {
       } else {
         params.set("state", new_state_filter);
       }
-      console.log("setting search params", params.toString());
       return params;
     });
   };
 
   // the callback to update the displayed user list
-  const updateUsersWithParams = () =>
-    updateUsers({
-      offset,
+  const updateUsersWithParams = (params) => {
+    if (params) {
+      if (params.offset !== undefined && params.offset < 0) {
+        params.offset = 0;
+      }
+    }
+    return updateUsers({
+      offset: offset,
       limit,
       name_filter,
       sort,
       state: state_filter,
+      ...params,
     });
+  };
 
-  useEffect(() => {
-    updateUsersWithParams()
+  // single callback to reload the page
+  // uses current state, or params can be specified if state
+  // should be updated _after_ load, e.g. offset
+  const loadPageData = (params) => {
+    return updateUsersWithParams(params)
       .then((data) => dispatchPageUpdate(data.items, data._pagination))
       .catch((err) => setErrorAlert("Failed to update user list."));
-  }, [offset, limit, name_filter, sort, state_filter]);
+  };
+
+  useEffect(() => {
+    loadPageData();
+  }, [limit, name_filter, sort, state_filter]);
 
   if (!user_data || !user_page) {
     return <div data-testid="no-show"></div>;
@@ -172,14 +185,7 @@ const ServerDashboard = (props) => {
           action(user.name, server.name)
             .then((res) => {
               if (res.status < 300) {
-                updateUsersWithParams()
-                  .then((data) => {
-                    dispatchPageUpdate(data.items, data._pagination);
-                  })
-                  .catch(() => {
-                    setIsDisabled(false);
-                    setErrorAlert(`Failed to update users list.`);
-                  });
+                loadPageData();
               } else {
                 setErrorAlert(`Failed to ${name.toLowerCase()}.`);
                 setIsDisabled(false);
@@ -519,13 +525,7 @@ const ServerDashboard = (props) => {
                         return res;
                       })
                       .then((res) => {
-                        updateUsersWithParams()
-                          .then((data) => {
-                            dispatchPageUpdate(data.items, data._pagination);
-                          })
-                          .catch(() =>
-                            setErrorAlert(`Failed to update users list.`),
-                          );
+                        loadPageData();
                         return res;
                       })
                       .catch(() => setErrorAlert(`Failed to start servers.`));
@@ -556,13 +556,7 @@ const ServerDashboard = (props) => {
                         return res;
                       })
                       .then((res) => {
-                        updateUsersWithParams()
-                          .then((data) => {
-                            dispatchPageUpdate(data.items, data._pagination);
-                          })
-                          .catch(() =>
-                            setErrorAlert(`Failed to update users list.`),
-                          );
+                        loadPageData();
                         return res;
                       })
                       .catch(() => setErrorAlert(`Failed to stop servers.`));
@@ -590,8 +584,13 @@ const ServerDashboard = (props) => {
           limit={limit}
           visible={user_data.length}
           total={total}
-          next={() => setOffset(offset + limit)}
-          prev={() => setOffset(offset - limit)}
+          // don't trigger via setOffset state change,
+          // which can cause infinite cycles.
+          // offset state will be set upon reply via setPagination
+          next={() => loadPageData({ offset: offset + limit })}
+          prev={() =>
+            loadPageData({ offset: limit > offset ? 0 : offset - limit })
+          }
           handleLimit={handleLimit}
         />
         <br></br>

diff --git a/jsx/src/components/ServerDashboard/ServerDashboard.test.js b/jsx/src/components/ServerDashboard/ServerDashboard.test.js
@@ -608,7 +608,7 @@ test("Search for user calls updateUsers with name filter", async () => {
   // expect(mockUpdateUsers).toBeCalledWith(0, 100, "ab");
 });
 
-test("Interacting with PaginationFooter causes state update and refresh via useEffect call", async () => {
+test("Interacting with PaginationFooter requests page update", async () => {
   await act(async () => {
     render(serverDashboardJsx());
   });
@@ -625,14 +625,10 @@ test("Interacting with PaginationFooter causes state update and refresh via useE
     jest.runAllTimers();
   });
 
-  expect(searchParams.get("offset")).toEqual("2");
-  expect(searchParams.get("limit")).toEqual("2");
-
-  // FIXME: should call updateUsers, does in reality.
-  // tests don't reflect reality due to mocked state/useSelector
-  // unclear how to fix this.
-  // expect(callbackSpy.mock.calls).toHaveLength(2);
-  // expect(callbackSpy).toHaveBeenCalledWith(2, 2, "");
+  expect(mockUpdateUsers).toBeCalledWith({
+    ...defaultUpdateUsersParams,
+    offset: 2,
+  });
 });
 
 test("Server delete button exists for named servers", async () => {