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

Improve Handling of noData Import Errors #2494

Merged
merged 22 commits into from
Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f9a856f
Implement == for DataTypeImportResult. Don't fire Pixel on `noData` e…
aataraxiaa Mar 21, 2024
e32b283
Update DataImportViewModel tests. Add noData tests.
aataraxiaa Mar 21, 2024
de7c066
Iterate on == impl, improve tests
aataraxiaa Mar 22, 2024
4ef9ada
Iterate on proposed UI updates and copy changes
aataraxiaa Mar 22, 2024
9c6bc1e
Add new error operation type for no bookmarks file. Add unit test.
aataraxiaa Mar 22, 2024
d1bf50e
Undo noData pixel fire removal
aataraxiaa Mar 25, 2024
7bfe827
Remove localized zero
aataraxiaa Mar 25, 2024
abf1998
Remove localizable file newline
aataraxiaa Mar 25, 2024
ea45855
Fix swiftlint warnings
aataraxiaa Mar 25, 2024
8dfc621
Add noData-backed error for missing Firefox keys DB
aataraxiaa Mar 26, 2024
f9db576
Merge branch 'main' into pete/improve-nodata-error-handling
aataraxiaa Mar 27, 2024
902ff1a
Add Placeholder noData summary icon
aataraxiaa Mar 27, 2024
b68d831
Update success, failure icons, add noData (skipped) icon]
aataraxiaa Mar 28, 2024
a42be42
Merge branch 'main' into pete/improve-nodata-error-handling
aataraxiaa Mar 28, 2024
68a10dd
Fix logic, update tests\
aataraxiaa Mar 28, 2024
d79475f
Update noData copy based on ship review
aataraxiaa Mar 29, 2024
0bbbd5d
Skip summary when manual import skipped, based on ship review
aataraxiaa Mar 29, 2024
2814e2a
Merge branch 'main' into pete/improve-nodata-error-handling
aataraxiaa Mar 29, 2024
23524fa
PR feedback
aataraxiaa Apr 1, 2024
71077dc
PR feedback
aataraxiaa Apr 2, 2024
dc2b685
No data password/bookmark subtitle translations
aataraxiaa Apr 4, 2024
477ae70
Fix translation
aataraxiaa Apr 4, 2024
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
@@ -1,7 +1,7 @@
{
"images" : [
{
"filename" : "Error.pdf",
"filename" : "Exclamation-Recolorable-16.pdf",
"idiom" : "universal"
}
],
Expand Down
Binary file not shown.
12 changes: 12 additions & 0 deletions DuckDuckGo/Assets.xcassets/Images/Skipped.imageset/Contents.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"images" : [
{
"filename" : "Jump-Recolorable-16.pdf",
"idiom" : "universal"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"images" : [
{
"filename" : "SuccessCheckmark.pdf",
"filename" : "Check-Recolorable-16.pdf",
"idiom" : "universal"
}
],
Expand Down
Binary file not shown.
7 changes: 7 additions & 0 deletions DuckDuckGo/Common/Localizables/UserText.swift
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,13 @@ struct UserText {
String(format: NSLocalizedString("import.logins.select-csv-file.source", value: "Select %@ CSV File…", comment: "Button text for selecting a CSV file exported from (LastPass or Bitwarden or 1Password - %@)"), source.importSourceName)
}

static func importNoDataBookmarksSubtitle(from source: DataImport.Source) -> String {
String(format: NSLocalizedString("import.nodata.bookmarks.subtitle", value: "If you have %@ bookmarks, try importing them manually instead.", comment: "Data import error subtitle: suggestion to import Bookmarks manually by selecting a CSV or HTML file."), source.importSourceName)
}
static func importNoDataPasswordsSubtitle(from source: DataImport.Source) -> String {
String(format: NSLocalizedString("import.nodata.passwords.subtitle", value: "If you have %@ passwords, try importing them manually instead.", comment: "Data import error subtitle: suggestion to import passwords manually by selecting a CSV or HTML file."), source.importSourceName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider adding a comment for translator what is "%@" here,

BTW, will localization of the added copy be run as a follow-up task or is it in progress?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This updated copy was added as part of ship review and the copy review is still open. Once approved I’ll run translation as part of this task.

}

static let importLoginsPasswords = NSLocalizedString("import.logins.passwords", value: "Passwords", comment: "Title text for the Passwords import option")

static let importBookmarksButtonTitle = NSLocalizedString("bookmarks.import.button.title", value: "Import", comment: "Button text to open bookmark import dialog")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ final class FirefoxBookmarksReader {
case fetchAllFolders

case copyTemporaryFile

case couldNotFindBookmarksFile
}

var action: DataImportAction { .bookmarks }
Expand All @@ -53,7 +55,7 @@ final class FirefoxBookmarksReader {

var errorType: DataImport.ErrorType {
switch type {
case .dbOpen: .noData
case .dbOpen, .couldNotFindBookmarksFile: .noData
case .fetchRootEntries, .noRootEntries, .fetchTopLevelFolders, .fetchAllBookmarks, .fetchAllFolders: .dataCorrupted
case .copyTemporaryFile: .other
}
Expand All @@ -72,6 +74,10 @@ final class FirefoxBookmarksReader {
}

func readBookmarks() -> DataImportResult<ImportedBookmarks> {
guard FileManager.default.fileExists(atPath: firefoxPlacesDatabaseURL.path) else {
return .failure(ImportError(type: .couldNotFindBookmarksFile, underlyingError: nil))
}

do {
currentOperationType = .copyTemporaryFile
return try firefoxPlacesDatabaseURL.withTemporaryFile { temporaryDatabaseURL in
Expand Down
7 changes: 4 additions & 3 deletions DuckDuckGo/DataImport/Logins/Firefox/FirefoxLoginReader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ final class FirefoxLoginReader {
case couldNotDetermineFormat = -2

case couldNotFindLoginsFile = 0
case couldNotFindKeyDB
Copy link
Collaborator

Choose a reason for hiding this comment

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

please don‘t change the case ordering to keep the original pixel error codes, add the new case to the end of the list, also consider adding a comment to keep the ordering when adding new errors

case couldNotReadLoginsFile

case key3readerStage1
Expand All @@ -49,7 +50,7 @@ final class FirefoxLoginReader {

var errorType: DataImport.ErrorType {
switch type {
case .couldNotFindLoginsFile, .couldNotReadLoginsFile: .noData
case .couldNotFindLoginsFile, .couldNotFindKeyDB, .couldNotReadLoginsFile: .noData
case .key3readerStage1, .key3readerStage2, .key3readerStage3, .key4readerStage1, .key4readerStage2, .key4readerStage3, .decryptUsername, .decryptPassword: .decryptionError
case .couldNotDetermineFormat: .dataCorrupted
case .requiresPrimaryPassword: .other
Expand Down Expand Up @@ -94,7 +95,7 @@ final class FirefoxLoginReader {
func readLogins(dataFormat: DataFormat?) -> DataImportResult<[ImportedLoginCredential]> {
var currentOperationType: ImportError.OperationType = .couldNotFindLoginsFile
do {
let dataFormat = try dataFormat ?? detectLoginFormat() ?? { throw ImportError(type: .couldNotDetermineFormat, underlyingError: nil) }()
let dataFormat = try dataFormat ?? detectLoginFormat() ?? { throw ImportError(type: .couldNotFindKeyDB, underlyingError: nil) }()
let keyData = try getEncryptionKey(dataFormat: dataFormat)
let result = try reallyReadLogins(dataFormat: dataFormat, keyData: keyData, currentOperationType: &currentOperationType)
return .success(result)
Expand All @@ -106,7 +107,7 @@ final class FirefoxLoginReader {
}

func getEncryptionKey() throws -> Data {
let dataFormat = try detectLoginFormat() ?? { throw ImportError(type: .couldNotDetermineFormat, underlyingError: nil) }()
let dataFormat = try detectLoginFormat() ?? { throw ImportError(type: .couldNotFindKeyDB, underlyingError: nil) }()
return try getEncryptionKey(dataFormat: dataFormat)
}

Expand Down
19 changes: 14 additions & 5 deletions DuckDuckGo/DataImport/Model/DataImportViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ struct DataImportViewModel {
self.dataType = dataType
self.result = result
}

static func == (lhs: DataTypeImportResult, rhs: DataTypeImportResult) -> Bool {
lhs.dataType == rhs.dataType &&
lhs.result.description == rhs.result.description
}
}

/// collected import summary for current import operation per selected import source
Expand Down Expand Up @@ -248,6 +253,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))
}

Pixel.fire(.dataImportFailed(source: importSource, sourceVersion: importSource.installedAppsMajorVersionDescription(selectedProfile: selectedProfile), error: error))
}
}
Expand Down Expand Up @@ -322,16 +328,19 @@ struct DataImportViewModel {
}

/// Skip button press
@MainActor mutating func skipImport() {
@MainActor mutating func skipImportOrDismiss(using dismiss: @escaping () -> Void) {
if let screen = screenForNextDataTypeRemainingToImport(after: screen.fileImportDataType) {
// skip to next non-imported data type
self.screen = screen
} else if selectedDataTypes.first(where: { error(for: $0) != nil }) != nil {
} else if selectedDataTypes.first(where: {
let error = error(for: $0)
return error != nil && error?.errorType != .noData
}) != nil {
// errors occurred during import: show feedback screen
self.screen = .feedback
} else {
// display total summary
self.screen = .summary(selectedDataTypes)
// When we skip a manual import, and there are no next non-imported data types, we dismiss
self.dismiss(using: dismiss)
}
}

Expand Down Expand Up @@ -671,7 +680,7 @@ extension DataImportViewModel {
initiateImport()

case .skip:
skipImport()
skipImportOrDismiss(using: dismiss)

case .cancel:
importTask?.cancel()
Expand Down
6 changes: 2 additions & 4 deletions DuckDuckGo/DataImport/View/DataImportNoDataView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,13 @@ struct DataImportNoDataView: View {
Text("We couldn‘t find any bookmarks.", comment: "Data import error message: Bookmarks weren‘t found.")
.bold()

Text("Try importing bookmarks manually instead.",
comment: "Data import error subtitle: suggestion to import Bookmarks manually by selecting a CSV or HTML file.")
Text(UserText.importNoDataBookmarksSubtitle(from: source))

case .passwords:
Text("We couldn‘t find any passwords.", comment: "Data import error message: Passwords weren‘t found.")
.bold()

Text("Try importing passwords manually instead.",
comment: "Data import error subtitle: suggestion to import Passwords manually by selecting a CSV or HTML file.")
Text(UserText.importNoDataPasswordsSubtitle(from: source))
}
}
}
Expand Down
49 changes: 38 additions & 11 deletions DuckDuckGo/DataImport/View/DataImportSummaryView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ struct DataImportSummaryView: View {
self.model = model
}

private let zeroString = "0"

var body: some View {
VStack(alignment: .leading, spacing: 8) {
{
Expand Down Expand Up @@ -61,7 +63,7 @@ struct DataImportSummaryView: View {
}
if summary.duplicate > 0 {
HStack {
failureImage()
skippedImage()
Text("Duplicate Bookmarks Skipped:",
comment: "Data import summary format of how many duplicate bookmarks (%lld) were skipped during import.")
+ Text(" " as String)
Expand All @@ -78,18 +80,38 @@ struct DataImportSummaryView: View {
}
}

case (.bookmarks, .failure):
HStack {
failureImage()
Text("Bookmark import failed.",
comment: "Data import summary message of failed bookmarks import.")
case (.bookmarks, .failure(let error)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix the 'error' was never used warning

if error.errorType == .noData {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be split into different cases to remove the nested if: case (.bookmarks, .failure(let error)) where error.errorType == .noData: and case (.bookmarks, .failure(let error)):

HStack {
skippedImage()
Text("Bookmarks:",
comment: "Data import summary format of how many bookmarks (%lld) were successfully imported.")
+ Text(" " as String)
+ Text(zeroString).bold()
}
} else {
HStack {
failureImage()
Text("Bookmark import failed.",
comment: "Data import summary message of failed bookmarks import.")
}
}

case (.passwords, .failure):
HStack {
failureImage()
Text("Password import failed.",
comment: "Data import summary message of failed passwords import.")
case (.passwords, .failure(let error)):
if error.errorType == .noData {
HStack {
skippedImage()
Text("Passwords:",
comment: "Data import summary format of how many passwords (%lld) were successfully imported.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix the comment (no formatting here)

+ Text(" " as String)
+ Text(zeroString).bold()
}
} else {
HStack {
failureImage()
Text("Password import failed.",
comment: "Data import summary message of failed passwords import.")
}
}

case (.passwords, .success(let summary)):
Expand Down Expand Up @@ -126,6 +148,11 @@ private func failureImage() -> some View {
.frame(width: 16, height: 16)
}

private func skippedImage() -> some View {
Image(.skipped)
.frame(width: 16, height: 16)
}

#if DEBUG
#Preview {
VStack {
Expand Down
26 changes: 26 additions & 0 deletions DuckDuckGo/Localizable.xcstrings
Original file line number Diff line number Diff line change
Expand Up @@ -24083,6 +24083,30 @@
}
}
},
"import.nodata.bookmarks.subtitle" : {
"comment" : "Data import error subtitle: suggestion to import Bookmarks manually by selecting a CSV or HTML file.",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "If you have %@ bookmarks, try importing them manually instead."
}
}
}
},
"import.nodata.passwords.subtitle" : {
"comment" : "Data import error subtitle: suggestion to import passwords manually by selecting a CSV or HTML file.",
"extractionState" : "extracted_with_value",
"localizations" : {
"en" : {
"stringUnit" : {
"state" : "new",
"value" : "If you have %@ passwords, try importing them manually instead."
}
}
}
},
"import.onePassword.app.version.info" : {
"comment" : "Instructions how to find an installed 1Password password manager app version.\n%1$s, %2$s - app name (1Password)",
"extractionState" : "extracted_with_value",
Expand Down Expand Up @@ -50184,6 +50208,7 @@
},
"Try importing bookmarks manually instead." : {
"comment" : "Data import error subtitle: suggestion to import Bookmarks manually by selecting a CSV or HTML file.",
"extractionState" : "stale",
"localizations" : {
"de" : {
"stringUnit" : {
Expand Down Expand Up @@ -50237,6 +50262,7 @@
},
"Try importing passwords manually instead." : {
"comment" : "Data import error subtitle: suggestion to import Passwords manually by selecting a CSV or HTML file.",
"extractionState" : "stale",
"localizations" : {
"de" : {
"stringUnit" : {
Expand Down
Loading
Loading