Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Fix processing of Identify User response if the user has changed since then #1427

Merged
merged 9 commits into from
May 10, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented May 6, 2024

Description

One Line Summary

Fix processing of Identify User response when the user has changed since, so that pending updates will still be sent and transfer subscription happens only in correct situations.

Details

Note that this bug only manifests if multiple logins to different users are called one after another, or the requests are not able to succeed (server is down, or there is no internet connection during which time multiple logins are called). This PR tests and addresses those scenarios.

Motivation

Consider the scenario Anon -> Login User A (Identify User) -> Login User B (Create User)

  • Dropping user updates
    • When the Identify User request returns, because the user is now different, we were not fetching User A to hydrate the OSID, so any updates made to it get dropped. Now, we mark the Identity Model to use the external_id for pending requests.
  • Transferring push subscription incorrectly
    • In addition, transfer subscription had a bug. Create User B already contains the push subscription in the payload, but when Identify User A returns, we still enqueue a Transfer Subscription to User A request, which would create this flow: Anon -> Login User A (Identify User) -> Login User B (Create User) -> Transfer Subscription to User A
    • Now we only transfer if it's still the same user. Any Create User requests would already have the push subscription in it.
    • This was a recoverable bug as the next Fetch User B call would find the push subscription gone, and the SDK would call Create Subscription for User B with the push subscription data.

This PR also fixes the bugs mentioned under Manual Testing section for #1418

User State Change Observer

⚠️ The User Observer was never triggered for the 409'd middle user. That is still the case after this PR, as we never hydrate its onesignal_id.

Scope

Testing

Unit testing

Manual testing

Tested on iPhone 13 with iOS 17.4

✅ Scenario 1:

  1. On new app install after initialization, immediately call these methods.
  2. Confirm tags are sent to <some_existing_euid> by way of external_id
// New App Install

[OneSignal initialize:<app_id> withLaunchOptions:launchOptions];

[OneSignal login:<some_existing_euid>]; // returns 409
[OneSignal.User addTagWithKey:@"a" value:@"a"];
[OneSignal login:<user_b>];

✅ Scenario 2:

  1. On new app install after initialization, immediately call these methods.
  2. Confirm every update is sent and is sent to the correct user.
// New App Install

[OneSignal initialize:<app_id> withLaunchOptions:launchOptions];

[OneSignal.User addTagWithKey:@"a" value:@"a"];
[OneSignal.User setLanguage:@"fr"];
[OneSignal.User addAliasWithLabel:@"a" id:@"a"];
[OneSignal.User addEmail:@"a@example.com"];

[OneSignal login:<some_existing_euid>];
[OneSignal.User addAliasWithLabel:@"existing" id:@"existing"];
[OneSignal.User addTagWithKey:@"existing" value:@"existing"];
[OneSignal.User setLanguage:@"zh"];
[OneSignal.User addEmail:@"existing@example.com"];

[OneSignal logout];
[OneSignal.User addTagWithKey:@"c" value:@"c"];
[OneSignal.User setLanguage:@"es"];
[OneSignal.User addEmail:@"c@example.com"];
[OneSignal.User addAliasWithLabel:@"anonymous" id:@"anonymous"];

✅ Scenario 3:

  1. Check everything is correct when the current user is still the same as the Identified User (no regression).
  2. On new app install after initialization, immediately call these methods.
  3. Confirm every update is sent and is sent to the correct user.
  4. The user in the SDK is correctly hydrated.
// New App Install

[OneSignal initialize:<app_id> withLaunchOptions:launchOptions];

[OneSignal.User addTagWithKey:@"a" value:@"a"];
[OneSignal.User setLanguage:@"fr"];
[OneSignal.User addAliasWithLabel:@"a" id:@"a"];
[OneSignal.User addEmail:@"a@example.com"];

[OneSignal login:<some_existing_euid>];
[OneSignal.User addAliasWithLabel:@"existing" id:@"existing"];
[OneSignal.User addTagWithKey:@"existing" value:@"existing"];
[OneSignal.User setLanguage:@"zh"];
[OneSignal.User addEmail:@"existing@example.com"];

