Add POST /users/invite and extend /confirm with optional password #1801
Conversation
|
✔️ Deploy Preview for dev-storybook-bloom ready! 🔨 Explore the source changes: a6b73ca 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/61405947f8b79e00070a2b00 😎 Browse the preview: https://deploy-preview-1801--dev-storybook-bloom.netlify.app |
|
✔️ Deploy Preview for dev-partners-bloom ready! 🔨 Explore the source changes: a6b73ca 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/61405947287529000829119a 😎 Browse the preview: https://deploy-preview-1801--dev-partners-bloom.netlify.app |
|
✔️ Deploy Preview for dev-bloom ready! 🔨 Explore the source changes: a6b73ca 🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/61405947c6f74d00076a415f 😎 Browse the preview: https://deploy-preview-1801--dev-bloom.netlify.app |
|
✔️ Deploy Preview for clever-edison-cd22c1 ready! 🔨 Explore the source changes: 609ce3b 🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/6138cf30e9ffad0007ca1dbc 😎 Browse the preview: https://deploy-preview-1801--clever-edison-cd22c1.netlify.app |
There was a problem hiding this comment.
Hey @pbn4 ,
For the most part this looks great. I do have a few questions though:
- should roles be required when inviting a user? If we don't assign them any roles, then they won't be able to login
- I think jurisdiction should also be required and not empty
- should the invite endpoint check if the user exists? What happens with what's here and a user with that email already exists?
If we make roles and jurisdiction required, then I think we should have the tests you wrote updated to include cases where those values are not passed in. Likewise with trying to invite a user with an email that already exists.
| @IsOptional() | ||
| @IsDefined({ groups: [ValidationsGroupsEnum.default] }) | ||
| @Type(() => UserRolesCreateDto) | ||
| roles?: UserRolesCreateDto | null |
There was a problem hiding this comment.
Should roles be optional for the invite? If we don't assign them isPartner, then they won't be able to access the portal.
There was a problem hiding this comment.
should roles be required when inviting a user? If we don't assign them any roles, then they won't be able to login
@seanmalbert We don't have any logic in place to prevent partners portal login based on roles, any user can login in (applicants too), but yet for this use case it makes sense to make them required
I think jurisdiction should also be required and not empty
I have not made it required because in the designs there is no jurisidction input.
should the invite endpoint check if the user exists? What happens with what's here and a user with that email already exists?
invite reuses UserService.createUser which in turn checks that and throws:
EMAIL_IN_USE: { message: "emailInUse", status: HttpStatus.BAD_REQUEST },
There was a problem hiding this comment.
@seanmalbert Why jurisdictions should be required?
There was a problem hiding this comment.
@pbn4 ,
This doesn't matter so much for user's assigned with isPartner, since they are directly assigned listings and can't create them, but for admin users, they need to be assigned a jurisdiction so we can know which jurisdictions they can create listings in. The mocks don't show it, but if an admin has access to more than one jurisdiction there needs to be an option to select which jurisdictions the new user has. If the admin is only assigned to one, which will be the case for everyone but us (for now anyway), then the input will be hidden with the value set to the jurisdiction they have. So on the backend with these new endpoints, we also need to ensure that the jurisdictions passed into creating a partner user are limited to what the current user has. I think we already updated the user update function to check this.
) * Add POST /users/invite and extend /confirm with optional password * Update CHANGELOG.md * Add missing .circleci env variables (PARNTERS_PORTAL_URL) * Fix user.service.spec.ts and email.service.spec.ts tests * Make UserInviteDto roles and jurisdictions required * Fix imports in user.service.spec.ts * Fix e2e tests related to jurisidctions and user roles being required now in UserInviteDto
…oom-housing#1801) * Add POST /users/invite and extend /confirm with optional password * Update CHANGELOG.md * Add missing .circleci env variables (PARNTERS_PORTAL_URL) * Fix user.service.spec.ts and email.service.spec.ts tests * Make UserInviteDto roles and jurisdictions required * Fix imports in user.service.spec.ts * Fix e2e tests related to jurisidctions and user roles being required now in UserInviteDto
Pull Request Template
Issue
Addresses # (#1653)
Description
This change add
POST /users/inviteendpoint and extendsPUT /users/confirmwith optional password change.There is a new required environment variable:
PARTNERS_PORTAL_URLwhich has to be added to every heroku app, otherwise an app will crash.Invite flow:
UserInviteDtoto/users/invitehttp://partners_portal_hostname/?token=<token>ConfirmDtowith new optional password fieldType of change
How Can This Be Tested/Reviewed?
Please describe the tests that you ran to verify your changes. Provide instructions so we can review. Please also list any relevant details for your test configuration
Checklist: