Auto-include propagates correctly #7680

NejcZdovc · 2017-03-13T09:43:13Z

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).

Resolves #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

Automated test plan

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

bsclifton · 2017-03-13T17:47:09Z

Added @cezaraugusto; during our face to face, this was one that he and @mrose17 were talking about. I believe Cezar had started looking into this

mrose17

LGTM!

bsclifton · 2017-03-13T21:34:28Z

Per discussion in #7451, we need to set the include flag (since it currently changes include status for items when you toggle auto include on/off- this is very confusing for the user)

NejcZdovc · 2017-03-14T11:52:26Z

Ok so now I did two things

when publisher is added we set it false if auto include is off c4094cd#diff-9b959b675446ba95e1abe0b81e0f7a9bR599
I updated clean up functionality, which would go through the undefined auto include property of the publisher and set it to true (we used true as a default fallback in the UI till now) c4094cd#diff-9b959b675446ba95e1abe0b81e0f7a9bR990

This PR is ready for a second review

mrose17

LGTM!

cezaraugusto · 2017-03-15T23:10:33Z

app/ledger.js

+      result = synopsis.publishers[publisher].options.stickyP
+    } else {
+      result = true
+    }
    appActions.changeSiteSetting(pattern, 'ledgerPayments', result)
  }
  delete synopsis.publishers[publisher].options.stickyP


I think you need a check here as well. With those changes synopsis.publishers[publisher] is still undefined at this point. App is crashing for me and pointing on that line.

Worth note: if you have a site in publisher list error doesn't happen. For publishers on exclusion list (such as Youtube), crash is intermittent. Other publishers instantly crash app.

