From 022fd6035dff8b224e7174a9cef57e2ef48e6da3 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Tue, 30 Apr 2024 13:05:02 -0700 Subject: [PATCH] Remove ATB from default params (#2430) Task/Issue URL: https://app.asana.com/0/414235014887631/1206435379975124/f Tech Design URL: CC: Description: This PR updates the Pixel and DailyPixel fire functions to change the default parameters. --- Core/DailyPixel.swift | 4 ++-- Core/Pixel.swift | 4 ++-- Core/UniquePixel.swift | 3 ++- .../AdAttributionPixelReporter.swift | 12 +++++++---- DuckDuckGo/AppDelegate.swift | 4 ++-- DuckDuckGo/Feedback/VPNFeedbackSender.swift | 2 +- .../NetworkProtectionTunnelController.swift | 8 ++++---- DuckDuckGo/TabViewController.swift | 2 +- .../AdAttributionPixelReporterTests.swift | 18 ++++++++++++++++- ...etworkProtectionPacketTunnelProvider.swift | 20 +++++++++++-------- 10 files changed, 51 insertions(+), 26 deletions(-) diff --git a/Core/DailyPixel.swift b/Core/DailyPixel.swift index 1d03a8c619..db0b5f0b33 100644 --- a/Core/DailyPixel.swift +++ b/Core/DailyPixel.swift @@ -51,7 +51,7 @@ public final class DailyPixel { public static func fire(pixel: Pixel.Event, error: Swift.Error? = nil, withAdditionalParameters params: [String: String] = [:], - includedParameters: [Pixel.QueryParameters] = [.atb, .appVersion], + includedParameters: [Pixel.QueryParameters] = [.appVersion], onComplete: @escaping (Swift.Error?) -> Void = { _ in }) { var key: String = pixel.name @@ -79,7 +79,7 @@ public final class DailyPixel { public static func fireDailyAndCount(pixel: Pixel.Event, error: Swift.Error? = nil, withAdditionalParameters params: [String: String] = [:], - includedParameters: [Pixel.QueryParameters] = [.atb, .appVersion], + includedParameters: [Pixel.QueryParameters] = [.appVersion], onDailyComplete: @escaping (Swift.Error?) -> Void = { _ in }, onCountComplete: @escaping (Swift.Error?) -> Void = { _ in }) { let key: String = pixel.name diff --git a/Core/Pixel.swift b/Core/Pixel.swift index 34539ccd98..48d6d22a32 100644 --- a/Core/Pixel.swift +++ b/Core/Pixel.swift @@ -179,7 +179,7 @@ public class Pixel { withAdditionalParameters params: [String: String] = [:], allowedQueryReservedCharacters: CharacterSet? = nil, withHeaders headers: APIRequest.Headers = APIRequest.Headers(), - includedParameters: [QueryParameters] = [.atb, .appVersion], + includedParameters: [QueryParameters] = [.appVersion], onComplete: @escaping (Error?) -> Void = { _ in }, debounce: Int = 0) { @@ -209,7 +209,7 @@ public class Pixel { withAdditionalParameters params: [String: String] = [:], allowedQueryReservedCharacters: CharacterSet? = nil, withHeaders headers: APIRequest.Headers = APIRequest.Headers(), - includedParameters: [QueryParameters] = [.atb, .appVersion], + includedParameters: [QueryParameters] = [.appVersion], onComplete: @escaping (Error?) -> Void = { _ in }) { var newParams = params if includedParameters.contains(.appVersion) { diff --git a/Core/UniquePixel.swift b/Core/UniquePixel.swift index 1032b6e6d2..5767d91a83 100644 --- a/Core/UniquePixel.swift +++ b/Core/UniquePixel.swift @@ -52,6 +52,7 @@ public final class UniquePixel { /// This requires the pixel name to end with `_u` public static func fire(pixel: Pixel.Event, withAdditionalParameters params: [String: String] = [:], + includedParameters: [Pixel.QueryParameters] = [.appVersion], onComplete: @escaping (Swift.Error?) -> Void = { _ in }) { guard pixel.name.hasSuffix("_u") else { assertionFailure("Unique pixel: must end with _u") @@ -59,7 +60,7 @@ public final class UniquePixel { } if !pixel.hasBeenFiredEver(uniquePixelStorage: storage) { - Pixel.fire(pixel: pixel, withAdditionalParameters: params, onComplete: onComplete) + Pixel.fire(pixel: pixel, withAdditionalParameters: params, includedParameters: includedParameters, onComplete: onComplete) storage.set(Date(), forKey: pixel.name) } else { onComplete(Error.alreadyFired) diff --git a/DuckDuckGo/AdAttribution/AdAttributionPixelReporter.swift b/DuckDuckGo/AdAttribution/AdAttributionPixelReporter.swift index 1cd3248691..a6330819b0 100644 --- a/DuckDuckGo/AdAttribution/AdAttributionPixelReporter.swift +++ b/DuckDuckGo/AdAttribution/AdAttributionPixelReporter.swift @@ -21,7 +21,7 @@ import Foundation import Core protocol PixelFiring { - static func fire(pixel: Pixel.Event, withAdditionalParameters params: [String: String]) async throws + static func fire(pixel: Pixel.Event, withAdditionalParameters params: [String: String], includedParameters: [Pixel.QueryParameters]) async throws } final class AdAttributionPixelReporter { @@ -52,7 +52,11 @@ final class AdAttributionPixelReporter { if attributionData.attribution { let parameters = self.pixelParametersForAttribution(attributionData) do { - try await pixelFiring.fire(pixel: .appleAdAttribution, withAdditionalParameters: parameters) + try await pixelFiring.fire( + pixel: .appleAdAttribution, + withAdditionalParameters: parameters, + includedParameters: [.appVersion, .atb] + ) } catch { return false } @@ -83,10 +87,10 @@ final class AdAttributionPixelReporter { } extension Pixel: PixelFiring { - static func fire(pixel: Event, withAdditionalParameters params: [String: String]) async throws { + static func fire(pixel: Event, withAdditionalParameters params: [String: String], includedParameters: [QueryParameters]) async throws { try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in - Pixel.fire(pixel: pixel, withAdditionalParameters: params) { error in + Pixel.fire(pixel: pixel, withAdditionalParameters: params, includedParameters: includedParameters) { error in if let error { continuation.resume(throwing: error) } else { diff --git a/DuckDuckGo/AppDelegate.swift b/DuckDuckGo/AppDelegate.swift index 05ee8b1793..a769c147bf 100644 --- a/DuckDuckGo/AppDelegate.swift +++ b/DuckDuckGo/AppDelegate.swift @@ -584,7 +584,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate { PixelParameters.widgetError: "1", PixelParameters.widgetErrorCode: "\((error as NSError).code)", PixelParameters.widgetErrorDomain: (error as NSError).domain - ]) + ], includedParameters: [.appVersion, .atb]) case .success(let widgetInfo): let params = widgetInfo.reduce([String: String]()) { @@ -594,7 +594,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate { } return result } - Pixel.fire(pixel: .appLaunch, withAdditionalParameters: params) + Pixel.fire(pixel: .appLaunch, withAdditionalParameters: params, includedParameters: [.appVersion, .atb]) } } diff --git a/DuckDuckGo/Feedback/VPNFeedbackSender.swift b/DuckDuckGo/Feedback/VPNFeedbackSender.swift index 4b80c1b961..869c99791f 100644 --- a/DuckDuckGo/Feedback/VPNFeedbackSender.swift +++ b/DuckDuckGo/Feedback/VPNFeedbackSender.swift @@ -34,7 +34,7 @@ struct DefaultVPNFeedbackSender: VPNFeedbackSender { "breakageCategory": category.rawValue, "breakageDescription": encodedUserText, "breakageMetadata": metadata.toBase64(), - ]) { error in + ], includedParameters: [.appVersion, .atb]) { error in if let error { continuation.resume(throwing: error) } else { diff --git a/DuckDuckGo/NetworkProtectionTunnelController.swift b/DuckDuckGo/NetworkProtectionTunnelController.swift index 5e0d632537..3aaedb8115 100644 --- a/DuckDuckGo/NetworkProtectionTunnelController.swift +++ b/DuckDuckGo/NetworkProtectionTunnelController.swift @@ -79,16 +79,16 @@ final class NetworkProtectionTunnelController: TunnelController { /// Starts the VPN connection used for Network Protection /// func start() async { - Pixel.fire(pixel: .networkProtectionControllerStartAttempt) + Pixel.fire(pixel: .networkProtectionControllerStartAttempt, includedParameters: [.appVersion, .atb]) do { try await startWithError() - Pixel.fire(pixel: .networkProtectionControllerStartSuccess) + Pixel.fire(pixel: .networkProtectionControllerStartSuccess, includedParameters: [.appVersion, .atb]) } catch { if case StartError.configSystemPermissionsDenied = error { return } - Pixel.fire(pixel: .networkProtectionControllerStartFailure, error: error) + Pixel.fire(pixel: .networkProtectionControllerStartFailure, error: error, includedParameters: [.appVersion, .atb]) #if DEBUG errorStore.lastErrorMessage = error.localizedDescription @@ -190,7 +190,7 @@ final class NetworkProtectionTunnelController: TunnelController { do { try tunnelManager.connection.startVPNTunnel(options: options) - UniquePixel.fire(pixel: .networkProtectionNewUser) { error in + UniquePixel.fire(pixel: .networkProtectionNewUser, includedParameters: [.appVersion, .atb]) { error in guard error != nil else { return } UserDefaults.networkProtectionGroupDefaults.vpnFirstEnabled = Pixel.Event.networkProtectionNewUser.lastFireDate( uniquePixelStorage: UniquePixel.storage diff --git a/DuckDuckGo/TabViewController.swift b/DuckDuckGo/TabViewController.swift index 2c34f7ee25..f75782be74 100644 --- a/DuckDuckGo/TabViewController.swift +++ b/DuckDuckGo/TabViewController.swift @@ -1186,7 +1186,7 @@ extension TabViewController: WKNavigationDelegate { #if NETWORK_PROTECTION if webView.url?.isDuckDuckGoSearch == true, case .connected = netPConnectionStatus { - DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnabledOnSearch) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnabledOnSearch, includedParameters: [.appVersion, .atb]) } #endif } diff --git a/DuckDuckGoTests/AdAttributionPixelReporterTests.swift b/DuckDuckGoTests/AdAttributionPixelReporterTests.swift index 97eef8546e..bc36e3a538 100644 --- a/DuckDuckGoTests/AdAttributionPixelReporterTests.swift +++ b/DuckDuckGoTests/AdAttributionPixelReporterTests.swift @@ -86,6 +86,17 @@ final class AdAttributionPixelReporterTests: XCTestCase { XCTAssertEqual(pixelAttributes["ad_id"], "5") } + func testPixelAdditionalParameters() async throws { + let sut = createSUT() + attributionFetcher.fetchResponse = AdServicesAttributionResponse(attribution: true) + + await sut.reportAttributionIfNeeded() + + let pixelAttributes = try XCTUnwrap(PixelFiringMock.lastIncludedParams) + + XCTAssertEqual(pixelAttributes, [.appVersion, .atb]) + } + func testPixelAttributes_WhenPartialAttributionData() async throws { let sut = createSUT() attributionFetcher.fetchResponse = AdServicesAttributionResponse( @@ -172,9 +183,13 @@ final actor PixelFiringMock: PixelFiring { static var lastParams: [String: String]? static var lastPixel: Pixel.Event? - static func fire(pixel: Pixel.Event, withAdditionalParameters params: [String: String]) async throws { + static var lastIncludedParams: [Pixel.QueryParameters]? + static func fire(pixel: Pixel.Event, + withAdditionalParameters params: [String: String], + includedParameters: [Pixel.QueryParameters]) async throws { lastParams = params lastPixel = pixel + lastIncludedParams = includedParameters if let expectedFireError { throw expectedFireError @@ -184,6 +199,7 @@ final actor PixelFiringMock: PixelFiring { static func tearDown() { lastParams = nil lastPixel = nil + lastIncludedParams = nil expectedFireError = nil } diff --git a/PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift b/PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift index 4d0be973a9..7eff1b1b18 100644 --- a/PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift +++ b/PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift @@ -43,35 +43,39 @@ final class NetworkProtectionPacketTunnelProvider: PacketTunnelProvider { switch event { case .userBecameActive: DailyPixel.fire(pixel: .networkProtectionActiveUser, - withAdditionalParameters: [PixelParameters.vpnCohort: UniquePixel.cohort(from: defaults.vpnFirstEnabled)]) + withAdditionalParameters: [PixelParameters.vpnCohort: UniquePixel.cohort(from: defaults.vpnFirstEnabled)], + includedParameters: [.appVersion, .atb]) case .reportConnectionAttempt(attempt: let attempt): switch attempt { case .connecting: - DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnableAttemptConnecting) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnableAttemptConnecting, + includedParameters: [.appVersion, .atb]) case .success: let versionStore = NetworkProtectionLastVersionRunStore(userDefaults: .networkProtectionGroupDefaults) versionStore.lastExtensionVersionRun = AppVersion.shared.versionAndBuildNumber - DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnableAttemptSuccess) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnableAttemptSuccess, + includedParameters: [.appVersion, .atb]) case .failure: - DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnableAttemptFailure) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionEnableAttemptFailure, + includedParameters: [.appVersion, .atb]) } case .reportTunnelFailure(result: let result): switch result { case .failureDetected: - DailyPixel.fireDailyAndCount(pixel: .networkProtectionTunnelFailureDetected) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionTunnelFailureDetected, includedParameters: [.appVersion, .atb]) case .failureRecovered: - DailyPixel.fireDailyAndCount(pixel: .networkProtectionTunnelFailureRecovered) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionTunnelFailureRecovered, includedParameters: [.appVersion, .atb]) case .networkPathChanged(let newPath): defaults.updateNetworkPath(with: newPath) } case .reportLatency(result: let result): switch result { case .error: - DailyPixel.fire(pixel: .networkProtectionLatencyError) + DailyPixel.fire(pixel: .networkProtectionLatencyError, includedParameters: [.appVersion, .atb]) case .quality(let quality): guard quality != .unknown else { return } - DailyPixel.fireDailyAndCount(pixel: .networkProtectionLatency(quality: quality)) + DailyPixel.fireDailyAndCount(pixel: .networkProtectionLatency(quality: quality), includedParameters: [.appVersion, .atb]) } case .rekeyAttempt(let step): switch step {