Repository navigation
Dhruv - Fix blue square related issues - #2346
DeMoliT1on wants to merge 13 commits into
Conversation
- Stop Blue Squares for late joiners - Close enough hours email should also remove the blue square - Add full name to Administrative Details
one-community
added
the
High Priority - Please Review First
shubhamjakhete
left a comment
shubhamjakhete
left a comment
There was a problem hiding this comment.
Reviewed the changes. The Wednesday-or-later new-user handling is updated correctly, the close-enough-hours flow removes the associated Blue Square, and the user's full name is now included in the Administrative Details section. I did not find any blocking issues with the implementation. Looks good to me.
Isha2303165
left a comment
Isha2303165
left a comment
There was a problem hiding this comment.
Tested the changed backend helpers and all 110 focused tests passed. However, I found issues in the new Blue Square/email flow that should be addressed before approval. During assignBlueSquareForTimeNotMet, the resolved email recipients are showing as Promises, for example: “Email BCCs for blue square assignment: Promise { }”, rather than resolved arrays. This indicates the async CC/BCC helpers may be used without awaiting them.
I also noticed that the new close-enough-hours flow calls getUserRoleByEmail(user), which iterates over user.teams, and also uses user.lastName, while the user projection in weeklyAutoReplyEmailFunction does not appear to include teams or lastName. This could cause the new path to fail with real queried user documents.
The test run also logs a caught error, “TypeError: original.toObject is not a function”, from weeklyAutoReplyEmailFunction even though the suite reports passing. Please address these runtime issues and add or adjust tests so the full success path completes without errors.
Isha2303165
left a comment
Isha2303165
left a comment
There was a problem hiding this comment.
Rechecked the latest updates after my previous review. All 110 focused tests across 3 suites passed, and the backend build completed successfully with 767 files compiled. The previously observed issues also appear resolved: Blue Square email BCC values are now resolved correctly instead of remaining pending Promises, the weekly auto-reply query includes the required lastName and teams fields, and the previous toObject runtime error did not recur during testing. No blocking issues found in the updated changes. Approved.
AaditTrivedi
left a comment
AaditTrivedi
left a comment
There was a problem hiding this comment.
Reviewed by Aadit Trivedi.
How I verified
Checked out the PR locally (Node 22), installed with npm ci from package-lock.json, ran the three affected suites, and read the changes in userHelper.js against the description and @Isha2303165's review. I did not run the cron flow end to end against a database.
Note for other reviewers: installing with yarn makes userHelper.spec.js and getInfringementEmailBody.test.js fail to load (htmlparser2 is ESM-only). The same failure happens on development, so it is pre-existing and not caused by this PR. yarn.lock is out of sync with package.json on development too. Use npm ci to run these tests.
Verified working
- Tests:
userHelper.spec.js,warningsHelper.spec.js, andgetInfringementEmailBody.test.jspass 110/110. Screenshot below. - @Isha2303165's three issues are resolved:
- No
Promise { <pending> }recipients appear in the test output. - The
weeklyAutoReplyEmailFunctionuser query now includeslastNameandteams(line 1299), which the close-enough path uses. - No
toObject is not a functionerror appears in the test output.
- No
- Wednesday rule: both checks moved from
> 1to> 2, so only start dates of Wednesday or later get a pass, matching the description. - The user's full name appears in the Administrative Details section of the email.
Issues found
- Scope is much larger than described. The description lists
userHelper.jsand the test script, but the PR also moves about 250 lines fromwarningsController.jsinto a newwarningsHelper.js, bumpsmongodb-memory-serverfrom ^7.6.3 to ^11.2.0 (four major versions, affecting every test that uses it), rewrites large parts of both lockfiles, and editssonar-project.properties. Please document these or move them to separate PRs. sonar-project.propertiesaddssrc/scripts/**tosonar.exclusions. That excludes every script in the repo from the quality gate, including the test script this PR edits, so its issues are hidden rather than fixed. Please revert this and address the issues directly.- Jae's name and email are hardcoded in
monitorData(jae@onecommunityglobal.org). Please read the monitor contact from configuration or an existing lookup rather than code. - Conflict with #2369: this PR moves
filterWarningsout ofwarningsController.js, and #2369 modifiesfilterWarningsin that same file to return tracker descriptions. Whichever merges second needs to carry the other's changes over, or the description field will be lost. Worth coordinating with @AnthonyWeathers.
Pointers, not blocking
- The warning tracker query (
currentWarnings.find({ activeWarning: true }, ...)) runs inside the per-user loop, so it repeats for every qualifying user. It returns the same data each time and can be fetched once before the loop. if (typeof currentWarningDescriptions !== 'undefined')is always true right after theawait, so the check and its comment can be removed.- Pre-existing, not from this PR: around line 380 the sort compares
a.lastNamewithb.lastname(lowercase n), so the second name always ends in "undefined."
Evidence
Test run, 110/110 passing with npm ci:
Required action
Please revert the Sonar exclusion, document or split out the extra changes, move the monitor contact out of the code, and coordinate with #2369. Happy to re-review once updated.
manavkheni1
left a comment
manavkheni1
left a comment
There was a problem hiding this comment.
Reviewed by Manav. Pulled the branch locally (npm ci, per @AaditTrivedi's note about npm install failing on htmlparser2), and independently verified the following:
Tests: Ran the same three suites (userHelper.spec.js, warningsHelper.spec.js, getInfringementEmailBody.test.js) — 110/110 passing, no Promise { } values, no toObject is not a function errors.
I also confirmed @AaditTrivedi's four outstanding concerns are still present and should be addressed before merge:
- mongodb-memory-server bumped from ^7.6.3 to ^11.2.0 (4 major versions) — unrelated to this PR's stated scope
- sonar-project.properties now excludes src/scripts/** from the quality gate, hiding rather than fixing issues in the test script
- Hardcoded email "jae@onecommunityglobal.org" at userHelper.js line 173, inside getUserRoleByEmail — should come from config or an existing lookup instead
- Merge conflict risk with #2369, which also modifies filterWarnings/warningsController.js
Agreeing changes should be addressed before this merges. Screenshot below showing my test run.
…config * Update sonar-project.properties to use sonar.coverage.exclusions for src/scripts instead of full exclusion * Fix a pre-existing case-sensitivity sorting bug using lastName instead of lastname in userHelper.js * Optimize performance by moving the currentWarningDescriptions query outside of the user loop in userHelper.js * Simplify warningId lookup using optional chaining and remove redundant undefined checks * Replace hardcoded monitor contact details in monitorData with MONITOR_CONTACT_CONFIG
Thanks for the detailed review! Here is what I've updated:
|
vidiyala99
left a comment
vidiyala99
left a comment
There was a problem hiding this comment.
Reviewed d2b0143 (no review on this head yet). I ran the PR's suites offline (no DB, no .env): userHelper.spec.js, warningsHelper.spec.js and getInfringementEmailBody.test.js pass 110/110. I also started this branch's backend and checked Dashboard > Show Trackers as Admin: 67 GET /api/warnings/:id calls succeeded and the trackers render as before (including the "Blu Sq Rmvd - Hrs Close Enoug" row), so the warningsHelper extraction looks behavior-preserving. The Wednesday start-day rule and the admin Name: line match the description, and the earlier review points (hoisted warning query, lastName typo) are addressed.
I did not run testEmailJobs.js against the shared dev DB, because of point 1. Instead I wrote three small mocked unit tests (below, collapsed); each one passing demonstrates one of the first three issues:
- Targeted test runs are not isolated. With
targetUserIdset,assignBlueSquareForTimeNotMetscopes only the active-user query (userHelper.js:886-888). The inactive-user block (:954-962) still runsprocessWeeklySummariesByUserIdfor every inactive user, which pushes an empty summary with$slice: 4and drops their oldest stored summary, anddeleteOldTimeOffRequests()(:938) still deletes expired time-off requests for everyone. Following test step 7 on the shared dev DB therefore changes data for users who are not the target (proof P1). Please skip both blocks when a target is given, and consider a dry-run flag for the script. - Auto-removal also removes blue squares earned for a missing summary. A user at 85 to 99 percent of their hours with no summary still resolves to
MISSED_HOURS_BY_<15%(resolveAutoReplyTemplatehas no summary input; onlymetHours && !hasSummaryis skipped at:1325), and the new code then pulls their blue square (:1347-1349), whose description cites both reasons (proof P2). Please only remove it when the summary was submitted. - The
$pullmatches on date only.$pull: { infringements: { date: assignmentDate } }removes every blue square dated that Sunday, including manually assigned ones, although the comment says "system-assigned" (proof P3). AddingmanuallyAssigned: { $ne: true }(the field exists on the schema) would match the intent. The current test steps use a manual blue square, so they pass for the wrong reason. - Not idempotent. Each run pushes another warning, and on the fourth-plus path a second run would pull the re-issued blue square (same date) and issue another.
infringementCountis also left stale after the pull in the blue and yellow cases. - Contradictory emails in production. A fourth-offense user gets "issue blue square" (
:1412), then "New Infringement Assigned" (:1446), then the "close enough for us to remove this blue square" template (:1460). These new sends also ignore the email/cc/bcc overrides, andsendEmailToUseris not awaited. - The monitor contact is still hardcoded:
monitorContactConfig.jsholds literals, andgetUserRoleByEmail(:174) andsendBlueSquareEmail(:1174-1176) still embed the address. Themongodb-memory-serverbump and lockfile churn are also still in, and the conflict with #2369 remains.
Proof tests (mocked models, no DB). Save as src/helpers/__tests__/PREP-2346.proof.spec.js and run npx jest src/helpers/__tests__/PREP-2346.proof.spec.js
const mongoose = require('mongoose');
const moment = require('moment-timezone');
/* =======================
MOCKS (MUST COME FIRST)
======================= */
jest.mock('../../models/userProfile', () => ({
findById: jest.fn(),
find: jest.fn(),
aggregate: jest.fn(),
updateOne: jest.fn().mockResolvedValue({}),
findByIdAndUpdate: jest.fn((id, update, cb) => {
if (typeof cb === 'function') cb(null);
return Promise.resolve({});
}),
}));
jest.mock('../../models/badge', () => ({
find: jest.fn(),
findOne: jest.fn(),
}));
jest.mock('../../models/team', () => ({
aggregate: jest.fn(),
}));
jest.mock('../../utilities/timeUtils');
jest.mock('../../utilities/emailSender');
jest.mock('../dashboardhelper', () =>
jest.fn(() => ({
laborthisweek: jest.fn().mockResolvedValue([{ timeSpent_hrs: 36 }]),
})),
);
jest.mock('../../models/BlueSquareEmailAssignment', () => ({
find: jest.fn().mockImplementation(() => ({
populate: jest.fn().mockImplementation(() => ({
exec: jest.fn().mockResolvedValue([
{
email: 'bcc-test@example.com',
assignedTo: { isActive: true },
},
]),
})),
})),
}));
jest.mock('../../models/timeOffRequest', () => ({
deleteMany: jest.fn(),
find: jest.fn(),
}));
/* =======================
IMPORTS AFTER MOCKS
======================= */
const userProfile = require('../../models/userProfile');
const currentWarnings = require('../../models/currentWarnings');
const warningsHelper = require('../warningsHelper');
const userHelperFactory = require('../userHelper');
const emailSender = require('../../utilities/emailSender');
const { COMPANY_TZ } = require('../../constants/company');
const timeOffRequest = require('../../models/timeOffRequest');
const { weeklyAutoReplyEmailFunction, assignBlueSquareForTimeNotMet } = userHelperFactory();
/* ===== PREP-2346 proof tests (reviewer-only, not part of PR) ===== */
describe('PREP-2346 proofs', () => {
const WARNING_DESC = 'Blu Sq Rmvd - Hrs Close Enoug';
const assignmentDate = moment().tz(COMPANY_TZ).startOf('week').format('YYYY-MM-DD');
beforeEach(() => {
jest.clearAllMocks();
emailSender.mockResolvedValue(true);
});
it('P1: targeted assignBlueSquareForTimeNotMet still sweeps ALL inactive users weeklySummaries', async () => {
userProfile.find
.mockResolvedValueOnce([]) // target user query
.mockResolvedValueOnce([{ _id: new mongoose.Types.ObjectId() }, { _id: new mongoose.Types.ObjectId() }]); // inactive
userProfile.findByIdAndUpdate.mockClear();
await assignBlueSquareForTimeNotMet({
targetUserId: new mongoose.Types.ObjectId(),
ccOverride: ['cc@x.com'],
bccOverride: ['bcc@x.com'],
});
expect(userProfile.find).toHaveBeenNthCalledWith(2, { isActive: false }, '_id');
const summaryShifts = userProfile.findByIdAndUpdate.mock.calls.filter(
(c) => c[1] && c[1].$push && c[1].$push.weeklySummaries,
);
expect(summaryShifts.length).toBe(2); // both inactive users had summaries shifted ($slice: 4 drops oldest)
expect(summaryShifts[0][1].$push.weeklySummaries.$slice).toBe(4);
expect(timeOffRequest.deleteMany).toHaveBeenCalled(); // global time-off cleanup also runs
});
it('P2: 85-99% hours user WITHOUT a weekly summary still gets the blue square pulled', async () => {
jest.spyOn(timeOffRequest, 'find').mockResolvedValue([]);
jest.spyOn(currentWarnings, 'find').mockReturnValue({
sort: jest.fn().mockResolvedValue([{ warningTitle: WARNING_DESC, _id: 'w1' }]),
});
jest.spyOn(warningsHelper, 'filterWarnings').mockReturnValue({ sendEmail: null, size: 0 });
jest.spyOn(warningsHelper, 'sendEmailToUser').mockImplementation(() => {});
const user = {
_id: '60c72b2f9b1d8b2bad709999',
email: 'nosummary@example.com',
firstName: 'No',
lastName: 'Summary',
weeklycommittedHours: 40, // dashboardhelper mock returns 36h = 90% of 40
missedHours: 0,
startDate: '2022-01-01',
weeklySummaryOption: 'Required',
weeklySummaryNotReq: false,
weeklySummaries: [{ summary: '' }, { summary: '' }], // NO summary last week
infringements: [
{ date: assignmentDate, description: 'System auto-assigned infringement for two reasons: not meeting weekly volunteer time commitment as well as not submitting a weekly summary.' },
],
teams: [],
warnings: [],
};
userProfile.find.mockResolvedValueOnce([user]);
userProfile.findByIdAndUpdate.mockReset();
userProfile.findByIdAndUpdate.mockResolvedValue(user);
await weeklyAutoReplyEmailFunction({ targetUserId: user._id, bccOverride: ['b@x.com'] });
expect(userProfile.findByIdAndUpdate).toHaveBeenCalledWith(user._id, {
$pull: { infringements: { date: assignmentDate } },
});
const body = emailSender.mock.calls.map((c) => c[2]).join(' ');
expect(body).toContain('close enough to your total hours for us to remove this blue square');
});
it('P3: $pull is by date only, so a MANUAL blue square on the same Sunday is removed too', async () => {
jest.spyOn(timeOffRequest, 'find').mockResolvedValue([]);
jest.spyOn(currentWarnings, 'find').mockReturnValue({ sort: jest.fn().mockResolvedValue([]) });
jest.spyOn(warningsHelper, 'filterWarnings').mockReturnValue({ sendEmail: null, size: 0 });
const user = {
_id: '60c72b2f9b1d8b2bad708888', email: 'm@example.com', firstName: 'M', lastName: 'A',
weeklycommittedHours: 40, missedHours: 0, startDate: '2022-01-01',
weeklySummaryOption: 'Required', weeklySummaries: [{}, { summary: 'ok' }],
infringements: [
{ date: assignmentDate, description: 'system hours' },
{ date: assignmentDate, description: 'manual: missed video call', manuallyAssigned: true },
],
teams: [], warnings: [],
};
userProfile.find.mockResolvedValueOnce([user]);
userProfile.findByIdAndUpdate.mockReset();
userProfile.findByIdAndUpdate.mockResolvedValue(user);
await weeklyAutoReplyEmailFunction({ targetUserId: user._id, bccOverride: ['b@x.com'] });
const pull = userProfile.findByIdAndUpdate.mock.calls.find((c) => c[1].$pull);
expect(pull[1].$pull.infringements).toEqual({ date: assignmentDate }); // no manuallyAssigned / description filter
});
});02: the three mocked proof tests pass on d2b0143; each pass demonstrates issue 1, 2 or 3
03a: targetUserId only scopes the active-user query; time-off cleanup and the inactive sweep stay global
03b: the near-miss path skips only met-hours users without a summary, and the pull is keyed on date alone
01: the PR's own suites pass 110/110 (working as intended)
04 (Admin, light): Show Trackers works on this branch's backend (working as intended)
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
Re-reviewed the current head d2b0143 after the remediation commit.
I verified the latest changes locally without a database: the three affected suites passed (3/3 suites, 110/110 tests), and npm run build compiled 775 files successfully. I also confirmed that the Sonar exclusion is now limited to coverage, the warning query was hoisted, and the lastName sort typo was corrected.
Changes are still required before merge. The monitor contact was moved to monitorContactConfig.js, but the name and email remain hardcoded there, so the original configurability concern is not resolved. More importantly, the current head still has the same production-behavior problems documented with mocked proofs in the existing same-head review: targeted runs can mutate unrelated users/global data, the close-enough path can remove a blue square when the summary is missing, and the date-only $pull can remove manual infringements. I confirmed those paths remain present in userHelper.js.
I’m not adding duplicate inline comments because the existing review on this exact commit already provides reproducible proof tests and precise locations. Please address those current-head findings and add regression coverage for the targeted/dry-run boundary and selective infringement removal.
|
@vidiyala99 @adit24dhaya Have included the required changes for the PR. Also would like to add that the monitor contact config is temporarily hardcoded, and the responsible DevOps team can convert it to env variables since I do not have permission to add the same for Dev/Production, hence have left a note in the config file. I have also coordinated with @AnthonyWeathers in regards to #2369 that once one of the PR gets merged, the other would incorporate the required changes while handle the merge conflicts. |
56952f2 to
bd27455
Compare
RichaSapre
left a comment
RichaSapre
left a comment
There was a problem hiding this comment.
Tested PR #2346 locally using npm ci. The three affected test suites passed with 110/110 tests, and npm run build completed successfully with 775 files compiled.
I reviewed the updated Blue Square assignment and weekly auto-reply flows, including the new handling for close-enough hours, warning creation, infringement updates, and targeted-user testing.
I found one remaining correctness issue: manually assigned Blue Squares are still considered eligible by hasTodayBlueSquare. Although the later $pull excludes manually assigned infringements, the function can still create a “Removed Blue Square” warning and send the removal email while the manually assigned square remains. Please restrict this flow to system-assigned squares and add a regression test.
AaditTrivedi
left a comment
AaditTrivedi
left a comment
There was a problem hiding this comment.
Re-reviewed by Aadit Trivedi.
Follow-up to my earlier review, checking the commits from Sep 28 and Oct 3 ("sonar exclusions, optimize warning queries, and extract monitor config" and "Address reviewer comments") against the diff with development.
Fixed since my review
- Sonar:
src/scripts/**is no longer insonar.exclusions; it is now only insonar.coverage.exclusions, so scripts are still analyzed for issues. That addresses my concern. - The monitor contact is now read from the new
src/config/monitorContactConfig.jsinstead of being hardcoded in the warning logic.
Still open
mongodb-memory-serveris still bumped from ^7.6.3 to ^11.2.0. If the upgrade is needed for these tests, please explain why in the description; otherwise please move it to a separate PR, since it affects every test that uses an in-memory database.- A new hardcoded recipient was added:
const recipients = ['jae@onecommunityglobal.org'];(around line 174 ofuserHelper.js). Since the monitor contact now lives inmonitorContactConfig.js, please read this from the same config. The other occurrences of that address in the file were already on development, so they are outside this PR.
Pointer, not blocking
- Please confirm coordination with #2369, since both PRs still change
filterWarnings.
Required action
Please address the two open points. The rest of my earlier review is resolved.
|
vidiyala99
left a comment
vidiyala99
left a comment
There was a problem hiding this comment.
Re-reviewed at 0c1a62a (after "Address reviewer comments" and "Blue Square only process System Assigned infringements"). I re-ran the three proof tests from my earlier review against this head: each one passed while its bug was present, and all three now fail because the behavior changed. The PR's suites still pass (userHelper, warningsHelper, getInfringementEmailBody: 110/110).
Fixed since my review:
- Targeted runs are isolated: with
targetUserIdset,userProfile.findis called once (the active-user query) and the inactive-user summary sweep no longer runs (P1 now fails as expected). - No removal without a summary: the pull only happens for
MISSED_HOURS_BY_<15%andhasSummary(userHelper.js:1351), so a user who is close on hours but skipped the summary keeps the blue square (P2 now fails as expected). - Manual blue squares are safe: the pull is now
{ date: assignmentDate, manuallyAssigned: { $ne: true } }(:1365), and the latest commit only considers system-assigned infringements (:1334). A manual blue square on the same Sunday stays (P3 now fails as expected). - Re-runs are safe: the
alreadyProcessedTodaycheck skips users already handled for that date, andinfringementCountis re-set after both the pull and the 4th-offense push. - Emails:
sendEmailToUseris awaited, and the "close enough" email is skipped when a 4th-offense blue square is issued, so a user no longer gets contradictory messages. - Monitor contact: it's now read from
src/config/monitorContactConfig.js. Thanks for the note about moving it to env variables in deployment, and for coordinating the overlap with #2369.
One note, not blocking: mongodb-memory-server still goes from ^7.6.3 to ^11.2.0 (a major version jump), with about 850 lines of package-lock.json / yarn.lock changes. It doesn't seem needed for this fix. The suites I ran pass with it, but since it changes the test runtime for every backend suite, it would be safer in its own PR. If it has to stay, please mention it in the description.
Approving.
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
Rechecked 0c1a62a. Thanks for the updates: targeted runs now skip unrelated cleanup, missing summaries no longer trigger removal, and manual squares are preserved. Sequential reruns also behaved correctly. The documented dependency/configuration decisions are noted; I’m not repeating those comments.
Two additional cases still need fixing: an email failure can permanently lose the fourth-offense square, and overlapping runs can issue two replacement squares. Reproduction steps are inline.
Validation: 110/110 existing tests passed across three suites. I ran 17 isolated checks against the unchanged helpers with in-memory model/email mocks: normal warning colors, hour boundaries and manual-square controls passed; fault injection reproduced the lost square after retry, and two simultaneous calls produced two squares and two new warnings. No real emails or database writes. Babel compiled 775 files; ESLint had 0 errors/114 warnings. Tests used existing local dependencies; the new memory-server version and a live cron run were not validated.
Commit audit (nonblocking): one subject is 75 characters; several subjects are vague or non-imperative and most lack explanatory bodies/ticket references. Existing bodies are blank-line separated; no subjects end in a period.
| }; | ||
|
|
||
| if (sendEmail !== null) { | ||
| await warningsHelper.sendEmailToUser( |
There was a problem hiding this comment.
[P1] An email failure can permanently remove the fourth-offense square. I used a volunteer with 36/40 hours, a submitted summary, three previous close-enough warnings and one system square. When the warning email rejects, the square is already removed and the fourth warning saved, but the replacement below never runs. Retrying leaves zero squares because the user is no longer eligible. Please complete the required state change before sending email and make interrupted processing recoverable. Add this failure-and-retry test.
| if (alreadyProcessedToday) continue; | ||
|
|
||
| // Remove only the system-assigned blue square for that day | ||
| const userAfterPull = await userProfile.findByIdAndUpdate( |
There was a problem hiding this comment.
[P2] The repeat-run check does not protect overlapping calls. With the same fourth-offense fixture, I ran two weeklyAutoReplyEmailFunction calls together. Both read the old warnings and passed the check; the final profile had two replacement squares and two new warnings. This can happen if a manual run overlaps the cron job. Please claim each user/week in the database before changing it, so only one call processes that occurrence, and add an overlapping-call regression test.
RichaSapre
left a comment
RichaSapre
left a comment
There was a problem hiding this comment.
Re-reviewed the latest changes at 0c1a62a.
The correctness issue from my previous review is resolved. The Blue Square auto-reply flow now only treats system-assigned infringements as eligible, so manually assigned Blue Squares no longer trigger an incorrect removal warning/email while remaining on the user's profile.
I also verified the updated logic around selective infringement removal and reviewed the current test/CI results. The affected suites continue to pass, and the current CI/quality gate is green.
A dedicated regression test for the manuallyAssigned: true case would still be useful, but I don't consider that blocking now that the behavior itself has been corrected.
Approving.
manavkheni1
left a comment
manavkheni1
left a comment
There was a problem hiding this comment.
Re-reviewed at 0c1a62a. Confirmed all 110 tests pass, and my four previous concerns (Sonar exclusion, monitor contact, hardcoded email, #2369 coordination) are resolved per the author's latest commits. However, I independently traced the code for adit24dhaya's two still-open findings and can confirm both are accurate at this head: the blue-square $pull, the warning $push, and the replacement blue-square $push for 4th+ offenses are three separate, unawaited/non-transactional findByIdAndUpdate calls with no per-user idempotency guard between them. A failure or interruption between the pull and the replacement push (P1) can permanently leave a user with zero blue squares; concurrent or overlapping runs (P2) can each independently compute the same occurrence count and both issue a replacement square, producing duplicates. These are real correctness risks in a function that runs via cron and directly affects user standing, and should be addressed (e.g., a single atomic update or a per-user/week claim) before merge.
PrajwalpGM256
left a comment
PrajwalpGM256
left a comment
There was a problem hiding this comment.
Reviewed by Prajwal.
How I verified
Reviewed the latest head f6a4d769 ("Prevent concurrency function call producing duplicates"), which no review covers yet. Installed with npm ci on Node 20.20.2, which matches .nvmrc and CI. Ran the three affected suites, npm run build and ESLint on the changed helpers, then read the new commit against @adit24dhaya's P1/P2 and Jae's four original requirements. I also ran the PR's new findOneAndUpdate filter and pipeline against an in-memory MongoDB, using the real UserProfile model, to check what it writes (proof test below). I did not run the cron flow against the shared dev DB.
Verified working
- Tests:
userHelper.spec.js,warningsHelper.spec.js, andgetInfringementEmailBody.test.jspass 112/112. npm run buildcompiles 775 files. ESLint on the changed helpers: 0 errors (91 warnings, mostlymax-params/no-magic-numbers).- @adit24dhaya's P1 is addressed. The square removal, the warning, the replacement square, and
infringementCountare now one atomic write (userHelper.js:1384-1423), and emails are sent only after it commits (:1428). An email failure no longer leaves the user with zero squares. - @adit24dhaya's P2 is addressed in design. The claim is conditional (this week's system square still exists AND this week's warning is absent), so an overlapping second call matches nothing and returns
null(:1426). - The Wednesday rule index is correct:
moment.day()counts Sunday as 0, so> 2means Wednesday to Saturday (:607,:616). Name:appears between Start Date and Role in thegetInfringementEmailBodytemplate (:329-332) and has a test.- Confirmed @AaditTrivedi's note about
const recipients = ['jae@...'](:174): it is moved unchanged fromwarningsController.js:40ondevelopment, so it is outside this PR's scope.
Issues found
-
Blocking: the new atomic write saves the warning and the 4th-offense blue square without an
_id.
The newfindOneAndUpdateuses an aggregation pipeline ([ { $set: ... } ]). Mongoose 5 (^5.13.23) does not cast pipeline updates, so subdocument defaults like_idare never added tonewWarningorreplacementSquares. The proof test below runs the exact filter and pipeline from:1384-1423: existing warnings keep their ids, but the new warning and the replacement square come back with no_id.Impact in production:
- The replacement blue square can't be viewed, edited, or deleted. The frontend opens squares by
blueSquare._id(BlueSquare.jsx:57-65). - Every "Blu Sq Rmvd - Hrs Close Enoug" warning this code creates can never be deleted, because
warningsController.js:213deletes with$pull: { warnings: { _id: warningId } }.
Fix: generate the ids explicitly, e.g.
_id: new mongoose.Types.ObjectId()onnewWarningand on each entry ofreplacementSquares. Please add a test that asserts both have an_id. - The replacement blue square can't be viewed, edited, or deleted. The frontend opens squares by
Proof test (in-memory MongoDB, real UserProfile model). Save as src/helpers/__tests__/PR-2346.proof.spec.js and run npx jest src/helpers/__tests__/PR-2346.proof.spec.js --verbose
/* Proof for PR #2346 review. Not part of the PR; delete after use. */
const mongoose = require('mongoose');
// eslint-disable-next-line import/no-extraneous-dependencies
const { MongoMemoryServer } = require('mongodb-memory-server');
const UserProfile = require('../../models/userProfile');
const WARNING_DESC = 'Blu Sq Rmvd - Hrs Close Enoug';
const assignmentDate = '2026-10-04';
describe('PR-2346 proof: atomic close-enough update writes subdocs without _id', () => {
let mongod;
beforeAll(async () => {
mongod = await MongoMemoryServer.create();
await mongoose.connect(mongod.getUri(), { useNewUrlParser: true, useUnifiedTopology: true });
});
afterAll(async () => {
await mongoose.disconnect();
await mongod.stop();
});
it('4th offense: the new warning and the replacement blue square have no _id', async () => {
const userId = new mongoose.Types.ObjectId();
const oldWarning = (date, color) => ({
_id: new mongoose.Types.ObjectId(),
date,
description: WARNING_DESC,
color,
});
// Raw insert: a user with 3 previous close-enough warnings and this Sunday's system square
await UserProfile.collection.insertOne({
_id: userId,
email: 'proof@example.com',
infringements: [
{
_id: new mongoose.Types.ObjectId(),
date: assignmentDate,
description: 'system square',
manuallyAssigned: false,
},
],
warnings: [
oldWarning('2026-09-13', 'blue'),
oldWarning('2026-09-20', 'blue'),
oldWarning('2026-09-27', 'yellow'),
],
});
// Same shapes as userHelper.js:1362-1382
const newWarning = { iconId: 'proof', color: 'red', date: assignmentDate, description: WARNING_DESC };
const replacementSquares = [
{
date: assignmentDate,
description: 'Issued a blue square for an Admin having to remove past blue squares 4 times',
createdDate: assignmentDate,
manuallyAssigned: false,
reason: 'missingHours',
reasons: ['time not met'],
editedBy: [],
},
];
// Filter + pipeline copied from userHelper.js:1384-1423
const updated = await UserProfile.findOneAndUpdate(
{
_id: userId,
infringements: { $elemMatch: { date: assignmentDate, manuallyAssigned: { $ne: true } } },
warnings: { $not: { $elemMatch: { description: WARNING_DESC, date: assignmentDate } } },
},
[
{
$set: {
infringements: {
$concatArrays: [
{
$filter: {
input: { $ifNull: ['$infringements', []] },
cond: {
$not: {
$and: [
{ $eq: ['$$this.date', assignmentDate] },
{ $ne: ['$$this.manuallyAssigned', true] },
],
},
},
},
},
replacementSquares,
],
},
warnings: { $concatArrays: [{ $ifNull: ['$warnings', []] }, [newWarning]] },
},
},
{ $set: { infringementCount: { $size: '$infringements' } } },
],
{ new: true },
).lean();
const addedWarning = updated.warnings.find((w) => w.date === assignmentDate);
const replacement = updated.infringements.find((i) => i.date === assignmentDate);
console.log('OLD warnings _id:', updated.warnings.slice(0, 3).map((w) => String(w._id)));
console.log('NEW warning:', JSON.stringify(addedWarning));
console.log('NEW replacement square:', JSON.stringify(replacement));
// Existing subdocs keep their _id ...
expect(updated.warnings.slice(0, 3).every((w) => w._id)).toBe(true);
// ... but everything this PR's pipeline writes has none
expect(addedWarning._id).toBeUndefined();
expect(replacement._id).toBeUndefined();
});
});PASS = bug present. Once _ids are added to newWarning and replacementSquares, this test fails as expected.
- The 4th-offense test passes while
notifyInfringementsthrows.
Running the suite logs[autoReply] infringement email failed for test@example.com: TypeError: original.toObject is not a function(userHelper.js:1469). The test atuserHelper.spec.js:1360returns plain arrays from the mocked claim, sonotifyInfringementsthrows at:1892, and thecatchat:1468swallows it. This is the same error @Isha2303165 flagged earlier. The "New Infringement Assigned" email on a 4th offense therefore has no passing test. Please make the mock return arrays withtoObject(or real documents) and assert that the infringement email is sent.
-
There is still no overlapping-call regression test for P2.
The test at:1402mocks the claim to returnnull. It checks the early exit but never runs two calls together. Since this PR already addsmongodb-memory-server, please add a test that runs twoweeklyAutoReplyEmailFunctioncalls withPromise.allagainst the in-memory DB and asserts one warning and at most one replacement square. -
Full name is missing from one of the two Administrative Details templates.
Jae's requirement says "ALL email templates".src/has exactly twoADMINISTRATIVE DETAILSblocks, and this PR only updateduserHelper.js:$ git grep -n -i "ADMINISTRATIVE DETAILS" -- src ":!**/__tests__/**" src/controllers/timeEntryController.js:427: <p><b>ADMINISTRATIVE DETAILS:</b></p> src/helpers/userHelper.js:329: <p><b>ADMINISTRATIVE DETAILS:</b></p> $ git diff --stat origin/development...HEAD -- src/controllers/timeEntryController.js (no output: file not changed by this PR)The time-entry-edit blue square email at
timeEntryController.js:427-430still goes Start Date → Role → Title with noName:line.
Question
- Wednesday rule. Jae's wording is "if a person's first logged time is Wednesday or later".
checkIsNewUserchecks the weekday ofstartDate, so a user whose start date is Monday and who first logs time on Thursday still gets a blue square. Was the start-date interpretation confirmed with Jae? If yes, a note in the description would help.
Pointers, not blocking
- After the claim commits, a failed infringement or warning email is only logged (
:1452,:1469). It is never retried, because the next run sees the claim as done. That's an acceptable trade-off for data safety, but worth a comment in the code. - The
mongodb-memory-server^7.6.3 → ^11.2.0 bump is now documented; no further ask. It would also make the test in item 3 straightforward.
Evidence
- Build and lint on
f6a4d769:
Required action
Please add _ids to the warning and replacement square written by the atomic update (item 1), fix the 4th-offense test so the infringement email path passes without errors (item 2), add an overlapping-call test (item 3), and add the full name to the time-entry-edit template (item 4). Happy to re-review once updated.
rahulrubugunday
left a comment
rahulrubugunday
left a comment
There was a problem hiding this comment.
Tested locally on dhruv/fix-blue-square-email-issues at f6a4d76 as Administrator, using testEmailJobs.js on a Volunteer test account.
- Unit tests: 112/112 passing.
- Close-enough flow removes the week's system Blue Square, leaves a manual one untouched and records the warning. A rerun in the same week makes no further changes.
- On the 4th occurrence, the system Blue Square is replaced by the "remove past blue squares 4 times" one and a red warning is added.
Requesting changes:
- The replacement Blue Square is saved without an _id. The findOneAndUpdate in f6a4d76 is an aggregation pipeline, which Mongoose 5.13 does not cast. As a result, notifyInfringements throws "Cannot read properties of undefined (reading 'equals')" at userHelper.js:1967, so the user never gets the Blue Square email, and the Blue Square can't be opened in the profile to edit or delete. Suggested fix: set _id: new mongoose.Types.ObjectId() on the replacement square and on newWarning.
- notifyInfringements receives user.infringements from before the update, so "Previous Blue Square Reasons" includes the removed square. Once item 1 is fixed, the warning email and the infringement email will also both fire for the same Blue Square.
Niket07pathak
left a comment
Niket07pathak
left a comment
There was a problem hiding this comment.
Reviewed by Niket Pathak
How I verified
Checked out PR #2346 locally using the PR backend branch and development frontend. Started both applications and verified that the frontend was connected to the local backend.
Logged in using an Owner account and selected an eligible volunteer test account with 10 weekly committed hours and 9 logged hours.
Verified working
- Opened the volunteer's profile and manually assigned a Blue Square dated Sunday of the current week.
- Updated
TARGET_USER_IDand test email addresses intestEmailJobs.js. - Executed all three test options:
assignBlueSquareForTimeNotMetweeklyAutoReplyEmailFunctionRun Both
- All three test script options executed successfully without reported errors.
- Refreshed the volunteer's profile to check the Blue Square removal behavior.
Issues found
No issues were observed during the executed test scripts.
Evidence
Attached screenshots.
Required action
No changes requested based on the completed testing.
Approved
There was a problem hiding this comment.
Reviewed the PR conversation and earlier reviews, then verified on dhruv/fix-blue-square-email-issues at f6a4d76 by running the affected unit tests and checking the changed code in userHelper.js.
Fixed: The BCC issue @Isha2303165 reported is resolved. The log now shows a resolved array (Email BCCs for blue square assignment: [ 'bcc-test@example.com' ]) instead of Promise { <pending> }.
Still failing: userHelper, warningsHelper, and getInfringementEmailBody pass 112/112, but the run still logs TypeError: original.toObject is not a function from notifyInfringements (userHelper.js:1892), called by weeklyAutoReplyEmailFunction. The error is caught and only logged, so the 4th-offense infringement email path fails without failing the test (screenshots attached).
Confirmed in code: Neither newWarning nor the entries in replacementSquares define an _id, which confirms the blocking issue raised in the earlier reviews about the findOneAndUpdate pipeline.
Requesting changes.
Happy to re-review and run the full flow once these are fixed.
vidiyala99
left a comment
vidiyala99
left a comment
There was a problem hiding this comment.
Re-reviewed the new commit f6a4d76 ("Prevent concurrency function call producing duplicates"). The PR's mocked tests can't show what MongoDB actually stores, so I replayed the exact findOneAndUpdate filter and pipeline from userHelper.js through the real UserProfile model (Mongoose 5.13.23) on an in-memory MongoDB. Each test user had a system blue square and a manual square for the week, plus an older square.
What works:
- The claim is atomic. Running it twice for the same user and week does nothing the second time (returns
null), and three concurrent claims for one user produce exactly one winner, one warning and no leftover system square. That's the duplicate problem this commit targets, and it's fixed. - Only the system square for that week is removed; the manual square and the older square stay, and
infringementCountis recomputed in the same write (3 → 2). userHelper.spec.jspasses (89/89).
-
The new warning and the replacement blue square are saved without an
_id. Pipeline updates skip Mongoose, so the objects added by$concatArraysare stored exactly as written. In the database, the pushed warning has no_id, and on a 4th offense the red replacement square has no_ideither (the existing squares keep theirs). The old$pushpath let Mongoose add one. The rest of the app relies on those ids (from reading the code):- Deleting a warning uses
$pull: { warnings: { _id: warningId } }(warningsController.deleteUsersWarnings), so these warnings can't be removed from the tracker. - Deleting a blue square pulls by
infringements._id, and editing one runsoriginalinfringements.find((bs) => bs._id.equals(blueSquareId))(userProfileController.js:2666). The replacement square can't be edited or deleted, and editing a square that comes after it in the list would throw onundefined.equals.
Adding_id: new mongoose.Types.ObjectId()tonewWarningand to each replacement square before building the pipeline fixes this (literal ObjectIds are stored as-is).
- Deleting a warning uses
-
Minor:
warningIdis stored as an ObjectId, while the schema field is aString(the old$pushcast it).String(...)on it keeps the stored type consistent with other warnings.
Requesting changes for item 1. The concurrency fix itself is right, and with ids added this is good to go. (This PR has 6 approvals, so I'd hold the merge for this one.)
…rning and replacement squares
|
vidiyala99
left a comment
vidiyala99
left a comment
There was a problem hiding this comment.
Re-reviewed the new commit ca58396 ("Resolve issue where findOneAndUpdate missed _id for the added warning and replacement squares"). As in my last review, I replayed the exact findOneAndUpdate filter and pipeline from userHelper.js through the real UserProfile model (Mongoose 5.13.23) on an in-memory MongoDB, then exercised the code paths that depend on the new ids (screenshot 1; the probe is in the collapsed block below). Nothing touched the shared dev database.
My blocking item is fixed:
- The new warning and the replacement blue square now have an
_idin the database. On a 4th offense, the stored red warning and the "Issued a blue square..." square both have ids. Every square on the user has one. - The ids work where the app uses them. Looking up the replacement square with
bs._id.equals(...)(the edit path) finds it.$pullbyinfringements._idremoves it, and$pull: { warnings: { _id } }(thedeleteUsersWarningspath) removes the new warning (4 warnings to 3). - The 4th-offense infringement email now goes out. I called the real
notifyInfringementswith real documents, fetched the same wayweeklyAutoReplyEmailFunctionfetches them (no.lean()), and the claimed result. It no longer throws, and it sends one "New Infringement Assigned" email that includes the replacement square. This also covers theundefined.equals/toObjecterrors @rahulrubugunday and @snehitakandula reported onf6a4d76.
Still good from last time: running the claim twice in the same week does nothing the second time. Three concurrent claims produce exactly one winner, one warning and no leftover system square. Only this week's system square is removed; the manual square and the older square stay. userHelper.spec.js, warningsHelper.spec.js and getInfringementEmailBody.test.js pass 112/112.
One minor note, not blocking: warningId is still stored as an ObjectId in the database, while the schema field is a String (Mongoose casts it back to a string when the profile is loaded, so the app reads it fine). The new test checks typeof warning.warningId === 'string', but the mocked _id there is already the string 'warning123', so the test can't catch this. String(...) around the find(...)?._id would match how the old $push stored it.
Approving.
Probe used (drop into src/controllers/, run with npx jest <file> --runInBand)
jest.mock('../utilities/emailSender', () => jest.fn().mockResolvedValue(true));
const mongoose = require('mongoose');
const { MongoMemoryServer } = require('mongodb-memory-server');
let server;
beforeAll(async () => { server = await MongoMemoryServer.create(); await mongoose.connect(server.getUri(), { useNewUrlParser: true, useUnifiedTopology: true }); });
afterAll(async () => { await mongoose.disconnect(); await server.stop(); });
const D = '2026-10-04'; const WD = 'Blu Sq Rmvd - Hrs Close Enoug';
// Exact filter + pipeline from userHelper.js @ ca58396 (weeklyAutoReplyEmailFunction, MISSED_HOURS_BY_<15%)
const claim = (UP, id, occurrence, warningId) => {
let color = 'blue'; if (occurrence === 3) color = 'yellow'; else if (occurrence >= 4) color = 'red';
const newWarning = { _id: new mongoose.Types.ObjectId(), iconId: 'icon-' + occurrence, color, date: D, description: WD, warningId };
const replacementSquares = occurrence >= 4 ? [{ _id: new mongoose.Types.ObjectId(), date: D, description: 'Issued a blue square ...', createdDate: D, manuallyAssigned: false, reason: 'missingHours', reasons: ['time not met'], editedBy: [] }] : [];
return UP.findOneAndUpdate(
{ _id: id, infringements: { $elemMatch: { date: D, manuallyAssigned: { $ne: true } } }, warnings: { $not: { $elemMatch: { description: WD, date: D } } } },
[{ $set: { infringements: { $concatArrays: [{ $filter: { input: { $ifNull: ['$infringements', []] }, cond: { $not: { $and: [{ $eq: ['$$this.date', D] }, { $ne: ['$$this.manuallyAssigned', true] }] } } } }, replacementSquares] }, warnings: { $concatArrays: [{ $ifNull: ['$warnings', []] }, [newWarning]] } } },
{ $set: { infringementCount: { $size: '$infringements' } } }],
{ new: true },
);
};
const seed = async (UP, priorWarnings) => {
const _id = new mongoose.Types.ObjectId();
await UP.collection.insertOne({ _id, firstName: 'P', lastName: 'Probe', email: `${_id}@x.org`, role: 'Volunteer',
infringements: [
{ _id: new mongoose.Types.ObjectId(), date: D, description: 'system square', manuallyAssigned: false, reason: 'missingHours' },
{ _id: new mongoose.Types.ObjectId(), date: D, description: 'manual square', manuallyAssigned: true, reason: 'other' },
{ _id: new mongoose.Types.ObjectId(), date: '2026-09-27', description: 'older square', manuallyAssigned: false, reason: 'missingHours' }],
infringementCount: 3,
warnings: Array.from({ length: priorWarnings }, (_, i) => ({ _id: new mongoose.Types.ObjectId(), date: `2026-0${i + 1}-01`, description: WD, color: 'blue' })) });
return _id;
};
const show = (u) => u && JSON.stringify({ count: u.infringementCount,
inf: u.infringements.map((x) => `${x.description}|${x.date}|_id:${x._id ? 'yes' : 'NO'}`),
lastWarning: (() => { const w = u.warnings[u.warnings.length - 1]; return `${w.color}|${w.date}|_id:${w._id ? 'yes' : 'NO'}|warningId:${typeof w.warningId}`; })() });
test('PROBE 2346 atomic claim on real MongoDB', async () => {
const UP = require('../models/userProfile');
const wid = new mongoose.Types.ObjectId();
const a = await seed(UP, 0);
const r1 = await claim(UP, a, 1, wid);
console.log('PROBE A 1st run (blue):', r1 ? 'claimed' : 'null', show(r1));
const r2 = await claim(UP, a, 1, wid);
console.log('PROBE A 2nd run same week:', r2 ? 'claimed AGAIN' : 'null (skipped, good)');
const raw = await UP.collection.findOne({ _id: a });
console.log('PROBE A raw warnings stored:', raw.warnings.length, 'last _id in DB:', raw.warnings[raw.warnings.length - 1]._id ? 'yes' : 'NO');
const b = await seed(UP, 3);
const r3 = await claim(UP, b, 4, wid);
console.log('PROBE B 4th occurrence (red + replacement square):', r3 ? 'claimed' : 'null', show(r3));
const rawB = await UP.collection.findOne({ _id: b });
console.log('PROBE B raw replacement square _id in DB:', rawB.infringements.filter((x) => x.description.startsWith('Issued')).map((x) => (x._id ? 'yes' : 'NO')).join(','));
const c = await seed(UP, 0);
const res = await Promise.all([claim(UP, c, 1, wid), claim(UP, c, 1, wid), claim(UP, c, 1, wid)]);
const rawC = await UP.collection.findOne({ _id: c });
console.log('PROBE C 3 concurrent claims -> winners:', res.filter(Boolean).length, '| warnings stored:', rawC.warnings.length, '| system squares left for the week:', rawC.infringements.filter((x) => x.date === D && !x.manuallyAssigned).length);
});
test('PROBE 2346 ids usable by delete/edit paths', async () => {
const UP = require('../models/userProfile');
const wid = new mongoose.Types.ObjectId();
const b = await seed(UP, 3);
const r = await claim(UP, b, 4, wid);
const w = r.warnings[r.warnings.length - 1];
const sq = r.infringements.find((x) => x.description.startsWith('Issued'));
const rawB = await UP.collection.findOne({ _id: b });
const rawW = rawB.warnings[rawB.warnings.length - 1];
console.log('PROBE D stored warningId type:', rawW.warningId && rawW.warningId.constructor.name, '| other warnings have warningId:', rawB.warnings.slice(0, -1).map((x) => typeof x.warningId).join(','));
// edit path (userProfileController): originalinfringements.find((bs) => bs._id.equals(blueSquareId))
const found = rawB.infringements.find((bs) => bs._id.equals(sq._id));
console.log('PROBE D edit lookup of replacement square by _id:', found ? 'found' : 'NOT FOUND', '| every square has _id:', rawB.infringements.every((x) => x._id));
// delete square path: $pull by infringements._id
await UP.updateOne({ _id: b }, { $pull: { infringements: { _id: sq._id } } });
// delete warning path (warningsController.deleteUsersWarnings): $pull: { warnings: { _id: warningId } }
await UP.updateOne({ _id: b }, { $pull: { warnings: { _id: w._id } } });
const after = await UP.collection.findOne({ _id: b });
console.log('PROBE D after $pull -> replacement squares left:', after.infringements.filter((x) => x.description.startsWith('Issued')).length, '| warnings', rawB.warnings.length, '->', after.warnings.length);
});
test('PROBE 2346 notifyInfringements on real docs after claim', async () => {
const UP = require('../models/userProfile');
const emailSender = require('../utilities/emailSender');
const userHelper = require('../helpers/userHelper')();
const b = await seed(UP, 3);
// same fetch shape as weeklyAutoReplyEmailFunction (no .lean())
const [user] = await UP.find({ _id: b }, '_id email firstName lastName infringements warnings');
const updated = await claim(UP, b, 4, new mongoose.Types.ObjectId());
emailSender.mockClear();
let err = null;
try {
await userHelper.notifyInfringements(user.infringements || [], updated.infringements, updated.firstName, updated.lastName, updated.email, updated.role, updated.startDate, 'Member', 10);
} catch (e) { err = e; }
const subjects = emailSender.mock.calls.map((c) => c.find((x) => typeof x === 'string' && /Infringement/i.test(x)));
console.log('PROBE E notifyInfringements threw:', err ? String(err) : 'no', '| emailSender calls:', emailSender.mock.calls.length, '| subjects:', JSON.stringify(subjects));
const body = emailSender.mock.calls[0] && emailSender.mock.calls[0].find((x) => typeof x === 'string' && x.includes('Issued a blue square'));
console.log('PROBE E email body mentions the replacement square:', body ? 'yes' : 'no');
});Screenshot 1: real-MongoDB probe on ca58396 (ids stored and usable, infringement email sent) and the PR's suites, 112/112
There was a problem hiding this comment.
Re-reviewed the latest commit ca58396 on this PR’s branch, following up on my earlier review of f6a4d76.
My earlier findings are fixed:
toObject error: userHelper, warningsHelper, and getInfringementEmailBody pass 112/112. With console output forced on (--silent=false), the [autoReply] logs still print, but the [autoReply] infringement email failed ... TypeError: original.toObject is not a function line no longer appears. So the 4th-offense email path completes instead of failing silently (screenshot attached).
Missing _id: newWarning and each entry in replacementSquares now set _id: new mongoose.Types.ObjectId(), so the new warning and the replacement blue square can be edited and deleted.
Non-blocking: The full name is still missing from the second Administrative Details template, in timeEntryController.js (the time-entry-edit blue square email). It still goes Start Date → Role → Title with no Name line, while the template in userHelper.js now includes it. If the requirement covers all email templates, this one may need the same change, here or in a follow-up.
screenshots:
Approving.
AaditTrivedi
left a comment
AaditTrivedi
left a comment
There was a problem hiding this comment.
Re-reviewed by Aadit Trivedi.
Third review, after the commits from Oct 7 to Oct 9: "Blue Square only process System Assigned infringements," "Prevent concurrency function call producing duplicates," and "Resolve issue where findOneAndUpdate missed _id for the added warning and replacement squares."
How I verified
Checked out the latest commit (ca58396), installed with npm ci, ran userHelper.spec.js, warningsHelper.spec.js, and getInfringementEmailBody.test.js (112/112 passing), confirmed no conflicts with development using git merge-tree, and reviewed the new commits.
Verified
- The close-enough path now applies the warning and the replacement infringements in a single atomic
findOneAndUpdatewith an update pipeline, instead of separate updates, so overlapping runs of the weekly job can't both modify the same user and produce duplicates. The tests were rewritten to assert one atomic claim per user. - Only system-assigned infringements are processed, so manually assigned blue squares are left alone.
- The fix for the missing
_idon added warnings and replacement squares is covered by the updated tests. - From my earlier reviews: the Sonar change is coverage-only, and the monitor contact comes from
monitorContactConfig.js.
Pointers, not blocking
mongodb-memory-serveris still bumped from ^7.6.3 to ^11.2.0 without explanation. The tests pass with it, but since it affects every test that uses an in-memory database, a sentence in the description on why it's needed would help.const recipients = ['jae@onecommunityglobal.org'];is still hardcoded inuserHelper.js; reading it frommonitorContactConfig.js, like the monitor contact, would be consistent.
Approving.
aaryashah22
left a comment
aaryashah22
left a comment
There was a problem hiding this comment.
Tested locally on ca58396c (Chrome, 9 Oct): this branch's backend with the development frontend, logged in as Owner, on my own Manager test account (0 squares; set up for the test with a 9 h entry dated 10/01 against 10 committed, start date 09/20, summary Not Required).
What works
testEmailJobs.jsoption 1 added a system square (picture 1). After I changed its date to 10/04, option 2 printedMISSED_HOURS_BY_<15%, and after a refresh the square was gone (picture 2) and one "Blu Sq Rmvd - Hrs Close Enoug" warning was logged (picture 3).- Running option 2 again added no second warning (still one dot after a refresh).
- The new warning can be deleted and stays deleted after a refresh (what the new
_idcommit fixes). - 4th time (3 earlier warnings logged by hand): the system square was removed, a replacement square "Issued a blue square for an Admin having to remove past blue squares 4 times..." was added (picture 4), it opens and deletes, and the 4th dot is red. The script output had no "infringement email failed" or "warning email failed" line.
Suggestion (non-blocking, from the code)
Step 6 of How to test ("Assign a blue square manually") no longer leads to a removal: hand-added squares are saved with manuallyAssigned: true and skipped, so only a system-assigned square is removed (for testing: the script route, steps 7-8). Could you update step 6?
Not tested: emails (from the code, a local backend does not send them), including the new Name line in Administrative details and the 4th-offense emails; the Sunday scheduled job (only the test script); the Wednesday / late-joiner rule; Volunteer and Administrator logins (Owner only, per step 2); unit tests. Dark mode: N/A (backend only).
GitHub: 12 files (+1821 -621), all checks passed. Approving - thanks!
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
Thanks for the fixes. I rechecked ca58396 and both issues from my previous review are resolved. The fourth-offense replacement square is saved even when the warning email exhausts its retries, and overlapping runs do not add duplicate warnings or squares. The explicitly assigned IDs also work for finding and deleting the new entries.
Validation: 112 existing tests and 13 independent integration tests passed. The integration tests used the actual Mongoose models and update pipeline against a fresh, temporary local MongoDB 4.4 database. They covered synchronized concurrent reads, ten additional overlap cases, permanent/transient email failures, reruns, manual squares, missing summaries, hour boundaries, and recovery after a failed database update. SMTP was mocked; no real email or shared database writes. Babel compiled 775 files; changed-file ESLint had zero errors and 115 warnings; the whitespace check passed.
Commit audit: the two follow-up commits are focused and use imperative subjects without ending periods. Optional cleanup: shorten the latest 98-character subject and add explanatory bodies and issue references. Approving this re-review.
AlekyaJogini
left a comment
AlekyaJogini
left a comment
There was a problem hiding this comment.
Reviewed by Alekya.
How I tested
Checked out dhruv/fix-blue-square-email-issues (PR #2346) in HGNRest and ran npx jest on userHelper.spec.js, warningsHelper.spec.js and getInfringementEmailBody.test.js. I also read the manual test script src/scripts/testEmailJobs.js and searched userHelper.js for targetUserId to see how the script limits its queries. I did not read the full 12-file diff line by line.
Verified working
- On the PR branch: 3 suites, 112 of 112 tests passed.
- In userHelper.js, the cron function queries I looked at use emailConfig.targetUserId to limit the query to one user when it is set and testEmailJobs.js always sets it and passes tester email overrides.
Issues found
- None found in what I tested.
Not tested - The manual cron flow with testEmailJobs.js (assignBlueSquareForTimeNotMet and weeklyAutoReplyEmailFunction). My local backend uses the shared dev database and my test accounts do not have last-week hours at 85-99% of their commitment so I did not run it.
- Email delivery, since the PR notes infringement emails only work on production.
- Dark mode, since the PR has no UI changes.
RichaSapre
left a comment
RichaSapre
left a comment
There was a problem hiding this comment.
I reviewed PR #2346. The Blue Square updates look good, and the previous issues with duplicate updates, manual Blue Squares, missing IDs, and email details appear to be fixed. The focused tests passed with 112/112 tests, but the latest GitHub Actions test/coverage check is failing. I recommend requesting changes until that CI failure is resolved.
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
The new Name line is correct. I tested the actual email-building function with ordinary, Unicode and empty-last-name fixtures: it adds the name under Administrative Details without changing recipients, subjects or the infringement threshold. Approving the code recheck.
I also investigated the red CI reported by Richa. That run tested merge d48f74d against an older development commit. I reproduced its two task-controller failures. On GitHub's current merge, 3ee4519, all 169 focused tests pass (57 task-controller and 112 blue-square/email tests). The fixes are already in development; this PR does not change that task-controller code.
The posted CI check is still red. Please obtain a fresh passing run before merging; these focused results do not replace the full test/coverage gate.
Validation on PR head 30bf097: 112 blue-square/email tests and 55 task-controller tests passed, plus six independent email-function cases. Babel compiled 775 files; ESLint had zero errors/155 warnings; whitespace passed. HTTP and database connections were blocked, and email sending was stubbed. No shared data or real email was used. The previous concurrency code is unchanged; its real-Mongo tests were not rerun in this follow-up.
Commit audit: the new commit is focused, imperative, under 72 characters and has no final period. Optional: add a short why-body and issue reference.
This branch has not been deployed
15 participants
Description
This PR fixes few of the Blue Square assignment issues:
Related PRS (if any):
This is a backend only PR
Main changes explained:
warningsController.jsinto a newwarningsHelper.jsfile. These helper functions support the Warning Logging system i.e automating the process where admins previously had to manually remove close-enough blue squares from the UI, while still properly logging the warning and notifying both the user and the admin via email.userHelper.jsto handle new user logic, append full names to Administrative Details, and automate the removal of blue squares for close-enough hours within the updated cron job function.sonar-project.propertiesto usesonar.coverage.exclusionsinstead of a full script exclusion, allowing quality gate security analyses while ignoring irrelevant test script coverage.testEmailJobs.jsto manually test cron job functions modified in this PR.How to test:
MISSED_BY_<15%i.e no of hours logged in previous week is 85-99% of the weekly commitment./userprofile/<target_user_id>testEmailJobs.jsscript to call theassignBlueSquareForTimeNotMetfunction. Please make sure to changeTARGET_USER_IDto the id from userporfile page ``/userprofile/<target_user_id> before calling the function.weeklyAutoReplyEmailFunctionfrom the test script with the same user id.Screenshots or videos of changes:
Blue.Square.Fix.Summary.Video.webm
Note:
Since the changes in the PR are related to cron job functions which runs weekly, the test script is the only way to test the changes. Additionally you won't be able to receive any infringement emails since those are only possible on Production.