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

Migrate crypto widgets visibility in NTP #11266

Merged
merged 5 commits into from
Nov 25, 2021
Merged

Migrate crypto widgets visibility in NTP #11266

merged 5 commits into from
Nov 25, 2021

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Nov 24, 2021

fix brave/brave-browser#19708

Migration is done in the first NTP loading only once.

For new users, all crypto widgets are hidden by default.
For existing users, if current foremost widget is crypto widget,
only it is visible and others are hidden.
After migration, user can control crypto widget's visibility from gallery.

To make all crypto widgets hidden by default, each pref's default
value is not changed. Their default prefs is still "true".
As their visibility are managed by prefs and widget order is managed in local
storage, it's difficult to start migration from c++.

If user has already authed crypto widgets, they will be also visible regardless of position in the widget stack.

Resolves

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
  • 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, npm run lint, 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:

Test 1 (New user)

  1. Launch with clean profile
  2. Check all crypto widgets are hidden in NTP widget stack
  3. Control their visibility from gallery and check visibility are retained with new NTP or after restarting

Test 2-1 (Existing user)

  1. Launch old version brave with existing profile
  2. Show some(or all) crypto widgets in stack but set Rewards as a foremost
  3. Close old version brave and Launch new Brave
  4. Check all crypto widgets are hidden in widget stack

Test 2-2 (Existing user)

  1. Launch old version brave with existing profile
  2. Show some(or all) crypto widgets in stack and set one of crypto widget as a foremost one
  3. Close old version brave and Launch new Brave
  4. Check only foremost crypto widget is visible in the stack

Test 2-3 (Existing user that has authed widget)

  1. Launch old version brave with existing profile
  2. Connect ftx widget
  3. Show some(or all) crypto widgets in stack and set one of crypto widget
  4. Close old verion brave and Launch new Brave
  5. Check ftx widget is not hidden regardless of its position

fix brave/brave-browser#19708

Migration is done once in the first NTP loading.

For new users, all crypto widgets are hidden by default.
For existing users, if current foremost widget is crypto widget,
only it is visible and others are hidden.
After migration, user can control crypto widget's visibility from gallery.

To make all crypto widgets hidden by default, each pref's default
value is not changed. Their default prefs is still "true".
As their visibility are managed by prefs and widget order is managed in local
storage, it's difficult to start migration from c++. If default is changed
to false, it's very difficult to determine whether foremost widget is crypto
or not in the NTP webui.
@simonhong simonhong changed the title WIP: Migrate crypto widgets visibility in NTP Migrate crypto widgets visibility in NTP Nov 24, 2021
Start widget visibility migation when ftx auth state is fetched.
Start widget visibility migation when ftx auth state is fetched.
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Nice fast and clean work. 😄

Would be great to squash the commits if you have time.

@simonhong simonhong merged commit 5b02b59 into master Nov 25, 2021
@simonhong simonhong deleted the ntp_crypto_widget branch November 25, 2021 04:43
@simonhong
Copy link
Member Author

Nice fast and clean work. 😄

Would be great to squash the commits if you have time.

Used squash & merge option!

@simonhong simonhong added this to the 1.34.x - Nightly milestone Nov 25, 2021
simonhong added a commit that referenced this pull request Nov 25, 2021
* Migrate crypto widgets visibility in NTP

fix brave/brave-browser#19708

Migration is done once in the first NTP loading.

For new users, all crypto widgets are hidden by default.
For existing users, if current foremost widget is crypto widget,
only it is visible and others are hidden.
And, authed widgets are visible regardless of its position in widget stack
After migration, user can control crypto widget's visibility from gallery.

To make all crypto widgets hidden by default, each pref's default
value is not changed. Their default prefs is still "true".
As their visibility are managed by prefs and widget order is managed in local
storage, it's difficult to start migration from c++. If default is changed
to false, it's very difficult to determine whether foremost widget is crypto
or not in the NTP webui.
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.34.38 Chromium: 96.0.4664.45 (Official Build) nightly (arm64)
Revision 76e4c1bb2ab4671b8beba3444e61c0f17584b2fc-refs/branch-heads/4664@{#947}
OS macOS Version 12.0.1 (Build 21A559)
  • Verified test plan from pr description
  • Verified for clean profile there are not widgets enabled by default except rewards
  • Verified for upgrade, when none of the widgets are in foreground, post upgrade only rewards is shown
  • Verified when a crypto widget(auth'd or not) is in foreground and rewards is the second one, post upgrade the same order is retained except all other widgets behind rewards is hidden
  • Verified when two widgets one auth'd and one that isn't is the foremost widget and rewards is the third one, post upgrade, only the auth'd widget is retained and is the foremost widget and rewards is behind it. unauth'd widget is hidden
  • Verified when rewards is the last widget, post upgrade all widgets are hidden and only rewards is shown
  • Verified post upgrade only rewards and Brave Talk widgets are retained when Talk is enabled on the older version
  • Verified if all widgets are hidden on older version, post upgrade none of them are enabled including rewards
  • Verified if rewards is disabled and Binance is the foremost widget, post upgrade only Binance widget is retained and rest all is hidden
  • Verified enabling a widget post upgrade is retained between browser restarts

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.

Update default widget list for desktop
3 participants