WPB-20214: Add member searchability #4786

eyeinsky · 2025-09-23T11:27:06Z

Task checklist:

Open questions

is the changed admin toggle API type ok? The task designates this as POST /users/:uid/searchable, but as TeamId is required, then it currently is POST /users/:uid/:tid/searchable -- could this be improved?
POST /handles: should this endpoint also filter based on searchability?
are there no other locations than /search/contacts to take searchability into account?
should the test be moved into the integration package? (@battermann mentioned this -- are we moving tests there and hence, should this test also go there as well?)

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

eyeinsky · 2025-09-25T12:49:49Z

The below is now resolved.

The current error when running the test is this:

[[email protected]] E, IO Exception occurred, message=cql-io: protocol error: parse error: response body reading: Failed reading: column count: 24 =/= 26 Empty call stack , request=37d3c1fa-f1ee-4050-b623-e4c9c79e5584
[[email protected]] E, request=37d3c1fa-f1ee-4050-b623-e4c9c79e5584, code=500, label=server-error, "Server Error"
brig-integration: Assertions failed:
 1: 201 =/= 500

Response was:

Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Thu, 25 Sep 2025 09:00:01 GMT"),("Server","Warp/3.4.2"),("traceparent","00-96836cd3b1eedf0553338dde6b2d4b0c-1ced0252881c377c-01"),("tracestate",""),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = Just "{\"code\":500,\"label\":\"server-error\",\"message\":\"Internal Server Error\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose, responseOriginalRequest = Request {
  host                 = "127.0.0.1"
  port                 = 8082
  secure               = False
  requestHeaders       = [("Content-Type","application/json")]
  path                 = "/i/users"
  queryString          = ""
  method               = "POST"
  proxy                = Nothing
  rawBody              = False
  redirectCount        = 10
  responseTimeout      = ResponseTimeoutDefault
  requestVersion       = HTTP/1.1
  proxySecureMode      = ProxySecureWithConnect
}
, responseEarlyHints = []}
CallStack (from HasCallStack):
  error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-inplace:Bilge.Assert
  <!!, called at test/integration/API/Team/Util.hs:140:9 in brig-2.0-inplace-brig-integration:API.Team.Util
  createUserWithTeam', called at test/integration/API/Team/Util.hs:89:21 in brig-2.0-inplace-brig-integration:API.Team.Util
  createPopulatedBindingTeamWithNames, called at test/integration/API/Team/Util.hs:68:25 in brig-2.0-inplace-brig-integration:API.Team.Util
  createPopulatedBindingTeamWithNamesAndHandles, called at test/integration/API/Search.hs:165:38 in brig-2.0-inplace-brig-integration:API.Search

From running this command: make c package=all && ./hack/bin/cabal-run-integration.sh brig -p '/testUserSearchable/'

eyeinsky · 2025-10-06T10:18:02Z

@akshaymankar Noting here what you asked about in the standup: the /search/contacts endpoint fetches the results from ES which is populated from Cassandra's user table. This has the searchable field, so filtering there is easy. The /team/:tid/members?searchable=false on the other hand gets its results from Cassandra's team_member table, which doesn't have a searchable field.. But since I need to return all non-searchable users from this table, then I think the current (pre-postgres) way to do it would be to get all rows from team_member and then filter that down to only non-searchable users by the help of users table. With large table this might costly to do. 🤔

When we had all data in postgres though, the query could entirely be done on the database side: join team_member with user, then filter by searchable.

akshaymankar · 2025-10-06T11:23:49Z

@eyeinsky I think we shouldn't support this query param on /teams/:tid/members endpoint, but rather on /teams/:tid/search endpoint. I just saw the ticket and it seems like I made a mistake in the name of this endpoint. Sorry about that 😞

eyeinsky · 2025-10-07T09:51:54Z

@akshaymankar I'm ready for another round of review!

akshaymankar · 2025-10-07T13:34:21Z

is the changed admin toggle API type ok? The task designates this as POST /users/:uid/searchable, but as TeamId is required, then it currently is POST /users/:uid/:tid/searchable -- could this be improved?

I think we can just lookup the team id of the targetted user and check whether the user who made the request is an admin of that team. We shouldn't need team id in the path.

POST /handles: should this endpoint also filter based on searchability?

No, we can expose the fact that his handle is taken as far as we don't tell who has this handle.

are there no other locations than /search/contacts to take searchability into account?

Yeah, this is the only location.

should the test be moved into the integration package? (@battermann mentioned this -- are we moving tests there and hence, should this test also go there as well?)

Ideally, yes. Sometimes we've been lazy, but the agreed upon rule was to move a test if you need to work on it and not to add more tests in the legacy test suites.

