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

Fixes maximum daily ads at 21 instead of 20 (follow up to #3849) #3806

Merged
merged 1 commit into from
Nov 4, 2019
Merged

Fixes maximum daily ads at 21 instead of 20 (follow up to #3849) #3806

merged 1 commit into from
Nov 4, 2019

Conversation

masparrow
Copy link
Contributor

@masparrow masparrow commented Oct 29, 2019 •
edited by tmancey
Loading

Resolves brave/brave-browser#4207

Submitter Checklist:

Test Plan:

Regarding the original 4207 issue - see that issue for testing.

This PR refactors the core logic of the 'Should an Ad be shown' logic - as such, QA should check all functionality in this area.

For Sanity testing purposes...

  • Capture the JSON from https://ads-serve.bravesoftware.com/v1/catalog
  • Edit it to contain only one campaign, with at least 21 unique ads.
  • Ensure that 'segments' 'name' is "technology & computing"
  • Alter 'perDay' to 20
  • Alter 'dailyCap' to 30
  • Save this JSON
  • Using Charles Proxy (or similar) use 'Map Local' to redirect the above URL to your edited JSON
  • Clean install Brave
  • Enable Rewards
  • Visit https://brave.com/
  • Ensure 20 ads are shown during a given day (and not 21)

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.

@masparrow masparrow added this to the 0.73.x - Nightly milestone Oct 29, 2019
@masparrow masparrow requested review from tmancey and a team October 29, 2019 10:02
@masparrow masparrow self-assigned this Oct 29, 2019
tmancey
tmancey requested changes Oct 29, 2019
View reviewed changes
vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc Outdated Show resolved Hide resolved
...ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap_unittest.cc Outdated Show resolved Hide resolved
...ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap_unittest.cc Outdated Show resolved Hide resolved
...ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap_unittest.cc Outdated Show resolved Hide resolved
...s/src/bat/ads/internal/frequency_capping/exclusion_rules/total_max_frequency_cap_unittest.cc Show resolved Hide resolved
...s/src/bat/ads/internal/frequency_capping/permission_rules/minimum_wait_time_frequency_cap.cc Outdated Show resolved Hide resolved
...s/src/bat/ads/internal/frequency_capping/permission_rules/minimum_wait_time_frequency_cap.cc Outdated Show resolved Hide resolved
...s/src/bat/ads/internal/frequency_capping/permission_rules/minimum_wait_time_frequency_cap.cc Outdated Show resolved Hide resolved
...ve-ads/src/bat/ads/internal/frequency_capping/permission_rules/per_day_limit_frequency_cap.h Outdated Show resolved Hide resolved
...t-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap.cc Outdated Show resolved Hide resolved
@tmancey tmancey self-requested a review October 29, 2019 11:39
@tmancey tmancey self-requested a review October 29, 2019 11:56
@masparrow masparrow marked this pull request as ready for review November 1, 2019 19:14
@tmancey tmancey self-requested a review November 1, 2019 21:56
tmancey
tmancey reviewed Nov 2, 2019
View reviewed changes
...ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap_unittest.cc

namespace ads {

const uint64_t kSecondsPerDay = base::Time::kSecondsPerHour *
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be in empty namespace above

tmancey
tmancey reviewed Nov 2, 2019
View reviewed changes
...ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap_unittest.cc
Comment on lines +40 to +41
: mock_ads_client_(std::make_unique<MockAdsClient>()),
ads_(std::make_unique<AdsImpl>(mock_ads_client_.get())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 4 additional spaces

tmancey
tmancey reviewed Nov 2, 2019
View reviewed changes
vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/permission_rule.h

class PermissionRule {
public:
virtual ~PermissionRule() = default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this not be = 0

tmancey
tmancey approved these changes Nov 2, 2019
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM++ but CI needs to run as it failed due to skip labels

@tmancey tmancey self-requested a review November 4, 2019 18:11
tmancey
tmancey approved these changes Nov 4, 2019
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++

@tmancey tmancey self-requested a review November 4, 2019 22:38
tmancey
tmancey approved these changes Nov 4, 2019
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++

@tmancey
Copy link
Collaborator

tmancey commented Nov 4, 2019

Unrelated CI failure due to security tests

@tmancey tmancey merged commit 69dc5fc into master Nov 4, 2019
@tmancey tmancey deleted the issues/4207 branch November 4, 2019 22:41
fmarier pushed a commit to fmarier/brave-core that referenced this pull request Nov 6, 2019
@masparrow @fmarier
Fixes maximum daily ads at 21 instead of 20 (follow up to brave#3849)
ad7fdc5 
Fixes brave#3806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmancey tmancey tmancey approved these changes

@emerick emerick Awaiting requested review from emerick

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@masparrow masparrow

Labels
feature/ads
Projects
None yet
Milestone
1.2.x - Release
Development

Successfully merging this pull request may close these issues.

maximum daily ads at 21 instead of 20 (follow up to #3849)
2 participants
@masparrow @tmancey