Skip to content

Comments

Fix MLS message notification bug#3610

Merged
pcapriotti merged 5 commits intodevelopfrom
pcapriotti/mls-message-notif-bug
Oct 13, 2023
Merged

Fix MLS message notification bug#3610
pcapriotti merged 5 commits intodevelopfrom
pcapriotti/mls-message-notif-bug

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Sep 25, 2023

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

This PR fixes a bug where some clients do not see MLS message notifications when fetching them with a client query parameter. The problem is that MLS message notifications are sent to gundeck as a single push, which will in general contain different Recipient objects for different clients of the same users. This makes the entries conflict with one another in cassandra, and therefore only one client ends up being stored in the database.

The fix here is "condensing" consecutive Recipient items relating to the same user by joining them together into a single Recipient object. This works because because recipient lists are generated by user.

This is now fixed by overloading newMessagePush so that it can work both with simple (User, Client) pairs - employed by Proteus messaging code - and with actual Recipient objects (i.e. one user, multiple clients) - used by MLS.

TODO

  • Fix bug for remote notifications

Checklist

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

@pcapriotti pcapriotti requested a review from smatting September 25, 2023 14:07
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 25, 2023
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-message-notif-bug branch 2 times, most recently from b1eadf7 to 584f2dc Compare September 28, 2023 14:41
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-message-notif-bug branch 2 times, most recently from c567483 to fb984ea Compare October 10, 2023 09:03
@pcapriotti pcapriotti requested a review from smatting October 11, 2023 12:44
@pcapriotti pcapriotti marked this pull request as ready for review October 11, 2023 12:44
@pcapriotti pcapriotti changed the base branch from mls to develop October 12, 2023 12:50
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-message-notif-bug branch from 7337774 to a8160a2 Compare October 13, 2023 07:26
Reorganise remote MLS message recipients by user, so that notifications
can be more easily reconstructed on the receiving side.
@pcapriotti pcapriotti force-pushed the pcapriotti/mls-message-notif-bug branch from a8160a2 to 2faadcf Compare October 13, 2023 07:53
@pcapriotti pcapriotti merged commit 43da11c into develop Oct 13, 2023
@pcapriotti pcapriotti deleted the pcapriotti/mls-message-notif-bug branch October 13, 2023 13:14
@pcapriotti pcapriotti mentioned this pull request Mar 8, 2024
2 tasks
@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

echoes: unplanned Any work item that isn’t part of the product or technical roadmap. 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.

4 participants