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

Jimin/dev8 #1179

Merged
merged 12 commits into from
Aug 25, 2022
Merged

Jimin/dev8 #1179

merged 12 commits into from
Aug 25, 2022

Conversation

jiminwen-msft
Copy link
Member

  1. Modify the error message thrown for users not making push notification configurations.
  2. Deprecate existing startPushNotification method with error handler. And create new methods without error handlers. The error handling logic will get executed in the RegistrationRenewalWorker class

@jiminwen-msft jiminwen-msft requested review from JianpingChen and a team as code owners August 23, 2022 17:28
@ghost ghost added the Communication label Aug 23, 2022
Copy link
Member

@vcolin7 vcolin7 left a comment

Choose a reason for hiding this comment

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

Things look good to me from the technical standpoint. I would wait to hear back from Jonathan to go ahead with this though.

@@ -225,11 +225,26 @@ public void stopRealtimeNotifications() {
* @param deviceRegistrationToken Device registration token obtained from the FCM SDK.
* @param errorHandler error handler callback for registration failures
* @throws RuntimeException if push notifications failed to start.
* @deprecated The function will be replaced by startPushNotifications(String deviceRegistrationToken)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about using JavaDoc linking.

jiminwen-msft and others added 3 commits August 23, 2022 16:01
…zure/android/communication/chat/implementation/notifications/fcm/RegistrationRenewalWorker.java

Co-authored-by: vcolin7 <vicolina@microsoft.com>
…zure/android/communication/chat/implementation/notifications/fcm/RegistrationRenewalWorker.java

Co-authored-by: vcolin7 <vicolina@microsoft.com>
Copy link
Member

@vcolin7 vcolin7 left a comment

Choose a reason for hiding this comment

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

Let's just make sure to get the JavaDoc changes in for the public clients.

@jiminwen-msft jiminwen-msft merged commit e4e5391 into main Aug 25, 2022
@jiminwen-msft jiminwen-msft deleted the jimin/dev8 branch August 25, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants