Replace hideLower with showLess #10164

luixxiul · 2017-07-27T02:48:15Z

Based on the discussion on Slack.

Also:

Rename constants to sort them not only alphabetically but semantically (just sorting alphabetically breaks the semantic order of the existing constants)
Add lacking states to docs/state.md

Closes #10158

Auditors:

Test Plan:

Run npm run add-simulated-synopsis-visits 100
Open about:preferences#payments
Click Show All button
Click Show Less button
Make sure the buttons work

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:

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

NejcZdovc · 2017-07-27T06:29:24Z

docs/state.md

@@ -163,6 +163,7 @@ AppStore
 'advanced.default-zoom-level': number, // the default zoom level for sites that have no specific setting
 'advanced.hardware-acceleration-enabled': boolean, // false if hardware acceleration should be explicitly disabled
 'advanced.hide-excluded-sites': boolean, // whether to hide excluded sites in the payments list
+ 'advanced.show-less-sites': boolean, // whether to show less sites in the payments list


Please sort this alphabetically

d -> ha -> hi -> s, which is not alphabetically?

I thought hide-excluded-sites and show-less-sites should be grouped.

all properties in state.md should be sorted alphabetically

now is d -> ha -> hi -> s -> m

all properties in state.md should be sorted alphabetically

so should be ones in appConfig.js right? working on it.

luixxiul · 2017-07-29T22:53:43Z

docs/state.md

- 'payments.notificationTryPaymentsDismissed': boolean, // true if you dismiss the message or enable Payments
+ 'payments.sites-auto-suggest': boolean, // show auto suggestion
+ 'payments.sites-hide-excluded': boolean, // whether to hide excluded sites in the payments list
+ 'payments.sites-show-less': boolean, // whether to show less sites in the payments list


Renamed above 3 ones to group semantically

codecov-io · 2017-07-29T23:08:29Z

Codecov Report

Merging #10164 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #10164   +/-   ##
=======================================
  Coverage   52.79%   52.79%           
=======================================
  Files         227      227           
  Lines       20214    20214           
  Branches     3236     3236           
=======================================
  Hits        10672    10672           
  Misses       9542     9542

Flag	Coverage Δ
#unittest	`52.79% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
js/constants/settings.js	`100% <ø> (ø)`	⬆️
...components/preferences/payment/advancedSettings.js	`100% <ø> (ø)`	⬆️
app/renderer/components/preferences/paymentsTab.js	`83.51% <100%> (ø)`	⬆️
app/common/lib/publisherUtil.js	`97.22% <100%> (ø)`	⬆️
js/constants/appConfig.js	`100% <100%> (ø)`	⬆️
...erer/components/preferences/payment/ledgerTable.js	`89.72% <100%> (ø)`	⬆️

Also: - Rename constants to sort them not only alphabetically but semantically - Add lacking states to docs/state.md Closes #10158 Auditors: Test Plan: 1. Run `npm run add-simulated-synopsis-visits 100` 2. Open about:preferences#payments 3. Click `Show All` button 4. Click `Show Less` button 5. Make sure the buttons work

cezaraugusto

changes on const names look way more readable. also nice work on docs.

I'm having some ledger tests failing but seems unrelated to this PR so ++

cezaraugusto · 2017-08-06T23:03:47Z

also moving to 0.21.x

luixxiul · 2017-08-06T23:47:20Z

the milestone was moved: #10164 (comment)

bsclifton · 2017-08-07T03:33:53Z

@luixxiul I believe we have a problem here

We can't update the names of the preferences if they are already in production. People already have an existing value and this value is lost completely when they upgrade

Steps to reproduce:

Use an existing version of Brave
dismiss the notification to use Brave payments (this sets payments.notificationTryPaymentsDismissed)
Use this version of Brave (where payments.notificationTryPaymentsDismissed was renamed to payments.notification-try-payments-dismissed)
Notice you'll get prompted again (because the setting was lost)

I checked the other values and they all kept the same name (which is good). We have to be careful about changing the names of any existing preferences. In sessionStore.js, there is a place where you can handle this migration:

browser-laptop/app/sessionStore.js

Line 465 in 0c57538

module.exports.runPreMigrations = (data) => {

cc: @cezaraugusto

bsclifton · 2017-08-07T03:37:17Z

The fix would look something like:

module.exports.runPreMigrations = (data) => {

  // existing code here...

  if (data.settings['payments.notificationTryPaymentsDismissed'] != null) {
    data.settings['payments.notification-try-payments-dismissed'] = data.settings['payments.notificationTryPaymentsDismissed']
  }
}

If you wanted to keep the old value (and constant), you could move the old value here:

browser-laptop/js/constants/settings.js

Line 89 in 821b207

// DEPRECATED settings

All settings stored here are no longer used. We can't ignore them though, because folks could be upgrading from an old version.

cezaraugusto · 2017-08-07T04:39:13Z

sorry I was unaware of that and thanks for the heads-up. I'll came along with a follow-up to avoid revert.

luixxiul · 2017-08-07T05:36:21Z

@bsclifton thanks for the heads-up from me too.

@cezaraugusto thanks in advance :-)

