diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt index 70ccbc4cb2..dee1e09d20 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/background/IBackgroundService.kt @@ -4,9 +4,9 @@ import androidx.annotation.WorkerThread /** * Implement and provide this interface as part of service registration to indicate the service - * wants to be instantiated and its [backgroundRun] function called when the app is in the background. The - * background process is initiated when the application is no longer in focus. Each background - * service's [scheduleBackgroundRunIn] will be analyzed to determine when [backgroundRun] should be called. + * wants to be instantiated and its [backgroundRun] function called when the app is in the background. + * Each background service's [scheduleBackgroundRunIn] will be analyzed to determine when + * [backgroundRun] should be called. */ interface IBackgroundService { /** @@ -16,7 +16,12 @@ interface IBackgroundService { val scheduleBackgroundRunIn: Long? /** - * Run the background service + * Run the background service. + * WARNING: This may not follow your scheduleBackgroundRunIn schedule: + * 1. May run more often as the lowest scheduleBackgroundRunIn + * value is used across the SDK. + * 2. Android doesn't guarantee exact timing on when the job is run, + * so it's possible for it to be delayed by a few minutes. */ @WorkerThread suspend fun backgroundRun() diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt index c769f7d0ba..310824167b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt @@ -8,6 +8,7 @@ import com.onesignal.session.internal.influence.Influence import com.onesignal.session.internal.influence.InfluenceChannel import com.onesignal.session.internal.influence.InfluenceType import com.onesignal.session.internal.influence.InfluenceType.Companion.fromString +import com.onesignal.session.internal.outcomes.migrations.RemoveZeroSessionTimeRecords import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import org.json.JSONArray @@ -101,6 +102,7 @@ internal class OutcomeEventsRepository( override suspend fun getAllEventsToSend(): List { val events: MutableList = ArrayList() withContext(Dispatchers.IO) { + RemoveZeroSessionTimeRecords.run(_databaseProvider) _databaseProvider.os.query(OutcomeEventsTable.TABLE_NAME) { cursor -> if (cursor.moveToFirst()) { do { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt new file mode 100644 index 0000000000..93bab4d4a3 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt @@ -0,0 +1,24 @@ +package com.onesignal.session.internal.outcomes.migrations + +import com.onesignal.core.internal.database.IDatabaseProvider +import com.onesignal.session.internal.outcomes.impl.OutcomeEventsTable + +/** + * Purpose: Clean up invalid cached os__session_duration outcome records + * with zero session_time produced in SDK versions 5.1.15 to 5.1.20 so we stop + * sending these requests to the backend. + * + * Issue: SessionService.backgroundRun() didn't account for it being run more + * than one time in the background, when this happened it would create a + * outcome record with zero time which is invalid. + */ +object RemoveZeroSessionTimeRecords { + fun run(databaseProvider: IDatabaseProvider) { + databaseProvider.os.delete( + OutcomeEventsTable.TABLE_NAME, + OutcomeEventsTable.COLUMN_NAME_NAME + " = \"os__session_duration\"" + + " AND " + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " = 0", + null, + ) + } +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt index e49e96c26e..13c31ec50c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/SessionModel.kt @@ -16,7 +16,10 @@ class SessionModel : Model() { } /** - * Whether the session is valid. + * Indicates if there is an active session. + * True when app is in the foreground. + * Also true in the background for a short period of time (default 30s) + * as a debouncing mechanism. */ var isValid: Boolean get() = getBooleanProperty(::isValid.name) { false } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt index d831ef54b1..a76425b9a3 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionService.kt @@ -47,18 +47,28 @@ internal class SessionService( private var config: ConfigModel? = null private var shouldFireOnSubscribe = false + // True if app has been foregrounded at least once since the app started + private var hasFocused = false + override fun start() { session = _sessionModelStore.model config = _configModelStore.model - // Reset the session validity property to drive a new session - session!!.isValid = false _applicationService.addApplicationLifecycleHandler(this) } + /** NOTE: This triggers more often than scheduleBackgroundRunIn defined above, + * as it runs on the lowest IBackgroundService.scheduleBackgroundRunIn across + * the SDK. + */ override suspend fun backgroundRun() { + endSession() + } + + private fun endSession() { + if (!session!!.isValid) return val activeDuration = session!!.activeDuration - // end the session Logging.debug("SessionService.backgroundRun: Session ended. activeDuration: $activeDuration") + session!!.isValid = false sessionLifeCycleNotifier.fire { it.onSessionEnded(activeDuration) } session!!.activeDuration = 0L @@ -75,6 +85,13 @@ internal class SessionService( */ override fun onFocus(firedOnSubscribe: Boolean) { Logging.log(LogLevel.DEBUG, "SessionService.onFocus() - fired from start: $firedOnSubscribe") + + // Treat app cold starts as a new session, we attempt to end any previous session to do this. + if (!hasFocused) { + hasFocused = true + endSession() + } + if (!session!!.isValid) { // As the old session was made inactive, we need to create a new session shouldFireOnSubscribe = firedOnSubscribe @@ -82,7 +99,6 @@ internal class SessionService( session!!.startTime = _time.currentTimeMillis session!!.focusTime = session!!.startTime session!!.isValid = true - Logging.debug("SessionService: New session started at ${session!!.startTime}") sessionLifeCycleNotifier.fire { it.onSessionStarted() } } else { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt index 2a912d69e6..1daa409ac0 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/SessionServiceTests.kt @@ -86,10 +86,10 @@ class SessionServiceTests : FunSpec({ // Then sessionModelStore.model.isValid shouldBe true - sessionModelStore.model.startTime shouldBe startTime + sessionModelStore.model.startTime shouldBe mocks.currentTime sessionModelStore.model.focusTime shouldBe mocks.currentTime - verify(exactly = 1) { mocks.spyCallback.onSessionActive() } - verify(exactly = 0) { mocks.spyCallback.onSessionStarted() } + verify(exactly = 0) { mocks.spyCallback.onSessionActive() } + verify(exactly = 1) { mocks.spyCallback.onSessionStarted() } } test("session active duration updated when unfocused") { @@ -140,4 +140,19 @@ class SessionServiceTests : FunSpec({ sessionModelStore.model.isValid shouldBe false verify(exactly = 1) { mocks.spyCallback.onSessionEnded(activeDuration) } } + + test("do not trigger onSessionEnd if session is not active") { + // Given + val mocks = Mocks() + mocks.sessionModelStore { it.isValid = false } + val sessionService = mocks.sessionService + sessionService.subscribe(mocks.spyCallback) + sessionService.start() + + // When + sessionService.backgroundRun() + + // Then + verify(exactly = 0) { mocks.spyCallback.onSessionEnded(any()) } + } }) diff --git a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt index fcbb33ab9d..e0dc47fcbe 100644 --- a/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt +++ b/OneSignalSDK/onesignal/location/src/main/java/com/onesignal/location/internal/background/LocationBackgroundService.kt @@ -35,6 +35,10 @@ internal class LocationBackgroundService( return scheduleTime } + /** NOTE: This triggers more often than scheduleBackgroundRunIn defined above, + * as it runs on the lowest IBackgroundService.scheduleBackgroundRunIn across + * the SDK. + */ override suspend fun backgroundRun() { _capturer.captureLastLocation() }