feat: add fcm_options property to webpush config by Odonno · Pull Request #64 · firebase/firebase-admin-dotnet

Odonno · 2019-06-03T07:58:24Z

No description provided.

googlebot · 2019-06-03T07:58:27Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Odonno · 2019-06-03T08:00:24Z

I signed it!

googlebot · 2019-06-03T08:00:28Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

hiranya911

Looks good. Please update the tests, and add an entry to the CHANGELOG file.

hiranya911 · 2019-06-03T17:57:12Z

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushFcmOptions.cs

@@ -0,0 +1,32 @@
+// Copyright 2018, Google Inc. All rights reserved.


hiranya911 · 2019-06-03T17:57:28Z

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushFcmOptions.cs

+        [JsonProperty("link")]
+        public string Link { get; set; }
+    }
+}


Add newline at eof

hiranya911

Thanks @Odonno. Just a couple of updates needed to the validation logic, and rename the property to FcmOptions.

hiranya911 · 2019-06-03T20:26:13Z

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushFcmOptions.cs

+        /// </summary>
+        internal WebpushFcmOptions CopyAndValidate()
+        {
+            if (this.Link != null && !this.Link.StartsWith("https"))


Create the copy first and then run validation on that.

var copy = ...; if (copy.Link != null && !copy.Link.StartsWuth("https://") { }

hiranya911 · 2019-06-03T20:27:51Z

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushFcmOptions.cs

+        /// </summary>
+        internal WebpushFcmOptions CopyAndValidate()
+        {
+            if (this.Link != null && !this.Link.StartsWith("https"))


Can we also check the well-formedness of the URL by running Uri.IsWellFormedUriString(copy.Link, UriKind.Absolute)?

hiranya911 · 2019-06-03T20:29:16Z

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushConfig.cs

+        /// Gets or sets the Webpush options that will be included in the message.
+        /// </summary>
+        [JsonProperty("fcm_options")]
+        public WebpushFcmOptions Options { get; set; }


I missed this in my previous review. Lets rename this property to FcmOptions to be consistent with other languages (namely Node.js and Go).

Odonno · 2019-06-03T20:54:16Z

@hiranya911 Thank you for your help on this.

hiranya911

Found a small bug in the new validation logic. I can merge once that's fixed.

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushFcmOptions.cs

…-admin-dotnet into feat/fcm_options

hiranya911

LGTM 👍

feat: add fcm_options property to webpush config

fb648a1

fix: fix xml documentation

4999df3

hiranya911 suggested changes Jun 3, 2019

View reviewed changes

Odonno added 3 commits June 3, 2019 20:10

fix

4428d33

docs: update changelog

fefb0da

tests: add tests and implement CopyAndValidate method

8c4aaa8

hiranya911 suggested changes Jun 3, 2019

View reviewed changes

hiranya911 self-assigned this Jun 3, 2019

Odonno and others added 2 commits June 3, 2019 22:51

refactor: rename FcmOptions property and verify well formed uri

90e57fa

Merge branch 'master' into feat/fcm_options

481c60e

hiranya911 suggested changes Jun 3, 2019

View reviewed changes

FirebaseAdmin/FirebaseAdmin/Messaging/WebpushFcmOptions.cs Outdated Show resolved Hide resolved

Odonno added 2 commits June 3, 2019 23:32

fix: correctly check error on link

3650f93

Merge branch 'feat/fcm_options' of https://github.com/Odonno/firebase…

6cfdad8

…-admin-dotnet into feat/fcm_options

hiranya911 approved these changes Jun 3, 2019

View reviewed changes

hiranya911 merged commit b54e7ea into firebase:master Jun 3, 2019

Odonno deleted the feat/fcm_options branch June 4, 2019 06:52

		@@ -0,0 +1,32 @@
		// Copyright 2018, Google Inc. All rights reserved.

Conversation

Odonno commented Jun 3, 2019

Uh oh!

googlebot commented Jun 3, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Odonno commented Jun 3, 2019

Uh oh!

googlebot commented Jun 3, 2019

Uh oh!

hiranya911 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

hiranya911 Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

Odonno commented Jun 3, 2019

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hiranya911 left a comment •

edited

Loading