From 1078e7a5b18c45827510ee667b3a9a4bf37a46e2 Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Fri, 13 Sep 2024 10:54:38 +0100 Subject: [PATCH] Fixes [ads] Toggle between Likes and Dislikes in 30-Day history is broken --- .../user_engagement/reactions/reactions.cc | 21 +++++++++-- .../reactions/reactions_type_util.cc | 37 ++++++++++++++++--- .../reactions/reactions_type_util_unittest.cc | 8 ++-- .../reactions/reactions_unittest.cc | 6 +-- 4 files changed, 55 insertions(+), 17 deletions(-) diff --git a/components/brave_ads/core/internal/user_engagement/reactions/reactions.cc b/components/brave_ads/core/internal/user_engagement/reactions/reactions.cc index 323840eb7aa8..527d8fab9dbb 100644 --- a/components/brave_ads/core/internal/user_engagement/reactions/reactions.cc +++ b/components/brave_ads/core/internal/user_engagement/reactions/reactions.cc @@ -36,8 +36,10 @@ void Reactions::ToggleLikeAd(mojom::ReactionInfoPtr mojom_reaction, const mojom::ReactionType toggled_mojom_reaction_type = ToggleLikedReactionType(mojom_reaction_type); if (toggled_mojom_reaction_type == mojom::ReactionType::kNeutral) { + // Neutral reaction, so remove it. ad_reactions_.erase(advertiser_id); } else { + // Non-neutral reaction, so set it. ad_reactions_[advertiser_id] = toggled_mojom_reaction_type; } SetProfileDictPref(prefs::kAdReactions, ReactionMapToDict(ad_reactions_)); @@ -63,8 +65,10 @@ void Reactions::ToggleDislikeAd(mojom::ReactionInfoPtr mojom_reaction, const mojom::ReactionType toggled_mojom_reaction_type = ToggleDislikedReactionType(mojom_reaction_type); if (toggled_mojom_reaction_type == mojom::ReactionType::kNeutral) { + // Neutral reaction, so remove it. ad_reactions_.erase(advertiser_id); } else { + // Non-neutral reaction, so set it. ad_reactions_[advertiser_id] = toggled_mojom_reaction_type; } SetProfileDictPref(prefs::kAdReactions, ReactionMapToDict(ad_reactions_)); @@ -82,8 +86,10 @@ mojom::ReactionType Reactions::AdReactionTypeForId( const std::string& advertiser_id) const { const auto iter = ad_reactions_.find(advertiser_id); if (iter == ad_reactions_.cend()) { + // No reaction, so neutral. return mojom::ReactionType::kNeutral; } + // Reaction found. const auto& [_, reaction_type] = *iter; return reaction_type; } @@ -100,8 +106,10 @@ void Reactions::ToggleLikeSegment(mojom::ReactionInfoPtr mojom_reaction, const mojom::ReactionType toggled_mojom_reaction_type = ToggleLikedReactionType(mojom_reaction_type); if (toggled_mojom_reaction_type == mojom::ReactionType::kNeutral) { + // Neutral reaction, so remove it. segment_reactions_.erase(segment); } else { + // Non-neutral reaction, so set it. segment_reactions_[segment] = toggled_mojom_reaction_type; } SetProfileDictPref(prefs::kSegmentReactions, @@ -126,8 +134,10 @@ void Reactions::ToggleDislikeSegment(mojom::ReactionInfoPtr mojom_reaction, const mojom::ReactionType toggled_mojom_reaction_type = ToggleDislikedReactionType(mojom_reaction_type); if (toggled_mojom_reaction_type == mojom::ReactionType::kNeutral) { + // Neutral reaction, so remove it. segment_reactions_.erase(segment); } else { + // Non-neutral reaction, so set it. segment_reactions_[segment] = toggled_mojom_reaction_type; } SetProfileDictPref(prefs::kSegmentReactions, @@ -144,6 +154,7 @@ mojom::ReactionType Reactions::SegmentReactionTypeForId( const std::string& segment) const { const auto iter = segment_reactions_.find(segment); if (iter == segment_reactions_.cend()) { + // No reaction, so neutral. return mojom::ReactionType::kNeutral; } const auto [_, reaction_type] = *iter; @@ -156,10 +167,11 @@ void Reactions::ToggleSaveAd(mojom::ReactionInfoPtr mojom_reaction, return std::move(callback).Run(/*success=*/false); } - const auto [iterator, inserted] = + const auto [iter, inserted] = saved_ads_.insert(mojom_reaction->creative_instance_id); if (!inserted) { - saved_ads_.erase(iterator); + // Already saved, so unsave it. + saved_ads_.erase(iter); } SetProfileListPref(prefs::kSaveAds, ReactionSetToList(saved_ads_)); @@ -183,10 +195,11 @@ void Reactions::ToggleMarkAdAsInappropriate( return std::move(callback).Run(/*success=*/false); } - const auto [iterator, inserted] = + const auto [iter, inserted] = marked_as_inappropriate_.insert(mojom_reaction->creative_set_id); if (!inserted) { - marked_as_inappropriate_.erase(iterator); + // Already marked as inappropriate, so unmark it. + marked_as_inappropriate_.erase(iter); } SetProfileListPref(prefs::kMarkedAsInappropriate, ReactionSetToList(marked_as_inappropriate_)); diff --git a/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util.cc b/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util.cc index 0b4f0b396496..88f0a7d647a2 100644 --- a/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util.cc +++ b/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util.cc @@ -5,22 +5,47 @@ #include "brave/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util.h" +#include "base/notreached.h" #include "brave/components/brave_ads/core/mojom/brave_ads.mojom.h" namespace brave_ads { mojom::ReactionType ToggleLikedReactionType( const mojom::ReactionType mojom_reaction_type) { - return mojom_reaction_type == mojom::ReactionType::kNeutral - ? mojom::ReactionType::kLiked - : mojom::ReactionType::kNeutral; + switch (mojom_reaction_type) { + case mojom::ReactionType::kNeutral: + case mojom::ReactionType::kDisliked: { + // If the reaction is neutral or disliked, toggle to liked. + return mojom::ReactionType::kLiked; + } + + case mojom::ReactionType::kLiked: { + // If the reaction is liked, toggle to neutral. + return mojom::ReactionType::kNeutral; + } + } + + NOTREACHED_NORETURN() << "Unexpected value for mojom::ReactionType: " + << mojom_reaction_type; } mojom::ReactionType ToggleDislikedReactionType( const mojom::ReactionType mojom_reaction_type) { - return mojom_reaction_type == mojom::ReactionType::kNeutral - ? mojom::ReactionType::kDisliked - : mojom::ReactionType::kNeutral; + switch (mojom_reaction_type) { + case mojom::ReactionType::kNeutral: + case mojom::ReactionType::kLiked: { + // If the reaction is neutral or liked, toggle to disliked. + return mojom::ReactionType::kDisliked; + } + + case mojom::ReactionType::kDisliked: { + // If the reaction is disliked, toggle to neutral. + return mojom::ReactionType::kNeutral; + } + } + + NOTREACHED_NORETURN() << "Unexpected value for mojom::ReactionType: " + << mojom_reaction_type; } } // namespace brave_ads diff --git a/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util_unittest.cc b/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util_unittest.cc index 5a8027495716..a715832dcaa8 100644 --- a/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util_unittest.cc +++ b/components/brave_ads/core/internal/user_engagement/reactions/reactions_type_util_unittest.cc @@ -29,9 +29,9 @@ TEST_F(BraveAdsReactionsTypeUtilTest, } TEST_F(BraveAdsReactionsTypeUtilTest, - ToggleLikedReactionTypeFromDislikedToNeutral) { + ToggleLikedReactionTypeFromDislikedToLiked) { // Act & Assert - EXPECT_EQ(mojom::ReactionType::kNeutral, + EXPECT_EQ(mojom::ReactionType::kLiked, ToggleLikedReactionType(mojom::ReactionType::kDisliked)); } @@ -50,9 +50,9 @@ TEST_F(BraveAdsReactionsTypeUtilTest, } TEST_F(BraveAdsReactionsTypeUtilTest, - ToggleDislikedReactionTypeFromLikedToNeutral) { + ToggleDislikedReactionTypeFromLikedToDisliked) { // Act & Assert - EXPECT_EQ(mojom::ReactionType::kNeutral, + EXPECT_EQ(mojom::ReactionType::kDisliked, ToggleDislikedReactionType(mojom::ReactionType::kLiked)); } diff --git a/components/brave_ads/core/internal/user_engagement/reactions/reactions_unittest.cc b/components/brave_ads/core/internal/user_engagement/reactions/reactions_unittest.cc index 8c16d8c9d837..677d1713a208 100644 --- a/components/brave_ads/core/internal/user_engagement/reactions/reactions_unittest.cc +++ b/components/brave_ads/core/internal/user_engagement/reactions/reactions_unittest.cc @@ -137,7 +137,7 @@ TEST_F(BraveAdsReactionsTest, Ads) { const ReactionMap expected_ad_reactions = { {test::kAdvertiserId, mojom::ReactionType::kLiked}, {"2c0577b2-097b-41e8-81db-685de60d26e5", mojom::ReactionType::kDisliked}, - }; + {"57b26e2f-2e77-4b31-a3ce-d3faf82dd574", mojom::ReactionType::kDisliked}}; EXPECT_EQ(expected_ad_reactions, GetReactions().Ads()); } @@ -218,8 +218,8 @@ TEST_F(BraveAdsReactionsTest, Segments) { // Act & Assert const ReactionMap expected_segment_reactions = { {test::kSegment, mojom::ReactionType::kLiked}, - {"food & drink", mojom::ReactionType::kDisliked}, - }; + {"technology & computing", mojom::ReactionType::kDisliked}, + {"food & drink", mojom::ReactionType::kDisliked}}; EXPECT_EQ(expected_segment_reactions, GetReactions().Segments()); }