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

feat: do not show modal message if change screens and page rule enabled #731

Merged
merged 7 commits into from
Jun 17, 2024
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ generate:
./binny sourcery --sources Sources/MessagingPush --templates Sources/Templates --output Sources/MessagingPush/autogenerated --args imports=CioInternalCommon
./binny sourcery --sources Sources/MessagingPushAPN --templates Sources/Templates --output Sources/MessagingPushAPN/autogenerated --args imports=CioMessagingPush-CioInternalCommon
./binny sourcery --sources Sources/MessagingPushFCM --templates Sources/Templates --output Sources/MessagingPushFCM/autogenerated --args imports=CioMessagingPush-CioInternalCommon
./binny sourcery --sources Sources/MessagingInApp --templates Sources/Templates --output Sources/MessagingInApp/autogenerated --args imports=CioInternalCommon
./binny sourcery --sources Sources/MessagingInApp --templates Sources/Templates --output Sources/MessagingInApp/autogenerated --args imports=CioInternalCommon-UIKit
./binny sourcery --sources Sources/Migration --templates Sources/Templates --output Sources/Migration/autogenerated --args imports=CioInternalCommon


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Foundation

/// Inherit in a UIViewController subclass to avoid having an automatic screenview event tracked for it.
/// Currently only being used internally. `public` only because multiple SDK modules use it.
public protocol DoNotTrackScreenViewEvent {}
7 changes: 7 additions & 0 deletions Sources/DataPipeline/Plugins/AutoTrackingScreenViews.swift
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ extension AutoTrackingScreenViews {
return
}

// View is an internal Cio View, ignore event.
// This logic needs to exist outside of the `filterAutoScreenViewEvents` feature because customers can override the event filter.
// There are side effects (such as infinite loops) caused when our SDK tracks screenview events for internal classes.
if viewController is DoNotTrackScreenViewEvent {
return
}

// Before we track event, apply a filter to remove events that could be unhelpful.
let customerOverridenFilter = filterAutoScreenViewEvents
let defaultSdkFilter: (UIViewController) -> Bool = { viewController in
Expand Down
9 changes: 8 additions & 1 deletion Sources/MessagingInApp/Gist/EngineWeb/EngineWeb.swift
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import CioInternalCommon
import Foundation
import UIKit
import WebKit
Expand All @@ -12,7 +13,13 @@ public protocol EngineWebDelegate: AnyObject {
func error()
}

public class EngineWeb: NSObject {
protocol EngineWebInstance: AutoMockable {
var delegate: EngineWebDelegate? { get set }
var view: UIView { get }
func cleanEngineWeb()
}

public class EngineWeb: NSObject, EngineWebInstance {
private var _currentRoute = ""
private var _timeoutTimer: Timer?
private var _elapsedTimer = ElapsedTimer()
Expand Down
14 changes: 14 additions & 0 deletions Sources/MessagingInApp/Gist/EngineWeb/EngineWebProvider.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import CioInternalCommon
import Foundation

// Allows us to mock EngineWeb instances for testing.
protocol EngineWebProvider {
func getEngineWebInstance(configuration: EngineWebConfiguration) -> EngineWebInstance
}

// sourcery: InjectRegisterShared = "EngineWebProvider"
class EngineWebProviderImpl: EngineWebProvider {
func getEngineWebInstance(configuration: EngineWebConfiguration) -> EngineWebInstance {
EngineWeb(configuration: configuration)
}
}
39 changes: 38 additions & 1 deletion Sources/MessagingInApp/Gist/Gist.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,22 @@
_ = Gist.shared.getMessageView(Message(messageId: ""))
}

// For testing to reset the singleton state
func reset() {
clearUserToken()
messageQueueManager = MessageQueueManager()
messageManagers = []
RouteManager.clearCurrentRoute()
}

// MARK: User

public func setUserToken(_ userToken: String) {
UserManager().setUserToken(userToken: userToken)
}

public func clearUserToken() {
cancelModalMessage(ifDoesNotMatchRoute: "") // provide a new route to trigger a modal cancel.
UserManager().clearUserToken()
messageQueueManager.clearUserMessagesFromLocalStore()
}
Expand All @@ -46,6 +55,12 @@
}

public func setCurrentRoute(_ currentRoute: String) {
if RouteManager.getCurrentRoute() == currentRoute {
return // ignore request, route has not changed.
}

cancelModalMessage(ifDoesNotMatchRoute: currentRoute)

RouteManager.setCurrentRoute(currentRoute)
messageQueueManager.fetchUserMessagesFromLocalStore()
}
Expand Down Expand Up @@ -97,6 +112,8 @@
Logger.instance.debug(message: "Message with id: \(message.messageId) dismissed")
removeMessageManager(instanceId: message.instanceId)
delegate?.messageDismissed(message: message)

messageQueueManager.fetchUserMessagesFromLocalStore()

Check warning on line 116 in Sources/MessagingInApp/Gist/Gist.swift

View check run for this annotation

Codecov / codecov/patch

Sources/MessagingInApp/Gist/Gist.swift#L116

Added line #L116 was not covered by tests
}

