feat: Add PUT guests endpoint #78
Conversation
danctila
left a comment
danctila
left a comment
There was a problem hiding this comment.
very solid implementation and testing just requersted one change to think about in the SQL update and one other small thing
| func (r *GuestsRepository) UpdateGuest(ctx context.Context, id string, update *models.UpdateGuest) (*models.Guest, error) { | ||
| var guest models.Guest | ||
|
|
||
| row := r.db.QueryRow(ctx, ` | ||
| UPDATE guests | ||
| SET | ||
| first_name = COALESCE($2, first_name), | ||
| last_name = COALESCE($3, last_name), | ||
| profile_picture = COALESCE($4, profile_picture), | ||
| timezone = COALESCE($5, timezone), | ||
| updated_at = NOW() | ||
| WHERE id = $1 | ||
| RETURNING | ||
| id, created_at, updated_at, | ||
| first_name, last_name, profile_picture, timezone`, | ||
| id, | ||
| update.FirstName, | ||
| update.LastName, | ||
| update.ProfilePicture, | ||
| update.Timezone, | ||
| ) | ||
|
|
There was a problem hiding this comment.
I think the COALESCE approach in the UPDATE query might have a flaw bc you can't intentionally set the fields to NULL.
For ex if a client wants to remove their profile picture or timezone they won't be able to do it with this implementation
Might want to just add more to the pattern to distinguish between "not provided" and "set to null/empty/etc." OR document that the endpoint specifically doesn't support clearing optional fields
| func validateUpdateGuest(update *models.UpdateGuest) error { | ||
| errors := make(map[string]string) | ||
|
|
||
| if update.FirstName != nil && strings.TrimSpace(*update.FirstName) == "" { | ||
| errors["first_name"] = "must not be an empty string" | ||
| } | ||
|
|
||
| if update.LastName != nil && strings.TrimSpace(*update.LastName) == "" { | ||
| errors["last_name"] = "must not be an empty string" | ||
| } | ||
|
|
||
| if update.Timezone != nil { | ||
| if _, err := time.LoadLocation(*update.Timezone); err != nil { | ||
| errors["timezone"] = "invalid IANA timezone" | ||
| } | ||
| } | ||
|
|
||
| if len(errors) > 0 { | ||
| var keys []string | ||
| for k := range errors { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
|
|
||
| var parts []string | ||
| for _, k := range keys { | ||
| parts = append(parts, k+": "+errors[k]) | ||
| } | ||
| return errs.BadRequest(strings.Join(parts, ", ")) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
this is small but in validateUpdateGuest there is no check for an empty update where all fields are nil so a PUT request with {} would succeed but not do anything except update the timestamp -> so it mightb e good to have some validation that requires at least one field to be updated
Description
Added PUT endpoint for guests as the last part of the whole guest entity integration #68
Type of Change
Related Issue(s)
Closes #68
What Changed?
PUT /api/v1/guests/:idendpoint for guest updatesTesting & Validation
How this was tested
Screenshots/Recordings
Unfinished Work & Known Issues
Notes & Nuances
Pre-Merge Checklist
Code Quality
Testing & CI
Documentation
Reviewer Notes