JS-7167 Add fcm api support #13

nat-aryucharoen · 2024-05-02T17:39:39Z

This PR: in progress

follow FCM Support rpush/rpush#620 and FCM support - v1.1 rpush/rpush#660 to implement fcm to the rpush gem
only handle fcm for active record (not radis)
use token from app (will need to be manually create)
web pr: https://github.com/Codefied/housecall-web/pull/51310
jira: https://housecall.atlassian.net/browse/JS-7167

danielnho22 · 2024-05-14T17:57:19Z

lib/rpush/daemon/fcm/delivery.rb

+        PROJECT_ID = Settings.firebase.fcm_push_notification.project_id
+        ENDPOINT = "https://fcm.googleapis.com/v1/projects/#{PROJECT_ID}/messages:send".freeze


[Question/Blocker]: Is there a reason why this is being set here? I think either of these should be configurable from outside of this project

The library shouldn't be dependent on Settings.

It looks like this should be stored in the rpush_apps table. It uses STI, so we'd need to add a new column for it to do it properly.

add new columns to rpush apps: https://github.com/Codefied/housecall-web/pull/51155

lib/rpush/client/active_model/fcm/notification.rb

danielnho22 · 2024-05-14T18:18:01Z

lib/rpush/client/active_model/fcm/notification.rb

+              validates_with Rpush::Client::ActiveModel::PayloadDataSizeValidator, limit: 4096
+              validates_with Rpush::Client::ActiveModel::RegistrationIdsCountValidator, limit: 1000


[NOTE]: These are the same validations as current GCM, so we're good. I think this is good.

lib/rpush/client/active_model/fcm/notification.rb

danielnho22 · 2024-05-14T18:52:14Z

I think as it stands, the PR mostly works. I'd like to see if we can find away around not passing accessing Settings directly.

I also don't know how data is set in this case..

danielnho22 · 2024-05-14T18:52:27Z

@v7chan Could you take a look too?

v7chan · 2024-05-14T23:34:47Z

lib/rpush/client/active_model/fcm/app.rb

+              # TODO: Add whatever validation is needed here
+              # validates :auth_key, presence: true


We should fill this in to make it clear which columns we need for Fcm (presumably with the new column for project_id)

also need other columns also to make OAuth logic for fetching an access token self-contained in the Rpush: https://github.com/Codefied/housecall-web/pull/51155/files#diff-ad01b5deb18c1010b83f08165e8b3bdedd5e72e17a62a2c644506a8617f1b2ffR9

looks ok?

added validates :fcm_json_token, presence: true

fcm_access_token and fcm_access_token_expiration will be filled in with the first call, so i guess i cannot add the validation here

v7chan · 2024-05-15T16:46:17Z

lib/rpush/client/active_model/fcm/notification.rb

+              validates :priority, inclusion: { in: FCM_PRIORITIES }, allow_nil: true
+
+              validates_with Rpush::Client::ActiveModel::PayloadDataSizeValidator, limit: 4096
+              validates_with Rpush::Client::ActiveModel::RegistrationIdsCountValidator, limit: 1000


Shouldn't registration_ids not be set for FCM notifications? GCM allowed for multiple as array, but it looks like we want to use the device_token (singular) field here

fcm does not use registration_ids, i can remove this validation

v7chan · 2024-05-15T16:57:32Z

lib/rpush/client/active_model/fcm/notification.rb

+          def priority_for_notification
+            case priority
+            when 0 then 'PRIORITY_UNSPECIFIED'
+            when 1 then 'PRIORITY_MIN'
+            when 2 then 'PRIORITY_LOW'
+            when 5 then 'PRIORITY_DEFAULT'
+            when 6 then 'PRIORITY_HIGH'
+            when 10 then 'PRIORITY_MAX'
+            else
+              'PRIORITY_DEFAULT'
+            end


I'm not sure if I'm following this correctly. priority= only allows the values of 10 and 5, do we need to handle these other values?

we can remove this since we remove the duplicate override

v7chan · 2024-05-15T17:01:00Z

lib/rpush/client/active_model/fcm/notification.rb

+            json = {
+              'notification' => android_notification,
+            }
+            json['data'] = data if data


Does this need to be duplicated here? I think this is only for Android-specific override if necessary and since we are not using FCM to deliver to both platforms, not having to duplicate this key makes the payload smaller

I see similar duplicated values like notification_priority within the Android overrides. In general, I think the first phase of FCM we are adding is only to deliver to Android, Apns8 will continue to be used for iOS. So we can avoid duplicating platform-overrides for now.

How we use rpush will need to change if we are to deliver to both iOS and Android via FCM and that's not the intention right now anyway.

v7chan · 2024-05-15T17:06:41Z

lib/rpush/client/active_model/fcm/notification.rb

+            # Android does not appear to handle content_available anymore. Instead "priority" should be used
+            # with "low" being a background only message. APN however should support this field.
+            # json['content_available'] = content_available if content_available


I think we should either map this or disallow this field from being set, this comment won't be visible to users of rpush

looks like we are not currently using 'content_available' for sending push to android, if the anyone add this to the fcm notification, the NotificationKeysInAllowedListValidator will catch it.