bsclifton · 2017-08-07T08:02:42Z

@cezaraugusto you may check out the conversation in #10181 (comment) for a proposal that would solve this issue too 😄

bsclifton · 2017-10-04T05:49:27Z

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

@NejcZdovc

…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

@NejcZdovc

…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 · 2017-10-04T07:24:34Z

master f0d9c3d
0.20.x 3bc7c1f
0.19.x be048b8

luixxiul added copy/content/text misc/button polish Nice to have — usually related to front-end/visual tasks. labels Jul 27, 2017

luixxiul added this to the 0.20.x (Nightly Channel) milestone Jul 27, 2017

luixxiul self-assigned this Jul 27, 2017

luixxiul requested review from cezaraugusto and NejcZdovc July 27, 2017 02:48

NejcZdovc reviewed Jul 27, 2017

View reviewed changes

luixxiul added the PR/work-in-progress ⚒ label Jul 27, 2017

brave deleted a comment from codecov-io Jul 29, 2017

luixxiul commented Jul 29, 2017

View reviewed changes

luixxiul removed the PR/work-in-progress ⚒ label Jul 30, 2017

luixxiul requested review from NejcZdovc and removed request for NejcZdovc July 30, 2017 06:02

luixxiul added PR/work-in-progress ⚒ and removed PR/work-in-progress ⚒ labels Jul 30, 2017

luixxiul requested a review from NejcZdovc July 30, 2017 07:57

cezaraugusto approved these changes Aug 6, 2017

View reviewed changes

cezaraugusto merged commit f0d9c3d into brave:master Aug 6, 2017

cezaraugusto deleted the polish-hideLower branch August 6, 2017 23:04

luixxiul modified the milestones: 0.21.x (Nightly Channel), 0.20.x (Developer Channel) Aug 6, 2017

luixxiul removed the request for review from NejcZdovc August 6, 2017 23:46

luixxiul mentioned this pull request Aug 6, 2017

Replace 'Hide lower' button's label with something else #10158

Closed

cezaraugusto mentioned this pull request Aug 7, 2017

add support for legacy ledger settings #10322

Closed

This was referenced Aug 11, 2017

can't set minimum visits for publisher relevancy #10439

Closed

fix setting for minimum visits for publisher relevancy #10440

Merged

bsclifton modified the milestones: 0.21.x (Nightly Channel), 0.19.x (Beta Channel) Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace hideLower with showLess #10164

Replace hideLower with showLess #10164

luixxiul commented Jul 27, 2017 •

edited

Loading

NejcZdovc Jul 27, 2017

luixxiul Jul 27, 2017

luixxiul Jul 27, 2017

NejcZdovc Jul 27, 2017

NejcZdovc Jul 27, 2017 •

edited

Loading

luixxiul Jul 27, 2017

luixxiul Jul 29, 2017

codecov-io commented Jul 29, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading

cezaraugusto commented Aug 6, 2017

luixxiul commented Aug 6, 2017

bsclifton commented Aug 7, 2017 •

edited

Loading

bsclifton commented Aug 7, 2017 •

edited

Loading

cezaraugusto commented Aug 7, 2017

luixxiul commented Aug 7, 2017

bsclifton commented Aug 7, 2017

bsclifton commented Oct 4, 2017

bsclifton commented Oct 4, 2017

Replace hideLower with showLess #10164

Replace hideLower with showLess #10164

Conversation

luixxiul commented Jul 27, 2017 • edited Loading

NejcZdovc Jul 27, 2017

Choose a reason for hiding this comment

luixxiul Jul 27, 2017

Choose a reason for hiding this comment

luixxiul Jul 27, 2017

Choose a reason for hiding this comment

NejcZdovc Jul 27, 2017

Choose a reason for hiding this comment

NejcZdovc Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

luixxiul Jul 27, 2017

Choose a reason for hiding this comment

luixxiul Jul 29, 2017

Choose a reason for hiding this comment

codecov-io commented Jul 29, 2017 • edited Loading

Codecov Report

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

cezaraugusto commented Aug 6, 2017

luixxiul commented Aug 6, 2017

bsclifton commented Aug 7, 2017 • edited Loading

bsclifton commented Aug 7, 2017 • edited Loading

cezaraugusto commented Aug 7, 2017

luixxiul commented Aug 7, 2017

bsclifton commented Aug 7, 2017

bsclifton commented Oct 4, 2017

bsclifton commented Oct 4, 2017

luixxiul commented Jul 27, 2017 •

edited

Loading

NejcZdovc Jul 27, 2017 •

edited

Loading

codecov-io commented Jul 29, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading

bsclifton commented Aug 7, 2017 •

edited

Loading

bsclifton commented Aug 7, 2017 •

edited

Loading