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