From c202afdec67f3ed017ddb343206e2874fe911913 Mon Sep 17 00:00:00 2001
From: PJ <pj@forem.com>
Date: Wed, 9 Aug 2023 19:07:57 +0100
Subject: [PATCH] Show billboards to users by location (async requests)
 (#19886)

* use HTTP_CLIENT_GEO in billboards show action

* limit targeting to signed in users

* re-add cypress test

* simplify billboards request spec

* fix header problems hopefully?

* whoops

* Squash filleduchaos/caching-billboards-by-location

* export ISO 3166-2 regions
* wip
* remove ISO 3166-2 regions
* finally, a VCL config that...works?
* update billboard controller & specs

* fix specs

* clean up and document VCL some more

* remove vcl_init sub

* remove billboard fragment caching
---
 app/controllers/billboards_controller.rb      | 16 +++-
 app/controllers/concerns/caching_headers.rb   |  5 +
 app/models/display_ad.rb                      |  3 +-
 app/views/billboards/show.html.erb            |  8 +-
 app/views/layouts/_footer.html.erb            | 10 +-
 config/fastly/snippets/client_geolocation.vcl | 30 +++++-
 .../homeFeedFlows/billboards.spec.js          | 60 ++++++++++++
 spec/requests/billboards_spec.rb              | 94 +++++++++++++++++--
 spec/support/seeds/seeds_e2e.rb               | 22 ++++-
 9 files changed, 225 insertions(+), 23 deletions(-)
 create mode 100644 cypress/e2e/seededFlows/homeFeedFlows/billboards.spec.js

diff --git a/app/controllers/billboards_controller.rb b/app/controllers/billboards_controller.rb
index 6281ea420..60fca1073 100644
--- a/app/controllers/billboards_controller.rb
+++ b/app/controllers/billboards_controller.rb
@@ -5,7 +5,12 @@ class BillboardsController < ApplicationController
 
   def show
     skip_authorization
-    set_cache_control_headers(CACHE_EXPIRY_FOR_BILLBOARDS) unless session_current_user_id
+    unless session_current_user_id
+      set_cache_control_headers(CACHE_EXPIRY_FOR_BILLBOARDS)
+      if FeatureFlag.enabled?(Geolocation::FEATURE_FLAG)
+        add_vary_header("X-Cacheable-Client-Geo")
+      end
+    end
 
     if placement_area
       if params[:username].present? && params[:slug].present?
@@ -18,6 +23,7 @@ class BillboardsController < ApplicationController
         user_id: current_user&.id,
         article: @article ? ArticleDecorator.new(@article) : nil,
         user_tags: user_tags,
+        location: client_geolocation,
       )
 
       if @billboard && !session_current_user_id
@@ -39,4 +45,12 @@ class BillboardsController < ApplicationController
 
     current_user&.cached_followed_tag_names
   end
+
+  def client_geolocation
+    if session_current_user_id
+      request.headers["X-Client-Geo"]
+    else
+      request.headers["X-Cacheable-Client-Geo"]
+    end
+  end
 end
diff --git a/app/controllers/concerns/caching_headers.rb b/app/controllers/concerns/caching_headers.rb
index cd2e1f081..8159871c2 100644
--- a/app/controllers/concerns/caching_headers.rb
+++ b/app/controllers/concerns/caching_headers.rb
@@ -44,6 +44,11 @@ module CachingHeaders
     response.headers["Surrogate-Key"] = surrogate_keys.join(" ")
   end
 
+  def add_vary_header(*vary_keys)
+    existing = response.headers["Vary"]
+    response.headers["Vary"] = [existing, *vary_keys].compact.join(", ")
+  end
+
   private
 
   def build_surrogate_control(max_age, stale_while_revalidate: nil, stale_if_error: 26_400)
diff --git a/app/models/display_ad.rb b/app/models/display_ad.rb
index dc1e28381..2809c07d3 100644
--- a/app/models/display_ad.rb
+++ b/app/models/display_ad.rb
@@ -60,7 +60,7 @@ class DisplayAd < ApplicationRecord
 
   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, user_tags: nil)
+  def self.for_display(area:, user_signed_in:, user_id: nil, article: nil, user_tags: nil, location: nil)
     permit_adjacent = article ? article.permit_adjacent_sponsors? : true
 
     billboards_for_display = Billboards::FilteredAdsQuery.call(
@@ -73,6 +73,7 @@ class DisplayAd < ApplicationRecord
       permit_adjacent_sponsors: permit_adjacent,
       user_id: user_id,
       user_tags: user_tags,
+      location: location,
     )
 
     case rand(99) # output integer from 0-99
diff --git a/app/views/billboards/show.html.erb b/app/views/billboards/show.html.erb
index b2f3e221a..a54cca829 100644
--- a/app/views/billboards/show.html.erb
+++ b/app/views/billboards/show.html.erb
@@ -1,9 +1,3 @@
 <% if @billboard %>
-  <% if user_signed_in? %>
-    <%= render partial: "shared/billboard", locals: { billboard: @billboard, data_context_type: BillboardEvent::CONTEXT_TYPE_ARTICLE } %>
-  <% else %>
-    <% cache([params[:username], params[:slug], params[:placement_area]], expires_in: 15.minutes) do %>
-      <%= render partial: "shared/billboard", locals: { billboard: @billboard, data_context_type: BillboardEvent::CONTEXT_TYPE_ARTICLE } %>
-    <% end %>
-  <% end %>
+  <%= render partial: "shared/billboard", locals: { billboard: @billboard, data_context_type: BillboardEvent::CONTEXT_TYPE_ARTICLE } %>
 <% end %>
diff --git a/app/views/layouts/_footer.html.erb b/app/views/layouts/_footer.html.erb
index fb5a8879e..55818aa17 100644
--- a/app/views/layouts/_footer.html.erb
+++ b/app/views/layouts/_footer.html.erb
@@ -1,9 +1,9 @@
 <% navigation_links = Rails.cache.fetch("navigation_links", expires_in: 15.minutes) do
-  {
-    default_nav_ids: NavigationLink.default_section.ordered.ids,
-    other_nav_ids: NavigationLink.other_section.ordered.ids
-  }
-end %>
+     {
+       default_nav_ids: NavigationLink.default_section.ordered.ids,
+       other_nav_ids: NavigationLink.other_section.ordered.ids
+     }
+   end %>
 
 <footer id="footer" class="crayons-footer print-hidden">
   <div id="footer-container" class="crayons-footer__container">
diff --git a/config/fastly/snippets/client_geolocation.vcl b/config/fastly/snippets/client_geolocation.vcl
index 35ec353fe..82827ecda 100644
--- a/config/fastly/snippets/client_geolocation.vcl
+++ b/config/fastly/snippets/client_geolocation.vcl
@@ -1,5 +1,18 @@
+table active_geofencing_regions {
+  // This table can be managed independently of the deployed configuration as an edge dictionary.
+
+  // This accomplishes a couple of things:
+  // 1. It greatly reduces the number of variants that need to be cached
+  // 2. It lets us enable more granular caching (for big campaigns and/or customers)
+  //    without having to redeploy the app.
+  // 3. It lets us turn off geofenced caching entirely fairly easily (by emptying the table)
+  "CA": "CA",
+  "US": "US"
+}
+
 sub vcl_recv {
   declare local var.code STRING;
+  declare local var.cacheable_code STRING;
 
   if (client.geo.country_code != "**") { // Indicates a reserved country block
     set var.code = client.geo.country_code;
@@ -13,6 +26,21 @@ sub vcl_recv {
       set var.code = var.code + "-" + client.geo.region;
     }
 
-    set req.http.HTTP_CLIENT_GEO = var.code;
+    set req.http.X-Client-Geo = var.code;
+    // Ternary "if" construct lets us fall back to trying to cache at a country level
+    // if the more specific country+region code isn't currently set to be cached.
+    set var.cacheable_code = if(
+      table.contains(active_geofencing_regions, var.code),
+      var.code,
+      client.geo.country_code
+    );
+    // If neither country nor country+region code are in the edge dictionary, this header
+    // will be empty. All locations where we don't offer targeting (yet) will thus share
+    // the same cache.
+    set req.http.X-Cacheable-Client-Geo = table.lookup(
+      active_geofencing_regions,
+      var.cacheable_code,
+      ""
+    );
   }
 }
