Anti-Targeting for Brave Ads #8167

moritzhaller · 2021-03-06T11:44:56Z

Resolves brave/brave-browser#14224

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Detailed test plan https://github.com/brave/internal/issues/767

Make sure the anti-targeting resource mkdhnfmjhklfnamlheoliekgeohamoig is added to the staging component for the respective country (matching the test machine system's settings) and the components models.json has been updated.
Make sure to have a test catalog with some anti-targeting test lists that match a creative set id from the catalog that is being triggered during test (see below for example)
Without having visited a site on the anti-targeting list, verify that the ad with the respective ID is being delivered
Now visit a site on the list and verify that the ad with the respective ID is not being delivered [87942:775:0401/172513.733280:VERBOSE2:exclusion_rule_util.h(26)] creativeSetId 5bdeab83-048f-48a7-9602-a1092ded123c excluded as user has previously visited an anti-targeting site

Example anti-targeting test lists

{
  "locale": "gb",
  "version": 1,
  "timestamp": "2021-03-32 00:00:00",
  "sites": {
    "5bdeab83-048f-48a7-9602-a1092ded123c": [
      "https://www.brave.com",
      "https://www.bravesoftware.com/"
    ],
    "ccdaf616-b8a8-481a-aa8b-caf5fb131353": [
      "https://www.bar.org"
    ]
  }
}

vendor/brave-ios/Ads/Generated/NativeAdsClient.h

kylehickinson

Stubs look good, implementation of the callback just needs fixing but other than that 👍

vendor/brave-ios/Ads/BATBraveAds.mm

kylehickinson

iOS++

tmancey · 2021-04-01T11:31:10Z

browser/brave_ads/BUILD.gn

+  # Remove when https://github.com/brave/brave-browser/issues/10639 is resolved
+  check_includes = false


Do we still need these?

this should have been removed @moritzhaller

tmancey · 2021-04-01T11:45:55Z

...ative-ads/src/bat/ads/internal/ad_pacing/ad_notifications/ad_notification_pacing_unittest.cc

@@ -94,6 +97,7 @@ class BatAdsAdNotificationPacingTest : public UnitTestBase {
  std::unique_ptr<AdTargeting> ad_targeting_;
  std::unique_ptr<ad_targeting::geographic::SubdivisionTargeting>
      subdivision_targeting_;
+  std::unique_ptr<resource::AntiTargeting> anti_targeting_;


nit: anti_targeting_resource_

tmancey · 2021-04-01T11:47:52Z

...or/bat-native-ads/src/bat/ads/internal/ad_serving/ad_notifications/ad_notification_serving.h

@@ -33,7 +38,8 @@ class AdServing {
 public:
  AdServing(
      AdTargeting* ad_targeting,
-      ad_targeting::geographic::SubdivisionTargeting* subdivision_targeting);
+      ad_targeting::geographic::SubdivisionTargeting* subdivision_targeting,
+      resource::AntiTargeting* anti_targeting);


nit: anti_targeting_resource

tmancey · 2021-04-01T11:48:20Z

...or/bat-native-ads/src/bat/ads/internal/ad_serving/ad_notifications/ad_notification_serving.h

@@ -97,6 +106,8 @@ class AdServing {

  ad_targeting::geographic::SubdivisionTargeting*
      subdivision_targeting_;  // NOT OWNED
+
+  resource::AntiTargeting* anti_targeting_;  // NOT OWNED


nit: anti_targeting_resource_

tmancey · 2021-04-01T11:52:01Z

...ads/src/bat/ads/internal/ad_targeting/resources/frequency_capping/anti_targeting_resource.cc

+          return;
+        }
+
+        BLOG(1, "Successfully loaded resource " << kResourceId);


nit: Successfully loaded anti-targeting resource ... (same for other logs, check if console logs contain filename and if so we then do not need the verbosity)

file name shows in log, so keeping as is for now

tmancey · 2021-04-01T11:58:33Z

...e-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/anti_targeting_frequency_cap.cc

+
+  AntiTargetingInfo anti_targeting = anti_targeting_->get();
+  const auto iter = anti_targeting.sites.find(ad.creative_set_id);
+  // Always respect if creative set has no anti-targeting sites


nit: move to inside if statement

tmancey · 2021-04-01T11:59:42Z

vendor/brave-ios/Ads/BATBraveAds.mm

+- (void)getBrowsingHistory:(const int)max_count
+                   forDays:(const int)days_ago
+                  callback:(ads::GetBrowsingHistoryCallback)callback {
+  // Not implemented for iOS


nit: 'To be implementedwith a link to abrave-ios` issue so that the work can be prioritized

tmancey

LGTM++

github-actions bot assigned moritzhaller Mar 6, 2021

moritzhaller force-pushed the issues/14224 branch 5 times, most recently from 29b7e60 to 2cef13c Compare March 25, 2021 13:29

moritzhaller changed the title ~~[WIP] Anti-Targeting~~ Anti-Targeting for Brave Ads Mar 25, 2021

moritzhaller force-pushed the issues/14224 branch from 2cef13c to a98c68a Compare March 25, 2021 13:51

moritzhaller marked this pull request as ready for review March 25, 2021 13:52

moritzhaller requested review from kylehickinson and tmancey as code owners March 25, 2021 13:52

moritzhaller added this to the 1.24.x - Nightly milestone Mar 25, 2021

kylehickinson requested changes Mar 25, 2021

View reviewed changes

vendor/brave-ios/Ads/Generated/NativeAdsClient.h Outdated Show resolved Hide resolved

moritzhaller force-pushed the issues/14224 branch 2 times, most recently from e1a66fe to f099e14 Compare March 25, 2021 14:47

moritzhaller requested a review from kylehickinson March 26, 2021 09:48

moritzhaller force-pushed the issues/14224 branch 2 times, most recently from a2c15fd to bff6d16 Compare March 26, 2021 12:46

kylehickinson requested changes Mar 26, 2021

View reviewed changes

vendor/brave-ios/Ads/BATBraveAds.mm Outdated Show resolved Hide resolved

moritzhaller requested a review from kylehickinson March 31, 2021 07:13

moritzhaller force-pushed the issues/14224 branch 3 times, most recently from 08a7e7f to e32906e Compare March 31, 2021 08:42

kylehickinson approved these changes Mar 31, 2021

View reviewed changes

moritzhaller force-pushed the issues/14224 branch 4 times, most recently from 8031132 to ca67885 Compare April 1, 2021 08:00

tmancey reviewed Apr 1, 2021

View reviewed changes

tmancey approved these changes Apr 1, 2021

View reviewed changes

moritzhaller force-pushed the issues/14224 branch from ca67885 to 564a3e5 Compare April 1, 2021 16:02

Add anti-targeting frequency capping for Brave Ads

75e7d7f

moritzhaller force-pushed the issues/14224 branch from 564a3e5 to 75e7d7f Compare April 1, 2021 16:48

moritzhaller merged commit 21f9100 into master Apr 1, 2021

moritzhaller deleted the issues/14224 branch April 1, 2021 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anti-Targeting for Brave Ads #8167

Anti-Targeting for Brave Ads #8167

moritzhaller commented Mar 6, 2021 •

edited by btlechowski

Loading

kylehickinson left a comment

kylehickinson left a comment

tmancey Apr 1, 2021

iefremov Jun 9, 2021

tmancey Apr 1, 2021

tmancey Apr 1, 2021

tmancey Apr 1, 2021

tmancey Apr 1, 2021 •

edited

Loading

moritzhaller Apr 1, 2021

tmancey Apr 1, 2021

tmancey Apr 1, 2021

tmancey left a comment

		# Remove when https://github.com/brave/brave-browser/issues/10639 is resolved
		check_includes = false

Anti-Targeting for Brave Ads #8167

Anti-Targeting for Brave Ads #8167

Conversation

moritzhaller commented Mar 6, 2021 • edited by btlechowski Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

kylehickinson left a comment

Choose a reason for hiding this comment

kylehickinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey left a comment

Choose a reason for hiding this comment

moritzhaller commented Mar 6, 2021 •

edited by btlechowski

Loading

tmancey Apr 1, 2021 •

edited

Loading