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 converted ads keep showing #5381

Merged
merged 1 commit into from
Apr 28, 2020
Merged

Fixes converted ads keep showing #5381

merged 1 commit into from
Apr 28, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Apr 27, 2020

Resolves brave/brave-browser#9436

Submitter Checklist:

Test Plan:

See brave/brave-browser#9436

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.

namespace ads {

ConversionFrequencyCap::ConversionFrequencyCap(
const FrequencyCapping* const frequency_capping)
Copy link
Contributor

Choose a reason for hiding this comment

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

double const?

Copy link
Collaborator Author

@tmancey tmancey Apr 28, 2020

Choose a reason for hiding this comment

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

const X* const p means "p is a const pointer to an X that is const": you can’t change the pointer p itself, nor can you change the X object via p.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tmancey tmancey requested a review from NejcZdovc April 28, 2020 09:45
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.

code looks good to me, didn't test out functionality of it

@tmancey tmancey added this to the 1.10.x - Nightly milestone Apr 28, 2020
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.

lgtm! (focussed mainly on logic, did not run tests locally)

@tmancey tmancey merged commit e66ec69 into master Apr 28, 2020
@tmancey tmancey deleted the issues/9436 branch April 28, 2020 11:51
@btlechowski
Copy link

LGTM

Brave 1.10.28 Chromium: 81.0.4044.129 (Official Build) nightly (64-bit)
Revision 3d71af9f5704a40b85806f4d08925db24605ba25-refs/branch-heads/4044@{#979}
OS Linux

Verified test plan from brave/brave-browser#9436.
Also checked the logs for creativeSetId (...) has exceeded the frequency capping for conversions message.
Verified ads from the same creativeSetId were not shown after conversion.

[6080:1:0505/010858.182044:INFO:ads_impl.cc(359)] Browser state changed to unidle
[6080:1:0505/010858.204730:INFO:ads_impl.cc(954)] Serving ad from categories:
[6080:1:0505/010858.204876:INFO:ads_impl.cc(956)]   technology & computing-technology & computing
[6080:1:0505/010858.205649:INFO:ads_impl.cc(956)]   arts & entertainment-video games
[6080:1:0505/010858.205814:INFO:ads_impl.cc(956)]   technology & computing-software
[6080:1:0505/010858.207833:INFO:ads_impl.cc(974)] No eligible ads found in categories:
[6080:1:0505/010858.210006:INFO:ads_impl.cc(976)]   technology & computing-technology & computing
[6080:1:0505/010858.216231:INFO:ads_impl.cc(976)]   arts & entertainment-video games
[6080:1:0505/010858.216346:INFO:ads_impl.cc(976)]   technology & computing-software
[6080:1:0505/010858.216469:INFO:ads_impl.cc(1007)] Serving ad from parent categories:
[6080:1:0505/010858.216551:INFO:ads_impl.cc(1009)]   technology & computing
[6080:1:0505/010858.216631:INFO:ads_impl.cc(1009)]   arts & entertainment
[6080:1:0505/010858.231964:INFO:ads_impl.cc(1188)] All advertisers have been shown, so round robin
[6080:1:0505/010858.234288:INFO:client.cc(420)] Resetting seen advertisers
[6080:1:0505/010858.234825:INFO:ads_impl.cc(1130)] creativeSetId de5a82e1-17d6-47e0-a368-17f8f56dfeb3 has exceeded the frequency capping for conversions
[6080:1:0505/010858.235719:INFO:ads_impl.cc(974)] No eligible ads found in categories:
[6080:1:0505/010858.235923:INFO:ads_impl.cc(976)]   technology & computing
[6080:1:0505/010858.236101:INFO:ads_impl.cc(976)]   arts & entertainment
[6080:1:0505/010858.236275:INFO:ads_impl.cc(1020)] Serving ad notification from untargeted category
[6080:1:0505/010858.243086:INFO:client.cc(593)] Successfully saved client state
[6080:1:0505/010858.243522:INFO:ads_impl.cc(1080)] Notification not made: No eligible ads found

Verified that without conversion, ads keep showing for the same creative set

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.

Follow up to #8645 - Converted ads keep showing
4 participants