Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes Ads History popup shows 2 entries for the same ad (even if only served once) #3960

Merged
merged 1 commit into from
Nov 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions components/services/bat_ads/bat_ads_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,13 +159,14 @@ void BatAdsImpl::GetAdsHistory(
GetAdsHistoryCallback callback) {
std::map<uint64_t, std::vector<std::string>> result;

auto ads_history_map = ads_->GetAdsHistory();
for (const auto& entry : ads_history_map) {
std::vector<std::string> ads_history;
for (const auto& ads_history_entry : entry.second) {
ads_history.push_back(ads_history_entry.ToJson());
auto ads_histories = ads_->GetAdsHistory(
ads::AdsHistoryFilterType::kConfirmationType);
for (const auto& ads_history : ads_histories) {
std::vector<std::string> ads_history_json;
for (const auto& ads_history_entry : ads_history.second) {
ads_history_json.push_back(ads_history_entry.ToJson());
}
result[entry.first] = ads_history;
result[ads_history.first] = ads_history_json;
}

std::move(callback).Run(mojo::MapToFlatMap(result));
Expand Down
1 change: 1 addition & 0 deletions test/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ test("brave_unit_tests") {
"//brave/components/brave_ads/browser/ads_service_impl_unittest.cc",
"//brave/vendor/bat-native-ads/src/bat/ads/internal/client_mock.h",
"//brave/vendor/bat-native-ads/src/bat/ads/internal/client_mock.cc",
"//brave/vendor/bat-native-ads/src/bat/ads/internal/filters/ad_history_confirmation_filter_unittest.cc",
"//brave/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/daily_cap_frequency_cap_unittest.cc",
"//brave/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_day_frequency_cap_unittest.cc",
"//brave/vendor/bat-native-ads/src/bat/ads/internal/frequency_capping/exclusion_rules/per_hour_frequency_cap_unittest.cc",
Expand Down
3 changes: 3 additions & 0 deletions vendor/bat-native-ads/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ source_set("ads") {
"src/bat/ads/internal/filtered_category.h",
"src/bat/ads/internal/flagged_ad.cc",
"src/bat/ads/internal/flagged_ad.h",
"src/bat/ads/internal/filters/ad_history_filter.h",
"src/bat/ads/internal/filters/ad_history_confirmation_filter.h",
"src/bat/ads/internal/filters/ad_history_confirmation_filter.cc",
"src/bat/ads/internal/frequency_capping/exclusion_rule.h",
"src/bat/ads/internal/frequency_capping/exclusion_rules/daily_cap_frequency_cap.cc",
"src/bat/ads/internal/frequency_capping/exclusion_rules/daily_cap_frequency_cap.h",
Expand Down
6 changes: 3 additions & 3 deletions vendor/bat-native-ads/include/bat/ads/ads.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@
#include "bat/ads/notification_event_type.h"
#include "bat/ads/notification_info.h"
#include "bat/ads/public/interfaces/ads.mojom.h"
#include "bat/ads/ads_history.h"

namespace ads {

struct AdsHistory;

using Environment = mojom::Environment;

using InitializeCallback = std::function<void(const Result)>;
Expand Down Expand Up @@ -206,7 +205,8 @@ class ADS_EXPORT Ads {

// Should be called to get ads history. Returns |std::map<uint64_t,
// std::vector<AdsHistory>>| in the format |{timestamp, array<AdsHistory>}|
virtual std::map<uint64_t, std::vector<AdsHistory>> GetAdsHistory() = 0;
virtual std::map<uint64_t, std::vector<AdsHistory>> GetAdsHistory(
const AdsHistoryFilterType ad_history_filterype) = 0;

// Should be called to indicate interest in the specified ad. This is a
// toggle, so calling it again returns the setting to the neutral state
Expand Down
5 changes: 5 additions & 0 deletions vendor/bat-native-ads/include/bat/ads/ads_history.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ namespace ads {

struct AdHistoryDetail;

enum class AdsHistoryFilterType {
kNone = 0,
kConfirmationType
};

struct ADS_EXPORT AdsHistory {
AdsHistory();
explicit AdsHistory(const AdsHistory& history);
Expand Down
20 changes: 19 additions & 1 deletion vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "bat/ads/internal/frequency_capping/permission_rules/minimum_wait_time_frequency_cap.h"
#include "bat/ads/internal/frequency_capping/permission_rules/ads_per_day_frequency_cap.h"
#include "bat/ads/internal/frequency_capping/permission_rules/ads_per_hour_frequency_cap.h"
#include "bat/ads/internal/filters/ad_history_confirmation_filter.h"

#include "rapidjson/document.h"
#include "rapidjson/error/en.h"
Expand Down Expand Up @@ -550,11 +551,28 @@ void AdsImpl::SetConfirmationsIsReady(
is_confirmations_ready_ = is_ready;
}

std::map<uint64_t, std::vector<AdsHistory>> AdsImpl::GetAdsHistory() {
std::map<uint64_t, std::vector<AdsHistory>> AdsImpl::GetAdsHistory(
const AdsHistoryFilterType ads_history_filter_type) {
std::map<uint64_t, std::vector<AdsHistory>> ads_history;
base::Time now = base::Time::Now().LocalMidnight();

auto ad_history_details = client_->GetAdsShownHistory();

std::unique_ptr<AdHistoryFilter> ad_history_filter;
switch (ads_history_filter_type) {
case AdsHistoryFilterType::kNone: {
break;
}
case AdsHistoryFilterType::kConfirmationType: {
ad_history_filter = std::make_unique<AdHistoryConfirmationFilter>();
break;
}
}

if (ad_history_filter) {
ad_history_details = ad_history_filter->ApplyFilter(ad_history_details);
}

for (auto& detail_item : ad_history_details) {
auto history_item = std::make_unique<AdsHistory>();
history_item->details.push_back(detail_item);
Expand Down
3 changes: 2 additions & 1 deletion vendor/bat-native-ads/src/bat/ads/internal/ads_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ class AdsImpl : public Ads {
void SetConfirmationsIsReady(
const bool is_ready) override;

std::map<uint64_t, std::vector<AdsHistory>> GetAdsHistory() override;
std::map<uint64_t, std::vector<AdsHistory>> GetAdsHistory(
const AdsHistoryFilterType ads_history_filter_type) override;
AdContent::LikeAction ToggleAdThumbUp(
const std::string& id,
const std::string& creative_set_id,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include <algorithm>
#include <memory>
#include <string>

#include "bat/ads/internal/filters/ad_history_confirmation_filter.h"
#include "bat/ads/confirmation_type.h"
#include "bat/ads/ad_history_detail.h"

#include "bat/ads/ads_history.h"

namespace ads {

bool IsConfirmationTypeOfInterest(
const ConfirmationType& confirmation_type) {
bool is_of_interest = false;

switch (confirmation_type.value()) {
case ConfirmationType::Value::CLICK:
case ConfirmationType::Value::VIEW:
case ConfirmationType::Value::DISMISS: {
is_of_interest = true;
break;
}
case ConfirmationType::Value::UNKNOWN:
case ConfirmationType::Value::LANDED:
case ConfirmationType::Value::FLAG:
case ConfirmationType::Value::UPVOTE:
case ConfirmationType::Value::DOWNVOTE: {
is_of_interest = false;
break;
}
}
return is_of_interest;
}

bool DoesConfirmationTypeATrumpB(
const ConfirmationType& confirmation_type_a,
const ConfirmationType& confirmation_type_b) {
bool does_type_a_trump_type_b = false;

switch (confirmation_type_a.value()) {
case ConfirmationType::Value::CLICK: {
switch (confirmation_type_b.value()) {
case ConfirmationType::Value::CLICK:
case ConfirmationType::Value::VIEW:
case ConfirmationType::Value::DISMISS: {
does_type_a_trump_type_b = true;
break;
}
default: {
break;
}
}
break;
}
case ConfirmationType::Value::VIEW: {
switch (confirmation_type_b.value()) {
case ConfirmationType::Value::VIEW:
case ConfirmationType::Value::DISMISS: {
does_type_a_trump_type_b = true;
break;
}
default: {
break;
}
}
break;
}
case ConfirmationType::Value::DISMISS: {
switch (confirmation_type_b.value()) {
case ConfirmationType::Value::DISMISS: {
does_type_a_trump_type_b = true;
break;
}
default: {
break;
}
}
break;
}
}

return does_type_a_trump_type_b;
}

AdHistoryConfirmationFilter::~AdHistoryConfirmationFilter() = default;

std::deque<AdHistoryDetail> AdHistoryConfirmationFilter::ApplyFilter(
const std::deque<AdHistoryDetail>& ad_history_details) const {

std::map<std::string, AdHistoryDetail> filtered_ad_history;

for (const AdHistoryDetail& ad_history_detail : ad_history_details) {
if (!IsConfirmationTypeOfInterest(ad_history_detail.ad_content.ad_action)) {
continue;
}
if (filtered_ad_history.count(ad_history_detail.ad_content.uuid) != 0) {
const AdHistoryDetail& check_ad_history_detail =
filtered_ad_history[ad_history_detail.ad_content.uuid];
tmancey marked this conversation as resolved.
Show resolved Hide resolved

if (ad_history_detail.timestamp_in_seconds >=
check_ad_history_detail.timestamp_in_seconds) {
if (DoesConfirmationTypeATrumpB(ad_history_detail.ad_content.ad_action,
check_ad_history_detail.ad_content.ad_action)) {
filtered_ad_history[ad_history_detail.ad_content.uuid] =
tmancey marked this conversation as resolved.
Show resolved Hide resolved
ad_history_detail;
}
}
} else {
filtered_ad_history[ad_history_detail.ad_content.uuid] =
tmancey marked this conversation as resolved.
Show resolved Hide resolved
ad_history_detail;
}
}

std::deque<AdHistoryDetail> filtered_ad_history_details;

for (const auto& ad_history : filtered_ad_history) {
filtered_ad_history_details.push_back(ad_history.second);
}

return filtered_ad_history_details;
}

} // namespace ads
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BAT_ADS_INTERNAL_AD_HISTORY_CONFIRMATION_FILTER_H_
#define BAT_ADS_INTERNAL_AD_HISTORY_CONFIRMATION_FILTER_H_

#include <deque>
#include <map>
#include <vector>

#include "bat/ads/internal/filters/ad_history_filter.h"

namespace ads {

struct AdsHistory;

class AdHistoryConfirmationFilter : public AdHistoryFilter {
public :
AdHistoryConfirmationFilter() = default;
~AdHistoryConfirmationFilter() override;

std::deque<AdHistoryDetail> ApplyFilter(
const std::deque<AdHistoryDetail>& ad_history_details) const override;
};

} // namespace ads

#endif // BAT_ADS_INTERNAL_AD_HISTORY_CONFIRMATION_FILTER_H_
Loading