Skip to content
This repository has been archived by the owner on Jun 17, 2022. It is now read-only.

Skip FirefoxAccounts during Firefox CSV Import #323

Merged
merged 3 commits into from
Apr 12, 2021

Conversation

tzdybal
Copy link
Contributor

@tzdybal tzdybal commented Apr 5, 2021

Firefox exports chrome://FirefoxAccounts if Firefox Accouts are used in browser. It's quite hacky - password field in CSV is actually a JSON encoded data, not a password.
Because it's not a useful record, it should be skipped during import.

This PR adds filtering and unit test for Firefox importer.

Firefox exports 'chrome://FirefoxAccounts' if Firefox Accouts are used
in browser. It's quite hacky - password field in CSV is actually a JSON
encoded data, not a password.
Because it's not a useful record, it should be skipped during import.
@CLAassistant
Copy link

CLAassistant commented Apr 5, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@cscharf cscharf left a comment

Choose a reason for hiding this comment

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

Thanks for the submission! Outside of some formatting issues I think this looks okay. Would like @MGibson1 to take a quick peek at the unit tests as well. Overall please ensure member/property indentation and level-alignment (using spaces only) matches across the board.

src/importers/firefoxCsvImporter.ts Outdated Show resolved Hide resolved
spec/common/importers/firefoxCsvImporter.spec.ts Outdated Show resolved Hide resolved
@cscharf cscharf requested a review from MGibson1 April 5, 2021 21:26
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! I don't see how the linter didn't pick this up, but thanks!

@MGibson1 MGibson1 self-requested a review April 5, 2021 23:02
@MGibson1
Copy link
Member

MGibson1 commented Apr 5, 2021

Sorry about reversing that approval! for some reason when I first clicked review it showed me only spacing fixes. 🤷

@tzdybal tzdybal requested a review from MGibson1 April 11, 2021 18:54
@tzdybal
Copy link
Contributor Author

tzdybal commented Apr 11, 2021

Test data is now in separate files, and this time I fixed all linter errors before commiting ;)

Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants