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

Implementation of InstanceIdClient to allow subscribe/unsubscribe to topic #94

Merged
merged 14 commits into from Jul 31, 2019

Conversation

@Leo-Mepham
Copy link

commented Jul 25, 2019

As per #93 (referencing #83 )
Contributor License Agreement signed
Unit Tests & Integration Tests passing.

This is a conversion of the Java file at https://github.com/firebase/firebase-admin-java/blob/master/src/main/java/com/google/firebase/messaging/InstanceIdClientImpl.java and incorporating that into the main FirebaseMessaging class.

Look forward to the review :)

Leo added some commits Jul 24, 2019

Leo
Leo
Leo
Leo
@googlebot

This comment has been minimized.

Copy link

commented Jul 25, 2019

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@Leo-Mepham

This comment has been minimized.

Copy link
Author

commented Jul 25, 2019

Have done another CLA - Forgot my git config email address is different to my github email address.

@googlebot

This comment has been minimized.

Copy link

commented Jul 25, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@hiranya911
Copy link
Member

left a comment

Thanks @Leo-Mepham for the pull request. It looks pretty good. I've pointed out a bunch of minor improvements and fixes that can be made.

Leo
@Leo-Mepham

This comment has been minimized.

Copy link
Author

commented Jul 27, 2019

Thanks for the review @hiranya911 a lot of good suggestions, I'm grateful for that and have implemented those. Let me know what you think about the new changes?

I also wanted to ask - I've not yet built and run this against a real application (IE: Subscribing my personal phone to a topic using this code, then sending a message to the topic and receiving it) - Is that something I should do, or maintainers do, or is there another practice commonly followed?

Leo
@hiranya911
Copy link
Member

left a comment

Thanks for making the changes @Leo-Mepham. I've pointed out a few more things that should be addressed. But overall this looks quite good.

As for testing, if we have good unit and integration test coverage, we can go ahead and get this released. That's what we have done in the other SDK implementations for this API.

Leo added some commits Jul 31, 2019

Leo
Leo
Leo
@Leo-Mepham

This comment has been minimized.

Copy link
Author

commented Jul 31, 2019

Hi @hiranya911 , I've made the changes you suggested, thanks again for your feedback.

Making the InstanceIdServiceResponse class a private class of InstanceIdClient means I can't provide it to the constructor of TopicManagementResponse (Unless I've missed a trick here?) so I have passed a list of all the messages from the InstanceIdServiceResponse instead. Let me know if you think there is a better approach. I had wanted to convert the InstanceIdServiceResponse to a List of ErrorInfo inside the InstanceIdClient and pass that to TopicManagementResponse constructor - but then although I can provide the index of the error back to the library caller, I can't provide the success count without adding another parameter, which seems a little "off" - It may be a cleaner approach though - Let me know what you think.

I've added in HTTP status code error tests as requested, they all throw FirebaseMessagingException which looks to come from the FirebaseAdmin.Messaging.HttpErrorHandler so should give something useful to the caller.

Looking forward to the feedback again, let me know what you think :)

Leo
@hiranya911
Copy link
Member

left a comment

Thanks @Leo-Mepham. I see why InstanceIdServiceResponse needed to be not private. Sorry for not noticing it earlier. Lets make it an internal class, and change TopicManagementResponse to accept InstanceIdServiceResponse like you had earlier. I've also added some comments on how the new exception test cases can be improved.

Leo added some commits Jul 31, 2019

Leo
Leo
@hiranya911
Copy link
Member

left a comment

LGTM with a few comments/suggestions. Please address them, and then I can merge.

Leo
@Leo-Mepham

This comment has been minimized.

Copy link
Author

commented Jul 31, 2019

Thanks again for all you time, help, and patience here @hiranya911

I have a couple of questions if that's OK?

What sort of time does it usually take for this to end up in the nuget package?

Once this goes in, can I call myself a "Google code contributor" or "Google contributor" or is there another term for people who add to this project :)

@hiranya911 hiranya911 self-assigned this Jul 31, 2019

@hiranya911

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

Thanks again for the pull request @Leo-Mepham. I'll try to get this included in this week's release. If not definitely next week.

I don't think we have an official term for the developers who contribute to our open source projects :) @samtstern do you have any thoughts on that?

In any case, please note that we are really grateful for your contribution, and we will make a note of that in the release notes when this goes out. I look forward to receiving more contributions from you in the future :)

@hiranya911 hiranya911 merged commit 78714da into firebase:master Jul 31, 2019

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hiranya911

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

@Leo-Mepham I'm told it's ok to refer to developers like you as "Google contributors". But please be sure to not represent yourself as a Google employee :)

@Leo-Mepham

This comment has been minimized.

Copy link
Author

commented Jul 31, 2019

Great stuff, thanks @hiranya911 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.