// Don't logout or login to another user

✅ Scenario 4:

  1. Turn off wifi and data (no network connection)
  2. New app install and call the following methods
// New App Install

[OneSignal initialize:<app_id> withLaunchOptions:launchOptions];
[OneSignal login:<some_existing_euid>];
[OneSignal.User addTagWithKey:@"a" value:@"a"];
[OneSignal login:<user_b>];

// No requests go through
  1. Wait 30 seconds and kill app
  2. Turn on wifi and reopen app
  3. Requests go to the correct users.

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
  • Any Public API changes are explained in the PR details and conform to existing APIs - none

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.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li changed the title [Bug] Fix processing of Identify User response if the user has changed since then WIP - [Bug] Fix processing of Identify User response if the user has changed since then May 7, 2024
@nan-li nan-li force-pushed the fix/identify_user_bug_when_in_the_middle branch from ecd010b to 46edda1 Compare May 8, 2024 02:02
@nan-li nan-li changed the title WIP - [Bug] Fix processing of Identify User response if the user has changed since then [Bug] Fix processing of Identify User response if the user has changed since then May 8, 2024
@nan-li nan-li force-pushed the fix/identify_user_bug_when_in_the_middle branch from 46edda1 to f214e69 Compare May 8, 2024 02:17
nan-li added 4 commits May 8, 2024 14:01
* This is more realistic as real requests have a delay
* This change was motivated by this flow: anon -> login (identify user) -> login (create user)
  * When the requests ran synchronously, when the identify user request returned, the "current user" was still itself.
* Create some default helper methods
* Before the fix in this PR, this test would fail because tags for user A do not get sent
@nan-li nan-li force-pushed the fix/identify_user_bug_when_in_the_middle branch from f214e69 to 6cdedc3 Compare May 8, 2024 21:35
nan-li added 5 commits May 8, 2024 15:12
* OneSignalUserMocks needed a dependency on XCTest apparently
* Because it depends on OneSignalCoreMocks now, who uses XCTest
* Fix an assert about where a set of tags is sent
  * This test previously succeeded because all the requests are processed synchronously, but after we introduced async client requests, this test failed because it was not the correct actual behavior
* Also use the new client response setting helpers (no logic change)
* Motivation: consider the scenario Anon -> Login (Identify User) -> Make Updates like Tags -> Login (Create User)
* When the Identify User request returns, because the user is now different, we were not hydrating the previous user's OSID, so updates made to it were dropped. Now we still fetch the user to hydrate the OSID.
* In addition, transfer subscription had a bug. What would happen is after the last Create User is sent with the device's push subscription in the payload, the Transfer Subscription request would come next and move it back to the middle user. Now we only transfer if it's still the same user. Any Create User requests after would already have the push subscription in it.
* See the URL that the response was for
* When Identify User returns 409, there may be pending requests for that user.
* Avoid another user fetch just to hydrate the OSID, and instead use the external_id that we have for this user to send those pending requests.
* Also, the `fetchUser` is meant only for the current user, as it clears local state for hydration. We avoid refactoring that method as well.
* Add new concept of "default alias" that lives on an Identity Model and indicates what alias should be used for requests.
@nan-li nan-li force-pushed the fix/identify_user_bug_when_in_the_middle branch from 6cdedc3 to 9f156d7 Compare May 8, 2024 22:13
@nan-li
Copy link
Contributor Author

nan-li commented May 9, 2024

Followup PR #1430 testing user switching is also testing these changes.

Note CodeQL is broken after push to start live activities merged to main, its autobuild action no longer works.

@emawby emawby self-assigned this May 10, 2024
@nan-li nan-li merged commit 445b5d2 into main May 10, 2024
3 of 4 checks passed
@nan-li nan-li deleted the fix/identify_user_bug_when_in_the_middle branch May 10, 2024 20:44
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.

None yet

2 participants