chore: dismiss message if message expires #733
Merged
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
Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from Create a WebView to display inline in-app message and display in the Inline UIView. To do this, we can re-use a lot of the same logic that Modal messages use to create WebViews and display them. Testing: There are some automated tests added in this commit. Reviewer notes: This commit will not show an in-app message if you were to add it to a sample app. Autolayout constraints need to be added to have Views resize and that will come in a future commit. commit-id:025c7bf5
…UIKit app Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from This commit implements UIKit AutoLayout constraints to the Inline View and the WebView. Adding these constraints make inline messages visible inside of UIKit apps! The Inline View will start at height 0 and then after the webview content is loaded, it will instantly change the height to display the full in-app message. Testing: All testing is done manually in sample apps. AutoLayout constraints change the visuals of Views so viewing how the View looks in an app is required to test. If you want to test this commit in a sample app build, follow these instructions: * Open the UIKit app on device. * Immediately after the app opens, send yourself a test in-app message with element ID "test". * Wait 10 seconds. In this time, the in-app SDK will fetch the test in-app message and then will display it inline on the screen. commit-id:5ab2c403
Part of: https://linear.app/customerio/issue/MBL-310/create-a-uikit-uiview-that-displays-an-inline-in-app-message-sent-from Fetching of in-app message is an async operation running in the background. There is a chance that in-app messages are not available when the inline View is constructed but become available after. This commit gets notified when a fetch is complete so it can check if there are in-app messages to display after the View has already been constructed. Testing: * Automated tests are added. * Verified that messages display in sample app. If you want to test this commit in a sample app build, follow these instructions: 1. Kill the app on device. 2. Send yourself a test inline message from Fly. 3. Open app. After you wait a few seconds, the inline message you sent yourself will display. commit-id:aea90dff
… View Part of: https://linear.app/customerio/issue/MBL-311/provide-positive-ux-when-inline-messages-displayed-by-animating-height Fetching of in-app message is an async operation running in the background. There is a chance that an inline in-app message is rendered and ready to display on the screen currently in the foreground. We want to display the inline message when it's ready to show, and we also want to display these messages without causing a negative UX. This commit does the following: 1. Hides the inline View when the View is constructed by setting the height to 0. 2. When the in-app message web content is rendered, the inline View is given a height that matches the web content aspect ratio. The inline View animates it's height from 0 to the new height to make the inline View visible. Testing: This change is heavy on UI so testing is done mostly in UIKit sample app QA testing. If you want to test this commit in a sample app build, follow these instructions: 1. Open the app on device. 2. Send yourself a test inline message from Fly to one of the inline Views on Dashboard screen. 3. After you wait a few seconds, you will see the inline message animate into visibility. Notes for reviewer: * During development, I encountered difficulties with getting the animation to work when the inline View is embedded in a StackView. Because of this, I modified the Dashboard screen's UI to nest the inline Views inside of the StackView to make sure we QA test this scenario.
levibostian
changed the title
just testing the animation here. Testing * I created a timer that after 5 seconds it dismisses.
Testing: * in sample app, simulate a fetch by calling MessageQueueManager directly with an empty array.
…ires, it dismisses
levibostian
force-pushed
the
levi/inline-dismiss-when-expire
branch
from
236a9dc to
66e9b4d
Compare
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
commented
Jun 4, 2024
SDK binary size reports 📊SDK binary size of this PRSDK binary size diff report between this PR and the main branch |
levibostian
commented
Jun 4, 2024
There was a problem hiding this comment.
I copied this change from this PR which also adds testing support to the in-app SDK.
By mocking the EngineWeb, we are able to do the same thing as if we were mocking HTTP requests in our SDK. We can mock the process of rendering in-app messages from the web server.
levibostian
commented
Jun 4, 2024
There was a problem hiding this comment.
See MessageManager for use of the provider.
levibostian
commented
Jun 4, 2024
levibostian
commented
Jun 4, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-inline-inapp #733 +/- ##
=======================================================
Coverage ? 64.61%
=======================================================
Files ? 145
Lines ? 4123
Branches ? 0
=======================================================
Hits ? 2664
Misses ? 1459
Partials ? 0 ☔ View full report in Codecov by Sentry. |
levibostian
commented
Jun 4, 2024
levibostian
commented
Jun 4, 2024
| queueMock.getInlineMessagesReturnValue = [] | ||
|
|
||
| let inlineView = InAppMessageView(elementId: .random) | ||
|
|
||
| XCTAssertNil(getInAppMessageWebView(fromInlineView: inlineView)) | ||
| XCTAssertFalse(isDisplayingInAppMessage(inlineView)) |
There was a problem hiding this comment.
I changed some of the getInAppMessageWebView calls to the new isDisplayingInAppMessage to make the test function more accurate checking if the inline View is displaying a in-app message. By testing the height of the View is not 0, we are more certain that the inline View is visible/displayed to the user.
… we wait for main thread tasks to finish
… into levi/inline-dismiss-when-expire
levibostian
commented
Jun 4, 2024
There was a problem hiding this comment.
Besides adding new test functions, this file needed other changes.
-
I added a
onDoneRenderingInAppMessagefunction to run the code that animates the height from 0 to N to display in-app messages. I designedonDoneRenderingInAppMessageto not return until thesizeChanged()function is called on the inline View. -
Because
onDoneRenderingInAppMessageis nowasync, I needed to make all test functionsasyncto use it. When you do that, you can no longer createInAppMessageViewinstances in the test function unless you assign the test function to the main thread (@MainActor).
|
How will the changes in that pr impact this one? |
The logic is actually backwards. I modified the test with the expected behavior.
Shahroz16
reviewed
Jun 12, 2024
| } | ||
|
|
||
| // MARK: View constructed | ||
|
|
||
| @MainActor | ||
| func test_whenViewConstructedUsingStoryboards_expectCheckForMessagesToDisplay() { |
There was a problem hiding this comment.
i think we should check for view.inlineMessageManager to be nil and view.viewHeightConstraint and be 0?
There was a problem hiding this comment.
Added 2 new tests to check that after Views are constructed they start with a height of 0 (aka: dismissed state).
|
|
||
| let webViewAfterFetch = getInAppMessageWebView(fromInlineView: inlineView) | ||
|
|
||
| // If the WebViews are different, it means the message was reloaded. | ||
| // If the WebViews are the same instance, it means the message was not reloaded. |
There was a problem hiding this comment.
good check? should we also probably check for inlinemanager.currentMessage.queueId, givenInlineMessage.queueId ?
There was a problem hiding this comment.
Since the single-source-of-truth is the UI, I like how this test does an assertion on the View layer. I don't know if the test would increase in accuracy by doing this check. Can you think of a reason to?
There was a problem hiding this comment.
Great collection of tests! few areas that I feel could use some tests
- Multiple event bus events,
something like
queueMock.getInlineMessagesReturnValue = []
DIGraphShared.shared.eventBusHandler.postEvent(InAppMessagesFetchedEvent())
await waitForMainThreadToFinishPendingTasks()
queueMock.getInlineMessagesReturnValue = [secondMessage]
DIGraphShared.shared.eventBusHandler.postEvent(InAppMessagesFetchedEvent())
await onDoneRenderingInAppMessage(secondMessage)
- Multiple messages returned, should we be getting the one with higher priority to be displayed?
queueMock.getInlineMessagesReturnValue = [firstMessage, secondMessage]
-
Delayed Setting of element ID?
-
Multiple instances of InAppMessageView operate independently without interference
-
Changes to dynamic constraints? we are already changing height constraints, if user changes height/width what end height width constraint are we expecting?
There was a problem hiding this comment.
Thanks for the test suggestions. You put a lot of thought into that, thanks!
Let me try and put these in my own words to see if we align.
- Multiple event bus events
I can think of these tests:
- Given fetch event returning 0 events, expect View to not display any message. Then another fetch happens, event gets returned, we expect the View to display that message.
- Given fetch event returning N events, but none with element Id matching the View. We expect that the View does not display any messages. Then a fetch happens returning a message with element id, we expect the View to display that message.
- Multiple messages returned, should we be getting the one with higher priority to be displayed?
Priority logic is in the business layer and tests exist for that so I don't see a need to add a test for that in the View.
- Delayed Setting of element ID?
A test like this? Given no element id set of View yet, fetch occurs returns messages with a matching element id, then an element id is set on View, we expect that the inline View displays a message.
- Multiple instances of InAppMessageView operate independently without interference
Good idea. Tests where 2 Views are created with matching IDs and non-matching IDs would be good tests.
- Changes to dynamic constraints? we are already changing height constraints, if user changes height/width what end height width constraint are we expecting?
I will need some help with this one. Could you give an example of tests cases?
There was a problem hiding this comment.
You are spot on all the additions of tests you mentioned. We are on the same page.
Changes to dynamic constraints? we are already changing height constraints, if user changes height/width what end height width constraint are we expecting?
I was referring to, if user set the height and width of the view at beginning, and when we inflate the in-app view, the final height and width of the controller should be what we expect it not what user set its.
Similarly, if we inflate the view and set the height and width of the controller, if user updates the width/height, what size are we expecting at the end?
There was a problem hiding this comment.
I've looked into each of the suggestions. Either the test case is now covered, or I suggest a follow-up PR to introduce it. See my responses below:
Multiple event bus events
- Test case: Given fetch event returning 0 events, expect View to not display any message. Then another fetch happens, event gets returned, we expect the View to display that message.
This is covered in test test_givenInAppMessageFetchedAfterViewConstructed_expectShowInAppMessageFetched
- Test case: Given fetch event returning N events, but none with element Id matching the View. We expect that the View does not display any messages. Then a fetch happens returning a message with element id, we expect the View to display that message.
This is going to require some refactors to the test class to be more of an integration test then a unit test as it is now. Because this will require some refactors to the test suite, I suggest we don't block this PR for it and we introduce this change in a follow-up PR.
Delayed Setting of element ID
- Test case: Given no element id set of View yet, fetch occurs returns messages with a matching element id, then an element id is set on View, we expect that the inline View displays a message.
This is covered in test test_whenViewConstructedUsingStoryboards_expectCheckForMessagesToDisplay, since a delay in setting element ID is only with constructing a View used in a Storyboard.
Multiple instances of InAppMessageView operate independently without interference
Same response as above when referring to integration tests. Same suggestion applies - unblock this PR and introduce follow-up PR.
Changes to dynamic constraints
Tests have been pushed for these scenarios.
Base automatically changed from
levi/inline-animate-open
to
feature-inline-inapp
June 12, 2024 13:00
after merging main, some code needed fixed
levibostian
commented
Jun 26, 2024
Comment on lines
+267
to
268
| // swiftlint:disable:next force_cast | ||
| (Gist.shared.messageQueueManager as! MessageQueueManagerImpl).processFetchedMessages(messages) |
There was a problem hiding this comment.
I prefer to not have private functions such as processFetchedMessages as part of the MessageQueueManager protocol as the protocol is meant to be the public API that the SDK calls and only the tests uses processFetchedMessages.
Instead of directly calling processFetchedMessages, we could mock the GistQueueNetwork HTTP request which then calls processFetchedMessages. But I believe that change would go beyond the scope of this PR and may not be worth the effort when this call does the same thing.
Shahroz16
approved these changes
Jun 27, 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.
https://linear.app/customerio/issue/MBL-315/dismiss-message-if-message-expires
As a marketer I want to stop displaying an in-app message inline by setting a expiration on the in-app message. If a message is being displayed in the app > a fetch gets message from backend for the user > fetch no longer contains the message being displayed, it can be considered expired. If this scenario happens, we want the inline View to gracefully stop showing the in-app message. This is done by animating the height from N to 0 which makes the inline View no longer visible.
Here is a demo of this behavior:
dismiss.demo.mov
Testing:
I tested...
Testing in-app message expiration is difficult because messages have a minimum of 1 hour of expiration.
Instead of waiting for 1 hour, I simulated this behavior by adding this line of code to the inline View's
setupView():The demo video was recorded with this block of code used.
Based on PR #724
Full chain of PRs as of 2024-06-04
levi/inline-dismiss-when-expire➔levi/inline-animate-openlevi/inline-animate-open➔levi/display-inline-uiview-afterfetchlevi/display-inline-uiview-afterfetch➔spr/main/5ab2c403spr/main/5ab2c403➔spr/main/025c7bf5spr/main/025c7bf5➔feature-inline-inapp