-
Notifications
You must be signed in to change notification settings - Fork 144
FCM Implementation (Part 1) #12
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
Conversation
…merable<KeyValuePair> instead of IDictionary
bklimt
left a comment
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.
LGTM, but with some minor feedback.
| RestrictedPackageName = "com.google.firebase.testing", | ||
| }, | ||
| }; | ||
| var id = await FirebaseMessaging.DefaultInstance.SendAsync(message, dryRun: true); |
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 don't suppose there's any way to see that the message actually got all the parts it was supposed to?
It just seems kind of odd for an "integration test" to do a dryRun anyway.
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.
dryRun mode does validate for message content (both locally and at the server). The operation will fail if the message is invalid.
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.
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.
We do check the message content in our unit tests.
FirebaseAdmin/FirebaseAdmin.Tests/Messaging/FirebaseMessageClientTest.cs
Outdated
Show resolved
Hide resolved
FirebaseAdmin/FirebaseAdmin.Tests/Messaging/FirebaseMessageClientTest.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private void AssertJsonEquals(JObject expected, Message actual) | ||
| { | ||
| var json = NewtonsoftJsonSerializer.Instance.Serialize(actual.Validate()); |
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.
You could use JsonConvert here as well.
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.
This is actually needed for serialization (ok for deserialization). By default JsonConvert serializes null/zero values, which we don't want in the output.
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.
You can use this btw:
string json = JsonConvert.SerializeObject(message, Formatting.None,
new JsonSerializerSettings { NullValueHandling = NullValueHandling.Ignore})```
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.
Thanks @DenSmoke. NewtonsoftJsonSerializer from Google API client basically handles this. This prevents us from having to write the settings boilerplate in our code.
FirebaseAdmin/FirebaseAdmin/Messaging/FirebaseMessagingClient.cs
Outdated
Show resolved
Hide resolved
bklimt
left a comment
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.
Still LGTM
Implemented FCM Webpush API
Implemented APNs Support in the API
go/dotnet-fcm
Implements the following parts of the API:
Rest of the API will be fleshed out in subsequent PRs.