maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Allow billboards to target specific pages with "fixed bottom" placement (#20748)

Browse source 
* Add page fixed bottom billboard

* Add page fixed bottom billboard

* Fix schema
This commit is contained in:
Ben Halpern 2024-03-15 14:32:30 -04:00 • committed by GitHub
parent 53f311b059
commit a30b86a025
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 69 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
app/controllers/api/v1/billboards_controller.rb
View file

@ -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,

3
app/controllers/billboards_controller.rb
View file

@ -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

9
app/models/billboard.rb
View file

@ -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,

2
app/models/page.rb
View file

@ -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 }

8
app/queries/billboards/filtered_ads_query.rb
View file

@ -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

3
app/views/pages/show.en.html.erb
View file

@ -41,4 +41,5 @@
<%= @page.processed_html.html_safe %>
<% end %>
</main>
<div id="IS_CENTERED_PAGE"></div>
<div class="js-billboard-container pb-4 crayons-layout__comments-billboard" data-async-url="<%= billboard_path(page_id: @page.id, placement_area: :page_fixed_bottom) %>"></div>
<%= javascript_include_tag "billboard", defer: true %>

2
app/views/pages/show.fr.html.erb
View file

@ -41,4 +41,4 @@
<%= @page.processed_html.html_safe %>
<% end %>
</main>
<div id="IS_CENTERED_PAGE"></div>
<div class="js-billboard-container pb-4 crayons-layout__comments-billboard" data-async-url="<%= billboard_path(page_id: @page.id, placement_area: :page_fixed_bottom) %>"></div>

8
app/views/shared/_billboard.html.erb
View file

@ -57,7 +57,7 @@
<%= billboard.processed_html.html_safe %>
</div>
</div>
<% elsif billboard.placement_area == "post_fixed_bottom" %>
<% elsif billboard.placement_area == "post_fixed_bottom" || billboard.placement_area == "page_fixed_bottom" %>
<div class="crayons-card crayons-card--secondary crayons-sponsorship popover-billboard crayons-story__billboard js-billboard fixed bottom-0 left-0 right-0"
data-display-unit data-id="<%= billboard.id %>"
data-category-click="<%= BillboardEvent::CATEGORY_CLICK %>"
@ -65,7 +65,11 @@
data-context-type="<%= data_context_type %>"
data-dismissal-sku="<%= billboard.dismissal_sku %>"
data-special="<%= billboard.special_behavior %>"
data-article-id="<%= @article&.id %>"
<% if billboard.placement_area == "post_fixed_bottom" && @article.present? %>
data-article-id="<%= @article&.id %>"
<% else %>
data-page-id="<%= @page&.id %>"
<% end %>
data-type-of="<%= billboard.type_of %>">
<div class="flex">
<%= render partial: "shared/billboard_header", locals: { billboard: billboard } %>

7
db/migrate/20240306173309_add_page_reference_to_billboards.rb Normal file
View file

@ -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

4
db/schema.rb
View file

@ -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

26
spec/queries/billboards/filtered_ads_query_spec.rb
View file

@ -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 }

6
spec/requests/api/v1/billboards_spec.rb
View file

@ -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")

10
spec/requests/billboards_spec.rb
View file

@ -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")