Bugfix FXIOS-9490 [Microsurvey] Using default action causes issues opening survey (mozilla-mobile#21019)

Author: cyndichin

firefox-ios/Client/Experiments/Messaging/GleanPlumbMessage.swift

@@ -41,7 +41,7 @@ struct GleanPlumbMessage {
     /// The action URL as resolved by the Nimbus Messaging component.
     ///
     /// Embedding apps should not read from this directly.
-    let action: String
+    let action: String?
 
     /// The conditions that need to be satisfied for a message to be considered eligible to present.
     ///

firefox-ios/Client/Experiments/Messaging/GleanPlumbMessageManager.swift

@@ -194,11 +194,33 @@ class GleanPlumbMessageManager: GleanPlumbMessageManagerProtocol {
     func onMessagePressed(_ message: GleanPlumbMessage, window: WindowUUID?, shouldExpire: Bool = true) {
         messagingStore.onMessagePressed(message, shouldExpire: shouldExpire)
 
-        guard let helper = createMessagingHelper.createNimbusMessagingHelper() else { return }
+        var extras = baseTelemetryExtras(using: message)
+        if let action = message.action {
+            if let uuid = handleLinkAction(for: message, action: action, window: window) {
+                extras[MessagingKey.actionUUID.rawValue] = uuid
+            }
+        }
+
+        TelemetryWrapper.recordEvent(category: .action,
+                                     method: .tap,
+                                     object: .messaging,
+                                     value: .messageInteracted,
+                                     extras: extras)
+    }
 
-        guard let (uuid, urlToOpen) = try? self.generateUuidAndFormatAction(for: message, with: helper) else {
+    private func handleLinkAction(
+        for message: GleanPlumbMessage,
+        action: String,
+        window: WindowUUID?
+    ) -> String? {
+        guard let helper = createMessagingHelper.createNimbusMessagingHelper() else { return nil }
+        guard let (uuid, urlToOpen) = try? self.generateUuidAndFormatAction(
+            for: action,
+            with: message.data.actionParams,
+            with: helper
+        ) else {
             self.onMalformedMessage(id: message.id, surface: message.surface)
-            return
+            return nil
         }
 
         // With our well-formed URL, we can handle the action here.
@@ -207,25 +229,16 @@ class GleanPlumbMessageManager: GleanPlumbMessageManagerProtocol {
         } else {
             applicationHelper.open(urlToOpen)
         }
-
-        var extras = baseTelemetryExtras(using: message)
-        if let uuid = uuid {
-            extras[MessagingKey.actionUUID.rawValue] = uuid
-        }
-        TelemetryWrapper.recordEvent(category: .action,
-                                     method: .tap,
-                                     object: .messaging,
-                                     value: .messageInteracted,
-                                     extras: extras)
+        return uuid
     }
 
-    private func generateUuidAndFormatAction(for message: GleanPlumbMessage,
+    private func generateUuidAndFormatAction(for action: String,
+                                             with actionParams: [String: String],
                                              with helper: NimbusMessagingHelperProtocol) throws -> (String?, URL) {
         // Make substitutions where they're needed.
-        let actionTemplate = message.action
+        let actionTemplate = action
         var uuid = helper.getUuid(template: actionTemplate)
         let action = helper.stringFormat(template: actionTemplate, uuid: uuid)
-        let actionParams = message.data.actionParams
 
         let urlString: String = action.hasPrefix("://") ? deepLinkScheme + action : action
         guard var components = URLComponents(string: urlString) else {
@@ -325,16 +338,19 @@ class GleanPlumbMessageManager: GleanPlumbMessageManagerProtocol {
         message: MessageData,
         lookupTables: Messaging
     ) -> Result<GleanPlumbMessage, CreateMessageError> {
-        var action: String
+        var action: String?
         if !message.isControl {
             // Guard against a message with a blank `text` property.
             guard !message.text.isEmpty else { return .failure(.malformed) }
 
-            // The message action should be either from the lookup table OR a URL.
-            guard let safeAction = sanitizeAction(message.action, table: lookupTables.actions) else {
-                return .failure(.malformed)
+            // Message action can be null. If not null,
+            // the message action should be either from the lookup table OR a URL.
+            if let messageAction = message.action {
+                guard let safeAction = sanitizeAction(messageAction, table: lookupTables.actions) else {
+                    return .failure(.malformed)
+                }
+                action = safeAction
             }
-            action = safeAction
         } else {
             action = "CONTROL_ACTION"
         }

firefox-ios/firefox-ios-tests/Tests/ClientTests/Messaging/GleanPlumbMessageManagerTests.swift

@@ -240,6 +240,19 @@ class GleanPlumbMessageManagerTests: XCTestCase {
         XCTAssertEqual(hardcodedNimbusFeatures.getExposureCount(featureId: "messaging"), 2)
     }
 
+    func testManagerGetMessages_happyPath_withNoAction() throws {
+        let expectedId = "infoCard"
+        let messages = [
+            createMessage(messageId: "infoCard-notyet", action: nil, surface: .newTabCard, trigger: ["true", "false"]),
+            createMessage(messageId: expectedId, action: nil, surface: .newTabCard, trigger: ["true", "true"])
+        ]
+        guard let observed = subject.getNextMessage(for: .newTabCard, from: messages) else {
+            XCTFail("Expected to retrieve message")
+            return
+        }
+        XCTAssertEqual(observed.id, expectedId)
+    }
+
     func testManagerOnMessageDisplayed() {
         let message = createMessage(messageId: messageId)
         subject.onMessageDisplayed(message)
@@ -357,6 +370,16 @@ class GleanPlumbMessageManagerTests: XCTestCase {
         testEventMetricRecordingSuccess(metric: GleanMetrics.Messaging.malformed)
     }
 
+    func testManagerOnMessagePressed_withNoAction() {
+        let message = createMessage(messageId: messageId, action: nil)
+        subject.onMessagePressed(message, window: nil)
+        let messageMetadata = messagingStore.getMessageMetadata(messageId: messageId)
+        XCTAssertTrue(messageMetadata.isExpired)
+        XCTAssertEqual(applicationHelper.openURLCalled, 0)
+        XCTAssertNil(applicationHelper.lastOpenURL)
+        testEventMetricRecordingSuccess(metric: GleanMetrics.Messaging.clicked)
+    }
+
     func testManagerOnMessageDismissed() {
         let message = createMessage(messageId: messageId)
         subject.onMessageDismissed(message)
@@ -369,7 +392,7 @@ class GleanPlumbMessageManagerTests: XCTestCase {
     // MARK: - Helper function
 
     private func createMessage(messageId: String,
-                               action: String = "MAKE_DEFAULT_BROWSER",
+                               action: String? = "MAKE_DEFAULT_BROWSER",
                                actionParams: [String: String] = [:],
                                surface: MessageSurfaceId = .newTabCard,
                                trigger: [String] = ["true"],

firefox-ios/nimbus-features/messaging/messaging.fml.yaml

@@ -103,11 +103,10 @@ objects:
       action:
         # We would like this to be of ActionName type, but it accepts https:// URLs
         # so changing this would be a breaking change.
-        type: ActionName
+        type: Option<ActionName>
         description: >
           The name of a deeplink URL to be opened if the button is clicked.
-        # This should never be defaulted.
-        default: OPEN_URL
+        default: null
       action-params:
         type: Map<String, String>
         description: Query parameters appended to the deeplink action URL