Improve Handling of noData Import Errors #2494

aataraxiaa · 2024-03-25T11:46:06Z

Task/Issue URL: https://app.asana.com/0/0/1206886138223455/f

Description:
Why

When an import data file cannot be found, we treat this as a error, noData , and ask the user for feedback
However, in some cases this error is completely acceptable, and this feedback is non-actionable
User feedback relating to this is noise, not signal

Changes

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)
Update our ’skipped’ icon (see ✓ Design: New Summary noData Icon)
Update our ‘failure’ icon (see ✓ Design: New Summary noData Icon)
Treat Firefox Bookmarks file not found as noData

Note: Testing below will involve moving your Firefox/Chrome browser data. Make sure you put these files back if you use Firefox/Chrome as your browser

Pre-Requisites for Tests

Install Firefox, and save one username and password in Firefox, and bookmark one site
Install Chrome, and save one username and password in Firefox, and bookmark one site

Steps to test this PR:

Test Firefox No Passwords File

Go to ~/Library/Application Support/Firefox/Profiles/
Locate your active Firefox profile (if you never set up a Firefox profile, it’s likely called something like "ebovof0c.default-release”)
In this directory, move the logins.json file outside the directory (i.e to your desktop).
Begin Import in the DDG browser
Select Firefox, Bookmarks and Passwords
Observe Bookmarks succeeds
Observe Passwords could not be found, “We couldn’t find any passwords.”
Observe the bottom right button displays “Skip passwords”
Select “Skip passwords”
Observe the import dialog is dismissed

Test Firefox No Bookmarks File

Go to ~/Library/Application Support/Firefox/Profiles/
Locate your active Firefox profile (if you never set up a Firefox profile, it’s likely called something like "ebovof0c.default-release”)
In this directory, move the places.sqlite file outside the directory (i.e to your desktop)
Ensure that the logins.json file is present (i.e put it back).
Begin Import in the DDG browser
Select Firefox, Bookmarks and Passwords
Observe Passwords succeeds
Observe Bookmarks could not be found, “We couldn’t find any bookmarks.”
Observe the bottom right button displays “Skip bookmarks”
Select “Skip bookmarks”
Observe the import dialog is dismissed

Test Chrome No Passwords File

Go to ~/Library/Application Support/Google/Chrome/Default/
In this directory, move the Login Data and Login Data For Account files outside the directory (i.e to your desktop)
Begin Import in the DDG browser
Select Chrome, Bookmarks and Passwords
Enter your system password when prompted
Observe Bookmarks succeeds
Observe Passwords could not be found, “We couldn’t find any passwords.”
Observe the bottom right button displays “Skip passwords”
Select “Skip passwords”
Observe the import dialog is dismissed

Test Chrome No Bookmarks File

Go to ~/Library/Application Support/Google/Chrome/Default/
In this directory, move the Bookmarks file outside the directory (i.e to your desktop)
- Ensure that the Login Data and Login Data For Account files are present (i.e put it back).
Begin Import in the DDG browser
Select Chrome, Bookmarks and Passwords
Enter your system password when prompted
Observe Passwords succeeds
Observe Bookmarks could not be found, “We couldn’t find any bookmarks.”
Observe the bottom right button displays “Skip bookmarks”
Select “Skip bookmarks”
Observe the import dialog is dismissed

Test Import Failure Icon

Attempt an import from CSV, using the attached CSV
On failure, the ’failure’ icon should be displayed for passwords, with the text “Password import failed”

—

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

…rror. Don't show feedback on `noData` errors.

mallexxx · 2024-04-01T13:27:36Z

DuckDuckGo/DataImport/Logins/Firefox/FirefoxLoginReader.swift

@@ -29,6 +29,7 @@ final class FirefoxLoginReader {
            case couldNotDetermineFormat = -2

            case couldNotFindLoginsFile = 0
+            case couldNotFindKeyDB


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

mallexxx · 2024-04-01T13:30:45Z

DuckDuckGo/DataImport/View/DataImportSummaryView.swift

+                case (.bookmarks, .failure(let error)):
+                    if error.errorType == .noData {


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)):

mallexxx · 2024-04-01T13:34:08Z

DuckDuckGo/Common/Localizables/UserText.swift

+        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)


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?

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.

aataraxiaa · 2024-04-01T16:09:47Z

Thanks for the review @mallexxx. PR updated.

mallexxx · 2024-04-02T09:43:28Z

DuckDuckGo/DataImport/View/DataImportSummaryView.swift

+                        + Text(zeroString).bold()
+                    }
+
+                case (.bookmarks, .failure(let error)):


fix the 'error' was never used warning

mallexxx · 2024-04-02T09:44:43Z

DuckDuckGo/DataImport/View/DataImportSummaryView.swift

