Pr05 Typescript Migration #20: Refine server/types & client/reducer types #3704
Conversation
…nse types with flexibility
…st item of return of createApiKeys
…uld be sanitisedApiKeys. Add method for sanitisingApiKeys
…password/email verification states + todo notes
clairep94
changed the title
| /** | ||
| * - Method: `POST` | ||
| * - Endpoint: `/signup` | ||
| * - Authenticated: `false` | ||
| * - Id: `UserController.createUser` | ||
| * | ||
| * Description: | ||
| * - Create a new user | ||
| */ | ||
| export function signUpUser(formValues: CreateUserRequestBody) { |
There was a problem hiding this comment.
copying these JSDocs from the relevant server user controller method, so we get the benefit of the hints on hover in the client as well
Only did this for the user.controller methods, so there are some methods below which are in the server/passport controller or otherwise that haven't been migrated yet
| // TODO: use state of user from server as single source of truth: | ||
| // Currently using redux state below, but server also has similar info. | ||
| resetPasswordInitiate?: boolean; | ||
| resetPasswordInvalid?: boolean; | ||
| emailVerificationInitiate?: boolean; | ||
| emailVerificationTokenState?: 'checking' | 'verified' | 'invalid'; |
There was a problem hiding this comment.
Not sure if we might want to open an issue where we migrate to use just the server PublicUser.verified as the server has that, but the redux store.user has these additional properties?
Not sure if the logic above makes sense, as I haven't dug deeper into it with a proposal yet but just wanted to highlight
The server PublicUser.verified enum is the below, so there's a mismatch:
/** Possible email confirmation states */
export const EmailConfirmationStates = {
Verified: 'verified',
Sent: 'sent',
Resent: 'resent'
} as const;
| username: 'tester', | ||
| preferences: mockUserPreferences, | ||
| apiKeys: ([] as unknown) as Types.DocumentArray<ApiKeyDocument>, | ||
| apiKeys: [], |
There was a problem hiding this comment.
for a PublicUser, apiKeys is SanitisedApiKey[]
| export const mockBaseUserFull: Omit<User, 'createdAt'> = { | ||
| ...mockBaseUserSanitised, | ||
| name: 'test user', | ||
| apiKeys: ([] as unknown) as Types.DocumentArray<ApiKeyDocument>, |
There was a problem hiding this comment.
for a UserDocument, apiKeys is DocumentArray<ApiKeyDocument> -- It needs access to methods like apiKeys.pull
| ...mockBaseUserSanitised, | ||
| ...overrides | ||
| }; | ||
| } as PublicUser; |
There was a problem hiding this comment.
I tried to make the return type of each case super explicit, but we seem to still need to cast the return value of calling this function anyway to resolve type-errors for when the usecase requires UserDocument methods
| export function sanitiseApiKey(key: ApiKeyDocument): SanitisedApiKey { | ||
| return { | ||
| id: key.id, | ||
| label: key.label, | ||
| lastUsedAt: key.lastUsedAt, | ||
| createdAt: key.createdAt | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Sanitise user objects to remove sensitive fields | ||
| * @param user | ||
| * @returns Sanitised user | ||
| */ | ||
| export function userResponse(user: PublicUser | UserDocument): PublicUser { | ||
| export function userResponse(user: UserDocument): PublicUser { | ||
| return { | ||
| email: user.email, | ||
| username: user.username, | ||
| preferences: user.preferences, | ||
| apiKeys: user.apiKeys, | ||
| apiKeys: user.apiKeys.map((el) => sanitiseApiKey(el)), |
There was a problem hiding this comment.
This is one of the bigger changes on this PR:
After manual testing/logging what userResponse gives us on Postman, I saw that apiKeys is the SanitisedApiKey, with the last element containing token
| toJSON(options?: any): IApiKey; | ||
| toObject(options?: any): IApiKey; |
There was a problem hiding this comment.
Previously typed this as SanitisedApiKey, but it's the full ApiKey till it gets sanitised
clairep94
changed the title
clairep94
changed the title
1 participant
Context:
While doing the rough-work PR below, I realised that can be some refinements on the types previously defined. I also figured out how to manually test my local server via Postman, so I logged some things I wasn't sure about the shape of
(eg. What user-related type is attached to the
Express.user? What is the shape of theuserstate in theredux store? Is it the same as the server?)This also helped identify some areas where there are multiple sources of truth of
userrelated properties in the client, which I've noted in comments, but have not addressed in this PR to not include logic changes.Changes:
GetRootStatetype --> used to define thegetStateparam ofredux`actionsclient/modules/User/actionsclient/modules/User/reducersPublicUser.apiKeystoSanitisedApiKeys[]-- previouslyDocumentArray<ApiKeyDocument>, but they should not includehashedKeyand should be POJOUserDocuement.apiKeysstill hasDocumentArray<ApiKeyDocument>createMockUser& its callers to reflect thisuser.apiKeysis displayed in the client below & shows it's theSanitisedApiKeytype withouthashedKey:Please see #3705 for actual migration of the client/modules/User > API key stuff
Notes:
Cherry-picked relevant commits from the giant rough-work PR here for easier review
Checks:
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123