Skip to content

Commit c324f43

Browse files
jinliu9508jinliu9508
authored
Merge pull request #2341 from OneSignal/fix/android_push_subscription_issues
fix(subs): Fix parsing subscriptions from server, and improve handling of deleted push subscriptions
2 parents d611cd2 + 50c9e1f commit c324f43

File tree

4 files changed

+160
-30
lines changed

4 files changed

+160
-30
lines changed

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt‎

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ import com.onesignal.user.internal.identity.IdentityModel
4444
import com.onesignal.user.internal.identity.IdentityModelStore
4545
import com.onesignal.user.internal.operations.LoginUserFromSubscriptionOperation
4646
import com.onesignal.user.internal.operations.LoginUserOperation
47-
import com.onesignal.user.internal.operations.TransferSubscriptionOperation
4847
import com.onesignal.user.internal.properties.PropertiesModel
4948
import com.onesignal.user.internal.properties.PropertiesModelStore
5049
import com.onesignal.user.internal.subscriptions.SubscriptionModel
@@ -467,10 +466,8 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
467466
// to the queue.
468467
val currentPushSubscription = subscriptionModelStore!!.list().firstOrNull { it.id == configModel!!.pushSubscriptionId }
469468
val newPushSubscription = SubscriptionModel()
470-
val localSubscriptionID = IDManager.createLocalId()
471-
val currentSubscriptionID = currentPushSubscription?.id ?: localSubscriptionID
472469

473-
newPushSubscription.id = currentSubscriptionID
470+
newPushSubscription.id = currentPushSubscription?.id ?: IDManager.createLocalId()
474471
newPushSubscription.type = SubscriptionType.PUSH
475472
newPushSubscription.optedIn = currentPushSubscription?.optedIn ?: true
476473
newPushSubscription.address = currentPushSubscription?.address ?: ""
@@ -480,6 +477,11 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
480477
newPushSubscription.carrier = DeviceUtils.getCarrierName(services.getService<IApplicationService>().appContext) ?: ""
481478
newPushSubscription.appVersion = AndroidUtils.getAppVersion(services.getService<IApplicationService>().appContext) ?: ""
482479

480+
// ensure we always know this devices push subscription ID
481+
configModel!!.pushSubscriptionId = newPushSubscription.id
482+
483+
subscriptions.add(newPushSubscription)
484+
483485
// The next 4 lines makes this user the effective user locally. We clear the subscriptions
484486
// first as a `NO_PROPOGATE` change because we don't want to drive deleting the cleared subscriptions
485487
// on the backend. Once cleared we can then setup the new identity/properties model, and add
@@ -488,24 +490,11 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
488490
identityModelStore!!.replace(identityModel)
489491
propertiesModelStore!!.replace(propertiesModel)
490492

491-
var changeTag = ModelChangeTags.NO_PROPOGATE
492-
493-
if (!suppressBackendOperation) {
494-
if (currentPushSubscription?.id != null && identityModel.externalId != null) {
495-
// add a transfer-subscription operation when switching user
496-
operationRepo!!.enqueue(TransferSubscriptionOperation(configModel!!.appId, currentPushSubscription.id, sdkId))
497-
} else {
498-
// reset subscription when calling logout or login for the first time
499-
newPushSubscription.id = localSubscriptionID
500-
changeTag = ModelChangeTags.NORMAL
501-
}
493+
if (suppressBackendOperation) {
494+
subscriptionModelStore!!.replaceAll(subscriptions, ModelChangeTags.NO_PROPOGATE)
495+
} else {
496+
subscriptionModelStore!!.replaceAll(subscriptions)
502497
}
503-
504-
// ensure we always know this devices push subscription ID
505-
configModel!!.pushSubscriptionId = newPushSubscription.id
506-
507-
subscriptions.add(newPushSubscription)
508-
subscriptionModelStore!!.replaceAll(subscriptions, changeTag)
509498
}
510499

511500
override fun <T> hasService(c: Class<T>): Boolean = services.hasService(c)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TransferSubscriptionOperation.kt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,14 @@ import com.onesignal.core.internal.operations.Operation
66
import com.onesignal.user.internal.operations.impl.executors.SubscriptionOperationExecutor
77

88
/**
9+
* Deprecated as of 5.1.36, the SDK will no longer enqueue this operation. Use Create Subscription instead.
10+
* TransferSubscriptionOperation only contains the ID, but the SDK should include accurate subscription data
11+
* in the case that the push subscription may potentially have been deleted on the server.
12+
* This class remains due to potentially cached operations.
13+
* -------
914
* An [Operation] to transfer a subscription to a new owner on the OneSignal backend.
1015
*/
16+
@Deprecated("The SDK will no longer enqueue this operation. Use Create Subscription instead.")
1117
class TransferSubscriptionOperation() : Operation(SubscriptionOperationExecutor.TRANSFER_SUBSCRIPTION) {
1218
/**
1319
* The application ID this subscription will be transferred under.

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -187,21 +187,36 @@ internal class LoginUserOperationExecutor(
187187
propertiesModel.setStringProperty(PropertiesModel::onesignalId.name, backendOneSignalId, ModelChangeTags.HYDRATE)
188188
}
189189

190-
for (index in subscriptionList.indices) {
191-
if (index >= response.subscriptions.size) {
192-
break
190+
val backendSubscriptions = response.subscriptions.toMutableSet()
191+
192+
for (pair in subscriptionList) {
193+
// Find the corresponding subscription (subscriptions are not returned in the order they are sent)
194+
// 1. Start by matching the subscription ID
195+
var backendSubscription = backendSubscriptions.firstOrNull { it.id == pair.first }
196+
// 2. If ID fails, match the token, this should always succeed for email or sms
197+
if (backendSubscription == null) {
198+
backendSubscription = backendSubscriptions.firstOrNull { it.token == pair.second.token && !it.token.isNullOrBlank() }
199+
}
200+
// 3. Match by type. By this point, only a single push subscription should remain, at most
201+
if (backendSubscription == null) {
202+
backendSubscription = backendSubscriptions.firstOrNull { it.type == pair.second.type }
193203
}
194204

195-
val backendSubscription = response.subscriptions[index]
205+
if (backendSubscription != null) {
206+
idTranslations[pair.first] = backendSubscription.id!!
196207

197-
idTranslations[subscriptionList[index].first] = backendSubscription.id!!
208+
if (_configModelStore.model.pushSubscriptionId == pair.first) {
209+
_configModelStore.model.pushSubscriptionId = backendSubscription.id
210+
}
198211

199-
if (_configModelStore.model.pushSubscriptionId == subscriptionList[index].first) {
200-
_configModelStore.model.pushSubscriptionId = backendSubscription.id
212+
val subscriptionModel = _subscriptionsModelStore.get(pair.first)
213+
subscriptionModel?.setStringProperty(SubscriptionModel::id.name, backendSubscription.id!!, ModelChangeTags.HYDRATE)
214+
} else {
215+
Logging.error("LoginUserOperationExecutor.createUser response is missing subscription data for ${pair.first}")
201216
}
202217

203-
val subscriptionModel = _subscriptionsModelStore.get(subscriptionList[index].first)
204-
subscriptionModel?.setStringProperty(SubscriptionModel::id.name, backendSubscription.id, ModelChangeTags.HYDRATE)
218+
// Remove the processed backend subscription
219+
backendSubscriptions.remove(backendSubscription)
205220
}
206221

207222
val wasPossiblyAnUpsert = identities.isNotEmpty()

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,4 +739,124 @@ class LoginUserOperationExecutorTests : FunSpec({
739739
exactly = 0,
740740
) { mockUserBackendService.createUser(appId, any(), any(), any()) }
741741
}
742+
743+
test("create user maps subscriptions when backend order is different (match by id/token)") {
744+
// Given
745+
val mockUserBackendService = mockk<IUserBackendService>()
746+
// backend returns EMAIL first (with token), then PUSH — out of order
747+
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
748+
CreateUserResponse(
749+
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
750+
PropertiesObject(),
751+
listOf(
752+
SubscriptionObject(id = remoteSubscriptionId2, type = SubscriptionObjectType.EMAIL, token = "name@company.com"),
753+
SubscriptionObject(id = remoteSubscriptionId1, type = SubscriptionObjectType.ANDROID_PUSH, token = "pushToken2"),
754+
),
755+
)
756+
757+
val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()
758+
val mockIdentityModelStore = MockHelper.identityModelStore()
759+
val mockPropertiesModelStore = MockHelper.propertiesModelStore()
760+
val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
761+
every { mockSubscriptionsModelStore.get(any()) } returns null
762+
763+
val executor =
764+
LoginUserOperationExecutor(
765+
mockIdentityOperationExecutor,
766+
AndroidMockHelper.applicationService(),
767+
MockHelper.deviceService(),
768+
mockUserBackendService,
769+
mockIdentityModelStore,
770+
mockPropertiesModelStore,
771+
mockSubscriptionsModelStore,
772+
MockHelper.configModelStore(),
773+
MockHelper.languageContext(),
774+
)
775+
776+
// send PUSH then EMAIL (local IDs 1,2) — order differs from backend response
777+
val ops =
778+
listOf(
779+
LoginUserOperation(appId, localOneSignalId, null, null),
780+
CreateSubscriptionOperation(appId, localOneSignalId, localSubscriptionId1, SubscriptionType.PUSH, true, "pushToken2", SubscriptionStatus.SUBSCRIBED),
781+
CreateSubscriptionOperation(appId, localOneSignalId, localSubscriptionId2, SubscriptionType.EMAIL, true, "name@company.com", SubscriptionStatus.SUBSCRIBED),
782+
)
783+
784+
// When
785+
val response = executor.execute(ops)
786+
787+
// Then
788+
response.result shouldBe ExecutionResult.SUCCESS
789+
// ensure local to remote mapping is correct despite different order
790+
response.idTranslations shouldBe
791+
mapOf(
792+
localOneSignalId to remoteOneSignalId,
793+
// push
794+
localSubscriptionId1 to remoteSubscriptionId1,
795+
// email
796+
localSubscriptionId2 to remoteSubscriptionId2,
797+
)
798+
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(), any(), any()) }
799+
}
800+
801+
test("create user maps push subscription by type when id and token don't match (case for deleted push sub)") {
802+
// Given
803+
val mockUserBackendService = mockk<IUserBackendService>()
804+
// simulate server-side push sub recreated with new ID and no token; must match by type
805+
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
806+
CreateUserResponse(
807+
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
808+
PropertiesObject(),
809+
listOf(
810+
SubscriptionObject(id = remoteSubscriptionId1, type = SubscriptionObjectType.ANDROID_PUSH, token = null),
811+
),
812+
)
813+
814+
val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()
815+
val mockIdentityModelStore = MockHelper.identityModelStore()
816+
val mockPropertiesModelStore = MockHelper.propertiesModelStore()
817+
818+
val mockSubscriptionsModelStore = mockk<SubscriptionModelStore>()
819+
// provide a local push model so the executor can hydrate its id
820+
val localPushModel = SubscriptionModel().apply { id = localSubscriptionId1 }
821+
every { mockSubscriptionsModelStore.get(localSubscriptionId1) } returns localPushModel
822+
823+
val configModelStore = MockHelper.configModelStore()
824+
// assume current push sub is the local one we are creating
825+
configModelStore.model.pushSubscriptionId = localSubscriptionId1
826+
827+
val executor =
828+
LoginUserOperationExecutor(
829+
mockIdentityOperationExecutor,
830+
AndroidMockHelper.applicationService(),
831+
MockHelper.deviceService(),
832+
mockUserBackendService,
833+
mockIdentityModelStore,
834+
mockPropertiesModelStore,
835+
mockSubscriptionsModelStore,
836+
configModelStore,
837+
MockHelper.languageContext(),
838+
)
839+
840+
val ops =
841+
listOf(
842+
LoginUserOperation(appId, localOneSignalId, null, null),
843+
CreateSubscriptionOperation(appId, localOneSignalId, localSubscriptionId1, SubscriptionType.PUSH, true, "pushToken1", SubscriptionStatus.SUBSCRIBED),
844+
)
845+
846+
// When
847+
val response = executor.execute(ops)
848+
849+
// Then
850+
response.result shouldBe ExecutionResult.SUCCESS
851+
// should map by type and update both idTranslations and local model
852+
response.idTranslations shouldBe
853+
mapOf(
854+
localOneSignalId to remoteOneSignalId,
855+
localSubscriptionId1 to remoteSubscriptionId1,
856+
)
857+
localPushModel.id shouldBe remoteSubscriptionId1
858+
// pushSubscriptionId should be updated from local to remote id
859+
configModelStore.model.pushSubscriptionId shouldBe remoteSubscriptionId1
860+
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(), any(), any()) }
861+
}
742862
})

0 commit comments

Comments
 (0)