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

Fixes include/excluded update in the panel #1782

Merged
merged 3 commits into from
Feb 28, 2019
Merged

Fixes include/excluded update in the panel #1782

merged 3 commits into from
Feb 28, 2019

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Feb 26, 2019

Resolves brave/brave-browser#3480
Resolves brave/brave-browser#3476

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).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  • enable rewards
  • add some sites to ac table
  • make sure that exclude/restore works correctly in settings page
  • in new window to one page that is in your ac table
  • open panel
  • try toggling include and make sure that toggle updates and settings page updates
  • toggle off current site in the panel
  • (don't close panel) click on restore in settings page and make sure that panel reflects this change (toggle is set to include)

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@NejcZdovc NejcZdovc added this to the 0.63.x - Nightly milestone Feb 26, 2019
@NejcZdovc NejcZdovc self-assigned this Feb 26, 2019
@NejcZdovc NejcZdovc force-pushed the include-switch branch 3 times, most recently from 8606c66 to 37606b2 Compare February 27, 2019 06:33
@@ -10,6 +10,7 @@
#include <map>
#include <memory>
#include <string>
#include <utility>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for something in this PR or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes linter error

Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

ac list isn't populating:

  1. Start Brave and enable rewards
  2. Tear off second tab and have rewards dashboard in background
  3. Go to brave.com for the required amount of time
  4. Notice it doesn't show up in rewards screen
  5. Open panel on brave.com tab and exclude the site
  6. Notice dashboard says 1 site excluded
  7. Open panel on brave.com and include site
  8. Site still does not appear

After required amount of time:
screen shot 2019-02-27 at 4 26 09 pm

After excluding using panel:
screen shot 2019-02-27 at 4 26 19 pm

After including again:
screen shot 2019-02-27 at 4 26 09 pm

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Feb 28, 2019

@jasonrsadler so code didn't changed for adding stuff to ac table. Could you please test it on master and see if you see the same problem? I can't reproduce this problem

@NejcZdovc NejcZdovc dismissed jasonrsadler’s stale review February 28, 2019 08:56

I can't reproduce. Can you please check again?

@jasonrsadler
Copy link
Contributor

Yes, that seems to be a separate issue outside this PR

@jasonrsadler
Copy link
Contributor

@NejcZdovc can you rebase brave-browser and re-init. I'm unable to build this currently.

@NejcZdovc NejcZdovc merged commit 027fa5c into master Feb 28, 2019
@NejcZdovc NejcZdovc deleted the include-switch branch February 28, 2019 16:32
NejcZdovc added a commit that referenced this pull request Mar 1, 2019
Fixes include/excluded update in the panel
NejcZdovc added a commit that referenced this pull request Mar 1, 2019
Fixes include/excluded update in the panel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect exclude count displayed in a-c table Cannot switch off Include in Auto-contribute in BR panel
2 participants