WPB-20429: add replace channel's member endpoint #4819

blackheaven · 2025-10-20T18:25:03Z

https://wearezeta.atlassian.net/browse/WPB-20429

Checklist

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

services/galley/src/Galley/API/Update.hs

integration/test/Test/Conversation.hs

services/galley/src/Galley/API/Update.hs

blackheaven · 2025-10-24T18:31:10Z

@fisx thanks you!

integration/test/Test/Conversation.hs

fisx · 2025-10-27T09:38:05Z

btw i think that the problem with the test that only failed on CI, but not locally, was that ensureLocal was called with example.com, which of course failed on CI (even though I'm not sure why it didn't locally?). anyway if i make the entire test non-federated it works fine. for remote conversations, this end-point should fail indeed (even though 4xx would be nicer than 5xx), so it's ok to not that that here.

i've opened another related ticket to be addressed elsehwere.

pcapriotti

Looks good. Some comments below. I'm also not sure about the following edge case: what if we want to add user u as admin, but u is already present as a regular member? If I understand the code correctly, nothing would happen. But then u wouldn't be an admin as requested. This violates a general property which you would probably expect. Namely that the outcome of a replace operation does not depend on the current state.

pcapriotti · 2025-10-28T08:18:01Z

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

+    :<|> Named
+           "replace-members-in-conversation"
+           ( Summary "Replace the members of a conversation."
+               :> From 'V2


I don't think it's a problem in practice, but why V2?

I have missed the 1 in 12.

Thanks, fixed.

Shouldn't it be 13, though?

added, but I had to create it

services/galley/src/Galley/API/Update.hs

blackheaven · 2025-10-28T15:18:43Z

Looks good. Some comments below. I'm also not sure about the following edge case: what if we want to add user u as admin, but u is already present as a regular member? If I understand the code correctly, nothing would happen. But then u wouldn't be an admin as requested. This violates a general property which you would probably expect. Namely that the outcome of a replace operation does not depend on the current state.

should I upsert them then?

blackheaven · 2025-10-30T08:16:54Z

Looks good. Some comments below. I'm also not sure about the following edge case: what if we want to add user u as admin, but u is already present as a regular member? If I understand the code correctly, nothing would happen. But then u wouldn't be an admin as requested. This violates a general property which you would probably expect. Namely that the outcome of a replace operation does not depend on the current state.

should I upsert them then?

As discussed in DM:

I'm not sure about the requirements there, you probably have a better idea than me about what should be the correct behaviour here. I was just pointing out that slight inconsistency. You can probably leave it, I don't think it matters too much.

The RFC is clear on this:

determine which members need to be added and which members need to be removed and execute the adding and removal,

However, I'm not sure it is intentional

fisx · 2025-10-30T09:13:58Z

However, I'm not sure it is intentional

i'm pretty sure it's not, but it's the letter of the law. i'll make a ticket where we can decide about actually intended behavior so we can move on here. in short: please follow the RFC.

eyeinsky · 2025-10-30T10:08:50Z

Just a drive-by comment, but as I just noticed the last commit adding V13: it's already present in the develop branch from here 1c2d211 -- just need to rebase.

blackheaven · 2025-10-30T10:27:00Z

Just a drive-by comment, but as I just noticed the last commit adding V13: it's already present in the develop branch from here 1c2d211 -- just need to rebase.

nice, thanks!

fisx · 2025-10-30T11:57:44Z

----- MLS.SubConversation.testResendingProposals FAIL (7.48 s; failed at 2025-10-30T10:53:54.755Z) -----
assertion failure:
Failed spawning process

call stack: 
1. assertFailure at test/MLS/Util.hs:836

2. spawn at test/MLS/Util.hs:130

3. runCli at test/MLS/Util.hs:108

4. mlscli at test/MLS/Util.hs:704

5. mlsCliConsume at test/MLS/Util.hs:677

6. consumeMessageWithPredicate at test/MLS/Util.hs:683

7. consumeMessage at test/Test/MLS/SubConversation.hs:381

huh? as in "Failed spawning unix process"? smells flaky.

battermann

Approved on behalf of Paolo and Matthias

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

blackheaven force-pushed the gdifolco/WPB-20429_add-replace-channel-member branch from 3a65538 to 7d6cd8d Compare October 20, 2025 20:31

blackheaven marked this pull request as ready for review October 21, 2025 16:53

blackheaven requested review from a team as code owners October 21, 2025 16:53

blackheaven commented Oct 21, 2025

View reviewed changes

services/galley/src/Galley/API/Update.hs Outdated Show resolved Hide resolved

fisx reviewed Oct 22, 2025

View reviewed changes

fisx reviewed Oct 27, 2025

View reviewed changes

integration/test/Test/Conversation.hs Outdated Show resolved Hide resolved

integration/test/Test/Conversation.hs Outdated Show resolved Hide resolved

pcapriotti approved these changes Oct 28, 2025

View reviewed changes

blackheaven requested a review from pcapriotti October 29, 2025 11:10

blackheaven force-pushed the gdifolco/WPB-20429_add-replace-channel-member branch from 7834b0b to 2ec2d87 Compare October 30, 2025 10:03

WPB-20429: add replace channel's member endpoint

0399b86

blackheaven force-pushed the gdifolco/WPB-20429_add-replace-channel-member branch from 2ec2d87 to 7da2b34 Compare October 30, 2025 10:27

fix: add V13

6e29aad

blackheaven force-pushed the gdifolco/WPB-20429_add-replace-channel-member branch from 7da2b34 to 6e29aad Compare October 30, 2025 16:28

battermann approved these changes Oct 31, 2025

View reviewed changes

blackheaven merged commit bc98c08 into develop Oct 31, 2025
9 checks passed

blackheaven deleted the gdifolco/WPB-20429_add-replace-channel-member branch October 31, 2025 08:01

This was referenced Nov 26, 2025

Release 2025-11-26 - (expected chart version 5.24.0) #4875

Closed

Release 2025-11-26 - (expected chart version 5.24.0) #4876

Merged

WPB-20429: add replace channel's member endpoint #4819

WPB-20429: add replace channel's member endpoint #4819

Uh oh!

Conversation

blackheaven commented Oct 20, 2025

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blackheaven commented Oct 24, 2025

Uh oh!

Uh oh!

Uh oh!

fisx commented Oct 27, 2025

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

pcapriotti Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

blackheaven Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

blackheaven Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blackheaven commented Oct 28, 2025

Uh oh!

blackheaven commented Oct 30, 2025

Uh oh!

fisx commented Oct 30, 2025

Uh oh!

eyeinsky commented Oct 30, 2025

Uh oh!

blackheaven commented Oct 30, 2025

Uh oh!

fisx commented Oct 30, 2025

Uh oh!

battermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

blackheaven Oct 28, 2025 •

edited

Loading