-
Notifications
You must be signed in to change notification settings - Fork 332
[WPB-22549] connect users to apps. #4970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
14b7ab8 to
9d436c0
Compare
e1925d0 to
89c8d44
Compare
services/brig/src/Brig/API/User.hs
Outdated
| Nothing -> case userTeam u of | ||
| Nothing -> pure Nothing | ||
| Just tid -> do | ||
| mApp <- AppStore.getApp (userId u) tid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this happen in UserStore?
| Store.createApp app | ||
| Store.createUser u Nothing | ||
| now <- toUTCTimeMillis <$> get | ||
| void $ addTeamMember u.id tid (Just (tUnqualified lusr, now)) R.RoleMember |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an essential fix. Before this, apps had a user record, but no team member record. (Excercise to the reader: why was that bad?)
4f892ce to
a06642e
Compare
integration/test/Test/Connection.hs
Outdated
|
|
||
| -- the same goes for apps from the same team | ||
| postConnection mem1 app `bindResponse` \resp -> do | ||
| resp.status `shouldMatchInt` 403 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still fails, because the response status is 404. shouldn't be too hard to fix.
bfbd6fa to
1186a9c
Compare
battermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new column user_type is not needed for the solution taken in this PR.
integration/test/Test/Apps.hs
Outdated
| -- make sure that matches from previous test runs don't get in the way. | ||
| catMaybes | ||
| <$> forM foundDocs (\doc -> do | ||
| tidActual :: String <- doc %. "team" & asString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: type annotation is redundant
|
|
||
| -- the same goes for apps from the same team | ||
| postConnection mem1 app `bindResponse` \resp -> do | ||
| resp.status `shouldMatchInt` 404 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 403, but not high prio issue.
| data UserType = UserTypeRegular | UserTypeApp | UserTypeBot | ||
| deriving (Eq, Show, Ord, Generic) | ||
| deriving (Arbitrary) via (GenericUniform UserType) | ||
| deriving (A.FromJSON, A.ToJSON, S.ToSchema) via (Schema UserType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: maybe this can go back into the User module?
d90af36 to
d344fec
Compare
https://wearezeta.atlassian.net/browse/WPB-22549
Checklist
changelog.d