From 4094e7cbb05b9ba774dfbda321bd20b833973d2e Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 27 Oct 2021 20:17:23 -0500 Subject: [PATCH] Fixes ads will not be served for campaign with more than one state geotarget --- .../creative_ad_notification_info_aliases.h | 4 + .../creative_inline_content_ad_info_aliases.h | 4 + .../creative_new_tab_page_ad_info_aliases.h | 3 + ...reative_promoted_content_ad_info_aliases.h | 4 + ...reative_ad_notifications_database_table.cc | 81 ++++++++----- ...creative_ad_notifications_database_table.h | 4 + ...d_notifications_database_table_unittest.cc | 7 +- ...ative_inline_content_ads_database_table.cc | 106 +++++++++++------- ...eative_inline_content_ads_database_table.h | 4 + ...ine_content_ads_database_table_unittest.cc | 5 +- ...reative_new_tab_page_ads_database_table.cc | 92 +++++++++------ ...creative_new_tab_page_ads_database_table.h | 4 + ...ew_tab_page_ads_database_table_unittest.cc | 7 +- ...ive_promoted_content_ads_database_table.cc | 96 ++++++++++------ ...tive_promoted_content_ads_database_table.h | 4 + ...ted_content_ads_database_table_unittest.cc | 7 +- ...vision_targeting_frequency_cap_unittest.cc | 16 +++ .../bat/ads/internal/segments/segments_util.h | 15 +++ 18 files changed, 319 insertions(+), 144 deletions(-) diff --git a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_ad_notification_info_aliases.h b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_ad_notification_info_aliases.h index 00a5803ff301..a3ddd9ca6c16 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_ad_notification_info_aliases.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_ad_notification_info_aliases.h @@ -6,6 +6,8 @@ #ifndef BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_AD_NOTIFICATION_INFO_ALIASES_H_ #define BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_AD_NOTIFICATION_INFO_ALIASES_H_ +#include +#include #include #include "bat/ads/internal/bundle/creative_ad_notification_info.h" @@ -13,6 +15,8 @@ namespace ads { using CreativeAdNotificationList = std::vector; +using CreativeAdNotificationMap = + std::map; } // namespace ads diff --git a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_inline_content_ad_info_aliases.h b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_inline_content_ad_info_aliases.h index 2f0fb1e536fb..397ec7ad9dea 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_inline_content_ad_info_aliases.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_inline_content_ad_info_aliases.h @@ -6,6 +6,8 @@ #ifndef BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_INLINE_CONTENT_AD_INFO_ALIASES_H_ #define BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_INLINE_CONTENT_AD_INFO_ALIASES_H_ +#include +#include #include #include "bat/ads/internal/bundle/creative_inline_content_ad_info.h" @@ -13,6 +15,8 @@ namespace ads { using CreativeInlineContentAdList = std::vector; +using CreativeInlineContentAdMap = + std::map; } // namespace ads diff --git a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_new_tab_page_ad_info_aliases.h b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_new_tab_page_ad_info_aliases.h index f390be361502..8ae45242f648 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_new_tab_page_ad_info_aliases.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_new_tab_page_ad_info_aliases.h @@ -6,6 +6,8 @@ #ifndef BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_NEW_TAB_PAGE_AD_INFO_ALIASES_H_ #define BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_NEW_TAB_PAGE_AD_INFO_ALIASES_H_ +#include +#include #include #include "bat/ads/internal/bundle/creative_new_tab_page_ad_info.h" @@ -13,6 +15,7 @@ namespace ads { using CreativeNewTabPageAdList = std::vector; +using CreativeNewTabPageAdMap = std::map; } // namespace ads diff --git a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_promoted_content_ad_info_aliases.h b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_promoted_content_ad_info_aliases.h index d963998e8358..4ee67cd0a189 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_promoted_content_ad_info_aliases.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/bundle/creative_promoted_content_ad_info_aliases.h @@ -6,6 +6,8 @@ #ifndef BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_PROMOTED_CONTENT_AD_INFO_ALIASES_H_ #define BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_BUNDLE_CREATIVE_PROMOTED_CONTENT_AD_INFO_ALIASES_H_ +#include +#include #include #include "bat/ads/internal/bundle/creative_promoted_content_ad_info.h" @@ -14,6 +16,8 @@ namespace ads { using CreativePromotedContentAdList = std::vector; +using CreativePromotedContentAdMap = + std::map; } // namespace ads diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.cc index e4d3ca1020c4..277dba63bc82 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.cc @@ -5,7 +5,6 @@ #include "bat/ads/internal/database/tables/creative_ad_notifications_database_table.h" -#include #include #include @@ -15,7 +14,8 @@ #include "base/time/time.h" #include "bat/ads/ads_client.h" #include "bat/ads/internal/ads_client_helper.h" -#include "bat/ads/internal/bundle/creative_ad_info.h" +#include "bat/ads/internal/bundle/creative_ad_info_aliases.h" +#include "bat/ads/internal/bundle/creative_ad_notification_info.h" #include "bat/ads/internal/container_util.h" #include "bat/ads/internal/database/database_statement_util.h" #include "bat/ads/internal/database/database_table_util.h" @@ -26,6 +26,7 @@ #include "bat/ads/internal/database/tables/geo_targets_database_table.h" #include "bat/ads/internal/database/tables/segments_database_table.h" #include "bat/ads/internal/logging.h" +#include "bat/ads/internal/segments/segments_util.h" #include "bat/ads/internal/time_formatting_util.h" namespace ads { @@ -356,16 +357,10 @@ void CreativeAdNotifications::OnGetForSegments( return; } - CreativeAdNotificationList creative_ad_notifications; + CreativeAdNotificationList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - for (const auto& record : response->result->get_records()) { - const CreativeAdNotificationInfo creative_ad_notification = - GetFromRecord(record.get()); - - creative_ad_notifications.push_back(creative_ad_notification); - } - - callback(/* success */ true, segments, creative_ad_notifications); + callback(/* success */ true, segments, creative_ads); } void CreativeAdNotifications::OnGetAll( @@ -378,24 +373,12 @@ void CreativeAdNotifications::OnGetAll( return; } - CreativeAdNotificationList creative_ad_notifications; - - SegmentList segments; - - for (const auto& record : response->result->get_records()) { - const CreativeAdNotificationInfo creative_ad_notification = - GetFromRecord(record.get()); - - creative_ad_notifications.push_back(creative_ad_notification); - - segments.push_back(creative_ad_notification.segment); - } + const CreativeAdNotificationList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - std::sort(segments.begin(), segments.end()); - const auto iter = std::unique(segments.begin(), segments.end()); - segments.erase(iter, segments.end()); + const SegmentList segments = GetSegments(creative_ads); - callback(/* success */ true, segments, creative_ad_notifications); + callback(/* success */ true, segments, creative_ads); } CreativeAdNotificationInfo CreativeAdNotifications::GetFromRecord( @@ -435,6 +418,50 @@ CreativeAdNotificationInfo CreativeAdNotifications::GetFromRecord( return creative_ad_notification; } +CreativeAdNotificationMap CreativeAdNotifications::GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + CreativeAdNotificationMap creative_ads; + + for (const auto& record : response->result->get_records()) { + const CreativeAdNotificationInfo creative_ad = GetFromRecord(record.get()); + + const auto iter = creative_ads.find(creative_ad.creative_instance_id); + if (iter == creative_ads.end()) { + creative_ads.insert({creative_ad.creative_instance_id, creative_ad}); + continue; + } + + // Creative instance already exists, so append the geo targets and dayparts + // to the existing creative ad + iter->second.geo_targets.insert(creative_ad.geo_targets.begin(), + creative_ad.geo_targets.end()); + + iter->second.dayparts.insert(iter->second.dayparts.end(), + creative_ad.dayparts.begin(), + creative_ad.dayparts.end()); + } + + return creative_ads; +} + +CreativeAdNotificationList CreativeAdNotifications::GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + const CreativeAdNotificationMap grouped_creative_ads = + GroupCreativeAdsFromResponse(std::move(response)); + + CreativeAdNotificationList creative_ads; + for (const auto& grouped_creative_ad : grouped_creative_ads) { + const CreativeAdNotificationInfo creative_ad = grouped_creative_ad.second; + creative_ads.push_back(creative_ad); + } + + return creative_ads; +} + void CreativeAdNotifications::CreateTableV16( mojom::DBTransaction* transaction) { DCHECK(transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.h b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.h index ea4e54223ee5..8e30b7725e8b 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table.h @@ -74,6 +74,10 @@ class CreativeAdNotifications final : public Table { GetCreativeAdNotificationsCallback callback); CreativeAdNotificationInfo GetFromRecord(mojom::DBRecord* record) const; + CreativeAdNotificationMap GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); + CreativeAdNotificationList GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); void CreateTableV16(mojom::DBTransaction* transaction); void MigrateToV16(mojom::DBTransaction* transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table_unittest.cc index bd33b7bd498b..a59066b3a528 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_ad_notifications_database_table_unittest.cc @@ -260,6 +260,7 @@ TEST_F(BatAdsCreativeAdNotificationsDatabaseTableTest, CreativeAdNotificationList creative_ads; CreativeDaypartInfo daypart_info; + CreativeAdNotificationInfo info_1; info_1.creative_instance_id = "3519f52c-46a4-4c48-9c2b-c264c0067f04"; info_1.creative_set_id = "c2ba3e7d-f688-4bc4-a053-cbe7ac1e6123"; @@ -276,6 +277,7 @@ TEST_F(BatAdsCreativeAdNotificationsDatabaseTableTest, info_1.value = 1.0; info_1.segment = "technology & computing-software"; info_1.dayparts.push_back(daypart_info); + info_1.dayparts.push_back(daypart_info); info_1.geo_targets = {"US"}; info_1.target_url = "https://brave.com"; info_1.title = "Test Ad 1 Title"; @@ -299,7 +301,8 @@ TEST_F(BatAdsCreativeAdNotificationsDatabaseTableTest, info_2.value = 1.0; info_2.segment = "technology & computing-software"; info_2.dayparts.push_back(daypart_info); - info_2.geo_targets = {"US"}; + info_2.dayparts.push_back(daypart_info); + info_2.geo_targets = {"US-FL", "US-CA"}; info_2.target_url = "https://brave.com"; info_2.title = "Test Ad 2 Title"; info_2.body = "Test Ad 2 Body"; @@ -372,7 +375,7 @@ TEST_F(BatAdsCreativeAdNotificationsDatabaseTableTest, } TEST_F(BatAdsCreativeAdNotificationsDatabaseTableTest, - GetCreativeAdNotificationsForNonExistentCategory) { + GetCreativeAdNotificationsForNonExistentSegment) { // Arrange CreativeAdNotificationList creative_ads; diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.cc index 4a3172c884bc..c513c2a2f994 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.cc @@ -5,7 +5,6 @@ #include "bat/ads/internal/database/tables/creative_inline_content_ads_database_table.h" -#include #include #include @@ -15,6 +14,7 @@ #include "base/time/time.h" #include "bat/ads/ads_client.h" #include "bat/ads/internal/ads_client_helper.h" +#include "bat/ads/internal/bundle/creative_ad_info.h" #include "bat/ads/internal/bundle/creative_inline_content_ad_info.h" #include "bat/ads/internal/container_util.h" #include "bat/ads/internal/database/database_statement_util.h" @@ -26,6 +26,7 @@ #include "bat/ads/internal/database/tables/geo_targets_database_table.h" #include "bat/ads/internal/database/tables/segments_database_table.h" #include "bat/ads/internal/logging.h" +#include "bat/ads/internal/segments/segments_util.h" #include "bat/ads/internal/time_formatting_util.h" namespace ads { @@ -570,19 +571,18 @@ void CreativeInlineContentAds::OnGetForCreativeInstanceId( return; } - if (response->result->get_records().size() != 1) { + const CreativeInlineContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); + + if (creative_ads.size() != 1) { BLOG(0, "Failed to get creative inline content ad"); callback(/* success */ false, creative_instance_id, {}); return; } - mojom::DBRecord* record = response->result->get_records().at(0).get(); - - const CreativeInlineContentAdInfo creative_inline_content_ad = - GetFromRecord(record); + const CreativeInlineContentAdInfo creative_ad = creative_ads.front(); - callback(/* success */ true, creative_instance_id, - creative_inline_content_ad); + callback(/* success */ true, creative_instance_id, creative_ad); } void CreativeInlineContentAds::OnGetForSegmentsAndDimensions( @@ -596,16 +596,10 @@ void CreativeInlineContentAds::OnGetForSegmentsAndDimensions( return; } - CreativeInlineContentAdList creative_inline_content_ads; + const CreativeInlineContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - for (const auto& record : response->result->get_records()) { - const CreativeInlineContentAdInfo creative_inline_content_ad = - GetFromRecord(record.get()); - - creative_inline_content_ads.push_back(creative_inline_content_ad); - } - - callback(/* success */ true, segments, creative_inline_content_ads); + callback(/* success */ true, segments, creative_ads); } void CreativeInlineContentAds::OnGetForDimensions( @@ -618,16 +612,10 @@ void CreativeInlineContentAds::OnGetForDimensions( return; } - CreativeInlineContentAdList creative_inline_content_ads; - - for (const auto& record : response->result->get_records()) { - const CreativeInlineContentAdInfo creative_inline_content_ad = - GetFromRecord(record.get()); + const CreativeInlineContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - creative_inline_content_ads.push_back(creative_inline_content_ad); - } - - callback(/* success */ true, creative_inline_content_ads); + callback(/* success */ true, creative_ads); } void CreativeInlineContentAds::OnGetAll( @@ -640,24 +628,12 @@ void CreativeInlineContentAds::OnGetAll( return; } - CreativeInlineContentAdList creative_inline_content_ads; - - SegmentList segments; - - for (const auto& record : response->result->get_records()) { - const CreativeInlineContentAdInfo creative_inline_content_ad = - GetFromRecord(record.get()); - - creative_inline_content_ads.push_back(creative_inline_content_ad); - - segments.push_back(creative_inline_content_ad.segment); - } + const CreativeInlineContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - std::sort(segments.begin(), segments.end()); - const auto iter = std::unique(segments.begin(), segments.end()); - segments.erase(iter, segments.end()); + const SegmentList segments = GetSegments(creative_ads); - callback(/* success */ true, segments, creative_inline_content_ads); + callback(/* success */ true, segments, creative_ads); } CreativeInlineContentAdInfo CreativeInlineContentAds::GetFromRecord( @@ -700,6 +676,52 @@ CreativeInlineContentAdInfo CreativeInlineContentAds::GetFromRecord( return creative_inline_content_ad; } +CreativeInlineContentAdMap +CreativeInlineContentAds::GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + CreativeInlineContentAdMap creative_ads; + + for (const auto& record : response->result->get_records()) { + const CreativeInlineContentAdInfo creative_ad = GetFromRecord(record.get()); + + const auto iter = creative_ads.find(creative_ad.creative_instance_id); + if (iter == creative_ads.end()) { + creative_ads.insert({creative_ad.creative_instance_id, creative_ad}); + continue; + } + + // Creative instance already exists, so append the geo targets and dayparts + // to the existing creative ad + iter->second.geo_targets.insert(creative_ad.geo_targets.begin(), + creative_ad.geo_targets.end()); + + iter->second.dayparts.insert(iter->second.dayparts.end(), + creative_ad.dayparts.begin(), + creative_ad.dayparts.end()); + } + + return creative_ads; +} + +CreativeInlineContentAdList +CreativeInlineContentAds::GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + const CreativeInlineContentAdMap grouped_creative_ads = + GroupCreativeAdsFromResponse(std::move(response)); + + CreativeInlineContentAdList creative_ads; + for (const auto& grouped_creative_ad : grouped_creative_ads) { + const CreativeInlineContentAdInfo creative_ad = grouped_creative_ad.second; + creative_ads.push_back(creative_ad); + } + + return creative_ads; +} + void CreativeInlineContentAds::CreateTableV16( mojom::DBTransaction* transaction) { DCHECK(transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.h b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.h index d98439a68d49..f48e6985ded5 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table.h @@ -95,6 +95,10 @@ class CreativeInlineContentAds final : public Table { GetCreativeInlineContentAdsCallback callback); CreativeInlineContentAdInfo GetFromRecord(mojom::DBRecord* record) const; + CreativeInlineContentAdMap GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); + CreativeInlineContentAdList GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); void CreateTableV16(mojom::DBTransaction* transaction); void MigrateToV16(mojom::DBTransaction* transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table_unittest.cc index 1b7808fc770b..97c274c7f5db 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_inline_content_ads_database_table_unittest.cc @@ -324,7 +324,6 @@ TEST_F(BatAdsCreativeInlineContentAdsDatabaseTableTest, // Arrange CreativeInlineContentAdList creative_ads; - CreativeDaypartInfo daypart_info; CreativeInlineContentAdInfo info; info.creative_instance_id = "3519f52c-46a4-4c48-9c2b-c264c0067f04"; info.creative_set_id = "c2ba3e7d-f688-4bc4-a053-cbe7ac1e6123"; @@ -338,8 +337,10 @@ TEST_F(BatAdsCreativeInlineContentAdsDatabaseTableTest, info.total_max = 4; info.value = 1.0; info.segment = "food & drink"; + CreativeDaypartInfo daypart_info; info.dayparts.push_back(daypart_info); - info.geo_targets = {"US"}; + info.dayparts.push_back(daypart_info); + info.geo_targets = {"US-FL", "US-CA"}; info.target_url = "https://brave.com"; info.title = "Test Ad Title"; info.description = "Test Ad Description"; diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.cc index 407dc2bd5453..9317c68fd660 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.cc @@ -5,7 +5,6 @@ #include "bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.h" -#include #include #include @@ -15,6 +14,8 @@ #include "base/time/time.h" #include "bat/ads/ads_client.h" #include "bat/ads/internal/ads_client_helper.h" +#include "bat/ads/internal/bundle/creative_ad_info.h" +#include "bat/ads/internal/bundle/creative_new_tab_page_ad_info.h" #include "bat/ads/internal/container_util.h" #include "bat/ads/internal/database/database_statement_util.h" #include "bat/ads/internal/database/database_table_util.h" @@ -25,6 +26,7 @@ #include "bat/ads/internal/database/tables/geo_targets_database_table.h" #include "bat/ads/internal/database/tables/segments_database_table.h" #include "bat/ads/internal/logging.h" +#include "bat/ads/internal/segments/segments_util.h" #include "bat/ads/internal/time_formatting_util.h" namespace ads { @@ -439,18 +441,18 @@ void CreativeNewTabPageAds::OnGetForCreativeInstanceId( return; } - if (response->result->get_records().size() != 1) { + const CreativeNewTabPageAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); + + if (creative_ads.size() != 1) { BLOG(0, "Failed to get creative new tab page ad"); callback(/* success */ false, creative_instance_id, {}); return; } - mojom::DBRecord* record = response->result->get_records().at(0).get(); - - const CreativeNewTabPageAdInfo creative_new_tab_page_ad = - GetFromRecord(record); + const CreativeNewTabPageAdInfo creative_ad = creative_ads.front(); - callback(/* success */ true, creative_instance_id, creative_new_tab_page_ad); + callback(/* success */ true, creative_instance_id, creative_ad); } void CreativeNewTabPageAds::OnGetForSegments( @@ -464,16 +466,10 @@ void CreativeNewTabPageAds::OnGetForSegments( return; } - CreativeNewTabPageAdList creative_new_tab_page_ads; - - for (const auto& record : response->result->get_records()) { - const CreativeNewTabPageAdInfo creative_new_tab_page_ad = - GetFromRecord(record.get()); - - creative_new_tab_page_ads.push_back(creative_new_tab_page_ad); - } + const CreativeNewTabPageAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - callback(/* success */ true, segments, creative_new_tab_page_ads); + callback(/* success */ true, segments, creative_ads); } void CreativeNewTabPageAds::OnGetAll( @@ -486,24 +482,12 @@ void CreativeNewTabPageAds::OnGetAll( return; } - CreativeNewTabPageAdList creative_new_tab_page_ads; + const CreativeNewTabPageAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - SegmentList segments; - - for (const auto& record : response->result->get_records()) { - const CreativeNewTabPageAdInfo creative_new_tab_page_ad = - GetFromRecord(record.get()); + const SegmentList segments = GetSegments(creative_ads); - creative_new_tab_page_ads.push_back(creative_new_tab_page_ad); - - segments.push_back(creative_new_tab_page_ad.segment); - } - - std::sort(segments.begin(), segments.end()); - const auto iter = std::unique(segments.begin(), segments.end()); - segments.erase(iter, segments.end()); - - callback(/* success */ true, segments, creative_new_tab_page_ads); + callback(/* success */ true, segments, creative_ads); } CreativeNewTabPageAdInfo CreativeNewTabPageAds::GetFromRecord( @@ -542,6 +526,50 @@ CreativeNewTabPageAdInfo CreativeNewTabPageAds::GetFromRecord( return creative_new_tab_page_ad; } +CreativeNewTabPageAdMap CreativeNewTabPageAds::GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + CreativeNewTabPageAdMap creative_ads; + + for (const auto& record : response->result->get_records()) { + const CreativeNewTabPageAdInfo creative_ad = GetFromRecord(record.get()); + + const auto iter = creative_ads.find(creative_ad.creative_instance_id); + if (iter == creative_ads.end()) { + creative_ads.insert({creative_ad.creative_instance_id, creative_ad}); + continue; + } + + // Creative instance already exists, so append the geo targets and dayparts + // to the existing creative ad + iter->second.geo_targets.insert(creative_ad.geo_targets.begin(), + creative_ad.geo_targets.end()); + + iter->second.dayparts.insert(iter->second.dayparts.end(), + creative_ad.dayparts.begin(), + creative_ad.dayparts.end()); + } + + return creative_ads; +} + +CreativeNewTabPageAdList CreativeNewTabPageAds::GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + const CreativeNewTabPageAdMap grouped_creative_ads = + GroupCreativeAdsFromResponse(std::move(response)); + + CreativeNewTabPageAdList creative_ads; + for (const auto& grouped_creative_ad : grouped_creative_ads) { + const CreativeNewTabPageAdInfo creative_ad = grouped_creative_ad.second; + creative_ads.push_back(creative_ad); + } + + return creative_ads; +} + void CreativeNewTabPageAds::CreateTableV16(mojom::DBTransaction* transaction) { DCHECK(transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.h b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.h index 77e1dd41d55c..643c492065cc 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table.h @@ -83,6 +83,10 @@ class CreativeNewTabPageAds final : public Table { GetCreativeNewTabPageAdsCallback callback); CreativeNewTabPageAdInfo GetFromRecord(mojom::DBRecord* record) const; + CreativeNewTabPageAdMap GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); + CreativeNewTabPageAdList GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); void CreateTableV16(mojom::DBTransaction* transaction); void MigrateToV16(mojom::DBTransaction* transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table_unittest.cc index f60e89a2444b..6f71d8d458c9 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_new_tab_page_ads_database_table_unittest.cc @@ -335,7 +335,6 @@ TEST_F(BatAdsCreativeNewTabPageAdsDatabaseTableTest, // Arrange CreativeNewTabPageAdList creative_new_tab_page_ads; - CreativeDaypartInfo daypart_info; CreativeNewTabPageAdInfo info; info.creative_instance_id = "3519f52c-46a4-4c48-9c2b-c264c0067f04"; info.creative_set_id = "c2ba3e7d-f688-4bc4-a053-cbe7ac1e6123"; @@ -351,8 +350,10 @@ TEST_F(BatAdsCreativeNewTabPageAdsDatabaseTableTest, info.total_max = 6; info.value = 1.0; info.segment = "technology & computing-software"; + CreativeDaypartInfo daypart_info; info.dayparts.push_back(daypart_info); - info.geo_targets = {"US"}; + info.dayparts.push_back(daypart_info); + info.geo_targets = {"US-FL", "US-CA"}; info.target_url = "https://brave.com"; info.company_name = "Test Ad 1 Title"; info.alt = "Test Ad 1 Body"; @@ -473,7 +474,7 @@ TEST_F(BatAdsCreativeNewTabPageAdsDatabaseTableTest, } TEST_F(BatAdsCreativeNewTabPageAdsDatabaseTableTest, - GetCreativeNewTabPageAdsForNonExistentCategory) { + GetCreativeNewTabPageAdsForNonExistentSegment) { // Arrange CreativeNewTabPageAdList creative_new_tab_page_ads; diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.cc index ba77d7f56e8c..80e790e7ee63 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.cc @@ -5,7 +5,6 @@ #include "bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.h" -#include #include #include @@ -15,6 +14,7 @@ #include "base/time/time.h" #include "bat/ads/ads_client.h" #include "bat/ads/internal/ads_client_helper.h" +#include "bat/ads/internal/bundle/creative_ad_info.h" #include "bat/ads/internal/bundle/creative_promoted_content_ad_info.h" #include "bat/ads/internal/container_util.h" #include "bat/ads/internal/database/database_statement_util.h" @@ -26,6 +26,7 @@ #include "bat/ads/internal/database/tables/geo_targets_database_table.h" #include "bat/ads/internal/database/tables/segments_database_table.h" #include "bat/ads/internal/logging.h" +#include "bat/ads/internal/segments/segments_util.h" #include "bat/ads/internal/time_formatting_util.h" namespace ads { @@ -443,19 +444,18 @@ void CreativePromotedContentAds::OnGetForCreativeInstanceId( return; } - if (response->result->get_records().size() != 1) { + CreativePromotedContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); + + if (creative_ads.size() != 1) { BLOG(0, "Failed to get creative new tab page ad"); callback(/* success */ false, creative_instance_id, {}); return; } - mojom::DBRecord* record = response->result->get_records().at(0).get(); - - const CreativePromotedContentAdInfo creative_promoted_content_ad = - GetFromRecord(record); + const CreativePromotedContentAdInfo creative_ad = creative_ads.front(); - callback(/* success */ true, creative_instance_id, - creative_promoted_content_ad); + callback(/* success */ true, creative_instance_id, creative_ad); } void CreativePromotedContentAds::OnGetForSegments( @@ -469,16 +469,10 @@ void CreativePromotedContentAds::OnGetForSegments( return; } - CreativePromotedContentAdList creative_promoted_content_ads; - - for (const auto& record : response->result->get_records()) { - const CreativePromotedContentAdInfo creative_promoted_content_ad = - GetFromRecord(record.get()); - - creative_promoted_content_ads.push_back(creative_promoted_content_ad); - } + const CreativePromotedContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - callback(/* success */ true, segments, creative_promoted_content_ads); + callback(/* success */ true, segments, creative_ads); } void CreativePromotedContentAds::OnGetAll( @@ -491,24 +485,12 @@ void CreativePromotedContentAds::OnGetAll( return; } - CreativePromotedContentAdList creative_promoted_content_ads; - - SegmentList segments; + const CreativePromotedContentAdList creative_ads = + GetCreativeAdsFromResponse(std::move(response)); - for (const auto& record : response->result->get_records()) { - const CreativePromotedContentAdInfo creative_promoted_content_ad = - GetFromRecord(record.get()); - - creative_promoted_content_ads.push_back(creative_promoted_content_ad); - - segments.push_back(creative_promoted_content_ad.segment); - } - - std::sort(segments.begin(), segments.end()); - const auto iter = std::unique(segments.begin(), segments.end()); - segments.erase(iter, segments.end()); + const SegmentList segments = GetSegments(creative_ads); - callback(/* success */ true, segments, creative_promoted_content_ads); + callback(/* success */ true, segments, creative_ads); } CreativePromotedContentAdInfo CreativePromotedContentAds::GetFromRecord( @@ -547,6 +529,54 @@ CreativePromotedContentAdInfo CreativePromotedContentAds::GetFromRecord( return creative_promoted_content_ad; } +CreativePromotedContentAdMap +CreativePromotedContentAds::GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + CreativePromotedContentAdMap creative_ads; + + for (const auto& record : response->result->get_records()) { + const CreativePromotedContentAdInfo creative_ad = + GetFromRecord(record.get()); + + const auto iter = creative_ads.find(creative_ad.creative_instance_id); + if (iter == creative_ads.end()) { + creative_ads.insert({creative_ad.creative_instance_id, creative_ad}); + continue; + } + + // Creative instance already exists, so append the geo targets and dayparts + // to the existing creative ad + iter->second.geo_targets.insert(creative_ad.geo_targets.begin(), + creative_ad.geo_targets.end()); + + iter->second.dayparts.insert(iter->second.dayparts.end(), + creative_ad.dayparts.begin(), + creative_ad.dayparts.end()); + } + + return creative_ads; +} + +CreativePromotedContentAdList +CreativePromotedContentAds::GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response) { + DCHECK(response); + + const CreativePromotedContentAdMap grouped_creative_ads = + GroupCreativeAdsFromResponse(std::move(response)); + + CreativePromotedContentAdList creative_ads; + for (const auto& grouped_creative_ad : grouped_creative_ads) { + const CreativePromotedContentAdInfo creative_ad = + grouped_creative_ad.second; + creative_ads.push_back(creative_ad); + } + + return creative_ads; +} + void CreativePromotedContentAds::CreateTableV16( mojom::DBTransaction* transaction) { DCHECK(transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.h b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.h index 1be148ff81ec..211ee98d8dda 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table.h @@ -85,6 +85,10 @@ class CreativePromotedContentAds final : public Table { GetCreativePromotedContentAdsCallback callback); CreativePromotedContentAdInfo GetFromRecord(mojom::DBRecord* record) const; + CreativePromotedContentAdMap GroupCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); + CreativePromotedContentAdList GetCreativeAdsFromResponse( + mojom::DBCommandResponsePtr response); void CreateTableV16(mojom::DBTransaction* transaction); void MigrateToV16(mojom::DBTransaction* transaction); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table_unittest.cc index 615b7317e73b..0bce595c0342 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/database/tables/creative_promoted_content_ads_database_table_unittest.cc @@ -339,7 +339,6 @@ TEST_F(BatAdsCreativePromotedContentAdsDatabaseTableTest, // Arrange CreativePromotedContentAdList creative_promoted_content_ads; - CreativeDaypartInfo daypart_info; CreativePromotedContentAdInfo info; info.creative_instance_id = "3519f52c-46a4-4c48-9c2b-c264c0067f04"; info.creative_set_id = "c2ba3e7d-f688-4bc4-a053-cbe7ac1e6123"; @@ -355,8 +354,10 @@ TEST_F(BatAdsCreativePromotedContentAdsDatabaseTableTest, info.total_max = 6; info.value = 1.0; info.segment = "technology & computing-software"; + CreativeDaypartInfo daypart_info; info.dayparts.push_back(daypart_info); - info.geo_targets = {"US"}; + info.dayparts.push_back(daypart_info); + info.geo_targets = {"US-FL", "US-CA"}; info.target_url = "https://brave.com"; info.title = "Test Ad 1 Title"; info.description = "Test Ad 1 Body"; @@ -481,7 +482,7 @@ TEST_F(BatAdsCreativePromotedContentAdsDatabaseTableTest, } TEST_F(BatAdsCreativePromotedContentAdsDatabaseTableTest, - GetCreativePromotedContentAdsForNonExistentCategory) { + GetCreativePromotedContentAdsForNonExistentSegment) { // Arrange CreativePromotedContentAdList creative_promoted_content_ads; diff --git a/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/subdivision_targeting_frequency_cap_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/subdivision_targeting_frequency_cap_unittest.cc index b86456173411..40427898d39e 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/subdivision_targeting_frequency_cap_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/subdivision_targeting_frequency_cap_unittest.cc @@ -54,6 +54,22 @@ TEST_F(BatAdsSubdivisionTargetingFrequencyCapTest, EXPECT_FALSE(should_exclude); } +TEST_F(BatAdsSubdivisionTargetingFrequencyCapTest, + AllowAdIfSubdivisionTargetingIsSupportedForMultipleGeoTargets) { + // Arrange + ads_client_mock_->SetStringPref(prefs::kAdsSubdivisionTargetingCode, "US-FL"); + + CreativeAdInfo creative_ad; + creative_ad.creative_set_id = kCreativeSetId; + creative_ad.geo_targets = {"US-FL", "US-CA"}; + + // Act + const bool should_exclude = frequency_cap_->ShouldExclude(creative_ad); + + // Assert + EXPECT_FALSE(should_exclude); +} + TEST_F( BatAdsSubdivisionTargetingFrequencyCapTest, AllowAdIfSubdivisionTargetingIsSupportedAndAutoDetectedForNonSubdivisionGeoTarget) { // NOLINT diff --git a/vendor/bat-native-ads/src/bat/ads/internal/segments/segments_util.h b/vendor/bat-native-ads/src/bat/ads/internal/segments/segments_util.h index 62d7ece55e86..cdb9e16002fd 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/segments/segments_util.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/segments/segments_util.h @@ -6,6 +6,7 @@ #ifndef BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_SEGMENTS_SEGMENTS_UTIL_H_ #define BRAVE_VENDOR_BAT_NATIVE_ADS_SRC_BAT_ADS_INTERNAL_SEGMENTS_SEGMENTS_UTIL_H_ +#include #include #include "bat/ads/internal/segments/segments_aliases.h" @@ -16,6 +17,20 @@ class Catalog; SegmentList GetSegments(const Catalog& catalog); +template +SegmentList GetSegments(const T& creative_ads) { + SegmentList segments; + for (const auto& creative_ad : creative_ads) { + segments.push_back(creative_ad.segment); + } + + std::sort(segments.begin(), segments.end()); + const auto iter = std::unique(segments.begin(), segments.end()); + segments.erase(iter, segments.end()); + + return segments; +} + std::string GetParentSegment(const std::string& segment); SegmentList GetParentSegments(const SegmentList& segments);