This differential uses EncryptedNotifsUtilsAPI to move notif generation and encryption to lib. APNs notifs are exception since in this case we will need to implement new types, creation and encryption from scratch instead of just smart copy-paste. It will be handled in separate diff.
Details
Details
- Flow
- Test all notifs for all platform and large notifs as well.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Looks good but want to see responses to the inline comments before accepting.
| keyserver/src/push/send.js | ||
|---|---|---|
| 419 | could you explain why not dbID? | |
| lib/push/notif-creators.js | ||
| 1 | This file is messy a bit, what do you think about slicing code here and moving code to specific files for each platform to make it a bit more readable? I am not a fan of creating huge files with multiple purposes but if you have a different opinion I am okay with it. Something like lib/push/android-notif-creators.js, lib/push/web-notif-creators.js etc. | |
| 31 | nit: maybe worth now moving those to a separate file with constants? | |
| 252 | I think this prop was added. Could you annotate all changes made additionally to moving code from /keyserver to /lib? Without annotating this, this diff is really hard to review | |
Comment Actions
Split notif creators for each platform to separate files.
| keyserver/src/push/send.js | ||
|---|---|---|
| 419 | This function will be used on the client where we decided to cut scope and skip notif persistence. Therefore dbID doesn't make sense in general context. | |
| lib/push/android-notif-creators.js | ||
|---|---|---|
| 3 ↗ | (On Diff #41554) | Consider this logic a copy paste from the keyserver |
| lib/push/web-notif-creators.js | ||
| 3 ↗ | (On Diff #41554) | Consider this logic a copy paste from the kesyerver. I only made it responsibility of the sender to create the id and pass it to function instead of generating it inside a function. |
| lib/push/wns-notif-creators.js | ||
| 1 ↗ | (On Diff #41554) | Consider this a copy paste from the keyserver. |
| lib/types/notif-types.js | ||
|---|---|---|
| 83 ↗ | (On Diff #41554) | For thick threads we don't include the badge count in the notif since we don't know the badge count of the client we send notif to. It will be the responsibility of the client to calculate the badge count. |
Comment Actions
Add type of encrypted message to encrypted notification (1 for text and 0 for prekey)