v7chan · 2024-05-15T17:16:51Z

lib/rpush/daemon/fcm/delivery.rb

+          when 400
+            bad_request(response)
+          when 401
+            unauthorized


As we roll this out, it's worth catching up with the 400/401s we've been facing from Firebase in our web push. As far as I remember, there was a bit of intermittent nature to them and I don't know if Voice ever got to the bottom of it

https://housecall.atlassian.net/browse/VOICE-6463

At the very least we'll need to monitor closely to know if we're failing to deliver more than normal on FCM v1 compared to legacy.

it's worth catching up with the 400/401s

What do you mean by "catch up with"? As in have visibility into why they happen? We'll ensure we have some monitoring before we roll out.

Since failures get logged to sumologic we can add some reporting there. If needed, we can also add some metrics to datadog with the Reflections API, but I figured the sumologic logs should be good enough.

As in have visibility into why they happen?

Yeah, just chat with Voice and/or read the investigations they've left behind. There was some number of mysterious errors that I don't know if they ever got to the bottom of, just don't want us to assume all requests will magically work

might be this? for 401: https://firebase.google.com/docs/reference/fcm/rest/v1/ErrorCode

THIRD_PARTY_AUTH_ERROR (HTTP error code = 401) APNs certificate or web push auth key was invalid or missing. | A message targeted to an iOS device or a web push registration could not be sent. Check the validity of your development and production credentials

danielnho22 · 2024-05-16T22:40:43Z

lib/rpush/daemon/fcm/delivery.rb

+          when 400
+            bad_request(response)
+          when 401
+            unauthorized


it's worth catching up with the 400/401s

What do you mean by "catch up with"? As in have visibility into why they happen? We'll ensure we have some monitoring before we roll out.

Since failures get logged to sumologic we can add some reporting there. If needed, we can also add some metrics to datadog with the Reflections API, but I figured the sumologic logs should be good enough.

lib/rpush/daemon/fcm/delivery.rb

nat-aryucharoen added 3 commits May 1, 2024 15:43

Add fcm api support

b43033b

Add proj id and key with setting, use make_creds to get access token

c6dc361

Use access token from app and message from body

3e1a2f2

nat-aryucharoen requested a review from danielnho22 May 14, 2024 16:25

danielnho22 reviewed May 14, 2024

View reviewed changes

danielnho22 requested a review from v7chan May 14, 2024 18:52

v7chan reviewed May 15, 2024

View reviewed changes

nat-aryucharoen added 2 commits May 16, 2024 11:14

Get google creds and save token, no dup payload

a1f35eb

Add fcm to log message and add reflect

7112e15

nat-aryucharoen requested review from v7chan and danielnho22 May 16, 2024 21:31

danielnho22 approved these changes May 16, 2024

View reviewed changes

danielnho22 approved these changes May 17, 2024

View reviewed changes

lib/rpush/daemon/fcm/delivery.rb Show resolved Hide resolved

danielnho22 merged commit 2c7cfdd into branch54 May 22, 2024

danielnho22 deleted the JS-7167-add-fcm branch May 22, 2024 22:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS-7167 Add fcm api support #13

JS-7167 Add fcm api support #13

nat-aryucharoen commented May 2, 2024 •

edited

Loading

danielnho22 May 14, 2024

v7chan May 14, 2024

nat-aryucharoen May 15, 2024

danielnho22 May 14, 2024

danielnho22 commented May 14, 2024

danielnho22 commented May 14, 2024

v7chan May 14, 2024

nat-aryucharoen May 15, 2024

nat-aryucharoen May 15, 2024

v7chan May 15, 2024

nat-aryucharoen May 15, 2024

v7chan May 15, 2024

nat-aryucharoen May 15, 2024

v7chan May 15, 2024

v7chan May 15, 2024

nat-aryucharoen May 15, 2024

v7chan May 15, 2024

danielnho22 May 16, 2024 •

edited

Loading

v7chan May 17, 2024 •

edited

Loading

nat-aryucharoen May 17, 2024

danielnho22 May 16, 2024 •

edited

Loading

		PROJECT_ID = Settings.firebase.fcm_push_notification.project_id
		ENDPOINT = "https://fcm.googleapis.com/v1/projects/#{PROJECT_ID}/messages:send".freeze

		validates_with Rpush::Client::ActiveModel::PayloadDataSizeValidator, limit: 4096
		validates_with Rpush::Client::ActiveModel::RegistrationIdsCountValidator, limit: 1000

		# TODO: Add whatever validation is needed here
		# validates :auth_key, presence: true

JS-7167 Add fcm api support #13

JS-7167 Add fcm api support #13

Conversation

nat-aryucharoen commented May 2, 2024 • 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

danielnho22 commented May 14, 2024

danielnho22 commented May 14, 2024

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

danielnho22 May 16, 2024 • edited Loading

Choose a reason for hiding this comment

v7chan May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnho22 May 16, 2024 • edited Loading

Choose a reason for hiding this comment

nat-aryucharoen commented May 2, 2024 •

edited

Loading

danielnho22 May 16, 2024 •

edited

Loading

v7chan May 17, 2024 •

edited

Loading

danielnho22 May 16, 2024 •

edited

Loading