A notif can be sent after the app updates, but before migration is run or the keyserver is notified of the new codeVersion. In this case we need to handle notifications with older ids. Fortunately we can just detect if the id's contain '|' character and if not, we can append the keyserver id.
We don't need to do any c++ changes on the native side, because data is saved to the db immediately, so it will just get migrated normally.
Details
Details
Test web, iOS, macOS (windows wasn't tested but all desktop changes are on the web app level, so they are platform agnostic):
- try sending a notif with new id schema, check if they are correctly handled
- try sending a notif with old id schema, check if they are correctly handled
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
My general feedback:
- It would be ideal to have one generic/(reusable between types) function that does parsing + conversion, so that both of:
JSON.parse(rawMessageInfos)
and
....indexOf('|') === -1appear only inside this function, so that the code is maintainable. Having custom checks like:
let threadID = event.notification.data.threadID;
if (threadID.indexOf('|') === -1) {
threadID = `${keyserverPrefixID}|${threadID}`;
}makes code harder to understand and maintain.
Test web, iOS, macOS (windows wasn't tested but all desktop changes are on the web app level, so they are platform agnostic):
Android should be added to the test plan.
From my perspective the code correctly does what it is supposed to do. Requested changes apply to some code quality issues.
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 49–64 ↗ | (On Diff #28516) | messageInfos parsing + ID conversion code is the same for both notifications. I think it would be a good code quality measure to extract it to a common function (using generic for instance) and avoid repeated code. |
| native/push/push-handler.react.js | ||
| 534–544 ↗ | (On Diff #28516) | This also looks like a repeated parsing + conversion code that could extracted to a single function. |
| 540 ↗ | (On Diff #28516) | Let's avoid magic numbers like this. We should rather use some here (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some) or any other way of checking if there is any element that passes the test. However I have an intuition that we could use threadID for this purpose. |
Comment Actions
Extracted logic into two functions - one for threadID and second one for messageInfos.
Android should be added to the test plan.
Android was tested, I just missed it when writing the test plan, sorry!
| lib/utils/migration-utils.js | ||
|---|---|---|
| 100 ↗ | (On Diff #28525) | Is it possible to receive notification with threadID compliant with new schema and messageInfos not? Perhaps we can always rely on threadID to decide whether to convert. |
| native/push/push-handler.react.js | ||
| 589 ↗ | (On Diff #28525) | We should this if statement with newly introduced: convertNotificationThreadIDToNewIDSchema. |
| lib/utils/migration-utils.js | ||
|---|---|---|
| 100 ↗ | (On Diff #28525) | It's not possible but in some cases we don't have access to the threadID (e.g. iosBackgroundNotificationReceived) |