Feature-flagged support for filtering billboards by location (#19834)

* add migrations

* add countries gem?

* first pass at location model

* specs ✨

* add location to filtered ads query

* feature flag and nicer errors

* cosmetic fixes

* more tweaks (remove unnecessary index, add initializer)

Gemfile

@@ -21,9 +21,11 @@ gem "blazer", "~> 2.6" # Allows admins to query data
 gem "bootsnap", ">= 1.1.0", require: false # Boot large ruby/rails apps faster
 gem "carrierwave", "~> 2.2" # Upload files in your Ruby applications, map them to a range of ORMs, store them on different backends
 gem "carrierwave-bombshelter", "~> 0.2" # Protect your carrierwave from image bombs
+gem "cgi", "~> 0.3.6" # Support for the Common Gateway Interface protocol.
 gem "cld3", "~> 3.5" # Ruby interface for Compact Language Detector v3
 gem "cloudinary", "~> 1.23" # Client library for easily using the Cloudinary service
 gem "counter_culture", "~> 3.2" # counter_culture provides turbo-charged counter caches that are kept up-to-date
+gem "countries", "~> 5.5" # All sorts of useful information about every country packaged as pretty little country objects. It includes data from ISO 3166
 gem "ddtrace", "~> 1.3.0" # ddtrace is Datadog’s tracing client for Ruby.
 gem "devise", "~> 4.8" # Flexible authentication solution for Rails
 gem "devise_invitable", "~> 2.0.6" # Allows invitations to be sent for joining
@@ -174,5 +176,3 @@ group :test do
   gem "with_model", "~> 2.1.6" # Dynamically build a model within an RSpec context
   gem "zonebie", "~> 0.6.1" # Runs your tests in a random timezone
 end
-
-gem "cgi", "~> 0.3.6" # Support for the Common Gateway Interface protocol.

Gemfile.lock

@@ -175,6 +175,8 @@ GEM
     counter_culture (3.4.0)
       activerecord (>= 4.2)
       activesupport (>= 4.2)
+    countries (5.5.0)
+      unaccent (~> 0.3)
     crack (0.4.5)
       rexml
     crass (1.0.6)
@@ -907,6 +909,7 @@ GEM
       concurrent-ruby (~> 1.0)
     uglifier (4.2.0)
       execjs (>= 0.3.0, < 3)
+    unaccent (0.4.0)
     unf (0.1.4)
       unf_ext
     unf_ext (0.0.8.2)
@@ -986,6 +989,7 @@ DEPENDENCIES
   cld3 (~> 3.5)
   cloudinary (~> 1.23)
   counter_culture (~> 3.2)
+  countries (~> 5.5)
   cuprite (~> 0.13)
   cypress-rails (~> 0.5)
   ddtrace (~> 1.3.0)

app/models/display_ad.rb

@@ -27,6 +27,7 @@ class DisplayAd < ApplicationRecord
   RANDOM_RANGE_MAX_FALLBACK = 5
   NEW_AND_PRIORITY_RANGE_MAX_FALLBACK = 35
 
+  attribute :target_geolocations, :geolocation_array
   enum display_to: { all: 0, logged_in: 1, logged_out: 2 }, _prefix: true
   enum type_of: { in_house: 0, community: 1, external: 2 }
 
@@ -43,7 +44,8 @@ class DisplayAd < ApplicationRecord
   validate :valid_audience_segment_match,
            :validate_in_house_hero_ads,
            :valid_manual_audience_segment,
-           :validate_tag
+           :validate_tag,
+           :validate_geolocations
 
   before_save :process_markdown
   after_save :generate_display_ad_name
@@ -128,6 +130,14 @@ class DisplayAd < ApplicationRecord
     validate_tag_name(tag_list)
   end
 
+  def validate_geolocations
+    target_geolocations.each do |geo|
+      unless geo.valid?
+        errors.add(:target_geolocations, I18n.t("models.billboard.invalid_location", location: geo.to_iso3166))
+      end
+    end
+  end
+
   def validate_in_house_hero_ads
     return unless placement_area == "home_hero" && type_of != "in_house"
 

app/models/geolocation.rb

@@ -0,0 +1,94 @@
+require Rails.root.join("lib/ISO3166/country") # so the class definition has access to extensions
+
+class Geolocation
+  class ArrayType < ActiveRecord::Type::Value
+    # Since an array can be modified directly, this module flags any field using this type as internally
+    # mutable (and provides different implementations of methods used internally by ActiveRecord to mark
+    # fields as changed/unchanged)
+    include ActiveModel::Type::Helpers::Mutable
+
+    def cast(codes)
+      return [] if codes.blank?
+
+      # Allows setting comma-separated string
+      codes = codes.split(/[\s,]+/) unless codes.is_a?(Array)
+
+      codes.map { |code| Geolocation.from_iso3166(code) }
+    end
+
+    def serialize(geo_locations)
+      PG::TextEncoder::Array.new.encode(cast(geo_locations).map(&:to_ltree))
+    end
+
+    def deserialize(geo_ltrees)
+      PG::TextDecoder::Array.new.decode(geo_ltrees).map { |geo_ltree| Geolocation.from_ltree(geo_ltree) }
+    end
+  end
+
+  include ActiveModel::Validations
+
+  FEATURE_FLAG = :billboard_location_targeting
+  ISO3166_SEPARATOR = "-".freeze
+  LTREE_SEPARATOR = ".".freeze
+  # Maybe this should be a config of some kind...?
+  PERMITTED_COUNTRIES = [
+    ISO3166::Country.code_from_name("United States"),
+    ISO3166::Country.code_from_name("Canada"),
+  ].freeze
+
+  attr_reader :country_code, :region_code
+
+  def self.from_iso3166(iso_3166)
+    parse(iso_3166, separator: ISO3166_SEPARATOR)
+  end
+
+  def self.from_ltree(ltree)
+    parse(ltree, separator: LTREE_SEPARATOR)
+  end
+
+  def self.parse(code, separator:)
+    return if code.blank?
+    return code if code.is_a?(Geolocation)
+
+    country, region = code.split(separator)
+
+    new(country, region)
+  end
+
+  def initialize(country_code, region_code = nil)
+    @country_code = country_code
+    @region_code = region_code
+  end
+
+  # validates :country_code, inclusion: { in: ISO3166::Country.codes }
+  validates :country_code, inclusion: { in: PERMITTED_COUNTRIES }
+  validates :region_code, inclusion: {
+    in: ->(geolocation) { ISO3166::Country.region_codes_if_exists(geolocation.country_code) }
+  }, allow_nil: true
+
+  def ==(other)
+    country_code == other.country_code && region_code == other.region_code
+  end
+
+  def to_iso3166
+    [country_code, region_code].compact.join("-")
+  end
+
+  def to_ltree
+    [country_code, region_code].compact.join(".")
+  end
+
+  def to_sql_query_clause(column_name = :target_geolocations)
+    return unless valid?
+
+    lquery = country_code
+    # Match region if specified
+    lquery += ".#{region_code}{,}" if region_code
+
+    "'#{lquery}' ~ #{column_name}"
+  end
+
+  def errors_as_sentence
+    errors.full_messages.to_sentence
+  end
+end

app/queries/display_ads/filtered_ads_query.rb

@@ -8,9 +8,10 @@ module DisplayAds
     # @param area [String] the site area where the ad is visible
     # @param user_signed_in [Boolean] whether or not the visitor is signed-in
     # @param display_ads [DisplayAd] can be a filtered scope or Arel relationship
+    # @param location [Geolocation|String] the visitor's geographic location
     def initialize(area:, user_signed_in:, organization_id: nil, article_tags: [],
                    permit_adjacent_sponsors: true, article_id: nil, display_ads: DisplayAd,
-                   user_id: nil, user_tags: nil)
+                   user_id: nil, user_tags: nil, location: nil)
       @filtered_display_ads = display_ads.includes([:organization])
       @area = area
       @user_signed_in = user_signed_in
@@ -20,6 +21,7 @@ module DisplayAds
       @article_id = article_id
       @permit_adjacent_sponsors = permit_adjacent_sponsors
       @user_tags = user_tags
+      @location = Geolocation.from_iso3166(location)
     end
 
     def call
@@ -57,6 +59,10 @@ module DisplayAds
                                 authenticated_ads(%w[all logged_out])
                               end
 
+      if FeatureFlag.enabled?(Geolocation::FEATURE_FLAG)
+        @filtered_display_ads = location_targeted_ads
+      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)
@@ -100,6 +106,15 @@ module DisplayAds
       end
     end
 
+    def location_targeted_ads
+      geo_query = "cardinality(target_geolocations) = 0" # Empty array
+      if @location&.valid?
+        geo_query += " OR (#{@location.to_sql_query_clause})"
+      end
+
+      @filtered_display_ads.where(geo_query)
+    end
+
     def type_of_ads
       # If this is an organization article and community-type ads exist, show them
       if @organization_id.present?

config/initializers/custom_types.rb

@@ -0,0 +1,5 @@
+# Custom ActiveRecord types, for example mapping from a Postgres database extension type to a Ruby object
+Rails.application.reloader.to_prepare do
+  ActiveRecord::Type.register(:geolocation_array, Geolocation::ArrayType)
+  ActiveModel::Type.register(:geolocation_array, Geolocation::ArrayType)
+end

config/initializers/iso3166.rb

@@ -0,0 +1,6 @@
+Rails.application.config.to_prepare do
+  # Explicitly load extensions on the `countries` gem so they are always available
+  Rails.root.glob("lib/ISO3166/*.rb").each do |filename|
+    require_dependency filename
+  end
+end

config/locales/models/en.yml

@@ -41,6 +41,8 @@ en:
         experience3: Have experience level three
         experience4: Have experience level four
         experience5: Have experience level five
+    billboard:
+      invalid_location: '%{location} is not a supported ISO 3166-2 code'
     broadcast:
       single_active: You can only have one active announcement broadcast
     comment:

config/locales/models/fr.yml

@@ -40,6 +40,8 @@ fr:
         experience3: Have experience level three
         experience4: Have experience level four
         experience5: Have experience level five
+    billboard:
+      invalid_location: "%{location} n'est pas un code pris en charge de la norme ISO 3166-2"
     broadcast:
       single_active: Vous ne pouvez avoir qu'une seule diffusion d'annonce active
     comment:

db/migrate/20230720211750_add_target_geolocations_to_display_ads.rb

@@ -0,0 +1,15 @@
+class AddTargetGeolocationsToDisplayAds < ActiveRecord::Migration[7.0]
+  def up
+    enable_extension "ltree"
+
+    add_column :display_ads, :target_geolocations, :ltree, array: true, default: []
+  end
+
+  def down
+    safety_assured do
+      remove_column :display_ads, :target_geolocations
+
+      disable_extension "ltree"
+    end
+  end
+end

db/migrate/20230720211851_add_indices_to_display_ad_target_geolocations.rb

@@ -0,0 +1,12 @@
+class AddIndicesToDisplayAdTargetGeolocations < ActiveRecord::Migration[7.0]
+  disable_ddl_transaction!
+
+  def change
+    # GiST index enables custom ltree operators
+    add_index :display_ads,
+              :target_geolocations,
+              using: :gist,
+              name: "gist_index_display_ads_on_target_geolocations",
+              algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -10,9 +10,10 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_07_13_150940) do
+ActiveRecord::Schema[7.0].define(version: 2023_07_20_211851) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
+  enable_extension "ltree"
   enable_extension "pg_trgm"
   enable_extension "pgcrypto"
   enable_extension "plpgsql"
@@ -478,11 +479,14 @@ ActiveRecord::Schema[7.0].define(version: 2023_07_13_150940) do
     t.text "processed_html"
     t.boolean "published", default: false
     t.float "success_rate", default: 0.0
+    t.ltree "target_geolocations", default: [], array: true
     t.integer "type_of", default: 0, null: false
     t.datetime "updated_at", precision: nil, 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"
+    t.index ["target_geolocations"], name: "gist_index_display_ads_on_target_geolocations", using: :gist
+    t.index ["target_geolocations"], name: "index_display_ads_on_target_geolocations"
   end
 
   create_table "email_authorizations", force: :cascade do |t|

lib/ISO3166/country.rb

@@ -0,0 +1,18 @@
+# Modifications to the `countries` gem for clearer application code.
+# In the future, we might want to modify the gem's dataset here (e.g. to include
+# a mapping of the relationship between hierarchical subdivisions)
+module ISO3166
+  class Country
+    def self.code_from_name(name)
+      find_country_by_any_name(name).alpha2
+    end
+
+    def self.region_codes_if_exists(alpha2_code)
+      new(alpha2_code)&.region_codes || []
+    end
+
+    def region_codes
+      @region_codes ||= subdivisions.keys
+    end
+  end
+end

lib/data_update_scripts/20230725143428_add_billboard_location_targeting_feature_flag.rb

@@ -0,0 +1,7 @@
+module DataUpdateScripts
+  class AddBillboardLocationTargetingFeatureFlag
+    def run
+      FeatureFlag.add Geolocation::FEATURE_FLAG
+    end
+  end
+end

spec/lib/data_update_scripts/add_billboard_location_targeting_feature_flag_spec.rb

@@ -0,0 +1,15 @@
+require "rails_helper"
+require Rails.root.join(
+  "lib/data_update_scripts/20230725143428_add_billboard_location_targeting_feature_flag.rb",
+)
+
+describe DataUpdateScripts::AddBillboardLocationTargetingFeatureFlag do
+  before do
+    allow(FeatureFlag).to receive(:add)
+  end
+
+  it "adds the feature flag" do
+    described_class.new.run
+    expect(FeatureFlag).to have_received(:add).with(:billboard_location_targeting)
+  end
+end

spec/models/display_ad_spec.rb

@@ -263,6 +263,98 @@ RSpec.describe DisplayAd do
     end
   end
 
+  describe ".validate_geolocations" do
+    subject(:billboard) do
+      build(
+        :display_ad,
+        name: "This is an Ad",
+        body_markdown: "Ad Body",
+        placement_area: "post_comments",
+        target_geolocations: geo_input,
+      )
+    end
+
+    let(:target_geolocations) do
+      [
+        Geolocation.new("US", "CA"),
+        Geolocation.new("CA", "ON"),
+        Geolocation.new("CA", "BC"),
+      ]
+    end
+
+    context "with nil" do
+      let(:geo_input) { nil }
+
+      it "permits them" do
+        expect(billboard).to be_valid
+        expect(billboard.target_geolocations).to be_empty
+      end
+    end
+
+    context "with empty string" do
+      let(:geo_input) { "" }
+
+      it "permits them" do
+        expect(billboard).to be_valid
+        expect(billboard.target_geolocations).to be_empty
+      end
+    end
+
+    context "with empty array" do
+      let(:geo_input) { [] }
+
+      it "permits them" do
+        expect(billboard).to be_valid
+        expect(billboard.target_geolocations).to be_empty
+      end
+    end
+
+    context "with comma-separated list of ISO 3166-2" do
+      let(:geo_input) { "US-CA, CA-ON, CA-BC" }
+
+      it "permits them" do
+        expect(billboard).to be_valid
+        expect(billboard.target_geolocations).to match_array(target_geolocations)
+      end
+    end
+
+    context "with array of ISO 3166-2" do
+      let(:geo_input) { %w[US-CA CA-ON CA-BC] }
+
+      it "permits them" do
+        expect(billboard).to be_valid
+        expect(billboard.target_geolocations).to match_array(target_geolocations)
+      end
+    end
+
+    context "with array of geolocations" do
+      let(:geo_input) { [Geolocation.new("US", "CA"), Geolocation.new("CA", "ON"), Geolocation.new("CA", "BC")] }
+
+      it "permits them" do
+        expect(billboard).to be_valid
+        expect(billboard.target_geolocations).to match_array(target_geolocations)
+      end
+    end
+
+    context "with invalid comma-separated ISO 3166-2 codes" do
+      let(:geo_input) { "US-CA, NOT-REAL" }
+
+      it "does not permit them" do
+        expect(billboard).not_to be_valid
+        expect(billboard.errors_as_sentence).to include("NOT-REAL is not a supported ISO 3166-2 code")
+      end
+    end
+
+    context "with invalid array of ISO 3166-2 codes" do
+      let(:geo_input) { %w[CA-QC CA-FAKE] }
+
+      it "does not permit them" do
+        expect(billboard).not_to be_valid
+        expect(billboard.errors_as_sentence).to include("CA-FAKE is not a supported ISO 3166-2 code")
+      end
+    end
+  end
+
   describe "#exclude_articles_ids" do
     it "processes array of integer ids as expected" do
       display_ad.exclude_article_ids = ["11"]

spec/models/geolocation_spec.rb

@@ -0,0 +1,109 @@
+require "rails_helper"
+
+RSpec.describe Geolocation do
+  describe "validations" do
+    describe "country validation" do
+      it "allows only the US and Canada (for now)" do
+        united_states = described_class.new("US")
+        canada = described_class.new("CA")
+        netherlands = described_class.new("NL")
+        india = described_class.new("IN")
+        south_africa = described_class.new("ZA")
+        unassigned_country = described_class.new("ZZ")
+        invalid_code_country = described_class.new("not iso3166")
+
+        expect(united_states).to be_valid
+        expect(canada).to be_valid
+        expect(netherlands).not_to be_valid
+        expect(india).not_to be_valid
+        expect(south_africa).not_to be_valid
+        expect(unassigned_country).not_to be_valid
+        expect(invalid_code_country).not_to be_valid
+      end
+    end
+
+    describe "region validation" do
+      it "allows an empty region" do
+        without_region = described_class.new("US")
+        expect(without_region).to be_valid
+      end
+
+      it "allows only regions that are a part of the country" do
+        # WA: State of Washington (within the US)
+        # QC: Province of Québec (within Canada)
+        us_with_us_region = described_class.new("US", "WA")
+        canada_with_canada_region = described_class.new("CA", "QC")
+        us_with_canada_region = described_class.new("US", "QC")
+        canada_with_us_region = described_class.new("CA", "WA")
+
+        expect(us_with_us_region).to be_valid
+        expect(canada_with_canada_region).to be_valid
+        expect(us_with_canada_region).not_to be_valid
+        expect(canada_with_us_region).not_to be_valid
+      end
+    end
+  end
+
+  describe "parsing" do
+    shared_examples "handles edge cases" do
+      context "when the code to be parsed is nil" do
+        let(:code) { nil }
+
+        it { is_expected.to be_nil }
+      end
+
+      context "when the code to be parsed is an empty string" do
+        let(:code) { "" }
+
+        it { is_expected.to be_nil }
+      end
+
+      context "when the code to be parsed is already a geolocation" do
+        let(:code) { described_class.new("US") }
+
+        it { is_expected.to eq(code) }
+      end
+    end
+
+    describe ".from_iso3166" do
+      subject(:geo) { described_class.from_iso3166(code) }
+
+      let(:code) { "US-ME" } # Maine, USA
+
+      it "returns the correct geolocation from an ISO 3166-2 format code" do
+        expect(geo).to be_valid
+        expect(geo.country_code).to eq("US")
+        expect(geo.region_code).to eq("ME")
+      end
+
+      include_examples "handles edge cases"
+    end
+
+    describe ".from_ltree" do
+      subject(:geo) { described_class.from_ltree(code) }
+
+      let(:code) { "CA.NL" } # Newfoundland, Canada
+
+      it "returns the correct geolocation from a Postgres ltree format" do
+        expect(geo).to be_valid
+        expect(geo.country_code).to eq("CA")
+        expect(geo.region_code).to eq("NL")
+      end
+
+      include_examples "handles edge cases"
+    end
+  end
+
+  describe "equality" do
+    let(:texas) { described_class.new("US", "TX") }
+    let(:ontario) { described_class.new("CA", "ON") }
+
+    it "considers two geolocations equal if their country and region codes are equal" do
+      expect(texas).to eq(described_class.new("US", "TX"))
+      expect(texas).not_to eq(described_class.new("US", "CA"))
+      expect(ontario).to eq(described_class.new("CA", "ON"))
+      expect(ontario).not_to eq(described_class.new("CA", "QC"))
+      expect(texas).not_to eq(ontario)
+    end
+  end
+end

