Always carry a Role in the Invitation type. #577
Conversation
without this, the default role would have to be fixed twice (once in the backend, and once in team settings), and we would risk running out of sync.
fisx
changed the title
Invitation type.
| @@ -49,7 +49,7 @@ instance ToJSON InvitationRequest where | |||
| instance FromJSON Invitation where | |||
| parseJSON = withObject "invitation" $ \o -> | |||
| Invitation <$> o .: "team" | |||
| <*> o .:? "role" | |||
| <*> o .: "role" | |||
There was a problem hiding this comment.
old clients won't have this field, this change is backwards-incompatible.
| invitation :: Model | ||
| invitation = defineModel "Invitation" $ do | ||
| description "An invitation to join Wire" | ||
| property "inviter" bytes' $ | ||
| description "User ID of the inviter" | ||
| property "role" role $ do | ||
| description "User ID of the inviter" |
There was a problem hiding this comment.
sorry.
... third try? (or is it 4th?)
services/brig/src/Brig/Team/API.hs
Outdated
| @@ -166,7 +166,8 @@ createInvitation (_ ::: uid ::: tid ::: req) = do | |||
| body :: InvitationRequest <- parseJsonBody req | |||
| idt <- maybe (throwStd noIdentity) return =<< lift (fetchUserIdentity uid) | |||
| from <- maybe (throwStd noEmail) return (emailIdentity idt) | |||
| let inviteePerms = Team.rolePermissions . fromMaybe Team.RoleMember . irRole $ body | |||
| let inviteePerms = Team.rolePermissions inviteeRole | |||
| inviteeRole = fromMaybe Team.RoleMember . irRole $ body | |||
services/brig/src/Brig/Team/DB.hs
Outdated
| @@ -162,5 +163,8 @@ countInvitations t = fromMaybe 0 . fmap runIdentity <$> | |||
| cqlSelect :: PrepQuery R (Identity TeamId) (Identity Int64) | |||
| cqlSelect = "SELECT count(*) FROM team_invitation WHERE team = ?" | |||
|
|
|||
| -- | FUTUREWORK: two weeks after https://github.com/wearezeta/backend-issues/issues/775 has been | |||
There was a problem hiding this comment.
Is this an accident, or is the rule going to be "backend-issues can be referred-to in code but not in issue comments"?
There was a problem hiding this comment.
accident. :-) will fix, too!
services/brig/src/Brig/Team/DB.hs
Outdated
| @@ -162,5 +163,8 @@ countInvitations t = fromMaybe 0 . fmap runIdentity <$> | |||
| cqlSelect :: PrepQuery R (Identity TeamId) (Identity Int64) | |||
| cqlSelect = "SELECT count(*) FROM team_invitation WHERE team = ?" | |||
|
|
|||
| -- | FUTUREWORK: two weeks after https://github.com/wearezeta/backend-issues/issues/775 has been | |||
| -- deployed, we won't find any `Nothing` role values any more; then it will be safe to change the | |||
There was a problem hiding this comment.
then it will be safe
Actually, I would remove the FUTUREWORK comment entirely; since our code is open-sourced, and this condition of "two weeks" only holds on our infrastructure, and not for anyone running wire-server at an earlier version and deciding to upgrade at some point (possibly after the default is removed and a role inserted in the database being assumed, but then creating exceptions). So Maybe Role / defaultRole should stay here.
There was a problem hiding this comment.
So Maybe Role / defaultRole should stay here.
I like this principle.
I don't like this principle applied inconsistently, however. Sometimes we leave stuff in, sometimes we deprecate things (like managed convs) but make it fairly easy for people to reenable them, sometimes we just don't communicate anything to anybody at all (like when a DB migration tool has to be run manually and we just write a tool but don't write anywhere that it has to be used).
If we decide to apply this principle consistently then I'd like everyone to explicitly agree on that because otherwise I have no idea what I'm allowed / not allowed to do and what I should / shouldn't look for when reviewing other people's PRs.
|
additionally, do we have a place where we place things we've all agreed to?
…On Wed, Jan 23, 2019 at 11:34 AM Artyom Kazak ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In services/brig/src/Brig/Team/DB.hs
<#577 (comment)>:
> @@ -162,5 +163,8 @@ countInvitations t = fromMaybe 0 . fmap runIdentity <$>
cqlSelect :: PrepQuery R (Identity TeamId) (Identity Int64)
cqlSelect = "SELECT count(*) FROM team_invitation WHERE team = ?"
+-- | FUTUREWORK: two weeks after zinfra/backend-issues#775 has been
+-- deployed, we won't find any `Nothing` role values any more; then it will be safe to change the
So Maybe Role / defaultRole should stay here.
I like this principle.
I don't like this principle applied inconsistently, however. Sometimes we
leave stuff in, sometimes we deprecate things (like managed convs) but make
it fairly easy for people to reenable them, sometimes we just don't
communicate anything to anybody at all (like when a DB migration tool has
to be run manually and we just write a tool but don't write anywhere that
it has to be used).
If we decide to apply this principle consistently then I'd like everyone
to explicitly agree on that because otherwise I have no idea what I'm
allowed / not allowed to do and what I should / shouldn't look for when
reviewing other people's PRs.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#577 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AQDAAoi-VMh1QNvij9folW7OxmKldUHwks5vGEi8gaJpZM4aKyCe>
.
|
Without this, the default role would have to be picked twice: once in the backend, and once in team settings. This is both extra work and poses the risk of running out of sync.
With these changes, the backend stores all future
Invitations in the DB with a default role that is defined near the type. For migration, we allow for database entries with missingRole, and fill in the default there again. This is marked in the code asFUTUREWORK: remove.