(PXP-8992): PATCH /user/{username} #140
Conversation
arborist/server_test.go
Outdated
| assertUsernameAndEmail(t, originalUsername, newUserEmail) | ||
| }) | ||
|
|
||
| t.Run("BothUsernameAndEmail", func(t *testing.T) { |
There was a problem hiding this comment.
what about neither, can you test that error case too?
| result, err := db.Exec(stmt, name, email, user.Name) | ||
| if err != nil { | ||
| // this should only fail because the target name was not unique | ||
| msg := fmt.Sprintf(`failed to update name to "%s": user with this name already exists`, *name) |
There was a problem hiding this comment.
what if a user with name = $3 doesn't exist at all?
There was a problem hiding this comment.
In the case that name = $3 doesn't exist at all, the SQL query still executes successfully, but rowsAffected below is 0 and a 404 is returned.
| @@ -133,6 +133,7 @@ func (server *Server) MakeRouter(out io.Writer) http.Handler { | |||
| router.Handle("/user", http.HandlerFunc(server.handleUserList)).Methods("GET") | |||
| router.Handle("/user", http.HandlerFunc(server.parseJSON(server.handleUserCreate))).Methods("POST") | |||
| router.Handle("/user/{username}", http.HandlerFunc(server.handleUserRead)).Methods("GET") | |||
| router.Handle("/user/{username}", http.HandlerFunc(server.parseJSON(server.handleUserUpdate))).Methods("PATCH") | |||
There was a problem hiding this comment.
I feel this /user API is a little odd using username as the id, since it's mutable now. I don't think there's much to do at this point b/c we don't want to rework the whole API... but we'll want to watch out b/c now clients must update their internal representation of username only on a successful PATCH (which is fine and makes sense, but it leaves room for some weird errors where a client might keep trying to patch something that's already been updated and starts getting errors b/c the username they're trying to patch doesn't exist anymore).
Maybe they didn't parse the success from Arborist right or persist it or something. Anything, since this is a new API and we are the client in most cases, this is fine. Just thinking out loud
arborist/user.go
Outdated
| return newErrorResponse(msg, 409, &err) | ||
| } | ||
|
|
||
| rowsAffeted, _ := result.RowsAffected() |
There was a problem hiding this comment.
Should be rowsAffected, will correct this.
Jira Ticket: PXP-8992
New Features
PATCH /user/{username}endpoint