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

Add new sidebar billboard locations and remove legacy campaign area (#20562)

Browse source 
* Add new sidebar billboard locations

* Remove some specs

* Remove campaign tests

* Adjust tag-allowed billboards

* Fix up targeting functionality

* Fix sidebar third render
This commit is contained in:
Ben Halpern 2024-02-08 11:55:48 -05:00 • committed by GitHub
parent 55f191a5ec
commit 2f9769a89d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 70 additions and 124 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
app/controllers/application_controller.rb
View file

@ -271,6 +271,14 @@ class ApplicationController < ActionController::Base
end
end
def client_geolocation
if session_current_user_id
request.headers["X-Client-Geo"]
else
request.headers["X-Cacheable-Client-Geo"]
end
end
def forward_to_app_config_domain
# Let's only redirect get requests for this purpose.
return unless request.get? &&

8
app/controllers/application_metal_controller.rb
View file

@ -15,4 +15,12 @@ class ApplicationMetalController < ActionController::Metal
def logger
ActionController::Base.logger
end
def client_geolocation
if session_current_user_id
request.headers["X-Client-Geo"]
else
request.headers["X-Cacheable-Client-Geo"]
end
end
end

10
app/controllers/billboard_events_controller.rb
View file

@ -41,14 +41,4 @@ class BillboardEventsController < ApplicationMetalController
event_params[:geolocation] = client_geolocation
event_params.slice(:context_type, :category, :billboard_id, :article_id, :geolocation)
end
def client_geolocation
# Copied here instead of re-used due to this controller
# inhereting from ApplicationMetalController instead of ApplicationController
if session_current_user_id
request.headers["X-Client-Geo"]
else
request.headers["X-Cacheable-Client-Geo"]
end
end
end

8
app/controllers/billboards_controller.rb
View file

@ -59,14 +59,6 @@ class BillboardsController < ApplicationController
current_user&.cached_followed_tag_names&.first(rand(RANDOM_USER_TAG_RANGE_MIN..RANDOM_USER_TAG_RANGE_MAX))
end
def client_geolocation
if session_current_user_id
request.headers["X-Client-Geo"]
else
request.headers["X-Cacheable-Client-Geo"]
end
end
def return_test_billboard?
param_present = params[:bb_test_placement_area] == placement_area && params[:bb_test_id].present?
present_and_admin = param_present && current_user&.any_admin?

20
app/helpers/billboard_helper.rb
View file

@ -1,4 +1,7 @@
module BillboardHelper
RANDOM_USER_TAG_RANGE_MIN = 5
RANDOM_USER_TAG_RANGE_MAX = 32
def billboards_placement_area_options_array
Billboard::ALLOWED_PLACEMENT_AREAS_HUMAN_READABLE.zip(Billboard::ALLOWED_PLACEMENT_AREAS)
end
@ -25,4 +28,21 @@ module BillboardHelper
def feed_targeted_tag_placement?(area)
Billboard::HOME_FEED_PLACEMENTS.include?(area)
end
# Including here because multiple controllers render this in different contexts
# When signed in, this is uncached, but it is cached for signed-out contexts
def get_homepage_sidebar_billboards
user_tags = current_user&.cached_followed_tag_names
&.first(rand(RANDOM_USER_TAG_RANGE_MIN..RANDOM_USER_TAG_RANGE_MAX))
common_params = {
user_signed_in: user_signed_in?,
user_id: current_user&.id,
user_tags: user_tags
}
common_params[:location] = client_geolocation if user_signed_in? && FeatureFlag.enabled?(Geolocation::FEATURE_FLAG)
billboards = []
billboards << Billboard.for_display(**common_params, area: "sidebar_right")
billboards << Billboard.for_display(**common_params, area: "sidebar_right_second")
billboards << Billboard.for_display(**common_params, area: "sidebar_right_third")
end
end

3
app/javascript/packs/admin/billboards.jsx
View file

@ -144,6 +144,9 @@ document.ready.then(() => {
const targetedTagPlacements = [
'post_comments',
'post_sidebar',
'sidebar_right',
'sidebar_right_second',
'sidebar_right_third',
'feed_first',
'feed_second',
'feed_third',

6
app/models/billboard.rb
View file

@ -6,11 +6,13 @@ class Billboard < ApplicationRecord
belongs_to :audience_segment, optional: true
# rubocop:disable Layout/LineLength
ALLOWED_PLACEMENT_AREAS = %w[sidebar_left sidebar_left_2 sidebar_right feed_first feed_second feed_third home_hero post_sidebar post_comments].freeze
ALLOWED_PLACEMENT_AREAS = %w[sidebar_left sidebar_left_2 sidebar_right sidebar_right_second sidebar_right_third feed_first feed_second feed_third home_hero 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)",
"Sidebar Right (Home first position)",
"Sidebar Right (Home second position)",
"Sidebar Right (Home third position)",
"Home Feed First",
"Home Feed Second",
"Home Feed Third",

16
app/views/sidebars/_homepage_content.html.erb
View file

@ -1,7 +1,7 @@
<% @billboards = get_homepage_sidebar_billboards %>
<aside class="side-bar sidebar-additional showing grid gap-4" id="sidebar-additional">
<% @sidebar_billboard = Billboard.for_display(area: "sidebar_right", user_signed_in: user_signed_in?) %>
<% if @sidebar_billboard %>
<%= render partial: "shared/billboard", locals: { billboard: @sidebar_billboard, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %>
<% if @billboards.first %>
<%= render partial: "shared/billboard", locals: { billboard: @billboards.first, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %>
<% end %>
<% if user_signed_in? && @active_discussions.any? %>
<% if Settings::General.display_sidebar_active_discussions %>
@ -20,10 +20,11 @@
<% end %>
<% end %>
<% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}-#{user_signed_in?}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %>
<% if @billboards.second %>
<%= render partial: "shared/billboard", locals: { billboard: @billboards.second, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %>
<% end %>
<%= render "articles/sidebar_campaign" if Campaign.current.show_in_sidebar? %>
<%= render "articles/sidebar_listings" if Listing.feature_enabled? %>
<% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}-#{user_signed_in?}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %>
<% Tag.where(name: Settings::General.sidebar_tags).find_each do |tag| %>
<section class="crayons-card crayons-card--secondary crayons-layout__content">
@ -47,6 +48,9 @@
</section>
<% end %>
<% end %>
<% if @billboards.third %>
<%= render partial: "shared/billboard", locals: { billboard: @billboards.third, data_context_type: BillboardEvent::CONTEXT_TYPE_HOME } %>
<% end %>
<% unless user_signed_in? %>
<% cache("seo-boostable-posts-homepage-#{params[:timeframe]}", expires_in: 18.hours) do %>
<% boostable_posts = Article.seo_boostable(nil, Timeframe.datetime(params[:timeframe])) %>

1
cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js
View file

@ -14,7 +14,6 @@ describe('Billboards Form', () => {
describe('Targeted Tags field', () => {
[
'Sidebar Right (Home)',
'Sidebar Left (First Position)',
'Sidebar Left (Second Position)',
'Home Hero',

15
cypress/e2e/seededFlows/listingFlows/viewListing.spec.js
View file

@ -6,21 +6,6 @@ describe('View listing', () => {
cy.visit('/');
});
it('opens a listing from the Feed page', () => {
cy.findByText('Listing title').click();
cy.findByTestId('listings-modal').as('listingsModal');
cy.get('@listingsModal').findByText('Listing').should('exist');
cy.get('@listingsModal').findByText('Listing title').should('exist');
cy.get('@listingsModal')
.findAllByRole('button')
.first()
.should('have.focus');
cy.get('@listingsModal').findByRole('button', { name: /Close/ }).click();
cy.get('@listingsModal').should('not.exist');
});
it('opens a listing from the listings page', () => {
cy.intercept(
'/search/listings?category=&listing_search=&page=0&per_page=75&tag_boolean_mode=all',

7
spec/requests/listings_spec.rb
View file

@ -66,13 +66,6 @@ RSpec.describe "/listings" do
end
end
context "when the user has category and slug params for active listing" do
it "shows that direct listing" do
get "/listings", params: { category: listing.category, slug: listing.slug }
expect(response.body).to include(CGI.escapeHTML(listing.title))
end
end
context "when the user has category and slug params for expired listing" do
it "shows only active listings from that category" do
expired_listing.published = false

1
spec/requests/sidebars_spec.rb
View file

@ -8,7 +8,6 @@ RSpec.describe "Sidebars" do
allow(Settings::General).to receive(:sidebar_tags).and_return(["rubymagoo"])
get "/sidebars/home"
expect(response.body).to include("rubymagoo")
expect(response.body).to include(CGI.escapeHTML(listing.title))
end
context "when active discussions exist" do

91
spec/requests/stories_index_spec.rb
View file

@ -79,7 +79,7 @@ RSpec.describe "StoriesIndex" do
expect(response.body).to include("This is a landing page!")
end
it "renders billboards when published and approved" do
it "renders billboards when published and approved for sidebar right (first position)" do
ad = create(:billboard, published: true, approved: true, placement_area: "sidebar_right",
organization: org)
@ -87,6 +87,22 @@ RSpec.describe "StoriesIndex" do
expect(response.body).to include(ad.processed_html)
end
it "renders billboards when published and approved for sidebar right (second position)" do
ad = create(:billboard, published: true, approved: true, placement_area: "sidebar_right_second",
organization: org)
get "/"
expect(response.body).to include(ad.processed_html)
end
it "renders billboards when published and approved for sidebar right (third position)" do
ad = create(:billboard, published: true, approved: true, placement_area: "sidebar_right_third",
organization: org)
get "/"
expect(response.body).to include(ad.processed_html)
end
it "does not render billboards when not approved" do
ad = create(:billboard, published: true, approved: false, placement_area: "sidebar_right",
organization: org)
@ -114,13 +130,6 @@ RSpec.describe "StoriesIndex" do
expect(response.body).to include(billboard.processed_html)
end
it "shows listings" do
user = create(:user)
listing = create(:listing, user_id: user.id)
get "/"
expect(response.body).to include(CGI.escapeHTML(listing.title))
end
it "does not set cache-related headers if private" do
allow(Settings::UserExperience).to receive(:public).and_return(false)
get "/"
@ -215,72 +224,6 @@ RSpec.describe "StoriesIndex" do
end
end
context "with campaign_sidebar" do
before do
allow(Settings::Campaign).to receive(:featured_tags).and_return("mytag,yourtag")
allow(Settings::UserExperience).to receive(:home_feed_minimum_score).and_return(7)
a_body = "---\ntitle: Super-sheep#{rand(1000)}\npublished: true\ntags: heyheyhey,mytag\n---\n\nHello"
create(:article, approved: true, body_markdown: a_body, score: 1)
u_body = "---\ntitle: Unapproved-post#{rand(1000)}\npublished: true\ntags: heyheyhey,mytag\n---\n\nHello"
create(:article, approved: false, body_markdown: u_body, score: 1)
end
it "displays display name when it is set" do
allow(Settings::Campaign).to receive(:display_name).and_return("Backstreet is back")
get "/"
expect(response.body).not_to include("Backstreet is back (0)")
end
it "displays Stories fallback when display name is not set" do
allow(Settings::Campaign).to receive(:display_name).and_return("")
get "/"
expect(response.body).not_to include("Stories (0)")
end
it "doesn't display posts with the campaign tags when sidebar is disabled" do
allow(Settings::Campaign).to receive(:sidebar_enabled).and_return(false)
get "/"
expect(response.body).not_to include(CGI.escapeHTML("Super-sheep"))
end
it "doesn't display unapproved posts" do
allow(Settings::Campaign).to receive_messages(sidebar_enabled: true,
sidebar_image: "https://example.com/image.png",
articles_require_approval: true)
Article.last.update_column(:score, -2)
get "/"
expect(response.body).not_to include(CGI.escapeHTML("Unapproved-post"))
end
it "displays unapproved post if approval is not required" do
allow(Settings::Campaign).to receive_messages(sidebar_enabled: true,
sidebar_image: "https://example.com/image.png",
articles_require_approval: false)
get "/"
expect(response.body).to include(CGI.escapeHTML("Unapproved-post"))
end
it "displays only approved posts with the campaign tags" do
allow(Settings::Campaign).to receive(:sidebar_enabled).and_return(false)
get "/"
expect(response.body).not_to include(CGI.escapeHTML("Super-puper"))
end
it "displays sidebar url if url is set" do
allow(Settings::Campaign).to receive_messages(sidebar_enabled: true, url: "https://campaign-lander.com",
sidebar_image: "https://example.com/image.png")
get "/"
expect(response.body).to include('<a href="https://campaign-lander.com"')
end
it "does not display sidebar url if image is not present is set" do
allow(Settings::Campaign).to receive_messages(sidebar_enabled: true, url: "https://campaign-lander.com")
get "/"
expect(response.body).not_to include('<a href="https://campaign-lander.com"')
end
end
context "with default_locale configured to fr" do
before do
allow(Settings::UserExperience).to receive(:default_locale).and_return("fr")