Skip to content

Conversation

@jinliu9508
Copy link
Contributor

@jinliu9508 jinliu9508 commented Jun 26, 2025

Description

One Line Summary

Fixes a bug where the subscription status is lost when a subscription is deleted via the OneSignal dashboard after 'login()' is called.

Details

Motivation

There is a use case where inactive subscriptions are manually removed from the dashboard. If the SDK subsequently calls 'login()' or 'logout()' after such deletion, a new subscription is created, but its status defaults to 'NEVER_SUBSCRIBE' — even though the user had previously subscribed. We expect the newly created subscription to inherit the setup and subscription state of the original.

OTHER - Root Cause Explained

When 'logout()' is called, the SDK sends a 'login-user' request that includes a 'transfer-subscription' operation. Normally, the backend returns a subscription that matches what is stored locally. However, if the original subscription has been deleted from the dashboard, the backend instead returns a newly created default subscription, which lacks prior subscription attributes such as the subscription status.

Scope

The 'TransferSubscription' operation is now only added when an 'externalID' is present.

  • If 'externalID' is not present (i.e., during a 'logout()' or first-time 'login()' as an anonymous user), we now use a 'CreateSubscription' operation instead. This will also be consistent with the behavior in our iOS SDK.
  • This change prevents the system from attempting to transfer a deleted subscription, which would otherwise result in a new subscription without inherited subscription state.

Testing

Unit testing

Not feasible, as the process involves manual deletion via the dashboard, which cannot be automated in unit tests.

Manual testing

  • Test Case 1 – 'subscribe()' → 'login()' → delete subscription → 'logout()'

    • Expected: A new subscription is created after 'logout()' that retains the original subscription state, including the subscription status.
    • Actual: Matches expected behavior.
  • Test Case 2 – 'subscribe()' → 'login()' → 'logout()' → 'login()' (same 'externalID')

    • After 'logout()', the 'externalID' is successfully cleared on the dashboard. The push status remains unchanged.
    • On re-login with the same 'externalID', it is correctly re-associated with the subscription.
  • Test Case 3 – 'subscribe()' → 'login()' → 'logout()' → 'login()' (different 'externalID')

    • The SDK successfully transfers the subscription to the new 'externalID'.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left two comments to address on specific lines of code.

It would be beneficial to add tests for the code, however since there are pretty significant changes around this code in the 5.2.0 branch it would be a better fit there, outside of this PR.

@jinliu9508 jinliu9508 requested a review from jkasten2 June 27, 2025 15:42
@jinliu9508 jinliu9508 force-pushed the subscription-status-not-updated branch from 47257e3 to f208562 Compare June 27, 2025 17:50
@jinliu9508
Copy link
Contributor Author

Squashed and merging...

@jinliu9508 jinliu9508 merged commit 29314f2 into main Jun 27, 2025
2 checks passed
@jinliu9508 jinliu9508 deleted the subscription-status-not-updated branch June 27, 2025 19:06
@jkasten2 jkasten2 mentioned this pull request Jun 30, 2025
nan-li pushed a commit that referenced this pull request Jul 8, 2025
fix: logout incorrectly uses the old subscription ID
nan-li pushed a commit that referenced this pull request Jul 8, 2025
fix: logout incorrectly uses the old subscription ID
@nan-li nan-li mentioned this pull request Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants