SQSERVICES 1011 Servantify Galley/Team API by battermann · Pull Request #2008 · wireapp/wire-server

battermann · 2021-12-20T15:55:39Z

https://wearezeta.atlassian.net/browse/SQSERVICES-1011

PUT /teams/:tid

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
changelog.d contains the following bits of information (details):
- A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

battermann · 2021-12-22T15:32:35Z

libs/wire-api/src/Wire/API/Team.hs

+  deriving (S.ToSchema) via (Schema NonBindingNewTeam)
+
+instance ToSchema NonBindingNewTeam where
+  schema = error "todo"


I missed this, will fix it

argh!! we should not have non-binding teams any more in the code, they have never been implemented on the client side and have been dead code since the beginning. i expect this to take less than an hour, but now i want to remove non-binding teams again. immediately!

fisx

sorry about missing the non-binding teams thing in the issue, but i'm hopeful that most of the code is still going to be usdful for non-dead end-points?

I can take another look tomorrow when the last hole is filled, then this should be good to merge.

fisx · 2021-12-22T21:08:25Z

libs/wire-api/test/golden/Test/Wire/API/Golden/Generated/NewTeamMember_team.hs

 testObject_NewTeamMember_team_20 :: NewTeamMember
 testObject_NewTeamMember_team_20 =
-  ( newNewTeamMember
+  ( mkNewTeamMember


it would be helpful if you did these renamings in separate PRs.

fisx · 2021-12-22T21:10:07Z

services/galley/src/Galley/API/Teams.hs

  Sem r Response
 getTeamMemberH (zusr ::: tid ::: uid ::: _) = do
  (member, withPerms) <- getTeamMember zusr tid uid
+  -- pure $ member & permissions .~ \p -> guard (withPerms member) $> p


pcapriotti

Looks good to me, except for the missing schema. I guess since we don't need this endpoint at all, it's maybe better if we just remove it instead of servantifying it (and then we don't need the missing schema either).

I left some minor comments, most of which are about mistakes that I made.

pcapriotti · 2021-12-23T06:58:00Z

libs/wire-api/src/Wire/API/Team/Member.hs

+
+deriving instance Show (PermissionType tag) => Show (TeamMember' tag)
+
+deriving instance Generic (TeamMember' tag)


This one instance has no context, so it doesn't need to be standalone.

pcapriotti · 2021-12-23T06:58:27Z

libs/wire-api/src/Wire/API/Team/Member.hs

 -- FUTUREWORK:
 -- There must be a cleaner way to do this, with a separate type
 -- instead of logic in the JSON instance.


We can remove this FUTUREWORK now.

pcapriotti · 2021-12-23T06:59:50Z

libs/wire-api/src/Wire/API/Team/Member.hs

+
+deriving instance (Show (PermissionType tag)) => Show (NewTeamMember' tag)
+
+deriving instance (Generic (PermissionType tag)) => Generic (NewTeamMember' tag)


This doesn't need a context, so it can also be a normal deriving clause.

pcapriotti · 2021-12-23T07:01:12Z

libs/wire-api/src/Wire/API/Team/Member.hs

+invitation :: Lens' TeamMember (Maybe (UserId, UTCTimeMillis))
+invitation = newTeamMember . nInvitation
+
+-- JSON serialisation utilities (TODO: remove after servantification)


Since these are staying for the time being, we should turn the TODO into a FUTUREWORK.

pcapriotti · 2021-12-23T07:04:03Z

services/galley/src/Galley/API/Teams.hs

  Sem r Response
 getTeamMemberH (zusr ::: tid ::: uid ::: _) = do
  (member, withPerms) <- getTeamMember zusr tid uid
+  -- pure $ member & permissions .~ \p -> guard (withPerms member) $> p


Suggested change

-- pure $ member & permissions .~ \p -> guard (withPerms member) $> p

fisx · 2021-12-24T12:05:05Z

testObject_NewUser_user_7.json:                                                 FAIL
test/golden/Test/Wire/API/Golden/Runner.hs:61:
NewUser: FromJSON of testObject_NewUser_user_7.json should match object
expected: Success (NewUser {newUserDisplayName = Name {fromName = "test name"}, newUserUUID = Nothing, newUserIdentity = Just (PhoneIdentity (Phone {fromPhone = "+12345678"})), newUserPict = Nothing, newUserAssets = [], newUserAccentId = Nothing, newUserEmailCode = Nothing, newUserPhoneCode = Nothing, newUserOrigin = Just (NewUserOriginTeamUser (NewTeamCreator (BindingNewTeamUser {bnuTeam = BindingNewTeam (NewTeam {_newTeamName = Range {fromRange = "\fe\ENQ\1011760zm"}, _newTeamIcon = Range {fromRange = "Coq\52427\v\182208"}, _newTeamIconKey = Just (Range {fromRange = "\ACKc\151665L ,"}), _newTeamMembers = Nothing}), bnuCurrency = Just XUA}))), newUserLabel = Nothing, newUserLocale = Nothing, newUserPassword = Just PlainTextPassword <hidden>, newUserExpiresIn = Nothing, newUserManagedBy = Nothing})
but got: Success (NewUser {newUserDisplayName = Name {fromName = "test name"}, newUserUUID = Nothing, newUserIdentity = Just (PhoneIdentity (Phone {fromPhone = "+12345678"})), newUserPict = Nothing, newUserAssets = [], newUserAccentId = Nothing, newUserEmailCode = Nothing, newUserPhoneCode = Nothing, newUserOrigin = Just (NewUserOriginTeamUser (NewTeamCreator (BindingNewTeamUser {bnuTeam = BindingNewTeam (NewTeam {_newTeamName = Range {fromRange = "\fe\ENQ\1011760zm"}, _newTeamIcon = Range {fromRange = "Coq\52427\v\182208"}, _newTeamIconKey = Just (Range {fromRange = "\ACKc\151665L ,"}), _newTeamMembers = Just ()}), bnuCurrency = Just XUA}))), newUserLabel = Nothing, newUserLocale = Nothing, newUserPassword = Just PlainTextPassword <hidden>, newUserExpiresIn = Nothing, newUserManagedBy = Nothing})

_newTeamMembers is landing on Just () instead of Nothing. this looks like we collapse two different haskell values on the same json. not sure if there's a good solution. (I'll also take a hack to make the golden tests pass!)

pcapriotti · 2022-01-03T06:17:34Z

I merged the schemas for binding and non-binding NewTeam. This should also fix the tests.

battermann and others added 11 commits December 20, 2021 15:54

wip

cf30fb3

wip

1a7f6a5

wip

3d5d7b2

Make it possible to hide team member permissions

de39daf

ToSchema instances for types in Team.Member

91f0ae9

fix tests

65274d3

fix failing tests

04d7480

fix compile errors in integration tests

e296082

fixed compile errors

a7af414

changelog update

50f53d8

removed comments

92a3674

battermann requested review from fisx and pcapriotti December 22, 2021 14:27

battermann marked this pull request as ready for review December 22, 2021 14:45

battermann commented Dec 22, 2021

View reviewed changes

fisx reviewed Dec 22, 2021

View reviewed changes

pcapriotti approved these changes Dec 23, 2021

View reviewed changes

battermann added 2 commits December 23, 2021 14:18

ToSchema instances for team models

44e4227

fixed PR issues

78789c1

Fix and simplify NewTeam schemas

6fa2e2a

battermann merged commit f6fd70d into develop Jan 3, 2022

battermann deleted the SQSERVICES-1011-servantify-gally-team-api branch January 3, 2022 08:28

akshaymankar mentioned this pull request Jan 18, 2022

Release 2022-01-18 #2055

Merged


		deriving instance Show (PermissionType tag) => Show (TeamMember' tag)

		deriving instance Generic (TeamMember' tag)


		deriving instance (Show (PermissionType tag)) => Show (NewTeamMember' tag)

		deriving instance (Generic (PermissionType tag)) => Generic (NewTeamMember' tag)

Conversation

battermann commented Dec 20, 2021 • edited by fisx Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx commented Dec 24, 2021

Uh oh!

pcapriotti commented Jan 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

battermann commented Dec 20, 2021 •

edited by fisx

Loading