From 16923503369f4efbb3267b4eee3c533ae419d0a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Sun, 7 Apr 2013 17:52:46 +0200 Subject: [PATCH] added some tests for uploads --- .../discourse/views/composer_view.js | 4 + app/controllers/uploads_controller.rb | 4 +- app/models/upload.rb | 92 ++++++++++--------- spec/controllers/uploads_controller_spec.rb | 17 +++- spec/models/upload_spec.rb | 52 +++++++++++ 5 files changed, 122 insertions(+), 47 deletions(-) diff --git a/app/assets/javascripts/discourse/views/composer_view.js b/app/assets/javascripts/discourse/views/composer_view.js index 0ad1a7136c5..39f6aad189c 100644 --- a/app/assets/javascripts/discourse/views/composer_view.js +++ b/app/assets/javascripts/discourse/views/composer_view.js @@ -330,6 +330,10 @@ Discourse.ComposerView = Discourse.View.extend({ case 413: bootbox.alert(Em.String.i18n('post.errors.upload_too_large', {max_size_kb: Discourse.SiteSettings.max_upload_size_kb})); return; + // 415 == media type not recognized (ie. not an image) + case 415: + bootbox.alert(Em.String.i18n('post.errors.only_images_are_supported')); + return; } } // otherwise, display a generic error message diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index e3554723690..301354e50fb 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -4,7 +4,9 @@ class UploadsController < ApplicationController def create requires_parameter(:topic_id) file = params[:file] || params[:files].first - upload = Upload.create_for(current_user, file, params[:topic_id]) + # only supports images for now + return render status: 415, json: failed_json unless file.content_type =~ /^image\/.+/ + upload = Upload.create_for(current_user.id, file, params[:topic_id]) render_serialized(upload, UploadSerializer, root: false) end end diff --git a/app/models/upload.rb b/app/models/upload.rb index 1e70e7539e2..bbd68209a1b 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -9,57 +9,33 @@ class Upload < ActiveRecord::Base validates_presence_of :filesize validates_presence_of :original_filename + # Create an upload given a user, file and topic + def self.create_for(user_id, file, topic_id) + return create_on_imgur(user_id, file, topic_id) if SiteSetting.enable_imgur? + return create_on_s3(user_id, file, topic_id) if SiteSetting.enable_s3_uploads? + return create_locally(user_id, file, topic_id) + end - # Create an upload given a user, file and optional topic_id - def self.create_for(user, file, topic_id = nil) - # TODO: Need specs/tests for this functionality - return create_on_imgur(user, file, topic_id) if SiteSetting.enable_imgur? - return create_on_s3(user, file, topic_id) if SiteSetting.enable_s3_uploads? - return create_locally(user, file, topic_id) + # Store uploads on imgur + def self.create_on_imgur(user_id, file, topic_id) + @imgur_loaded = require 'imgur' unless @imgur_loaded + + info = Imgur.upload_file(file) + + Upload.create!({ + user_id: user_id, + topic_id: topic_id, + original_filename: file.original_filename + }.merge!(info)) end # Store uploads on s3 - def self.create_on_imgur(user, file, topic_id) - @imgur_loaded = require 'imgur' unless @imgur_loaded - - - info = Imgur.upload_file(file) - Upload.create!({user_id: user.id, - topic_id: topic_id, - original_filename: file.original_filename}.merge!(info)) - end - - def self.create_locally(user, file, topic_id) - upload = Upload.create!(user_id: user.id, - topic_id: topic_id, - url: "", - filesize: File.size(file.tempfile), - original_filename: file.original_filename) - - # populate the rest of the info - clean_name = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16] - image_info = FastImage.new(file.tempfile) - clean_name += ".#{image_info.type}" - url_root = "/uploads/#{RailsMultisite::ConnectionManagement.current_db}/#{upload.id}" - path = "#{Rails.root}/public#{url_root}" - upload.width, upload.height = ImageSizer.resize(*image_info.size) - FileUtils.mkdir_p path - # not using cause mv, cause permissions are no good on move - File.open("#{path}/#{clean_name}", "wb") do |f| - f.write File.read(file.tempfile) - end - upload.url = Discourse::base_uri + "#{url_root}/#{clean_name}" - upload.save - - upload - end - - def self.create_on_s3(user, file, topic_id) + def self.create_on_s3(user_id, file, topic_id) @fog_loaded = require 'fog' unless @fog_loaded tempfile = file.tempfile - upload = Upload.new(user_id: user.id, + upload = Upload.new(user_id: user_id, topic_id: topic_id, filesize: File.size(tempfile), original_filename: file.original_filename) @@ -76,7 +52,6 @@ class Upload < ActiveRecord::Base location = "#{SiteSetting.s3_upload_bucket}#{path}" directory = fog.directories.create(key: location) - Rails.logger.info "#{blob.size.inspect}" file = directory.files.create(key: remote_filename, body: tempfile, public: true, @@ -88,4 +63,33 @@ class Upload < ActiveRecord::Base upload end + + def self.create_locally(user_id, file, topic_id) + upload = Upload.create!({ + user_id: user_id, + topic_id: topic_id, + url: "", + filesize: File.size(file.tempfile), + original_filename: file.original_filename + }) + + # populate the rest of the info + clean_name = Digest::SHA1.hexdigest("#{Time.now.to_s}#{file.original_filename}")[0,16] + image_info = FastImage.new(file.tempfile) + clean_name += ".#{image_info.type}" + url_root = "/uploads/#{RailsMultisite::ConnectionManagement.current_db}/#{upload.id}" + path = "#{Rails.root}/public#{url_root}" + upload.width, upload.height = ImageSizer.resize(*image_info.size) + FileUtils.mkdir_p path + # not using cause mv, cause permissions are no good on move + File.open("#{path}/#{clean_name}", "wb") do |f| + f.write File.read(file.tempfile) + end + upload.url = Discourse::base_uri + "#{url_root}/#{clean_name}" + + upload.save + + upload + end + end diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 6a1da3fc210..d3a971ff573 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -25,7 +25,7 @@ describe UploadsController do let(:logo) do ActionDispatch::Http::UploadedFile.new({ filename: 'logo.png', - content_type: 'image/png', + type: 'image/png', tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png") }) end @@ -33,11 +33,19 @@ describe UploadsController do let(:logo_dev) do ActionDispatch::Http::UploadedFile.new({ filename: 'logo-dev.png', - content_type: 'image/png', + type: 'image/png', tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo-dev.png") }) end + let(:text_file) do + ActionDispatch::Http::UploadedFile.new({ + filename: 'LICENSE.txt', + type: 'text/plain', + tempfile: File.new("#{Rails.root}/LICENSE.txt") + }) + end + let(:files) { [ logo_dev, logo ] } context 'with a file' do @@ -45,6 +53,11 @@ describe UploadsController do xhr :post, :create, topic_id: 1234, file: logo response.should be_success end + + it 'supports only images' do + xhr :post, :create, topic_id: 1234, file: text_file + response.status.should eq 415 + end end context 'with some files' do diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index e136d7dd962..c09c46cda26 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -4,6 +4,58 @@ describe Upload do it { should belong_to :user } it { should belong_to :topic } + it { should validate_presence_of :original_filename } it { should validate_presence_of :filesize } + + context '.create_for' do + + let(:user_id) { 1 } + let(:topic_id) { 42 } + + let(:logo) do + ActionDispatch::Http::UploadedFile.new({ + filename: 'logo.png', + content_type: 'image/png', + tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png") + }) + end + + it "uses imgur when it is enabled" do + SiteSetting.stubs(:enable_imgur?).returns(true) + Upload.expects(:create_on_imgur).with(user_id, logo, topic_id) + Upload.create_for(user_id, logo, topic_id) + end + + it "uses s3 when it is enabled" do + SiteSetting.stubs(:enable_s3_uploads?).returns(true) + Upload.expects(:create_on_s3).with(user_id, logo, topic_id) + Upload.create_for(user_id, logo, topic_id) + end + + it "uses local storage otherwise" do + Upload.expects(:create_locally).with(user_id, logo, topic_id) + Upload.create_for(user_id, logo, topic_id) + end + + context 'imgur' do + + # TODO + + end + + context 's3' do + + # TODO + + end + + context 'local' do + + # TODO + + end + + end + end