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

Updates state.excluded in reducer when publisher exclusion is updated #1010

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Dec 3, 2018

Fixes: brave/brave-browser#2340
Native ledger PR: brave-intl/bat-native-ledger#194

Previously, state.excluded was only set when a publisher exclude action was triggered (by clicking on 'excluded' in the -ac table). https://github.com/brave/brave-core/blob/master/components/brave_rewards/resources/ui/reducers/publishers_reducer.ts#L27

With muon import, excluded publishers are handled directly via this call:

void RewardsServiceImpl::ExcludePublisher(const std::string publisherKey) const {
ledger_->SetPublisherExclude(publisherKey, ledger::PUBLISHER_EXCLUDE::EXCLUDED);
}

Because of the direct call, the above reducer wasn't getting hit, therefore state was not reflected.

This adds the publisherKey of the updated publisher to the callback that feeds in to ON_NUM_EXCLUDED_SITES, which is triggered whenever there is an exclusion update. The publisherKey is then added or removed from state.excluded depending on what their new status is.

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
  • 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:

  1. Launch Brave with rewards enabled.
  2. Visit some sites to add them to the a-c table.
  3. Ensure publisher exclusion works correctly.
  4. Ensure that publisher restoration works correctly.
  5. Ensure that publishers can be re-excluded after a restore.
  6. Confirm that state.excluded is updated appropriately.

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

@bsclifton
Copy link
Member

I did a rebase on this and tried to build locally:

src/brave/vendor/bat-native-ledger (ERROR)
----------------------------------------
[0:00:08] Started.
[0:00:11] From ssh://github.com/brave-intl/bat-native-ledger
[0:00:11]    16bfe91..48344ee  0.59.x     -> origin/0.59.x
[0:00:11]    b4fb7f9..c07de55  master     -> origin/master
----------------------------------------
Error: Command 'git checkout --quiet 6ca82c7c00971ed8bd90cc0ab0f5c7a6a8e5fa8b' returned non-zero exit status 128 in /Users/clifton/Documents/brave-browser/src/brave/vendor/bat-native-ledger
fatal: reference is not a tree: 6ca82c7c00971ed8bd90cc0ab0f5c7a6a8e5fa8b

@NejcZdovc NejcZdovc self-requested a review December 4, 2018 05:35
@ryanml
Copy link
Contributor Author

ryanml commented Dec 4, 2018

@clifton - strange. Can you ensure the native-ledger in DEPS is pointing to 6ca82c7c00971ed8bd90cc0ab0f5c7a6a8e5fa8b on your local branch?

@ryanml ryanml force-pushed the fix-2340 branch 6 times, most recently from d8b0873 to c53a7f5 Compare December 4, 2018 12:21
@bsclifton
Copy link
Member

I think this might be too large a change (late in the game) for 0.57.x ☹️ But it would be a good candidate for 0.58.x (if we wanted to fix). The original issue was closed as wontfix but we can re-open

@NejcZdovc NejcZdovc merged commit 0c068d3 into brave:master Dec 5, 2018
NejcZdovc added a commit that referenced this pull request Dec 5, 2018
Updates state.excluded in reducer when publisher exclusion is updated
@NejcZdovc
Copy link
Contributor

master (0.60) 0c068d3
0.59.x 548daf0

@bbondy bbondy added this to the 0.59.x - Beta milestone Jan 14, 2019
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.

deleted sites from muon are not being imported to Rewards - follow up to PR 736
4 participants