Page MenuHomePhabricator

[keyserver] Extract sendPushNotif
ClosedPublic

Authored by ashoat on Tue, Sep 12, 6:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 2, 7:26 AM
Unknown Object (File)
Tue, Sep 26, 8:30 PM
Unknown Object (File)
Mon, Sep 25, 5:45 PM
Unknown Object (File)
Sun, Sep 24, 7:26 AM
Unknown Object (File)
Tue, Sep 19, 9:14 PM
F757683: Screenshot 2023-09-13 at 1.12.54 PM.png
Wed, Sep 13, 10:13 AM
F757685: Screenshot 2023-09-13 at 1.12.55 PM.png
Wed, Sep 13, 10:13 AM
Subscribers
None

Details

Summary

In D6935 I made a mistake by awaiting inside a loop. In order to await the notifTextsForMessageInfo in parallel, I'm going to extract the code inside the loop into its own async function.

The name sendPushNotif isn't exactly accurate, in the sense that it sends multiple... but sendPushNotifs is already the name of its caller. One alternative I considered was sendPushNotifsForMessage, but that's also slightly inaccurate because there might be multiple new messages. Open to alternatives (or votes for the latter name).

Test Plan
  1. In the local dev environment I can test iOS & Android notifs. I tested creating a single message that generates notifs for both iOS and Android
  2. Flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
keyserver/src/push/send.js
174 ↗(On Diff #31020)

This name makes sense, as we're sending a single notifInfo but to multiple recipients

181–182 ↗(On Diff #31020)

It might be worth considering to avoid returning values by parameter (by mutating the input), by e.g. extending a return value to be an object containing dbIDs, rowsToSave and PushResult[]. On the other hand, it isn't a big issue and if it would take too much time, we can keep it as it is.

This revision is now accepted and ready to land.Wed, Sep 13, 6:02 AM
keyserver/src/push/send.js
181–182 ↗(On Diff #31020)

I thought about it, but the pattern of mutation to both dbIDs and rowsToSave is fairly complex...

In the case of dbIDs, we have items being removed. We'll have multiple sendPushNotif calls going in parallel, and we can't wait until one returns before we remove the dbID, because then multiple sendPushNotif calls might end up using the same dbIDs. We need to make sure that the value is removed immediately.

rowsToSave is not quite as complex, but it would still require an additional merging step if we did it that way. And it seems like it's not quite worth it given dbIDs will probably stay as a mutable parameter.

Successfully tested this:

iOSAndroid
Screenshot 2023-09-13 at 1.12.55 PM.png (2×1 px, 2 MB)
Screenshot 2023-09-13 at 1.12.54 PM.png (1×1 px, 832 KB)
This revision was automatically updated to reflect the committed changes.