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

Remove region support for Ads as CDN now controls catalog for each region #1983

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Mar 16, 2019

fixes brave/brave-browser#3752

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Test that Ads are shown by using a VPN to set your location to one of the supported regions. And that Ads are not shown when setting your location to one of the unsupported regions.

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

@NejcZdovc
Copy link
Contributor

@tmancey how can regular Joe like me from Slovenia test ads if local will be determinate via CDN and ads are not available here? The only thing that I can think of is VPN.

@tmancey
Copy link
Collaborator Author

tmancey commented Mar 17, 2019

@NejcZdovc Only way is using a VPN

@srirambv
Copy link
Contributor

I am not seeing any ads on Linux when device region is set to Canada, and using a VPN(US node) still doesn't seem to trigger ads.
image
Console shows
image
Catalog shows region as US
image

@mbacchi mentioned in slack that he was able to see ad on Linux however when I try with/without VPN I am still not able to see any ads on Linux. When spoke to @tmancey on call he mentioned this PR fixes the issue of region and not seeing ads. @jsecretan we should probably get this reviewed soon and have it uplifted to dev.

bridiver
bridiver previously approved these changes Mar 29, 2019
@tmancey tmancey requested a review from bridiver April 1, 2019 14:18
@tmancey tmancey merged commit 2423a10 into master Apr 2, 2019
@tmancey tmancey deleted the issues/3752 branch April 2, 2019 00:08
fmarier pushed a commit that referenced this pull request Oct 29, 2019
Enable hangouts extension to allow screen share
petemill pushed a commit that referenced this pull request Jul 27, 2020
Enable hangouts extension to allow screen share
petemill pushed a commit that referenced this pull request Jul 28, 2020
Enable hangouts extension to allow screen share
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.

Remove region support for Ads as CDN now controls catalog for each region
4 participants