akshaymankar · 2025-10-07T13:35:17Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

+    :<|> Named
+           "set-user-searchable"
+           ( Summary "Set user's visibility in search"
+               :> From 'V12
+               :> ZLocalUser
+               :> "users"
+               :> CaptureUserId "uid"
+               :> Capture "tid" TeamId
+               :> ReqBody '[JSON] Bool
+               :> "searchable"
+               :> Post '[JSON] ()
+           )


As mentioned in earlier comment, we shouldn't need the TeamId in the path.

akshaymankar · 2025-10-07T13:41:27Z

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

  | SetMemberPermissions
  | GetTeamConversations
  | DeleteTeam
+  | SetMemberSearchable


Shouldn't this go to the HiddenPerm type?

akshaymankar · 2025-10-07T13:50:19Z

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

-              teamContactEmailUnvalidated = Nothing
+              teamContactEmailUnvalidated = Nothing,
+              teamContactSearchable = Nothing


We should reduce these tests and perhaps add a test where this value is not Nothing.

akshaymankar · 2025-10-07T13:50:49Z

libs/wire-api/src/Wire/API/User/Search.hs

    teamContactSso :: Maybe Sso,
-    teamContactEmailUnvalidated :: Maybe EmailAddress
+    teamContactEmailUnvalidated :: Maybe EmailAddress,
+    teamContactSearchable :: Maybe Bool


Why is this a Maybe?

akshaymankar · 2025-10-07T13:52:45Z

libs/wire-api/test/unit/Test/Wire/API/User.hs

  let domain = Domain "example.com"
  let colour = ColourId 0
-  let userProfile = UserProfile (Qualified uid domain) (Name "name") Nothing (Pict []) [] colour False Nothing Nothing Nothing Nothing Nothing UserLegalHoldNoConsent defSupportedProtocols UserTypeRegular
+  let userProfile = UserProfile (Qualified uid domain) (Name "name") Nothing (Pict []) [] colour False Nothing Nothing Nothing Nothing Nothing UserLegalHoldNoConsent defSupportedProtocols UserTypeRegular True


I'm refactoring these to record syntax whenever I come across them. Its a bit painful, but IMO its necessary to do this as we go.

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

akshaymankar · 2025-10-07T14:00:24Z

services/brig/test/integration/API/User/Account.hs

      test p "get /users/<localdomain>/:uid - 404" $ testNonExistingUser b,
      test p "get /users/:domain/:uid - 422" $ testUserInvalidDomain b,
      test p "get /users/:uid - 200" $ testExistingUserUnqualified b,
+      test p "testUserSearchable" $ testUserSearchable b g,


We should write this test in the new test suite.

akshaymankar · 2025-10-07T14:01:40Z

libs/wire-subsystems/src/Wire/IndexedUserStore/ElasticSearch.hs

                ],
-              ES.boolQueryShouldMatch = [ES.QueryExistsQuery (ES.FieldName "handle")]
+              ES.boolQueryShouldMatch = [ES.QueryExistsQuery (ES.FieldName "handle")],
+              ES.boolQueryMustNotMatch = [ES.TermQuery (ES.Term "searchable" "false") Nothing]


This looks like it should work with old users who don't have anything set in searchable, just to be sure did you test it?
If it does work we should write a comment explaining this double negation (mustNot and false)

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

