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

[Feat] Combine user property updates for network call improvements, fix purchase bug #1444

Merged
merged 15 commits into from
Jun 11, 2024

Conversation

nan-li
Copy link
Contributor

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

Description

One Line Summary

Combine user property updates for network call improvements by combining these updates into one request, and fix a purchase tracking bug.

Details

This PR is best read commit by commit

  1. Session time, count, purchases - these changes are not model driven and now enqueue as a Delta whereas they used to be turned into Request objects immediately
  2. Updated attributed and unattributed time processors to send the session time using the changes above. They will not manage their own background tasks when the app is backgrounding. They will not manage the storage of the user's session time once that time is sent to the User module to handle (except for outcomes which is separate of the user).
  3. Introduce an enum OSPropertiesSupportedProperty, that restricts the properties we allow for updating a user.
  4. Actual changes to combine User Update Deltas into one request
  5. Fix a bug in purchase tracking where a number is sent when the server expects a string (amount).
  6. Update existing tests after the above changes and add tests.

Motivation

Reduce server load.

Scope

  • Individual OSDeltas' updates are combined into one payload and request.
  • Other than that, Deltas and Requests are still handled and cached the same.
  • Previously, a single change was a single request.
  • Now there may be more surface area for request failure with increased payload.
  • Added an enum representing the user property updates that can be made (such as language, session_time, etc) and block against any unsupported ones (mostly for future-proofing).
  • OSDeltas can now be created and enqueued manually, whereas in the past, they were solely driven by model changes.
  • Fixed purchase tracking's price property.

Testing

Unit testing

  • Updated existing unit tests to reflect combining Deltas
  • Added helpers to compare requests and their payload now that the payload can hold more data
  • Added a test in which various user updates are made and confirm they are sent in one payload correctly
  • Added a test in Objective C to test sending purchases

Manual testing

Physical iPhone 13 on iOS 17.4

Tested session ending influences, both attributed and unattributed

  • Confirmed: When app is backgrounded, session time is sent to the Operation Repo before it flushes.

Tested adding tags, purchases, and backgrounding app to get session time

  • Sample request payload sent
{
  "deltas" : {
    "session_time" : 32,
    "purchases" : [
      {
        "amount" : "3.00",
        "sku" : "productSku1",
        "iso" : "EUR"
      },
      {
        "iso" : "USD",
        "sku" : "productSku2",
        "amount" : "3.05"
      }
    ]
  },
  "properties" : {
    "tags" : {
      "c" : "c",
      "g" : "g",
      "h" : "h",
      "b" : "b",
      "e" : "e",
      "d" : "d"
    }
  },
  "refresh_device_metadata" : false
}

Tested logging into users and making tags updates

  • Saw they are batched together for each user
OneSignal.login(userA)
OneSignal.User.addTag(key: "a", value: "a")
OneSignal.User.addTag(key: "b", value: "b")
OneSignal.User.addTag(key: "c", value: "c")
OneSignal.login(userB)
OneSignal.User.addTag(key: "d", value: "d")

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 13 commits May 24, 2024 10:05
* session_count, session_time, and purchases are not model-driven. We were turning their data into `OSRequestUpdateProperties` instances immediately instead of enqueueing as Deltas first. This means they could not be combined with other User Updates in a single request.
* Now, when they will send their data to the User Manager, it will manually enqueue a Delta onto the Operation Repo.

* Previously, we used a flag to indicate if we should send these requests immediately (meaning the app is backgrounded), but that is no longer needed because the Deltas are enqueued to the Operation Repo, and then the Operation Repo has its own background task to flush.
* The time processor will not handle its own background task.
* The ordering will be correct when the app is backgrounded. This time is sent to the user manager who enqueues a delta with the session time. Then the Operation Repo will start its background task and flush deltas.
* The attributed time processor needs to send data to two separate places: session_time for Update User and session time for Outcomes.
* Before sending data to outcomes, it needs to wait 30 seconds to see if the app re-opens in case the SDK will continue on the same session, this is for the outcomes endpoint to know session count for influences.
* It will now send session_time to the user manager immediately as this does not need to wait for 30 seconds. The ordering will be correct when the app is backgrounded. This time is sent to the user manager who enqueues a delta with the session time. Then the Operation Repo will start its background task and flush deltas.
* It will only send the elapsed time and does not need to handle unsent active time. This is because previous session times will be their own Delta instances that the operation repo / executor handles with retrying. These previous deltas already encompass the unsent active time.
* The Attributed Time Processor still has a separate background task that handles sending the outcome, manages the 30 seconds wait time, and maintains a background task for it. It uses unsent active time and resets to 0 when the request is successful.
* OSDeltas can be created by Model changes and now manually created by the User Manager to enqueue updates not driven by model changes. These are for session time, session count, and purchases. In the latter scenario, the User Manager can set a value of "session_time" for example, but could potentially set any random string.
* Introduce an enum OSPropertiesSupportedProperty, that restricts the properties we allow for updating a user.
* When processing the Delta queue and turning them into requests, we now combine deltas into one User Update request.
* Also noted that we enqueue only one Properties Delta change at a time - for example, one of session_time, session_count, or purchases - so the struct OSPropertiesDeltas is not needed. It was always being created with only 1 property and the rest being nil.
* Now that these properties deltas are going to be combined together, we are not using this struct anymore anyways.
* SKProduct.price is an NSDecimalNumber and we had been sending this as is, but the backend expects a String
* This can be reproduced manually by a creating purchase array and calling `OneSignalUserManagerImpl sendPurchases`
* It returns a 400 with: "Field deltas.purchases.amount was expecting value of type 'string', received value of type 'number' instead"
* Purchases are tracked in Objective-C and data is passed to the user manager to send purchases.
* It is more accurate to keep purchase tracking tests in Objective-C as well as passing data can be a source of errors
* There is a need to compare two User Update Requests, with limitations of comparing Objc dictionaries to Swift dictionaries, and the Mock Client not knowing about product-specific OneSignalRequest implementations like OSRequestUpdateUser.
* So, we can do comparison via their parameters payload.
* Add logic to stringify the params in alphabetical order to allow streamlined comparison
* When a tag and a language are added, they are now set in 1 request containing both updates instead of 2 requests
* Add a test where many user properties updates are made and check they are all sent in one payload correctly
* Update an equality checker helper method to handle testing float equality, needed for checking lat and long in the payload.
* Some tests require more time for the user updates to enqueue before the flush
@nan-li nan-li requested review from jkasten2, emawby, jinliu9508, shepherd-l and jennantilla and removed request for jkasten2 May 29, 2024 16:28
@emawby emawby self-assigned this Jun 4, 2024
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 23 files reviewed, 2 unresolved discussions (waiting on @jennantilla, @jinliu9508, @jkasten2, @nan-li, and @shepherd-l)


iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift line 559 at r1 (raw file):

    /// 
    /// - Parameter property:Expected inputs are `"session_time"`, `"session_count"`, and `"purchases"`.
    func updatePropertiesDeltas(property: String, value: Any) {

Should the property parameter be of type OSPropertySupportedProperty instead of a String to ensure a supported value is being sent? The OSPropertySupportedProperty enum could have an associated string value that we use for creating the delta


iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/Support/SupportedProperty.swift line 0 at r1 (raw file):
This file name should match the enum file name and maybe shouldn't be under the executor folder since it is used by the model store listener as well.

* Rename `SupportedProperty.swift` to `OSPropertiesSupportedProperty.swift` and move to a higher-up folder in the user module
* Simplify a piece of logic for combining properties by removing unnecessary intermediate step
* Change a parameter type to an enum for more clarity and control
@nan-li nan-li requested a review from emawby June 11, 2024 15:18
@nan-li nan-li changed the title [Feat] Combine user property updates for network call improvements [Feat] Combine user property updates for network call improvements, fix purchase bug Jun 11, 2024
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

LGTM! The unit tests are flaky but lets address in a different pr

@nan-li nan-li merged commit 67a455d into main Jun 11, 2024
4 of 5 checks passed
@nan-li nan-li deleted the feat/combine_deltas branch June 11, 2024 22:53
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