public func messageError(message: Message) {
Expand Down Expand Up @@ -126,6 +143,26 @@
}
}

// When the user navigates to a different screen, modal messages should only appear if they are meant for the current screen.
// If the currently displayed/loading modal message has a page rule, it should not be shown anymore.
private func cancelModalMessage(ifDoesNotMatchRoute newRoute: String) {
if let messageManager = getModalMessageManager() {
let modalMessageLoadingOrDisplayed = messageManager.currentMessage

if modalMessageLoadingOrDisplayed.doesHavePageRule(), !modalMessageLoadingOrDisplayed.doesPageRuleMatch(route: newRoute) {
// the page rule has changed and the currently loading/visible modal has page rules set, it should no longer be shown.
Logger.instance.debug(message: "Cancelled showing message with id: \(modalMessageLoadingOrDisplayed.messageId)")

// Stop showing the current message synchronously meaning to remove from UI instantly.
// We want to be sure the message is gone when this function returns and be ready to display another message if needed.
messageManager.cancelShowingMessage()

// Removing the message manager allows you to show a new modal message. Otherwise, request to show will be ignored.
removeMessageManager(instanceId: modalMessageLoadingOrDisplayed.instanceId)
}
}
}

// Message Manager

private func createMessageManager(siteId: String, message: Message) -> MessageManager {
Expand All @@ -135,7 +172,7 @@
return messageManager
}

private func getModalMessageManager() -> MessageManager? {
func getModalMessageManager() -> MessageManager? {
messageManagers.first(where: { !$0.isMessageEmbed })
}

Expand Down
35 changes: 26 additions & 9 deletions Sources/MessagingInApp/Gist/Managers/MessageManager.swift
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import CioInternalCommon
import Foundation
import UIKit

Expand All @@ -6,17 +7,18 @@
}

class MessageManager: EngineWebDelegate {
private var engine: EngineWeb?
private var engine: EngineWebInstance
private let siteId: String
private var messagePosition: MessagePosition = .top
private var messageLoaded = false
var messageLoaded = false
levibostian marked this conversation as resolved.
Show resolved Hide resolved
private var modalViewManager: ModalViewManager?
var isMessageEmbed = false
let currentMessage: Message
var gistView: GistView!
private var currentRoute: String
private var elapsedTimer = ElapsedTimer()
weak var delegate: GistDelegate?
private let engineWebProvider: EngineWebProvider = DIGraphShared.shared.engineWebProvider

init(siteId: String, message: Message) {
self.siteId = siteId
Expand All @@ -32,11 +34,17 @@
properties: message.toEngineRoute().properties
)

self.engine = EngineWeb(configuration: engineWebConfiguration)
if let engine = engine {
engine.delegate = self
self.gistView = GistView(message: currentMessage, engineView: engine.view)
self.engine = engineWebProvider.getEngineWebInstance(configuration: engineWebConfiguration)
engine.delegate = self
self.gistView = GistView(message: currentMessage, engineView: engine.view)
}

var isShowingMessage: Bool {
guard let modalViewManager = modalViewManager else {
return false
}

return modalViewManager.isShowingMessage
Comment on lines +43 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
guard let modalViewManager = modalViewManager else {
return false
}
return modalViewManager.isShowingMessage
return messageLoaded

Copy link
Contributor

Choose a reason for hiding this comment

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

why not just return the value of messageLoaded from here? is there a case where
they can have different values?

Copy link
Contributor Author

@levibostian levibostian Jun 7, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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)
                }
            }
        }
    }

Copy link
Contributor Author

@levibostian levibostian Jun 13, 2024

Choose a reason for hiding this comment

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

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).

}

func showMessage(position: MessagePosition) {
Expand All @@ -60,6 +68,16 @@
}
}

func cancelShowingMessage() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could definitely increase the scope, happy for it be a follow up 👍

guard let modalViewManager = modalViewManager else {
return // no message being shown to cancel
}

engine.delegate = nil // to make sure we do not get a callback when message loaded and we try to show it.

modalViewManager.cancel()
}

func dismissMessage(completionHandler: (() -> Void)? = nil) {
if let modalViewManager = modalViewManager {
modalViewManager.dismissModalView { [weak self] in
Expand All @@ -82,7 +100,7 @@

// Cleaning after engine web is bootstrapped and all assets downloaded.
if currentMessage.messageId == "" {
engine?.cleanEngineWeb()
engine.cleanEngineWeb()

Check warning on line 103 in Sources/MessagingInApp/Gist/Managers/MessageManager.swift

View check run for this annotation

Codecov / codecov/patch

Sources/MessagingInApp/Gist/Managers/MessageManager.swift#L103

Added line #L103 was not covered by tests
}
}

Expand Down Expand Up @@ -223,8 +241,7 @@
}

deinit {
engine?.cleanEngineWeb()
engine = nil
engine.cleanEngineWeb()
}

private func showNewMessage(url: URL) {
Expand Down
54 changes: 30 additions & 24 deletions Sources/MessagingInApp/Gist/Managers/MessageQueueManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
}
}

deinit {
queueTimer?.invalidate()
}

func fetchUserMessagesFromLocalStore() {
Logger.instance.info(message: "Checking local store with \(localMessageStore.count) messages")
let sortedMessages = localMessageStore.sorted {
Expand All @@ -44,7 +48,7 @@
}
}
sortedMessages.forEach { message in
handleMessage(message: message.value)
showMessageIfMeetsCriteria(message: message.value)
}
}

Expand All @@ -59,11 +63,13 @@
localMessageStore.removeValue(forKey: queueId)
}

private func addMessageToLocalStore(message: Message) {
guard let queueId = message.queueId else {
return
func addMessagesToLocalStore(messages: [Message]) {
messages.forEach { message in
guard let queueId = message.queueId else {
return

Check warning on line 69 in Sources/MessagingInApp/Gist/Managers/MessageQueueManager.swift

View check run for this annotation

Codecov / codecov/patch

Sources/MessagingInApp/Gist/Managers/MessageQueueManager.swift#L69

Added line #L69 was not covered by tests
}
localMessageStore.updateValue(message, forKey: queueId)
}
localMessageStore.updateValue(message, forKey: queueId)
}

@objc
Expand All @@ -80,13 +86,10 @@
guard let responses else {
return
}
// To prevent us from showing expired / revoked messages, clear user messages from local queue.
self.clearUserMessagesFromLocalStore()

Logger.instance.info(message: "Gist queue service found \(responses.count) new messages")
for queueMessage in responses {
let message = queueMessage.toMessage()
self.handleMessage(message: message)
}

self.processFetchResponse(responses.map { $0.toMessage() })

Check warning on line 92 in Sources/MessagingInApp/Gist/Managers/MessageQueueManager.swift

View check run for this annotation

Codecov / codecov/patch

Sources/MessagingInApp/Gist/Managers/MessageQueueManager.swift#L92

Added line #L92 was not covered by tests
case .failure(let error):
Logger.instance.error(message: "Error fetching messages from Gist queue service. \(error.localizedDescription)")
}
Expand All @@ -99,7 +102,18 @@
}
}

