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

Reset expire date for ads catalog when it is fetched from the server #20843

Closed
btlechowski opened this issue Feb 2, 2022 · 4 comments · Fixed by brave/brave-core#12795
Closed

Comments

@btlechowski
Copy link

btlechowski commented Feb 2, 2022

Hypothetically, it could happen that catalog has not been updated server side for 24h and therefor the catalog would expire on all clients even though server would serve the latest catalog. We don't show ads when catalog is expired.

Important: This issue has not been observed in the wild.

Solution is to always reset expire date when catalog is successfully fetched from the server, so the catalog would only expire if not fetched from the server for 24h.

Steps to Reproduce

  1. Download the catalog
  2. Overwrite the catalog with the downloaded catalog through Charles (so that catalog is not updated)
  3. Run Brave
  4. Enable rewards and ads
  5. Make sure ads catalog is fetched
  6. Close browser
  7. Wait 24h
  8. Run browser
  9. Make sure ads catalog is fetched
  10. Trigger an ad

Actual result:

catalog is downloaded and logs show that catalog is up to date.
ad is not shown due to expired catalog

Expected result:

catalog is downloaded and logs show that catalog is up to date.
ad is shown

Reproduces how often:

Easily reproduced

Brave version (brave://version info)

Brave 1.35.100 Chromium: 98.0.4758.87 (Official Build) (64-bit)
Revision e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS Ubuntu 18.04 LTS

cc @brave/legacy_qa @rebron @jsecretan @tmancey

@btlechowski btlechowski changed the title Reset expire date for ads catalog when server acknowledges no update Reset expire date for ads catalog when it was not updated on the server Feb 2, 2022
@btlechowski btlechowski changed the title Reset expire date for ads catalog when it was not updated on the server Reset expire date for ads catalog when it is fetched from the server Feb 2, 2022
@tmancey tmancey added blocked needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. and removed QA/Yes QA/Test-Plan-Specified labels Feb 15, 2022
@tmancey
Copy link
Contributor

tmancey commented Feb 15, 2022

@jsecretan ads are not shown if the catalog has expired (older than 1 day). We fetch a catalog every ~2 hours (configurable via the ping node in catalog.json. Your thoughts on the above change? Thanks

@tmancey tmancey added QA/Yes release-notes/exclude priority/P4 Planned work. We expect to get to it "soon". and removed blocked needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. suggestion labels Mar 28, 2022
@tmancey tmancey self-assigned this Mar 28, 2022
@tmancey
Copy link
Contributor

tmancey commented Mar 28, 2022

@jsecretan a fix is in place but just want to get your thoughts before merging. Thanks

@jsecretan
Copy link

sgtm

@btlechowski
Copy link
Author

Verification passed on

Brave 1.39.87 Chromium: 101.0.4951.41 (Official Build) beta (64-bit)
Revision 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS Ubuntu 18.04 LTS

Verified catalog was overwritten
image

Verified catalog was not updated after 24h

[5205:5205:0510/105511.916912:VERBOSE1:ad_server.cc(80)] OnGetCatalog
[5205:5205:0510/105511.917059:VERBOSE1:ad_server.cc(88)] Successfully fetched catalog
[5205:5205:0510/105511.917093:VERBOSE1:ad_server.cc(90)] Parsing catalog
[5205:5205:0510/105511.935048:VERBOSE1:ad_server.cc(127)] Catalog id 10c61c2f55bbd2b7bd47b113bd2b479d747df5a6 is up to date

Verified ad was shown after 24h
image

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

Successfully merging a pull request may close this issue.

3 participants