FIX: crop & optimize user background profile/card images

app/controllers/uploads_controller.rb

@@ -59,13 +59,7 @@ class UploadsController < ApplicationController
         content_type = file.content_type
       end
 
-      # when we're dealing with an avatar, crop it to its maximum size
-      if type == "avatar" && FileHelper.is_image?(filename)
-        max = Discourse.avatar_sizes.max
-        OptimizedImage.resize(tempfile.path, tempfile.path, max, max, allow_animation: SiteSetting.allow_animated_avatars)
-      end
-
-      upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type)
+      upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)
 
       if upload.errors.empty? && current_user.admin?
         retain_hours = params[:retain_hours].to_i

app/models/upload.rb

@@ -49,13 +49,62 @@ class Upload < ActiveRecord::Base
     File.extname(original_filename)
   end
 
+  # list of image types that will be cropped
+  CROPPED_IMAGE_TYPES ||= ["avatar", "profile_background", "card_background"]
+
   # options
   #   - content_type
   #   - origin
+  #   - image_type
   def self.create_for(user_id, file, filename, filesize, options = {})
-    sha1 = Digest::SHA1.file(file).hexdigest
+    DistributedMutex.synchronize("upload_#{user_id}_#{filename}") do
+      # do some work on images
+      if FileHelper.is_image?(filename)
+        if filename =~ /\.svg$/i
+          svg = Nokogiri::XML(file).at_css("svg")
+          w = svg["width"].to_i
+          h = svg["height"].to_i
+        else
+          # fix orientation first
+          fix_image_orientation(file.path)
+          # retrieve image info
+          image_info = FastImage.new(file) rescue nil
+          w, h = *(image_info.try(:size) || [0, 0])
+        end
+
+        # default size
+        width, height = ImageSizer.resize(w, h)
+
+        # make sure we're at the beginning of the file (both FastImage and Nokogiri move the pointer)
+        file.rewind
+
+        # crop images depending on their type
+        if CROPPED_IMAGE_TYPES.include?(options[:image_type])
+          allow_animation = false
+          max_pixel_ratio = Discourse::PIXEL_RATIOS.max
+
+          case options[:image_type]
+          when "avatar"
+            allow_animation = SiteSetting.allow_animated_avatars
+            width = height = Discourse.avatar_sizes.max
+          when "profile_background"
+            max_width = 850 * max_pixel_ratio
+            width, height = ImageSizer.resize(w, h, max_width: max_width, max_height: max_width)
+          when "card_background"
+            max_width = 590 * max_pixel_ratio
+            width, height = ImageSizer.resize(w, h, max_width: max_width, max_height: max_width)
+          end
+
+          OptimizedImage.resize(file.path, file.path, width, height, allow_animation: allow_animation)
+        end
+
+        # optimize image
+        ImageOptim.new.optimize_image!(file.path) rescue nil
+      end
+
+      # compute the sha of the file
+      sha1 = Digest::SHA1.file(file).hexdigest
 
-    DistributedMutex.synchronize("upload_#{sha1}") do
       # do we already have that upload?
       upload = find_by(sha1: sha1)
 
@@ -75,13 +124,12 @@ class Upload < ActiveRecord::Base
       upload.filesize          = filesize
       upload.sha1              = sha1
       upload.url               = ""
+      upload.width             = width
+      upload.height            = height
       upload.origin            = options[:origin][0...1000] if options[:origin]
 
-      if FileHelper.is_image?(filename)
-        # deal with width & height for images
-        upload = resize_image(filename, file, upload)
-        # optimize image
-        ImageOptim.new.optimize_image!(file.path) rescue nil
+      if FileHelper.is_image?(filename) && (upload.width == 0 || upload.height == 0)
+        upload.errors.add(:base, I18n.t("upload.images.size_not_found"))
       end
 
       return upload unless upload.save
@@ -97,43 +145,10 @@ class Upload < ActiveRecord::Base
         end
       end
 
-      # return the uploaded file
       upload
     end
   end
 
-  def self.resize_image(filename, file, upload)
-    begin
-      if filename =~ /\.svg$/i
-        svg = Nokogiri::XML(file).at_css("svg")
-        width, height = svg["width"].to_i, svg["height"].to_i
-        if width == 0 || height == 0
-          upload.errors.add(:base, I18n.t("upload.images.size_not_found"))
-        else
-          upload.width, upload.height = ImageSizer.resize(width, height)
-        end
-      else
-        # fix orientation first
-        Upload.fix_image_orientation(file.path)
-        # retrieve image info
-        image_info = FastImage.new(file, raise_on_failure: true)
-          # compute image aspect ratio
-        upload.width, upload.height = ImageSizer.resize(*image_info.size)
-      end
-      # make sure we're at the beginning of the file
-      # (FastImage and Nokogiri move the pointer)
-      file.rewind
-    rescue FastImage::ImageFetchFailure
-      upload.errors.add(:base, I18n.t("upload.images.fetch_failure"))
-    rescue FastImage::UnknownImageType
-      upload.errors.add(:base, I18n.t("upload.images.unknown_image_type"))
-    rescue FastImage::SizeNotFound
-      upload.errors.add(:base, I18n.t("upload.images.size_not_found"))
-    end
-
-    upload
-  end
-
   def self.get_from_url(url)
     return if url.blank?
     # we store relative urls, so we need to remove any host/cdn

