Conference call initiation flag for personal accounts #1811

fisx · 2021-09-25T09:00:16Z

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

Notes to the reviewer: I recommend looking at the headlines of the individual commits and get a rough idea of what's in them, and then read the whole thing in one step.

The issue describes roughly what's happening here. Some more random remarks:

you may want to start reading here
d172c7f PUT/DELETE are needed mostly for integration testing and QA; in the wild, GET should be the only thing that's needed.
dca1745 proved to be unnecessary (the integration tests that was needed for ended up in brig rather than galley), but I kept it because I think the structure is actually better now.

Checklist

fisx · 2021-09-25T10:27:20Z

ci failed in docker images:

#10 exporting to oci image format
#10 sha256:69a2560eef4d3ece902c3b5149d142e9bd132f25db0bc9e35b94201534c415d2
#10 exporting layers
#10 exporting layers 5.8s done
#10 exporting manifest sha256:c26eefe08db72c9c7af863b563996b898a2333abc38720136c921ce866c6f168
#10 exporting manifest sha256:c26eefe08db72c9c7af863b563996b898a2333abc38720136c921ce866c6f168 0.0s done
#10 exporting config sha256:6069f7d6395d507224780259c7d9dc938c3af30cdccdd3391ef1b6a79b4ed7b0 0.0s done
#10 ERROR: no active session for 7mugv1yl1d0w03oe0vf1geo94: context deadline exceeded
------
 > exporting to oci image format:
------
error: failed to solve: rpc error: code = DeadlineExceeded desc = no active session for 7mugv1yl1d0w03oe0vf1geo94: context deadline exceeded
FATA[0139] failed to build: build: exit status 1        
FATA[0139] failed to run task: exit status 1

pretty sure that's unrelated...

With default for NULL and default for new records.

(will need this in a moment to generate servant-clients in galley integration tests.)

(Turns out I didn't need to move the servant types to wire-api after all, the tests need to be in brig because they require the brig server config. But I'm pretty sure we're going to need that stuff in galley eventually...)

I forgot to actually write this default into newly created users if available. Detail: if you want `defaultIfNew` to be `null` (so you can change your mind about teh default for everybody later), just omit the line in the yaml file. This wasn't possible before, as there is also a hard-coded default in the source.

fisx · 2021-09-27T14:50:52Z

I need to finish the checklist, but this should be ready for review as it is. I expect the remaining changes to be very small and isolated.

fisx · 2021-09-27T17:37:28Z

Hum, I think this last(?) one is still genuine.

      All features:                                                                                                                           FAIL
        Exception: Assertions failed:
         2: Just (Object (fromList [("legalhold",Object (fromList [("status",String "disabled")])),("sso",Object (fromList [("status",String "disabled")])),("validateSAMLemails",Object (fromList [("status",String "disabled")])),("digitalSignatures",Object (fromList [("status",String "disabled")])),("fileSharing",Object (fromList [("status",String "enabled")])),("conferenceCalling",Object (fromList [("status",String "disabled")])),("appLock",Object (fromList [("status",String "enabled"),("config",Object (fromList [("enforceAppLock",Bool False),("inactivityTimeoutSecs",Number 60.0)]))])),("classifiedDomains",Object (fromList [("status",String "enabled"),("config",Object (fromList [("domains",Array [String "example.com"])]))])),("searchVisibility",Object (fromList [("status",String "disabled")]))])) =/= Just (Object (fromList [("legalhold",Object (fromList [("status",String "disabled")])),("sso",Object (fromList [("status",String "disabled")])),("validateSAMLemails",Object (fromList [("status",String "disabled")])),("digitalSignatures",Object (fromList [("status",String "disabled")])),("fileSharing",Object (fromList [("status",String "enabled")])),("conferenceCalling",Object (fromList [("status",String "enabled")])),("appLock",Object (fromList [("status",String "enabled"),("config",Object (fromList [("enforceAppLock",Bool False),("inactivityTimeoutSecs",Number 60.0)]))])),("classifiedDomains",Object (fromList [("status",String "enabled"),("config",Object (fromList [("domains",Array [String "example.com"])]))])),("searchVisibility",Object (fromList [("status",String "disabled")]))]))
        
        Response was:
        
        Response {responseStatus = Status {statusCode = 200, statusMessage = "OK"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Mon, 27 Sep 2021 17:29:10 GMT"),("Server","Warp/3.3.13"),("Content-Encoding","gzip"),("Content-Type","application/json;charset=utf-8")], responseBody = Just "{\"legalhold\":{\"status\":\"disabled\"},\"sso\":{\"status\":\"disabled\"},\"validateSAMLemails\":{\"status\":\"disabled\"},\"digitalSignatures\":{\"status\":\"disabled\"},\"fileSharing\":{\"status\":\"enabled\"},\"conferenceCalling\":{\"status\":\"disabled\"},\"appLock\":{\"status\":\"enabled\",\"config\":{\"enforceAppLock\":false,\"inactivityTimeoutSecs\":60}},\"classifiedDomains\":{\"status\":\"enabled\",\"config\":{\"domains\":[\"example.com\"]}},\"searchVisibility\":{\"status\":\"disabled\"}}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:89:5 in bilge-0.22.0-2K7mJKDUfGc17BkrPwuOOW:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:107:19 in bilge-0.22.0-2K7mJKDUfGc17BkrPwuOOW:Bilge.Assert
          !!!, called at test/integration/API/Teams/Feature.hs:393:3 in main:API.Teams.Feature

pcapriotti

Looks good. I left a few minor comments. Feel free to address them in a separate PR, if this needs to be merged quickly.

pcapriotti · 2021-09-28T12:27:40Z

services/brig/src/Brig/API/User.hs

@@ -402,6 +404,11 @@ createUser new = do
          return Nothing
      pure pdata

+initAccountFeatureConfig :: UserId -> AppIO ()
+initAccountFeatureConfig uid = do
+  mbCciDefNew <- asks (^. settings . getAfcConferenceCallingDefNewMaybe)


You can just use view with a lens instead of ask . view.

7710228

(thanks :)

pcapriotti · 2021-09-28T12:31:35Z

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

+            >>= either (liftIO . throwIO . ErrorCall . ("putAccountFeatureConfigClient: " <>) . show) pure
+
+        mbStatus' <- getAccountFeatureConfigClient brigep mgr uid
+        liftIO $ assertEqual "1" (Right status) mbStatus'


Could you use write some meaningful error messages here, so when these fail we know what went wrong?

pcapriotti · 2021-09-28T12:38:46Z

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

@@ -206,10 +219,12 @@ setFeatureStatusNoConfig applyState tid status = do
    Event.Event Event.Update (Public.knownTeamFeatureName @a) (EdFeatureWithoutConfigChanged newStatus)
  pure newStatus

-getSSOStatusInternal :: Maybe TeamId -> Galley (Public.TeamFeatureStatus 'Public.TeamFeatureSSO)
+type GetFeatureInternalParam = Either (Maybe UserId) TeamId


I guess this is alright, but for the future would it make sense to make this a type family dependent on the feature flag, so that we get more type safety?

5a3f4ee

fully agreed, yet de-prioritized (sorry...)

pcapriotti · 2021-09-28T12:47:48Z

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

+    getAccountFeatureConfigClientM ::
+      UserId -> Client.ClientM Public.TeamFeatureStatusNoConfig
+    ( _
+        :<|> getAccountFeatureConfigClientM
+        :<|> _
+        :<|> _
+      ) = Client.client (Proxy @IAPI.API)


Ah, this is pretty ugly. It took me a while to parse it. Wouldn't it make sense to change the API definition to be a record like the other ones, so we wouldn't need to do this?

I though about that, and was too lazy. But you're right, I'll try to make the time!

01955d8

i was being a bit lazy. hope this is better?

Sure, it's fine, but the other API definitions are now using the Generic-based approach where you can just define a single record data type that contains all of the endpoints, instead of separate types joined by :<|>. See for example https://github.com/wireapp/wire-server/blob/develop/libs/wire-api/src/Wire/API/Routes/Public/Galley.hs#L75.

…counts

(brig.yaml is different in our CI and in my local integration test setup.)

fisx force-pushed the SQSERVICES-763-conf-call-init-personal-accounts branch 2 times, most recently from f33c959 to c3a17f9 Compare September 25, 2021 09:16

fisx force-pushed the SQSERVICES-763-conf-call-init-personal-accounts branch from 5c98e0d to 5d871b2 Compare September 25, 2021 20:12

fisx changed the title ~~Conference call initiation flag for personal accounts~~ Conference call initiation flag for personal accounts [skip ci] Sep 25, 2021

fisx force-pushed the SQSERVICES-763-conf-call-init-personal-accounts branch 2 times, most recently from 1ae03c0 to 1efeacf Compare September 27, 2021 10:35

fisx changed the title ~~Conference call initiation flag for personal accounts [skip ci]~~ Conference call initiation flag for personal accounts Sep 27, 2021

fisx added 15 commits September 27, 2021 16:38

Cassandra migration: personal account feature "conf call init".

f46c25b

Clean up after ormolu.

6a184db

Add optSettings.featureFlags.conferenceCalling to brig.yaml.

7f84300

With default for NULL and default for new records.

hi ci

dc8c97a

Internal brig PUT/GET/DELETE end-points for account features.

64c3f13

Move brig's internal servant api to wire-api.

66bd22f

(will need this in a moment to generate servant-clients in galley integration tests.)

Fixup d172c7f

5b21986

Fixup dca1745

5cb93ae

Integration tests for account features.

5699f8d

(Turns out I didn't need to move the servant types to wire-api after all, the tests need to be in brig because they require the brig server config. But I'm pretty sure we're going to need that stuff in galley eventually...)

Fixes.

b10f494

Fixes.

17fd447

Fixes.

0c1db1a

Fixes.

30bcdd0

make git-add-cassandra-schema

8c4f64e

fisx force-pushed the SQSERVICES-763-conf-call-init-personal-accounts branch from 3a72cf3 to 80607cb Compare September 27, 2021 14:40

Add docs.

5df1d10

fisx marked this pull request as ready for review September 27, 2021 14:50

Fixup.

8b513b8

Fixup.

6ecbffc

hi ci

641081a

pcapriotti approved these changes Sep 28, 2021

View reviewed changes

fisx added 7 commits September 28, 2021 16:10

Merge branch 'develop' into SQSERVICES-763-conf-call-init-personal-ac…

1dd1a16

…counts

Hard-wired default for all account features should be "enabled".

2b36e61

CHANGELOG

42aaa2f

hi ci

e6bc141

Fixup 2b36e61.

1847892

Fix swagger syntax.

4546289

Fixup 2b36e61.

d3b985d

(brig.yaml is different in our CI and in my local integration test setup.)

fisx merged commit 7e76125 into develop Sep 29, 2021

fisx deleted the SQSERVICES-763-conf-call-init-personal-accounts branch September 29, 2021 05:49

fisx mentioned this pull request Sep 29, 2021

Conference call initiation as a personal account feature: errata #1818

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Conference call initiation flag for personal accounts #1811

Conference call initiation flag for personal accounts #1811

Uh oh!

fisx commented Sep 25, 2021 •

edited

Loading

Uh oh!

fisx commented Sep 25, 2021

Uh oh!

fisx commented Sep 27, 2021

Uh oh!

fisx commented Sep 27, 2021

Uh oh!

pcapriotti left a comment

Uh oh!

pcapriotti Sep 28, 2021

Uh oh!

fisx Sep 29, 2021

Uh oh!

pcapriotti Sep 28, 2021

Uh oh!

fisx Sep 29, 2021

Uh oh!

pcapriotti Sep 28, 2021

Uh oh!

fisx Sep 29, 2021

Uh oh!

pcapriotti Sep 28, 2021

Uh oh!

fisx Sep 28, 2021

Uh oh!

fisx Sep 29, 2021

Uh oh!

pcapriotti Sep 29, 2021

Uh oh!

Uh oh!

Conference call initiation flag for personal accounts #1811

Conference call initiation flag for personal accounts #1811

Uh oh!

Conversation

fisx commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

fisx commented Sep 25, 2021

Uh oh!

fisx commented Sep 27, 2021

Uh oh!

fisx commented Sep 27, 2021

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!

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!

Uh oh!

fisx commented Sep 25, 2021 •

edited

Loading