Details

Reviewers

kamil
michal
ashoat

Commits

rCOMM71fee1b8ed13: Send visual notifications in the same order as they were encrypted

Summary

This differential ensure that visual (the ones making an alert) notifications are sent in the same order as they were encrypted with respect to the same
device withing single sendPushNotifs promise resolution. This is achieved by separating notification preparation and notification delivery steps. First
notifications are prepared (constructed and encrypted). This
step is entirely asynchronous. Then encrypted notifications are groupped by device token. In the same group notifications are sent sequentially in order mthcing
encryption order. Different groups are awaited asynchronously.

Test Plan

Test that notifications functionality remains unchanged - I sent messages and rescinds between four devices: two webs and two natives (Android and iOS).
Test that notifications are indeed sent in encryption order for single device - I added console.log to each <platform>Push function to examine whether notifications are sent in the right order based on encryptionOrder field of Targeted<platform>Notification instance.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Nov 10 2023, 6:03 AM

Herald added subscribers: will, tomek. · View Herald TranscriptNov 10 2023, 6:03 AM

Rebase to update commit message

marcin added a parent revision: D9818: Refactor Windows push code to match conventions for other platforms.Nov 10 2023, 6:09 AM

Harbormaster completed remote builds in B23991: Diff 33065.Nov 10 2023, 6:24 AM

marcin edited the test plan for this revision. (Show Details)Nov 10 2023, 6:27 AM

Harbormaster completed remote builds in B23994: Diff 33068.Nov 10 2023, 6:53 AM

marcin requested review of this revision.Nov 10 2023, 6:53 AM

Fix bug: Add encryptionOrder field from the value returned from prepareEncrypted<IOS/Android>Notifications to the value returned from
prepare<APNs/Android>Notification.

marcin added a child revision: D9821: Send badgeOnly notifications in the same order as they were encrypted.Nov 10 2023, 7:20 AM

Harbormaster completed remote builds in B23996: Diff 33070.Nov 10 2023, 7:31 AM

ashoat added inline comments.Nov 10 2023, 8:27 AM

keyserver/src/push/send.js
159 ↗	(On Diff #33070)	Can you separate this out so the `await` is its own assignment?
494 ↗	(On Diff #33070)	Can you separate this out so the `.flat()` happens on its own line?

ashoat accepted this revision.Nov 11 2023, 4:01 PM

ashoat added inline comments.

keyserver/src/push/send.js
459–473 ↗	(On Diff #33070)	Can these two `map`s be combined into one?
502–505 ↗	(On Diff #33070)	I think this is supposed to be two function invocations in `lodash/fp`, no?
507–516 ↗	(On Diff #33070)	Does this function need to be redefined every time `deliverPushNotifsInEncryptionOrder` is invoked?
529 ↗	(On Diff #33070)	Please pull `await` to the top level, you are hiding it here. `await` should either be the first word in the statement, or the first part of the right side of an assignment
554 ↗	(On Diff #33070)	Please pull the `flat` to its own line