From 0771a5cd2e6d01b5822c4d21c5f17114c42c8a23 Mon Sep 17 00:00:00 2001
From: Ridhwana <Ridhwana.Khan16@gmail.com>
Date: Wed, 12 Apr 2023 18:15:15 +0200
Subject: [PATCH] Load `post_comments` billboard asynchronously (#19285)

* feat: add a route to the async_info for display ads

* feat: load the billboard asyncronously

* feat: move the methods to the display_ads controller

* feat: handle params better

* feat: cache control headers

* feat: test fastly caching headers on display ads

* fix: surrogate key test

* feat: use safe navigation operator to handle cases where there is no article id

* fix: article id present then find the article

* feat: add a response test

* Fragment caching

* feat: update the article decorator

* feat: update cache keys for fragment

* feat: remove an empty line

* feat: add article id

* feat: bust cache

* feat: setup dropdown for billboard

* chore: add chunk to same line

* feat: add to safe params for caching

* Update app/controllers/display_ads_controller.rb

Co-authored-by: Joshua Wehner <joshua@forem.com>

* feat: remove the cache deletion

* feat: update the routes to follow a new scope

* feat: update the cache params

---------

Co-authored-by: Joshua Wehner <joshua@forem.com>
---
 .github/PULL_REQUEST_TEMPLATE.md              |  1 -
 app/controllers/display_ads_controller.rb     | 30 +++++++++
 app/decorators/article_decorator.rb           |  2 +
 app/javascript/packs/billboard.js             | 23 +++++++
 app/views/articles/_sidebar.html.erb          |  4 +-
 app/views/articles/_sticky_nav.html.erb       |  2 +-
 app/views/articles/show.html.erb              | 18 ++---
 app/views/display_ads/show.html.erb           |  9 +++
 app/views/shared/_display_ad.html.erb         |  6 +-
 app/views/shared/_display_ad_header.html.erb  |  2 +-
 app/views/sidebars/_homepage_content.html.erb |  2 +-
 app/views/stories/_main_stories_feed.html.erb |  6 +-
 config/fastly/snippets/safe_params_list.vcl   |  2 +-
 config/routes.rb                              |  5 ++
 spec/decorators/article_decorator_spec.rb     | 11 +++
 spec/requests/display_ads_spec.rb             | 67 +++++++++++++++++++
 .../user_visits_articles_by_timeframe_spec.rb |  1 -
 17 files changed, 163 insertions(+), 28 deletions(-)
 create mode 100644 app/controllers/display_ads_controller.rb
 create mode 100644 app/javascript/packs/billboard.js
 create mode 100644 app/views/display_ads/show.html.erb
 create mode 100644 spec/requests/display_ads_spec.rb

diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
index 5dd9054d1..110d1d819 100644
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -67,4 +67,3 @@ accessibility is impacted and tested. For more info, check out the
 ## [optional] What gif best describes this PR or how it makes you feel?
 
 ![alt_text](gif_link)
-
diff --git a/app/controllers/display_ads_controller.rb b/app/controllers/display_ads_controller.rb
new file mode 100644
index 000000000..eec3106db
--- /dev/null
+++ b/app/controllers/display_ads_controller.rb
@@ -0,0 +1,30 @@
+class DisplayAdsController < ApplicationController
+  before_action :set_cache_control_headers, only: %i[for_display], unless: -> { current_user }
+  CACHE_EXPIRY_FOR_DISPLAY_ADS = 15.minutes.to_i.freeze
+
+  def show
+    skip_authorization
+    set_cache_control_headers(CACHE_EXPIRY_FOR_DISPLAY_ADS) unless session_current_user_id
+
+    if params[:placement_area]
+      if params[:username].present? && params[:slug].present?
+        @article = Article.find_by(slug: params[:slug])
+      end
+
+      @display_ad = DisplayAd.for_display(
+        area: params[:placement_area],
+        user_signed_in: user_signed_in?,
+        organization_id: @article&.organization_id,
+        permit_adjacent_sponsors: ArticleDecorator.new(@article).permit_adjacent_sponsors?,
+        article_tags: @article&.decorate&.cached_tag_list_array || [],
+        article_id: @article&.id,
+      )
+
+      if @display_ad && !session_current_user_id
+        set_surrogate_key_header @display_ad.record_key
+      end
+    end
+
+    render layout: false
+  end
+end
diff --git a/app/decorators/article_decorator.rb b/app/decorators/article_decorator.rb
index 821947f98..fb07d7a48 100644
--- a/app/decorators/article_decorator.rb
+++ b/app/decorators/article_decorator.rb
@@ -113,6 +113,8 @@ class ArticleDecorator < ApplicationDecorator
   end
 
   def permit_adjacent_sponsors?
+    return true unless respond_to?(:user_id) && user_id.present?
+
     author_ids = [user_id] + co_author_ids
     Users::Setting.where(user_id: author_ids).all?(&:permit_adjacent_sponsors)
   end
diff --git a/app/javascript/packs/billboard.js b/app/javascript/packs/billboard.js
new file mode 100644
index 000000000..f9b7d7ccb
--- /dev/null
+++ b/app/javascript/packs/billboard.js
@@ -0,0 +1,23 @@
+import { setupDisplayAdDropdown } from '../utilities/displayAdDropdown';
+
+// the term billboard can be synonymously interchanged with displayAd
+async function getBillboard() {
+  const placeholderElement = document.getElementsByClassName(
+    'js-display-ad-comments-container',
+  )[0];
+
+  const { asyncUrl } = placeholderElement.dataset || {};
+
+  if (placeholderElement.innerHTML.trim() === '') {
+    const response = await window.fetch(`${asyncUrl}`);
+    const htmlContent = await response.text();
+
+    const generatedElement = document.createElement('div');
+    generatedElement.innerHTML = htmlContent;
+
+    placeholderElement.appendChild(generatedElement);
+    setupDisplayAdDropdown();
+  }
+}
+
+getBillboard();
diff --git a/app/views/articles/_sidebar.html.erb b/app/views/articles/_sidebar.html.erb
index 500da4414..2af17ff10 100644
--- a/app/views/articles/_sidebar.html.erb
+++ b/app/views/articles/_sidebar.html.erb
@@ -8,14 +8,14 @@
     <% cache("display-area-left-#{rand(5)}-#{user_signed_in?}", expires_in: 5.minutes) do %>
       <% @left_sidebar_ad = DisplayAd.for_display(area: "sidebar_left", user_signed_in: user_signed_in?) %>
       <% if @left_sidebar_ad %>
-        <%= render partial: "shared/display_ad", locals: {display_ad: @left_sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME} %>
+        <%= render partial: "shared/display_ad", locals: { display_ad: @left_sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %>
       <% end %>
     <% end %>
     <% cache("display-area-left-2-#{rand(5)}-#{user_signed_in?}", expires_in: 5.minutes) do %>
       <% @second_left_sidebar_ad = DisplayAd.for_display(area: "sidebar_left_2", user_signed_in: user_signed_in?) %>
       <% if @second_left_sidebar_ad %>
         <div class="pt-4">
-          <%= render partial: "shared/display_ad", locals: {display_ad: @second_left_sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME} %>
+          <%= render partial: "shared/display_ad", locals: { display_ad: @second_left_sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %>
         </div>
       <% end %>
     <% end %>
diff --git a/app/views/articles/_sticky_nav.html.erb b/app/views/articles/_sticky_nav.html.erb
index 78f517bf3..fda46aee7 100644
--- a/app/views/articles/_sticky_nav.html.erb
+++ b/app/views/articles/_sticky_nav.html.erb
@@ -64,7 +64,7 @@
                                         permit_adjacent_sponsors: @article.decorate.permit_adjacent_sponsors?) %>
   <% if sidebar_ad %>
     <div class="crayons-article-sticky grid gap-4 break-word pt-4">
-      <%= render partial: "shared/display_ad", locals: { display_ad: sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE} %>
+      <%= render partial: "shared/display_ad", locals: { display_ad: sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %>
     </div>
   <% end %>
 <%# end %>
diff --git a/app/views/articles/show.html.erb b/app/views/articles/show.html.erb
index fdbb4fb87..6eb951f0f 100644
--- a/app/views/articles/show.html.erb
+++ b/app/views/articles/show.html.erb
@@ -192,19 +192,9 @@
 
       </article>
 
-      <% cache("article-bottom-content-#{rand(5)}-#{@article.id}-#{user_signed_in?}-#{(@organization || @user).latest_article_updated_at}", expires_in: 15.minutes) do %>
-        <% @display_ad = DisplayAd.for_display(area: "post_comments",
-                                               user_signed_in: user_signed_in?,
-                                               organization_id: @article.organization_id,
-                                               permit_adjacent_sponsors: @article.decorate.permit_adjacent_sponsors?,
-                                               article_tags: @article.decorate.cached_tag_list_array,
-                                               article_id: @article.id) %>
-        <% if @display_ad %>
-          <div class="pb-4 crayons-layout__comments-display-ad">
-            <%= render partial: "shared/display_ad", locals: { display_ad: @display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE} %>
-          </div>
-        <% end %>
-      <% end %>
+      <div class="pb-4 crayons-layout__comments-display-ad">
+        <div class="js-display-ad-comments-container pb-4 crayons-layout__comments-display-ad" data-async-url="<%= "/#{@article.username}/#{@article.slug}/display_ads/post_comments" %>"></div>
+      </div>
 
       <% cache("article-bottom-content-#{@article.id}-#{user_signed_in?}-#{(@organization || @user).latest_article_updated_at}", expires_in: 18.hours) do %>
         <% suggested_articles = Articles::Suggest.call(@article, max: 4) %>
@@ -241,7 +231,7 @@
 <%= render "moderations/modals/unflag_user_modal" %>
 
 <div class="fullscreen-code js-fullscreen-code"></div>
-<%= javascript_packs_with_chunks_tag "followButtons", defer: true %>
+<%= javascript_packs_with_chunks_tag "followButtons", "billboard", defer: true %>
 
 <% cache("article-show-scripts", expires_in: 8.hours) do %>
   <script>
diff --git a/app/views/display_ads/show.html.erb b/app/views/display_ads/show.html.erb
new file mode 100644
index 000000000..e7d3bcdb4
--- /dev/null
+++ b/app/views/display_ads/show.html.erb
@@ -0,0 +1,9 @@
+<% if @display_ad %>
+  <% if user_signed_in? %>
+    <%= render partial: "shared/display_ad", locals: { display_ad: @display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %>
+  <% else %>
+    <% cache([params[:username], params[:slug], params[:placement_area]], expires_in: 15.minutes) do %>
+      <%= render partial: "shared/display_ad", locals: { display_ad: @display_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_ARTICLE } %>
+    <% end %>
+  <% end %>
+<% end %>
diff --git a/app/views/shared/_display_ad.html.erb b/app/views/shared/_display_ad.html.erb
index 149e588c7..ef01b0887 100644
--- a/app/views/shared/_display_ad.html.erb
+++ b/app/views/shared/_display_ad.html.erb
@@ -7,8 +7,8 @@
     data-type-of="<%= display_ad.type_of %>">
     <div class="crayons-story__body">
       <div class="crayons-story__top">
-        <%= render partial: "shared/display_ad_header", locals: {display_ad: display_ad} %>
-      </div>      
+        <%= render partial: "shared/display_ad_header", locals: { display_ad: display_ad } %>
+      </div>
       <div class="crayons-story__indention-display-ad">
         <div class="text-styles text-styles--display-ad">
           <%= display_ad.processed_html.html_safe %>
@@ -23,7 +23,7 @@
     data-category-impression="<%= DisplayAdEvent::CATEGORY_IMPRESSION %>"
     data-context-type="<%= data_context_type %>"
     data-type-of="<%= display_ad.type_of %>">
-    <%= render partial: "shared/display_ad_header", locals: {display_ad: display_ad} %>
+    <%= render partial: "shared/display_ad_header", locals: { display_ad: display_ad } %>
     <div class="p-1 pt-3 text-styles text-styles--display-ad">
       <%= display_ad.processed_html.html_safe %>
     </div>
diff --git a/app/views/shared/_display_ad_header.html.erb b/app/views/shared/_display_ad_header.html.erb
index 7bf0d9d5f..f90793879 100644
--- a/app/views/shared/_display_ad_header.html.erb
+++ b/app/views/shared/_display_ad_header.html.erb
@@ -42,4 +42,4 @@
       </li>
     </ul>
   </div>
-</div>
\ No newline at end of file
+</div>
diff --git a/app/views/sidebars/_homepage_content.html.erb b/app/views/sidebars/_homepage_content.html.erb
index e07cdd6a7..7fba92426 100644
--- a/app/views/sidebars/_homepage_content.html.erb
+++ b/app/views/sidebars/_homepage_content.html.erb
@@ -2,7 +2,7 @@
   <% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}-#{user_signed_in?}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %>
     <% @sidebar_ad = DisplayAd.for_display(area: "sidebar_right", user_signed_in: user_signed_in?) %>
     <% if @sidebar_ad %>
-      <%= render partial: "shared/display_ad", locals: {display_ad: @sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME} %>
+      <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %>
     <% end %>
 
     <%= render "articles/sidebar_campaign" if Campaign.current.show_in_sidebar? %>
diff --git a/app/views/stories/_main_stories_feed.html.erb b/app/views/stories/_main_stories_feed.html.erb
index d5e0ad898..29c1d7687 100644
--- a/app/views/stories/_main_stories_feed.html.erb
+++ b/app/views/stories/_main_stories_feed.html.erb
@@ -10,7 +10,7 @@
 <% if !user_signed_in? %>
   <% @sidebar_ad_first = DisplayAd.for_display(area: "feed_first", user_signed_in: user_signed_in?) %>
   <% if @sidebar_ad_first %>
-    <%= render partial: "shared/display_ad", locals: {display_ad: @sidebar_ad_first, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME} %>
+    <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_first, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %>
   <% end %>
 <% end %>
 
@@ -30,14 +30,14 @@
       <% if !user_signed_in? && i == 1 %>
         <% @sidebar_ad_second = DisplayAd.for_display(area: "feed_second", user_signed_in: user_signed_in?) %>
         <% if @sidebar_ad_second %>
-          <%= render partial: "shared/display_ad", locals: {display_ad: @sidebar_ad_second, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME} %>
+          <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_second, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %>
         <% end %>
       <% end %>
 
       <% if !user_signed_in? && i == 7 %>
         <% @sidebar_ad_third = DisplayAd.for_display(area: "feed_third", user_signed_in: user_signed_in?) %>
         <% if @sidebar_ad_third %>
-          <%= render partial: "shared/display_ad", locals: {display_ad: @sidebar_ad_third, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME} %>
+          <%= render partial: "shared/display_ad", locals: { display_ad: @sidebar_ad_third, data_context_type: DisplayAdEvent::CONTEXT_TYPE_HOME } %>
         <% end %>
       <% end %>
       <% next if story.id == @featured_story.id %>
diff --git a/config/fastly/snippets/safe_params_list.vcl b/config/fastly/snippets/safe_params_list.vcl
index 3b8f3bf2c..28ff4ec14 100644
--- a/config/fastly/snippets/safe_params_list.vcl
+++ b/config/fastly/snippets/safe_params_list.vcl
@@ -2,6 +2,6 @@ import querystring;
 sub vcl_recv {
     # return this URL with only the parameters that match this regular expression
     if (req.url !~ "/admin/" && req.url !~ "/search/" && req.url !~ "/bulk_show") {
-      set req.url = querystring.regfilter_except(req.url, "^(a_id|args|article_id|article_ids|articles|asc|callback_url|category|client_id|code|collection_id|commentable_id|commentable_type|confirmation_token|created_at|end|email|filter|followable_id|followable_type|forem_owner_secret|fork_id|i|key|message_offset|name|oauth_token|oauth_verifier|offset|onboarding|org_id|organization_id|p|page|per_page|p_id|prefill|preview|purchaser|q|reactable_ids|redirect_uri|reported_url|reporter_username|response_type|scope|search|signature|sort|source_id|source_type|start|state|status|tag|tag_list|top|type_of|url|username|invitation_token|reset_password_token|ut|verb|invitation_slug|period|comments_sort)$");
+      set req.url = querystring.regfilter_except(req.url, "^(a_id|args|article_id|article_ids|articles|asc|callback_url|category|client_id|code|collection_id|commentable_id|commentable_type|confirmation_token|created_at|end|email|filter|followable_id|followable_type|forem_owner_secret|fork_id|i|key|message_offset|name|oauth_token|oauth_verifier|offset|onboarding|org_id|organization_id|p|page|per_page|p_id|placement_area|prefill|preview|purchaser|q|reactable_ids|redirect_uri|reported_url|reporter_username|response_type|scope|search|signature|sort|source_id|source_type|start|state|status|tag|tag_list|top|type_of|url|username|invitation_token|reset_password_token|ut|verb|invitation_slug|period|comments_sort)$");
     }
 }
diff --git a/config/routes.rb b/config/routes.rb
index 91d677c0c..c40a3a6bf 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -190,6 +190,11 @@ Rails.application.routes.draw do
     get "/async_info/base_data", controller: "async_info#base_data", defaults: { format: :json }
     get "/async_info/navigation_links", controller: "async_info#navigation_links"
 
+    # Display ads
+    scope "/:username/:slug" do
+      get "/display_ads/:placement_area", to: "display_ads#show", as: :article_display_ad
+    end
+
     # Settings
     post "users/join_org", to: "users#join_org"
     post "users/leave_org/:organization_id", to: "users#leave_org", as: :users_leave_org
diff --git a/spec/decorators/article_decorator_spec.rb b/spec/decorators/article_decorator_spec.rb
index 32f38b699..5baa6179c 100644
--- a/spec/decorators/article_decorator_spec.rb
+++ b/spec/decorators/article_decorator_spec.rb
@@ -243,4 +243,15 @@ RSpec.describe ArticleDecorator, type: :decorator do
       expect(article.decorate.pinned?).to be(true)
     end
   end
+
+  describe "#permit_adjacent_sponsors" do
+    it "returns true if there is a no user id" do
+      expect(described_class.new(nil).permit_adjacent_sponsors?).to be(true)
+    end
+
+    it "returns the false if the author of the article has their setting set to false" do
+      article.user.setting.update(permit_adjacent_sponsors: false)
+      expect(article.decorate.permit_adjacent_sponsors?).to be(false)
+    end
+  end
 end
diff --git a/spec/requests/display_ads_spec.rb b/spec/requests/display_ads_spec.rb
new file mode 100644
index 000000000..40c1478c4
--- /dev/null
+++ b/spec/requests/display_ads_spec.rb
@@ -0,0 +1,67 @@
+require "rails_helper"
+
+RSpec.describe "DisplayAds" do
+  let(:user)    { create(:user) }
+  let(:article) { create(:article, user: user) }
+
+  let(:max_age) { 15.minutes.to_i }
+  let(:stale_if_error) { 26_400 }
+
+  describe "GET /:username/:slug/display_ads/:placement_area" do
+    before do
+      create(:display_ad, placement_area: "post_comments", published: true, approved: true)
+    end
+
+    it "returns the correct response" do
+      get article_display_ad_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)
+    end
+
+    context "when signed in" do
+      before do
+        sign_in user
+        get article_display_ad_path(username: article.username, slug: article.slug, placement_area: "post_comments")
+      end
+
+      it "does not set surrogate key headers" do
+        expect(response.headers["Surrogate-key"]).to be_nil
+      end
+
+      it "does not set x-accel-expires headers" do
+        expect(response.headers["x-accel-expires"]).to be_nil
+      end
+
+      it "does not set Fastly cache control and surrogate control headers" do
+        expect(response.headers.to_hash).not_to include(
+          "Cache-Control" => "public, no-cache",
+          "Surrogate-Control" => "max-age=#{max_age}, stale-if-error=#{stale_if_error}",
+        )
+      end
+    end
+
+    context "when signed out" do
+      before do
+        get article_display_ad_path(username: article.username, slug: article.slug, placement_area: "post_comments")
+      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}")
+      end
+
+      it "sets the x-accel-expires header equal to max-age for article" do
+        expect(response.headers["X-Accel-Expires"]).to eq(max_age.to_s)
+      end
+
+      it "sets Fastly cache control and surrogate control headers" do
+        expect(response.headers.to_hash).to include(
+          "Cache-Control" => "public, no-cache",
+          "Surrogate-Control" => "max-age=#{max_age}, stale-if-error=#{stale_if_error}",
+        )
+      end
+    end
+  end
+end
diff --git a/spec/system/articles/user_visits_articles_by_timeframe_spec.rb b/spec/system/articles/user_visits_articles_by_timeframe_spec.rb
index 24167451f..21aa4a29d 100644
--- a/spec/system/articles/user_visits_articles_by_timeframe_spec.rb
+++ b/spec/system/articles/user_visits_articles_by_timeframe_spec.rb
@@ -1,6 +1,5 @@
 require "rails_helper"
 
-# rubocop:disable RSpec/ExampleLength
 RSpec.describe "User visits articles by timeframe", js: true do
   let(:author) { create(:user) }
   let(:minimum_score) { Settings::UserExperience.home_feed_minimum_score + 1 }