[V3] Add Networking to Send Analytics #75

richherrera · 2025-03-04T18:39:31Z

Summary of changes

Add basic Network Layer
Implement AnalyticsService class
Change FPTI Batch Model
Add UTs
Add TestPlan

Checklist

Added a changelog entry

Authors

@richherrera

jaxdesmarais · 2025-03-04T22:09:52Z

Sources/PopupBridge/POPPopupBridge.swift

@@ -8,13 +8,16 @@ public class POPPopupBridge: NSObject, WKScriptMessageHandler {
    /// Exposed for testing
    var returnedWithURL: Bool = false

+    static var analyticsService: AnalyticsServiceable = AnalyticsService()


Since this is static have we ensure it's being destroyed properly when this class is deinitalized? Just want to make sure we aren't keeping this around somehow.

Excellent observation, let me check. The reason i created this static property is because we need to send the started event when this class is instantiated. I couldn't find a way to use dependency injection, and this approach allows me to conduct the necessary tests

Currently this property does not get destroyed, once created, it persists throughout the application's lifecycle. As I mentioned earlier, the reason it is static is that it's the only way I found to conduct tests. I need to test the start event sent in the public init, which requires injecting a mock of the AnalyticsService class. Any other ideas, or shall we continue with the static property?

The PopupBridge class never deinitializes properly. I tested this on the main branch, and the same issue occurs. It seems we do have some retain cycles, but they are not due to the integration of the static property (once we manage to deinitialize PopupBridge, we will be able to see if the AnalyticsService class deinitializes correctly).

Main branch:

🤔 yeah we should definitely address it not deinitializing or provide merchants more explicit instructions on destroying this class. If it's happening on main as well I don't want to make it a blocker, but can you make a ticket for us to dig into this a bit more post release?

Should we consider moving the started event out of the init and into the first thing that's called post init? Would that allow us to make this non static? Sounds like we'd still want to address the deinit of this class at a later time.

Ticket DTMOBILES-1487

Maybe i can think of another approach. We have two flows: PayPal (via PayPal -> Pay with Credit Card) and Venmo, where there isn't another method called after the init that we could use to send the 'start' event.

Why does it need to be static? Can it just be an instance property?

The "start" event is triggered in the public init, and there's no way to inject the mock AnalyticsService for testing. Any suggestions?

Please take a look at the public init,

popup-bridge-ios/Sources/PopupBridge/POPPopupBridge.swift

Line 36 in 70bb98e

Self.analyticsService.sendAnalyticsEvent(PopupBridgeAnalytics.started, sessionID: sessionID)

public init(webView: WKWebView, prefersEphemeralWebBrowserSession: Bool = true) { self.webView = webView super.init() Self.analyticsService.sendAnalyticsEvent(PopupBridgeAnalytics.started, sessionID: sessionID) configureWebView() webAuthenticationSession.prefersEphemeralWebBrowserSession = prefersEphemeralWebBrowserSession returnBlock = { url in guard let script = self.constructJavaScriptCompletionResult(returnURL: url) else { return } self.injectWebView(webView: webView, withJavaScript: script) return } }

Sources/PopupBridge/FPTIBatchData.swift

Sources/PopupBridge/NetworkClient.swift

Sources/PopupBridge/AnalyticsService.swift

Sources/PopupBridge/FPTIBatchData.swift

jaxdesmarais · 2025-03-07T14:15:49Z

Sources/PopupBridge/POPPopupBridge.swift

@@ -8,13 +8,16 @@ public class POPPopupBridge: NSObject, WKScriptMessageHandler {
    /// Exposed for testing
    var returnedWithURL: Bool = false

+    static var analyticsService: AnalyticsServiceable = AnalyticsService()


🤔 yeah we should definitely address it not deinitializing or provide merchants more explicit instructions on destroying this class. If it's happening on main as well I don't want to make it a blocker, but can you make a ticket for us to dig into this a bit more post release?

Should we consider moving the started event out of the init and into the first thing that's called post init? Would that allow us to make this non static? Sounds like we'd still want to address the deinit of this class at a later time.

Sources/PopupBridge/POPPopupBridge.swift

jaxdesmarais

One small docstrings cleanup, but assuming we verified we are seeing these events in FPTI, this looks great to me! 🚀

Sources/PopupBridge/FPTIBatchData.swift

Co-authored-by: Jax DesMarais-Leder <jdesmarais@paypal.com>

richherrera · 2025-03-11T18:09:04Z

One small docstrings cleanup, but assuming we verified we are seeing these events in FPTI, this looks great to me! 🚀

yeah, we are seeing these events in FPTI

warmkesselj · 2025-03-12T18:52:21Z

Sources/PopupBridge/NetworkError.swift

+import Foundation
+
+enum NetworkError: Error {
+    case invalidResponse


Thoughts on passing the statusCode so it can be logged?

case invalidResponse(statusCode: Int)

I think this way it remains as basic as possible, which is what's expected, since im using this in the NetworkClient, the scope of httpResponse is limited within the guard block, so i cannot directly use it in the throw statement outside of that block. If in the future we need to add more endpoints, I think that would add more value.

UnitTests/MockNetworkClient.swift

scannillo · 2025-03-13T20:03:41Z

PopupBridge.xcodeproj/project.pbxproj

 		45CD0C2C2D64F08F0072C5A4 /* FPTIBatchData.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45CD0C2B2D64F0810072C5A4 /* FPTIBatchData.swift */; };
 		45CD0C2E2D664D140072C5A4 /* Date+MilisecondTimestamp.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45CD0C2D2D664CF50072C5A4 /* Date+MilisecondTimestamp.swift */; };
 		45CD0C302D6788A10072C5A4 /* Bundle+Extension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45CD0C2F2D67888F0072C5A4 /* Bundle+Extension.swift */; };
 		45CD0C322D6793FB0072C5A4 /* PopupBridgeAnalytics.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45CD0C312D6793F90072C5A4 /* PopupBridgeAnalytics.swift */; };
