Add priority weight to billboards (#20019)

* Add priority weight to billboards

* Rewrite query as activerecord

* Change naming from priority_weight to just weight

app/controllers/api/v1/billboards_controller.rb

@@ -49,7 +49,7 @@ 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,
+                      :tag_list, :type_of, :exclude_article_ids, :weight,
                       :audience_segment_type, :audience_segment_id, :priority,
                       # Permitting twice allows both comma-separated string and array values
                       :target_geolocations, target_geolocations: []

app/models/billboard.rb

@@ -38,6 +38,7 @@ class Billboard < ApplicationRecord
                              inclusion: { in: ALLOWED_PLACEMENT_AREAS }
   validates :body_markdown, presence: true
   validates :organization, presence: true, if: :community?
+  validates :weight, numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: 10_000 }
   validates :audience_segment_type,
             inclusion: { in: AudienceSegment.type_ofs },
             allow_blank: true
@@ -88,7 +89,8 @@ class Billboard < ApplicationRecord
     when (random_range_max(area)..new_and_priority_range_max(area)) # medium range, 30%
       # Here we sample from only billboards with fewer than 1000 impressions (with a fallback
       # if there are none of those, causing an extra query, but that shouldn't happen very often).
-      billboards_for_display.seldom_seen(area).sample || billboards_for_display.sample
+      relation = billboards_for_display.seldom_seen(area)
+      weighted_random_selection(relation) || billboards_for_display.sample
     else # large range, 65%
 
       # Ads that get engagement have a higher "success rate", and among this category, we sample from the top 15 that
@@ -101,6 +103,27 @@ class Billboard < ApplicationRecord
     end
   end
 
+  def self.weighted_random_selection(relation)
+    base_query = relation.to_sql
+    random_val = rand.to_f
+
+    query = <<-SQL
+      WITH base AS (#{base_query}),
+      weighted AS (
+        SELECT *, weight,
+        SUM(weight) OVER () AS total_weight,
+        SUM(weight) OVER (ORDER BY id ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) AS running_weight
+        FROM base
+      )
+      SELECT *, running_weight, ? * total_weight AS random_value FROM weighted
+      WHERE running_weight >= ? * total_weight
+      ORDER BY running_weight ASC
+      LIMIT 1
+    SQL
+
+    relation.find_by_sql([query, random_val, random_val]).first
+  end
+
   # Temporary ENV configs, to eventually be replaced by permanent configurations
   # once we determine what the appropriate long-term config approach is.
 

db/migrate/20230831160030_add_priority_weight_to_billboards.rb

@@ -0,0 +1,5 @@
+class AddPriorityWeightToBillboards < ActiveRecord::Migration[7.0]
+  def change
+    add_column :display_ads, :weight, :float, default: 1.0, null: false
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_08_03_133228) do
+ActiveRecord::Schema[7.0].define(version: 2023_08_31_160030) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
   enable_extension "ltree"
@@ -483,6 +483,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_08_03_133228) do
     t.ltree "target_geolocations", default: [], array: true
     t.integer "type_of", default: 0, null: false
     t.datetime "updated_at", precision: nil, null: false
+    t.float "weight", default: 1.0, null: false
     t.index ["cached_tag_list"], name: "index_display_ads_on_cached_tag_list", opclass: :gin_trgm_ops, using: :gin
     t.index ["exclude_article_ids"], name: "index_display_ads_on_exclude_article_ids", using: :gin
     t.index ["placement_area"], name: "index_display_ads_on_placement_area"

spec/factories/billboards.rb

@@ -4,5 +4,6 @@ FactoryBot.define do
     sequence(:body_markdown) { |n| "Hello _hey_ Hey hey #{n}" }
     organization
     priority { false }
+    weight { 1.0 }
   end
 end

spec/models/billboard_spec.rb

@@ -493,4 +493,39 @@ RSpec.describe Billboard do
       expect(described_class.seldom_seen("sidebar_left").size).to be 2
     end
   end
+
+  describe ".weighted_random_selection" do
+    it "samples with weights correctly" do
+      described_class.delete_all
+      bb1 = create(:billboard, weight: 5)
+      bb2 = create(:billboard, weight: 1)
+      bb3 = create(:billboard, weight: 1)
+      bb4 = create(:billboard, weight: 2)
+      bb5 = create(:billboard, weight: 1)
+
+      total_weight = 5 + 1 + 1 + 2 + 1 # 10
+      expected_probabilities = {
+        bb1.id => 5.0 / total_weight,
+        bb2.id => 1.0 / total_weight,
+        bb3.id => 1.0 / total_weight,
+        bb4.id => 2.0 / total_weight,
+        bb5.id => 1.0 / total_weight
+      }
+
+      counts = Hash.new(0)
+      num_trials = 5_000
+
+      num_trials.times do
+        id = described_class.weighted_random_selection(described_class.all).id
+        counts[id] += 1
+      end
+
+      counts.each do |id, count|
+        observed_probability = count.to_f / num_trials
+        expected_probability = expected_probabilities[id]
+
+        expect(observed_probability).to be_within(0.025).of(expected_probability)
+      end
+    end
+  end
 end

spec/requests/api/v1/billboards_spec.rb

@@ -52,7 +52,7 @@ RSpec.describe "Api::V1::Billboards" do
                           "success_rate", "tag_list", "type_of", "updated_at",
                           "creator_id", "exclude_article_ids",
                           "audience_segment_type", "audience_segment_id",
-                          "priority", "target_geolocations")
+                          "priority", "weight", "target_geolocations")
         expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC")
       end
 
@@ -71,7 +71,7 @@ RSpec.describe "Api::V1::Billboards" do
                           "success_rate", "tag_list", "type_of", "updated_at",
                           "creator_id", "exclude_article_ids",
                           "audience_segment_type", "audience_segment_id",
-                          "priority", "target_geolocations")
+                          "priority", "weight", "target_geolocations")
         expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC")
       end
 
@@ -109,6 +109,7 @@ RSpec.describe "Api::V1::Billboards" do
           "published" => true,
           "approved" => true,
           "priority" => false,
+          "weight" => 1.0,
           "type_of" => "in_house",
           "cached_tag_list" => "",
           "clicks_count" => 0,
@@ -135,7 +136,7 @@ RSpec.describe "Api::V1::Billboards" do
                           "success_rate", "tag_list", "type_of", "updated_at",
                           "creator_id", "exclude_article_ids",
                           "audience_segment_type", "audience_segment_id",
-                          "priority", "target_geolocations")
+                          "priority", "weight", "target_geolocations")
       end
 
       it "also accepts target geolocations as an array" do