221 add validation to patch#227
Conversation
tsudhakar87
left a comment
tsudhakar87
left a comment
There was a problem hiding this comment.
Looks great! Just one question
|
|
||
| const profileImageResult = UserValidationUtils.validateProfileImage(body.profileImage); | ||
| if (!profileImageResult.isValid) return json(400, { message: profileImageResult.error }); | ||
|
|
There was a problem hiding this comment.
I might be missing something obvious but do isAdmin and profile_image also need to be checked against the validation utils here?
There was a problem hiding this comment.
added checking isAdmin against validation utils!
nourshoreibah
left a comment
nourshoreibah
left a comment
There was a problem hiding this comment.
Mainly one comment about required fields!
| let profileImage = body.profileImage as string | undefined; | ||
| const updates: { email?: string; name?: string; is_admin?: boolean; profile_image?: string } = {}; | ||
|
|
||
| const emailResult = UserValidationUtils.validateEmail(body.email); |
There was a problem hiding this comment.
Very much a nit but I wonder if we should just throw the whole body into the validation utils and let it handle all these conditionals 👀 I don't feel super strongly about this just an idea
| static readonly EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
|
|
||
| // Validates email - if provided, must be a correctly formatted string | ||
| static validateEmail(input: unknown): ValidationResult<string | null> { |
There was a problem hiding this comment.
I think email, name, isAdmin should be required. profile image optional makes sense
3 participants
#221
Closes #221
📝 Description
Briefly list the changes made to the code:
✔️ Verification
Added tests
🏕️ (Optional) Future Work / Notes
Did you notice anything ugly during the course of this ticket? Any bugs, design challenges, or unexpected behavior? Write it down so we can clean it up in a future ticket!