[FIX] IAM with dynamic trigger showing forever #2137

jinliu9508 · 2024-06-27T18:55:20Z

Description

One Line Summary

Fix an issue with IAM with dynamic triggers like "SESSION_TIME" showing up repetitively after being dismissed.

Details

Motivation

Fix the incorrect app behavior when IAM with both custom trigger and session_time trigger continues to show up even it was dismissed.

Scope

This issue happens strictly when both a time based trigger like Session_Time and an in-app trigger of "IS NOT EQUAL" exist with AND. The first time both triggers evaluated to true, IAM shows up, gets dismissed, and will reevaluate the triggers again. In the reevaluation, the system first determines if there is only dynamic trigger. If there is, it forces evaluate to false to suppress the IAM from showing up again. But in this case, another in-app trigger exists and will always evaluate to true due to the IS NOT EQUAL characteristics we currently have, thus a repeating IAM occurs.
This PR alters the logic on when to redisplay message so that we no longer make redisplay messages available to avoid duplicate redisplay of a same message. A message can be redisplayed if at least one trigger has changed or a new trigger is added when a dynamic trigger exists.

Manual testing

Before the fix:
IAM setting to reproduce the issue:

This IAM would show up 5 seconds after app start up and continue to show up after being dismissed.

After the fix:

IAM with whatever IS NOT 5 AND Session_Time > 5 with Every time trigger conditions are satisfied checked:
- Result: This IAM will show up 5 seconds after app start up and will not show up again once dismissed. Whenever I add a new trigger, the IAM shows up again.
IAM with whatever = true AND Session_Time > 5 with Every time trigger conditions are satisfied checked:
- Result: This IAM will not show up initially, but will show up if I add a trigger whatever = true EVERY TIME after 5 seconds of startup. If I add a trigger with an unrelated key, the IAM will not show.
IAM with a single trigger whatever2 = 2
- Result: This IAM will not show up unless a trigger whatever2 = 2 is added. Any value for the same key that is not equals to 2 will not show the IAM.
IAM with a single dynamic trigger Session_Time > 5
- Result: This IAM will only show once 5 seconds after startup.

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
- If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
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.
- Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
I have reviewed this PR myself, ensuring it meets each checklist item
- WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt

...src/main/java/com/onesignal/inAppMessages/internal/triggers/impl/DynamicTriggerController.kt

jkasten2

This latest solution still looks like it would also break the "Every time trigger conditions are satisfied" behavior. We want to keep the behavior where the developer should be able to reshow the IAM by calling addTrigger again with the same key. Can you test this scenario and add what you tested to the PR notes?

jinliu9508 · 2024-07-16T20:21:47Z

This latest solution still looks like it would also break the "Every time trigger conditions are satisfied" behavior. We want to keep the behavior where the developer should be able to reshow the IAM by calling addTrigger again with the same key. Can you test this scenario and add what you tested to the PR notes?

The latest change adds a set that stores evaluated combinations of trigger to prevent a certain combination of trigger from showing multiple time. The set will get cleared every time addTrigger or removeTrigger is called. And it appears in my test that the IAM will show up once per startup and every time I add a new trigger.

jkasten2

This new logic has a lot of overlap with the pre-existing isTriggerChanged and isDisplayedInSession logic. I think we should remove these (if they cover everything) before merging in these changes so we don't have redundant logic that would much harder to reason about later.

...ssages/src/main/java/com/onesignal/inAppMessages/internal/triggers/impl/TriggerController.kt

Summary of changes: 1. When a time-based trigger is evaluated and 'onTriggerCompleted()' is called, we no longer make redisplay messages available to avoid duplicate redisplay of a same message. 2. Modify the logic of 'makeRedisplayMessagesAvailableWithTriggers()' so that a message can be redisplayed only if at least one trigger has changed or a new trigger is added. 3. 'onTriggerConditionChanged()' now accepts a new parameter 'triggerId' that indicates the time-based trigger that fires.

jinliu9508 · 2024-07-18T06:26:54Z

