Skip to content

Sitaram - PRs Needed bands, reviewer groups, weekly requirements, History, PR ratings and Process Promotions - #2317

Open
sitaram1921 wants to merge 7 commits into
developmentfrom
sitaram/feature/promotion-eligibility-dashboard-backend
Open

Sitaram - PRs Needed bands, reviewer groups, weekly requirements, History, PR ratings and Process Promotions#2317
sitaram1921 wants to merge 7 commits into
developmentfrom
sitaram/feature/promotion-eligibility-dashboard-backend

Conversation

@sitaram1921

@sitaram1921 sitaram1921 commented Aug 22, 2026

Copy link
Copy Markdown

Implements the backend half of doc item #23 (PRIORITY URGENT) from the PR Review Team Admin and Analytics Dashboard doc, "Finishing followup fixes for PR-4895". Frontend half is being built by Swathi Angadi.
image
image
image

The Promotion Eligibility page at /pr-dashboard/promotion-eligibility implemented almost none of its spec. requiredPRs was computed as pledgedHours / 2 against a hardcoded two-week window, which matches nothing in the spec, and there was no concept of reviewer groups at all. This PR delivers every requirement in the spec: the PRs Needed bands and Owner override, the reviewer group dropdown and its Owner editing, the rebuilt weekly requirement columns, the History column, the "+ Add New" column with per-PR ratings, and Process Promotions with team placement and a preview step.

On the reported "Failed to load Reviewers" / 403: investigated and it is a permissions-data problem, not a code defect. The endpoint gates on getReports, which on live dev is held by Administrator, Owner and Core Team but not by Manager, Mentor or Volunteer. Calling it as Administrator returns 200 with 1643 reviewers. The fix is the one PR-4895 already describes, an Admin granting "See Analytics Reports" per user. No code change was needed and none is included for it.

Related PRS (if any):

This backend PR is related to the frontend PR for doc item #23, #XXX (to be filled in when Swathi opens it).
To test the dropdown and Owner editing UI end to end you need to check out that frontend PR alongside this one. The endpoints here can also be exercised on their own in Postman, see below.

Main changes explained:

  • Create src/models/reviewerGroup.js for the "Review for This Week" groups. Deliberately stores no member list: a group owns an alphabetical range and membership is derived from it at read time, so membership needs no maintenance as volunteers join and leave.
  • Create src/helpers/reviewerGroupHelper.js holding all the pure range logic (letter normalisation, the grouping letter for a reviewer, range validation, overlap and gap detection, key slugification). Kept free of any database access so it is exhaustively unit testable.
  • Create src/controllers/reviewerGroupController.js with the three group handlers. The read seeds the spec's three defaults on first call, so no migration or seed script is needed and a fresh local database works immediately.
  • Update src/controllers/promotionEligibilityController.js to accept an optional groupKey and filter the table by it. Filtering happens before the per-reviewer queries, so a narrow group does proportionally less database work. Also adds the PRs Needed bands, committed-hours change detection, and the Owner-only override (first commit on this branch).
  • Update src/routes/promotionEligibilityRouter.js to mount the three new group routes alongside the existing ones.
  • Update src/startup/routes.js to pass the new model through to the router.
  • Update src/helpers/promotionEligibilityHelper.js with the weeks logic: summariseWeeks, weekMeetsRequirement, and mongoWeekOf, which reproduces Mongo's $year/$week in JavaScript. Kept pure and database free, next to the bands they depend on.
  • Update src/models/promotionEligibility.js with successfulWeeks, plus the promotion and placement fields.
  • Update src/models/team.js with optional hoursBand, standupDay, standupTime and standupTimezone. All default to null and a team missing any of them is not a placement candidate, so the 1000+ existing teams need no backfill and nothing outside this dashboard reads them.
  • Update src/controllers/teamController.js so postTeam and putTeam accept the placement fields. putTeam writes only the fields actually present in the body, because it assigns everything else unconditionally and reading these the same way would let the existing Teams page wipe a team's standup on an ordinary rename.
  • Create src/helpers/teamPlacementHelper.js with the spec's placement rules, kept pure and database free.
  • Create src/helpers/prEntryHelper.js and src/models/promotionPrEntry.js for the "+ Add New" column. A separate collection rather than an array on the promotionEligibility doc, because the spec asks for unlimited weeks and that document is rewritten on every dashboard read. A unique index on reviewer, year, week and PR number makes the summary import safe to re-run and turns a duplicate into a 409.
  • Create src/controllers/teamController.placement.spec.js. A separate file because the existing teamController.spec.js mocks permissions with jest.spyOn, which cannot reach putTeam: it calls the hasPermission it destructured at import time, not helper.hasPermission. That is also why putTeam had no coverage before.
  • Create src/helpers/reviewerGroupHelper.spec.js and src/controllers/reviewerGroupController.test.js, 75 tests, plus 29 more across the promotion eligibility helper and controller suites for the weeks work.

New endpoints

POST  /api/reviewer-groups            read groups, any getReports holder, seeds defaults
POST  /api/reviewer-groups/new        Owner only, add a group
PATCH /api/reviewer-groups/:groupKey  Owner only, rename and/or change range
PATCH /api/promotion-eligibility/:reviewerId/prs-needed   Owner only, override PRs Needed
POST  /api/promote-members/preview    proposed team placements, writes nothing
POST  /api/promotion-eligibility/pr-ratings                       the five rating options
POST  /api/promotion-eligibility/:reviewerId/pr-entries           read, grouped by week
POST  /api/promotion-eligibility/:reviewerId/pr-entries/new       add one by hand
POST  /api/promotion-eligibility/:reviewerId/pr-entries/import    populate from weekly summary
PATCH /api/promotion-eligibility/pr-entries/:entryId/rating       rate or clear

The dashboard read also gains a history array per reviewer, one entry per prior week with a precomputed belowRequirement flag.

POST /api/promote-members gains an optional placements array. Omitting it behaves exactly as before: role change only, no team touched.

POST /api/team and PUT /api/team/:teamId accept the four optional placement fields.

POST /api/promotion-eligibility gains an optional groupKey in the body. Omitting it behaves exactly as before, so the current frontend is unaffected.

How to test:

  1. Check out this branch, sitaram/feature/promotion-eligibility-dashboard-backend
  2. npm install, then run the backend (Redis must be running locally)
  3. Log in via POST /api/login and use the returned token as the Authorization header for everything below
  4. As Administrator or Owner, POST /api/reviewer-groups with body {}. Expect 200 and three groups: All Members (no range), 95XXPRT Members (A-N), 97XXPRT Members (O-Z), with warnings: []. Call it a second time and confirm it does not duplicate them.
  5. POST /api/promotion-eligibility with body {}. Expect the same result as on development, unchanged.
  6. POST /api/promotion-eligibility with body {"groupKey":"95xx"}. Expect only reviewers whose first name starts A-N (on dev: 892 of 1644, with 752 under 97xx, summing exactly). Accounts whose first name starts with a digit fall back to the last name, which is intended. Then {"groupKey":"nope"} and expect a 400.
  7. As an Owner, PATCH /api/reviewer-groups/95xx with {"label":"Renamed"}. Expect 200 and confirm the key is still 95xx.
  8. PATCH /api/reviewer-groups/95xx with {"rangeStart":"a","rangeEnd":"p"}. Expect 200, the range normalised to A and P, and a warning naming the O-P overlap with 97XXPRT.
  9. PATCH /api/reviewer-groups/95xx with {"rangeStart":"A","rangeEnd":"K"}. Expect a warning that no group covers L-N.
  10. POST /api/reviewer-groups/new with {"label":"99XXPRT Members","rangeStart":"L","rangeEnd":"N"}. Expect 201 and a derived key of 99xxprt-members.
  11. Guards. PATCH /api/reviewer-groups/all expect 403. An unknown key expect 404. A half range (rangeStart only) expect 400. A backwards range (N to A) expect 400. An empty body expect 400.
  12. PATCH /api/promotion-eligibility/<reviewerId>/prs-needed with {"prsNeeded":12}. Expect 200, prsNeededSource of ownerOverride, and requiredPRs matching. Re-read the dashboard and confirm it persisted. Then send {"prsNeeded":null} and confirm it returns to auto and recalculates from the bands.
  13. As Administrator, repeat step 12 and expect 403. All four write endpoints are Owner only.
  14. Weeks columns. POST /api/promotion-eligibility with body {} and check any row carries successfulWeeks, remainingWeeks and weeklyRequirementsMet. On dev successfulWeeks is 0 for everyone, which is correct: the best single week any dev account has is 4 distinct review tasks and the lowest bar is 7. To see a non-zero value, override a reviewer's PRs Needed down to 3 with step 12 and re-read.
  15. Team placement. Create a team with POST /api/team and body {"teamName":"Test 10hr","isActive":true,"hoursBand":"10-19.99","standupDay":"Tuesday","standupTime":"11AM"}. Then POST /api/promote-members/preview with {"memberIds":["<a reviewer on 10-19.99 hours>"]} and expect a 200 naming that team, with a reason and needsReview. Send an unconfigured team's reviewer and expect noTeamConfiguredForBand. Send somebody under 10 hr/wk and expect committedHoursOutOfBands with no team.
  16. The wipe regression. PUT /api/team/:teamId with only {"teamName":"Renamed","isActive":true}, no placement fields, then re-run the preview. The team must still be offered. If the placement fields were cleared the reason flips to noTeamConfiguredForBand.
  17. Placement is opt-in. POST /api/promote-members with just {"memberIds":[...]} must promote without touching any team, exactly as on development. Then with {"memberIds":[...],"placements":[{"reviewerId":"...","teamId":"..."}]} and confirm the person appears in the team's members and the team appears on their profile.
  18. "+ Add New". POST /api/promotion-eligibility/pr-ratings with {} returns the five options. POST /api/promotion-eligibility/<reviewerId>/pr-entries/new with {"prNumber":"PR #1234"} expect 201 and the number stored as 1234; repeat it and expect 409. Try {"prNumber":"fe 2284"} for FE-2284, a full github.com/.../pull/2108 URL for 2108, and {"prNumber":"999","rating":"Sufficient"} for a 400, since that is the analytics vocabulary and not one of the five. {"prNumber":"777","year":2025} expect 400, half a week. PATCH /api/promotion-eligibility/pr-entries/<entryId>/rating with {"rating":"Good"} then {"rating":null} to clear.
  19. History. POST /api/promotion-eligibility with {} and check any row has a history array, oldest week first, each entry carrying belowRequirement. On dev only a handful of reviewers have any, which is correct.
  20. Unit tests: npx jest src/helpers/reviewerGroupHelper.spec.js src/helpers/promotionEligibilityHelper.spec.js src/controllers/reviewerGroupController.test.js src/controllers/promotionEligibilityController.test.js. Expect 4 suites, 145 tests, all passing. For the placement work add src/helpers/teamPlacementHelper.spec.js src/controllers/teamController.placement.spec.js src/controllers/teamController.spec.js. Add src/helpers/prEntryHelper.spec.js for the "+ Add New" work. Whole suite: 145 of 146 suites and 2195 tests pass; reasonSchedulingController is an unrelated database integration suite that times out under contention and passes standalone in 19s.

Please restore anything you change, since reviewer groups live in a shared collection on dev. Rename 95XXPRT back, reset its range to A-N, clear any override you set, and delete any group you created.

…ands

Doc item #23. PRs Needed was calculated as pledgedHours / 2, which does
not match the spec. Replace it with the specified bands (7 for 10-14.99,
10 for 15-25.99, 20 for 26-35.99, 30 for 36-40 hr/wk) and add the Owner
override that goes with them.

- Add promotionEligibilityHelper with the band table and resolution logic,
  kept pure so the bands are testable without a database.
- Detect when a reviewer's committed hours changed since the last
  calculation and report it as committedHoursChanged, per the spec's
  "check when loading" requirement.
- Add PATCH /promotion-eligibility/:reviewerId/prs-needed, Owner only,
  which pins the figure and stops the committed hours check. Sending null
  clears the override and returns the reviewer to the bands.
- Read existing records in one query rather than per user, since the
  change check needs the previously stored hours.
- Keep requiredPRs in step with prsNeeded so the current page keeps
  working until the frontend moves to the new field.

Committed hours outside the specified 10 to 40 range are clamped to the
nearest band, and zero or negative hours require nothing. Dev data has 46
accounts below 10 hr/wk and one at -3, so this is not hypothetical. The
intended handling is an open question for Jae.
… Week"

Adds the three-group dropdown from doc item #23, with membership derived
from each group's alphabetical range rather than stored as a member list.
The range is the rule, so membership needs no maintenance as volunteers
join and leave, and an Owner editing a range re-splits the table on the
next load. This also matches the spec, which only ever describes editing
a group's range and never adding a person to one.

The grouping letter comes from the reviewer's last name, falling back to
the first name when there is no usable last name, with accents folded so
Alvarez and Álvarez group together. The spec does not say which name to
use, so this is an assumption kept in one function to make it a one-line
change.

Endpoints, all on the existing promotion eligibility router:

  POST  /api/reviewer-groups            read, seeds the three defaults
  POST  /api/reviewer-groups/new        Owner only, add a group
  PATCH /api/reviewer-groups/:groupKey  Owner only, rename and re-range

Reads stay POST because the permission check reads req.body.requestor
and a GET carries no body, which is why creating a group posts to /new
rather than to the collection path.

POST /api/promotion-eligibility gains an optional groupKey in the body.
Omitting it behaves exactly as before, so the current frontend is
unaffected. An unknown key returns 400 rather than silently returning
the whole table. Filtering happens before the per-reviewer queries, so a
narrow group does proportionally less database work.

Overlapping and gapped ranges are reported in a warnings array rather
than refused. Refusing them would stop an Owner widening A-N to A-P
before shrinking O-Z, and since a group is a filter rather than an
assignment, a reviewer matching two groups is harmless.

75 new unit tests, 115 passing across the four suites for this task.
Verified against live dev as Owner: seeding is idempotent, rename keeps
the key stable, ranges normalise case, both warning types fire, and the
403/404/400 guards all behave.
Doc item #23 names the groups explicitly: "95XXPRT Members (Members with
first names starting with A-N)" and "97XXPRT Members (Members with first
names starting with O-Z)". The first implementation read the last name on
the assumption that the spec was silent on which to use. It is not.

groupingLetter now reads the first name and falls back to the last name
for accounts with no usable first name, which is the previous rule with
its two halves swapped. Accent folding and the "no A-Z letter means All
Members only" behaviour are unchanged.

This moves a large share of reviewers between the two lettered groups, so
it is a visible change to the table rather than a quiet fix.
…not hours

Doc item #23. "Weekly Requirements" and "Remaining Weeks" still used the
old halved-hours threshold, so since PRs Needed moved to the committed
hours bands the two columns disagreed about what the requirement was.

The spec counts weeks where the reviewer "met or exceeded PR requirement",
so a week is now successful when the reviewer reviewed at least prsNeeded
PRs in it, honouring an Owner override where one is set. Actual PR review
records exist but store GitHub's numeric account id, which cannot be
joined to an HGN profile yet, so the count stays on the same review-task
proxy the rest of the page uses: distinct review tasks worked on in a
week, via $addToSet so several time entries against one task stay one
review.

- Add summariseWeeks and weekMeetsRequirement to the helper, pure and
  testable without a database, next to the bands they depend on.
- Exclude the current, still running week from successfulWeeks. The spec
  counts "previous weeks where they have satisfied the minimum
  requirement", and a week in progress has not finished failing yet.
- weeklyRequirementsMet now means the requirement is met for the current
  week, which is the spec's "satisfied for the current period". It used
  to be a synonym for successfulWeeks >= 2, which is promotion
  eligibility and is still available as remainingWeeks === 0. This
  changes what the column shows, so it needs flagging to the frontend.
- Expose successfulWeeks so the page can show progress, not only what
  is left.
- Fix isNewMember, which used six months against a spec that says "New
  Members (joined <= 1 week ago)" and "Existing Members (older than a
  week)".
- Group the per-week aggregation by year as well as week. $week alone
  repeats annually, so the same week number from different years was
  being folded into one group.
- Take one timestamp for the whole read, so two reviewers cannot land on
  different sides of a week boundary partway through the loop.

A requirement of zero is treated as not assessable rather than trivially
met, so reviewers on zero or negative committed hours accumulate no
successful weeks. Dev has 46 accounts below 10 hr/wk and one at -3, and
counting their empty weeks would have walked them to zero remaining weeks
and offered them for promotion without a single review. This moves with
open question 3 to Jae.

