-
Notifications
You must be signed in to change notification settings - Fork 888
feat: add 'Show all tokens' toggle for owners #6325
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
Conversation
I tried to write a test for this switch:
But I keep getting this error: I believe |
If it is in the authenticated route, I would use renderWithAuth. But if the render helper has the query client, it should work... maybe we are missing something |
I did try |
coderd/apikey.go
Outdated
for _, key := range keys { | ||
apiKeys = append(apiKeys, convertAPIKey(key)) | ||
user, err := api.Database.GetUserByID(ctx, key.UserID) |
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 use GetUsersByIDs
here instead, otherwise, you could execute hundreds of separate queries if there are hundreds of users.
Generally, we should never query in for
loops!
codersdk/apikey.go
Outdated
@@ -90,6 +90,11 @@ type TokensFilter struct { | |||
IncludeAll bool `json:"include_all"` | |||
} | |||
|
|||
type ConvertedAPIKey struct { |
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.
I think APIKeyWithOwner
or APIKeyWithUsername
might make more sense here.
Oops, I meant to click request changes, but feel free to re-tag me if you need! |
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.
FE LGTM
resolves #6214
Adds a new 'Show all tokens' toggle which only appears if the user has read all permissions.