-
Notifications
You must be signed in to change notification settings - Fork 368
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
Add onNewToken with Bundle HMS method support #1433
Conversation
578a4ca
to
52c859c
Compare
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.
Unit test failing that looks related:
com.test.onesignal.PushRegistratorHMSIntegrationTestsRunner > EMUIPre10Device_shouldRegister FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:87)
at org.junit.Assert.assertTrue(Assert.java:42)
at org.junit.Assert.assertTrue(Assert.java:53)
at com.test.onesignal.RestClientAsserts.assertPlayerCreateSubscribedAtIndex(RestClientAsserts.java:91)
at com.test.onesignal.PushRegistratorHMSIntegrationTestsRunner.assertHuaweiSubscribe(PushRegistratorHMSIntegrationTestsRunner.java:83)
at com.test.onesignal.PushRegistratorHMSIntegrationTestsRunner.EMUIPre10Device_shouldRegister(PushRegistratorHMSIntegrationTestsRunner.java:133)
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @emawby and @nan-li)
da7bfa7
to
458afe0
Compare
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.
Code changes look good to me. @Jeasmine Could you include the minimum version of hms:push
now required? Since the HmsMessageService
class as a new signature for onNewToken
I don't believe the older versions of hms:push
will work.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nan-li)
*/ | ||
@Override | ||
public void onNewToken(String token) { | ||
OneSignalHmsEventBridge.onNewToken(this, token); | ||
public void onNewToken(String token, Bundle bundle) { |
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.
@Jeasmine What happens if the app is using an old hms:push
library in their app/build.gradle
, a version before this method signature was added? Would they get runtime error? Or would this method just not be called?
If the above is an issue, is there anything that could be done to handle it? Or do we just need to instruct developers to upgrade their hms.push
when they update OneSignal?
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.
@jkasten2 There won't be a problem because they will have implemented the
public void onNewToken(String token) that will be calling the
public static void onNewToken(@NonNull Context context, @NonNull String token) {
onNewToken(context, token, null);
}
The new method won't be called unless the user overrides it. But we should recommend updating
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.
The older hms:push
doesn't know about the new onNewToken(String, Bundle)
, only onNewToken(String)
. If the app includes our new OneSignal-Android-SDK (with this change) AND the app has the old hms:push
version 4 I believe HMS is going to try to call onNewToken(String)
on our HmsMessageServiceOneSignal
and it's not going to find it. It think it might crash, or OneSignal won't get a push token.
If the app developer has their own class that extends HmsMessageService
this won't be an issue, as HmsMessageServiceOneSignal
won't be called by HMS. We instead instruct the app developer to call OneSignalHmsEventBridge
from their class which I see you have kept both methods for backwards compatibility as expected.
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.
Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Jeasmine and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/HmsMessageServiceOneSignal.java, line 35 at r2 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
The older
hms:push
doesn't know about the newonNewToken(String, Bundle)
, onlyonNewToken(String)
. If the app includes our new OneSignal-Android-SDK (with this change) AND the app has the oldhms:push
version 4 I believe HMS is going to try to callonNewToken(String)
on ourHmsMessageServiceOneSignal
and it's not going to find it. It think it might crash, or OneSignal won't get a push token.If the app developer has their own class that extends
HmsMessageService
this won't be an issue, asHmsMessageServiceOneSignal
won't be called by HMS. We instead instruct the app developer to callOneSignalHmsEventBridge
from their class which I see you have kept both methods for backwards compatibility as expected.
I think having both signatures should allow the app to use hms:push
version 4 or 5 during the transition. Would be good to note exactly what was tested though to confirm this works.
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalHmsEventBridge.java, line 29 at r4 (raw file):
*/ public static void onNewToken(@NonNull Context context, @NonNull String token, @Nullable Bundle bundle) { if (firstToken.get()) {
This should be compareAndSet
to be thread safe. We can then remove firstToken.set(false);
below.
e1481a7
to
1cd8b04
Compare
* Add onNewToken method support to HmsMessageServiceOneSignal, HmsMessageServiceAppLevel and OneSignalHmsEventBridge
* Add OneSignalHmsEventBridge onNewToken comment
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.
Just one comment on a unneeded line. Feel free to remove the line add merge in.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jeasmine and @nan-li)
OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalHmsEventBridge.java, line 31 at r5 (raw file):
if (firstToken.compareAndSet(true, false)) { OneSignal.Log(OneSignal.LOG_LEVEL.INFO, "OneSignalHmsEventBridge onNewToken - HMS token: " + token + " Bundle: " + bundle); firstToken.set(false);
Remove this line, compareAndSet
is already doing this.
* Keep old onNewToken without bundle param
1cd8b04
to
c72be26
Compare
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.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nan-li)
Tested versions:
com.huawei.hms:push:5.0.0.300
com.huawei.hms:push:5.1.1.301
com.huawei.hms:push:5.3.0.304
com.huawei.hms:push:6.1.0.300
Flow tested:
Fresh app install
Check that onNewToken is called
Check that if both onNewToken methods are called, the registration only happens once
This change is