This new logic has a lot of overlap with the pre-existing isTriggerChanged and isDisplayedInSession logic. I think we should remove these (if they cover everything) before merging in these changes so we don't have redundant logic that would much harder to reason about later.

Yes, I also realized that this particular approach uses similar logic as 'isTriggerChanged' so I made a different approach that only alter the 'makeRedisplayMessagesAvailableWithTriggers' logic so that onTriggerCompleted no longer redisplay messages. Instead, it was done by onTriggerConditionChanged before trigger evaluation. I have updated the test part in the description to include some common IAM setup that is potentially affected by the change.

jkasten2

IAM with whatever = true AND Session_Time > 5 with Every time trigger conditions are satisfied checked:

There is still a bug with that IAM in the follow scenario.

Call addTriger("whatever", true)
IAM displays as expected
Call addTriger("somethingElse", true)
BUG: IAM displays again when it should not
- It is expected that it only re-display if addTriger("whatever", true) was called again.

…r is added when there is only dynamic trigger

jinliu9508 · 2024-07-19T05:00:17Z

IAM with whatever = true AND Session_Time > 5 with Every time trigger conditions are satisfied checked:

There is still a bug with that IAM in the follow scenario.

Call addTriger("whatever", true)

IAM displays as expected

Call addTriger("somethingElse", true)

BUG: IAM displays again when it should not

It is expected that it only re-display if addTriger("whatever", true) was called again.

Nice found! I have fixed this bug and removed some unneeded code for the latest commit.

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt

jkasten2

No action needed in this PR, but noted here for the future. I think there is still more states the can be cleaned up to simplify this manager's logic. Some that come to mind are:

I don't think messageHasOnlyDynamicTriggers needs to be a concept
redisplayedInAppMessages overlaps with message.isDisplayedInSession

jkasten2 · 2024-07-24T20:57:07Z

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt

-            if (!message.isTriggerChanged && redisplayedInAppMessages.contains(message) &&
-                _triggerController.isTriggerOnMessage(message, newTriggersKeys)
-            ) {
+            val isMessageDisplayed = redisplayedInAppMessages.contains(message)


isMessageDisplayed is an incorrect name for this, however the pre-existing redisplayedInAppMessages is also a misleading name. A recommend a better name such as canRedisplay instead of isMessageDisplayed.

Ok I just fixed this.

jkasten2 · 2024-07-24T20:58:22Z

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt

+            val isMessageDisplayed = redisplayedInAppMessages.contains(message)
+            val isTriggerOnMessage = _triggerController.isTriggerOnMessage(message, newTriggersKeys)
+            val isOnlyDynamicTriggers = _triggerController.messageHasOnlyDynamicTriggers(message)
+            if (!message.isTriggerChanged && isMessageDisplayed && (isTriggerOnMessage || isNewTriggerAdded && isOnlyDynamicTriggers)) {


This is acceptable for now, however keep in mind for the future ideally we won't want to have an if statement with a nested && and || like this in most scenarios, including this one.

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt

jinliu9508 changed the title ~~WIP: [FIX] IAM with dynamic trigger does not show forever~~ [FIX] IAM with dynamic trigger does not show forever Jun 27, 2024

jinliu9508 requested review from jkasten2, nan-li, jennantilla and shepherd-l June 27, 2024 19:03

jinliu9508 assigned jkasten2 Jun 27, 2024

jkasten2 requested changes Jun 27, 2024

View reviewed changes

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt Outdated Show resolved Hide resolved

jinliu9508 force-pushed the IAM-display-forever branch 2 times, most recently from ba52ee5 to 49be2fc Compare July 1, 2024 17:52

jinliu9508 changed the title ~~[FIX] IAM with dynamic trigger does not show forever~~ [FIX] IAM with dynamic trigger showing forever Jul 1, 2024

jinliu9508 requested a review from jkasten2 July 1, 2024 18:01

jkasten2 requested changes Jul 1, 2024

View reviewed changes

...src/main/java/com/onesignal/inAppMessages/internal/triggers/impl/DynamicTriggerController.kt Outdated Show resolved Hide resolved

...src/main/java/com/onesignal/inAppMessages/internal/triggers/impl/DynamicTriggerController.kt Outdated Show resolved Hide resolved

jinliu9508 force-pushed the IAM-display-forever branch 2 times, most recently from 30dc970 to a3bc16c Compare July 2, 2024 19:55

jinliu9508 requested a review from jkasten2 July 2, 2024 19:58

jkasten2 requested changes Jul 3, 2024

View reviewed changes

jinliu9508 force-pushed the IAM-display-forever branch from a3bc16c to 8b53cc6 Compare July 16, 2024 20:22

jinliu9508 requested a review from jkasten2 July 16, 2024 20:24

jkasten2 requested changes Jul 16, 2024

View reviewed changes

...ssages/src/main/java/com/onesignal/inAppMessages/internal/triggers/impl/TriggerController.kt Outdated Show resolved Hide resolved

jinliu9508 force-pushed the IAM-display-forever branch from 20678ea to 7582adf Compare July 18, 2024 05:25

jinliu9508 requested a review from jkasten2 July 18, 2024 06:27

jkasten2 requested changes Jul 18, 2024

View reviewed changes

jinliu9508 force-pushed the IAM-display-forever branch from 0f42d93 to 66df887 Compare July 19, 2024 04:56

Allow redisplay if either an expected trigger has changed or a trigge…

ea912ff

…r is added when there is only dynamic trigger

jinliu9508 force-pushed the IAM-display-forever branch from 66df887 to ea912ff Compare July 19, 2024 04:58

jkasten2 requested changes Jul 23, 2024

View reviewed changes

...l/in-app-messages/src/main/java/com/onesignal/inAppMessages/internal/InAppMessagesManager.kt Outdated Show resolved Hide resolved

Rephrase if condition for more readability

26a1dfd

jinliu9508 requested a review from jkasten2 July 24, 2024 14:38

jkasten2 approved these changes Jul 24, 2024

View reviewed changes

jinliu9508 merged commit cb37657 into main Jul 25, 2024
2 of 3 checks passed

jinliu9508 deleted the IAM-display-forever branch July 25, 2024 15:40

jinliu9508 mentioned this pull request Jul 26, 2024

Release 5.1.18 #2156

Merged

This was referenced Aug 15, 2024

Release 5.2.4 OneSignal/OneSignal-Cordova-SDK#1013

Merged

Release 5.2.3 OneSignal/OneSignal-Flutter-SDK#933

Merged

Release 5.2.3 OneSignal/react-native-onesignal#1731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] IAM with dynamic trigger showing forever #2137

[FIX] IAM with dynamic trigger showing forever #2137

jinliu9508 commented Jun 27, 2024 •

edited

Loading

jkasten2 left a comment •

edited

Loading

jinliu9508 commented Jul 16, 2024

jkasten2 left a comment

jinliu9508 commented Jul 18, 2024

jkasten2 left a comment

jinliu9508 commented Jul 19, 2024

jkasten2 left a comment

jkasten2 Jul 24, 2024

jinliu9508 Jul 25, 2024

jkasten2 Jul 24, 2024

[FIX] IAM with dynamic trigger showing forever #2137

[FIX] IAM with dynamic trigger showing forever #2137

Conversation

jinliu9508 commented Jun 27, 2024 • edited Loading

Description

One Line Summary

Details

Motivation

Scope

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

jkasten2 left a comment • edited Loading

Choose a reason for hiding this comment

jinliu9508 commented Jul 16, 2024

jkasten2 left a comment

Choose a reason for hiding this comment

jinliu9508 commented Jul 18, 2024

jkasten2 left a comment

Choose a reason for hiding this comment

jinliu9508 commented Jul 19, 2024

jkasten2 left a comment

Choose a reason for hiding this comment

jkasten2 Jul 24, 2024

Choose a reason for hiding this comment

jinliu9508 Jul 25, 2024

Choose a reason for hiding this comment

jkasten2 Jul 24, 2024

Choose a reason for hiding this comment

jinliu9508 commented Jun 27, 2024 •

edited

Loading

jkasten2 left a comment •

edited

Loading