-
Notifications
You must be signed in to change notification settings - Fork 3.4k
API tokens UI #9829
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
base: develop
Are you sure you want to change the base?
API tokens UI #9829
Conversation
export type APIApiTokenSaveFields = Partial<Pick<SerializedApiTokenData, 'name' | 'expiry_date' | 'read_only'>>; | ||
export type ApiTokenSaveFields = CamelizedV2<APIApiTokenSaveFields>; | ||
|
||
export type APIUserSaveFields = Partial<Pick<SerializedUser, 'first_name' | 'last_name'>>; | ||
export type UserSaveFields = CamelizedV2<APIUserSaveFields>; |
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.
What do you think of using Modifiable/Mutable/Updatable fields instead?
SaveFields does not exactly reflect the idea IMO
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.
Updated to Modifiable
cvat-core/src/api-token.ts
Outdated
result.value = this.#value; | ||
} | ||
|
||
return result as SerializedApiTokenData; |
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.
From implementation I see it returns Partial<SerializedApiTokenData>
, not SerializedApiTokenData
.
Declaration is not correct
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.
Updated implementation and SerializedApiToken type so no conversion is needed
updateUserFailed: (error: unknown) => createAction(AuthActionTypes.UPDATE_USER_FAILED, { error }), | ||
getApiTokens: () => createAction(AuthActionTypes.GET_API_TOKENS), | ||
getApiTokensSuccess: (tokens: ApiToken[]) => createAction(AuthActionTypes.GET_API_TOKENS_SUCCESS, { tokens }), | ||
getApiTokensFailed: (error: any) => createAction(AuthActionTypes.GET_API_TOKENS_FAILED, { error }), |
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.
Everywhere (here and lines below) should be error: Error
according to ErrorState
interface and your code reason: action.payload.error
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.
The problem with typing error here as Error
is that we get typescript error in actions in catch section: Catch clause variable type annotation must be 'any' or 'unknown' if specified
. Should we catch any
and convert it explicitly to Error?
return true; | ||
} | ||
|
||
async function getApiTokens(filter: APIApiTokensFilter = {}): Promise<PaginatedResource<SerializedRequest>> { |
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.
The filter supports pagination and, nevertheless, always returns all results.
Do you see any reasons why we cannot paginate it?
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.
Technically, we could paginate it. But to do that, we’d need to add support for server-side pagination in our cvat-table
component, which currently expects the full dataset.
The question is: do we really need it? I’d assume the table will have, at most, tens of rows—not hundreds.
cvat-ui/src/components/profile-page/security-content/api-tokens-card.tsx
Show resolved
Hide resolved
cvat-ui/src/components/profile-page/security-content/api-tokens-card.tsx
Show resolved
Hide resolved
cvat-ui/src/components/profile-page/security-content/api-tokens-card.tsx
Outdated
Show resolved
Hide resolved
Please, merge with develop and resolve conflicts |
@klakhov, the server has the updated |
|
Motivation and context
Provides a card inside 'security' of user profile with a table of API keys, and a form to create/edit them
Depends on #9680, #9835
How has this been tested?
Checklist
develop
branchLicense
Feel free to contact the maintainers if that's a concern.