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.
Details
Details
Tested at the end of the stack
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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? |
| 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. |
Comment Actions
Reduce code duplication - remove duplicated for loops over platform -> version key maps
Comment Actions
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? |
| 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.
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. |
| 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. |