Add getter for onesignalId and UserStateObserver

[jinliu9508]

Description

Add getters for onesignal ID and external ID, and a UserStateObserver to listen on User State changes to know when these values are changed.

Details

Motivation

From developer feedback and to support integration partners, we are exposing the onesignal ID and external ID with getters.

We also want to grant developers the ability to add observers that can be called when there is a change in user state.

Scope

UserState contains onesignalId and externalId (both can be empty strings if not available or not set).

Testing

Unit testing

** need to figure out a good testing plan here

Manual testing

App built

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
  • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one 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.
  • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
• I have reviewed this PR myself, ensuring it meets each checklist item
  • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

---

This change is 

[brismithers]

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ISingletonModelStoreChangeHandler.kt line 15 at r8 (raw file):

     */
    fun onModelReplaced(
        model: ModelReplacedArgs<TModel>,

nit: rename model to args as it now represents more than the model.

[brismithers]

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt line 252 at r8 (raw file):

        tag: String,
    ) {
        val oldUserState = UserState(args.oldModel[IdentityConstants.ONESIGNAL_ID], args.oldModel.externalId)

just curious, couldn't you use args.oldModel.onesignalId here instead?

[brismithers]

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt line 271 at r8 (raw file):

        tag: String,
    ) {
        if (args.property.equals(IdentityConstants.ONESIGNAL_ID)) {

I think you also need to check and fire if the externalId has changed here as well?

[brismithers]

Reviewed 3 of 7 files at r2, 1 of 6 files at r6, 3 of 7 files at r7, 21 of 21 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @emawby, @jennantilla, @jinliu9508, @jkasten2, @nan-li, and @shepherd-l)

[jinliu9508]

Reviewable status: 12 of 29 files reviewed, 13 unresolved discussions (waiting on @brismithers, @emawby, @jennantilla, @jkasten2, @nan-li, and @shepherd-l)

---

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/model/MainActivityViewModel.java line 31 at r8 (raw file):

Previously, nan-li (Nan) wrote…

Since the rest of the dev app uses android.util.Log, let's use that instead of the SDK's internal logging system.

Done.

---

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/model/MainActivityViewModel.java line 70 at r8 (raw file):

Previously, nan-li (Nan) wrote…

Since the user state observer is not affecting anything in ActivityViewModel, I recommend to put into MainApplication. You can see some examples there already of listeners that are added like Notification Click Listener.

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/IUserManager.kt line 142 at r8 (raw file):

Previously, nan-li (Nan) wrote…

Getters for onesignal ID and external ID needs to be exposed through this interface for public consumption

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserState.kt line 9 at r8 (raw file):

Previously, nan-li (Nan) wrote…

Consider making onesignalId and externalId non-optional and default to "" to match push subscription state and look nice in toJSONObject() method.

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt line 252 at r8 (raw file):

Previously, brismithers (Brian Smith) wrote…

just curious, couldn't you use args.oldModel.onesignalId here instead?

onesignalId might not be exist in the model so we couldn't directly get onesignalId that way. We have decided to not carrying over the old model at this point so I have removed this part of code.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt line 271 at r8 (raw file):

Previously, brismithers (Brian Smith) wrote…

I think you also need to check and fire if the externalId has changed here as well?

Good question. In the team meeting we have gone through all possible scenarios when we need to fire up the callback, we found that change of externalId only is not enough to determine whether we should fire the callback. So only the change of onesignalId is under the watch here.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/events/EventProducer.kt line 47 at r8 (raw file):

Previously, nan-li (Nan) wrote…

to self: why made open? Come back and check at end of review. Is it because this is called from UserManager.kt?

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/IModelChangedHandler.kt line 55 at r8 (raw file):

Previously, nan-li (Nan) wrote…

this class should go in ISingletonModelStoreChangeHandler.kt and the comment updated to reflect that it is used in ISingletonModelStoreChangeHandler.

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/ISingletonModelStoreChangeHandler.kt line 15 at r8 (raw file):

Previously, nan-li (Nan) wrote…

args

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/SingletonModelStore.kt line 42 at r8 (raw file):

Previously, nan-li (Nan) wrote…

Hmm, this is a bit convoluted workaround and do prefer to check if there is even an old model

We just went through this and the old model is no longer stored.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/listeners/SingletonModelStoreListener.kt line 66 at r8 (raw file):

Previously, nan-li (Nan) wrote…

args

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/state/UserState.kt line 8 at r1 (raw file):

Previously, brismithers (Brian Smith) wrote…

The majority of the external API is defined through interfaces, even for simple POJO types like this (example), to solidify an understanding of what is intended as an external interface.

I think it's okay to define a class as part of the external interface, but keep in mind anything that goes in here could be interpreted to be part of the API.

Done.

---

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/state/UserState.kt line 11 at r1 (raw file):

Previously, brismithers (Brian Smith) wrote…

If it will be an empty string when there is no onesignalId, what does a null onesignalId represent?

The code has changed so now that it is no longer an optional field. When onesignalId is not available, it will be an empty string "" so that it is consistent and more friendly to JsonObject across wrappers.

[nan-li]

Reviewed 2 of 7 files at r7, 3 of 21 files at r8.
Reviewable status: 12 of 29 files reviewed, 13 unresolved discussions (waiting on @brismithers, @emawby, @jennantilla, @jkasten2, and @shepherd-l)

[nan-li]

We should call out somewhere in the KDoc that app developers should be checking both the onesignalId and the externalId in the UserState to make sure they are grabbing the correct onesignalId.

For example, a new app install that calls login("ext1") will trigger the callback twice. App developers may just grab the very first callback and save the onesignalId. However, that first callback will contain an "" externalId, and it is the second callback that contains the actual onesignalId they want and an externalId of ext1.

I'm not sure where best to put this information since it can go in the description of IUserStateObserver or fun onUserStateChange, UserChangedState, or UserState.

I need to do the equivalent for iOS as well.

[nan-li]

Not sure how important this is, a behavior I noticed during testing is if internet is turned off:

1. login("someNewUser") and add tags
2. call logout
3. Turn internet back on
4. The new user is created on server and tags are attached to that user correctly
5. The user state observer is not fired to notify the app developer of that user's onesignalId

App developers may always be waiting for onesignalId every time they login to send to their own server. If the user is already logged out or changed by the time the response is received in the SDK, the SDK really shouldn't receive anything for that user anyway. However, the app developer may have some integration they still want to attach the onesignalId to that user on their own server.

[nan-li]

We should update the migration guide? I forgot about it.