This diff implements generation of notification texts for each recipient user.
Details
Details
Use the priviede hook in some client code that should generate notif (e.x. sendTextMessage). Log and examine the result
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/push/send-utils.js | ||
|---|---|---|
| 408–414 ↗ | (On Diff #41453) | This is a lot of params. Can you make it a param object instead? |
| lib/push/send-utils.js | ||
|---|---|---|
| 275 ↗ | (On Diff #41560) | This is simple logic:
|
| 331 ↗ | (On Diff #41560) | This function does the same as above but for all users. Additionally for each user it prepares separate threadInfo as it is done on the keyserver. |
| 381 ↗ | (On Diff #41451) | In next diff I will change it to return mapping from platform to array of notifications coupled with deviceID. |
| lib/push/send-utils.js | ||
|---|---|---|
| 376 ↗ | (On Diff #41566) | notifTextsForMessageInfo expects array since it potentially will suport a case of notification coalescing. But now we don't do it on the client so we always pass one element array. |
Comment Actions
Does this diff represent "factoring out" some logic, or are we reproducing a limited subset of some logic? If either is true, can you link the original logic this was inspired by?
| lib/push/send-utils.js | ||
|---|---|---|
| 300 ↗ | (On Diff #41566) | Is there a task tracking this? |
| 374 ↗ | (On Diff #41566) | Please replace "This" with something more specific. It's not clear what you're talking about |
| lib/push/send-utils.js | ||
|---|---|---|
| 392–395 ↗ | (On Diff #41566) | Can we move it to a type? It's used multiple times here |
| lib/push/send-utils.js | ||
|---|---|---|
| 275–317 ↗ | (On Diff #41566) | This is reproducing a limited subset of original logic that lives on the keyserver. Refer to: https://github.com/CommE2E/comm/blob/master/keyserver/src/push/send.js#L226-L296 |
| 333–345 ↗ | (On Diff #41566) | This is reproducing limited subset of original logic that lives on the keyserver. Refer to: https://github.com/CommE2E/comm/blob/master/keyserver/src/push/send.js#L695-L715 |
| 356–395 ↗ | (On Diff #41566) | This is reproducing limited subset of logic that lives on the keyserver. Refer to: https://github.com/CommE2E/comm/blob/master/keyserver/src/push/send.js#L134-L172 |
Comment Actions
Nit: when you linking code it's better to use the latest commit from the master, not the master branch as after some changes link will start pointing to different lines, and if someone needs context after a while it might get lost.
| lib/push/send-utils.js | ||
|---|---|---|
| 369–374 ↗ | (On Diff #41946) | nit: you can use PerUserNotifBuildResult but define perUserNotifTextsPromises as with let and use spreads but up to you |
| 413 ↗ | (On Diff #41946) | why this spread is needed? |
| lib/push/send-utils.js | ||
|---|---|---|
| 413 ↗ | (On Diff #41946) | Makes the type writable |
| lib/push/send-utils.js | ||
|---|---|---|
| 275–317 ↗ | (On Diff #41566) | Linked bound to specific commit: https://github.com/CommE2E/comm/blob/5be17d8cefbc45c01f10d41ce404cb0213705035/keyserver/src/push/send.js#L226-L296 |
| 333–345 ↗ | (On Diff #41566) | |
| 356–395 ↗ | (On Diff #41566) | Link bound to specific commit: https://github.com/CommE2E/comm/blob/5be17d8cefbc45c01f10d41ce404cb0213705035/keyserver/src/push/send.js#L134-L172 |