app/models/user_avatar.rb

@@ -20,7 +20,7 @@ class UserAvatar < ActiveRecord::Base
         max = Discourse.avatar_sizes.max
         gravatar_url = "http://www.gravatar.com/avatar/#{email_hash}.png?s=#{max}&d=404"
         tempfile = FileHelper.download(gravatar_url, SiteSetting.max_image_size_kb.kilobytes, "gravatar")
-        upload = Upload.create_for(user.id, tempfile, 'gravatar.png', tempfile.size, { origin: gravatar_url })
+        upload = Upload.create_for(user.id, tempfile, 'gravatar.png', tempfile.size, origin: gravatar_url, image_type: "avatar")
 
         if gravatar_upload_id != upload.id
           gravatar_upload.try(:destroy!)
@@ -68,7 +68,7 @@ class UserAvatar < ActiveRecord::Base
     ext = FastImage.type(tempfile).to_s
     tempfile.rewind
 
-    upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, tempfile.size, { origin: avatar_url })
+    upload = Upload.create_for(user.id, tempfile, "external-avatar." + ext, tempfile.size, origin: avatar_url, image_type: "avatar")
     user.uploaded_avatar_id = upload.id
 
     unless user.user_avatar

config/locales/server.en.yml

@@ -2055,8 +2055,6 @@ en:
       too_large: "Sorry, the file you are trying to upload is too big (maximum size is %{max_size_kb}KB)."
     images:
       too_large: "Sorry, the image you are trying to upload is too big (maximum size is %{max_size_kb}KB), please resize it and try again."
-      fetch_failure: "Sorry, there has been an error while fetching the image."
-      unknown_image_type: "Sorry, but the file you tried to upload doesn't appear to be an image."
       size_not_found: "Sorry, but we couldn't determine the size of the image. Maybe your image is corrupted?"
 
   flag_reason:

lib/image_sizer.rb

@@ -1,18 +1,18 @@
 module ImageSizer
 
   # Resize an image to the aspect ratio we want
-  def self.resize(width, height)
+  def self.resize(width, height, opts = {})
     return if width.blank? || height.blank?
 
-    max_width = SiteSetting.max_image_width.to_f
-    max_height = SiteSetting.max_image_height.to_f
+    max_width = (opts[:max_width] || SiteSetting.max_image_width).to_f
+    max_height = (opts[:max_height] || SiteSetting.max_image_height).to_f
 
     w = width.to_f
     h = height.to_f
 
     return [w.floor, h.floor] if w <= max_width && h <= max_height
 
-    ratio = [max_width / w, max_height / h].min;
+    ratio = [max_width / w, max_height / h].min
     [(w * ratio).floor, (h * ratio).floor]
   end
 

spec/components/email/receiver_spec.rb

@@ -303,8 +303,11 @@ This is a link http://example.com"
       let(:upload_sha) { '04df605be528d03876685c52166d4b063aabb78a' }
 
       it "creates a post with an attachment" do
+        Upload.stubs(:fix_image_orientation)
+        ImageOptim.any_instance.stubs(:optimize_image!)
+
         start_count = topic.posts.count
-        Upload.find_by(sha1: upload_sha).try :destroy
+        Upload.find_by(sha1: upload_sha).try(:destroy)
 
         receiver.process
 

spec/models/upload_spec.rb

@@ -45,7 +45,10 @@ describe Upload do
 
   context "#create_for" do
 
-    before { Upload.stubs(:fix_image_orientation) }
+    before do
+      Upload.stubs(:fix_image_orientation)
+      ImageOptim.any_instance.stubs(:optimize_image!)
+    end
 
     it "does not create another upload if it already exists" do
       Upload.expects(:find_by).with(sha1: image_sha1).returns(upload)
@@ -65,13 +68,6 @@ describe Upload do
       Upload.create_for(user_id, image, image_filename, image_filesize)
     end
 
-    it "does not create an upload when there is an error with FastImage" do
-      FileHelper.expects(:is_image?).returns(true)
-      Upload.expects(:save).never
-      upload = Upload.create_for(user_id, attachment, attachment_filename, attachment_filesize)
-      expect(upload.errors.size).to be > 0
-    end
-
     it "does not compute width & height for non-image" do
       FastImage.any_instance.expects(:size).never
       upload = Upload.create_for(user_id, attachment, attachment_filename, attachment_filesize)