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

Implement state level ads delivery #5602

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Implement state level ads delivery #5602

merged 1 commit into from
Jun 11, 2020

Conversation

moritzhaller
Copy link
Contributor

@moritzhaller moritzhaller commented May 20, 2020 •
edited by tmancey
Loading

Resolves brave/brave-browser#9200

Submitter Checklist:

Test Plan:

  • verify country subdivision fetches only when locale is US (irrespective of VPN setting)
  • verify country subdivision retries with exponential backoff if server request fails
  • verify country subdivision fetches periodically (in debug ~2 mins other ~24 hours)
  • verify subdivision is not fetched if state-level targeting is disabled
  • verify that subdivision is not updated after the user manually sets the State
  • verify that subdivision targeting is auto-detected by default on a fresh install

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.

@moritzhaller moritzhaller self-assigned this May 20, 2020
@moritzhaller moritzhaller force-pushed the issues/9200 branch 3 times, most recently from 6d4d418 to 6a4e169 Compare May 29, 2020 15:10
@moritzhaller moritzhaller marked this pull request as ready for review May 29, 2020 15:14
@moritzhaller moritzhaller requested a review from NejcZdovc as a code owner May 29, 2020 15:14
tmancey
tmancey requested changes May 29, 2020
View reviewed changes
components/services/bat_ads/bat_ads_client_mojo_bridge.cc Outdated Show resolved Hide resolved
components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc Outdated Show resolved Hide resolved
components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc Outdated Show resolved Hide resolved
components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc Outdated Show resolved Hide resolved
components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc Outdated Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/country_subdivision.cc Outdated Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/country_subdivision.h Outdated Show resolved Hide resolved
.../src/bat/ads/internal/frequency_capping/exclusion_rules/country_subdivision_frequency_cap.cc Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.h Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
kylehickinson
kylehickinson requested changes May 29, 2020
View reviewed changes
vendor/brave-ios/Ads/Generated/NativeAdsClient.mm Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.h Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved
@moritzhaller moritzhaller force-pushed the issues/9200 branch 2 times, most recently from a3ec561 to 38d3c38 Compare June 1, 2020 11:07
@moritzhaller moritzhaller requested a review from tmancey June 1, 2020 11:09
@moritzhaller moritzhaller force-pushed the issues/9200 branch 2 times, most recently from 311c519 to b69174c Compare June 1, 2020 15:18
fmarier
fmarier reviewed Jun 2, 2020
View reviewed changes
Copy link
Member

@fmarier fmarier left a comment

Choose a reason for hiding this comment

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

I didn't see a filter to prevent WY targeting. Is that because the filter is actually on the Ads Platform side? (i.e. we don't create campaigns with WY-level targeting)

Also, are the two state-selection UI preferences visible to users outside of the USA?

vendor/bat-native-ads/src/bat/ads/internal/country_subdivision.h Outdated Show resolved Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/static_values.h Outdated Show resolved Hide resolved
@moritzhaller moritzhaller mentioned this pull request Jun 3, 2020
Closed
@tmancey tmancey force-pushed the issues/9200 branch 5 times, most recently from 228132f to 59e6ba7 Compare June 10, 2020 10:32
@tmancey tmancey changed the title Implement US state level targeting Implement state level ads delivery Jun 10, 2020
@tmancey tmancey force-pushed the issues/9200 branch 3 times, most recently from a5dfb29 to 9814678 Compare June 10, 2020 16:14
tmancey
tmancey approved these changes Jun 10, 2020
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

Approving for Moritz for my work and for me for Moritiz work

@tmancey tmancey force-pushed the issues/9200 branch 11 times, most recently from 7fa920d to 22fed70 Compare June 11, 2020 11:00
NejcZdovc
NejcZdovc approved these changes Jun 11, 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

yachtcaptain23
yachtcaptain23 reviewed Jun 11, 2020
View reviewed changes
components/brave_rewards/resources/page/components/adsBox.tsx
@@ -66,6 +66,82 @@ class AdsBox extends React.Component<Props, State> {
)
}

getAdsSubdivisions = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway we can merge this the getAdsSubdivisions() method in components/brave_rewards/resources/android_page/components/adsBox.tsx?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have just seen this, and in a future update we will move the list from adsBox to the ads lib. Thanks

@tmancey tmancey force-pushed the issues/9200 branch 6 times, most recently from d60e7f5 to d2a91f5 Compare June 11, 2020 17:31
@tmancey tmancey merged commit ed2e0e6 into master Jun 11, 2020
@tmancey tmancey deleted the issues/9200 branch June 11, 2020 22:55
@tmancey tmancey added this to the 1.12.x - Nightly milestone Jun 11, 2020
@tmancey tmancey mentioned this pull request Jun 15, 2020
Closed
4 tasks
@LaurenWags LaurenWags mentioned this pull request Aug 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yachtcaptain23 yachtcaptain23 yachtcaptain23 left review comments

@tmancey tmancey tmancey approved these changes

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@fmarier fmarier fmarier approved these changes

Assignees

@moritzhaller moritzhaller

Labels
feature/ads
Projects
None yet
Milestone
1.12.x - Release #1
Development

Successfully merging this pull request may close these issues.

State level ads delivery
6 participants
@moritzhaller @fmarier @kylehickinson @tmancey @yachtcaptain23 @NejcZdovc