From a30b86a0255f29f649f43b6105dafade5878057d Mon Sep 17 00:00:00 2001 From: Ben Halpern Date: Fri, 15 Mar 2024 14:32:30 -0400 Subject: [PATCH] Allow billboards to target specific pages with "fixed bottom" placement (#20748) * Add page fixed bottom billboard * Add page fixed bottom billboard * Fix schema --- .../api/v1/billboards_controller.rb | 2 +- app/controllers/billboards_controller.rb | 3 ++- app/models/billboard.rb | 9 ++++--- app/models/page.rb | 2 ++ app/queries/billboards/filtered_ads_query.rb | 8 +++++- app/views/pages/show.en.html.erb | 3 ++- app/views/pages/show.fr.html.erb | 2 +- app/views/shared/_billboard.html.erb | 8 ++++-- ...173309_add_page_reference_to_billboards.rb | 7 +++++ db/schema.rb | 4 ++- .../billboards/filtered_ads_query_spec.rb | 26 ++++++++++++++----- spec/requests/api/v1/billboards_spec.rb | 6 ++--- spec/requests/billboards_spec.rb | 10 +++++++ 13 files changed, 69 insertions(+), 21 deletions(-) create mode 100644 db/migrate/20240306173309_add_page_reference_to_billboards.rb diff --git a/app/controllers/api/v1/billboards_controller.rb b/app/controllers/api/v1/billboards_controller.rb index 09a9b74c2..88cc8abe5 100644 --- a/app/controllers/api/v1/billboards_controller.rb +++ b/app/controllers/api/v1/billboards_controller.rb @@ -47,7 +47,7 @@ module Api end def permitted_params - params.permit :approved, :body_markdown, :creator_id, :display_to, + params.permit :approved, :body_markdown, :creator_id, :display_to, :page_id, :name, :organization_id, :placement_area, :published, :dismissal_sku, :tag_list, :type_of, :exclude_article_ids, :weight, :requires_cookies, :audience_segment_type, :audience_segment_id, :priority, :special_behavior, diff --git a/app/controllers/billboards_controller.rb b/app/controllers/billboards_controller.rb index 175c5d80e..8ffcb37da 100644 --- a/app/controllers/billboards_controller.rb +++ b/app/controllers/billboards_controller.rb @@ -30,6 +30,7 @@ class BillboardsController < ApplicationController user_signed_in: user_signed_in?, user_id: current_user&.id, article: @article ? ArticleDecorator.new(@article) : nil, + page_id: params[:page_id], user_tags: user_tags, cookies_allowed: cookies_allowed?, location: client_geolocation, @@ -60,7 +61,7 @@ class BillboardsController < ApplicationController end def return_test_billboard? - param_present = params[:bb_test_placement_area] == placement_area && params[:bb_test_id].present? + param_present = params[:bb_test_placement_area] == placement_area && params[:bb_test_id].present? present_and_admin = param_present && current_user&.any_admin? present_and_live = param_present && Billboard.approved_and_published.where(id: params[:bb_test_id]).any? present_and_admin || present_and_live diff --git a/app/models/billboard.rb b/app/models/billboard.rb index d5556b4a0..bf963a477 100644 --- a/app/models/billboard.rb +++ b/app/models/billboard.rb @@ -4,8 +4,8 @@ class Billboard < ApplicationRecord resourcify belongs_to :creator, class_name: "User", optional: true belongs_to :audience_segment, optional: true + belongs_to :page, optional: true - # rubocop:disable Layout/LineLength ALLOWED_PLACEMENT_AREAS = %w[sidebar_left sidebar_left_2 sidebar_right @@ -14,10 +14,10 @@ class Billboard < ApplicationRecord feed_first feed_second feed_third home_hero + page_fixed_bottom post_fixed_bottom post_sidebar post_comments].freeze - # rubocop:enable Layout/LineLength ALLOWED_PLACEMENT_AREAS_HUMAN_READABLE = ["Sidebar Left (First Position)", "Sidebar Left (Second Position)", "Sidebar Right (Home first position)", @@ -27,6 +27,7 @@ class Billboard < ApplicationRecord "Home Feed Second", "Home Feed Third", "Home Hero", + "Fixed Bottom (Page)", "Fixed Bottom (Individual Post)", "Sidebar Right (Individual Post)", "Below the comment section"].freeze @@ -79,7 +80,8 @@ class Billboard < ApplicationRecord self.table_name = "display_ads" - def self.for_display(area:, user_signed_in:, user_id: nil, article: nil, user_tags: nil, location: nil, cookies_allowed: false) + def self.for_display(area:, user_signed_in:, user_id: nil, article: nil, user_tags: nil, + location: nil, cookies_allowed: false, page_id: nil) permit_adjacent = article ? article.permit_adjacent_sponsors? : true billboards_for_display = Billboards::FilteredAdsQuery.call( @@ -91,6 +93,7 @@ class Billboard < ApplicationRecord organization_id: article&.organization_id, permit_adjacent_sponsors: permit_adjacent, user_id: user_id, + page_id: page_id, user_tags: user_tags, location: location, cookies_allowed: cookies_allowed, diff --git a/app/models/page.rb b/app/models/page.rb index 17fce7900..47e5b9952 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -6,6 +6,8 @@ class Page < ApplicationRecord CODE_OF_CONDUCT_SLUG = "code-of-conduct".freeze PRIVACY_SLUG = "privacy".freeze + has_many :billboards, dependent: :nullify + validates :title, presence: true validates :description, presence: true validates :template, inclusion: { in: TEMPLATE_OPTIONS } diff --git a/app/queries/billboards/filtered_ads_query.rb b/app/queries/billboards/filtered_ads_query.rb index df9d4ff1c..80cc6312a 100644 --- a/app/queries/billboards/filtered_ads_query.rb +++ b/app/queries/billboards/filtered_ads_query.rb @@ -9,13 +9,14 @@ module Billboards # @param user_signed_in [Boolean] whether or not the visitor is signed-in # @param billboards [Billboard] 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: [], + def initialize(area:, user_signed_in:, organization_id: nil, article_tags: [], page_id: nil, permit_adjacent_sponsors: true, article_id: nil, billboards: Billboard, user_id: nil, user_tags: nil, location: nil, cookies_allowed: false) @filtered_billboards = billboards.includes([:organization]) @area = area @user_signed_in = user_signed_in @user_id = user_signed_in ? user_id : nil + @page_id = page_id @organization_id = organization_id @article_tags = article_tags @article_id = article_id @@ -28,6 +29,7 @@ module Billboards def call @filtered_billboards = approved_and_published_ads @filtered_billboards = placement_area_ads + @filtered_billboards = page_ads if @page_id.present? @filtered_billboards = cookies_allowed_ads unless @cookies_allowed if @article_id.present? @@ -83,6 +85,10 @@ module Billboards @filtered_billboards.where(placement_area: @area) end + def page_ads + @filtered_billboards.where(page_id: @page_id) + end + def tagged_ads(tag_type) @filtered_billboards.cached_tagged_with_any(tag_type) end diff --git a/app/views/pages/show.en.html.erb b/app/views/pages/show.en.html.erb index 52f122524..6e18311bb 100644 --- a/app/views/pages/show.en.html.erb +++ b/app/views/pages/show.en.html.erb @@ -41,4 +41,5 @@ <%= @page.processed_html.html_safe %> <% end %> -
+
+<%= javascript_include_tag "billboard", defer: true %> diff --git a/app/views/pages/show.fr.html.erb b/app/views/pages/show.fr.html.erb index 52f122524..958bbdf6f 100644 --- a/app/views/pages/show.fr.html.erb +++ b/app/views/pages/show.fr.html.erb @@ -41,4 +41,4 @@ <%= @page.processed_html.html_safe %> <% end %> -
+
diff --git a/app/views/shared/_billboard.html.erb b/app/views/shared/_billboard.html.erb index 81517f918..0ebe7b298 100644 --- a/app/views/shared/_billboard.html.erb +++ b/app/views/shared/_billboard.html.erb @@ -57,7 +57,7 @@ <%= billboard.processed_html.html_safe %> -<% elsif billboard.placement_area == "post_fixed_bottom" %> +<% elsif billboard.placement_area == "post_fixed_bottom" || billboard.placement_area == "page_fixed_bottom" %>
+ data-article-id="<%= @article&.id %>" + <% else %> + data-page-id="<%= @page&.id %>" + <% end %> data-type-of="<%= billboard.type_of %>">
<%= render partial: "shared/billboard_header", locals: { billboard: billboard } %> diff --git a/db/migrate/20240306173309_add_page_reference_to_billboards.rb b/db/migrate/20240306173309_add_page_reference_to_billboards.rb new file mode 100644 index 000000000..324969100 --- /dev/null +++ b/db/migrate/20240306173309_add_page_reference_to_billboards.rb @@ -0,0 +1,7 @@ +class AddPageReferenceToBillboards < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + add_reference :display_ads, :page, type: :bigint, index: { algorithm: :concurrently } + end +end diff --git a/db/schema.rb b/db/schema.rb index 5372b180a..afc8c3823 100644 --- a/db/schema.rb +++ b/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: 2024_03_01_160047) do +ActiveRecord::Schema[7.0].define(version: 2024_03_06_173309) do # These are extensions that must be enabled in order to support this database enable_extension "citext" enable_extension "ltree" @@ -485,6 +485,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_03_01_160047) do t.integer "impressions_count", default: 0 t.string "name" t.bigint "organization_id" + t.bigint "page_id" t.string "placement_area" t.integer "preferred_article_ids", default: [], array: true t.boolean "priority", default: false @@ -501,6 +502,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_03_01_160047) do 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 ["page_id"], name: "index_display_ads_on_page_id" t.index ["placement_area"], name: "index_display_ads_on_placement_area" t.index ["preferred_article_ids"], name: "index_display_ads_on_preferred_article_ids", using: :gin t.index ["target_geolocations"], name: "gist_index_display_ads_on_target_geolocations", using: :gist diff --git a/spec/queries/billboards/filtered_ads_query_spec.rb b/spec/queries/billboards/filtered_ads_query_spec.rb index 52580a4f5..fdb074d6c 100644 --- a/spec/queries/billboards/filtered_ads_query_spec.rb +++ b/spec/queries/billboards/filtered_ads_query_spec.rb @@ -37,13 +37,13 @@ RSpec.describe Billboards::FilteredAdsQuery, type: :query do let!(:no_tags) { create_billboard cached_tag_list: "" } let!(:mismatched) { create_billboard cached_tag_list: "career" } - it "will show no-tag billboards if the article tags do not contain matching tags" do + it "shows no-tag billboards if the article tags do not contain matching tags" do filtered = filter_billboards(article_id: 11, article_tags: %w[javascript]) expect(filtered).not_to include(mismatched) expect(filtered).to include(no_tags) end - it "will show billboards with no tags set if there are no article tags" do + it "shows billboards with no tags set if there are no article tags" do filtered = filter_billboards(article_id: 11, article_tags: []) expect(filtered).not_to include(mismatched) expect(filtered).to include(no_tags) @@ -52,7 +52,7 @@ RSpec.describe Billboards::FilteredAdsQuery, type: :query do context "when available ads have matching tags" do let!(:matching) { create_billboard cached_tag_list: "linux, git, go" } - it "will show the billboards that contain tags that match any of the article tags" do + it "shows the billboards that contain tags that match any of the article tags" do filtered = filter_billboards article_id: 11, article_tags: %w[linux productivity] expect(filtered).not_to include(mismatched) expect(filtered).to include(matching) @@ -65,13 +65,13 @@ RSpec.describe Billboards::FilteredAdsQuery, type: :query do let!(:no_tags) { create_billboard placement_area: "feed_first", cached_tag_list: "" } let!(:mismatched) { create_billboard placement_area: "feed_first", cached_tag_list: "career" } - it "will show no-tag billboards if the user tags do not contain matching tags" do + it "shows no-tag billboards if the user tags do not contain matching tags" do filtered = filter_billboards(area: "feed_first", user_tags: %w[javascript]) expect(filtered).not_to include(mismatched) expect(filtered).to include(no_tags) end - it "will show billboards with no tags set if there are no user tags" do + it "shows billboards with no tags set if there are no user tags" do filtered = filter_billboards(area: "feed_first", user_tags: []) expect(filtered).not_to include(mismatched) expect(filtered).to include(no_tags) @@ -80,7 +80,7 @@ RSpec.describe Billboards::FilteredAdsQuery, type: :query do context "when available ads have matching tags" do let!(:matching) { create_billboard placement_area: "feed_first", cached_tag_list: "linux, git, go" } - it "will show the billboards that contain tags that match any of the user tags" do + it "shows the billboards that contain tags that match any of the user tags" do filtered = filter_billboards area: "feed_first", user_tags: %w[linux productivity] expect(filtered).not_to include(mismatched) expect(filtered).to include(matching) @@ -108,7 +108,7 @@ RSpec.describe Billboards::FilteredAdsQuery, type: :query do let!(:another_ex_article2) { create_billboard exclude_article_ids: "12,13" } let!(:no_excludes) { create_billboard } - it "will show billboards that exclude articles appropriately" do + it "shows billboards that exclude articles appropriately" do filtered = filter_billboards article_id: 11 expect(filtered).to contain_exactly(another_ex_article2, no_excludes) @@ -146,6 +146,18 @@ RSpec.describe Billboards::FilteredAdsQuery, type: :query do end end + context "when considering page_id" do + let(:page) { create(:page) } + let(:page_bb) { create_billboard page_id: page.id } + let(:non_page_bb) { create_billboard page_id: nil } + + it "shows page billboard if page is passed" do + filtered = filter_billboards page_id: page.id + expect(filtered).to contain_exactly(page_bb) + expect(filtered).not_to include(non_page_bb) + end + end + context "when considering ads with organization_id" do let!(:in_house_ad) { create_billboard type_of: :in_house } diff --git a/spec/requests/api/v1/billboards_spec.rb b/spec/requests/api/v1/billboards_spec.rb index 023f093c4..a8604b239 100644 --- a/spec/requests/api/v1/billboards_spec.rb +++ b/spec/requests/api/v1/billboards_spec.rb @@ -52,7 +52,7 @@ RSpec.describe "Api::V1::Billboards" do "placement_area", "processed_html", "published", "success_rate", "tag_list", "type_of", "updated_at", "creator_id", "exclude_article_ids", "dismissal_sku", - "audience_segment_type", "audience_segment_id", + "audience_segment_type", "audience_segment_id", "page_id", "custom_display_label", "template", "render_mode", "preferred_article_ids", "priority", "weight", "target_geolocations", "requires_cookies", "special_behavior") expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC") @@ -72,7 +72,7 @@ RSpec.describe "Api::V1::Billboards" do "placement_area", "processed_html", "published", "success_rate", "tag_list", "type_of", "updated_at", "creator_id", "exclude_article_ids", "dismissal_sku", - "audience_segment_type", "audience_segment_id", + "audience_segment_type", "audience_segment_id", "page_id", "custom_display_label", "template", "render_mode", "preferred_article_ids", "priority", "weight", "target_geolocations", "requires_cookies", "special_behavior") expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC") @@ -137,7 +137,7 @@ RSpec.describe "Api::V1::Billboards" do "impressions_count", "name", "organization_id", "placement_area", "processed_html", "published", "dismissal_sku", "success_rate", "tag_list", "type_of", "updated_at", - "creator_id", "exclude_article_ids", "requires_cookies", + "creator_id", "exclude_article_ids", "requires_cookies", "page_id", "audience_segment_type", "audience_segment_id", "special_behavior", "custom_display_label", "template", "render_mode", "preferred_article_ids", "priority", "weight", "target_geolocations") diff --git a/spec/requests/billboards_spec.rb b/spec/requests/billboards_spec.rb index 713420d0b..8c2f1d1ad 100644 --- a/spec/requests/billboards_spec.rb +++ b/spec/requests/billboards_spec.rb @@ -180,6 +180,16 @@ RSpec.describe "Billboards" do end end + context "when the placement area is page_fixed_bottom" do + let(:page) { create(:page) } + + it "contains close button" do + billboard = create_billboard(placement_area: "page_fixed_bottom", page_id: page.id) + get billboard_path(page_id: page.id, placement_area: "page_fixed_bottom") + expect(response.body).to include "sponsorship-close-trigger-#{billboard.id}" + end + end + context "when the placement area is feed_first" do it "includes sponsorship-close-trigger when there is a dismissal_sku" do billboard = create_billboard(placement_area: "feed_first", dismissal_sku: "DISMISS_ME")