Skip to content

[5.0.0] Combine less operations with the create user operation #1794

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.

Sign up for GitHub

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

Jump to bottom
Merged
emawby merged 2 commits into from
Aug 10, 2023
Merged

[5.0.0] Combine less operations with the create user operation #1794

emawby merged 2 commits into from
Aug 10, 2023

Conversation

@nan-li
Copy link
Contributor

@nan-li nan-li commented Aug 7, 2023 •
edited
Loading

Description

One Line Summary

Only send enqueued subscription operations with the CreateUser operation, and not tags, aliases, and properties operations.

Details

Motivation

Modify the create user operation to send less associated info. Let's send less data into a create user request to minimize chances that other information in the request payload causes the create user request to fail.

  • At time of writing, known failure cases are subscriptions over limit or tags over limit.
  • We won't combine alias, tags, and properties operations to the create user operation.
  • Ideally we only want to combine the push subscription operations and not any sms or email operations to the create user operation, but that will require more changes to the SDK, as currently all subscriptions are treated the same by the model / model store / operation repo infrastructure.
  • For now, we will send any subscription operations with the create user operation, but no tags, aliases, or properties. It is unlikely that many email or sms subscription operations will be enqueued before the create user request happens.

Scope

  • The create user request will now only have subscription related updates in the payload.
  • Tag, alias, and properties operations will stay in the operation repo and be sent separately.

Testing

Unit testing

  • No new unit tests are added but existing test are updated after these changes.
  • Should consider testing the changes in this PR

Manual testing

Tested on Pixel emulator on API 30.

  • Both of the below scenarios led to a bad SDK user state prior to this PR

New app install creating a guest user

  1. New app install without wifi
  2. Add many tags
  3. Turn on wifi
  4. Create user request is sent with the push subscription, and succeeds
  5. The SDK has a user with a onesignal_id and a push subscription with an id
  6. Then the tags are sent and fails
  7. Note that tags still exist on the properties model even though they are not updated to the server but they will be refreshed on a new session.

When the SDK has an identified user, login to another user

  1. The SDK has a user with an external_id
  2. Turn off wifi
  3. Call login to to different external_id and then add many tags
  4. Turn on wifi
  5. The create user request is sent with the push subscription, and succeeds
  6. Then the tags are sent and fails

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 added 2 commits August 7, 2023 03:46
@nan-li
Modify create user operation to send less associated info
f1d204e 
* Let's send less data into a create user request to minimize changes that other information in the request payload causes the create user request to fail.
* At time of writing, common failure cases are subscriptions over limit or tags over limit.
* We won't combine alias, tags, and properties operations to the create user operation.
* Ideally we only want to combine the push subscription operations and not any sms or email operations to the create user operation, but it will require more changes to the SDK.
* For now, we will send any subscription operations with the create user operation, but no tags, aliases, or properties.
@nan-li
Fix tests after create user operation changes
da91c6a 
* Delete test `create user with an alias and properties creates a new user`
* change arguments to `createUser()` after the `properties` parameter is removed
Base automatically changed from user_model/fix_outcome_requests to user-model/main August 10, 2023 03:46
@emawby emawby merged commit 67c2c52 into user-model/main Aug 10, 2023
@emawby emawby deleted the user_model/improve_create_user_erroring branch August 10, 2023 16:31
@nan-li nan-li mentioned this pull request Aug 10, 2023
Merged
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
@emawby @jinliu9508
Merge pull request #1794 from OneSignal/user_model/improve_create_use…
170448d 
…r_erroring

[5.0.0] Combine less operations with the create user operation
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
@emawby @jinliu9508
Merge pull request #1794 from OneSignal/user_model/improve_create_use…
5c50d77 
…r_erroring

[5.0.0] Combine less operations with the create user operation
jinliu9508 pushed a commit that referenced this pull request Feb 6, 2024
@emawby @jinliu9508
Merge pull request #1794 from OneSignal/user_model/improve_create_use…
5bc8415 
…r_erroring

[5.0.0] Combine less operations with the create user operation
jkasten2 added a commit that referenced this pull request Feb 15, 2024
@jkasten2
fixed create user test to ensure we omit aliases
8f924a7 
PR #1794 made changes to omit aliases on user create, but the test was
never updated. Also the SubscriptionStatus arg part of this test wasn't
compare the corret types, causing it to fail the test.
jkasten2 added a commit that referenced this pull request Feb 21, 2024
@jkasten2
fixed create user test to ensure we omit aliases
6ddcaf9 
PR #1794 made changes to omit aliases on user create, but the test was
never updated. Also the SubscriptionStatus arg part of this test wasn't
compare the corret types, causing it to fail the test.
jkasten2 added a commit that referenced this pull request Apr 8, 2024
@jkasten2
fix missing modify keys on some operations
7fe2010 
All operations must have at least a create or modify key, this is
required so operations are not grouped together when they should not be.

This fixes a bug where calling addTag() back-to-back with login() would
cause the login to be dropped. This is because
SetTagOperation.modifyComparisonKey was "" which matched
LoginUserOperation.modifyComparisonKey as it was also "" and both these
operations we returned by OperationRepo.getGroupableOperations. Both
operations were given to UpdateUserOperationExecutor which it would skip
the LoginUserOperation silently.

This mistake originated from f1d204e
which wasn't in the 5.0.0-beta4 release, but was introduced in 5.0.0.
The intent of PR #1794 is still preserved in this commit, we just simply
fixed the bug noted above.
@jkasten2 jkasten2 mentioned this pull request Apr 8, 2024
Merged
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkasten2 jkasten2 Awaiting requested review from jkasten2

@shepherd-l shepherd-l Awaiting requested review from shepherd-l

@jennantilla jennantilla Awaiting requested review from jennantilla

1 more reviewer

@emawby emawby emawby approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nan-li @emawby