diff --git a/cypress/e2e/seededFlows/homeFeedFlows/billboards.spec.js b/cypress/e2e/seededFlows/homeFeedFlows/billboards.spec.js
new file mode 100644
index 000000000..4d82079d5
--- /dev/null
+++ b/cypress/e2e/seededFlows/homeFeedFlows/billboards.spec.js
@@ -0,0 +1,60 @@
+describe('Home page billboards', () => {
+  beforeEach(() => {
+    cy.testSetup();
+    cy.fixture('users/adminUser.json').as('user');
+  });
+
+  context('with location targeting', () => {
+    beforeEach(() => {
+      cy.intercept(`/**`, (req) => {
+        // User in Ontario, Canada
+        req.headers['X-Client-Geo'] = 'CA-ON';
+        req.headers['X-Cacheable-Client-Geo'] = 'CA';
+      });
+      cy.enableFeatureFlag('billboard_location_targeting');
+    });
+
+    context('when a user is signed in', () => {
+      beforeEach(() => {
+        cy.get('@user').then((user) => {
+          cy.loginAndVisit(user, '/');
+        });
+      });
+
+      it('shows billboards targeting their location', () => {
+        cy.findByRole('main').within(() => {
+          cy.get('.billboard')
+            .should('contain', 'This is a billboard shown to people in Ontario')
+            .and(
+              'not.contain',
+              'This is a billboard shown to people in the US',
+            );
+        });
+      });
+
+      it('shows billboards that do not target any location', () => {
+        cy.findByLabelText('Primary sidebar').within(() => {
+          cy.get('.billboard').should('contain', 'This is a regular billboard');
+        });
+      });
+    });
+
+    context('when a user is not signed in', () => {
+      beforeEach(() => {
+        cy.visit('/');
+      });
+
+      it('only shows billboards with a cacheable target geolocation', () => {
+        cy.findByRole('main').within(() => {
+          cy.get('.billboard').should('not.exist');
+        });
+      });
+
+      it('shows billboards that do not target any location', () => {
+        cy.findByLabelText('Primary sidebar').within(() => {
+          cy.get('.billboard').should('contain', 'This is a regular billboard');
+        });
+      });
+    });
+  });
+});
diff --git a/spec/requests/billboards_spec.rb b/spec/requests/billboards_spec.rb
index b66e7a1e2..41e5b874b 100644
--- a/spec/requests/billboards_spec.rb
+++ b/spec/requests/billboards_spec.rb
@@ -7,19 +7,100 @@ RSpec.describe "Billboards" do
   let(:max_age) { 15.minutes.to_i }
   let(:stale_if_error) { 26_400 }
 
+  def create_billboard(**options)
+    defaults = {
+      approved: true,
+      published: true,
+      display_to: :all
+    }
+    create(:display_ad, **options.reverse_merge(defaults))
+  end
+
   describe "GET /:username/:slug/billboards/:placement_area" do
-    before do
-      create(:display_ad, placement_area: "post_comments", published: true, approved: true)
-    end
+    let!(:billboard) { create_billboard(placement_area: "post_comments") }
 
     it "returns the correct response" do
       get article_billboard_path(username: article.username, slug: article.slug, placement_area: "post_comments")
-      display_ad = DisplayAd.find_by(placement_area: "post_comments")
 
       expect(response).to have_http_status(:ok)
-      expect(response.parsed_body).to include(display_ad.processed_html)
+      expect(response.parsed_body).to include(billboard.processed_html)
     end
 
