fix: stop using broken addTeamMemberInternal in testCreateOne2OneWith - #5420
Merged
blackheaven merged 6 commits intoAug 7, 2026
Merged
Conversation
…Members The test intermittently failed with a 500 from galley's internal POST i/teams/:tid/members endpoint (uncheckedAddTeamMember), reached via the addTeamMemberInternal helper. That helper is documented as broken (SQSERVICES-471): it force-adds an existing user through a path that fans out synchronous brig RPCs (getSize/getUser/ getAppIdsForTeam) plus Cassandra writes and SQS journaling, any of which can transiently exhaust its retries and surface as an unhandled 500 server-error. Switch to addUserToTeamWithRole, the proper brig invitation flow that updates both brig and galley and is used by every sibling test in this file. The member-join event is now asserted on the owner's WebSocket (the invitee is created inside the call and cannot have a pre-established connection), matching the testAddTeamConvWithRole precedent (Teams.hs:679-680). Also corrects the misleading doc-comment on the role parameter.
zebot
added
the
ok-to-test
battermann
reviewed
Aug 5, 2026
battermann
left a comment
battermann
left a comment
Contributor
There was a problem hiding this comment.
If addTeamMemberInternal is broken, can we remove it from Util, too?
Also could this test be migrated to the integration package, and be removed here all together?
Addresses @battermann's review on #5420. 1. Remove the broken `Util.addTeamMemberInternal` helper (SQSERVICES-471). All 14 call sites migrate to the proper invitation flow: - standard-role sites -> `addUserToTeamWithRole` - custom-permission sites -> `addUserToTeamWithRole` + new `updateTeamMemberPermissions` (public PUT /teams/:tid/members) - `testAddTeamMemberInternal` keeps inlined coverage of the internal add-member endpoint (its purpose; the invite flow cannot reproduce the member-websocket event assertion) - `testAddTeamMemberToConv` and `testCannotCreateLegalHoldDeviceOldAPI` restructured to mint members via the invitation flow. In `testAddTeamMemberToConv` the same-team members are no longer passed to `connectUsers` (same-binding-team connection requests are rejected with 403; those assertions are satisfied by the same-team condition). 2. Port `testCreateOne2OneWithMembers` and `testCreateOne2OneFailForNonTeamMembers` to the Testlib /integration package (`Test.One2OneTeamConv`) and remove them from galley. The galley-specific `assertTeamUpdate` (SQS queue) is replaced by a member-count check plus the real-time member-join websocket event. Cleanup: collapse `makeOwner` onto `updateTeamMemberPermissions`; delete the now-dead `createOne2OneTeamConv` helper. Compile-verified (exe:galley-integration + integration); runtime deferred to CI. `testAddTeamMemberToConv` was restructured and is the most behaviour-sensitive change.
Contributor
Author
|
I have migrated everything |
battermann
approved these changes
Aug 7, 2026
battermann
left a comment
battermann
left a comment
Contributor
There was a problem hiding this comment.
I approve because these are just tests, but please still consider my comments.
Comment on lines
+445
to
+451
| g <- viewGalley | ||
| post | ||
| ( g | ||
| . paths ["i", "teams", toByteString' tid, "members"] | ||
| . json (Member.mkNewTeamMember (mem1 ^. userId) (mem1 ^. permissions) (mem1 ^. invitation)) | ||
| ) | ||
| !!! const 200 === statusCode |
Contributor
There was a problem hiding this comment.
It is a bit weird that the function is removed from Util but is inlined here. Is there a forcing reason for this?
Contributor
Author
There was a problem hiding this comment.
Yes, testAddTeamMemberInternal is the test for the internal POST /i/teams/:tid/members endpoint (formerly Util.addTeamMemberInternal), so it has to hit it directly. The helper was removed from Util precisely because it's a footgun: it bypasses the invitation flow (SQSERVICES-471), so re-exposing it as a reusable helper would tempt other tests to reach for it. I added a comment recording the reasoning next to the call.
Contributor
There was a problem hiding this comment.
But do we need to test it still? can we remove the endpoint all together even?
…d internal endpoint Follow-up to ad1336f addressing remaining review comments on #5420. 1. In testCreateOne2OneWithMembers, replace the manual Control.Retry.retrying block with the idiomatic `eventually (... >>= assertStatus 201)`, which retries on AssertionFailure every 100ms up to the suite timeout - the same "retry until 201" semantics in one line. Drop the now-dead Control.Retry import. 2. In testAddTeamMemberInternal, add a comment explaining why the internal POST /i/teams/:tid/members call is inlined rather than re-exposed as a helper: that endpoint bypasses the invitation flow and must not be reused by other tests (SQSERVICES-471). Compile-verified (integration + exe:galley-integration) under -Werror; runtime deferred to CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
changelog.d