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

Add data import failure pixels #552

Merged
merged 6 commits into from
Apr 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,11 @@ internal class SafariDataImporter: DataImporter {
do {
summary.bookmarksResult = try bookmarkImporter.importBookmarks(bookmarks, source: .safari)
} catch {
completion(.failure(.cannotAccessSecureVault))
completion(.failure(.bookmarks(.cannotAccessCoreData)))
return
}
case .failure:
completion(.failure(.browserNeedsToBeClosed))
completion(.failure(.bookmarks(.browserNeedsToBeClosed)))
return
}
}
Expand Down
76 changes: 66 additions & 10 deletions DuckDuckGo/Data Import/DataImport.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import AppKit
enum DataImport {

enum Source: CaseIterable {

case brave
case chrome
case edge
Expand Down Expand Up @@ -60,6 +61,19 @@ enum DataImport {
var canImportData: Bool {
return (ThirdPartyBrowser.browser(for: self)?.isInstalled ?? false) || [.csv, .onePassword, .lastPass].contains(self)
}

var pixelEventSource: Pixel.Event.DataImportSource {
switch self {
case .brave: return .brave
case .chrome: return .chrome
case .edge: return .edge
case .firefox: return .firefox
case .safari: return .safari
case .onePassword: return .onePassword
case .lastPass: return .lastPass
case .csv: return .csv
}
}
}

enum DataType {
Expand Down Expand Up @@ -212,20 +226,55 @@ enum DataImport {

}

enum DataImportError: Error {
struct DataImportError: Error {

enum ImportErrorAction {
case bookmarks
case logins
case generic

var pixelEventAction: Pixel.Event.DataImportAction {
switch self {
case .bookmarks: return .importBookmarks
case .logins: return .importLogins
case .generic: return .generic
}
}
}

enum ImportErrorType: String {
case noFileFound
case cannotReadFile
case couldNotFindProfile
case browserNeedsToBeClosed
case needsLoginPrimaryPassword
case cannotAccessSecureVault
case cannotAccessCoreData
case couldNotGetDecryptionKey
case cannotDecryptFile
}

static func generic(_ errorType: ImportErrorType) -> DataImportError {
return DataImportError(actionType: .generic, errorType: errorType)
}

case noFileFound
case cannotReadFile
case browserNeedsToBeClosed
case needsLoginPrimaryPassword
case cannotAccessSecureVault
case unknownError(Error)
static func bookmarks(_ errorType: ImportErrorType) -> DataImportError {
return DataImportError(actionType: .bookmarks, errorType: errorType)
}

static func logins(_ errorType: ImportErrorType) -> DataImportError {
return DataImportError(actionType: .logins, errorType: errorType)
}

let actionType: ImportErrorAction
let errorType: ImportErrorType

}

extension DataImportError: LocalizedError {

public var errorDescription: String? {
switch self {
switch self.errorType {
case .noFileFound:
return "Could not find file"
case .cannotReadFile:
Expand All @@ -236,10 +285,17 @@ extension DataImportError: LocalizedError {
return "Failed to get primary password"
case .cannotAccessSecureVault:
return "Failed to read Secure Vault data"
case .unknownError(let error):
return error.localizedDescription
samsymons marked this conversation as resolved.
Show resolved Hide resolved
case .cannotAccessCoreData:
return "Failed to access Bookmarks database"
case .couldNotFindProfile:
return "Could not find browser profile"
case .couldNotGetDecryptionKey:
return "Could not read decryption key"
case .cannotDecryptFile:
return "Could not decrypt file"
}
}

}

/// Represents an object able to import data from an outside source. The outside source may be capable of importing multiple types of data.
Expand Down
6 changes: 3 additions & 3 deletions DuckDuckGo/Data Import/Logins/CSV/CSVImporter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,11 @@ final class CSVImporter: DataImporter {
from profile: DataImport.BrowserProfile?,
completion: @escaping (Result<DataImport.Summary, DataImportError>) -> Void) {
guard let fileContents = try? String(contentsOf: fileURL, encoding: .utf8) else {
completion(.failure(.cannotReadFile))
completion(.failure(.logins(.cannotReadFile)))
return
}
guard let loginImporter = self.loginImporter else {
completion(.failure(.cannotAccessSecureVault))
completion(.failure(.logins(.cannotAccessSecureVault)))
return
}

Expand All @@ -144,7 +144,7 @@ final class CSVImporter: DataImporter {
DispatchQueue.main.async { completion(.success(.init(bookmarksResult: nil,
loginsResult: .completed(result)))) }
} catch {
DispatchQueue.main.async { completion(.failure(.cannotAccessSecureVault)) }
DispatchQueue.main.async { completion(.failure(.bookmarks(.cannotAccessSecureVault))) }
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ internal class ChromiumDataImporter: DataImporter {
func importData(types: [DataImport.DataType],
from profile: DataImport.BrowserProfile?,
completion: @escaping (Result<DataImport.Summary, DataImportError>) -> Void) {

var summary = DataImport.Summary()
let dataDirectoryPath = profile?.profileURL.path ?? applicationDataDirectoryPath

Expand All @@ -53,11 +54,11 @@ internal class ChromiumDataImporter: DataImporter {
do {
summary.loginsResult = .completed(try loginImporter.importLogins(logins))
} catch {
completion(.failure(.cannotAccessSecureVault))
completion(.failure(.logins(.cannotAccessSecureVault)))
return
}
case .failure:
completion(.failure(.browserNeedsToBeClosed))
completion(.failure(.logins(.browserNeedsToBeClosed)))
return
}
}
Expand All @@ -71,7 +72,7 @@ internal class ChromiumDataImporter: DataImporter {
do {
summary.bookmarksResult = try bookmarkImporter.importBookmarks(bookmarks, source: .chromium)
} catch {
completion(.failure(.cannotAccessSecureVault))
completion(.failure(.bookmarks(.cannotAccessSecureVault)))
return
}
case .failure(let error):
Expand All @@ -82,7 +83,7 @@ internal class ChromiumDataImporter: DataImporter {
summary.bookmarksResult = result

case .bookmarksFileDecodingFailed:
completion(.failure(.cannotReadFile))
completion(.failure(.bookmarks(.cannotReadFile)))
return
}
}
Expand Down
25 changes: 15 additions & 10 deletions DuckDuckGo/Data Import/Logins/Firefox/FirefoxDataImporter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ final class FirefoxDataImporter: DataImporter {
return [.logins, .bookmarks]
}

// swiftlint:disable cyclomatic_complexity
// swiftlint:disable:next cyclomatic_complexity function_body_length
func importData(types: [DataImport.DataType],
from profile: DataImport.BrowserProfile?,
completion: @escaping (Result<DataImport.Summary, DataImportError>) -> Void) {
guard let firefoxProfileURL = profile?.profileURL ?? defaultFirefoxProfilePath() else {
completion(.failure(.cannotReadFile))
completion(.failure(.generic(.cannotReadFile)))
return
}

Expand All @@ -54,16 +54,22 @@ final class FirefoxDataImporter: DataImporter {
do {
summary.loginsResult = .completed(try loginImporter.importLogins(logins))
} catch {
completion(.failure(.cannotAccessSecureVault))
completion(.failure(.logins(.cannotAccessSecureVault)))
}
case .failure(let error):
switch error {
case .requiresPrimaryPassword:
completion(.failure(.needsLoginPrimaryPassword))
completion(.failure(.logins(.needsLoginPrimaryPassword)))
case .databaseAccessFailed:
completion(.failure(.browserNeedsToBeClosed))
default:
completion(.failure(.unknownError(error)))
completion(.failure(.logins(.browserNeedsToBeClosed)))
case .couldNotFindProfile:
completion(.failure(.logins(.couldNotFindProfile)))
case .couldNotGetDecryptionKey:
completion(.failure(.logins(.couldNotGetDecryptionKey)))
case .couldNotReadLoginsFile:
completion(.failure(.logins(.cannotReadFile)))
case .decryptionFailed:
completion(.failure(.logins(.cannotDecryptFile)))
}
}
}
Expand All @@ -77,18 +83,17 @@ final class FirefoxDataImporter: DataImporter {
do {
summary.bookmarksResult = try bookmarkImporter.importBookmarks(bookmarks, source: .firefox)
} catch {
completion(.failure(.cannotAccessSecureVault))
completion(.failure(.bookmarks(.cannotAccessSecureVault)))
return
}
case .failure:
completion(.failure(.browserNeedsToBeClosed))
completion(.failure(.bookmarks(.browserNeedsToBeClosed)))
return
}
}

completion(.success(summary))
}
// swiftlint:enable cyclomatic_complexity

private func defaultFirefoxProfilePath() -> URL? {
guard let potentialProfiles = try? FileManager.default.contentsOfDirectory(atPath: profilesDirectoryURL().path) else {
Expand Down
18 changes: 13 additions & 5 deletions DuckDuckGo/Data Import/View/DataImportViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ final class DataImportViewController: NSViewController {
}
} catch {
os_log("dataImporter initialization failed: %{public}s", type: .error, error.localizedDescription)
self.presentAlert(for: .cannotAccessSecureVault)
self.presentAlert(for: .generic(.cannotAccessSecureVault))
}
}
}
Expand Down Expand Up @@ -400,7 +400,7 @@ final class DataImportViewController: NSViewController {

NotificationCenter.default.post(name: .dataImportComplete, object: nil)
case .failure(let error):
switch error {
switch error.errorType {
case .needsLoginPrimaryPassword:
self.presentAlert(for: error)
default:
Expand All @@ -414,7 +414,7 @@ final class DataImportViewController: NSViewController {
private func presentAlert(for error: DataImportError) {
guard let window = view.window else { return }

switch error {
switch error.errorType {
case .needsLoginPrimaryPassword:
let alert = NSAlert.passwordRequiredAlert(source: viewState.selectedImportSource)
let response = alert.runModal()
Expand All @@ -427,6 +427,14 @@ final class DataImportViewController: NSViewController {
completeImport()
}
default:
let pixel = Pixel.Event.dataImportFailed(
action: error.actionType.pixelEventAction,
source: viewState.selectedImportSource.pixelEventSource
)

let parameters = ["error": error.errorType.rawValue]
Pixel.fire(pixel, withAdditionalParameters: parameters)

let alert = NSAlert.importFailedAlert(source: viewState.selectedImportSource, errorMessage: error.localizedDescription)
alert.beginSheetModal(for: window, completionHandler: nil)
}
Expand All @@ -449,7 +457,7 @@ final class DataImportViewController: NSViewController {
switch self.viewState.selectedImportSource {
case .brave: Pixel.fire(.importedBookmarks(source: .brave))
case .chrome: Pixel.fire(.importedBookmarks(source: .chrome))
case .csv, .lastPass, .onePassword: assertionFailure("Attempted to fire CSV bookmark import pixel")
case .csv, .lastPass, .onePassword: assertionFailure("Attempted to fire invalid bookmark import pixel")
case .edge: Pixel.fire(.importedBookmarks(source: .edge))
case .firefox: Pixel.fire(.importedBookmarks(source: .firefox))
case .safari: Pixel.fire(.importedBookmarks(source: .safari))
Expand All @@ -476,7 +484,7 @@ extension DataImportViewController: CSVImportViewControllerDelegate {
self.viewState.interactionState = .ableToImport
} catch {
self.viewState.interactionState = .unableToImport
self.presentAlert(for: .cannotAccessSecureVault)
self.presentAlert(for: .logins(.cannotAccessSecureVault))
}
}

Expand Down
8 changes: 8 additions & 0 deletions DuckDuckGo/Statistics/PixelArguments.swift
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,14 @@ extension Pixel.Event {
}
}

enum DataImportAction: String, CustomStringConvertible {
var description: String { rawValue }

case importBookmarks = "bookmarks"
case importLogins = "logins"
case generic = "generic"
}

enum DataImportSource: String, CustomStringConvertible {
var description: String { rawValue }

Expand Down
5 changes: 5 additions & 0 deletions DuckDuckGo/Statistics/PixelEvent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ extension Pixel {
case exportedLogins(repetition: Repetition = .init(key: "exported-logins"))
case importedBookmarks(repetition: Repetition = .init(key: "imported-bookmarks"), source: DataImportSource)
case exportedBookmarks(repetition: Repetition = .init(key: "exported-bookmarks"))

case dataImportFailed(action: DataImportAction, source: DataImportSource)

case formAutofilled(kind: FormAutofillKind)
case autofillItemSaved(kind: FormAutofillKind)
Expand Down Expand Up @@ -300,6 +302,9 @@ extension Pixel.Event {

case .exportedBookmarks(repetition: let repetition):
return "m_mac_exported-bookmarks_\(repetition)"

case .dataImportFailed(action: let action, source: let source):
return "m_mac_data-import-failed_\(action)_\(source)"

case .formAutofilled(kind: let kind):
return "m_mac_autofill_\(kind)"
Expand Down
1 change: 1 addition & 0 deletions DuckDuckGo/Statistics/PixelParameters.swift
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ extension Pixel.Event {
.exportedLogins,
.importedBookmarks,
.exportedBookmarks,
.dataImportFailed,
.formAutofilled,
.autofillItemSaved,
.onboardingStartPressed,
Expand Down