Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add frontend support for Manage Users pagination #8371

Merged
merged 26 commits into from
Feb 13, 2025
Merged

Add frontend support for Manage Users pagination #8371

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f7d6075
Add frontend pagination on manage users
mpbrown Dec 18, 2024
a792c4b
Add paginated search frontend
mpbrown Dec 24, 2024
7882003
Update snapshots
mpbrown Dec 26, 2024
3a698f0
Handle total users in org
mpbrown Jan 17, 2025
812ad30
Update styling for Manage Users
mpbrown Jan 27, 2025
e6a3da8
Run graphql codegen
mpbrown Jan 28, 2025
9cb7122
Update snapshot
mpbrown Jan 28, 2025
0c83837
Handle out of bounds requested page index
mpbrown Jan 29, 2025
625bd39
Update page size to 14
mpbrown Feb 3, 2025
1cb6838
Remove left padding for pagination on manage users
mpbrown Feb 3, 2025
485485f
Move search to query param for easier redirects
mpbrown Feb 3, 2025
e178422
Update filter effect
mpbrown Feb 3, 2025
ffe8864
Use debounced effect hook for query string
mpbrown Feb 4, 2025
1115978
Update test
mpbrown Feb 4, 2025
75f3ee5
Update page size to 12
mpbrown Feb 4, 2025
f95d192
Set page group size to 5
mpbrown Feb 4, 2025
a6ce8d3
Use constant page size to fix frontend pagination math bug
mpbrown Feb 5, 2025
9ad3181
Update mock name in test
mpbrown Feb 5, 2025
31e1517
Remove button id
mpbrown Feb 10, 2025
3c3ff4f
Add settings page index redirect
mpbrown Feb 10, 2025
e47766c
Add more flexible search filtering
mpbrown Feb 10, 2025
3452eca
Update test entries per page
mpbrown Feb 10, 2025
b5ee413
Update lighthouse url for settings page
mpbrown Feb 10, 2025
f23eb3c
Add active facility id for new redirect
mpbrown Feb 11, 2025
c818e21
Appease eslint import order
mpbrown Feb 11, 2025
e44c7c0
Fix facility query param redirect
mpbrown Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions backend/src/main/java/gov/cdc/usds/simplereport/service/ApiUserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,17 @@ public ManageUsersPageWrapper getPagedUsersAndStatusInCurrentOrg(
}

int totalSearchResults = filteredUsers.size();
int startIndex = totalSearchResults > pageSize ? pageNumber * pageSize : 0;
int endIndex = Math.min((startIndex + pageSize), filteredUsers.size());

int startIndex = pageNumber * pageSize;

boolean onlyOnePageOfResults = totalSearchResults <= pageSize;
boolean requestedPageOutOfBounds = startIndex > totalSearchResults;

if (onlyOnePageOfResults || requestedPageOutOfBounds) {
startIndex = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to handle this edge case because I encountered an error when I was on page 7 of all results and entered a search query. The app requested page 7 of the search results and the backend calculated the start index as a higher index than the end index. This caused an illegal argument error when attempting to create the sublist

}

int endIndex = Math.min((startIndex + pageSize), totalSearchResults);

List<ApiUserWithStatus> filteredSublist = filteredUsers.subList(startIndex, endIndex);

Expand Down
14 changes: 14 additions & 0 deletions backend/src/test/java/gov/cdc/usds/simplereport/service/ApiUserServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,20 @@ void searchUsersAndStatusInCurrentOrgPaged_success() {
checkApiUserWithStatus(users.get(0), "[email protected]", "Bobberoo", UserStatus.ACTIVE);
}

@Test
@WithSimpleReportOrgAdminUser
void searchUsersAndStatusInCurrentOrgPaged_pageNumberOutOfBounds_success() {
initSampleData();
ManageUsersPageWrapper usersPageWrapper =
_service.getPagedUsersAndStatusInCurrentOrg(25, 10, "Will");
Page<ApiUserWithStatus> usersPage = usersPageWrapper.getPageContent();
List<ApiUserWithStatus> users = usersPage.stream().toList();
assertEquals(1, users.size());
assertEquals(6, usersPageWrapper.getTotalUsersInOrg());
checkApiUserWithStatus(
users.get(0), "[email protected]", "Williams", UserStatus.ACTIVE);
}

@Test
@WithSimpleReportOrgAdminUser
void getUser_withAdminUser_withOktaMigrationDisabled_success() {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/app/Settings/Settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const Settings = () => {
path={"self-registration"}
element={<ManageSelfRegistrationLinksContainer />}
/>
<Route path="/" element={<ManageUsersContainer />} />
<Route path="users/:pageNumber" element={<ManageUsersContainer />} />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot we could also add something like the following:

<Route index element={<Navigate replace to="users/1"/>}/>

to default it to the users page like before if someone were to navigate to /settings

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I just added this redirect

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the urls we have configured for the Lighthouse checks since adding this new redirect confused it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo ok so I just tested this with an org with multiple facilities and it prompts the user to select the facility dropdown twice... 😓 I'll look in to seeing how we can get this to work for multiple facilities... Feel free to remove this redirect since everything else works as expected and I don't this was even required but a nice to have! My bad, Mike!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that's because we'd have to add the active facility id to the search params like we do in the ManageUsersContainer component. I can test that out and see if it works

let searchParams: Record<string, string> = {
      facility: activeFacilityId,
    };
...
navigate({
      pathname: "/settings/users/1",
      search: new URLSearchParams(searchParams).toString(),
    });

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emyl3 I'm redeploying to dev5 with a fix for this, although it will still prompt the user to select which facility but only once

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo wow ok! Thank you for that quick fix. That prompt happening once totally makes sense!

</Routes>
</div>
</div>
Expand Down
6 changes: 5 additions & 1 deletion frontend/src/app/Settings/SettingsNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ const SettingsNav = () => {
<nav className="prime-secondary-nav" aria-label="Secondary navigation">
<ul className="usa-nav__secondary-links prime-nav">
<li className="usa-nav__secondary-item">
<LinkWithQuery to={`/settings`} end className={classNameByActive}>
<LinkWithQuery
to={`/settings/users/1`}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anywhere else we need to update the routing to this settings page? Like possibly from any email templates?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 ooo that's a good callout, Mike! Based on a quick search of the static site as well as our email templates in this repo I don't see anything linking to that.
Maybe we should check-in with Jayna or check our email templates in sendgrid to be sure we covered everything?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with Jayna and we confirmed there isn't anything else that needs to be updated for the settings route

end
className={classNameByActive}
>
Manage users
</LinkWithQuery>
</li>
Expand Down
8 changes: 8 additions & 0 deletions frontend/src/app/Settings/Users/ManageUsers.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@
margin: 1em auto;
}

.no-results-found {
width: 75%;
}

.clear-filter-button {
width: auto !important;
}

.usa-sidenav__item.users-sidenav-item button {
width: 100%;
}
Expand Down
106 changes: 46 additions & 60 deletions frontend/src/app/Settings/Users/ManageUsers.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ let reactivateUserAndResetPassword: (obj: any) => Promise<any>;
let resetUserPassword: (obj: any) => Promise<any>;
let resetUserMfa: (obj: any) => Promise<any>;
let resendUserActivationEmail: (obj: any) => Promise<any>;
let setDebouncedQueryString = jest.fn();

type TestContainerProps = {
children: React.ReactNode;
Expand Down Expand Up @@ -362,71 +363,20 @@ describe("ManageUsers", () => {
resendUserActivationEmail={resendUserActivationEmail}
updateUserName={updateUserName}
updateUserEmail={updateUserEmail}
currentPage={1}
entriesPerPage={10}
totalEntries={3}
debouncedQueryString={""}
setDebouncedQueryString={setDebouncedQueryString}
queryLoadingStatus={false}
totalUsersInOrg={3}
/>
</TestContainer>
);
await waitForElementToBeRemoved(() => screen.queryByText("?, ?"));
return { user: userEvent.setup(), ...renderControls };
};

it("is searchable", async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These search tests got moved to ManageUsersContainer.test.tsx since that is where search is being handled now

//given
const { user } = await renderAndWaitForLoad();

//when
const searchBox = screen.getByRole("searchbox", {
name: /search by name/i,
});

await user.type(searchBox, "john");

//then
await waitFor(() => {
expect(
screen.getByRole("tab", {
name: displayFullName("John", "", "Arthur"),
})
).toBeInTheDocument();
});
await waitFor(() => {
expect(
screen.queryByText(displayFullName("Bob", "", "Bobberoo"), {
exact: false,
})
).not.toBeInTheDocument();
});
});

it("displays no results message for empty filtered list", async () => {
//given
const { user } = await renderAndWaitForLoad();

//when
const searchBox = screen.getByRole("searchbox", {
name: /search by name/i,
});
await user.type(searchBox, "john wick");

//then
await waitFor(() => {
expect(
screen.queryByText(displayFullName("Jane", "", "Doe"), {
exact: false,
})
).not.toBeInTheDocument();
});

await waitFor(() => {
expect(
screen.queryByText(displayFullName("Bob", "", "Bobberoo"), {
exact: false,
})
).not.toBeInTheDocument();
});

expect(screen.getByText("No results found.")).toBeInTheDocument();
});

it("enables logged-in user's settings except deletion and roles", async () => {
const { user } = await renderAndWaitForLoad();
const nameButton = screen.getByRole("tab", {
Expand Down Expand Up @@ -773,6 +723,13 @@ describe("ManageUsers", () => {
resendUserActivationEmail={resendUserActivationEmail}
updateUserName={() => Promise.resolve()}
updateUserEmail={() => Promise.resolve()}
totalEntries={0}
entriesPerPage={10}
currentPage={1}
debouncedQueryString={""}
setDebouncedQueryString={setDebouncedQueryString}
queryLoadingStatus={false}
totalUsersInOrg={0}
/>
</TestContainer>
);
Expand All @@ -783,8 +740,9 @@ describe("ManageUsers", () => {

it("fails gracefully when there are no users", async () => {
await renderWithNoUsers();
const noUsers = await screen.findByText("no users", { exact: false });
expect(noUsers).toBeInTheDocument();
expect(
screen.getByText("There are no users in this organization.")
).toBeInTheDocument();
});

it("adds a user when zero users exist", async () => {
Expand Down Expand Up @@ -835,6 +793,13 @@ describe("ManageUsers", () => {
resendUserActivationEmail={resendUserActivationEmail}
updateUserName={() => Promise.resolve()}
updateUserEmail={() => Promise.resolve()}
totalEntries={2}
entriesPerPage={10}
currentPage={1}
debouncedQueryString={""}
setDebouncedQueryString={setDebouncedQueryString}
queryLoadingStatus={false}
totalUsersInOrg={2}
/>
</TestContainer>
);
Expand Down Expand Up @@ -875,6 +840,13 @@ describe("ManageUsers", () => {
resendUserActivationEmail={resendUserActivationEmail}
updateUserName={() => Promise.resolve()}
updateUserEmail={() => Promise.resolve()}
totalEntries={2}
entriesPerPage={10}
currentPage={1}
debouncedQueryString={""}
setDebouncedQueryString={setDebouncedQueryString}
queryLoadingStatus={false}
totalUsersInOrg={2}
/>
</TestContainer>
);
Expand Down Expand Up @@ -916,6 +888,13 @@ describe("ManageUsers", () => {
resendUserActivationEmail={resendUserActivationEmail}
updateUserName={() => Promise.resolve()}
updateUserEmail={() => Promise.resolve()}
totalEntries={2}
entriesPerPage={10}
currentPage={1}
debouncedQueryString={""}
setDebouncedQueryString={setDebouncedQueryString}
queryLoadingStatus={false}
totalUsersInOrg={2}
/>
</TestContainer>
);
Expand Down Expand Up @@ -1013,6 +992,13 @@ describe("ManageUsers", () => {
resendUserActivationEmail={resendUserActivationEmail}
updateUserName={() => Promise.resolve()}
updateUserEmail={() => Promise.resolve()}
totalEntries={3}
entriesPerPage={10}
currentPage={1}
debouncedQueryString={""}
setDebouncedQueryString={setDebouncedQueryString}
queryLoadingStatus={false}
totalUsersInOrg={3}
/>
);
render(
Expand Down
Loading
Loading