From 73116ec9b9f14bc90da56366697ad281fa2d0a24 Mon Sep 17 00:00:00 2001 From: Aleksey Seren Date: Wed, 9 Feb 2022 20:48:20 +0700 Subject: [PATCH] Do not frequency cap on new tab page view event. fix https://github.com/brave/brave-browser/issues/14015 --- .../new_tab_page_ad_serving.cc | 8 +- .../new_tab_page_ad_serving_test.cc | 156 +++++++++++++++++- .../ads/new_tab_page_ads/new_tab_page_ad.cc | 20 --- .../new_tab_page_ad_unittest.cc | 121 +------------- 4 files changed, 160 insertions(+), 145 deletions(-) diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving.cc b/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving.cc index db76bf9e63e4..1b8849656b25 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving.cc @@ -112,8 +112,6 @@ bool AdServing::ServeAd(const NewTabPageAdInfo& ad, GetNewTabPageAdCallback callback) const { DCHECK(ad.IsValid()); - // TODO(https://github.com/brave/brave-browser/issues/14015): Add logging for - // wallpapers BLOG(1, "Serving new tab page ad:\n" << " uuid: " << ad.uuid << "\n" << " creativeInstanceId: " << ad.creative_instance_id << "\n" @@ -124,7 +122,11 @@ bool AdServing::ServeAd(const NewTabPageAdInfo& ad, << " companyName: " << ad.company_name << "\n" << " imageUrl: " << ad.image_url << "\n" << " alt: " << ad.alt << "\n" - << " targetUrl: " << ad.target_url); + << " targetUrl: " << ad.target_url << "\n" + << " wallpaperImageUrl: " << ad.wallpapers[0].image_url << "\n" + << " wallpaperFocalPointX: " << ad.wallpapers[0].focal_point.x + << "\n" + << " wallpaperFocalPointY: " << ad.wallpapers[0].focal_point.y); callback(/* success */ true, ad); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving_test.cc b/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving_test.cc index 2ee5671a599a..a695bb5f4bd6 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving_test.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving_test.cc @@ -5,13 +5,17 @@ #include "bat/ads/internal/ad_serving/new_tab_page_ads/new_tab_page_ad_serving.h" +#include "bat/ads/internal/ad_events/ad_event_unittest_util.h" +#include "bat/ads/internal/ad_serving/ad_serving_features.h" #include "bat/ads/internal/ad_serving/ad_targeting/geographic/subdivision/subdivision_targeting.h" #include "bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_builder.h" +#include "bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_permission_rules_unittest_util.h" #include "bat/ads/internal/bundle/creative_new_tab_page_ad_unittest_util.h" #include "bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.h" #include "bat/ads/internal/frequency_capping/permission_rules/user_activity_permission_rule_unittest_util.h" #include "bat/ads/internal/resources/frequency_capping/anti_targeting/anti_targeting_resource.h" #include "bat/ads/internal/unittest_base.h" +#include "bat/ads/internal/unittest_time_util.h" #include "bat/ads/internal/unittest_util.h" #include "bat/ads/new_tab_page_ad_info.h" #include "net/http/http_status_code.h" @@ -97,10 +101,10 @@ class BatAdsNewTabPageAdServingTest : public UnitTestBase { TEST_F(BatAdsNewTabPageAdServingTest, ServeAd) { // Arrange - ForceUserActivityFrequencyCapPermission(); + new_tab_page_ads::frequency_capping::ForcePermissionRules(); CreativeNewTabPageAdList creative_ads; - const CreativeNewTabPageAdInfo& creative_ad = BuildCreativeNewTabPageAd(); + CreativeNewTabPageAdInfo creative_ad = BuildCreativeNewTabPageAd(); creative_ads.push_back(creative_ad); Save(creative_ads); @@ -118,6 +122,30 @@ TEST_F(BatAdsNewTabPageAdServingTest, ServeAd) { // Assert } +TEST_F(BatAdsNewTabPageAdServingTest, + DoNotServeAdIfExceededPerDayCapFromCatalog) { + // Arrange + new_tab_page_ads::frequency_capping::ForcePermissionRules(); + + CreativeNewTabPageAdList creative_ads; + CreativeNewTabPageAdInfo creative_ad = BuildCreativeNewTabPageAd(); + creative_ads.push_back(creative_ad); + Save(creative_ads); + + AdEventInfo ad_event = BuildAdEvent(creative_ad, AdType::kNewTabPageAd, + ConfirmationType::kServed, Now()); + for (int i = 0; i < creative_ad.per_day; ++i) { + FireAdEvent(ad_event); + } + + // Act + ad_serving_->MaybeServeAd([](const bool success, const NewTabPageAdInfo& ad) { + EXPECT_FALSE(success); + }); + + // Assert +} + TEST_F(BatAdsNewTabPageAdServingTest, DoNotServeAdIfNotAllowedDueToPermissionRules) { // Arrange @@ -134,4 +162,128 @@ TEST_F(BatAdsNewTabPageAdServingTest, // Assert } +TEST_F(BatAdsNewTabPageAdServingTest, ServeAdIfNotExceededAdsPerHourCap) { + // Arrange + new_tab_page_ads::frequency_capping::ForcePermissionRules(); + + CreativeNewTabPageAdList creative_ads; + CreativeNewTabPageAdInfo creative_ad1 = BuildCreativeNewTabPageAd(); + CreativeNewTabPageAdInfo creative_ad2 = BuildCreativeNewTabPageAd(); + creative_ads.push_back(creative_ad1); + creative_ads.push_back(creative_ad2); + Save(creative_ads); + + AdEventInfo ad_event1 = BuildAdEvent(creative_ad1, AdType::kNewTabPageAd, + ConfirmationType::kServed, Now()); + + const int ads_per_hour = features::GetMaximumNewTabPageAdsPerHour(); + for (int i = 0; i < ads_per_hour - 1; ++i) { + FireAdEvent(ad_event1); + } + + // Act + ad_serving_->MaybeServeAd( + [&creative_ad2](const bool success, const NewTabPageAdInfo& ad) { + ASSERT_TRUE(success); + + NewTabPageAdInfo expected_ad = BuildNewTabPageAd(creative_ad2); + expected_ad.uuid = ad.uuid; + + EXPECT_EQ(expected_ad, ad); + }); + + // Assert +} + +TEST_F(BatAdsNewTabPageAdServingTest, DoNotServeAdIfExceededAdsPerHourCap) { + // Arrange + new_tab_page_ads::frequency_capping::ForcePermissionRules(); + + CreativeNewTabPageAdList creative_ads; + CreativeNewTabPageAdInfo creative_ad1 = BuildCreativeNewTabPageAd(); + CreativeNewTabPageAdInfo creative_ad2 = BuildCreativeNewTabPageAd(); + creative_ads.push_back(creative_ad1); + creative_ads.push_back(creative_ad2); + Save(creative_ads); + + AdEventInfo ad_event1 = BuildAdEvent(creative_ad1, AdType::kNewTabPageAd, + ConfirmationType::kServed, Now()); + + const int ads_per_hour = features::GetMaximumNewTabPageAdsPerHour(); + for (int i = 0; i < ads_per_hour; ++i) { + FireAdEvent(ad_event1); + } + + // Act + ad_serving_->MaybeServeAd([](const bool success, const NewTabPageAdInfo& ad) { + EXPECT_FALSE(success); + }); + + // Assert +} + +TEST_F(BatAdsNewTabPageAdServingTest, ServeAdIfNotExceededAdsPerDayCap) { + // Arrange + new_tab_page_ads::frequency_capping::ForcePermissionRules(); + + CreativeNewTabPageAdList creative_ads; + CreativeNewTabPageAdInfo creative_ad1 = BuildCreativeNewTabPageAd(); + CreativeNewTabPageAdInfo creative_ad2 = BuildCreativeNewTabPageAd(); + creative_ads.push_back(creative_ad1); + creative_ads.push_back(creative_ad2); + Save(creative_ads); + + AdEventInfo ad_event1 = BuildAdEvent(creative_ad1, AdType::kNewTabPageAd, + ConfirmationType::kServed, Now()); + + const int ads_per_day = features::GetMaximumNewTabPageAdsPerDay(); + for (int i = 0; i < ads_per_day - 1; ++i) { + FireAdEvent(ad_event1); + } + + AdvanceClock(base::Hours(1)); + + // Act + ad_serving_->MaybeServeAd( + [&creative_ad2](const bool success, const NewTabPageAdInfo& ad) { + ASSERT_TRUE(success); + + NewTabPageAdInfo expected_ad = BuildNewTabPageAd(creative_ad2); + expected_ad.uuid = ad.uuid; + + EXPECT_EQ(expected_ad, ad); + }); + + // Assert +} + +TEST_F(BatAdsNewTabPageAdServingTest, DoNotServeAdIfExceededAdsPerDayCap) { + // Arrange + new_tab_page_ads::frequency_capping::ForcePermissionRules(); + + CreativeNewTabPageAdList creative_ads; + CreativeNewTabPageAdInfo creative_ad1 = BuildCreativeNewTabPageAd(); + CreativeNewTabPageAdInfo creative_ad2 = BuildCreativeNewTabPageAd(); + creative_ads.push_back(creative_ad1); + creative_ads.push_back(creative_ad2); + Save(creative_ads); + + AdEventInfo ad_event1 = BuildAdEvent(creative_ad1, AdType::kNewTabPageAd, + ConfirmationType::kServed, Now()); + + const int ads_per_day = features::GetMaximumNewTabPageAdsPerDay(); + for (int i = 0; i < ads_per_day; ++i) { + FireAdEvent(ad_event1); + } + + AdvanceClock(base::Hours(1)); + + // Act + ad_serving_->MaybeServeAd([](const bool success, const NewTabPageAdInfo& ad) { + EXPECT_FALSE(success); + }); + + // Assert +} + } // namespace ads diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad.cc b/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad.cc index 685edf743b4d..a7c7784bf88c 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad.cc @@ -44,18 +44,6 @@ void NewTabPageAd::FireEvent(const std::string& uuid, return; } - // TODO(https://github.com/brave/brave-browser/issues/14015): Refactor this - // logic to only apply frequency capping if the new tab page ad was not served - // by the library. |AdServing::MaybeServeAd| is responsible for applying - // frequency caps for new tab page ads served by the library - new_tab_page_ads::frequency_capping::PermissionRules permission_rules; - if (event_type == mojom::NewTabPageAdEventType::kViewed && - !permission_rules.HasPermission()) { - BLOG(1, "New tab page ad: Not allowed due to permission rules"); - NotifyNewTabPageAdEventFailed(uuid, creative_instance_id, event_type); - return; - } - database::table::CreativeNewTabPageAds database_table; database_table.GetForCreativeInstanceId( creative_instance_id, @@ -100,14 +88,6 @@ void NewTabPageAd::FireEvent(const NewTabPageAdInfo& ad, return; } - if (event_type == mojom::NewTabPageAdEventType::kViewed) { - // TODO(https://github.com/brave/brave-browser/issues/14015): We need - // to fire an ad served event until new tab page ads are served by the - // ads library - FireEvent(uuid, creative_instance_id, - mojom::NewTabPageAdEventType::kServed); - } - const auto ad_event = new_tab_page_ads::AdEventFactory::Build(event_type); ad_event->FireEvent(ad); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_unittest.cc index 1117f239e938..6e0600c4a464 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_unittest.cc @@ -14,12 +14,10 @@ #include "bat/ads/internal/ad_serving/ad_serving_features.h" #include "bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_builder.h" #include "bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_observer.h" -#include "bat/ads/internal/ads/new_tab_page_ads/new_tab_page_ad_permission_rules_unittest_util.h" #include "bat/ads/internal/bundle/creative_new_tab_page_ad_info.h" #include "bat/ads/internal/bundle/creative_new_tab_page_ad_unittest_util.h" #include "bat/ads/internal/database/tables/ad_events_database_table.h" #include "bat/ads/internal/unittest_base.h" -#include "bat/ads/internal/unittest_time_util.h" #include "bat/ads/internal/unittest_util.h" #include "bat/ads/new_tab_page_ad_info.h" #include "bat/ads/public/interfaces/ads.mojom.h" @@ -103,9 +101,6 @@ class BatAdsNewTabPageAdTest : public NewTabPageAdObserver, }; TEST_F(BatAdsNewTabPageAdTest, FireViewedEvent) { - // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); // Act @@ -113,7 +108,7 @@ TEST_F(BatAdsNewTabPageAdTest, FireViewedEvent) { mojom::NewTabPageAdEventType::kViewed); // Assert - EXPECT_TRUE(did_serve_ad_); + EXPECT_FALSE(did_serve_ad_); EXPECT_TRUE(did_view_ad_); EXPECT_FALSE(did_click_ad_); EXPECT_FALSE(did_fail_to_fire_event_); @@ -125,8 +120,6 @@ TEST_F(BatAdsNewTabPageAdTest, FireViewedEvent) { TEST_F(BatAdsNewTabPageAdTest, FireClickedEvent) { // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); // Act @@ -146,8 +139,6 @@ TEST_F(BatAdsNewTabPageAdTest, FireClickedEvent) { TEST_F(BatAdsNewTabPageAdTest, DoNotFireViewedEventIfAlreadyFired) { // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); new_tab_page_ad_->FireEvent(kUuid, creative_ad.creative_instance_id, @@ -193,26 +184,8 @@ TEST_F(BatAdsNewTabPageAdTest, DoNotFireEventWithInvalidCreativeInstanceId) { ExpectAdEventCountEquals(ConfirmationType::kViewed, 0); } -TEST_F(BatAdsNewTabPageAdTest, DoNotFireEventWhenNotPermitted) { - // Arrange - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); - - // Act - new_tab_page_ad_->FireEvent(kUuid, creative_ad.creative_instance_id, - mojom::NewTabPageAdEventType::kViewed); - - // Assert - EXPECT_FALSE(did_serve_ad_); - EXPECT_FALSE(did_view_ad_); - EXPECT_FALSE(did_click_ad_); - EXPECT_TRUE(did_fail_to_fire_event_); - - ExpectAdEventCountEquals(ConfirmationType::kViewed, 0); -} - TEST_F(BatAdsNewTabPageAdTest, DoNotFireEventIfCreativeInstanceIdWasNotFound) { // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); // Act new_tab_page_ad_->FireEvent(kUuid, kCreativeInstanceId, @@ -227,96 +200,4 @@ TEST_F(BatAdsNewTabPageAdTest, DoNotFireEventIfCreativeInstanceIdWasNotFound) { ExpectAdEventCountEquals(ConfirmationType::kViewed, 0); } -TEST_F(BatAdsNewTabPageAdTest, FireEventIfNotExceededAdsPerHourCap) { - // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); - const AdEventInfo& ad_event = BuildAdEvent(creative_ad, AdType::kNewTabPageAd, - ConfirmationType::kViewed, Now()); - - const int ads_per_hour = features::GetMaximumNewTabPageAdsPerHour(); - - FireAdEvents(ad_event, ads_per_hour - 1); - - const std::string& uuid = base::GenerateGUID(); - - // Act - new_tab_page_ad_->FireEvent(uuid, creative_ad.creative_instance_id, - mojom::NewTabPageAdEventType::kViewed); - - // Assert - ExpectAdEventCountEquals(ConfirmationType::kViewed, ads_per_hour); -} - -TEST_F(BatAdsNewTabPageAdTest, DoNotFireEventIfExceededAdsPerHourCap) { - // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); - const AdEventInfo& ad_event = BuildAdEvent(creative_ad, AdType::kNewTabPageAd, - ConfirmationType::kViewed, Now()); - - const int ads_per_hour = features::GetMaximumNewTabPageAdsPerHour(); - - FireAdEvents(ad_event, ads_per_hour); - - const std::string& uuid = base::GenerateGUID(); - - // Act - new_tab_page_ad_->FireEvent(uuid, creative_ad.creative_instance_id, - mojom::NewTabPageAdEventType::kViewed); - - // Assert - ExpectAdEventCountEquals(ConfirmationType::kViewed, ads_per_hour); -} - -TEST_F(BatAdsNewTabPageAdTest, FireEventIfNotExceededAdsPerDayCap) { - // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); - const AdEventInfo& ad_event = BuildAdEvent(creative_ad, AdType::kNewTabPageAd, - ConfirmationType::kViewed, Now()); - - const int ads_per_day = features::GetMaximumNewTabPageAdsPerDay(); - - FireAdEvents(ad_event, ads_per_day - 1); - - AdvanceClock(base::Hours(1)); - - const std::string& uuid = base::GenerateGUID(); - - // Act - new_tab_page_ad_->FireEvent(uuid, creative_ad.creative_instance_id, - mojom::NewTabPageAdEventType::kViewed); - - // Assert - ExpectAdEventCountEquals(ConfirmationType::kViewed, ads_per_day); -} - -TEST_F(BatAdsNewTabPageAdTest, DoNotFireEventIfExceededAdsPerDayCap) { - // Arrange - new_tab_page_ads::frequency_capping::ForcePermissionRules(); - - const CreativeNewTabPageAdInfo& creative_ad = BuildAndSaveCreativeAd(); - const AdEventInfo& ad_event = BuildAdEvent(creative_ad, AdType::kNewTabPageAd, - ConfirmationType::kViewed, Now()); - - const int ads_per_day = features::GetMaximumNewTabPageAdsPerDay(); - - FireAdEvents(ad_event, ads_per_day); - - AdvanceClock(base::Hours(1)); - - const std::string& uuid = base::GenerateGUID(); - - // Act - new_tab_page_ad_->FireEvent(uuid, creative_ad.creative_instance_id, - mojom::NewTabPageAdEventType::kViewed); - - // Assert - ExpectAdEventCountEquals(ConfirmationType::kViewed, ads_per_day); -} - } // namespace ads