From 58f78e90012c827012f677720a0a76e707bb816c Mon Sep 17 00:00:00 2001
From: railsaholic <manoj.mk27@gmail.com>
Date: Mon, 11 Nov 2013 23:21:14 +0530
Subject: [PATCH] Refactor Users#upload_avatar method
Moved avatar file upload to ```AvatarUploadService``` class and
```AvatarUploadPolicy```
Address review comments + require missing file in spec
---
app/controllers/users_controller.rb | 71 ++++++++++---------
lib/avatar_upload_service.rb | 43 +++++++++++
spec/components/avatar_upload_service_spec.rb | 66 +++++++++++++++++
spec/controllers/users_controller_spec.rb | 4 +-
4 files changed, 148 insertions(+), 36 deletions(-)
create mode 100644 lib/avatar_upload_service.rb
create mode 100644 spec/components/avatar_upload_service_spec.rb
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 2f6a456a0..abbed4486 100644
--- a/app/controllers/users_controller.rb
+++ b/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
diff --git a/lib/avatar_upload_service.rb b/lib/avatar_upload_service.rb
new file mode 100644
index 000000000..3a9cb8388
--- /dev/null
+++ b/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
diff --git a/spec/components/avatar_upload_service_spec.rb b/spec/components/avatar_upload_service_spec.rb
new file mode 100644
index 000000000..6248470bb
--- /dev/null
+++ b/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
diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb
index ea76e2652..be80e6bd6 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/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