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

Fixes ads enabled by default on upgrade of 1.2 to 1.3 for new ad regions #4392

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Fixes ads enabled by default on upgrade of 1.2 to 1.3 for new ad regions #4392

merged 1 commit into from
Jan 23, 2020

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Jan 19, 2020 •
edited
Loading

Fixes brave/brave-browser#7741

Submitter Checklist:

Test Plan:

Recommend tests:

Following upgrade paths from 0.63, 0.67, 0.68, 0.69, 0.70, 0.71, 0.72, 1.0, 1.1 and 1.2

  • Test upgrade path for unsupported locale (on above versions) to supported locale on 1.3.x
  • Test upgrade path for unsupported locale (on above versions) to unsupported locale on 1.3.x
  • Test upgrade path for supported locale (on above versions, with Brave Ads enabled) to supported locale on 1.3.x

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

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey tmancey added this to the 1.5.x - Nightly milestone Jan 19, 2020
@tmancey tmancey requested a review from gdregalo as a code owner January 19, 2020 11:35
@tmancey tmancey self-assigned this Jan 19, 2020
tmancey
tmancey commented Jan 19, 2020
View reviewed changes
browser/extensions/api/brave_rewards_api.cc
@@ -605,7 +605,9 @@ ExtensionFunction::ResponseAction BraveRewardsSaveAdsSettingFunction::Run() {
AdsService* ads_service_ = AdsServiceFactory::GetForProfile(profile);
if (ads_service_) {
if (params->key == "adsEnabled") {
ads_service_->SetEnabled(params->value == "true");
const auto is_enabled =
params->value == "true" && ads_service_->IsSupportedLocale();
Copy link
Collaborator Author

@tmancey tmancey Jan 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The bug was caused by this line, due to a legacy issue where the ads flag was enabled even if the user's locale was unsupported

@tmancey tmancey force-pushed the issues/7741 branch 2 times, most recently from 97401ff to e80c4dd Compare January 19, 2020 12:05
This was referenced Jan 19, 2020
Merged
Merged
@tmancey
Copy link
Collaborator Author

tmancey commented Jan 20, 2020

@NejcZdovc @gdregalo Unrelated CI failure

masparrow
masparrow approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@masparrow masparrow left a comment

Choose a reason for hiding this comment

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

LGTM

NejcZdovc
NejcZdovc approved these changes Jan 21, 2020
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

rewards code looks good

@bsclifton
Copy link
Member

Helping merge since code owner unavailable. CI looks good- it's yellow (acceptable - ran into problem on network audit + test install. there are known issues). Good to merge if otherwise approved 👍

@bsclifton bsclifton merged commit 6dbf4db into master Jan 23, 2020
@bsclifton bsclifton deleted the issues/7741 branch January 23, 2020 16:46
@LaurenWags
Copy link
Member

Verified using

Brave 1.5.57 Chromium: 79.0.3945.130 (Official Build) nightly (64-bit)
Revision e22de67c28798d98833a7137c0e22876237fc40a-refs/branch-heads/3945@{#1047}
OS macOS Version 10.14.6 (Build 18G103)
  • Verified test plan from description:

  1. Upgrade path for unsupported locale to supported locale

    • FAIL When upgrading from 0.63.55 or 0.64.77 I encounter Upgrade from 0.63.x to 1.3.x resets Ads viewed count to zero, AC switch OFF and wallet creation failed brave-browser#7833 (comment)
    • PASS Installed 0.65.121 for Spain. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • PASS Installed 0.66.101 for Spain. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region. Enabled Ads and confirmed I received an ad.
    • PASS Installed 0.67.125 for Norway. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • PASS Installed 0.68.142 for Norway. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • PASS Installed 0.69.135 for Norway. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • PASS Installed 0.70.123 for Norway. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • PASS Installed 1.0.1 for Russia. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • PASS Installed 1.1.23 for Russia. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.
    • Installed 1.2.43 for Russia. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: ads were not enabled by default and there was a BAT logo notification notifying the user that Brave Ads are now available in their region.

  2. Upgrade path for unsupported locale to unsupported locale

    • FAIL When upgrading from 0.63.55 or 0.64.77 for Aruba I encounter Upgrade from 0.63.x to 1.3.x resets Ads viewed count to zero, AC switch OFF and wallet creation failed brave-browser#7833 (comment)
    • PASS Installed 0.65.121 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 0.66.101 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 0.67.125 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 0.68.142 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 0.69.135 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 0.70.123 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 1.0.1 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 1.1.23 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.
    • PASS Installed 1.2.43 for Aruba. Enabled Rewards. Confirmed “Sorry” message on Ads panel. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: still see "Sorry" message and there is no BAT logo encouraging user to try Ads.

  3. Upgrade path for supported locale (with Ads enabled) to supported locale

    • FAIL When upgrading from 0.63.55 or 0.64.77 for US I encounter Upgrade from 0.63.x to 1.3.x resets Ads viewed count to zero, AC switch OFF and wallet creation failed brave-browser#7833 (comment)
    • PASS Installed 0.65.121 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 0.66.101 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 0.67.125 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 0.68.142 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 0.69.135 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 0.70.123 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 1.0.1 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 1.1.23 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.
    • PASS Installed 1.2.43 for US. Enabled Rewards and viewed an Ad. Upgraded to 1.5.x (renamed profile). After upgrade worked as expected: Ads still on after upgrade, Ads panel displays expected data, can still receive ad notifications.

@kjozwiak kjozwiak mentioned this pull request Jan 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masparrow masparrow masparrow approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@emerick emerick Awaiting requested review from emerick

@gdregalo gdregalo Awaiting requested review from gdregalo

Assignees

@tmancey tmancey

Labels
bug feature/ads
Projects
None yet
Milestone
1.5.x - Release
Development

Successfully merging this pull request may close these issues.

Ads enabled by default on upgrade for new ad regions
5 participants
@tmancey @bsclifton @LaurenWags @NejcZdovc @masparrow