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

[Fix] Don't evaluate in app messages when paused #1524

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

nan-li
Copy link
Contributor

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

Description

One Line Summary

Don't evaluate in app messages when paused to improve accuracy once they are unpaused.

Details

Motivation

A SDK consumer reached out with the following issue:

  1. Set up a regular IAM and a duration since last IAM
  2. Pause IAMs and open app
  3. Now unpause IAMs
  4. The first IAM will show, then the duration IAM will show immediately after, ignoring the duration
  • This is caused by message evaluation after fetching.
  • Since no IAM has shown yet in the paused state, the duration IAM passes the duration check and is queued for display
  • Thus, evaluating IAMs while paused will lead to inaccuracies once IAMs are unpaused.
  • In this case, duration-since-last IAMs will be evaluated incorrectly and then queue for display once unpaused.
  • When IAMs are unpaused, IAM evaluation is re-triggered anyway and will queue the messages for display at that time. There is no need to evaluate while paused.

Scope

More accurate In App Message evaluation

Testing

Unit testing

Will be part of a subsequent task to build IAM tests.

Manual testing

iPhone simulator 18.1

  • Reproduced issue with 2 IAMs (a display on app open, and a 30 second duration since last)
  • In app messaging starts out paused
  • Then unpause
  • First IAM should display then 30 seconds later the next one should display

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

* Evaluating IAMs while paused will lead to inaccuracies once IAMs are unpaused.
* For example, duration-since-last IAMs will be evaluated incorrectly and then queue for display once unpaused.
* When IAMs are unpaused, IAM evaluation is re-triggered anyway and will queue the messages for display at that time.
@nan-li
Copy link
Contributor Author

nan-li commented Dec 4, 2024

Here is where evaluateMessages is called when IAMs are unpaused.

@jinliu9508
Copy link

Logic looks good to fix this issue. Just curious about a scenario when there is a condition to show IAM when either a trigger is satisfied OR duration > 30 seconds. It looks like the trigger won't work when IAM is paused. Is this what we expect?

@nan-li
Copy link
Contributor Author

nan-li commented Dec 4, 2024

Logic looks good to fix this issue. Just curious about a scenario when there is a condition to show IAM when either a trigger is satisfied OR duration > 30 seconds. It looks like the trigger won't work when IAM is paused. Is this what we expect?

I think this is the expected behavior, since we don't show any IAMs while in the paused state. Are you saying maybe it won't work if IAMs are then unpaused?

@nan-li nan-li merged commit f08811e into main Dec 16, 2024
3 of 4 checks passed
@nan-li nan-li deleted the fix/iam_duration_since_last branch December 16, 2024 23: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.

3 participants