Page MenuHomePhabricator

Implement code in lib to create thick threads rescinds and badge updates
ClosedPublic

Authored by marcin on Aug 26 2024, 4:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 26, 8:02 AM
Unknown Object (File)
Tue, Oct 22, 1:18 PM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Mon, Oct 21, 3:33 AM
Unknown Object (File)
Fri, Oct 18, 9:47 PM
Unknown Object (File)
Fri, Oct 18, 9:41 PM
Subscribers

Details

Summary

This differential implements lib code to generate thick thread rescind and badge updates. Thick thread rescinds and bagde updates are sent only to devices of the same user. Rescinds are supported only on Android and iOS while badge updates are supported only for iOS, Android and MacOS.

Test Plan

Tested at the end of the stack

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
lib/push/send-hooks.react.js
164 ↗(On Diff #43628)

senderUserID should be truthy here - because of an early return.

lib/push/send-utils.js
296–297 ↗(On Diff #43628)

What's happening here?

710–764 ↗(On Diff #43628)

Can we reduce the duplication in this file?

lib/types/notif-types.js
32–38 ↗(On Diff #43628)

Would it be convenient to introduce a type field?

This revision is now accepted and ready to land.Aug 29 2024, 5:11 AM
lib/push/send-utils.js
296–297 ↗(On Diff #43628)

deliveryID is the information for Tunnelbroker which device is the recipient, cryptoID is the information for the sender which olm session to user for encryption. For dm's both are deviceID but they differ for keyserver notifs.

lib/push/send-hooks.react.js
164 ↗(On Diff #43628)

Flow doesn't understand it and forces me to put it in if statement.

lib/push/send-utils.js
710–764 ↗(On Diff #43628)

The duplication you are referring to is looping over platforms and code versions and calling the right notification creator function. It is easy to deduplicate for rescinds and badge updates since notification creator functions in those cases take the same sets of arguments regardless of platform. For visual notifs notification creator functions take different set of arguments for each platform so deduplication is much harder so I would skip it. Also this kind of duplication is present in keyserver/src/push/send.js file anyway.

lib/types/notif-types.js
32–38 ↗(On Diff #43628)

I am skeptical here. The type field is helpful when alternatives have overlapping field but in this case they can already by differentiated between using single if statement.

Reduce code duplication - remove duplicated for loops over platform -> version key maps

marcin requested review of this revision.Sep 3 2024, 2:46 AM

At this point, this code reduces a lot of duplication, but by introducing loops over platforms we can deduplicate a lot more.

lib/push/send-utils.js
447–450 ↗(On Diff #43855)

Can we have longer names for the type parameters?

583–607 ↗(On Diff #43855)

We probably can deduplicate it further by introducing a mapping from platform to an object with a couple of function and then iterating through platforms. Something like:

const notifFunctions = {
  ios: {
    notifCreatorCallback: createAPNsVisualNotification,
    transformInputBase: ...,
    ...
  },
  ...
};



.....



for (const platform in notifFunctions) {
  const versionsToDevices = devicesByPlatform.get(platform);
  if (versionsToDevices) {
    promises.push(buildNotifsForPlatform({platform, ...}));
  }
}

What do you think?

This revision is now accepted and ready to land.Sep 3 2024, 3:43 AM
lib/push/send-utils.js
583–607 ↗(On Diff #43855)

Does this approach account for the fact that different notifCreatorCallback have different type of input arguments?

lib/push/send-utils.js
583–607 ↗(On Diff #43855)

At least for visual notifs. For rescinds and badge updates it could work.

lib/push/send-utils.js
583–607 ↗(On Diff #43855)

I think there should be some ways of dealing with the fact that the types might be different. But I'm not sure we should spend too much time on it.

At least for visual notifs. For rescinds and badge updates it could work.

That's great! Let's do it for it then, and if the other case is more complicated, we can leave it as it is.

lib/push/send-utils.js
583–607 ↗(On Diff #43855)

I've just tried it and flow doesn't accept this. I think that if we create a map from Platform to a notif creator function and iterate over it flow cannot guarantee that we are passing the right combination of platform and notif creator function to buildNotifForPlatform. Fir instance it think we can pass a combo of android and createAPNsNotificationRescind.

Make generic types more verbose

lib/push/send-utils.js
583–607 ↗(On Diff #43855)

After a couple of hours of trying to get it done with specs to no avail (https://gist.github.com/marcinwasowicz/7288a089e2801d83670bb3336b568f47) we decided that we will proceed with current form of this diff.