Skip to content

Add NTP sponsored images frequency capping. #11945

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

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Conversation

aseren
Copy link
Collaborator

@aseren aseren commented Jan 21, 2022

Now information about which SI should be rendered on NTP is requested from ads library.
NTP tries to render SI each 4th view. SI could be frequency capped. In this case NTP renders background wallpaper.

If Brave Ads are disabled, then frequency capping is not applied and SI is shown each 4th NTP view.

Resolves brave/brave-browser#14015

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Case 1

  • Start browser with clean profile. Do not enable rewards. Enable rewards verbose logging.
  • Check that NTP Sponsored images ads are shown on each 4th refresh of NTP.
  • Enable rewards.
  • Check that NTP SI ads become served by Ads Library. Each 4th NTP refresh requests NTP ad serving.
  • If NTP ad wasn’t served then following message should appear in rewards log: New tab page ad not served
  • Disable Brave Private Ads.
  • Check that NTP Sponsored images ads are shown on each 4th refresh of NTP.

Case 2

  • Start browser with clean profile. Enable rewards. Enable rewards verbose logging.
  • NTP ad serving is first requested on second NTP refresh after browser start. After that it is requested each 4th NTP refresh. If NTP ad is served then it will be shown after 4 NTP refreshes.
  • Check that NTP ad is served. There should be following message in rewards log: Served new tab page ad
  • Check that if NTP ad is served, then it is shown on NTP after 4 refreshes. There should be following messages in rewards log: Viewed new tab page ad with placement id and Successfully logged new tab page ad viewed event.
  • Check that subsequent attempts of NTP ad serving (at least during 5 minutes after success serving) are not successful and lead to New tab page ad not served message in rewards log.

@aseren aseren requested a review from simonhong January 21, 2022 06:49
@aseren aseren force-pushed the issues/14015_wallpapers branch 2 times, most recently from f98053f to 8482f44 Compare January 25, 2022 16:30
@aseren aseren requested a review from tmancey January 25, 2022 16:36
@aseren aseren force-pushed the issues/14015_wallpapers branch from 8482f44 to 3a4b39a Compare January 28, 2022 12:38
@aseren aseren marked this pull request as ready for review January 28, 2022 14:31
@github-actions github-actions bot added rebase and removed rebase labels Jan 28, 2022
@brave-builds brave-builds force-pushed the issues/14015_wallpapers branch from 3a4b39a to ea97398 Compare January 28, 2022 14:33
@aseren aseren force-pushed the issues/14015_wallpapers branch 3 times, most recently from 896ac53 to 8c5df23 Compare January 31, 2022 15:51
@aseren aseren requested a review from a team as a code owner January 31, 2022 15:51
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

++ DEPS

@aseren aseren force-pushed the issues/14015_wallpapers branch from 8c5df23 to 7625160 Compare February 2, 2022 15:11
@aseren aseren changed the title Issues/14015 wallpapers Add NTP sponsored images frequency capping. Feb 2, 2022
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

ntp_background_images ++

@aseren aseren force-pushed the issues/14015_wallpapers branch from 7625160 to 9a427a5 Compare February 4, 2022 11:35
@aseren aseren force-pushed the issues/14015_wallpapers branch from 9a427a5 to 29c4ce6 Compare February 10, 2022 12:41
@aseren aseren requested a review from a team as a code owner February 10, 2022 12:41
@aseren aseren requested a review from tmancey February 10, 2022 12:47
@aseren aseren force-pushed the issues/14015_wallpapers branch 2 times, most recently from f7069c2 to fcaf023 Compare February 14, 2022 06:36
@aseren aseren force-pushed the issues/14015_wallpapers branch from 0567fd2 to 122f058 Compare February 21, 2022 16:36
@aseren aseren force-pushed the issues/14015_wallpapers branch 2 times, most recently from 0fea4ea to 212267b Compare February 22, 2022 15:55
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

Looked at the wrong callback type, thanks, iOS++

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++ (added a couple nits)

@aseren aseren force-pushed the issues/14015_wallpapers branch 7 times, most recently from 9673e76 to 11a3608 Compare June 16, 2022 11:39
@aseren aseren force-pushed the issues/14015_wallpapers branch from 11a3608 to 25c169e Compare June 16, 2022 17:30
@tmancey tmancey merged commit 81564f0 into master Jun 17, 2022
@tmancey tmancey deleted the issues/14015_wallpapers branch June 17, 2022 15:04
@tmancey tmancey added this to the 1.42.x - Nightly milestone Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not show NTP sponsored images if the user has exceeded the frequency caps
5 participants