-
Notifications
You must be signed in to change notification settings - Fork 264
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
Conversation
0d03a50
to
b0a9e37
Compare
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`.
b0a9e37
to
d3bae81
Compare
* We are removing the previous user state from the public user state changed state. We will consider adding this in the future.
Ready for review, with |
0bdb39c
to
d9792fe
Compare
* 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.
d9792fe
to
06ab04a
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 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)
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 7 of 12 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jennantilla, @jinliu9508, @jkasten2, and @shepherd-l)
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 existingaddObserver
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
Login Anon → Existing User
Login Anon → New Nonexistent User
No change in
onesignalId
Login Identified → Identified
Call logout
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:
Upgrading from SDK 3.x.x to 5.x.x fires
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
Login Anon → Existing User
Login Anon → New Nonexistent User
No change in
onesignalId
Login Identified → Identified
Call logout
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:
Upgrading from SDK 3.x.x to 5.x.x fires
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is