Fixes setting to disable Brave Ads ad conversion detection isn't working

brave · Mar 5, 2020 · a55412c · a55412c
1 parent a2379f2
commit a55412c
Show file tree

Hide file tree

Showing 17 changed files with 75 additions and 47 deletions.
diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc
@@ -227,15 +227,14 @@ ads::CreativeAdNotificationList GetCreativeAdNotificationsOnFileTaskRunner(
 }
 
 ads::AdConversionList GetAdConversionsOnFileTaskRunner(
-    const std::string& url,
     BundleStateDatabase* backend) {
   ads::AdConversionList ad_conversions;
 
   if (!backend) {
     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
@@ -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
@@ -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
@@ -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
@@ -459,32 +459,30 @@ void BatAdsClientMojoBridge::GetCreativeAdNotifications(
 void OnGetAdConversions(
     const ads::OnGetAdConversionsCallback& callback,
     const int32_t result,
-    const std::string& url,
     const std::vector<std::string>& 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
@@ -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
@@ -501,7 +501,6 @@ void AdsClientMojoBridge::GetCreativeAdNotifications(
 void AdsClientMojoBridge::OnGetAdConversions(
     CallbackHolder<GetAdConversionsCallback>* 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<GetAdConversionsCallback>(
       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
@@ -134,7 +134,6 @@ class AdsClientMojoBridge
       const std::vector<std::string>& categories,
       GetCreativeAdNotificationsCallback callback) override;
   void GetAdConversions(
-      const std::string& url,
       GetAdConversionsCallback callback) override;
 
  private:
@@ -197,7 +196,6 @@ class AdsClientMojoBridge
   static void OnGetAdConversions(
       CallbackHolder<GetAdConversionsCallback>* 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
@@ -60,7 +60,7 @@ interface BatAdsClient {
   LoadSampleBundle() => (int32 result, string value);
   Reset(string name) => (int32 result);
   GetCreativeAdNotifications(array<string> categories) => (int32 result, array<string> categories, array<string> json_list);
-  GetAdConversions(string url) => (int32 result, string url, array<string> json_list);
+  GetAdConversions() => (int32 result, array<string> 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
@@ -53,7 +53,7 @@ using OnGetCreativeAdNotificationsCallback = std::function<void(const Result,
     const std::vector<std::string>&, const CreativeAdNotificationList&)>;
 
 using OnGetAdConversionsCallback = std::function<void(const Result,
-    const std::string&, const AdConversionList&)>;
+    const AdConversionList&)>;
 
 using OnLoadSampleBundleCallback = std::function<void(const Result,
     const std::string&)>;
@@ -240,13 +240,11 @@ class ADS_EXPORT AdsClient {
       const std::vector<std::string>& 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
@@ -127,8 +127,7 @@ class MockAdsClient : public AdsClient {
       const std::vector<std::string>& 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
@@ -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
@@ -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
@@ -396,12 +396,12 @@ - (void)getCreativeAdNotifications:(const std::vector<std::string> &)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<ads::IssuersInfo>)info

diff --git 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<ads::BundleState> state, ads::OnSaveCallback callback) override;
   void GetCreativeAdNotifications(const std::vector<std::string> & 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<ads::LogStream> 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
@@ -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
@@ -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<std::string> &)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;