[breaking] Make #images_cache= and #remote_images_urls= compatiblie w…

…ith keep-stored feature

This changes behavior of remote file download not to overwrite newly uploaded file

lib/carrierwave/mounter.rb

@@ -42,7 +42,9 @@ def cache(new_files)
       old_uploaders = uploaders
       @uploaders = new_files.map do |new_file|
         if new_file.is_a?(String)
-          old_uploaders.detect { |uploader| uploader.identifier == new_file }
+          uploader = old_uploaders.detect { |uploader| uploader.identifier == new_file }
+          uploader.staged = true if uploader
+          uploader
         else
           uploader = blank_uploader
           uploader.cache!(new_file)
@@ -65,11 +67,12 @@ def cache_names
     end
 
     def cache_names=(cache_names)
-      return if cache_names.blank? || uploaders.any?(&:cached?)
-      @uploaders = cache_names.map do |cache_name|
+      return if cache_names.blank?
+      clear_unstaged
+      cache_names.map do |cache_name|
         uploader = blank_uploader
         uploader.retrieve_from_cache!(cache_name)
-        uploader
+        @uploaders << uploader
       end
     rescue CarrierWave::InvalidParameter
     end
@@ -81,10 +84,11 @@ def remote_urls=(urls)
       @download_error = nil
       @integrity_error = nil
 
-      @uploaders = urls.zip(remote_request_headers || []).map do |url, header|
+      clear_unstaged
+      urls.zip(remote_request_headers || []).map do |url, header|
         uploader = blank_uploader
         uploader.download!(url, header || {})
-        uploader
+        @uploaders << uploader
       end
 
     rescue CarrierWave::DownloadError => e
@@ -164,5 +168,9 @@ def option(name)
       self.uploader_options[name] ||= record.class.uploader_option(column, name)
     end
 
+    def clear_unstaged
+      @uploaders ||= []
+      @uploaders.keep_if(&:staged)
+    end
   end # Mounter
 end # CarrierWave

lib/carrierwave/uploader/cache.rb

@@ -38,6 +38,16 @@ module Cache
       include CarrierWave::Uploader::Callbacks
       include CarrierWave::Uploader::Configuration
 
+      included do
+        prepend Module.new {
+          def initialize(*)
+            super
+            @staged = false
+          end
+        }
+        attr_accessor :staged
+      end
+
       module ClassMethods
 
         ##
@@ -125,6 +135,7 @@ def cache!(new_file = sanitized_file)
 
         self.cache_id = CarrierWave.generate_cache_id unless cache_id
 
+        @staged = true
         @filename = new_file.filename
         self.original_filename = new_file.filename
 
@@ -158,6 +169,7 @@ def cache!(new_file = sanitized_file)
       def retrieve_from_cache!(cache_name)
         with_callbacks(:retrieve_from_cache, cache_name) do
           self.cache_id, self.original_filename = cache_name.to_s.split('/', 2)
+          @staged = true
           @filename = original_filename
           @file = cache_storage.retrieve_from_cache!(full_filename(original_filename))
         end

lib/carrierwave/uploader/store.rb

@@ -70,6 +70,7 @@ def store!(new_file=nil)
             end
             @file = new_file
             @cache_id = @identifier = nil
+            @staged = false
           end
         end
       end

spec/mount_multiple_spec.rb