private func handleMessage(message: Message) {
func processFetchResponse(_ fetchedMessages: [Message]) {
// To prevent us from showing expired / revoked messages, reset the local queue with the latest queue from the backend service.
// The backend service is the single-source-of-truth for in-app messages for each user.
clearUserMessagesFromLocalStore()
addMessagesToLocalStore(messages: fetchedMessages)
Shahroz16 marked this conversation as resolved.
Show resolved Hide resolved

for message in fetchedMessages {
showMessageIfMeetsCriteria(message: message)
}
}

private func showMessageIfMeetsCriteria(message: Message) {
// Skip shown messages
if let queueId = message.queueId, Gist.shared.shownMessageQueueIds.contains(queueId) {
Logger.instance.info(message: "Message with queueId: \(queueId) already shown, skipping.")
Expand All @@ -108,18 +122,10 @@

let position = message.gistProperties.position

if let routeRule = message.gistProperties.routeRule {
let cleanRouteRule = routeRule.replacingOccurrences(of: "\\", with: "/")
if let regex = try? NSRegularExpression(pattern: cleanRouteRule) {
let range = NSRange(location: 0, length: Gist.shared.getCurrentRoute().utf16.count)
if regex.firstMatch(in: Gist.shared.getCurrentRoute(), options: [], range: range) == nil {
Logger.instance.debug(message: "Current route is \(Gist.shared.getCurrentRoute()), needed \(cleanRouteRule)")
addMessageToLocalStore(message: message)
return
}
} else {
Logger.instance.info(message: "Problem processing route rule message regex: \(cleanRouteRule)")
return
if message.doesHavePageRule(), let cleanPageRule = message.cleanPageRule {
if !message.doesPageRuleMatch(route: Gist.shared.getCurrentRoute()) {
Logger.instance.debug(message: "Current route is \(Gist.shared.getCurrentRoute()), needed \(cleanPageRule)")
return // exit early to not show the message since page rule doesnt match
}
}

Expand Down
28 changes: 21 additions & 7 deletions Sources/MessagingInApp/Gist/Managers/ModalViewManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@
}

class ModalViewManager {
var window: UIWindow!
var window: UIWindow?
var viewController: GistModalViewController!
var position: MessagePosition
var isShowingMessage: Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

just a note: if we do end up going with messageLoaded, need to remove it

window != nil
}

init(gistView: GistView, position: MessagePosition) {
self.viewController = GistModalViewController()
Expand All @@ -21,8 +24,8 @@
func showModalView(completionHandler: @escaping () -> Void) {
viewController.view.isHidden = true
window = getUIWindow()
window.rootViewController = viewController
window.isHidden = false
window?.rootViewController = viewController
window?.isHidden = false
var finalPosition: CGFloat = 0

switch position {
Expand All @@ -49,6 +52,11 @@
viewController.view.isHidden = false
}

// Like dismiss, but no animation. Instantly removes the view from the screen.
func cancel() {
removeModalViewFromScreen()
}

func dismissModalView(completionHandler: @escaping () -> Void) {
var finalPosition: CGFloat = 0
switch position {
Expand All @@ -66,15 +74,21 @@
UIView.animate(withDuration: 0.1, delay: 0, options: [.curveEaseIn], animations: {
self.viewController.view.center.y = finalPosition
}, completion: { _ in
self.window?.isHidden = false
self.viewController.removeFromParent()
self.window = nil
self.removeModalViewFromScreen()

Check warning on line 77 in Sources/MessagingInApp/Gist/Managers/ModalViewManager.swift

View check run for this annotation

Codecov / codecov/patch

Sources/MessagingInApp/Gist/Managers/ModalViewManager.swift#L77

Added line #L77 was not covered by tests

completionHandler()
})
})
}

func getUIWindow() -> UIWindow {
private func removeModalViewFromScreen() {
viewController?.view.isHidden = true
window?.isHidden = true
viewController.removeFromParent()
window = nil
}

private func getUIWindow() -> UIWindow {
var modalWindow = UIWindow(frame: UIScreen.main.bounds)
if #available(iOS 13.0, *) {
for connectedScene in UIApplication.shared.connectedScenes
Expand Down
Loading
Loading