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

feat: Route notifications to FCM (Android) #171

Merged
merged 23 commits into from
Jul 20, 2020

Conversation

AzureMarker
Copy link
Contributor

@AzureMarker AzureMarker commented Jul 9, 2020

Major changes:

  • Add the Firebase Cloud Messaging (FCM) router to support routing notifications to Android devices.

Minor changes:

  • Added .gitguardian.yml to ignore the fake OAuth credentials used during the FCM tests. config docs
  • Added a note to operation_notes.md about how to set nested settings via environment variables.
  • Unknown settings are now an error (serde's deny_unknown_fields)
  • Added an InsertOpt trait to autopush_common to make conditionally inserting an Option into a HashMap easier.

Closes #162

jrconlin
jrconlin previously approved these changes Jul 10, 2020
autoendpoint/src/server/routers/fcm/client.rs Outdated Show resolved Hide resolved
autoendpoint/src/server/routers/fcm/client.rs Show resolved Hide resolved
autoendpoint/src/server/routers/fcm/router.rs Outdated Show resolved Hide resolved
Google Cloud Messaging has been deprecated in favor of FCM, and as of
May 29, 2019 it no longer functions.
Also renamed the Content-Encoding header field to "encoding", which is
what the rest of autopush/webpush expects.
This will make testing easier, for example with mockito.
This fixes the integration tests, because sometimes users would not have
any router data in DynamoDB. Also, just to be safe, router_data uses
rusoto's AttributeValue instead of serde_json::Value.
This allows us to use DynamoDbUser with any version of rusoto.
@AzureMarker AzureMarker force-pushed the feat/autoendpoint-fcm-router branch from 64c6f5f to 14abb92 Compare July 13, 2020 18:19
@AzureMarker AzureMarker changed the base branch from feat/autoendpoint-webpush-router to master July 13, 2020 18:20
@AzureMarker AzureMarker dismissed jrconlin’s stale review July 13, 2020 18:20

The base branch was changed.

@AzureMarker AzureMarker reopened this Jul 13, 2020
@AzureMarker AzureMarker marked this pull request as ready for review July 13, 2020 18:21
@AzureMarker AzureMarker requested a review from jrconlin July 13, 2020 18:37
Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

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

Looks good.
We can probably make the gateway 404 == user delete a different issue, since it will hit all the routers.

let client = self.clients.get(app_id).ok_or(FcmError::InvalidAppId)?;
trace!("Sending message to FCM: {:?}", message_data);
if let Err(e) = client.send(message_data, fcm_token.to_string(), ttl).await {
return Err(self.handle_error(e));
Copy link
Member

Choose a reason for hiding this comment

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

The python version was recently updated to drop UAIDs for users when we get a NOTFOUND from the bridge. We should replicate that here. It would also help trim up the router table.

Copy link
Member

Choose a reason for hiding this comment

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

Added: #190

@AzureMarker AzureMarker merged commit d9a0d9d into master Jul 20, 2020
@AzureMarker AzureMarker deleted the feat/autoendpoint-fcm-router branch July 20, 2020 13:16
This was referenced Oct 16, 2020
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.

[autoendpoint] Route FCM notifications
2 participants