Skip to content

Commit d53db6e

Browse files
nan-liclaude
andcommitted
fix(iv): restore logout unsubscribe-on-OLD-user behavior
Revert the order swap from dc93995 — that commit reordered switchUserIv to switch users first and then set isDisabledInternally on the new push sub with NO_PROPOGATE, on the bot review's claim that firing an UpdateSubscriptionOperation against the OLD user was a bug. It is not a bug — it is the intentional behavior in reference branches #2599 and #2613: setting the flag on the CURRENT push sub with the default NORMAL tag fires an UpdateSubscriptionOperation that tells the backend "this device is unsubscribing as the user logs out", dispatched with the OLD user's still-valid JWT before the switch. Without this, logout under IV silently leaves the just-departed user's push sub subscribed server-side. The new (anonymous) user's model not carrying isDisabledInternally is fine: anon ops are filtered by hasValidJwtIfRequired (externalId=null under IV-active), so they accumulate but never dispatch. Update the test to verify the corrected order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6360d74 commit d53db6e

2 files changed

Lines changed: 22 additions & 31 deletions

File tree

Lines changed: 13 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package com.onesignal.user.internal
22

3-
import com.onesignal.common.modeling.ModelChangeTags
43
import com.onesignal.core.internal.config.ConfigModel
5-
import com.onesignal.user.internal.subscriptions.SubscriptionModel
64
import com.onesignal.user.internal.subscriptions.SubscriptionModelStore
75

86
/**
@@ -15,21 +13,17 @@ import com.onesignal.user.internal.subscriptions.SubscriptionModelStore
1513
/**
1614
* Performs the IV-aware logout user-switch when [ivBehaviorActive] is true.
1715
*
18-
* Under IV-required, the new device-scoped (anonymous) user can't authenticate against
19-
* the backend without a JWT. To prevent the model-store listener from generating create-
20-
* subscription ops that would 401/permanently-block the queue:
21-
* 1. Switch to the anonymous user with [UserSwitcher.createAndSwitchToNewUser] in
22-
* `suppressBackendOperation` mode so subscription replacement does not propagate to
23-
* listeners that would enqueue backend ops.
24-
* 2. Mark the new push subscription as internally disabled with [ModelChangeTags.NO_PROPOGATE]
25-
* so subsequent property mutations (FCM token refresh, permission change, etc.)
26-
* short-circuit through [com.onesignal.user.internal.operations.impl.listeners.SubscriptionModelStoreListener.getSubscriptionEnabledAndStatus]
27-
* instead of enqueueing real ops.
28-
*
29-
* Order matters: setting the flag on the OLD model first (with default NORMAL tag) would
30-
* fire `getUpdateOperation` against the OLD user with their still-valid JWT — the listener
31-
* would build an `UpdateSubscriptionOperation(externalId = OLD)` carrying `(false, UNSUBSCRIBE)`,
32-
* dispatch it, and unsubscribe the just-departed user server-side.
16+
* Order matters and is intentional (mirrors reference branches #2599 and #2613):
17+
* 1. Set `isDisabledInternally = true` on the CURRENT push subscription with the default
18+
* NORMAL tag. This propagates through [com.onesignal.user.internal.operations.impl.listeners.SubscriptionModelStoreListener.getUpdateOperation],
19+
* which reads the still-current OLD identity and enqueues an `UpdateSubscriptionOperation`
20+
* carrying `(enabled = false, status = UNSUBSCRIBE)` — letting the backend know this device's
21+
* push subscription is unsubscribing as the user logs out. The OLD user's JWT is still valid
22+
* here, so the op dispatches successfully.
23+
* 2. Switch to the new device-scoped (anonymous) user via
24+
* [UserSwitcher.createAndSwitchToNewUser] with `suppressBackendOperation = true` so the
25+
* subscription replacement does NOT propagate to listeners — the new anonymous user has no
26+
* JWT and any create-subscription op for it would 401 indefinitely.
3327
*
3428
* Returns `true` when IV-specific handling was applied (caller skips legacy enqueue),
3529
* or `false` when IV behavior is inactive (caller falls through to the legacy logout).
@@ -42,13 +36,9 @@ internal fun switchUserIv(
4236
): Boolean {
4337
if (!ivBehaviorActive) return false
4438

45-
userSwitcher.createAndSwitchToNewUser(suppressBackendOperation = true)
4639
configModel.pushSubscriptionId?.let { pushSubId ->
47-
subscriptionModelStore.get(pushSubId)?.setBooleanProperty(
48-
SubscriptionModel::isDisabledInternally.name,
49-
true,
50-
ModelChangeTags.NO_PROPOGATE,
51-
)
40+
subscriptionModelStore.get(pushSubId)?.let { it.isDisabledInternally = true }
5241
}
42+
userSwitcher.createAndSwitchToNewUser(suppressBackendOperation = true)
5343
return true
5444
}

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

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -196,16 +196,16 @@ class LogoutHelperTests : FunSpec({
196196
verify(atLeast = 1) { mockOperationRepo.enqueue(any()) }
197197
}
198198

199-
test("switchUserIv switches user before marking new push sub as internally disabled") {
199+
test("switchUserIv marks current push sub as internally disabled before switching users") {
200200
// Given - IV active, push sub model in store
201201
val pushSubId = "push-sub-id"
202-
val newPushSubModel =
202+
val pushSubModel =
203203
SubscriptionModel().apply {
204204
id = pushSubId
205205
type = SubscriptionType.PUSH
206206
}
207207
val mockSubscriptionModelStore = mockk<SubscriptionModelStore>(relaxed = true)
208-
every { mockSubscriptionModelStore.get(pushSubId) } returns newPushSubModel
208+
every { mockSubscriptionModelStore.get(pushSubId) } returns pushSubModel
209209
val mockUserSwitcher = mockk<UserSwitcher>(relaxed = true)
210210
val mockConfigModel = mockk<ConfigModel>()
211211
every { mockConfigModel.pushSubscriptionId } returns pushSubId
@@ -216,16 +216,17 @@ class LogoutHelperTests : FunSpec({
216216
// Then
217217
handled shouldBe true
218218

219-
// Order: user-switch must happen BEFORE the flag is set on the new push sub.
220-
// Setting the flag on the OLD model first would fire an UpdateSubscriptionOperation
221-
// against the OLD user with their valid JWT and unsubscribe them server-side.
219+
// Order: flag must be set on the OLD push sub BEFORE the user-switch — setting it
220+
// with the default NORMAL tag fires an UpdateSubscriptionOperation against the OLD
221+
// user with their still-valid JWT, telling the backend that this device's push
222+
// subscription is unsubscribing as the user logs out.
222223
verifyOrder {
223-
mockUserSwitcher.createAndSwitchToNewUser(suppressBackendOperation = true)
224224
mockSubscriptionModelStore.get(pushSubId)
225+
mockUserSwitcher.createAndSwitchToNewUser(suppressBackendOperation = true)
225226
}
226227

227228
// The flag is set on the model (verified via the underlying property).
228-
newPushSubModel.isDisabledInternally shouldBe true
229+
pushSubModel.isDisabledInternally shouldBe true
229230
}
230231

231232
test("switchUserIv returns false when IV behavior is inactive") {

0 commit comments

Comments
 (0)