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

Add getters and observer for onesignal ID and external ID #1344

Merged
merged 8 commits into from Dec 15, 2023

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Nov 28, 2023

Description

One Line Summary

Add getters for onesignal ID and external ID, and a user state observer 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.

Additionally, we are also adding a user state observer to know when these values are changed. App developers want to know the onesignal ID for a user, but it can be null if we have not received the response from the server after a fresh install or after logging in. The user state observer will fire when the response is received and fire with the onesignal ID and external ID.

Scope

1. Moved push subscription namespace implementation to another class
The User Manager is itself the user namespace AND the push subscription namespace via implementing both protocols. The problem is that we will be adding an addObserver method to the User namespace and this will conflict with the existing addObserver method that exists on the Push Subscription namespace.
Solution:
Use a separate class OSPushSubscriptionImpl that will implement the push subscription namespace. And an instance of this class will live on the User Manager.

2. Implementation details of the user state observer:
👉🏼 The implementation is all in this single commit, for easier review. Half of this PR is moving the push subscription namespace.

On Identity Model hydration from the server (when onesignal_id is received), we will snapshot the onesignal_id and external_id as the conclusive IDs for the current user. On future hydrations after logging in, we will compare to the previous snapshot to know if the observer should be fired.

We must not only compare onesignal_id but also external_id for the case of logging in Anonymous -> New Identified. In this case, the onesignal_id will not change from the previous snapshot / hydration but the external_id will be attached to the user on the server.

See Manual Testing Scenarios below >

Testing

Unit testing

None

Manual testing

iPhone 13 iOS 17.1
Fires once in each scenario

New App Install

{
    current =     {
        externalId = "";
        onesignalId = "b6ba65b09-cde7-4a1a4-badc3-4bf4f9281b23";
    };
}

Login Anon → Existing User

{
    current =     {
        externalId = nan01;
        onesignalId = "f26404e2c-8f9b-436e-8438-e6716e7dfc677";
    };
}

Login Anon → New Nonexistent User
No change in onesignalId

{
    current =     {
        externalId = nan06;
        onesignalId = "da46e6451-e11c-48fa-b500-9c61206c5c11";
    };
}

Login Identified → Identified

{
    current =     {
        externalId = nan02;
        onesignalId = "a7a17c6de-7ac3-4969-9954-5d4ba1ce515c";
    };
}

Call logout

{
    current =     {
        externalId = "";
        onesignalId = "e46d64531-e11c-48fa-b500-9c61206c5c11";
    };
}

If we receive a 404
If a Fetch User ever returns 404, indicating this user has been deleted, the SDK responds with creating an anonymous user. In this case, the observer also fires:

{
    current =     {
        externalId = "";
        onesignalId = "8e5d3bba3-8583-438a3-9bfb-9c1bfd4452c3";
    };
}

Upgrading from SDK 3.x.x to 5.x.x fires

{
    current =     {
        externalId = "";
        onesignalId = "8dbd19d83-238dd-2be30-3bf0f-f95734252e937";
    };
}

Future Work

Manual testing with a previous property (Omitted in this PR)
iPhone 13 iOS 17.1

This was testing with a previous property, which we are omitting for now. Keeping these test scenarios here for the future.

Click Me to See

New App Install

{
    current =     {
        externalId = "";
        onesignalId = "b6ba65b09-cde7-4a1a4-badc3-4bf4f9281b23";
    };
    previous =     {
        externalId = "";
        onesignalId = "";
    };
}

Login Anon → Existing User

{
    current =     {
        externalId = nan01;
        onesignalId = "f26404e2c-8f9b-436e-8438-e6716e7dfc677";
    };
    previous =     {
        externalId = "";
        onesignalId = "469de8cd6-24d1-4547-a744-695ed99c31f17";
    };
}

Login Anon → New Nonexistent User
No change in onesignalId

{
    current =     {
        externalId = nan06;
        onesignalId = "da46e6451-e11c-48fa-b500-9c61206c5c11";
    };
    previous =     {
        externalId = "";
        onesignalId = "da46e6451-e11c-48fa-b500-9c61206c5c11";
    };
}

Login Identified → Identified

{
    current =     {
        externalId = nan02;
        onesignalId = "a7a17c6de-7ac3-4969-9954-5d4ba1ce515c";
    };
    previous =     {
        externalId = nan01;
        onesignalId = "f26404d2c-8e9b-4368-8438-e67167dfc677";
    };
}

Call logout

{
    current =     {
        externalId = "";
        onesignalId = "e46d64531-e11c-48fa-b500-9c61206c5c11";
    };
    previous =     {
        externalId = nan01;
        onesignalId = "f2440432c-8f9b-4368-8438-e67167dfc677";
    };
}

If we receive a 404
If a Fetch User ever returns 404, indicating this user has been deleted, the SDK responds with creating an anonymous user. In this case, the observer also fires:

{
    current =     {
        externalId = "";
        onesignalId = "8e5d3bba3-8583-438a3-9bfb-9c1bfd4452c3";
    };
    previous =     {
        externalId = nan02;
        onesignalId = "a5ad17c6e-7ac3-43969-9954-5d4ba1ce515c";
    };
}

Upgrading from SDK 3.x.x to 5.x.x fires

{
    current =     {
        externalId = "";
        onesignalId = "8dbd19d83-238dd-2be30-3bf0f-f95734252e937";
    };
    previous =     {
        externalId = "";
        onesignalId = "";
    };
}

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
  • 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 Reviewable

@nan-li nan-li requested review from emawby, jkasten2, jinliu9508, shepherd-l and jennantilla and removed request for emawby November 30, 2023 20:13
@nan-li nan-li force-pushed the add_get_onesignal_id branch 3 times, most recently from 0d03a50 to b0a9e37 Compare December 5, 2023 21:14
Background:
The User Manager is itself the user namespace AND the push subscription namespace via implementing both protocols. The problem is that we will be adding an `addObserver` method to the User namespace and this will conflict with the existing `addObserver` method that exists on the Push Subscription namespace.

Solution:
Use a separate class `OSPushSubscriptionImpl` that will implement the push subscription namespace. And an instance of this class will live on the User Manager.
Also update existing code to go through pushSubscriptionImpl.
Problem:
App developers want to know the onesignal ID for a user, but it can be null if we have not received the response from the server after a fresh install or after logging in.

Solution:
Offer a user state observer that will fire when the response is received and fire with the onesignal ID and external ID.

Implementation Details:
On Identity Model hydration from the server (when onesignal_id is received), we will snapshot the onesignal_id and external_id as the conclusive IDs for the current user. On future hydrations after logging in, we will compare to the previous snapshot to know if the observer should be fired.

We must not only compare onesignal_id but also external_id for the case of logging in Anonymous -> New Identified. In this case, the onesignal_id will not change but the external_id will be attached to the user on the server.
When giving a jsonRepresentation of OSUserState, choose empty string as the representation of a null value instead of the string literal "nil". The string "nil" can be interpreted as an existing value whereas the empty string is a better option as it can be checked for `isEmpty`.
* We are removing the previous user state from the public user state changed state. We will consider adding this in the future.
@nan-li
Copy link
Contributor Author

nan-li commented Dec 6, 2023

Ready for review, with previous removed in commit 813933c

* Add a callout on the usage of the user state observer.
* App developers should be checking the `externalId` when they retreive the `onesignalId`. This is to make sure they are grabbing the onesignalId for the user that they expect.
@emawby emawby self-assigned this Dec 12, 2023
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 5 of 12 files reviewed, all discussions resolved (waiting on @jennantilla, @jinliu9508, @jkasten2, and @shepherd-l)

@emawby emawby self-requested a review December 13, 2023 23:49
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 12 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jennantilla, @jinliu9508, @jkasten2, and @shepherd-l)

@nan-li nan-li merged commit 21adb8f into main Dec 15, 2023
4 of 5 checks passed
@nan-li nan-li deleted the add_get_onesignal_id branch December 15, 2023 20:23
@nan-li nan-li mentioned this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants