diff --git a/README.md b/README.md index 47b81fbef..07a341138 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,17 @@ u.avatars[0].current_path # => 'path/to/file.png' u.avatars[0].identifier # => 'file.png' ``` +If you want to preserve existing files on uploading new one, you can go like: + +```erb +<% user.avatars.each do |avatar| %> + <%= hidden_field :user, :avatars, multiple: true, value: avatar.identifier %> +<% end %> +<%= form.file_field :avatars, multiple: true %> +``` + +Sorting avatars is supported as well by reordering `hidden_field`, an example using jQuery UI Sortable is available [here](https://github.com/carrierwaveuploader/carrierwave/wiki/How-to%3A-Add%2C-remove-and-reorder-images-using-multiple-file-upload). + ## Changing the storage directory In order to change where uploaded files are put, just override the `store_dir` diff --git a/lib/carrierwave/mount.rb b/lib/carrierwave/mount.rb index 718ca3adb..7c47ad3a5 100644 --- a/lib/carrierwave/mount.rb +++ b/lib/carrierwave/mount.rb @@ -185,6 +185,18 @@ def #{column}_identifier _mounter(:#{column}).read_identifiers[0] end + def #{column}_integrity_error + #{column}_integrity_errors.last + end + + def #{column}_processing_error + #{column}_processing_errors.last + end + + def #{column}_download_error + #{column}_download_errors.last + end + def store_previous_changes_for_#{column} attribute_changes = ::ActiveRecord.version.to_s.to_f >= 5.1 ? saved_changes : changes @_previous_changes_for_#{column} = attribute_changes[_mounter(:#{column}).serialization_column] @@ -240,9 +252,9 @@ def remove_previously_stored_#{column} # [store_images!] Stores all files that have been assigned with +images=+ # [remove_images!] Removes the uploaded file from the filesystem. # - # [images_integrity_error] Returns an error object if the last files to be assigned caused an integrity error - # [images_processing_error] Returns an error object if the last files to be assigned caused a processing error - # [images_download_error] Returns an error object if the last files to be remotely assigned caused a download error + # [image_integrity_errors] Returns error objects of files which failed to pass integrity check + # [image_processing_errors] Returns error objects of files which failed to be processed + # [image_download_errors] Returns error objects of files which failed to be downloaded # # [image_identifiers] Reads out the identifiers of the files # @@ -395,16 +407,16 @@ def store_#{column}! _mounter(:#{column}).store! end - def #{column}_integrity_error - _mounter(:#{column}).integrity_error + def #{column}_integrity_errors + _mounter(:#{column}).integrity_errors end - def #{column}_processing_error - _mounter(:#{column}).processing_error + def #{column}_processing_errors + _mounter(:#{column}).processing_errors end - def #{column}_download_error - _mounter(:#{column}).download_error + def #{column}_download_errors + _mounter(:#{column}).download_errors end def mark_remove_#{column}_false diff --git a/lib/carrierwave/mounter.rb b/lib/carrierwave/mounter.rb index 7cd1dda66..c6a45781a 100644 --- a/lib/carrierwave/mounter.rb +++ b/lib/carrierwave/mounter.rb @@ -3,14 +3,17 @@ module CarrierWave # this is an internal class, used by CarrierWave::Mount so that # we don't pollute the model with a lot of methods. class Mounter #:nodoc: - attr_reader :column, :record, :remote_urls, :integrity_error, - :processing_error, :download_error + attr_reader :column, :record, :remote_urls, :integrity_errors, + :processing_errors, :download_errors attr_accessor :remove, :remote_request_headers def initialize(record, column, options={}) @record = record @column = column @options = record.class.uploader_options[column] + @download_errors = [] + @processing_errors = [] + @integrity_errors = [] end def uploader_class @@ -38,21 +41,30 @@ def uploaders end def cache(new_files) - return if new_files.blank? + return if !new_files.is_a?(Array) && new_files.blank? + old_uploaders = uploaders @uploaders = new_files.map do |new_file| - uploader = blank_uploader - uploader.cache!(new_file) - uploader - end - - @integrity_error = nil - @processing_error = nil - rescue CarrierWave::IntegrityError => e - @integrity_error = e - raise e unless option(:ignore_integrity_errors) - rescue CarrierWave::ProcessingError => e - @processing_error = e - raise e unless option(:ignore_processing_errors) + handle_error do + if new_file.is_a?(String) + if (uploader = old_uploaders.detect { |uploader| uploader.identifier == new_file }) + uploader.staged = true + uploader + else + begin + uploader = blank_uploader + uploader.retrieve_from_cache!(new_file) + uploader + rescue CarrierWave::InvalidParameter + nil + end + end + else + uploader = blank_uploader + uploader.cache!(new_file) + uploader + end + end + end.compact end def cache_names @@ -60,37 +72,32 @@ def cache_names end def cache_names=(cache_names) - return if cache_names.blank? || uploaders.any?(&:cached?) - @uploaders = cache_names.map do |cache_name| - uploader = blank_uploader - uploader.retrieve_from_cache!(cache_name) - uploader + return if cache_names.blank? + clear_unstaged + cache_names.map do |cache_name| + begin + uploader = blank_uploader + uploader.retrieve_from_cache!(cache_name) + @uploaders << uploader + rescue CarrierWave::InvalidParameter + # ignore + end end - rescue CarrierWave::InvalidParameter end def remote_urls=(urls) return if urls.blank? || urls.all?(&:blank?) @remote_urls = urls - @download_error = nil - @integrity_error = nil - @uploaders = urls.zip(remote_request_headers || []).map do |url, header| - uploader = blank_uploader - uploader.download!(url, header || {}) - uploader + clear_unstaged + urls.zip(remote_request_headers || []).map do |url, header| + handle_error do + uploader = blank_uploader + uploader.download!(url, header || {}) + @uploaders << uploader + end end - - rescue CarrierWave::DownloadError => e - @download_error = e - raise e unless option(:ignore_download_errors) - rescue CarrierWave::ProcessingError => e - @processing_error = e - raise e unless option(:ignore_processing_errors) - rescue CarrierWave::IntegrityError => e - @integrity_error = e - raise e unless option(:ignore_integrity_errors) end def store! @@ -159,5 +166,22 @@ def option(name) self.uploader_options[name] ||= record.class.uploader_option(column, name) end + def clear_unstaged + @uploaders ||= [] + @uploaders.keep_if(&:staged) + end + + def handle_error + yield + rescue CarrierWave::DownloadError => e + @download_errors << e + raise e unless option(:ignore_download_errors) + rescue CarrierWave::ProcessingError => e + @processing_errors << e + raise e unless option(:ignore_processing_errors) + rescue CarrierWave::IntegrityError => e + @integrity_errors << e + raise e unless option(:ignore_integrity_errors) + end end # Mounter end # CarrierWave diff --git a/lib/carrierwave/uploader/cache.rb b/lib/carrierwave/uploader/cache.rb index f35da282f..c741a10af 100644 --- a/lib/carrierwave/uploader/cache.rb +++ b/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 diff --git a/lib/carrierwave/uploader/mountable.rb b/lib/carrierwave/uploader/mountable.rb index 9dd961680..bba6878b9 100644 --- a/lib/carrierwave/uploader/mountable.rb +++ b/lib/carrierwave/uploader/mountable.rb @@ -33,6 +33,12 @@ def initialize(model=nil, mounted_as=nil) @mounted_as = mounted_as end + ## + # Returns array index of given uploader within currently mounted uploaders + # + def index + model.__send__(:_mounter, mounted_as).uploaders.index(self) + end end # Mountable end # Uploader end # CarrierWave diff --git a/lib/carrierwave/uploader/store.rb b/lib/carrierwave/uploader/store.rb index 272159899..457cc7b5a 100644 --- a/lib/carrierwave/uploader/store.rb +++ b/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 diff --git a/lib/carrierwave/validations/active_model.rb b/lib/carrierwave/validations/active_model.rb index 27dad42aa..6be0a3e70 100644 --- a/lib/carrierwave/validations/active_model.rb +++ b/lib/carrierwave/validations/active_model.rb @@ -11,7 +11,7 @@ module ActiveModel class ProcessingValidator < ::ActiveModel::EachValidator def validate_each(record, attribute, value) - if e = record.__send__("#{attribute}_processing_error") + record.__send__("#{attribute}_processing_errors").each do |e| message = (e.message == e.class.to_s) ? :carrierwave_processing_error : e.message record.errors.add(attribute, message) end @@ -21,7 +21,7 @@ def validate_each(record, attribute, value) class IntegrityValidator < ::ActiveModel::EachValidator def validate_each(record, attribute, value) - if e = record.__send__("#{attribute}_integrity_error") + record.__send__("#{attribute}_integrity_errors").each do |e| message = (e.message == e.class.to_s) ? :carrierwave_integrity_error : e.message record.errors.add(attribute, message) end @@ -31,7 +31,7 @@ def validate_each(record, attribute, value) class DownloadValidator < ::ActiveModel::EachValidator def validate_each(record, attribute, value) - if e = record.__send__("#{attribute}_download_error") + record.__send__("#{attribute}_download_errors").each do |e| message = (e.message == e.class.to_s) ? :carrierwave_download_error : e.message record.errors.add(attribute, message) end diff --git a/spec/mount_multiple_spec.rb b/spec/mount_multiple_spec.rb index 48475687b..ba9f08ace 100644 --- a/spec/mount_multiple_spec.rb +++ b/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 @@ -194,47 +199,139 @@ def rotate instance.images = [test_file_stub] end - context "fails silently if the images fails a white list integrity check" do + context "if the images fails a white list integrity check" do before do uploader.class_eval do def extension_whitelist %w(txt) end end + end - instance.images = [text_file_stub, test_file_stub] + it "fails silently" do + expect { instance.images = [test_file_stub] }.not_to raise_error end - it { expect(instance.images).to be_empty } + it "keeps files which passed the check" do + instance.images = [test_file_stub, text_file_stub] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end end - describe "fails silently if the images fails a black list integrity check" do + describe "if the images fails a black list integrity check" do before do uploader.class_eval do def extension_blacklist %w(jpg) end end + end - instance.images = [text_file_stub, test_file_stub] + it "fails silently" do + expect { instance.images = [test_file_stub] }.not_to raise_error end - it { expect(instance.images).to be_empty } + it "keeps files which passed the check" do + instance.images = [test_file_stub, text_file_stub] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end end - describe "fails silently if the images fails to be processed" do + describe "if the images fails to be processed" do before do uploader.class_eval do process :monkey def monkey - raise CarrierWave::ProcessingError, "Ohh noez!" + raise CarrierWave::ProcessingError, "Ohh noez!" if file.path =~ /test\.jpg/ end end + end - instance.images = [test_file_stub] + it "fails silently" do + expect { instance.images = [test_file_stub] }.not_to raise_error end - it { expect(instance.images).to be_empty } + it "keeps files which was processed successfully" do + instance.images = [test_file_stub, text_file_stub] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end + end + + describe "with cached files" do + before do + instance.images = [text_file_stub, test_file_stub] + end + let(:cache_names) { instance.images.map(&:cache_name) } + let(:identifiers) { instance.images.map(&:identifier) } + + it "accepts cache name and retrieves from cache" do + instance.images = [cache_names[1]] + expect(instance.images.map { |u| u.file.filename }).to eq ['test.jpg'] + end + end + + describe "with stored files" do + before do + instance.images = [text_file_stub, test_file_stub] + instance.store_images! + 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! + expect(instance.images.map(&:identifier)).to eq ['bork.txt','old.jpeg'] + end + + it "reorders existing image" do + instance.images = identifiers.reverse + instance.store_images! + expect(instance.images.map(&:identifier)).to eq ['test.jpg', 'bork.txt'] + end + + it "allows uploading and reordering at once" do + instance.images = [identifiers[1], old_image_stub, identifiers[0]] + instance.store_images! + expect(instance.images.map(&:identifier)).to eq ['test.jpg', 'old.jpeg', 'bork.txt'] + end + + it "allows repeating the same identifiers" do + instance.images = ['bork.txt', 'test.jpg', 'bork.txt'] + instance.store_images! + expect(instance.images.map(&:identifier)).to eq ['bork.txt', 'test.jpg', 'bork.txt'] + end + + it "removes image which is unused" do + @image_paths = instance.images.map(&:current_path) + instance.images = [identifiers[0]] + instance.store_images! + instance.send(:_mounter, :images).remove_previous(identifiers, identifiers[0..0]) + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + expect(File.exist?(@image_paths[0])).to be_truthy + expect(File.exist?(@image_paths[1])).to be_falsey + end + + it "ignores unknown identifier" do + instance.images = ['unknown.txt'] + expect { instance.store_images! }.not_to raise_error + expect(instance.images.map(&:identifier)).to be_empty + end + + it "allows deleting all files" do + instance.images = [] + expect(instance.images.map(&:identifier)).to be_empty + end + + it "allows assignment of uploader instances" do + instance.images = [instance.images[0]] + expect(instance.images.map(&:identifier)).to eq ['bork.txt'] + end end end @@ -342,33 +439,54 @@ 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 + + context "when valid and invalid cache names are assigned" do + before { instance.images_cache = ['1369894322-123-0123-1234/test.jpg', 'invalid'].to_json } + + it "retrieves valid file only from cache" do + expect(instance.images.map(&:cache_name)).to eq(['1369894322-123-0123-1234/test.jpg']) + end + end end describe "#remote_images_urls" do @@ -392,6 +510,7 @@ def monkey before do stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub)) + stub_request(:get, "http://www.example.com/test.txt").to_return(status: 404) instance.remote_images_urls = remote_images_url end @@ -413,20 +532,46 @@ 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 + + context "if a file fails to be downloaded" do + let(:remote_images_url) { ["http://www.example.com/test.txt", "http://www.example.com/test.jpg"] } + + it "keeps files which was downloaded successfully" do + expect(instance.images.map(&:identifier)).to eq ['test.jpg'] + end + end end describe '#store_images!' do @@ -450,6 +595,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 @@ -537,17 +686,17 @@ def monkey end end - describe '#images_integrity_error' do - subject(:images_integrity_error) { instance.images_integrity_error } + describe '#images_integrity_errors' do + subject(:images_integrity_errors) { instance.images_integrity_errors } describe "default behaviour" do - it { is_expected.to be_nil } + it { is_expected.to be_empty } end context "when a file is cached" do before { instance.images = test_file_stub } - it { is_expected.to be_nil } + it { is_expected.to be_empty } end describe "when an integrity check fails" do @@ -562,10 +711,10 @@ def extension_whitelist context "when file is cached" do before { instance.images = [test_file_stub] } - it { is_expected.to be_an_instance_of(CarrierWave::IntegrityError) } + it { is_expected.to include(a_kind_of(CarrierWave::IntegrityError)) } it "has an error message" do - expect(images_integrity_error.message.lines.grep(/^You are not allowed to upload/)).to be_truthy + expect(images_integrity_errors[0].message.lines.grep(/^You are not allowed to upload/)).to be_truthy end end @@ -575,10 +724,10 @@ def extension_whitelist instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"] end - it { is_expected.to be_an_instance_of(CarrierWave::IntegrityError) } + it { is_expected.to include(a_kind_of(CarrierWave::IntegrityError)) } it "has an error message" do - expect(images_integrity_error.message.lines.grep(/^You are not allowed to upload/)).to be_truthy + expect(images_integrity_errors[0].message.lines.grep(/^You are not allowed to upload/)).to be_truthy end end @@ -588,26 +737,26 @@ def extension_whitelist instance.remote_images_urls = "" end - it { is_expected.to be_an_instance_of(CarrierWave::IntegrityError) } + it { is_expected.to include(a_kind_of(CarrierWave::IntegrityError)) } it "has an error message" do - expect(images_integrity_error.message.lines.grep(/^You are not allowed to upload/)).to be_truthy + expect(images_integrity_errors[0].message.lines.grep(/^You are not allowed to upload/)).to be_truthy end end end end - describe '#images_processing_error' do - subject(:images_processing_error) { instance.images_processing_error } + describe '#images_processing_errors' do + subject(:images_processing_errors) { instance.images_processing_errors } describe "default behavior" do - it { is_expected.to be_nil } + it { is_expected.to be_empty } end context "when file is cached" do before { instance.images = [test_file_stub] } - it { is_expected.to be_nil } + it { is_expected.to be_empty } end describe "when an processing error occurs" do @@ -623,7 +772,7 @@ def monkey context "when file is cached" do before { instance.images = [test_file_stub] } - it { is_expected.to be_an_instance_of(CarrierWave::ProcessingError) } + it { is_expected.to include(a_kind_of(CarrierWave::ProcessingError)) } end context "when file was downloaded" do @@ -632,13 +781,13 @@ def monkey instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"] end - it { is_expected.to be_an_instance_of(CarrierWave::ProcessingError) } + it { is_expected.to include(a_kind_of(CarrierWave::ProcessingError)) } end end end - describe '#images_download_error' do - subject(:images_download_error) { instance.images_download_error } + describe '#images_download_errors' do + subject(:images_download_errors) { instance.images_download_errors } before do stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub)) @@ -646,19 +795,19 @@ def monkey end describe "default behaviour" do - it { expect(instance.images_download_error).to be_nil } + it { expect(instance.images_download_errors).to be_empty } end context "when file download was successful" do before { instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"] } - it { is_expected.to be_nil } + it { is_expected.to be_empty } end context "when file couldn't be found" do before { instance.remote_images_urls = ["http://www.example.com/missing.jpg"] } - it { is_expected.to be_an_instance_of(CarrierWave::DownloadError) } + it { is_expected.to include(a_kind_of(CarrierWave::DownloadError)) } end end diff --git a/spec/mount_single_spec.rb b/spec/mount_single_spec.rb index ed69ca1ff..a8069eef8 100644 --- a/spec/mount_single_spec.rb +++ b/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 diff --git a/spec/uploader/mountable_spec.rb b/spec/uploader/mountable_spec.rb index e1088af0a..75e479b58 100644 --- a/spec/uploader/mountable_spec.rb +++ b/spec/uploader/mountable_spec.rb @@ -23,4 +23,17 @@ expect(uploader.mounted_as).to eq(:llama) end end + + describe '#index' do + let(:model) { Class.new.send(:extend, CarrierWave::Mount) } + let(:instance) { model.new } + before do + model.mount_uploaders(:images, uploader_class) + instance.images = [stub_file('test.jpg'), stub_file('bork.txt')] + end + + it "returns the current index in uploaders" do + expect(instance.images.map(&:index)).to eq [0, 1] + end + end end