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.

Sign up for GitHub

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

Jump to bottom

V4 to v5 upgrade will migrate app ID for wrapper SDKs #2244

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

V4 to v5 upgrade will migrate app ID for wrapper SDKs #2244

wants to merge 1 commit into from

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jan 14, 2025 •
edited by niels-s
Loading

Description

One Line Summary

(Resolves an issue that only affects wrappers) When migrating from v4 to v5, check for a cached app ID from v4 if none are available on v5.

Details

Motivation

  • In wrappers, after an app upgrades from player to user model... if the app is not opened yet and a notification is received, there was no app ID detected and initWithContext returns early.
  • Now, check for the cached app ID from v4 and use that to continue the init process. This mimics what should happen when using the native SDK directly.
  • The reason this is an issue on wrappers is because when an app upgrades, the Application onCreate is automatically called, without needing to open the app. In native SDK clients, their OneSignal initialization code will run here and populate the app ID, setup the user, etc. However, in wrappers, clients don't modify the native code so the onCreate does not set app ID.

Scope

  • Using cached app ID from v4 if the app ID has not been set yet after migration
  • Affects wrappers
  • I aimed to mimic the same behavior as an upgrade using the native SDK, what would happen before the application is opened.

Testing

Unit testing

Manual testing

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 added the WIP Work In Progress label Jan 14, 2025
@nan-li nan-li changed the title V4 to v5 upgrade will migrate app ID [tests are wip] V4 to v5 upgrade will migrate app ID Jan 14, 2025
@nan-li nan-li force-pushed the v4_to_v5_upgrade_migrate_app_id branch 3 times, most recently from b3d9c40 to 5287fef Compare January 30, 2025 21:56
@nan-li nan-li changed the title [tests are wip] V4 to v5 upgrade will migrate app ID V4 to v5 upgrade will migrate app ID for wrapper SDKs Jan 30, 2025
@nan-li nan-li requested review from jkasten2 and jinliu9508 January 30, 2025 22:27
jinliu9508
jinliu9508 approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@jinliu9508 jinliu9508 left a comment

Choose a reason for hiding this comment

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

LGTM!

Also verified that Player Model is using the same key to store a legacy app ID.

@nan-li
Migrating from v4 to v5 checks for cached app ID
f572b8d 
* In wrappers, after an app upgrades from player to user model, if the app is not opened yet and a notification is received, there was no app ID detected and init returns early.
* Now, check for the cached app ID from v4 and use that to continue the init process. This mimics what should happen when using the native SDK directly.
* The reason this is an issue on wrappers is because when an app upgrades, the Application `onCreate` is called, without needing to open the app. However, in wrappers, the `onCreate` does not set app ID.
@nan-li nan-li force-pushed the v4_to_v5_upgrade_migrate_app_id branch from 5287fef to f572b8d Compare January 31, 2025 01:45
@nan-li nan-li changed the base branch from main to ci_upgrade_actions_deprecation January 31, 2025 01:45
@nan-li
Copy link
Contributor Author

nan-li commented Jan 31, 2025 •
edited
Loading

Just rebased the branch, no changes.

Looks like a maybe-flaky test added recently is failing the unit tests: https://github.com/OneSignal/OneSignal-Android-SDK/actions/runs/13064783338/job/36455552170?pr=2244

com.onesignal.core.internal.operations.OperationRepoTests > ensure loading in the background thread does not block enqueue FAILED
    io.kotest.assertions.AssertionFailedError: expected:<TERMINATED> but was:<BLOCKED>
        at app//com.onesignal.core.internal.operations.OperationRepoTests$1$2.invokeSuspend(OperationRepoTests.kt:135)
        at app//com.onesignal.core.internal.operations.OperationRepoTests$1$2.invoke(OperationRepoTests.kt)
        at app//com.onesignal.core.internal.operations.OperationRepoTests$1$2.invoke(OperationRepoTests.kt)

Base automatically changed from ci_upgrade_actions_deprecation to main January 31, 2025 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinliu9508 jinliu9508 jinliu9508 approved these changes

@jkasten2 jkasten2 jkasten2 approved these changes

Assignees
No one assigned
Labels
WIP Work In Progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nan-li @jkasten2 @jinliu9508