From 60e624e76845d5a0359315204121bda48f98d2c5 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Sat, 7 Oct 2023 19:54:26 +0200 Subject: [PATCH] DEV: Replace custom Onebox blank implementation with ActiveSupport (#23827) We have a custom implementation of #blank? in our Onebox helpers. This is likely a legacy from when Onebox was a standalone gem. This change replaces all usages with respective incarnations of #blank?, #present?, and #presence from ActiveSupport. It changes a bunch of "unless blank" to "if present" as well. --- .../engine/allowlisted_generic_onebox.rb | 39 +++++++++---------- lib/onebox/engine/audio_com_onebox.rb | 2 +- lib/onebox/engine/facebook_media_onebox.rb | 3 +- lib/onebox/engine/google_docs_onebox.rb | 2 +- lib/onebox/engine/reddit_media_onebox.rb | 3 +- lib/onebox/engine/simplecast_onebox.rb | 2 +- lib/onebox/engine/sound_cloud_onebox.rb | 2 +- lib/onebox/engine/standard_embed.rb | 16 ++++---- lib/onebox/engine/wistia_onebox.rb | 2 +- lib/onebox/engine/youtube_onebox.rb | 2 +- lib/onebox/helpers.rb | 18 ++------- lib/onebox/json_ld.rb | 2 +- lib/onebox/normalizer.rb | 5 +-- lib/onebox/open_graph.rb | 8 ++-- spec/lib/onebox/helpers_spec.rb | 13 ------- 15 files changed, 42 insertions(+), 77 deletions(-) diff --git a/lib/onebox/engine/allowlisted_generic_onebox.rb b/lib/onebox/engine/allowlisted_generic_onebox.rb index 9f30d923de9..235102591ae 100644 --- a/lib/onebox/engine/allowlisted_generic_onebox.rb +++ b/lib/onebox/engine/allowlisted_generic_onebox.rb @@ -94,18 +94,18 @@ module Onebox html_entities = HTMLEntities.new d = { link: link }.merge(raw) - if !Onebox::Helpers.blank?(d[:title]) + if d[:title].present? d[:title] = html_entities.decode(Onebox::Helpers.truncate(d[:title], 80)) end d[:description] ||= d[:summary] - if !Onebox::Helpers.blank?(d[:description]) + if d[:description].present? d[:description] = html_entities.decode(Onebox::Helpers.truncate(d[:description], 250)) end - if !Onebox::Helpers.blank?(d[:site_name]) + if d[:site_name].present? d[:domain] = html_entities.decode(Onebox::Helpers.truncate(d[:site_name], 80)) - elsif !Onebox::Helpers.blank?(d[:domain]) + elsif d[:domain].present? d[:domain] = "http://#{d[:domain]}" unless d[:domain] =~ %r{^https?://} d[:domain] = begin URI(d[:domain]).host.to_s.sub(/^www\./, "") @@ -118,15 +118,14 @@ module Onebox d[:image] = d[:image_secure_url] || d[:image_url] || d[:thumbnail_url] || d[:image] d[:image] = Onebox::Helpers.get_absolute_image_url(d[:image], @url) d[:image] = Onebox::Helpers.normalize_url_for_output(html_entities.decode(d[:image])) - d[:image] = nil if Onebox::Helpers.blank?(d[:image]) + d[:image] = nil if d[:image].blank? d[:video] = d[:video_secure_url] || d[:video_url] || d[:video] - d[:video] = nil if Onebox::Helpers.blank?(d[:video]) + d[:video] = nil if d[:video].blank? - d[:published_time] = d[:article_published_time] unless Onebox::Helpers.blank?( - d[:article_published_time], - ) - if !Onebox::Helpers.blank?(d[:published_time]) + d[:published_time] = d[:article_published_time] if d[:article_published_time].present? + + if d[:published_time].present? d[:article_published_time] = Time.parse(d[:published_time]).strftime("%-d %b %y") d[:article_published_time_title] = Time.parse(d[:published_time]).strftime( "%I:%M%p - %d %B %Y", @@ -134,18 +133,18 @@ module Onebox end # Twitter labels - if !Onebox::Helpers.blank?(d[:label1]) && !Onebox::Helpers.blank?(d[:data1]) && + if d[:label1].present? && d[:data1].present? && !!AllowlistedGenericOnebox.allowed_twitter_labels.find { |l| d[:label1] =~ /#{l}/i } d[:label_1] = Onebox::Helpers.truncate(d[:label1]) d[:data_1] = Onebox::Helpers.truncate(d[:data1]) end - if !Onebox::Helpers.blank?(d[:label2]) && !Onebox::Helpers.blank?(d[:data2]) && + if d[:label2].present? && d[:data2].present? && !!AllowlistedGenericOnebox.allowed_twitter_labels.find { |l| d[:label2] =~ /#{l}/i } - if Onebox::Helpers.blank?(d[:label_1]) + if d[:label_1].blank? d[:label_1] = Onebox::Helpers.truncate(d[:label2]) d[:data_1] = Onebox::Helpers.truncate(d[:data2]) else @@ -154,8 +153,7 @@ module Onebox end end - if Onebox::Helpers.blank?(d[:label_1]) && !Onebox::Helpers.blank?(d[:price_amount]) && - !Onebox::Helpers.blank?(d[:price_currency]) + if d[:label_1].blank? && d[:price_amount].present? && d[:price_currency].present? d[:label_1] = "Price" d[:data_1] = Onebox::Helpers.truncate( "#{d[:price_currency].strip} #{d[:price_amount].strip}", @@ -205,11 +203,11 @@ module Onebox end def has_text? - has_title? && !Onebox::Helpers.blank?(data[:description]) + has_title? && data[:description].present? end def has_title? - !Onebox::Helpers.blank?(data[:title]) + data[:title].present? end def is_image_article? @@ -221,12 +219,11 @@ module Onebox end def has_image? - !Onebox::Helpers.blank?(data[:image]) + data[:image].present? end def is_video? - data[:type] =~ %r{^video[/\.]} && data[:video_type] == "video/mp4" && # Many sites include 'videos' with text/html types (i.e. iframes) - !Onebox::Helpers.blank?(data[:video]) + data[:type] =~ %r{^video[/\.]} && data[:video_type] == "video/mp4" && data[:video].present? # Many sites include 'videos' with text/html types (i.e. iframes) end def is_embedded? @@ -267,7 +264,7 @@ module Onebox end def image_html - return if Onebox::Helpers.blank?(data[:image]) + return if data[:image].blank? escaped_src = ::Onebox::Helpers.normalize_url_for_output(data[:image]) diff --git a/lib/onebox/engine/audio_com_onebox.rb b/lib/onebox/engine/audio_com_onebox.rb index 394cab24b26..faa3faf0543 100644 --- a/lib/onebox/engine/audio_com_onebox.rb +++ b/lib/onebox/engine/audio_com_onebox.rb @@ -17,7 +17,7 @@ module Onebox def placeholder_html oembed = get_oembed - return if Onebox::Helpers.blank?(oembed.thumbnail_url) + return if oembed.thumbnail_url.blank? "" end diff --git a/lib/onebox/engine/facebook_media_onebox.rb b/lib/onebox/engine/facebook_media_onebox.rb index cdf4d699ff3..9e676e18558 100644 --- a/lib/onebox/engine/facebook_media_onebox.rb +++ b/lib/onebox/engine/facebook_media_onebox.rb @@ -25,8 +25,7 @@ module Onebox HTML else html = Onebox::Engine::AllowlistedGenericOnebox.new(@url, @timeout).to_html - return if Onebox::Helpers.blank?(html) - html + html.presence end end end diff --git a/lib/onebox/engine/google_docs_onebox.rb b/lib/onebox/engine/google_docs_onebox.rb index ebb29a43bb5..03c436736b7 100644 --- a/lib/onebox/engine/google_docs_onebox.rb +++ b/lib/onebox/engine/google_docs_onebox.rb @@ -22,7 +22,7 @@ module Onebox short_type = SHORT_TYPES[match[:endpoint].to_sym] description = - if Onebox::Helpers.blank?(og_data.description) + if og_data.description.blank? "This #{short_type.to_s.chop.capitalize} is private" else Onebox::Helpers.truncate(og_data.description, 250) diff --git a/lib/onebox/engine/reddit_media_onebox.rb b/lib/onebox/engine/reddit_media_onebox.rb index 876ed41fd56..5f7450fee2d 100644 --- a/lib/onebox/engine/reddit_media_onebox.rb +++ b/lib/onebox/engine/reddit_media_onebox.rb @@ -46,8 +46,7 @@ module Onebox HTML else html = Onebox::Engine::AllowlistedGenericOnebox.new(@url, @timeout).to_html - return if Onebox::Helpers.blank?(html) - html + html.presence end end end diff --git a/lib/onebox/engine/simplecast_onebox.rb b/lib/onebox/engine/simplecast_onebox.rb index df474b3832e..10e9afe507a 100644 --- a/lib/onebox/engine/simplecast_onebox.rb +++ b/lib/onebox/engine/simplecast_onebox.rb @@ -16,7 +16,7 @@ module Onebox def placeholder_html oembed = get_oembed - return if Onebox::Helpers.blank?(oembed.thumbnail_url) + return if oembed.thumbnail_url.blank? "" end diff --git a/lib/onebox/engine/sound_cloud_onebox.rb b/lib/onebox/engine/sound_cloud_onebox.rb index 1571c1bd942..48db722d504 100644 --- a/lib/onebox/engine/sound_cloud_onebox.rb +++ b/lib/onebox/engine/sound_cloud_onebox.rb @@ -17,7 +17,7 @@ module Onebox def placeholder_html oembed = get_oembed - return if Onebox::Helpers.blank?(oembed.thumbnail_url) + return if oembed.thumbnail_url.blank? "" end diff --git a/lib/onebox/engine/standard_embed.rb b/lib/onebox/engine/standard_embed.rb index df5aa9aa9fb..2473dfe5829 100644 --- a/lib/onebox/engine/standard_embed.rb +++ b/lib/onebox/engine/standard_embed.rb @@ -82,9 +82,7 @@ module Onebox if (m["property"] && m["property"][/^twitter:(.+)$/i]) || (m["name"] && m["name"][/^twitter:(.+)$/i]) value = (m["content"] || m["value"]).to_s - twitter[$1.tr("-:", "_").to_sym] ||= value unless ( - Onebox::Helpers.blank?(value) || value == "0 minutes" - ) + twitter[$1.tr("-:", "_").to_sym] ||= value if (value.present? && value != "0 minutes") end end @@ -115,7 +113,7 @@ module Onebox def get_json_response oembed_url = get_oembed_url - return "{}" if Onebox::Helpers.blank?(oembed_url) + return "{}" if oembed_url.blank? begin Onebox::Helpers.fetch_response(oembed_url) @@ -137,12 +135,12 @@ module Onebox end if html_doc - if Onebox::Helpers.blank?(oembed_url) + if oembed_url.blank? application_json = html_doc.at("//link[@type='application/json+oembed']/@href") oembed_url = application_json.value if application_json end - if Onebox::Helpers.blank?(oembed_url) + if oembed_url.blank? text_json = html_doc.at("//link[@type='text/json+oembed']/@href") oembed_url ||= text_json.value if text_json end @@ -170,7 +168,7 @@ module Onebox def set_twitter_data_on_raw twitter = get_twitter - twitter.each { |k, v| @raw[k] ||= v unless Onebox::Helpers.blank?(v) } + twitter.each { |k, v| @raw[k] ||= v if v.present? } end def set_oembed_data_on_raw @@ -185,13 +183,13 @@ module Onebox def set_favicon_data_on_raw favicon = get_favicon - @raw[:favicon] = favicon unless Onebox::Helpers.blank?(favicon) + @raw[:favicon] = favicon if favicon.present? end def set_description_on_raw unless @raw[:description] description = get_description - @raw[:description] = description unless Onebox::Helpers.blank?(description) + @raw[:description] = description if description.present? end end end diff --git a/lib/onebox/engine/wistia_onebox.rb b/lib/onebox/engine/wistia_onebox.rb index ad85d0731dc..de5b9c27526 100644 --- a/lib/onebox/engine/wistia_onebox.rb +++ b/lib/onebox/engine/wistia_onebox.rb @@ -34,7 +34,7 @@ module Onebox def placeholder_html oembed = get_oembed - return if Onebox::Helpers.blank?(oembed.thumbnail_url) + return if oembed.thumbnail_url.blank? "" end diff --git a/lib/onebox/engine/youtube_onebox.rb b/lib/onebox/engine/youtube_onebox.rb index b316746ee0a..0b6839d012c 100644 --- a/lib/onebox/engine/youtube_onebox.rb +++ b/lib/onebox/engine/youtube_onebox.rb @@ -78,7 +78,7 @@ module Onebox else # for channel pages html = Onebox::Engine::AllowlistedGenericOnebox.new(@url, @timeout).to_html - return if Onebox::Helpers.blank?(html) + return if html.blank? html.gsub!(%r{['"]//}, "https://") html end diff --git a/lib/onebox/helpers.rb b/lib/onebox/helpers.rb index 1e4da8016ab..108af6d0260 100644 --- a/lib/onebox/helpers.rb +++ b/lib/onebox/helpers.rb @@ -165,9 +165,7 @@ module Onebox end http.request_head([uri.path, uri.query].join("?")) do |response| - code = response.code.to_i - return nil unless code === 200 || Onebox::Helpers.blank?(response.content_length) - return response.content_length + return response.code.to_i == 200 ? response.content_length.presence : nil end end end @@ -190,27 +188,17 @@ module Onebox "
" end - def self.blank?(value) - if value.nil? - true - elsif String === value - value.empty? || !(/[[:^space:]]/ === value) - else - value.respond_to?(:empty?) ? !!value.empty? : !value - end - end - def self.truncate(string, length = 50) return string if string.nil? string.size > length ? string[0...(string.rindex(" ", length) || length)] + "..." : string end def self.get(meta, attr) - (meta && !blank?(meta[attr])) ? sanitize(meta[attr]) : nil + (meta && meta[attr].present?) ? sanitize(meta[attr]) : nil end def self.sanitize(value, length = 50) - return nil if blank?(value) + return nil if value.blank? Sanitize.fragment(value).strip end diff --git a/lib/onebox/json_ld.rb b/lib/onebox/json_ld.rb index b9137b96bdd..1dbc8faf554 100644 --- a/lib/onebox/json_ld.rb +++ b/lib/onebox/json_ld.rb @@ -13,7 +13,7 @@ module Onebox private def extract(doc) - return {} if Onebox::Helpers.blank?(doc) + return {} if doc.blank? doc .css('script[type="application/ld+json"]') diff --git a/lib/onebox/normalizer.rb b/lib/onebox/normalizer.rb index ecd0b509f15..2a5e3c2e401 100644 --- a/lib/onebox/normalizer.rb +++ b/lib/onebox/normalizer.rb @@ -14,14 +14,13 @@ module Onebox def method_missing(attr, *args, &block) value = get(attr, *args) - return nil if Onebox::Helpers.blank?(value) + return nil if value.blank? method_name = attr.to_s if method_name.end_with?(*integer_suffixes) value.to_i elsif method_name.end_with?(*url_suffixes) - result = Onebox::Helpers.normalize_url_for_output(value) - result unless Onebox::Helpers.blank?(result) + Onebox::Helpers.normalize_url_for_output(value).presence else value end diff --git a/lib/onebox/open_graph.rb b/lib/onebox/open_graph.rb index 10c1f165d9c..d25075222b7 100644 --- a/lib/onebox/open_graph.rb +++ b/lib/onebox/open_graph.rb @@ -25,7 +25,7 @@ module Onebox COLLECTIONS = %i[article_section article_section_color article_tag] def extract(doc) - return {} if Onebox::Helpers.blank?(doc) + return {} if doc.blank? data = {} @@ -35,7 +35,7 @@ module Onebox if (m["property"] && m["property"][/\A(?:og|article|product):(.+)\z/i]) || (m["name"] && m["name"][/\A(?:og|article|product):(.+)\z/i]) value = (m["content"] || m["value"]).to_s - next if Onebox::Helpers.blank?(value) + next if value.blank? key = $1.tr("-:", "_").to_sym data[key] ||= value if key.in?(COLLECTIONS) @@ -48,9 +48,7 @@ module Onebox # Attempt to retrieve the title from the meta tag title_element = doc.at_css("title") - if title_element && title_element.text - data[:title] ||= title_element.text unless Onebox::Helpers.blank?(title_element.text) - end + data[:title] ||= title_element.text if title_element && title_element.text.present? data end diff --git a/spec/lib/onebox/helpers_spec.rb b/spec/lib/onebox/helpers_spec.rb index 86196f732a8..195b24ec5f3 100644 --- a/spec/lib/onebox/helpers_spec.rb +++ b/spec/lib/onebox/helpers_spec.rb @@ -1,19 +1,6 @@ # frozen_string_literal: true RSpec.describe Onebox::Helpers do - describe ".blank?" do - it { expect(described_class.blank?("")).to be(true) } - it { expect(described_class.blank?(" ")).to be(true) } - it { expect(described_class.blank?("test")).to be(false) } - it { expect(described_class.blank?(%w[test testing])).to be(false) } - it { expect(described_class.blank?([])).to be(true) } - it { expect(described_class.blank?({})).to be(true) } - it { expect(described_class.blank?(nil)).to be(true) } - it { expect(described_class.blank?(true)).to be(false) } - it { expect(described_class.blank?(false)).to be(true) } - it { expect(described_class.blank?(a: "test")).to be(false) } - end - describe ".truncate" do let(:test_string) { "Chops off on spaces" } it { expect(described_class.truncate(test_string)).to eq(test_string) }