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

Decouple business logic for newly supported regions into ads library #6612

Closed
tmancey opened this issue Oct 25, 2019 · 3 comments · Fixed by brave/brave-core#3805
Closed
Assignees
Labels
ARM Android ARM related issues feature/ads OS/Android Fixes related to Android browser functionality priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/exclude

Comments

@tmancey
Copy link
Contributor

tmancey commented Oct 25, 2019

Add the ability to detect if a users locale is newly supported, i.e. was not supported in their previous version of the browser, i.e.

if (ads_service_->IsNewlySupportedLocale()) {
  DoSomething();
}

Refactor kSupportedRegions in static_values.h to be version driven, i.e.

const std::map<int, std::map<std::string, bool>> kSupportedRegions = {
  // schema_version : {{region, targeted}}
  0 : {
    // We default to schema version 0 so that all regions are considered new if
    // upgrading from a version of the browser which does not support Brave ads
  },

  1 : {
    { "US", true  },  // United States of America
    { "CA", true  },  // Canada
    { "GB", true  },  // United Kingdom (Great Britain and Northern Ireland)
    { "DE", true  },  // Germany
    { "FR", true  }   // France
  },

  2 : {
    { "AU", true  },  // Australia
    { "NZ", true  },  // New Zealand
    { "IE", true  }   // Ireland
  },

  3 : {
    { "AR", false },  // Argentina
    { "AT", false },  // Austria
    { "BR", false },  // Brazil
    { "CH", false },  // Switzerland
    { "CL", false },  // Chile
    { "CO", false },  // Colombia
    { "DK", false },  // Denmark
    { "EC", false },  // Ecuador
    { "IL", false },  // Israel
    { "IN", false },  // India
    { "IT", false },  // Italy
    { "JP", false },  // Japan
    { "KR", false },  // Korea
    { "MX", false },  // Mexico
    { "NL", false },  // Netherlands
    { "PE", false },  // Peru
    { "PH", false },  // Philippines
    { "PL", false },  // Poland
    { "SE", false },  // Sweden
    { "SG", false },  // Singapore
    { "VE", false },  // Venezuela
    { "ZA", false }   // South Africa
  },

  4 : {
    { "KY", true  }   // Cayman Islands
  }
};
@tmancey tmancey added feature/ads OS/Android Fixes related to Android browser functionality labels Oct 25, 2019
@tmancey
Copy link
Contributor Author

tmancey commented Oct 25, 2019

@SergeyZhukovsky @anthonypkeane It is important this brave-core ads library improvement is prioritised before the release of Android 1.5 so that all business logic for newly supported regions is platform agnostic

@tmancey
Copy link
Contributor Author

tmancey commented Nov 27, 2019

@btlechowski @GeetaSarvadnya Users upgrading from an older build to a build with this fix that has ads switched off will see the Brave ads have arrived! on-boarding. Please take this into account when testing. I discussed with @jsecretan as this is due to transitioning to region versioning

@LaurenWags
Copy link
Member

LaurenWags commented Jan 2, 2020

Verified passed with

Brave 1.2.40 Chromium: 79.0.3945.88 (Official Build) (64-bit)
Revision c2a58a36b9411c80829b4b154bfcab97e581f1f3-refs/branch-heads/3945@{#954}
OS macOS Version 10.14.6 (Build 18G103)
  • Verified test plan from Decouple business logic for newly supported regions into ads library brave-core#3805 (except Android testing)

  • Clean install of 1.2.40 using country from each of the kSupportedRegions list sections 1 (US), 2 (Australia), and 3 (Brazil).

    • Confirmed enabling Rewards enabled Ads as expected.
    • Confirmed Ad notifications were shown as expected.
    • Confirmed Ads panel on brave://rewards updated as expected.
    • Confirmed Ads could be disabled and no ad messages were shown in logs. (Also, ads_service folder was removed from ~/Library/Application Support/BraveSoftware/Brave-Browser/Default)
    • Confirmed Ads could be re-enabled and ad notifications were shown again.
    • note, for section 3 countries attempted (Sweden, Poland, Brazil) catalogs were empty. However, I could see that it was trying to find an ad to show as Notification not made: No eligible ads found was shown in the logs.
  • Also clean install of 1.2.40 for non-Ads country (Bulgaria) and enabling Rewards showed the "Sorry" message on Ads panel and NTP widget as expected.

  • Upgrade test 1 - Ads OFF prior to upgrade:

  • Upgrade test 2 - Ads ON prior to upgrade:

    • Install 1.1.23 using US, Australia, or Brazil as above.
    • Enable Rewards.
    • Upgrade to 1.2.40.
    • Confirmed Ads are still ON as expected.
    • Confirmed still able to view Ad notifications. (Except for Brazil, see note above about empty catalog).

Verification passed on

Brave 1.2.40 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)
  • Verified the test plan from Decouple business logic for newly supported regions into ads library brave-core#3805 (except Android)

  • Clean install of 1.2.40 using country from each of the supportedRegions list sections 1 (US), 2 (Australia), and 3 (Brazil).

    • Confirmed enabling Rewards enabled Ads as expected.
    • Confirmed Ad notifications were shown as expected.
    • Confirmed Ads panel on brave://rewards updated as expected.
    • Confirmed Ads could be disabled and no ad messages were shown in logs.
    • Confirmed Ads could be re-enabled and ad notifications were shown again.
      (While testing found that the countries listed in the array(3) Japan, Netherlands and India catalogs ae empty and unable to view ads due to this)
  • Upgrade test 1 - Ads OFF prior to upgrade:

  • Upgrade test 2 - Ads ON prior to upgrade:

    • Install 1.1.23 using US, Australia, or Brazil as above.
    • Enable Rewards.
    • Upgrade to 1.2.40.
    • Confirmed Ads are still ON as expected.
    • Confirmed still able to view Ad notifications. (Except for Brazil, see note above about empty catalog).

Verification passed on

Brave 1.2.40 Chromium: 79.0.3945.88 (Official Build) (64-bit)
Revision c2a58a36b9411c80829b4b154bfcab97e581f1f3-refs/branch-heads/3945@{#954}
OS Ubuntu 18.04 LTS
  • Verified the test plan from Decouple business logic for newly supported regions into ads library brave-core#3805 (except Android)

  • Upgrade test 1 - Ads OFF prior to upgrade:

  • Upgrade test 2 - Ads ON prior to upgrade:

    • Install 1.1.23 using US, Australia, or Brazil as above.
    • Enable Rewards.
    • Upgrade to 1.2.40.
    • Confirmed Ads are still ON as expected.
    • Confirmed still able to view Ad notifications. (Except for Brazil, see note above about empty catalog).
  • Clean install of 1.2.40 using country from each of the supportedRegions list sections 1 (US), 2 (Australia), and 3 (Brazil).

    • Confirmed enabling Rewards enabled Ads as expected.
    • Confirmed Ad notifications were shown as expected.
    • Confirmed Ads panel on brave://rewards updated as expected.
    • Confirmed Ads could be disabled and no ad messages were shown in logs.
    • Confirmed Ads could be re-enabled and ad notifications were shown again.
      (While testing found that the countries listed in the array(3) Japan, Netherlands and India catalogs ae empty and unable to view ads due to this)

@LaurenWags LaurenWags added the ARM Android ARM related issues label Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARM Android ARM related issues feature/ads OS/Android Fixes related to Android browser functionality priority/P2 A bad problem. We might uplift this to the next planned release. QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants