Feature/wbs by vnseattle · Pull Request #23 · OneCommunityGlobal/HGNRest

vnseattle · 2020-09-02T02:23:13Z

Re-merge the WBS Endpoints

…HGNRest into developement

chrisweilacker

I don't see any changes to any files here.

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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…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.

vnseattle added 5 commits March 28, 2020 15:44

Merge branch 'feature/wbs' into developement

1b99782

Merge branch 'developement' of https://github.com/OneCommunityGlobal/…

859f150

…HGNRest into developement

Merge branch 'developement' into feature/wbs

a158dc4

Merge branch 'developement' of https://github.com/OneCommunityGlobal/…

1d467d5

…HGNRest into developement

Merge branch 'developement' into feature/wbs

6f65adb

vnseattle requested a review from chrisweilacker September 2, 2020 02:23

chrisweilacker reviewed Sep 2, 2020

View reviewed changes

vnseattle closed this Sep 2, 2020

sitaram1921 mentioned this pull request Aug 22, 2026

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

Open

Navigation Menu

Uh oh!

Uh oh!

Feature/wbs - #23

Feature/wbs#23
vnseattle wants to merge 5 commits into
developementfrom
feature/wbs

vnseattle commented Sep 2, 2020

Uh oh!

chrisweilacker left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

vnseattle commented Sep 2, 2020

Uh oh!

chrisweilacker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants