Invite users via email #4539
Invite users via email #4539
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #8665 ↗︎
Details:
Review all test suite changes for PR #4539 ↗︎ |
|||||||||||||||
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
@galvana thanks for reviving this effort and addressing the main issues i'd flagged in the initial review! generally things look good with those.
i haven't tested this functionality myself manually, but i do want to make sure that we've done some recent manual testing on this, especially since it's been sitting dormant for a while, which makes it a bit more likely that some bugs would come up that slip underneath automated test coverage. have you been able to do some solid manual testing on the functionality?
the most important things i've flagged on this round are with the migration, i think. unless i'm missing something, i think those need to be resolved before we merge - and they generally tell me that we haven't tested the migration on a realistic DB state too much - so can we make sure that's well tested?
my other comments are relatively minor and should either be quick updates or are not must-haves.
let me know if you've got any questions or if i can lend a hand at all on the migration work!
| @@ -169,19 +186,23 @@ async def workers_health() -> Dict: | |||
| }, | |||
| }, | |||
| ) | |||
| async def health() -> Dict: | |||
| async def health(db: Session = Depends(get_db)) -> Dict: | |||
There was a problem hiding this comment.
k - so just to be sure, there was no functional requirement to have this included in the health check, i.e. nothing on the FE that was relying on this?
| op.f("ix_fides_user_invite_id"), "fides_user_invite", ["id"], unique=False | ||
| ) | ||
| op.add_column("fidesuser", sa.Column("email_address", CIText(), nullable=True)) | ||
| op.add_column("fidesuser", sa.Column("disabled", sa.Boolean(), nullable=False)) |
There was a problem hiding this comment.
doesn't this need a default value and/or data migration to ensure that existing records have the value populated?
There was a problem hiding this comment.
Yes! I updated this to set disabled to False by default
src/fides/api/alembic/migrations/versions/31493e48c1d8_table_updates_for_user_invite.py
Show resolved
Hide resolved
There was a problem hiding this comment.
looking good to me @galvana , thanks for this latest round of adjustments. let's ship it! 🚢
note: haven't reviewed the FE code, and didn't do manual testing myself. but assuming you've done that and trusting your discretion on the FE pieces - i'm good for this to be merged 👍
src/fides/api/alembic/migrations/versions/31493e48c1d8_table_updates_for_user_invite.py
Show resolved
Hide resolved
Passing run #8668 ↗︎
Details:
Review all test suite changes for PR #4539 ↗︎ |
|||||||||||||||
|
|
Co-authored-by: Robert Keyser <[email protected]> Co-authored-by: Allison King <[email protected]> Co-authored-by: Allison King <[email protected]>
Closes https://ethyca.atlassian.net/browse/PROD-1553
Description Of Changes
Invite users by email!
Initial design doc here: https://ethyca.atlassian.net/wiki/spaces/EN/pages/2872344650/Hackathon+2023+-+Team+1
Code Changes
Frontend
/user-managementwhile not logged in, got redirected to the login page, then signed in, you'll automatically be redirected to/user-management(where in the past, you'd go to the home screen)Backend
disabledanddisabled_reason. A user pending an invite would havedisabled=trueanddisabled_reason="pending_invite"Steps to Confirm
FIDES__ADMIN_UI__URL="http://localhost:3000"to your.envfilePre-Merge Checklist
CHANGELOG.md