From 197b7f712826532a28ad1b00c6e12bcd89f85769 Mon Sep 17 00:00:00 2001 From: Joshua Wehner Date: Mon, 3 Apr 2023 14:44:35 +0200 Subject: [PATCH] Update FilteredAdQuery to better support community ads (#19281) * Update FilteredAdQuery to better support community ads * Maybe organization_id could be blank? * Try an explanatory comment for filter ordering * Update app/queries/display_ads/filtered_ads_query.rb Co-authored-by: Anna Buianova * Rubocop * Tidy up type matching logic --------- Co-authored-by: Anna Buianova --- app/queries/display_ads/filtered_ads_query.rb | 36 ++++++++++--------- .../display_ads/filtered_ads_query_spec.rb | 12 ++++--- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/app/queries/display_ads/filtered_ads_query.rb b/app/queries/display_ads/filtered_ads_query.rb index 23d7be96d..fe125b7e0 100644 --- a/app/queries/display_ads/filtered_ads_query.rb +++ b/app/queries/display_ads/filtered_ads_query.rb @@ -32,6 +32,9 @@ module DisplayAds authenticated_ads(%w[all logged_out]) end + # type_of filter needs to be applied as near to the end as possible + # as it checks if any type-matching ads exist (this will apply all/any + # filters applied up to this point, thus near the end is best) @filtered_display_ads = type_of_ads @filtered_display_ads = @filtered_display_ads.order(success_rate: :desc) @@ -61,25 +64,26 @@ module DisplayAds end def type_of_ads + # If this is an organization article and community-type ads exist, show them + if @organization_id.present? + community = @filtered_display_ads.where(type_of: DisplayAd.type_ofs[:community], + organization_id: @organization_id) + return community if community.any? + end + + types_matching = [] + # Always match in-house-type ads - in_house = "(type_of = :in_house)" + types_matching << :in_house - # If this is an article that belongs to an organization, we might show community-type ads - community = if @organization_id - "(type_of = :community AND organization_id = :organization_id)" - end + # If the article is an organization's article (non-nil organization_id), + # or if the current_user has opted-out of sponsors, + # then do not show external ads + if @organization_id.blank? && @permit_adjacent_sponsors + types_matching << :external + end - # If the article's author permits adjacent sponsors, we might show an external-type ad - # *if* the organization_id doesn't match the current article's organization id - external = if @permit_adjacent_sponsors && @organization_id - "(type_of = :external AND organization_id != :organization_id)" - elsif @permit_adjacent_sponsors - "(type_of = :external)" - end - - types_matching = [in_house, community, external].compact.join(" OR ") - @filtered_display_ads.where(types_matching, - DisplayAd.type_ofs.merge({ organization_id: @organization_id })) + @filtered_display_ads.where(type_of: DisplayAd.type_ofs.slice(*types_matching).values) end end end diff --git a/spec/queries/display_ads/filtered_ads_query_spec.rb b/spec/queries/display_ads/filtered_ads_query_spec.rb index 0e76b6c02..09e4cb5d9 100644 --- a/spec/queries/display_ads/filtered_ads_query_spec.rb +++ b/spec/queries/display_ads/filtered_ads_query_spec.rb @@ -86,19 +86,23 @@ RSpec.describe DisplayAds::FilteredAdsQuery, type: :query do let!(:external_ad) { create_display_ad organization_id: organization.id, type_of: :external } let!(:other_external) { create_display_ad organization_id: other_org.id, type_of: :external } - it "always shows :in_house, only shows community/external appropriately" do + it "always shows :community ad if matching, otherwise shows in_house/external" do filtered = filter_ads organization_id: organization.id - expect(filtered).to contain_exactly(community_ad, in_house_ad, other_external) + expect(filtered).to contain_exactly(community_ad) + expect(filtered).not_to include(other_community) + + filtered = filter_ads organization_id: 123 + expect(filtered).to contain_exactly(in_house_ad) expect(filtered).not_to include(other_community) filtered = filter_ads organization_id: nil expect(filtered).to contain_exactly(in_house_ad, external_ad, other_external) - expect(filtered).not_to include(other_community) + expect(filtered).not_to include(community_ad, other_community) end it "suppresses external ads when permit_adjacent_sponsors is false" do filtered = filter_ads organization_id: organization.id, permit_adjacent_sponsors: false - expect(filtered).to contain_exactly(community_ad, in_house_ad) + expect(filtered).to contain_exactly(community_ad) expect(filtered).not_to include(other_community) filtered = filter_ads organization_id: nil, permit_adjacent_sponsors: false