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.

Sign up for GitHub

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

Jump to bottom

Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update #2232

Merged
merged 1 commit into from
Apr 17, 2019
Merged

Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update #2232

merged 1 commit into from
Apr 17, 2019

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Apr 16, 2019 •
edited
Loading

fixes brave/brave-browser#3967
fixes: brave/brave-browser#4131

Several steps were taken to harden the process for retrieving and reacting to whether or not the user's region is supported.

Also hardened were the preference migrations during upgrade, so Ads does not enable on upgrade if it was disabled previously.

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 && npm run test-security) 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:

Confirm Ads UI is only shown for supported regions: US, UK, Canada, France and Germany based upon the operating systems locale.

Additional test plan for upgrade here: brave/brave-browser#4131

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

@tmancey tmancey added this to the 0.65.x - Nightly milestone Apr 16, 2019
@tmancey tmancey self-assigned this Apr 16, 2019
emerick
emerick reviewed Apr 16, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

Probably should have some unit tests for this.

@ryanml ryanml self-assigned this Apr 17, 2019
@ryanml ryanml changed the title [WIP] Fix Ads enabled for unsupported regions Fix Ads enabled for unsupported regions, Fixes Ads upgrade issues Apr 17, 2019
@ryanml ryanml marked this pull request as ready for review April 17, 2019 06:57
@ryanml ryanml requested a review from emerick April 17, 2019 07:01
@tmancey tmancey force-pushed the issues/3967 branch 8 times, most recently from 11e4d16 to 74c486a Compare April 17, 2019 15:03
@tmancey tmancey changed the title Fix Ads enabled for unsupported regions, Fixes Ads upgrade issues Fix Ads enabled for unsupported regions and are enabled by default after update to 0.63.x Apr 17, 2019
@tmancey tmancey force-pushed the issues/3967 branch 2 times, most recently from 369629a to a6718ea Compare April 17, 2019 15:08
@tmancey tmancey changed the title Fix Ads enabled for unsupported regions and are enabled by default after update to 0.63.x Fix Ads enabled for unsupported regions and enabled by default after update to 0.63.x Apr 17, 2019
@tmancey tmancey force-pushed the issues/3967 branch 3 times, most recently from 45a7773 to 45b607d Compare April 17, 2019 15:41
ryanml
ryanml approved these changes Apr 17, 2019
View reviewed changes
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

Test plans work great through the changes, all lgtm

@ryanml ryanml merged commit 9a78717 into master Apr 17, 2019
@ryanml ryanml deleted the issues/3967 branch April 17, 2019 20:55
ryanml added a commit that referenced this pull request Apr 17, 2019
@ryanml
Merge pull request #2232 from /issues/3967
8c32507 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
ryanml added a commit that referenced this pull request Apr 17, 2019
@ryanml
Merge pull request #2232 from /issues/3967
bb32118 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
@ryanml ryanml mentioned this pull request Apr 17, 2019
Closed
3 tasks
tmancey pushed a commit that referenced this pull request Apr 17, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
514984d 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey pushed a commit that referenced this pull request Apr 17, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
7af4bcb 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey pushed a commit that referenced this pull request Apr 18, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
d9af978 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
# Conflicts:
#	browser/ui/webui/brave_rewards_ui.cc
#	components/brave_rewards/resources/ui/brave_rewards.tsx
#	components/brave_rewards/resources/ui/constants/rewards_types.ts
tmancey pushed a commit that referenced this pull request Apr 18, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
dd379de 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
# Conflicts:
#	browser/ui/webui/brave_rewards_ui.cc
#	components/brave_rewards/resources/ui/brave_rewards.tsx
#	components/brave_rewards/resources/ui/constants/rewards_types.ts
This was referenced Apr 18, 2019
Merged
Merged
tmancey pushed a commit that referenced this pull request Apr 18, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
86f241b 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey pushed a commit that referenced this pull request Apr 18, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
851b273 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey pushed a commit that referenced this pull request Apr 18, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
79facd3 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey pushed a commit that referenced this pull request Apr 18, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
25b94d5 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey pushed a commit that referenced this pull request Apr 19, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
3c140ca 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
# Conflicts:
#	browser/ui/webui/brave_rewards_ui.cc
#	components/brave_rewards/resources/ui/brave_rewards.tsx
#	components/brave_rewards/resources/ui/constants/rewards_types.ts
tmancey pushed a commit that referenced this pull request Apr 19, 2019
@ryanml @tmancey
Merge pull request #2232 from /issues/3967
35eefaf 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey added a commit that referenced this pull request Apr 19, 2019
@tmancey
Merge pull request #2232 from /issues/3967
70b40dd 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
tmancey added a commit that referenced this pull request Apr 19, 2019
@tmancey
Merge pull request #2232 from /issues/3967
c2d86c7 
Fix Ads enabled for unsupported regions and enabled by default after 0.63.x update
@srirambv srirambv mentioned this pull request Apr 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emerick emerick emerick approved these changes

@ryanml ryanml ryanml approved these changes

@bridiver bridiver Awaiting requested review from bridiver

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@tmancey tmancey

@ryanml ryanml

Labels
feature/ads feature/rewards
Projects
None yet
Milestone
0.65.x - Release
Development

Successfully merging this pull request may close these issues.

Ads are enabled by default after update to 0.63.x Ads enabled for unsupported regions
3 participants
@tmancey @emerick @ryanml