Skip to content

feat(fcm): Added support for specifying analytics label in notifications - #89

Merged
hiranya911 merged 6 commits into
firebase:masterfrom
RannyRanny:support_for_analytics_label
Jul 23, 2019
Merged

feat(fcm): Added support for specifying analytics label in notifications#89
hiranya911 merged 6 commits into
firebase:masterfrom
RannyRanny:support_for_analytics_label

Conversation

@RannyRanny

Copy link
Copy Markdown
Contributor

Solves #88

@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@RannyRanny

Copy link
Copy Markdown
Contributor Author

I signed it!

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @RannyRanny for putting this together. We need a couple of new types in this PR and test coverage.

Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/AndroidConfig.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/ApnsConfig.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/FcmOptions.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/FcmOptions.cs Outdated
@RannyRanny

Copy link
Copy Markdown
Contributor Author

@hiranya911 Added new classes and coveder tests

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @RannyRanny. Looks pretty good. Just a few changes/improvements needed. Then we can merge.

Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/MessageTest.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/AndroidFcmOptions.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/ApnsFcmOptions.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/FcmOptions.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/Util/AnalyticsLabelChecker.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/Util/AnalyticsLabelChecker.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/Util/AnalyticsLabelChecker.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/Util/AnalyticsLabelChecker.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/MessageTest.cs
Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/MessageTest.cs
@RannyRanny

Copy link
Copy Markdown
Contributor Author

@hiranya911 Added requested changes

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, but it looks like this needs a few more updates. The main points are:

  1. Add the license header to all new source files.
  2. Make sure there's a newline at the end of each file.
  3. AnalyticsLabel is optional, and therefore should allow null.
Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/FcmOptionsTest.cs
Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/FcmOptionsTest.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/FcmOptionsTest.cs
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/ApnsFcmOptions.cs
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/FcmOptions.cs
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/AndroidFcmOptions.cs
Comment thread FirebaseAdmin/FirebaseAdmin/Messaging/Util/AnalyticsLabelChecker.cs Outdated
Comment thread FirebaseAdmin/FirebaseAdmin.Tests/Messaging/FcmOptionsTest.cs Outdated
@hiranya911

Copy link
Copy Markdown
Contributor

@egilmorez can you look at the documentation bits of this PR?

@hiranya911 hiranya911 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM :+1

@hiranya911 hiranya911 assigned hiranya911 and unassigned RannyRanny Jul 23, 2019
@hiranya911
hiranya911 merged commit 48659aa into firebase:master Jul 23, 2019
@RannyRanny

Copy link
Copy Markdown
Contributor Author

@hiranya911 This is my first PR for open source. Can U please say when this will be available to use in NuGet package?

@hiranya911

Copy link
Copy Markdown
Contributor

@RannyRanny most likely next week.

@hiranya911 hiranya911 changed the title Added support for analytics_label Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants