Skip to content

[5.0.0] Migrating SDK 3.x players#1243

Merged
nan-li merged 7 commits into
major_release_5.0.0from
5.0.0/migrating_v3_sdk_users
Apr 17, 2023
Merged

[5.0.0] Migrating SDK 3.x players#1243
nan-li merged 7 commits into
major_release_5.0.0from
5.0.0/migrating_v3_sdk_users

Conversation

@nan-li
Copy link
Copy Markdown
Contributor

@nan-li nan-li commented Apr 3, 2023

Description

One Line Summary

Migrates a cached 3.x player to a 5.x user with the player ID becoming the new push subscription ID.

Details

Motivation

Handles migrating someone using SDK 3.x to the new user model, who may not uninstall and reinstall the app.

Scope

Affects migrating old players. If there is a cached player ID, we will create a new user with that player ID as the new push subscription ID, and fetch this subscription ID's identity.

Error handling server responses will be part of another PR.

  1. Detect there is a player_id when the User Manager starts up
  2. Create a push subscription model using that ID
  3. Create a blank user in the SDK and make a fetch identity by subscription ID request
  4. Get the onesignal_id in the response and hydrate it locally
  5. Fetch this user to hydrate the local user (if it is the same user)
  • 🐛 also fixed a bug with firing the push sub observer in this commit: 5a5a648

Testing

Unit testing

None

Manual testing

Tested on iPhone 13 physical device with iOS 16.x.

  1. Install the app with SDK 3.12.4, and add a tag
  2. Check the player ID, onesignal ID, and push token in the dashboard for this player
  3. Now run the app with SDK 5.0.0-beta-02
  4. See that the IDs are the same as in (2) and no new player was created in the dashboard
  5. See the SDK user has the tag added in (1) above
  6. Add a tag to see it applied to this user/player in the dashboard

Tested on iPhone 13 physical device with iOS 16.x. Testing the request is cached

  1. Install the app with SDK 3.12.4
  2. Check the player ID, onesignal ID, and push token in the dashboard for this player
  3. Turn off wifi/data so we test caching.
  4. Now run the app with SDK 5.0.0-beta-02
  5. See the requests are not able to go through
  6. Turn back on wifi/data and run the app with SDK 5.0.0-beta-02
  7. See the cached requests go through
  8. See that the IDs are the same as in (2) and no new player was created in the dashboard
  9. Add a tag to see it applied to this user/player in the dashboard

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 added 7 commits April 3, 2023 16:17
- when someone calls `optOut`, fire the push sub observer with that change BEFORE setting `notification_types` (which will also fire the push sub observer but won't have any effect)
* Remove the call to `removeFromQueue(request)` here. It was too early and would remove the request even for a retry-able error.
* Not sure where it is stored so remove from both standard and shared user defaults
* Split `OSUD_PUSH_SUBSCRIPTION_ID` into `OSUD_LEGACY_PLAYER_ID` and `OSUD_PUSH_SUBSCRIPTION_ID` so we can track any legacy player IDs
* Add another path in the `start()` method of the User Manager that will detect legacy players and create a new user from that information.
* This is needed to migrate legacy players within the SDK
* Refactor and reuse `setNewInternalUser` and `createDefaultPushSubscription` in `createUserFromLegacyPlayer`

* Make a helper method `isCurrentUser` to wrap checking if a user is the current one.
Copy link
Copy Markdown
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 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @jennantilla, @jkasten2, @nan-li, and @shepherd-l)


iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift line 193 at r1 (raw file):

            }
            firePushSubscriptionChanged(.isDisabled(oldValue))
            notificationTypes = -2

What is the significance of this change?

@emawby emawby self-requested a review April 13, 2023 17:31
@nan-li
Copy link
Copy Markdown
Contributor Author

nan-li commented Apr 13, 2023

            }
            firePushSubscriptionChanged(.isDisabled(oldValue))
             notificationTypes = -2

What is the significance of this change?

@emawby This was to fix this issue: The app has notification permission already and optOut is called. The push subscription observer is not fired in this case.

What was happening was we update the notification_types first which update the reachable property, and when we went to figure out if there are any changes to fire the push subscription observer using the old and new reachable value, the previous state for comparison was wrong because we were not using the previous value of isDisabled (we used the new value of isDisabled as we already updated it). Multiple properties changed but we were only considering one.

So now, once the isDisabled changes, we detect if this results in a change.

This is confusing without drawing it out but hope its still somewhat clear.

@nan-li nan-li merged commit 37ad22a into major_release_5.0.0 Apr 17, 2023
@nan-li nan-li deleted the 5.0.0/migrating_v3_sdk_users branch April 17, 2023 18:22
@emawby emawby mentioned this pull request May 1, 2023
nan-li added a commit that referenced this pull request Oct 30, 2023
nan-li added a commit that referenced this pull request Oct 30, 2023
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.

2 participants