Skip to content

Sireesha taking over for Linh - Add multi-select and bulk actions to Materials table _be - #2178

Open
linh2020 wants to merge 8 commits into
developmentfrom
linh_bulk_actions_materials_backend
Open

Sireesha taking over for Linh - Add multi-select and bulk actions to Materials table _be#2178
linh2020 wants to merge 8 commits into
developmentfrom
linh_bulk_actions_materials_backend

Conversation

@linh2020

@linh2020 linh2020 commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

Description

image

Please include the exact bug/functionality description and a summary of the changes/ related issues. Please also include any other relevant motivation and context:
Adds backend persistence for new bulk material actions so frontend actions are not local-only and survive refresh/reload.

Fixes # (bug list priority high/medium/low x.y.z)
Fixes #MEDIUM-bmdashboard-materials-bulk-actions

Or Implements # (WBS)
Implements #WBS-BM-Materials-Bulk-Actions

Related PRS (if any):

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

Main changes explained:

Update buildingMaterial.js to add persistent fields:

stockHold (Boolean)
isReviewed (Boolean)
notes (String)
Update bmMaterialsController.js to add bmApplyMaterialBulkAction:

validates materialIds and action
supports actions: hold, review, notes
updates selected materials via updateMany
returns applied-record count summary
Update bmMaterialsRouter.js to expose:

POST /bm/materials/bulk-actions

How to test:

check into current branch (frontend + backend PR branches)
backend:
cd backend/HGNRest
npm install
npm run dev (or project-standard start command)
frontend:
cd frontend/HighestGoodNetworkApp
npm install
npm run dev
Clear site data/cache
log as admin user
go to /bmdashboard/materials
verify:
row checkboxes appear and select-all works
rows are highlighted when selected
Bulk Actions is disabled with 0 selected rows
select multiple rows and run Mark as Hold, then confirm Hold column/status reflects persisted value after refresh
select multiple rows and run Mark as Reviewed, then confirm persisted value after refresh
run Add/Update Notes and confirm status chip appears and persists after refresh
run Export Selected (CSV/PDF) and confirm file contents are for selected items only
verify this new feature works in dark mode
verify responsive behavior at mobile/tablet widths (bulk bar wraps correctly and remains usable)

Screenshots
Screenshot 2026-06-12 at 11 06 14 PM

Screenshot 2026-06-12 at 11 04 56 PM

… hold/review/notes

- extend buildingMaterial schema with persistent bulk-action fields:
  - stockHold (Boolean, default false)
  - isReviewed (Boolean, default false)
  - notes (String, default empty)
- add controller handler bmApplyMaterialBulkAction to process bulk material updates
- support bulk actions:
  - hold
  - review
  - notes
- validate request payload (materialIds, action, notes requirements)
- apply updates via updateMany and return affected count in response
- expose new route:
  - POST /bm/materials/bulk-actions

Files updated:
- backend/HGNRest/src/models/bmdashboard/buildingMaterial.js
- backend/HGNRest/src/controllers/bmdashboard/bmMaterialsController.js
- backend/HGNRest/src/routes/bmdashboard/bmMaterialsRouter.js
@sonarqubecloud

Copy link
Copy Markdown

@one-community one-community added the High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible label Apr 24, 2026
@sonarqubecloud

Copy link
Copy Markdown

@handikaharianto handikaharianto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, i just reviewed this PR and here are my findings:

Worked well

No bulk action results in bad request✅

When action property is not passed, the API endpoint returns a 400 Bad Request, which is a proper response to this scenario.
Image

Requires some updates

Not so helpful message❌

The following image shows that it returns a result properties that says "Applied 'hold' to undefined material records". Showing undefined to the users will confuse them.
Image

@handikaharianto

Copy link
Copy Markdown
Contributor

hi @linh2020 Tested POST /bm/materials/bulk-actions together with the related frontend implementation.

Bulk actions execute successfully, however the expected persisted fields could not be verified after execution.

Observed:

  • Response reports "Applied 'hold' to undefined material records."

  • GET /bm/materials does not appear to return:

    • stockHold
    • isReviewed
    • notes
  • Database records do not appear to contain these fields.

Please verify:

  1. buildingMaterial schema contains stockHold, isReviewed, and notes.
  2. bmApplyMaterialBulkAction updates these fields correctly.
  3. GET /bm/materials returns these fields and they are not excluded by select/projection logic.

Requesting changes pending verification of persistence and retrieval behavior.

I also agree with this. Could you clarify on this matter? @linh2020

@pixelpix13 pixelpix13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hi @linh2020
Tested POST /bm/materials/bulk-actions together with the related frontend implementation.

Bulk actions execute successfully, however the expected persisted fields could not be verified after execution.

Observed:

  • Response reports "Applied 'hold' to undefined material records."
  • GET /bm/materials does not appear to return:
    • stockHold
    • isReviewed
    • notes
  • Database records do not appear to contain these fields.

Please verify:

  1. buildingMaterial schema contains stockHold, isReviewed, and notes.
  2. bmApplyMaterialBulkAction updates these fields correctly.
  3. GET /bm/materials returns these fields and they are not excluded by select/projection logic.

Requesting changes pending verification of persistence and retrieval behavior.

@AmaanSyed09 AmaanSyed09 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested backend PR #2178 together with frontend PR #4680.

I completed end-to-end testing of the bulk material actions using both the frontend and backend branches.

The primary issues observed during testing appear to originate from the backend responses and/or persistence layer.

  1. The bulk action response returns an incorrect update count.

After executing Hold, Reviewed, and Add Notes actions, the frontend displayed success messages such as:

Applied 'hold' to undefined material records.
Applied 'review' to undefined material records.
Applied 'notes' to undefined material records.

Expected behavior:
The backend should return the correct number of updated material records so the frontend can display an accurate success message.

  1. The applied bulk actions could not be verified after refreshing the Materials page.

After applying Hold, Reviewed, and Notes, I refreshed the page and searched for the same material by PID. The Bulk Status remained unchanged, and the applied note could not be verified through the frontend.

Please refer to my frontend review on PR #4680 for the complete testing summary, screenshots, and detailed observations regarding the UI behavior.

Requesting changes until the backend response and persistence behavior are verified.

@akv-iu akv-iu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Findings and Next Steps

What I Found:

  • The bulk action requests complete successfully, no info if updated fields are actually persisted afterward.
  • The response message reads "Applied 'hold' to undefined material records."
  • GET /bm/materials doesn't appear to return notes for the affected records.

Areas to Investigate:

  1. bmApplyMaterialBulkAction is updating the right fields correctly.
  2. GET /bm/materials is returning these fields, or if a select excluding them.

Next Steps:

  • Requesting changes until persistence and retrieval can be confirmed for these fields.
Image Image

@AnshShah-18 AnshShah-18 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tested frontend PR #4680 locally together with backend PR #2178 as an Administrator.

Working as expected

  • Individual row checkboxes work correctly.
  • The Select All checkbox selects all visible rows.
  • Selected rows are highlighted clearly.
  • The selected-item count updates correctly.
  • Bulk Actions is disabled when no materials are selected and enabled after selection.
  • CSV export downloads only the selected materials.
  • PDF export downloads only the selected materials.
  • Exported table data is readable.

Issues found

  1. Bulk-action success messages show an undefined count
    All three server-backed actions display an incorrect success message:
  • Applied 'hold' to undefined material records.
  • Applied 'review' to undefined material records.
  • Applied 'notes' to undefined material records.
    The messages should display the actual number of updated materials.
  1. Mark as Hold is not reflected in Bulk Status

