From a55412c5d689e546409999a60ec82be9bd8c93d9 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Wed, 4 Mar 2020 23:21:10 +0000 Subject: [PATCH] Fixes setting to disable Brave Ads ad conversion detection isn't working --- .../brave_ads/browser/ads_service_impl.cc | 11 ++--- .../brave_ads/browser/ads_service_impl.h | 2 - .../browser/bundle_state_database.cc | 4 +- .../brave_ads/browser/bundle_state_database.h | 1 - .../bat_ads/bat_ads_client_mojo_bridge.cc | 10 ++-- .../bat_ads/bat_ads_client_mojo_bridge.h | 1 - .../public/cpp/ads_client_mojo_bridge.cc | 8 ++-- .../public/cpp/ads_client_mojo_bridge.h | 2 - .../bat_ads/public/interfaces/bat_ads.mojom | 2 +- .../include/bat/ads/ads_client.h | 10 ++-- .../src/bat/ads/internal/ads_client_mock.h | 3 +- .../src/bat/ads/internal/ads_impl.cc | 47 +++++++++++++++++-- .../src/bat/ads/internal/ads_impl.h | 9 +++- vendor/brave-ios/Ads/BATBraveAds.mm | 4 +- .../brave-ios/Ads/Generated/NativeAdsClient.h | 2 +- .../Ads/Generated/NativeAdsClient.mm | 4 +- .../Ads/Generated/NativeAdsClientBridge.h | 2 +- 17 files changed, 75 insertions(+), 47 deletions(-) diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc index 402e09572d15..90e8d6456c87 100644 --- a/components/brave_ads/browser/ads_service_impl.cc +++ b/components/brave_ads/browser/ads_service_impl.cc @@ -227,7 +227,6 @@ ads::CreativeAdNotificationList GetCreativeAdNotificationsOnFileTaskRunner( } ads::AdConversionList GetAdConversionsOnFileTaskRunner( - const std::string& url, BundleStateDatabase* backend) { ads::AdConversionList ad_conversions; @@ -235,7 +234,7 @@ ads::AdConversionList GetAdConversionsOnFileTaskRunner( return ad_conversions; } - backend->GetAdConversions(url, &ad_conversions); + backend->GetAdConversions(&ad_conversions); return ad_conversions; } @@ -974,7 +973,6 @@ void AdsServiceImpl::OnGetCreativeAdNotifications( void AdsServiceImpl::OnGetAdConversions( const ads::OnGetAdConversionsCallback& callback, - const std::string& url, const ads::AdConversionList& ad_conversions) { if (!connected()) { return; @@ -983,7 +981,7 @@ void AdsServiceImpl::OnGetAdConversions( const auto result = ad_conversions.empty() ? ads::Result::FAILED : ads::Result::SUCCESS; - callback(result, url, ad_conversions); + callback(result, ad_conversions); } void AdsServiceImpl::OnGetAdsHistory( @@ -2118,13 +2116,12 @@ void AdsServiceImpl::GetCreativeAdNotifications( } void AdsServiceImpl::GetAdConversions( - const std::string& url, ads::OnGetAdConversionsCallback callback) { base::PostTaskAndReplyWithResult(file_task_runner_.get(), FROM_HERE, - base::BindOnce(&GetAdConversionsOnFileTaskRunner, url, + base::BindOnce(&GetAdConversionsOnFileTaskRunner, bundle_state_backend_.get()), base::BindOnce(&AdsServiceImpl::OnGetAdConversions, AsWeakPtr(), - std::move(callback), url)); + std::move(callback))); } void AdsServiceImpl::EventLog( diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h index 2e6a6df90630..114858c0b8ed 100644 --- a/components/brave_ads/browser/ads_service_impl.h +++ b/components/brave_ads/browser/ads_service_impl.h @@ -227,7 +227,6 @@ class AdsServiceImpl : public AdsService, void OnGetAdConversions( const ads::OnGetAdConversionsCallback& callback, - const std::string& url, const ads::AdConversionList& ad_conversions); void OnGetAdsHistory( @@ -437,7 +436,6 @@ class AdsServiceImpl : public AdsService, ads::OnGetCreativeAdNotificationsCallback callback) override; void GetAdConversions( - const std::string& url, ads::OnGetAdConversionsCallback callback) override; void EventLog( diff --git a/components/brave_ads/browser/bundle_state_database.cc b/components/brave_ads/browser/bundle_state_database.cc index 0905cea8333a..084b0a13e671 100644 --- a/components/brave_ads/browser/bundle_state_database.cc +++ b/components/brave_ads/browser/bundle_state_database.cc @@ -490,7 +490,6 @@ bool BundleStateDatabase::GetCreativeAdNotifications( } bool BundleStateDatabase::GetAdConversions( - const std::string& url, ads::AdConversionList* ad_conversions) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); @@ -502,7 +501,8 @@ bool BundleStateDatabase::GetAdConversions( const std::string sql = "SELECT " "ac.creative_set_id, " - "ac.type, ac.url_pattern, " + "ac.type, " + "ac.url_pattern, " "ac.observation_window " "FROM ad_conversions AS ac"; diff --git a/components/brave_ads/browser/bundle_state_database.h b/components/brave_ads/browser/bundle_state_database.h index c6ae8f02e777..41148549e860 100644 --- a/components/brave_ads/browser/bundle_state_database.h +++ b/components/brave_ads/browser/bundle_state_database.h @@ -47,7 +47,6 @@ class BundleStateDatabase { ads::CreativeAdNotificationList* ads); bool GetAdConversions( - const std::string& url, ads::AdConversionList* ad_conversions); // Returns the current version of the bundle state database diff --git a/components/services/bat_ads/bat_ads_client_mojo_bridge.cc b/components/services/bat_ads/bat_ads_client_mojo_bridge.cc index 65247131cb03..99390b6f2009 100644 --- a/components/services/bat_ads/bat_ads_client_mojo_bridge.cc +++ b/components/services/bat_ads/bat_ads_client_mojo_bridge.cc @@ -459,32 +459,30 @@ void BatAdsClientMojoBridge::GetCreativeAdNotifications( void OnGetAdConversions( const ads::OnGetAdConversionsCallback& callback, const int32_t result, - const std::string& url, const std::vector& json_list) { ads::AdConversionList ad_conversions; for (const auto& json : json_list) { ads::AdConversionInfo ad_conversion; if (ad_conversion.FromJson(json) != ads::Result::SUCCESS) { - callback(ads::Result::FAILED, url, {}); + callback(ads::Result::FAILED, {}); return; } ad_conversions.push_back(ad_conversion); } - callback(ToAdsResult(result), url, ad_conversions); + callback(ToAdsResult(result), ad_conversions); } void BatAdsClientMojoBridge::GetAdConversions( - const std::string& url, ads::OnGetAdConversionsCallback callback) { if (!connected()) { - callback(ads::Result::FAILED, url, {}); + callback(ads::Result::FAILED, {}); return; } - bat_ads_client_->GetAdConversions(url, + bat_ads_client_->GetAdConversions( base::BindOnce(&OnGetAdConversions, std::move(callback))); } diff --git a/components/services/bat_ads/bat_ads_client_mojo_bridge.h b/components/services/bat_ads/bat_ads_client_mojo_bridge.h index 70705b096c9f..29b7e2edbb18 100644 --- a/components/services/bat_ads/bat_ads_client_mojo_bridge.h +++ b/components/services/bat_ads/bat_ads_client_mojo_bridge.h @@ -107,7 +107,6 @@ class BatAdsClientMojoBridge ads::OnGetCreativeAdNotificationsCallback callback) override; void GetAdConversions( - const std::string& url, ads::OnGetAdConversionsCallback callback) override; void EventLog( diff --git a/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc b/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc index 62cd5b2a36e7..64d9dbb4e859 100644 --- a/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc +++ b/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.cc @@ -501,7 +501,6 @@ void AdsClientMojoBridge::GetCreativeAdNotifications( void AdsClientMojoBridge::OnGetAdConversions( CallbackHolder* holder, const ads::Result result, - const std::string& url, const ads::AdConversionList& ad_conversions) { DCHECK(holder); @@ -512,21 +511,20 @@ void AdsClientMojoBridge::OnGetAdConversions( json_list.push_back(ad_conversion.ToJson()); } - std::move(holder->get()).Run(ToMojomResult(result), url, json_list); + std::move(holder->get()).Run(ToMojomResult(result), json_list); } delete holder; } void AdsClientMojoBridge::GetAdConversions( - const std::string& url, GetAdConversionsCallback callback) { // this gets deleted in OnGetAdConversions auto* holder = new CallbackHolder( AsWeakPtr(), std::move(callback)); - ads_client_->GetAdConversions(url, - std::bind(AdsClientMojoBridge::OnGetAdConversions, holder, _1, _2, _3)); + ads_client_->GetAdConversions( + std::bind(AdsClientMojoBridge::OnGetAdConversions, holder, _1, _2)); } } // namespace bat_ads diff --git a/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.h b/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.h index bfb51a8fddfc..07361eb40105 100644 --- a/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.h +++ b/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.h @@ -134,7 +134,6 @@ class AdsClientMojoBridge const std::vector& categories, GetCreativeAdNotificationsCallback callback) override; void GetAdConversions( - const std::string& url, GetAdConversionsCallback callback) override; private: @@ -197,7 +196,6 @@ class AdsClientMojoBridge static void OnGetAdConversions( CallbackHolder* holder, const ads::Result result, - const std::string& url, const ads::AdConversionList& ad_conversions); ads::AdsClient* ads_client_; // NOT OWNED diff --git a/components/services/bat_ads/public/interfaces/bat_ads.mojom b/components/services/bat_ads/public/interfaces/bat_ads.mojom index 89d6847b5498..6e633118a275 100644 --- a/components/services/bat_ads/public/interfaces/bat_ads.mojom +++ b/components/services/bat_ads/public/interfaces/bat_ads.mojom @@ -60,7 +60,7 @@ interface BatAdsClient { LoadSampleBundle() => (int32 result, string value); Reset(string name) => (int32 result); GetCreativeAdNotifications(array categories) => (int32 result, array categories, array json_list); - GetAdConversions(string url) => (int32 result, string url, array json_list); + GetAdConversions() => (int32 result, array json_list); EventLog(string json); }; diff --git a/vendor/bat-native-ads/include/bat/ads/ads_client.h b/vendor/bat-native-ads/include/bat/ads/ads_client.h index 9c27d96f166d..75e6d90a0512 100644 --- a/vendor/bat-native-ads/include/bat/ads/ads_client.h +++ b/vendor/bat-native-ads/include/bat/ads/ads_client.h @@ -53,7 +53,7 @@ using OnGetCreativeAdNotificationsCallback = std::function&, const CreativeAdNotificationList&)>; using OnGetAdConversionsCallback = std::function; + const AdConversionList&)>; using OnLoadSampleBundleCallback = std::function; @@ -240,13 +240,11 @@ class ADS_EXPORT AdsClient { const std::vector& categories, OnGetCreativeAdNotificationsCallback callback) = 0; - // Should fetch all ad conversions for the specified |url| from the - // previously persisted bundle state. The callback takes 3 arguments — - // |Result| should be set to |SUCCESS| if successful; otherwise, should be set - // to |FAILED|. |url| should contain the url. |ad_conversions| should + // Should fetch all ad conversions from the previously persisted bundle state. + // The callback takes 2 arguments — |Result| should be set to |SUCCESS| if + // successful; otherwise, should be set to |FAILED|. |ad_conversions| should // contain an array of ad conversions virtual void GetAdConversions( - const std::string& url, OnGetAdConversionsCallback callback) = 0; // Should log an event diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ads_client_mock.h b/vendor/bat-native-ads/src/bat/ads/internal/ads_client_mock.h index 5cd0c9224897..d063e944f2b2 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ads_client_mock.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/ads_client_mock.h @@ -127,8 +127,7 @@ class MockAdsClient : public AdsClient { const std::vector& categories, OnGetCreativeAdNotificationsCallback callback)); - MOCK_METHOD2(GetAdConversions, void( - const std::string& url, + MOCK_METHOD1(GetAdConversions, void( OnGetAdConversionsCallback callback)); MOCK_CONST_METHOD1(EventLog, void( diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc b/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc index e0fbd7cf3177..9222758f4c8e 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc @@ -715,13 +715,13 @@ void AdsImpl::CheckAdConversion( return; } - auto callback = std::bind(&AdsImpl::OnGetAdConversions, this, _1, _2, _3); - ads_client_->GetAdConversions(url, callback); + auto callback = std::bind(&AdsImpl::OnGetAdConversions, this, url, _1, _2); + ads_client_->GetAdConversions(callback); } void AdsImpl::OnGetAdConversions( - const Result result, const std::string& url, + const Result result, const AdConversionList& ad_conversions) { for (const auto& ad_conversion : ad_conversions) { if (!helper::Uri::MatchesWildcard(url, ad_conversion.url_pattern)) { @@ -1198,8 +1198,24 @@ void AdsImpl::OnServeUntargetedAdNotification( void AdsImpl::ServeAdNotification( const CreativeAdNotificationList& ads) { - auto rand = base::RandInt(0, ads.size() - 1); - auto ad = ads.at(rand); + auto callback = + std::bind(&AdsImpl::OnServeAdNotification, this, _1, _2, ads); + ads_client_->GetAdConversions(callback); +} + +void AdsImpl::OnServeAdNotification( + const Result result, + const AdConversionList& ad_conversions, + const CreativeAdNotificationList& ads) { + CreativeAdNotificationList eligible_ads = + GetEligibleAdsForConversions(ads, ad_conversions); + if (eligible_ads.empty()) { + FailedToServeAdNotification("No eligible ads found"); + return; + } + + const int rand = base::RandInt(0, eligible_ads.size() - 1); + const CreativeAdNotificationInfo ad = eligible_ads.at(rand); ShowAdNotification(ad); SuccessfullyServedAd(); @@ -1288,6 +1304,27 @@ CreativeAdNotificationList AdsImpl::GetEligibleAds( return eligible_ads; } +CreativeAdNotificationList AdsImpl::GetEligibleAdsForConversions( + const CreativeAdNotificationList& ads, + const AdConversionList& ad_conversions) { + CreativeAdNotificationList eligible_ads = ads; + + if (ads_client_->ShouldAllowAdConversionTracking()) { + return eligible_ads; + } + + for (const auto& ad_conversion : ad_conversions) { + const auto iter = std::remove_if(eligible_ads.begin(), eligible_ads.end(), + [&ad_conversion](CreativeAdNotificationInfo& ad_notification) { + return ad_notification.creative_set_id == ad_conversion.creative_set_id; + }); + + eligible_ads.erase(iter, eligible_ads.end()); + } + + return eligible_ads; +} + CreativeAdNotificationList AdsImpl::GetUnseenAdsAndRoundRobinIfNeeded( const CreativeAdNotificationList& ads) const { if (ads.empty()) { diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.h b/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.h index 1b93826556db..4488f409044b 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.h @@ -187,8 +187,8 @@ class AdsImpl : public Ads { void CheckAdConversion( const std::string& url); void OnGetAdConversions( - const Result result, const std::string& url, + const Result result, const AdConversionList& ad_conversions); void MaybeServeAdNotification( @@ -210,12 +210,19 @@ class AdsImpl : public Ads { const CreativeAdNotificationList& ads); void ServeAdNotification( const CreativeAdNotificationList& ads); + void OnServeAdNotification( + const Result result, + const AdConversionList& ad_conversions, + const CreativeAdNotificationList& ads); void SuccessfullyServedAd(); void FailedToServeAdNotification( const std::string& reason); CreativeAdNotificationList GetEligibleAds( const CreativeAdNotificationList& ads); + CreativeAdNotificationList GetEligibleAdsForConversions( + const CreativeAdNotificationList& ads, + const AdConversionList& ad_conversions); CreativeAdNotificationList GetUnseenAdsAndRoundRobinIfNeeded( const CreativeAdNotificationList& ads) const; CreativeAdNotificationList GetUnseenAds( diff --git a/vendor/brave-ios/Ads/BATBraveAds.mm b/vendor/brave-ios/Ads/BATBraveAds.mm index 5fccdaad29a7..87a2f6727d33 100644 --- a/vendor/brave-ios/Ads/BATBraveAds.mm +++ b/vendor/brave-ios/Ads/BATBraveAds.mm @@ -396,12 +396,12 @@ - (void)getCreativeAdNotifications:(const std::vector &)categories callback(ads::Result::SUCCESS, categories, found_ads); } -- (void)getAdConversions:(const std::string &)url callback:(ads::OnGetAdConversionsCallback)callback +- (void)getAdConversions:(ads::OnGetAdConversionsCallback)callback { // TODO(khickinson): To be implemented if (![self isAdsServiceRunning]) { return; } - callback(ads::Result::SUCCESS, url, {}); + callback(ads::Result::SUCCESS, {}); } - (void)setCatalogIssuers:(std::unique_ptr)info diff --git a/vendor/brave-ios/Ads/Generated/NativeAdsClient.h b/vendor/brave-ios/Ads/Generated/NativeAdsClient.h index 91c3dbb9823d..87d757dfbffc 100644 --- a/vendor/brave-ios/Ads/Generated/NativeAdsClient.h +++ b/vendor/brave-ios/Ads/Generated/NativeAdsClient.h @@ -45,7 +45,7 @@ class NativeAdsClient : public ads::AdsClient { void LoadSampleBundle(ads::OnLoadSampleBundleCallback callback) override; void SaveBundleState(std::unique_ptr state, ads::OnSaveCallback callback) override; void GetCreativeAdNotifications(const std::vector & categories, ads::OnGetCreativeAdNotificationsCallback callback) override; - void GetAdConversions(const std::string & url, ads::OnGetAdConversionsCallback callback) override; + void GetAdConversions(ads::OnGetAdConversionsCallback callback) override; void EventLog(const std::string & json) const override; std::unique_ptr Log(const char * file, const int line, const ads::LogLevel log_level) const override; }; diff --git a/vendor/brave-ios/Ads/Generated/NativeAdsClient.mm b/vendor/brave-ios/Ads/Generated/NativeAdsClient.mm index bc5f9f3b1822..00a0da705aac 100644 --- a/vendor/brave-ios/Ads/Generated/NativeAdsClient.mm +++ b/vendor/brave-ios/Ads/Generated/NativeAdsClient.mm @@ -127,8 +127,8 @@ [bridge_ getCreativeAdNotifications:categories callback:callback]; } -void NativeAdsClient::GetAdConversions(const std::string & url, ads::OnGetAdConversionsCallback callback) { - [bridge_ getAdConversions:url callback:callback]; +void NativeAdsClient::GetAdConversions(ads::OnGetAdConversionsCallback callback) { + [bridge_ getAdConversions:callback]; } void NativeAdsClient::EventLog(const std::string & json) const { diff --git a/vendor/brave-ios/Ads/Generated/NativeAdsClientBridge.h b/vendor/brave-ios/Ads/Generated/NativeAdsClientBridge.h index 60404cc95c8f..e4b9d9ed8ff9 100644 --- a/vendor/brave-ios/Ads/Generated/NativeAdsClientBridge.h +++ b/vendor/brave-ios/Ads/Generated/NativeAdsClientBridge.h @@ -14,7 +14,7 @@ - (void)confirmAction:(const std::string &)uuid creativeSetId:(const std::string &)creative_set_id confirmationType:(const ads::ConfirmationType &)confirmation_type; - (void)eventLog:(const std::string &)json; - (void)getCreativeAdNotifications:(const std::vector &)categories callback:(ads::OnGetCreativeAdNotificationsCallback)callback; -- (void)getAdConversions:(const std::string &)url callback:(ads::OnGetAdConversionsCallback)callback; +- (void)getAdConversions:(ads::OnGetAdConversionsCallback)callback; - (std::string)getLocale; - (uint64_t)getAdsPerDay; - (uint64_t)getAdsPerHour;