+    # rubocop:disable RSpec/NestedGroups
+    context "when client geolocation is present" do
+      let(:client_in_alberta_canada) { { "X-Client-Geo" => "CA-AB", "X-Cacheable-Client-Geo" => "CA" } }
+      let(:client_in_california_usa) { { "X-Client-Geo" => "US-CA", "X-Cacheable-Client-Geo" => "US" } }
+
+      let!(:canada_billboard) { create_billboard(placement_area: "sidebar_left", target_geolocations: "CA") }
+      let!(:california_billboard) { create_billboard(placement_area: "sidebar_left", target_geolocations: "US-CA") }
+
+      before do
+        allow(FeatureFlag).to receive(:enabled?).with(:billboard_location_targeting).and_return(true)
+      end
+
+      context "with signed-in user" do
+        before do
+          sign_in user
+        end
+
+        it "returns only billboards targeting their location" do
+          get article_billboard_path(username: article.username, slug: article.slug, placement_area: "sidebar_left"),
+              headers: client_in_alberta_canada
+
+          expect(response).to have_http_status(:ok)
+          expect(response.parsed_body).to include(canada_billboard.processed_html)
+          expect(response.parsed_body).not_to include(california_billboard.processed_html)
+        end
+
+        it "is accurate for more precise locations" do
+          get article_billboard_path(username: article.username, slug: article.slug, placement_area: "sidebar_left"),
+              headers: client_in_california_usa
+
+          expect(response).to have_http_status(:ok)
+          expect(response.parsed_body).to include(california_billboard.processed_html)
+          expect(response.parsed_body).not_to include(canada_billboard.processed_html)
+        end
+
+        it "does not set Vary header" do
+          get article_billboard_path(username: article.username, slug: article.slug, placement_area: "sidebar_left"),
+              headers: client_in_alberta_canada
+
+          expect(response).to have_http_status(:ok)
+          expect(response.headers["Vary"]).not_to include("X-Client-Geo", "X-Cacheable-Client-Geo")
+        end
+      end
+
+      context "without signed-in user" do
+        it "does not return billboards targeted more accurately than the specified cacheable level" do
+          get article_billboard_path(username: article.username, slug: article.slug, placement_area: "sidebar_left"),
+              headers: client_in_california_usa
+
+          expect(response).to have_http_status(:ok)
+          # X-Cacheable-Client-Geo is set to all of the US, so billboards targeted at a single state are filtered out
+          expect(response.parsed_body).to be_empty
+        end
+
+        it "is accurate for more precise locations" do
+          get article_billboard_path(username: article.username, slug: article.slug, placement_area: "sidebar_left"),
+              headers: { "X-Client-Geo" => "US-CA", "X-Cacheable-Client-Geo" => "US-CA" }
+
+          expect(response).to have_http_status(:ok)
+          expect(response.parsed_body).to include(california_billboard.processed_html)
+          expect(response.parsed_body).not_to include(canada_billboard.processed_html)
+        end
+
+        it "sets Vary header" do
+          get article_billboard_path(username: article.username, slug: article.slug, placement_area: "sidebar_left"),
+              headers: client_in_alberta_canada
+
+          expect(response).to have_http_status(:ok)
+          expect(response.headers["Vary"]).not_to include("X-Client-Geo")
+          expect(response.headers["Vary"]).to include("X-Cacheable-Client-Geo")
+        end
+      end
+    end
+    # rubocop:enable RSpec/NestedGroups
+
     context "when signed in" do
       before do
         sign_in user
@@ -48,8 +129,7 @@ RSpec.describe "Billboards" do
       end
 
       it "sets the surrogate key header equal to params for article" do
-        display_ad = DisplayAd.find_by(placement_area: "post_comments")
-        expect(response.headers["Surrogate-Key"]).to eq("display_ads/#{display_ad.id}")
+        expect(response.headers["Surrogate-Key"]).to eq("display_ads/#{billboard.id}")
       end
 
       it "sets the x-accel-expires header equal to max-age for article" do
diff --git a/spec/support/seeds/seeds_e2e.rb b/spec/support/seeds/seeds_e2e.rb
index fed9c35cd..26d3fb603 100644
--- a/spec/support/seeds/seeds_e2e.rb
+++ b/spec/support/seeds/seeds_e2e.rb
@@ -1097,7 +1097,7 @@ seeder.create_if_none(DisplayAd) do
   org_id = Organization.find_by(slug: "bachmanity").id
   DisplayAd.create!(
     organization_id: org_id,
-    body_markdown: "<h1>This is an add</h1>",
+    body_markdown: "<h1>This is a regular billboard</h1>",
     placement_area: "sidebar_left",
     name: "Tests Display Ad",
     published: true,
@@ -1113,4 +1113,24 @@ seeder.create_if_none(DisplayAd) do
     approved: true,
     audience_segment: AudienceSegment.where(type_of: :manual).first,
   )
+
+  DisplayAd.create!(
+    organization_id: org_id,
+    body_markdown: "<h1>This is a billboard shown to people in Ontario</h1>",
+    placement_area: "feed_first",
+    name: "Ontario-targeted Billboard",
+    published: true,
+    approved: true,
+    target_geolocations: "CA-ON",
+  )
+
+  DisplayAd.create!(
+    organization_id: org_id,
+    body_markdown: "<h1>This is a billboard shown to people in the US</h1>",
+    placement_area: "feed_first",
+    name: "US-targeted Billboard",
+    published: true,
+    approved: true,
+    target_geolocations: "US",
+  )
 end