Note [ephemeral user sideeffect] was removed in commit: b0934a3 WPB-15801 GET and DELETE Registered Domains (#4438)

- make `!!!` definition easier to understand - extract `getProfile`

…=false`" This reverts commit 4790b05.

Co-authored-by: Akshay Mankar <[email protected]>

Make SetMemberSearchable from Perm to HiddenPerm.

This reverts commit 68b6c6e.

battermann

This needs a release note explaining how the search index mapping has to be updated and how to avoid downtime, etc.

And there are some minor comments, looks good overall.

battermann · 2025-10-10T09:45:39Z

integration/test/Test/Search.hs

+  let -- Helper to change user searchability.
+      setSearchable self uid searchable = do
+        req <- baseRequest self Brig Versioned $ joinHttpPath ["users", uid, "searchable"]
+        submit "POST" $ addJSON searchable req


this should go into API.Brig

battermann · 2025-10-10T09:49:36Z

integration/test/Test/Search.hs

+  u1' <- BrigP.getUser u1 u1 >>= getJSON 200
+  assertBool "Searchable is still True" =<< (u1' %. "searchable" & asBool)


If u1' is not used later I would prefer to do the assertion in a bindResponse body, to reduce number of ticked variable names. Also you could assert like this: u1 %. "searchable" shouldMatch True (I think) which I find more readable.

The above applies to the following lines, too.

battermann · 2025-10-10T09:54:42Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

+        :> QueryParam'
+             [ Optional,
+               Strict,
+               Description "Optional, return only non-seacrhable members when false."


will it return only searchable members when true?

battermann · 2025-10-10T10:04:29Z

services/brig/test/integration/Util.hs

+randomUserPrefix ::
+  (MonadCatch m, MonadIO m, MonadHttp m, HasCallStack) =>
+  Text ->
+  Brig ->
+  m User
+randomUserPrefix prefix brig = do
+  n <- fromName <$> randomName
+  createUser' True (prefix <> n) brig
+


this seems unused, no?

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

Co-authored-by: Leif Battermann <[email protected]>

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 23, 2025

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch 5 times, most recently from bc31287 to 78ad65e Compare September 25, 2025 08:44

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch 3 times, most recently from 9891c56 to 6a1a1b8 Compare October 3, 2025 07:56

eyeinsky marked this pull request as ready for review October 3, 2025 08:17

eyeinsky requested a review from a team as a code owner October 3, 2025 08:17

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 6a1a1b8 to 4790b05 Compare October 6, 2025 09:59

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 4790b05 to b0f782c Compare October 7, 2025 09:47

akshaymankar requested changes Oct 7, 2025

View reviewed changes

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 7901c95 to d791f07 Compare October 8, 2025 09:02

eyeinsky added 10 commits October 9, 2025 19:06

Remove references to non-existent note

2a5e689

Note [ephemeral user sideeffect] was removed in commit: b0934a3 WPB-15801 GET and DELETE Registered Domains (#4438)

Make the Wire.API.Team.Member.userId lens more general

4d51c98

Minor refactor

68b6c6e

- make `!!!` definition easier to understand - extract `getProfile`

Add searchable field to data types

5d75888

Add Elastic Search boolean field type

ede3f7f

Add POST /users/:uid/searchable

07685fd

Add Elastic Search indexing

dbfa240

Filter by searchable in Elastic Search

0de7f7d

Filter by searchable in exact handle search

b8d7ad8

Test searchable field and contact search

0d6dfc7

eyeinsky and others added 10 commits October 9, 2025 19:06

Use common CQL splice for team member queries

d02426b

Test /team/:tid/members?searchable=false

1dd2110

[wip] Filter searchable with /team/:tid/members?searchable=false

a1c8e34

Revert "[wip] Filter searchable with `/team/:tid/members?searchable…

c3301be

…=false`" This reverts commit 4790b05.

Add query param to Brig

201ccea

Update services/brig/src/Brig/Provider/API.hs

842e955

Co-authored-by: Akshay Mankar <[email protected]>

Update libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

fa7beb8

Co-authored-by: Akshay Mankar <[email protected]>

fixup! Add POST /users/:uid/searchable

f1ff0eb

fixup! Test searchable field and contact search

221c23c

fixup! Add POST /users/:uid/searchable

33a1956

Make SetMemberSearchable from Perm to HiddenPerm.

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 99bfb21 to 671e93a Compare October 9, 2025 20:20

eyeinsky added 8 commits October 10, 2025 12:27

Move test from brig to integration package

92c46c6

fixup! Add query param to Brig

34f2093

Partially revert "Minor refactor": inline getProfile back again

c7bb682

This reverts commit 68b6c6e.

Minor refactor: use record syntax, deduplicate golden tests

6f02b68

make sanitize-pr

01bb060

fixup! Add query param to Brig

0600dbd

fixup! Add POST /users/:uid/searchable

68dd8d3

fixup! Add query param to Brig

cfc7888

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from c0e5357 to cfc7888 Compare October 10, 2025 09:27

battermann requested changes Oct 10, 2025

View reviewed changes

Update libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

15e438d

Co-authored-by: Leif Battermann <[email protected]>

		u1' <- BrigP.getUser u1 u1 >>= getJSON 200
		assertBool "Searchable is still True" =<< (u1' %. "searchable" & asBool)

WPB-20214: Add member searchability #4786

Are you sure you want to change the base?

WPB-20214: Add member searchability #4786

Conversation

eyeinsky commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Open questions

Checklist

Uh oh!

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

Uh oh!

Uh oh!

eyeinsky commented Oct 6, 2025

Uh oh!

akshaymankar commented Oct 6, 2025

Uh oh!

eyeinsky commented Oct 7, 2025

Uh oh!

akshaymankar commented Oct 7, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

battermann 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!

Uh oh!

Uh oh!

eyeinsky commented Sep 23, 2025 •

edited

Loading

eyeinsky commented Sep 25, 2025 •

edited

Loading