feat: do not show modal message if change screens and page rule enabled #731
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.
|
levibostian
changed the title
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report between this PR and the main branch |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #731 +/- ##
==========================================
+ Coverage 56.95% 61.30% +4.34%
==========================================
Files 139 140 +1
Lines 3880 3962 +82
==========================================
+ Hits 2210 2429 +219
+ Misses 1670 1533 -137 ☔ View full report in Codecov by Sentry. |
Shahroz16
requested changes
Jun 3, 2024
There was a problem hiding this comment.
Before I look into the changes thoroughly, wanted to check on something. I believe the changes (which could be great to have) are out of scope for this.
Why couldn't we just check the route on MessageManager.routeLoaded and if it was changed, then we dismiss and return? something like what Android is doing?
I am okay to changes made to make make the classes testable, but changes to changing route and other areas is expanding scope to test and verify
Part of: https://linear.app/customerio/issue/MBL-355/ios-fix-in-app-message-displayed-on-wrong-page When a customer sets a page rule on a Modal in-app message, they expect that the Modal is only shown on that 1 screen. The problem is there is a chance that a Modal will be displayed over another screen because rendering of in-app messages takes some time and a user can navigate to another screen in that time. This change dismisses a modal if the page changes. Testing: * QA testing. The test cases are identical to all of the automated integration tests I wrote. * Wrote automated integration tests. As the number of unique QA test cases increased more then I anticipated, the more I felt manual QA testing this change is unsustainable in the long-term. With this change, I believe the existing QA test suite we have today for modal tests will cover all we need into the future after this gets merged. Note to reviewers, if you're wondering why I went with the solution that I did, see this [comment](https://linear.app/customerio/issue/MBL-353/android-fix-in-app-message-displayed-on-wrong-page#comment-99f1512d). TL;DR, there are 2 ways to solve this problem. I tried both ways and the solution I ended up using handles all test cases while the other does not.
levibostian
force-pushed
the
levi/inapp-strict-pagerules
branch
from
ca93d92 to
2db44f9
Compare
levibostian
commented
Jun 3, 2024
Comment on lines
153
to
162
| if messageManagerToCancel.isShowingMessage { | ||
| // The modal is already visible, don't cancel it. | ||
| // This can prevent an infinite loop scenario: | ||
| // * page rule changed and that triggers showing a Modal | ||
| // * Modal message is displayed on screen | ||
| // * Modal being displayed triggers an auto screenview tracking event. This triggers a SDK page route change | ||
| // * Request to cancel modal message | ||
| // * Back to the foreground screen that originally triggered showing a Modal message...repeat... | ||
| return | ||
| } |
There was a problem hiding this comment.
This code gets removed in another PR in stack #732
There was a problem hiding this comment.
This code originally existed to prevent an infinite loop but it does not handle test case explained in this PR description.
Shahroz16
requested changes
Jun 4, 2024
| @@ -60,6 +68,16 @@ class MessageManager: EngineWebDelegate { | |||
| } | |||
| } | |||
|
|
|||
| func cancelShowingMessage() { | |||
There was a problem hiding this comment.
I think we should be making these methods synchronized, cancel loadModalMessage since we relying on them for route logic. Could be considered out of scope because previously we were also just relying on messageLoaded but I think since we are relying on it for multiple actions, the chance of hitting race condition are gonna increase.
There was a problem hiding this comment.
I do agree, better threading support in the in-app SDK is needed. I also agree this is out-of-scope because I don't think there is a quick win for MessageManager, for example, synchronized when the rest of the module could also be improved.
I think bigger module-wide refactors could be a more productive approach to this problem. I also think that swift concurrency could solve this problem really well.
Do you disagree and think there is a quick win here? Is it a blocker?
There was a problem hiding this comment.
It could definitely increase the scope, happy for it be a follow up 👍
Comment on lines
+43
to
+47
| guard let modalViewManager = modalViewManager else { | ||
| return false | ||
| } | ||
|
|
||
| return modalViewManager.isShowingMessage |
There was a problem hiding this comment.
Suggested change
| guard let modalViewManager = modalViewManager else { | |
| return false | |
| } | |
| return modalViewManager.isShowingMessage | |
| return messageLoaded |
There was a problem hiding this comment.
why not just return the value of messageLoaded from here? is there a case where
they can have different values?
There was a problem hiding this comment.
messageLoaded is true when an in-app message is showing on the screen and when the in-app message is rendering but not yet on the screen. modalViewManager.isShowingMessage is true only when the message is already rendered and is displayed on screen.
This function is meant to tell you if the message is showing, so it's already been rendered and it's on the screen.
There was a problem hiding this comment.
messageLoadedis true when an in-app message is showing on the screen and when the in-app message is rendering but not yet on the screen
but looking into this method, it is where we are setting messageLoaded as true and also where modalViewManager is being initialized and view is being set, so modalViewManager.isShowingMessage is also going to be true here.
So, both are being done at the same point and rendering is also done, we are just view here?
func routeLoaded(route: String) {
Logger.instance.info(message: "Message loaded with route: \(route)")
currentRoute = route
if route == currentMessage.messageId, !messageLoaded {
messageLoaded = true
if isMessageEmbed {
delegate?.messageShown(message: currentMessage)
} else {
if UIApplication.shared.applicationState == .active {
loadModalMessage()
} else {
Gist.shared.removeMessageManager(instanceId: currentMessage.instanceId)
}
}
}
}
There was a problem hiding this comment.
I worked on improvements to the test suite today, including isShowingMessage. These improvements go beyond the scope of this PR, so I'll be opening another PR for this improvement.
The intention of isShowingMessage remains the same - determine if a message is loaded and is shown (animations complete).
| var viewController: GistModalViewController! | ||
| var position: MessagePosition | ||
| var isShowingMessage: Bool { |
There was a problem hiding this comment.
just a note: if we do end up going with messageLoaded, need to remove it
| import SharedTests | ||
| import XCTest | ||
|
|
||
| class MessagingInAppIntegrationTest: IntegrationTest { |
There was a problem hiding this comment.
Great addition !! few tests that i think can us here
- Multiple messages in queue with page rules
- Test for Navigating Back to Initial Screen After Dismissal
There was a problem hiding this comment.
Could you elaborate more on the steps in these tests? I'm having a hard time understanding what these have to do with this ticket? I think you're mentioning tests that are good to have that is outside the scope of these changes?
If you could give scenarios that are in scope, I am happy to write these tests in this PR.
There was a problem hiding this comment.
The use case here is, if a customer has already sent multiple in-app messages with different page rules, so when we fetch messages we are going to get the list of messages in queue already. We need to verify, if we change route before rendering what message is being displayed then according to page rule.
The second test, talks about, Being on a page then moving before its rendered and then coming back to the same page.
There was a problem hiding this comment.
The second test, talks about, Being on a page then moving before its rendered and then coming back to the same page.
Test test_givenUserOnScreenDuringFetch_givenUserNavigatedToDifferentScreenWhileMessageLoading_expectShowModalMessageAfterGoBack should be covering that one.
if a customer has already sent multiple in-app messages with different page rules, so when we fetch messages we are going to get the list of messages in queue already. We need to verify, if we change route before rendering what message is being displayed then according to page rule.
I'll add this test and push it up. I created tests involving multiple messages in another PR.
Shahroz16
reviewed
Jun 12, 2024
| return | ||
| } | ||
|
|
||
| guard currentMessage.gistProperties.routeRule != nil else { |
There was a problem hiding this comment.
we also need to check regex as well? incase route rule is not nil and reges still matches we don't need to cancel message.
There was a problem hiding this comment.
At first, I thought that a regex check was overkill because the logic in setCurrentRoute detects when the route changes which would mean that the user navigated to a different screen and as long as a message has a page rule attached, we could assume that the new screen would not match the modal's page rule.
However, you helped me realize that you can use wildcards in page rules. So, there is indeed a chance that you change the screen and it does match the page rule of the message.
I'll fix this in another PR.
There was a problem hiding this comment.
PR created #740
Shahroz16
approved these changes
Jun 14, 2024
| @@ -60,6 +68,16 @@ class MessageManager: EngineWebDelegate { | |||
| } | |||
| } | |||
|
|
|||
| func cancelShowingMessage() { | |||
There was a problem hiding this comment.
It could definitely increase the scope, happy for it be a follow up 👍
This was referenced Jun 14, 2024
…ustomerio-ios into levi/inapp-strict-pagerules
github-actions bot
pushed a commit
that referenced
this pull request
Jun 17, 2024
## [3.3.0](3.2.3...3.3.0) (2024-06-17) ### Features * do not show modal message if change screens and page rule enabled ([#731](#731)) ([cb1d014](cb1d014))
This was referenced Jun 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of: https://linear.app/customerio/issue/MBL-355/ios-fix-in-app-message-displayed-on-wrong-page
When a customer sets a page rule on a Modal in-app message, they expect that the Modal is only shown on that 1 screen. The problem is there is a chance that a Modal will be displayed over another screen because rendering of in-app messages takes some time and a user can navigate to another screen in that time. This change dismisses a modal if the page changes.
Testing:
Note to reviewers, if you're wondering why I went with the solution that I did, see this comment. TL;DR, there are 2 ways to solve this problem. I tried both ways and the solution I ended up using handles all test cases while the other does not.