spec/queries/display_ads/filtered_ads_query_spec.rb

@@ -197,4 +197,92 @@ RSpec.describe DisplayAds::FilteredAdsQuery, type: :query do
       expect(filtered).not_to include(community_ad)
     end
   end
+
+  context "with target geolocations" do
+    let!(:no_targets) { create_display_ad }
+    let!(:targets_canada) { create_display_ad(target_geolocations: "CA") }
+    let!(:targets_new_york_and_canada) { create_display_ad(target_geolocations: "US-NY, CA") }
+    let!(:targets_california_and_texas) { create_display_ad(target_geolocations: "US-CA, US-TX") }
+    let!(:targets_quebec_and_newfoundland) { create_display_ad(target_geolocations: "CA-QC, CA-NL") }
+    let!(:targets_maine_alberta_and_ontario) { create_display_ad(target_geolocations: "US-ME, CA-AB, CA-ON") }
+
+    context "when location targeting feature is not enabled" do
+      before do
+        allow(FeatureFlag).to receive(:enabled?).with(:billboard_location_targeting).and_return(false)
+      end
+
+      it "ignores the target geolocations" do
+        filtered = filter_ads(location: "CA-NL") # User is in Newfoundland, Canada
+
+        expect(filtered).to include(
+          no_targets,
+          targets_canada,
+          targets_new_york_and_canada,
+          targets_california_and_texas,
+          targets_quebec_and_newfoundland,
+          targets_maine_alberta_and_ontario,
+        )
+      end
+    end
+
+    context "when location targeting feature is enabled" do
+      before do
+        allow(FeatureFlag).to receive(:enabled?).with(:billboard_location_targeting).and_return(true)
+      end
+
+      it "shows only billboards with no targeting if no location is provided" do
+        filtered = filter_ads
+        expect(filtered).to include(no_targets)
+        expect(filtered).not_to include(
+          targets_canada,
+          targets_new_york_and_canada,
+          targets_california_and_texas,
+          targets_quebec_and_newfoundland,
+          targets_maine_alberta_and_ontario,
+        )
+      end
+
+      it "shows only billboards whose target location includes the specified location" do
+        filtered = filter_ads(location: "CA-NL") # User is in Newfoundland, Canada
+
+        expect(filtered).to include(
+          no_targets,
+          targets_canada,
+          targets_new_york_and_canada,
+          targets_quebec_and_newfoundland,
+        )
+        expect(filtered).not_to include(
+          targets_california_and_texas,
+          targets_maine_alberta_and_ontario,
+        )
+
+        filtered = filter_ads(location: "US-CA") # User is in California, USA
+        expect(filtered).to include(
+          no_targets,
+          targets_california_and_texas,
+        )
+        expect(filtered).not_to include(
+          targets_canada,
+          targets_new_york_and_canada,
+          targets_quebec_and_newfoundland,
+          targets_maine_alberta_and_ontario,
+        )
+      end
+
+      it "shows only billboards targeting the country specifically if no region is provided" do
+        filtered = filter_ads(location: "CA") # User is in "Canada"
+
+        expect(filtered).to include(
+          no_targets,
+          targets_canada,
+          targets_new_york_and_canada,
+        )
+        expect(filtered).not_to include(
+          targets_california_and_texas,
+          targets_quebec_and_newfoundland,
+          targets_maine_alberta_and_ontario,
+        )
+      end
+    end
+  end
 end

spec/requests/api/v1/billboards_spec.rb

@@ -51,7 +51,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")
+                          "priority", "target_geolocations")
       end
 
       it "returns a malformed response" do
@@ -100,7 +100,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")
+                          "priority", "target_geolocations")
       end
     end