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

Remove ATB from default params #2430

Merged
merged 12 commits into from
Apr 30, 2024
Merged
4 changes: 2 additions & 2 deletions Core/DailyPixel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions Core/Pixel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 2 additions & 1 deletion Core/UniquePixel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,15 @@ 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],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to the UniquePixel API so that the caller can control the parameters, before this it was relying on the default parameters.

onComplete: @escaping (Swift.Error?) -> Void = { _ in }) {
guard pixel.name.hasSuffix("_u") else {
assertionFailure("Unique pixel: must end with _u")
return
}

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)
Expand Down
12 changes: 8 additions & 4 deletions DuckDuckGo/AdAttribution/AdAttributionPixelReporter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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]
)
Comment on lines +55 to +59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attribution feature requires ATB, so I've added it and updated its tests. cc @dus7.

} catch {
return false
}
Expand Down Expand Up @@ -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<Void, Error>) 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 {
Expand Down
4 changes: 2 additions & 2 deletions DuckDuckGo/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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]()) {
Expand All @@ -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])
}

}
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/Feedback/VPNFeedbackSender.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions DuckDuckGo/NetworkProtectionTunnelController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/TabViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
18 changes: 17 additions & 1 deletion DuckDuckGoTests/AdAttributionPixelReporterTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -184,6 +199,7 @@ final actor PixelFiringMock: PixelFiring {
static func tearDown() {
lastParams = nil
lastPixel = nil
lastIncludedParams = nil
expectedFireError = nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down