[ads] Conversions should match up to the set cap, not one less

brave · Sep 6, 2024 · cf07fb9 · cf07fb9
1 parent 5388ecf
commit cf07fb9
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 13 deletions.
diff --git a/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_util.cc b/components/brave_ads/core/internal/creatives/conversions/creative_set_conversion_util.cc
@@ -69,7 +69,7 @@ void FilterCreativeSetConversionBucketsThatExceedTheCap(
 
   for (const auto& [creative_set_id, creative_set_conversion_count] :
        creative_set_conversion_counts) {
-    if (creative_set_conversion_count >= creative_set_conversion_cap) {
+    if (creative_set_conversion_count > creative_set_conversion_cap) {
       creative_set_conversion_buckets.erase(creative_set_id);
     }
   }

diff --git a/...ts/brave_ads/core/internal/creatives/conversions/creative_set_conversion_util_unittest.cc b/...ts/brave_ads/core/internal/creatives/conversions/creative_set_conversion_util_unittest.cc
@@ -170,6 +170,7 @@ TEST_F(BraveAdsCreativeSetConversionUtilTest,
           /*url_pattern=*/"https://foo.com/*",
           /*observation_window=*/base::Days(3));  // Bucket #1
   creative_set_conversions.push_back(creative_set_conversion_1);
+  creative_set_conversions.push_back(creative_set_conversion_1);
 
   const CreativeSetConversionInfo creative_set_conversion_2 =
       test::BuildCreativeSetConversion(
@@ -178,9 +179,10 @@ TEST_F(BraveAdsCreativeSetConversionUtilTest,
           /*observation_window=*/base::Days(30));  // Bucket #1
   creative_set_conversions.push_back(creative_set_conversion_2);
   creative_set_conversions.push_back(creative_set_conversion_2);
+  creative_set_conversions.push_back(creative_set_conversion_2);
 
   const CreativeSetConversionCountMap creative_set_conversion_counts = {
-      {creative_set_conversion_1.id, 1}, {creative_set_conversion_2.id, 2}};
+      {creative_set_conversion_1.id, 2}, {creative_set_conversion_2.id, 3}};
 
   CreativeSetConversionBucketMap creative_set_conversion_buckets =
       SortCreativeSetConversionsIntoBuckets(creative_set_conversions);
@@ -193,7 +195,8 @@ TEST_F(BraveAdsCreativeSetConversionUtilTest,
   // Assert
   const CreativeSetConversionBucketMap
       expected_creative_set_conversion_buckets = {
-          {creative_set_conversion_1.id, {creative_set_conversion_1}}};
+          {creative_set_conversion_1.id,
+           {creative_set_conversion_1, creative_set_conversion_1}}};
   EXPECT_EQ(expected_creative_set_conversion_buckets,
             creative_set_conversion_buckets);
 }
@@ -209,17 +212,19 @@ TEST_F(BraveAdsCreativeSetConversionUtilTest,
           /*url_pattern=*/"https://foo.com/*",
           /*observation_window=*/base::Days(3));  // Bucket #1
   creative_set_conversions.push_back(creative_set_conversion_1);
+  creative_set_conversions.push_back(creative_set_conversion_1);
 
   const CreativeSetConversionInfo creative_set_conversion_2 =
       test::BuildCreativeSetConversion(
           /*creative_set_id=*/"4e83a23c-1194-40f8-8fdc-2f38d7ed75c8",
           /*url_pattern=*/"https://baz.com/",
-          /*observation_window=*/base::Days(30));  // Bucket #1
+          /*observation_window=*/base::Days(30));  // Bucket #2
+  creative_set_conversions.push_back(creative_set_conversion_2);
   creative_set_conversions.push_back(creative_set_conversion_2);
   creative_set_conversions.push_back(creative_set_conversion_2);
 
   const CreativeSetConversionCountMap creative_set_conversion_counts = {
-      {creative_set_conversion_1.id, 1}, {creative_set_conversion_2.id, 2}};
+      {creative_set_conversion_1.id, 2}, {creative_set_conversion_2.id, 3}};
 
   CreativeSetConversionBucketMap creative_set_conversion_buckets =
       SortCreativeSetConversionsIntoBuckets(creative_set_conversions);
@@ -232,9 +237,11 @@ TEST_F(BraveAdsCreativeSetConversionUtilTest,
   // Assert
   const CreativeSetConversionBucketMap
       expected_creative_set_conversion_buckets = {
-          {creative_set_conversion_1.id, {creative_set_conversion_1}},
+          {creative_set_conversion_1.id,
+           {creative_set_conversion_1, creative_set_conversion_1}},
           {creative_set_conversion_2.id,
-           {creative_set_conversion_2, creative_set_conversion_2}}};
+           {creative_set_conversion_2, creative_set_conversion_2,
+            creative_set_conversion_2}}};
   EXPECT_EQ(expected_creative_set_conversion_buckets,
             creative_set_conversion_buckets);
 }

diff --git a/components/brave_ads/core/internal/user_engagement/conversions/conversions.cc b/components/brave_ads/core/internal/user_engagement/conversions/conversions.cc
@@ -152,6 +152,13 @@ void Conversions::CheckForConversions(
     }
     const auto& [creative_set_id, creative_set_conversion_bucket] = *iter;
 
+    // Have we exceeded the limit for creative set conversions?
+    if (creative_set_conversion_cap > 0 &&
+        creative_set_conversion_counts[creative_set_id] ==
+            creative_set_conversion_cap) {
+      continue;
+    }
+
     // Yes, so are we allowed to convert this ad event?
     if (!IsAllowedToConvertAdEvent(ad_event)) {
       // No, so skip this ad event.
@@ -178,13 +185,8 @@ void Conversions::CheckForConversions(
       did_convert = true;
 
       // Have we exceeded the limit for creative set conversions?
-      if (creative_set_conversion_cap == 0) {
-        // There is no limit, so continue converting.
-        continue;
-      }
-
       ++creative_set_conversion_counts[creative_set_id];
-      if (creative_set_conversion_counts[creative_set_id] >=
+      if (creative_set_conversion_counts[creative_set_id] ==
           creative_set_conversion_cap) {
         // Yes, so stop converting.
         break;