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.

Sign up for GitHub

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

Jump to bottom

Import source profiles to separate profiles #16640

Merged
merged 5 commits into from
Jan 25, 2023
Merged

Import source profiles to separate profiles #16640

merged 5 commits into from
Jan 25, 2023

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Jan 12, 2023 •
edited
Loading

Resolves brave/brave-browser#27733
Resolves brave/brave-browser#27591

Removed parallel import feature because the onboarding has been merged.

Chrome:

profile.mp4

Firefox:

profile_ff.mp4

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Import multiple profiles from different browsers
  • Check each profile imported to separate Brave's profile
  • Avatars for newly created profiles should be assigned randomly
  • If user importing only one source profile it should not create a new Brave profile and import to Default one
  • Steps:
    • Install Chrome/Firefox/any other and create there 3-4 profile with own bookmarks, history, extensions, password etc.
    • Go to brave welcome page and go through onboarding flow
    • All profiles should be created in Brave with same names and contain imported data

@spylogsster spylogsster self-assigned this Jan 12, 2023
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jan 12, 2023
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@spylogsster spylogsster force-pushed the brave-27591 branch 2 times, most recently from 8b99cd7 to b9539e6 Compare January 13, 2023 07:04
@brave brave deleted a comment from github-actions bot Jan 13, 2023
@brave brave deleted a comment from github-actions bot Jan 13, 2023
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@simonhong
Copy link
Member

and it would be nice to have some test cases for checking whether new profiles are created or not based on some condition.

@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: d10800fbe39f480b5da13777f2bc96e16340d716
reason: unsigned
Please follow the handbook to configure commit signing
cc: @spylogsster

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@spylogsster spylogsster marked this pull request as draft January 16, 2023 09:48
@spylogsster spylogsster force-pushed the brave-27591 branch 2 times, most recently from bd66980 to fda7e84 Compare January 16, 2023 11:52
@spylogsster spylogsster marked this pull request as ready for review January 16, 2023 11:55
@brave brave deleted a comment from github-actions bot Jan 16, 2023
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@spylogsster spylogsster force-pushed the brave-27591 branch 3 times, most recently from 1f2f7cd to 2839556 Compare January 16, 2023 14:25
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@simonhong
Copy link
Member

When I repeats bulk importing multiple times from same browser, same profile is created again like below.
I tested with my Whale browser install and it has two profiles(Whale Person 1 and Whale Person 2).
I think we don't want to create same named profile again?
image

@spylogsster spylogsster marked this pull request as draft January 19, 2023 10:03
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@spylogsster spylogsster marked this pull request as ready for review January 19, 2023 10:27
@spylogsster spylogsster force-pushed the brave-27591 branch 3 times, most recently from dde4bca to 981dff0 Compare January 19, 2023 10:31
@spylogsster
Copy link
Contributor Author

When I repeats bulk importing multiple times from same browser, same profile is created again like below. I tested with my Whale browser install and it has two profiles(Whale Person 1 and Whale Person 2). I think we don't want to create same named profile again? image

fixed

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

simonhong
simonhong reviewed Jan 20, 2023
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I realized that CheckDiskAccess is only for importing from safari (#4530). and bulk importing will not be applied to safari because it doesn't use multiple profiles. So, bulk data handler could be simplified.

BTW, safari seems not listed in import list?

browser/ui/webui/settings/brave_import_data_handler.cc Outdated Show resolved Hide resolved
browser/ui/webui/settings/brave_import_bulk_data_handler.cc Outdated Show resolved Hide resolved
browser/ui/webui/settings/brave_import_bulk_data_handler.cc Show resolved Hide resolved
browser/ui/webui/settings/brave_import_bulk_data_handler.cc Outdated Show resolved Hide resolved
browser/ui/webui/settings/brave_import_bulk_data_handler.cc Show resolved Hide resolved
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

simonhong
simonhong approved these changes Jan 25, 2023
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++ 👍🏼

@spylogsster spylogsster merged commit bb9368a into master Jan 25, 2023
@spylogsster spylogsster deleted the brave-27591 branch January 25, 2023 07:20
@spylogsster spylogsster added this to the 1.49.x - Nightly milestone Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

@nullhook nullhook Awaiting requested review from nullhook

Assignees

@spylogsster spylogsster

Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Milestone
1.49.x - Release
3 participants
@spylogsster @brave-builds @simonhong