[AnthonyWeathers]

Added description value to current warnings and added order value during creation of a new warning tracker, sanitation and validations for description value, relocated the add missing description for trackers and adjusted the conditionals for all 3 helpers. Added unique constraint to prevent concurrent tracker creations and retry the ones that get blocked.

Description

Added code to handle new description value for warning trackers, and addressed missing order value for when new trackers are created. Added validation to description, added unique constraint, updated tests to pass with code changes.

Related PRS (if any):

This backend PR is related to the #5588 frontend PR.
To test this backend PR you need to checkout the #5588 frontend PR.
…

Main changes explained:

• Updated currentWarningsController.js to accept description value, and calculate the order value to assign to new warning tracker. Added htmlContentSanitizer, moved add missing description to this file, added other validation checks as listed by a reviewer. Used lean to clean up the retrieved objects and allow me to remove the orderSequence, the unique constraint from being sent to the frontend.
• Updated currentWarningsController.spec.js to adjust for the changes, and added new tests to cover most of the new code, and ensured they all passed.
• Updated warningsController, added retrieval of the description value for the trackers during the getWarningDescriptions function.
• Updated warningsHelper.js by adding the description value so filterWarnings returns it
• Updated currentWarnings.js, with adding the description value with a default value and orderSequence as a unique constraint to prevent concurrent requests of tracker creation.
  …

How to test:

1. check into current branch
2. do npm install and ... to run this PR locally
3. Follow steps on the linked frontend PR

Note:

I know I left some comments in the handling of the concurrent creation requests in currentWarningsController.js, but I'll remove them later if more changes are needed besides them, hopefully for those that can better test creating multiple trackers at the same time than me as I was unable, can find them a bit helpful in telling if it worked or not.

[AaditTrivedi]

Reviewed by Aadit Trivedi.

Reviewed the changes to currentWarningsController.js, warningsController.js, and the currentWarnings model alongside frontend PR #5588, and ran the controller tests.

Verified

• postNewWarningDescription saves newWarningDescription as description on new trackers.
• The list query projects description and filterWarnings returns it, so the frontend receives it.
• The model adds description with a default of "No description provided yet."
• npm test -- src/controllers/currentWarningsController.spec.js: [result]. Screenshot below.

Pointers, not blocking

1. checkWarningDescriptions now runs updateMany to fill in missing descriptions every time the warnings cache is refreshed, which puts a database write inside a read path. A one-time migration script would be cleaner.
2. An empty string from the frontend is saved as '' rather than falling back to the default, since Mongoose defaults only apply to undefined values. Consider treating an empty description as missing.
3. The PR also assigns order to new trackers (last order + 1). That looks like a sensible fix, but it is not mentioned in the description.

Evidence
Test run:

Approving.

[vidiyala99]

Tested with frontend #5588 (see the XSS screenshot on that PR). The XSS starts here: postNewWarningDescription saves newWarningDescription straight from req.body with no validation.

1. Validate newWarningDescription on the server: check it's a string, trim it, cap its length (for example 500 characters), and strip HTML before saving. The repo already has src/utilities/htmlContentSanitizer.js (built on sanitize-html, currently used only for email templates), which could be reused here.
2. Only save the description when it's provided: ...(desc?.trim() && { description: desc.trim() }). An explicit '' overrides the schema default, so new trackers without a description behave differently from old ones.
3. updateMany inside checkWarningDescriptions: a database write now runs on the read path every time the warnings cache is outdated. The schema default covers new documents, so a one-time migration script for existing ones would be cleaner.
4. Edge case in the order calculation: lastWarning.order + 1 gives NaN if the highest-sorted document has no order field. (lastWarning?.order ?? -1) + 1 is safer.
5. (Pre-existing, not from this PR) deleteWarningDescription ends with return res.status(200); without .send(), so delete requests never get a response and time out. I hit this while cleaning up my test trackers. res.status(200).send(...) would fix it, here or in a follow-up.

[adit24dhaya]

Hi Anthony, I reviewed this with frontend #5588 and focused on a separate issue from the current comments. I found one additional ordering edge case and left the details inline: concurrent tracker creations can receive the same order. My isolated controller test started two creates while the current maximum was 7, and both records were saved with order 8. Equal values make later { order: 1 } sorting ambiguous.

All 23 existing current-warning controller tests passed, as did the independent concurrency check (24 tests total). Changed-file ESLint had zero errors and seven pre-existing warnings in warningsController.js; the Babel build compiled 773 files successfully. The concurrency test used the actual controller with a stubbed model and permission helper, so it made no database writes. I did not repeat the description-sanitization and read-path findings already documented by other reviewers.

Requesting changes for order allocation. An atomic counter/update, or a uniqueness constraint with retry, would prevent duplicate positions under concurrent submissions.

[adit24dhaya]

This read-then-increment is not atomic. I ran two concurrent creates that both read a maximum order of 7, and both saved order 8. Since the frontend Add action has no in-flight guard, a double submission can reach this path. Could order allocation be atomic, or protected with a unique constraint and retry?

[Niket07pathak]

Reviewed by Niket Pathak

How I verified

Reviewed backend PR #2369 and its integration with frontend PR #5588. Examined the changes in currentWarningsController.js, warningsController.js, currentWarnings.js, and currentWarningsController.spec.js, focusing on tracker description persistence, default values, order assignment, existing tracker compatibility, and unit test coverage.

Verified working

• The controller accepts and saves the new warning tracker description.
• New tracker creation includes an order field calculated from existing tracker records.
• The implementation assigns order 0 when no previous tracker exists.
• The Mongoose schema includes a default description of "No description provided yet".
• Existing trackers missing the description field are handled through an updateMany() operation.
• The warning tracker retrieval logic includes descriptions in the returned data.
• The updated unit test mocks findOne() to support the new order calculation.

These observations are based on code inspection; backend execution and unit test results were not independently verified.

Issues found

1. Potential Duplicate Order Values During Concurrent Tracker Creation

In currentWarningsController.js, the new order is calculated using findOne().sort({ order: -1 }) followed by incrementing the last order.

Expected behavior: Newly created trackers should receive consistent order values without unintended collisions.

Potential issue: If two trackers are created simultaneously, both requests could retrieve the same last order and assign identical values because the calculation is not atomic.

2. Empty or Null Descriptions May Bypass the Default Value

The controller directly assigns newWarningDescription to the description field.

Expected behavior: Missing or empty descriptions should follow the intended default-value or validation behavior.

Potential issue: Mongoose defaults do not apply to explicitly empty strings or null values, so these could be stored instead of "No description provided yet".

3. Missing Order Values in Historical Tracker Records

The order calculation assumes the previously retrieved tracker contains a valid numeric order value.

Expected behavior: Tracker creation should handle older records with missing or invalid order values safely.

Potential issue: If the selected record has no valid order value, incrementing it could produce an invalid result.

4. Insufficient Unit Test Coverage for Order Calculation

The updated test mocks findOne() returning null, covering the empty collection scenario.

Expected behavior: Unit tests should also verify order calculation when existing trackers are present and when historical records contain missing order values.

Current implementation: The provided test changes do not demonstrate coverage for these additional scenarios.

Evidence

Code references:

• currentWarningsController.js — order calculation and description assignment.
• currentWarnings.js — description default.
• warningsController.js — historical description updates.
• currentWarningsController.spec.js — updated findOne() mock.

Required action

Please:

1. Review the order-generation logic for concurrent tracker creation and consider an atomic approach if unique order values are required.
2. Add explicit handling or validation for empty and null descriptions.
3. Ensure historical trackers without valid order values are handled safely.
4. Extend unit tests to cover existing tracker orders, missing order values, and description edge cases.
5. Verify that these changes preserve existing tracker functionality.

Request Changes

Happy to re-review once updated.

[RichaSapre]

I reviewed the backend description persistence, default values, and tracker ordering logic. I found:

• Descriptions are saved without server-side validation or sanitization, enabling the stored-XSS issue.
• Empty or null descriptions can bypass the schema default.
• The read-then-increment order logic can assign duplicate order values during concurrent tracker creation.
• Missing or invalid historical order values may produce NaN.
• updateMany() performs database writes during the normal read/cache-refresh path.
• SonarQube reports 57.1% coverage on new code, below the required 80%.

[sai-velagala-swe]

PR #2369 – Warning Tracker Description Support

Reviewed by Sai Manojna Velagala

How I Tested

Checked out backend PR #2369 alongside frontend PR #5588 and tested the Warning Tracker functionality locally. Reviewed the backend changes in currentWarningsController.js, warningsController.js, and the currentWarnings model, focusing on description handling, tracker ordering, and data retrieval.

Verified Functionality

• Tracker Creation: Successfully created new trackers with custom descriptions.
• Description Handling: Custom descriptions appeared correctly in their respective InfoModals.
• Existing Trackers: Existing tracker information remained accessible through the Dashboard.
• Frontend-Backend Integration: The newly added description field was correctly reflected in the frontend.
• Schema Integration: The backend model includes a default description, and the retrieval logic returns descriptions with tracker information.

Automated Testing

• Executed npm test -- src/controllers/currentWarningsController.spec.js.
• Results: 23/23 tests passed, with no failures.

Code Review Observations

1. Potential Duplicate or Invalid Tracker Order Values

File: src/controllers/currentWarningsController.js – Lines 82–83

The controller retrieves the tracker with the highest order using findOne().sort({ order: -1 }) and calculates the next order by incrementing lastWarning.order.

Since retrieving and incrementing the order are separate operations, two simultaneous tracker creation requests could read the same maximum value and assign identical order numbers. Additionally, if a historical tracker has an undefined order value, the calculation could produce NaN.

Suggested Fix: Consider atomic order assignment to prevent conflicts during concurrent requests, along with a fallback for missing or invalid order values. Regression tests covering concurrent creation and historical tracker records would help validate the correction.

2. Empty Description Values Bypassing the Default

File: src/controllers/currentWarningsController.js – Line 90

The controller assigns newWarningDescription directly to the description field when creating a tracker. Since Mongoose defaults do not replace explicitly provided empty strings, a blank description may be stored instead of the intended "No description provided yet" value.

Suggested Fix: Normalize and validate incoming description values before saving. Empty or whitespace-only descriptions should either trigger validation or consistently use the default description. This would prevent inconsistent information being returned for newly created and existing trackers.

Additional Observation – Database Update Efficiency

File: src/controllers/warningsController.js – Lines 28–36

The checkWarningDescriptions() function calls updateMany() during warnings cache refresh to populate missing descriptions. Even when no records require modification, this still issues a database update operation as part of the retrieval flow.

Suggested Improvement: Consider moving the historical description updates into a one-time migration to avoid repeated database operations during normal retrieval.

Screenshots

Backend Automated Tests**

Conclusion

The main tracker-description functionality works as expected during local integration testing, and all 23 backend unit tests passed.

However, the current implementation has potential data-consistency issues involving tracker order allocation and empty descriptions. These should be addressed to ensure reliable behavior across concurrent requests and edge cases.

Request Changes: Please address the tracker-ordering and description-validation concerns, with appropriate regression coverage. The database update approach is also worth optimizing to avoid unnecessary operations during retrieval.

[aaryashah22]

Tested locally on 12446e60 with the frontend from OneCommunityGlobal/HighestGoodNetworkApp#5588 (5c114e9d) (Chrome, 10 Oct): Administrator, Owner and Volunteer.

Works

• Creating my test tracker with a description: POST /api/currentWarnings returned 201, the tracker was at the bottom of the list after reopening +/-, and its "i" showed exactly my description (picture 1).
• An existing tracker without its own description (Not Logging Time To Tasks) showed "No description provided yet".
• The test tracker was deleted afterwards and was gone after a refresh.

Side note (not from this PR): right after Add, the +/- list showed the trackers in a different order until I reopened it. The create response is currentWarnings.find({}) without the sort by order. Display only.

Not tested: what the server does with HTML, empty or spaces-only descriptions, two trackers created at the same time, a top tracker without an order, and the updateMany write when the list loads (all raised by other reviewers); local tests.

GitHub: 1 commit; SonarCloud Quality Gate failed (57.1% coverage on new code, 80% required, as RichaSapre noted), the other 3 checks passed. Requesting changes for the quality gate: the setup instructions say quality gate issues must be fixed before a PR is ready to merge. Everything I tested above worked. My dark-mode change requests are on OneCommunityGlobal/HighestGoodNetworkApp#5588.

[adit24dhaya]

Hi Anthony, the unique-index/retry approach works once the index exists, but I found an upgrade problem with older trackers. Two records missing orderSequence make the new unique index fail with E11000. Without the index, concurrent creates still both save order 0. Please backfill and verify the index before accepting creates; details are inline.

Validation: 28 controller tests passed. In a disposable local MongoDB 7.0.14 database, I reproduced the failed index build and two HTTP 201 responses with duplicate order 0. As a control, backfilling first and creating the index gave distinct orders 2 and 3. Synthetic records only; the test database was stopped and removed. No shared database writes. Babel build passed; changed-file lint had 0 errors and 8 warnings.

Commit audit: several subjects are long and past tense, with no explanatory body or issue reference. Please use shorter imperative subjects for future commits. Paired frontend: #5588.

[adit24dhaya]

Older trackers do not have orderSequence. With two such records, creating this unique index fails with E11000 on the missing/null key. The backfill currently runs only when GET /currentWarnings is called, after model initialization, and does not rebuild the failed index. I reproduced two concurrent creates both saving order 0 with HTTP 201 after that failed build. Could the upgrade backfill and verify the index before accepting creates? A control with backfill before index creation returned distinct orders 2 and 3.

[iAbhi001]

Tested this locally alongside frontend #5588. The main feature and tests look solid, but there are a few blockers that still need addressing:

1. The new unique index on orderSequence breaks on existing databases. Because older trackers don't have that field, Mongo throws an E11000 duplicate key error on null, the index fails to build, and the concurrency protection drops completely. Adding sparse: true or handling the backfill beforehand should fix it.
2. The database write inside the warnings check is still running on the read path. That backfill really belongs in a one-time migration rather than firing on list retrieval.
3. Empty or whitespace-only descriptions bypass the schema default and save as blank strings instead of using the fallback text.
4. Tracker deletion hangs because the response status is set without actually sending or closing the response.

Once those edge cases are sorted out, this should be good to go.

[anushkaadwivedii]

Reviewed backend PR #2369 locally at commit 6b2c587 using Node 20.19.4 and a disposable MongoDB database. All 28 focused tests pass. The API sanitizes descriptions, validates input, backfills legacy fields, and rejects a Volunteer without the required permission. However, the new unique orderSequence index fails with E11000 when legacy records lack that field; the later GET backfill does not rebuild it, and concurrent creates then receive duplicate sequence values. Please migrate legacy values before building the index or use an appropriate partial unique index. Creating a tracker also leaves the warmed warnings cache stale until its outdated flag is explicitly set; please restore invalidation after saving. Finally, simultaneous requests for the same title create duplicate trackers even with the sequence index active. Please enforce normalized-title uniqueness across retries or at the database layer. Requesting changes.

[AnthonyWeathers]

Tested this locally alongside frontend #5588. The main feature and tests look solid, but there are a few blockers that still need addressing:

1. The new unique index on orderSequence breaks on existing databases. Because older trackers don't have that field, Mongo throws an E11000 duplicate key error on null, the index fails to build, and the concurrency protection drops completely. Adding sparse: true or handling the backfill beforehand should fix it.
2. The database write inside the warnings check is still running on the read path. That backfill really belongs in a one-time migration rather than firing on list retrieval.
3. Empty or whitespace-only descriptions bypass the schema default and save as blank strings instead of using the fallback text.
4. Tracker deletion hangs because the response status is set without actually sending or closing the response.

Once those edge cases are sorted out, this should be good to go.

Got it, will work on trying to resolve these. The script thing, I can probably do, but I'd like your opinion regarding it. I assume I mostly would just be moving the functions to add the missing columns to legacy trackers to a separate file, meant as a script only. But the thing I'm unsure about, is how would I call it? I saw one script already in the repo where someone made a unique url/endpoint you could call/trigger on the app to call the script. Should I do something like that, or what would be the console command I can use to run the script manually on the backend only?

[AnthonyWeathers]

Hi Anthony, the unique-index/retry approach works once the index exists, but I found an upgrade problem with older trackers. Two records missing orderSequence make the new unique index fail with E11000. Without the index, concurrent creates still both save order 0. Please backfill and verify the index before accepting creates; details are inline.

Validation: 28 controller tests passed. In a disposable local MongoDB 7.0.14 database, I reproduced the failed index build and two HTTP 201 responses with duplicate order 0. As a control, backfilling first and creating the index gave distinct orders 2 and 3. Synthetic records only; the test database was stopped and removed. No shared database writes. Babel build passed; changed-file lint had 0 errors and 8 warnings.

Commit audit: several subjects are long and past tense, with no explanatory body or issue reference. Please use shorter imperative subjects for future commits. Paired frontend: #5588.

Thanks for the review, gonna try to solve this, this is serving as a good challenge for me to figure this out. For the commit audit stuff though, I am not fully following your explanation. I understand I should be making the commit msgs smaller, but I wasn't aware i could add a body or issue reference, how do I do that? I'll try to improve, but as an additional query, should I while attempting to address the different points you and others make, and commit a fix for each separately, or I can do a fix for multiple but make a concise commit msg as a general fix?

[AnthonyWeathers]

Reviewed backend PR #2369 locally at commit 6b2c587 using Node 20.19.4 and a disposable MongoDB database. All 28 focused tests pass. The API sanitizes descriptions, validates input, backfills legacy fields, and rejects a Volunteer without the required permission. However, the new unique orderSequence index fails with E11000 when legacy records lack that field; the later GET backfill does not rebuild it, and concurrent creates then receive duplicate sequence values. Please migrate legacy values before building the index or use an appropriate partial unique index. Creating a tracker also leaves the warmed warnings cache stale until its outdated flag is explicitly set; please restore invalidation after saving. Finally, simultaneous requests for the same title create duplicate trackers even with the sequence index active. Please enforce normalized-title uniqueness across retries or at the database layer. Requesting changes.

Thank you very much for your review. I'll take your points into current list of stuff to look into along with the earlier two, and try to address them. So I could repeat the same tests, how do you do the tests that you shared as a screenshot or picture? I'd love to repeat those tests with my attempts to fix it, and see how it compares or if it passes first before I commit and push for new reviews.

[Sripriya03-hub]

Tested this with frontend PR 5588 locally.

• Tracker descriptions come back and show in the info modal
• After creating a new tracker, setOutdatedWarningsFlag is no longer called, so the new tracker won't show in users' tracking lists until the server restarts
• The unique index on orderSequence will fail to build on a database where older trackers don't have that field yet
• The console logs in the retry loop are still there

Requesting changes.

[rahulrubugunday]

I have reviewed your PR locally (6b2c587, frontend on #5588), logged in as a volunteer.

1. The user's warning trackers load from the backend and each one opens its info modal on the frontend.
2. Trackers without a saved description come back as "No description provided yet".

Requesting changes along with the frontend PR #5588, where the info icon alignment needs fixing.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
98.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[vidiyala99]

Re-reviewed at head 524f17a. My 09/29 points are all addressed:

1. The description is now checked on the server (must be a string, 500 characters max, trimmed, run through cleanHtml).
2. The schema default is only overridden when a description is actually given.
3. The updateMany on the read path is gone, moved into src/scripts/updateLegacyCurrentWarnings.js.
4. The NaN order case is gone ((lastWarning?.orderSequence ?? -1) + 1).
5. deleteWarningDescription now sends a response.

currentWarningsController.spec.js passes (28 of 28), and Lint, SonarCloud and the coverage job pass.

How I tested
Besides the spec, I wrote a proof test that runs the real controller and model against an in-memory MongoDB (no shared dev data). I seeded three trackers the way they exist on dev today, with warningTitle and order only, so no normalizedWarningTitle and no orderSequence.

Requesting changes: the PR depends on the migration script, but nothing says it has to run
Until updateLegacyCurrentWarnings.js runs on a database, the new code misbehaves on the existing trackers:

• Duplicate trackers. The duplicate check now queries normalizedWarningTitle, which the existing trackers don't have, and the unique index is sparse, so it skips them too. Posting "Log Time to Tasks" when that tracker already exists returned 201, and the collection then had two "Log Time to Tasks" trackers.
• New trackers jump to the top. orderSequence is computed from the highest existing orderSequence, and none exist yet, so a new tracker got order: 0, the same as the first existing tracker. getCurrentWarnings no longer backfills order, so nothing fixes it later.

After I ran the script's logic (set orderSequence and normalizedWarningTitle on every record), both were fine: the duplicate returned 400, and the new tracker got order: 3.

So the script is required, not optional. To answer your question in the 10/25 comment about how to call it, either of these would work:

• Document it as a required deploy step: add an npm script (for example "migrate:warnings": "node src/scripts/updateLegacyCurrentWarnings.js") and a "Deploy steps" section in the PR description, so whoever merges knows to run it on dev and prod right after deploying.
• Or make the code safe without it: fall back to a case-insensitive warningTitle match in the duplicate check, and compute the new order from the highest order when no orderSequence exists yet. The script then becomes a cleanup, not a requirement.

Non-blocking

• Merge order with HighestGoodNetworkApp#5588. postNewWarningDescription now returns 400 "Description must be a string" when newWarningDescription is missing, and the frontend on development doesn't send it. If this merges before the frontend, adding a tracker breaks. Either merge them together, or treat a missing description as ''.
• HTML in descriptions. cleanHtml uses sanitize-html's default tag list, so <b>bold</b><img src=x onerror=alert(1)><script>x</script> is stored as <b>bold</b><img src="x" />. The script and event handler are removed, which is good. But the modal shows plain text, so stripHtml from the same file (already used for plain-text fields) fits better and won't store markup.
• updateLegacyCurrentWarnings.js builds the URI with user unencoded, while src/startup/db.js encodes both user and password. Matching db.js avoids a failure if the user name has special characters.

The proof test is short. I can paste it here if it helps.

[AnthonyWeathers]

The proof test is short. I can paste it here if it helps.

Thanks for the review, currently working on another PR, for fixup from past reviews, but again your input is very appreciated for ensuring I don't miss stuff out.

And yes for the proof test if it's not too much trouble, any help is appreciated for reference or understanding on my part.