Skip to content
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

Use wire-api types in public endpoints (brig) #1114

Merged
merged 18 commits into from
May 28, 2020

Conversation

mheinzel
Copy link
Contributor

This guarantees that the public API only depends on types defined in wire-api (and currently also types-common), so we can't break the public API by changing internal types.

The other services will follow.

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving tentatively, please check the loop question!

services/brig/src/Brig/API/Internal.hs Show resolved Hide resolved
(Just (Public.NewTeamCreator (Public.BindingNewTeamUser (Public.BindingNewTeam t) _))) ->
sendTeamActivationMail e u p l (fromRange $ t ^. Public.newTeamName)
_ ->
sendActivationMail e u p l Nothing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this loop? (If so, the old one does, too.)

Also, I want to have an hlint rule that disallows catch-alls like this. If there was another Just _ and then a Nothing, it would have been much harder to get confused here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is very confusing here, so I think you saw two different names as the same? There are three different functions involved:

  • sendActivationMail
  • sendActivationEmail
  • sendTeamActivationMail

So at a first glance it doesn't seem like it's looping, but maybe I can make this less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried improving the pattern matching, but I don't think it really got better:

@@ -1006,9 +1007,14 @@ createUser (Public.NewUserPublic new) = do
   pure $ CreateUserResponse cok userId (Public.SelfProfile usr)
   where
     sendActivationEmail e u p l = \case
-      (Just (Public.NewTeamCreator (Public.BindingNewTeamUser (Public.BindingNewTeam t) _))) ->
-        sendTeamActivationMail e u p l (fromRange $ t ^. Public.newTeamName)
-      _ ->
+      Just (Public.NewTeamCreator newTeamUser) -> do
+        let teamName = Public._newTeamName . coerce @_ @(Public.NewTeam ()) . Public.bnuTeam $ newTeamUser
+        sendTeamActivationMail e u p l (fromRange teamName)
+      Just _ ->
+        -- not a creater
+        sendActivationMail e u p l Nothing
+      Nothing ->
+        -- not a team user at all
         sendActivationMail e u p l Nothing
     sendWelcomeEmail :: Public.Email -> CreateUserTeam -> Public.NewTeamUser -> Maybe Public.Locale -> AppIO ()
     -- NOTE: Welcome e-mails for the team creator are not dealt by brig anymore

So I think I'll just merge this instead of going through another CI cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another possibility:

@@ -1005,10 +1005,12 @@ createUser (Public.NewUserPublic new) = do
     UserAccount _ _ -> lift $ Auth.newCookie @ZAuth.User userId Public.PersistentCookie newUserLabel
   pure $ CreateUserResponse cok userId (Public.SelfProfile usr)
   where
-    sendActivationEmail e u p l = \case
-      (Just (Public.NewTeamCreator (Public.BindingNewTeamUser (Public.BindingNewTeam t) _))) ->
-        sendTeamActivationMail e u p l (fromRange $ t ^. Public.newTeamName)
-      _ ->
+    sendActivationEmail e u p l mTeamUser
+      | Just teamUser <- mTeamUser,
+        Public.NewTeamCreator creator <- teamUser,
+        let Public.BindingNewTeamUser (Public.BindingNewTeam team) _ = creator =
+        sendTeamActivationMail e u p l (fromRange $ team ^. Public.newTeamName)
+      | otherwise =
         sendActivationMail e u p l Nothing
     sendWelcomeEmail :: Public.Email -> CreateUserTeam -> Public.NewTeamUser -> Maybe Public.Locale -> AppIO ()
     -- NOTE: Welcome e-mails for the team creator are not dealt by brig anymore

Or I just add a comment like "not only a NewTeamUser, but NewTeamCreator"?

I could do either of those in #1116, not sure which. Opinions?

services/brig/src/Brig/API/Public.hs Show resolved Hide resolved
@mheinzel mheinzel merged commit 2e61305 into develop May 28, 2020
@mheinzel mheinzel deleted the mheinzel/wire-api-in-public-endpoints-brig branch May 28, 2020 13:08
@akshaymankar akshaymankar mentioned this pull request Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants