Fix: NPE when setting timezone on app focus (#2505)

Co-authored-by: AR Abdul Azeez <[email protected]>
Co-authored-by: Nan <[email protected]>

Author: 3 people

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/session/impl/SessionListener.kt

@@ -1,5 +1,6 @@
 package com.onesignal.session.internal.session.impl
 
+import com.onesignal.common.TimeUtils
 import com.onesignal.common.threading.suspendifyOnIO
 import com.onesignal.core.internal.config.ConfigModelStore
 import com.onesignal.core.internal.operations.IOperationRepo
@@ -11,6 +12,7 @@ import com.onesignal.session.internal.session.ISessionService
 import com.onesignal.user.internal.identity.IdentityModelStore
 import com.onesignal.user.internal.operations.TrackSessionEndOperation
 import com.onesignal.user.internal.operations.TrackSessionStartOperation
+import com.onesignal.user.internal.properties.PropertiesModelStore
 
 /**
  * The [SessionListener] is responsible for subscribing itself as an [ISessionLifecycleHandler]
@@ -33,17 +35,20 @@ internal class SessionListener(
     private val _sessionService: ISessionService,
     private val _configModelStore: ConfigModelStore,
     private val _identityModelStore: IdentityModelStore,
+    private val _propertiesModelStore: PropertiesModelStore,
     private val _outcomeEventsController: IOutcomeEventsController,
 ) : IStartableService, ISessionLifecycleHandler {
     override fun start() {
         _sessionService.subscribe(this)
     }
 
     override fun onSessionStarted() {
+        _propertiesModelStore.model.timezone = TimeUtils.getTimeZoneId()
         _operationRepo.enqueue(TrackSessionStartOperation(_configModelStore.model.appId, _identityModelStore.model.onesignalId), true)
     }
 
     override fun onSessionActive() {
+        _propertiesModelStore.model.timezone = TimeUtils.getTimeZoneId()
     }
 
     override fun onSessionEnded(duration: Long) {

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt

@@ -2,12 +2,9 @@ package com.onesignal.user.internal
 
 import com.onesignal.common.IDManager
 import com.onesignal.common.OneSignalUtils
-import com.onesignal.common.TimeUtils
 import com.onesignal.common.events.EventProducer
 import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler
 import com.onesignal.common.modeling.ModelChangedArgs
-import com.onesignal.core.internal.application.IApplicationLifecycleHandler
-import com.onesignal.core.internal.application.IApplicationService
 import com.onesignal.core.internal.language.ILanguageContext
 import com.onesignal.debug.LogLevel
 import com.onesignal.debug.internal.logging.Logging
@@ -29,8 +26,7 @@ internal open class UserManager(
     private val _identityModelStore: IdentityModelStore,
     private val _propertiesModelStore: PropertiesModelStore,
     private val _languageContext: ILanguageContext,
-    private val _applicationService: IApplicationService,
-) : IUserManager, IApplicationLifecycleHandler, ISingletonModelStoreChangeHandler<IdentityModel> {
+) : IUserManager, ISingletonModelStoreChangeHandler<IdentityModel> {
     override val onesignalId: String
         get() = if (IDManager.isLocalId(_identityModel.onesignalId)) "" else _identityModel.onesignalId
 
@@ -59,7 +55,6 @@ internal open class UserManager(
     }
 
     init {
-        _applicationService.addApplicationLifecycleHandler(this)
         _identityModelStore.subscribe(this)
     }
 
@@ -265,11 +260,4 @@ internal open class UserManager(
             }
         }
     }
-
-    override fun onFocus(firedOnSubscribe: Boolean) {
-        // Detect any user properties updates that changed
-        _propertiesModel.timezone = TimeUtils.getTimeZoneId()
-    }
-
-    override fun onUnfocused() { }
 }

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/session/impl/SessionListenerTests.kt

@@ -1,25 +1,38 @@
 package com.onesignal.session.internal.session.impl
 
+import com.onesignal.common.TimeUtils
 import com.onesignal.mocks.MockHelper
 import com.onesignal.user.internal.operations.TrackSessionStartOperation
 import io.kotest.core.spec.style.FunSpec
+import io.mockk.every
 import io.mockk.mockk
+import io.mockk.mockkObject
+import io.mockk.unmockkAll
 import io.mockk.verify
 
+private class Mocks {
+    val mockOperationRepo = mockk<com.onesignal.core.internal.operations.IOperationRepo>(relaxed = true)
+
+    val propertiesModelStore = MockHelper.propertiesModelStore()
+
+    val sessionListener =
+        SessionListener(
+            mockOperationRepo,
+            mockk<com.onesignal.session.internal.session.ISessionService>(relaxed = true),
+            MockHelper.configModelStore(),
+            MockHelper.identityModelStore(),
+            propertiesModelStore,
+            mockk<com.onesignal.session.internal.outcomes.IOutcomeEventsController>(relaxed = true),
+        )
+}
+
 class SessionListenerTests : FunSpec({
 
     test("onSessionStarted enqueues TrackSessionStartOperation") {
         // Given
-        val mockOperationRepo = mockk<com.onesignal.core.internal.operations.IOperationRepo>(relaxed = true)
-
-        val sessionListener =
-            SessionListener(
-                mockOperationRepo,
-                mockk<com.onesignal.session.internal.session.ISessionService>(relaxed = true),
-                MockHelper.configModelStore(),
-                MockHelper.identityModelStore(),
-                mockk<com.onesignal.session.internal.outcomes.IOutcomeEventsController>(relaxed = true),
-            )
+        val mocks = Mocks()
+        val mockOperationRepo = mocks.mockOperationRepo
+        val sessionListener = mocks.sessionListener
 
         // When
         sessionListener.onSessionStarted()
@@ -29,4 +42,47 @@ class SessionListenerTests : FunSpec({
             mockOperationRepo.enqueue(any<TrackSessionStartOperation>(), true)
         }
     }
+
+    test("onSessionStarted updates timezone") {
+        // Given
+        val mocks = Mocks()
+        val sessionListener = mocks.sessionListener
+        val propertiesModel = mocks.propertiesModelStore.model
+        var timezoneId = "1"
+        mockkObject(TimeUtils)
+        every { TimeUtils.getTimeZoneId() } returns timezoneId
+
+        // When
+        sessionListener.onSessionStarted()
+
+        // Then
+        propertiesModel.timezone = timezoneId
+
+        // Once timezone is changed, repetitive onSessionStarted call will update timezone
+        timezoneId = "2"
+        sessionListener.onSessionStarted()
+        propertiesModel.timezone = timezoneId
+
+        unmockkAll()
+    }
+
+    test("onSessionActive updates timezone") {
+        // Given
+        val mocks = Mocks()
+        val sessionListener = mocks.sessionListener
+        val propertiesModel = mocks.propertiesModelStore.model
+        var timezoneId = "1"
+        mockkObject(TimeUtils)
+        every { TimeUtils.getTimeZoneId() } returns timezoneId
+
+        // When
+        sessionListener.onSessionStarted()
+
+        // Once timezone is changed, onSessionActive will update timezone
+        timezoneId = "2"
+        sessionListener.onSessionActive()
+        propertiesModel.timezone = timezoneId
+
+        unmockkAll()
+    }
 })

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/UserManagerTests.kt

@@ -1,6 +1,5 @@
 package com.onesignal.user.internal
 
-import com.onesignal.common.TimeUtils
 import com.onesignal.core.internal.language.ILanguageContext
 import com.onesignal.mocks.MockHelper
 import com.onesignal.user.internal.subscriptions.ISubscriptionManager
@@ -11,10 +10,8 @@ import io.kotest.matchers.shouldNotBe
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
-import io.mockk.mockkObject
 import io.mockk.runs
 import io.mockk.slot
-import io.mockk.unmockkObject
 import io.mockk.verify
 
 class UserManagerTests : FunSpec({
@@ -29,7 +26,7 @@ class UserManagerTests : FunSpec({
         every { languageContext.language = capture(languageSlot) } answers { }
 
         val userManager =
-            UserManager(mockSubscriptionManager, MockHelper.identityModelStore(), MockHelper.propertiesModelStore(), languageContext, MockHelper.applicationService())
+            UserManager(mockSubscriptionManager, MockHelper.identityModelStore(), MockHelper.propertiesModelStore(), languageContext)
 
         // When
         userManager.setLanguage("new-language")
@@ -47,7 +44,7 @@ class UserManagerTests : FunSpec({
             }
 
         val userManager =
-            UserManager(mockSubscriptionManager, identityModelStore, MockHelper.propertiesModelStore(), MockHelper.languageContext(), MockHelper.applicationService())
+            UserManager(mockSubscriptionManager, identityModelStore, MockHelper.propertiesModelStore(), MockHelper.languageContext())
 
         // When
         val externalId = userManager.externalId
@@ -66,7 +63,7 @@ class UserManagerTests : FunSpec({
             }
 
         val userManager =
-            UserManager(mockSubscriptionManager, identityModelStore, MockHelper.propertiesModelStore(), MockHelper.languageContext(), MockHelper.applicationService())
+            UserManager(mockSubscriptionManager, identityModelStore, MockHelper.propertiesModelStore(), MockHelper.languageContext())
 
         // When
         val alias1 = userManager.aliases["my-alias-key1"]
@@ -105,7 +102,7 @@ class UserManagerTests : FunSpec({
             }
 
         val userManager =
-            UserManager(mockSubscriptionManager, MockHelper.identityModelStore(), propertiesModelStore, MockHelper.languageContext(), MockHelper.applicationService())
+            UserManager(mockSubscriptionManager, MockHelper.identityModelStore(), propertiesModelStore, MockHelper.languageContext())
 
         // When
         val tag1 = propertiesModelStore.model.tags["my-tag-key1"]
@@ -144,7 +141,7 @@ class UserManagerTests : FunSpec({
                 it.tags["my-tag-key1"] = "my-tag-value1"
             }
 
-        val userManager = UserManager(mockSubscriptionManager, MockHelper.identityModelStore(), propertiesModelStore, MockHelper.languageContext(), MockHelper.applicationService())
+        val userManager = UserManager(mockSubscriptionManager, MockHelper.identityModelStore(), propertiesModelStore, MockHelper.languageContext())
 
         // When
         val tagSnapshot1 = userManager.getTags()
@@ -177,7 +174,6 @@ class UserManagerTests : FunSpec({
                 MockHelper.identityModelStore(),
                 MockHelper.propertiesModelStore(),
                 MockHelper.languageContext(),
-                MockHelper.applicationService(),
             )
 
         // When
@@ -195,36 +191,4 @@ class UserManagerTests : FunSpec({
         verify(exactly = 1) { mockSubscriptionManager.addSmsSubscription("+15558675309") }
         verify(exactly = 1) { mockSubscriptionManager.removeSmsSubscription("+15558675309") }
     }
-
-    test("onFocus updates timezone") {
-        // Given
-        val mockTimeZone = "Europe/Foo"
-        mockkObject(TimeUtils)
-        every { TimeUtils.getTimeZoneId() } returns mockTimeZone
-
-        val mockPropertiesModelStore = MockHelper.propertiesModelStore()
-
-        val userManager =
-            UserManager(
-                mockk<ISubscriptionManager>(),
-                MockHelper.identityModelStore(),
-                mockPropertiesModelStore,
-                MockHelper.languageContext(),
-                MockHelper.applicationService(),
-            )
-
-        val propertiesModel = mockPropertiesModelStore.model
-        propertiesModel.timezone shouldNotBe mockTimeZone
-
-        try {
-            // When
-            userManager.onFocus(firedOnSubscribe = false)
-
-            // Then
-            propertiesModel.timezone shouldBe mockTimeZone
-        } finally {
-            // Clean up the mock
-            unmockkObject(TimeUtils)
-        }
-    }
 })