Page MenuHomePhabricator

[lib][web][native] Move deviceToken to keyserverStore
ClosedPublic

Authored by inka on Oct 27 2023, 9:02 AM.
Tags
None
Referenced Files
F3389272: D9622.id32880.diff
Fri, Nov 29, 6:20 PM
F3388118: D9622.id32638.diff
Fri, Nov 29, 12:51 PM
Unknown Object (File)
Wed, Nov 27, 3:23 PM
Unknown Object (File)
Wed, Nov 27, 2:54 PM
Unknown Object (File)
Wed, Nov 27, 2:25 PM
Unknown Object (File)
Sat, Nov 23, 9:55 PM
Unknown Object (File)
Sat, Nov 23, 9:00 PM
Unknown Object (File)
Sat, Nov 23, 6:54 PM

Details

Summary

issue: https://linear.app/comm/issue/ENG-5284/refactor-setdevicetoken-action
We will need to communicate the device token to every keyserver we are connected to. The device token may change. A keyserver might not have our current device token. Each keyserver may realise they have the wrong token at a different time. We want to keep track of each keyservers knowledge. I am moving the logic that was used for one keyserver to multiple keyservers. This logic is flawed, but it will be updated in a different task: https://linear.app/comm/issue/ENG-5599/device-token-setting-mechanizm

Test Plan

Checked:

The migration, the keyserver selectors, sanitizeState, logInExtraInfoSelector

that the reducer sets the device token in the keyserver store on setDeviceTokenActionTypes.success action,

that the notifs still work,

that when the ks has an incorrect device token it is able to get a new one in the same way as it was before

checked that if a user turns off and back on the notifications, they work

checked that login works. Assuming that register and siweAuth work by symmetry

ran yarn flow-all

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka requested review of this revision.Oct 27 2023, 9:19 AM
native/push/push-handler.react.js
245–247 ↗(On Diff #32467)

This doesn't make much sense now because setDeviceToken action is not yet refactored for multiple keyserver. It will be done in the next diff, and this logic will be updated

277–285 ↗(On Diff #32467)

The keyserver might try to send a notif and get an error from the Push service even before the client is notified about the token change. Also, the keyservers don't necesserly lear this at the same time. Thats why if any of the keyservers sets the deviceID to null we want to check if notifs are enabled

ashoat added a subscriber: marcin.

It might make sense for @marcin to take a look at this given his experience with notifs

Don't we need a migration for web too?

lib/selectors/keyserver-selectors.js
102–107 ↗(On Diff #32638)
lib/types/account-types.js
51–55 ↗(On Diff #32638)

Does this make sense?

Don't we need a migration for web too?

deviceToken wasn't being persisted, so I don't think so (it's not on the persistWhitelist)

lib/types/account-types.js
51–55 ↗(On Diff #32638)

If you mean DeviceTokenUpdateRequest then yes

This revision is now accepted and ready to land.Nov 7 2023, 2:11 AM
This revision was landed with ongoing or failed builds.Nov 7 2023, 6:34 AM
This revision was automatically updated to reflect the committed changes.