@@ -181,6 +181,11 @@ def rotate
         it "copies files into the cache directory" do
           expect(instance.images[0].current_path).to match(/^#{public_path('uploads/tmp')}/)
         end
+
+        it "marks the uploader as staged" do
+          expect(instance.images[0].staged).to be true
+          expect(instance.images[1].staged).to be true
+        end
       end
 
       it "does nothing when nil is assigned" do
@@ -244,6 +249,12 @@ def monkey
         end
         let(:identifiers) { instance.images.map(&:identifier) }
 
+        it "writes over a previously stored file" do
+          instance.images = [old_image_stub]
+          instance.store_images!
+          expect(instance.images.map(&:identifier)).to eq ['old.jpeg']
+        end
+
         it "preserves existing image of given identifier" do
           instance.images = [identifiers[0], old_image_stub]
           instance.store_images!
@@ -390,30 +401,43 @@ def monkey
         CarrierWave::SanitizedFile.new(test_file_stub).copy_to(public_path('uploads/tmp/1369894322-123-0123-1234/test.jpg'))
       end
 
-      before { instance.images_cache = images_cache }
-
       context "does nothing when nil is assigned" do
-        let(:images_cache) { nil }
+        before { instance.images_cache = nil }
 
         it { expect(instance.images).to be_empty }
       end
 
       context "does nothing when an empty string is assigned" do
-        let(:images_cache) { '' }
+        before { instance.images_cache = '' }
 
         it { expect(instance.images).to be_empty }
       end
 
       context "retrieve from cache when a cache name is assigned" do
-        let(:images_cache) { ['1369894322-123-0123-1234/test.jpg'].to_json }
+        before { instance.images_cache = ['1369894322-123-0123-1234/test.jpg'].to_json }
 
         it { expect(instance.images[0].current_path).to eq(public_path('uploads/tmp/1369894322-123-0123-1234/test.jpg')) }
+
+        it "marks the uploader as staged" do
+          expect(instance.images[0].staged).to be true
+        end
       end
 
-      context "doesn't write over a previously assigned file" do
-        let(:images_cache) { ['1369894322-123-0123-1234/monkey.jpg'].to_json }
+      context "writes over a previously stored file" do
+        before do
+          instance.images = [test_file_stub]
+          instance.store_images!
+          instance.images_cache = ['1369894322-123-0123-1234/monkey.jpg'].to_json
+        end
 
-        before { instance.images = [test_file_stub] }
+        it { expect(instance.images[0].current_path).to match(/monkey.jpg$/) }
+      end
+
+      context "doesn't write over a previously assigned file" do
+        before do
+          instance.images = [test_file_stub]
+          instance.images_cache = ['1369894322-123-0123-1234/monkey.jpg'].to_json
+        end
 
         it { expect(instance.images[0].current_path).to match(/test.jpg$/) }
       end
@@ -461,20 +485,38 @@ def monkey
         let(:remote_images_url) { ["http://www.example.com/test.jpg"] }
 
         it { is_expected.to match(/test.jpg$/) }
+
+        it "marks the uploader as staged" do
+          expect(instance.images[0].staged).to be true
+        end
       end
 
-      context "writes over a previously assigned file" do
+      context "writes over a previously stored file" do
         subject { images[0].current_path }
 
         let(:remote_images_url) { ["http://www.example.com/test.jpg"] }
 
         before do
           instance.images = [stub_file("portrait.jpg")]
+          instance.store_images!
           instance.remote_images_urls = remote_images_url
         end
 
         it { is_expected.to match(/test.jpg$/) }
       end
+
+      context "does not write over a previously assigned file" do
+        subject { images[0].current_path }
+
+        let(:remote_images_url) { ["http://www.example.com/test.jpg"] }
+
+        before do
+          instance.images = [stub_file("portrait.jpg")]
+          instance.remote_images_urls = remote_images_url
+        end
+
+        it { is_expected.to match(/portrait.jpg$/) }
+      end
     end
 
     describe '#store_images!' do
@@ -498,6 +540,10 @@ def monkey
         end
 
         it { expect(instance.images[0].current_path).to eq(public_path("uploads/#{test_file_name}")) }
+
+        it "marks the uploader as unstaged" do
+          expect(instance.images[0].staged).to be false
+        end
       end
 
       context "removes an uploaded file when remove_images is true" do

spec/mount_single_spec.rb

@@ -348,11 +348,11 @@ def default_url
         expect(@instance.image.current_path).to match(/test.jpg$/)
       end
 
-      it "writes over a previously assigned file" do
+      it "does not write over a previously assigned file" do
         @instance.image = stub_file("portrait.jpg")
         @instance.remote_image_url = "http://www.example.com/test.jpg"
 
-        expect(@instance.image.current_path).to match(/test.jpg$/)
+        expect(@instance.image.current_path).to match(/portrait.jpg$/)
       end
     end