FCM support - v1.1 #660
FCM support - v1.1 #660
Conversation
There is a type mismatch between message.notification and message.android.notification. The message.notification only accepts the attributes title, body, and image, whereas message.android.notification holds all the rest of the configuration. So in order to use message.notification along with message.android.notification there would be the need for an additional database field called something like android_configuration
See rpush#620 for further information
Priority value is now using 'normal' or 'high' instead of numeric values Dry Run option is not implemented so throws an exception Mutable Content is not implemented so throws an exception
Get tests working without all the ENV variables
|
Hey @AnilRh, I made some changes on a fork regarding the usage of ENV vars. Maybe you would like to incorporate them or use them as inspiration. Just as a disclaimer, I haven't touched the tests and it is in general a quick-and-dirty solution. |
Add properties to FCM App to allow operation without env vars
|
@nigimaxx Looks great, thank you! I've added them to the PR. |
…andle both stirng and symbol keys.
|
Nice work all. We've relied on rpush for a while and would like to continue to use it after the deadline. Are there plans to merge this? Any blockers / testing the GH community could help with to get this over the line? |
…sponse from FCM - Adding specs for FCM daemon - Clarifying error message
|
I'd love to hear from maintainers on the next steps here. This is definitely a beta implementation. I've been trying to fit it into a staging environment and have a few more updates to push to this MR (commits if you're interested) Current status as far as I see it is that FCM appears to have basic features enabled to send a single notifications to an Android client. What's not clear to me is what the expectation here? Is there a strong use case to send a single notification to multiple devices, to both Apple(APN) and Android via FCM? What notification parameters do y'all use? I'm guessing @mirkode's use case was fairly straightforward. I hit an error immediately when trying to add "click_action" to the payload (should be fixed now). Also it doesn't appear to be a simple drop-in replacement for the old version as the "data" hash only allows string values. I apparently have some existing code that sends hashes within hashes. This will not work anymore. I don't think there's any getting around this; just pointing it out for those hoping this is a drop-in replacement (like I was). |
|
I've merged in a few changes based on my above comment:
Outstanding Otherwise, I think this is ready for me to try out in a real environment. |
|
Ok, I've added a 1 hour cache to the Google OAuth ServiceAccountCredentials code. |
|
Hi guys, can I gently ask you if there is any information about when this fix could be released? Thank you so much! |
|
@sfolador While I know you are waiting to hear from moderators, I can say that we're testing that branch in a staging environment at the moment, and I'll provide an update once it's in production. Obviously everyone has different use cases, but at a least getting it into production would put it under some real load. |
|
Thanks @AnilRh! We use Firebase to send notifications to both Android and iOS devices: do you know if this version of the package will allow also messages to iOS? I am asking because in your first message you said: "This implementation only appears to handle FCM Android messages, not Apple messages." |
|
@sfolador No, I wouldn't expect it to work as is. It should send iOS notifications, but for any attributes that are iOS specific (different from Android) a small bit of code likely needs to be added. If you look at Google's doc for the upgrade here you'll see the AFTER example has an "apns" part of the payload specific for iOS. In this FCM RPush implementation you can see here that there's an Android specific payload but apns is missing. I expect it's a simple addition for most attributes - we'd just need to know what they are. If you have a payload you send to FCM today that might be helpful in figuring out the change needed to add support. That said - if FCM is doing the work of supporting multiple platforms for you, you might also consider a gem that's specific to FCM. To me the advantage of RPush is to use it to talk to different platforms. I say this with hesitation, as I don't want people moving away from RPush... but at the same time, it's been awfully quiet in this repo. |
|
|
||
| def android_config | ||
| json = { | ||
| 'notification' => android_notification, |
There was a problem hiding this comment.
don't think we need to set explicitly notification here since original library don't have it and we can set it dynamically when someone creating the object, for some use case devs used to configure all stuff under data key, on those cases empty notification object is needed which helps custom rendering of the pushes
There was a problem hiding this comment.
let's make it more dynamic and remove setting custom sound, default sound under notification key
There was a problem hiding this comment.
Setting the notification here can cause issues as data messages would not get handled when the application is running in the background.
There was a problem hiding this comment.
@muzzamilpulsate @KrakenWagen This was done to try and make migration as easy as possible. My understanding of the api changes google provided (documentation) was that previously Google's backend manipulated things to make them Android or Apple friendly, while the new api does not.
So either when upgrading to Rpush FCM users will need to change their backend payload or we put something like this in place. I don't mind either way, I thought easy upgrade was preferable.
That said, if this will cause issues with data messages that obviously would need to be fixed. What exactly is the issue with data messages? Is it that default_sound would be set to true in this section? I think everything else would be empty for a data message.
There was a problem hiding this comment.
The problem here could be that 'notification' is a reserved keyword, so even if this is not setting the root 'notification' key, setting 'android'->'notification' is enough to cause data messages to not be handled in the background.
https://firebase.google.com/docs/cloud-messaging/concept-options#data_messages
Make sure that you do not use any reserved words in your custom key-value pairs. Reserved words include "from", "notification," "message_type", or any word starting with "google" or "gcm."
|
Anyone running this on production already? |
…o be suported in Android
|
I'm going to have a crack at running this in our staging environment in the next few weeks. We're really really low volume so not a good indicator for production (running around 600 notifications/day evenly split between iOS/Android). I'm assuming so long as it functionally works then there's no issues introduced with scale. We're doing a combination of standard user notifications, and silent messages (without the notification payload). Will report back on testing. |
|
Thanks @RowanH ! |
|
@aried3r @benlangfeld Any chance you could take a look at this? |
|
Hey everyone! I apologize for the long silence but I'm very happy to see people being interested and actively working on adding support for the new FCM protocol. Personally, I think we should make sure that both Android and iOS is covered and I saw some discussion around that already. I'm not sure how much time I'll find to review this so I'd appreciate anyone who can take a look and leave feedback. If existing functionality is not affected, we could consider releasing a pre-release version for anyone to try and collect feedback. What do you think? |
|
Hi @aried3r , I think it's a great idea for a pre-release version, can you help to proceed then we can deploy and test. We will get back to you for the result. Thank you! |
@CraigWangWWW @AnilRh guys I tried testing this in our prod env against test app, but I am getting notification very few times like 1 out of 15 is being received on android device |
|
@muzzamilpulsate That's interesting and rather concerning. Are you seeing failures in the rpush_notifications table or are the messages being marked as delivered? I've been slammed with other projects so haven't spent much attention on this lately. We have tested it in our staging environments, but due to FCM payloads needing to be string hashes with string values we were forced to change what data we send. This of course requires app changes with backwards compatibility so it's really slowed us down from moving to production. |
it has been fixed, actually prod instance was running old daemon process which wasn't reflecting the fcm changes |
|
What would next steps be for moving this forward @muzzamilpulsate @AnilRh? Anything I can help with? |
|
I was just reading back through the code to understand what to do for running new code. I haven't tried yet (likely tomorrows job) but to be clear- and in case others are looking on what they have to do:
And that's it - No other magic and it should just go ? |
On top of that:
notification = Rpush::Fcm::Notification.new
notification.notification = {title: 'Your title', body: 'Your body message'} |
|
@RowanH To add on to @KrakenWagen 's first point, I'd recommend reviewing all |
|
anyone thinking about / doing apns support? :) right now im thinking of just subclassing and handle |
| expect(notification.as_json['message']['notification']).to eq({"title"=>"title", "body"=>"body"}) | ||
| end | ||
|
|
||
| it "moves notification keys to the correcdt location" do |
There was a problem hiding this comment.
| it "moves notification keys to the correcdt location" do | |
| it "moves notification keys to the correct location" do |
Making minor changes to the great work already done by @mirkode in #620.
Changes:
Fixes:
Open Questions / Review Notes
Important Notes
firebase_project_id,json_keyand the ENV variables.