+		45FBAF272D6F9CCF000D550B /* AnalyticsService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 45FBAF262D6F9CC6000D550B /* AnalyticsService.swift */; };


Can we add all of the new files that were specifically added for analytics to an Analytics folder under Sources/PopupBridge? nitpick, but would help navigation of the PopupBridge source files

Updated: c8fd4cf

scannillo · 2025-03-13T20:06:37Z

Sources/PopupBridge/POPPopupBridge.swift

    // MARK: - Private Properties

    private let messageHandlerName = "POPPopupBridge"
-    private let hostName = "popupbridgev1"    
+    private let hostName = "popupbridgev1"
+    private let sessionID = UUID().uuidString.replacingOccurrences(of: "-", with: "")


Can this sessionID creation live in AnalyticsService instead? Since it isn't required by any of the login in POPPopupBridge.swift

At the moment, no, because the sessionID would always be the same, due to AnalyticsService being a static var.

If you have any suggestions for adding AnalyticsService that isn't a static var, I'd be happy to work on it. The reason it's a static variable is that in tests, I need to inject the mock of AnalyticsService. I can't inject it in the init, and the "start" event is triggered in the init :(

scannillo · 2025-03-13T20:06:56Z

Sources/TestPlan.xctestplan

@@ -0,0 +1,24 @@
+{


Can this file be deleted?

yeap, deleted: 5911546

scannillo · 2025-03-13T20:09:44Z

Sources/PopupBridge/NetworkClient.swift

+    func post(url: URL, body: FPTIBatchData) async throws
+}
+
+final class NetworkClient: Networkable {


If we know that PopupBridge doesn't do any other networking besides just sending a single POST for analytics, do we think it's worth having this as a separate layer from AnalyticsService? Thinking of the KISS principe

Updated: 7bc9147

scannillo · 2025-03-13T20:12:00Z

Sources/PopupBridge/AnalyticsService.swift

+    // MARK: - Private Properties
+
+    /// The FPTI URL to post all analytic events.
+    private let url = URL(string: "https://api.paypal.com/v1/tracking/batch/events")!


Since we aren't actually batching, FPTI has a separate endpoint for single event upload /v1/tracking/events/. The format is slightly different from /v1/tracking/batch/events/. I'm not sure if there are performance benefits to using the single event API vs the multiple event API, but we could confirm in #help-fpti.

To see the structure of each, you can look at the API details in PPAAS (I also have it in a Postman collection)

Yes, it's the endpoint used in PPCP, however, we would need a custom encode method. I did a POC with those changes and was thinking that it would be better to have the same implementation we have in BT, which is why I opted for this implementation. While working on BT v7, we chose not to use a custom encode method. Do you think we could use it for this repo?

richherrera · 2025-03-18T19:36:34Z

Events were sent successfully after removing the network client

richherrera added 26 commits February 26, 2025 15:28

Change FPTIBatchData structure

d272d2c

Add NetworkClient and AnalyticsServices files

304190d

Add Sessionable and NetworkError files

03f9365

Add Network errors

52fd81c

Add Sessionable Protocol and Extension

6224055

Sort files

4a4e97f

Add NetworkClient

4f87b20

Implement AnalyticsService class

0d71ffb

Add Private Mark

060e7e4

Add necessary files (TestPlan, mock files and UT files)

ddb944c

Setup Test Plan

37ea28a

Create MockSession class and NonEncodable structure

05cf278

Add NetworkClient tests

6310175

Create MockNetworkClient class

b03bdac

Sort files

d6fc4fc

Add missing blank space

022530d

Add UTs to test success and failures

c9e846c

Remove iOS 15 validation

292affb

Add analytics calls

5308717

Set sessionID value

a268cd0

Add AnalyticsService property

9734fb3

Add MockAnalyticsService

3ba4d21

Fix PopupBridge tests adding mock

700f695

Increase time interval

fd33535

Add MockAnalyticsService

d0a8d8d

Add PopupBrdige UTs

147dc9b

richherrera added the v3 label Mar 4, 2025

richherrera self-assigned this Mar 4, 2025

richherrera requested a review from a team as a code owner March 4, 2025 18:39

jaxdesmarais reviewed Mar 5, 2025

View reviewed changes

warmkesselj reviewed Mar 5, 2025

View reviewed changes

Sources/PopupBridge/AnalyticsService.swift Outdated Show resolved Hide resolved

Strongly type FPTIBatchData

9a5e980

warmkesselj reviewed Mar 6, 2025

View reviewed changes

Sources/PopupBridge/AnalyticsService.swift Outdated Show resolved Hide resolved

warmkesselj reviewed Mar 6, 2025

View reviewed changes

Sources/PopupBridge/FPTIBatchData.swift Outdated Show resolved Hide resolved

Add catch block

a658633

jaxdesmarais reviewed Mar 7, 2025

View reviewed changes

jaxdesmarais approved these changes Mar 10, 2025

View reviewed changes

Sources/PopupBridge/FPTIBatchData.swift Outdated Show resolved Hide resolved

Update Sources/PopupBridge/FPTIBatchData.swift

70bb98e

Co-authored-by: Jax DesMarais-Leder <jdesmarais@paypal.com>

warmkesselj reviewed Mar 12, 2025

View reviewed changes

UnitTests/MockNetworkClient.swift Outdated Show resolved Hide resolved

scannillo reviewed Mar 13, 2025

View reviewed changes

richherrera added 5 commits March 14, 2025 11:06

Add analytics folder

c8fd4cf

Remove test plan

5911546

Address feedback

7bc9147

Merge branch 'v3' into add-networking

39a61a6

Sort files

ec82e7c

jaxdesmarais approved these changes Mar 19, 2025

View reviewed changes

richherrera merged commit 6b53ebf into v3 Mar 20, 2025
4 checks passed

richherrera deleted the add-networking branch March 20, 2025 15:26

jaxdesmarais mentioned this pull request May 6, 2025

Send analytics events in PopupBridgeClient braintree/popup-bridge-android#81

Merged

1 task

[V3] Add Networking to Send Analytics #75

[V3] Add Networking to Send Analytics #75

Uh oh!

Conversation

richherrera commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Checklist

Authors

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richherrera Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaxdesmarais left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

richherrera commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richherrera commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

richherrera commented Mar 4, 2025 •

edited

Loading

richherrera Mar 6, 2025 •

edited

Loading