1650675 - Login CSV import summary dialog

Status: VERIFIED FIXED

(Firefox :: about:logins, enhancement, P1)

Description

[Matthew N. [:MattN]]

After a user imports from a CSV file, show a confirmation dialog indicating the number of additions, updates, unchanged exact matches, and errors.

UX Spec: https://mozilla.invisionapp.com/share/R3XX2AF2XN5#/screens/423736091_Import_From_CSV

Comment 1

[Matthew N. [:MattN]]

Perhaps maybeImportLogins can return an array corresponding to the input array

[
  { result: "added", login: {…nsILoginInfo…}},
  { result: "modified", login: {…nsILoginInfo…}},
  { result: "no_change", login: {…nsILoginInfo…}},
  { result: "error_invalid_origin", loginData: {…nsILoginInfo…}},
  { result: "error_invalid_password", loginData: {…nsILoginInfo…}},
]

The CSV code will also have errors that don't apply to a specific login e.g. missing columns, can't read the file, etc.

Comment 2

[Andrei Petcu]

Attachment: Bug 1650675 Login CSV import summary backend

Comment 3

[Andrei Petcu]

Attachment: Bug 1650675 Import dialog summary

Depends on D83193

Comment 4

[Andrei Petcu]

Attachment: Bug 1650675 added mock data

Depends on D85647

Comment 5

[Andrei Petcu]

Attachment: Bug 1650675 move to import-summary-dialog

Depends on D87851

Comment 6

[Andrei Petcu]

Attachment: Bug 1650675 localize messages in one row

Depends on D85647

Comment 7

[Andrei Petcu]

Attachment: Bug 1650675 Import CSV error dialog

Depends on D85647

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e281a982040
Login CSV import summary backend r=MattN
https://hg.mozilla.org/integration/autoland/rev/6b9b5c6906b8
Import dialog summary r=jaws,flod

Comment 9

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1650675) for browser-chrome failures in browser/components/aboutlogins/tests/browser/browser_openImport.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=321385481&repo=autoland&lineNumber=2974

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=6b9b5c6906b848096a32eaa60d93227d22669677

Backout:
https://hg.mozilla.org/integration/autoland/rev/1792d04f16e669ae472b82d7042b7c624e475e97

Comment 10

[Andrei Petcu]

Attachment: Bug 1650675 Import dialog summaryDifferential Revision: https://phabricator.services.mozilla.com/D85647Depends on D83193

Depends on D83193

Comment 11

[Andrei Petcu]

Attachment: Bug 1650675 Detailed import summary DRAFT

Depends on D96101

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bfd23bcf736
Login CSV import summary backend r=MattN
https://hg.mozilla.org/integration/autoland/rev/7ac7236854d4
Import dialog summary r=jaws,flod

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fa2ae373bb9
Fixed lint error in import-summary-dialog.css. a=lint-fix CLOSED TREE

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4bfd23bcf736
https://hg.mozilla.org/mozilla-central/rev/7ac7236854d4
https://hg.mozilla.org/mozilla-central/rev/0fa2ae373bb9

Comment 15

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I have started to test these changes on the latest Nightly 86.0a1 (Build ID: 20210111093448) on Windows 10 x64, macOS 10.15.6 and Linux Mint 20.
There are a few scenarios that I still need to cover before marking this bug as verified fix.

• However, during testing I have noticed that the "Import Error" dialog is not displayed if trying to import an invalid CSV file (eg: without the password column) or a different type of file (eg: .txt or .json).
  I am not sure if the "Import Error" dialog is implemented in this bug or there is an issue with this dialog? Andrei, can you please confirm if the "Import Error" dialog is implemented and should be displayed?

Comment 16

[Andrei Petcu]

Import error and import details list are not implemented. I hhave 2 patches for them under various degrees of review.

Comment 17

[Phabricator Automation]

Comment on attachment 9195137 [details]
Bug 1650675 Detailed import summary DRAFT

Revision D100639 was moved to bug 1649940. Setting attachment 9195137 [details] to obsolete.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Try: https://treeherder.mozilla.org/jobs?repo=try&revision=d78a74f5cb4fa407558e57d292005a21a52972bc

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac6aaf82ba76
Import CSV error dialog r=sfoster

Comment 20

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ac6aaf82ba76

Comment 21

[Simona Rosu [:srosu], Ecosystem QA]

I have verified this issue using the latest Firefox Nightly 88.0a1 (Build ID: 20210303215027) on Windows 10 x64, Ubuntu 20.04 and macOS 11.1.

• In order to verify this bug, I have tested the following scenarios:

1. The “Import Complete” modal is displayed after importing logins from a CSV file and this modal contains the following summary of logins:
   • New logins added:
   • Existing logins updated:
   • Duplicate logins found:
   • Errors:
2. The “Import Error” modal with the “File Format Issue” message is displayed after importing a non-CSV file. However, based on this mock-up the “Unable to Read File, Make sure you selected a CSV file” error should be displayed in this type of the “Import Error” modal.
3. The “Import Error” modal with the “Multiple Conflicting Values for One Login” message is displayed after importing a CSV file that contains multiple columns header.
4. The “Import Error” modal with the “File Format Issue” message is displayed after importing a CSV file with missing columns header.
5. Also, in the mock-up, there is specified that the “Unable to Read File '' error is displayed when trying to import a file that doesn't have permission to read. I am not sure if this case is also implemented. If this is implemented, can you give more information how we can verify this?

@Sam, @Andrei should I log new bugs for the scenarios described in point 2 and provide more details for the scenario described at point 5?

Comment 22

[Sam Foster [:sfoster] (he/him)]

(In reply to Simona Rosu [:srosu], Ecosystem QA from comment #21)

2. The “Import Error” modal with the “File Format Issue” message is displayed after importing a non-CSV file. However, based on this mock-up the “Unable to Read File, Make sure you selected a CSV file” error should be displayed in this type of the “Import Error” modal.

I think "Unable to Read File" is specifically for file i/o problems. If a non-CSV format but otherwise readable file is provided, "File Format Issue" seems like the correct result to me.

5. Also, in the mock-up, there is specified that the “Unable to Read File '' error is displayed when trying to import a file that doesn't have permission to read. I am not sure if this case is also implemented. If this is implemented, can you give more information how we can verify this?

On a mac or linux system, this case can be reproduced by removing the read permissions on a file, e.g. chmod 266 logins.csv. I think it should also be possible to reproduce by providing a file on a path that becomes unreachable, e.g. a disconnected network drive.

Comment 23

[Simona Rosu [:srosu], Ecosystem QA]

Attachment: image_2021_03_05T09_36_51_489Z.png

Comment 24

[Simona Rosu [:srosu], Ecosystem QA]

@Sam, thank you for providing more details. I was able to trigger the “Import Error” modal with the “Unable to read file” error after importing a CSV file that doesn’t have read permission on MacOS and Windows 10 x64. Also, I tried to verify this on Windows 10 x64 and a “Import Logins File” OS pop-up with an error message was displayed after importing the CSV file that doesn’t have any kind of permission. Probably this is an OS limitation.

• I have attached a screenshot with the pop-up.
• Based on this and comment 21, I’m marking this issue as Verified Fixed.