From 94e30ff62061c354f8f10dd00f88d5b6e441b765 Mon Sep 17 00:00:00 2001 From: Graeme Arthur Date: Mon, 11 Nov 2024 14:50:03 +0100 Subject: [PATCH 1/3] Send success pixel on successful data import (#3437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task/Issue URL: https://app.asana.com/0/1203822806345703/1207768516988587/f **Description**: During [✓ Postmortem: Deduplicate passwords on import](https://app.asana.com/0/0/1207598056251596) it was mentioned that having insight on the rate of import success vs failure would be useful. We already have Pixels for failures but cannot calculate a success rate from that alone. This adds import success Pixels with source and make sure the failure Pixels and their source parameter are being sent properly. **Steps to test this PR**: 1. Run this with the debugger attached 2. Perform password / bookmark / favicon imports from all the import sources (or at least a sensible subset) 3. Check that, on successful import, the pixel: m_mac_data-import-succeeded_{action}_{source} is sent **Definition of Done**: * [ ] Does this PR satisfy our [Definition of Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)? --- ###### Internal references: [Pull Request Review Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f) [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) [Pull Request Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f) --- .../Bookmarks/Safari/SafariDataImporter.swift | 4 +++- .../Logins/Chromium/ChromiumDataImporter.swift | 5 +++-- .../Logins/Firefox/FirefoxDataImporter.swift | 4 +++- .../DataImport/Model/DataImportViewModel.swift | 4 +++- DuckDuckGo/Statistics/GeneralPixel.swift | 12 ++++++++++++ 5 files changed, 24 insertions(+), 5 deletions(-) diff --git a/DuckDuckGo/DataImport/Bookmarks/Safari/SafariDataImporter.swift b/DuckDuckGo/DataImport/Bookmarks/Safari/SafariDataImporter.swift index 0f3daaef5d..ddf719b5f1 100644 --- a/DuckDuckGo/DataImport/Bookmarks/Safari/SafariDataImporter.swift +++ b/DuckDuckGo/DataImport/Bookmarks/Safari/SafariDataImporter.swift @@ -96,6 +96,7 @@ final class SafariDataImporter: DataImporter { private func importFavicons(from dataDirectoryURL: URL) async { let faviconsReader = SafariFaviconsReader(safariDataDirectoryURL: dataDirectoryURL) let faviconsResult = faviconsReader.readFavicons() + let sourceVersion = profile.installedAppsMajorVersionDescription() switch faviconsResult { case .success(let faviconsByURL): @@ -112,9 +113,10 @@ final class SafariDataImporter: DataImporter { result[pageURL] = favicons } await faviconManager.handleFaviconsByDocumentUrl(faviconsByDocument) + PixelKit.fire(GeneralPixel.dataImportSucceeded(action: .favicons, source: source, sourceVersion: sourceVersion)) case .failure(let error): - PixelKit.fire(GeneralPixel.dataImportFailed(source: source, sourceVersion: profile.installedAppsMajorVersionDescription(), error: error)) + PixelKit.fire(GeneralPixel.dataImportFailed(source: source, sourceVersion: sourceVersion, error: error)) } } diff --git a/DuckDuckGo/DataImport/Logins/Chromium/ChromiumDataImporter.swift b/DuckDuckGo/DataImport/Logins/Chromium/ChromiumDataImporter.swift index 1f920dd809..8db3ac1878 100644 --- a/DuckDuckGo/DataImport/Logins/Chromium/ChromiumDataImporter.swift +++ b/DuckDuckGo/DataImport/Logins/Chromium/ChromiumDataImporter.swift @@ -122,6 +122,7 @@ internal class ChromiumDataImporter: DataImporter { private func importFavicons() async { let faviconsReader = ChromiumFaviconsReader(chromiumDataDirectoryURL: profile.profileURL) let faviconsResult = faviconsReader.readFavicons() + let sourceVersion = profile.installedAppsMajorVersionDescription() switch faviconsResult { case .success(let faviconsByURL): @@ -138,9 +139,9 @@ internal class ChromiumDataImporter: DataImporter { result[pageURL] = favicons } await faviconManager.handleFaviconsByDocumentUrl(faviconsByDocument) - + PixelKit.fire(GeneralPixel.dataImportSucceeded(action: .favicons, source: source, sourceVersion: sourceVersion)) case .failure(let error): - PixelKit.fire(GeneralPixel.dataImportFailed(source: source, sourceVersion: profile.installedAppsMajorVersionDescription(), error: error)) + PixelKit.fire(GeneralPixel.dataImportFailed(source: source, sourceVersion: sourceVersion, error: error)) } } diff --git a/DuckDuckGo/DataImport/Logins/Firefox/FirefoxDataImporter.swift b/DuckDuckGo/DataImport/Logins/Firefox/FirefoxDataImporter.swift index 5eb7ca6353..ed42e8740e 100644 --- a/DuckDuckGo/DataImport/Logins/Firefox/FirefoxDataImporter.swift +++ b/DuckDuckGo/DataImport/Logins/Firefox/FirefoxDataImporter.swift @@ -122,6 +122,7 @@ internal class FirefoxDataImporter: DataImporter { private func importFavicons() async { let faviconsReader = FirefoxFaviconsReader(firefoxDataDirectoryURL: profile.profileURL) let faviconsResult = faviconsReader.readFavicons() + let sourceVersion = profile.installedAppsMajorVersionDescription() switch faviconsResult { case .success(let faviconsByURL): @@ -138,9 +139,10 @@ internal class FirefoxDataImporter: DataImporter { result[pageURL] = favicons } await faviconManager.handleFaviconsByDocumentUrl(faviconsByDocument) + PixelKit.fire(GeneralPixel.dataImportSucceeded(action: .favicons, source: source, sourceVersion: sourceVersion)) case .failure(let error): - PixelKit.fire(GeneralPixel.dataImportFailed(source: source, sourceVersion: profile.installedAppsMajorVersionDescription(), error: error)) + PixelKit.fire(GeneralPixel.dataImportFailed(source: source, sourceVersion: sourceVersion, error: error)) } } diff --git a/DuckDuckGo/DataImport/Model/DataImportViewModel.swift b/DuckDuckGo/DataImport/Model/DataImportViewModel.swift index 3660f02777..254295f6d5 100644 --- a/DuckDuckGo/DataImport/Model/DataImportViewModel.swift +++ b/DuckDuckGo/DataImport/Model/DataImportViewModel.swift @@ -245,12 +245,14 @@ struct DataImportViewModel { var nextScreen: Screen? // merge new import results into the model import summary keeping the original DataType sorting order for (dataType, result) in DataType.allCases.compactMap({ dataType in summary[dataType].map { (dataType, $0) } }) { + let sourceVersion = importSource.installedAppsMajorVersionDescription(selectedProfile: selectedProfile) switch result { case .success(let dataTypeSummary): // if a data type can‘t be imported (Yandex/Passwords) - switch to its file import displaying successful import results if dataTypeSummary.isEmpty, !(screen.isFileImport && screen.fileImportDataType == dataType), nextScreen == nil { nextScreen = .fileImport(dataType: dataType, summary: Set(summary.filter({ $0.value.isSuccess }).keys)) } + PixelKit.fire(GeneralPixel.dataImportSucceeded(action: .init(dataType), source: importSource, sourceVersion: sourceVersion)) case .failure(let error): // successful imports are appended above self.summary.append( .init(dataType, result) ) @@ -260,7 +262,7 @@ struct DataImportViewModel { // switch to file import of the failed data type displaying successful import results nextScreen = .fileImport(dataType: dataType, summary: Set(summary.filter({ $0.value.isSuccess }).keys)) } - PixelKit.fire(GeneralPixel.dataImportFailed(source: importSource, sourceVersion: importSource.installedAppsMajorVersionDescription(selectedProfile: selectedProfile), error: error)) + PixelKit.fire(GeneralPixel.dataImportFailed(source: importSource, sourceVersion: sourceVersion, error: error)) } } diff --git a/DuckDuckGo/Statistics/GeneralPixel.swift b/DuckDuckGo/Statistics/GeneralPixel.swift index d7861dd042..2f5a897199 100644 --- a/DuckDuckGo/Statistics/GeneralPixel.swift +++ b/DuckDuckGo/Statistics/GeneralPixel.swift @@ -37,6 +37,7 @@ enum GeneralPixel: PixelKitEventV2 { case dailyOsVersionCounter case dataImportFailed(source: DataImport.Source, sourceVersion: String?, error: any DataImportError) + case dataImportSucceeded(action: DataImportAction, source: DataImport.Source, sourceVersion: String?) case formAutofilled(kind: FormAutofillKind) case autofillItemSaved(kind: FormAutofillKind) @@ -474,6 +475,9 @@ enum GeneralPixel: PixelKitEventV2 { case .dataImportFailed(source: let source, sourceVersion: _, error: let error): return "m_mac_data-import-failed_\(error.action)_\(source)" + case .dataImportSucceeded(action: let action, source: let source, sourceVersion: _): + return "m_mac_data-import-succeeded_\(action)_\(source)" + case .formAutofilled(kind: let kind): return "m_mac_autofill_\(kind)" @@ -1134,6 +1138,14 @@ enum GeneralPixel: PixelKitEventV2 { } return params + case .dataImportSucceeded(action: _, source: _, sourceVersion: let version): + var params = [String: String]() + + if let version { + params[PixelKit.Parameters.sourceBrowserVersion] = version + } + return params + case .launchInitial(let cohort): return [PixelKit.Parameters.experimentCohort: cohort] From 564ea8cf0bfdfa861a22fd8f03f28bf2062db345 Mon Sep 17 00:00:00 2001 From: Graeme Arthur Date: Mon, 11 Nov 2024 16:55:45 +0100 Subject: [PATCH 2/3] Sync: Send pixels for account removal + decoding issues (#3530) Task/Issue URL: https://app.asana.com/0/1201493110486074/1208723035104886/f Tech Design URL: CC: **Description**: Adds sync errors for catching when the keychain read throws a decoding error and when the account is deleted for various reasons Steps to test this PR: 1. Make sure you've already activated sync. 2. Deactivate sync (either delete the server data or just turn it off) 3. You should see the sync_account_removed_reason_user-turned-off pixel in the console. **Definition of Done**: * [ ] Does this PR satisfy our [Definition of Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)? --- ###### Internal references: [Pull Request Review Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f) [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) [Pull Request Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f) --- DuckDuckGo.xcodeproj/project.pbxproj | 31 +------------------ .../xcshareddata/swiftpm/Package.resolved | 8 ++--- DuckDuckGo/Statistics/GeneralPixel.swift | 5 +++ DuckDuckGo/Sync/SyncErrorHandler.swift | 16 +++++++--- .../DataBrokerProtection/Package.swift | 2 +- .../NetworkProtectionMac/Package.swift | 2 +- LocalPackages/SubscriptionUI/Package.swift | 2 +- 7 files changed, 25 insertions(+), 41 deletions(-) diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj index c6874ad10a..e295b7b2a8 100644 --- a/DuckDuckGo.xcodeproj/project.pbxproj +++ b/DuckDuckGo.xcodeproj/project.pbxproj @@ -15024,31 +15024,7 @@ repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit"; requirement = { kind = exactVersion; - version = 208.0.0; - }; - }; - 9D84E4002CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "OHHTTPStubs" */ = { - isa = XCRemoteSwiftPackageReference; - repositoryURL = "https://github.com/AliSoftware/OHHTTPStubs.git"; - requirement = { - kind = exactVersion; - version = 9.1.0; - }; - }; - 9D84E4032CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "swift-snapshot-testing" */ = { - isa = XCRemoteSwiftPackageReference; - repositoryURL = "https://github.com/pointfreeco/swift-snapshot-testing"; - requirement = { - kind = exactVersion; - version = 1.15.4; - }; - }; - 9D84E4052CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "BrowserServicesKit" */ = { - isa = XCRemoteSwiftPackageReference; - repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit"; - requirement = { - kind = exactVersion; - version = 200.3.0; + version = 208.1.0; }; }; 9FF521422BAA8FF300B9819B /* XCRemoteSwiftPackageReference "lottie-spm" */ = { @@ -15650,22 +15626,18 @@ }; 9D84E3FF2CD4E66F0046CD8B /* OHHTTPStubs */ = { isa = XCSwiftPackageProductDependency; - package = 9D84E4002CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "OHHTTPStubs" */; productName = OHHTTPStubs; }; 9D84E4012CD4E66F0046CD8B /* OHHTTPStubsSwift */ = { isa = XCSwiftPackageProductDependency; - package = 9D84E4002CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "OHHTTPStubs" */; productName = OHHTTPStubsSwift; }; 9D84E4022CD4E66F0046CD8B /* SnapshotTesting */ = { isa = XCSwiftPackageProductDependency; - package = 9D84E4032CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "swift-snapshot-testing" */; productName = SnapshotTesting; }; 9D84E4042CD4E66F0046CD8B /* PixelKitTestingUtilities */ = { isa = XCSwiftPackageProductDependency; - package = 9D84E4052CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "BrowserServicesKit" */; productName = PixelKitTestingUtilities; }; 9D84E4062CD4E66F0046CD8B /* AppKitExtensions */ = { @@ -15674,7 +15646,6 @@ }; 9D84E4072CD4E66F0046CD8B /* Common */ = { isa = XCSwiftPackageProductDependency; - package = 9D84E4052CD4E66F0046CD8B /* XCRemoteSwiftPackageReference "BrowserServicesKit" */; productName = Common; }; 9D9AE8F82AAA3AD00026E7DC /* DataBrokerProtection */ = { diff --git a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index 1bc6693de4..1659b95a86 100644 --- a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -32,8 +32,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/duckduckgo/BrowserServicesKit", "state" : { - "revision" : "17154907fe86c75942331ed6d037694c666ddd95", - "version" : "208.0.0" + "revision" : "6be781530a2516c703b8e1bcf0c90e6e763d3300", + "version" : "208.1.0" } }, { @@ -41,8 +41,8 @@ "kind" : "remoteSourceControl", "location" : "https://github.com/duckduckgo/content-scope-scripts", "state" : { - "revision" : "1733ee59f06f6e725a98cf6cd8322159f59d664b", - "version" : "6.31.0" + "revision" : "adca39c379b1a124f9990e9d0308c374f32f5018", + "version" : "6.32.0" } }, { diff --git a/DuckDuckGo/Statistics/GeneralPixel.swift b/DuckDuckGo/Statistics/GeneralPixel.swift index 2f5a897199..095bc239c0 100644 --- a/DuckDuckGo/Statistics/GeneralPixel.swift +++ b/DuckDuckGo/Statistics/GeneralPixel.swift @@ -414,6 +414,8 @@ enum GeneralPixel: PixelKitEventV2 { case syncLoginExistingAccountError(error: Error) case syncCannotCreateRecoveryPDF case syncSecureStorageReadError(error: Error) + case syncSecureStorageDecodingError(error: Error) + case syncAccountRemoved(reason: String) case bookmarksCleanupFailed case bookmarksCleanupAttemptedWhileSyncWasEnabled @@ -1058,6 +1060,8 @@ enum GeneralPixel: PixelKitEventV2 { case .syncLoginExistingAccountError: return "sync_login_existing_account_error" case .syncCannotCreateRecoveryPDF: return "sync_cannot_create_recovery_pdf" case .syncSecureStorageReadError: return "sync_secure_storage_read_error" + case .syncSecureStorageDecodingError: return "sync_secure_storage_decoding_error" + case .syncAccountRemoved(let reason): return "sync_account_removed_reason_\(reason)" case .bookmarksCleanupFailed: return "bookmarks_cleanup_failed" case .bookmarksCleanupAttemptedWhileSyncWasEnabled: return "bookmarks_cleanup_attempted_while_sync_was_enabled" @@ -1119,6 +1123,7 @@ enum GeneralPixel: PixelKitEventV2 { .syncDeleteAccountError(let error), .syncLoginExistingAccountError(let error), .syncSecureStorageReadError(let error), + .syncSecureStorageDecodingError(let error), .bookmarksCouldNotLoadDatabase(let error?): return error default: return nil diff --git a/DuckDuckGo/Sync/SyncErrorHandler.swift b/DuckDuckGo/Sync/SyncErrorHandler.swift index beca1a0e2c..1f6c8f3522 100644 --- a/DuckDuckGo/Sync/SyncErrorHandler.swift +++ b/DuckDuckGo/Sync/SyncErrorHandler.swift @@ -88,18 +88,26 @@ public class SyncErrorHandler: EventMapping, ObservableObject { let alertPresenter: SyncAlertsPresenting - public init(alertPresenter: SyncAlertsPresenting = SyncAlertsPresenter()) { - self.alertPresenter = alertPresenter - super.init { event, _, _, _ in + static var errorHandlerMapping: Mapping { + return { event, _, _, _ in switch event { - case .failedToReadSecureStore(let status): + case .failedToReadSecureStore: PixelKit.fire(DebugEvent(GeneralPixel.syncSecureStorageReadError(error: event), error: event)) + case .failedToDecodeSecureStoreData(let error): + PixelKit.fire(DebugEvent(GeneralPixel.syncSecureStorageDecodingError(error: error), error: error)) + case .accountRemoved(let reason): + PixelKit.fire(DebugEvent(GeneralPixel.syncAccountRemoved(reason: reason.rawValue), error: event)) default: PixelKit.fire(DebugEvent(GeneralPixel.syncSentUnauthenticatedRequest, error: event)) } } } + public init(alertPresenter: SyncAlertsPresenting = SyncAlertsPresenter()) { + self.alertPresenter = alertPresenter + super.init(mapping: Self.errorHandlerMapping) + } + override init(mapping: @escaping EventMapping.Mapping) { fatalError("Use init()") } diff --git a/LocalPackages/DataBrokerProtection/Package.swift b/LocalPackages/DataBrokerProtection/Package.swift index 1ca148fcbd..979cbc8746 100644 --- a/LocalPackages/DataBrokerProtection/Package.swift +++ b/LocalPackages/DataBrokerProtection/Package.swift @@ -29,7 +29,7 @@ let package = Package( targets: ["DataBrokerProtection"]) ], dependencies: [ - .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "208.0.0"), + .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "208.1.0"), .package(path: "../SwiftUIExtensions"), .package(path: "../AppKitExtensions"), .package(path: "../XPCHelper"), diff --git a/LocalPackages/NetworkProtectionMac/Package.swift b/LocalPackages/NetworkProtectionMac/Package.swift index 10a3daafbf..4177532a74 100644 --- a/LocalPackages/NetworkProtectionMac/Package.swift +++ b/LocalPackages/NetworkProtectionMac/Package.swift @@ -32,7 +32,7 @@ let package = Package( .library(name: "VPNAppLauncher", targets: ["VPNAppLauncher"]), ], dependencies: [ - .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "208.0.0"), + .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "208.1.0"), .package(url: "https://github.com/airbnb/lottie-spm", exact: "4.4.3"), .package(path: "../AppLauncher"), .package(path: "../UDSHelper"), diff --git a/LocalPackages/SubscriptionUI/Package.swift b/LocalPackages/SubscriptionUI/Package.swift index 5674c884bc..fa22cbe544 100644 --- a/LocalPackages/SubscriptionUI/Package.swift +++ b/LocalPackages/SubscriptionUI/Package.swift @@ -12,7 +12,7 @@ let package = Package( targets: ["SubscriptionUI"]), ], dependencies: [ - .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "208.0.0"), + .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "208.1.0"), .package(path: "../SwiftUIExtensions") ], targets: [ From 03ef4f9ad6d1ac737f982ef30da600a0c75df367 Mon Sep 17 00:00:00 2001 From: Dax the Duck Date: Tue, 12 Nov 2024 06:35:42 +0000 Subject: [PATCH 3/3] Bump version to 1.114.0 (305) --- Configuration/BuildNumber.xcconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Configuration/BuildNumber.xcconfig b/Configuration/BuildNumber.xcconfig index 27403d57b0..60e2adb5be 100644 --- a/Configuration/BuildNumber.xcconfig +++ b/Configuration/BuildNumber.xcconfig @@ -1 +1 @@ -CURRENT_PROJECT_VERSION = 304 +CURRENT_PROJECT_VERSION = 305