mongoWeekOf reproduces MongoDB's $year and $week in JavaScript so the
aggregation and the "which week is now" check agree. Verified against the
database itself over 128 dates across 8 years, including every Jan 1-8
and Dec 25-31 boundary, with no mismatches.
Doc item #23, Process Promotions. The spec assigns a promoted reviewer to
a 10-hour or 20+ hour team matched to that team's weekly standup, and
none of that information existed anywhere.

The premise this was blocked on was wrong. It was recorded as "the hours
band and standup time only live inside the team name", from the example
team-binary-brigade-tues-at-11am-pacific. That is a Slack channel name,
not a team record, and no such team exists. Checked properly: of 1046
active teams on dev, one has a weekday in its name, none has a clock time
and none has an hours band, and among the 161 teams with three or more
members it is zero on every count. teamCode is free text (S-PRc on 1302
profiles, TESTVEN on 626) and models/meeting.js is one-off meetings, not
recurring standups. The data does not exist, so something had to create
it.

- Add optional hoursBand, standupDay, standupTime and standupTimezone to
  the team model. A team missing any of them is not a placement
  candidate, which is what avoids backfilling 1046 mostly-disposable
  teams: only the real PR review teams need configuring, and an
  unconfigured team is invisible rather than wrongly eligible.
- postTeam and putTeam accept them. putTeam only writes the fields
  actually present in the body, because it assigns everything else
  unconditionally and reading these the same way would let the existing
  Teams page wipe a standup on an ordinary rename. Explicit null clears.
- Add teamPlacementHelper with the spec's rules, kept pure: band is
  required, then exact availability match, then smallest of several
  matches, then a standup within two hours, then smallest in band. Ties
  break on team name so preview and commit cannot disagree.
- Add POST /promote-members/preview, which writes nothing. A separate
  route rather than a flag, so nobody can promote by accident while
  asking what would happen. Rows carry a reason and needsReview so the
  confirmation modal can lead with the guesses.
- promoteMembers takes an optional placements array. Omitting it behaves
  exactly as before, role change only, no team touched. When present it
  is trusted over recalculating, since the modal exists so a human can
  override, and membership is written to both the team and the profile
  the way assignTeamToUsers does it.
- Promoted reviewers come back under All Members per the spec, but only
  for an explicit groupKey "all". Omitting the key, which is what the
  current page sends, is unchanged.

Two things the spec does not cover are flagged rather than hidden. Under
10 hr/wk is not placed at all, and someone with no availability on file
gets the smallest team in band marked needsReview. The second is the
common case, not the exception: only 94 of 2639 active profiles have ever
answered the questionnaire, against 1644 rows on the table. Both move
with the open questions to Jae.

Verified live against dev on every branch of the algorithm using real
questionnaire data, via one throwaway team that was deleted afterwards.
An 8AM-9AM person against an 11AM standup resolved as withinTwoHours at
exactly the two hour boundary, a 6AM-7AM person as smallestInBand, an
account whose availability is the string "N/A" as noAvailabilityOnFile,
and moving the standup to 8:30AM flipped the first to an exact match and
the second to withinTwoHours. Renaming the team without sending the
placement fields left it fully placeable, which is the wipe regression.

Full suite: 145 suites, 2164 tests, no failures.
…ratings

Doc item #23, spec items 2 and 5. With this the backend covers every item
in the spec.

History comes back on the existing dashboard read as one entry per prior
week, oldest first so it renders left to right the way the spec's example
does, with unlimited weeks and the current week excluded. belowRequirement
is precomputed rather than left to the frontend: the spec colours a week
red when it is under PRs Needed, and PRs Needed can be an Owner override
rather than the band value, so re-deriving it client side would go wrong
for exactly the reviewers somebody has intervened on. It is always false
when the requirement is zero.

"+ Add New" gets its own collection rather than an array on the
promotionEligibility doc, because the spec asks for unlimited weeks and
that document is rewritten on every dashboard read. A unique index on
reviewer, year, week and PR number is what makes the import safe to
re-run and turns a duplicate into a 409 instead of a 500.

- Add prEntryHelper with the five rating options verbatim from the spec,
  PR number normalisation, and the weekly summary parser, all pure.
- Add POST pr-ratings, which serves the options so the dropdown and the
  validation cannot drift apart. These are deliberately NOT the four
  buckets in services/analytics/fetchGithubReviews.js, which grades
  GitHub review states rather than review quality; two of the names are
  close enough to be mixed up, so that vocabulary is rejected outright.
- Add read, add, import and rate endpoints, all gated on getReports. The
  spec singles out the Owner for PRs Needed and the reviewer groups but
  says only "the person with access" for rating, so rating is open to
  anyone who can see the page.
- Normalise PR numbers on the way in, so 1234, #1234, PR 1234, FE-1234,
  fe 1234 and a full GitHub pull URL all work, keeping a repo prefix
  where one is given.

The weekly summary import is built because the spec asks for it, and I
do not trust it. It has never run against a real summary: zero profiles
on dev have any weekly summary text at all, so there is no sample of how
people write PR numbers and the patterns are assumptions. It is
deliberately conservative, wanting an explicit marker rather than a bare
number in prose, entries land with source "weeklySummary" so they can be
told from typed ones, and the response always warns that the results are
suggestions. The synced pullRequestReview data remains the better source
and that is open question 2 to Jae.

Verified live against dev: normalisation of every accepted format, the
409 on a duplicate, rejection of the analytics vocabulary, backfill into
a past week, half a week rejected, grouping newest week first, rating set
and cleared, and the import correctly reporting that it found nothing.
History checked on the real table, including one reviewer whose weeks run
2025 week 38, 2025 week 52, 2026 week 1, which exercises the year-aware
grouping across a boundary. Test entries removed afterwards.

