-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/4 crud and dtos for event entity #7
Feature/4 crud and dtos for event entity #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work, but it can be extended a bit. We can merge without the custom validator, that could be a separate task
apps/backend/.eslintrc.js
Outdated
@@ -16,5 +16,6 @@ module.exports = { | |||
'@typescript-eslint/no-empty-function': 'off', | |||
'@typescript-eslint/no-extra-semi': 'off', | |||
'@typescript-eslint/no-unused-vars': 'error', | |||
'ordered-imports': [true, { 'grouped-imports': false }], //disable anoying empty line between import groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather deleted it because it caused more problems and is not as annoying as I thought before
@IsDate() | ||
startDate: Date; | ||
|
||
@IsDate() | ||
endDate: Date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will probably need @IsDateString() here, IsDate expects a Date object, but wie'll get a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, it'd be cool to validate that endDate is after startDate. You probably have to create a custom decorator for that, it's a great task if you have the time
} | ||
|
||
@Get(':id') | ||
findOne(@Param('id') id: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should validate the id here. In the course, we used ParseIntPipe, but that's not going to work here since we don't use number based ID.s Instead take a look at ParseUUIDPipe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well done, just fix the eslint stuff and then it's good to go. You can comment out the parameters in the service methods
Solution for #4