Rescinds also need to be converted. To do that we need to pass stateVersion in the delivery, alongside the codeVersion and using it convert the rescinds.
Details
Details
- Reviewers
marcin tomek - Commits
- rCOMMc8cac1994fe9: [keyserver] Convert ids in rescinds
Checked that notification were rescinded on macOS and Android, after displaying them on web app. Checked if the rescinded column in the notification table was set to 1, and a new rescind row was created.
Done this for both stateVersion < 43 (the ids aren't converted) and stateVersion >= 43 (the ids were converted).
Tested variant when the notification was sent to stateVersion < 43 but rescind was sent to stateVersion >= 43 (in this case I also checked if the group summary notification was removed).
Also tested if the notifications were rescinded if they sent from before these changes (current master) and so wouldn't have the stateVersion in delivery.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/push/rescind.js | ||
|---|---|---|
| 266 ↗ | (On Diff #29486) | Does this function always convert to ashoatKeyserverID? Or is it just a default parameter used in case threadIID represents old id schema. |
| 313 ↗ | (On Diff #29486) | I think this if statement should not be here. convertServerIDsToClientIfs should internally check whether the client can handle new id schema. This will make the code cleaner and more maintainable. Additionally I think this usage of NEXT_CODE_VERSION is incorrect. We have already released clients that can handle new id schema so there is a specific codeVersion that can be used here, So basically I think convertServerIDsToClientIfs implementation should start with an if statement that check against this specific codeVersion making it a no-op it the client cannot handle new id schema yet, |
| keyserver/src/push/rescind.js | ||
|---|---|---|
| 313 ↗ | (On Diff #29486) | I understand that refactoring `convertServerIDsToClientIfs is not necessary for this diff to work correctly. But it should be a quick change so lets avoid increasing technological debt if it does not cost much to do so. |
| keyserver/src/push/rescind.js | ||
|---|---|---|
| 265 ↗ | (On Diff #29486) | We probably shouldn't decide based on the code version. Instead, we should check a notification that we're rescinding and decide based on threadID we've sent there. The conversion function keeps the prefix when it's there, so it should never be necessary to convert here. What we probably should do is to update how we save notifications in our keyserver db. If we converted the id - save converted id, if not - then not. By doing that we should be able to avoid converting here. |
Comment Actions
Changed approach, more explanation in the updated description. Also updated the test plan.
I've tried keeping the updated ids inside the db, but the problematic thing is that we use them to query which notification should be rescinded, which "leaks" the id schema to the business logic so I decided against it.
Comment Actions
Checked that notification were rescinded on macOS and Android, after displaying them on web app. Checked if the rescinded column in the notification table was set to 1, and a new rescind row was created.
Did you try testing this with different codeVersion and stateVersion pairs - one that triggers threadID conversions and one that doesn't? Did you test edge case when notifications are delivered to device with low (now triggering threadID conversion) stateVersion, devices gets updated to higher stateVersion and then receives rescinds?
Removed stateVersion from one of the rescinds manually and checked if the notification was still rescinded.
How did you "manually removed" stateVersion from rescind?
| keyserver/src/push/rescind.js | ||
|---|---|---|
| 266 ↗ | (On Diff #29509) | This if statement should be a part of validateOutput function. |
Comment Actions
Fixed the readonly types. Amended the test plan for versions with and without conversion, and for rescinds where the notification was send to an older version/ from the older version of the keyserver.
This if statement should be a part of validateOutput function.
I decided to create a PlatformDetails object in the above function and make prepareIOSNotification and prepareAndroidNotification to take the whole platformDetails instead of separate versions. This way missing versions are handled by already existing code (hasMinStateVersion inside validateOutput).
How did you "manually removed" stateVersion from rescind?
I meant in the database. But please ignore this comment, I have removed it in the new test plan, for some reason hadn't thought that I can just test notification from a previous version of the keyserver the proper way.
| keyserver/src/push/rescind.js | ||
|---|---|---|
| 31 | New version of Flow caught a type error here: we forgot to update the ParsedDelivery type to reflect the addition of stateVersion | |