Make conditional filename changes work by applying #force_extension o…

…nly for versions

Closes 2723

lib/carrierwave/uploader/cache.rb

@@ -169,6 +169,14 @@ def retrieve_from_cache!(cache_name)
           @staged = true
           @filename = original_filename
           @file = cache_storage.retrieve_from_cache!(full_original_filename)
+          unless @file.exists?
+            # For compatibility with CarrierWave 3.0.x
+            extension = self.class.processors.reverse.detect do |processor, *_|
+              processor == :convert
+            end&.[](1)
+            self.force_extension = extension
+            @file = cache_storage.retrieve_from_cache!(full_original_filename) if extension
+          end
         end
       end
 

lib/carrierwave/uploader/processing.rb

@@ -66,11 +66,6 @@ def process(*args)
           condition = new_processors.delete(:if) || new_processors.delete(:unless)
           new_processors.each do |processor, processor_args|
             self.processors += [[processor, processor_args, condition, condition_type]]
-
-            if processor == :convert
-              # Treat :convert specially, since it should trigger the file extension change
-              force_extension processor_args
-            end
           end
         end
       end # ClassMethods

lib/carrierwave/uploader/versions.rb

@@ -27,6 +27,18 @@ def build(superclass)
           @klass.processors = []
           @klass.version_options = @options
           @klass.class_eval <<-RUBY, __FILE__, __LINE__ + 1
+            def self.process(*args)
+              super
+              processors.each do |processor, processor_args, condition, _|
+                next unless processor == :convert
+
+                raise 'Conditionals are now allowed for `process :convert` within versions, as filename needs to be deterministic' if condition
+
+                # Treat :convert specially, since it should trigger the file extension change
+                force_extension processor_args
+              end
+            end
+
             # Define the enable_processing method for versions so they get the
             # value from the parent class unless explicitly overwritten
             def self.enable_processing(value=nil)

spec/uploader/processing_spec.rb

@@ -267,6 +267,57 @@ def filename
     end
   end
 
+  context "when using #convert conditionally" do
+    before do
+      uploader_class.class_eval do
+        include CarrierWave::MiniMagick
+        process convert: :png, if: :process?
+      end
+    end
+
+    it "changes the extension when processed" do
+      allow(uploader).to receive(:process?).and_return(true)
+      uploader.store!(File.open(file_path('landscape.jpg')))
+      expect(uploader).to be_format('png')
+      expect(uploader.file.filename).to eq 'landscape.png'
+      expect(uploader.identifier).to eq 'landscape.png'
+    end
+
+    it "keeps the extension when unprocessed" do
+      allow(uploader).to receive(:process?).and_return(false)
+      uploader.store!(File.open(file_path('landscape.jpg')))
+      expect(uploader).to be_format('jpeg')
+      expect(uploader.file.filename).to eq 'landscape.jpg'
+      expect(uploader.identifier).to eq 'landscape.jpg'
+    end
+
+    context "with #filename overridden" do
+      let(:changed_extension) { '.png' }
+
+      before do
+        uploader_class.class_eval <<-RUBY, __FILE__, __LINE__+1
+          def filename
+            "image.\#{file.extension}"
+          end
+        RUBY
+      end
+
+      it "changes the extension when processed" do
+        allow(uploader).to receive(:process?).and_return(true)
+        uploader.store!(File.open(file_path('landscape.jpg')))
+        expect(uploader.identifier).to eq 'image.png'
+        expect(uploader.url).to eq '/uploads/image.png'
+      end
+
+      it "changes the extension when unprocessed" do
+        allow(uploader).to receive(:process?).and_return(false)
+        uploader.store!(File.open(file_path('landscape.jpg')))
+        expect(uploader.identifier).to eq 'image.jpg'
+        expect(uploader.url).to eq '/uploads/image.jpg'
+      end
+    end
+  end
+
   context "when converting format not using CarrierWave::MiniMagick#convert" do
     let(:another_uploader) { uploader_class.new }
     before do

spec/uploader/store_spec.rb

@@ -180,6 +180,7 @@
     before do
       @cached_file = double('a cached file')
       allow(@cached_file).to receive(:delete)
+      allow(@cached_file).to receive(:exists?)
 
       @stored_file = double('a stored file')
       allow(@stored_file).to receive(:path).and_return('/path/to/somewhere')

spec/uploader/versions_spec.rb

@@ -956,6 +956,13 @@ def condition(_); false; end
       expect(@another_uploader.thumb.url).to eq @uploader.thumb.url
     end
 
+    it 'does not allow conditionals' do
+      @uploader_class.version(:preview) do
+        process convert: :png, if: :true?
+      end
+      expect { @uploader.preview }.to raise_error /Conditionals are now allowed/
+    end
+
     context 'with base filename overridden' do
       before do
         @uploader_class.class_eval do