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

Fixed publishers are auto included even when auto-include sites is turned off #7451

Closed
srirambv opened this issue Mar 2, 2017 · 7 comments
Closed

Comments

@srirambv
Copy link
Collaborator

srirambv commented Mar 2, 2017

Test Plan

  • Enable payments in settings
  • Disable auto-include switch
  • Visit brianbondy.com in a new tab, site is added in the list and is not included

Automated test plan

  • npm run test -- --grep="auto include"

Original issue description

  • Did you search for similar issues before submitting this one?
    Yes

  • Describe the issue you encountered:
    Publishers are auto included even when auto-include sites is turned off

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    Windows 10 x64

  • Brave Version (revision SHA):
    Brave 0.13.5
    rev 0e68342

  • Steps to reproduce:

    1. Enable payments in settings
    2. Disable auto-include switch
    3. Visit brianbondy.com in a new tab, site is added in the list and is auto included
  • Actual result:
    Publishers are auto included even when auto-include sites is turned off

  • Expected result:
    If auto-suggest is off then it should be set as disabled

  • Will the steps above reproduce in a fresh profile? If not what other info can be added?
    Yes

  • Is this an issue in the currently released version?
    No

  • Can this issue be consistently reproduced?
    Yes

  • Extra QA steps:
    1.
    2.
    3.

  • Screenshot if needed:
    image

  • Any related issues:
    Include switch is enabled by default for publishers even though auto include is turned off #7429
    cc: @mrose17 @cezaraugusto

@cezaraugusto
Copy link
Contributor

Stepping back and assigning @mrose17

@mrose17
Copy link
Member

mrose17 commented Mar 8, 2017

@cezaraugusto - i think i understand what's going on here in enabledForSite at https://github.com/brave/browser-laptop/blob/master/app/renderer/components/preferences/ledgerTable.js#L51 the default return true should return the value of settings.advanced.auto-suggest-sites (not the exact syntax).

does that make sense to you?

@mrose17 mrose17 assigned cezaraugusto and unassigned mrose17 Mar 8, 2017
@NejcZdovc
Copy link
Contributor

@cezaraugusto also we need to change it here https://github.com/brave/browser-laptop/blob/master/app/renderer/components/preferences/ledgerTable.js#L101, we are sending true as default value

@srirambv
Copy link
Collaborator Author

@NejcZdovc NejcZdovc assigned NejcZdovc and unassigned cezaraugusto Mar 13, 2017
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Mar 13, 2017

@mrose17 I fixed this auto include problem, but I think that we have other problem as well. We don't set ledgerPayments to false, when we have auto include off. Which it the root cause for this "problem" (you can find updated code in #7680):
7

Publisher 1 (24ur.com): added when auto include was off
Publisher 2 (dailymotion.com): added when auto include was on
Publisher 3 (clifton.io): added when auto include was off, but then changed to include
Publisher 4 (slo-tech): added when auto include was off

When we add a publisher we don't set a value for the ledgerPayments, but we use default value based on auto include setting only when rendering (we don't save it). This means that when you toggle auto include sites include changes based on that.

My proposal
I would expect that if you have an auto include off, this should be saved into the store and can't be effected with a later change of auto include checkbox.

What do you think?

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 13, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
@mrose17
Copy link
Member

mrose17 commented Mar 13, 2017

@NejcZdovc - i agree!

cc: @bsclifton

@bsclifton
Copy link
Member

Sounds good to me! I'll hold off on the merge until this is ready 😄

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 14, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 14, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 18, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 18, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 18, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 18, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 18, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 19, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 19, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 19, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Mar 19, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included
bsclifton pushed a commit to NejcZdovc/browser-laptop that referenced this issue Mar 20, 2017
Resolves brave#7451

Auditors: @mrose17

Test Plan:
- Enable payments in settings
- Disable auto-include switch
- Visit brianbondy.com in a new tab, site is added in the list and is not included

Includes webdriver tests
bsclifton added a commit that referenced this issue Mar 20, 2017
@alexwykoff alexwykoff changed the title Publishers are auto included even when auto-include sites is turned off Fixed publishers are auto included even when auto-include sites is turned off Mar 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.