diff --git a/OneSignalSDK/detekt/detekt-baseline-core.xml b/OneSignalSDK/detekt/detekt-baseline-core.xml index 9beb98a826..743ae6ca39 100644 --- a/OneSignalSDK/detekt/detekt-baseline-core.xml +++ b/OneSignalSDK/detekt/detekt-baseline-core.xml @@ -211,6 +211,7 @@ LongMethod:TrackGooglePurchase.kt$TrackGooglePurchase$private fun queryBoughtItems() LongMethod:TrackGooglePurchase.kt$TrackGooglePurchase$private fun sendPurchases( skusToAdd: ArrayList<String>, newPurchaseTokens: ArrayList<String>, ) LongMethod:UpdateUserOperationExecutor.kt$UpdateUserOperationExecutor$override suspend fun execute(operations: List<Operation>): ExecutionResponse + LongParameterList:CrashDirCleanup.kt$( label: String, path: String, entries: List<CrashDirEntry>, nowMs: Long, maxSample: Int, ownedSuffix: String = CRASH_OWNED_SUFFIX, ) LongParameterList:CreateSubscriptionOperation.kt$CreateSubscriptionOperation$(appId: String, onesignalId: String, externalId: String?, subscriptionId: String, type: SubscriptionType, enabled: Boolean, address: String, status: SubscriptionStatus) LongParameterList:ICustomEventBackendService.kt$ICustomEventBackendService$( appId: String, onesignalId: String, externalId: String?, timestamp: Long, eventName: String, eventProperties: String?, metadata: CustomEventMetadata, jwt: String? = null, ) LongParameterList:IDatabase.kt$IDatabase$( table: String, columns: Array<String>? = null, whereClause: String? = null, whereArgs: Array<String>? = null, groupBy: String? = null, having: String? = null, orderBy: String? = null, limit: String? = null, action: (ICursor) -> Unit, ) @@ -257,7 +258,7 @@ MagicNumber:PermissionsActivity.kt$PermissionsActivity$23 MagicNumber:RefreshUserOperationExecutor.kt$RefreshUserOperationExecutor$404 MagicNumber:SessionListener.kt$SessionListener$1000 - MagicNumber:SubscriptionModel.kt$SubscriptionStatus.DISABLED_FROM_REST_API_DEFAULT_REASON$30 + MagicNumber:SubscriptionModel.kt$SubscriptionStatus.DISABLED_FROM_REST_API$31 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.ERROR$9999 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.FIREBASE_FCM_ERROR_IOEXCEPTION_AUTHENTICATION_FAILED$29 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.FIREBASE_FCM_ERROR_IOEXCEPTION_OTHER$11 @@ -268,6 +269,7 @@ MagicNumber:SubscriptionModel.kt$SubscriptionStatus.HMS_ARGUMENTS_INVALID$26 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.HMS_TOKEN_TIMEOUT$25 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.INVALID_FCM_SENDER_ID$6 + MagicNumber:SubscriptionModel.kt$SubscriptionStatus.MANUALLY_UNSUBSCRIBED$22 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.MISSING_FIREBASE_FCM_LIBRARY$4 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.MISSING_HMS_PUSHKIT_LIBRARY$28 MagicNumber:SubscriptionModel.kt$SubscriptionStatus.MISSING_JETPACK_LIBRARY$3 @@ -308,7 +310,7 @@ ReturnCount:ConfigModel.kt$ConfigModel$override fun createModelForProperty( property: String, jsonObject: JSONObject, ): Model? ReturnCount:ExecutorsIvExtensions.kt$internal fun resolveIvBackendParams( op: Operation, onesignalId: String, jwtTokenStore: JwtTokenStore, ivBehaviorActive: Boolean, ): IvBackendParams ReturnCount:ExecutorsIvExtensions.kt$internal fun resolveIvJwt( op: Operation, jwtTokenStore: JwtTokenStore, ivBehaviorActive: Boolean, ): String? - ReturnCount:FeatureFlagsBackendService.kt$FeatureFlagsBackendService$override suspend fun fetchRemoteFeatureFlags(appId: String): RemoteFeatureFlagsFetchOutcome + ReturnCount:FeatureFlagsRefreshService.kt$FeatureFlagsRefreshService$private suspend fun fetchAndApply(appId: String) ReturnCount:HttpClient.kt$HttpClient$private suspend fun makeRequest( url: String, method: String?, jsonBody: JSONObject?, timeout: Int, headers: OptionalHeaders?, ): HttpResponse ReturnCount:IdentityOperationExecutor.kt$IdentityOperationExecutor$override suspend fun execute(operations: List<Operation>): ExecutionResponse ReturnCount:JSONUtils.kt$JSONUtils$fun compareJSONArrays( jsonArray1: JSONArray?, jsonArray2: JSONArray?, ): Boolean @@ -321,7 +323,6 @@ ReturnCount:Model.kt$Model$protected fun getOptIntProperty( name: String, create: (() -> Int?)? = null, ): Int? ReturnCount:Model.kt$Model$protected fun getOptLongProperty( name: String, create: (() -> Long?)? = null, ): Long? ReturnCount:Model.kt$Model$protected inline fun <reified T : Enum<T>> getOptEnumProperty(name: String): T? - ReturnCount:OneSignalImp.kt$OneSignalImp$private fun internalInit( context: Context, appId: String?, ): Boolean ReturnCount:OperationModelStore.kt$OperationModelStore$override fun create(jsonObject: JSONObject?): Operation? ReturnCount:OperationModelStore.kt$OperationModelStore$private fun isValidOperation(jsonObject: JSONObject): Boolean ReturnCount:OperationRepo.kt$OperationRepo$private fun shouldSuppressAnonymousOp(op: Operation): Boolean @@ -354,7 +355,6 @@ SwallowedException:PreferencesService.kt$PreferencesService$t: Throwable SwallowedException:SyncJobService.kt$SyncJobService$e: Exception SwallowedException:TrackGooglePurchase.kt$TrackGooglePurchase.Companion$t: Throwable - ThrowsCount:OneSignalImp.kt$OneSignalImp$private suspend fun waitUntilInitInternal(operationName: String? = null) TooGenericExceptionCaught:AndroidUtils.kt$AndroidUtils$e: Throwable TooGenericExceptionCaught:DeviceUtils.kt$DeviceUtils$t: Throwable TooGenericExceptionCaught:FeatureFlagsRefreshService.kt$FeatureFlagsRefreshService$e: Exception @@ -363,6 +363,7 @@ TooGenericExceptionCaught:JSONUtils.kt$JSONUtils$t: Throwable TooGenericExceptionCaught:Logging.kt$Logging$t: Throwable TooGenericExceptionCaught:OneSignalDispatchers.kt$OneSignalDispatchers$e: Exception + TooGenericExceptionCaught:OneSignalDispatchers.kt$OneSignalDispatchers.Pools$e: Exception TooGenericExceptionCaught:OperationRepo.kt$OperationRepo$e: Throwable TooGenericExceptionCaught:PreferenceStoreFix.kt$PreferenceStoreFix$e: Throwable TooGenericExceptionCaught:PreferencesService.kt$PreferencesService$e: Throwable @@ -632,15 +633,7 @@ UnusedPrivateMember:JSONUtils.kt$JSONUtils$`object`: Any UnusedPrivateMember:OSDatabase.kt$OSDatabase.Companion$private const val FLOAT_TYPE = " FLOAT" UnusedPrivateMember:OperationRepo.kt$OperationRepo$private val _time: ITime - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("'initWithContext failed' before 'login'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("'initWithContext failed' before 'logout'") UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("'initWithContext failed' before 'updateUserJwt'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'addUserJwtInvalidatedListener'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'login'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'logout'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'removeUserJwtInvalidatedListener'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before 'updateUserJwt'") - UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw IllegalStateException("Must call 'initWithContext' before use") UseCheckOrError:OneSignalImp.kt$OneSignalImp$throw initFailureException ?: IllegalStateException("Initialization failed. Cannot proceed.") diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt index b5c226ba38..b2a060d24c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/modeling/Model.kt @@ -544,7 +544,9 @@ open class Model( val value = getOptAnyProperty(name) ?: return null if (value is T) return value - if (value is String) return enumValueOf(value) + // Enum properties persist by name; a name this build's enum lacks (a removed case, or a + // downgrade from a newer SDK) must read as null rather than throw at model load. + if (value is String) return enumValues().firstOrNull { it.name == value } return value as T } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/PushSubscription.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/PushSubscription.kt index 3e5dac63f4..1b33b5de12 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/PushSubscription.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/PushSubscription.kt @@ -19,9 +19,21 @@ internal open class PushSubscription( get() = model.address override val optedIn: Boolean - get() = model.optedIn && model.status != SubscriptionStatus.NO_PERMISSION + // A remote disable suppresses delivery just as surely as a missing permission or an + // opt-out, so it belongs in the same answer. Reported through the model's recorded reason + // rather than its status, which stays device-owned. + get() = model.optedIn && + model.status != SubscriptionStatus.NO_PERMISSION && + model.remoteDisabledReason == 0 override fun optIn() { + // A deliberate opt-in overrides a remote disable; clearing it with a NORMAL-tagged + // change drives a subscription update that re-enables it on the server. The flag keeps + // a fetch that started before that update went out from recording the disable again. + model.remoteDisableClearedByUser = true + if (model.remoteDisabledReason != 0) { + model.remoteDisabledReason = 0 + } // we set `optedIn` using the lower level method so we can set `forceChange=true`, which // will result in *always* driving change notification. model.setBooleanProperty(SubscriptionModel::optedIn.name, true, forceChange = true) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserSwitcher.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserSwitcher.kt index f1401e031a..5cf46983ef 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserSwitcher.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserSwitcher.kt @@ -68,6 +68,7 @@ class UserSwitcher( optedIn = currentPushSubscription?.optedIn ?: true address = currentPushSubscription?.address ?: "" status = currentPushSubscription?.status ?: SubscriptionStatus.NO_PERMISSION + remoteDisabledReason = currentPushSubscription?.remoteDisabledReason ?: 0 sdk = oneSignalUtils.sdkVersion deviceOS = this@UserSwitcher.deviceOS ?: "" carrier = carrierName ?: "" diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/builduser/impl/RebuildUserService.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/builduser/impl/RebuildUserService.kt index 22442072e5..c88f0197b6 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/builduser/impl/RebuildUserService.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/builduser/impl/RebuildUserService.kt @@ -1,5 +1,6 @@ package com.onesignal.user.internal.builduser.impl +import com.onesignal.common.modeling.ModelChangeTags import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.operations.Operation import com.onesignal.user.internal.builduser.IRebuildUserService @@ -8,6 +9,7 @@ import com.onesignal.user.internal.identity.IdentityModelStore import com.onesignal.user.internal.operations.CreateSubscriptionOperation import com.onesignal.user.internal.operations.LoginUserOperation import com.onesignal.user.internal.operations.RefreshUserOperation +import com.onesignal.user.internal.operations.impl.listeners.SubscriptionModelStoreListener import com.onesignal.user.internal.properties.PropertiesModel import com.onesignal.user.internal.properties.PropertiesModelStore import com.onesignal.user.internal.subscriptions.SubscriptionModel @@ -48,20 +50,36 @@ class RebuildUserService( operations.add(LoginUserOperation(appId, onesignalId, identityModel.externalId)) val pushSubscription = subscriptionModels.firstOrNull { it.id == _configModelStore.model.pushSubscriptionId } if (pushSubscription != null) { - operations.add( - CreateSubscriptionOperation( - appId, - onesignalId, - identityModel.externalId, - pushSubscription.id, - pushSubscription.type, - pushSubscription.optedIn, - pushSubscription.address, - pushSubscription.status, - ), - ) + operations.add(buildPushRecoveryOperation(appId, onesignalId, identityModel.externalId, pushSubscription)) } operations.add(RefreshUserOperation(appId, onesignalId, identityModel.externalId)) return operations } + + // The server records this rebuild recreates no longer exist, so a recorded remote + // disable died with them; clear it and recreate from device truth. + private fun buildPushRecoveryOperation( + appId: String, + onesignalId: String, + externalId: String?, + pushSubscription: SubscriptionModel, + ): CreateSubscriptionOperation { + _subscriptionsModelStore.get(pushSubscription.id)?.setIntProperty( + SubscriptionModel::remoteDisabledReason.name, + 0, + ModelChangeTags.HYDRATE, + ) + pushSubscription.remoteDisabledReason = 0 + val (enabled, status) = SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(pushSubscription) + return CreateSubscriptionOperation( + appId, + onesignalId, + externalId, + pushSubscription.id, + pushSubscription.type, + enabled, + pushSubscription.address, + status, + ) + } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt index c1335a1b25..c094e63132 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt @@ -77,7 +77,8 @@ class CreateSubscriptionOperation() : Operation(SubscriptionOperationExecutor.CR * The status of this subscription. */ var status: SubscriptionStatus - get() = getEnumProperty(::status.name) + // A persisted name this build's enum lacks reads as SUBSCRIBED instead of dropping the op batch. + get() = getOptEnumProperty(::status.name) ?: SubscriptionStatus.SUBSCRIBED private set(value) { setEnumProperty(::status.name, value) } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt index 51ea11282b..8f8bcef53a 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt @@ -76,7 +76,8 @@ class UpdateSubscriptionOperation() : Operation(SubscriptionOperationExecutor.UP * The status of this subscription. */ var status: SubscriptionStatus - get() = getEnumProperty(::status.name) + // A persisted name this build's enum lacks reads as SUBSCRIBED instead of dropping the op batch. + get() = getOptEnumProperty(::status.name) ?: SubscriptionStatus.SUBSCRIBED private set(value) { setEnumProperty(::status.name, value) } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt index 7ab631f961..19b335b5ac 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt @@ -125,7 +125,8 @@ internal class RefreshUserOperationExecutor( SubscriptionType.PUSH } } - subscriptionModel.optedIn = subscriptionModel.status != SubscriptionStatus.UNSUBSCRIBE && subscriptionModel.status != SubscriptionStatus.DISABLED_FROM_REST_API_DEFAULT_REASON + subscriptionModel.optedIn = subscriptionModel.status != SubscriptionStatus.UNSUBSCRIBE && + !SubscriptionStatus.isRemoteDisable(subscriptionModel.status.value) subscriptionModel.sdk = subscription.sdk ?: "" subscriptionModel.deviceOS = subscription.deviceOS ?: "" subscriptionModel.carrier = subscription.carrier ?: "" @@ -136,6 +137,7 @@ internal class RefreshUserOperationExecutor( if (subscriptionModel.type != SubscriptionType.PUSH) { subscriptionModels.add(subscriptionModel) } else if (subscription.id == pushSubscriptionIdFromConfig && pushSelfHealOperationForStuckSubscription == null) { + hydrateRemoteDisableState(subscription, pushSubscriptionIdFromConfig) // Self-heal for users stuck at "Never Subscribed". Older SDK builds dispatched // the merged create-subscription + update-subscription(SUBSCRIBED) batch as a // POST /subscriptions carrying the already-existing server-side id; the server @@ -218,7 +220,10 @@ internal class RefreshUserOperationExecutor( val (localEnabled, localStatus) = SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(cachedPushSubscriptionModel) val serverEnabled = (serverSubscription.enabled == true) && ((serverSubscription.notificationTypes ?: 0) > 0) - val divergent = localEnabled && !serverEnabled + // A remote disable is deliberate suppression, not the stuck-subscription drift this + // self-heal exists for; leave it in place. + val serverDisabledRemotely = SubscriptionStatus.isRemoteDisable(serverSubscription.notificationTypes) + val divergent = localEnabled && !serverEnabled && !serverDisabledRemotely return if (divergent) { Logging.info( @@ -242,6 +247,45 @@ internal class RefreshUserOperationExecutor( } } + /** + * Records or clears the server's remote disable state on the cached push model. Only that + * state is server-owned; the device stays the source of truth for the rest of the push model, + * which is why push subscriptions are otherwise not hydrated from the backend. An opt-in whose + * update has not reached the server yet outranks a fetch that still reports the disable it cleared. + */ + private fun hydrateRemoteDisableState( + serverSubscription: SubscriptionObject, + pushSubscriptionId: String, + ) { + val cachedPushSubscriptionModel = _subscriptionsModelStore.get(pushSubscriptionId) + val serverTypes = serverSubscription.notificationTypes + if (cachedPushSubscriptionModel == null || serverTypes == null) return + // The recorded reason mirrors the server's field verbatim, so -22 and -31 stay + // distinguishable; any other reported value clears. + val target = if (SubscriptionStatus.isRemoteDisable(serverTypes)) serverTypes else 0 + if (target == 0) { + cachedPushSubscriptionModel.remoteDisableClearedByUser = false + } else if (cachedPushSubscriptionModel.remoteDisableClearedByUser) { + // This fetch predates the opt-in's update, so it reports the state that update replaces. + Logging.debug("RefreshUserOperationExecutor: keeping an opt-in over a stale remote disable report") + return + } + if (cachedPushSubscriptionModel.remoteDisabledReason != target) { + Logging.debug( + if (target != 0) { + "RefreshUserOperationExecutor: recording remote disable $target for push subscription $pushSubscriptionId" + } else { + "RefreshUserOperationExecutor: clearing remote disable ${cachedPushSubscriptionModel.remoteDisabledReason} for push subscription $pushSubscriptionId" + }, + ) + cachedPushSubscriptionModel.setIntProperty( + SubscriptionModel::remoteDisabledReason.name, + target, + ModelChangeTags.HYDRATE, + ) + } + } + companion object { const val REFRESH_USER = "refresh-user" } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt index 6db548a206..7e76e3ca81 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt @@ -31,9 +31,11 @@ import com.onesignal.user.internal.operations.CreateSubscriptionOperation import com.onesignal.user.internal.operations.DeleteSubscriptionOperation import com.onesignal.user.internal.operations.TransferSubscriptionOperation import com.onesignal.user.internal.operations.UpdateSubscriptionOperation +import com.onesignal.user.internal.operations.impl.listeners.SubscriptionModelStoreListener import com.onesignal.user.internal.operations.impl.states.NewRecordsState import com.onesignal.user.internal.subscriptions.SubscriptionModel import com.onesignal.user.internal.subscriptions.SubscriptionModelStore +import com.onesignal.user.internal.subscriptions.SubscriptionStatus import com.onesignal.user.internal.subscriptions.SubscriptionType internal class SubscriptionOperationExecutor( @@ -265,11 +267,22 @@ internal class SubscriptionOperationExecutor( // emitting Creates with the same subscriptionId, so they dedupe instead // of producing two POST /users subscription rows. HYDRATE prevents the // SubscriptionModelStoreListener from enqueuing follow-on operations. - _subscriptionModelStore.get(staleSubscriptionId)?.setStringProperty( + val recoveryModel = _subscriptionModelStore.get(staleSubscriptionId) + recoveryModel?.setStringProperty( SubscriptionModel::id.name, recoveryLocalId, ModelChangeTags.HYDRATE, ) + // The stale record died with any recorded remote disable; recreate from + // device truth rather than the values frozen on the failed operation. + recoveryModel?.setIntProperty( + SubscriptionModel::remoteDisabledReason.name, + 0, + ModelChangeTags.HYDRATE, + ) + val (recoveryEnabled, recoveryStatus) = + recoveryModel?.let { SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(it) } + ?: freshStartWithoutDeadDisable(lastOperation) if (_configModelStore.model.pushSubscriptionId == staleSubscriptionId) { _configModelStore.model.pushSubscriptionId = recoveryLocalId } @@ -284,9 +297,9 @@ internal class SubscriptionOperationExecutor( lastOperation.externalId, recoveryLocalId, lastOperation.type, - lastOperation.enabled, + recoveryEnabled, lastOperation.address, - lastOperation.status, + recoveryStatus, ), ), ) @@ -376,6 +389,15 @@ internal class SubscriptionOperationExecutor( return ExecutionResponse(ExecutionResult.SUCCESS) } + /** The failed op's enabled/status, minus a remote disable that belonged to the dead record. */ + private fun freshStartWithoutDeadDisable(operation: UpdateSubscriptionOperation): Pair { + return if (SubscriptionStatus.isRemoteDisable(operation.status.value)) { + Pair(true, SubscriptionStatus.SUBSCRIBED) + } else { + Pair(operation.enabled, operation.status) + } + } + companion object { const val CREATE_SUBSCRIPTION = "create-subscription" const val UPDATE_SUBSCRIPTION = "update-subscription" diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/listeners/SubscriptionModelStoreListener.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/listeners/SubscriptionModelStoreListener.kt index c210193e69..8a22cedd8b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/listeners/SubscriptionModelStoreListener.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/listeners/SubscriptionModelStoreListener.kt @@ -73,7 +73,14 @@ internal class SubscriptionModelStoreListener( val status: SubscriptionStatus val enabled: Boolean - if (model.optedIn && model.status == SubscriptionStatus.SUBSCRIBED && model.address.isNotEmpty()) { + // A remote disable is server-owned; report the recorded code back rather than the + // device state so subscription payloads don't re-enable a suppressed subscription. + val remoteDisabledStatus = SubscriptionStatus.remoteDisableStatus(model.remoteDisabledReason) + + if (remoteDisabledStatus != null) { + enabled = false + status = remoteDisabledStatus + } else if (model.optedIn && model.status == SubscriptionStatus.SUBSCRIBED && model.address.isNotEmpty()) { enabled = true status = SubscriptionStatus.SUBSCRIBED } else { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModel.kt index 84825dfda3..6bd5a6f8c0 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModel.kt @@ -47,7 +47,10 @@ enum class SubscriptionStatus(val value: Int) { /** The subscription is not enabled due to any other FCM Exception, this can be retried */ FIREBASE_FCM_ERROR_MISC_EXCEPTION(-12), - // -13 to -24 reserved for other platforms + // -13 to -21, -23, and -24 reserved for other platforms + + /** The subscription is not enabled because it was unsubscribed by hand from the dashboard */ + MANUALLY_UNSUBSCRIBED(-22), /** The subscription is not enabled due the an HMS timeout, this can be retried */ HMS_TOKEN_TIMEOUT(-25), @@ -67,8 +70,8 @@ enum class SubscriptionStatus(val value: Int) { /** The subscription is not enabled due to an FCM authentication failed IOException, this can be retried */ FIREBASE_FCM_ERROR_IOEXCEPTION_AUTHENTICATION_FAILED(-29), - /** The subscription is not enabled because the app has disabled the subscription via API */ - DISABLED_FROM_REST_API_DEFAULT_REASON(-30), + /** The subscription is not enabled because it was disabled through the REST API */ + DISABLED_FROM_REST_API(-31), /** The subscription is not enabled due to some other (unknown locally) error */ ERROR(9999), @@ -101,6 +104,34 @@ enum class SubscriptionStatus(val value: Int) { FIREBASE_FCM_ERROR_IOEXCEPTION_AUTHENTICATION_FAILED, // -29 ) + /** + * The codes the server owns, meaning the app owner turned this subscription off remotely. + * The SDK never derives either from device state, and every other server-reported error + * code stays device-recoverable. + */ + private val REMOTE_DISABLES = + setOf( + MANUALLY_UNSUBSCRIBED, // -22 + DISABLED_FROM_REST_API, // -31 + ) + + /** + * The status for a remote-disable code, or `null` when [value] is not one. The two codes + * stay distinct in [SubscriptionModel.remoteDisabledReason] and on the wire, so callers + * report back the exact code the server sent rather than collapsing them. + */ + fun remoteDisableStatus(value: Int?): SubscriptionStatus? { + return REMOTE_DISABLES.firstOrNull { it.value == value } + } + + /** + * True when [value] is one of the codes for a subscription the app owner disabled + * remotely. The SDK treats them the same because both mean the server turned this off. + */ + fun isRemoteDisable(value: Int?): Boolean { + return remoteDisableStatus(value) != null + } + fun fromInt(value: Int): SubscriptionStatus? { return SubscriptionStatus.values().firstOrNull { it.value == value } } @@ -135,6 +166,32 @@ class SubscriptionModel : Model() { setBooleanProperty(::isDisabledInternally.name, value) } + /** + * The code for a subscription the app owner turned off remotely, either by hand from the + * dashboard ([SubscriptionStatus.MANUALLY_UNSUBSCRIBED], -22) or through the REST API + * ([SubscriptionStatus.DISABLED_FROM_REST_API], -31), or 0 when the server has not disabled + * this subscription. Both codes mean the same thing to the SDK but are recorded separately, so + * payloads echo back the one the server sent. Hydrated by RefreshUser and never derived from + * device state; while set, [SubscriptionModelStoreListener] reports `enabled = false` with the + * matching status so subscription payloads don't re-enable a suppressed subscription. Cleared + * when the server reports any other state, or by [IPushSubscription.optIn]. + */ + var remoteDisabledReason: Int + get() = getIntProperty(::remoteDisabledReason.name) { 0 } + set(value) { + setIntProperty(::remoteDisabledReason.name, value) + } + + /** + * True from [IPushSubscription.optIn] until the server reports this subscription in any state + * other than a remote disable. Every opt-in sends a subscription update, and a fetch that + * started before that update went out still reports the disable the opt-in cleared; while + * this is set, RefreshUser leaves [remoteDisabledReason] alone instead of recording that + * stale answer. Memory only, since a fresh process has no update in flight to protect. + */ + @Volatile + var remoteDisableClearedByUser: Boolean = false + var type: SubscriptionType get() = getEnumProperty(::type.name) set(value) { @@ -160,7 +217,8 @@ class SubscriptionModel : Model() { setEnumProperty(::status.name, SubscriptionStatus.SUBSCRIBED) } - return getEnumProperty(::status.name) + // A persisted name this build's enum lacks reads as SUBSCRIBED instead of throwing. + return getOptEnumProperty(::status.name) ?: SubscriptionStatus.SUBSCRIBED } set(value) { setEnumProperty(::status.name, value) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModelStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModelStore.kt index 089d2bc881..60294042b2 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModelStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/subscriptions/SubscriptionModelStore.kt @@ -27,6 +27,7 @@ open class SubscriptionModelStore(prefs: IPreferencesService) : SimpleModelStore model.carrier = existingPushModel.carrier model.appVersion = existingPushModel.appVersion model.status = existingPushModel.status + model.remoteDisabledReason = existingPushModel.remoteDisabledReason } break } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/IPushSubscription.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/IPushSubscription.kt index 1430141f96..34b310982d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/IPushSubscription.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/IPushSubscription.kt @@ -15,7 +15,8 @@ interface IPushSubscription : ISubscription { * Whether the user of this subscription is opted-in to received notifications. When true, * the user is able to receive notifications through this subscription. Otherwise, the * user will not receive notifications through this subscription (even when the user has - * granted app permission). + * granted app permission). This is false while the app owner has the subscription disabled + * remotely, from the dashboard or the REST API; [optIn] clears that suppression. */ val optedIn: Boolean diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/PushSubscriptionState.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/PushSubscriptionState.kt index f43dbdb8b6..f60dba08db 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/PushSubscriptionState.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/subscriptions/PushSubscriptionState.kt @@ -22,7 +22,8 @@ class PushSubscriptionState( * Whether the user of this subscription is opted-in to received notifications. When true, * the user is able to receive notifications through this subscription. Otherwise, the * user will not receive notifications through this subscription (even when the user has - * granted app permission). + * granted app permission). This is false while the app owner has the subscription disabled + * remotely, from the dashboard or the REST API. */ val optedIn: Boolean, ) { diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/UserSwitcherTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/UserSwitcherTests.kt index 18c4c53ea2..f73c8caa53 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/UserSwitcherTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/UserSwitcherTests.kt @@ -20,6 +20,7 @@ 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.impl.listeners.SubscriptionModelStoreListener import com.onesignal.user.internal.properties.PropertiesModelStore import com.onesignal.user.internal.subscriptions.SubscriptionModel import com.onesignal.user.internal.subscriptions.SubscriptionModelStore @@ -257,6 +258,33 @@ class UserSwitcherTests : FunSpec({ verify(exactly = 1) { mockSubscriptionModelStore.add(any(), ModelChangeTags.NO_PROPOGATE) } } + test("createAndSwitchToNewUser carries a remote disable onto the new push model") { + // Uses -22 rather than -31 so the assertions below also prove the exact recorded code + // survives the switch instead of every remote disable collapsing to one status. + // Given + val mocks = Mocks() + val userSwitcher = mocks.createUserSwitcher() + val disabledPushModel = + SubscriptionModel().apply { + id = mocks.testSubscriptionId + type = SubscriptionType.PUSH + address = "test-token" + optedIn = true + remoteDisabledReason = SubscriptionStatus.MANUALLY_UNSUBSCRIBED.value + } + mocks.subscriptionModelStore!!.add(disabledPushModel, ModelChangeTags.NO_PROPOGATE) + + // When + userSwitcher.createAndSwitchToNewUser() + + // Then the login create for the new user still reports the subscription disabled + val newPushModel = mocks.subscriptionModelStore!!.list().first { it.type == SubscriptionType.PUSH } + newPushModel.remoteDisabledReason shouldBe SubscriptionStatus.MANUALLY_UNSUBSCRIBED.value + val (enabled, status) = SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(newPushModel) + enabled shouldBe false + status shouldBe SubscriptionStatus.MANUALLY_UNSUBSCRIBED + } + test("initUser with forceCreateUser creates new user") { // Given val mocks = Mocks() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/builduser/RebuildUserServiceTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/builduser/RebuildUserServiceTests.kt new file mode 100644 index 0000000000..399f37b4dc --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/builduser/RebuildUserServiceTests.kt @@ -0,0 +1,75 @@ +package com.onesignal.user.internal.builduser + +import com.onesignal.mocks.MockHelper +import com.onesignal.user.internal.builduser.impl.RebuildUserService +import com.onesignal.user.internal.operations.CreateSubscriptionOperation +import com.onesignal.user.internal.operations.LoginUserOperation +import com.onesignal.user.internal.operations.RefreshUserOperation +import com.onesignal.user.internal.subscriptions.SubscriptionModel +import com.onesignal.user.internal.subscriptions.SubscriptionModelStore +import com.onesignal.user.internal.subscriptions.SubscriptionStatus +import com.onesignal.user.internal.subscriptions.SubscriptionType +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.mockk.every +import io.mockk.mockk + +class RebuildUserServiceTests : FunSpec({ + val appId = "appId" + val onesignalId = "onesignalId" + val subscriptionId = "subscriptionId" + + fun buildService(pushModel: SubscriptionModel?): RebuildUserService { + val subscriptionModelStore = mockk() + every { subscriptionModelStore.list() } returns listOfNotNull(pushModel) + every { subscriptionModelStore.get(any()) } returns pushModel + return RebuildUserService( + MockHelper.identityModelStore { it.onesignalId = onesignalId }, + MockHelper.propertiesModelStore { it.onesignalId = onesignalId }, + subscriptionModelStore, + MockHelper.configModelStore { it.pushSubscriptionId = subscriptionId }, + ) + } + + test("rebuild recreates a remotely disabled push subscription from device truth") { + // Given: the records being rebuilt are gone, so the recorded disable goes with them + val pushModel = + SubscriptionModel().apply { + id = subscriptionId + type = SubscriptionType.PUSH + address = "pushToken" + optedIn = true + status = SubscriptionStatus.SUBSCRIBED + remoteDisabledReason = SubscriptionStatus.DISABLED_FROM_REST_API.value + } + val service = buildService(pushModel) + + // When + val operations = service.getRebuildOperationsIfCurrentUser(appId, onesignalId)!! + + // Then + (operations[0] is LoginUserOperation) shouldBe true + val create = operations[1] as CreateSubscriptionOperation + create.subscriptionId shouldBe subscriptionId + create.enabled shouldBe true + create.status shouldBe SubscriptionStatus.SUBSCRIBED + (operations[2] is RefreshUserOperation) shouldBe true + pushModel.remoteDisabledReason shouldBe 0 + } + + test("rebuild without a push subscription emits only the login and refresh") { + val service = buildService(null) + + val operations = service.getRebuildOperationsIfCurrentUser(appId, onesignalId)!! + + operations.size shouldBe 2 + (operations[0] is LoginUserOperation) shouldBe true + (operations[1] is RefreshUserOperation) shouldBe true + } + + test("rebuild returns null when the current user is no longer the one that needs rebuilding") { + val service = buildService(null) + + service.getRebuildOperationsIfCurrentUser(appId, "otherOnesignalId") shouldBe null + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt index 072a6f2213..d4dbf73c87 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt @@ -529,4 +529,131 @@ class RefreshUserOperationExecutorTests : FunSpec({ mockUserBackendService.getUser(appId, IdentityConstants.ONESIGNAL_ID, remoteOneSignalId) } } + + // Both remote-disable codes mean "the app owner turned this off", so both suppress the + // self-heal, and each is recorded verbatim so the payload echoes back the code the server sent + // rather than a single collapsed one. + listOf( + SubscriptionStatus.MANUALLY_UNSUBSCRIBED, + SubscriptionStatus.DISABLED_FROM_REST_API, + ).forEach { remoteDisable -> + test("push self-heal: does NOT enqueue follow-up op when the server reports ${remoteDisable.value}") { + // Given: server says push is disabled with a remote-disable code, local view says enabled + val (executor, cachedPushSubscriptionModel, _) = + buildSelfHealHarness( + serverPushEnabled = false, + serverNotificationTypes = remoteDisable.value, + localOptedIn = true, + localStatus = SubscriptionStatus.SUBSCRIBED, + localAddress = onDevicePushToken, + ) + + // When + val response = executor.execute(listOf(RefreshUserOperation(appId, remoteOneSignalId, null))) + + // Then no follow-up op, and that exact code is recorded on the cached push model + response.result shouldBe ExecutionResult.SUCCESS + response.operations shouldBe null + cachedPushSubscriptionModel.remoteDisabledReason shouldBe remoteDisable.value + } + } + + test("push self-heal: still re-asserts local truth when the server reports another disabled code") { + // Any disabled code other than the remote-disable codes (-22, -31) stays device-recoverable + val (executor, cachedPushSubscriptionModel, _) = + buildSelfHealHarness( + serverPushEnabled = false, + serverNotificationTypes = -2, + localOptedIn = true, + localStatus = SubscriptionStatus.SUBSCRIBED, + localAddress = onDevicePushToken, + ) + + val originalLogLevel = Logging.logLevel + Logging.logLevel = LogLevel.NONE + try { + // When + val response = executor.execute(listOf(RefreshUserOperation(appId, remoteOneSignalId, null))) + + // Then the self-heal op is emitted and nothing is recorded as a remote disable + response.result shouldBe ExecutionResult.SUCCESS + response.operations?.count() shouldBe 1 + (response.operations!![0] is UpdateSubscriptionOperation) shouldBe true + cachedPushSubscriptionModel.remoteDisabledReason shouldBe 0 + } finally { + Logging.logLevel = originalLogLevel + } + } + + test("push refresh: clears a recorded remote disable when the server reports another code") { + // Given: -31 recorded locally, server now reports a different code + val (executor, cachedPushSubscriptionModel, _) = + buildSelfHealHarness( + serverPushEnabled = false, + serverNotificationTypes = -2, + localOptedIn = true, + localStatus = SubscriptionStatus.SUBSCRIBED, + localAddress = onDevicePushToken, + ) + cachedPushSubscriptionModel.remoteDisabledReason = SubscriptionStatus.DISABLED_FROM_REST_API.value + + val originalLogLevel = Logging.logLevel + Logging.logLevel = LogLevel.NONE + try { + // When + val response = executor.execute(listOf(RefreshUserOperation(appId, remoteOneSignalId, null))) + + // Then the mirror clears and the self-heal still re-asserts local truth + response.result shouldBe ExecutionResult.SUCCESS + cachedPushSubscriptionModel.remoteDisabledReason shouldBe 0 + response.operations?.count() shouldBe 1 + } finally { + Logging.logLevel = originalLogLevel + } + } + + test("push refresh: clears a recorded remote disable when the server reports enabled again") { + // Given: a locally recorded remote disable, server now reports the subscription enabled + val (executor, cachedPushSubscriptionModel, _) = + buildSelfHealHarness( + serverPushEnabled = true, + serverNotificationTypes = 1, + localOptedIn = true, + localStatus = SubscriptionStatus.SUBSCRIBED, + localAddress = onDevicePushToken, + ) + cachedPushSubscriptionModel.remoteDisabledReason = SubscriptionStatus.DISABLED_FROM_REST_API.value + cachedPushSubscriptionModel.remoteDisableClearedByUser = true + + // When + val response = executor.execute(listOf(RefreshUserOperation(appId, remoteOneSignalId, null))) + + // Then the mirror clears and the opt-in's precedence over stale reports ends + response.result shouldBe ExecutionResult.SUCCESS + response.operations shouldBe null + cachedPushSubscriptionModel.remoteDisabledReason shouldBe 0 + cachedPushSubscriptionModel.remoteDisableClearedByUser shouldBe false + } + + test("push refresh: keeps an opt-in over a fetch that still reports the remote disable it cleared") { + // Given: optIn() ran while this fetch was pending, so the server still reports -31 + val (executor, cachedPushSubscriptionModel, _) = + buildSelfHealHarness( + serverPushEnabled = false, + serverNotificationTypes = SubscriptionStatus.DISABLED_FROM_REST_API.value, + localOptedIn = true, + localStatus = SubscriptionStatus.SUBSCRIBED, + localAddress = onDevicePushToken, + ) + cachedPushSubscriptionModel.remoteDisableClearedByUser = true + + // When + val response = executor.execute(listOf(RefreshUserOperation(appId, remoteOneSignalId, null))) + + // Then the stale disable is not recorded, the flag stays, and no self-heal fires + response.result shouldBe ExecutionResult.SUCCESS + response.operations shouldBe null + cachedPushSubscriptionModel.remoteDisabledReason shouldBe 0 + cachedPushSubscriptionModel.remoteDisableClearedByUser shouldBe true + } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt index 77c58b109f..48ae3eda4d 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt @@ -715,6 +715,64 @@ class SubscriptionOperationExecutorTests : configModelStore.model.pushSubscriptionId shouldBe recovery.subscriptionId } + test("update subscription 404 recovery recreates from device truth, not the dead record's remote disable") { + // Given: the cached model carries a recorded remote disable for the record that 404s + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } throws BackendException(404) + + val mockSubscriptionsModelStore = mockk() + val cachedSubscriptionModel = + SubscriptionModel().apply { + id = remoteSubscriptionId + type = SubscriptionType.PUSH + address = "pushToken2" + optedIn = true + remoteDisabledReason = SubscriptionStatus.DISABLED_FROM_REST_API.value + } + every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns cachedSubscriptionModel + + val configModelStore = MockHelper.configModelStore().also { it.model.pushSubscriptionId = remoteSubscriptionId } + val mockBuildUserService = mockk() + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + configModelStore, + mockBuildUserService, + getNewRecordState(), + mockConsistencyManager, + getJwtTokenStore(), getIdentityVerificationService(), + ) + + // The queued op is the -31 echo for the now-deleted record + val operations = + listOf( + UpdateSubscriptionOperation( + appId, + remoteOneSignalId, + "ext-1", + remoteSubscriptionId, + SubscriptionType.PUSH, + false, + "pushToken2", + SubscriptionStatus.DISABLED_FROM_REST_API, + ), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then the recovery create is born from device truth and the dead record's disable is gone + response.result shouldBe ExecutionResult.FAIL_NORETRY + val recovery = response.operations!!.first() as CreateSubscriptionOperation + recovery.enabled shouldBe true + recovery.status shouldBe SubscriptionStatus.SUBSCRIBED + cachedSubscriptionModel.remoteDisabledReason shouldBe 0 + } + test("update subscription fails with retry when the backend returns MISSING, when isInMissingRetryWindow") { // Given val mockSubscriptionBackendService = mockk() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/PushSubscriptionObserverTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/PushSubscriptionObserverTests.kt new file mode 100644 index 0000000000..99264c13f6 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/PushSubscriptionObserverTests.kt @@ -0,0 +1,111 @@ +package com.onesignal.user.internal.subscriptions + +import com.onesignal.common.modeling.ModelChangeTags +import com.onesignal.core.internal.application.IApplicationService +import com.onesignal.debug.LogLevel +import com.onesignal.debug.internal.logging.Logging +import com.onesignal.mocks.MockPreferencesService +import com.onesignal.session.internal.session.ISessionService +import com.onesignal.user.internal.subscriptions.impl.SubscriptionManager +import com.onesignal.user.subscriptions.IPushSubscriptionObserver +import com.onesignal.user.subscriptions.PushSubscriptionChangedState +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.mockk.mockk +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit + +/** + * End-to-end coverage for the push subscription observer, kept in its own spec because + * [IPushSubscriptionObserver] callbacks are delivered through `Dispatchers.Main` and the rest of the + * subscription specs run without a main dispatcher installed. + * + * The chain under test is the real one: a property write on a model held by a [SubscriptionModelStore] + * notifies the store, which re-broadcasts to [SubscriptionManager], which builds the changed state + * from the push subscription and hands it to the app's observer. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class PushSubscriptionObserverTests : FunSpec({ + + beforeTest { + Logging.logLevel = LogLevel.NONE + // Observer callbacks go out via suspendifyOnMain, so tests need a main dispatcher. + Dispatchers.setMain(UnconfinedTestDispatcher()) + } + + afterTest { + Dispatchers.resetMain() + } + + /** + * A store holding one opted-in push subscription, with [SubscriptionManager] subscribed to it and + * an observer attached. Returns the model to mutate and a latch plus recorded states to assert on. + */ + fun attachObserverToPushSubscription(): Triple> { + val pushSubscriptionModel = SubscriptionModel() + pushSubscriptionModel.id = "subscription1" + pushSubscriptionModel.type = SubscriptionType.PUSH + pushSubscriptionModel.address = "pushToken" + pushSubscriptionModel.status = SubscriptionStatus.SUBSCRIBED + pushSubscriptionModel.optedIn = true + + val subscriptionModelStore = SubscriptionModelStore(MockPreferencesService()) + subscriptionModelStore.add(pushSubscriptionModel) + + // Constructing the manager subscribes it to the store and builds its subscription list. + val subscriptionManager = + SubscriptionManager( + mockk(), + mockk(relaxed = true), + subscriptionModelStore, + ) + + val observedStates = mutableListOf() + val observerCalled = CountDownLatch(1) + subscriptionManager.subscriptions.push.addObserver( + object : IPushSubscriptionObserver { + override fun onPushSubscriptionChange(state: PushSubscriptionChangedState) { + observedStates.add(state) + observerCalled.countDown() + } + }, + ) + + return Triple(pushSubscriptionModel, observerCalled, observedStates) + } + + // Both codes take the same path, so both are pinned end to end rather than only the one the + // unit-level tests happen to exercise. + listOf( + SubscriptionStatus.MANUALLY_UNSUBSCRIBED, + SubscriptionStatus.DISABLED_FROM_REST_API, + ).forEach { remoteDisable -> + test("hydrating a ${remoteDisable.value} disable reports optedIn false to the app's observer") { + // Given an opted-in push subscription with an observer attached + val (pushSubscriptionModel, observerCalled, observedStates) = attachObserverToPushSubscription() + + // When RefreshUser records the server's disable, which it writes with the HYDRATE tag. + // Nothing on the path from the model to the observer filters on that tag, which is why + // the app hears about a disable it never asked for locally. + pushSubscriptionModel.setIntProperty( + SubscriptionModel::remoteDisabledReason.name, + remoteDisable.value, + ModelChangeTags.HYDRATE, + ) + + // Then the observer sees a real transition, not an unchanged pair + observerCalled.await(5, TimeUnit.SECONDS) shouldBe true + observedStates.size shouldBe 1 + observedStates[0].previous.optedIn shouldBe true + observedStates[0].current.optedIn shouldBe false + // The rest of the state is untouched: only the opt-in answer moved. + observedStates[0].current.id shouldBe "subscription1" + observedStates[0].current.token shouldBe "pushToken" + } + } +}) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt index a127a250e9..acee1c0d08 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/subscriptions/SubscriptionManagerTests.kt @@ -9,7 +9,10 @@ import com.onesignal.core.internal.application.IApplicationService import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging import com.onesignal.session.internal.session.ISessionService +import com.onesignal.user.internal.PushSubscription import com.onesignal.user.internal.Subscription +import com.onesignal.user.internal.operations.UpdateSubscriptionOperation +import com.onesignal.user.internal.operations.impl.listeners.SubscriptionModelStoreListener import com.onesignal.user.internal.subscriptions.impl.SubscriptionManager import com.onesignal.user.subscriptions.ISmsSubscription import io.kotest.core.spec.style.FunSpec @@ -24,6 +27,7 @@ import io.mockk.mockk import io.mockk.runs import io.mockk.spyk import io.mockk.verify +import org.json.JSONObject class SubscriptionManagerTests : FunSpec({ @@ -681,7 +685,6 @@ class SubscriptionManagerTests : FunSpec({ listOf( SubscriptionStatus.NO_PERMISSION, SubscriptionStatus.UNSUBSCRIBE, - SubscriptionStatus.DISABLED_FROM_REST_API_DEFAULT_REASON, ) for (status in nonRetryableStatuses) { @@ -799,8 +802,165 @@ class SubscriptionManagerTests : FunSpec({ SubscriptionStatus.INVALID_FCM_SENDER_ID, SubscriptionStatus.OUTDATED_GOOGLE_PLAY_SERVICES_APP, SubscriptionStatus.HMS_ARGUMENTS_INVALID, - SubscriptionStatus.DISABLED_FROM_REST_API_DEFAULT_REASON, + SubscriptionStatus.MANUALLY_UNSUBSCRIBED, + SubscriptionStatus.DISABLED_FROM_REST_API, SubscriptionStatus.ERROR, ).forEach { it.isRetryableTokenError shouldBe false } } + + test("status persisted under an unknown enum name reads as SUBSCRIBED instead of throwing") { + // Models persist enum properties by name; a cached model written under an enum case this + // version does not have must still load. + val model = SubscriptionModel() + model.initializeFromJson( + JSONObject() + .put("id", "subscription1") + .put("status", "STATUS_UNKNOWN_TO_THIS_VERSION"), + ) + + model.status shouldBe SubscriptionStatus.SUBSCRIBED + } + + test("operation status persisted under an unknown enum name reads as SUBSCRIBED instead of throwing") { + // Operation batches persist by enum name like models; an unknown name must not drop the batch. + val operation = UpdateSubscriptionOperation() + operation.initializeFromJson(JSONObject().put("status", "STATUS_UNKNOWN_TO_THIS_VERSION")) + + operation.status shouldBe SubscriptionStatus.SUBSCRIBED + } + + test("SubscriptionStatus.isRemoteDisable is true only for the two server-owned disable codes") { + // -22 (unsubscribed by hand from the dashboard) and -31 (disabled through the REST API) are + // the only codes the app owner sets remotely. Every other negative code describes a device + // or delivery problem the device recovers from by re-asserting its own state, so widening + // this predicate would make the SDK stop re-enabling those subscriptions. + SubscriptionStatus.values().forEach { + SubscriptionStatus.isRemoteDisable(it.value) shouldBe + (it == SubscriptionStatus.MANUALLY_UNSUBSCRIBED || it == SubscriptionStatus.DISABLED_FROM_REST_API) + } + // 0 is the "nothing recorded" sentinel for remoteDisabledReason, not a disable. + SubscriptionStatus.isRemoteDisable(0) shouldBe false + SubscriptionStatus.isRemoteDisable(null) shouldBe false + } + + // Both codes are treated the same but recorded separately, so the payload reports back the + // exact code the server sent instead of collapsing -22 into -31. + listOf( + SubscriptionStatus.MANUALLY_UNSUBSCRIBED, + SubscriptionStatus.DISABLED_FROM_REST_API, + ).forEach { remoteDisable -> + test("getSubscriptionEnabledAndStatus reports a ${remoteDisable.value} disable back to the server") { + // Given a push subscription the app owner disabled remotely + val pushSubscription = SubscriptionModel() + pushSubscription.id = "subscription1" + pushSubscription.type = SubscriptionType.PUSH + pushSubscription.address = "pushToken" + pushSubscription.status = SubscriptionStatus.SUBSCRIBED + pushSubscription.optedIn = true + pushSubscription.remoteDisabledReason = remoteDisable.value + + // When + val (enabled, status) = SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(pushSubscription) + + // Then the recorded code round-trips rather than being reported as the other one + enabled shouldBe false + status shouldBe remoteDisable + } + + test("optedIn reports false while a ${remoteDisable.value} disable is recorded") { + // A remote disable suppresses delivery, so the property clients read to decide whether + // push works must say so. Before this, a preference center showed "subscribed" on a + // device the app owner had turned off, and nothing in the public API revealed why. + val pushSubscriptionModel = SubscriptionModel() + pushSubscriptionModel.id = "subscription1" + pushSubscriptionModel.type = SubscriptionType.PUSH + pushSubscriptionModel.address = "pushToken" + pushSubscriptionModel.status = SubscriptionStatus.SUBSCRIBED + pushSubscriptionModel.optedIn = true + + val pushSubscription = PushSubscription(pushSubscriptionModel) + pushSubscription.optedIn shouldBe true + + // When the server's disable is recorded + pushSubscriptionModel.remoteDisabledReason = remoteDisable.value + + // Then + pushSubscription.optedIn shouldBe false + } + + test("refreshState carries a ${remoteDisable.value} disable into the observer payload") { + // The observer already fires on the hydration write; this pins the payload it carries, + // since the previous/current pair is built from refreshState. + val pushSubscriptionModel = SubscriptionModel() + pushSubscriptionModel.id = "subscription1" + pushSubscriptionModel.type = SubscriptionType.PUSH + pushSubscriptionModel.address = "pushToken" + pushSubscriptionModel.status = SubscriptionStatus.SUBSCRIBED + pushSubscriptionModel.optedIn = true + + val pushSubscription = PushSubscription(pushSubscriptionModel) + val previousState = pushSubscription.savedState + + // When + pushSubscriptionModel.remoteDisabledReason = remoteDisable.value + val currentState = pushSubscription.refreshState() + + // Then the observer sees a real transition rather than an unchanged pair + previousState.optedIn shouldBe true + currentState.optedIn shouldBe false + } + + test("optIn clears a ${remoteDisable.value} disable so the update re-enables the subscription") { + // Given a push subscription the app owner disabled remotely + val pushSubscriptionModel = SubscriptionModel() + pushSubscriptionModel.id = "subscription1" + pushSubscriptionModel.type = SubscriptionType.PUSH + pushSubscriptionModel.address = "pushToken" + pushSubscriptionModel.status = SubscriptionStatus.SUBSCRIBED + pushSubscriptionModel.optedIn = true + pushSubscriptionModel.remoteDisabledReason = remoteDisable.value + + // When + PushSubscription(pushSubscriptionModel).optIn() + + // Then + pushSubscriptionModel.remoteDisabledReason shouldBe 0 + pushSubscriptionModel.remoteDisableClearedByUser shouldBe true + // The toggle a client drives off is not a dead end: opting in reports true again. + PushSubscription(pushSubscriptionModel).optedIn shouldBe true + val (enabled, status) = SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus(pushSubscriptionModel) + enabled shouldBe true + status shouldBe SubscriptionStatus.SUBSCRIBED + } + } + + test("optedIn ignores a device-recoverable error status") { + // Only the two server-owned codes reach optedIn, and they arrive through + // remoteDisabledReason rather than status. A device-side delivery error is recoverable by + // re-asserting local state, so it must not read as an opt-out to the app. + val pushSubscriptionModel = SubscriptionModel() + pushSubscriptionModel.id = "subscription1" + pushSubscriptionModel.type = SubscriptionType.PUSH + pushSubscriptionModel.address = "pushToken" + pushSubscriptionModel.optedIn = true + pushSubscriptionModel.status = SubscriptionStatus.FIREBASE_FCM_ERROR_IOEXCEPTION_SERVICE_NOT_AVAILABLE + + PushSubscription(pushSubscriptionModel).optedIn shouldBe true + } + + test("optIn takes precedence over a pending fetch even when no remote disable was recorded") { + // Given a push subscription with no recorded remote disable + val pushSubscriptionModel = SubscriptionModel() + pushSubscriptionModel.id = "subscription1" + pushSubscriptionModel.type = SubscriptionType.PUSH + pushSubscriptionModel.address = "pushToken" + pushSubscriptionModel.status = SubscriptionStatus.SUBSCRIBED + pushSubscriptionModel.optedIn = true + + // When + PushSubscription(pushSubscriptionModel).optIn() + + // Then the flag is set, since every opt-in sends an update a pending fetch may predate + pushSubscriptionModel.remoteDisableClearedByUser shouldBe true + } }) diff --git a/examples/demo/app/src/main/java/com/onesignal/example/ui/main/MainViewModel.kt b/examples/demo/app/src/main/java/com/onesignal/example/ui/main/MainViewModel.kt index 2d6bd010fc..07dbd8789c 100644 --- a/examples/demo/app/src/main/java/com/onesignal/example/ui/main/MainViewModel.kt +++ b/examples/demo/app/src/main/java/com/onesignal/example/ui/main/MainViewModel.kt @@ -695,6 +695,7 @@ class MainViewModel(application: Application) : AndroidViewModel(application), I private fun logDebug(message: String) = DemoLog.d(TAG, message) override fun onPushSubscriptionChange(state: PushSubscriptionChangedState) { + DemoLog.i(TAG, "onPushSubscriptionChange: ${state.toJSONObject()}") _pushSubscriptionId.postValue(state.current.id) _pushEnabled.postValue(state.current.optedIn) }