Merge pull request #1637 from railsaholic/refactor_users_upload_avatar_action

Refactor Users#upload_avatar method

app/controllers/users_controller.rb

@@ -1,6 +1,7 @@
 require_dependency 'discourse_hub'
 require_dependency 'user_name_suggester'
 require_dependency 'user_activator'
+require_dependency 'avatar_upload_service'
 
 class UsersController < ApplicationController
 
@@ -168,15 +169,17 @@ class UsersController < ApplicationController
   end
 
   def logon_after_password_reset
-    if Guardian.new(@user).can_access_forum?
-      # Log in the user
-      log_on_user(@user)
-      flash[:success] = I18n.t('password_reset.success')
-    else
-      @requires_approval = true
-      flash[:success] = I18n.t('password_reset.success_unapproved')
-    end
-  end
+    message = if Guardian.new(@user).can_access_forum?
+                # Log in the user
+                log_on_user(@user)
+                'password_reset.success'
+              else
+                @requires_approval = true
+                'password_reset.success_unapproved'
+              end
+
+    flash[:success] = I18n.t(message)
+   end
 
   def change_email
     params.require(:email)
@@ -285,35 +288,18 @@ class UsersController < ApplicationController
     # Only allow url uploading for API users
     # TODO: Does not protect from huge uploads
     # https://github.com/discourse/discourse/pull/1512
-    if file.is_a?(String) && is_api?
-      adapted   = ::UriAdapter.new(file)
-      file      = adapted.build_uploaded_file
-      filesize  = adapted.file_size
-    elsif file.is_a?(String)
-      return render status: 422, text: I18n.t("upload.images.unknown_image_type")
-    end
-
     # check the file size (note: this might also be done in the web server)
-    filesize  ||= File.size(file.tempfile)
-    max_size_kb = SiteSetting.max_image_size_kb * 1024
-    if filesize > max_size_kb
-      return render status: 413, text: I18n.t("upload.images.too_large", max_size_kb: max_size_kb)
-    else
-      filesize
+    avatar        = build_avatar_from(file)
+    avatar_policy = AvatarUploadPolicy.new(avatar)
+
+    if avatar_policy.too_big?
+      return render status: 413, text: I18n.t("upload.images.too_large",
+                                              max_size_kb: avatar_policy.max_size_kb)
     end
 
-    return render status: 422, text: I18n.t("upload.images.unknown_image_type") unless SiteSetting.authorized_image?(file)
+    raise FastImage::UnknownImageType unless SiteSetting.authorized_image?(avatar.file)
 
-    upload = Upload.create_for(user.id, file, filesize)
-    user.upload_avatar(upload)
-
-    Jobs.enqueue(:generate_avatars, user_id: user.id, upload_id: upload.id)
-
-    render json: {
-      url: upload.url,
-      width: upload.width,
-      height: upload.height,
-    }
+    upload_avatar_for(user, avatar)
 
   rescue Discourse::InvalidParameters
     render status: 422, text: I18n.t("upload.images.unknown_image_type")
@@ -413,4 +399,21 @@ class UsersController < ApplicationController
       auth
     end
 
+    def build_avatar_from(file)
+      source = if file.is_a?(String)
+                 is_api? ? :url : (raise FastImage::UnknownImageType)
+               else
+                 :image
+               end
+      AvatarUploadService.new(file, source)
+    end
+
+    def upload_avatar_for(user, avatar)
+      upload = Upload.create_for(user.id, avatar.file, avatar.filesize)
+      user.upload_avatar(upload)
+
+      Jobs.enqueue(:generate_avatars, user_id: user.id, upload_id: upload.id)
+      render json: { url: upload.url, width: upload.width, height: upload.height }
+    end
+
 end

lib/avatar_upload_service.rb

@@ -0,0 +1,43 @@
+class AvatarUploadService
+
+  attr_accessor :source
+  attr_reader :filesize, :file
+
+  def initialize(file, source)
+    @source = source
+    @file , @filesize = construct(file)
+  end
+
+  def construct(file)
+    case source
+    when :url
+      build_from_url(file)
+    when :image
+      [file, File.size(file.tempfile)]
+    end
+  end
+
+  private
+
+  def build_from_url(url)
+    temp = ::UriAdapter.new(url)
+    return temp.build_uploaded_file, temp.file_size
+  end
+
+end
+
+class AvatarUploadPolicy
+
+  def initialize(avatar)
+    @avatar = avatar
+  end
+
+  def max_size_kb
+    SiteSetting.max_image_size_kb * 1024
+  end
+
+  def too_big?
+    @avatar.filesize > max_size_kb
+  end
+
+end

spec/components/avatar_upload_service_spec.rb

@@ -0,0 +1,66 @@
+require "spec_helper"
+require "avatar_upload_service"
+
+describe AvatarUploadService do
+  let(:file) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: 'logo.png',
+      tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+    })
+  end
+
+  let(:url) { "http://cdn.discourse.org/assets/logo.png" }
+
+  describe "#construct" do
+    context "when avatar is in the form of a file upload" do
+      let(:avatar_file) { AvatarUploadService.new(file, :image) }
+
+      it "should have a filesize" do
+        expect(avatar_file.filesize).to eq(2290)
+      end
+
+      it "should have a source as 'image'" do
+        expect(avatar_file.source).to eq(:image)
+      end
+
+      it "is an instance of File class" do
+        file = avatar_file.file
+        expect(file.tempfile).to be_instance_of File
+      end
+
+      it "returns the file object built from File" do
+        file = avatar_file.file
+        file.should be_instance_of(ActionDispatch::Http::UploadedFile)
+        file.original_filename.should == "logo.png"
+      end
+    end
+
+    context "when file is in the form of a URL" do
+      let(:avatar_file) { AvatarUploadService.new(url, :url) }
+
+      before :each do
+        UriAdapter.any_instance.stubs(:open).returns StringIO.new(fixture_file("images/logo.png"))
+      end
+
+      it "should have a filesize" do
+        expect(avatar_file.filesize).to eq(2290)
+      end
+
+      it "should have a source as 'url'" do
+        expect(avatar_file.source).to eq(:url)
+      end
+
+      it "is an instance of Tempfile class" do
+        file = avatar_file.file
+        expect(file.tempfile).to be_instance_of Tempfile
+      end
+
+      it "returns the file object built from URL" do
+        file = avatar_file.file
+        file.should be_instance_of(ActionDispatch::Http::UploadedFile)
+        file.original_filename.should == "logo.png"
+      end
+    end
+  end
+
+end

spec/controllers/users_controller_spec.rb

@@ -996,7 +996,7 @@ describe UsersController do
         end
 
         it 'rejects large images' do
-          SiteSetting.stubs(:max_image_size_kb).returns(1)
+          AvatarUploadPolicy.any_instance.stubs(:too_big?).returns(true)
           xhr :post, :upload_avatar, username: user.username, file: avatar
           response.status.should eq 413
         end
@@ -1041,7 +1041,7 @@ describe UsersController do
           end
 
           it 'rejects large images' do
-            SiteSetting.stubs(:max_image_size_kb).returns(1)
+            AvatarUploadPolicy.any_instance.stubs(:too_big?).returns(true)
             xhr :post, :upload_avatar, username: user.username, file: avatar_url
             response.status.should eq 413
           end