From 7ecb72413d5a55f9a56a6a3036ccbcf4c8687ca2 Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Wed, 5 Jul 2023 08:45:14 -0400 Subject: [PATCH] Allow LOW_IMPRESSION_COUNT and related values set by ENV (#19678) * Allow LOW_IMPRESSION_COUNT and related values set by ENV * Update app/models/display_ad.rb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update app/models/display_ad.rb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update app/models/display_ad.rb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Adjust test and move methods to non-private * Change fallback numbers to constants * Add global config fallbacks * Update spec/models/display_ad_spec.rb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update app/models/display_ad.rb Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- app/models/display_ad.rb | 34 +++++++++++++++++++++++------ spec/models/display_ad_spec.rb | 39 ++++++++++++++++++++++++++++------ 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/app/models/display_ad.rb b/app/models/display_ad.rb index d3f45b62c..f2c396664 100644 --- a/app/models/display_ad.rb +++ b/app/models/display_ad.rb @@ -22,8 +22,9 @@ class DisplayAd < ApplicationRecord POST_WIDTH = 775 SIDEBAR_WIDTH = 350 LOW_IMPRESSION_COUNT = 1_000 - RARELY = (0...5) # 5 percent chance - SELDOM = (5...35) # 30 percent chance + RANDOM_RANGE_MAX_FALLBACK = 5 + NEW_AND_PRIORITY_RANGE_MAX_FALLBACK = 35 + enum display_to: { all: 0, logged_in: 1, logged_out: 2 }, _prefix: true enum type_of: { in_house: 0, community: 1, external: 2 } @@ -54,7 +55,7 @@ class DisplayAd < ApplicationRecord search: "%#{term}%" } - scope :seldom_seen, -> { where("impressions_count < ?", LOW_IMPRESSION_COUNT).or(where(priority: true)) } + scope :seldom_seen, ->(area) { where("impressions_count < ?", low_impression_count(area)).or(where(priority: true)) } def self.for_display(area:, user_signed_in:, user_id: nil, article: nil) permit_adjacent = article ? article.permit_adjacent_sponsors? : true @@ -70,16 +71,16 @@ class DisplayAd < ApplicationRecord ) case rand(99) # output integer from 0-99 - when RARELY # smallest range, 5% + when (0..random_range_max(area)) # smallest range, 5% # We are always showing more of the good stuff — but we are also always testing the system to give any a chance to # rise to the top. 5 out of every 100 times we show an ad (5%), it is totally random. This gives "not yet # evaluated" stuff a chance to get some engagement and start showing up more. If it doesn't get engagement, it # stays in this area. ads_for_display.sample - when SELDOM # medium range, 30% + 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). - ads_for_display.seldom_seen.sample || ads_for_display.sample + ads_for_display.seldom_seen(area).sample || ads_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 @@ -138,6 +139,27 @@ class DisplayAd < ApplicationRecord write_attribute :exclude_article_ids, (adjusted_input || []) end + # Temporary ENV configs, to eventually be replaced by permanent configurations + # once we determine what the appropriate long-term config approach is. + + def self.low_impression_count(placement_area) + ApplicationConfig["LOW_IMPRESSION_COUNT_FOR_#{placement_area.upcase}"] || + ApplicationConfig["LOW_IMPRESSION_COUNT"] || + LOW_IMPRESSION_COUNT + end + + def self.random_range_max(placement_area) + ApplicationConfig["SELDOM_SEEN_MIN_FOR_#{placement_area.upcase}"] || + ApplicationConfig["SELDOM_SEEN_MIN"] || + RANDOM_RANGE_MAX_FALLBACK + end + + def self.new_and_priority_range_max(placement_area) + ApplicationConfig["SELDOM_SEEN_MAX_FOR_#{placement_area.upcase}"]|| + ApplicationConfig["SELDOM_SEEN_MAX"] || + NEW_AND_PRIORITY_RANGE_MAX_FALLBACK + end + private def generate_display_ad_name diff --git a/spec/models/display_ad_spec.rb b/spec/models/display_ad_spec.rb index 9408457ab..eb0d2d5e3 100644 --- a/spec/models/display_ad_spec.rb +++ b/spec/models/display_ad_spec.rb @@ -340,19 +340,44 @@ RSpec.describe DisplayAd do let!(:priority_ad) { create(:display_ad, priority: true, impressions_count: low_impression_count + 1) } it "includes ads with impressions count less than LOW_IMPRESSION_COUNT" do - expect(described_class.seldom_seen).to include(low_impression_ad) + expect(described_class.seldom_seen("sidebar_left")).to include(low_impression_ad) + end + + it "does not include ads with impression count more than env area override of LOW_IMPRESSION_COUNT" do + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT_FOR_SIDEBAR_LEFT").and_return("990") + expect(described_class.seldom_seen("sidebar_left")).not_to include(low_impression_ad) + end + + it "includes ads with impression count less than than env area override of LOW_IMPRESSION_COUNT" do + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT_FOR_SIDEBAR_LEFT").and_return("1010") + expect(described_class.seldom_seen("sidebar_left")).to include(low_impression_ad) + end + + it "does not include ads with impression count more than env GLOBAL override of LOW_IMPRESSION_COUNT" do + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT").and_return("990") + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT_FOR_SIDEBAR_LEFT").and_return(nil) + expect(described_class.seldom_seen("sidebar_left")).not_to include(low_impression_ad) + end + + it "includes ads with impression count less than than env GLOBAL override of LOW_IMPRESSION_COUNT" do + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT").and_return("1010") + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT_FOR_SIDEBAR_LEFT").and_return(nil) + expect(described_class.seldom_seen("sidebar_left")).to include(low_impression_ad) + end + + it "ignores override of LOW_IMPRESSION_COUNT to a different location" do + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT_FOR_POST_COMMENTS").and_return("990") + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT_FOR_SIDEBAR_LEFT").and_return(nil) + allow(ApplicationConfig).to receive(:[]).with("LOW_IMPRESSION_COUNT").and_return(nil) + expect(described_class.seldom_seen("sidebar_left")).to include(low_impression_ad) end it "excludes ads with impressions count greater than or equal to LOW_IMPRESSION_COUNT" do - expect(described_class.seldom_seen).not_to include(high_impression_ad) - end - - it "includes ads with priority set to true" do - expect(described_class.seldom_seen).to include(priority_ad) + expect(described_class.seldom_seen("sidebar_left")).not_to include(high_impression_ad) end it "includes both priority to be proper size when two qualifying ads exist" do - expect(described_class.seldom_seen.size).to be 2 + expect(described_class.seldom_seen("sidebar_left").size).to be 2 end end end