Allow displaying emails of users #719

ChrisPenner · 2019-04-15T14:07:57Z

The behaviour of this setting is as such; there is a new config called setEmailVisibility.
It has two valid settings:

visible_if_on_team: Will display a users email to EVERYONE if they are on a Team
visible_to_self: The current behaviour; only expose email on the /self endpoint

Yes this behaviour seems a bit strange, but after chatting with Tiago several times it seems this is all that we currently need for EY. It's certainly possible to add an additional option visible_within_teams or something in the future but it adds non-trivial complexity to the code so we'll leave it until necessary.

Note that this PR adds a '/i/settings' endpoint currently used only for testing; and indeed shouldn't be used for anything else because it allows the settings of a server to differ from its config file which is BAD. It adds a test helper which allow mutating the config for the scope of a single test.

Note that this PR also uses the barbies lib for working with 'partial' structures; specifically it allows us to define the structure once and use it in cases where the JSON may be missing fields; this makes endpoints like PUT /i/settings much easier to write, at the expense of making other uses slightly less clear. Another option would be to write PUT /i/settings in terms of untyped JSON, i.e. Value which would also work but which would be less clean in that particular case. I'm open to opinions on this.

mutableSettings is currently no different from settings except that they are able to be changed. Open to thoughts on this as well.

TODO:

add EmailVisibility setting to all required config-maps

libs/brig-types/src/Brig/Types/User.hs

ChrisPenner · 2019-04-15T14:13:47Z

libs/brig-types/src/Brig/Types/User.hs

+
+instance FromJSON EmailVisibility where
+    parseJSON = withText "EmailVisibility" $ \case
+        "visible_if_on_team" -> pure EmailVisibleIfOnTeam


This name isn't super clear about its behaviour. Anyone have suggestions?

ChrisPenner · 2019-04-15T14:14:13Z

services/brig/brig.integration.yaml

@@ -141,6 +141,8 @@ optSettings:
  setDefaultLocale: en
  setMaxTeamSize: 32
  setMaxConvSize: 16
+optMutableSettings:


Is there a better name for this? It's not great that our testing strategy leaks into our config 😢

One more way to test this would be to create a brig Application with a different config, and test against that. done here

services/brig/src/Brig/App.hs

services/brig/src/Brig/Options.hs

fisx

Looks good! Most comments are nit-picks, use at your own disgression.

But yes, after reading all of this I feel strongly that it would be better to launch a separate brig app with the malleable options in the tests, and revert all your (admittedly very cool) production code about para-malleable options.

If you disagree or if you want more details let's talk!

fisx · 2019-04-16T11:34:52Z

services/brig/brig.integration.yaml

@@ -141,6 +141,8 @@ optSettings:
  setDefaultLocale: en
  setMaxTeamSize: 32
  setMaxConvSize: 16
+optMutableSettings:


One more way to test this would be to create a brig Application with a different config, and test against that. done here

fisx · 2019-04-16T11:45:07Z

services/brig/src/Brig/Options.hs

@@ -21,7 +24,7 @@ import qualified Data.Yaml   as Y

 newtype Timeout = Timeout
    { timeoutDiff :: DiffTime
-    } deriving (Eq, Enum, Ord, Num, Real, Fractional, RealFrac, Show)
+    } deriving newtype (Eq, Enum, Ord, Num, Real, Fractional, RealFrac, Show)


What does this do? I could not find it in the ghc docs.

It's called DerivingStrategies 😄 ; it's a bit tough to find real docs on; but it's a simple enough concept. Just allows you to tell it how you want it to do the derivation; here I'm saying to explicitly use GND rather than some other strategy (e.g. DeriveAnyClass)

https://gitlab.haskell.org/ghc/ghc/wikis/commentary/compiler/deriving-strategies

There's no downside to annotating deriving strategies; so I like to do it whenever I'm deriving things (when I remember).

Examples of strategies are:

anyclass: Just write instance Blah x without an implementation. This is sufficient for things like ToJSON/FromJSON if there's a Generic instance, etc.

newtype: Use GND

stock: The standard one for Eq, Show, Functor, etc.

services/brig/src/Brig/Options.hs

fisx · 2019-04-16T11:52:01Z

services/brig/test/integration/API/Settings.hs

+jsonField f u = u ^? key f >>= maybeFromJSON
+
+testUsersEmailShowsEmailsIfExpected :: Brig -> Galley -> EmailVisibilityAssertion -> Http ()
+testUsersEmailShowsEmailsIfExpected brig galley shouldShowEmail = do


Suggested change

testUsersEmailShowsEmailsIfExpected brig galley shouldShowEmail = do

testUsersEmailShowsEmailsIffExpected brig galley shouldShowEmail = do

fisx · 2019-04-16T11:52:17Z

services/brig/test/integration/API/Settings.hs

+            <$> decodeBody r
+
+testGetUserEmailShowsEmailsIfExpected :: Brig -> Galley -> EmailVisibilityAssertion -> Http ()
+testGetUserEmailShowsEmailsIfExpected brig galley shouldShowEmail = do


Suggested change

testGetUserEmailShowsEmailsIfExpected brig galley shouldShowEmail = do

testGetUserEmailShowsEmailsIffExpected brig galley shouldShowEmail = do

services/brig/test/integration/API/Settings.hs

fisx · 2019-04-16T11:56:00Z

services/brig/test/integration/API/Settings.hs

+testUsersEmailShowsEmailsIfExpected brig galley shouldShowEmail = do
+    (creatorId, tid) <- createUserWithTeam brig galley
+    (otherTeamCreatorId, otherTid) <- createUserWithTeam brig galley
+    userA <- createTeamMember brig galley creatorId tid Team.fullPermissions


Suggested change

userA <- createTeamMember brig galley creatorId tid Team.fullPermissions

userA <- createTeamMember brig galley creatorId tid (Team.rolePermissions Team.RoleMember)

No strong opinion, ideally (for the sake of coverage, not for the sake of test runtimes) you'd check all roles here. :)

fisx · 2019-04-16T12:00:26Z

services/brig/test/integration/API/Settings.hs

+    userB <- createTeamMember brig galley otherTeamCreatorId otherTid Team.fullPermissions
+    nonTeamUser <- createUser "joe" brig
+    let expectations :: [(UserId, Maybe Email)]
+        expectations =


I personally prefer test code to be less redundant (but I think I am in a minority among people who write tests).

Also I haven't thought it through in this case. It looks like it'd be easy to share more code, but I may well be wrong.

It would be much easier to share code if we had a better 'fixtures' system in our tests; I've already collapsed about 4 tests down into 2 by allowing to pass the behaviour in, if I squish it down much further I fear it will hurt the already sketchy readability. If you have a concrete code suggestion I'll gladly accept it 😄

ChrisPenner · 2019-04-16T12:36:14Z

I feel strongly that it would be better to launch a separate brig app with the malleable options in the tests

@tiago-loureiro I'd love your opinion on this; I think all approaches are pretty annoying and ugly, perhaps @fisx is correct that it's best to shuffle the gross-ness into the tests rather than have it in the server though.

fisx

After discussing this further, I believe we should go ahead with this (and not rewrite the tests to work on a separately launched brig Application now, but do it in a separate PR). There are a few smaller changes I would like to happen, especially the change about which user calls the end-points in the tests, but other than that good to go for me!

fisx · 2019-04-17T11:41:38Z

services/brig/test/integration/API/Settings.hs

+                     , if shouldShowEmail NoTeam then userEmail nonTeamUser
+                                                 else Nothing)
+                   ]
+    get (brig . zUser (userId userB) . path "users" . queryItem "ids" uids) !!! do


Suggested change

get (brig . zUser (userId userB) . path "users" . queryItem "ids" uids) !!! do

get (brig . zUser (userId userA) . path "users" . queryItem "ids" uids) !!! do

it doesn't make a difference with the current definition of expectEmailVisible, but it's still good to get this right...

fisx · 2019-04-17T11:41:51Z

services/brig/test/integration/API/Settings.hs

