I was using copy-pasted code in several places, which is an anti-pattern. This diff should be a pure refactor.
Details
Details
- Reviewers
tomek atul - Commits
- rCOMM9a1eee09a936: [keyserver] Factor out importJSON
Here's my plan:
- Test backup: create keyservers/facts/backups.json, update cronjob schedule to run right now, and make sure backup is created:
- Test GeoIP update: create keyservers/secrets/geoip_license.json, and then run cd keyserver && yarn update-geoip
- Test push notifs on both iOS and Android (see @palys-swm's instructions in a comment below)
- Test URLs (make sure keyserver works)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- ashoat/dockerize_node
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Test push notifs (@palys-swm, any advice? I've never done this and I think you have)
On iOS we need to have comm_apn.p8 and comm_apn_config.json with "production": false. With these it should be possible to send notifications to a physical device in dev mode.
On Android, as far as I remember, the notifications work when comm_fcm_config.json is present, even on a simulator.
As to how test them, any scenario in which a notification is generated should work ok, e.g. when sending a message from user A to user B while the app is backgrounded.
| keyserver/src/push/providers.js | ||
|---|---|---|
| 30–54 | It doesn't look like these two usages will work because in the previous version we were importing the files and then using .default when creating Provider / cert, but now importJSON takes the default and returns its value. | |
| keyserver/src/utils/import-json.js | ||
| 3–6 | It is possible to avoid any by using a generic function, but I'm not sure if that's an improvement | |
| keyserver/src/utils/import-json.js | ||
|---|---|---|
| 3–4 ↗ | (On Diff #12698) | Should we update the comment? We're returning ?T instead of any |
| keyserver/src/utils/import-json.js | ||
|---|---|---|
| 3–4 ↗ | (On Diff #12698) | Good call – this comment can just be removed |