Add ability to assign arbitrary segment ID to /display_ads API (#19529)

* DisplayAd audience_segment_id in API

* Rubocop

* Manual not valid if missing id

* manual-type requires an id

* Better audience_segment_type reader

* Use inclusion validation

* Swagger update

* Remove unnecessary

app/controllers/api/v1/display_ads_controller.rb

@@ -52,7 +52,8 @@ module Api
       def permitted_params
         params.permit :approved, :body_markdown, :creator_id, :display_to,
                       :name, :organization_id, :placement_area, :published,
-                      :tag_list, :type_of, :exclude_article_ids, :audience_segment_type
+                      :tag_list, :type_of, :exclude_article_ids,
+                      :audience_segment_type, :audience_segment_id
       end
     end
   end

app/models/display_ad.rb

@@ -35,8 +35,13 @@ class DisplayAd < ApplicationRecord
                              inclusion: { in: ALLOWED_PLACEMENT_AREAS }
   validates :body_markdown, presence: true
   validates :organization, presence: true, if: :community?
-  validate :validate_tag
-  validate :validate_in_house_hero_ads
+  validates :audience_segment_type,
+            inclusion: { in: AudienceSegment.type_ofs },
+            allow_blank: true
+  validate :valid_audience_segment_match,
+           :validate_in_house_hero_ads,
+           :valid_manual_audience_segment,
+           :validate_tag
 
   before_save :process_markdown
   after_save :generate_display_ad_name
@@ -104,23 +109,24 @@ class DisplayAd < ApplicationRecord
     errors.add(:type_of, "must be in_house if display ad is a Home Hero")
   end
 
+  def audience_segment_type
+    @audience_segment_type ||= audience_segment&.type_of
+  end
+
   def audience_segment_type=(type)
-    self.audience_segment = if type.blank?
-                              nil
-                            elsif (segment = AudienceSegment.find_by(type_of: type))
-                              segment
-                            end
+    errors.delete(:audience_segment_type)
+    @audience_segment_type = type
   end
 
   # This needs to correspond with Rails built-in method signature
   # rubocop:disable Style/OptionHash
   def as_json(options = {})
     overrides = {
-      "audience_segment_type" => audience_segment&.type_of,
+      "audience_segment_type" => audience_segment_type,
       "tag_list" => cached_tag_list,
       "exclude_article_ids" => exclude_article_ids.join(",")
     }
-    super(options.merge(except: %i[tags tag_list audience_segment_id])).merge(overrides)
+    super(options.merge(except: %i[tags tag_list])).merge(overrides)
   end
   # rubocop:enable Style/OptionHash
 
@@ -187,4 +193,16 @@ class DisplayAd < ApplicationRecord
       audience_segment &&
       audience_segment.updated_at < 1.day.ago
   end
+
+  def valid_audience_segment_match
+    return if audience_segment.blank? || audience_segment_type.blank?
+
+    errors.add(:audience_segment_type) if audience_segment.type_of.to_s != audience_segment_type.to_s
+  end
+
+  def valid_manual_audience_segment
+    return if audience_segment_type != "manual"
+
+    errors.add(:audience_segment_type) if audience_segment.blank?
+  end
 end

spec/models/display_ad_spec.rb

@@ -3,6 +3,7 @@ require "rails_helper"
 RSpec.describe DisplayAd do
   let(:organization) { build(:organization) }
   let(:display_ad) { build(:display_ad, organization: nil) }
+  let(:audience_segment) { create(:audience_segment) }
 
   before { allow(FeatureFlag).to receive(:enabled?).with(:consistent_rendering, any_args).and_return(true) }
 
@@ -75,6 +76,35 @@ RSpec.describe DisplayAd do
       expect(display_ad).to be_valid
       expect(display_ad.errors[:organization]).to be_blank
     end
+
+    it "allows clear audience segment" do
+      display_ad.audience_segment_id = audience_segment.id
+      expect(display_ad).to be_valid
+
+      display_ad.audience_segment_id = audience_segment.id
+      display_ad.audience_segment_type = audience_segment.type_of
+      expect(display_ad).to be_valid
+    end
+
+    it "disallows invalid audience segment" do
+      display_ad.audience_segment_id = audience_segment.id
+      display_ad.audience_segment_type = "something_invalid_here"
+      expect(audience_segment.type_of).not_to eq("something_invalid_here")
+      expect(display_ad).not_to be_valid
+    end
+
+    it "disallows valid but ambiguous audience segment & id mismatch" do
+      display_ad.audience_segment_id = audience_segment.id
+      display_ad.audience_segment_type = "posted"
+      expect(audience_segment.type_of).not_to eq("posted")
+      expect(display_ad).not_to be_valid
+    end
+
+    it "disallows valid but imprecise manual audience segment type" do
+      display_ad.audience_segment = nil
+      display_ad.audience_segment_type = "manual"
+      expect(display_ad).not_to be_valid
+    end
   end
 
   context "when parsing liquid tags" do

spec/models/organization_spec.rb

@@ -354,7 +354,7 @@ RSpec.describe Organization do
       expect(organization.public_articles_count).to eq(0)
     end
   end
-  
+
   describe ".simple_name_match" do
     before do
       create(:organization, name: "Not Matching")

spec/requests/api/v1/display_ads_spec.rb

@@ -49,7 +49,8 @@ RSpec.describe "Api::V1::DisplayAds" do
                           "impressions_count", "name", "organization_id",
                           "placement_area", "processed_html", "published",
                           "success_rate", "tag_list", "type_of", "updated_at",
-                          "creator_id", "exclude_article_ids", "audience_segment_type")
+                          "creator_id", "exclude_article_ids",
+                          "audience_segment_type", "audience_segment_id")
       end
 
       it "returns a malformed response" do
@@ -95,7 +96,8 @@ RSpec.describe "Api::V1::DisplayAds" do
                           "impressions_count", "name", "organization_id",
                           "placement_area", "processed_html", "published",
                           "success_rate", "tag_list", "type_of", "updated_at",
-                          "creator_id", "exclude_article_ids", "audience_segment_type")
+                          "creator_id", "exclude_article_ids",
+                          "audience_segment_type", "audience_segment_id")
       end
     end
 

spec/swagger_helper.rb

@@ -415,9 +415,11 @@ The default maximum value can be overridden by \"API_PER_PAGE_MAX\" environment
               article_exclude_ids: { type: :string,
                                      nullable: true,
                                      description: "Articles this ad should *not* appear on (blank means no articles are disallowed, and this ad can appear next to any/all articles). Comma-separated list of integer Article IDs" }, # rubocop:disable Layout/LineLength
+              audience_segment_id: { type: :integer,
+                                     description: "Specifies a specific audience segment who will see this billboard" },
               audience_segment_type: { type: :string,
                                        enum: AudienceSegment.type_ofs.keys,
-                                       description: "Specifies an group of users to show this ad to (only works with logged-in users)" },
+                                       description: "Specifies a group of users who will see this billboard (must match audience_segment_id if both provided)" },
               display_to: { type: :string, enum: DisplayAd.display_tos.keys, default: "all",
                             description: "Potentially limits visitors to whom the ad is visible" },
               type_of: { type: :string, enum: DisplayAd.type_ofs.keys, default: "in_house",