+                                                             else Nothing)
+            ]
+    forM_ expectations $ \(uid, expectedEmail) ->
+        get (brig . zUser (userId userB) . paths ["users", toByteString' uid]) !!! do


Suggested change

get (brig . zUser (userId userB) . paths ["users", toByteString' uid]) !!! do

get (brig . zUser (userId userA) . paths ["users", toByteString' uid]) !!! do

see above

ChrisPenner · 2019-04-23T11:28:17Z

Closing in favour of #724

ChrisPenner added 8 commits April 12, 2019 12:33

Add Email to UserProfile and friends

028e259

Add mutable settings to Env

3936785

Implement putSettings endpoint via barbies approach

abaf242

Fix bad imports

90c26b4

Initial setup of email visibility tests

f2edbe2

Working tests asserting email visibility behaviour

f84734c

Add tests for email visibility for /users/:id

150aaee

Update Swagger docs

bb61f58

ChrisPenner commented Apr 15, 2019

View reviewed changes

libs/brig-types/src/Brig/Types/User.hs Outdated Show resolved Hide resolved

ChrisPenner commented Apr 15, 2019

View reviewed changes

services/brig/src/Brig/App.hs Show resolved Hide resolved

ChrisPenner commented Apr 15, 2019

View reviewed changes

services/brig/src/Brig/Options.hs Show resolved Hide resolved

Switch putSettings -> patchSettings

d8ae071

ChrisPenner changed the title ~~[WIP] Allow displaying emails of users~~ Allow displaying emails of users Apr 15, 2019

Move EmailVisibility to Options file

6f8df0e

fisx requested changes Apr 16, 2019

View reviewed changes

fisx approved these changes Apr 17, 2019

View reviewed changes

Force CI rebuild

176d32d

ChrisPenner mentioned this pull request Apr 23, 2019

Allow displaying emails of users v2.0 #724

Merged

ChrisPenner closed this Apr 23, 2019

jschaul mentioned this pull request May 13, 2019

Release 2019 05 13 #756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow displaying emails of users #719

Allow displaying emails of users #719

ChrisPenner commented Apr 15, 2019 •

edited

Loading

ChrisPenner Apr 15, 2019

ChrisPenner Apr 15, 2019 •

edited

Loading

fisx Apr 16, 2019

fisx left a comment

fisx Apr 16, 2019

fisx Apr 16, 2019

ChrisPenner Apr 16, 2019 •

edited

Loading

fisx Apr 16, 2019

fisx Apr 16, 2019

fisx Apr 16, 2019

fisx Apr 16, 2019

fisx Apr 16, 2019

ChrisPenner Apr 16, 2019

ChrisPenner commented Apr 16, 2019 •

edited

Loading

fisx left a comment

fisx Apr 17, 2019

fisx Apr 17, 2019

ChrisPenner commented Apr 23, 2019

	testUsersEmailShowsEmailsIfExpected brig galley shouldShowEmail = do
	testUsersEmailShowsEmailsIffExpected brig galley shouldShowEmail = do

	testGetUserEmailShowsEmailsIfExpected brig galley shouldShowEmail = do
	testGetUserEmailShowsEmailsIffExpected brig galley shouldShowEmail = do

	userA <- createTeamMember brig galley creatorId tid Team.fullPermissions
	userA <- createTeamMember brig galley creatorId tid (Team.rolePermissions Team.RoleMember)

	get (brig . zUser (userId userB) . path "users" . queryItem "ids" uids) !!! do
	get (brig . zUser (userId userA) . path "users" . queryItem "ids" uids) !!! do

	get (brig . zUser (userId userB) . paths ["users", toByteString' uid]) !!! do
	get (brig . zUser (userId userA) . paths ["users", toByteString' uid]) !!! do

Allow displaying emails of users #719

Allow displaying emails of users #719

Conversation

ChrisPenner commented Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

ChrisPenner Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisPenner Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisPenner commented Apr 16, 2019 • edited Loading

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisPenner commented Apr 23, 2019

ChrisPenner commented Apr 15, 2019 •

edited

Loading

ChrisPenner Apr 15, 2019 •

edited

Loading

ChrisPenner Apr 16, 2019 •

edited

Loading

ChrisPenner commented Apr 16, 2019 •

edited

Loading