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

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

Closed
kjozwiak opened this issue Apr 23, 2019 · 6 comments · Fixed by brave/brave-core#2804 or brave/brave-core#3806
Closed

Comments

@kjozwiak
Copy link
Member

Description

As per #3849, the daily limit for ads should be 20. However, it's currently 21. Assuming we're using 0-20 instead of 1-20. Once you reach 21 daily ads, you'll finally receive:

[46648:775:0423/180021.213012:INFO:ads_impl.cc(208)] Browser state changed to idle
[46648:775:0423/180023.214693:INFO:ads_impl.cc(215)] Browser state changed to unidle
[46648:775:0423/180023.217725:INFO:ads_impl.cc(622)] Notification not made: Not allowed based on history

Steps to Reproduce

  1. install a version of Brave that has ads and enable BR via brave://rewards
  2. change Maximum number of ads displayed to 5 ads per hour
  3. claim the maximum amount of ads for the day (should be 20 but currently is 21)

Actual result:

Screen Shot 2019-04-23 at 5 47 21 PM

Expected result:

Should only be 20 ads as per #3849 instead of 21 ads.

Reproduces how often:

100% reproducible when going through the above STR.

Brave version (brave://version info)

Brave 0.63.47 Chromium: 74.0.3729.108 (Official Build) (64-bit)
Revision daaff52abef89988bf2a26091062160b1482b108-refs/branch-heads/3729@{#901}
OS Mac OS X

Version/Channel Information:

  • Can you reproduce this issue with the current release? Yes
  • Can you reproduce this issue with the beta channel? Haven't checked (assuming the same)
  • Can you reproduce this issue with the dev channel? Haven't checked (assuming the same)
  • Can you reproduce this issue with the nightly channel? Haven't checked (assuming the same)

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields? N/A
  • Does the issue resolve itself when disabling Brave Rewards? N/A
  • Is the issue reproducible on the latest version of Chrome? N/A

Miscellaneous Information:

* ad #1 - received ad @ ~12:50pm - confirmed count via `brave://rewards/`
* ad #2 - recieved ad @ ~1:02pm - confirmed count via `brave://rewards/`
* ad #3 - recieved ad @ ~1:15pm - confirmed count via `brave://rewards/`
* ad #4 - recieved ad @ ~1:28pm - confirmed count via `brave://rewards/`
* ad #5 - recieved ad @ ~1:41pm - confirmed count via `brave://rewards/`
* ad #6 - recieved ad @ ~1:55pm - confirmed count via `brave://rewards/`
* ad #7 - recieved ad @ ~2:19pm - confirmed count via `brave://rewards/`
* ad #8 - recieved ad @ ~2:38pm - confirmed count via `brave://rewards/`
* ad #9 - recieved ad @ ~2:50pm - confirmed count via `brave://rewards/`
* ad #10 - recieved ad @ ~3:09pm - confirmed count via `brave://rewards/`
* ad #11 - recieved ad @ ~3:27pm - confirmed count via `brave://rewards/`
* ad #12 - recieved ad @ ~3:40pm - confirmed count via `brave://rewards/`
* ad #13 - recieved ad @ ~3:54pm - confirmed count via `brave://rewards/`
* ad #14 - recieved ad @ ~4:08pm - confirmed count via `brave://rewards/`
* ad #15 - recieved ad @ ~4:26pm - confirmed count via `brave://rewards/`
* ad #16 - recieved ad @ ~4:39pm - confirmed count via `brave://rewards/`
* ad #17 - recieved ad @ ~4:54pm - confirmed count via `brave://rewards/`
* ad #18 - recieved ad @ ~5:07pm - confirmed count via `brave://rewards/`
* ad #19 - recieved ad @ ~5:19pm - confirmed count via `brave://rewards/`
* ad #20 - recieved ad @ ~5:34pm - confirmed count via `brave://rewards/`
* ad #21 - recieved ad @ ~5:48pm - confirmed count via `brave://rewards/`

ad #22 - ~6:00pm (expected):

[46648:775:0423/180021.213012:INFO:ads_impl.cc(208)] Browser state changed to idle
[46648:775:0423/180023.214693:INFO:ads_impl.cc(215)] Browser state changed to unidle
[46648:775:0423/180023.217725:INFO:ads_impl.cc(622)] Notification not made: Not allowed based on history

ad #23 - ~6:13pm (expected):

[46648:775:0423/182623.714091:INFO:ads_impl.cc(208)] Browser state changed to idle
[46648:775:0423/182630.719808:INFO:ads_impl.cc(215)] Browser state changed to unidle
[46648:775:0423/182630.722322:INFO:ads_impl.cc(622)] Notification not made: Not allowed based on history
@srirambv
Copy link
Contributor

Issue reproduced on Linux

Brave 0.63.48 Chromium: 74.0.3729.108 (Official Build) (64-bit)
Revision daaff52abef89988bf2a26091062160b1482b108-refs/branch-heads/3729@{#901}
OS Linux

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Apr 24, 2019

The issue is Reproduced on windows:

image

Brave 0.63.48 Chromium: 74.0.3729.108 (Official Build) (64-bit)
Revision daaff52abef89988bf2a26091062160b1482b108-refs/branch-heads/3729@{#901}
OS Windows 10 OS Build 17134.523

@LaurenWags
Copy link
Member

Reproduced on macOS

Brave 0.63.47 Chromium: 74.0.3729.108 (Official Build) (64-bit)
Revision daaff52abef89988bf2a26091062160b1482b108-refs/branch-heads/3729@{#901}
OS Mac OS X

Screen Shot 2019-04-24 at 9 43 43 AM

  • Sent Default/ads_service/client.json to @tmancey for investigation

@tmancey
Copy link
Contributor

tmancey commented Jun 26, 2019

Reverted last fix as broke frequency capping

@tmancey tmancey assigned tmancey and unassigned tmancey Aug 27, 2019
@tmancey tmancey removed their assignment Aug 27, 2019
@NejcZdovc NejcZdovc removed this from the 0.69.x - Beta milestone Aug 28, 2019
@jsecretan jsecretan added the priority/P2 A bad problem. We might uplift this to the next planned release. label Sep 16, 2019
@jsecretan jsecretan removed the priority/P4 Planned work. We expect to get to it "soon". label Sep 16, 2019
@jsecretan
Copy link

Fixes might be simple, but needs new unit tests for all the capping cases.

@LaurenWags
Copy link
Member

LaurenWags commented Dec 6, 2019

Verified passed with

Brave 1.2.19 Chromium: 79.0.3945.56 (Official Build) beta (64-bit)
Revision 73cc6bf591f792b99f8fc7cdfb8addedbd084bf8-refs/branch-heads/3945@{#788}
OS macOS Version 10.13.6 (Build 17G5019)
  • In order to save time and effort, verified using same file/test plan from Change maximum ads on Android/iOS to 20 #6362 thanks to assistance from @tmancey ❤️
  • Verified more than 20 ads were not shown. When attempting to view 21st ad, this was displayed in the terminal:
[8646:775:1206/124951.062073:INFO:ads_impl.cc(1277)] You have exceeded the allowed ads per day
[8646:775:1206/124951.062291:INFO:ads_impl.cc(1074)] Notification not made: Not allowed based on history

Verification passed on

Brave 1.2.20 Chromium: 79.0.3945.56 (Official Build) beta (64-bit)
Revision 73cc6bf591f792b99f8fc7cdfb8addedbd084bf8-refs/branch-heads/3945@{#788}
OS Ubuntu 18.04 LTS

image

Verification passed on

Brave 1.2.41 Chromium: 79.0.3945.88 (Official Build) (64-bit)
Revision c2a58a36b9411c80829b4b154bfcab97e581f1f3-refs/branch-heads/3945@{#954}
OS Windows 10 OS Version 1803 (Build 17134.1006)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment