From 5a099c0c1cb3af9674ff0e6daa454b4e4b307354 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 22 Apr 2024 17:13:30 -0400 Subject: [PATCH] avoid unnecessary fetch users This commit avoids to unnecessary fetch user scenarios: 1. App open, first time. 2. Login, when user is currently anonymous Case 1 UserRefreshService would always add a RefreshUserOperation to the OperationRepo on cold start. However this is not needed for the first time the app is opened, as there is no need to fetch a User we just created. Case 2 If OneSignal.login() is called and the user is currently anonymous then we simply identify the User. In this case there is no need to fetch the user, as it is the same user, we just updated it. --- .../com/onesignal/internal/OneSignalImp.kt | 12 ------ .../executors/LoginUserOperationExecutor.kt | 11 ++++- .../internal/service/UserRefreshService.kt | 3 ++ .../LoginUserOperationExecutorTests.kt | 41 +++++++++++++++++++ 4 files changed, 54 insertions(+), 13 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt index 67b0befc8a..bd34b5c4f8 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt @@ -44,7 +44,6 @@ import com.onesignal.user.internal.identity.IdentityModel import com.onesignal.user.internal.identity.IdentityModelStore import com.onesignal.user.internal.operations.LoginUserFromSubscriptionOperation import com.onesignal.user.internal.operations.LoginUserOperation -import com.onesignal.user.internal.operations.RefreshUserOperation import com.onesignal.user.internal.operations.TransferSubscriptionOperation import com.onesignal.user.internal.properties.PropertiesModel import com.onesignal.user.internal.properties.PropertiesModelStore @@ -384,17 +383,6 @@ internal class OneSignalImp : IOneSignal, IServiceProvider { if (!result) { Logging.log(LogLevel.ERROR, "Could not login user") - } else { - // enqueue a RefreshUserOperation to pull the user from the backend and refresh the models. - // This is a separate enqueue operation to ensure any outstanding operations that happened - // after the createAndSwitchToNewUser have been executed, and the retrieval will be the - // most up to date reflection of the user. - operationRepo!!.enqueueAndWait( - RefreshUserOperation( - configModel!!.appId, - identityModelStore!!.model.onesignalId, - ), - ) } } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt index 280207803d..e98023c9b8 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt @@ -26,6 +26,7 @@ import com.onesignal.user.internal.identity.IdentityModelStore import com.onesignal.user.internal.operations.CreateSubscriptionOperation import com.onesignal.user.internal.operations.DeleteSubscriptionOperation import com.onesignal.user.internal.operations.LoginUserOperation +import com.onesignal.user.internal.operations.RefreshUserOperation import com.onesignal.user.internal.operations.SetAliasOperation import com.onesignal.user.internal.operations.TransferSubscriptionOperation import com.onesignal.user.internal.operations.UpdateSubscriptionOperation @@ -196,7 +197,15 @@ internal class LoginUserOperationExecutor( subscriptionModel?.setStringProperty(SubscriptionModel::id.name, backendSubscription.id, ModelChangeTags.HYDRATE) } - return ExecutionResponse(ExecutionResult.SUCCESS, idTranslations) + val wasPossiblyAnUpsert = identities.isNotEmpty() + val followUpOperations = + if (wasPossiblyAnUpsert) { + listOf(RefreshUserOperation(createUserOperation.appId, backendOneSignalId)) + } else { + null + } + + return ExecutionResponse(ExecutionResult.SUCCESS, idTranslations, followUpOperations) } catch (ex: BackendException) { val responseType = NetworkUtils.getResponseStatusType(ex.statusCode) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt index b2371d3214..44f90cdffe 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/service/UserRefreshService.kt @@ -1,5 +1,6 @@ package com.onesignal.user.internal.service +import com.onesignal.common.IDManager import com.onesignal.core.internal.application.IApplicationLifecycleHandler import com.onesignal.core.internal.application.IApplicationService import com.onesignal.core.internal.config.ConfigModelStore @@ -21,6 +22,8 @@ class UserRefreshService( ) : IStartableService, IApplicationLifecycleHandler { private fun refreshUser() { + if (IDManager.isLocalId(_identityModelStore.model.onesignalId)) return + _operationRepo.enqueue( RefreshUserOperation( _configModelStore.model.appId, diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt index b992fdc28c..57ceb3b8b0 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt @@ -156,6 +156,47 @@ class LoginUserOperationExecutorTests : FunSpec({ ) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any(), any()) } } + // If the User is identified then the backend may have found an existing User, if so + // we need to refresh it so we get all it's full subscription list + test("login identified user returns result with RefreshUser") { + // Given + val mockUserBackendService = mockk() + coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns + CreateUserResponse(mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId), PropertiesObject(), listOf()) + + val mockIdentityOperationExecutor = mockk() + + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockPropertiesModelStore = MockHelper.propertiesModelStore() + val mockSubscriptionsModelStore = mockk() + + val loginUserOperationExecutor = + LoginUserOperationExecutor( + mockIdentityOperationExecutor, + MockHelper.applicationService(), + MockHelper.deviceService(), + mockUserBackendService, + mockIdentityModelStore, + mockPropertiesModelStore, + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + MockHelper.languageContext(), + ) + val operations = listOf(LoginUserOperation(appId, localOneSignalId, "externalId", null)) + + // When + val response = loginUserOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.SUCCESS + response.operations!!.size shouldBe 1 + (response.operations!![0] as RefreshUserOperation).let { + it.shouldBeInstanceOf() + it.appId shouldBe appId + it.onesignalId shouldBe remoteOneSignalId + } + } + test("login identified user with association succeeds when association is successful") { // Given val mockUserBackendService = mockk()