FIX: create an upload when FastImage throws an exception

FastImage might throw an exception when it isn't able to recognize a
file as being an image (ie. happens when users changes the extension
manually)

Also improved upload specs a lot

app/models/upload.rb

@@ -17,7 +17,7 @@ class Upload < ActiveRecord::Base
   validates_presence_of :original_filename
 
   def thumbnail
-    @thumbnail ||= optimized_images.where(width: width, height: height).first
+    optimized_images.where(width: width, height: height).first
   end
 
   def thumbnail_url
@@ -48,23 +48,25 @@ class Upload < ActiveRecord::Base
     sha1 = Digest::SHA1.file(file.tempfile).hexdigest
     # check if the file has already been uploaded
     unless upload = Upload.where(sha1: sha1).first
+      # deal with width & heights for images
+      if SiteSetting.authorized_image?(file)
+        # retrieve image info
+        image_info = FastImage.new(file.tempfile, raise_on_failure: true)
+        # compute image aspect ratio
+        width, height = ImageSizer.resize(*image_info.size)
+        # make sure we're at the beginning of the file (FastImage is moving the pointer)
+        file.rewind
+      end
       # create a db record (so we can use the id)
       upload = Upload.create!({
         user_id: user_id,
         original_filename: file.original_filename,
         filesize: File.size(file.tempfile),
         sha1: sha1,
-        url: ""
+        url: "",
+        width: width,
+        height: height,
       })
-      # deal with width & heights for images
-      if SiteSetting.authorized_image?(file)
-        # retrieve image info
-        image_info = FastImage.new(file.tempfile, raise_on_failure: true)
-        # compute image aspect ratio
-        upload.width, upload.height = ImageSizer.resize(*image_info.size)
-        # make sure we're at the beginning of the file (FastImage is moving the pointer)
-        file.rewind
-      end
       # store the file and update its url
       upload.url = Upload.store_file(file, sha1, upload.id)
       # save the url
@@ -80,8 +82,8 @@ class Upload < ActiveRecord::Base
   end
 
   def self.remove_file(url)
-    S3.remove_file(url) if SiteSetting.enable_s3_uploads?
-    LocalStore.remove_file(url)
+    return S3.remove_file(url) if SiteSetting.enable_s3_uploads?
+    return LocalStore.remove_file(url)
   end
 
   def self.has_been_uploaded?(url)

spec/fabricators/optimized_image_fabricator.rb

@@ -0,0 +1,7 @@
+Fabricator(:optimized_image) do
+  upload
+  sha1 "abcdef"
+  extension ".png"
+  width 100
+  height 200
+end

spec/models/upload_spec.rb

@@ -13,52 +13,142 @@ describe Upload do
   it { should validate_presence_of :original_filename }
   it { should validate_presence_of :filesize }
 
-  context '.create_for' do
+  let(:upload) { build(:upload) }
+  let(:thumbnail) { build(:optimized_image, upload: upload) }
 
-    let(:user_id) { 1 }
+  let(:user_id) { 1 }
+  let(:url) { "http://domain.com" }
 
-    let(:logo) do
-      ActionDispatch::Http::UploadedFile.new({
-        filename: 'logo.png',
-        content_type: 'image/png',
-        tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
-      })
+  let(:image) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: 'logo.png',
+      tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo.png")
+    })
+  end
+
+  let(:image_sha1) { Digest::SHA1.file(image.tempfile).hexdigest }
+
+  let(:attachment) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: File.basename(__FILE__),
+      tempfile: File.new(__FILE__)
+    })
+  end
+
+  context ".create_thumbnail!" do
+
+    it "does not create a thumbnail when disabled" do
+      SiteSetting.stubs(:create_thumbnails?).returns(false)
+      SiteSetting.expects(:enable_s3_uploads?).never
+      upload.create_thumbnail!
     end
 
-    let(:upload) { Upload.create_for(user_id, logo) }
-
-    let(:url) { "http://domain.com" }
-
-    shared_examples_for "upload" do
-      it "is valid" do
-        upload.user_id.should == user_id
-        upload.original_filename.should == logo.original_filename
-        upload.filesize.should == File.size(logo.tempfile)
-        upload.sha1.should == Digest::SHA1.file(logo.tempfile).hexdigest
-        upload.width.should == 244
-        upload.height.should == 66
-        upload.url.should == url
-      end
+    it "does not create a thumbnail when using S3" do
+      SiteSetting.expects(:create_thumbnails?).returns(true)
+      SiteSetting.expects(:enable_s3_uploads?).returns(true)
+      upload.expects(:has_thumbnail?).never
+      upload.create_thumbnail!
     end
 
-    context "s3" do
-      before(:each) do
-        SiteSetting.stubs(:enable_s3_uploads?).returns(true)
-        S3.stubs(:store_file).returns(url)
-      end
-
-      it_behaves_like "upload"
-
+    it "does not create another thumbnail" do
+      SiteSetting.expects(:create_thumbnails?).returns(true)
+      SiteSetting.expects(:enable_s3_uploads?).returns(false)
+      upload.expects(:has_thumbnail?).returns(true)
+      OptimizedImage.expects(:create_for).never
+      upload.create_thumbnail!
     end
 
-    context "locally" do
-      before(:each) { LocalStore.stubs(:store_file).returns(url) }
-      it_behaves_like "upload"
+    it "creates a thumbnail" do
+      upload = Fabricate(:upload)
+      thumbnail = Fabricate(:optimized_image, upload: upload)
+      SiteSetting.expects(:create_thumbnails?).returns(true)
+      SiteSetting.expects(:enable_s3_uploads?).returns(false)
+      upload.expects(:has_thumbnail?).returns(false)
+      OptimizedImage.expects(:create_for).returns(thumbnail)
+      upload.create_thumbnail!
+      upload.reload
+      upload.optimized_images.count.should == 1
     end
 
   end
 
-  context 'has_been_uploaded?' do
+  context ".create_for" do
+
+    it "does not create another upload if it already exists" do
+      Upload.expects(:where).with(sha1: image_sha1).returns([upload])
+      Upload.expects(:create!).never
+      Upload.create_for(user_id, image).should == upload
+    end
+
+    it "computes width & height for images" do
+      SiteSetting.expects(:authorized_image?).returns(true)
+      FastImage.any_instance.expects(:size).returns([100, 200])
+      ImageSizer.expects(:resize)
+      ActionDispatch::Http::UploadedFile.any_instance.expects(:rewind)
+      Upload.create_for(user_id, image)
+    end
+
+    it "does not create an upload when there is an error with FastImage" do
+      SiteSetting.expects(:authorized_image?).returns(true)
+      Upload.expects(:create!).never
+      expect { Upload.create_for(user_id, attachment) }.to raise_error(FastImage::UnknownImageType)
+    end
+
+    it "does not compute width & height for non-image" do
+      SiteSetting.expects(:authorized_image?).returns(false)
+      FastImage.any_instance.expects(:size).never
+      Upload.create_for(user_id, image)
+    end
+
+    it "saves proper information" do
+      Upload.expects(:store_file).returns(url)
+      upload = Upload.create_for(user_id, image)
+      upload.user_id.should == user_id
+      upload.original_filename.should == image.original_filename
+      upload.filesize.should == File.size(image.tempfile)
+      upload.sha1.should == Digest::SHA1.file(image.tempfile).hexdigest
+      upload.width.should == 244
+      upload.height.should == 66
+      upload.url.should == url
+    end
+
+  end
+
+  context ".store_file" do
+
+    it "store files on s3 when enabled" do
+      SiteSetting.expects(:enable_s3_uploads?).returns(true)
+      LocalStore.expects(:store_file).never
+      S3.expects(:store_file)
+      Upload.store_file(image, image_sha1, 1)
+    end
+
+    it "store files locally by default" do
+      S3.expects(:store_file).never
+      LocalStore.expects(:store_file)
+      Upload.store_file(image, image_sha1, 1)
+    end
+
+  end
+
+  context ".remove_file" do
+
+    it "remove files on s3 when enabled" do
+      SiteSetting.expects(:enable_s3_uploads?).returns(true)
+      LocalStore.expects(:remove_file).never
+      S3.expects(:remove_file)
+      Upload.remove_file(upload.url)
+    end
+
+    it "remove files locally by default" do
+      S3.expects(:remove_file).never
+      LocalStore.expects(:remove_file)
+      Upload.remove_file(upload.url)
+    end
+
+  end
+
+  context ".has_been_uploaded?" do
 
     it "identifies internal or relatives urls" do
       Discourse.expects(:base_url_no_prefix).returns("http://discuss.site.com")
@@ -84,4 +174,14 @@ describe Upload do
 
   end
 
+  context ".get_from_url" do
+
+    it "works only when the file has been uploaded" do
+      Upload.expects(:has_been_uploaded?).returns(false)
+      Upload.expects(:where).never
+      Upload.get_from_url("discourse.org")
+    end
+
+  end
+
 end