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

Import error improvements #406

Merged
merged 9 commits into from
Feb 9, 2022
Merged

Import error improvements #406

merged 9 commits into from
Feb 9, 2022

Conversation

samsymons
Copy link
Collaborator

Task/Issue URL:
Tech Design URL:
CC: @brindy

Description:

This PR makes some small tweaks to the import code to squash some errors:

  1. Browsers are no longer displayed in the list of import targets if they are installed but have no available profiles. In development, this would trip an assertion failure, and in production it led to a weird state where it just gave you a blank screen.
  2. The Chrome importer now shows the name of each profile if you have multiple available
  3. Firefox no longer gives you an error if you import from it, but have no logins. Now it counts as a successful import of 0 logins

Steps to test this PR:

  1. Install Brave, but don't launch it. If you already have it installed, delete it and its application support directory, and reinstall it. Then, open the importer and check that it isn't visible.

  2. Now launch Brave, and open the importer again. Check that it is present in the list – try to import from it, you should get a successful import of 0 logins and bookmarks.

  3. Create multiple profiles in Chrome, and open the importer; check that the profile list shows the expected names

  4. Delete all of your Firefox logins (or, optionally, go to ~/Library/Application Support/Firefox/Profiles in Finder and drill down into the default release profile, then delete logins.json) and run the importer. It should give you a successful import of 0 logins

Testing checklist:

  • Test with Release configuration

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@samsymons
Copy link
Collaborator Author

@bstandaert-ddg This one will be of interest to you - I'd love if you were able to take some time to try and break the import flow 🙂

@bstandaert-ddg
Copy link

Will do! The original issue I had is definitely fixed now.

After making a second profile in Brave, the DDG importer is showing more profiles than I actually have:
Screen Shot 2022-01-24 at 4 32 32 PM

Perhaps profile 2 and "testing custom profile" are the same thing?

@bstandaert-ddg
Copy link

Also, password import from Brave doesn't seem to be working, at least with this multi-profile setup. Here, I have a password saved in profile 1, but DDG shows "0 passwords" no matter which profile I import from: https://drive.google.com/file/d/11a2FWJl0hWei8nte5a9deaYOXsCv88kN/view

@samsymons
Copy link
Collaborator Author

@bstandaert-ddg Thanks! Good spot on the profiles thing, I wasn't seeing that with Chrome but let me muck about with multiple Brave profiles more and see what happens. Might pull the trigger on this BrowserProfile code update after all.

@samsymons samsymons self-assigned this Jan 24, 2022
* develop:
  Version 0.18.5
  support privacy config for clickToLoad (#407)
  Automatically select available login (#405)
  initial FB Click to Load (WIP) (#329)
  onboarding updates (#398)
  Version 0.18.4
  Configuration of Sparkle - Setting SUAllowsAutomaticUpdates to NO (#404)
  Hide downloads button if the popover is opened/closed manually (#397)
@samsymons
Copy link
Collaborator Author

Finally catching up on this after a busy week last week. It looked like Chromium defines a System Profile directory which contains the new Preferences file that the importer is looking for, but it isn't user facing.

Going to ignore it when importing – nice catch @bstandaert-ddg! Look for a commit some time tomorrow that fixes the issues you found.

@samsymons
Copy link
Collaborator Author

@bstandaert-ddg This is now updated, are you able to take another look to verify that you don't see the extra profile? I'm curious to see if you're still having problems importing logins from Brave - I was able to import some logins just now successfully.

@bstandaert-ddg
Copy link

Sorry for the delay here. The profile list looks correct now. Importing passwords seems to be working now also 🤷

@mallexxx mallexxx assigned mallexxx and unassigned samsymons Feb 3, 2022
@samsymons samsymons requested a review from brindy February 4, 2022 06:10
@mallexxx mallexxx removed their assignment Feb 4, 2022
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM! The code in the FileStoreMock looks hairy, but given it's for testing I'm not too worried.

@samsymons samsymons merged commit 5ebba9c into develop Feb 9, 2022
@samsymons samsymons deleted the sam/improve-import-errors branch February 9, 2022 00:19
samsymons added a commit that referenced this pull request Feb 10, 2022
* develop:
  Point to BrowserServicesKit 8.0. (#425)
  Import error improvements (#406)
samsymons added a commit that referenced this pull request Feb 18, 2022
* develop:
  Fix non-debug builds (#428)
  new tds url (#430)
  Sparkle 1.27.1 (#411)
  Disable CVDisplayLing logging (#421)
  Update Fire Popover graphics (#426)
  Version 0.18.7
  Refresh the address bar when reloading (#413)
  Fix tabs leakage after Drag-Drop (#423)
  Point to BrowserServicesKit 8.0. (#425)
  Import error improvements (#406)
  Replace Burn and Fireproof icons (#416)
  don't disable the UI unless onboarding has been marked as finished (#420)
  fix nested RunLoop waiting (#422)
  Version 0.18.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants