Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Push notifications #9

Merged
merged 4 commits into from
Aug 7, 2018
Merged

Push notifications #9

merged 4 commits into from
Aug 7, 2018

Conversation

rmi22186
Copy link
Collaborator

@rmi22186 rmi22186 commented Jul 31, 2018

This PR enables push notifications in MP's Braze SDK integration. I implemented a "soft push" as well, per Braze best practices for soft push prompts before the actual push notification request comes in.

A customer will follow along with the above instructions to set up a campaign, but can label the event whatever they want (not necessarily prime-for-push). Then in their app, they can call mParticle.logEvent('nameOfPushPrimerEvent') to activate.

Copy link

@froodian froodian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review request - in general this looks good to me, a couple things to note about this:

  • The integrator is still going to have to create their own Prime for Push campaign in the Braze dashboard as described at https://www.braze.com/documentation/Web/#soft-push-prompts

  • The event to trigger that campaign ("prime-for-push" in that example in the documentation) will have to be logged somehow. If the desire is to do that prompt on initial page load, note further that logging that event will need to be embedded in the messagingReadyCallback for openSession, e.g.

appboy.openSession(function() {
  appboy.logCustomEvent("prime-for-push");
});

to ensure that the soft prompt message will deliver. We have it slated to improve the behavior around message syncing and event logging in the future to prevent that necessity, but such is the current state.

  • Also note that for anybody who has the register_inapp option set to false, this won't work of course, because the primer will not be displayed - I'm not sure how that option is exposed or not.

@froodian
Copy link

froodian commented Jul 31, 2018

Also note it looks to me like the operations in this step https://www.braze.com/documentation/Web/#step-2-configure-your-site will also still need to be performed by the integrator:

  • declaring the manifest - this part is somewhat optional, strictly speaking, in today's world - it's only older versions of webkit based browsers that required gcm for web push, which is why it's necessary. We haven't yet removed it from our recommended setup but will be removing it soon as that portion of the global browser share (a few percent at my last estimate several months ago?) continues to shrink.
  • configure fcm key in the braze dashboard - only necessary if the above step is done
  • hosting service-worker.js (that name can be changed with an option to appboy.initialize) with either the importScripts to our cdn (as described in the above docs link) or the contents of that service worker. One potential snag here is that the version of that service worker should really match the version of the SDK that mparticle is using... we try not to make any changes in the SDK that would break the service worker or visa versa but there is some shared data they communicate with to enable analytics and I can't guarantee we won't ever make breaking changes there. The service worker needs to be served from the domain of the requesting site, but maybe what we could do is tell them to importScripts to a version of the service worker that mparticle rehosts that can stay in sync with this integration's version?
  • safari push certificate generation and upload if you want to support safari push. safari push is painful.

@rmi22186
Copy link
Collaborator Author

thanks for all the feedback. yes we plan to have all of the additional integration steps (manifest, service-worker.js, etc), in our docs when this is pushed, referencing the Braze docs where applicable.

@froodian
Copy link

froodian commented Jul 31, 2018

Sounds good, I guess the only two things to make sure you think more about is that if you recommend that they log the event on page load through your integration, that probably won't work, because of the messagingReadyCallback situation, and the possibility I noted about syncing the service worker version through an mparticle-hosted mirror of the correct version.

@rmi22186
Copy link
Collaborator Author

rmi22186 commented Aug 1, 2018

Cool. Reviewed this again.

  1. I updated the integration to incorporate the forwarderSettings.register_inapp properly. Now, we display the in app web message when the in app web message is a push-primer message regardless of the value of register_inapp, or if it's an inapp web message and register_inapp = true. tested and works in all situations:

    • when forwarderSettings.register_inapp === 'false', push primer still shows. user does not receive future in app messages, but does receive push notifications if elected
    • when forwarderSettings.register_inapp === 'true', push primer shows, and all future in app messages will show, as well as push notifications is elected
  2. wrt appboy.logCustomEvent('push-for-prime'), I had it in appboy.openSession(), but removed if for a few reasons:

    • avoid any potential inflating of the push-for-prime custom event for clients in the Braze dashboard. * we thought we'd give more control over to our clients as to when they primed their users (perhaps they don't want to immediately on session load).
    • giving clients the option to log a different name in the custom event from the mparticle side so that it shows up how they want to (in our docs we would tell them to user a different custom event name trigger in Braze's campaign set up)
  3. Question regarding not including it as the messagingReadyCallback to appboy.openSession. Isn't it true that events that are logged before Braze is fully loaded will be queued, and so if someone logs mParticle.logEvent('ask-for-push'), it will be queued Braze isn't loaded, then logged once Braze loads, and so would trigger a soft push request? Or will that queue be emptied before a session starts, and a session starting is required for triggers to occur? Seems to work fine when everything is loaded and then I log a custom push-request event from mParticle, but it's being finicky when appboy isn't loaded yet, so thought I'd ask.

  4. I will discuss internally your recommendation regarding hosting the servicer worker so that when we release it is always in line.

@rmi22186
Copy link
Collaborator Author

rmi22186 commented Aug 1, 2018

  1. I added a serviceWorkerLocation option as well, and will be sure to include in our docs the following from Braze's docs:

image

@rmi22186
Copy link
Collaborator Author

rmi22186 commented Aug 2, 2018

@froodian - quick ping as we'd like to get this out by next wednesday. can you clarify my point 2/3?

thanks!

@froodian
Copy link

froodian commented Aug 2, 2018

  1. yeah, these are valid counter-arguments to doing that, I agree that flexibility is nice.
  2. the appboyQueue you refer to handles the period before the script is even loaded from the remote URL - once the script file has been loaded, it replays those methods, flushes that queue, and it's not used from that point on. The period I'm referring to here is the situation where the SDK has been loaded and initialized, but is in the process of requesting updated messaging content - this happens most visibly on the very first page load ever for a user, and then subsequently happens at session boundaries or changeUser (which is why those messagingReadyCallbacks exist), but the work that we have slated which I mentioned previously is to automatically internally queue events any time there's a pending request for updated messaging content and replay them when that messaging arrives, so that integrators won't have to manage that implementation detail any longer.

But unfortunately until then, if the event is going to be fired on page load, the only way that brand new users will get it on their first session is to use that messagingReadyCallback - after that initial sync has completed, future page loads will continue to use that cached messaging data, so firing the event on page load outside the callback should start working from that point on (assuming the campaign is targeted appropriately), just not on that first page load.

Sounds good on all other points, thanks, sorry it took me a sec to reply.

@rmi22186
Copy link
Collaborator Author

rmi22186 commented Aug 3, 2018

great. i've reverted the messagingReadyCallback and added an option for them to rename the prime-for-push whatever they want in our configuration options under softPushCustomEventName. if there is no value for it, then we won't prompt the user, to ensure our current customers are unaffected by the change.

one more thing for you @froodian - i noticed that for a first time visitor, when i log appboy.logCustomEvent('prime-for-push') before appboy fully loads, it'll get queued, and gets executed when the script loads, but the prime-for-push in app message doesn't come back. however, if i reload, and log the event again, it is queued, executed when the appboy script loads, then i do get the in app message. is that a bug related to what you were saying above regarding not the first page load?

thanks and have a great weekend!

@froodian
Copy link

froodian commented Aug 7, 2018

i noticed that for a first time visitor, when i log appboy.logCustomEvent('prime-for-push') before appboy fully loads, it'll get queued, and gets executed when the script loads, but the prime-for-push in app message doesn't come back. however, if i reload, and log the event again, it is queued, executed when the appboy script loads, then i do get the in app message. is that a bug related to what you were saying above regarding not the first page load?

So, that behavior sounds precisely like what logging inside the messagingReadyCallback should prevent. Are you saying you're seeing that behavior with the code that's currently in this request? I wouldn't expect that to be the case... when you call initialize, if you include enableLogging: true in the options, when you log the custom event the SDK should console log (or you can use appboy.setLogger to specify your own log output) what it thinks is happening around triggers. If you can reproduce the behavior you're describing and copy in that log output, I might be able to tell you more.

The other thing that I would check is that your campaign is properly targeted at a segment which includes all users - no external id or previous events or attributes required, etc - and that the campaign has 0% going to the control group in its targeting phase.

Additionally, you may want to control the "Allow users to become re-eligible to receive campaign" checkbox in the campaign if you want a given user to be able to see the prompt more than once - anonymous users will be persistently identified by cookies and localStorage, so unless both of those get cleared, they will remain the same user and will not receive the prompt more than once unless that box is checked.

@rmi22186
Copy link
Collaborator Author

rmi22186 commented Aug 7, 2018

Thanks for the response @froodian. That was the behavior with the previous code when I allowed the integrator to choose when to log the prime-for-push event.

Now I have the messagingReadyCallback back as you recommended and everything works great on page load! Let me know if you are good with this PR and I will merge it tomorrow.

@froodian
Copy link

froodian commented Aug 7, 2018

Awesome, yeah, that's what I'd expect - and the precise issue that I'm planning on solving within the SDK in a future version so that this won't be required any longer. Thanks, and this all looks good to me!

@rmi22186
Copy link
Collaborator Author

rmi22186 commented Aug 7, 2018

Great. Will merge into the v2.2 PR and that will then go out tomorrow. Thanks again!

@rmi22186 rmi22186 closed this Aug 7, 2018
@rmi22186 rmi22186 reopened this Aug 7, 2018
@rmi22186 rmi22186 merged commit 6676ed4 into update-version-to-2.2 Aug 7, 2018
@rmi22186 rmi22186 deleted the push-notifications branch March 28, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants