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.

Sign up for GitHub

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

Jump to bottom

Auto-Contribute list only contains included publishers #475

Merged
merged 1 commit into from
Oct 2, 2018
Merged

Auto-Contribute list only contains included publishers #475

merged 1 commit into from
Oct 2, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Sep 19, 2018 •
edited
Loading

Fixes: brave/brave-browser#1175

This requires brave-intl/bat-native-ledger#117 for testing

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.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

Essentially this can be best described as a regression test for the exclusion/restoration of excluded publishers.

  1. Build Brave from this branch with storage
  2. Enable Payments
  3. Add 6 - 10 sites to the auto-contribute table
  4. Exclude 1 or 2 sites
  5. Ensure that the number of excluded sites updates appropriately
  6. Restore the excluded publishers
  7. Ensure that the restoration message goes away
  8. Re-excluded the same publishers
  9. Ensure that accurate count is reflected.

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

@ryanml ryanml self-assigned this Sep 19, 2018
@ryanml ryanml force-pushed the fix-1175 branch 2 times, most recently from 419c3ef to 3440658 Compare September 26, 2018 02:06
@ryanml ryanml force-pushed the fix-1175 branch 2 times, most recently from a671772 to 98b64bd Compare September 27, 2018 05:27
@ryanml ryanml mentioned this pull request Sep 27, 2018
Merged
@ryanml ryanml force-pushed the fix-1175 branch 2 times, most recently from dd27728 to 7fcaf22 Compare September 27, 2018 23:38
ryanml
ryanml commented Sep 27, 2018
View reviewed changes
browser/ui/webui/brave_rewards_ui.cc Outdated
@@ -293,6 +294,13 @@ void RewardsDOMHandler::GetWalletPassphrase(const base::ListValue* args) {
}
}

void RewardsDOMHandler::GetNumExcludedSites() {
if (rewards_service_ && web_ui()->CanCallJavascript()) {
int num = (int)rewards_service_->GetNumExcludedSites();
Copy link
Contributor Author

@ryanml ryanml Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note: This is casted to int because base::Value doesn't support unsigned int types for some reason

@ryanml ryanml changed the title WIP: Auto-Contribute list only contains included publishers Auto-Contribute list only contains included publishers Sep 28, 2018
NejcZdovc
NejcZdovc reviewed Sep 29, 2018
View reviewed changes
components/brave_rewards/browser/publisher_info_database.cc Outdated
@@ -422,6 +422,8 @@ std::string PublisherInfoDatabase::BuildClauses(int start,
if (filter.year > 0)
clauses += " AND ai.year = ?";

clauses += " AND pi.excluded = ?";
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible that excluded is not provided in the filter?

NejcZdovc
NejcZdovc reviewed Sep 29, 2018
View reviewed changes
components/brave_rewards/browser/publisher_info_database.cc Outdated
@@ -452,6 +454,8 @@ void PublisherInfoDatabase::BindFilter(sql::Statement& statement,

if (filter.year > 0)
statement.BindInt(column++, filter.year);

statement.BindInt(column++, filter.excluded);
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment above

NejcZdovc
NejcZdovc reviewed Sep 29, 2018
View reviewed changes
browser/ui/webui/brave_rewards_ui.cc Outdated
@@ -442,6 +450,8 @@ void RewardsDOMHandler::OnGetContentSiteList(std::unique_ptr<brave_rewards::Cont
publishers->Append(std::move(publisher));
}

// numExcludedSites should update with the new content list
GetNumExcludedSites();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to get new number list when we are fetching regular list?

Shouldn't we just get it when you do exclude/include action?

@ryanml ryanml force-pushed the fix-1175 branch 5 times, most recently from db0d4df to ffd1001 Compare October 1, 2018 23:35
@ryanml ryanml requested a review from NejcZdovc October 1, 2018 23:35
@ryanml
Copy link
Contributor Author

ryanml commented Oct 1, 2018

@NejcZdovc this is ready for another look along with: brave-intl/bat-native-ledger#117

We should now be listening to the OnExcludedSitesChanged observer, so we are not checking for the excluded sites number unless a publisher's exclusion status has changed.

@NejcZdovc NejcZdovc force-pushed the fix-1175 branch 2 times, most recently from 549d3a7 to c175d3d Compare October 2, 2018 22:37
NejcZdovc
NejcZdovc previously approved these changes Oct 2, 2018
View reviewed changes
@ryanml @NejcZdovc
Fixes brave/brave-browser#1175
d1c9c82 
Auto-Contribute list only contains included pubs by default
numExcludedSites is now part of the AppState and passed as a modalContribute prop
@NejcZdovc NejcZdovc merged commit dc2f9fb into brave:master Oct 2, 2018
NejcZdovc added a commit that referenced this pull request Oct 2, 2018
@NejcZdovc
Merge pull request #475 from ryanml/fix-1175
097cc65 
Auto-Contribute list only contains included publishers
@NejcZdovc
Copy link
Contributor

master dc2f9fb
0.56.x 097cc65
0.55.x 9868227

NejcZdovc added a commit that referenced this pull request Oct 2, 2018
@NejcZdovc
Merge pull request #475 from ryanml/fix-1175
9868227 
Auto-Contribute list only contains included publishers
@ryanml ryanml mentioned this pull request Oct 9, 2018
Closed
@NejcZdovc NejcZdovc modified the milestones: Closed, 0.55.x - Release Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@jasonrsadler jasonrsadler Awaiting requested review from jasonrsadler

@SergeyZhukovsky SergeyZhukovsky Awaiting requested review from SergeyZhukovsky

Assignees

@ryanml ryanml

Labels
feature/rewards
Projects
None yet
Milestone
0.55.x - Release
Development

Successfully merging this pull request may close these issues.

Auto-contribute list should only contain included publishers
2 participants
@ryanml @NejcZdovc