Merge pull request #670 from ZogStriP/add-some-tests-for-uploads

added some tests for uploads

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

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

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

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

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