Repository navigation
Mahathi - fix: return project names for supplier performance Backend - #2306
mahathiganimi wants to merge 2 commits into
Conversation
|
Adit0717
left a comment
There was a problem hiding this comment.
Tested the PR locally on Postman. Checked out the mentioned branches.
Tested the "api/suppliers/performance" endpoint with all filter combinations - projectId, startDate, endDate and everything works correctly. Found one minor issue.
Invalid projectId returns a 500 instead of 400 - Not reachable through the current UI, since the dropdown only sends valid project IDs - but the API route itself doesn't validate projectId format before passing it to mongoose.Types.ObjectId(). If it's called directly (e.g. via Postman) with a wrong or invalid ID, ObjectId() throws and it falls to the catch block, returning 500.
It would be more correct to validate projectId and return a 400 with a clear message (e.g. "Invalid projectId").
linlin-husky
left a comment
There was a problem hiding this comment.
Hi @mahathiganimi,
Thank you for your work on this PR! I tested the backend endpoints locally via Postman alongside frontend PR #5448.
Verified Points:
- Project Lookup (
GET /api/suppliers/projects): Successfully returns200 OKwith an array of project objects containing properprojectNameand_idpairs (e.g., "Orientation and Initial Setup", "XiaoFei test project 4").
- Performance Metrics (
GET /api/suppliers/performance): Correctly returns aggregated metrics (e.g.,onTimeDeliveryPercentage) forprojectId=allacross the specified date ranges.
- Required Parameter Check: The endpoint properly returns
400 Bad Requestwith"Missing required query parameters: startDate, endDate"when dates are omitted.
Areas for Improvement / Questions:
- Handling Invalid
projectIdFormat:-
When querying
GET /api/suppliers/performance?projectId=invalid_project_123&startDate=1970-01-01&endDate=2026-08-28, the endpoint returns a500 Internal Server Errordue to an unhandled CastError. -
Could we add a format validation check (such as
mongoose.Types.ObjectId.isValid(projectId)) whenprojectId !== 'all'and return a400 Bad Requestwith an informative error message instead?
-
There was a problem hiding this comment.
Reviewed c4d2837 against the real supplier-performance data on the shared dev database (read-only).
Most supplier projects disappear from the list after this change. GET /api/suppliers/projects on the current backend returns 7 distinct projectIds. I matched them against both project collections:
- 2 exist in the HGN
Projectcollection ("SiddharthTest" and "XiaoFei test project 4"). - 0 exist in the BM building projects (
/api/bm/projects). - The other 5 have no project document at all: 4 are seed placeholders (
507f1f77bcf86cd799439011to...014) and 1 is a real-looking ID that matches neither collection.
The new Project.find({ _id: { $in: projectIds } }) only returns documents that exist, so the dropdown goes from 7 entries to 2. The records for the other 5 IDs are still in SupplierPerformance but can't be selected any more, and nothing tells the user they were dropped. That matches the earlier tester seeing names like "XiaoFei test project 4", which are general HGN test projects rather than building projects.
Two things worth settling:
- Which collection supplier performance should point to. The model says
ref: 'project', but this is a BM dashboard chart. If it's meant to use BM building projects, the lookup (and the seed data) should use that collection instead. - What to do with IDs that don't resolve. Returning them with a placeholder name (for example
Unknown project (…9011)) instead of dropping them, or cleaning up the placeholder seed rows, would keep the dropdown and the data in sync.
Requesting changes so the project source is confirmed before this merges.
c4d2837 to
044d701
Compare
vidiyala99
left a comment
There was a problem hiding this comment.
Re-checked the updated branch (044d701, rebased onto current development) on a test server against the shared dev data (read-only), comparing it with development:
| Endpoint | development |
this PR |
|---|---|---|
GET /api/suppliers/projects |
7 entries, raw IDs only | 3 named projects ("Orientation and Initial Setup", "SiddharthTest", "XiaoFei test project 4") |
GET /api/bm/tools-availability/projects |
stub {"message":"Get unique project IDs"} |
1 building project ("Building 1") |
What's new and works:
projectIdvalidation:GET /api/suppliers/performance?projectId=notanidnow returns400 Invalid projectId.instead of a cast error, andprojectId=allstill returns every supplier.- Tool availability: both routes now reach the real controller instead of the placeholder responses, and the project list has names from
BuildingProject. - The debug
console.logs are gone from the supplier controller.
Still open from my last review:
- Supplier data for unresolved projects is still hidden. Four of the seven project IDs are the seed placeholders (
507f1f77bcf86cd799439011to...014) and still have records:GET /api/suppliers/performance?projectId=507f1f77bcf86cd799439011returns "Supplier A" at 95.8%. BecausegetProjectsWithSupplierDataonly returns IDs found inProject, those projects can no longer be picked in the dropdown, and nothing tells the user. Either keep them with a placeholder name (for example "Unknown project (...9011)") or remove the placeholder seed rows, so the list and the data agree. - Which project collection is right is still worth confirming: supplier performance looks names up in HGN
Project, while tool availability (in this same PR) usesBuildingProject. Both are BM dashboard charts, so one of them is probably pointing at the wrong collection.
Note: src/startup/routes.js mounts toolAvailabilityRouter twice (lines 621 and 624) and also mounts bmToolAvailabilityRoutes under /api. That predates this PR, but it's worth tidying so it's clear which handler serves these paths.
Requesting changes for item 1.
|
vidiyala99
left a comment
There was a problem hiding this comment.
Re-tested the new commit b160af8 on a test server against the shared dev data (read-only GET requests only), together with the frontend OneCommunityGlobal/HighestGoodNetworkApp#5448 merged into current development, as Administrator.
My blocking item is fixed: supplier data for projects that aren't in Project is no longer hidden.
GET /api/suppliers/projectsnow returns all 7 project IDs that have supplier data: the 3 named projects plusUnknown project (…9011)to(…9014)for the 4 seed IDs, sorted by name.- Picking "Unknown project (…9011)" in the Supplier Performance dropdown shows Supplier A at 95.8%, the same as
GET /api/suppliers/performance?projectId=507f1f77bcf86cd799439011(screenshot 1). So the list and the data agree now.
Also checked:
GET /api/bm/tools-availability/projectsreturns[{ projectId, projectName: "Building 1" }]; with the frontend, picking it loads Hammer, Power Drill and Circular Saw in Tools by Availability.projectId=notanidstill returns400 Invalid projectId., andprojectId=allreturns all 4 suppliers.- The two updated suites (
supplierPerformance.test.js,toolAvailabilityController.test.js) pass 27/27.
The Project vs BuildingProject question from my last review is still worth a quick confirmation from whoever owns the BM data, but it isn't blocking.
Approving the backend. (The frontend PR has a separate mobile layout issue, see my review there.)
Screenshot 1: 'Unknown project (…9011)' selected, Supplier A at 95.8% (matches the API)












Description
Related PRS (if any):
To test this backend PR you need to checkout the https://github.com/OneCommunityGlobal/HighestGoodNetworkApp/pull/5448 frontend PR
Main changes explained:
How to test:
Note:
Note that the local pre-commit related-test command may still fail because the unrelated reasonSchedulingController integration tests cannot connect to the local test database; the backend build itself succeeds.