Improve Handling of noData Import Errors (#2494)

Task/Issue URL: https://app.asana.com/0/0/1206886138223455/f **Description**: Stop prompting the user for feedback when we are handling a noData error. When no data is found initially, and user chooses to skip manual import, no summary screen is displayed (the import dialog is dismissed)
duckduckgo · Apr 4, 2024 · 26dfe3f · 26dfe3f
1 parent c1d920c
commit 26dfe3f
Show file tree

Hide file tree

Showing 17 changed files with 322 additions and 47 deletions.
diff --git a/DuckDuckGo/Assets.xcassets/Images/Error.imageset/Contents.json b/DuckDuckGo/Assets.xcassets/Images/Error.imageset/Contents.json
@@ -1,7 +1,7 @@
 {
   "images" : [
     {
-      "filename" : "Error.pdf",
+      "filename" : "Exclamation-Recolorable-16.pdf",
       "idiom" : "universal"
     }
   ],

diff --git a/....xcassets/Images/Error.imageset/Error.pdf → ...r.imageset/Exclamation-Recolorable-16.pdf b/....xcassets/Images/Error.imageset/Error.pdf → ...r.imageset/Exclamation-Recolorable-16.pdf
diff --git a/DuckDuckGo/Assets.xcassets/Images/Skipped.imageset/Contents.json b/DuckDuckGo/Assets.xcassets/Images/Skipped.imageset/Contents.json
@@ -0,0 +1,12 @@
+{
+  "images" : [
+    {
+      "filename" : "Jump-Recolorable-16.pdf",
+      "idiom" : "universal"
+    }
+  ],
+  "info" : {
+    "author" : "xcode",
+    "version" : 1
+  }
+}
diff --git a/DuckDuckGo/Assets.xcassets/Images/Skipped.imageset/Jump-Recolorable-16.pdf b/DuckDuckGo/Assets.xcassets/Images/Skipped.imageset/Jump-Recolorable-16.pdf
diff --git a/DuckDuckGo/Assets.xcassets/Images/SuccessCheckmark.imageset/Check-Recolorable-16.pdf b/DuckDuckGo/Assets.xcassets/Images/SuccessCheckmark.imageset/Check-Recolorable-16.pdf
diff --git a/DuckDuckGo/Assets.xcassets/Images/SuccessCheckmark.imageset/Contents.json b/DuckDuckGo/Assets.xcassets/Images/SuccessCheckmark.imageset/Contents.json
@@ -1,7 +1,7 @@
 {
   "images" : [
     {
-      "filename" : "SuccessCheckmark.pdf",
+      "filename" : "Check-Recolorable-16.pdf",
       "idiom" : "universal"
     }
   ],

diff --git a/DuckDuckGo/Assets.xcassets/Images/SuccessCheckmark.imageset/SuccessCheckmark.pdf b/DuckDuckGo/Assets.xcassets/Images/SuccessCheckmark.imageset/SuccessCheckmark.pdf
diff --git a/DuckDuckGo/Common/Localizables/UserText.swift b/DuckDuckGo/Common/Localizables/UserText.swift
@@ -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. The placeholder here represents the source browser, e.g Firefox"), source.importSourceName)
+    }
+
     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")

diff --git a/DuckDuckGo/DataImport/Bookmarks/Firefox/FirefoxBookmarksReader.swift b/DuckDuckGo/DataImport/Bookmarks/Firefox/FirefoxBookmarksReader.swift
@@ -45,6 +45,8 @@ final class FirefoxBookmarksReader {
             case fetchAllFolders
 
             case copyTemporaryFile
+
+            case couldNotFindBookmarksFile
         }
 
         var action: DataImportAction { .bookmarks }
@@ -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
             }
@@ -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

diff --git a/DuckDuckGo/DataImport/Logins/Firefox/FirefoxLoginReader.swift b/DuckDuckGo/DataImport/Logins/Firefox/FirefoxLoginReader.swift
@@ -41,6 +41,8 @@ final class FirefoxLoginReader {
 
             case decryptUsername
             case decryptPassword
+
+            case couldNotFindKeyDB
         }
 
         var action: DataImportAction { .passwords }
@@ -49,7 +51,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
@@ -94,7 +96,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)
@@ -106,7 +108,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)
     }
 

diff --git a/DuckDuckGo/DataImport/Model/DataImportViewModel.swift b/DuckDuckGo/DataImport/Model/DataImportViewModel.swift
@@ -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
@@ -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))
             }
         }
@@ -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)
         }
     }
 
@@ -671,7 +680,7 @@ extension DataImportViewModel {
             initiateImport()
 
         case .skip:
-            skipImport()
+            skipImportOrDismiss(using: dismiss)
 
         case .cancel:
             importTask?.cancel()

diff --git a/DuckDuckGo/DataImport/View/DataImportNoDataView.swift b/DuckDuckGo/DataImport/View/DataImportNoDataView.swift
@@ -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))
             }
         }
     }

diff --git a/DuckDuckGo/DataImport/View/DataImportSummaryView.swift b/DuckDuckGo/DataImport/View/DataImportSummaryView.swift
@@ -33,6 +33,8 @@ struct DataImportSummaryView: View {
         self.model = model
     }
 
+    private let zeroString = "0"
+
     var body: some View {
         VStack(alignment: .leading, spacing: 8) {
             {
@@ -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)
@@ -78,18 +80,37 @@ struct DataImportSummaryView: View {
                         }
                     }
 
+                case (.bookmarks, .failure(let error)) where error.errorType == .noData:
+                    HStack {
+                        skippedImage()
+                        Text("Bookmarks:",
+                             comment: "Data import summary format of how many bookmarks were successfully imported.")
+                        + Text(" " as String)
+                        + Text(zeroString).bold()
+                    }
+
                 case (.bookmarks, .failure):
                     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 were successfully imported.")
+                            + 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)):
@@ -126,6 +147,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 {