Delete a user's public key via admin api (closes #3014) #3059
Conversation
vtemian changed the title 
Codecov Report
@@ Coverage Diff @@ ## master #3059 +/- ## ========================================== + Coverage 33.63% 34.07% +0.44% ========================================== Files 273 273 Lines 39954 39998 +44 ========================================== + Hits 13437 13629 +192 + Misses 24610 24429 -181 - Partials 1907 1940 +33
Continue to review full report at Codecov.
|
tboerger added the lgtm/need 2 
vtemian force-pushed the delete-user-key-as-admin-3014 branch from f3bea44 to 7d646ed Compare 
| Can you also add test that adds and later deletes that key? |
| @lafriks done! |
routers/api/v1/admin/user.go Outdated
routers/api/v1/admin/user.go Outdated
There was a problem hiding this comment.
nit: "id of the key to delete"
routers/api/v1/admin/user.go Outdated
There was a problem hiding this comment.
nit: "Delete a user's public key". Don't need to repeat "user" twice.
routers/api/v1/admin/user.go Outdated
There was a problem hiding this comment.
It would be nice if we didn't have to call GetPublicKeyByID before calling DeletePublicKey, since DeletePublicKey already calls GetPublicKeyByID. Perhaps it's worthwhile to have DeletePublicKey return a ErrKeyNotExist when you try to delete a non-existent key?
There was a problem hiding this comment.
Yeap, I wanted to do that, but my concern was that right now DeletePublicKey returns nil if it encounters an ErrKeyNotExist error and maybe, somewhere in the code, somebody is relying on that. (even though is not a sane behavior)
Is it safe to return ErrKeyNotExist ?
There was a problem hiding this comment.
That function is used only in one file, so additional checks for that could be added for that error
integrations/api_admin_test.go Outdated
There was a problem hiding this comment.
nit: please use models.NonexistentID instead of 99999
vtemian force-pushed the delete-user-key-as-admin-3014 branch from 565c4b7 to 0e3388f Compare 
vtemian force-pushed the delete-user-key-as-admin-3014 branch from 0e3388f to 65871ea Compare | @ethantkoenig done |
| ], | ||
| "summary": "Create a user", | ||
| "operationId": "adminCreateUser", | ||
| "security": null, |
There was a problem hiding this comment.
I've run swagger generate spec and this was the output. I didn't want to manually change it since it's easier to maintain this way.
| LGTM |
tboerger added lgtm/need 1 | LGTM |
tboerger added lgtm/done 
6 participants
Let admin delete a user's key via API