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] Some pending properties can be sent to new user incorrectly, when users change #1418

Merged
merged 13 commits into from
Apr 30, 2024

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Apr 25, 2024

Description

One Line Summary

Fixes bug that when users change, user updates like tags for the previous user can be sent incorrectly to the new user.

Details

This PR has two main changes:

  1. OSDelta will now have an ID of the Identity Model it is meant for. Previously, user updates such as add tags and adding email / sms subscriptions did not have any reference to the Identity Model. As a result, when the OSDelta is processed into Requests to send to the server, we attach them to the current user. However, since the Operation Repo and Property Executor use a dispatch queue, the user may have switched by then.

  2. Use a central repo to hold Identity Model instances that are being used during an app session. On subsequent cold starts, when Deltas and Requests are uncached, they will go through the repo to hook up the identity model instances so they are all operating on, and updating, the same instance. If their identity model does not exist yet, it will be added to the repo.

Motivation

Bug fix

Scope

  • OSDelta has a new field containing the ID of the identity model it is meant for
  • On the first cold start after the upgrade: The old OSDeltas will be dropped since they will be missing the Identity Model ID
    • It is unlikely for Deltas to still exist as we flush them into Requests when users change or app backgrounding
  • Add a lightweight Identity Model Repo to manage instances that are still being used
    • An alternative considered is refactoring the Identity Model Store and would require it to basically override all of the OSModelStore methods
  • Remove unused property: The Update User requests were holding on to a property model reference and this was unused and not needed, so it has been removed.

Testing

Unit testing

Manual testing

iPhone 13 on iOS 17.4

Scenario 1:

  1. Reproduced the bug by calling addTag() and immediately logging into another user. The tag went to the new user.
  2. After this PR, the tag goes to the correct user.

Scenario 2:

  1. Prerequisite: Already logged into an identified user, <user_a>
  2. Call these methods in immediate succession
  3. There are 3 users involved. Confirmed all data is sent to the correct user.
// Current user in SDK is <user_a>

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

[OneSignal login:<user_b>];
[OneSignal.User addAliasWithLabel:@"b" id:@"b"];
[OneSignal.User addTagWithKey:@"b" value:@"b"];
[OneSignal.User setLanguage:@"zh"];
[OneSignal.User addEmail:@"b@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. Same as Scenario 2, but those methods are now called on a new app install after initialization.
  2. The SDK forms the requests correctly, and they are sent to the correct user.
  3. However, a bug was found when anon -> login -> logout / login are called in succession. The middle user's data (who underwent Identify User) gets dropped. This bug is not related to the changes in this PR, so the fix will be in an upcoming PR.
// 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:<user_b>];
[OneSignal.User addAliasWithLabel:@"b" id:@"b"];
[OneSignal.User addTagWithKey:@"b" value:@"b"];
[OneSignal.User setLanguage:@"zh"];
[OneSignal.User addEmail:@"b@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 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:<user_a>];
[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. See requests uncache and models are connected through the Identity Model Repo
  4. Requests go to the correct users but also suffers from the bug mentioned above.

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

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 base branch from main to fix/user_module_user_defaults_crashes April 25, 2024 21:13
@nan-li nan-li force-pushed the use_central_identity_model_repo branch from 3e2359e to 06b672e Compare April 26, 2024 05:03
@nan-li nan-li changed the title WIP [Bug] Some pending properties can be sent to new user incorrectly, when users change [Bug] Some pending properties can be sent to new user incorrectly, when users change Apr 26, 2024
@nan-li nan-li force-pushed the use_central_identity_model_repo branch from 72f8c69 to 06b672e Compare April 26, 2024 05:47
@nan-li nan-li changed the base branch from fix/user_module_user_defaults_crashes to main April 26, 2024 17:01
@nan-li nan-li changed the base branch from main to fix/user_module_user_defaults_crashes April 26, 2024 17:01
@nan-li nan-li force-pushed the use_central_identity_model_repo branch 4 times, most recently from a01efe3 to 382ada3 Compare April 29, 2024 07:14
@nan-li
Copy link
Contributor Author

nan-li commented Apr 29, 2024

I pushed some changes for testing, reproducing the behavior in [tests] Add test for switching users with adding tags where the test fails before (both tags are sent to UserB), and passes after.

@nan-li nan-li force-pushed the use_central_identity_model_repo branch from 382ada3 to 3f8c676 Compare April 29, 2024 07:24
Base automatically changed from fix/user_module_user_defaults_crashes to main April 29, 2024 17:23
* We don't need the reference to the property model for an Update User request, and this reference has gone unused so far
* We don't hydrate from the response as the response is not the full user, just the property that was updated.
* The request still has a reference to the identity model it is operating on
* OneSignalCoreMocks now has XCTest dependency
* It uses a waiter to allow the test to wait for any async calls to run, because we don't have control over all threads. For example, a test may need to wait for operation repo to run everything in the dispatch queue
* We will need to know if a particular payload exists in a dictionary to confirm what requests are sending
* Adds additional property to track if all requests have been handled by mock responses set
* Adds method to use in asserts to confirm if there is only one executed request that contains the payload provided, and the url matches the path provided.f
* Change OneSignalUserTests build configs to "Test"
* Add OS_TEST preprocessor macro (Test) to OneSignalOSCore which is needed for the reduced flush interval
* Starts with anon user, log into 2 users via Identify User and Create User
* Add tags to each user
* The model on a Delta for property change is the properties model. However, the identity is needed to send the request.
* We have been setting the identity model as the current user's when these Deltas become Requests.
* However, the timing can be buggy when we flush Deltas when users change. The Operation Repo and Property Executor use a dispatch queue so by the time the request is made, the user may have changed.
* Introduce OSIdentityModelRepo managed by the User Manager where a dictionary of identity models live
* This will hold identity models added when users change
* When requests are uncached, their identity models will also be added as they are still being used.
* This is a lightweight option over modifying the Identity Model Store (which is an option if we wish to use the model store to manage all identity models)
* When Requests are uncached in the executors, hook them up to the same identity model instance via the Identity Model Repo
@nan-li nan-li force-pushed the use_central_identity_model_repo branch from 3f8c676 to c4d8f44 Compare April 29, 2024 18:18
@nan-li nan-li force-pushed the use_central_identity_model_repo branch from c4d8f44 to 581f159 Compare April 29, 2024 18:29
@nan-li nan-li force-pushed the use_central_identity_model_repo branch from 6f0d5aa to ef9d2c6 Compare April 29, 2024 20:29
@nan-li
Copy link
Contributor Author

nan-li commented Apr 29, 2024

Tests were failing because some state carried over, added more resets to the User Manager

@nan-li nan-li merged commit 3003044 into main Apr 30, 2024
4 checks passed
@nan-li nan-li deleted the use_central_identity_model_repo branch April 30, 2024 22:39
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