From 27774e06fe26f5323c110fef8e79660c66ceb8c0 Mon Sep 17 00:00:00 2001 From: sitaram Date: Fri, 14 Aug 2026 17:36:56 -0400 Subject: [PATCH 1/7] feat(promotion-eligibility): derive PRs Needed from committed hours bands 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. --- .../promotionEligibilityController.js | 91 ++++++++- .../promotionEligibilityController.test.js | 179 ++++++++++++++++++ src/helpers/promotionEligibilityHelper.js | 89 +++++++++ .../promotionEligibilityHelper.spec.js | 116 ++++++++++++ src/models/promotionEligibility.js | 16 ++ src/routes/promotionEligibilityRouter.js | 2 + 6 files changed, 490 insertions(+), 3 deletions(-) create mode 100644 src/controllers/promotionEligibilityController.test.js create mode 100644 src/helpers/promotionEligibilityHelper.js create mode 100644 src/helpers/promotionEligibilityHelper.spec.js diff --git a/src/controllers/promotionEligibilityController.js b/src/controllers/promotionEligibilityController.js index 9b0e7b8875..742dc8ccc1 100644 --- a/src/controllers/promotionEligibilityController.js +++ b/src/controllers/promotionEligibilityController.js @@ -3,6 +3,7 @@ const mongoose = require('mongoose'); const { hasPermission } = require('../utilities/permissions'); const logger = require('../startup/logger'); const { ValidationError } = require('../utilities/errorHandling/customError'); +const { resolvePrsNeeded } = require('../helpers/promotionEligibilityHelper'); const promotionEligibilityController = function ( UserProfile, @@ -41,10 +42,24 @@ const promotionEligibilityController = function ( '_id firstName lastName weeklycommittedHours createdDate', ).lean(); + // Existing records are read up front, in one query rather than one per + // user, because PRs Needed has to know the previously calculated hours to + // detect a change and whether an Owner has overridden the figure. + const existingRecords = await PromotionEligibility.find( + { reviewerId: { $in: users.map((user) => user._id) } }, + 'reviewerId pledgedHours prsNeededOverride', + ).lean(); + const recordsByReviewerId = new Map( + existingRecords.map((record) => [record.reviewerId.toString(), record]), + ); + // Refactor: Use map and Promise.all for concurrent processing const eligibilityPromises = users.map(async (user) => { const pledgedHours = user.weeklycommittedHours || 0; - const requiredPRs = pledgedHours / 2; + const { prsNeeded, prsNeededSource, committedHoursChanged } = resolvePrsNeeded({ + committedHours: pledgedHours, + existingRecord: recordsByReviewerId.get(user._id.toString()) || null, + }); const totalReviews = await Task.countDocuments({ resources: { $elemMatch: { userID: user._id, completedTask: true } }, @@ -62,7 +77,13 @@ const promotionEligibilityController = function ( reviewerId: user._id, reviewerName: `${user.firstName} ${user.lastName}`, pledgedHours, - requiredPRs, + // `requiredPRs` is the field the current page reads. It carries the + // same value as `prsNeeded` so the rebuild can move across without a + // flag day, and is dropped once the frontend reads `prsNeeded`. + requiredPRs: prsNeeded, + prsNeeded, + prsNeededSource, + committedHoursChanged, totalReviews, remainingWeeks, isNewMember, @@ -89,6 +110,70 @@ const promotionEligibilityController = function ( } }; + /** + * Owner-only edit of a reviewer's PRs Needed figure. + * + * Sending a number pins the figure and stops it tracking committed hours. + * Sending null clears the override and hands the reviewer back to the bands. + */ + const updatePrsNeeded = async (req, res) => { + if (req.body.requestor.role !== 'Owner') { + return res.status(403).send('Only an Owner can edit PRs Needed.'); + } + + const { reviewerId } = req.params; + const { prsNeeded } = req.body; + + if (!mongoose.Types.ObjectId.isValid(reviewerId)) { + return res.status(400).send(`Invalid reviewer ID: ${reviewerId}`); + } + + const isClearing = prsNeeded === null; + if (!isClearing && (!Number.isInteger(prsNeeded) || prsNeeded < 0)) { + return res + .status(400) + .send('prsNeeded must be a non-negative whole number, or null to clear the override.'); + } + + try { + const updated = await PromotionEligibility.findOneAndUpdate( + { reviewerId }, + { + $set: { + prsNeededOverride: isClearing ? null : prsNeeded, + prsNeededOverrideBy: isClearing ? null : req.body.requestor.requestorId, + prsNeededOverrideAt: isClearing ? null : new Date(), + prsNeededSource: isClearing ? 'auto' : 'ownerOverride', + // An override replaces the committed hours check, so any pending + // change flag is no longer something the page should act on. + committedHoursChanged: false, + ...(isClearing ? {} : { prsNeeded, requiredPRs: prsNeeded }), + }, + }, + { new: true }, + ); + + // Reviewers only get a record once the dashboard has been loaded at least + // once, so a missing one means the id is not on the table rather than that + // the write failed. + if (!updated) { + return res.status(404).send('No promotion eligibility record for that reviewer.'); + } + + logger.logInfo( + `PRs Needed for reviewer ${reviewerId} ${ + isClearing ? 'reset to automatic' : `overridden to ${prsNeeded}` + }`, + { action: 'updatePrsNeeded', updatedBy: req.body.requestor.requestorId }, + ); + + return res.status(200).json(updated); + } catch (error) { + logger.logException(error, { endpoint: 'updatePrsNeeded', payload: req.body }); + return res.status(500).send('Error updating PRs Needed.'); + } + }; + const promoteMembers = async (req, res) => { if (!(await hasPermission(req.body.requestor, 'putUserProfile'))) { return res.status(403).send('You are not authorized to promote members.'); @@ -152,7 +237,7 @@ const promotionEligibilityController = function ( } }; - return { getPromotionEligibilityData, promoteMembers }; + return { getPromotionEligibilityData, updatePrsNeeded, promoteMembers }; }; module.exports = promotionEligibilityController; diff --git a/src/controllers/promotionEligibilityController.test.js b/src/controllers/promotionEligibilityController.test.js new file mode 100644 index 0000000000..e4ca375205 --- /dev/null +++ b/src/controllers/promotionEligibilityController.test.js @@ -0,0 +1,179 @@ +jest.mock('../startup/logger', () => ({ + logInfo: jest.fn(), + logException: jest.fn(), +})); + +const mongoose = require('mongoose'); +const promotionEligibilityController = require('./promotionEligibilityController'); + +const OWNER_ID = '665234c757ca141fe891e1ca'; +const REVIEWER_ID = '637af0c0fb9bbc1e308cff62'; + +describe('updatePrsNeeded', () => { + let PromotionEligibility; + let controller; + let mockReq; + let mockRes; + + beforeEach(() => { + PromotionEligibility = { findOneAndUpdate: jest.fn() }; + controller = promotionEligibilityController({}, {}, {}, PromotionEligibility); + + mockReq = { + params: { reviewerId: REVIEWER_ID }, + body: { + requestor: { requestorId: OWNER_ID, role: 'Owner' }, + prsNeeded: 12, + }, + }; + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + + jest.clearAllMocks(); + }); + + describe('authorisation', () => { + it.each(['Administrator', 'Manager', 'Core Team', 'Volunteer'])( + 'refuses a %s, since the spec limits editing to the Owner class', + async (role) => { + mockReq.body.requestor.role = role; + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(PromotionEligibility.findOneAndUpdate).not.toHaveBeenCalled(); + }, + ); + + it('allows an Owner through', async () => { + PromotionEligibility.findOneAndUpdate.mockResolvedValue({ prsNeeded: 12 }); + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + }); + }); + + describe('validation', () => { + it('rejects a reviewer id that is not a valid ObjectId', async () => { + mockReq.params.reviewerId = 'not-an-object-id'; + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionEligibility.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it.each([-1, 2.5, '10', undefined, NaN])('rejects %p as a PRs Needed value', async (value) => { + mockReq.body.prsNeeded = value; + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionEligibility.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('accepts zero, which is a meaningful requirement rather than a missing value', async () => { + mockReq.body.prsNeeded = 0; + PromotionEligibility.findOneAndUpdate.mockResolvedValue({ prsNeeded: 0 }); + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + const [, update] = PromotionEligibility.findOneAndUpdate.mock.calls[0]; + expect(update.$set.prsNeededOverride).toBe(0); + }); + }); + + describe('setting an override', () => { + beforeEach(() => { + PromotionEligibility.findOneAndUpdate.mockResolvedValue({ prsNeeded: 12 }); + }); + + it('pins the figure and records who set it', async () => { + await controller.updatePrsNeeded(mockReq, mockRes); + + const [filter, update] = PromotionEligibility.findOneAndUpdate.mock.calls[0]; + expect(filter).toEqual({ reviewerId: REVIEWER_ID }); + expect(update.$set).toMatchObject({ + prsNeededOverride: 12, + prsNeededSource: 'ownerOverride', + prsNeededOverrideBy: OWNER_ID, + prsNeeded: 12, + }); + expect(update.$set.prsNeededOverrideAt).toBeInstanceOf(Date); + }); + + it('keeps requiredPRs in step so the current page shows the edited figure', async () => { + await controller.updatePrsNeeded(mockReq, mockRes); + + const [, update] = PromotionEligibility.findOneAndUpdate.mock.calls[0]; + expect(update.$set.requiredPRs).toBe(12); + }); + + it('clears any pending committed hours change, which the override replaces', async () => { + await controller.updatePrsNeeded(mockReq, mockRes); + + const [, update] = PromotionEligibility.findOneAndUpdate.mock.calls[0]; + expect(update.$set.committedHoursChanged).toBe(false); + }); + }); + + describe('clearing an override', () => { + beforeEach(() => { + mockReq.body.prsNeeded = null; + PromotionEligibility.findOneAndUpdate.mockResolvedValue({ prsNeeded: 7 }); + }); + + it('returns the reviewer to automatic tracking', async () => { + await controller.updatePrsNeeded(mockReq, mockRes); + + const [, update] = PromotionEligibility.findOneAndUpdate.mock.calls[0]; + expect(update.$set).toMatchObject({ + prsNeededOverride: null, + prsNeededOverrideBy: null, + prsNeededOverrideAt: null, + prsNeededSource: 'auto', + }); + expect(mockRes.status).toHaveBeenCalledWith(200); + }); + + it('leaves prsNeeded alone so the next load recalculates it from committed hours', async () => { + await controller.updatePrsNeeded(mockReq, mockRes); + + const [, update] = PromotionEligibility.findOneAndUpdate.mock.calls[0]; + expect(update.$set).not.toHaveProperty('prsNeeded'); + expect(update.$set).not.toHaveProperty('requiredPRs'); + }); + }); + + describe('failure handling', () => { + it('404s when the reviewer has no record yet', async () => { + PromotionEligibility.findOneAndUpdate.mockResolvedValue(null); + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(404); + }); + + it('500s and logs when the write fails', async () => { + PromotionEligibility.findOneAndUpdate.mockRejectedValue(new Error('mongo is down')); + + await controller.updatePrsNeeded(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(500); + // eslint-disable-next-line global-require + expect(require('../startup/logger').logException).toHaveBeenCalled(); + }); + }); +}); + +describe('mongoose ObjectId validation assumption', () => { + it('treats the ids used above as valid, so the 400 tests fail for the right reason', () => { + expect(mongoose.Types.ObjectId.isValid(REVIEWER_ID)).toBe(true); + expect(mongoose.Types.ObjectId.isValid('not-an-object-id')).toBe(false); + }); +}); diff --git a/src/helpers/promotionEligibilityHelper.js b/src/helpers/promotionEligibilityHelper.js new file mode 100644 index 0000000000..9aa534fa4b --- /dev/null +++ b/src/helpers/promotionEligibilityHelper.js @@ -0,0 +1,89 @@ +/** + * Helpers for the Promotion Eligibility dashboard (doc item #23). + * + * The "PRs Needed" figure is driven by the reviewer's weekly committed hours. + * Bands come straight from the spec: + * + * 7 PRs for 10 - 14.99 hr/wk + * 10 PRs for 15 - 25.99 hr/wk + * 20 PRs for 26 - 35.99 hr/wk + * 30 PRs for 36 - 40 hr/wk + */ + +const PRS_NEEDED_BANDS = [ + { minHours: 10, maxHours: 14.99, prsNeeded: 7 }, + { minHours: 15, maxHours: 25.99, prsNeeded: 10 }, + { minHours: 26, maxHours: 35.99, prsNeeded: 20 }, + { minHours: 36, maxHours: 40, prsNeeded: 30 }, +]; + +/** + * Map weekly committed hours onto the number of PR reviews required that week. + * + * The spec only defines bands from 10 to 40 hours, but `weeklycommittedHours` + * allows values outside that range, so the edges are handled here: + * + * - 0 or less, or a non-numeric value: 0 PRs required. Somebody committed to + * no hours cannot be held to a review quota. + * - above 0 but below 10: the lowest band (7). Requiring nothing at all would + * let a 9 hr/wk reviewer sit on the table indefinitely with no requirement. + * - above 40: the highest band (30). + * + * The two clamped cases are open question 3 to Jae. They are deliberately kept + * in one place so a single edit changes the behaviour once he answers. + * + * @param {number} committedHours value of userProfile.weeklycommittedHours + * @returns {number} PRs the reviewer must review that week + */ +function getPrsNeeded(committedHours) { + const hours = Number(committedHours); + + if (!Number.isFinite(hours) || hours <= 0) return 0; + + const band = PRS_NEEDED_BANDS.find((b) => hours >= b.minHours && hours <= b.maxHours); + if (band) return band.prsNeeded; + + // Outside the specified range, clamp to the nearest defined band. + const lowest = PRS_NEEDED_BANDS[0]; + const highest = PRS_NEEDED_BANDS[PRS_NEEDED_BANDS.length - 1]; + return hours < lowest.minHours ? lowest.prsNeeded : highest.prsNeeded; +} + +/** + * Work out the PRs Needed figure for one reviewer, honouring an Owner override. + * + * Per the spec, once an Owner edits PRs Needed by hand the value stops tracking + * committed hours, so the change check is skipped entirely for that reviewer. + * + * @param {object} params + * @param {number} params.committedHours current userProfile.weeklycommittedHours + * @param {object|null} params.existingRecord the stored PromotionEligibility doc, if any + * @returns {{prsNeeded: number, prsNeededSource: string, committedHoursChanged: boolean}} + */ +function resolvePrsNeeded({ committedHours, existingRecord }) { + const override = existingRecord ? existingRecord.prsNeededOverride : null; + + if (override !== null && override !== undefined) { + return { + prsNeeded: override, + prsNeededSource: 'ownerOverride', + committedHoursChanged: false, + }; + } + + // No override, so the figure tracks committed hours and we report whether + // those hours moved since the last time this reviewer was calculated. + const previousHours = existingRecord ? existingRecord.pledgedHours : undefined; + const committedHoursChanged = + previousHours !== undefined && + previousHours !== null && + Number(previousHours) !== Number(committedHours); + + return { + prsNeeded: getPrsNeeded(committedHours), + prsNeededSource: 'auto', + committedHoursChanged, + }; +} + +module.exports = { PRS_NEEDED_BANDS, getPrsNeeded, resolvePrsNeeded }; diff --git a/src/helpers/promotionEligibilityHelper.spec.js b/src/helpers/promotionEligibilityHelper.spec.js new file mode 100644 index 0000000000..b78d805d3d --- /dev/null +++ b/src/helpers/promotionEligibilityHelper.spec.js @@ -0,0 +1,116 @@ +const { getPrsNeeded, resolvePrsNeeded } = require('./promotionEligibilityHelper'); + +describe('getPrsNeeded', () => { + test('returns 7 across the 10 to 14.99 band', () => { + expect(getPrsNeeded(10)).toBe(7); + expect(getPrsNeeded(12.5)).toBe(7); + expect(getPrsNeeded(14.99)).toBe(7); + }); + + test('returns 10 across the 15 to 25.99 band', () => { + expect(getPrsNeeded(15)).toBe(10); + expect(getPrsNeeded(25.99)).toBe(10); + }); + + test('returns 20 across the 26 to 35.99 band', () => { + expect(getPrsNeeded(26)).toBe(20); + expect(getPrsNeeded(35.99)).toBe(20); + }); + + test('returns 30 across the 36 to 40 band', () => { + expect(getPrsNeeded(36)).toBe(30); + expect(getPrsNeeded(40)).toBe(30); + }); + + test('band boundaries do not overlap or leave gaps at the integer edges', () => { + expect(getPrsNeeded(14.99)).toBe(7); + expect(getPrsNeeded(15)).toBe(10); + expect(getPrsNeeded(25.99)).toBe(10); + expect(getPrsNeeded(26)).toBe(20); + expect(getPrsNeeded(35.99)).toBe(20); + expect(getPrsNeeded(36)).toBe(30); + }); + + test('clamps below the lowest band to 7 and above the highest to 30', () => { + expect(getPrsNeeded(9.99)).toBe(7); + expect(getPrsNeeded(0.5)).toBe(7); + expect(getPrsNeeded(60)).toBe(30); + }); + + test('requires nothing of a reviewer committed to no hours', () => { + expect(getPrsNeeded(0)).toBe(0); + expect(getPrsNeeded(-5)).toBe(0); + }); + + test('treats missing or non-numeric committed hours as no requirement', () => { + expect(getPrsNeeded(undefined)).toBe(0); + expect(getPrsNeeded(null)).toBe(0); + expect(getPrsNeeded('not a number')).toBe(0); + expect(getPrsNeeded(NaN)).toBe(0); + }); + + test('accepts a numeric string, since committed hours reach us from user input', () => { + expect(getPrsNeeded('20')).toBe(10); + }); +}); + +describe('resolvePrsNeeded', () => { + test('derives the figure from committed hours when no record exists yet', () => { + expect(resolvePrsNeeded({ committedHours: 20, existingRecord: null })).toEqual({ + prsNeeded: 10, + prsNeededSource: 'auto', + committedHoursChanged: false, + }); + }); + + test('reports no change when committed hours match the stored value', () => { + const result = resolvePrsNeeded({ + committedHours: 20, + existingRecord: { pledgedHours: 20, prsNeededOverride: null }, + }); + expect(result.committedHoursChanged).toBe(false); + expect(result.prsNeeded).toBe(10); + }); + + test('flags a change and recalculates when committed hours have moved', () => { + const result = resolvePrsNeeded({ + committedHours: 30, + existingRecord: { pledgedHours: 20, prsNeededOverride: null }, + }); + expect(result).toEqual({ + prsNeeded: 20, + prsNeededSource: 'auto', + committedHoursChanged: true, + }); + }); + + test('an Owner override wins and stops the committed hours check', () => { + const result = resolvePrsNeeded({ + committedHours: 40, + existingRecord: { pledgedHours: 10, prsNeededOverride: 5 }, + }); + expect(result).toEqual({ + prsNeeded: 5, + prsNeededSource: 'ownerOverride', + committedHoursChanged: false, + }); + }); + + test('an override of 0 is honoured rather than treated as absent', () => { + const result = resolvePrsNeeded({ + committedHours: 20, + existingRecord: { pledgedHours: 20, prsNeededOverride: 0 }, + }); + expect(result.prsNeeded).toBe(0); + expect(result.prsNeededSource).toBe('ownerOverride'); + }); + + test('clearing an override returns the reviewer to the committed hours bands', () => { + const result = resolvePrsNeeded({ + committedHours: 20, + existingRecord: { pledgedHours: 20, prsNeededOverride: null }, + }); + expect(result.prsNeededSource).toBe('auto'); + expect(result.prsNeeded).toBe(10); + }); +}); diff --git a/src/models/promotionEligibility.js b/src/models/promotionEligibility.js index 4d1a79e5bf..51a41cf9dd 100644 --- a/src/models/promotionEligibility.js +++ b/src/models/promotionEligibility.js @@ -13,6 +13,22 @@ const promotionEligibilitySchema = new Schema({ isNewMember: { type: Boolean, required: true }, weeklyRequirementsMet: { type: Boolean, required: true }, calculatedAt: { type: Date, default: Date.now }, + + // PRs Needed, derived from the reviewer's weekly committed hours bands. + // `requiredPRs` above is kept in step with this so the current frontend, + // which reads `requiredPRs`, keeps working while the page is rebuilt. + prsNeeded: { type: Number }, + prsNeededSource: { type: String, enum: ['auto', 'ownerOverride'], default: 'auto' }, + + // Set only when an Owner edits PRs Needed by hand. While this holds a value + // the figure stops tracking committed hours, per the spec. null means "auto". + prsNeededOverride: { type: Number, default: null }, + prsNeededOverrideBy: { type: Schema.Types.ObjectId, ref: 'userProfiles', default: null }, + prsNeededOverrideAt: { type: Date, default: null }, + + // True when committed hours moved since the last calculation, so the page can + // surface the change. Always false while an Owner override is in place. + committedHoursChanged: { type: Boolean, default: false }, }); module.exports = mongoose.model('promotionEligibility', promotionEligibilitySchema); diff --git a/src/routes/promotionEligibilityRouter.js b/src/routes/promotionEligibilityRouter.js index a5bd45d2a0..0ae50e2e9b 100644 --- a/src/routes/promotionEligibilityRouter.js +++ b/src/routes/promotionEligibilityRouter.js @@ -14,6 +14,8 @@ const routes = function (userProfile, timeEntry, task, PromotionEligibility) { router.route('/promotion-eligibility').post(controller.getPromotionEligibilityData); + router.route('/promotion-eligibility/:reviewerId/prs-needed').patch(controller.updatePrsNeeded); + router.route('/promote-members').post(controller.promoteMembers); return router; From 3116bf72dd050c2bd541440135a0631edc7136d8 Mon Sep 17 00:00:00 2001 From: sitaram Date: Thu, 20 Aug 2026 12:41:39 -0400 Subject: [PATCH 2/7] feat(promotion-eligibility): add reviewer groups for "Review for This Week" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../promotionEligibilityController.js | 36 +- .../promotionEligibilityController.test.js | 141 +++++++ src/controllers/reviewerGroupController.js | 203 ++++++++++ .../reviewerGroupController.test.js | 374 ++++++++++++++++++ src/helpers/reviewerGroupHelper.js | 222 +++++++++++ src/helpers/reviewerGroupHelper.spec.js | 204 ++++++++++ src/models/reviewerGroup.js | 35 ++ src/routes/promotionEligibilityRouter.js | 16 +- src/startup/routes.js | 6 +- 9 files changed, 1231 insertions(+), 6 deletions(-) create mode 100644 src/controllers/reviewerGroupController.js create mode 100644 src/controllers/reviewerGroupController.test.js create mode 100644 src/helpers/reviewerGroupHelper.js create mode 100644 src/helpers/reviewerGroupHelper.spec.js create mode 100644 src/models/reviewerGroup.js diff --git a/src/controllers/promotionEligibilityController.js b/src/controllers/promotionEligibilityController.js index 742dc8ccc1..7f604e36ee 100644 --- a/src/controllers/promotionEligibilityController.js +++ b/src/controllers/promotionEligibilityController.js @@ -4,12 +4,14 @@ const { hasPermission } = require('../utilities/permissions'); const logger = require('../startup/logger'); const { ValidationError } = require('../utilities/errorHandling/customError'); const { resolvePrsNeeded } = require('../helpers/promotionEligibilityHelper'); +const { DEFAULT_REVIEWER_GROUPS, isReviewerInGroup } = require('../helpers/reviewerGroupHelper'); const promotionEligibilityController = function ( UserProfile, TimeEntry, Task, PromotionEligibility, + ReviewerGroup, ) { const calculateWeeksMetRequirement = async (userId, pledgedHours) => { const weeklyTasks = await TimeEntry.aggregate([ @@ -28,12 +30,36 @@ const promotionEligibilityController = function ( return weeklyTasks.length; }; + /** + * Resolve the "Review for This Week" group the caller asked to see. + * + * Returns null when the whole table is wanted, which is both the default and + * what the All Members group means. Falls back to the spec's default ranges + * when no group has been stored yet, so a filtered read works on a fresh + * database without this read path writing anything. + */ + const resolveRequestedGroup = async (groupKey) => { + if (!groupKey || groupKey === 'all') return null; + + const stored = ReviewerGroup ? await ReviewerGroup.find({}).lean() : []; + const groups = stored.length ? stored : DEFAULT_REVIEWER_GROUPS; + + return groups.find((group) => group.key === groupKey) || undefined; + }; + const getPromotionEligibilityData = async (req, res) => { if (!(await hasPermission(req.body.requestor, 'getReports'))) { return res.status(403).send('You are not authorized to view promotion eligibility data.'); } try { + // `undefined` means the key was supplied but matches nothing, which is a + // stale dropdown on the client rather than a request for the whole table. + const group = await resolveRequestedGroup(req.body.groupKey); + if (group === undefined) { + return res.status(400).send(`No reviewer group with key: ${req.body.groupKey}`); + } + const users = await UserProfile.find( { isActive: true, @@ -42,11 +68,17 @@ const promotionEligibilityController = function ( '_id firstName lastName weeklycommittedHours createdDate', ).lean(); + // Group membership is derived from the reviewer's name rather than stored, + // so the filter happens here in memory. Doing it before the per-reviewer + // queries below matters: those run one aggregation and one count each, so + // a narrow group does proportionally less database work. + const scopedUsers = group ? users.filter((user) => isReviewerInGroup(user, group)) : users; + // Existing records are read up front, in one query rather than one per // user, because PRs Needed has to know the previously calculated hours to // detect a change and whether an Owner has overridden the figure. const existingRecords = await PromotionEligibility.find( - { reviewerId: { $in: users.map((user) => user._id) } }, + { reviewerId: { $in: scopedUsers.map((user) => user._id) } }, 'reviewerId pledgedHours prsNeededOverride', ).lean(); const recordsByReviewerId = new Map( @@ -54,7 +86,7 @@ const promotionEligibilityController = function ( ); // Refactor: Use map and Promise.all for concurrent processing - const eligibilityPromises = users.map(async (user) => { + const eligibilityPromises = scopedUsers.map(async (user) => { const pledgedHours = user.weeklycommittedHours || 0; const { prsNeeded, prsNeededSource, committedHoursChanged } = resolvePrsNeeded({ committedHours: pledgedHours, diff --git a/src/controllers/promotionEligibilityController.test.js b/src/controllers/promotionEligibilityController.test.js index e4ca375205..0fd25df15e 100644 --- a/src/controllers/promotionEligibilityController.test.js +++ b/src/controllers/promotionEligibilityController.test.js @@ -3,7 +3,12 @@ jest.mock('../startup/logger', () => ({ logException: jest.fn(), })); +jest.mock('../utilities/permissions', () => ({ + hasPermission: jest.fn(), +})); + const mongoose = require('mongoose'); +const { hasPermission } = require('../utilities/permissions'); const promotionEligibilityController = require('./promotionEligibilityController'); const OWNER_ID = '665234c757ca141fe891e1ca'; @@ -171,6 +176,142 @@ describe('updatePrsNeeded', () => { }); }); +describe('getPromotionEligibilityData, filtering by reviewer group', () => { + const REVIEWERS = [ + { + _id: '637af0c0fb9bbc1e308cff01', + firstName: 'Ann', + lastName: 'Adams', + weeklycommittedHours: 20, + }, + { + _id: '637af0c0fb9bbc1e308cff02', + firstName: 'Jane', + lastName: 'Doe', + weeklycommittedHours: 20, + }, + { + _id: '637af0c0fb9bbc1e308cff03', + firstName: 'Ola', + lastName: 'Olsen', + weeklycommittedHours: 20, + }, + { + _id: '637af0c0fb9bbc1e308cff04', + firstName: 'Wei', + lastName: 'Zhang', + weeklycommittedHours: 20, + }, + ]; + + const GROUPS = [ + { key: 'all', label: 'All Members', rangeStart: null, rangeEnd: null }, + { key: '95xx', label: '95XXPRT Members', rangeStart: 'A', rangeEnd: 'N' }, + { key: '97xx', label: '97XXPRT Members', rangeStart: 'O', rangeEnd: 'Z' }, + ]; + + let UserProfile; + let TimeEntry; + let Task; + let PromotionEligibility; + let ReviewerGroup; + let controller; + let mockRes; + + const requestFor = (body = {}) => ({ + body: { requestor: { requestorId: OWNER_ID, role: 'Administrator' }, ...body }, + }); + + const namesReturned = () => + mockRes.json.mock.calls[0][0].map((entry) => entry.reviewerName).sort(); + + beforeEach(() => { + jest.clearAllMocks(); + hasPermission.mockResolvedValue(true); + + UserProfile = { find: jest.fn(() => ({ lean: () => Promise.resolve(REVIEWERS) })) }; + TimeEntry = { aggregate: jest.fn().mockResolvedValue([]) }; + Task = { countDocuments: jest.fn().mockResolvedValue(0) }; + PromotionEligibility = { + find: jest.fn(() => ({ lean: () => Promise.resolve([]) })), + findOneAndUpdate: jest.fn().mockResolvedValue({}), + }; + ReviewerGroup = { find: jest.fn(() => ({ lean: () => Promise.resolve(GROUPS) })) }; + + controller = promotionEligibilityController( + UserProfile, + TimeEntry, + Task, + PromotionEligibility, + ReviewerGroup, + ); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + it('returns every reviewer when no group is requested, as it does today', async () => { + await controller.getPromotionEligibilityData(requestFor(), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(namesReturned()).toEqual(['Ann Adams', 'Jane Doe', 'Ola Olsen', 'Wei Zhang']); + expect(ReviewerGroup.find).not.toHaveBeenCalled(); + }); + + it('returns every reviewer for the All Members group without consulting a range', async () => { + await controller.getPromotionEligibilityData(requestFor({ groupKey: 'all' }), mockRes); + + expect(namesReturned()).toHaveLength(4); + }); + + it('returns only the A-N half for the 95XXPRT group', async () => { + await controller.getPromotionEligibilityData(requestFor({ groupKey: '95xx' }), mockRes); + + expect(namesReturned()).toEqual(['Ann Adams', 'Jane Doe']); + }); + + it('returns only the O-Z half for the 97XXPRT group', async () => { + await controller.getPromotionEligibilityData(requestFor({ groupKey: '97xx' }), mockRes); + + expect(namesReturned()).toEqual(['Ola Olsen', 'Wei Zhang']); + }); + + it('filters before the per-reviewer queries, so a narrow group does less work', async () => { + await controller.getPromotionEligibilityData(requestFor({ groupKey: '95xx' }), mockRes); + + expect(Task.countDocuments).toHaveBeenCalledTimes(2); + expect(PromotionEligibility.findOneAndUpdate).toHaveBeenCalledTimes(2); + }); + + it('400s on a group key that does not exist rather than silently returning everyone', async () => { + await controller.getPromotionEligibilityData(requestFor({ groupKey: 'nope' }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(UserProfile.find).not.toHaveBeenCalled(); + }); + + it('falls back to the default ranges when no group has been stored yet', async () => { + ReviewerGroup.find = jest.fn(() => ({ lean: () => Promise.resolve([]) })); + + await controller.getPromotionEligibilityData(requestFor({ groupKey: '95xx' }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(namesReturned()).toEqual(['Ann Adams', 'Jane Doe']); + }); + + it('still refuses a requestor without getReports', async () => { + hasPermission.mockResolvedValue(false); + + await controller.getPromotionEligibilityData(requestFor({ groupKey: '95xx' }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(UserProfile.find).not.toHaveBeenCalled(); + }); +}); + describe('mongoose ObjectId validation assumption', () => { it('treats the ids used above as valid, so the 400 tests fail for the right reason', () => { expect(mongoose.Types.ObjectId.isValid(REVIEWER_ID)).toBe(true); diff --git a/src/controllers/reviewerGroupController.js b/src/controllers/reviewerGroupController.js new file mode 100644 index 0000000000..8bffcf22e2 --- /dev/null +++ b/src/controllers/reviewerGroupController.js @@ -0,0 +1,203 @@ +// src/controllers/reviewerGroupController.js +const { hasPermission } = require('../utilities/permissions'); +const logger = require('../startup/logger'); +const { + DEFAULT_REVIEWER_GROUPS, + validateRange, + rangeWarnings, + slugifyGroupKey, +} = require('../helpers/reviewerGroupHelper'); + +/** + * Reviewer groups behind the "Review for This Week" dropdown (doc item #23). + * + * Membership is derived from each group's alphabetical range rather than + * stored, so these handlers only ever manage the ranges themselves. The + * promotion eligibility read applies them when filtering the table. + */ +const reviewerGroupController = function (ReviewerGroup) { + const sortGroups = (groups) => + [...groups].sort( + (a, b) => (a.sortOrder || 0) - (b.sortOrder || 0) || a.label.localeCompare(b.label), + ); + + /** + * Read every group, seeding the spec's three defaults the first time. + * + * Seeding here rather than in a migration means a fresh database, and any + * teammate's local copy, works without a deploy step. It only fires when the + * collection is completely empty, so a deliberately deleted group stays + * deleted. + */ + const loadGroups = async () => { + const existing = await ReviewerGroup.find({}).lean(); + if (existing.length) return sortGroups(existing); + + await ReviewerGroup.bulkWrite( + DEFAULT_REVIEWER_GROUPS.map((group) => ({ + updateOne: { + filter: { key: group.key }, + update: { $setOnInsert: group }, + upsert: true, + }, + })), + ); + + return sortGroups(await ReviewerGroup.find({}).lean()); + }; + + const getReviewerGroups = async (req, res) => { + // Same gate as the dashboard read itself, so a viewer who can see the table + // can always see the dropdown that filters it. + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to view reviewer groups.'); + } + + try { + const groups = await loadGroups(); + return res.status(200).json({ groups, warnings: rangeWarnings(groups) }); + } catch (error) { + logger.logException(error, { endpoint: 'getReviewerGroups' }); + return res.status(500).send('Error fetching reviewer groups.'); + } + }; + + const createReviewerGroup = async (req, res) => { + if (req.body.requestor.role !== 'Owner') { + return res.status(403).send('Only an Owner can add a reviewer group.'); + } + + const { label, rangeStart, rangeEnd } = req.body; + + if (typeof label !== 'string' || !label.trim()) { + return res.status(400).send('A group label is required.'); + } + + const range = validateRange({ rangeStart, rangeEnd }); + if (!range.valid) { + return res.status(400).send(range.error); + } + + try { + const groups = await loadGroups(); + + // The key is what the frontend sends back to filter the table, so it is + // derived once here and never changes, even if the label is renamed later. + const key = slugifyGroupKey( + label, + groups.map((group) => group.key), + ); + if (!key) { + return res + .status(400) + .send('That label contains no letters or numbers to build a key from.'); + } + + const sortOrder = + groups.reduce((highest, group) => Math.max(highest, group.sortOrder || 0), -1) + 1; + + const doc = { + key, + label: label.trim(), + rangeStart: range.rangeStart, + rangeEnd: range.rangeEnd, + editable: true, + sortOrder, + updatedBy: req.body.requestor.requestorId, + updatedAt: new Date(), + }; + + const created = await ReviewerGroup.create(doc); + + logger.logInfo(`Reviewer group ${key} created covering ${doc.rangeStart}-${doc.rangeEnd}`, { + action: 'createReviewerGroup', + createdBy: req.body.requestor.requestorId, + }); + + return res.status(201).json({ group: created, warnings: rangeWarnings([...groups, doc]) }); + } catch (error) { + logger.logException(error, { endpoint: 'createReviewerGroup', payload: req.body }); + return res.status(500).send('Error creating reviewer group.'); + } + }; + + const updateReviewerGroup = async (req, res) => { + if (req.body.requestor.role !== 'Owner') { + return res.status(403).send('Only an Owner can edit a reviewer group.'); + } + + const { groupKey } = req.params; + const { label, rangeStart, rangeEnd } = req.body; + + try { + const groups = await loadGroups(); + const target = groups.find((group) => group.key === groupKey); + + if (!target) { + return res.status(404).send(`No reviewer group with key: ${groupKey}`); + } + if (target.editable === false) { + return res.status(403).send(`The ${target.label} group cannot be edited.`); + } + + const hasLabel = label !== undefined; + const hasRange = rangeStart !== undefined || rangeEnd !== undefined; + + if (!hasLabel && !hasRange) { + return res.status(400).send('Supply a label, a range, or both.'); + } + if (hasLabel && (typeof label !== 'string' || !label.trim())) { + return res.status(400).send('A group label cannot be empty.'); + } + + const $set = { + updatedBy: req.body.requestor.requestorId, + updatedAt: new Date(), + }; + + if (hasLabel) $set.label = label.trim(); + + if (hasRange) { + // Both boundaries are required together. Accepting one would leave the + // group holding half a range, which no caller can interpret. + const range = validateRange({ rangeStart, rangeEnd }); + if (!range.valid) { + return res.status(400).send(range.error); + } + $set.rangeStart = range.rangeStart; + $set.rangeEnd = range.rangeEnd; + } + + const updated = await ReviewerGroup.findOneAndUpdate( + { key: groupKey }, + { $set }, + { new: true }, + ).lean(); + + if (!updated) { + return res.status(404).send(`No reviewer group with key: ${groupKey}`); + } + + logger.logInfo(`Reviewer group ${groupKey} updated`, { + action: 'updateReviewerGroup', + updatedBy: req.body.requestor.requestorId, + }); + + // Overlaps and gaps are reported, not 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 double match is harmless. + const projected = groups.map((group) => + group.key === groupKey ? { ...group, ...$set } : group, + ); + + return res.status(200).json({ group: updated, warnings: rangeWarnings(projected) }); + } catch (error) { + logger.logException(error, { endpoint: 'updateReviewerGroup', payload: req.body }); + return res.status(500).send('Error updating reviewer group.'); + } + }; + + return { getReviewerGroups, createReviewerGroup, updateReviewerGroup }; +}; + +module.exports = reviewerGroupController; diff --git a/src/controllers/reviewerGroupController.test.js b/src/controllers/reviewerGroupController.test.js new file mode 100644 index 0000000000..1aa0317fa7 --- /dev/null +++ b/src/controllers/reviewerGroupController.test.js @@ -0,0 +1,374 @@ +jest.mock('../startup/logger', () => ({ + logInfo: jest.fn(), + logException: jest.fn(), +})); + +jest.mock('../utilities/permissions', () => ({ + hasPermission: jest.fn(), +})); + +const { hasPermission } = require('../utilities/permissions'); +const logger = require('../startup/logger'); +const { DEFAULT_REVIEWER_GROUPS } = require('../helpers/reviewerGroupHelper'); +const reviewerGroupController = require('./reviewerGroupController'); + +const OWNER_ID = '665234c757ca141fe891e1ca'; + +const storedGroups = () => [ + { + key: 'all', + label: 'All Members', + rangeStart: null, + rangeEnd: null, + editable: false, + sortOrder: 0, + }, + { + key: '95xx', + label: '95XXPRT Members', + rangeStart: 'A', + rangeEnd: 'N', + editable: true, + sortOrder: 1, + }, + { + key: '97xx', + label: '97XXPRT Members', + rangeStart: 'O', + rangeEnd: 'Z', + editable: true, + sortOrder: 2, + }, +]; + +describe('reviewerGroupController', () => { + let ReviewerGroup; + let controller; + let mockRes; + let found; + + const respondWith = (docs) => { + found = docs; + }; + + beforeEach(() => { + jest.clearAllMocks(); + respondWith(storedGroups()); + + ReviewerGroup = { + find: jest.fn(() => ({ lean: jest.fn(() => Promise.resolve(found)) })), + bulkWrite: jest.fn().mockResolvedValue({}), + create: jest.fn((doc) => Promise.resolve(doc)), + findOneAndUpdate: jest.fn(() => ({ lean: jest.fn(() => Promise.resolve(null)) })), + }; + controller = reviewerGroupController(ReviewerGroup); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + + hasPermission.mockResolvedValue(true); + }); + + const ownerReq = (body = {}, params = {}) => ({ + params, + body: { requestor: { requestorId: OWNER_ID, role: 'Owner' }, ...body }, + }); + + describe('getReviewerGroups', () => { + it('refuses a requestor without getReports, matching the dashboard read', async () => { + hasPermission.mockResolvedValue(false); + + await controller.getReviewerGroups(ownerReq(), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(ReviewerGroup.find).not.toHaveBeenCalled(); + }); + + it('returns the stored groups in dropdown order', async () => { + respondWith([storedGroups()[2], storedGroups()[0], storedGroups()[1]]); + + await controller.getReviewerGroups(ownerReq(), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + const [payload] = mockRes.json.mock.calls[0]; + expect(payload.groups.map((g) => g.key)).toEqual(['all', '95xx', '97xx']); + }); + + it('seeds the three default groups when the collection is empty', async () => { + let calls = 0; + ReviewerGroup.find = jest.fn(() => ({ + lean: jest.fn(() => { + calls += 1; + return Promise.resolve(calls === 1 ? [] : storedGroups()); + }), + })); + + await controller.getReviewerGroups(ownerReq(), mockRes); + + expect(ReviewerGroup.bulkWrite).toHaveBeenCalledTimes(1); + const [operations] = ReviewerGroup.bulkWrite.mock.calls[0]; + expect(operations.map((op) => op.updateOne.filter.key)).toEqual( + DEFAULT_REVIEWER_GROUPS.map((g) => g.key), + ); + expect(operations.every((op) => op.updateOne.upsert === true)).toBe(true); + + const [payload] = mockRes.json.mock.calls[0]; + expect(payload.groups).toHaveLength(3); + }); + + it('does not re-seed when groups already exist', async () => { + await controller.getReviewerGroups(ownerReq(), mockRes); + + expect(ReviewerGroup.bulkWrite).not.toHaveBeenCalled(); + }); + + it('reports coverage warnings alongside the groups', async () => { + const groups = storedGroups(); + groups[1].rangeEnd = 'K'; + respondWith(groups); + + await controller.getReviewerGroups(ownerReq(), mockRes); + + const [payload] = mockRes.json.mock.calls[0]; + expect(payload.warnings).toEqual(['No group covers L-N']); + }); + + it('500s and logs when the read fails', async () => { + ReviewerGroup.find = jest.fn(() => ({ + lean: jest.fn(() => Promise.reject(new Error('mongo is down'))), + })); + + await controller.getReviewerGroups(ownerReq(), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(logger.logException).toHaveBeenCalled(); + }); + }); + + describe('createReviewerGroup', () => { + const validBody = { label: '99XXPRT Members', rangeStart: 'a', rangeEnd: 'c' }; + + it.each(['Administrator', 'Manager', 'Core Team', 'Volunteer'])( + 'refuses a %s, since the spec limits adding groups to the Owner', + async (role) => { + const req = ownerReq(validBody); + req.body.requestor.role = role; + + await controller.createReviewerGroup(req, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(ReviewerGroup.create).not.toHaveBeenCalled(); + }, + ); + + it.each([undefined, '', ' ', 42])('rejects %p as a label', async (label) => { + await controller.createReviewerGroup(ownerReq({ ...validBody, label }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(ReviewerGroup.create).not.toHaveBeenCalled(); + }); + + it('rejects a label that slugs to nothing usable', async () => { + await controller.createReviewerGroup(ownerReq({ ...validBody, label: '!!!' }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(ReviewerGroup.create).not.toHaveBeenCalled(); + }); + + it('rejects an invalid range', async () => { + await controller.createReviewerGroup( + ownerReq({ ...validBody, rangeStart: 'N', rangeEnd: 'A' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(ReviewerGroup.create).not.toHaveBeenCalled(); + }); + + it('creates an editable group with a normalised range and a derived key', async () => { + await controller.createReviewerGroup(ownerReq(validBody), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(201); + const [doc] = ReviewerGroup.create.mock.calls[0]; + expect(doc).toMatchObject({ + key: '99xxprt-members', + label: '99XXPRT Members', + rangeStart: 'A', + rangeEnd: 'C', + editable: true, + }); + }); + + it('puts the new group last in the dropdown', async () => { + await controller.createReviewerGroup(ownerReq(validBody), mockRes); + + const [doc] = ReviewerGroup.create.mock.calls[0]; + expect(doc.sortOrder).toBe(3); + }); + + it('derives a non-colliding key when the label matches an existing group', async () => { + await controller.createReviewerGroup( + ownerReq({ ...validBody, label: '95XXPRT Members' }), + mockRes, + ); + + const [doc] = ReviewerGroup.create.mock.calls[0]; + expect(doc.key).toBe('95xxprt-members'); + expect(doc.key).not.toBe('95xx'); + }); + + it('warns about the overlap the new group introduces rather than refusing it', async () => { + await controller.createReviewerGroup(ownerReq(validBody), mockRes); + + const [payload] = mockRes.json.mock.calls[0]; + expect(payload.warnings.join(' ')).toContain('A-C'); + }); + }); + + describe('updateReviewerGroup', () => { + const updated = { + key: '95xx', + label: 'Renamed', + rangeStart: 'A', + rangeEnd: 'N', + editable: true, + sortOrder: 1, + }; + + beforeEach(() => { + ReviewerGroup.findOneAndUpdate = jest.fn(() => ({ + lean: jest.fn(() => Promise.resolve(updated)), + })); + }); + + it('refuses anyone who is not an Owner', async () => { + const req = ownerReq({ label: 'Renamed' }, { groupKey: '95xx' }); + req.body.requestor.role = 'Administrator'; + + await controller.updateReviewerGroup(req, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(ReviewerGroup.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('refuses to edit All Members, which is locked by design', async () => { + await controller.updateReviewerGroup( + ownerReq({ label: 'Everyone' }, { groupKey: 'all' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(ReviewerGroup.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('404s for a group key that does not exist', async () => { + await controller.updateReviewerGroup( + ownerReq({ label: 'Renamed' }, { groupKey: 'nope' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(404); + expect(ReviewerGroup.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('400s when neither a label nor a range is supplied', async () => { + await controller.updateReviewerGroup(ownerReq({}, { groupKey: '95xx' }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(ReviewerGroup.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('renames without touching the range', async () => { + await controller.updateReviewerGroup( + ownerReq({ label: 'Renamed' }, { groupKey: '95xx' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(200); + const [filter, update] = ReviewerGroup.findOneAndUpdate.mock.calls[0]; + expect(filter).toEqual({ key: '95xx' }); + expect(update.$set.label).toBe('Renamed'); + expect(update.$set).not.toHaveProperty('rangeStart'); + }); + + it('keeps the key stable across a rename, since the frontend filters by it', async () => { + await controller.updateReviewerGroup( + ownerReq({ label: 'Something Else Entirely' }, { groupKey: '95xx' }), + mockRes, + ); + + const [, update] = ReviewerGroup.findOneAndUpdate.mock.calls[0]; + expect(update.$set).not.toHaveProperty('key'); + }); + + it('edits the range without touching the label', async () => { + await controller.updateReviewerGroup( + ownerReq({ rangeStart: 'a', rangeEnd: 'p' }, { groupKey: '95xx' }), + mockRes, + ); + + const [, update] = ReviewerGroup.findOneAndUpdate.mock.calls[0]; + expect(update.$set).toMatchObject({ rangeStart: 'A', rangeEnd: 'P' }); + expect(update.$set).not.toHaveProperty('label'); + }); + + it('rejects an invalid range', async () => { + await controller.updateReviewerGroup( + ownerReq({ rangeStart: 'P', rangeEnd: 'B' }, { groupKey: '95xx' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(ReviewerGroup.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('rejects a half-supplied range rather than storing one boundary', async () => { + await controller.updateReviewerGroup( + ownerReq({ rangeStart: 'A' }, { groupKey: '95xx' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(ReviewerGroup.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('allows an overlap and warns, so a range can be widened before the neighbour shrinks', async () => { + await controller.updateReviewerGroup( + ownerReq({ rangeStart: 'A', rangeEnd: 'P' }, { groupKey: '95xx' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(200); + const [payload] = mockRes.json.mock.calls[0]; + expect(payload.warnings.join(' ')).toContain('O-P'); + }); + + it('records who made the edit', async () => { + await controller.updateReviewerGroup( + ownerReq({ label: 'Renamed' }, { groupKey: '95xx' }), + mockRes, + ); + + const [, update] = ReviewerGroup.findOneAndUpdate.mock.calls[0]; + expect(update.$set.updatedBy).toBe(OWNER_ID); + expect(update.$set.updatedAt).toBeInstanceOf(Date); + }); + + it('500s and logs when the write fails', async () => { + ReviewerGroup.findOneAndUpdate = jest.fn(() => ({ + lean: jest.fn(() => Promise.reject(new Error('mongo is down'))), + })); + + await controller.updateReviewerGroup( + ownerReq({ label: 'Renamed' }, { groupKey: '95xx' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(500); + expect(logger.logException).toHaveBeenCalled(); + }); + }); +}); diff --git a/src/helpers/reviewerGroupHelper.js b/src/helpers/reviewerGroupHelper.js new file mode 100644 index 0000000000..448c13ec6b --- /dev/null +++ b/src/helpers/reviewerGroupHelper.js @@ -0,0 +1,222 @@ +/** + * Helpers for the "Review for This Week" reviewer groups (doc item #23). + * + * A group's membership is derived from an alphabetical range rather than stored + * as a list of people. The range is the rule, so nobody has to maintain + * membership as volunteers join and leave, and an Owner editing a range + * re-splits the table immediately. + * + * The letter comes from the reviewer's LAST name, falling back to the first + * name when there is no usable last name. The spec does not say which name to + * use, so this is an assumption, deliberately kept in `groupingLetter` alone so + * that switching to first names is a one-line change. + */ + +const LETTERS = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''); + +/** + * The three groups the spec names. Seeded on first read, so a fresh database + * needs no migration step. + */ +const DEFAULT_REVIEWER_GROUPS = [ + { + key: 'all', + label: 'All Members', + rangeStart: null, + rangeEnd: null, + editable: false, + sortOrder: 0, + }, + { + key: '95xx', + label: '95XXPRT Members', + rangeStart: 'A', + rangeEnd: 'N', + editable: true, + sortOrder: 1, + }, + { + key: '97xx', + label: '97XXPRT Members', + rangeStart: 'O', + rangeEnd: 'Z', + editable: true, + sortOrder: 2, + }, +]; + +/** Fold accents down to plain ASCII so Álvarez and Alvarez group together. */ +function stripAccents(value) { + return value.normalize('NFD').replace(/[̀-ͯ]/g, ''); +} + +/** + * Coerce a range boundary to a single uppercase A-Z letter, or null if it is + * not one. Boundaries reach us from a text input, so anything can arrive. + */ +function normaliseLetter(value) { + if (typeof value !== 'string') return null; + const trimmed = value.trim().toUpperCase(); + return /^[A-Z]$/.test(trimmed) ? trimmed : null; +} + +function firstLetterOf(name) { + if (typeof name !== 'string') return null; + return normaliseLetter(stripAccents(name).trim().charAt(0)); +} + +/** + * The letter that decides which group a reviewer falls into. + * + * Returns null when neither name yields an A-Z letter, which happens on + * placeholder and test accounts. Those reviewers match no lettered group, and + * `rangeWarnings` is not the place that surfaces them, so they are only ever + * visible under All Members. + */ +function groupingLetter(reviewer) { + if (!reviewer) return null; + return firstLetterOf(reviewer.lastName) || firstLetterOf(reviewer.firstName); +} + +function letterInGroupRange(letter, group) { + const start = normaliseLetter(group.rangeStart); + const end = normaliseLetter(group.rangeEnd); + if (!start || !end) return false; + return letter >= start && letter <= end; +} + +/** + * Whether a reviewer belongs to a group. + * + * A group with no range, which is only ever All Members, takes everybody + * including reviewers whose name yields no letter. + */ +function isReviewerInGroup(reviewer, group) { + const start = normaliseLetter(group.rangeStart); + const end = normaliseLetter(group.rangeEnd); + if (!start || !end) return true; + + const letter = groupingLetter(reviewer); + if (!letter) return false; + + return letter >= start && letter <= end; +} + +/** + * Validate an Owner-supplied range and hand back the normalised letters. + * + * @returns {{valid: boolean, rangeStart: string|null, rangeEnd: string|null, error: string|null}} + */ +function validateRange({ rangeStart, rangeEnd }) { + const start = normaliseLetter(rangeStart); + const end = normaliseLetter(rangeEnd); + + if (!start || !end) { + return { + valid: false, + rangeStart: null, + rangeEnd: null, + error: 'rangeStart and rangeEnd must each be a single letter from A to Z.', + }; + } + + if (start > end) { + return { + valid: false, + rangeStart: start, + rangeEnd: end, + error: `Range ends before it starts: ${end} comes before ${start}.`, + }; + } + + return { valid: true, rangeStart: start, rangeEnd: end, error: null }; +} + +function describeLetters(letters) { + if (letters.length === 1) return letters[0]; + return `${letters[0]}-${letters[letters.length - 1]}`; +} + +/** Collapse a sorted letter list into contiguous runs, so gaps read as A-C not A, B, C. */ +function contiguousRuns(letters) { + return letters.reduce((runs, letter) => { + const current = runs[runs.length - 1]; + const isNext = + current && LETTERS.indexOf(letter) === LETTERS.indexOf(current[current.length - 1]) + 1; + + if (isNext) current.push(letter); + else runs.push([letter]); + + return runs; + }, []); +} + +/** + * Describe overlaps and gaps across the lettered groups. + * + * These are warnings rather than validation errors on purpose. Rejecting an + * overlap would mean an Owner could not widen A-N to A-P without shrinking O-Z + * first, which is a trap. Since a group is a filter on the table rather than an + * assignment, a reviewer appearing under two groups is harmless. + * + * @returns {string[]} empty when the lettered groups tile A-Z exactly + */ +function rangeWarnings(groups) { + const ranged = (groups || []).filter( + (group) => normaliseLetter(group.rangeStart) && normaliseLetter(group.rangeEnd), + ); + const warnings = []; + + ranged.forEach((group, index) => { + ranged.slice(index + 1).forEach((other) => { + const shared = LETTERS.filter( + (letter) => letterInGroupRange(letter, group) && letterInGroupRange(letter, other), + ); + if (shared.length) { + warnings.push(`${group.label} and ${other.label} both cover ${describeLetters(shared)}`); + } + }); + }); + + const uncovered = LETTERS.filter( + (letter) => !ranged.some((group) => letterInGroupRange(letter, group)), + ); + contiguousRuns(uncovered).forEach((run) => { + warnings.push(`No group covers ${describeLetters(run)}`); + }); + + return warnings; +} + +/** + * Derive a stable, url safe key from an Owner-supplied group label. + * + * The key is what the frontend sends back to filter the table, so it must not + * collide with an existing group and must not change when the label is later + * renamed. Returns null if the label contains nothing sluggable. + */ +function slugifyGroupKey(label, existingKeys = []) { + if (typeof label !== 'string') return null; + + const base = stripAccents(label) + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, ''); + + if (!base) return null; + if (!existingKeys.includes(base)) return base; + + let suffix = 2; + while (existingKeys.includes(`${base}-${suffix}`)) suffix += 1; + return `${base}-${suffix}`; +} + +module.exports = { + DEFAULT_REVIEWER_GROUPS, + normaliseLetter, + groupingLetter, + isReviewerInGroup, + validateRange, + rangeWarnings, + slugifyGroupKey, +}; diff --git a/src/helpers/reviewerGroupHelper.spec.js b/src/helpers/reviewerGroupHelper.spec.js new file mode 100644 index 0000000000..ef03563362 --- /dev/null +++ b/src/helpers/reviewerGroupHelper.spec.js @@ -0,0 +1,204 @@ +const { + DEFAULT_REVIEWER_GROUPS, + normaliseLetter, + groupingLetter, + isReviewerInGroup, + validateRange, + rangeWarnings, + slugifyGroupKey, +} = require('./reviewerGroupHelper'); + +const ALL_GROUP = { key: 'all', label: 'All Members', rangeStart: null, rangeEnd: null }; +const A_TO_N = { key: '95xx', label: '95XXPRT Members', rangeStart: 'A', rangeEnd: 'N' }; +const O_TO_Z = { key: '97xx', label: '97XXPRT Members', rangeStart: 'O', rangeEnd: 'Z' }; + +describe('DEFAULT_REVIEWER_GROUPS', () => { + test('seeds the three groups the spec names, in dropdown order', () => { + expect(DEFAULT_REVIEWER_GROUPS.map((g) => g.key)).toEqual(['all', '95xx', '97xx']); + expect(DEFAULT_REVIEWER_GROUPS.map((g) => g.label)).toEqual([ + 'All Members', + '95XXPRT Members', + '97XXPRT Members', + ]); + }); + + test('splits the alphabet A-N and O-Z, leaving All Members unranged', () => { + const [all, first, second] = DEFAULT_REVIEWER_GROUPS; + expect(all.rangeStart).toBeNull(); + expect(all.rangeEnd).toBeNull(); + expect([first.rangeStart, first.rangeEnd]).toEqual(['A', 'N']); + expect([second.rangeStart, second.rangeEnd]).toEqual(['O', 'Z']); + }); + + test('only All Members is locked against editing', () => { + expect(DEFAULT_REVIEWER_GROUPS.filter((g) => g.editable === false).map((g) => g.key)).toEqual([ + 'all', + ]); + }); +}); + +describe('normaliseLetter', () => { + test('uppercases a single letter', () => { + expect(normaliseLetter('a')).toBe('A'); + expect(normaliseLetter('N')).toBe('N'); + }); + + test('trims surrounding whitespace, since the range arrives from a text input', () => { + expect(normaliseLetter(' n ')).toBe('N'); + }); + + test.each(['AB', '', '1', '-', null, undefined, 5, {}])( + 'rejects %p as a range letter', + (value) => { + expect(normaliseLetter(value)).toBeNull(); + }, + ); +}); + +describe('groupingLetter', () => { + test('takes the first letter of the last name', () => { + expect(groupingLetter({ firstName: 'Jane', lastName: 'Doe' })).toBe('D'); + }); + + test('is case insensitive', () => { + expect(groupingLetter({ firstName: 'jane', lastName: 'doe' })).toBe('D'); + }); + + test('strips accents so Alvarez and Álvarez land in the same group', () => { + expect(groupingLetter({ firstName: 'Ana', lastName: 'Álvarez' })).toBe('A'); + expect(groupingLetter({ firstName: 'Omar', lastName: 'Ödegaard' })).toBe('O'); + }); + + test('falls back to the first name when the last name is missing or blank', () => { + expect(groupingLetter({ firstName: 'Prince', lastName: '' })).toBe('P'); + expect(groupingLetter({ firstName: 'Prince' })).toBe('P'); + }); + + test('returns null when neither name yields an A-Z letter', () => { + expect(groupingLetter({ firstName: '', lastName: '' })).toBeNull(); + expect(groupingLetter({ firstName: '123', lastName: '456' })).toBeNull(); + expect(groupingLetter({})).toBeNull(); + }); +}); + +describe('isReviewerInGroup', () => { + test('All Members takes everybody, including names with no usable letter', () => { + expect(isReviewerInGroup({ firstName: 'Jane', lastName: 'Doe' }, ALL_GROUP)).toBe(true); + expect(isReviewerInGroup({ firstName: '', lastName: '' }, ALL_GROUP)).toBe(true); + }); + + test('matches a reviewer whose letter sits inside the range', () => { + expect(isReviewerInGroup({ firstName: 'Jane', lastName: 'Doe' }, A_TO_N)).toBe(true); + expect(isReviewerInGroup({ firstName: 'Jane', lastName: 'Doe' }, O_TO_Z)).toBe(false); + }); + + test('the range is inclusive at both ends', () => { + expect(isReviewerInGroup({ lastName: 'Adams', firstName: 'A' }, A_TO_N)).toBe(true); + expect(isReviewerInGroup({ lastName: 'Nolan', firstName: 'N' }, A_TO_N)).toBe(true); + expect(isReviewerInGroup({ lastName: 'Olsen', firstName: 'O' }, A_TO_N)).toBe(false); + }); + + test('a reviewer with no usable letter falls into no lettered group', () => { + expect(isReviewerInGroup({ firstName: '123', lastName: '' }, A_TO_N)).toBe(false); + expect(isReviewerInGroup({ firstName: '123', lastName: '' }, O_TO_Z)).toBe(false); + }); + + test('the two default ranges partition every reviewer exactly once', () => { + const reviewers = ['Adams', 'Doe', 'Nolan', 'Olsen', 'Zhang'].map((lastName) => ({ + firstName: 'Test', + lastName, + })); + + reviewers.forEach((reviewer) => { + const matches = [A_TO_N, O_TO_Z].filter((group) => isReviewerInGroup(reviewer, group)); + expect(matches).toHaveLength(1); + }); + }); +}); + +describe('validateRange', () => { + test('accepts a well formed range and returns the normalised letters', () => { + expect(validateRange({ rangeStart: 'a', rangeEnd: 'n' })).toEqual({ + valid: true, + rangeStart: 'A', + rangeEnd: 'N', + error: null, + }); + }); + + test('accepts a single letter range', () => { + expect(validateRange({ rangeStart: 'Q', rangeEnd: 'Q' }).valid).toBe(true); + }); + + test('rejects a range that ends before it starts', () => { + const result = validateRange({ rangeStart: 'N', rangeEnd: 'A' }); + expect(result.valid).toBe(false); + expect(result.error).toMatch(/before/i); + }); + + test.each([ + ['AB', 'N'], + ['A', ''], + ['1', 'N'], + [null, 'N'], + ['A', undefined], + ])('rejects the range %p to %p', (rangeStart, rangeEnd) => { + expect(validateRange({ rangeStart, rangeEnd }).valid).toBe(false); + }); +}); + +describe('rangeWarnings', () => { + test('is silent when the lettered groups tile the alphabet exactly', () => { + expect(rangeWarnings([ALL_GROUP, A_TO_N, O_TO_Z])).toEqual([]); + }); + + test('reports letters that two groups both claim', () => { + const widened = { ...A_TO_N, rangeEnd: 'P' }; + const warnings = rangeWarnings([ALL_GROUP, widened, O_TO_Z]); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('95XXPRT Members'); + expect(warnings[0]).toContain('97XXPRT Members'); + expect(warnings[0]).toContain('O-P'); + }); + + test('reports letters no group covers', () => { + const narrowed = { ...A_TO_N, rangeEnd: 'K' }; + const warnings = rangeWarnings([ALL_GROUP, narrowed, O_TO_Z]); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('L-N'); + }); + + test('describes a single uncovered letter without a range dash', () => { + const narrowed = { ...A_TO_N, rangeEnd: 'M' }; + const warnings = rangeWarnings([ALL_GROUP, narrowed, O_TO_Z]); + + expect(warnings[0]).toContain('N'); + expect(warnings[0]).not.toContain('N-N'); + }); + + test('ignores the unranged All Members group rather than treating it as a gap', () => { + expect(rangeWarnings([ALL_GROUP])).toEqual(['No group covers A-Z']); + }); +}); + +describe('slugifyGroupKey', () => { + test('builds a url safe key from the label', () => { + expect(slugifyGroupKey('99XXPRT Members', [])).toBe('99xxprt-members'); + }); + + test('drops punctuation and collapses whitespace', () => { + expect(slugifyGroupKey(' Weekend Crew (new)! ', [])).toBe('weekend-crew-new'); + }); + + test('suffixes a number rather than colliding with an existing key', () => { + expect(slugifyGroupKey('All Members', ['all-members'])).toBe('all-members-2'); + expect(slugifyGroupKey('All Members', ['all-members', 'all-members-2'])).toBe('all-members-3'); + }); + + test('returns null for a label with nothing sluggable in it', () => { + expect(slugifyGroupKey('!!!', [])).toBeNull(); + expect(slugifyGroupKey('', [])).toBeNull(); + }); +}); diff --git a/src/models/reviewerGroup.js b/src/models/reviewerGroup.js new file mode 100644 index 0000000000..b23e4cd876 --- /dev/null +++ b/src/models/reviewerGroup.js @@ -0,0 +1,35 @@ +// src/models/reviewerGroup.js +const mongoose = require('mongoose'); + +const { Schema } = mongoose; + +/** + * A "Review for This Week" group on the Promotion Eligibility dashboard. + * + * Membership is NOT stored here. A group owns an alphabetical range and + * membership is derived from it at read time, so nobody has to maintain a + * member list as volunteers join and leave. See `helpers/reviewerGroupHelper.js`. + */ +const reviewerGroupSchema = new Schema({ + // Stable identifier the frontend sends back to filter the table. Derived from + // the label once at creation and never changed, so a rename does not break a + // dropdown selection the user already made. + key: { type: String, required: true, unique: true }, + label: { type: String, required: true }, + + // Inclusive single letters, A to Z. Both null means the group takes everybody, + // which is only ever the All Members group. + rangeStart: { type: String, default: null }, + rangeEnd: { type: String, default: null }, + + // False locks the group against renaming and range edits. Only All Members. + editable: { type: Boolean, default: true }, + + // Dropdown position. + sortOrder: { type: Number, default: 0 }, + + updatedBy: { type: Schema.Types.ObjectId, ref: 'userProfiles', default: null }, + updatedAt: { type: Date, default: Date.now }, +}); + +module.exports = mongoose.model('reviewerGroup', reviewerGroupSchema); diff --git a/src/routes/promotionEligibilityRouter.js b/src/routes/promotionEligibilityRouter.js index 0ae50e2e9b..3fa4e28031 100644 --- a/src/routes/promotionEligibilityRouter.js +++ b/src/routes/promotionEligibilityRouter.js @@ -1,15 +1,15 @@ // src/routes/promotionEligibilityRouter.js const express = require('express'); -// Modify the 'routes' function to accept 'PromotionEligibility' as well -const routes = function (userProfile, timeEntry, task, PromotionEligibility) { - // Pass the new model to your controller +const routes = function (userProfile, timeEntry, task, PromotionEligibility, ReviewerGroup) { const controller = require('../controllers/promotionEligibilityController')( userProfile, timeEntry, task, PromotionEligibility, + ReviewerGroup, ); + const reviewerGroups = require('../controllers/reviewerGroupController')(ReviewerGroup); const router = express.Router(); router.route('/promotion-eligibility').post(controller.getPromotionEligibilityData); @@ -18,6 +18,16 @@ const routes = function (userProfile, timeEntry, task, PromotionEligibility) { router.route('/promote-members').post(controller.promoteMembers); + // Reads are POST for the same reason the dashboard read is: the permission + // check reads `req.body.requestor`, and a GET carries no body. That was fixed + // once already in BE PR 2201, so it is not re-litigated here. It is also why + // creating a group posts to /new rather than to the collection path. + router.route('/reviewer-groups').post(reviewerGroups.getReviewerGroups); + + router.route('/reviewer-groups/new').post(reviewerGroups.createReviewerGroup); + + router.route('/reviewer-groups/:groupKey').patch(reviewerGroups.updateReviewerGroup); + return router; }; diff --git a/src/startup/routes.js b/src/startup/routes.js index f0bd8d0ced..abf1f555d9 100644 --- a/src/startup/routes.js +++ b/src/startup/routes.js @@ -410,6 +410,7 @@ const permissionRouter = require('../routes/permissionRouter'); // Analytics const analyticsPopularPRsRouter = require('../routes/analyticsPopularPRsRouter')(); const PromotionEligibility = require('../models/promotionEligibility'); +const ReviewerGroup = require('../models/reviewerGroup'); const promotionEligibilityRouter = require('../routes/promotionEligibilityRouter'); @@ -660,7 +661,10 @@ module.exports = function (app) { app.use('/api/userstate', userStateRouter); app.use('/api', promotionDetailsRouter); app.use('/api/analytics', analyticsPopularPRsRouter); - app.use('/api/', promotionEligibilityRouter(userProfile, timeEntry, task, PromotionEligibility)); + app.use( + '/api/', + promotionEligibilityRouter(userProfile, timeEntry, task, PromotionEligibility, ReviewerGroup), + ); // PR Analytics app.use('/api', prInsightsRouter); From 5da3a5f6bc92931038e52e22c3c4ebcd3f11cdea Mon Sep 17 00:00:00 2001 From: sitaram Date: Fri, 21 Aug 2026 13:03:12 -0400 Subject: [PATCH 3/7] fix(promotion-eligibility): group reviewers by first name, not last 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. --- src/helpers/reviewerGroupHelper.js | 13 +++++---- src/helpers/reviewerGroupHelper.spec.js | 37 ++++++++++++++----------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/helpers/reviewerGroupHelper.js b/src/helpers/reviewerGroupHelper.js index 448c13ec6b..8541de40ed 100644 --- a/src/helpers/reviewerGroupHelper.js +++ b/src/helpers/reviewerGroupHelper.js @@ -6,10 +6,10 @@ * membership as volunteers join and leave, and an Owner editing a range * re-splits the table immediately. * - * The letter comes from the reviewer's LAST name, falling back to the first - * name when there is no usable last name. The spec does not say which name to - * use, so this is an assumption, deliberately kept in `groupingLetter` alone so - * that switching to first names is a one-line change. + * The letter comes from the reviewer's FIRST name, falling back to the last + * name when there is no usable first name. The spec is explicit about this: + * "95XXPRT Members (Members with first names starting with A-N)". It is kept in + * `groupingLetter` alone so the choice stays a one-line change. */ const LETTERS = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''); @@ -68,6 +68,9 @@ function firstLetterOf(name) { /** * The letter that decides which group a reviewer falls into. * + * The spec names the groups by first name, so that is what is read first. The + * last name is only a fallback for accounts with no usable first name. + * * Returns null when neither name yields an A-Z letter, which happens on * placeholder and test accounts. Those reviewers match no lettered group, and * `rangeWarnings` is not the place that surfaces them, so they are only ever @@ -75,7 +78,7 @@ function firstLetterOf(name) { */ function groupingLetter(reviewer) { if (!reviewer) return null; - return firstLetterOf(reviewer.lastName) || firstLetterOf(reviewer.firstName); + return firstLetterOf(reviewer.firstName) || firstLetterOf(reviewer.lastName); } function letterInGroupRange(letter, group) { diff --git a/src/helpers/reviewerGroupHelper.spec.js b/src/helpers/reviewerGroupHelper.spec.js index ef03563362..35177f7ce7 100644 --- a/src/helpers/reviewerGroupHelper.spec.js +++ b/src/helpers/reviewerGroupHelper.spec.js @@ -56,22 +56,22 @@ describe('normaliseLetter', () => { }); describe('groupingLetter', () => { - test('takes the first letter of the last name', () => { - expect(groupingLetter({ firstName: 'Jane', lastName: 'Doe' })).toBe('D'); + test('takes the first letter of the first name, as the spec names the groups', () => { + expect(groupingLetter({ firstName: 'Jane', lastName: 'Doe' })).toBe('J'); }); test('is case insensitive', () => { - expect(groupingLetter({ firstName: 'jane', lastName: 'doe' })).toBe('D'); + expect(groupingLetter({ firstName: 'jane', lastName: 'doe' })).toBe('J'); }); test('strips accents so Alvarez and Álvarez land in the same group', () => { - expect(groupingLetter({ firstName: 'Ana', lastName: 'Álvarez' })).toBe('A'); - expect(groupingLetter({ firstName: 'Omar', lastName: 'Ödegaard' })).toBe('O'); + expect(groupingLetter({ firstName: 'Álvaro', lastName: 'Ruiz' })).toBe('A'); + expect(groupingLetter({ firstName: 'Ödegaard', lastName: 'Ruiz' })).toBe('O'); }); - test('falls back to the first name when the last name is missing or blank', () => { - expect(groupingLetter({ firstName: 'Prince', lastName: '' })).toBe('P'); - expect(groupingLetter({ firstName: 'Prince' })).toBe('P'); + test('falls back to the last name when the first name is missing or blank', () => { + expect(groupingLetter({ firstName: '', lastName: 'Prince' })).toBe('P'); + expect(groupingLetter({ lastName: 'Prince' })).toBe('P'); }); test('returns null when neither name yields an A-Z letter', () => { @@ -92,21 +92,26 @@ describe('isReviewerInGroup', () => { expect(isReviewerInGroup({ firstName: 'Jane', lastName: 'Doe' }, O_TO_Z)).toBe(false); }); + test('the first name decides the group, not the last name', () => { + expect(isReviewerInGroup({ firstName: 'Ana', lastName: 'Zhang' }, A_TO_N)).toBe(true); + expect(isReviewerInGroup({ firstName: 'Ana', lastName: 'Zhang' }, O_TO_Z)).toBe(false); + }); + test('the range is inclusive at both ends', () => { - expect(isReviewerInGroup({ lastName: 'Adams', firstName: 'A' }, A_TO_N)).toBe(true); - expect(isReviewerInGroup({ lastName: 'Nolan', firstName: 'N' }, A_TO_N)).toBe(true); - expect(isReviewerInGroup({ lastName: 'Olsen', firstName: 'O' }, A_TO_N)).toBe(false); + expect(isReviewerInGroup({ firstName: 'Adams', lastName: 'A' }, A_TO_N)).toBe(true); + expect(isReviewerInGroup({ firstName: 'Nolan', lastName: 'N' }, A_TO_N)).toBe(true); + expect(isReviewerInGroup({ firstName: 'Olsen', lastName: 'O' }, A_TO_N)).toBe(false); }); test('a reviewer with no usable letter falls into no lettered group', () => { - expect(isReviewerInGroup({ firstName: '123', lastName: '' }, A_TO_N)).toBe(false); - expect(isReviewerInGroup({ firstName: '123', lastName: '' }, O_TO_Z)).toBe(false); + expect(isReviewerInGroup({ firstName: '123', lastName: '456' }, A_TO_N)).toBe(false); + expect(isReviewerInGroup({ firstName: '123', lastName: '456' }, O_TO_Z)).toBe(false); }); test('the two default ranges partition every reviewer exactly once', () => { - const reviewers = ['Adams', 'Doe', 'Nolan', 'Olsen', 'Zhang'].map((lastName) => ({ - firstName: 'Test', - lastName, + const reviewers = ['Adams', 'Doe', 'Nolan', 'Olsen', 'Zhang'].map((firstName) => ({ + firstName, + lastName: 'Test', })); reviewers.forEach((reviewer) => { From 2be8fdaf8a5f7bf1e343957fccf817bda92e06bc Mon Sep 17 00:00:00 2001 From: sitaram Date: Fri, 21 Aug 2026 13:09:35 -0400 Subject: [PATCH 4/7] feat(promotion-eligibility): count successful weeks by PRs reviewed, 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. --- .../promotionEligibilityController.js | 75 +++++-- .../promotionEligibilityController.test.js | 202 ++++++++++++++++++ src/helpers/promotionEligibilityHelper.js | 115 +++++++++- .../promotionEligibilityHelper.spec.js | 170 ++++++++++++++- src/models/promotionEligibility.js | 11 + 5 files changed, 557 insertions(+), 16 deletions(-) diff --git a/src/controllers/promotionEligibilityController.js b/src/controllers/promotionEligibilityController.js index 7f604e36ee..00e704ffcc 100644 --- a/src/controllers/promotionEligibilityController.js +++ b/src/controllers/promotionEligibilityController.js @@ -3,9 +3,15 @@ const mongoose = require('mongoose'); const { hasPermission } = require('../utilities/permissions'); const logger = require('../startup/logger'); const { ValidationError } = require('../utilities/errorHandling/customError'); -const { resolvePrsNeeded } = require('../helpers/promotionEligibilityHelper'); +const { + resolvePrsNeeded, + summariseWeeks, + mongoWeekOf, +} = require('../helpers/promotionEligibilityHelper'); const { DEFAULT_REVIEWER_GROUPS, isReviewerInGroup } = require('../helpers/reviewerGroupHelper'); +const ONE_WEEK_MS = 7 * 24 * 60 * 60 * 1000; + const promotionEligibilityController = function ( UserProfile, TimeEntry, @@ -13,22 +19,48 @@ const promotionEligibilityController = function ( PromotionEligibility, ReviewerGroup, ) { - const calculateWeeksMetRequirement = async (userId, pledgedHours) => { - const weeklyTasks = await TimeEntry.aggregate([ + /** + * How many PRs a reviewer reviewed in each week they logged review work. + * + * The spec counts weeks where the reviewer "met or exceeded PR requirement", + * so this has to be a count of reviews, not the hours the old version summed. + * Actual PR review records exist (`pullRequestReview`) 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 this page uses: one + * review task worked on in a week counts as one PR reviewed that week. + * `$addToSet` rather than a plain count, so several time entries against the + * same task in one week are still one review. + * + * Grouped by year as well as week. `$week` alone repeats every year, which + * would have folded the same week number from different years together. + * + * @returns {Promise>} newest first + */ + const weeklyReviewCounts = async (userId) => + TimeEntry.aggregate([ { $match: { personId: mongoose.Types.ObjectId(userId), isTangible: true } }, { $lookup: { from: 'tasks', localField: 'taskId', foreignField: '_id', as: 'taskInfo' } }, { $unwind: '$taskInfo' }, { $match: { 'taskInfo.taskName': { $regex: /review|pr/i } } }, { $group: { - _id: { $week: { $toDate: '$dateOfWork' } }, - totalHours: { $sum: { $divide: ['$totalSeconds', 3600] } }, + _id: { + year: { $year: { $toDate: '$dateOfWork' } }, + week: { $week: { $toDate: '$dateOfWork' } }, + }, + reviewedTaskIds: { $addToSet: '$taskId' }, }, }, - { $match: { totalHours: { $gte: pledgedHours / 2 } } }, + { + $project: { + _id: 0, + year: '$_id.year', + week: '$_id.week', + reviewCount: { $size: '$reviewedTaskIds' }, + }, + }, + { $sort: { year: -1, week: -1 } }, ]); - return weeklyTasks.length; - }; /** * Resolve the "Review for This Week" group the caller asked to see. @@ -53,6 +85,11 @@ const promotionEligibilityController = function ( } try { + // One clock for the whole read, so every reviewer is measured against the + // same "now" and two rows cannot land on different sides of a week + // boundary partway through the loop. + const now = new Date(); + // `undefined` means the key was supplied but matches nothing, which is a // stale dropdown on the client rather than a request for the whole table. const group = await resolveRequestedGroup(req.body.groupKey); @@ -98,12 +135,15 @@ const promotionEligibilityController = function ( taskName: { $regex: /review|pr/i }, }); - const successfulWeeks = await calculateWeeksMetRequirement(user._id, pledgedHours); + const { successfulWeeks, remainingWeeks, weeklyRequirementsMet } = summariseWeeks({ + weeklyCounts: await weeklyReviewCounts(user._id), + prsNeeded, + currentWeek: mongoWeekOf(now), + }); - const remainingWeeks = Math.max(0, 2 - successfulWeeks); - const isNewMember = - (new Date() - new Date(user.createdDate)) / (1000 * 60 * 60 * 24 * 30.44) < 6; - const weeklyRequirementsMet = successfulWeeks >= 2; + // The spec splits the table into "New Members (joined <= 1 week ago)" + // and "Existing Members (older than a week)". + const isNewMember = now - new Date(user.createdDate) <= ONE_WEEK_MS; const dataEntry = { reviewerId: user._id, @@ -117,10 +157,17 @@ const promotionEligibilityController = function ( prsNeededSource, committedHoursChanged, totalReviews, + // Prior weeks in which the reviewer cleared `prsNeeded`. Exposed + // alongside `remainingWeeks` so the page can show the progress rather + // than only what is left. + successfulWeeks, remainingWeeks, isNewMember, + // Per the spec, whether the requirement is met "for the current + // period", meaning this week. It is no longer a synonym for being + // eligible to promote, which is `remainingWeeks === 0`. weeklyRequirementsMet, - calculatedAt: new Date(), + calculatedAt: now, }; // Save/update the calculated data in the new collection concurrently diff --git a/src/controllers/promotionEligibilityController.test.js b/src/controllers/promotionEligibilityController.test.js index 0fd25df15e..bd341ffba9 100644 --- a/src/controllers/promotionEligibilityController.test.js +++ b/src/controllers/promotionEligibilityController.test.js @@ -312,6 +312,208 @@ describe('getPromotionEligibilityData, filtering by reviewer group', () => { }); }); +describe('getPromotionEligibilityData, weekly requirements and remaining weeks', () => { + const REVIEWER = { + _id: '637af0c0fb9bbc1e308cff62', + firstName: 'Ann', + lastName: 'Adams', + weeklycommittedHours: 10, // 7 PRs needed + createdDate: '2020-01-01', + }; + + let UserProfile; + let TimeEntry; + let Task; + let PromotionEligibility; + let controller; + let mockRes; + + // Fixed so the "current week" never drifts under the suite. 2026-08-19 is a + // Wednesday, which MongoDB's $week puts in 2026 week 33. + const NOW = new Date('2026-08-19T12:00:00Z'); + const CURRENT_WEEK = { year: 2026, week: 33 }; + + const request = () => ({ body: { requestor: { requestorId: OWNER_ID, role: 'Administrator' } } }); + + const entryReturned = () => mockRes.json.mock.calls[0][0][0]; + + const givenWeeklyCounts = (counts) => { + TimeEntry.aggregate = jest.fn().mockResolvedValue(counts); + }; + + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }).setSystemTime(NOW); + hasPermission.mockResolvedValue(true); + + UserProfile = { find: jest.fn(() => ({ lean: () => Promise.resolve([REVIEWER]) })) }; + TimeEntry = { aggregate: jest.fn().mockResolvedValue([]) }; + Task = { countDocuments: jest.fn().mockResolvedValue(0) }; + PromotionEligibility = { + find: jest.fn(() => ({ lean: () => Promise.resolve([]) })), + findOneAndUpdate: jest.fn().mockResolvedValue({}), + }; + + controller = promotionEligibilityController( + UserProfile, + TimeEntry, + Task, + PromotionEligibility, + null, + ); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('counts distinct review tasks per week, not hours, when deciding a successful week', async () => { + await controller.getPromotionEligibilityData(request(), mockRes); + + const pipeline = TimeEntry.aggregate.mock.calls[0][0]; + const group = pipeline.find((stage) => stage.$group); + const project = pipeline.find((stage) => stage.$project); + + // The old version summed hours and filtered on pledgedHours / 2. Nothing in + // the pipeline should be doing arithmetic on hours any more. + expect(JSON.stringify(pipeline)).not.toContain('totalSeconds'); + expect(group.$group.reviewedTaskIds).toEqual({ $addToSet: '$taskId' }); + expect(project.$project.reviewCount).toEqual({ $size: '$reviewedTaskIds' }); + }); + + it('groups by year as well as week, so the same week number across years stays separate', async () => { + await controller.getPromotionEligibilityData(request(), mockRes); + + const pipeline = TimeEntry.aggregate.mock.calls[0][0]; + const group = pipeline.find((stage) => stage.$group); + + expect(group.$group._id).toHaveProperty('year'); + expect(group.$group._id).toHaveProperty('week'); + }); + + it('counts a prior week that met the requirement and leaves one week remaining', async () => { + givenWeeklyCounts([{ year: 2026, week: 32, reviewCount: 7 }]); + + await controller.getPromotionEligibilityData(request(), mockRes); + + const entry = entryReturned(); + expect(entry.prsNeeded).toBe(7); + expect(entry.successfulWeeks).toBe(1); + expect(entry.remainingWeeks).toBe(1); + }); + + it('does not count a prior week that fell short', async () => { + givenWeeklyCounts([ + { year: 2026, week: 32, reviewCount: 6 }, + { year: 2026, week: 31, reviewCount: 7 }, + ]); + + await controller.getPromotionEligibilityData(request(), mockRes); + + expect(entryReturned().successfulWeeks).toBe(1); + }); + + it('reports the current week through weeklyRequirementsMet without counting it', async () => { + givenWeeklyCounts([{ ...CURRENT_WEEK, reviewCount: 8 }]); + + await controller.getPromotionEligibilityData(request(), mockRes); + + const entry = entryReturned(); + expect(entry.weeklyRequirementsMet).toBe(true); + expect(entry.successfulWeeks).toBe(0); + expect(entry.remainingWeeks).toBe(2); + }); + + it('holds weeklyRequirementsMet false when this week is still short', async () => { + givenWeeklyCounts([ + { ...CURRENT_WEEK, reviewCount: 3 }, + { year: 2026, week: 32, reviewCount: 7 }, + { year: 2026, week: 31, reviewCount: 7 }, + ]); + + await controller.getPromotionEligibilityData(request(), mockRes); + + const entry = entryReturned(); + expect(entry.weeklyRequirementsMet).toBe(false); + // Promotion eligibility is remainingWeeks, and it is separately satisfied. + expect(entry.remainingWeeks).toBe(0); + }); + + it('never counts a week for a reviewer whose committed hours require nothing', async () => { + UserProfile.find = jest.fn(() => ({ + lean: () => Promise.resolve([{ ...REVIEWER, weeklycommittedHours: 0 }]), + })); + givenWeeklyCounts([ + { ...CURRENT_WEEK, reviewCount: 0 }, + { year: 2026, week: 32, reviewCount: 0 }, + { year: 2026, week: 31, reviewCount: 0 }, + ]); + + await controller.getPromotionEligibilityData(request(), mockRes); + + const entry = entryReturned(); + expect(entry.prsNeeded).toBe(0); + expect(entry.successfulWeeks).toBe(0); + expect(entry.remainingWeeks).toBe(2); + expect(entry.weeklyRequirementsMet).toBe(false); + }); + + it('uses the Owner override, not the bands, as the weekly bar', async () => { + PromotionEligibility.find = jest.fn(() => ({ + lean: () => + Promise.resolve([{ reviewerId: REVIEWER._id, pledgedHours: 10, prsNeededOverride: 12 }]), + })); + givenWeeklyCounts([ + { year: 2026, week: 32, reviewCount: 9 }, // clears 7, short of 12 + { year: 2026, week: 31, reviewCount: 12 }, + ]); + + await controller.getPromotionEligibilityData(request(), mockRes); + + const entry = entryReturned(); + expect(entry.prsNeeded).toBe(12); + expect(entry.successfulWeeks).toBe(1); + }); + + describe('isNewMember', () => { + const withCreatedDate = (createdDate) => { + UserProfile.find = jest.fn(() => ({ + lean: () => Promise.resolve([{ ...REVIEWER, createdDate }]), + })); + }; + + it('is true for somebody who joined inside the last week', async () => { + withCreatedDate('2026-08-17T12:00:00Z'); + + await controller.getPromotionEligibilityData(request(), mockRes); + + expect(entryReturned().isNewMember).toBe(true); + }); + + it('is true exactly a week out, which the spec includes', async () => { + withCreatedDate('2026-08-12T12:00:00Z'); + + await controller.getPromotionEligibilityData(request(), mockRes); + + expect(entryReturned().isNewMember).toBe(true); + }); + + it('is false a day past the week, not months later as it used to be', async () => { + withCreatedDate('2026-08-11T12:00:00Z'); + + await controller.getPromotionEligibilityData(request(), mockRes); + + expect(entryReturned().isNewMember).toBe(false); + }); + }); +}); + describe('mongoose ObjectId validation assumption', () => { it('treats the ids used above as valid, so the 400 tests fail for the right reason', () => { expect(mongoose.Types.ObjectId.isValid(REVIEWER_ID)).toBe(true); diff --git a/src/helpers/promotionEligibilityHelper.js b/src/helpers/promotionEligibilityHelper.js index 9aa534fa4b..5d46ab64db 100644 --- a/src/helpers/promotionEligibilityHelper.js +++ b/src/helpers/promotionEligibilityHelper.js @@ -8,6 +8,10 @@ * 10 PRs for 15 - 25.99 hr/wk * 20 PRs for 26 - 35.99 hr/wk * 30 PRs for 36 - 40 hr/wk + * + * The "Weekly Requirements" and "Remaining Weeks" columns hang off the same + * figure: a week counts as successful when the reviewer reviewed at least + * `prsNeeded` PRs in it, and promotion needs two such weeks. */ const PRS_NEEDED_BANDS = [ @@ -86,4 +90,113 @@ function resolvePrsNeeded({ committedHours, existingRecord }) { }; } -module.exports = { PRS_NEEDED_BANDS, getPrsNeeded, resolvePrsNeeded }; +/** + * The year and week number a date falls in, matching MongoDB's `$year` and + * `$week` exactly. + * + * `$week` counts weeks that begin on Sunday, which is also how HGN weeks run, + * and puts the days before the year's first Sunday in week 0. Both sides of the + * comparison have to agree on that, because the per-week counts are grouped in + * an aggregation and "which week is now" is worked out here in JavaScript. + * + * UTC throughout, since `dateOfWork` is a plain "YYYY-MM-DD" string that + * `$toDate` reads as UTC midnight. + * + * @param {Date} date + * @returns {{year: number, week: number}} + */ +function mongoWeekOf(date) { + const year = date.getUTCFullYear(); + + const startOfYear = Date.UTC(year, 0, 1); + const dayOfYear = Math.floor( + (Date.UTC(year, date.getUTCMonth(), date.getUTCDate()) - startOfYear) / 86400000, + ); + + // Day index of the year's first Sunday. Jan 1 on a Sunday makes this 0. + const firstSunday = (7 - new Date(startOfYear).getUTCDay()) % 7; + + return { year, week: Math.floor((dayOfYear - firstSunday + 7) / 7) }; +} + +/** + * Successful weeks a reviewer must accumulate before they can be promoted. + * The spec: "They need 2 weeks of successful weeks to get promoted. So + * 'Remaining weeks' should be 2 - (number of previous weeks where they have + * satisfied the minimum requirement)". + */ +const SUCCESSFUL_WEEKS_REQUIRED = 2; + +/** + * Whether one week's review count clears that week's requirement. + * + * A requirement of zero is treated as "not assessable" rather than as trivially + * met. Committed hours of zero or less produce `prsNeeded === 0`, and dev alone + * has 46 accounts in that state plus one at -3 hours. Counting their weeks as + * successful would walk them to zero remaining weeks and offer them up for + * promotion without a single review. This is the same edge as open question 3 + * to Jae and moves with it. + * + * @param {number} reviewsThatWeek PRs the reviewer reviewed in the week + * @param {number} prsNeeded PRs required that week, from the committed hours bands + * @returns {boolean} + */ +function weekMeetsRequirement(reviewsThatWeek, prsNeeded) { + const required = Number(prsNeeded); + if (!Number.isFinite(required) || required <= 0) return false; + + const reviewed = Number(reviewsThatWeek); + if (!Number.isFinite(reviewed)) return false; + + return reviewed >= required; +} + +/** + * Turn a reviewer's per-week review counts into the three figures the table's + * "Weekly Requirements" and "Remaining Weeks" columns need. + * + * `weeklyCounts` is expected newest first and to include the current, still + * running week when there is one. The current week is deliberately excluded + * from `successfulWeeks`: the spec counts "previous weeks where they have + * satisfied the minimum requirement", and a week still in progress has not + * finished failing yet. It drives `weeklyRequirementsMet` instead, which the + * spec describes as satisfaction "for the current period". + * + * @param {object} params + * @param {Array<{year: number, week: number, reviewCount: number}>} params.weeklyCounts newest first + * @param {number} params.prsNeeded PRs required per week + * @param {{year: number, week: number}} params.currentWeek the week "now" falls in + * @returns {{successfulWeeks: number, remainingWeeks: number, weeklyRequirementsMet: boolean}} + */ +function summariseWeeks({ weeklyCounts, prsNeeded, currentWeek }) { + const counts = Array.isArray(weeklyCounts) ? weeklyCounts : []; + + const isCurrentWeek = (entry) => + Boolean(currentWeek) && entry.year === currentWeek.year && entry.week === currentWeek.week; + + const currentEntry = counts.find(isCurrentWeek); + const weeklyRequirementsMet = weekMeetsRequirement( + currentEntry ? currentEntry.reviewCount : 0, + prsNeeded, + ); + + const successfulWeeks = counts.filter( + (entry) => !isCurrentWeek(entry) && weekMeetsRequirement(entry.reviewCount, prsNeeded), + ).length; + + return { + successfulWeeks, + remainingWeeks: Math.max(0, SUCCESSFUL_WEEKS_REQUIRED - successfulWeeks), + weeklyRequirementsMet, + }; +} + +module.exports = { + PRS_NEEDED_BANDS, + SUCCESSFUL_WEEKS_REQUIRED, + getPrsNeeded, + resolvePrsNeeded, + mongoWeekOf, + weekMeetsRequirement, + summariseWeeks, +}; diff --git a/src/helpers/promotionEligibilityHelper.spec.js b/src/helpers/promotionEligibilityHelper.spec.js index b78d805d3d..6d2163d9df 100644 --- a/src/helpers/promotionEligibilityHelper.spec.js +++ b/src/helpers/promotionEligibilityHelper.spec.js @@ -1,4 +1,11 @@ -const { getPrsNeeded, resolvePrsNeeded } = require('./promotionEligibilityHelper'); +const { + getPrsNeeded, + resolvePrsNeeded, + mongoWeekOf, + weekMeetsRequirement, + summariseWeeks, + SUCCESSFUL_WEEKS_REQUIRED, +} = require('./promotionEligibilityHelper'); describe('getPrsNeeded', () => { test('returns 7 across the 10 to 14.99 band', () => { @@ -114,3 +121,164 @@ describe('resolvePrsNeeded', () => { expect(result.prsNeeded).toBe(10); }); }); + +describe('mongoWeekOf', () => { + const weekOf = (iso) => mongoWeekOf(new Date(`${iso}T00:00:00Z`)); + + // Every expectation below was taken from MongoDB itself, by running + // { $year: ... } and { $week: ... } over the same dates. The aggregation + // groups per week and this function decides which of those groups is "now", + // so the two have to agree exactly or the current week never matches. + test('weeks begin on Sunday', () => { + // 2026-08-16 is a Sunday, 2026-08-22 the Saturday that closes the week. + expect(weekOf('2026-08-16')).toEqual({ year: 2026, week: 33 }); + expect(weekOf('2026-08-21')).toEqual({ year: 2026, week: 33 }); + expect(weekOf('2026-08-22')).toEqual({ year: 2026, week: 33 }); + expect(weekOf('2026-08-23')).toEqual({ year: 2026, week: 34 }); + }); + + test('days before the first Sunday of the year are week 0', () => { + // 2026-01-01 is a Thursday, so the first Sunday is 2026-01-04. + expect(weekOf('2026-01-01')).toEqual({ year: 2026, week: 0 }); + expect(weekOf('2026-01-03')).toEqual({ year: 2026, week: 0 }); + expect(weekOf('2026-01-04')).toEqual({ year: 2026, week: 1 }); + }); + + test('a year that opens on a Sunday has no week 0', () => { + // 2023-01-01 was a Sunday. + expect(weekOf('2023-01-01')).toEqual({ year: 2023, week: 1 }); + expect(weekOf('2023-01-07')).toEqual({ year: 2023, week: 1 }); + expect(weekOf('2023-01-08')).toEqual({ year: 2023, week: 2 }); + }); + + test('the year rolls over with the calendar, not with the week', () => { + expect(weekOf('2025-12-31')).toEqual({ year: 2025, week: 52 }); + expect(weekOf('2026-01-01')).toEqual({ year: 2026, week: 0 }); + }); +}); + +describe('weekMeetsRequirement', () => { + test('meeting the requirement exactly counts', () => { + expect(weekMeetsRequirement(7, 7)).toBe(true); + }); + + test('exceeding it counts and falling short does not', () => { + expect(weekMeetsRequirement(8, 7)).toBe(true); + expect(weekMeetsRequirement(6, 7)).toBe(false); + }); + + test('a requirement of zero is never met, so nobody is promoted on no reviews', () => { + expect(weekMeetsRequirement(0, 0)).toBe(false); + expect(weekMeetsRequirement(50, 0)).toBe(false); + }); + + test('a negative requirement is treated the same as zero', () => { + expect(weekMeetsRequirement(5, -3)).toBe(false); + }); + + test('non-numeric input on either side is not a pass', () => { + expect(weekMeetsRequirement(undefined, 7)).toBe(false); + expect(weekMeetsRequirement(7, undefined)).toBe(false); + expect(weekMeetsRequirement(NaN, 7)).toBe(false); + }); +}); + +describe('summariseWeeks', () => { + const CURRENT = { year: 2026, week: 33 }; + + const week = (weekNumber, reviewCount, year = 2026) => ({ year, week: weekNumber, reviewCount }); + + test('counts only prior weeks that cleared the requirement', () => { + const result = summariseWeeks({ + weeklyCounts: [week(33, 0), week(32, 10), week(31, 3), week(30, 7)], + prsNeeded: 7, + currentWeek: CURRENT, + }); + + expect(result.successfulWeeks).toBe(2); + expect(result.remainingWeeks).toBe(0); + }); + + test('remaining weeks is what is left of the two required', () => { + expect( + summariseWeeks({ weeklyCounts: [], prsNeeded: 7, currentWeek: CURRENT }).remainingWeeks, + ).toBe(SUCCESSFUL_WEEKS_REQUIRED); + + expect( + summariseWeeks({ weeklyCounts: [week(32, 7)], prsNeeded: 7, currentWeek: CURRENT }) + .remainingWeeks, + ).toBe(1); + }); + + test('remaining weeks floors at zero rather than going negative', () => { + const result = summariseWeeks({ + weeklyCounts: [week(32, 9), week(31, 9), week(30, 9), week(29, 9)], + prsNeeded: 7, + currentWeek: CURRENT, + }); + + expect(result.successfulWeeks).toBe(4); + expect(result.remainingWeeks).toBe(0); + }); + + test('the current week drives weeklyRequirementsMet and nothing else', () => { + const met = summariseWeeks({ + weeklyCounts: [week(33, 7)], + prsNeeded: 7, + currentWeek: CURRENT, + }); + expect(met.weeklyRequirementsMet).toBe(true); + // Still in progress, so it does not count towards promotion yet. + expect(met.successfulWeeks).toBe(0); + expect(met.remainingWeeks).toBe(2); + }); + + test('a current week short of the requirement is not met', () => { + const result = summariseWeeks({ + weeklyCounts: [week(33, 6), week(32, 10)], + prsNeeded: 7, + currentWeek: CURRENT, + }); + expect(result.weeklyRequirementsMet).toBe(false); + expect(result.successfulWeeks).toBe(1); + }); + + test('no entry for the current week means the requirement is not met', () => { + const result = summariseWeeks({ + weeklyCounts: [week(32, 10)], + prsNeeded: 7, + currentWeek: CURRENT, + }); + expect(result.weeklyRequirementsMet).toBe(false); + }); + + test('the same week number in a different year is a different week', () => { + const result = summariseWeeks({ + weeklyCounts: [week(33, 10, 2025)], + prsNeeded: 7, + currentWeek: CURRENT, + }); + + // 2025 week 33 is a prior week, not the current one. + expect(result.weeklyRequirementsMet).toBe(false); + expect(result.successfulWeeks).toBe(1); + }); + + test('a reviewer who needs nothing accumulates no successful weeks', () => { + const result = summariseWeeks({ + weeklyCounts: [week(33, 0), week(32, 0), week(31, 0)], + prsNeeded: 0, + currentWeek: CURRENT, + }); + + expect(result.successfulWeeks).toBe(0); + expect(result.remainingWeeks).toBe(2); + expect(result.weeklyRequirementsMet).toBe(false); + }); + + test('missing or malformed input is treated as no weeks worked', () => { + expect(summariseWeeks({ weeklyCounts: undefined, prsNeeded: 7, currentWeek: CURRENT })).toEqual( + { successfulWeeks: 0, remainingWeeks: 2, weeklyRequirementsMet: false }, + ); + }); +}); diff --git a/src/models/promotionEligibility.js b/src/models/promotionEligibility.js index 51a41cf9dd..1dd6440311 100644 --- a/src/models/promotionEligibility.js +++ b/src/models/promotionEligibility.js @@ -9,8 +9,19 @@ const promotionEligibilitySchema = new Schema({ pledgedHours: { type: Number, required: true }, requiredPRs: { type: Number, required: true }, totalReviews: { type: Number, required: true }, + + // Prior weeks in which the reviewer reviewed at least `prsNeeded` PRs. Two + // are required for promotion, so `remainingWeeks` is 2 minus this, floored + // at zero. The current, still running week is deliberately not counted here; + // it drives `weeklyRequirementsMet` instead. + successfulWeeks: { type: Number, default: 0 }, remainingWeeks: { type: Number, required: true }, + // Joined a week ago or less, which is the spec's split between the table's + // "New Members" and "Existing Members" sections. isNewMember: { type: Boolean, required: true }, + + // Whether the requirement is met for the current week. Not a synonym for + // being eligible to promote, which is `remainingWeeks === 0`. weeklyRequirementsMet: { type: Boolean, required: true }, calculatedAt: { type: Date, default: Date.now }, From 09add2100ab0344fa0ecbbdd86943fc9fe2621ff Mon Sep 17 00:00:00 2001 From: sitaram Date: Fri, 21 Aug 2026 17:02:47 -0400 Subject: [PATCH 5/7] feat(promotion-eligibility): place promoted reviewers onto teams 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. --- .../promotionEligibilityController.js | 249 ++++++++++++- .../promotionEligibilityController.test.js | 341 ++++++++++++++++++ src/controllers/teamController.js | 71 ++++ .../teamController.placement.spec.js | 231 ++++++++++++ src/helpers/teamPlacementHelper.js | 252 +++++++++++++ src/helpers/teamPlacementHelper.spec.js | 338 +++++++++++++++++ src/models/promotionEligibility.js | 6 + src/models/team.js | 31 +- src/routes/promotionEligibilityRouter.js | 17 +- src/startup/routes.js | 10 +- 10 files changed, 1533 insertions(+), 13 deletions(-) create mode 100644 src/controllers/teamController.placement.spec.js create mode 100644 src/helpers/teamPlacementHelper.js create mode 100644 src/helpers/teamPlacementHelper.spec.js diff --git a/src/controllers/promotionEligibilityController.js b/src/controllers/promotionEligibilityController.js index 00e704ffcc..6977d78016 100644 --- a/src/controllers/promotionEligibilityController.js +++ b/src/controllers/promotionEligibilityController.js @@ -2,6 +2,7 @@ const mongoose = require('mongoose'); const { hasPermission } = require('../utilities/permissions'); const logger = require('../startup/logger'); +const cache = require('../utilities/nodeCache')(); const { ValidationError } = require('../utilities/errorHandling/customError'); const { resolvePrsNeeded, @@ -9,15 +10,25 @@ const { mongoWeekOf, } = require('../helpers/promotionEligibilityHelper'); const { DEFAULT_REVIEWER_GROUPS, isReviewerInGroup } = require('../helpers/reviewerGroupHelper'); +const { placeReviewer, isPlaceableTeam } = require('../helpers/teamPlacementHelper'); const ONE_WEEK_MS = 7 * 24 * 60 * 60 * 1000; +const PROMOTED_ROLE = 'Promoted Reviewer'; + +/** + * `Team` and `HgnFormResponses` are optional and only used by the promotion + * placement handlers. Leaving them off keeps every existing caller, and the + * existing tests, working exactly as before. + */ const promotionEligibilityController = function ( UserProfile, TimeEntry, Task, PromotionEligibility, ReviewerGroup, + Team, + HgnFormResponses, ) { /** * How many PRs a reviewer reviewed in each week they logged review work. @@ -97,12 +108,24 @@ const promotionEligibilityController = function ( return res.status(400).send(`No reviewer group with key: ${req.body.groupKey}`); } + // Promoted reviewers leave the lettered groups but stay under All + // Members, per the spec: "Keep them in the All Members filter in case we + // ever want to see how they did with training". + // + // Only an explicit `groupKey: "all"` brings them back. Omitting the key + // keeps the exact behaviour the current page already has, so nothing + // that exists today changes until the frontend opts in. + const excludedRoles = + req.body.groupKey === 'all' + ? ['Owner', 'Administrator'] + : ['Owner', 'Administrator', PROMOTED_ROLE]; + const users = await UserProfile.find( { isActive: true, - role: { $nin: ['Owner', 'Administrator', 'Promoted Reviewer'] }, + role: { $nin: excludedRoles }, }, - '_id firstName lastName weeklycommittedHours createdDate', + '_id firstName lastName weeklycommittedHours createdDate role', ).lean(); // Group membership is derived from the reviewer's name rather than stored, @@ -167,6 +190,9 @@ const promotionEligibilityController = function ( // period", meaning this week. It is no longer a synonym for being // eligible to promote, which is `remainingWeeks === 0`. weeklyRequirementsMet, + // Only ever true under All Members, since promoted reviewers are + // filtered out of every other view. The role is the source of truth. + isPromoted: user.role === PROMOTED_ROLE, calculatedAt: now, }; @@ -253,17 +279,180 @@ const promotionEligibilityController = function ( } }; - const promoteMembers = async (req, res) => { + /** + * Work out where a set of reviewers would be placed, without writing anything. + * + * This backs the spec's confirmation modal, which has to show the proposed + * team for each person and let it be changed by hand before anything + * happens. Preview and commit deliberately share `buildPlacements`, so what + * the modal shows is what the commit does. + */ + const buildPlacements = async (memberIds) => { + const objectIds = memberIds.map((id) => mongoose.Types.ObjectId(id)); + + const users = await UserProfile.find( + { _id: { $in: objectIds } }, + '_id firstName lastName weeklycommittedHours', + ).lean(); + + // Only configured teams can receive anyone, so the query is filtered to + // those rather than pulling all 1000+ teams into memory. + const teams = Team + ? ( + await Team.find( + { + isActive: true, + hoursBand: { $ne: null }, + standupDay: { $ne: null }, + standupTime: { $ne: null }, + }, + '_id teamName hoursBand standupDay standupTime standupTimezone members', + ).lean() + ).filter(isPlaceableTeam) + : []; + + const formsByUserId = new Map(); + if (HgnFormResponses) { + const forms = await HgnFormResponses.find( + { user_id: { $in: memberIds } }, + 'user_id general.availability', + ).lean(); + forms.forEach((form) => formsByUserId.set(String(form.user_id), form)); + } + + const usersById = new Map(users.map((user) => [String(user._id), user])); + + return memberIds.map((id) => { + const user = usersById.get(String(id)); + if (!user) { + return { + reviewerId: id, + reviewerName: null, + reason: 'reviewerNotFound', + needsReview: true, + }; + } + + const outcome = placeReviewer({ + committedHours: user.weeklycommittedHours, + formResponse: formsByUserId.get(String(id)) || null, + teams, + }); + + return { + reviewerId: String(user._id), + reviewerName: `${user.firstName} ${user.lastName}`, + committedHours: user.weeklycommittedHours || 0, + band: outcome.band, + teamId: outcome.team ? String(outcome.team._id) : null, + teamName: outcome.team ? outcome.team.teamName : null, + standupDay: outcome.team ? outcome.team.standupDay : null, + standupTime: outcome.team ? outcome.team.standupTime : null, + reason: outcome.reason, + needsReview: outcome.needsReview, + }; + }); + }; + + const previewPromotions = async (req, res) => { if (!(await hasPermission(req.body.requestor, 'putUserProfile'))) { return res.status(403).send('You are not authorized to promote members.'); } const { memberIds } = req.body; + if (!Array.isArray(memberIds) || memberIds.length === 0) { + return res.status(400).send('No member IDs provided for promotion.'); + } + + const invalid = memberIds.filter((id) => !mongoose.Types.ObjectId.isValid(id)); + if (invalid.length) { + return res.status(400).send(`Invalid member ID: ${invalid[0]}`); + } + + try { + const placements = await buildPlacements(memberIds); + + // Surfaced separately from the rows so the modal can lead with what a + // human has to look at rather than making them scan every line. + const warnings = []; + if (!Team) { + warnings.push('Team placement is unavailable, so no team will be assigned.'); + } + const unplaced = placements.filter((p) => !p.teamId).length; + if (unplaced) { + warnings.push(`${unplaced} of ${placements.length} could not be placed on a team.`); + } + const guessed = placements.filter((p) => p.teamId && p.needsReview).length; + if (guessed) { + warnings.push(`${guessed} placed without matching availability, please check.`); + } + + return res.status(200).json({ placements, warnings }); + } catch (error) { + logger.logException(error, { endpoint: 'previewPromotions', payload: req.body }); + return res.status(500).send('Error previewing promotions.'); + } + }; + + const promoteMembers = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'putUserProfile'))) { + return res.status(403).send('You are not authorized to promote members.'); + } + + const { memberIds, placements } = req.body; if (!Array.isArray(memberIds) || memberIds.length === 0) { return res.status(400).send('No member IDs provided for promotion.'); } + // Optional. Omitting it keeps the original behaviour, a role change with + // no team assignment, so the existing page keeps working untouched. When + // present these are the rows the confirmation modal showed, including any + // the user changed by hand, which is why they are trusted over + // recalculating: the whole point of the modal is that a human can override. + const placementsByReviewerId = new Map(); + if (placements !== undefined) { + if (!Array.isArray(placements)) { + return res.status(400).send('placements must be an array when provided.'); + } + if (!Team) { + return res.status(400).send('Team placement is unavailable on this deployment.'); + } + + const invalidPlacement = placements.find( + (entry) => + !entry || + !mongoose.Types.ObjectId.isValid(entry.reviewerId) || + (entry.teamId !== null && + entry.teamId !== undefined && + !mongoose.Types.ObjectId.isValid(entry.teamId)), + ); + if (invalidPlacement) { + return res.status(400).send('Each placement needs a valid reviewerId and teamId or null.'); + } + + const unknown = placements.find( + (entry) => !memberIds.some((id) => String(id) === String(entry.reviewerId)), + ); + if (unknown) { + return res + .status(400) + .send(`Placement for ${unknown.reviewerId}, who is not in memberIds.`); + } + + const teamIds = [...new Set(placements.map((e) => e.teamId).filter(Boolean))]; + if (teamIds.length) { + const found = await Team.countDocuments({ _id: { $in: teamIds } }); + if (found !== teamIds.length) { + return res.status(400).send('One or more placement teams do not exist.'); + } + } + + placements.forEach((entry) => { + if (entry.teamId) placementsByReviewerId.set(String(entry.reviewerId), entry.teamId); + }); + } + let session = null; try { @@ -281,15 +470,61 @@ const promotionEligibilityController = function ( } const user = await UserProfile.findById(memberId).session(session); if (user) { - user.role = 'Promoted Reviewer'; + user.role = PROMOTED_ROLE; + + // Team assignment, only when the caller sent one. Without + // `placements` this behaves exactly as it did before: role change + // only, no team touched. + const teamId = placementsByReviewerId.get(String(memberId)); + if (teamId) { + if (!(user.teams || []).some((existing) => String(existing) === String(teamId))) { + user.teams = [...(user.teams || []), mongoose.Types.ObjectId(teamId)]; + } + } + await user.save({ session }); - promotedMembers.push({ id: memberId, name: `${user.firstName} ${user.lastName}` }); + promotedMembers.push({ + id: memberId, + name: `${user.firstName} ${user.lastName}`, + teamId: teamId || null, + }); + + if (teamId) { + // Mirrors assignTeamToUsers in teamController: membership lives on + // both sides, so writing only one of them leaves the team looking + // empty on the Teams page. + const alreadyMember = await Team.exists({ + _id: teamId, + 'members.userId': mongoose.Types.ObjectId(memberId), + }); + if (!alreadyMember) { + await Team.findByIdAndUpdate( + teamId, + { + $push: { members: { userId: memberId, visible: true, addDateTime: new Date() } }, + $set: { modifiedDatetime: Date.now() }, + }, + { session }, + ); + } + } await PromotionEligibility.findOneAndUpdate( { reviewerId: memberId }, - { $set: { isPromoted: true, promotionDate: new Date() } }, + { + $set: { + isPromoted: true, + promotionDate: new Date(), + ...(teamId ? { assignedTeamId: teamId } : {}), + }, + }, { new: true, session }, ); + + // The profile's role and teams both changed, so a cached copy is now + // wrong. The role change had this problem before team assignment was + // added, it is just more visible now. + if (cache.hasCache(`user-${memberId}`)) cache.removeCache(`user-${memberId}`); } else { logger.logInfo(`Attempted to promote non-existent user with ID: ${memberId}`); } @@ -316,7 +551,7 @@ const promotionEligibilityController = function ( } }; - return { getPromotionEligibilityData, updatePrsNeeded, promoteMembers }; + return { getPromotionEligibilityData, updatePrsNeeded, previewPromotions, promoteMembers }; }; module.exports = promotionEligibilityController; diff --git a/src/controllers/promotionEligibilityController.test.js b/src/controllers/promotionEligibilityController.test.js index bd341ffba9..7af84fce22 100644 --- a/src/controllers/promotionEligibilityController.test.js +++ b/src/controllers/promotionEligibilityController.test.js @@ -514,6 +514,347 @@ describe('getPromotionEligibilityData, weekly requirements and remaining weeks', }); }); +describe('previewPromotions', () => { + const ID_A = '637af0c0fb9bbc1e308cff01'; + const ID_B = '637af0c0fb9bbc1e308cff02'; + const TEAM_SMALL = '637af0c0fb9bbc1e308cfa01'; + const TEAM_BIG = '637af0c0fb9bbc1e308cfa02'; + + const USERS = [ + { _id: ID_A, firstName: 'Ann', lastName: 'Adams', weeklycommittedHours: 12 }, + { _id: ID_B, firstName: 'Bob', lastName: 'Brown', weeklycommittedHours: 25 }, + ]; + + const TEAMS = [ + { + _id: TEAM_SMALL, + teamName: 'Small Ten', + hoursBand: '10-19.99', + standupDay: 'Tuesday', + standupTime: '10:30 AM', + members: [], + }, + { + _id: TEAM_BIG, + teamName: 'Big Twenty', + hoursBand: '20+', + standupDay: 'Friday', + standupTime: '3PM', + members: [{ userId: 'x' }, { userId: 'y' }], + }, + ]; + + let UserProfile; + let Team; + let HgnFormResponses; + let controller; + let mockRes; + + const request = (body = {}) => ({ + body: { requestor: { requestorId: OWNER_ID, role: 'Administrator' }, ...body }, + }); + + const build = () => + promotionEligibilityController( + UserProfile, + { aggregate: jest.fn().mockResolvedValue([]) }, + { countDocuments: jest.fn().mockResolvedValue(0) }, + { find: jest.fn(() => ({ lean: () => Promise.resolve([]) })), findOneAndUpdate: jest.fn() }, + null, + Team, + HgnFormResponses, + ); + + beforeEach(() => { + jest.clearAllMocks(); + hasPermission.mockResolvedValue(true); + + UserProfile = { find: jest.fn(() => ({ lean: () => Promise.resolve(USERS) })) }; + Team = { find: jest.fn(() => ({ lean: () => Promise.resolve(TEAMS) })) }; + HgnFormResponses = { find: jest.fn(() => ({ lean: () => Promise.resolve([]) })) }; + controller = build(); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + const body = () => mockRes.json.mock.calls[0][0]; + + it('refuses a requestor without the promote permission', async () => { + hasPermission.mockResolvedValue(false); + + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(UserProfile.find).not.toHaveBeenCalled(); + }); + + it('400s on an empty or missing member list', async () => { + await controller.previewPromotions(request({ memberIds: [] }), mockRes); + expect(mockRes.status).toHaveBeenCalledWith(400); + + await controller.previewPromotions(request({}), mockRes); + expect(mockRes.status).toHaveBeenCalledWith(400); + }); + + it('400s on a malformed member id rather than querying', async () => { + await controller.previewPromotions(request({ memberIds: ['not-an-id'] }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(UserProfile.find).not.toHaveBeenCalled(); + }); + + it('places each reviewer into a team matching their hours band', async () => { + await controller.previewPromotions(request({ memberIds: [ID_A, ID_B] }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + const { placements } = body(); + expect(placements).toHaveLength(2); + expect(placements[0]).toMatchObject({ + reviewerId: ID_A, + band: '10-19.99', + teamName: 'Small Ten', + }); + expect(placements[1]).toMatchObject({ reviewerId: ID_B, band: '20+', teamName: 'Big Twenty' }); + }); + + it('writes nothing at all', async () => { + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + // The only Team and UserProfile methods the mocks expose are reads, so a + // write would have thrown rather than passed silently. + expect(Team.find).toHaveBeenCalled(); + expect(Team.findByIdAndUpdate).toBeUndefined(); + expect(UserProfile.findByIdAndUpdate).toBeUndefined(); + }); + + it('queries only configured teams, so the 1000+ legacy teams are never loaded', async () => { + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + const query = Team.find.mock.calls[0][0]; + expect(query.isActive).toBe(true); + expect(query.hoursBand).toEqual({ $ne: null }); + expect(query.standupDay).toEqual({ $ne: null }); + expect(query.standupTime).toEqual({ $ne: null }); + }); + + it('flags a reviewer with no availability on file', async () => { + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + const { placements, warnings } = body(); + expect(placements[0].reason).toBe('noAvailabilityOnFile'); + expect(placements[0].needsReview).toBe(true); + expect(warnings.join(' ')).toContain('without matching availability'); + }); + + it('uses availability when it is on file', async () => { + HgnFormResponses.find = jest.fn(() => ({ + lean: () => + Promise.resolve([{ user_id: ID_A, general: { availability: { Tuesday: '10AM-11AM' } } }]), + })); + controller = build(); + + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + expect(body().placements[0]).toMatchObject({ + reason: 'availabilityMatch', + needsReview: false, + teamName: 'Small Ten', + }); + }); + + it('reports a reviewer it cannot place instead of dropping them', async () => { + UserProfile.find = jest.fn(() => ({ + lean: () => Promise.resolve([{ ...USERS[0], weeklycommittedHours: 4 }]), + })); + controller = build(); + + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + const { placements, warnings } = body(); + expect(placements[0]).toMatchObject({ teamId: null, reason: 'committedHoursOutOfBands' }); + expect(warnings.join(' ')).toContain('could not be placed'); + }); + + it('reports an id that matches no profile rather than throwing', async () => { + UserProfile.find = jest.fn(() => ({ lean: () => Promise.resolve([]) })); + controller = build(); + + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + expect(body().placements[0].reason).toBe('reviewerNotFound'); + }); + + it('warns when no Team model is wired in, instead of pretending it placed people', async () => { + controller = promotionEligibilityController( + UserProfile, + { aggregate: jest.fn() }, + { countDocuments: jest.fn() }, + { find: jest.fn(), findOneAndUpdate: jest.fn() }, + null, + undefined, + undefined, + ); + + await controller.previewPromotions(request({ memberIds: [ID_A] }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(body().warnings.join(' ')).toContain('Team placement is unavailable'); + expect(body().placements[0].teamId).toBeNull(); + }); +}); + +describe('promoteMembers, placement is opt-in', () => { + const ID_A = '637af0c0fb9bbc1e308cff01'; + const TEAM = '637af0c0fb9bbc1e308cfa01'; + + let UserProfile; + let Team; + let PromotionEligibility; + let controller; + let mockRes; + let savedUser; + + const request = (body = {}) => ({ + body: { requestor: { requestorId: OWNER_ID, role: 'Administrator' }, ...body }, + }); + + beforeEach(() => { + jest.clearAllMocks(); + hasPermission.mockResolvedValue(true); + + // promoteMembers wraps its writes in a transaction. There is no database + // here, so the session is stubbed rather than the transaction skipped, + // which keeps the code under test on its real path. + jest.spyOn(mongoose, 'startSession').mockResolvedValue({ + startTransaction: jest.fn(), + commitTransaction: jest.fn().mockResolvedValue(true), + abortTransaction: jest.fn().mockResolvedValue(true), + endSession: jest.fn(), + }); + + savedUser = { + _id: ID_A, + firstName: 'Ann', + lastName: 'Adams', + role: 'Volunteer', + teams: [], + save: jest.fn().mockResolvedValue(true), + }; + UserProfile = { findById: jest.fn(() => ({ session: () => Promise.resolve(savedUser) })) }; + Team = { + exists: jest.fn().mockResolvedValue(false), + countDocuments: jest.fn().mockResolvedValue(1), + findByIdAndUpdate: jest.fn().mockResolvedValue({}), + }; + PromotionEligibility = { findOneAndUpdate: jest.fn().mockResolvedValue({}) }; + + controller = promotionEligibilityController( + UserProfile, + {}, + {}, + PromotionEligibility, + null, + Team, + null, + ); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + it('without placements it behaves exactly as before: role change, no team touched', async () => { + await controller.promoteMembers(request({ memberIds: [ID_A] }), mockRes); + + expect(savedUser.role).toBe('Promoted Reviewer'); + expect(savedUser.teams).toEqual([]); + expect(Team.findByIdAndUpdate).not.toHaveBeenCalled(); + expect(mockRes.status).toHaveBeenCalledWith(200); + }); + + it('with placements it assigns the team on both sides of the relationship', async () => { + await controller.promoteMembers( + request({ memberIds: [ID_A], placements: [{ reviewerId: ID_A, teamId: TEAM }] }), + mockRes, + ); + + expect(savedUser.role).toBe('Promoted Reviewer'); + expect(savedUser.teams.map(String)).toEqual([TEAM]); + expect(Team.findByIdAndUpdate).toHaveBeenCalledTimes(1); + expect(Team.findByIdAndUpdate.mock.calls[0][0]).toBe(TEAM); + }); + + it('does not double-add somebody who is already on the team', async () => { + Team.exists = jest.fn().mockResolvedValue(true); + + await controller.promoteMembers( + request({ memberIds: [ID_A], placements: [{ reviewerId: ID_A, teamId: TEAM }] }), + mockRes, + ); + + expect(Team.findByIdAndUpdate).not.toHaveBeenCalled(); + }); + + it('a null teamId promotes without placing, which is the modal opting out', async () => { + await controller.promoteMembers( + request({ memberIds: [ID_A], placements: [{ reviewerId: ID_A, teamId: null }] }), + mockRes, + ); + + expect(savedUser.role).toBe('Promoted Reviewer'); + expect(Team.findByIdAndUpdate).not.toHaveBeenCalled(); + }); + + it('400s on a placement for somebody not in memberIds', async () => { + await controller.promoteMembers( + request({ + memberIds: [ID_A], + placements: [{ reviewerId: '637af0c0fb9bbc1e308cff99', teamId: TEAM }], + }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(savedUser.save).not.toHaveBeenCalled(); + }); + + it('400s on a malformed placement', async () => { + await controller.promoteMembers( + request({ memberIds: [ID_A], placements: [{ reviewerId: ID_A, teamId: 'nope' }] }), + mockRes, + ); + expect(mockRes.status).toHaveBeenCalledWith(400); + + await controller.promoteMembers( + request({ memberIds: [ID_A], placements: 'not-an-array' }), + mockRes, + ); + expect(mockRes.status).toHaveBeenCalledWith(400); + }); + + it('400s when a placement names a team that does not exist', async () => { + Team.countDocuments = jest.fn().mockResolvedValue(0); + + await controller.promoteMembers( + request({ memberIds: [ID_A], placements: [{ reviewerId: ID_A, teamId: TEAM }] }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(savedUser.save).not.toHaveBeenCalled(); + }); + + afterEach(() => { + mongoose.startSession.mockRestore(); + }); +}); + describe('mongoose ObjectId validation assumption', () => { it('treats the ids used above as valid, so the 400 tests fail for the right reason', () => { expect(mongoose.Types.ObjectId.isValid(REVIEWER_ID)).toBe(true); diff --git a/src/controllers/teamController.js b/src/controllers/teamController.js index abf7efacec..aed0c0a7c5 100644 --- a/src/controllers/teamController.js +++ b/src/controllers/teamController.js @@ -6,9 +6,62 @@ const { hasPermission } = require('../utilities/permissions'); const cache = require('../utilities/nodeCache')(); const Logger = require('../startup/logger'); const helper = require('../utilities/permissions'); +const placement = require('../helpers/teamPlacementHelper'); const INTERNAL_SERVER_ERROR = 'Internal server error'; +/** + * Validate the optional placement fields used by the Promotion Eligibility + * dashboard (doc item #23), and return only the ones the caller actually sent. + * + * Absent means "leave alone", which matters because putTeam assigns fields + * unconditionally: reading these the same way would let any existing client + * that does not know about them wipe them on every save. Explicit null is the + * way to clear one, and that takes the team out of placement. + * + * @returns {{error: string|null, updates: object}} + */ +const readPlacementFields = function (body) { + const updates = {}; + + if ('hoursBand' in body) { + const value = body.hoursBand; + if (value !== null && !placement.HOURS_BANDS.includes(value)) { + return { + error: `hoursBand must be one of ${placement.HOURS_BANDS.join(', ')}, or null`, + updates, + }; + } + updates.hoursBand = value; + } + + if ('standupDay' in body) { + const value = body.standupDay; + if (value !== null && !placement.DAYS.includes(value)) { + return { error: `standupDay must be a full weekday name, or null`, updates }; + } + updates.standupDay = value; + } + + if ('standupTime' in body) { + const value = body.standupTime; + if (value !== null && placement.toMinutes(value) === null) { + return { error: 'standupTime must look like "11AM", "9:30 PM" or "14:00", or null', updates }; + } + updates.standupTime = value; + } + + if ('standupTimezone' in body) { + const value = body.standupTimezone; + if (value !== null && (typeof value !== 'string' || !value.trim())) { + return { error: 'standupTimezone must be a non-empty string, or null', updates }; + } + updates.standupTimezone = value; + } + + return { error: null, updates }; +}; + const teamcontroller = function (Team) { const getAllTeams = function (req, res) { Team.aggregate([ @@ -89,6 +142,14 @@ const teamcontroller = function (Team) { return; } + // Validated before the duplicate-name lookup, so a malformed request is + // rejected without a database round trip. + const { error: placementError, updates: placementUpdates } = readPlacementFields(req.body); + if (placementError) { + res.status(400).send({ error: placementError }); + return; + } + if (await Team.exists({ teamName: req.body.teamName })) { res.status(403).send({ error: `Team Name "${req.body.teamName}" already exists` }); return; @@ -99,6 +160,7 @@ const teamcontroller = function (Team) { newTeam.isActive = req.body.isActive; newTeam.createdDatetime = Date.now(); newTeam.modifiedDatetime = Date.now(); + Object.assign(newTeam, placementUpdates); try { const result = await newTeam.save(); @@ -144,6 +206,12 @@ const teamcontroller = function (Team) { const { teamId } = req.params; + const { error: placementError, updates: placementUpdates } = readPlacementFields(req.body); + if (placementError) { + res.status(400).send({ error: placementError }); + return; + } + Team.findById(teamId, async (error, record) => { if (error || record === null) { res.status(400).send('No valid records found'); @@ -159,6 +227,9 @@ const teamcontroller = function (Team) { record.teamCode = newTeamCode; record.createdDatetime = Date.now(); record.modifiedDatetime = Date.now(); + // Only the placement fields the caller actually sent, so a client that + // knows nothing about them cannot clear them by omission. + Object.assign(record, placementUpdates); try { const savedTeam = await record.save(); diff --git a/src/controllers/teamController.placement.spec.js b/src/controllers/teamController.placement.spec.js new file mode 100644 index 0000000000..cce39931cc --- /dev/null +++ b/src/controllers/teamController.placement.spec.js @@ -0,0 +1,231 @@ +/** + * Placement fields on the team model (doc item #23). + * + * A separate file from teamController.spec.js on purpose. That suite mocks + * permissions with jest.spyOn, which cannot reach putTeam: putTeam calls the + * `hasPermission` it destructured at import time, not `helper.hasPermission`, + * so a spy on the module object never applies. That is also why putTeam has no + * coverage there. Mocking the whole module here is what makes it testable. + */ + +jest.mock('../utilities/permissions', () => ({ + hasPermission: jest.fn(), +})); + +jest.mock('../startup/logger', () => ({ + logInfo: jest.fn(), + logException: jest.fn(), +})); + +const { hasPermission } = require('../utilities/permissions'); +const teamController = require('./teamController'); + +const TEAM_ID = '637af0c0fb9bbc1e308cfa01'; + +describe('team placement fields', () => { + let Team; + let controller; + let mockRes; + let record; + + const flushPromises = () => new Promise(setImmediate); + + const request = (body) => ({ + params: { teamId: TEAM_ID }, + body: { requestor: { requestorId: '665234c757ca141fe891e1ca', role: 'Owner' }, ...body }, + }); + + /** putTeam works through a findById callback, so the record is captured. */ + const givenExistingTeam = (existing = {}) => { + record = { teamCode: '', save: jest.fn().mockResolvedValue(true), ...existing }; + Team.findById = jest.fn((id, cb) => cb(null, record)); + return record; + }; + + beforeEach(() => { + jest.clearAllMocks(); + hasPermission.mockResolvedValue(true); + + Team = { + exists: jest.fn().mockResolvedValue(false), + findById: jest.fn(), + }; + controller = teamController(Team); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + describe('putTeam', () => { + it('does NOT clear the placement fields when the caller omits them', async () => { + // The regression this exists to catch. putTeam assigns every field it + // knows about unconditionally, so reading the new ones the same way + // would let the existing Teams page wipe a team's standup on every + // ordinary rename. + givenExistingTeam({ hoursBand: '20+', standupDay: 'Friday', standupTime: '3PM' }); + + await controller.putTeam(request({ teamName: 'Renamed', isActive: true }), mockRes); + await flushPromises(); + + expect(record.teamName).toBe('Renamed'); + expect(record.hoursBand).toBe('20+'); + expect(record.standupDay).toBe('Friday'); + expect(record.standupTime).toBe('3PM'); + }); + + it('sets them when they are supplied', async () => { + givenExistingTeam(); + + await controller.putTeam( + request({ + teamName: 'T', + isActive: true, + hoursBand: '10-19.99', + standupDay: 'Tuesday', + standupTime: '11AM', + }), + mockRes, + ); + await flushPromises(); + + expect(record.hoursBand).toBe('10-19.99'); + expect(record.standupDay).toBe('Tuesday'); + expect(record.standupTime).toBe('11AM'); + }); + + it('an explicit null clears one, which takes the team out of placement', async () => { + givenExistingTeam({ hoursBand: '20+', standupDay: 'Friday' }); + + await controller.putTeam( + request({ teamName: 'T', isActive: true, hoursBand: null }), + mockRes, + ); + await flushPromises(); + + expect(record.hoursBand).toBeNull(); + expect(record.standupDay).toBe('Friday'); + }); + + it('rejects a bad hours band without reading the record', async () => { + await controller.putTeam( + request({ teamName: 'T', isActive: true, hoursBand: '30+' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(Team.findById).not.toHaveBeenCalled(); + }); + + it('rejects an abbreviated weekday', async () => { + await controller.putTeam( + request({ teamName: 'T', isActive: true, standupDay: 'Tues' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(Team.findById).not.toHaveBeenCalled(); + }); + + it('rejects an unreadable standup time', async () => { + await controller.putTeam( + request({ teamName: 'T', isActive: true, standupTime: 'lunchtime' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(Team.findById).not.toHaveBeenCalled(); + }); + + it('accepts the time formats the standup is likely to be entered in', async () => { + const accepted = ['11AM', '9:30 AM', '3PM', '14:00', '12PM']; + + // eslint-disable-next-line no-restricted-syntax + for (const standupTime of accepted) { + jest.clearAllMocks(); + hasPermission.mockResolvedValue(true); + givenExistingTeam(); + + // eslint-disable-next-line no-await-in-loop + await controller.putTeam(request({ teamName: 'T', isActive: true, standupTime }), mockRes); + // eslint-disable-next-line no-await-in-loop + await flushPromises(); + + expect(record.standupTime).toBe(standupTime); + expect(mockRes.status).not.toHaveBeenCalledWith(400); + } + }); + + it('still refuses a requestor without putTeam permission', async () => { + hasPermission.mockResolvedValue(false); + + await controller.putTeam( + request({ teamName: 'T', isActive: true, hoursBand: '20+' }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(403); + }); + }); + + describe('postTeam', () => { + it('creates a team carrying the placement fields', async () => { + const saved = []; + function FakeTeam() { + this.save = jest.fn().mockImplementation(() => { + saved.push(this); + return Promise.resolve(this); + }); + } + FakeTeam.exists = jest.fn().mockResolvedValue(false); + controller = teamController(FakeTeam); + + await controller.postTeam( + request({ + teamName: 'New', + isActive: true, + hoursBand: '20+', + standupDay: 'Friday', + standupTime: '3PM', + }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(saved[0]).toMatchObject({ + teamName: 'New', + hoursBand: '20+', + standupDay: 'Friday', + standupTime: '3PM', + }); + }); + + it('rejects a bad placement field on create before checking for a duplicate name', async () => { + await controller.postTeam(request({ teamName: 'New', hoursBand: 'nonsense' }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(Team.exists).not.toHaveBeenCalled(); + }); + + it('creates a team with no placement fields, exactly as before', async () => { + const saved = []; + function FakeTeam() { + this.save = jest.fn().mockImplementation(() => { + saved.push(this); + return Promise.resolve(this); + }); + } + FakeTeam.exists = jest.fn().mockResolvedValue(false); + controller = teamController(FakeTeam); + + await controller.postTeam(request({ teamName: 'Plain', isActive: true }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(saved[0].teamName).toBe('Plain'); + expect(saved[0].hoursBand).toBeUndefined(); + expect(saved[0].standupDay).toBeUndefined(); + }); + }); +}); diff --git a/src/helpers/teamPlacementHelper.js b/src/helpers/teamPlacementHelper.js new file mode 100644 index 0000000000..d5318d2eda --- /dev/null +++ b/src/helpers/teamPlacementHelper.js @@ -0,0 +1,252 @@ +/** + * Team placement for the Promotion Eligibility dashboard (doc item #23). + * + * The spec places a newly promoted reviewer onto a team using, in order: + * + * 1. Hours band, which is REQUIRED. A 10-19.99 hr/wk person is only ever + * eligible for a 10-hour team, a 20+ person only for a 20+ team. + * 2. Availability against the team's weekly standup: + * - more than one team matches -> the team with the fewest people + * - no team matches -> a team whose standup is within 2 + * hours of their availability, else the + * smallest team in the band + * + * None of that information existed anywhere before this change. It is not on + * the team model, and it is not encoded in team names either: of 1046 active + * teams on dev, one has a weekday in its name and none has a time or an hours + * band. So teams now carry the two facts explicitly, and a team missing either + * one is simply not a placement candidate. That is what keeps this from + * needing a backfill across a thousand mostly-disposable teams. + * + * Everything here is pure, so the placement rules are testable without a + * database. + */ + +const HOURS_BANDS = ['10-19.99', '20+']; + +const DAYS = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday']; + +/** How far a standup may sit from someone's availability and still be offered. */ +const NEARBY_HOURS = 2; + +/** + * The hours band a reviewer belongs to, from their committed hours. + * + * The spec's bands are "10-19.99" and "20+", which leaves people under 10 + * hr/wk unplaceable. That is not hypothetical, dev has 46 such accounts, so + * they return null and get reported rather than being quietly dropped into the + * low band. Same open question as the PRs Needed clamping (question 3 to Jae). + * + * @param {number} committedHours userProfile.weeklycommittedHours + * @returns {string|null} a member of HOURS_BANDS, or null when unplaceable + */ +function bandForCommittedHours(committedHours) { + const hours = Number(committedHours); + if (!Number.isFinite(hours)) return null; + if (hours >= 20) return '20+'; + if (hours >= 10) return '10-19.99'; + return null; +} + +/** + * Minutes past midnight for a "9AM", "10:30 PM" or "14:00" style time. + * + * The questionnaire stores availability as strings like "10AM-11AM", and the + * team standup time is typed in by hand, so both sides arrive as free text. + * + * @returns {number|null} minutes since midnight, or null if unparseable + */ +function toMinutes(value) { + if (typeof value !== 'string') return null; + + const match = value.trim().match(/^(\d{1,2})(?::(\d{2}))?\s*(AM|PM)?$/i); + if (!match) return null; + + let hour = Number(match[1]); + const minute = match[2] ? Number(match[2]) : 0; + const meridiem = match[3] ? match[3].toUpperCase() : null; + + if (minute > 59) return null; + + if (meridiem) { + if (hour < 1 || hour > 12) return null; + if (meridiem === 'AM') hour = hour === 12 ? 0 : hour; + else hour = hour === 12 ? 12 : hour + 12; + } else if (hour > 23) { + return null; + } + + return hour * 60 + minute; +} + +/** + * Parse one questionnaire availability entry, for example "10AM-11AM". + * + * A single time with no range ("10AM") is read as a one hour window, since a + * few responses on dev are stored that way. + * + * @returns {{start: number, end: number}|null} minutes since midnight + */ +function parseAvailabilityWindow(value) { + if (typeof value !== 'string' || !value.trim()) return null; + + const [rawStart, rawEnd] = value.split('-'); + const start = toMinutes(rawStart); + if (start === null) return null; + + const end = rawEnd === undefined ? start + 60 : toMinutes(rawEnd); + if (end === null || end < start) return null; + + return { start, end }; +} + +/** + * A reviewer's availability keyed by day, from the setup questionnaire. + * + * The real data lives at `general.availability` with all seven days. The older + * root level shape is read too, because one of the two competing form models + * declares it that way and accepting both costs nothing. + * + * @param {object|null} formResponse an hgnformresponses document + * @returns {Map} day to window + */ +function availabilityByDay(formResponse) { + const windows = new Map(); + if (!formResponse) return windows; + + const raw = + (formResponse.general && formResponse.general.availability) || formResponse.availability; + if (!raw || typeof raw !== 'object') return windows; + + DAYS.forEach((day) => { + const parsed = parseAvailabilityWindow(raw[day]); + if (parsed) windows.set(day, parsed); + }); + + return windows; +} + +/** Whether a team carries enough placement data to be a candidate at all. */ +function isPlaceableTeam(team) { + return Boolean( + team && + HOURS_BANDS.includes(team.hoursBand) && + DAYS.includes(team.standupDay) && + toMinutes(team.standupTime) !== null, + ); +} + +const memberCount = (team) => (Array.isArray(team.members) ? team.members.length : 0); + +/** + * Pick the team with the fewest people, breaking ties on name so the same + * inputs always give the same placement. A stable answer matters here: the + * preview somebody confirms has to be the placement that is committed. + */ +function smallestTeam(teams) { + if (!teams.length) return null; + + return teams.reduce((best, team) => { + const delta = memberCount(team) - memberCount(best); + if (delta !== 0) return delta < 0 ? team : best; + return String(team.teamName) < String(best.teamName) ? team : best; + }); +} + +/** + * Gap in minutes between a standup and a reviewer's availability that day. + * + * Zero when the standup starts inside the window they gave, otherwise the + * distance to the nearer edge. Returns null when they gave no availability for + * that day at all, which is a different thing from being far away. + */ +function standupGapMinutes(team, windows) { + const window = windows.get(team.standupDay); + if (!window) return null; + + const standup = toMinutes(team.standupTime); + if (standup === null) return null; + + if (standup >= window.start && standup <= window.end) return 0; + return standup < window.start ? window.start - standup : standup - window.end; +} + +/** + * Work out which team a reviewer should be placed on. + * + * @param {object} params + * @param {number} params.committedHours the reviewer's weeklycommittedHours + * @param {object|null} params.formResponse their questionnaire document, if any + * @param {Array} params.teams candidate teams, each carrying hoursBand, + * standupDay, standupTime and members + * @returns {{team: object|null, band: string|null, reason: string, needsReview: boolean}} + */ +function placeReviewer({ committedHours, formResponse, teams }) { + const band = bandForCommittedHours(committedHours); + if (!band) { + return { team: null, band: null, reason: 'committedHoursOutOfBands', needsReview: true }; + } + + const candidates = (teams || []).filter( + (team) => isPlaceableTeam(team) && team.hoursBand === band, + ); + if (!candidates.length) { + return { team: null, band, reason: 'noTeamConfiguredForBand', needsReview: true }; + } + + const windows = availabilityByDay(formResponse); + + // No availability on file at all. The spec's fallback chain does not cover + // this, and on dev it is the common case rather than the exception: only 94 + // active profiles out of 2639 have ever answered the questionnaire. Putting + // them on the smallest team in their band is the least surprising thing to + // do, but it is flagged so the confirmation modal can show it as a guess. + if (!windows.size) { + return { + team: smallestTeam(candidates), + band, + reason: 'noAvailabilityOnFile', + needsReview: true, + }; + } + + const exact = candidates.filter((team) => standupGapMinutes(team, windows) === 0); + if (exact.length === 1) { + return { team: exact[0], band, reason: 'availabilityMatch', needsReview: false }; + } + if (exact.length > 1) { + // Spec: "More than 1 - assign to the team with the least people". + return { + team: smallestTeam(exact), + band, + reason: 'availabilityMatchSmallest', + needsReview: false, + }; + } + + // Spec: "Assign to team with availability within 2 hours of their + // availability OR smallest team if no team is within 2 hours". + const nearby = candidates.filter((team) => { + const gap = standupGapMinutes(team, windows); + return gap !== null && gap <= NEARBY_HOURS * 60; + }); + if (nearby.length) { + return { team: smallestTeam(nearby), band, reason: 'withinTwoHours', needsReview: false }; + } + + return { team: smallestTeam(candidates), band, reason: 'smallestInBand', needsReview: true }; +} + +module.exports = { + HOURS_BANDS, + DAYS, + NEARBY_HOURS, + bandForCommittedHours, + toMinutes, + parseAvailabilityWindow, + availabilityByDay, + isPlaceableTeam, + smallestTeam, + standupGapMinutes, + placeReviewer, +}; diff --git a/src/helpers/teamPlacementHelper.spec.js b/src/helpers/teamPlacementHelper.spec.js new file mode 100644 index 0000000000..f8139cd901 --- /dev/null +++ b/src/helpers/teamPlacementHelper.spec.js @@ -0,0 +1,338 @@ +const { + bandForCommittedHours, + toMinutes, + parseAvailabilityWindow, + availabilityByDay, + isPlaceableTeam, + smallestTeam, + standupGapMinutes, + placeReviewer, +} = require('./teamPlacementHelper'); + +/** A fully configured, placeable team. */ +const team = (overrides = {}) => ({ + teamName: 'Team A', + hoursBand: '10-19.99', + standupDay: 'Tuesday', + standupTime: '11AM', + members: [], + ...overrides, +}); + +/** A questionnaire response in the shape the real data uses. */ +const form = (availability) => ({ general: { availability } }); + +const withMembers = (n) => Array.from({ length: n }, (unused, i) => ({ userId: `u${i}` })); + +describe('bandForCommittedHours', () => { + test('20 and above is the 20+ band', () => { + expect(bandForCommittedHours(20)).toBe('20+'); + expect(bandForCommittedHours(40)).toBe('20+'); + expect(bandForCommittedHours(200)).toBe('20+'); + }); + + test('10 up to just under 20 is the 10-19.99 band', () => { + expect(bandForCommittedHours(10)).toBe('10-19.99'); + expect(bandForCommittedHours(19.99)).toBe('10-19.99'); + }); + + test('under 10 is unplaceable rather than dropped into the low band', () => { + expect(bandForCommittedHours(9.99)).toBeNull(); + expect(bandForCommittedHours(0)).toBeNull(); + expect(bandForCommittedHours(-3)).toBeNull(); + }); + + test('non-numeric input is unplaceable', () => { + expect(bandForCommittedHours(undefined)).toBeNull(); + expect(bandForCommittedHours('twenty')).toBeNull(); + expect(bandForCommittedHours(NaN)).toBeNull(); + }); +}); + +describe('toMinutes', () => { + test('reads 12 hour times with a meridiem', () => { + expect(toMinutes('9AM')).toBe(9 * 60); + expect(toMinutes('11AM')).toBe(11 * 60); + expect(toMinutes('1PM')).toBe(13 * 60); + expect(toMinutes('10:30 PM')).toBe(22 * 60 + 30); + }); + + test('handles the two midnight and noon edge cases', () => { + expect(toMinutes('12AM')).toBe(0); + expect(toMinutes('12PM')).toBe(12 * 60); + }); + + test('reads 24 hour times without a meridiem', () => { + expect(toMinutes('14:00')).toBe(14 * 60); + expect(toMinutes('0:15')).toBe(15); + }); + + test('is case and whitespace insensitive', () => { + expect(toMinutes(' 11am ')).toBe(11 * 60); + expect(toMinutes('11 Am')).toBe(11 * 60); + }); + + test('rejects anything it cannot read rather than guessing', () => { + ['', 'lunchtime', '25:00', '13PM', '0AM', '9:99', null, undefined, 11].forEach((value) => { + expect(toMinutes(value)).toBeNull(); + }); + }); +}); + +describe('parseAvailabilityWindow', () => { + test('reads the questionnaire range format', () => { + expect(parseAvailabilityWindow('10AM-11AM')).toEqual({ start: 600, end: 660 }); + }); + + test('treats a bare time as a one hour window', () => { + expect(parseAvailabilityWindow('10AM')).toEqual({ start: 600, end: 660 }); + }); + + test('rejects a backwards or unreadable range', () => { + expect(parseAvailabilityWindow('11AM-10AM')).toBeNull(); + expect(parseAvailabilityWindow('whenever')).toBeNull(); + expect(parseAvailabilityWindow('')).toBeNull(); + expect(parseAvailabilityWindow(null)).toBeNull(); + }); +}); + +describe('availabilityByDay', () => { + test('reads the real nested shape', () => { + const windows = availabilityByDay(form({ Monday: '10AM-11AM', Thursday: '9AM-10AM' })); + expect([...windows.keys()].sort()).toEqual(['Monday', 'Thursday']); + expect(windows.get('Monday')).toEqual({ start: 600, end: 660 }); + }); + + test('also reads the older root level shape', () => { + const windows = availabilityByDay({ availability: { Friday: '2PM-3PM' } }); + expect(windows.get('Friday')).toEqual({ start: 840, end: 900 }); + }); + + test('skips days that are blank or unreadable instead of failing', () => { + const windows = availabilityByDay( + form({ Monday: '', Tuesday: 'sometime', Friday: '9AM-10AM' }), + ); + expect([...windows.keys()]).toEqual(['Friday']); + }); + + test('missing or malformed input gives an empty map', () => { + expect(availabilityByDay(null).size).toBe(0); + expect(availabilityByDay({}).size).toBe(0); + expect(availabilityByDay(form(null)).size).toBe(0); + }); +}); + +describe('isPlaceableTeam', () => { + test('a fully configured team is placeable', () => { + expect(isPlaceableTeam(team())).toBe(true); + }); + + test('a team missing any one field is not a candidate', () => { + expect(isPlaceableTeam(team({ hoursBand: null }))).toBe(false); + expect(isPlaceableTeam(team({ standupDay: null }))).toBe(false); + expect(isPlaceableTeam(team({ standupTime: null }))).toBe(false); + }); + + test('an unconfigured team, which is every team today, is not a candidate', () => { + expect(isPlaceableTeam({ teamName: 'Legacy', members: [] })).toBe(false); + expect(isPlaceableTeam(null)).toBe(false); + }); + + test('junk in a field does not make a team placeable', () => { + expect(isPlaceableTeam(team({ hoursBand: '30+' }))).toBe(false); + expect(isPlaceableTeam(team({ standupDay: 'Tues' }))).toBe(false); + expect(isPlaceableTeam(team({ standupTime: 'lunchtime' }))).toBe(false); + }); +}); + +describe('smallestTeam', () => { + test('picks the fewest members', () => { + const small = team({ teamName: 'Small', members: withMembers(2) }); + const big = team({ teamName: 'Big', members: withMembers(9) }); + expect(smallestTeam([big, small]).teamName).toBe('Small'); + }); + + test('breaks ties on name so the choice is stable', () => { + const a = team({ teamName: 'Alpha', members: withMembers(3) }); + const b = team({ teamName: 'Beta', members: withMembers(3) }); + expect(smallestTeam([b, a]).teamName).toBe('Alpha'); + expect(smallestTeam([a, b]).teamName).toBe('Alpha'); + }); + + test('treats a missing members array as empty', () => { + const none = team({ teamName: 'NoArray', members: undefined }); + const one = team({ teamName: 'AOne', members: withMembers(1) }); + expect(smallestTeam([one, none]).teamName).toBe('NoArray'); + }); + + test('returns null for an empty list', () => { + expect(smallestTeam([])).toBeNull(); + }); +}); + +describe('standupGapMinutes', () => { + const windows = availabilityByDay(form({ Tuesday: '10AM-11AM' })); + + test('is zero when the standup starts inside the window', () => { + expect(standupGapMinutes(team({ standupTime: '10:30 AM' }), windows)).toBe(0); + }); + + test('is zero on both edges of the window', () => { + expect(standupGapMinutes(team({ standupTime: '10AM' }), windows)).toBe(0); + expect(standupGapMinutes(team({ standupTime: '11AM' }), windows)).toBe(0); + }); + + test('measures to the nearer edge when outside', () => { + expect(standupGapMinutes(team({ standupTime: '9AM' }), windows)).toBe(60); + expect(standupGapMinutes(team({ standupTime: '1PM' }), windows)).toBe(120); + }); + + test('is null when they gave no availability that day, which is not the same as far away', () => { + expect(standupGapMinutes(team({ standupDay: 'Friday' }), windows)).toBeNull(); + }); +}); + +describe('placeReviewer', () => { + test('never places somebody into the wrong hours band', () => { + const twentyPlus = team({ teamName: 'Twenty', hoursBand: '20+' }); + + const result = placeReviewer({ + committedHours: 12, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [twentyPlus], + }); + + expect(result.team).toBeNull(); + expect(result.band).toBe('10-19.99'); + expect(result.reason).toBe('noTeamConfiguredForBand'); + expect(result.needsReview).toBe(true); + }); + + test('places into the single team whose standup they are available for', () => { + const match = team({ teamName: 'Match', standupDay: 'Tuesday', standupTime: '10:30 AM' }); + const miss = team({ teamName: 'Miss', standupDay: 'Friday', standupTime: '3PM' }); + + const result = placeReviewer({ + committedHours: 12, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [match, miss], + }); + + expect(result.team.teamName).toBe('Match'); + expect(result.reason).toBe('availabilityMatch'); + expect(result.needsReview).toBe(false); + }); + + test('when several teams match, takes the one with the fewest people', () => { + const big = team({ teamName: 'Big', standupTime: '10:15 AM', members: withMembers(8) }); + const small = team({ teamName: 'Small', standupTime: '10:45 AM', members: withMembers(2) }); + + const result = placeReviewer({ + committedHours: 25, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [ + { ...big, hoursBand: '20+' }, + { ...small, hoursBand: '20+' }, + ], + }); + + expect(result.team.teamName).toBe('Small'); + expect(result.reason).toBe('availabilityMatchSmallest'); + expect(result.needsReview).toBe(false); + }); + + test('falls back to a standup within two hours when nothing matches exactly', () => { + const near = team({ teamName: 'Near', standupTime: '12PM' }); // 1 hour after + const far = team({ teamName: 'Far', standupTime: '5PM' }); + + const result = placeReviewer({ + committedHours: 12, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [near, far], + }); + + expect(result.team.teamName).toBe('Near'); + expect(result.reason).toBe('withinTwoHours'); + expect(result.needsReview).toBe(false); + }); + + test('two hours is inclusive, and beyond it drops to the smallest team', () => { + const justInside = team({ teamName: 'Inside', standupTime: '1PM' }); // exactly 2h after 11AM + let result = placeReviewer({ + committedHours: 12, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [justInside], + }); + expect(result.reason).toBe('withinTwoHours'); + + const justOutside = team({ teamName: 'Outside', standupTime: '1:01 PM' }); + result = placeReviewer({ + committedHours: 12, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [justOutside], + }); + expect(result.team.teamName).toBe('Outside'); + expect(result.reason).toBe('smallestInBand'); + expect(result.needsReview).toBe(true); + }); + + test('somebody with no availability on file is placed but flagged for review', () => { + const small = team({ teamName: 'Small', members: withMembers(1) }); + const big = team({ teamName: 'Big', members: withMembers(7) }); + + const result = placeReviewer({ committedHours: 12, formResponse: null, teams: [big, small] }); + + expect(result.team.teamName).toBe('Small'); + expect(result.reason).toBe('noAvailabilityOnFile'); + expect(result.needsReview).toBe(true); + }); + + test('somebody under 10 hours is not placed at all', () => { + const result = placeReviewer({ + committedHours: 5, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: [team()], + }); + + expect(result.team).toBeNull(); + expect(result.band).toBeNull(); + expect(result.reason).toBe('committedHoursOutOfBands'); + expect(result.needsReview).toBe(true); + }); + + test('unconfigured teams are ignored entirely, so today nobody is placed', () => { + const legacy = [ + { teamName: 'Legacy A', members: [] }, + { teamName: 'Legacy B', members: [] }, + ]; + + const result = placeReviewer({ + committedHours: 12, + formResponse: form({ Tuesday: '10AM-11AM' }), + teams: legacy, + }); + + expect(result.team).toBeNull(); + expect(result.reason).toBe('noTeamConfiguredForBand'); + }); + + test('an empty or missing team list does not throw', () => { + expect(placeReviewer({ committedHours: 12, formResponse: null, teams: [] }).team).toBeNull(); + expect(placeReviewer({ committedHours: 12, formResponse: null }).team).toBeNull(); + }); + + test('availability on a different day than the standup does not count as a match', () => { + const tuesday = team({ teamName: 'Tuesday', standupDay: 'Tuesday', standupTime: '10:30 AM' }); + + const result = placeReviewer({ + committedHours: 12, + formResponse: form({ Wednesday: '10AM-11AM' }), + teams: [tuesday], + }); + + // They have availability, just not on the standup day, so this is the + // smallest-in-band fallback rather than the no-data one. + expect(result.reason).toBe('smallestInBand'); + expect(result.needsReview).toBe(true); + }); +}); diff --git a/src/models/promotionEligibility.js b/src/models/promotionEligibility.js index 1dd6440311..ea307fb270 100644 --- a/src/models/promotionEligibility.js +++ b/src/models/promotionEligibility.js @@ -37,6 +37,12 @@ const promotionEligibilitySchema = new Schema({ prsNeededOverrideBy: { type: Schema.Types.ObjectId, ref: 'userProfiles', default: null }, prsNeededOverrideAt: { type: Date, default: null }, + // Set by Process Promotions. `isPromoted` also comes back on the dashboard + // read, derived from the profile role, which is the source of truth. + isPromoted: { type: Boolean, default: false }, + promotionDate: { type: Date, default: null }, + assignedTeamId: { type: Schema.Types.ObjectId, ref: 'team', default: null }, + // True when committed hours moved since the last calculation, so the page can // surface the change. Always false while an Owner override is in place. committedHoursChanged: { type: Boolean, default: false }, diff --git a/src/models/team.js b/src/models/team.js index 109d93221b..7e58ebdf1d 100644 --- a/src/models/team.js +++ b/src/models/team.js @@ -15,12 +15,36 @@ const team = new Schema({ modifiedDatetime: { type: Date, default: Date.now() }, members: [ { - userId: { type: mongoose.SchemaTypes.ObjectId, required: true, index : true }, + userId: { type: mongoose.SchemaTypes.ObjectId, required: true, index: true }, addDateTime: { type: Date, default: Date.now(), ref: 'userProfile' }, - visible: { type : 'Boolean', default:true}, - + visible: { type: 'Boolean', default: true }, }, ], + /** + * Placement metadata for the Promotion Eligibility dashboard (doc item #23). + * + * All three are optional and default to null. A team missing any of them is + * not a placement candidate, which is deliberate: it means the 1000+ teams + * that already exist need no backfill, and only the real PR review teams + * have to be configured. Nothing outside that dashboard reads these. + * + * `standupTime` is stored as typed ("11AM", "14:00") and parsed on read, and + * is interpreted in `standupTimezone`, which defaults to Pacific because the + * setup questionnaire asks for availability in Pacific. + */ + hoursBand: { + type: 'String', + enum: ['10-19.99', '20+', null], + default: null, + }, + standupDay: { + type: 'String', + enum: ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', null], + default: null, + }, + standupTime: { type: 'String', default: null }, + standupTimezone: { type: 'String', default: 'America/Los_Angeles' }, + // Deprecated field teamCode: { type: 'String', @@ -36,5 +60,4 @@ const team = new Schema({ }, }); - module.exports = mongoose.model('team', team, 'teams'); diff --git a/src/routes/promotionEligibilityRouter.js b/src/routes/promotionEligibilityRouter.js index 3fa4e28031..d09f01967f 100644 --- a/src/routes/promotionEligibilityRouter.js +++ b/src/routes/promotionEligibilityRouter.js @@ -1,13 +1,23 @@ // src/routes/promotionEligibilityRouter.js const express = require('express'); -const routes = function (userProfile, timeEntry, task, PromotionEligibility, ReviewerGroup) { +const routes = function ( + userProfile, + timeEntry, + task, + PromotionEligibility, + ReviewerGroup, + Team, + HgnFormResponses, +) { const controller = require('../controllers/promotionEligibilityController')( userProfile, timeEntry, task, PromotionEligibility, ReviewerGroup, + Team, + HgnFormResponses, ); const reviewerGroups = require('../controllers/reviewerGroupController')(ReviewerGroup); const router = express.Router(); @@ -16,6 +26,11 @@ const routes = function (userProfile, timeEntry, task, PromotionEligibility, Rev router.route('/promotion-eligibility/:reviewerId/prs-needed').patch(controller.updatePrsNeeded); + // Preview is its own route rather than a flag on /promote-members, so there + // is no way for a caller to promote people by accident while asking what + // would happen. + router.route('/promote-members/preview').post(controller.previewPromotions); + router.route('/promote-members').post(controller.promoteMembers); // Reads are POST for the same reason the dashboard read is: the permission diff --git a/src/startup/routes.js b/src/startup/routes.js index abf1f555d9..18d223997b 100644 --- a/src/startup/routes.js +++ b/src/startup/routes.js @@ -663,7 +663,15 @@ module.exports = function (app) { app.use('/api/analytics', analyticsPopularPRsRouter); app.use( '/api/', - promotionEligibilityRouter(userProfile, timeEntry, task, PromotionEligibility, ReviewerGroup), + promotionEligibilityRouter( + userProfile, + timeEntry, + task, + PromotionEligibility, + ReviewerGroup, + team, + hgnFormResponses, + ), ); // PR Analytics From 144c756715c4c0a2d68605a6a5c7ebac27fb58cf Mon Sep 17 00:00:00 2001 From: sitaram Date: Sat, 22 Aug 2026 12:12:47 -0400 Subject: [PATCH 6/7] feat(promotion-eligibility): add History column and "+ Add New" with 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. --- .../promotionEligibilityController.js | 288 +++++++++++- .../promotionEligibilityController.test.js | 411 ++++++++++++++++++ src/helpers/prEntryHelper.js | 165 +++++++ src/helpers/prEntryHelper.spec.js | 153 +++++++ src/models/promotionPrEntry.js | 48 ++ src/routes/promotionEligibilityRouter.js | 18 + src/startup/routes.js | 2 + 7 files changed, 1082 insertions(+), 3 deletions(-) create mode 100644 src/helpers/prEntryHelper.js create mode 100644 src/helpers/prEntryHelper.spec.js create mode 100644 src/models/promotionPrEntry.js diff --git a/src/controllers/promotionEligibilityController.js b/src/controllers/promotionEligibilityController.js index 6977d78016..933a7be4a4 100644 --- a/src/controllers/promotionEligibilityController.js +++ b/src/controllers/promotionEligibilityController.js @@ -11,6 +11,13 @@ const { } = require('../helpers/promotionEligibilityHelper'); const { DEFAULT_REVIEWER_GROUPS, isReviewerInGroup } = require('../helpers/reviewerGroupHelper'); const { placeReviewer, isPlaceableTeam } = require('../helpers/teamPlacementHelper'); +const { + PR_RATINGS, + normalisePrNumber, + extractPrNumbersFromSummary, + isValidRating, + groupEntriesByWeek, +} = require('../helpers/prEntryHelper'); const ONE_WEEK_MS = 7 * 24 * 60 * 60 * 1000; @@ -29,6 +36,7 @@ const promotionEligibilityController = function ( ReviewerGroup, Team, HgnFormResponses, + PromotionPrEntry, ) { /** * How many PRs a reviewer reviewed in each week they logged review work. @@ -158,12 +166,31 @@ const promotionEligibilityController = function ( taskName: { $regex: /review|pr/i }, }); + const weeklyCounts = await weeklyReviewCounts(user._id); + const currentWeek = mongoWeekOf(now); + const { successfulWeeks, remainingWeeks, weeklyRequirementsMet } = summariseWeeks({ - weeklyCounts: await weeklyReviewCounts(user._id), + weeklyCounts, prsNeeded, - currentWeek: mongoWeekOf(now), + currentWeek, }); + // The spec's History column: "the number reviewed each prior week. The + // number should be red if less than the number in PRs Needed. Should + // allow for unlimited tracking." So every week is returned, oldest + // first to read left to right, with the red decision precomputed so the + // frontend is not re-deriving the rule. + const history = weeklyCounts + .filter((entry) => !(entry.year === currentWeek.year && entry.week === currentWeek.week)) + .slice() + .reverse() + .map((entry) => ({ + year: entry.year, + week: entry.week, + reviewCount: entry.reviewCount, + belowRequirement: prsNeeded > 0 && entry.reviewCount < prsNeeded, + })); + // The spec splits the table into "New Members (joined <= 1 week ago)" // and "Existing Members (older than a week)". const isNewMember = now - new Date(user.createdDate) <= ONE_WEEK_MS; @@ -180,6 +207,8 @@ const promotionEligibilityController = function ( prsNeededSource, committedHoursChanged, totalReviews, + // One entry per prior week, oldest first, for the History column. + history, // Prior weeks in which the reviewer cleared `prsNeeded`. Exposed // alongside `remainingWeeks` so the page can show the progress rather // than only what is left. @@ -394,6 +423,249 @@ const promotionEligibilityController = function ( } }; + /** + * The five rating options, served rather than hardcoded on the frontend so + * the dropdown and the validation cannot drift apart. + */ + const getPrRatings = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to view promotion eligibility data.'); + } + return res.status(200).json({ ratings: PR_RATINGS }); + }; + + /** + * Every PR listed for a reviewer, grouped by week for the "+ Add New" column. + * + * Gated on `getReports` like the table itself. The spec only ever singles out + * the Owner for editing PRs Needed and the reviewer groups, so rating a PR is + * open to "the person with access", meaning anyone who can see the page. + */ + const getPrEntries = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to view promotion eligibility data.'); + } + if (!PromotionPrEntry) { + return res.status(500).send('PR entries are unavailable on this deployment.'); + } + + const { reviewerId } = req.params; + if (!mongoose.Types.ObjectId.isValid(reviewerId)) { + return res.status(400).send(`Invalid reviewer ID: ${reviewerId}`); + } + + try { + const entries = await PromotionPrEntry.find({ reviewerId }).sort({ addedAt: 1 }).lean(); + return res.status(200).json({ weeks: groupEntriesByWeek(entries) }); + } catch (error) { + logger.logException(error, { endpoint: 'getPrEntries', reviewerId }); + return res.status(500).send('Error fetching PR entries.'); + } + }; + + /** + * Add one PR to a reviewer's week by hand, the spec's "ability for manual + * addition". + * + * Defaults to the current week, since that is what somebody adding a PR + * today almost always means, but an explicit year and week are accepted for + * backfilling. + */ + const addPrEntry = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to edit promotion eligibility data.'); + } + if (!PromotionPrEntry) { + return res.status(500).send('PR entries are unavailable on this deployment.'); + } + + const { reviewerId } = req.params; + const { prNumber, rating = null, year, week } = req.body; + + if (!mongoose.Types.ObjectId.isValid(reviewerId)) { + return res.status(400).send(`Invalid reviewer ID: ${reviewerId}`); + } + + const normalised = normalisePrNumber(prNumber); + if (!normalised) { + return res + .status(400) + .send('prNumber must look like 1234, #1234, PR 1234, FE-1234 or a GitHub pull URL.'); + } + + if (!isValidRating(rating)) { + return res.status(400).send('rating must be one of the five options, or null.'); + } + + // Both or neither, so a half-supplied week cannot silently land somewhere + // unexpected. + const hasYear = year !== undefined; + const hasWeek = week !== undefined; + if (hasYear !== hasWeek) { + return res.status(400).send('Send both year and week, or neither.'); + } + if (hasYear && (!Number.isInteger(year) || !Number.isInteger(week) || week < 0 || week > 53)) { + return res.status(400).send('year must be a whole number and week must be 0 to 53.'); + } + + const target = hasYear ? { year, week } : mongoWeekOf(new Date()); + + try { + const entry = await PromotionPrEntry.create({ + reviewerId, + year: target.year, + week: target.week, + prNumber: normalised, + rating, + source: 'manual', + addedBy: req.body.requestor.requestorId, + ratedBy: rating ? req.body.requestor.requestorId : null, + ratedAt: rating ? new Date() : null, + }); + + return res.status(201).json(entry); + } catch (error) { + // The unique index is what stops the same PR being listed twice for one + // reviewer in one week, so a duplicate is a 409 rather than a 500. + if (error && error.code === 11000) { + return res + .status(409) + .send(`PR ${normalised} is already listed for that reviewer in that week.`); + } + logger.logException(error, { endpoint: 'addPrEntry', payload: req.body }); + return res.status(500).send('Error adding PR entry.'); + } + }; + + /** + * Rate a PR, or change its rating. Sending null clears it back to unrated. + */ + const updatePrEntryRating = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to edit promotion eligibility data.'); + } + if (!PromotionPrEntry) { + return res.status(500).send('PR entries are unavailable on this deployment.'); + } + + const { entryId } = req.params; + const { rating } = req.body; + + if (!mongoose.Types.ObjectId.isValid(entryId)) { + return res.status(400).send(`Invalid entry ID: ${entryId}`); + } + if (rating === undefined || !isValidRating(rating)) { + return res.status(400).send('rating must be one of the five options, or null to clear it.'); + } + + try { + const updated = await PromotionPrEntry.findByIdAndUpdate( + entryId, + { + $set: { + rating, + ratedBy: rating ? req.body.requestor.requestorId : null, + ratedAt: rating ? new Date() : null, + }, + }, + { new: true }, + ); + + if (!updated) return res.status(404).send('No PR entry with that id.'); + return res.status(200).json(updated); + } catch (error) { + logger.logException(error, { endpoint: 'updatePrEntryRating', payload: req.body }); + return res.status(500).send('Error updating PR rating.'); + } + }; + + /** + * Populate a reviewer's week from their weekly summary submission, which is + * the spec's "Should populate itself with the numbers from the person's + * weekly summary submission". + * + * **Treat what this produces as suggestions.** The summary is free prose and + * the parsing has never been run against a real one: not a single profile on + * dev has any weekly summary text, so there is no sample of how people + * actually write PR numbers down. Entries land with source "weeklySummary" + * precisely so they can be told apart from typed ones and reviewed. + * + * Safe to re-run. The unique index means a second import adds only what was + * not already there, and it never overwrites a rating somebody has set. + */ + const importPrEntriesFromSummary = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to edit promotion eligibility data.'); + } + if (!PromotionPrEntry) { + return res.status(500).send('PR entries are unavailable on this deployment.'); + } + + const { reviewerId } = req.params; + if (!mongoose.Types.ObjectId.isValid(reviewerId)) { + return res.status(400).send(`Invalid reviewer ID: ${reviewerId}`); + } + + try { + const user = await UserProfile.findById(reviewerId, 'weeklySummaries').lean(); + if (!user) return res.status(404).send('No such reviewer.'); + + const summaries = user.weeklySummaries || []; + const latest = summaries[summaries.length - 1]; + const prNumbers = extractPrNumbersFromSummary(latest && latest.summary); + + if (!prNumbers.length) { + return res.status(200).json({ + added: [], + skipped: [], + warnings: [ + 'No PR numbers were found in the most recent weekly summary. Add them by hand.', + ], + }); + } + + // Dated by the summary's own due date where there is one, so an import + // lands in the week the work was reported for rather than today. + const target = mongoWeekOf(latest.dueDate ? new Date(latest.dueDate) : new Date()); + + const added = []; + const skipped = []; + + // Sequential on purpose: each insert can collide with the unique index + // and the outcome per PR is what the response reports. + await prNumbers.reduce(async (previous, prNumber) => { + await previous; + try { + const entry = await PromotionPrEntry.create({ + reviewerId, + year: target.year, + week: target.week, + prNumber, + rating: null, + source: 'weeklySummary', + addedBy: req.body.requestor.requestorId, + }); + added.push(entry); + } catch (error) { + if (error && error.code === 11000) skipped.push(prNumber); + else throw error; + } + }, Promise.resolve()); + + return res.status(200).json({ + added, + skipped, + warnings: [ + 'These were read out of free text and are suggestions. Please check them before rating.', + ...(skipped.length ? [`${skipped.length} already listed for that week.`] : []), + ], + }); + } catch (error) { + logger.logException(error, { endpoint: 'importPrEntriesFromSummary', reviewerId }); + return res.status(500).send('Error importing PR entries.'); + } + }; + const promoteMembers = async (req, res) => { if (!(await hasPermission(req.body.requestor, 'putUserProfile'))) { return res.status(403).send('You are not authorized to promote members.'); @@ -551,7 +823,17 @@ const promotionEligibilityController = function ( } }; - return { getPromotionEligibilityData, updatePrsNeeded, previewPromotions, promoteMembers }; + return { + getPromotionEligibilityData, + updatePrsNeeded, + getPrRatings, + getPrEntries, + addPrEntry, + updatePrEntryRating, + importPrEntriesFromSummary, + previewPromotions, + promoteMembers, + }; }; module.exports = promotionEligibilityController; diff --git a/src/controllers/promotionEligibilityController.test.js b/src/controllers/promotionEligibilityController.test.js index 7af84fce22..f007ba70f9 100644 --- a/src/controllers/promotionEligibilityController.test.js +++ b/src/controllers/promotionEligibilityController.test.js @@ -855,6 +855,417 @@ describe('promoteMembers, placement is opt-in', () => { }); }); +describe('PR entries and ratings', () => { + const REVIEWER = '637af0c0fb9bbc1e308cff01'; + const ENTRY = '637af0c0fb9bbc1e308cfe01'; + + let PromotionPrEntry; + let UserProfile; + let controller; + let mockRes; + + const request = (body = {}, params = {}) => ({ + params, + body: { requestor: { requestorId: OWNER_ID, role: 'Administrator' }, ...body }, + }); + + const build = () => + promotionEligibilityController( + UserProfile, + {}, + {}, + { find: jest.fn(), findOneAndUpdate: jest.fn() }, + null, + null, + null, + PromotionPrEntry, + ); + + beforeEach(() => { + jest.clearAllMocks(); + hasPermission.mockResolvedValue(true); + + PromotionPrEntry = { + find: jest.fn(() => ({ sort: () => ({ lean: () => Promise.resolve([]) }) })), + create: jest.fn().mockImplementation((doc) => Promise.resolve({ _id: ENTRY, ...doc })), + findByIdAndUpdate: jest.fn().mockResolvedValue({ _id: ENTRY, rating: 'Good' }), + }; + UserProfile = { + findById: jest.fn(() => ({ lean: () => Promise.resolve({ weeklySummaries: [] }) })), + }; + controller = build(); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + const body = () => mockRes.json.mock.calls[0][0]; + + describe('getPrRatings', () => { + it('serves the five spec options so the dropdown cannot drift', async () => { + await controller.getPrRatings(request(), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(body().ratings.map((r) => r.value)).toEqual([ + 'Did not review', + 'Needs more details', + 'Good', + 'Exceptional', + 'No Image', + ]); + }); + + it('refuses a requestor without getReports', async () => { + hasPermission.mockResolvedValue(false); + await controller.getPrRatings(request(), mockRes); + expect(mockRes.status).toHaveBeenCalledWith(403); + }); + }); + + describe('addPrEntry', () => { + it('normalises the PR number on the way in', async () => { + await controller.addPrEntry( + request({ prNumber: 'PR #1234' }, { reviewerId: REVIEWER }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(201); + expect(PromotionPrEntry.create.mock.calls[0][0].prNumber).toBe('1234'); + }); + + it('keeps a repo prefix', async () => { + await controller.addPrEntry( + request({ prNumber: 'fe 2284' }, { reviewerId: REVIEWER }), + mockRes, + ); + expect(PromotionPrEntry.create.mock.calls[0][0].prNumber).toBe('FE-2284'); + }); + + it('defaults to the current week when none is given', async () => { + jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }); + jest.setSystemTime(new Date('2026-08-19T12:00:00Z')); + + await controller.addPrEntry(request({ prNumber: '1234' }, { reviewerId: REVIEWER }), mockRes); + + expect(PromotionPrEntry.create.mock.calls[0][0]).toMatchObject({ year: 2026, week: 33 }); + jest.useRealTimers(); + }); + + it('accepts an explicit week for backfilling', async () => { + await controller.addPrEntry( + request({ prNumber: '1234', year: 2025, week: 8 }, { reviewerId: REVIEWER }), + mockRes, + ); + expect(PromotionPrEntry.create.mock.calls[0][0]).toMatchObject({ year: 2025, week: 8 }); + }); + + it('rejects half a week rather than guessing the other half', async () => { + await controller.addPrEntry( + request({ prNumber: '1234', year: 2025 }, { reviewerId: REVIEWER }), + mockRes, + ); + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionPrEntry.create).not.toHaveBeenCalled(); + }); + + it('rejects an unreadable PR number', async () => { + await controller.addPrEntry( + request({ prNumber: 'last weeks one' }, { reviewerId: REVIEWER }), + mockRes, + ); + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionPrEntry.create).not.toHaveBeenCalled(); + }); + + it('rejects a rating outside the five options', async () => { + await controller.addPrEntry( + request({ prNumber: '1234', rating: 'Sufficient' }, { reviewerId: REVIEWER }), + mockRes, + ); + expect(mockRes.status).toHaveBeenCalledWith(400); + }); + + it('records who rated it when a rating comes in with the entry', async () => { + await controller.addPrEntry( + request({ prNumber: '1234', rating: 'Exceptional' }, { reviewerId: REVIEWER }), + mockRes, + ); + const doc = PromotionPrEntry.create.mock.calls[0][0]; + expect(doc.rating).toBe('Exceptional'); + expect(doc.ratedBy).toBe(OWNER_ID); + expect(doc.ratedAt).toBeInstanceOf(Date); + }); + + it('leaves ratedBy unset when the entry arrives unrated', async () => { + await controller.addPrEntry(request({ prNumber: '1234' }, { reviewerId: REVIEWER }), mockRes); + const doc = PromotionPrEntry.create.mock.calls[0][0]; + expect(doc.rating).toBeNull(); + expect(doc.ratedBy).toBeNull(); + }); + + it('409s on a duplicate rather than failing as a server error', async () => { + const duplicate = new Error('E11000 duplicate key'); + duplicate.code = 11000; + PromotionPrEntry.create = jest.fn().mockRejectedValue(duplicate); + controller = build(); + + await controller.addPrEntry(request({ prNumber: '1234' }, { reviewerId: REVIEWER }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(409); + }); + + it('rejects a malformed reviewer id', async () => { + await controller.addPrEntry(request({ prNumber: '1234' }, { reviewerId: 'nope' }), mockRes); + expect(mockRes.status).toHaveBeenCalledWith(400); + }); + }); + + describe('updatePrEntryRating', () => { + it('sets a rating', async () => { + await controller.updatePrEntryRating( + request({ rating: 'Good' }, { entryId: ENTRY }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(200); + const update = PromotionPrEntry.findByIdAndUpdate.mock.calls[0][1].$set; + expect(update.rating).toBe('Good'); + expect(update.ratedBy).toBe(OWNER_ID); + }); + + it('clears a rating with null and forgets who set it', async () => { + await controller.updatePrEntryRating(request({ rating: null }, { entryId: ENTRY }), mockRes); + + const update = PromotionPrEntry.findByIdAndUpdate.mock.calls[0][1].$set; + expect(update.rating).toBeNull(); + expect(update.ratedBy).toBeNull(); + expect(update.ratedAt).toBeNull(); + }); + + it('treats a missing rating field as a bad request, not as a clear', async () => { + await controller.updatePrEntryRating(request({}, { entryId: ENTRY }), mockRes); + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionPrEntry.findByIdAndUpdate).not.toHaveBeenCalled(); + }); + + it('404s on an entry that does not exist', async () => { + PromotionPrEntry.findByIdAndUpdate = jest.fn().mockResolvedValue(null); + controller = build(); + + await controller.updatePrEntryRating( + request({ rating: 'Good' }, { entryId: ENTRY }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(404); + }); + }); + + describe('getPrEntries', () => { + it('groups a reviewer entries by week, newest first', async () => { + PromotionPrEntry.find = jest.fn(() => ({ + sort: () => ({ + lean: () => + Promise.resolve([ + { year: 2026, week: 32, prNumber: '1' }, + { year: 2026, week: 33, prNumber: '2' }, + ]), + }), + })); + controller = build(); + + await controller.getPrEntries(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(body().weeks.map((w) => w.week)).toEqual([33, 32]); + }); + }); + + describe('importPrEntriesFromSummary', () => { + const givenSummary = (summary, dueDate) => { + UserProfile.findById = jest.fn(() => ({ + lean: () => Promise.resolve({ weeklySummaries: [{ summary, dueDate }] }), + })); + controller = build(); + }; + + it('adds the PRs it finds, marked as coming from the summary', async () => { + givenSummary('Reviewed PR 1234 and #567 this week.', '2026-08-19T00:00:00Z'); + + await controller.importPrEntriesFromSummary(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(PromotionPrEntry.create).toHaveBeenCalledTimes(2); + expect(PromotionPrEntry.create.mock.calls[0][0].source).toBe('weeklySummary'); + expect(body().added).toHaveLength(2); + }); + + it('always warns that the results are guesses', async () => { + givenSummary('Reviewed PR 1234.', '2026-08-19T00:00:00Z'); + + await controller.importPrEntriesFromSummary(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(body().warnings.join(' ')).toContain('suggestions'); + }); + + it('dates entries by the summary due date, not by today', async () => { + givenSummary('PR 1234', '2025-02-19T00:00:00Z'); + + await controller.importPrEntriesFromSummary(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(PromotionPrEntry.create.mock.calls[0][0].year).toBe(2025); + }); + + it('reports rather than fails when the summary has no PR numbers', async () => { + givenSummary('Was on leave this week.', '2026-08-19T00:00:00Z'); + + await controller.importPrEntriesFromSummary(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(body().added).toEqual([]); + expect(body().warnings.join(' ')).toContain('No PR numbers were found'); + expect(PromotionPrEntry.create).not.toHaveBeenCalled(); + }); + + it('is safe to run twice: an already-listed PR is skipped, not an error', async () => { + givenSummary('PR 1234 and PR 5678', '2026-08-19T00:00:00Z'); + const duplicate = new Error('E11000'); + duplicate.code = 11000; + PromotionPrEntry.create = jest + .fn() + .mockRejectedValueOnce(duplicate) + .mockResolvedValueOnce({ _id: ENTRY, prNumber: '5678' }); + controller = build(); + + await controller.importPrEntriesFromSummary(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(body().skipped).toEqual(['1234']); + expect(body().added).toHaveLength(1); + }); + + it('404s on a reviewer that does not exist', async () => { + UserProfile.findById = jest.fn(() => ({ lean: () => Promise.resolve(null) })); + controller = build(); + + await controller.importPrEntriesFromSummary(request({}, { reviewerId: REVIEWER }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(404); + }); + }); +}); + +describe('History column', () => { + const REVIEWER = { + _id: '637af0c0fb9bbc1e308cff62', + firstName: 'Ann', + lastName: 'Adams', + weeklycommittedHours: 10, // 7 PRs needed + createdDate: '2020-01-01', + }; + + let TimeEntry; + let controller; + let mockRes; + + const NOW = new Date('2026-08-19T12:00:00Z'); // 2026 week 33 + + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers({ doNotFake: ['nextTick', 'setImmediate'] }).setSystemTime(NOW); + hasPermission.mockResolvedValue(true); + + TimeEntry = { aggregate: jest.fn().mockResolvedValue([]) }; + controller = promotionEligibilityController( + { find: jest.fn(() => ({ lean: () => Promise.resolve([REVIEWER]) })) }, + TimeEntry, + { countDocuments: jest.fn().mockResolvedValue(0) }, + { + find: jest.fn(() => ({ lean: () => Promise.resolve([]) })), + findOneAndUpdate: jest.fn().mockResolvedValue({}), + }, + null, + ); + + mockRes = { + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + }; + }); + + afterEach(() => jest.useRealTimers()); + + const req = () => ({ body: { requestor: { requestorId: OWNER_ID, role: 'Administrator' } } }); + const entry = () => mockRes.json.mock.calls[0][0][0]; + + it('returns one row per prior week, oldest first so it reads left to right', async () => { + TimeEntry.aggregate.mockResolvedValue([ + { year: 2026, week: 32, reviewCount: 10 }, + { year: 2026, week: 31, reviewCount: 6 }, + { year: 2026, week: 30, reviewCount: 7 }, + ]); + + await controller.getPromotionEligibilityData(req(), mockRes); + + expect(entry().history.map((h) => h.week)).toEqual([30, 31, 32]); + expect(entry().history.map((h) => h.reviewCount)).toEqual([7, 6, 10]); + }); + + it('flags a week below PRs Needed, which is what the spec colours red', async () => { + TimeEntry.aggregate.mockResolvedValue([ + { year: 2026, week: 32, reviewCount: 10 }, + { year: 2026, week: 31, reviewCount: 6 }, + ]); + + await controller.getPromotionEligibilityData(req(), mockRes); + + const byWeek = Object.fromEntries(entry().history.map((h) => [h.week, h.belowRequirement])); + expect(byWeek[31]).toBe(true); // 6 < 7 + expect(byWeek[32]).toBe(false); // 10 >= 7 + }); + + it('leaves the current week out, since History is prior weeks', async () => { + TimeEntry.aggregate.mockResolvedValue([ + { year: 2026, week: 33, reviewCount: 2 }, + { year: 2026, week: 32, reviewCount: 10 }, + ]); + + await controller.getPromotionEligibilityData(req(), mockRes); + + expect(entry().history.map((h) => h.week)).toEqual([32]); + }); + + it('never flags a week red when the requirement is zero', async () => { + TimeEntry.aggregate.mockResolvedValue([{ year: 2026, week: 32, reviewCount: 0 }]); + controller = promotionEligibilityController( + { + find: jest.fn(() => ({ + lean: () => Promise.resolve([{ ...REVIEWER, weeklycommittedHours: 0 }]), + })), + }, + TimeEntry, + { countDocuments: jest.fn().mockResolvedValue(0) }, + { + find: jest.fn(() => ({ lean: () => Promise.resolve([]) })), + findOneAndUpdate: jest.fn().mockResolvedValue({}), + }, + null, + ); + + await controller.getPromotionEligibilityData(req(), mockRes); + + expect(entry().history[0].belowRequirement).toBe(false); + }); + + it('is an empty array for somebody with no logged review work', async () => { + await controller.getPromotionEligibilityData(req(), mockRes); + expect(entry().history).toEqual([]); + }); +}); + describe('mongoose ObjectId validation assumption', () => { it('treats the ids used above as valid, so the 400 tests fail for the right reason', () => { expect(mongoose.Types.ObjectId.isValid(REVIEWER_ID)).toBe(true); diff --git a/src/helpers/prEntryHelper.js b/src/helpers/prEntryHelper.js new file mode 100644 index 0000000000..a298a0b53b --- /dev/null +++ b/src/helpers/prEntryHelper.js @@ -0,0 +1,165 @@ +/** + * "+ Add New" PR entries and their ratings (doc item #23, spec item 5). + * + * The spec asks for a per-week list of the PRs a reviewer reviewed, each one + * rated from a fixed set of options that change how it is displayed. It also + * asks for the list to populate itself from the reviewer's weekly summary + * submission, with manual addition on top. + * + * Everything here is pure so the parsing and validation are testable without a + * database. + */ + +/** + * The five rating options, exactly as the spec lists them, with the display + * treatment it specifies for each. + * + * Served to the frontend rather than hardcoded there, so the dropdown and the + * validation cannot drift apart. `display` is advisory: the backend does not + * render anything, it just carries the spec's intent to whoever does. + * + * Note these are NOT the same four buckets that + * `services/analytics/fetchGithubReviews.js` uses (Exceptional, Sufficient, + * Needs Changes, Did Not Review). That service grades GitHub review states; + * this is a human rating the quality of a review. They overlap but are not + * interchangeable, and merging them was explicitly left as a question. + */ +const PR_RATINGS = [ + { value: 'Did not review', display: 'red', sortOrder: 0 }, + { value: 'Needs more details', display: 'blue', sortOrder: 1 }, + { value: 'Good', display: 'black', sortOrder: 2 }, + { value: 'Exceptional', display: 'black-yellow-highlight', sortOrder: 3 }, + { value: 'No Image', display: 'strikethrough', sortOrder: 4 }, +]; + +const RATING_VALUES = PR_RATINGS.map((rating) => rating.value); + +/** Where an entry came from. Kept so a parsed entry can be told from a typed one. */ +const PR_ENTRY_SOURCES = ['manual', 'weeklySummary']; + +/** + * Normalise a PR number as typed into something storable. + * + * The synced PR data uses a "FE-1234" / "BE-1234" prefix, and people write PR + * numbers half a dozen ways, so this accepts the common shapes and keeps the + * repo prefix when one is given: + * + * "1234", "#1234", "PR 1234", "PR#1234" -> "1234" + * "FE-1234", "fe 1234", "FE#1234" -> "FE-1234" + * "https://github.com/org/repo/pull/1234" -> "1234" + * + * @returns {string|null} the normalised number, or null if unreadable + */ +function normalisePrNumber(value) { + if (typeof value === 'number' && Number.isInteger(value) && value > 0) return String(value); + if (typeof value !== 'string') return null; + + const trimmed = value.trim(); + if (!trimmed) return null; + + const fromUrl = trimmed.match(/\/pull\/(\d+)/i); + if (fromUrl) return fromUrl[1]; + + const prefixed = trimmed.match(/^(FE|BE)\s*[-#]?\s*(\d{1,6})$/i); + if (prefixed) return `${prefixed[1].toUpperCase()}-${prefixed[2]}`; + + const plain = trimmed.match(/^(?:PRs?\s*)?#?\s*(\d{1,6})$/i); + if (plain) return plain[1]; + + return null; +} + +/** + * Pull PR numbers out of a weekly summary submission. + * + * The spec says "+ Add New" should populate itself from the person's weekly + * summary, which is a free text field people write prose into, so this is + * best effort by nature. + * + * **This has never been run against a real summary.** Not one profile on dev + * has any weekly summary text at all, so there is no sample of how people + * actually write PR numbers, and the patterns below are assumptions. The + * synced `pullRequestReview` data is a far better source and is already in the + * database; that is open question 2 to Jae. Treat anything this returns as a + * suggestion for a human to confirm, which is why entries created this way are + * stored with source "weeklySummary" rather than silently mixed in with typed + * ones. + * + * Deliberately conservative: it wants an explicit PR marker (a #, a "PR", or a + * GitHub pull URL). A bare number in prose is far more likely to be an hour + * count or a date than a PR. + * + * @param {string} summary the raw weekly summary, which may contain HTML + * @returns {string[]} normalised PR numbers, de-duplicated, in order of appearance + */ +function extractPrNumbersFromSummary(summary) { + if (typeof summary !== 'string' || !summary.trim()) return []; + + // Strip tags but keep the text, since summaries are stored as HTML. + const text = summary.replace(/<[^>]+>/g, ' '); + + const found = []; + const seen = new Set(); + const add = (candidate) => { + const normalised = normalisePrNumber(candidate); + if (normalised && !seen.has(normalised)) { + seen.add(normalised); + found.push(normalised); + } + }; + + const patterns = [ + /https?:\/\/\S*?\/pull\/(\d+)/gi, // a real GitHub link, the strongest signal + /\b(FE|BE)\s*[-#]\s*(\d{1,6})\b/gi, // the prefix the synced data uses + /\bPRs?\s*#?\s*(\d{1,6})\b/gi, // "PR 1234", "PRs #1234" + /#(\d{2,6})\b/g, // a bare "#1234" + ]; + + patterns.forEach((pattern) => { + const regex = new RegExp(pattern); + let match = regex.exec(text); + while (match) { + // The FE/BE pattern captures the prefix and number separately. + add(match[2] !== undefined ? `${match[1]}-${match[2]}` : match[1]); + match = regex.exec(text); + } + }); + + return found; +} + +/** Whether a rating is one the spec allows. null means "not yet rated". */ +function isValidRating(rating) { + return rating === null || RATING_VALUES.includes(rating); +} + +/** + * Group flat entries into the per-week shape the "+ Add New" column renders. + * + * Newest week first, and within a week the entries stay in the order they were + * added, so a reviewer's list reads the way they built it. + * + * @param {Array} entries stored PR entries + * @returns {Array<{year: number, week: number, prs: Array}>} + */ +function groupEntriesByWeek(entries) { + const byWeek = new Map(); + + (entries || []).forEach((entry) => { + const key = `${entry.year}-${entry.week}`; + if (!byWeek.has(key)) byWeek.set(key, { year: entry.year, week: entry.week, prs: [] }); + byWeek.get(key).prs.push(entry); + }); + + return [...byWeek.values()].sort((a, b) => b.year - a.year || b.week - a.week); +} + +module.exports = { + PR_RATINGS, + RATING_VALUES, + PR_ENTRY_SOURCES, + normalisePrNumber, + extractPrNumbersFromSummary, + isValidRating, + groupEntriesByWeek, +}; diff --git a/src/helpers/prEntryHelper.spec.js b/src/helpers/prEntryHelper.spec.js new file mode 100644 index 0000000000..5b820b2216 --- /dev/null +++ b/src/helpers/prEntryHelper.spec.js @@ -0,0 +1,153 @@ +const { + PR_RATINGS, + RATING_VALUES, + normalisePrNumber, + extractPrNumbersFromSummary, + isValidRating, + groupEntriesByWeek, +} = require('./prEntryHelper'); + +describe('PR_RATINGS', () => { + test('is exactly the five options the spec lists, in order', () => { + expect(RATING_VALUES).toEqual([ + 'Did not review', + 'Needs more details', + 'Good', + 'Exceptional', + 'No Image', + ]); + }); + + test('carries the display treatment the spec specifies for each', () => { + const byValue = Object.fromEntries(PR_RATINGS.map((r) => [r.value, r.display])); + expect(byValue['Did not review']).toBe('red'); + expect(byValue['Needs more details']).toBe('blue'); + expect(byValue.Good).toBe('black'); + expect(byValue.Exceptional).toBe('black-yellow-highlight'); + expect(byValue['No Image']).toBe('strikethrough'); + }); +}); + +describe('normalisePrNumber', () => { + test('accepts the plain shapes people type', () => { + expect(normalisePrNumber('1234')).toBe('1234'); + expect(normalisePrNumber('#1234')).toBe('1234'); + expect(normalisePrNumber('PR 1234')).toBe('1234'); + expect(normalisePrNumber('PR#1234')).toBe('1234'); + expect(normalisePrNumber(1234)).toBe('1234'); + }); + + test('keeps the repo prefix the synced data uses', () => { + expect(normalisePrNumber('FE-1234')).toBe('FE-1234'); + expect(normalisePrNumber('fe 1234')).toBe('FE-1234'); + expect(normalisePrNumber('BE#567')).toBe('BE-567'); + }); + + test('reads a GitHub pull URL', () => { + expect(normalisePrNumber('https://github.com/OneCommunityGlobal/HGNRest/pull/2284')).toBe( + '2284', + ); + }); + + test('rejects things that are not PR numbers rather than guessing', () => { + ['', ' ', 'abc', '#', 'PR', '12345678', null, undefined, {}, -5, 0].forEach((value) => { + expect(normalisePrNumber(value)).toBeNull(); + }); + }); +}); + +describe('extractPrNumbersFromSummary', () => { + test('finds PR numbers written the common ways', () => { + const summary = 'This week I reviewed PR 1234, #567 and https://github.com/o/r/pull/890.'; + expect(extractPrNumbersFromSummary(summary)).toEqual( + expect.arrayContaining(['1234', '567', '890']), + ); + }); + + test('strips HTML, since summaries are stored as markup', () => { + expect(extractPrNumbersFromSummary('

Reviewed #1234

')).toEqual(['1234']); + }); + + test('de-duplicates a PR mentioned more than once', () => { + const found = extractPrNumbersFromSummary('#1234 and again PR 1234 and /pull/1234'); + expect(found.filter((n) => n === '1234')).toHaveLength(1); + }); + + test('keeps the FE and BE prefixes apart from bare numbers', () => { + expect(extractPrNumbersFromSummary('Did FE-1234 and BE#567')).toEqual( + expect.arrayContaining(['FE-1234', 'BE-567']), + ); + }); + + test('ignores bare numbers in prose, which are usually hours or dates', () => { + // Deliberately conservative. Without a marker these are far more likely to + // be something other than a PR. + expect(extractPrNumbersFromSummary('I worked 12 hours across 3 days on 2026-08-21')).toEqual( + [], + ); + }); + + test('returns nothing for empty or missing input rather than throwing', () => { + expect(extractPrNumbersFromSummary('')).toEqual([]); + expect(extractPrNumbersFromSummary(null)).toEqual([]); + expect(extractPrNumbersFromSummary(undefined)).toEqual([]); + expect(extractPrNumbersFromSummary('No PRs this week, was on leave.')).toEqual([]); + }); +}); + +describe('isValidRating', () => { + test('accepts each of the five and null', () => { + RATING_VALUES.forEach((value) => expect(isValidRating(value)).toBe(true)); + expect(isValidRating(null)).toBe(true); + }); + + test('rejects anything else, including near misses', () => { + ['Sufficient', 'Needs Changes', 'good', '', undefined, 3].forEach((value) => { + expect(isValidRating(value)).toBe(false); + }); + }); + + test('rejects the analytics service buckets, which are a different vocabulary', () => { + // fetchGithubReviews grades GitHub review states into four buckets. Two of + // its names look close enough to be mixed up with these by accident. + expect(isValidRating('Sufficient')).toBe(false); + expect(isValidRating('Did Not Review')).toBe(false); // note the capital N and R + }); +}); + +describe('groupEntriesByWeek', () => { + const entry = (year, week, prNumber) => ({ year, week, prNumber }); + + test('groups by week, newest week first', () => { + const grouped = groupEntriesByWeek([ + entry(2026, 31, 'a'), + entry(2026, 33, 'b'), + entry(2026, 32, 'c'), + ]); + + expect(grouped.map((g) => g.week)).toEqual([33, 32, 31]); + }); + + test('sorts across a year boundary rather than by week number alone', () => { + const grouped = groupEntriesByWeek([entry(2026, 1, 'a'), entry(2025, 52, 'b')]); + expect(grouped.map((g) => [g.year, g.week])).toEqual([ + [2026, 1], + [2025, 52], + ]); + }); + + test('keeps entries within a week in the order they arrived', () => { + const grouped = groupEntriesByWeek([ + entry(2026, 33, 'first'), + entry(2026, 33, 'second'), + entry(2026, 33, 'third'), + ]); + + expect(grouped[0].prs.map((p) => p.prNumber)).toEqual(['first', 'second', 'third']); + }); + + test('handles empty and missing input', () => { + expect(groupEntriesByWeek([])).toEqual([]); + expect(groupEntriesByWeek(null)).toEqual([]); + }); +}); diff --git a/src/models/promotionPrEntry.js b/src/models/promotionPrEntry.js new file mode 100644 index 0000000000..10ee64b9c5 --- /dev/null +++ b/src/models/promotionPrEntry.js @@ -0,0 +1,48 @@ +// src/models/promotionPrEntry.js +const mongoose = require('mongoose'); +const { RATING_VALUES, PR_ENTRY_SOURCES } = require('../helpers/prEntryHelper'); + +const { Schema } = mongoose; + +/** + * One PR a reviewer reviewed in one week, for the "+ Add New" column of the + * Promotion Eligibility dashboard (doc item #23, spec item 5). + * + * A separate collection rather than an array on the promotionEligibility doc, + * because the spec asks for "unlimited tracking" across weeks and these are + * written one at a time by hand. Growing an unbounded array inside a document + * that is rewritten on every dashboard read would be the wrong shape. + * + * `year` and `week` match MongoDB's $year and $week, the same pair the History + * column and the weekly requirement use, so everything on this page agrees + * about which week a thing belongs to. + */ +const promotionPrEntrySchema = new Schema({ + reviewerId: { type: Schema.Types.ObjectId, ref: 'userProfiles', required: true, index: true }, + + year: { type: Number, required: true }, + week: { type: Number, required: true }, + + // Normalised on the way in. Carries the repo prefix when one was given + // ("FE-1234"), otherwise a bare number ("1234"). + prNumber: { type: String, required: true }, + + // null until somebody rates it. The spec's five options and nothing else. + rating: { type: String, enum: [...RATING_VALUES, null], default: null }, + + // "weeklySummary" means it was parsed out of the reviewer's summary text + // rather than typed, which is a guess and should be treated as one. + source: { type: String, enum: PR_ENTRY_SOURCES, default: 'manual' }, + + addedBy: { type: Schema.Types.ObjectId, ref: 'userProfiles', default: null }, + addedAt: { type: Date, default: Date.now }, + ratedBy: { type: Schema.Types.ObjectId, ref: 'userProfiles', default: null }, + ratedAt: { type: Date, default: null }, +}); + +// The same PR cannot be listed twice for one reviewer in one week. This is +// what makes the weekly-summary import safe to re-run: a second import adds +// only PRs that were not already there. +promotionPrEntrySchema.index({ reviewerId: 1, year: 1, week: 1, prNumber: 1 }, { unique: true }); + +module.exports = mongoose.model('promotionPrEntry', promotionPrEntrySchema); diff --git a/src/routes/promotionEligibilityRouter.js b/src/routes/promotionEligibilityRouter.js index d09f01967f..92ef229ffd 100644 --- a/src/routes/promotionEligibilityRouter.js +++ b/src/routes/promotionEligibilityRouter.js @@ -9,6 +9,7 @@ const routes = function ( ReviewerGroup, Team, HgnFormResponses, + PromotionPrEntry, ) { const controller = require('../controllers/promotionEligibilityController')( userProfile, @@ -18,6 +19,7 @@ const routes = function ( ReviewerGroup, Team, HgnFormResponses, + PromotionPrEntry, ); const reviewerGroups = require('../controllers/reviewerGroupController')(ReviewerGroup); const router = express.Router(); @@ -26,6 +28,22 @@ const routes = function ( router.route('/promotion-eligibility/:reviewerId/prs-needed').patch(controller.updatePrsNeeded); + // "+ Add New" column. Reads are POST for the same requestor-in-body reason + // as everything else on this router. + router.route('/promotion-eligibility/pr-ratings').post(controller.getPrRatings); + + router.route('/promotion-eligibility/:reviewerId/pr-entries').post(controller.getPrEntries); + + router.route('/promotion-eligibility/:reviewerId/pr-entries/new').post(controller.addPrEntry); + + router + .route('/promotion-eligibility/:reviewerId/pr-entries/import') + .post(controller.importPrEntriesFromSummary); + + router + .route('/promotion-eligibility/pr-entries/:entryId/rating') + .patch(controller.updatePrEntryRating); + // Preview is its own route rather than a flag on /promote-members, so there // is no way for a caller to promote people by accident while asking what // would happen. diff --git a/src/startup/routes.js b/src/startup/routes.js index 18d223997b..322856bb90 100644 --- a/src/startup/routes.js +++ b/src/startup/routes.js @@ -411,6 +411,7 @@ const permissionRouter = require('../routes/permissionRouter'); const analyticsPopularPRsRouter = require('../routes/analyticsPopularPRsRouter')(); const PromotionEligibility = require('../models/promotionEligibility'); const ReviewerGroup = require('../models/reviewerGroup'); +const PromotionPrEntry = require('../models/promotionPrEntry'); const promotionEligibilityRouter = require('../routes/promotionEligibilityRouter'); @@ -671,6 +672,7 @@ module.exports = function (app) { ReviewerGroup, team, hgnFormResponses, + PromotionPrEntry, ), ); From dffee770bf34631206797a4eebb5621227c8896f Mon Sep 17 00:00:00 2001 From: sitaram Date: Fri, 4 Sep 2026 19:35:27 -0400 Subject: [PATCH 7/7] feat(promotion-eligibility): bulk read of PR entries for many reviewers 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: { "": { 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. --- .../promotionEligibilityController.js | 83 +++++++++++ .../promotionEligibilityController.test.js | 129 ++++++++++++++++++ src/routes/promotionEligibilityRouter.js | 5 + 3 files changed, 217 insertions(+) diff --git a/src/controllers/promotionEligibilityController.js b/src/controllers/promotionEligibilityController.js index 933a7be4a4..16cf3d7edf 100644 --- a/src/controllers/promotionEligibilityController.js +++ b/src/controllers/promotionEligibilityController.js @@ -23,6 +23,13 @@ const ONE_WEEK_MS = 7 * 24 * 60 * 60 * 1000; const PROMOTED_ROLE = 'Promoted Reviewer'; +/** + * Upper bound on a single bulk PR entries read. The active table is about + * 1,600 reviewers, so this only refuses a payload the page could not have + * produced. + */ +const MAX_REVIEWERS_PER_READ = 2000; + /** * `Team` and `HgnFormResponses` are optional and only used by the promotion * placement handlers. Leaving them off keeps every existing caller, and the @@ -463,6 +470,81 @@ const promotionEligibilityController = function ( } }; + /** + * The same read for many reviewers at once. + * + * The single reviewer route is per reviewer because its path is, so a table + * showing the "+ Add New" column for a whole page of people had to make one + * request each. This does it in one request and, more importantly, one + * database query. + * + * Every id the caller sends 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 route returns, so only the read location changes on the + * client. + * + * The single route stays. It is still the better one to hit straight after + * an add or a rating change, when only one row needs refreshing. + */ + const getPrEntriesForReviewers = async (req, res) => { + if (!(await hasPermission(req.body.requestor, 'getReports'))) { + return res.status(403).send('You are not authorized to view promotion eligibility data.'); + } + if (!PromotionPrEntry) { + return res.status(500).send('PR entries are unavailable on this deployment.'); + } + + const { reviewerIds } = req.body; + if (!Array.isArray(reviewerIds) || reviewerIds.length === 0) { + return res.status(400).send('reviewerIds must be a non-empty array of reviewer IDs.'); + } + + // The whole active table is about 1,600 people, so this refuses only a + // payload that could not have come from the page. + if (reviewerIds.length > MAX_REVIEWERS_PER_READ) { + return res + .status(400) + .send(`reviewerIds cannot exceed ${MAX_REVIEWERS_PER_READ} reviewers in one request.`); + } + + const invalid = reviewerIds.find((id) => !mongoose.Types.ObjectId.isValid(id)); + if (invalid !== undefined) { + return res.status(400).send(`Invalid reviewer ID: ${invalid}`); + } + + // Deduplicated, since a repeated id would otherwise widen the query for a + // key that can only be written once anyway. + const uniqueIds = [...new Set(reviewerIds.map(String))]; + + try { + const entries = await PromotionPrEntry.find({ reviewerId: { $in: uniqueIds } }) + .sort({ addedAt: 1 }) + .lean(); + + // Seeded with every requested id first, so a reviewer with no entries is + // still present, and the grouping below only ever fills these in. + const byReviewer = new Map(uniqueIds.map((id) => [id, []])); + entries.forEach((entry) => { + const key = String(entry.reviewerId); + if (byReviewer.has(key)) byReviewer.get(key).push(entry); + }); + + const reviewers = {}; + byReviewer.forEach((reviewerEntries, id) => { + reviewers[id] = { weeks: groupEntriesByWeek(reviewerEntries) }; + }); + + return res.status(200).json({ reviewers }); + } catch (error) { + logger.logException(error, { + endpoint: 'getPrEntriesForReviewers', + reviewerCount: uniqueIds.length, + }); + return res.status(500).send('Error fetching PR entries.'); + } + }; + /** * Add one PR to a reviewer's week by hand, the spec's "ability for manual * addition". @@ -828,6 +910,7 @@ const promotionEligibilityController = function ( updatePrsNeeded, getPrRatings, getPrEntries, + getPrEntriesForReviewers, addPrEntry, updatePrEntryRating, importPrEntriesFromSummary, diff --git a/src/controllers/promotionEligibilityController.test.js b/src/controllers/promotionEligibilityController.test.js index f007ba70f9..d95809af67 100644 --- a/src/controllers/promotionEligibilityController.test.js +++ b/src/controllers/promotionEligibilityController.test.js @@ -1083,6 +1083,135 @@ describe('PR entries and ratings', () => { }); }); + describe('getPrEntriesForReviewers', () => { + const OTHER_REVIEWER = '637af0c0fb9bbc1e308cff02'; + + /** Whatever the caller asks for, the model answers with these entries. */ + const givenEntries = (entries) => { + PromotionPrEntry.find = jest.fn(() => ({ + sort: () => ({ lean: () => Promise.resolve(entries) }), + })); + controller = build(); + }; + + it('keys the response by reviewer id', async () => { + givenEntries([ + { reviewerId: REVIEWER, year: 2026, week: 33, prNumber: '1' }, + { reviewerId: OTHER_REVIEWER, year: 2026, week: 33, prNumber: '2' }, + ]); + + await controller.getPrEntriesForReviewers( + request({ reviewerIds: [REVIEWER, OTHER_REVIEWER] }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(200); + expect(body().reviewers[REVIEWER].weeks[0].prs[0].prNumber).toBe('1'); + expect(body().reviewers[OTHER_REVIEWER].weeks[0].prs[0].prNumber).toBe('2'); + }); + + it('returns a key for a reviewer with no entries, so the caller never handles a missing one', async () => { + givenEntries([{ reviewerId: REVIEWER, year: 2026, week: 33, prNumber: '1' }]); + + await controller.getPrEntriesForReviewers( + request({ reviewerIds: [REVIEWER, OTHER_REVIEWER] }), + mockRes, + ); + + expect(body().reviewers[OTHER_REVIEWER]).toEqual({ weeks: [] }); + }); + + it('groups each reviewer newest week first, exactly like the single reviewer route', async () => { + givenEntries([ + { reviewerId: REVIEWER, year: 2026, week: 32, prNumber: '1' }, + { reviewerId: REVIEWER, year: 2026, week: 33, prNumber: '2' }, + ]); + + await controller.getPrEntriesForReviewers(request({ reviewerIds: [REVIEWER] }), mockRes); + + expect(body().reviewers[REVIEWER].weeks.map((w) => w.week)).toEqual([33, 32]); + }); + + it('reads every reviewer in one query rather than one query each', async () => { + givenEntries([]); + + await controller.getPrEntriesForReviewers( + request({ reviewerIds: [REVIEWER, OTHER_REVIEWER] }), + mockRes, + ); + + expect(PromotionPrEntry.find).toHaveBeenCalledTimes(1); + expect(PromotionPrEntry.find.mock.calls[0][0].reviewerId.$in).toHaveLength(2); + }); + + it('asks for each reviewer once when the same id is sent twice', async () => { + givenEntries([]); + + await controller.getPrEntriesForReviewers( + request({ reviewerIds: [REVIEWER, REVIEWER] }), + mockRes, + ); + + expect(PromotionPrEntry.find.mock.calls[0][0].reviewerId.$in).toEqual([REVIEWER]); + }); + + it.each([[undefined], [[]], ['not-an-array'], [{}]])( + 'rejects %p as reviewerIds', + async (reviewerIds) => { + givenEntries([]); + + await controller.getPrEntriesForReviewers(request({ reviewerIds }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionPrEntry.find).not.toHaveBeenCalled(); + }, + ); + + it('names the offending id when one is not a valid ObjectId', async () => { + givenEntries([]); + + await controller.getPrEntriesForReviewers( + request({ reviewerIds: [REVIEWER, 'not-an-object-id'] }), + mockRes, + ); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.send.mock.calls[0][0]).toContain('not-an-object-id'); + expect(PromotionPrEntry.find).not.toHaveBeenCalled(); + }); + + it('refuses a batch larger than the whole table rather than building the response', async () => { + givenEntries([]); + const tooMany = Array.from({ length: 2001 }, () => REVIEWER); + + await controller.getPrEntriesForReviewers(request({ reviewerIds: tooMany }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(PromotionPrEntry.find).not.toHaveBeenCalled(); + }); + + it('refuses a caller without getReports', async () => { + givenEntries([]); + hasPermission.mockResolvedValue(false); + + await controller.getPrEntriesForReviewers(request({ reviewerIds: [REVIEWER] }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(403); + expect(PromotionPrEntry.find).not.toHaveBeenCalled(); + }); + + it('reports a database failure as a 500 rather than throwing at the router', async () => { + PromotionPrEntry.find = jest.fn(() => ({ + sort: () => ({ lean: () => Promise.reject(new Error('mongo is down')) }), + })); + controller = build(); + + await controller.getPrEntriesForReviewers(request({ reviewerIds: [REVIEWER] }), mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(500); + }); + }); + describe('importPrEntriesFromSummary', () => { const givenSummary = (summary, dueDate) => { UserProfile.findById = jest.fn(() => ({ diff --git a/src/routes/promotionEligibilityRouter.js b/src/routes/promotionEligibilityRouter.js index 92ef229ffd..e61bffe321 100644 --- a/src/routes/promotionEligibilityRouter.js +++ b/src/routes/promotionEligibilityRouter.js @@ -34,6 +34,11 @@ const routes = function ( router.route('/promotion-eligibility/:reviewerId/pr-entries').post(controller.getPrEntries); + // Same read for a list of reviewers, so a table does not make one request per + // row. Two path segments where the single reviewer route has three, so the + // literal cannot be captured as a `:reviewerId`. + router.route('/promotion-eligibility/pr-entries').post(controller.getPrEntriesForReviewers); + router.route('/promotion-eligibility/:reviewerId/pr-entries/new').post(controller.addPrEntry); router