Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better events for deletion of team conversations #849

Merged
merged 7 commits into from
Sep 10, 2019

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Sep 10, 2019

Start sending conversation.delete event also to all team members (whether in the conversation or not). team.conversation-delete will still be sent as before.

This solves the following issue:

One side effect of getting only one of the team.converation-delete and conversation.delete events is that we can't display who deleted a conversation in a team. The team.converation-delete event doesn't contain the from field.

Also, lots of small refactoring.

@fisx
Copy link
Contributor Author

fisx commented Sep 10, 2019

galley
  Galley integration tests
    Teams API
      [...]
      delete team conversation:                  FAIL (3.28s)
        test/integration/API/Teams.hs:742:
        unexpected notification received: (0,Notification {ntfId = 6f6fe025-d3d0-11e9-8001-901b0e95d6e0, ntfTransient = False, ntfPayload = List1 {toNonEmpty = fromList [("conversation",String "6dd7e926-89fc-4c4c-9944-89430923582b"),("time",String "2019-09-10T13:39:38.776Z"),("data",Null),("from",String "02ce384a-fd6e-46f9-a1ab-d0a3a86b8e41"),("type",String "conversation.delete")] :| []}})

If I go to line 742 in test/integration/API/Teams.hs and try to pluck another conversation.delete event from either of the 3 web socket handles, I get different errors:

        Exception: Timeout: No matching notification received.

or

        Exception: Timeout: No matching notification received.
        Match failure: expected: d5696a08-b18b-40d9-8296-07ba27d73a15
         but got: a05af6e5-4baf-476e-a3fa-8f9973d4db39

@fisx fisx changed the title [WIP] Better events for deletion of team conversations Better events for deletion of team conversations Sep 10, 2019
@fisx fisx marked this pull request as ready for review September 10, 2019 15:02
Copy link
Contributor

@tiago-loureiro tiago-loureiro left a comment

Choose a reason for hiding this comment

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

Looks good to merge assuming it passes CI tests

@fisx fisx merged commit 923d555 into develop Sep 10, 2019
@fisx fisx deleted the fisx/conv-delete-events branch September 10, 2019 18:52
@fisx fisx mentioned this pull request Sep 16, 2019
jschaul added a commit that referenced this pull request Nov 4, 2019
Events about deleted conversations do not need to be sent to everyone
when a team is deleted. Indeed, it is harmful in that it leads to performance problems:
For large teams with many conversations, notifying
every team member about that fact that every conversation has been
deleted creates a lot of events.
None of these events are useful, since on a team deletion, the accounts
of every user are deleted, i.e. in practice these events cannot be read.

This PR fixes the bug that was introduced in #849
jschaul added a commit that referenced this pull request Nov 4, 2019
Events about deleted conversations do not need to be sent to everyone
when a team is deleted. Indeed, it is harmful in that it leads to performance problems:
For large teams with many conversations, notifying
every team member about that fact that every conversation has been
deleted creates a lot of events.
None of these events are useful, since on a team deletion, the accounts
of every user are deleted, i.e. in practice these events cannot be read.

This PR fixes the bug that was introduced in #849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants