fix: [SDK-5099] respect REST API-disabled push subscriptions - #1731
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Opus 5, GPT 5.6 Sol, Grok 4.6)
The mirror + snapshot updateParams() / jsonRepresentation() is the right shape for SDK-5099. optedIn staying device-owned matches the stated product call.
Act on (3/3)
clearRestApiDisable()returns before settingrestApiDisableClearedByUserwhen the mirror is already nil.optIn()then loses the race against a first-31fetch (cold start after an operator disable, or after a subscription-ID reset). Tests only cover opt-in after the disable was already recorded.recordRestApiDisable/clearRestApiDisableread and write the latch and reason across separate lock acquisitions, so a concurrent hydrate can still write-31after opt-in.
Consider (2/3)
- Cached
start()/startNewSession()flush a subscription PATCH before fetch hydrates-31. The first session after a REST disable can still sendenabled: true— the customer path this PR names. - Existing-ID hydration matches only
id. A miss plus new-session “sub gone” (subscriptionId = nilclears the mirror and POSTs a new enabled sub) can re-enable. - The latch is process-lifetime and generation-less: a later legitimate
-31can be ignored until restart, and a stale-31after a newer non-disable can re-record.
Dismissed: optedIn not flipping is the stated product choice. NSCoding decodeObject as? Int matches testType/netType and is covered. Assigning nil notification_types omits the key rather than inserting NSNull.
Sent by Cursor Automation: PR Reviews
A push subscription disabled through the REST API (notification_types -31) was re-enabled by the SDK: fetch responses never hydrated the disable onto an existing push subscription, and every Create User and update payload recomputed enabled from device state. Mirror the server's disable code on the subscription model when a response reports it, and echo it back in Create User and update payloads instead of the device-derived values. The mirror clears when the server reports any other state, when the subscription ID resets because the server record is gone, and on an explicit optIn(), whose clear outranks stale in-flight hydration until the server confirms. Both payload builders share one snapshot-based body so concurrent changes cannot tear enabled away from notification_types.
nan-li
changed the title
Recording, clearing, and accepting the server's disable state each read the opt-in guard and wrote the reason under separate lock acquisitions, so a hydrate racing an optIn() could leave the guard armed with the reason re-recorded. Each transition now does its guard and write in one critical section and fires events after release. OSPushSubscription.optedIn documents that it reflects the user's preference and OS permission, not a server-side disable.
On the first launch after upgrading, if the SDK sends a routine device update before it has asked the server about the user, it can turn the subscription back on once. We accept that. It happens at most once and the next daily check fixes it. If the server's subscription ID doesn't match ours, we treat the old record as gone and make a new enabled one, which is intended. The "user opted in" marker is wiped as soon as the server reports anything other than a disable, so it can't stick around long enough to cause the problem described. |
|
This PR fixes the coordination between "the server switched this subscription off through the API" and local SDK state. Opting in and out can happen on the SDK and enabling and disabling can happen on the REST API. Almost every review comment is about a moment where this server state is missing, stale, or ignored. The sync between SDK and server is a fundamental inherent problem regardless of this PR. It will never be completely right for all. Consider that if someone adds a tag or calls optIn() and there is no connection. 30 minutes later, they update something via the REST API. Later, the SDK gets a connection 8 hours later, it will send those updates even though it is technically stale. |
The server reports notification_types -22 when someone turns a subscription off by hand from the dashboard. That means the same thing as -31, disabled through the REST API, so both codes now suppress outgoing subscription payloads the same way and neither is derived from device state. The two codes stay distinct. remoteDisabledReason records whichever one the server sent, and hydration writes it verbatim, so a payload echoes back the code the server actually reported. A second disable arriving under the other code replaces the recorded one instead of being ignored as already disabled. Renamed the restApiDisable members to remoteDisable, since "REST API" no longer describes the concept, and moved the codes themselves into a new OSRemoteDisable namespace at file scope. Keeping them inside the class pushed its body past swiftlint's type_body_length error threshold, and they describe the wire protocol rather than any one model instance. The NSCoding key changed with the property name, but no release has written the old key.
recordRemoteDisable and acceptServerNonDisabledState changed the model silently, so parsing -22 or -31 off the wire left no trace at any log level short of the raw HTTP body. Add a DEBUG line at the point each one actually changes the value, matching the Android side. Reading the previous reason for that message also made the guard inside acceptServerNonDisabledState's lock redundant, since clearing a reason that is already nil is a no-op on a struct field. The outer guard already gates both the log and the persist.
clearRemoteDisable() returned before setting remoteDisableClearedByUser when nothing was recorded locally, which is exactly the state the race starts in. The customer disables the subscription, a fetch goes out that will report the code, and optIn() runs before that response lands. The guard never armed, so the fetch recorded the disable and the next update re-sent it, silently undoing the opt-in. Arm the flag on every opt-in and keep the persist and change event gated on there having been a recorded reason. Android already did this, in "keep an opt-in over a stale REST API disable report"; this brings iOS in line and adds the regression test that platform has.
optedIn was the user's preference combined with OS permission, which stopped describing "will push reach this device" once the SDK started respecting a remote disable. The disable now sticks instead of being flipped back on by the next routine update, and nothing else in the public API reveals it, so an app syncing preferences through the REST API would read opted in forever on a device receiving nothing. calculateIsOptedIn takes the recorded reason alongside reachable and isDisabled, so the value, the observer payload built by currentPushSubscriptionState, and the previous/current pair inside firePushSubscriptionChanged all agree. The two hydration paths now fire so the transition reaches observers, and they pass generateEnabledDelta: false because the state came from the server and an enabled delta would tell it what it just told us. An optIn() clear keeps the delta, which is how the server re-enables the subscription.
|
Thanks. Items 3 and 4 look resolved. I think 1 and 2 still need changes:
Could we add an OperationRepo-level test covering metadata update followed by RefreshUser and address that ordering directly? |
I think this is meant for Android PR right? I answered it there: OneSignal/OneSignal-Android-SDK#2728 (comment) |
|
Potential issues:
|
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Two issues on the hydration / opt-in path.
- Email/SMS hydrate records a remote disable and fires the push observer.
- The opt-in guard never settles, so
optIn()on launch ignores-22/-31for the process.
…vent state Three review findings on the remote disable work. Email and SMS models hydrate through the same path as push, and the server writes the same codes on their rows when the app owner disables them. The notification_types hydration recorded the code on those models and fired the push subscription observer with an email address or phone number as the token. Remote disable state is push-only now; email and SMS keep the plain enabled mapping they had before. The opt-in guard armed on every optIn(), including calls that changed nothing. Those send nothing, so the guard had no write to protect, and it stayed armed until a fetch reported a non-disabled state. Any disable that landed in the meantime was ignored for the life of the process, and the next routine PATCH re-enabled the subscription. Apps that call optIn() on every launch re-armed it before each session's fetch returned and never recorded a disable at all. The guard now arms only when the opt-in cleared a recorded disable or lifted the user's own opt-out. When the subscription ID resets, the recorded code was cleared before the observer event was built, so a disabled subscription reported its previous state as opted in. The old code now travels with the event, and the reset no longer generates an enabled delta, since the new record's state goes out with the create request.
thanks, All three fixed in b31ea63.
|
|
Still some issues:
|
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Both comments addressed in b31ea63.
- Email/SMS hydrate is push-only.
- No-op
optIn()no longer arms the guard.
Remaining: the guard still only drops on a non-disable fetch, and userWasOptedOut can arm with no write when there is no token or permission. Neither blocks.
optIn() prompts for permission when the device has none, and the grant is the write that re-enables the subscription. That opt-in set nothing, so a fetch already in flight could record the operator's first disable before the grant landed, and the grant's update then carried the disable instead of enabling the subscription the user asked for. clearRemoteDisable() now reads the model itself, before optIn() flips the opt-out, and sets the flag when a disable was recorded, the user had opted out, or permission is missing.
|
|
Yeah #1 is not really blocking. |
(cherry picked from commit 15561f4)
3 participants
Description
One Line Summary
Stop re-enabling push subscriptions that the app owner disabled remotely, from the dashboard (
notification_types-22) or through the REST API (-31).Details
Motivation
Customers who suppress users by disabling subscriptions via the REST API see them come back subscribed. The SDK never hydrated the server's disable onto an existing push subscription (
parseFetchUserResponseis gated onsubscriptionId == nil), and every Create User and PATCH body recomputedenabledfrom device state, so a login or a routine device-metadata update silently re-enabled the subscription. A dashboard unsubscribe writes a different code and was undone the same way. Tracked internally as SDK-5099.Scope
Push subscriptions only; email/SMS hydration is unchanged. The model gains a server-owned
remoteDisabledReasonfield that mirrors the server'snotification_types, andOSRemoteDisablenames the two codes the app owner sets remotely, -22 for a dashboard unsubscribe and -31 for a REST API disable. A response reporting either one records it verbatim, so the outgoing payload echoes back the code the server sent rather than collapsing both into one. Any other reported value clears it, and it also clears when the subscription ID resets (the record it describes is gone) or on an explicitoptIn(), whose clear outranks stale in-flight hydration until the server confirms. That opt-in guard arms only when theoptIn()changed something, either it cleared a recorded disable or lifted the user's own opt-out; a call that changes nothing sends nothing and must not blind the SDK to a later disable. While a code is recorded, Create User and PATCH payloads sendenabled: falsewith that code instead of device values; both builders share one snapshot-based body (updateParams()/outboundNotificationTypes). Recording and clearing log at DEBUG.Public
pushSubscription.optedInnow reports false while a disable is recorded, and its doc comment says so. No signature changes. A remote disable suppresses delivery as surely as a missing permission or an opt-out, and it is the only one of the three an app has no other way to see, so a preference center no longer shows "subscribed" on a device the app owner turned off. Hydration therefore fires the push subscription observer with the new state, and suppresses theenableddelta a local change of the same shape would generate, since sending that would tell the server what it just told us.For reviewers
Two product calls, both matched by the companion PR in OneSignal-Android-SDK. A remote disable now flips the public
optedInto false, which reverses the earlier decision on this branch to keep that property to preference plus permission. AndoptIn()still overrides the suppression, on the reasoning that a deliberate user action beats an exclusion list.Testing
Unit testing
Model-level tests cover code recognition, verbatim recording of each code and switching between them, mirror clearing, ID-reset clearing,
optIn()precedence over stale hydration when it changed something and none when it didn't, email and SMS hydration staying off the push path, the reset event's previous state, payload override, NSCoding round-trip, device-state refresh survival,optedInreporting false for both codes,optedInignoring device-recoverable codes, and hydration firing the observer without enqueueing anenableddelta. An integration test reproduces the customer scenario end to end: fetch reports the disable, login to a different external ID, and the Create User body still sendsenabled: falsewith that code.Manual testing
Device tested before and after with REST API calls.
Affected code checklist
Checklist
Overview
Testing
Final pass