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

Fix cold start to create a new session regardless of backgrounded time #1932

Open
wants to merge 278 commits into
base: main
Choose a base branch
from

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Dec 5, 2023

❌ TODO: Test with influences and upgrading influences
❌ TODO: Add tesitng

Basic testing shows these are fulfilled on cold start

  • Last active is updated
  • Refresh user data request is sent
  • Fetch user happens and hydrates
  • Session time accumulated correctly still
  • Fetch IAMs happen on cold start correctly (making multiple cold starts within a short time)

Description

One Line Summary

REQUIRED - Very short description that summaries the changes in this PR.

Details

Motivation

REQUIRED - Why is this code change being made? Or what is the goal of this PR? Examples: Fixes a specific bug, provides additional logging to debug future issues, feature to allow X.

Scope

RECOMMEND - OPTIONAL - What is intended to be effected. What is known not to change. Example: Notifications are grouped when parameter X is set, not enabled by default.

OPTIONAL - Other

OPTIONAL - Feel free to add any other sections or sub-sections that can explain your PR better.

Testing

Unit testing

OPTIONAL - Explain unit tests added, if not clear in the code.

Manual testing

RECOMMEND - OPTIONAL - Explain what scenarios were tested and the environment.
Example: Tested opening a notification while the app was foregrounded, app build with Android Studio 2020.3 with a fresh install of the OneSignal example app on a Pixel 6 with Android 12.

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 Reviewable

brismithers and others added 30 commits December 9, 2022 14:47
* Make `OneSignal` module a psuedo project which brings in all subprojects.
* Move `OneSignal` functionality down to new `core` subproject.
* All other subprojects now depend on `OneSignal:core` instead of `OneSignal`.
* Finalize new build/publishing scheme.
[User Model] Add name to each module for maven publish pom
* Remember device's push subscription ID in config and ensure that push subscription is returned in IUserManager.pushSubscription.
* Add TransferSubscriptionOperation
* Updates to support creating a subscription when the subscription already exists for the owner.
…dy-exists

[User Model] Subscription Already Exists
* Add app_version to SubscriptionObject
* Add migration guide
* Make OneSignal.User pascal-case
* Update ApplicationService to more accurately detect when the app goes in/out of focus
* Always switch to main thread for RequestPermission calls, to ensure that (1) a suspend occurs and (2) any UI operations happen on the main thread.
* Add ToJsonObject to NotificationClickResult, NotificationAction, and InAppMessageClickResult.
* Expose groupedNotifications in INotification
[User Model] Application Focus and Threading
jinliu9508 and others added 18 commits November 16, 2023 02:27
…o_create_user

Add refresh_device_metadata to create user so country / IP can be set
Subscription Manager contains a method previously called `addOrUpdatePushSubscription` that is meant to be used in relation to updating push tokens.

This method is renamed to `addOrUpdatePushSubscriptionToken` so as to not confuse with other updates on the push subscription such as detecting app version changes between app opens, which is a newly added feature
Subscription Model now has sdk, deviceOS, carrier, and appVersion as properties so that they can persist.
The getter is defaulted to "" (the empty string) as these properties do not exist prior to v5.0.5. They are only read when detecting changes so it is ok that they default to "" as the empty string will not be sent.
Additionally set these properties when a push subscription model is created:
- sdk, deviceOS, carrier, appVersion
Let the SubscriptionManager refresh the push subscription model on new sessions.
When hydrating an existing push subscription model, use existing local device-scoped information instead of remote information for:
- sdk
- deviceOS
- carrier
- appVersion
This information should always come from the local device. The reason for this change is that on a new session, we may detect a change to one of these properties and we do not want to overwrite it with old remote data from the get_user response.
Update push subscription model properties between sessions
[v5] Pause operation repo and retry failed user create
- Now that cold starts will trigger a new session, the logic for storing active duration is updated.
- Instead of active duration being reset to 0 at the start of every new session, it will reset to 0 after the session time is sent to the server. This is because cold starts may not have sent the old session time yet, so we should not reset to 0 here.
- Additionally, the logic to run the session service's background task, which is sending the session time, will be triggered by there being an active duration instead of whether the session is valid. This is because a force killed app will initialize OneSignal to run this background task. By initializing OneSignal, the SDK believes this is a cold start and set the isValid property on the session model to `false`. However, we want to send off this accumulated session time to the server.
- On cold starts, the app may not be in the background for over 30 seconds, which is when  the session time is sent and the session model's `isValid` property is set to false.
- Therefore, after the session model is read from cache, reset it's `isValid` property to `false` in order to drive the creation of a new session on cold starts.
Comment on lines +8 to +13
SimpleModelStore(
{ SessionModel() },
"session",
prefs,
),
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, this doesn't look as good as a one-liner. But linty lint was nitty picky.

@jinliu9508 jinliu9508 deleted the branch main January 31, 2024 21:21
@jinliu9508 jinliu9508 closed this Jan 31, 2024
@nan-li nan-li reopened this Jan 31, 2024
@jinliu9508 jinliu9508 force-pushed the user-model/main branch 3 times, most recently from 2072eac to d73bfc6 Compare February 6, 2024 16:38
Base automatically changed from user-model/main to main February 6, 2024 18:32
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

6 participants