Skip to content

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Aug 8, 2023

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

Checklist

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

@battermann battermann changed the title Wpb 3633 client receives conversation deleted conversation events after federation delete event [WPB-3633] client receives conversation deleted conversation events after federation delete event Aug 8, 2023
don't send event on remote conv delete

test that 1:1 conv do not trigger event

changelog
@battermann battermann force-pushed the WPB-3633-client-receives-conversation-deleted-conversation-events-after-federation-delete-event branch from 1a51a6d to d61ff76 Compare August 8, 2023 16:22
@battermann battermann marked this pull request as ready for review August 8, 2023 16:22
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 8, 2023
@fisx fisx self-requested a review August 8, 2023 20:12
-- call to brig. This is so we can ensure that domains are correctly cleaned up if a service
-- falls over for whatever reason.
deleteFederationRemoteGalley :: Domain -> ExceptT Brig.API.Error.Error (AppT r) ()
deleteFederationRemoteGalley dom = do
lift . wrapClient . Data.deleteRemoteConnectionsDomain $ dom
assertNoDomainsFromConfigFiles dom
Copy link
Contributor

Choose a reason for hiding this comment

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

the reason for this line is that if you delete a remote that's also present in the config file, it will fail silently (signal success), as read is always the union of cassandra and yaml, and yaml can't be changed here. there is a comment about this somewhere in this CRUD family.

why did you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it is in the wrong place. At this point, the federation domain has already been deleted, and we are now cleaning up the local state. There is no need to check this again here.

for_ (Map.toList remoteConvs) $ \(cnv, lUsers) -> do
-- All errors, either exceptions or Either e, get thrown into IO
mapError @NoChanges (const (InternalErrorWithDescription "No Changes: Could not remove a local member from a remote conversation.")) $ do
E.deleteMembersInRemoteConversation (toRemoteUnsafe dom cnv) (N.toList lUsers)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is certainly more to the point! :)

@battermann battermann merged commit c23800d into develop Aug 9, 2023
@battermann battermann deleted the WPB-3633-client-receives-conversation-deleted-conversation-events-after-federation-delete-event branch August 9, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants