Repository navigation
Priyanka: Prevent duplicate PR entries and retain added PRs - #5630
muthapriyanka wants to merge 5 commits into
Conversation
✅ Deploy Preview for highestgoodnetwork-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Tested c57b5ab as Administrator on /pr-grading-test → 91NePRT (Original), light mode. The vitest suites in PRGradingScreen/ pass (3 files, 11 tests). Saves to /weekly-grading/save were intercepted in the browser so nothing was written to the shared database.
What works:
- Adding
3100for Abi (already listed inside the pair3100 + 1410) shows "This PR number has already been added for this reviewer." and the count stays at 8 (screenshot 1).9999 + 3100is rejected the same way. - Adding
9998sends one save request and the count goes from 8 to 9 (screenshot 2). - When the save returns 500,
9997is not added, the count stays at 9 and "Could not save this PR. Please try again." is shown (screenshot 2).
Suggestions (none blocking):
- Coordinate with OneCommunityGlobal/HGNRest#2286. This sends
teamCodein the save body andteam=on the GET, while that PR renamesteamCodetoteamNamein the weekly-grading model and controller. Whichever merges second needs to match the other, or saves will fail. - The stored
prsReviewedincludes the sample PRs. The request for9998was{"reviewer":"Abi","prsReviewed":9,"gradedPrs":[{"prNumbers":"9998"}]}: 9 counts the 8 sample entries that only exist in the frontend, while the saved record has 1 PR. The screen recalculates the count on load, so it looks right here, but anything else readingweeklyGradingsgets a wrong number. - The record is keyed by team name (
pr-grading-screen:<teamName>). Renaming a custom team, or two custom teams with the same name, would lose or share saved PRs. The team_idwould be a stable key. - Only the add is saved. A grade changed after adding is still local, so after a refresh a saved PR is back to "Okay". Fine if that's planned for a follow-up, but worth saying in the description.
muthapriyanka
changed the title
muthapriyanka
added
the
High Priority - Please Review First
AaditTrivedi
left a comment
AaditTrivedi
left a comment
There was a problem hiding this comment.
Reviewed by Aadit Trivedi.
The branch has merge conflicts with development in src/components/PRGradingScreen/PRGradingScreen.jsx and PRGradingScreen.module.css (checked with git merge-tree). Both files were changed by #5491 and #5493, which are now merged, so this PR needs to be rebuilt on top of their version of the grading screen.
When rebasing, please also check the open points from the PR Review Team dashboard status report: the task's "freeze reviewed PRs" part isn't covered, it filters on teamCode, which BE #2286 renames to teamName, and grade changes reportedly don't persist after a refresh.
Required action
Please update the branch with development and resolve the conflicts. Happy to test once it's updated.
There was a problem hiding this comment.
Reviewed by Alekya.
How I tested
Checked out the PR branch (commit 25d7dcf) on Node 20.19.4 with the backend on development. Ran the three focused test files (3 files, 16 tests pass). Logged in as my Admin test account and opened /pr-grading-test , 91NePRT (Original). After the screen loaded I set Chrome DevTools to Offline so no save could reach the shared database.
Verified working
- Adding 2045 for Abi (already listed) shows "This PR number has already been added for this reviewer." and the count stays at 9.
- Adding 3100 (part of the existing pair 3100 + 1410) is rejected the same way and the count stays at 9.
- Adding a new number (9997) while the save fails shows "Could not save this PR. Please try again.", the number is not added and the count stays at 9.
Not tested
- Adding a new pair (for example 9981 + 9982)
rahulrubugunday
left a comment
rahulrubugunday
left a comment
There was a problem hiding this comment.
Tested locally on Priyanka_pr_grading_duplicates_persistence at 25d7dcf as DevAdmin, with backend on development, on the weekly PR grading screen (91NePRT test team).
- Added a test PR (9999) to a reviewer with no PRs. PR reviewed count went from 0 to 1 and matches the PR numbers listed.
- Adding 9999 again for the same reviewer is blocked with "This PR number has already been added for this reviewer."
- Changed the grade for 9999 from Okay to Exceptional.
- After refreshing the page, 9999 is still in the reviewer's row and still graded Exceptional. PRs saved earlier by others also load correctly.
The fix works, but the branch has merge conflicts with development in PRGradingScreen.jsx and PRGradingScreen.module.css, so it can't be merged as is. Please resolve them and I'll re-test.
Requesting changes.
snehitakandula
left a comment
snehitakandula
left a comment
There was a problem hiding this comment.
Reviewed on this PR's branch (pr-5630) at the latest commit 25d7dcf, frontend only, with backend on development. Tested as Admin on /pr-grading-test → 91NePRT (Original), focusing on the pair case listed as not tested in the earlier review.
Verified:
- Tests:
npx vitest run src/components/PRGradingScreenpasses (3 files, 16 tests). - New pair counts as one entry: adding
9981 + 9982for Lin Khant shows it as a single entry next to 2468, and the reviewed count is 2. - Pair persists after refresh:
9981 + 9982is still listed for Lin Khant after reloading the page. - Numbers inside a pair are protected:
- Adding
9981alone is rejected with "This PR number has already been added for this reviewer." - Adding
9982 + 9983is also rejected, since 9982 is already used in the existing pair.
- Adding
Requesting changes: merge conflicts
GitHub reports conflicts in src/components/PRGradingScreen/PRGradingScreen.jsx and PRGradingScreen.module.css, from #5491 and #5493 (confirming @AaditTrivedi and @rahulrubugunday). Please rebuild on the latest development. The duplicate checks and persistence work as described, so they should carry over once rebased.
Screenshots:
Tests passing (16/16):
Pair 9981 + 9982 added as one entry:
9981 rejected as a duplicate:
Requesting changes.
iAbhi001
left a comment
iAbhi001
left a comment
There was a problem hiding this comment.
Tested locally on branch pr-5630 (25d7dcf) frontend-only with backend on development as Admin on /pr-grading-test, focusing on the previously untested pair scenarios. All 16 unit tests across 3 files pass, adding a pair (9981 + 9982) correctly aggregates into a single reviewed entry, and the entry persists reliably across page reloads. Duplicate validation works as intended by blocking individual numbers from existing pairs as well as overlapping new pairs. However, requesting changes due to active merge conflicts with latest development from recent merges in the component and styling files; please rebase onto development to resolve the conflicts so this can proceed.
vidiyala99
left a comment
vidiyala99
left a comment
There was a problem hiding this comment.
Re-reviewed at e9f6097 (my earlier review was dismissed by the new commits).
Tested the Weekly PR grading screen in a browser in light and dark mode (the component rendered on its own with a mocked store). /weekly-grading and /weekly-grading/save were replaced by a small stateful mock that behaves like the HGNRest controller (rejects grades outside Unsatisfactory/Okay/Exceptional/No Correct Image, merges saved PRs per PR number), so nothing touched a real database. The vitest suites in PRGradingScreen/ pass (4 files, 28 tests).
Results:
- Changing a grade now saves:
2200to Exceptional and4100to Cannot find image both survive a refresh (screenshot 2). - Cannot find image is sent as
No Correct Image(the backend enum value) and shown as Cannot find image again after reload. Before this commit that choice was never saved. - Adding
9998for Abi saves and is still listed after refresh with the count at 3. Adding3100(already inside3100 + 1410) shows "This PR number has already been added for this reviewer." and nothing is saved (screenshot 1). - When the save returns 500, "Could not save this grade. Please try again." is shown and the previous grade stays checked (screenshot 3).
- The save body now carries both
teamCodeandteamName, so it keeps working whichever way HGNRest#2286 lands.
Dark mode (screenshot 4) renders the same state. The page wrapper is not part of my harness, so I only judged the grading panels themselves.
Still true from my first review, none blocking:
- The saved record is keyed by team name (
pr-grading-screen:<teamName>), so two custom teams with the same name would share data. The team_idwould be a stable key. prsReviewedin the saved record counts the sample PRs that only exist in the frontend (Abi's record said 3 with 2 stored PRs). The screen recalculates on load, so it looks right here, but anything else readingweeklyGradingsgets the larger number.
|
Description
The Weekly PR grading screen allowed the same PR number to be added more than once for a reviewer. Newly added PRs also disappeared after refreshing the page because they were held only in browser state.
This PR prevents duplicate PR numbers for the same reviewer and saves newly added PRs through the existing weekly-grading API. Saved entries are loaded again for the selected team and week after refresh. A frontend/backend pair counts as one reviewed entry.
Related PRs
Main changes explained
How to test
/pr-grading-test. Select a original to open the Weekly PR grading screen.9981 + 9982. Confirm it appears as one reviewed entry. Try reusing either number for that reviewer and confirm it is rejected.node node_modules/vitest/vitest.mjs run src/components/PRGradingScreen/PRGradingScreen.duplicates.test.jsx src/components/PRGradingScreen/PRGradingScreenContainer.test.jsx src/components/PRGradingScreen/prGradingPersistence.test.jsVideos of changes
Recording.mp4