After applying Mark as Hold:

  • The Bulk Status remains -.
  • The On Hold chip does not appear.
  • Refreshing the page does not update the status.
  1. Mark as Reviewed is not reflected in Bulk Status
    After applying Mark as Reviewed:
  • The Bulk Status remains -.
  • The Reviewed chip does not appear.
  • Refreshing the page does not update the status.
  1. Add/Update Notes is not reflected in Bulk Status
    After adding a note:
  • The Bulk Status remains -.
  • The Has Note chip does not appear.
  • Refreshing the page does not update the status.

@CarolineYXW CarolineYXW left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello, I tested this PR together with #4680 locally following the PR description.

Existing functionality such as searching, filtering, and sorting continues to work as expected. However, I encountered several issues with the new bulk actions feature:

  • Mark as Hold does not work. The request fails and the backend returns Cannot POST /api/bm/materials/bulk-actions.
  • Mark as Reviewed does not perform any action.
  • Add/Update Notes does not work.

The issues exist in both light and dark mode.

Image Image Image

@sitaram1921 sitaram1921 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested locally with frontend PR #4680 using an Admin account. Same issues prior reviewers found still exist: bulk action success messages show undefined instead of the actual count, and status does not persist after refresh. Root cause for the count issue: bmApplyMaterialBulkAction uses result.modifiedCount from updateMany, but this project runs Mongoose 5.13.23 where updateMany returns nModified, not modifiedCount, so it's always undefined. Should use result.nModified instead.

Screenshot (48) Screenshot (47) Screenshot (46)

@iAbhi001
iAbhi001 requested review from iAbhi001 and removed request for iAbhi001 July 30, 2026 03:20

@AdiDubbs AdiDubbs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This PR was easy to follow - it's good that you're validating the ObjectIds and action enum up front.

The main issue is that the undefined count reviewers keep hitting: this project is on Mongoose 5.13.23, so updateMany returns nModified rather than modifiedCount. @sitaram1921 traced this on 24 July and it's still open.

The endpoint also has no permission check despite being able to bulk-modify any materials, and the package-lock changes look unrelated.

);

return res.status(200).send({
result: `Applied '${action}' to ${result.modifiedCount} material records.`,

@AdiDubbs AdiDubbs Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This project uses Mongoose 5.13.23, where updateMany returns nModified rather than modifiedCount. This therefore returns "undefined material records," which is the issue other reviewers have reproduced in the paired frontend.

Please use nModified, or a compatibility fallback if this endpoint needs to support a future Mongoose upgrade.


materialsRouter.route('/updateMaterialRecordBulk').post(controller.bmPostMaterialUpdateBulk);

materialsRouter.route('/materials/bulk-actions').post(controller.bmApplyMaterialBulkAction);

@AdiDubbs AdiDubbs Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This can bulk-modify any materials but has no permission check - any authenticated user can call it. Most controllers here gate with hasPermission(req.body.requestor, ...). I think this might need to be restricted.

@kunchalasireesha kunchalasireesha Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The bulk-actions endpoint sits behind the same JWT auth as the rest of /api/bm. None of the existing materials write endpoints (bmPurchaseMaterials, bmPostMaterialUpdateBulk, bmupdatePurchaseStatus) use hasPermission, and there's no materials permission string in the seed yet. Adding a proper manageMaterials permission across the BM controllers is worth doing as its own PR rather than bolting it onto just this endpoint.

Comment thread package-lock.json Outdated
"node_modules/@babel/core": {
"version": "7.28.5",
"license": "MIT",
"peer": true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These peer/dev marker changes look like npm version churn rather than anything intentional - worth dropping from the diff.

@sawantgayatri19 sawantgayatri19 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for working on this! To get this PR ready to merge, please address the following key action items raised during review:

Fix Count in Success Message (undefined issue): Update bmApplyMaterialBulkAction to use result.nModified instead of result.modifiedCount, since Mongoose 5.13.23 returns nModified.

Fix Endpoint Route & Persistence: Ensure the route matches correctly (to resolve the Cannot POST /api/bm/materials/bulk-actions error) and verify that the database correctly persists and returns stockHold, isReviewed, and notes fields.

Add Permission Checks: Secure the new bulk-actions endpoint with appropriate permission validation (e.g., using hasPermission).

Clean Up Diff: Remove the unrelated version/peer-marker changes in package-lock.json.

@ShreyasB02 ShreyasB02 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested the following functionality locally. The previous issues mentioned regarding the hold/review/notes still persists.

  • Verified row checkboxes are displayed and the Select All functionality works as expected.
  • Confirmed selected rows are highlighted correctly.
  • Verified the Bulk Actions button remains disabled when no rows are selected.
Screenshot
  • Tested Mark as Hold with multiple selected rows.

    • The hold status is not persisted after a page refresh.
    • The success toast displays "undefined material" even though the material is defined.
Screenshot 2026-08-14 at 10 25 26 AM
  • Tested Mark as Reviewed with multiple selected rows.

    • The reviewed status is not persisted after a page refresh.
    • The success toast displays "undefined material" even though the material is defined.
Screenshot 2026-08-14 at 10 27 06 AM
  • Tested Add/Update Notes.

    • The success toast displays "undefined".
    • Notes do not persist after refreshing the page, and the status chip is not retained.
Screenshot 2026-08-14 at 10 29 05 AM
  • Verified Export Selected (CSV/PDF) exports only the selected items.
Screenshot 2026-08-14 at 10 33 39 AM
  • Verified the new bulk actions feature renders correctly in dark mode, with all text and UI elements remaining visible.
  • Verified responsive behavior across iPhone, iPad, and Samsung S20 Ultra viewports. The bulk action bar wraps correctly and remains fully usable.
Screenshot 2026-08-14 at 10 41 47 AM

My thoughts on the issue:
I traced this to the backend bulk action flow for materials and not the schema itself. The model already includes stockHold, isReviewed, and notes, and the route is wired correctly, but the bulk-action controller in bmApplyMaterialBulkAction in bmMaterialsController.js is the likely issue: it validates the payload and writes the updates, yet it returns inconsistent response/error payloads, which explains the undefined toast messages and the fact that the UI does not reflect persisted values after refresh. This looks like a mismatch between the frontend expectation and the backend response contract rather than a missing DB field.

@Adit0717 Adit0717 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested this locally on the mentioned frontend and backend branches. Found some issues related to it.,

  1. Fields missing in DB documents - The stockHold, isReviewed, and notes fields don't exist in documents in the buildingMaterials collection.

  2. 404 on page load When the page loaded, the browser made a request to GET /api/ownerMessageLogs?page=1&limit=10 and got a 404. Noticed this error while loading the page.

Image

@DeepighaJ DeepighaJ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The toast displays undefined instead of showing the actual number of records applied.

Image Image Image Image

@linlin-husky linlin-husky left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I tested the code on this branch for back end #2178 together with front end 4680 using admin account

As shown in Image 2, the endpoint is registered on the backend (port 4500) and handles auth as expected.

In Image 1, the frontend fails with a 404 because the request is sent to the Vite server (port 5173) instead of targeting the backend API.

In Image 3 (line 264 of bmMaterialsController.js), the response reads result.modifiedCount.

Because the project uses Mongoose 5.13.23, updateMany returns nModified instead of modifiedCount, evaluating to undefined.

This causes the success toast to display "Applied '...' to undefined material records" instead of the actual count.
image 1
Image

image 2
Image

image 3
Image

@MahalakshmiPFW MahalakshmiPFW left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey! I am unable to run the backend correctly. I have tried npm run dev and npm start commands and im getting these errors (see screenshots below). Please let me know if I'm doing it the wrong way or if there's some error in the code. Thanks!

Image Image

@one-community one-community changed the title Linh - Add multi-select and bulk actions to Materials table _be Sireesha taking over for Linh - Add multi-select and bulk actions to Materials table _be Sep 9, 2026
kunchalasireesha and others added 3 commits September 9, 2026 23:16
updateMany with Mongoose 5 / mongodb driver 3.x resolves to
{ ok, n, nModified }, so result.modifiedCount was undefined and the
response read "Applied 'hold' to undefined material records".

Read the count from nModified (with matchedCount/modifiedCount and a
0 fallback for driver-version safety) and also return matchedCount and
modifiedCount as structured fields. Restore mongoose.Types.ObjectId in
the bmPurchaseMaterials suite so it stops leaking a stub into later
suites, and cover the new endpoint.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DxAxzwAyo6STgnbVid1q7Q
bmMaterialsList runs BuildingMaterial.find() with no projection and no
.lean(), and the schema declares no `select: false`, so the three new
fields are already returned (defaults hydrate legacy documents). Add a
regression test so a future .select() cannot silently drop them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DxAxzwAyo6STgnbVid1q7Q

@shubhamjakhete shubhamjakhete left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi,

Tested the backend directly against the dev database rather than through the UI.

updateMany matches zero documents for valid material ids.

Using an id taken straight from GET /api/bm/materials:

POST /api/bm/materials/bulk-actions with {"action":"hold","materialIds":["66d03c80fee22b4fb08b313f"]} returns {"matchedCount":0,"modifiedCount":0,"result":"Applied 'hold' to 0 material records."}
Re-reading the same record shows stockHold, isReviewed and notes unchanged
The document is present in the same response the id came from
A second id from the same list behaves identically, so this is not specific to one record

materialIds passes the ObjectId.isValid check, so the request is accepted and reports success with a zero count. The _id: { $in: materialIds } filter receives strings, which may not be cast the way find() casts them here.

This looks like the root cause behind the "action succeeds but the status never persists" reports on this PR and #4680. The count now reads 0 rather than undefined because the nModified fallback is in place, but nothing is being written.

The tests do not cover this. updateMany is mocked, so the bulk action tests only exercise the count-mapping logic against fabricated driver responses. The filter itself is never executed against a real document.

Verified working
nModified / modifiedCount fallback handles the Mongoose 5 result shape
Schema adds stockHold, isReviewed and notes with defaults

@muthapriyanka muthapriyanka left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please look at the comment on the PR OneCommunityGlobal/HighestGoodNetworkApp#4680 (review)
Just to summarize the backend api is not functioning as required because it return the as undefined when the bulk action is applied to multiple rows.

Screenshot 2026-09-11 200243 Screenshot 2026-09-11 200453

Accepts development's rewritten badge-check functions (checkPersonalMax,
checkMinHoursMultiple, checkTotalHrsInCat, checkXHrsForXWeeks,
checkNoInfringementStreak) in src/helpers/userHelper.js, bypassing the
protected-methods pre-commit guard for this merge only, per explicit
confirmation that development's rewrites should be accepted as-is.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QCTDoudERAdEhtyhBiQZZt
kunchalasireesha and others added 2 commits September 12, 2026 20:28
Adds tests for the invalid-id, blank-notes, valid-notes, and update
failure branches of bmApplyMaterialBulkAction that SonarCloud flagged
as uncovered new code on PR #2178 (new_coverage 75.5%, below gate).

Also fixes a pre-existing test that mutated the real
mongoose.Types.ObjectId.isValid in place instead of the mocked
reference, which permanently corrupted ObjectId.isValid for every
test running after it in the same file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QCTDoudERAdEhtyhBiQZZt
MongoDB only counts a document as "modified" when a field's value
actually changes, so re-applying the same bulk action (e.g. holding
an already-held item, or re-testing the same selection) reported
"0 material records" even though the action matched and applied fine.
Use matchedCount, which reflects how many records the action targeted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QCTDoudERAdEhtyhBiQZZt
@sonarqubecloud

Copy link
Copy Markdown

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.