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 <lightallloy@gmail.com>

* Rubocop

* Tidy up type matching logic

---------

Co-authored-by: Anna Buianova <lightallloy@gmail.com>

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

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