Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

add support for legacy ledger settings #10441

Merged
merged 3 commits into from
Aug 23, 2017
Merged

add support for legacy ledger settings #10441

merged 3 commits into from
Aug 23, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Aug 11, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

npm run test -- --grep="runPreMigrations"

Manual test plan:

  • Go to payments tab, enable and:

before checking this PR

  1. set auto-incude to OFF
  2. set Show only included sites to ON
  3. click the gear on top-right
  4. set Minimum page time before logging a visit to 8 seconds
  5. set Minimum visits for publisher relevancy to 10 visits
  6. set Allow contributions to non-verified sites to false
  7. scroll down publishers list, toggle the bottom button to show more sites (button now says show less)

checkout this pr
ensure the above data is persisted:

  1. auto-incude is OFF
  2. Show only included sites is ON
  3. Minimum page time before logging a visit is 8 seconds
  4. Minimum visits for publisher relevancy is 10 visits
  5. Allow contributions to non-verified sites is false
  6. scroll down publishers list, bottom button should say show less

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@cezaraugusto cezaraugusto added this to the 0.21.x (Nightly Channel) milestone Aug 11, 2017
@cezaraugusto cezaraugusto self-assigned this Aug 11, 2017
@codecov-io
Copy link

codecov-io commented Aug 11, 2017

Codecov Report

Merging #10441 into master will increase coverage by 0.41%.
The diff coverage is 61.9%.

@@            Coverage Diff             @@
##           master   #10441      +/-   ##
==========================================
+ Coverage   53.81%   54.23%   +0.41%     
==========================================
  Files         238      244       +6     
  Lines       21054    21122      +68     
  Branches     3258     3262       +4     
==========================================
+ Hits        11330    11455     +125     
+ Misses       9724     9667      -57
Flag Coverage Δ
#unittest 54.23% <61.9%> (+0.41%) ⬆️
Impacted Files Coverage Δ
js/constants/appConfig.js 100% <ø> (ø) ⬆️
js/constants/settings.js 100% <ø> (ø) ⬆️
js/settings.js 96.92% <100%> (-0.22%) ⬇️
app/sessionStore.js 64.68% <60%> (-0.43%) ⬇️
app/channel.js 85% <0%> (-7.31%) ⬇️
app/renderer/reducers/frameReducer.js 26.31% <0%> (-1.67%) ⬇️
app/browser/reducers/historyReducer.js 98.87% <0%> (-1.13%) ⬇️
js/stores/windowStore.js 29.77% <0%> (-0.84%) ⬇️
app/renderer/components/common/browserButton.js 69.04% <0%> (-0.72%) ⬇️
... and 40 more

@cezaraugusto
Copy link
Contributor Author

xoxo codecov

Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

The test plan works. Thanks for taking this!

- Somehow extension preferences got added into deprecated section. Moved these out
- Made deprecated settings more explicit
- updated state.md to call out deprecated settings
- removed bitwarden/enpass tests and setting. This was never a value in 0.11.x or prior so no migration was needed

Auditors: @cezaraugusto
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

OK wow- this was a little bit of a mess, but glad we're fixing it up 😄 I added a commit which cleans things up a bit more.

Cezar, there is a section I think you can move your change over to (where I have made similar changes 😄 ). Please take a look at

// Retrofit a new setting based on old values; we don't want to lose existing user settings.
const getDefaultSetting = (settingKey, settingsCollection) => {
switch (settingKey) {
case settings.ACTIVE_PASSWORD_MANAGER:
return passwordManagerDefault(settingKey, settingsCollection)
case settings.BOOKMARKS_TOOLBAR_MODE:
return bookmarksBarDefault(settingKey, settingsCollection)
}
return undefined
}

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

On second thought... I think it's just fine how you implemented it 😄 We can revisit this with #10488

@bsclifton bsclifton merged commit 052277d into master Aug 23, 2017
@bsclifton bsclifton deleted the 10322 branch August 23, 2017 00:36
@darkdh
Copy link
Member

darkdh commented Aug 25, 2017

++ except missing delete legacy addressed in 9ad7b4f

@bsclifton
Copy link
Member

We're running into issues and unfortunately, this PR is causing issues with the BAT Mercury work (since settings were renamed). I'm going to pull this into 0.19.x

@bsclifton bsclifton modified the milestones: 0.21.x (Nightly Channel), 0.19.x (Beta Channel) Oct 4, 2017
bsclifton pushed a commit that referenced this pull request Oct 4, 2017
…BAT Mercury.

Details:
Because the settings were renamed, git didn't properly handle changes when cherry-picking backwards.
This caused the code in 0.19.x and 0.20.x to use the naming from 0.21.x

Merge pull request #10164 from luixxiul/polish-hideLower
Replace hideLower with showLess
f0d9c3d

Merge pull request #10441 from brave/10322
add support for legacy ledger settings
052277d

Also includes fixes from 9ad7b4f (most of this commit was already backported, just grabbed the settings migration fixes)

Auditors: @NejcZdovc, @luixxiul, @cezaraugusto

Fixes #11261
Fixes #11260
Fixes #11250
Fixes #11246
Fixes #11263
bsclifton pushed a commit that referenced this pull request Oct 4, 2017
…BAT Mercury.

Details:
Because the settings were renamed, git didn't properly handle changes when cherry-picking backwards.
This caused the code in 0.19.x and 0.20.x to use the naming from 0.21.x

Merge pull request #10164 from luixxiul/polish-hideLower
Replace hideLower with showLess
f0d9c3d

Merge pull request #10441 from brave/10322
add support for legacy ledger settings
052277d

Also includes fixes from 9ad7b4f (most of this commit was already backported, just grabbed the settings migration fixes)

Auditors: @NejcZdovc, @luixxiul, @cezaraugusto

Fixes #11261
Fixes #11260
Fixes #11250
Fixes #11246
Fixes #11263
@bsclifton
Copy link
Member

master 052277d
0.20.x 3bc7c1f
0.19.x be048b8

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

Successfully merging this pull request may close these issues.

5 participants