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

[5.0.0] Fix outcome requests #1793

Merged
merged 6 commits into from
Aug 10, 2023
Merged

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Aug 7, 2023

Description

One Line Summary

Fix outcome request payload to include device_type and session_time (if the outcome type is os__session_duration).

Details

Motivation

The outcomes endpoint expects device_type to be in the request body but we were not sending it, and we receive a 400 error in response. Now, we will always send type in the subscription data.

{
    "app_id": "YOUR_APP_ID",
    "onesignal_id": "YOUR_ONESIGNAL_ID",
    "subscription": {
        "id": "YOUR_PUSH_SUBSCRIPTION_ID",
        "type": "AndroidPush"
    },
    "id": "some_outcome_value"
}

The second problem this PR addresses is for influenced session ending outcomes, the server expects a non-zero value for session_time to be in the request body if the outcome has an ID of os__session_duration. However, we were not sending session_time and receive a 400 error response "There was a problem in the JSON you submitted","\"session_time\" must be positive when submitting os__session_duration". Now, we sill send an os__session_duration outcome with session_time if the session that just ended is attributed.

Another detail is that failed outcome requests keep the outcome in the cache (database) so that they are re-sent on the next app start.

Note that before, we were sending a os__session_duration outcome with no influences when a session that just ended is unattributed. We will no longer do this, but these outcomes may be cached and need to be addressed.

The session_time that we were sending to update user endpoint was actually in milliseconds, and this is fixed in this PR to be seconds, which is what the server expects.

To make the minimal amount of changes, I added a session_time property to OutcomeEvent and a new column to the outcomes table. Not all outcomes will utilize session_time if these are addOutcome methods.

Scope

  • Database table for outcomes is updated, and a migration occurs to add a new column called session_time.
  • All cached outcomes after database migration will send with a session_time of 1 just to get them successfully sent and done with, even though this session_time may not be accurate. Otherwise, they may keep staying in the cache and retrying.

Details about Commits Below

1. Add device type to the outcome event request

2. Add session_time to OutcomeEvent

  • Add sessionTime as an additional property to an OutcomeEvent object
  • This is for the influenced session ending outcomes that send session_time to the outcomes endpoint
  • For OutcomeEventParams constructor, don't make the parameter timestamp optional, so not to confuse with other parameters when instantiating an instance

3. Send influenced session outcome

  • Create separate outcome-related method sendSessionEndOutcomeEvent
  • We were also sending session_time in ms, but it should be seconds.

4. update outcomes table to include session_time column

  • Update database version to v9 and make migration.
  • I default this new session_time to a value of 1 instead of 0 intentionally to work around an issue from the v5.0.0-beta's for migration's sake and get these outcomes migrated, sent, and done with.
  • When outcome requests fail, they are cached for retrying. The v5.0.0-beta's likely encountered failure responses and cached these. If there are cached os__session_duration outcomes and we send them off with zero session_time, the server will keep sending us a failure response. So, let's just send a time of 1 second.

Testing

Unit testing

  • No new tests are added, but constructors in existing tests are updated after these outcomes changes.
  • Should consider adding tests for these in future

Manual testing

Tested on Pixel emulator on API 30.

  1. Run app without the changes in this PR
  2. Add an outcome
  3. Background the app after using for at least 60 seconds
  4. Click on a notification -> app opens and use the app
  5. Background the app
  6. See 3 outcome requests send, fail, and 3 outcomes get added to the outcomes table: (1) the outcome that is manually added, (2) the outcome for the unattributed session, (3) the outcome for the attributed session
  7. Run the app with the changes in this PR
  8. See the db upgrade happen
  9. See 3 outcome requests send with session_time of 1 added, successful response, and be removed from the outcomes table

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

* Add `sessionTime` as an additional property to an `OutcomeEvent` object
* This is for the influenced session ending outcomes that send session_time to the outcomes endpoint
* For `OutcomeEventParams` constructor, don't make the parameter `timestamp` optional, so not to confuse with other parameters when instantiating an instance
* Create separate outcome-related method `sendSessionEndOutcomeEvent`
* We were also sending session_time in ms, but it should be seconds.
* Update database version to v9 and make migration.
* I default this new `session_time` to a value of `1` instead of `0` intentionally to work around an issue from the v5.0.0-beta's for migration's sake and get these outcomes migrated, sent, and done with.
* When outcome requests fail, they are cached for retrying. The v5.0.0-beta's likely encountered failure responses and cached these. If there are cached `os__session_duration` outcomes and we send them off with zero session_time, the server will keep sending us a failure response. So, let's just send a time of 1 second.
@@ -7,7 +7,8 @@ internal class OutcomeEventParams constructor(
val outcomeId: String,
val outcomeSource: OutcomeSource?, // This field is optional
var weight: Float, // This field is optional.
var timestamp: Long = 0,
var sessionTime: Long, // This field is optional
var timestamp: Long,
Copy link
Contributor

Choose a reason for hiding this comment

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

timestamp previously set its default value to 0. Was removing this default intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was intentional, it was briefly mentioned in a commit but here is more information about why I changed it.

The SDK actually always passed in a value for timestamp to the OutcomeEventParams constructor, so this default was not needed. In addition, it was not to confuse with the new sessionTime parameter. After adding sessionTime to the constructor, the app and unit tests built because the argument we used to pass for timestamp is now interpreted as sessionTime and sessionTime started defaulting to 0.

But I think it is a good note what this parameter means, that it starts out 0 until it fails and becomes cached.

@emawby emawby self-requested a review August 7, 2023 18:17
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.

Reviewed 7 of 8 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jennantilla, @jkasten2, and @shepherd-l)

* When an outcome is created, its `timestamp` should start out as zero.
* When the outcome request is sent and fails, the SDK will save and retry the outcome, then it will save the current `timestamp` for future sending
@emawby emawby merged commit 977130c into user-model/main Aug 10, 2023
1 check failed
@emawby emawby deleted the user_model/fix_outcome_requests branch August 10, 2023 03:46
@nan-li nan-li mentioned this pull request Aug 10, 2023
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
jinliu9508 pushed a commit that referenced this pull request Feb 6, 2024
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