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

Move Confirmations into ads process #6248

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Move Confirmations into ads process #6248

merged 2 commits into from
Aug 5, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Jul 29, 2020

Resolves brave/brave-browser#9870

Submitter Checklist:

Test Plan:

  • Confirm visited http/https pages are classified
  • Confirm users are rewarded for viewing ads
  • Confirm confirmation is sent to the server when dismissing an ad
  • Confirm confirmation is sent to the server when an ad has landed (sustained)
  • Confirm confirmation is sent to the server when flagging an ad (Mark as inappropriate on ads history)
  • Confirm confirmation is sent to the server when upvoting an ad (thumbs up on ads history)
  • Confirm confirmation is sent to the server when downvoting an ad (thumbs down on ads history)
  • Confirm brave://rewards UI is updated when rewarded for viewing an ad (also confirm same value after restarting the browser)
  • Confirm brave://rewards UI is updated when viewing an ad (also confirm same value after restarting the browser)
  • Confirm brave://rewards UI is updated when claiming an ads grant (also confirm same value after restarting the browser)
  • Confirm "Ad notifications received this month" reset to 0 on the 1st of the month (also confirm same value after restarting the browser)
  • Confirm "Next payment date" is working as expected (also confirm same value after restarting the browser)
  • Confirm tokens are cashed-out periodically
  • Confirm tokens are refilled when running low
  • Confirm catalog is downloaded periodically
  • Confirm ad conversions are working as expected
  • Confirm state level targeting is working as expected
  • Confirm purchase intent is working as expected

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

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey
Copy link
Collaborator Author

tmancey commented Jul 29, 2020

@fmarier Could you please review security_util.cc, privacy_util.cc and unblinded_tokens.cc (and anything else concerning privacy). Thanks

@tmancey tmancey force-pushed the issues/9870 branch 2 times, most recently from 5ade9cb to 41d8ee9 Compare July 29, 2020 13:28
browser/ui/webui/brave_rewards_page_ui.cc Outdated Show resolved Hide resolved
browser/ui/webui/brave_rewards_page_ui.cc Outdated Show resolved Hide resolved
browser/ui/webui/brave_rewards_page_ui.cc Outdated Show resolved Hide resolved
Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

brave_license_helper.py changes look good.

vendor/brave-ios/BATBraveRewards.h Outdated Show resolved Hide resolved
vendor/brave-ios/Ledger/BATBraveLedger.h Show resolved Hide resolved
vendor/brave-ios/Ledger/BATBraveLedger.mm Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
@tmancey tmancey requested a review from NejcZdovc July 31, 2020 08:56
@tmancey tmancey force-pushed the issues/9870 branch 2 times, most recently from abcabac to e1bb1e6 Compare July 31, 2020 11:38
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.h Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
vendor/brave-ios/BATBraveRewards.h Outdated Show resolved Hide resolved
vendor/brave-ios/BATBraveRewards.h Outdated Show resolved Hide resolved
vendor/brave-ios/BATBraveRewards.mm Outdated Show resolved Hide resolved
vendor/brave-ios/BATBraveRewards.mm Outdated Show resolved Hide resolved
vendor/brave-ios/BATBraveRewards.h Outdated Show resolved Hide resolved
Copy link
Contributor

@moritzhaller moritzhaller left a comment

Choose a reason for hiding this comment

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

Tested and reviewed purchase intent, ad conversions and subdivision targeting. Reviewed search providers. LGTM!

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

approving rewards code, but @tmancey before you merge it please remove base::ScopedAllowBlockingForTesting allow_blocking; for every test as we need to find another way to do it, we can't add it to every test (it was not a problem before, not sure why it's now as you removed all confirmations from rewards code). Also make sure that CI is passing before merging

@NejcZdovc NejcZdovc added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Aug 5, 2020
@NejcZdovc
Copy link
Contributor

CI failed on windows (agent was removed), everything else is green https://ci.brave.com/job/pr-brave-browser-issues-9870/33/flowGraphTable/. Restarting windows

@tmancey tmancey merged commit 2766605 into master Aug 5, 2020
@tmancey tmancey deleted the issues/9870 branch August 5, 2020 10:22
@tmancey tmancey added this to the 1.14.x - Nightly milestone Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64 feature/ads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move confirmations into ads process
6 participants