Add firebase cloud messaging and browser push #491

nicksellen · 2017-04-10T16:42:46Z

Initial work only.

Currently only pushes messages when you register for push.

Need to have firebase.json in project root with service account credentials from FCM. Will move to config somewhere in future.

No tests yet...

nicksellen · 2017-04-10T17:01:18Z

simison · 2017-04-10T17:38:46Z

plug it into the job thing to send notifications when needed

This would be at message-unread.server.job.js for unread notifications. I don't think we want to complicate this with any other kind of notifications just yet.

So UX is nice and simple and looks like
...
get feedback on UX

I like it!

We will probably want to nag users who don't have it checked by asking them if they want to enable it, store that we've nagged them to the browser session and ask again only when they login with new browser. But that can be the next step once this PR is done.

load the fcm server config from somewhere better

You can just add an example config block to config/env/default.js and the real one to your config/env/local.js (and perhaps to config/env/local.sample.js)

I would like to only load the firebase js libs if they are needed (only ~11kb though... in dev even)

Another feature for another PR. :-)

notification actions when you click the message

Link to the message thread. Just add analytics tags to the URL, see an example.

nicksellen · 2017-04-12T00:31:44Z

We will probably want to nag users

Yup, getting it working and live first seems more useful, make sure it actually works... I think it's pretty easy to get too naggy and annoying with push.

I suspect it will be better to notify on every new message too. I believe you can tag the messages too such that newer ones replace older ones so they don't stack up.

nicksellen · 2017-04-12T01:37:40Z

Aha, my fight with travis was victorious! 🥇

simison · 2017-04-12T07:15:45Z

I suspect it will be better to notify on every new message too. I believe you can tag the messages too such that newer ones replace older ones so they don't stack up.

Yep.

What's up with the change to Firefox?

PR looks great!

nicksellen · 2017-04-12T07:39:24Z

PhantomJS doesn't have much of the newer browser stuff supported and I'd like to add tests for the browser notifications.

Although using PhantomJS locally (same version too, 2.1.1 iirc) doesn't fail in the tests (isSupported gets set to false) but in travis it failed inside the firebase js lib because Promise doesn't exist.

Would be good to check it more properly with an unsupported browser to make sure it works fine.

simison · 2017-04-12T07:58:37Z

Would be good to check it more properly with an unsupported browser to make sure it works fine.

We have a BrowserStack account you could use to test old browsers. @chmac could share it with you via LastPass. (@chmac could you also re-share it with me and tick "can modify the password"?)

chmac · 2017-04-12T11:15:04Z

@simison Don't you have your own account? Anyway, I've just shared the login with you.

@nicksellen If you send me your lastpass email I can share it with you.

simison · 2017-04-12T11:26:30Z

Don't you have your own account? Anyway, I've just shared the login with you.

Sure, but for some reason I couldn’t see the password nor share that one. Thanks!

…

On 12 Apr 2017, at 14.15, Callum Macdonald ***@***.***> wrote: @simison Don't you have your own account? Anyway, I've just shared the login with you. @nicksellen If you send me your lastpass email I can share it with you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

nicksellen · 2017-04-12T12:15:30Z

@chmac lastpass@nicksellen.co.uk

nicksellen · 2017-04-12T15:35:30Z

There are nice tests now :) It was quite a battle to get karma/etc to be happy, so easy to lose track of a promise somewhere...

I split the client service into firebaseMessaging and push, so I can test the push service without loading any firebase libs. I'm quite pleased with the simplicity of the promise-based stuff [1].

[1]

trustroots/modules/core/client/services/push.client.service.js

Line 117 in a5800a8

function enable() {

nicksellen · 2017-04-12T15:43:55Z

F*** you Travis!

nicksellen · 2017-04-12T16:44:08Z

Ready for review!

Just a few things to look at #491 (comment)

nicksellen · 2017-04-12T22:10:08Z

modules/core/tests/client/push.client.service.tests.js

+      firebase.token = token;
+
+      // .. and enable it to be initialized
+      firebaseMessaging.shouldInitialize = true;


set this to false in a beforeEach hook

simison · 2017-04-12T22:17:38Z

modules/core/client/config/core.client.run.js


    // Attempt to initialize Facebook SDK on first page load
    // If this fails, we'll try this again on successfull login
    Facebook.init();

+    push.init();


A short comment over this could do. :-)

simison · 2017-04-12T22:31:31Z

modules/core/server/routes/core.server.routes.js