+                    HStack {
+                        skippedImage()
+                        Text("Bookmarks:",
+                             comment: "Data import summary format of how many bookmarks (%lld) were successfully imported.")


fix the comment (no formatting here)

mallexxx · 2024-04-02T09:44:49Z

DuckDuckGo/DataImport/View/DataImportSummaryView.swift

+                        HStack {
+                            skippedImage()
+                            Text("Passwords:",
+                                 comment: "Data import summary format of how many passwords (%lld) were successfully imported.")


fix the comment (no formatting here)

mallexxx

LGTM!
see couple more minor notes about locatization comments above

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)

* main: (338 commits) Fixes the VPN restarting logic on update (#2545) fix download save panel disappearing on navigation (#2549) Add CI support for handling installation attribution (#2502) Bump version to 1.82.0 (152) Pixel changed in subscription (#2541) add LSHandlerRank for .duckload document type (#2537) Fix usertext comment to ensure it matches localizable string (#2546) Update Neighbor Report (#2542) DBP: Compare by url and not name (#2544) DBP: Compare by url and not name (#2544) Adds a series of UI tests for Bookmarks Bar visibility Improve Handling of noData Import Errors (#2494) Bump version to 1.82.0 (151) Use the default action button style for VPN onboarding (#2529) Redirect from purchase page to macOS native purchase flow (#2538) Closing empty tabs after download (#2510) Add Web UI loading state pixels (#2531) fix localization warnings (#2288) Select URL in address bar when editing and use cmd+L (#2536) Add attemptId to Captcha /result endpoint (#2530) ...

* main: (22 commits) Removes last instance of NETWORK_PROTECTION flag (#2573) Bye bye NETWORK_PROTECTION (#2509) QWD: Enable Hide/Show for Autofill Credit Card Number and CVV (#2539) Removed the VPN waitlist beta pixels (#2555) VPN: Cleanup authorize call (#2565) Revert "VPN: Cleanup authorize call (#2553)" VPN: Cleanup authorize call (#2553) Improves underlying error pixel information (#2543) Fixes the VPN restarting logic on update (#2545) fix download save panel disappearing on navigation (#2549) Add CI support for handling installation attribution (#2502) Fix usertext comment to ensure it matches localizable string (#2546) Update Neighbor Report (#2542) DBP: Compare by url and not name (#2544) Adds a series of UI tests for Bookmarks Bar visibility Improve Handling of noData Import Errors (#2494) Use the default action button style for VPN onboarding (#2529) Closing empty tabs after download (#2510) Add Web UI loading state pixels (#2531) fix localization warnings (#2288) ...

aataraxiaa added 6 commits March 21, 2024 15:28

Implement == for DataTypeImportResult. Don't fire Pixel on noData e…

f9a856f

…rror. Don't show feedback on `noData` errors.

Update DataImportViewModel tests. Add noData tests.

e32b283

Iterate on == impl, improve tests

de7c066

Iterate on proposed UI updates and copy changes

4ef9ada

Add new error operation type for no bookmarks file. Add unit test.

9c6bc1e

Undo noData pixel fire removal

d1bf50e

aataraxiaa changed the title ~~Improve Handling of noData Impot~~ Improve Handling of noData Import Errors Mar 25, 2024

aataraxiaa added 2 commits March 25, 2024 11:54

Remove localized zero

7bfe827

Remove localizable file newline

abf1998

github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Mar 25, 2024

aataraxiaa added 6 commits March 25, 2024 12:07

Fix swiftlint warnings

ea45855

Add noData-backed error for missing Firefox keys DB

8dfc621

Merge branch 'main' into pete/improve-nodata-error-handling

f9db576

Add Placeholder noData summary icon

902ff1a

Update success, failure icons, add noData (skipped) icon]

b68d831

Merge branch 'main' into pete/improve-nodata-error-handling

a42be42

aataraxiaa removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Mar 28, 2024

Fix logic, update tests\

68a10dd

aataraxiaa marked this pull request as ready for review March 28, 2024 11:09

aataraxiaa added 3 commits March 29, 2024 08:59

Update noData copy based on ship review

d79475f

Skip summary when manual import skipped, based on ship review

0bbbd5d

Merge branch 'main' into pete/improve-nodata-error-handling

2814e2a

mallexxx reviewed Apr 1, 2024

View reviewed changes

PR feedback

23524fa

mallexxx self-assigned this Apr 2, 2024

mallexxx reviewed Apr 2, 2024

View reviewed changes

mallexxx approved these changes Apr 2, 2024

View reviewed changes

mallexxx assigned aataraxiaa and unassigned mallexxx Apr 2, 2024

aataraxiaa added 3 commits April 2, 2024 11:24

PR feedback

71077dc

No data password/bookmark subtitle translations

dc2b685

Fix translation

477ae70

aataraxiaa merged commit 26dfe3f into main Apr 4, 2024
17 checks passed

aataraxiaa deleted the pete/improve-nodata-error-handling branch April 4, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Handling of noData Import Errors #2494

Improve Handling of noData Import Errors #2494

aataraxiaa commented Mar 25, 2024 •

edited

Loading

mallexxx Apr 1, 2024

mallexxx Apr 1, 2024

mallexxx Apr 1, 2024

aataraxiaa Apr 1, 2024

aataraxiaa commented Apr 1, 2024

mallexxx Apr 2, 2024

mallexxx Apr 2, 2024

mallexxx Apr 2, 2024

mallexxx left a comment

		case (.bookmarks, .failure(let error)):
		if error.errorType == .noData {

Improve Handling of noData Import Errors #2494

Improve Handling of noData Import Errors #2494

Conversation

aataraxiaa commented Mar 25, 2024 • edited Loading

Internal references:

mallexxx Apr 1, 2024

Choose a reason for hiding this comment

mallexxx Apr 1, 2024

Choose a reason for hiding this comment

mallexxx Apr 1, 2024

Choose a reason for hiding this comment

aataraxiaa Apr 1, 2024

Choose a reason for hiding this comment

aataraxiaa commented Apr 1, 2024

mallexxx Apr 2, 2024

Choose a reason for hiding this comment

mallexxx Apr 2, 2024

Choose a reason for hiding this comment

mallexxx Apr 2, 2024

Choose a reason for hiding this comment

mallexxx left a comment

Choose a reason for hiding this comment

aataraxiaa commented Mar 25, 2024 •

edited

Loading