STR: With a fresh profile (no synopsis, no session, no ledger-state), enable Payments and visit a publisher site (i.e. https://clifton.io)

Ok, but why would be failing now. It was outside the if statement before this PR. It was strange for me when I was doing this change, but I left it there because it was there before. Will move it inside the if

nice, let's cc @mrose17 for thoughts

@cezaraugusto can you please try again. I rebase it on the master and now I don't have any crashes.

thanks, no crashes this time. However I followed STR and publisher is still auto-including for me as soon as it populates payments panel.

I did a look inside publisherToggle (only part besides payments that changes publisher state) but state is only changed there if you hit urlbar icon, so it must come from ledger itself.

IMO you might want something similar to this:
https://github.com/brave/browser-laptop/pull/7289/files#diff-9b959b675446ba95e1abe0b81e0f7a9bL566

It's working for me. Do you have auto include set to off in payments?

yep, I removed last comment with wrong video showing STR. After your update it's working fine.

What I saw is that, if you have a clear session with a synopsis file, publishers you had previously are affected by changing auto-suggest, which should only be applied for new publishers. Note that without clearing session-store it works perfectly:

STR:

remove session-store file

keep ledger-synopsis.json (supposing it has publishers)

I suppose auto-include should only apply for new publishers and not for existing ones? If I'm wrong then PR is good to go ;)

No this is something that needs to be fixed. I have implemented this, but it was causing a crash for some X reason (you noticed this in your first comment). So I removed it for now and will try to find another solution today

@cezaraugusto I changed the logic now, can you please try it again

NejcZdovc · 2017-03-18T10:26:33Z

@bsclifton PR is ready now for a review

@mrose17

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

@NejcZdovc

…s-ci with brave#7773 and this broke the `uitest` task. Since the unit tests run quickly and `npm run unittest` still works great, this task isn't really needed anymore. Auditors: @NejcZdovc, @diracdeltas

bsclifton

Manually tested and also ran automated tests. Everything works as expected- great job on this one 😄

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Mar 13, 2017

NejcZdovc added this to the 0.13.6 milestone Mar 13, 2017

NejcZdovc self-assigned this Mar 13, 2017

NejcZdovc mentioned this pull request Mar 13, 2017

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

Closed

bsclifton requested review from mrose17 and cezaraugusto March 13, 2017 17:45

mrose17 approved these changes Mar 13, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#7451-autoinclude branch 2 times, most recently from fa56568 to c4094cd Compare March 14, 2017 11:49

NejcZdovc requested review from mrose17 and bsclifton March 14, 2017 11:52

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Mar 14, 2017

mrose17 approved these changes Mar 14, 2017

View reviewed changes

NejcZdovc added PR/ready-for-merge and removed PR/needs-QA-attention ☕ labels Mar 14, 2017

cezaraugusto suggested changes Mar 15, 2017

View reviewed changes

cezaraugusto removed the PR/ready-for-merge label Mar 15, 2017

NejcZdovc force-pushed the hotfix/#7451-autoinclude branch 7 times, most recently from f7fc142 to a9e6e4d Compare March 18, 2017 10:25

NejcZdovc added the PR/needs-QA-attention ☕ label Mar 18, 2017

NejcZdovc force-pushed the hotfix/#7451-autoinclude branch 4 times, most recently from 2607ca4 to d6c1bce Compare March 19, 2017 15:48

NejcZdovc and others added 2 commits March 19, 2017 23:34

Auto-include propagates correctly

3e4d7d8

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

Removed uitest npm run task. The tests were split/updated for travi…

c8b88bb

…s-ci with brave#7773 and this broke the `uitest` task. Since the unit tests run quickly and `npm run unittest` still works great, this task isn't really needed anymore. Auditors: @NejcZdovc, @diracdeltas

bsclifton force-pushed the hotfix/#7451-autoinclude branch from 0c73653 to c8b88bb Compare March 20, 2017 06:35

bsclifton added QA/test-plan-specified release-notes/include automated-tests and removed PR/needs-QA-attention ☕ labels Mar 20, 2017

bsclifton approved these changes Mar 20, 2017

View reviewed changes

bsclifton merged commit bee2993 into brave:master Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-include propagates correctly #7680

Auto-include propagates correctly #7680

NejcZdovc commented Mar 13, 2017 •

edited by bsclifton

Loading

bsclifton commented Mar 13, 2017

mrose17 left a comment

bsclifton commented Mar 13, 2017

NejcZdovc commented Mar 14, 2017 •

edited

Loading

mrose17 left a comment

cezaraugusto Mar 15, 2017 •

edited

Loading

NejcZdovc Mar 15, 2017 •

edited

Loading

cezaraugusto Mar 15, 2017

NejcZdovc Mar 16, 2017

cezaraugusto Mar 16, 2017 •

edited

Loading

NejcZdovc Mar 16, 2017

cezaraugusto Mar 16, 2017 •

edited

Loading

NejcZdovc Mar 16, 2017 •

edited

Loading

NejcZdovc Mar 18, 2017

NejcZdovc commented Mar 18, 2017

bsclifton left a comment •

edited

Loading

Auto-include propagates correctly #7680

Auto-include propagates correctly #7680

Conversation

NejcZdovc commented Mar 13, 2017 • edited by bsclifton Loading

Auditors

Test Plan

Automated test plan

bsclifton commented Mar 13, 2017

mrose17 left a comment

Choose a reason for hiding this comment

bsclifton commented Mar 13, 2017

NejcZdovc commented Mar 14, 2017 • edited Loading

mrose17 left a comment

Choose a reason for hiding this comment

cezaraugusto Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

cezaraugusto Mar 15, 2017

Choose a reason for hiding this comment

NejcZdovc Mar 16, 2017

Choose a reason for hiding this comment

cezaraugusto Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Mar 16, 2017

Choose a reason for hiding this comment

cezaraugusto Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

NejcZdovc Mar 18, 2017

Choose a reason for hiding this comment

NejcZdovc commented Mar 18, 2017

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Mar 13, 2017 •

edited by bsclifton

Loading

NejcZdovc commented Mar 14, 2017 •

edited

Loading

cezaraugusto Mar 15, 2017 •

edited

Loading

NejcZdovc Mar 15, 2017 •

edited

Loading

cezaraugusto Mar 16, 2017 •

edited

Loading

cezaraugusto Mar 16, 2017 •

edited

Loading

NejcZdovc Mar 16, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading