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

Fix inconsistencies with DisplayAd (#14789)

Browse source 
* Fix inconsistencies with DisplayAd

* Remove console log

* Fix tests

* Fix width/height being backwards

* Stub FastImage

* Update app/services/html/parser.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Update spec/factories/display_ads.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Fix display language

* Add check for img

* Change name of rotatingDisplayUnits

* Update app/helpers/display_ad_helper.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

Co-authored-by: Michael Kohl <citizen428@forem.com>
This commit is contained in:
Ben Halpern 2021-09-28 09:22:07 -04:00 • committed by GitHub
parent 5fa520387d
commit a5e8ed66ee
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 108 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
app/assets/javascripts/initializers/initializeBaseTracking.js.erb
View file

@ -72,7 +72,7 @@ function trackCustomImpressions() {
var ArticleElement = document.getElementById('article-body') || document.getElementById('comment-article-indicator');
var tokenMeta = document.querySelector("meta[name='csrf-token']")
var isBot = /bot|google|baidu|bing|msn|duckduckbot|teoma|slurp|yandex/i.test(navigator.userAgent) // is crawler
var windowBigEnough = window.innerWidth > 1250
var windowBigEnough = window.innerWidth > 1023
// Sidebar HTML variant tracking
var stickyNav = document.getElementById('article-show-primary-sticky-nav');
@ -136,16 +136,18 @@ function trackCustomImpressions() {
}
// display add
var adBox = document.getElementById('sponsorship-arbitrary-display-widget')
if (adBox
var displayAds = document.querySelectorAll('[data-display-unit]');
if (displayAds.length > 0
&& tokenMeta
&& !isBot
&& windowBigEnough
&& checkUserLoggedIn()) {
var csrfToken = tokenMeta.getAttribute('content');
trackAdImpression(csrfToken, adBox)
adBox.removeEventListener('click', trackAdClick, false );
adBox.addEventListener('click', function() { trackAdClick(csrfToken, adBox) });
[].forEach.call(displayAds, function(unit) {
trackAdImpression(csrfToken, unit);
unit.removeEventListener('click', trackAdClick, false );
unit.addEventListener('click', function(e) { trackAdClick(csrfToken, e) });
});
}
}, 1800)
}

1
app/assets/stylesheets/widgets.scss
View file

@ -360,5 +360,6 @@
border-radius: var(--radius);
display: inline-block;
vertical-align: middle;
margin: 10px auto;
}
}

5
app/helpers/display_ad_helper.rb Normal file
View file

@ -0,0 +1,5 @@
module DisplayAdHelper
def display_ads_placement_area_options_array
DisplayAd::ALLOWED_PLACEMENT_AREAS_HUMAN_READABLE.zip(DisplayAd::ALLOWED_PLACEMENT_AREAS)
end
end

29
app/models/display_ad.rb
View file

@ -1,17 +1,28 @@
class DisplayAd < ApplicationRecord
resourcify
ALLOWED_PLACEMENT_AREAS = %w[sidebar_left sidebar_left_2 sidebar_right].freeze
ALLOWED_PLACEMENT_AREAS_HUMAN_READABLE = ["Sidebar Left (First Position)",
"Sidebar Left (Second Position)",
"Sidebar Right"].freeze
belongs_to :organization
has_many :display_ad_events, dependent: :destroy
validates :organization_id, presence: true
validates :placement_area, presence: true,
inclusion: { in: %w[sidebar_left sidebar_right] }
inclusion: { in: ALLOWED_PLACEMENT_AREAS }
validates :body_markdown, presence: true
before_save :process_markdown
scope :approved_and_published, -> { where(approved: true, published: true) }
ALLOWED_TAGS = %w[
a abbr add b blockquote br center cite code col colgroup dd del dl dt em figcaption
h1 h2 h3 h4 h5 h6 hr img kbd li mark ol p pre q rp rt ruby small source span strong sub sup table
tbody td tfoot th thead time tr u ul video].freeze
ALLOWED_ATTRIBUTES = %w[href src alt height width].freeze
def self.for_display(area)
relation = approved_and_published.where(placement_area: area).order(success_rate: :desc)
@ -22,20 +33,20 @@ class DisplayAd < ApplicationRecord
end
end
def human_readable_placement_area
ALLOWED_PLACEMENT_AREAS_HUMAN_READABLE[ALLOWED_PLACEMENT_AREAS.find_index(placement_area)]
end
private
def process_markdown
renderer = Redcarpet::Render::HTMLRouge.new(hard_wrap: true, filter_html: false)
markdown = Redcarpet::Markdown.new(renderer)
initial_html = markdown.render(body_markdown)
# Temporarily disable the sanitisation in order to launch the SheCoded Campaign.
# TODO: find an alternate solution.
# stripped_html = ActionController::Base.helpers.sanitize initial_html,
# tags: %w[a em i b u br img h1 h2 h3 h4 div style],
# attributes: %w[href target src height width style]
stripped_html = initial_html.html_safe # rubocop:disable Rails/OutputSafety
stripped_html = ActionController::Base.helpers.sanitize initial_html,
tags: ALLOWED_TAGS,
attributes: ALLOWED_ATTRIBUTES
html = stripped_html.delete("\n")
self.processed_html = Html::Parser.new(html).prefix_all_images(350).html
self.processed_html = Html::Parser.new(html).prefix_all_images(350, synchronous_detail_detection: true).html
end
end

16
app/services/html/parser.rb
View file

@ -16,6 +16,7 @@ module Html
RAW_TAG_DELIMITERS = ["{", "}", "raw", "endraw", "----"].freeze
RAW_TAG = "{----% raw %----}".freeze
END_RAW_TAG = "{----% endraw %----}".freeze
TIMEOUT = 10
attr_accessor :html
private :html=
@ -32,7 +33,7 @@ module Html
self
end
def prefix_all_images(width = 880)
def prefix_all_images(width = 880, synchronous_detail_detection: false)
# wrap with Cloudinary or allow if from giphy or githubusercontent.com
doc = Nokogiri::HTML.fragment(@html)
@ -42,6 +43,12 @@ module Html
# allow image to render as-is
next if allowed_image_host?(src)
if synchronous_detail_detection && img
width, height = image_width_height(img)
img["width"] = width
img["height"] = height
end
img["loading"] = "lazy"
img["src"] = if Giphy::Image.valid_url?(src)
src.gsub("https://media.", "https://i.")
@ -55,6 +62,13 @@ module Html
self
end
def image_width_height(img)
src = img.attr("src")
return unless src
FastImage.size(src, timeout: TIMEOUT)
end
def wrap_all_images_in_links
doc = Nokogiri::HTML.fragment(@html)

2
app/views/admin/display_ads/_form.html.erb
View file

@ -10,7 +10,7 @@
<div class="form-group">
<%= label_tag :placement_area, "Placement Area:" %>
<%= select_tag :placement_area, options_for_select(%w[sidebar_left sidebar_right], selected: @display_ad.placement_area), include_blank: true %>
<%= select_tag :placement_area, options_for_select(display_ads_placement_area_options_array, selected: @display_ad.placement_area), include_blank: true %>
</div>
<div class="form-group">

4
app/views/admin/display_ads/index.html.erb
View file

@ -17,7 +17,6 @@
<th scope="col">ID</th>
<th scope="col">Organization</th>
<th scope="col">Placement Area</th>
<th scope="col">Body Markdown</th>
<th scope="col">Published</th>
<th scope="col">Approved</th>
<th scope="col">Success Rate</th>
@ -28,8 +27,7 @@
<tr>
<td><%= link_to display_ad.id, edit_admin_display_ad_path(display_ad) %></td>
<td><%= link_to display_ad.organization.name, admin_organization_path(display_ad.organization) %></td>
<td><%= display_ad.placement_area %></td>
<td><%= display_ad.body_markdown.truncate(30) %></td>
<td><%= display_ad.human_readable_placement_area %></td>
<td><%= display_ad.published %></td>
<td><%= display_ad.approved %></td>
<td><%= display_ad.success_rate %></td>

10
app/views/articles/_sidebar.html.erb
View file

@ -22,10 +22,18 @@
<% cache("display-area-left-#{rand(5)}", expires_in: 5.minutes) do %>
<% @left_sidebar_ad = DisplayAd.for_display("sidebar_left") %>
<% if @left_sidebar_ad %>
<div class="crayons-sponsorship-widget" id="sponsorship-arbitrary-display-widget" data-id="<%= @left_sidebar_ad.id %>">
<div class="crayons-card crayons-card--secondary p-3 crayons-sponsorship-widget" data-display-unit data-id="<%= @left_sidebar_ad.id %>">
<%= @left_sidebar_ad.processed_html.html_safe %>
</div>
<% end %>
<% end %>
<% cache("display-area-left-2-#{rand(5)}", expires_in: 5.minutes) do %>
<% @second_left_sidebar_ad = DisplayAd.for_display("sidebar_left_2") %>
<% if @second_left_sidebar_ad %>
<div class="mt-4 crayons-card crayons-card--secondary p-3 crayons-sponsorship-widget" data-display-unit data-id="<%= @second_left_sidebar_ad.id %>">
<%= @second_left_sidebar_ad.processed_html.html_safe %>
</div>
<% end %>
<% end %>
</aside>
</div>

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

@ -2,7 +2,7 @@
<% cache(release_adjusted_cache_key("main-article-right-sidebar-discussions-#{params[:timeframe]}"), expires_in: (params[:timeframe].blank? ? 120 : 360).seconds) do %>
<% @sidebar_ad = DisplayAd.for_display("sidebar_right") %>
<% if @sidebar_ad %>
<div class="crayons-card crayons-card--secondary p-4 crayons-sponsorship-widget">
<div class="crayons-card crayons-card--secondary p-4 crayons-sponsorship-widget" data-display-unit data-id="<%= @sidebar_ad.id %>">
<%= @sidebar_ad.processed_html.html_safe %>
</div>
<% end %>

2
spec/factories/display_ads.rb
View file

@ -1,7 +1,7 @@
FactoryBot.define do
factory :display_ad do
placement_area { "sidebar_left" }
body_markdown { "Hello _hey_ Hey hey" }
sequence(:body_markdown) { |n| "Hello _hey_ Hey hey #{n}" }
organization
end
end

9
spec/helpers/display_ad_helper_spec.rb Normal file
View file

@ -0,0 +1,9 @@
require "rails_helper"
describe DisplayAdHelper, type: :helper do
describe ".display_ads_placement_area_options_array" do
it "returns proper human value" do
expect(helper.display_ads_placement_area_options_array[1]).to eq ["Sidebar Left (Second Position)", "sidebar_left_2"]
end
end
end

17
spec/models/display_ad_spec.rb
View file

@ -30,11 +30,26 @@ RSpec.describe DisplayAd, type: :model do
display_ad.placement_area = "tsdsdsdds"
expect(display_ad).not_to be_valid
end
it "returns human readable name" do
display_ad.placement_area = "sidebar_left_2"
expect(display_ad.human_readable_placement_area).to eq("Sidebar Left (Second Position)")
end
end
context "when callbacks are triggered before save" do
it "generates #processed_html from #body_markdown" do
expect(display_ad.processed_html).to eq("<p>Hello <em>hey</em> Hey hey</p>")
expect(display_ad.processed_html).to start_with("<p>Hello <em>hey</em> Hey hey")
end
it "does not render disallowed tags" do
display_ad.update(body_markdown: "<style>body { color: black}</style> Hey hey")
expect(display_ad.processed_html).to eq("<p>body { color: black} Hey hey</p>")
end
it "does not render disallowed attributes" do
display_ad.update(body_markdown: "<p style='margin-top:100px'>Hello <em>hey</em> Hey hey</p>")
expect(display_ad.processed_html).to start_with("<p>Hello <em>hey</em> Hey hey</p>")
end
end

16
spec/requests/stories_index_spec.rb
View file

@ -59,14 +59,16 @@ RSpec.describe "StoriesIndex", type: :request do
expect(response.body).to include("This is a landing page!")
end
it "renders all display_ads when published and approved" do
it "renders all display_ads of different placements when published and approved" do
org = create(:organization)
ad = create(:display_ad, published: true, approved: true, organization: org)
ad = create(:display_ad, published: true, approved: true, organization: org, placement_area: "sidebar_left")
second_left_ad = create(:display_ad, published: true, approved: true, organization: org, placement_area: "sidebar_left_2")
right_ad = create(:display_ad, published: true, approved: true, placement_area: "sidebar_right",
organization: org)
get "/"
expect(response.body).to include(ad.processed_html)
expect(response.body).to include(second_left_ad.processed_html)
expect(response.body).to include(right_ad.processed_html)
end
@ -81,6 +83,16 @@ RSpec.describe "StoriesIndex", type: :request do
expect(response.body).not_to include(right_ad.processed_html)
end
it "renders only one display ad of placement" do
org = create(:organization)
left_ad = create(:display_ad, published: true, approved: true, placement_area: "sidebar_left", organization: org)
second_left_ad = create(:display_ad, published: true, approved: true, placement_area: "sidebar_left", organization: org)
get "/"
expect(response.body).to include(left_ad.processed_html).or(include(second_left_ad.processed_html))
expect(response.body).to include("crayons-sponsorship-widget").once
end
it "displays correct sponsors", :aggregate_failures do
org = create(:organization)
gold_sponsorship = create(:sponsorship, level: "gold", tagline: "GOLD!!!", status: "live", organization: org)

7
spec/services/html/parser_spec.rb
View file

@ -70,6 +70,13 @@ RSpec.describe Html::Parser, type: :service do
parsed_html = described_class.new(html).prefix_all_images.html
expect(parsed_html).to include("https://res.cloudinary.com")
end
it "detects height and width" do
allow(FastImage).to receive(:size).and_return([100,200])
html = "<img src='https://image.com/image.jpg'>"
parsed_html = described_class.new(html).prefix_all_images(350, synchronous_detail_detection: true).html
expect(parsed_html).to include("height=\"200\"")
end
end
end