From 3c5b01d490e9603baacbfe8018f04580466c62c9 Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Fri, 7 Aug 2020 11:04:31 -0400 Subject: [PATCH] [deploy] Implement ImageResizer module (#9652) * Create ImageResizer module * Refactor HtmlVariant * Refactor CloudCoverUrl * Fix broken spec * Refactor PodcastEpisode * Refactor Message * Fix broken spec * Refactor Articles::SocialImage * Refactor ProfileImage & add spec * Fix broken spec --- app/labor/profile_image.rb | 25 +++++----------------- app/lib/image_resizer.rb | 22 +++++++++++++++++++ app/models/html_variant.rb | 20 +---------------- app/models/message.rb | 9 +------- app/models/podcast_episode.rb | 13 +----------- app/view_objects/articles/social_image.rb | 21 ++---------------- app/view_objects/cloud_cover_url.rb | 14 ++---------- spec/helpers/social_image_helper_spec.rb | 1 + spec/labor/profile_image_spec.rb | 18 ++++++++++++++++ spec/models/html_variant_spec.rb | 2 +- spec/models/message_spec.rb | 7 ++++-- spec/models/podcast_episode_spec.rb | 5 ++++- spec/view_objects/cloud_cover_url_spec.rb | 26 ++++++++++++----------- 13 files changed, 77 insertions(+), 106 deletions(-) create mode 100644 app/lib/image_resizer.rb create mode 100644 spec/labor/profile_image_spec.rb diff --git a/app/labor/profile_image.rb b/app/labor/profile_image.rb index 76c8f865d..ac9df13fd 100644 --- a/app/labor/profile_image.rb +++ b/app/labor/profile_image.rb @@ -1,30 +1,15 @@ class ProfileImage include CloudinaryHelper - attr_accessor :resource, :image_link, :backup_link + + BACKUP_LINK = "https://thepracticaldev.s3.amazonaws.com/i/99mvlsfu5tfj9m7ku25d.png".freeze + + attr_accessor :image_link def initialize(resource) - @resource = resource @image_link = resource.profile_image_url - @backup_link = "https://thepracticaldev.s3.amazonaws.com/i/99mvlsfu5tfj9m7ku25d.png" end def get(width: 120) - cl_image_path(get_link, - type: "fetch", - crop: "fill", - width: width, - height: width, - quality: "auto", - flags: "progressive", - fetch_format: "auto", - sign_url: true) - end - - def get_link - image_link || backup_link - end - - def get_external_link - image_link ? "#{ApplicationConfig['APP_PROTOCOL']}#{ApplicationConfig['APP_DOMAIN']}#{image_link}" : backup_link + ImageResizer.call(image_link || BACKUP_LINK, width: width, height: width, crop: "fill") end end diff --git a/app/lib/image_resizer.rb b/app/lib/image_resizer.rb new file mode 100644 index 000000000..b0df55029 --- /dev/null +++ b/app/lib/image_resizer.rb @@ -0,0 +1,22 @@ +module ImageResizer + DEFAULT_CL_OPTIONS = { + type: "fetch", + height: nil, + width: nil, + crop: "limit", + quality: "auto", + flags: "progressive", + fetch_format: "auto", + sign_url: true + }.freeze + + def self.call(img_src, **kwargs) + options = DEFAULT_CL_OPTIONS.merge(kwargs).reject { |_, v| v.blank? } + + if img_src&.include?(".gif") + options[:quality] = 66 + end + + ActionController::Base.helpers.cl_image_path(img_src, options) + end +end diff --git a/app/models/html_variant.rb b/app/models/html_variant.rb index 256151ee6..51e809335 100644 --- a/app/models/html_variant.rb +++ b/app/models/html_variant.rb @@ -1,6 +1,4 @@ class HtmlVariant < ApplicationRecord - include CloudinaryHelper - GROUP_NAMES = %w[article_show_below_article_cta badge_landing_page campaign].freeze validates :html, presence: true @@ -66,7 +64,7 @@ class HtmlVariant < ApplicationRecord img["src"] = if Giphy::Image.valid_url?(src) src.gsub("https://media.", "https://i.") else - img_of_size(src, 420) + ImageResizer.call(src, width: 420).gsub(",", "%2C") end end self.html = doc.to_html @@ -75,20 +73,4 @@ class HtmlVariant < ApplicationRecord def allowed_image_host?(src) src.start_with?("https://res.cloudinary.com/") end - - def img_of_size(source, width = 420) - quality = if source && (source.include? ".gif") - 66 - else - "auto" - end - cl_image_path(source, - type: "fetch", - width: width, - crop: "limit", - quality: quality, - flags: "progressive", - fetch_format: "auto", - sign_url: true).gsub(",", "%2C") - end end diff --git a/app/models/message.rb b/app/models/message.rb index 8a8c795da..49e493a04 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -175,14 +175,7 @@ class Message < ApplicationRecord # rubocop:enable Rails/OutputSafety def cl_path(img_src) - ActionController::Base.helpers - .cl_image_path(img_src, - type: "fetch", - width: 725, - crop: "limit", - flags: "progressive", - fetch_format: "auto", - sign_url: true) + ImageResizer.call(img_src, width: 725) end def determine_user_validity diff --git a/app/models/podcast_episode.rb b/app/models/podcast_episode.rb index 614c6ddeb..dd9d2cec7 100644 --- a/app/models/podcast_episode.rb +++ b/app/models/podcast_episode.rb @@ -113,18 +113,7 @@ class PodcastEpisode < ApplicationRecord next unless img_src - quality = "auto" - quality = 66 if img_src.include?(".gif") - - cloudinary_img_src = ActionController::Base.helpers - .cl_image_path(img_src, - type: "fetch", - width: 725, - crop: "limit", - quality: quality, - flags: "progressive", - fetch_format: "auto", - sign_url: true) + cloudinary_img_src = ImageResizer.call(img_src, width: 725) self.processed_html = processed_html.gsub(img_src, cloudinary_img_src) end end diff --git a/app/view_objects/articles/social_image.rb b/app/view_objects/articles/social_image.rb index 58f7ec61e..ccd30cf17 100644 --- a/app/view_objects/articles/social_image.rb +++ b/app/view_objects/articles/social_image.rb @@ -1,7 +1,6 @@ module Articles class SocialImage include Rails.application.routes.url_helpers - include CloudinaryHelper SOCIAL_PREVIEW_MIGRATION_DATETIME = Time.zone.parse("2019-04-22T00:00:00Z") @@ -15,15 +14,7 @@ module Articles image = user_defined_image if image.present? image = image.split("w_1000/").last if image.include?("w_1000/https://") - return cl_image_path(image, - type: "fetch", - width: width, - height: height, - crop: "imagga_scale", - quality: "auto", - flags: "progressive", - fetch_format: "auto", - sign_url: true) + return ImageResizer.call(image, width: width, height: height, crop: "imagga_scale") end return legacy_article_social_image unless use_new_social_url? @@ -41,15 +32,7 @@ module Articles src = GeneratedImage.new(article).social_image return src if src.start_with? "https://res.cloudinary.com/" - cl_image_path(src, - type: "fetch", - width: "1000", - height: "500", - crop: "imagga_scale", - quality: "auto", - flags: "progressive", - fetch_format: "auto", - sign_url: true) + ImageResizer.call(src, width: "1000", height: "500", crop: "imagga_scale") end end diff --git a/app/view_objects/cloud_cover_url.rb b/app/view_objects/cloud_cover_url.rb index 4592e2669..bb51f2a25 100644 --- a/app/view_objects/cloud_cover_url.rb +++ b/app/view_objects/cloud_cover_url.rb @@ -1,5 +1,4 @@ class CloudCoverUrl - include CloudinaryHelper include ActionView::Helpers::AssetUrlHelper def initialize(url) @@ -11,18 +10,9 @@ class CloudCoverUrl return url if Rails.env.development? width = 1000 - height = 420 - quality = "auto" + img_src = url_without_prefix_nesting(url, width) - cl_image_path(url_without_prefix_nesting(url, width), - type: "fetch", - width: width, - height: height, - crop: "imagga_scale", - quality: quality, - flags: "progressive", - fetch_format: "auto", - sign_url: true) + ImageResizer.call(img_src, width: width, height: 420, crop: "imagga_scale") end private diff --git a/spec/helpers/social_image_helper_spec.rb b/spec/helpers/social_image_helper_spec.rb index 1e9e39ac8..d51647c38 100644 --- a/spec/helpers/social_image_helper_spec.rb +++ b/spec/helpers/social_image_helper_spec.rb @@ -47,6 +47,7 @@ describe SocialImageHelper do url = helper.article_social_image_url(article) expect(url).to match(/#{article.main_image}/) + expect(url).to include("c_imagga_scale,f_auto,fl_progressive,h_500,q_auto,w_1000/") end it "returns older url2png image if already generated" do diff --git a/spec/labor/profile_image_spec.rb b/spec/labor/profile_image_spec.rb new file mode 100644 index 000000000..13735983b --- /dev/null +++ b/spec/labor/profile_image_spec.rb @@ -0,0 +1,18 @@ +require "rails_helper" + +RSpec.describe ProfileImage, type: :labor do + describe "#get" do + it "returns user profile_image_url" do + user = build_stubbed(:user) + expect(described_class.new(user).get).to eq(user.profile_image_url) + end + + context "when user has no profile_image" do + it "returns backup image prefixed with Cloudinary" do + user = build_stubbed(:user, profile_image: nil) + correct_prefix = "/c_fill,f_auto,fl_progressive,h_120,q_auto,w_120/" + expect(described_class.new(user).get).to include(correct_prefix + described_class::BACKUP_LINK) + end + end + end +end diff --git a/spec/models/html_variant_spec.rb b/spec/models/html_variant_spec.rb index e87508143..07c20fc6f 100644 --- a/spec/models/html_variant_spec.rb +++ b/spec/models/html_variant_spec.rb @@ -40,6 +40,6 @@ RSpec.describe HtmlVariant, type: :model do it "prefixes an image with cloudinary" do html = "
" html_variant.update(approved: false, html: html) - expect(html_variant.html).to include("cloudinary") + expect(html_variant.html).to include("/c_limit%2Cf_auto%2Cfl_progressive%2Cq_auto%2Cw_420/https://devimages.com/image.jpg") end end diff --git a/spec/models/message_spec.rb b/spec/models/message_spec.rb index a4bf4a608..ec2d05552 100644 --- a/spec/models/message_spec.rb +++ b/spec/models/message_spec.rb @@ -46,8 +46,11 @@ RSpec.describe Message, type: :model do message.message_markdown = "hello http://#{ApplicationConfig['APP_DOMAIN']}#{article.path}" message.validate! - expect(message.message_html).to include(article.title) - expect(message.message_html).to include("sidecar-article") + expect(message.message_html).to include( + article.title, + "sidecar-article", + "/c_limit,f_auto,fl_progressive,q_auto,w_725/", + ) end it "creates target blank link" do diff --git a/spec/models/podcast_episode_spec.rb b/spec/models/podcast_episode_spec.rb index c4bca969e..3ba3d659a 100644 --- a/spec/models/podcast_episode_spec.rb +++ b/spec/models/podcast_episode_spec.rb @@ -112,7 +112,10 @@ RSpec.describe PodcastEpisode, type: :model do image_url = "https://dummyimage.com/10x10" podcast_episode.body = "" podcast_episode.validate! - expect(podcast_episode.processed_html.include?("res.cloudinary.com")).to be(true) + expect(podcast_episode.processed_html).to include( + "res.cloudinary.com", + "c_limit,f_auto,fl_progressive,q_auto,w_725/https://dummyimage.com/10x10", + ) end it "chooses the appropriate quality for an image" do diff --git a/spec/view_objects/cloud_cover_url_spec.rb b/spec/view_objects/cloud_cover_url_spec.rb index 1c7d1b85b..38a28defe 100644 --- a/spec/view_objects/cloud_cover_url_spec.rb +++ b/spec/view_objects/cloud_cover_url_spec.rb @@ -2,27 +2,29 @@ require "rails_helper" RSpec.describe CloudCoverUrl, type: :view_object do let(:article) { create(:article) } + let(:cloudinary_prefix) { "https://res.cloudinary.com/TEST-CLOUD/image/fetch/" } it "returns proper url" do - expect(described_class.new(article.main_image).call).to start_with("https://res.cloudinary.com/TEST-CLOUD/image/fetch/") - expect(described_class.new(article.main_image).call).to include("/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://robohash.org/") + expect(described_class.new(article.main_image).call) + .to start_with(cloudinary_prefix) + .and include("/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://robohash.org/") end it "returns proper url when nested cloudinary" do - article.update_column( - :main_image, - "https://res.cloudinary.com/practicaldev/image/fetch/s--A-gun7rr--/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://res.cloudinary.com/practicaldev/image/fetch/s--hcD8ZkbP--/c_imagga_scale%2Cf_auto%2Cfl_progressive%2Ch_420%2Cq_auto%2Cw_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png") + image_url = "https://res.cloudinary.com/practicaldev/image/fetch/s--A-gun7rr--/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://res.cloudinary.com/practicaldev/image/fetch/s--hcD8ZkbP--/c_imagga_scale%2Cf_auto%2Cfl_progressive%2Ch_420%2Cq_auto%2Cw_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png" - expect(described_class.new(article.main_image).call).to start_with("https://res.cloudinary.com/TEST-CLOUD/image/fetch/") - expect(described_class.new(article.main_image).call).to end_with("/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png") + article.update_column(:main_image, image_url) + expect(described_class.new(article.main_image).call) + .to start_with(cloudinary_prefix) + .and end_with("/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png") end it "returns proper url when single cloudinary" do - article.update_column( - :main_image, - "https://res.cloudinary.com/practicaldev/image/fetch/s--hcD8ZkbP--/c_imagga_scale%2Cf_auto%2Cfl_progressive%2Ch_420%2Cq_auto%2Cw_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png") + image_url = "https://res.cloudinary.com/practicaldev/image/fetch/s--hcD8ZkbP--/c_imagga_scale%2Cf_auto%2Cfl_progressive%2Ch_420%2Cq_auto%2Cw_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png" - expect(described_class.new(article.main_image).call).to start_with("https://res.cloudinary.com/TEST-CLOUD/image/fetch/") - expect(described_class.new(article.main_image).call).to end_with("/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png") + article.update_column(:main_image, image_url) + expect(described_class.new(article.main_image).call) + .to start_with(cloudinary_prefix) + .and end_with("/c_imagga_scale,f_auto,fl_progressive,h_420,q_auto,w_1000/https://dev-to-uploads.s3.amazonaws.com/i/th93d625o27nuz63oeen.png") end end