Page MenuHomePhabricator

[keyserver] Send web push notification
ClosedPublic

Authored by michal on Feb 21 2023, 4:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 5, 9:43 PM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Unknown Object (File)
Sun, Mar 31, 8:13 AM
Subscribers

Details

Summary

This diff actually sends a web push notification using the web-push library. The library is initialized during configuration loading (added in one of the previous diffs). Then we can just send a notification with the device_token (which contains e.g. an endpoint and keys but it's all handled by web-push). Otherwise the logic should be similar to iOS and Android notifications.

Depends on D6815, D6813, D6717

Test Plan

Tested with next diff initializes notif sending on the web app side:

  • test that notifs are send with correct values
  • check that the results are saved inside of the notifications table
  • check that errors are saved inside of the notifications table
  • check that device_token is removed after unsuccessful push notif

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.Feb 21 2023, 6:55 AM
ashoat added inline comments.
keyserver/src/push/send.js
621 ↗(On Diff #22827)

Can we type this more precisely? Object is basically any. We use it for iOS / Android because we have some untyped libraries there I think (could be wrong), but we should avoid introducing it here

631 ↗(On Diff #22827)

Why do we need this id? I thought it was an iOS-specific thing

746–749 ↗(On Diff #22827)

All new types should always be $ReadOnly. Please keep this in mind, and make sure it's addressed before putting diffs up in the future

749 ↗(On Diff #22827)

What is the meaning of codeVersion for web? Is this just something for the future after ENG-961 is complete?

753–755 ↗(On Diff #22827)

All new types should always be $ReadOnly. Please keep this in mind, and make sure it's addressed before putting diffs up in the future

758 ↗(On Diff #22827)

Can we avoid Object?

keyserver/src/push/utils.js
27 ↗(On Diff #22827)

We put the error code for APN up here. Should we put the error code for web in the same place?

212 ↗(On Diff #22827)

Can we avoid Object?

218–233 ↗(On Diff #22827)

I think this would be cleaner using .map

This revision now requires changes to proceed.Feb 21 2023, 6:55 AM

Fix types, move errors codes, answers to questions in inline comments.

keyserver/src/push/send.js
631 ↗(On Diff #22827)

There's no automatically generated ID for a notif by web push api, so I think it's good to have some id just to keep track of them. But they aren't necessary so I can remove them.

749 ↗(On Diff #22827)

Yeah, I just wanted it to be consistent with the rest of the notif system for now. Currently, it will be -1 as set by the getDevicesByPlatform function.

Accepting to unblock, but would be great if we could avoid Object for new code. Not sure how easy it is to avoid here and whether the type there is predictable from whatever API we are getting it from

keyserver/src/push/send.js
751 ↗(On Diff #23059)

Can we avoid Object?

keyserver/src/push/utils.js
204 ↗(On Diff #23059)

The style in the codebase is generally to make export type declarations inline with where the type is defined

214 ↗(On Diff #23059)

Can we avoid Object?

271 ↗(On Diff #23059)

Instead of here at the bottom

This revision is now accepted and ready to land.Feb 24 2023, 3:12 PM

Move WebNotification type to lib so we can use it to type the push message in the service worker.

keyserver/src/push/send.js
241 ↗(On Diff #23244)

In D6872, I updated prepareIOSNotification and prepareAndroidNotification to take a bag of params. Can we do the same for prepareWebNotification?

prepareWebNotification now takes a bag of parameters