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 IAM display & dismiss bugs #1799

Merged
merged 1 commit into from
Aug 10, 2023
Merged

[5.0.0] Fix IAM display & dismiss bugs #1799

merged 1 commit into from
Aug 10, 2023

Conversation

jennantilla
Copy link
Contributor

@jennantilla jennantilla commented Aug 9, 2023

Description

One Line Summary

This PR addresses two In App Message bugs:

  1. IAM with a custom trigger not displaying after dismissing previous IAM.
    InAppMessagesManager.attemptToShowInAppMessage: There is an IAM currently showing!
  2. messageWasDismissed returning early if !_lifecycleCallback.hasSubscribers

Details

Motivation

Issue 1
Issue is reproducible following these steps:

  1. Create IAM_1 that is triggered On App Open
  2. Create IAM_2 that has a custom trigger
  3. Run app
  4. IAM_1 displays
  5. Dismiss IAM_1
  6. Trigger IAM_2 with addTrigger method
  7. IAM_2 does not display, console logs InAppMessagesManager.attemptToShowInAppMessage: There is an IAM currently showing!

setting inAppMessageIdShowing back to null in attemptToShowInAppMessage allows IAM_2 to successfully display.

Issue 2
This issue was caught during a pair programming session where the early return would interfere with _lifecycleCallback.fireOnMain being called.

Scope

Changes affect displaying and dismissing IAMs, specifically attemptToShowInAppMessage & messageWasDismissed in InAppMessagesManager.

OPTIONAL - Other

OPTIONAL - Feel free to add any other sections or sub-sections that can explain your PR better.

Testing

Manual testing

Tested Issue 1 to ensure that second IAM successfully displays.

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
    • 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 Reviewable

@@ -350,6 +350,7 @@ internal class InAppMessagesManager(
// If there was a message dequeued, display it
if (messageToDisplay != null) {
var result = _displayer.displayMessage(messageToDisplay!!)
_state.inAppMessageIdShowing = null
Copy link
Contributor

Choose a reason for hiding this comment

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

If result isn't null or false that means that we are displaying a message correct? In that case setting the id to null seems wrong.

Remove return if lifecycleCallback.hasSubscribers is false
@emawby emawby merged commit ed80fcc into user-model/main Aug 10, 2023
1 of 2 checks passed
@emawby emawby deleted the fix/iam branch August 10, 2023 16:34
@nan-li nan-li mentioned this pull request Aug 10, 2023
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
[5.0.0] Fix IAM display & dismiss bugs
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
[5.0.0] Fix IAM display & dismiss bugs
jinliu9508 pushed a commit that referenced this pull request Feb 6, 2024
[5.0.0] Fix IAM display & dismiss bugs
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.

2 participants