-
Notifications
You must be signed in to change notification settings - Fork 354
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
FCM Support #620
FCM Support #620
Conversation
Hey! Thanks for the PR. Are you using this code in production already or is this an extraction from production and you're not using this fork directly? Testing push notifications is sadly a bit tricky since it's a long running task which also relies on clients and Google itself, so I'd love to hear at least a little success story. :) Ping @wolak88, I believe you were also interested in this. |
Hi @aried3r, yes, I am using my fork quite reliably in production. I was also thinking putting more effort in and also save the Firebase variables in the database, but in the end I was simply lacking time :) |
'token' => device_token | ||
} | ||
json['content_available'] = content_available if content_available | ||
json['notification'] = alert if alert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for jumping in. Thank you for implementing FCM support.
I'm trying to send push notification to iOS app with this branch, but the notification does not show up.
n = Rpush::Fcm::Notification.new
n.app = Rpush::Fcm::App.find_by(name: '...')
n.device_token = '***'
n.priority = 'normal'
n.notification = { title: 'Hello' }
n.save!
Rpush log says it sent succesfully but the Notification
JSON does not have message.notification
field.
pry(main)> Rpush::Fcm::Notification.first.as_json
=> {"message"=>
{"data"=>nil,
"android"=>
{"notification"=>
{"title"=>"Hello", "notification_priority"=>"PRIORITY_DEFAULT", "default_sound"=>true},
"priority"=>5,
"ttl"=>"86400s"},
"token"=> "***"}}
Then I dig into this PR code, found this line. I am not sure this is the cause of the problem I have, I guess this should be:
json['notification'] = alert if alert | |
json['notification'] = notification if notification |
or handle alert
like
rpush/lib/rpush/client/active_record/apns/active_record_serializable_notification.rb
Lines 16 to 32 in 31ae99c
def alert | |
string_or_json = read_attribute(:alert) | |
if has_attribute?(:alert_is_json) | |
if alert_is_json? | |
multi_json_load(string_or_json) | |
else | |
string_or_json | |
end | |
else | |
begin | |
multi_json_load(string_or_json) | |
rescue StandardError | |
string_or_json | |
end | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Joe-noh ,
Thanks for your suggestion!
If it is working for you, I don't see any issue changing it to that.
I think I remember that I had a reason to change it to that, but I would need to look into this again, since this was quite some time ago :)
Thanks a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forked and committed Joe-noh@9c81ece, then successfully pushed notification to iOS app. A bit naive implementation though, just FYI.
Once we have tests, maybe we can release this as an experimental feature so more people can submit feedback. It doesn't seem to be breaking existing functionality. However I'm a bit hesitant to add more gems. I wonder if we should have optional dependencies for this and perhaps also the webpush code? Perhaps something like WDYT @rpush/maintainers? |
I like that idea. |
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
:notification_failed, :notification_will_retry, :gcm_delivered_to_recipient, | ||
:gcm_failed_to_recipient, :gcm_canonical_id, :gcm_invalid_registration_id, | ||
:notification_failed, :notification_will_retry, :fcm_delivered_to_recipient, | ||
:fcm_failed_to_recipient, :fcm_canonical_id, :fcm_invalid_device_token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change not affect the GCM implementation of Rpush? I'd assume we should add, rather than replace the new reflections.
def as_json(options = nil) # rubocop:disable Metrics/PerceivedComplexity | ||
json = { | ||
'data' => data, | ||
'android' => android_config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this meant that this PR only implements Android support for FCM, not iOS, web etc?
|
||
def obtain_access_token | ||
authorizer = ::Google::Auth::ServiceAccountCredentials.make_creds(scope: SCOPE) | ||
authorizer.fetch_access_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could only find the bang-! version of this. https://googleapis.dev/ruby/googleauth/latest/Signet/OAuth2/Client.html#fetch_access_token!-instance_method. Is there an alias for it somewhere that I'm missing?
In general, I'm wondering if this is the most efficient implementation. As far as I understand, this will always request a new token, even if it hasn't expired yet.
This article mentions caching it. I don't think we need to use Google's cache stores but could use Rails.cache
for it. There's also a .needs_access_token?
helper. For this we'd need at least v1.1.0 of googleauth
according to googleapis/google-auth-library-ruby#347.
What do you think?
Some debug logs would probably help, because I don't recall how often Delivery
is being instantiated off the top of my head.
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change.
Any updates on this PR?? When will we be merging it to the core rpush?? |
Thanks everyone for the great work already put into this topic. Can I somehow offer my help in order to finalise it? |
any news? |
I'm sure we all just got the reminder email that June '24 is the decommission date of this API. @aried3r as you appear to be one of the maintainers here what's the plan for ongoing support of Rpush library?. I keep checking back and there's no commits in the main repo, and this particular change which seems fundamentally pivotal to Rpush being viable has not had any commentary. While it certainly seems that this is the go-to gem, it also seems like it might be not maintained going forward? As I said above very happy to help in what way we can (beer/gift cards etc!) to help show our appreciation, we're just not at the skill level that we would trust taking on core maintenance or providing an alternative pull request to resolve some of the PR feedback above. Also would be very useful to know if this isn't going to be maintained going forward so we can put in place alternative strategies. I completely understand this is the downside of a library becoming popular that overtime people just don't have the time to commit to it... it works so well we don't want to ditch it :) |
@notification = notification | ||
@batch = batch | ||
|
||
@uri = URI.parse("#{HOST}/v1/projects/#{ENV['FIREBASE_PROJECT_ID']}/messages:send") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to add firebase_project_id and other firebase credentials to the app model instead of the env variables.
There are projects which are supporting multi-project functionality such as "WhiteLabel" apps. At this point it will be an issue to support multiple apps.
Why not add credentials to the Rpush::Fcm::App in the same way as Rpush::Apnsp8::App?
E.g:
app = Rpush::Fcm::App.new(
name: 'My app',
project_id: ENV['FIREBASE_PROJECT_ID'],
account_type: ENV['FIREBASE_ACCOUNT_TYPE'],
client_id: ENV['FIREBASE_CLIENT_ID'],
client_email: ENV['FIREBASE_CLIENT_EMAIL'],
private_key: ENV['FIREBASE_PRIVATE_KEY']
)
app.save!
then just get these credentials from the app.
@uri = URI.parse("#{HOST}/v1/projects/#{app.project_id}/messages:send")
Thanks!
@mirkode are you still working on this? If not, can I (or someone else?) take this on? The deadline is in 5 months (June 2024). |
Get tests working without all the ENV variables
See rpush#620 for further information
Get tests working without all the ENV variables
Update CHANGELOG.md Implement proof of concept authorization Fix logger error Add some fields to AndroidNotification Remove notification node from message payload for FCM notifications 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 Fix notification handling in fcm/notification.rb See rpush#620 for further information Add comment to necessary_data_exists? method Stop auto-closing issues Not the same, but similar rationale to what Rails did recently: rails/rails@acf4816 Bump rack from 2.2.3 to 2.2.3.1 (rpush#638) Bump rails-html-sanitizer from 1.4.2 to 1.4.3 (rpush#642) Fix - missing gcm interface fix code formatting - no change Payload Data Size is now calculated based off the message data attribute 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 Adding tests Removing debug line Adding debug line requested in PR rpush#620 Get tests working without all the ENV variables Add properties to FCM App to allow operation without env vars Adding new attributes to redis model Adding 7.1 migration to active record tests. Updating Gemfile.lock FCM returning error when data is sent in Android payload Moving notification keys to the correct location - only supporting Android As the FCM code moves around notification keys, it would be good to handle both stirng and symbol keys. - Calling hook fcm_invalid_device_token if we get an invalid token response from FCM - Adding specs for FCM daemon - Clarifying error message Adding a google credential cache Fixing Logging Adding a google credential cache Remove content_available from FCM implementation. It doesn't appear to be suported in Android
This PR is a rough draft to handle the new Firebase Cloud Messaging protocol.
It currently only works by setting the needed authentication params as
ENV
variables.Those are:
FIREBASE_PROJECT_ID
GOOGLE_ACCOUNT_TYPE
GOOGLE_CLIENT_ID
GOOGLE_CLIENT_EMAIL
GOOGLE_PRIVATE_KEY
(please refer to
lib/rpush/daemon/fcm/delivery.rb:147
The code is based off of the
gcm
implementation.Also what is still missing are tests for everything
fcm
related.