Add database migration for the user_invites table #3536
Conversation
| invitee INTEGER NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| invited_by INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
Noticed that the users ID is SERIAL (not UUID) thus why the INTEGER type. I guess the other approach was to use the subject_identity (UUID saved as text)
There was a problem hiding this comment.
Might it make sense to add a UUID ID column to the users table, populate with autogenerated values, and then replace use of the integer ID with a UUID one?
There was a problem hiding this comment.
It's out of the scope of this effort but it's a good idea, i.e. I feel much better seeing uuids as IDs then 7 for example 😃
rdimitrov
force-pushed
the
invites-db
branch
2 times, most recently
from
728f179
to
bb19dd9
Compare
| CREATE TABLE IF NOT EXISTS user_invites( | ||
| id UUID NOT NULL DEFAULT gen_random_uuid() PRIMARY KEY, | ||
| email TEXT NOT NULL, | ||
| status invite_status NOT NULL DEFAULT 'pending', |
There was a problem hiding this comment.
What are other values of status would we use? (Do we need this column?)
There was a problem hiding this comment.
To explain further, I figured that we'd remove the row when an invitation is accepted / declined / revoked, and we'd periodically remove expired invitations.
If we do want to track how invitations are dispositioned, it feels like we should keep a separate audit log, rather than combining it with the live data.
For example, if we keep these as a log, then the unique index on (email, project) will block re-inviting someone to a project if they accidentally are removed from the project, or will delete the audit log entry.
There was a problem hiding this comment.
I was thinking of the use case where you list the invitations and see some that are pending, some expired, but it makes more sense to make that as separate audit log. Thanks!
| -- ensure there's one invite for this email per project | ||
| UNIQUE (email, project), |
There was a problem hiding this comment.
Why do we need to ensure that this is unique? What if I wanted to invite a user as both viewer and permission manager?
There was a problem hiding this comment.
I had the assumption that a user can have only one role per project or this was incorrect?
| -- create an index on the invited_by column | ||
| CREATE INDEX idx_user_invites_sponsor ON user_invites(sponsor); |
There was a problem hiding this comment.
What do we need this index for?
There was a problem hiding this comment.
I was thinking of helping the use case for "give me all invitations I've received from X".
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
|
@evankanderson - Thanks for the tips! 🙏 I like how much cleaner it got after applying your feedback 👍 |
|
|
||
| CREATE TABLE IF NOT EXISTS user_invites ( | ||
| code TEXT NOT NULL PRIMARY KEY, | ||
| email TEXT NOT NULL, |
There was a problem hiding this comment.
Removing the index on (email, project) means that we no longer have an index on email. I'd be tempted to re-add this.
There was a problem hiding this comment.
Ugh, you're right! I must have deleted it along the other stuff 🤦♂️
There was a problem hiding this comment.
I pointed out that a UNIQUE(email, project) index would prevent inviting someone to multiple roles in a project, so it was sort of my fault.
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Summary
The following adds a user_invites table.
The role column is a string where the expected values are some of 'admin', 'editor', 'viewer', 'policy_writer', 'permissions_manager' (at least for now).
Fixes: #3533
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: