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

[Feat] Application service focus update, cold start creates new session and drives user refresh #2113

Merged
merged 10 commits into from
Jun 18, 2024
Merged

[Feat] Application service focus update, cold start creates new session and drives user refresh #2113

merged 10 commits into from
Jun 18, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Jun 6, 2024 •
edited
Loading

Description

One Line Summary

(1) All cold starts create new sessions, (2) refresh user on all new sessions, (3) Application Service fills in gaps where it did not fire onFocus for awaiting subscribers.

Details

❓ Is my interpretation of this commit ApplicationService: Clarify a condition by naming it accurate?

❓ Now that "refresh-user" and "track-session-start" always enqueue at the same time, the UserRefreshService could be removed and its task could move to the SessionListener.

Application Service will fire onFocus even when it is too late

  • Typically, the app foregrounding will trigger the ApplicationService to fire onFocus to its subscribers.
  • However, it is possible for OneSignal to initialize too late, so the ApplicationService itself is started too late to capture the application's early lifecycle events.
  • In this case, the app is already foregrounded, so fire a subscriber's onFocus callback immediately upon it subscribing, as many services have logic to run in the first onFocus call of a cold start, and pass a parameter so the subscriber knows if the callback is fired from subscribing or from natural application lifecycle callbacks. Some subscribers such as permission controllers do not want the callback when it is fired from just subscribing.
  • This may fix some other bugs commonly seen on wrappers such as permission not being detected and updated between cold starts, or background runnables not being cancelled.

Session service creates new session on cold start

  • Also fix a bug where a cold start may not detect the start of a session correctly. On wrappers, it is common for OneSignal to initialize too late for the ApplicationService to start() and capture the Android onActivityStarted or onActivityResumed lifecycle callbacks, which was the primary way of session detection.
  • Player model reference: In player model, we addressed this by triggering the FocusTimeController in the init(context) method when the ActivityLifecycleHandler is added AND the application is already in the foreground: link.

Session service updates tracking active duration with cold starts

  • Additionally, the background task to send the session time will be triggered by the existence of an active duration of 1 second or more and under 1 day, instead of whether the session is valid. This is in line with player model logic.

Refresh user on new sessions (not just cold start)

  • New sessions can happen when the app is backgrounded over 30 seconds. Let's refresh the user then, not just when the app is cold started.
  • The user will still be refreshed only when the app is in the foreground.

Motivation

  • Align behavior with expectation
  • Update last active always when cold started
  • Fix some other bugs commonly found on wrappers due to OneSignal initializing later after activity already started

Scope

New session creation and when users are refreshed

  • Users are now refreshed on every new session triggered by backgrounding the app, whereas they were previously only refreshed on a cold start
  • All cold starts will create new sessions, instead of also relying on 30 seconds backgrounded time

Application Service will always fire onFocus when services are started up

  • Previously the SDK had to be initialized in the Applications onCreate call to capture and fire the early onFocus callback
  • Now if the SDK is initialized later, the Application Service will still fire that callback when handlers subscribe to it

Testing

Unit testing

Update Session Service and Application Service Tests

  • Extract helper methods them into a helper class for reuse
  • SessionServiceTests - Add test "session created in start when application is in the foreground"
  • ApplicationServiceTests - Add test "focus will occur on subscribe when activity is already started"

Manual testing

Tested these primary scenarios

  1. SDK initialized in Application onCreate
  2. SDK initialized after activity is already running
  3. Background app for under 30 seconds and over 30 seconds
  4. Swipe away app and re-open in under 30 seconds and over 30 seconds

Checked for:

  • Last active is updated (track-session-start operation)
  • Refresh user (refresh-user operation )
  • Session time accumulated correctly still
  • Fetch IAMs correct

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 changed the title [Feat] Cold start create new session and drives user refresh [Feat] Cold start creates new session and drives user refresh Jun 6, 2024
@nan-li nan-li mentioned this pull request Jun 6, 2024
Closed
23 tasks
@nan-li nan-li force-pushed the fix/cold_start_new_session branch 6 times, most recently from 5d9abc5 to 941e4a0 Compare June 7, 2024 18:18
@nan-li
ApplicationService: Clarify a condition by naming it
58df743 
* There are multiple checks for the conditional `(!isInForeground || nextResumeIsFirstActivity)` used in the Application Service and it is not obvious what this compound condition denotes
@nan-li
Application Service will fire onFocus even when too late
5ff9c76 
* Typically, the app foregrounding will trigger the ApplicationService to fire `onFocus` to its subscribers.
* However, it is possible for OneSignal to initialize too late, so the ApplicationService itself is started too late to capture the application's early lifecycle events.
* In this case, the app is already foregrounded, so fire a subscriber's `onFocus` callback immediately upon it subscribing, as many services have logic to run in the first onFocus call of a cold start.
@nan-li
session model defaults to invalid
b08684d 
* The getter for the session model's validity property will default to `false` to drive the creation of a new session. This is commonly read on new installs.
@nan-li
session model's focused time defaults to now, not 0
2910223 
* focusTime is a Unix timestamp, default it to now instead of 0
@nan-li
Session service creates new session on cold start
761b4e8 
* The SessionService was previously relying on the app backgrounding for 30 seconds to create a new session. It was doing this with the completion of a delayed background runnable.
* Now it will also create a new session on all cold starts.
* Also fix a bug where a cold start even over 30 seconds was previously not detecting the start of a session correctly. On wrappers, it is common for OneSignal to initialize too late to capture the Android lifecycle callbacks, which was the primary way of session detection. Now, there is enhanced logic to detect this in the onFocus callback and trigger subscribers correctly.

Reset session model on cold starts
- 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.
@nan-li
Session service change when it resets active duration
974df87 
- 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 when the session time is sent to the server. This is because cold starts may have stopped the old session time from sending, so we should not overwrite by resetting to 0 in the start method.
- Remove the isValid check in the backgroundRun method. This is because a force killed app will initialize OneSignal to run this background task. By initializing OneSignal, the SDK believes this could be 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, and that check would return early.
- Additionally, the only listener of the onSessionEnded callback is the SessionListener. It will return early when session time is invalid. We have a similar guard in player model.
@nan-li
Refresh user on new sessions (not just cold start)
6c6c0b5 
* New sessions can happen when the app is backgrounded over 30 seconds. Let's refresh the user then, not just when the app is cold started.
* When UserRefreshService starts up, it subscribes to the SessionService who will notify it when a new session is started either from backgrounding or from a cold start.
* The user will still be refreshed only when the app is in the foreground.
@nan-li
IAM manager doesn't need to check for onFocus in addition to onSessio…
69965cb 
…nStarted

* The onSessionStarted callback already will fetch messages.
* Now that session starting (including cold starts) should always trigger the subscribers, there is no need to also fetch messages within onFocus
@nan-li
[tests] Update Session and Application Service Tests
5aa61f7 
* Update existing tests
* Extract helper methods them into a helper class for reuse
* SessionServiceTests - Add test "session created in start when application is in the foreground"
* ApplicationServiceTests - Add test "focus will occur on subscribe when activity is already started"
@nan-li nan-li force-pushed the fix/cold_start_new_session branch from 941e4a0 to 5aa61f7 Compare June 10, 2024 22:12
@nan-li nan-li changed the title [Feat] Cold start creates new session and drives user refresh [Feat] Application service focus update, cold start creates new session and drives user refresh Jun 11, 2024
@nan-li
address review comments
9b582e0 
* Still send likely erroneous session time. This way the server can be aware of any issues.
* Simplify state when firing subscribers of the Application Service
@nan-li nan-li force-pushed the fix/cold_start_new_session branch from 32fec1b to 9b582e0 Compare June 12, 2024 21:54
@nan-li nan-li merged commit f5bf424 into main Jun 18, 2024
2 checks passed
@nan-li nan-li deleted the fix/cold_start_new_session branch June 18, 2024 17:27
This was referenced Jun 18, 2024
Merged
Merged
This was referenced Jun 21, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkasten2 jkasten2 jkasten2 approved these changes

@emawby emawby Awaiting requested review from emawby

@jinliu9508 jinliu9508 Awaiting requested review from jinliu9508

@shepherd-l shepherd-l Awaiting requested review from shepherd-l

@jennantilla jennantilla Awaiting requested review from jennantilla

Assignees

@jkasten2 jkasten2

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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