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.
ja-der
force-pushed
the
jason/events-endpoints
branch
from
3251133 to
0e23067
Compare
| seasonCode: string, | ||
| eventName: string, | ||
| startTime: string, | ||
| endTime: string, |
There was a problem hiding this comment.
nit: personally would make the startTime and endTime accept Date objects instead (if we ever want to reuse this function elsewhere, it should be clear these represent dates)
ja-der
force-pushed
the
jason/events-endpoints
branch
from
0e23067 to
c310fb3
Compare
| type CheckInReturn = Awaited<ReturnType<(typeof svc)["checkInUser"]>>; | ||
| import app from "@/server"; | ||
|
|
||
| vi.mock("@/config/env", () => ({ |
There was a problem hiding this comment.
@danielp1218 we should move this into some setup script for vitest
https://vitest.dev/config/#globalsetup
There was a problem hiding this comment.
Oh yeah ill do that, that'll be really useful
| let consoleErrorSpy: ReturnType<typeof vi.spyOn>; | ||
|
|
||
| beforeAll(() => { | ||
| consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| consoleErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
kinda sus, why we are hiding error logs?
| vi.mock("@/db", () => ({ | ||
| db: { | ||
| select: vi.fn(), | ||
| insert: vi.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock("@/db/schema", () => ({ | ||
| seasonResponse: { | ||
| seasonCode: "seasonCode", | ||
| seasonId: "seasonId", | ||
| eventName: "testEvent", | ||
| startTime: "1:00", | ||
| endTime: "2:00", | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock("drizzle-orm", () => ({ | ||
| eq: vi.fn((field, value) => ({ field, value, type: "eq" })), | ||
| and: vi.fn((...conditions) => ({ conditions, type: "and" })), | ||
| sql: vi.fn((strings, ...values) => ({ strings, values, type: "sql" })), | ||
| })); |
There was a problem hiding this comment.
why are we mocking the DB and also initializing it?
| if (!response) { | ||
| throw new ApiError(404, { | ||
| code: "NO_EVENTS_FOUND", | ||
| message: "No events found", | ||
| suggestion: "Verify the seasonCode is correct.", | ||
| }); | ||
| } | ||
| return c.json(response); |
There was a problem hiding this comment.
nit: we should just return empty - we dont want a variant between empty/non-existent because it allows for people to "mine" for our season codes
https://owasp.org/www-community/Improper_Error_Handling
| checkInAuthor, | ||
| checkInNotes: checkInNotes || null, | ||
| }) | ||
| .onConflictDoUpdate({ |
There was a problem hiding this comment.
Do we want to allow rechecking in a user? would there realistically be a reason why we would recheckin?
implemented events endpoints: list, create, and check-in