Suite: 145 of 146 suites pass, 2195 tests. The one failure is
reasonSchedulingController, a database integration suite unrelated to
this work that times out under contention and passes standalone in 19s.
@sitaram1921
sitaram1921 force-pushed the sitaram/feature/promotion-eligibility-dashboard-backend branch from b6a3b14 to 0b079d1 Compare August 22, 2026 23:33
@sitaram1921 sitaram1921 changed the title Promotion Eligibility Dashboard backend (doc item #23): PRs Needed bands, reviewer groups, weekly requirements, History, PR ratings and Process Promotions Sitaram - PRs Needed bands, reviewer groups, weekly requirements, History, PR ratings and Process Promotions Aug 22, 2026
@sitaram1921
sitaram1921 force-pushed the sitaram/feature/promotion-eligibility-dashboard-backend branch from 0b079d1 to 144c756 Compare August 23, 2026 03:52
@one-community one-community added the High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible label Aug 24, 2026

@linlin-husky linlin-husky left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @sitaram1921,

Thank you for the detailed PR description, comprehensive test instructions, and clean implementation.

I started by running the unit tests and then verified each endpoint and test scenario step-by-step using cURL and Postman:

  • Unit Tests: Ran all specified Jest test suites locally (reviewerGroupHelper.spec.js, promotionEligibilityHelper.spec.js, reviewerGroupController.test.js, promotionEligibilityController.test.js, teamPlacementHelper.spec.js, teamController.placement.spec.js, and prEntryHelper.spec.js). All test suites passed without issues.
  • Reviewer Groups & Filtering:
    • Verified default group seeding (All Members, 95XXPRT Members [A-N], 97XXPRT Members [O-Z]) via POST /api/reviewer-groups.
    • Confirmed dashboard filtering via POST /api/promotion-eligibility with groupKey: "95xx" accurately filters reviewers by name.
    • Tested editing labels and ranges, automatic range normalization (e.g., lowercase a-p converted to A-P), overlap warnings, gap warnings, and creating new custom groups (201 Created).
  • Guards & Edge Cases:
    • Confirmed modifications to built-in groups (all) are rejected with 403 Forbidden.
    • Validated parameter guards for unknown group keys (404), single-sided ranges (400), backwards ranges (400), and empty payloads (400).
  • Owner Overrides & Dashboard Metrics:
    • Tested Owner override for prsNeeded via PATCH /api/promotion-eligibility/:reviewerId/prs-needed, confirming prsNeededSource: "ownerOverride" and matching requiredPRs.
    • Verified clearing overrides with prsNeeded: null restores automatic calculation based on committed hours bands.
    • Checked successfulWeeks, remainingWeeks, weeklyRequirementsMet, and history array entries across dashboard reads.
  • Team Placement & Promotions:
    • Created a test team with hours band and standup configuration (10-19.99, Tuesday 11AM).
    • Validated placement preview logic via POST /api/promote-members/preview: correctly recommended the configured team for 10h reviewers and returned committedHoursOutOfBands for reviewers under 10 hr/wk.
    • Verified the wipe regression: updating general team metadata via PUT /api/team/:teamId preserves existing placement configuration.
    • Verified promotions are opt-in without mutating team structures unless explicit placements are passed.
  • PR Entry Management:
    • Retrieved the 5 standard rating options from POST /api/promotion-eligibility/pr-ratings.
    • Tested PR number format normalization ("PR #1234" stored as 1234), duplicate PR conflict guards (409 Conflict), and non-standard rating vocabulary rejection (400 Bad Request).
    • Tested rating updates ("Good") and clearing ratings (null) via PATCH /api/promotion-eligibility/pr-entries/:entryId/rating.
  • Clean-up:
    • Reverted the 95XXPRT Members range back to A-N.
    • Cleared all prsNeeded overrides back to auto.
    • Deleted the temporary reviewer group (99xxprt-members) and temporary test team (Test 10hr) to keep the shared database clean.

Please see the test execution screenshots below for reference.

Image Image Image Image Image Image Image Image Image Image Image Image Image

@iAbhi001
iAbhi001 self-requested a review August 27, 2026 00:09

@iAbhi001 iAbhi001 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi @sitaram1921,

Thank you for the detailed PR description, comprehensive test instructions, and clean implementation.

I’ve reviewed the code, executed the test suites locally, and manually validated the endpoints and edge cases via cURL and Postman. Everything is working as expected.

Test Verification Summary:

  • Unit Tests: All specified Jest test suites passed cleanly (reviewerGroupHelper, promotionEligibilityHelper, reviewerGroupController, promotionEligibilityController, teamPlacementHelper, teamController.placement, and prEntryHelper).
  • Reviewer Groups & Filtering: Verified default group seeding (All Members, 95XXPRT, 97XXPRT), range normalization (e.g., a-p $\rightarrow$ A-P), dashboard filtering, gap/overlap warnings, and custom group creation (201 Created).
  • Guards & Edge Cases: Validated that built-in group edits are rejected with 403 Forbidden, and parameter guards handle invalid keys (404), single-sided/backwards ranges (400), and empty bodies as expected.
  • Owner Overrides & Dashboard: Confirmed owner overrides for prsNeeded update the source flag and requiredPRs. Verified that resetting to null cleanly reverts to automatic band-based calculation, and metrics (successfulWeeks, history, etc.) track accurately.
  • Team Placement & Promotions: Verified placement preview recommendations based on hours bands, out-of-band handling, and confirmed that updating team metadata (PUT /api/team/:teamId) preserves existing placement configuration without wipe regression.
  • PR Entry Management: Verified PR number normalization ("PR #1234" $\rightarrow$ 1234), 5 standard ratings, duplicate conflict guards (409 Conflict), rating vocabulary validation (400), and update/clear patch logic.
  • Clean-up: Reverted test group ranges, cleared manual overrides, and deleted temporary test entities (99xxprt-members and Test 10hr) from the shared test database.

(Screenshots attached below)

Image Image Image Image Image

LGTM! 🚀 Approving.

@sitaram1921 sitaram1921 self-assigned this Sep 1, 2026
The "+ Add New" column had only a per reviewer read, so a table showing it
for a page of reviewers made one request, and one query, per row. This adds
the bulk read promised to the frontend on 09-01:

  POST /api/promotion-eligibility/pr-entries  { requestor, reviewerIds: [...] }
  -> { reviewers: { "<id>": { weeks: [...] } } }

Every id sent comes back as a key, including reviewers with nothing listed,
so the client never has to tell "no entries" apart from "id missing from the
response". The per reviewer value is the identical shape the single reviewer
route returns, so only the read location changes on the client.

One query for the whole batch, ids deduplicated first. The single reviewer
route stays, since it is still the better one to hit right after an add or a
rating change when only one row needs refreshing.

13 tests covering the keying, the empty reviewer, week ordering, the single
query, deduplication, the validation cases and the 403 and 500 paths.
@sonarqubecloud

sonarqubecloud Bot commented Sep 4, 2026

Copy link
Copy Markdown

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

Labels

High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants