feat: add v2 TypeScript water intake routes (updated)#1093
feat: add v2 TypeScript water intake routes (updated)#1093Soulplayer wants to merge 10 commits intoCodeWithCJ:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new v2 routes and comprehensive tests for water intake management. The review identifies a critical security vulnerability regarding the execution order of permission and impersonation middleware. Additional feedback points out inconsistencies in service layer argument ordering, missing actor IDs for audit logging in update and delete operations, and a recommendation to use standard TypeScript imports for better consistency and type support.
|
/gemini review |
Soulplayer
force-pushed
the
clean-water-intake
branch
from
6e90887 to
4fa0687
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces new Express routes and comprehensive tests for water intake tracking. The review identifies several critical integration issues, specifically parameter mismatches between the route handlers, the service layer, and the repository. These inconsistencies in the update and delete operations, along with missing user identifiers in service-to-repository calls, will cause functional failures and incorrect error responses.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a V2 API for water intake management, featuring new Express routes, service layer enhancements to support "on-behalf-of" functionality for family access, and comprehensive unit and integration tests. It also includes a new documentation file detailing lessons learned regarding security middleware ordering and audit trails. The review feedback highlights several areas for improvement: ensuring consistent parameter ordering (actor vs. target) across service calls to match documented best practices, correcting logically inverted log messages, and fixing documentation that mislabels user ID roles. Additionally, a potential logic issue was identified in the repository interaction that could hinder admin-on-behalf-of operations.
| const entryOwnerId = await measurementRepository.getWaterIntakeEntryOwnerId( | ||
| id, | ||
| authenticatedUserId | ||
| ); |
There was a problem hiding this comment.
The measurementRepository.getWaterIntakeEntryOwnerId method (as seen in the repository file) is currently implemented with a WHERE user_id = $2 clause. This will cause the lookup to fail when an admin is acting on behalf of a user, as it will only search for entries owned by the admin themselves. The repository method should be updated to only filter by id, relying on the RLS context provided by getClient(authenticatedUserId) to handle access control.
There was a problem hiding this comment.
I think it's right, but the rest of the service uses the same pattern. Water intake is not integrated into the family sharing right now. I'd leave it like this and when the feature will be implemented the service needs refinement anyway. Or what do you think @apedley
…arkyFitness into clean-water-intake
… intake routes were created in PR CodeWithCJ#1093 but not mounted in\nSparkyFitnessServer.js, causing /api/v2/measurements/water-intake endpoints\nto return 404 errors.\n\nThis adds the missing route mounting to make V2 endpoints accessible:\n- Import waterIntakeRoutesV2 from routes/v2/waterIntakeRoutes\n- Mount /api/v2/measurements endpoint\n\nAddresses the same critical oversight found in PR CodeWithCJ#1094 that prevented\nV2 routes from working in production despite all tests passing.\n\nGenerated by Mistral Vibe.\nCo-Authored-By: Mistral Vibe <vibe@mistral.ai>
… intake routes were using ES6 'export default' but the\nserver uses CommonJS 'require()', causing 'argument handler must be a function'\nerrors when starting the server.\n\nThis changes the export to use CommonJS 'module.exports = router;' to match\nthe pattern used by other V2 routes (foodRoutes.ts).\n\nGenerated by Mistral Vibe.\nCo-Authored-By: Mistral Vibe <vibe@mistral.ai>
2 participants
Description
What problem does this PR solve?
Adds V2 REST API endpoints for water intake management while maintaining backward compatibility with existing V1 routes.
How did you implement the solution?
Created new
/api/v2/measurements/water-intakeendpoints with comprehensive Zod validation, proper error handling, and complete test coverage. The V2 routes follow the established pattern used by other V2 endpoints in the codebase.Linked Issue: #1073
How to Test
cd SparkyFitnessServer && pnpm testGET /api/v2/measurements/water-intake/entry/:idGET /api/v2/measurements/water-intake/:datePOST /api/v2/measurements/water-intakePUT /api/v2/measurements/water-intake/:idDELETE /api/v2/measurements/water-intake/:id/api/water-containersPR Type
Checklist
All PRs:
New features only:
Backend changes (
SparkyFitnessServer/):rls_policies.sqlfor any new user-specific tables. (Not applicable - using existing tables)Notes for Reviewers