volunteer update api #43
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adds a PATCH endpoint for updating volunteer records with comprehensive validation and Supabase integration. The implementation follows good architectural patterns by separating validation logic from HTTP handling, and improves test type safety in existing test files.
Key changes:
- New volunteer update API with field validation and Supabase integration
- PATCH endpoint at
/api/volunteers/[id]with proper error handling - Improved type safety in existing test files using
vi.mocked()wrapper
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/api/updateVolunteer.ts |
Core API logic with validation function and Supabase update operation for volunteer records |
src/app/api/volunteers/[id]/route.ts |
Next.js API route handler implementing PATCH endpoint with ID validation and error handling |
src/lib/api/index.ts |
Exports new updateVolunteer functions for use across the application |
tests/lib/api/getExample.test.ts |
Enhanced type safety using proper vi.mocked() wrapper instead of type casting |
README.md |
Formatting improvements for better readability (whitespace and indentation) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/lib/api/updateVolunteer.ts
Outdated
| if (value !== null && value !== undefined && typeof value !== "string") { | ||
| return { error: `Field ${key} must be a string or null` }; | ||
| } | ||
|
|
||
| updates[key] = value as VolunteerUpdatePayload[StringFieldKey]; |
There was a problem hiding this comment.
The validation allows undefined values to be passed through to the database for optional fields. While this may be intentional, it could lead to unexpected behavior since undefined in JavaScript/TypeScript is not the same as null in databases. The condition on line 84 checks for undefined separately, but then line 88 casts the value (which could be undefined) directly to the payload type. Consider explicitly handling the undefined case:
if (value === undefined || value === null) {
updates[key] = null; // Explicitly convert undefined to null for database
} else if (typeof value === "string") {
updates[key] = value as VolunteerUpdatePayload[StringFieldKey];
} else {
return { error: `Field ${key} must be a string or null` };
}| if (value !== null && value !== undefined && typeof value !== "string") { | |
| return { error: `Field ${key} must be a string or null` }; | |
| } | |
| updates[key] = value as VolunteerUpdatePayload[StringFieldKey]; | |
| if (value === undefined || value === null) { | |
| updates[key] = null; | |
| } else if (typeof value === "string") { | |
| updates[key] = value as VolunteerUpdatePayload[StringFieldKey]; | |
| } else { | |
| return { error: `Field ${key} must be a string or null` }; | |
| } |
| if ("name_org" in payload) { | ||
| const value = payload.name_org; | ||
| if (value === null || value === undefined) { | ||
| return { error: "Field name_org must be provided as a non-empty string" }; |
There was a problem hiding this comment.
The error message is inconsistent with the validation logic. The message says "must be provided as a non-empty string" but the actual validation checks happen across three separate conditions (lines 57, 60, 63). Additionally, this error is returned for both null and undefined, but neither of those cases means the field was "provided". Consider making the error message more specific:
- Line 58:
"Field name_org cannot be null or undefined" - Line 61:
"Field name_org must be a string" - Line 64:
"Field name_org cannot be empty"
This provides clearer feedback about what exactly is wrong with the input.
| return { error: "Field name_org must be provided as a non-empty string" }; | |
| return { error: "Field name_org cannot be null or undefined" }; |
| // name_org is the only required patchable field; validate it eagerly | ||
| const updates: Partial<VolunteerUpdatePayload> = {}; | ||
| if ("name_org" in payload) { | ||
| const value = payload.name_org; | ||
| if (value === null || value === undefined) { | ||
| return { error: "Field name_org must be provided as a non-empty string" }; | ||
| } | ||
| if (typeof value !== "string") { | ||
| return { error: "Field name_org must be a string" }; | ||
| } | ||
| if (value.trim().length === 0) { | ||
| return { error: "Field name_org cannot be empty" }; | ||
| } | ||
| updates.name_org = value; | ||
| } |
There was a problem hiding this comment.
The validation logic for name_org has an inconsistency. Lines 57-58 reject null or undefined values, but according to line 53's comment ("name_org is the only required patchable field"), it seems the intent is that name_org must be provided when updating. However, the check at line 105 allows updates without name_org if other fields are present. This creates ambiguity: is name_org required for all updates, or only required to be non-empty if provided? Consider clarifying the validation logic to match the actual requirement.
src/app/api/volunteers/[id]/route.ts
Outdated
There was a problem hiding this comment.
We don't need a route.ts file
There was a problem hiding this comment.
When updating a row in the Volunteers table, we need to set the updated_at field to the timestamp of the update.
src/lib/api/index.ts
Outdated
| export { getExample } from "./getExample"; | ||
| export { | ||
| updateVolunteer, | ||
| validateVolunteerUpdateBody, |
There was a problem hiding this comment.
We only want to export updateVolunteer function. Can we validate the inputs/body inside the function before creating the Supabase client? We want to return an error if the input/body is wrong.
There was a problem hiding this comment.
Could you include a more detailed PR description, along with screenshots of the database showing that the volunteer's information was updated correctly? You can refer to other PRs. PR #49 is an excellent example of a good description. Let us know if you need any help!
Also, always make sure you're up to date with the latest main branch. There's some code in the latest main that runs formatting/linting checks before every commit, so it should make the code cleaner and prevent Vercel errors!
One last thing!
When updating a volunteer's data, if the user wants to update role and/or cohort we need to change the tables VolunteerRoles and/or VolunteerCohorts respectively. Make sure the role and/or cohort inputted exist as a row in the Roles and Cohorts tables respectively.
- For role, the user input should be a tuple of
name: stringandtype: string in ["prior", "current", "future_interest"]. - For cohort, the user input should be a tuple of
year: intandterm: string in ["fall", "summer", "winter", "spring"].
Add volunteer PATCH endpoint with validation and Supabase update