-
Notifications
You must be signed in to change notification settings - Fork 182
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
Rotate firebase token in case of error #3755
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
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 haven't tried it yet, but the code LGTM, just a couple of suggestions.
override suspend fun delete() { | ||
suspendCoroutine { continuation -> | ||
// 'app should always check the device for a compatible Google Play services APK before accessing Google Play services features' | ||
if (isPlayServiceAvailable.isAvailable()) { |
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.
Not a needed change, but an early return here with !isPlayServiceAvailable.isAvailable()
might be more readable. Also, for the exception, maybe a throw Exception(...).also(Timber::e)
might work?
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 will change. I am also wondering why we throw the exception here and we do not invoke continuation.resumeWithException(e)
🤔
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 class delete the Firebase token and generate a new one. | ||
*/ |
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 think the comment should be in the interface instead?
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.
Yes, thanks! 12f5839
|
Content
Add a quick fix for the troubleshoot notification test in case of PusherRejected error, which means that the token is unknown.
In this case, the quick fix will rotate the Firebase token.
Note: maybe later we will see if we can do the same thing for UnifiedPush provider.
Motivation and context
Closes #3752
Screenshots / GIFs
You can see in the video below that when clicking on QuickFix, the token is rotated.
FirebaseTokenRotation.mp4
Tests
Internally, this can be tested by patching the code and let
DefaultPushService.testPush()
throw aPushGatewayFailure.PusherRejected()
(this is what I have done for the video above).Tested devices
Checklist