Repository navigation
Swathi promotion eligibility button func - #5574
SwathiAngadi wants to merge 25 commits into
Conversation
…ighestGoodNetworkApp into Swathi_Promotion_Eligibility_Button_Func
…ighestGoodNetworkApp into Swathi_Promotion_Eligibility_Button_Func
✅ Deploy Preview for highestgoodnetwork-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ighestGoodNetworkApp into Swathi_Promotion_Eligibility_Button_Func
manavkheni1
left a comment
There was a problem hiding this comment.
Testing performed: Pulled the branch locally, tested on localhost with both an Owner-tier and a Volunteer-tier account.
What works well:
- "New Member" / "Existing Member" dropdown filters correctly
- "+ Add new" PR entry works as expected — count and badge update correctly
- Dark mode renders cleanly across the table, modal, and buttons
- Permission checks are correctly enforced: Volunteer-tier accounts are blocked from viewing this page (403), and only Owner-tier accounts can click "Process Promotions"
Bug found: "Import from summary" shows a success toast ("PRs imported from weekly summary") even when the reviewer has no weekly summary submitted at all. No PR numbers or counts actually change, but the UI reports success regardless. Suggest validating that a summary exists before showing success, or showing a distinct "no PRs found to import" message.
Minor: Clicking "Process Promotions" as a non-Owner produced two identical "Only an Owner can process promotions" toasts. Also, the generic "Failed to load Reviewers" error doesn't distinguish a permissions block from a real error — a more specific message would help.
iAbhi001
left a comment
There was a problem hiding this comment.
🧪 Local Testing & QA Review
Pulled the branch locally and tested on localhost using both Owner-tier and Volunteer-tier accounts.
✅ What's Working Well
- Filtering: The "New Member" / "Existing Member" dropdown filters correctly.
- PR Entries: The
+ Add newPR entry works as expected, updating counts and badges properly. - UI/UX: Dark mode renders cleanly across the table, modals, and buttons.
- Permissions: Access control is properly enforced—Volunteer-tier accounts are correctly blocked with a 403, and only Owner-tier accounts can successfully trigger "Process Promotions".
🐛 Bugs & Edge Cases Found
- False-Positive Import Toast:
- Issue: Clicking "Import from summary" triggers a success toast (
"PRs imported from weekly summary") even when the reviewer has submitted no weekly summary at all. No data actually changes, but the UI misleadingly reports success. - Suggestion: Validate that a weekly summary exists and contains importable PRs before showing a success toast, or fallback to a distinct message like
"No PRs found to import".
- Duplicate Error Toasts:
- Issue: Clicking "Process Promotions" as a non-Owner produces two identical
"Only an Owner can process promotions"toasts simultaneously.
- Ambiguous Error Messaging:
- Issue: The generic
"Failed to load Reviewers"error toast doesn't distinguish between a standard network/server failure and a permission block. - Suggestion: Provide a more specific error message based on the response status (e.g., differentiating 403/404 errors from 500s).
vidiyala99
left a comment
There was a problem hiding this comment.
Tested locally against backend development (which includes the merged #2317) as Admin in light and dark mode, and as Volunteer (screenshots below). Not tested: the Owner-only steps (promotion preview, team Edit, Confirm), because I don't have an Owner account; findings 1 and 2 are from reading the code only. I also left the "+ Add new", "Import from summary" and "PRs Needed" controls alone, since they save straight to the shared dev database.
Working as Admin: the table loads, "Review for This Week" lists the reviewer groups, the grading modal filters by the group's letter range, and it's readable in dark mode. Clicking "Process Promotions" as a non-Owner showed a single "Only an Owner can process promotions." toast for me, so I couldn't reproduce the duplicate toast reported earlier.
New issues:
- (Code review only, not tested) The team picker in the promotion preview can only offer teams that are already in the preview.
promotionTeamOptionsis built frompromotionPreviewitself (.filter(item => item.teamId && item.teamName)), andhandleSavePromotionTeamalso looks the team name up inpromotionPreview. So when promoting one reviewer, the only choices are the team the backend already suggested or "No team assigned". If the preview suggests no team for anyone, the dropdown has nothing to pick. The options should come from the full team list (for examplestate.allTeamsData.allTeams, filtered to active teams). - (Code review only, not tested)
isPromotedis set after confirming but never read.handleConfirmPromotionsmarks promoted reviewers withisPromoted: true, but nothing in the component uses that flag. The row stays selectable (itsdisabledcondition only checksisOwner,promoteEligibleandprocessing), so the same reviewer can be selected and promoted again in the same session. Please includeisPromotedin the disabled condition and show a "Promoted" state in the row. - About 30 of the 42 changed files are unrelated to this feature. Most are stylelint auto-fixes in other modules (Email Management, BM Dashboard, Education Portal, Announcements, Community Portal and others), plus
fix-conflicts.js,package-lock.json,Badge.cssandStudentBadgeGallery.jsx. They look harmless, but they widen the regression surface and make merge conflicts with other open PRs likely. Please move them to a separate PR. (TheopenIssueCharts.jsximport change toIssueCharts.module.cssis actually a correct fix, since that is the file's casing in git, but it belongs in that separate PR too.) - The dark mode status colors never apply (screenshot 05).
.promo_table_container.dark .promo_table td.status_not_met { color: #ff8b82 }is overridden by the globalbody.dark-mode * { color: #ffffff !important; }inpublic/index.css, so "Has not Met" renders white. Other components work around that rule with!important(for exampleEventPageOrganizer.module.css); the same is needed forstatus_metandstatus_not_met.
Minor:
handleReviewOptionSelecthas threeconsole.logcalls that dump the full reviewer list to the console on every group selection.- Question (not from this PR, same on
development): "PRs Reviewed" isgradedPrs.length, so PRs rated "Did not review" (red) also count as reviewed. Is that intended? - Note for testers:
POST /promotion-eligibilitytook about 50 seconds on dev, so the table shows "Loading..." for a long time before data appears (backend, already merged).
adit24dhaya
left a comment
There was a problem hiding this comment.
Hi Swathi, I checked the existing review feedback and focused on a separate integration issue. The reviewer-group frontend calls do not send the requestor object that the current HGNRest routes require for their permission checks. That prevents the "Review for This Week" list from loading reliably and makes Owner group creates/edits fail.
I reproduced the exact action payloads with mocked Axios against this PR head: the read sends no body, and the two write actions send only the group fields. The focused test passed because it confirms those requests omit requestor; it made no backend request or database write.
Please pass currentUser through the three action functions and send { requestor: currentUser } for the read and { ...groupData, requestor: currentUser } for the writes. I did not repeat the unrelated-file, import-toast, promotion-preview, or dark-mode findings already documented by other reviewers.
| }; | ||
|
|
||
| export const fetchReviewerGroups = async () => { | ||
| const res = await axios.post(ENDPOINTS.REVIEWER_GROUPS); |
There was a problem hiding this comment.
PromotionEligibility passes currentUser into this action, but the parameter is discarded. The current HGNRest reviewer-group route deliberately uses POST because its permission check reads req.body.requestor; its create and update handlers also read req.body.requestor.role. This call therefore cannot meet the backend contract, and the same missing requestor payload appears in the create/update actions below. Please include it in all three requests.
adit24dhaya
left a comment
There was a problem hiding this comment.
Thanks for the follow-up commits. I rechecked 62ccad8 against my earlier review: the read now carries requestor, and the actions accept the caller. The current JWT middleware also supplies the authenticated requestor, so the remaining finding below is a routing problem even with an allowed Owner, not a claim of an authentication bypass.
The follow-up introduces two write-route regressions. createReviewerGroup now posts to REVIEWER_GROUPS rather than REVIEWER_GROUPS_NEW. In current backend development, POST /reviewer-groups is the list handler, so the create action receives 200 with { groups, warnings } and never invokes create. updateReviewerGroup now uses PUT, but the backend only registers PATCH /reviewer-groups/:groupKey; the update receives 404 and never invokes update. Please restore POST /reviewer-groups/new and PATCH /reviewer-groups/:groupKey, keeping the requestor handling, and add routed request regressions.
Validation: 14 isolated assertions passed using the actual frontend action functions and URL constants, actual Express router and reviewer-group controller. The current create produced a list response with zero mock create calls; current update returned 404 with zero mock update calls. The correct-endpoint controls returned 201/200 and invoked the mock create/update once each. The backend route/controller files are byte-identical to refreshed development dcaec860. Persistence, authentication and the permission gate are stand-ins; the router runs in-process without a server. Four focused frontend request tests and 73 existing backend tests in two suites passed on Node 20.19.4. No database connection, shared write or external API call was made. Full authenticated UI interactions were not retested.
Frontend production build passed with 9,184 modules. Changed-JS ESLint: zero errors/16 warnings. Changed-CSS Stylelint still reports 95 errors, and git diff --check against the PR merge base reports two trailing-whitespace lines in PermissionsManagement.module.css, so this is not a clean lint/whitespace run. I have not repeated the other reviewers' unrelated-scope and UI findings.
Commit audit: the three follow-up subjects are under 72 characters and have no trailing periods, but are vague/non-imperative and have no explanatory body or ticket reference. For the next fix, a focused imperative subject and a blank-separated what/why body mentioning this PR would make the history easier to follow. Requesting changes for the reproduced create/update route regressions.
| }; | ||
|
|
||
| export const createReviewerGroup = async (groupData, currentUser) => { | ||
| const res = await axios.post(ENDPOINTS.REVIEWER_GROUPS, { |
There was a problem hiding this comment.
This follow-up changes create to the collection read URL. The current backend intentionally uses POST /reviewer-groups for listing and POST /reviewer-groups/new for creation. Running this actual action through the real router/controller returns 200 with { groups, warnings } and zero create calls, even with an allowed Owner requestor. Please use REVIEWER_GROUPS_NEW again and cover the routed create response. The correct /new control returns 201 and invokes the mocked create once.
| }; | ||
|
|
||
| export const updateReviewerGroup = async (groupId, groupData, currentUser) => { | ||
| const res = await axios.put(`${ENDPOINTS.REVIEWER_GROUPS}/${groupId}`, { |
There was a problem hiding this comment.
The backend registers PATCH /reviewer-groups/:groupKey, not PUT. The actual current action returns 404 through the real router, with no update call; the PATCH control returns 200 and invokes the mocked update once. Please restore PATCH and keep passing the stable group key (the page already supplies selectedOption.key).
RichaSapre
left a comment
There was a problem hiding this comment.
Tested locally with an Admin account. The unauthorized account was blocked, while Admin access worked. The New Member/Existing Member filter, reviewer-group menu, search filter, table loading, and dark-mode status colors worked as expected. The Admin account could not activate the Owner-only Process Promotions action.
I agree with the existing blocking concerns about the reviewer-group routes: creation must use POST /reviewer-groups/new, and updating must use PATCH /reviewer-groups/:groupKey. I also agree with the existing findings about the promotion team picker, repeat promotion possibility, misleading import message, and unrelated files.
The Owner-only promotion flow was not tested because an Owner account was unavailable. Requesting changes until the blocking route issues are fixed.
vidiyala99
left a comment
There was a problem hiding this comment.
Re-checked my earlier points at 62ccad8 (after "Included reviewers suggestion and fixes").
Fixed:
- Team picker:
promotionTeamOptionsnow comes fromstate.allTeamsData.allTeams, filtered toisActive, andhandleSavePromotionTeamlooks the team up there. So any active team can be chosen, not only the ones already in the preview. - Repeat promotion: the row checkbox is now
disabled={!isOwner || !promoteEligible || isPromoted || processing}, so a reviewer promoted in this session can't be selected again.
Still open:
3. Unrelated files: the PR still changes 40 files, and 34 of them are outside the PR Dashboard / grading screens: stylelint auto-fixes across Email Management, BM Dashboard, Education Portal, Community Portal, Announcements and others, plus fix-conflicts.js, Badge.css and StudentBadgeGallery.jsx. Moving them to a separate PR would make this one much easier to review and merge.
I also agree with the reviewer-group route regression from the review of 62ccad8: create must keep using REVIEWER_GROUPS_NEW (POST /reviewer-groups/new), and update must keep using PATCH. That's the remaining blocker.
Requesting changes for item 3 and the route regression.
|
Hi vidiyala99, The review and promotion functionality is implemented, and I have addressed the earlier feedback on team selection and preventing repeat promotions. Regarding the unrelated stylelint changes, separating them may cause the repository-wide lint checks to fail if the existing violations remain. I'd prefer to keep the current changes together for now, provided the build and tests pass, rather than risk introducing CI failures. Would it be acceptable to keep these changes in this PR, or would you prefer that I move the unrelated stylelint fixes into a separate PR? I will also address the reviewer-group route regression before requesting another review. |
…ighestGoodNetworkApp into Swathi_Promotion_Eligibility_Button_Func
…ighestGoodNetworkApp into Swathi_Promotion_Eligibility_Button_Func
|





Description
Implement the front end for the below feature
You can refer to the Task in detail using this link:
https://docs.google.com/document/d/11KglDgr_l3-8NdTLMGfu6YGmpGj-BSip_LD8IQbCtp8/edit?pli=1&tab=t.0#heading=h.xfui197wwd7y
Search : 23. to go to the particular Task.
Related PRS (if any):
This frontend PR is related to the 2317 backend PR
Main changes explained:
Made changes in Promotion Eligibility JSX and CSS file.
Added new PRGRadingModal JSX and module.css file.
Made changes in PRGRadingSCreen JSX and module.css file.
How to test:
npm installand...to run this PR locallyScreenshots or videos of changes:
Screen.Recording.2026-09-05.002513.mp4
Screen.Recording.2026-09-19.014156.mp4
Note:
Create Test Data for Process Promotion feature.