Count suspended team members correctly

CHANGELOG.md

@@ -1,3 +1,17 @@
+# Next Release
+
+## Release Notes
+
+* This release makes a couple of changes to the elasticsearch mapping and requires a data migration. The correct order of upgrade is:
+  1. [Update mapping](./docs/reference/elastic-search.md#update-mapping)
+  1. Upgrade brig as usual
+  1. [Run data migration](./docs/reference/elastic-search.md#migrate-data)
+  Search should continue to work normally during this upgrade.
+
+## Bug Fixes
+
+* Fix member count of suspended teams in journal events (#1171)
+
 # 2020-07-13
 
 ## Release Notes

services/brig/brig.cabal

@@ -4,7 +4,7 @@ cabal-version: 2.0
 --
 -- see: https://github.com/sol/hpack
 --
--- hash: 569c2e513825afb99305f83cb7146a6dd4c42b06b991923f534ad980a8bc8c2e
+-- hash: f150b19d31eda4f6f32ad2782dccc7deae05b01ffd46558fe1f403ba49500307
 
 name:           brig
 version:        1.35.0
@@ -227,7 +227,6 @@ library brig-index-lib
       Brig.Index.Eval
       Brig.Index.Migrations
       Brig.Index.Migrations.Types
-      Brig.Index.Migrations.V1_ReIndexForSearchTeamMembers
       Brig.Index.Options
       Main
   other-modules:

services/brig/index/src/Brig/Index/Migrations.hs

@@ -21,8 +21,8 @@ module Brig.Index.Migrations
 where
 
 import Brig.Index.Migrations.Types
-import qualified Brig.Index.Migrations.V1_ReIndexForSearchTeamMembers as V1_ReIndexForSearchTeamMembers
 import qualified Brig.Index.Options as Opts
+import qualified Brig.User.Search.Index as Search
 import qualified Cassandra as C
 import qualified Cassandra.Settings as C
 import Control.Lens ((^.), view)
@@ -45,7 +45,11 @@ migrate l es cas = do
       runMigrationAction env $ do
         failIfIndexAbsent (es ^. Opts.esIndex)
         createMigrationsIndexIfNotPresent
-        runMigrations [V1_ReIndexForSearchTeamMembers.migration]
+        runMigration expectedMigrationVersion
+
+-- | Increase this number any time you want to force reindexing.
+expectedMigrationVersion :: MigrationVersion
+expectedMigrationVersion = MigrationVersion 2
 
 indexName :: ES.IndexName
 indexName = ES.IndexName "wire_brig_migrations"
@@ -105,20 +109,16 @@ failIfIndexAbsent targetIndex =
     (throwM $ TargetIndexAbsent targetIndex)
 
 -- | Runs only the migrations which need to run
-runMigrations :: [Migration] -> MigrationActionT IO ()
-runMigrations migrations = do
+runMigration :: MigrationVersion -> MigrationActionT IO ()
+runMigration ver = do
   vmax <- latestMigrationVersion
-  let pendingMigrations = filter (\m -> version m > vmax) migrations
-  if null pendingMigrations
-    then info "No new migrations."
-    else info "New migrations found."
-  mapM_ runMigration pendingMigrations
-
-runMigration :: Migration -> MigrationActionT IO ()
-runMigration (Migration ver txt mig) = do
-  info $ "Running: [" <> show (migrationVersion ver) <> "] " <> Text.unpack txt
-  mig
-  persistVersion ver
+  if ver > vmax
+    then do
+      info "Migration necessary."
+      Search.reindexAllIfSameOrNewer
+      persistVersion ver
+    else do
+      info "No migration necessary."
 
 persistVersion :: (Monad m, MonadThrow m, MonadIO m) => MigrationVersion -> MigrationActionT m ()
 persistVersion v =

services/brig/index/src/Brig/Index/Migrations/Types.hs

@@ -32,12 +32,6 @@ import Numeric.Natural (Natural)
 import qualified System.Logger as Logger
 import System.Logger.Class (MonadLogger (..))
 
-data Migration = Migration
-  { version :: MigrationVersion,
-    text :: Text,
-    action :: MigrationActionT IO ()
-  }
-
 newtype MigrationVersion = MigrationVersion {migrationVersion :: Natural}
   deriving (Show, Eq, Ord)
 

services/brig/src/Brig/User/Search/Index.hs

@@ -167,15 +167,6 @@ searchIndex ::
   m (SearchResult Contact)
 searchIndex u teamSearchInfo q = queryIndex (defaultUserQuery u teamSearchInfo q)
 
--- [Note: suspended]  TODO: has this happened by now and we can resolve this note?
--- ~~~~~~~~~~~~~~~~~
---
--- The suspended field in the ES index is an artifact of the legacy system,
--- which would not remove data from the index upon user suspension. The filter
--- clause can be removed after the first full re-index, as this will have
--- cleared the data of suspended users.
---
-
 queryIndex ::
   (MonadIndexIO m, FromJSON r) =>
   IndexQuery r ->
@@ -258,11 +249,17 @@ mkUserQuery (review _TextId -> self) teamSearchInfo q =
   IndexQuery q
     $ ES.Filter . ES.QueryBoolQuery
     $ boolQuery
-      { ES.boolQueryMustNotMatch =
-          [ termQ "suspended" "1", -- [Note: suspended]
-            termQ "_id" self
-          ],
-        ES.boolQueryMustMatch = [optionallySearchWithinTeam teamSearchInfo]
+      { ES.boolQueryMustNotMatch = [termQ "_id" self],
+        ES.boolQueryMustMatch =
+          [ optionallySearchWithinTeam teamSearchInfo,
+            ES.QueryBoolQuery
+              boolQuery
+                { ES.boolQueryShouldMatch =
+                    [ termQ "account_status" "active",
+                      ES.QueryBoolQuery boolQuery {ES.boolQueryMustNotMatch = [ES.QueryExistsQuery (ES.FieldName "account_status")]} -- TODO: Write note about this being legacy
+                    ]
+                }
+          ]
       }
   where
     termQ f v =
@@ -468,6 +465,9 @@ updateMapping = liftIndexIO $ do
   ex <- ES.indexExists idx
   unless ex $
     throwM (IndexError "Index does not exist.")
+  -- FUTUREWORK: check return code (ES.isSuccess) and fail if appropriate.
+  -- But to do that we have to consider the consequences of this failing in our helm chart:
+  -- https://github.com/wireapp/wire-server-deploy/blob/92311d189818ffc5e26ff589f81b95c95de8722c/charts/elasticsearch-index/templates/create-index.yaml
   void $ traceES "Put mapping" $
     ES.putMapping idx (ES.MappingName "user") indexMapping
 
@@ -531,6 +531,7 @@ userDoc iu =
     { udId = _iuUserId iu,
       udTeam = _iuTeam iu,
       udName = _iuName iu,
+      udAccountStatus = _iuAccountStatus iu,
       udNormalized = normalized . fromName <$> _iuName iu,
       udHandle = _iuHandle iu,
       udColourId = _iuColourId iu
@@ -569,10 +570,19 @@ userDoc iu =
 -- searched tokens are not bigger than 30 characters by truncating them, this is
 -- necessary as our "prefix_index" analyzer only creates edge_ngrams until 30
 -- characters.
+--
+-- About the dynamic field: When this is not set and we add another field to our
+-- user document, elasticsearch will try to guess how it is supposed to be indexed.
+-- Changes to this require creating a new index and a cumbersome migration. So it is
+-- important that we set this field to `false`. This will make new fields will just
+-- not be indexed. After we decide what they should look like, we can just run a
+-- reindex to make them usable. More info:
+-- https://www.elastic.co/guide/en/elasticsearch/reference/7.7/dynamic.html
 indexMapping :: Value
 indexMapping =
   object
-    [ "properties"
+    [ "dynamic" .= False,
+      "properties"
         .= object
           [ "normalized" -- normalized user name
               .= MappingProperty
@@ -615,6 +625,14 @@ indexMapping =
                   mpIndex = False,
                   mpAnalyzer = Nothing,
                   mpFields = mempty
+                },
+            "account_status"
+              .= MappingProperty
+                { mpType = MPKeyword,
+                  mpStore = False,
+                  mpIndex = True,
+                  mpAnalyzer = Nothing,
+                  mpFields = mempty
                 }
           ]
     ]
@@ -755,20 +773,24 @@ reindexRowToIndexUser (u, mteam, name, t0, status, t1, handle, t2, colour, t4, a
     pure $
       if shouldIndex
         then
-          iu & set iuTeam mteam
+          iu
+            & set iuTeam mteam
             . set iuName (Just name)
             . set iuHandle handle
             . set iuColourId (Just colour)
-        else iu
+            . set iuAccountStatus status
+        else
+          iu
+            & set iuAccountStatus status
   where
     version :: [Maybe (Writetime Name)] -> m IndexVersion
     version = mkIndexVersion . getMax . mconcat . fmap Max . catMaybes
     shouldIndex =
       and
         [ case status of
-            Just Active -> True
             Nothing -> True
-            Just Suspended -> False
+            Just Active -> True
+            Just Suspended -> True
             Just Deleted -> False
             Just Ephemeral -> False,
           activated, -- FUTUREWORK: how is this adding to the first case?

services/brig/src/Brig/User/Search/Index/Types.hs

@@ -19,6 +19,7 @@
 
 module Brig.User.Search.Index.Types where
 
+import Brig.Types.Intra (AccountStatus)
 import Brig.Types.User
 import Control.Lens (makeLenses)
 import Control.Monad.Catch
@@ -45,7 +46,8 @@ data IndexUser = IndexUser
     _iuTeam :: Maybe TeamId,
     _iuName :: Maybe Name,
     _iuHandle :: Maybe Handle,
-    _iuColourId :: Maybe ColourId
+    _iuColourId :: Maybe ColourId,
+    _iuAccountStatus :: Maybe AccountStatus
   }
 
 data IndexQuery r = IndexQuery Query Filter
@@ -72,7 +74,8 @@ data UserDoc = UserDoc
     udName :: Maybe Name,
     udNormalized :: Maybe Text,
     udHandle :: Maybe Handle,
-    udColourId :: Maybe ColourId
+    udColourId :: Maybe ColourId,
+    udAccountStatus :: Maybe AccountStatus
   }
   deriving (Eq, Show)
 
@@ -84,7 +87,8 @@ instance ToJSON UserDoc where
         "name" .= udName ud,
         "normalized" .= udNormalized ud,
         "handle" .= udHandle ud,
-        "accent_id" .= udColourId ud
+        "accent_id" .= udColourId ud,
+        "account_status" .= udAccountStatus ud
       ]
 
 instance FromJSON UserDoc where
@@ -95,6 +99,7 @@ instance FromJSON UserDoc where
       <*> o .:? "normalized"
       <*> o .:? "handle"
       <*> o .:? "accent_id"
+      <*> o .:? "account_status"
 
 makeLenses ''IndexUser
 
@@ -112,5 +117,6 @@ mkIndexUser u v =
       _iuTeam = Nothing,
       _iuName = Nothing,
       _iuHandle = Nothing,
-      _iuColourId = Nothing
+      _iuColourId = Nothing,
+      _iuAccountStatus = Nothing
     }

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

@@ -50,6 +50,7 @@ import Imports
 import Network.HTTP.Client (Manager)
 import qualified Network.Wai.Test as WaiTest
 import qualified Network.Wai.Utilities.Error as Error
+import Numeric.Natural (Natural)
 import Test.Tasty hiding (Timeout)
 import qualified Test.Tasty.Cannon as WS
 import Test.Tasty.HUnit
@@ -107,11 +108,20 @@ testTeamSize :: Brig -> Http ()
 testTeamSize brig = do
   (tid, _, _) <- createPopulatedBindingTeam brig 10
   SearchUtil.refreshIndex brig
-  void $
-    get (brig . paths ["i", "teams", toByteString' tid, "size"]) <!! do
-      const 200 === statusCode
-      -- 10 Team Members and an admin
-      (const . Right $ TeamSize 11) === responseJsonEither
+  -- 10 Team Members and an admin
+  let expectedSize = 11
+  assertSize tid expectedSize
+
+  -- Even suspended teams should report correct size
+  suspendTeam brig tid !!! const 200 === statusCode
+  SearchUtil.refreshIndex brig
+  assertSize tid expectedSize
+  where
+    assertSize :: HasCallStack => TeamId -> Natural -> Http ()
+    assertSize tid expectedSize = void $
+      get (brig . paths ["i", "teams", toByteString' tid, "size"]) <!! do
+        const 200 === statusCode
+        (const . Right $ TeamSize expectedSize) === responseJsonEither
 
 -------------------------------------------------------------------------------
 -- Invitation Tests

services/brig/test/unit/Test/Brig/User/Search/Index/Types.hs

@@ -20,6 +20,7 @@
 module Test.Brig.User.Search.Index.Types where
 
 import Brig.Types.Common
+import Brig.Types.Intra (AccountStatus (..))
 import Brig.User.Search.Index
 import Data.Aeson
 import Data.Handle
@@ -39,7 +40,7 @@ tests =
           "failed"
           (eitherDecode' (encode userDoc1))
           (Right userDoc1),
-      testCase "aeson half-roundtrip: UserDoc" $
+      testCase "backwards comptibility test: UserDoc" $
         assertEqual
           "failed"
           (toJSON userDoc1)
@@ -59,14 +60,15 @@ userDoc1 =
       udName = Just . Name $ "Carl Phoomp",
       udNormalized = Just $ "carl phoomp",
       udHandle = Just . fromJust . parseHandle $ "phoompy",
-      udColourId = Just . ColourId $ 32
+      udColourId = Just . ColourId $ 32,
+      udAccountStatus = Just Active
     }
 
 userDoc1Value :: Value
 userDoc1Value = fromJust (decode userDoc1ByteString)
 
 userDoc1ByteString :: LByteString
-userDoc1ByteString = "{\"team\":\"17c59b18-57d6-11ea-9220-8bbf5eee961a\",\"handle\":\"phoompy\",\"accent_id\":32,\"name\":\"Carl Phoomp\",\"id\":\"0a96b396-57d6-11ea-a04b-7b93d1a5c19c\",\"normalized\":\"carl phoomp\"}"
+userDoc1ByteString = "{\"team\":\"17c59b18-57d6-11ea-9220-8bbf5eee961a\",\"handle\":\"phoompy\",\"accent_id\":32,\"name\":\"Carl Phoomp\",\"id\":\"0a96b396-57d6-11ea-a04b-7b93d1a5c19c\",\"normalized\":\"carl phoomp\",\"account_status\":\"active\"}"
 
 indexUser1 :: IndexUser
 indexUser1 =
@@ -76,5 +78,6 @@ indexUser1 =
       _iuTeam = udTeam userDoc1,
       _iuName = udName userDoc1,
       _iuHandle = udHandle userDoc1,
-      _iuColourId = udColourId userDoc1
+      _iuColourId = udColourId userDoc1,
+      _iuAccountStatus = udAccountStatus userDoc1
     }