-
Notifications
You must be signed in to change notification settings - Fork 84
Support duplicates request status in the UI #6999
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.
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
Support duplicates request status in the UI #6999
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile Summary
Confidence Score: 4/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
| View {duplicateCount} duplicate{" "} | ||
| {pluralize(duplicateCount, "request", "requests")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: incorrect pluralization logic creates grammatically incorrect output
The current code produces: "View 5 duplicate requests" (correct) but "View 1 duplicate request" (incorrect - should be "View 1 duplicate request").
The pluralize function takes parameters (count, singular, plural) but you're passing the noun twice and pluralizing "request"/"requests" separately. This creates "1 duplicate request" instead of the expected behavior.
| View {duplicateCount} duplicate{" "} | |
| {pluralize(duplicateCount, "request", "requests")} | |
| View {duplicateCount} {pluralize(duplicateCount, "duplicate request", "duplicate requests")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"View 1 duplicate request" (incorrect - should be "View 1 duplicate request").
lol
clients/admin-ui/src/features/privacy-requests/dashboard/DuplicateRequestsLink.tsx
Outdated
Show resolved
Hide resolved
| ), | ||
| status: parseAsArrayOf(parseAsStringEnum(allowedStatusFilterOptions)) | ||
| .withDefault(defaultStatusFilter) | ||
| .withOptions({ clearOnDefault: true }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is something we should have used in the action center 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yess, that clearOnDefault is really nice. you don't get a bunch of params when you just visit the page.
speaker-ender
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and everything works in nightly!
…rt-new-dsr-request-status-Duplicated
Ticket ENG-1804
Description Of Changes
For both the current Request Manager page and the new Request Manager page, it adds the option to filter by duplicate request. It also adds a notice that appears in the header when you have requests in the duplicate status.
For the new request manager page, it sets the default status filter to show all filters except duplicate.
Code Changes
Steps to Confirm
For the new request manger:
7. Go to Settings > About page and enable the feature flag "Privacy request v2"
8. Go to the Privacy requests > Request manager (new) page
9. Check the default filter on this page is to filter by all status except duplicates and results don't show duplicates
10. Click the 'X duplicate requests' text to filter only by duplicates
11. Check that approve / deny / delete actions are possible
12. Check that selecting duplicate requests with the checkbox causes the Action dropdown to show approve / deny / delete actions
13. Check that if you navigate to another page in the meny and then click on "Request manager (new)" you will see again the default filter status of not showing duplicates
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works