Skip to content

Comments

Deleting clients creates MLS remove proposals#2674

Merged
smatting merged 6 commits intodevelopfrom
FS-905/delete-client-remove-proposals
Sep 15, 2022
Merged

Deleting clients creates MLS remove proposals#2674
smatting merged 6 commits intodevelopfrom
FS-905/delete-client-remove-proposals

Conversation

@smatting
Copy link
Contributor

@smatting smatting commented Sep 5, 2022 •
edited
Loading

Checklist

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

@smatting smatting temporarily deployed to cachix September 5, 2022 14:21 Inactive
@smatting smatting temporarily deployed to cachix September 5, 2022 14:21 Inactive
@smatting smatting temporarily deployed to cachix September 5, 2022 14:52 Inactive
@smatting smatting temporarily deployed to cachix September 5, 2022 14:52 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 5, 2022
@stefanwire stefanwire temporarily deployed to cachix September 6, 2022 09:57 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 6, 2022 09:57 Inactive
@stefanwire stefanwire force-pushed the FS-905/delete-client-remove-proposals branch from 008b404 to dabf8d8 Compare September 6, 2022 15:54
@stefanwire stefanwire temporarily deployed to cachix September 6, 2022 15:55 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 6, 2022 15:55 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 07:28 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 07:28 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 09:33 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 09:33 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 14:06 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 14:06 Inactive
@stefanwire stefanwire force-pushed the FS-905/delete-client-remove-proposals branch from 66ecc4c to 888ad3d Compare September 7, 2022 14:15
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 14:15 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 7, 2022 14:15 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 8, 2022 08:58 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 8, 2022 08:58 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 8, 2022 09:09 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 8, 2022 09:09 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 8, 2022 09:55 Inactive
@stefanwire stefanwire temporarily deployed to cachix September 8, 2022 09:55 Inactive
@smatting smatting temporarily deployed to cachix September 8, 2022 11:05 Inactive
@smatting smatting temporarily deployed to cachix September 8, 2022 11:05 Inactive
@smatting smatting force-pushed the FS-905/delete-client-remove-proposals branch from 8e0a381 to cfbd4d6 Compare September 8, 2022 14:36
@smatting smatting temporarily deployed to cachix September 8, 2022 14:36 Inactive
@smatting smatting temporarily deployed to cachix September 8, 2022 14:36 Inactive
@smatting smatting force-pushed the FS-905/delete-client-remove-proposals branch from cc71c48 to 8c4b1a4 Compare September 9, 2022 08:08
@smatting smatting temporarily deployed to cachix September 9, 2022 08:08 Inactive
@smatting smatting temporarily deployed to cachix September 9, 2022 08:08 Inactive
@smatting smatting force-pushed the FS-905/delete-client-remove-proposals branch from 8c4b1a4 to 8223edc Compare September 14, 2022 15:47
@smatting smatting temporarily deployed to cachix September 14, 2022 15:47 Inactive
@smatting smatting temporarily deployed to cachix September 14, 2022 15:47 Inactive
@smatting smatting temporarily deployed to cachix September 14, 2022 15:49 Inactive
@smatting smatting temporarily deployed to cachix September 14, 2022 15:49 Inactive
@smatting smatting marked this pull request as ready for review September 14, 2022 15:50
@smatting smatting changed the title FS-905 Deleting clients creates MLS remove proposals Deleting clients creates MLS remove proposals Sep 14, 2022
mdimjasevic
mdimjasevic reviewed Sep 15, 2022
View reviewed changes
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good, but I have some questions and comments that are inlined.

services/galley/src/Galley/API/Clients.hs Outdated
>>= logAndIgnoreError "Error in onConversationUpdated call" usr

logAndIgnoreError message usr' res = do
case res of
Copy link
Contributor

@mdimjasevic mdimjasevic Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion: you can use whenLeft here:

whenLeft :: Applicative m => Either a b -> (a -> m ()) -> m ()

Copy link
Contributor Author

@smatting smatting Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice, didn't know about this one

services/galley/src/Galley/API/Clients.hs
removeRemoteMLSClients cids = do
for_ (bucketRemote (fromRange cids)) $ \remoteConvs -> do
runFederatedEither remoteConvs (rpc (ClientRemovedRequest usr cid (tUnqualified remoteConvs)))
>>= logAndIgnoreError "Error in onConversationUpdated call" usr
Copy link
Contributor

@mdimjasevic mdimjasevic Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't have the message been about "on-client-removed"?

Copy link
Contributor Author

@smatting smatting Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. rpc is bound to fedClient @'Galley @"on-client-removed". The reason we define this name outside is because creating the servant client rpc is costly apparently

Copy link
Contributor

@mdimjasevic mdimjasevic Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow. I am asking if instead of:

  logAndIgnoreError "Error in onConversationUpdated call" usr

it should have been:

  logAndIgnoreError "Error in onClientRemoved call" usr

instead.

@smatting smatting temporarily deployed to cachix September 15, 2022 12:05 Inactive
@smatting smatting temporarily deployed to cachix September 15, 2022 12:05 Inactive
@smatting
Copy link
Contributor Author

smatting commented Sep 15, 2022

@mdimjasevic Thanks for the review! I followed all your suggetions in 85575f0

pcapriotti
pcapriotti approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smatting smatting merged commit 8493c5c into develop Sep 15, 2022
@smatting smatting deleted the FS-905/delete-client-remove-proposals branch September 15, 2022 13:42
@zebot zebot mentioned this pull request Sep 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcapriotti pcapriotti pcapriotti approved these changes

@stefanwire stefanwire Awaiting requested review from stefanwire

@elland elland Awaiting requested review from elland

+1 more reviewer

@mdimjasevic mdimjasevic mdimjasevic left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@smatting @pcapriotti @mdimjasevic @zebot @stefanwire