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

Jump to bottom

Add frontend support for Manage Users pagination #8371

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Add frontend support for Manage Users pagination #8371

wants to merge 26 commits into from

Conversation

mpbrown
Copy link
Collaborator

@mpbrown mpbrown commented Dec 24, 2024 •
edited
Loading

FRONTEND PULL REQUEST

Related Issue

Changes Proposed

  • Adds pagination to the Manage Users side nav
  • Updates the routing for the default Settings page to use settings/users/:pageNumber so that the page number can be retrieved from the url params
  • Adds a "No results found" message with a button to clear the search filter

Testing

  • Deployed on dev5
  • Select Access organization account from the support admin dashboard
  • Select Big Organization, type in "test" for justification, and click Access data
  • Select either facility
  • Go to the Settings page and it should bring you to Manage Users
  • Test that pagination works as expected

Screenshots

Page 1 of results
image

Page 5 of results
image

Last page of results
image

Search results with multiple pages
image

Search results with single result page
image

No results found
image

@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-frontend branch 3 times, most recently from 52233eb to 31b2d0a Compare December 26, 2024 19:05
@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-backend branch from c3ecb82 to 3a6b61e Compare December 26, 2024 19:11
@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-frontend branch from 31b2d0a to b2174fb Compare December 26, 2024 19:20
@mpbrown mpbrown mentioned this pull request Dec 26, 2024
Merged
@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-backend branch from 3a6b61e to d3166f1 Compare January 2, 2025 16:57
Base automatically changed from mike/8103-manage-users-pagination-backend to main January 15, 2025 22:36
@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-frontend branch from 21a5f9b to 86fc1ca Compare January 16, 2025 15:18
@mpbrown mpbrown mentioned this pull request Jan 24, 2025
Merged
@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-frontend branch from cc6a935 to c7c7652 Compare January 27, 2025 16:29
@mpbrown mpbrown force-pushed the mike/8103-manage-users-pagination-frontend branch from c7c7652 to e6a3da8 Compare January 28, 2025 22:25
mpbrown
mpbrown commented Jan 29, 2025
View reviewed changes
backend/src/main/java/gov/cdc/usds/simplereport/service/ApiUserService.java
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

@mpbrown
Copy link
Collaborator Author

mpbrown commented Feb 10, 2025

Non blocking nit: The styling and spacing for the page toggles is inconsistent between the 3 states of first page, any middle page, and last page. Screenshots below:

I believe this is intentional so there is not left padding of white space on the first page, but tagging @kenieh to confirm

@mpbrown
Copy link
Collaborator Author

mpbrown commented Feb 10, 2025

@emyl3 I updated the search filtering so now it should handle more flexible search queries like the results page does, thanks for finding that!

@mpbrown mpbrown requested a review from bobbywells52 February 10, 2025 20:31
arinkulshi-skylight
arinkulshi-skylight previously approved these changes Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@arinkulshi-skylight arinkulshi-skylight left a comment

Choose a reason for hiding this comment

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

I have tested the changes in dev5. LGTM!

bobbywells52
bobbywells52 previously approved these changes Feb 10, 2025
View reviewed changes
emyl3
emyl3 previously approved these changes Feb 11, 2025
View reviewed changes
Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

LGTM! Left a comment about the redirect fix I initially proposed 😓 Everything else works as expected!! Thank you for all your work on this.

emyl3
emyl3 reviewed Feb 11, 2025
View reviewed changes
frontend/src/app/Settings/Settings.tsx Outdated
@@ -10,6 +12,10 @@ import { ManageSelfRegistrationLinksContainer } from "./ManageSelfRegistrationLi
import "./Settings.scss";

const Settings = () => {
const [facility] = useSelectedFacility();
const activeFacilityId = facility?.id || "";
const settingsIndexRedirect = "/settings/users/1?" + activeFacilityId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need the "/settings/users/1?facility=" + activeFacilityId here? (specifically adding facility=?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😬 face palm yes we definitely should need that here! Somehow the redirect still worked but that's probably why it's still hitting that other facility select screen. Thank you for catching this!

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

emyl3
emyl3 approved these changes Feb 11, 2025
View reviewed changes
Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

checked that change locally! and retested the other functionality on dev5 -- LGTM!!! Thank you, Mike!!! 🤩

@mpbrown
Copy link
Collaborator Author

mpbrown commented Feb 13, 2025

Design final approval from @kenieh on Slack linked here ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyl3 emyl3 emyl3 approved these changes

@mehansen mehansen Awaiting requested review from mehansen

@DanielSass DanielSass Awaiting requested review from DanielSass

@kenieh kenieh Awaiting requested review from kenieh

@DavidMcClatchey DavidMcClatchey Awaiting requested review from DavidMcClatchey

@khayeni-SL khayeni-SL Awaiting requested review from khayeni-SL

@bobbywells52 bobbywells52 Awaiting requested review from bobbywells52

@arinkulshi-skylight arinkulshi-skylight Awaiting requested review from arinkulshi-skylight

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add pagination to "Manage users" page for user list
5 participants
@mpbrown @emyl3 @khayeni-SL @bobbywells52 @arinkulshi-skylight