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

Migrate to the new FCM HTTP v1 API #980

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

shankari
Copy link
Contributor

As of July 2024, the FCM legacy API is being decommissioned and performance has been steadily degrading. This was reported by a deployment partner, who highlighted that they were not seeing notifications.
https://firebase.google.com/docs/cloud-messaging/migrate-v1

I fixed this by:

  • migrating to the most recent version of pyfcm
  • switching the authentication method and adding configuration variables for it
  • migrating to the new API, the biggest part of which involved creating a backwards compat function since the new version of the library only supports notifying one device at a time

@JGreenlee and @TeachMeTW for visibility. If you do end up implementing push notifications in the admin dashboard, it would be good to clean this up a bit.

The configuration that needs to be specified for v1 calls is different from the
legacy FCM API
https://github.com/olucurious/PyFCM?tab=readme-ov-file#updates-breaking-changes

We now need to specify the path to the service account JSON file, and the
project_id. Adding these to the config file temporarily so that we can test it
on staging more easily. Once they are known to work, we can put them into the
list of environment variables for the cloud services team to configure.
- Read the paramters for the new HTTP v1 API from the config
- The new version of the library does not support notifying multiple devices at
  the same time. Instead, it supports notifying only one device at a time. The
  return value from the `notify` method is also just the notification ID and
  not the summary with the structure showing the number of successes, number of
  failures, and then the results. Addressed this by creating a backwards compat
  function that has largely the same interface as before
- Renamed the arguments to match the new version
- The new version does not support sending structured data, only a flat K-V
  array, so changed the structure of the message sent for visible notifications
- The new version only supports strings in the K-V array, so changed the notID
  from an int to a string.

Testing done:

Running the following command with both `nrel-commute` and stage, saw visible notifications on both android and iOS

```
- ./e-mission-py.bash bin/push/push_to_users.py -a -t "Testing HTTP v1 API" "The legacy FCM APIs were shutdown starting July 22, 2024. Performance has been steadily degrading since then. This is testing the migration to the new API"
```

Silent push notifications generated a mixture of successful and unsuccessful results

```
./e-mission-py.bash bin/push/silent_ios_push.py -d 3600
{'ios': {'success': 16, 'failure': 14, 'results': ....}
```
@shankari shankari merged commit 802640c into e-mission:master Sep 12, 2024
5 checks passed
shankari added a commit to shankari/e-mission-server that referenced this pull request Oct 6, 2024
As part of e-mission#980
(and
e-mission@5a8ca40
in particular), we initialize the firebase module with the values we read from
the config.

However, we removed the initialization of the `server_auth_token`
https://github.com/e-mission/e-mission-server/pull/980/files#diff-5a90853314b14f3bec03976fa2ba81ec657e6327b770e9b376cfeabc0544487eL24

This is no longer used to send the actual notification, but it is used to map
the FCM tokens to APNS.

Note this is working for some deployments (e.g. nrel_commute) but not for other (e.g. stage). I can confirm that I get reminder notifications on my personal android phone. I don't understand how this ever worked - before e-mission#980, we weren't using the HTTP v1 version so no notifications were sent.  After the  change, this should have prevented notifications from being sent out.

Testing done.

Before the change

```
Connecting to database URL ...
Traceback (most recent call last):
  File "/usr/src/app/bin/push/silent_ios_push.py", line 24, in <module>
    response = pnu.send_silent_notification_to_ios_with_interval(args.interval, dev=args.dev)
  File "/usr/src/app/emission/net/ext_service/push/notify_usage.py", line 41, in send_silent_notification_to_ios_with_interval
    return __get_default_interface__().send_silent_notification(token_map, {}, dev)
  File "/usr/src/app/emission/net/ext_service/push/notify_interface_impl/firebase.py", line 194, in send_silent_notification
    fcm_token_map = self.convert_to_fcm_if_necessary(token_map, dev)
  File "/usr/src/app/emission/net/ext_service/push/notify_interface_impl/firebase.py", line 137, in convert_to_fcm_if_necessary
    importedResultList = self.retrieve_fcm_tokens(unmapped_token_list, dev)
  File "/usr/src/app/emission/net/ext_service/push/notify_interface_impl/firebase.py", line 98, in retrieve_fcm_tokens
    importHeaders = {"Authorization": "key=%s" % self.server_auth_token,
AttributeError: 'FirebasePush' object has no attribute 'server_auth_token'
```

After the change

```
Config file not found, returning a copy of the environment variables instead...
Retrieved config: {'DB_HOST': '...', 'DB_RESULT_LIMIT': None}
Connecting to database URL ...
Received invalid result for batch starting at = 0
after mapping iOS tokens, imported 0 -> processed 0
combo token map has 30 ios entries and 0 android entries
Successfully sent to ...
Successfully sent to ...
Successfully sent to ...
Found error Token not registered while sending to token ... skipping
Successfully sent to ...
Successfully sent to ...
Found error Token not registered while sending to token ... skipping
Successfully sent to ...
Successfully sent to -3sW...
Found error Token not registered while sending to token ... skipping
Successfully sent to ...
```
@shankari shankari mentioned this pull request Oct 6, 2024
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.

1 participant