Jason/events endpoints #3
Conversation
ja-der
requested review from
Lindsay-X,
NotMyLyfe,
Copilot,
danielp1218,
fpunny and
shoexue
There was a problem hiding this comment.
Pull request overview
This PR implements three new event-related endpoints for managing events and user check-ins: listing events by season, creating new events, and checking users into events. The implementation includes service layer functions, route handlers with validation, and comprehensive test coverage.
Key Changes:
- Added service layer functions (
fetchEvents,createEvent,checkInUser) with database operations and error handling - Implemented three REST endpoints with Zod validation for event management and user check-ins
- Added comprehensive unit tests for all service layer functions covering success and error scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/resources/events/events.test.ts | Comprehensive unit tests for all three service functions with mocked database operations |
| src/resources/events/events.service.ts | Service layer implementation for fetching, creating events and checking in users |
| src/resources/events/events.routes.ts | HTTP route handlers with validation schemas for event endpoints |
| src/api.ts | Registration of the new events routes with the API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| export const createEvent = async ( | ||
| seasonCode: string, | ||
| eventName: string, |
There was a problem hiding this comment.
The parameter order doesn't match the test expectations. Tests call createEvent(seasonCode, eventId, eventName, startTime, endTime) with 5 parameters including an eventId, but the function signature only accepts 4 parameters and is missing eventId. This mismatch will cause the tests to fail at runtime.
| async (c) => { | ||
| const seasonCode = c.req.param("seasonCode"); | ||
| const eventId = c.req.param("eventId"); | ||
| const body = await c.req.json(); |
There was a problem hiding this comment.
The request body is being parsed twice - once by the validator middleware on line 64 and again manually here. Use c.req.valid('json') instead to access the already-validated body, consistent with the pattern used in the create event endpoint (line 40).
| const body = await c.req.json(); | |
| const body = c.req.valid("json"); |
There was a problem hiding this comment.
i agree with mr. copilot
| export default api; | ||
|
|
||
| api.route("/", eventsRoute); |
There was a problem hiding this comment.
The events route is registered after the export statement. Move this line before the export (after line 10) to maintain consistency with other route registrations and ensure proper initialization order.
| export default api; | |
| api.route("/", eventsRoute); | |
| api.route("/", eventsRoute); | |
| export default api; |
| : c.json({ message: "No events found" }, 404); | ||
| } catch (error) { | ||
| const message = | ||
| error instanceof Error ? error.message : "Failed to fetch events"; |
There was a problem hiding this comment.
I changed the error handling to a different structure in my PR (#1). A better structure would be to throw an ApiError directly from the service function and not catch it (will be caught by Hono's error handling fn). Also, pls change all error responses to ApiError, we want to have consistent error handling. You can see how I did it in the formResponses route.
| const eventsRoute = new Hono(); | ||
|
|
||
| // list all events | ||
| eventsRoute.get("/seasons/:seasonCode/events", async (c) => { |
There was a problem hiding this comment.
we should get some doc specifications on this :) (and all the other routes here). Check https://hono.dev/examples/hono-openapi.
we can work on this after this gets merged in though.
| return result; | ||
| } catch (error: unknown) { | ||
| const dbError = handleDbError(error); | ||
| throw new Error(dbError.message); |
There was a problem hiding this comment.
outdated, check #1 for better way of doing this
| async (c) => { | ||
| const seasonCode = c.req.param("seasonCode"); | ||
| const eventId = c.req.param("eventId"); | ||
| const body = await c.req.json(); |
There was a problem hiding this comment.
i agree with mr. copilot
implemented events endpoints: list, create, and check-in