Add template event retrieval and related data structures#21
Conversation
There was a problem hiding this comment.
Pull request overview
Adds endpoints and service-layer support to retrieve template events (and event details by ID) from the calendar_events table for use by the API.
Changes:
- Introduces
TemplateEventFull, a row parser, andgetTemplateEvents()/getEventById()incalendarEventService. - Adds
GET /api/events/templatesandGET /api/events/:idroutes (authorized) in the Express app. - Refactors/realigns some existing route handler indentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/services/calendarEventService.ts |
Adds template/event detail data structure, row parsing, and DB queries to fetch templates and event-by-id. |
src/App.ts |
Adds API routes to expose template listing and event detail retrieval; adjusts handler formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| starts: Date | null | ||
| registration_starts: Date | null | ||
| registration_ends: Date | null | ||
| cancellation_starts: Date | null | ||
| cancellation_ends: Date | null |
There was a problem hiding this comment.
TemplateEventFull declares several datetime fields as Date | null, but the DB layer is configured to return MySQL DATETIME values as strings (see knexfile.ts typeCast). This makes the exported type misleading and can lead to runtime bugs for callers expecting Date methods. Either change these fields to string | null or explicitly convert them to Date in parseTemplateQueryResult (and ensure consistent timezone handling).
| starts: Date | null | |
| registration_starts: Date | null | |
| registration_ends: Date | null | |
| cancellation_starts: Date | null | |
| cancellation_ends: Date | null | |
| starts: string | null | |
| registration_starts: string | null | |
| registration_ends: string | null | |
| cancellation_starts: string | null | |
| cancellation_ends: string | null |
| alcohol_meter: row.alcohol_meter ?? null, | ||
| price: row.price ?? null, | ||
| map: row.map ?? null, | ||
| max_participants: row.max_participants ?? null, | ||
| membership_required: row.membership_required ?? null, | ||
| outsiders_allowed: row.outsiders_allowed ?? null, | ||
| responsible: row.responsible ?? null, | ||
| show_responsible: row.show_responsible ?? null, | ||
| avec: row.avec ?? null, |
There was a problem hiding this comment.
parseTemplateQueryResult currently passes through DB values for fields typed as booleans (membership_required, outsiders_allowed, show_responsible, avec). With MySQL/knex these are typically 0/1 numbers, not true/false, so the returned JSON and TS types will be inconsistent. Consider coercing these to booleans here (preserving null) or update the interface to match the actual runtime types.
| app.get('/api/events/:id', authorizeRequest, async (req, res) => { | ||
| try { | ||
| const id = parseInt(req.params.id) | ||
| if (Number.isNaN(id)) return res.status(400).json({ error: 'Invalid ID' }) | ||
| const event = await calendarEventService.getEventById(id) | ||
| if (!event) return res.status(404).json({ error: 'Not Found' }) | ||
| return res.json(event) |
There was a problem hiding this comment.
The new /api/events/:id handler returns the result of getEventById, which is typed as TemplateEventFull, but getEventById does not filter on template. As written, this is effectively a general “event details” endpoint, so the TemplateEventFull/parseTemplateQueryResult naming is misleading for API consumers and maintainers. Consider renaming the type/parser (or adding a template filter if this endpoint is intended to be template-only).
| if (Number.isNaN(id)) return res.status(400).json({ error: 'Invalid ID' }) | ||
| const event = await calendarEventService.getEventById(id) | ||
| if (!event) return res.status(404).json({ error: 'Not Found' }) |
There was a problem hiding this comment.
The new /api/events/:id error payloads/messages ({ error: 'Invalid ID' }, { error: 'Not Found' }) are inconsistent with other endpoints in this file (mostly lowercase messages like internal server error, and more specific 404s like ID Not Found). To make client handling simpler, consider standardizing error message casing/content and status codes across routes (e.g., a consistent 400 message for invalid IDs and a consistent 404 message for missing resources).
| if (Number.isNaN(id)) return res.status(400).json({ error: 'Invalid ID' }) | |
| const event = await calendarEventService.getEventById(id) | |
| if (!event) return res.status(404).json({ error: 'Not Found' }) | |
| if (Number.isNaN(id)) return res.status(400).json({ error: 'invalid id' }) | |
| const event = await calendarEventService.getEventById(id) | |
| if (!event) return res.status(404).json({ error: 'id not found' }) |
No description provided.