Update backend routes for events #1018
Conversation
| @Param('timetableId') timetableId: string | ||
| ) { | ||
| try { | ||
| const eventDetails = await this.userService.getEvent(eventId, timetableId); |
There was a problem hiding this comment.
Can just directly return the awaited value here, no need for variable.
| @@ -64,4 +66,79 @@ export class UserService { | |||
| }, | |||
| }); | |||
| } | |||
|
|
|||
| async getEvent(eventId: string, timetableId: string): Promise<EventParameters | null> { | |||
There was a problem hiding this comment.
What cases does this promise actually return null? If this is explicitly for the case when you throw an error, that isn't really the way to indicate that. (I'd just remove the null in that case.)
| await this.prisma.event.create({ | ||
| data: { | ||
| id: eventParameters.id, | ||
| colour: eventParameters.colour, |
There was a problem hiding this comment.
We will need to change this to use @jason4193's colour validation function once it's merged.
| id: eventParameters.id, | ||
| colour: eventParameters.colour, | ||
| dayOfWeek: eventParameters.dayOfWeek, | ||
| start: eventParameters.start, |
There was a problem hiding this comment.
I wonder if we need some validation on these... Might be worth checking FE code/discussing what we think makes sense to represent event times.
There was a problem hiding this comment.
Might be a good idea to hold the validation after combining #1000 into server-rewrite branch?
| }) | ||
| } | ||
|
|
||
| async removeEvent(eventId: string, timetableId: string): Promise<void> { |
There was a problem hiding this comment.
Wondering if it makes sense to omit the timetableId field on all of these routes. The eventId is already unique across all users, across all timetables. Thoughts?
There was a problem hiding this comment.
Agree on using eventID directly.
| export class EventParameters { | ||
| id: string; | ||
| colour: string; | ||
| dayOfWeek: number; | ||
| start: number; | ||
| end: number; | ||
| type: EventType; | ||
| timetableId: string; | ||
| } |
There was a problem hiding this comment.
Might need to obtain everything about the event from prisma. Since only prisma db contains the details of the event that being used on FE.
There was a problem hiding this comment.
You mean importing from server/src/generated/prisma/models/Event.ts?
There was a problem hiding this comment.
Since the /generated/ dir is auto generated from the schema. Not sure can we simply import it from that file. What I am thinking is having a duplicate type in here but with all most of the detail in our schema as all the information of the event will be displayed on FE.
|
|
||
| @Get('event/:timetableId/:eventId') | ||
| @UseGuards(AuthenticatedGuard) | ||
| async getEventById( |
There was a problem hiding this comment.
Need an additional function for getting all events'ID by using timetableID. Right now the FE is not holding any information about the Event.
There was a problem hiding this comment.
Alternative solution could be return EventID to FE after creating a new event. Then FE storing it as id attribute?
Thoughts on which way better?
There was a problem hiding this comment.
I think the required event information should come from the get timetable route. This may already be covered in #1023. Once the frontend is updated though, it may be beneficial from a performance viewpoint to add a route that gets all events from one timetable... 🤔 (Problem for later)
| id: eventParameters.id, | ||
| colour: eventParameters.colour, | ||
| dayOfWeek: eventParameters.dayOfWeek, | ||
| start: eventParameters.start, |
There was a problem hiding this comment.
Might be a good idea to hold the validation after combining #1000 into server-rewrite branch?
| }) | ||
| } | ||
|
|
||
| async removeEvent(eventId: string, timetableId: string): Promise<void> { |
There was a problem hiding this comment.
Agree on using eventID directly.
Description 🧾
Context of PR, summary of changes, as well as any relevant technical decisions/motivations.
Testing
Replace this line with instructions on how to test your changes, as well as any relevant images for UI changes.
Checklist
feature: for application feature improvement or new featuresbug: fixing something that previously wasn't workingdev: development-side related changesdocker: updates to the Docker codesetup: changes to the setup/infrastructure of the codebasetest: updates to internal testing