Fix onSuccess callback invoking for setting email or SMS

[nan-li]

Description

One Line Summary

Don't call the onSuccess callback for setting email and SMS too early, which can be before those players have been successfully created and set their channels in the SDK.

Details

Motivation

These changes are investigated and made after a customer report needing to set external user ID after setting email and SMS, and wanting all the players to be updated with the EUID. Time has to pass after a call to setEmail or setSMSNumber, before setExternalUserId can successfully update all players.

To work around this, the customer was recommended to make a call to setExternalUserId within the onSuccess callback of setEmail or setSMSNumber. However, this did not work as intended either since at the time the callback is invoked, the email or SMS player may not have been created or had the channel ID saved to the SDK, and setting external user ID only makes the request for the push channel, as a result.

Implementation Details

Previously, UserStatePushSynchronizer. onSuccessfulSync() would fire the email and SMS successful callbacks if the request sent has "sms_number" or "email" in it, but this is too early to fire those since this is only for the push player sync. The /players endpoint is then called later for the SMS or email player. In the dashboard, the email or SMS player is created a few seconds after the push player.

Instead, let the UserStateEmailSynchronizer and UserStateSMSSynchronizer call these successful callbacks in their own onSuccessfulSync, which will happen after the requests for the email player or SMS player is successful.

Did not make any changes to the failure callback.

Scope

This change affects the invoking of the successful callbacks and doesn't affect other OneSignal logic around setting email or SMS.

Testing

Unit testing

Added two test handlers that call setExternalUserId in the callback:

• TestEmailUpdateHandler_onSuccess_callSetExternalUserId
• TestSMSUpdateHandler_onSuccess_callSetExternalUserId

Added two unit tests to reproduce the customer's reported behavior (these failed before the changes in this PR)

• shouldSetExternalUserId_inOnSuccessOfEmailUpdate
• shouldSetExternalUserId_inOnSuccessOfSMSUpdate

Manual testing

App build with Android Studio 2020.3 using the OneSignal example app on a Pixel 5 API 30:

I made multiple calls to combinations of setEmail and setSMS with a callback set. This callback makes a call to setExternalUserId -> and then checked the callback is invoked correctly every time, and that all the players are updated with the EUID.

• in MainApplication onCreate
• returning to the app after a while away
• setting these within the app with a button tap
• setting email or SMS to the same value (meaning no change)
• calling logout, and setting again to same email/SMS
• calling logout, and then setting to different email/SMS

Affected code checklist

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing
  PR
• Any Public API changes are explained in the PR details and conform to existing APIs

Testing

• I have included test coverage for these changes, or explained why they are not needed
• All automated tests pass, or I explained why that is not possible
• I have personally tested this on my device, or explained why that is not possible

Final pass

• Code is as readable as possible.
• I have reviewed this PR myself, ensuring it meets each checklist item

---

This change is 

[jkasten2]

One test fail, looks unrelated but rerunning tests to confirm:

com.test.onesignal.NotificationOpenedActivityHMSIntegrationTestsRunner > osIAMPreview_showsPreview FAILED
    java.lang.Exception: Main looper has queued unexecuted runnables. This might be the cause of the test failure. You might need a shadowOf(getMainLooper()).idle() call.
        at org.robolectric.android.internal.AndroidTestEnvironment.checkStateAfterTestFailure(AndroidTestEnvironment.java:502)
        at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:581)
        at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:263)
        at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:89)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at java.base/java.lang.Thread.run(Thread.java:834)
        Caused by:
        junit.framework.ComparisonFailure: expected:<PGh0bWw+PC9odG1sPgoKPHNjcmlwdD4KICAgIHNldFBsYXllclRhZ3MobnVsbCk7Cjwvc2NyaXB0Pg==> but was:<null>
            at junit.framework.Assert.assertEquals(Assert.java:85)
            at junit.framework.Assert.assertEquals(Assert.java:91)
            at com.test.onesignal.NotificationOpenedActivityHMSIntegrationTestsRunner.osIAMPreview_showsPreview(NotificationOpenedActivityHMSIntegrationTestsRunner.java:203)

[jkasten2]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @emawby, @Jeasmine, and @nan-li)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/UserStatePushSynchronizer.java, line 232 at r1 (raw file):

    @Override
    protected void onSuccessfulSync(JSONObject jsonFields) {
        if (jsonFields.has(EMAIL_KEY))

Love it when just deleting code fixes the issue 😄

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/UserStatePushSynchronizer.java, line 233 at r1 (raw file):

    protected void onSuccessfulSync(JSONObject jsonFields) {
        // Previously, the onSuccess callbacks for setEmail and setSMSNumber were triggered here if applicable.
        // However, this is too early to call these callbacks as those players may not be created or set yet in the SDK.

I would add a comment that UserStateSecondaryChannelSynchronizer already fires the successful callback somehwhere in your comment here.

---

OneSignalSDK/unittest/src/test/java/com/test/onesignal/SynchronizerIntegrationTests.java, line 2084 at r1 (raw file):

        // 5. Make sure lastExternalUserIdResponse has push and email with success : true
        // The order of the JSONObject is email first then push

The order of JSON keys isn't reliable way to test something, as the order isn't meaningful in JSON.

Instead of checking success we should be checking we have a correct value for something. This could be we sent the correct data in the REST API calls or the order of the callbacks fired correctly.

[nan-li]

Instead of checking success we should be checking we have a correct value for something. This could be we sent the correct data in the REST API calls or the order of the callbacks fired correctly.

I updated the unit tests to check for the external ID request being sent for email and SMS. Previously, the request was only being sent for the push player.

I opted for this instead of looking at the order of callbacks firing because that seems more involved to implement, as well as being able to actually pass without the changes in this PR (the order of callbacks fired wouldn't actually be affected).

[jkasten2]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @emawby and @Jeasmine)