@@ -22,6 +22,9 @@ module.exports = function(app) {
  // Return a 404 for all undefined api, module or lib routes
  app.route('/:url(api|modules|lib|developers)/*').get(core.renderNotFound);

+  // Gives the service worker access to any config it needs
+  app.route('/config/sw.js').get(core.renderServiceWorkerConfig);


Normally these been exposed to index html template from config/lib/express.js.

The service worker does not have access to the HTML/DOM. It's very restricted.

Perhaps a better option is sending a message to it once it's started up [1]. I'm not very happy with the current approach.

[1] http://craig-russell.co.uk/2016/01/29/service-worker-messaging.html

simison · 2017-04-12T22:32:39Z

modules/core/server/controllers/core.server.controller.js

+exports.renderServiceWorkerConfig = function(req, res) {
+  res.set('Content-Type', 'text/javascript')
+     .send('var FCM_SENDER_ID = ' + JSON.stringify(config.fcm.senderId) + ';\n');
+};


ExpressJS has support for jsonp, but normally configs have been exposed to index html template from config/lib/express.js.

simison · 2017-04-12T22:39:15Z

public/push-messaging-sw.js

+
+messaging.setBackgroundMessageHandler(function(payload) {
+  // not actually used, but without it here firefox does not receive messages...
+  console.log('received payload', payload);


This function never gets fired but it has to be there? messaging.setBackgroundMessageHandler(Function.prototype) might be handy.

Also if Angular is already in window at this point, angular.noop is available.

No angular here, the service worker is a standalone script and they run with a limited environment.

firebase/quickstart-js#71 has a discussion about it the background handler stuff - the gist is that by sending a notification payload (as opposed to data) you are specifying it should be a regular notification, and not be overridable.

my suspicion about why it didn't work without that in firefox is that without defining some kind of listener it stops listening for new messages. it would be useful to verify it properly though...

Okidoki, thanks for clarifying!

simison · 2017-04-12T22:41:02Z

public/push-messaging-sw.js

+
+importScripts('/lib/firebase/firebase-app.js');
+importScripts('/lib/firebase/firebase-messaging.js');
+importScripts('/config/sw.js');


Okay, now I understood why the config was loaded like it was earlier! Ignore my earlier comments about this.

We should probably move all the configs to load like this. :-)

Ah, I liked them in index.html. This way would be 1 extra http request too, but could load the config into a standard TrustrootsConfig object or something. I'm easy.

They're handy in index.html indeed but stops making the app work self hosted and offline (e.g. on mobile). It's another discussion and another PR anyway.

simison · 2017-04-12T22:41:35Z

package.json

@@ -140,6 +141,7 @@
    "nodemailer-stub-transport": "~1.1.0",
    "phantomjs-prebuilt": ">=2.1",
    "promise": "~7.1.1",
+    "proxyquire": "^1.7.11",


~ just for consistency.

simison · 2017-04-12T22:41:38Z

package.json

@@ -61,6 +61,7 @@
    "express-paginate": "~0.2.1",
    "express-session": "~1.14.0",
    "fbgraph": "~1.3.0",
+    "firebase-admin": "^4.2.0",


~ just for consistency.

simison · 2017-04-12T22:50:06Z

modules/users/server/controllers/users/users.profile.server.controller.js

+      pushService.sendUserNotification(user, {
+        title: 'Trustroots',
+        body: 'A ' + platform + ' push device was added!',
+        click_action: 'http://localhost:3000/profile/edit/account'


This might work here better for flexibility with different setups (Docker IP, https):

var protocol = (config.https === true || req.protocol === 'https') ? 'https' : 'http'; var host = protocol + '://' + req.get('host'); // http://localhost:3000

...or actually this might be already available as res.locals.hostPort

Oh yeah, I forgot to fix that bit.

I also wonder whether it would be a good idea to send a notification at that point in production too, but only to the device you just added?

Also, maybe in development some other way to send notifications could be handy for manual testing/playing purposes...

I also wonder whether it would be a good idea to send a notification at that point in production too, but only to the device you just added?

Yeah I think so, it kinda confirms to the user that it worked.

So to send push to all the devices, just remove that if? I think it's okay to send it to all the devices even (how many there can be), just to avoid extra work. Feels kind of a security feature even in that way anyway. :-)

Copy could be something like "You just enabled Trustroots platform push notifications. Yay!"

Yup, sounds good :)

simison · 2017-04-12T23:13:22Z

modules/users/client/views/profile/profile-edit-account.client.view.html

+            </div>
+
+            <p class="help-block" ng-if="profileEditAccount.push.isSupported"><small>This will send notifications even when the website is closed.</small></p>
+            <p class="help-block" ng-if="!profileEditAccount.push.isSupported"><small>Your browser does not support push notifications.</small></p>


When unsupported, the whole block could just be hidden?

I did it that way so it communicates: if you use another browser then you can get push notifications

It also helps to keep a consistent interface, so users don't get confused wondering where the option went when they had it available on another browser.

I don't feel that strongly about it though. Happy to hide it all if unsupported if you prefer.

Good thinking 👍, let's leave it visible.

The section could go above password tho, so it would be more noticeable.

simison · 2017-04-13T13:23:29Z

use browserstack... automated or manual?

There's an old script for automated testing: https://github.com/Trustroots/trustroots/tree/master/scripts/selenium

...but frankly I'm not too keen to keep that up to date. I think manual testing and those few unit tests are great, we should notice pretty quick if it fails, esp. if we push some events to Influx stats.

simison · 2017-04-13T14:52:55Z

bower.json

@@ -4,6 +4,7 @@
  "dependencies": {
    "angular": "1.5.8",
    "angular-animate": "1.5.8",
+    "angular-cookies": "1.5.8",


BTW there's already https://github.com/tymondesigns/angular-locker which might save you some kb's since no need to add this lib, but I'm fine either way.

Seems a good idea

... it sends it to all devices for now

nicksellen · 2017-04-19T13:05:02Z

modules/core/server/services/push.server.service.js

+      source: 'push-notification',
+      medium: 'fcm',
+      campaign: 'device-added',
+      content: 'reply-to'


Not sure what this should be?

medium could probably be just push-notification, I'm not sure if we gonna differentiate different platforms in the future but doesn't matter for now.

Since there is just one place (notification click) pointing to the same url, content can probably be left empty. It's used to make difference e.g. between different parts of email linking to the same URL (e.g. "footer" and "header"). ...I think.

They're explained in this doc and at query builder

No stress over these, we can adjust them later.

nicksellen · 2017-04-19T13:23:53Z

Hmm a weird error from a simple change:

dst is undefined in public/lib/angular/angular.js

Investigating...

nicksellen · 2017-04-19T13:32:46Z

Aha, SettingsService is better than SettingsFactory...

nicksellen · 2017-04-19T13:46:23Z

@simison not sure why the pr build fails so frequently when the push one seems fine? I don't think there is an actual error.

One thing I haven't done is tried it in other browsers, but this is not totally simple because the security stuff requires https when not using localhost, so I can't access it from my phone via my machines ip address.

One solution would be to setup https locally, but also need to change a few settings to use DOMAIN env value instead of localhost (well, maybe just the csp dev config). I don't have time for this right now, and would suggest just trying it out when properly deployed.

nicksellen · 2017-04-19T13:46:59Z

Perhaps someone at least has a windows machine or mac to try it on though?

simison · 2017-04-19T14:10:15Z

I'll build this to dev machine, just a mo.

Edit: done!

simison · 2017-04-19T14:30:55Z

not sure why the pr build fails so frequently when the push one seems fine? I don't think there is an actual error.

Switching to Firefox is probably "causing" frontend Karma tests to fail now, but I'm not sure why exactly. Might be just some done():s missing since it's just timeouting.

nicksellen · 2017-04-20T09:39:20Z

Tried it on dev.trustroots.org on desktop and mobile, it all works!

Chrome

Chrome did not show the notification when the browser tab was the active one on the screen, but did when the tab was closed, and when the browser was not in the foreground. Not actually sure about if the browser is completely closed (I guess not).

When loading the site again after enable notifications it asked me if I wanted to add it to the home screen (so I suspect it only gets used once a web worker has been installed).

The notification icon is the chrome icon.

Firefox

Firefox always showed the notification (well, didn't try with browser completely closed, I guess not).

The notification icon is the firefox icon.

Android browser (not supported)

simison · 2017-04-20T09:43:06Z

Great!

So my todo seems to be:

look into those failing Karma test if I could help it out somehow
merge
get FCM account and config for Trustroots
deploy live

My goal is to get this live some time Thu—Sat.

Firebase is more abstracted now so should cause less problems not having all the latest browser APIs.

nicksellen · 2017-04-20T09:44:07Z

Switching to Firefox is probably "causing" frontend Karma tests to fail now, but I'm not sure why exactly. Might be just some done():s missing since it's just timeouting.

I do not feel motivated to try and fix it for this issue. I think it could take some time and is not really related to the push features. Actually it might be possible to switch to back to PhantomJS as I have abstracted the firebase stuff further away now - just pushed a commit to revert it (although I think it will have to change again eventually now the maintainer stepped down).

simison · 2017-04-20T09:45:36Z

Switching to Firefox is probably "causing" frontend Karma tests to fail now, but I'm not sure why exactly. Might be just some done():s missing since it's just timeouting.

I do not feel motivated to try and fix it for this issue. I think it could take some time and is not really related to the push features. Actually it might be possible to switch to back to PhantomJS as I have abstracted the firebase stuff further away now - just pushed a commit to revert it back (although I think it will have to change again eventually now the maintained stepped down).

I just checked, our Karma is 1.3.0 and newest is 1.6.0 so that might be causing it as well. Updating Karma + switching to Firefox seems like a separate, not so urgent PR.

Related to #491 "Add firebase cloud messaging and browser push"

nicksellen added the 2 - in progress label Apr 10, 2017

nicksellen mentioned this pull request Apr 10, 2017

Push notifications #394

Closed

3 tasks

nicksellen changed the title ~~WIP: Add firebase cloud messaging and browser push~~ Add firebase cloud messaging and browser push Apr 12, 2017

nicksellen commented Apr 12, 2017

View reviewed changes

simison reviewed Apr 12, 2017

View reviewed changes

simison added enhancement notifications labels Apr 13, 2017

simison reviewed Apr 13, 2017

View reviewed changes

nicksellen added 2 commits April 19, 2017 14:19

Add fcm config to local.sample.js

9ab70f1

Remove a comment

991ea3e

nicksellen force-pushed the fcm branch from 5f3941f to 991ea3e Compare April 19, 2017 12:21

nicksellen added 6 commits April 19, 2017 14:25

Set shouldInitialize to false before each test

11fbac8

Document use of push.init()

415dc31

Tilde instead of hat for deps

6e3b500

Notify use push device was added, update msg

f1e04fe

... it sends it to all devices for now

Move notifications config above password settings

052a166

Use locker instead of $cookies to save push config

ce837bd

nicksellen commented Apr 19, 2017

View reviewed changes

Use appSettings to get fcm senderId into frontend

7c58b82

Use SettingsService instead of SettingsFactory

e7b7219

Revert to PhantomJS for testing

8f25400

Firebase is more abstracted now so should cause less problems not having all the latest browser APIs.

simison merged commit 78596c1 into master Apr 20, 2017

simison removed the 2 - in progress label Apr 20, 2017

simison deleted the fcm branch April 20, 2017 14:03

This was referenced Apr 20, 2017

Ask people to turn desktop push notifications on #497

Closed

Push desktop notification on new message #498

Closed

HTML5 browser notifications #235

Closed

simison added a commit that referenced this pull request Jun 3, 2017

Add install instructions: how to enable FCM push notifications

c884544

Related to #491 "Add firebase cloud messaging and browser push"

Add firebase cloud messaging and browser push #491

Add firebase cloud messaging and browser push #491

Conversation

nicksellen commented Apr 10, 2017

nicksellen commented Apr 10, 2017 • edited by simison Loading

simison commented Apr 10, 2017 • edited Loading

nicksellen commented Apr 12, 2017

nicksellen commented Apr 12, 2017

simison commented Apr 12, 2017 • edited Loading

nicksellen commented Apr 12, 2017 • edited Loading

simison commented Apr 12, 2017

chmac commented Apr 12, 2017

simison commented Apr 12, 2017 via email

nicksellen commented Apr 12, 2017

nicksellen commented Apr 12, 2017 • edited Loading

nicksellen commented Apr 12, 2017

nicksellen commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksellen Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

nicksellen commented Apr 19, 2017

nicksellen commented Apr 19, 2017

nicksellen commented Apr 19, 2017

nicksellen commented Apr 19, 2017

simison commented Apr 19, 2017 • edited Loading

simison commented Apr 19, 2017 • edited Loading

nicksellen commented Apr 20, 2017

Chrome

Firefox

Android browser (not supported)

simison commented Apr 20, 2017 • edited Loading

nicksellen commented Apr 20, 2017 • edited Loading

simison commented Apr 20, 2017 • edited Loading

nicksellen commented Apr 10, 2017 •

edited by simison

Loading

simison commented Apr 10, 2017 •

edited

Loading

simison commented Apr 12, 2017 •

edited

Loading

nicksellen commented Apr 12, 2017 •

edited

Loading

nicksellen commented Apr 12, 2017 •

edited

Loading

simison Apr 12, 2017 •

edited

Loading

nicksellen Apr 12, 2017 •

edited

Loading

simison Apr 19, 2017 •

edited

Loading

simison commented Apr 19, 2017 •

edited

Loading

simison commented Apr 19, 2017 •

edited

Loading

simison commented Apr 20, 2017 •

edited

Loading

nicksellen commented Apr 20, 2017 •

edited

Loading

simison commented Apr 20, 2017 •

edited

Loading