Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Allow for custom CarrierWave error messages #450

Merged
merged 3 commits into from

3 participants

Bart Teeuwisse Jonas Nicklas Trevor Turk
Bart Teeuwisse

Hi Jonas,

this patch allows one to rais CarrierWave errors with custom messages. Prior to this patch all processing errors would read: 'failed to be processed' even when one raises a CarrierWave processing error with a custom message.

Similarly CarrierWave's own integrity error message is replaced with the rather short "is not an allowed file type" w/o explaining what extensions are accepted.

I hope you'll accept this patch.

Jonas Nicklas
Owner

Could you add some tests for this behaviour, not sure I quite understand what this is doing?

Bart Teeuwisse - I18n all error messages.
- Add test for all but mime_types
c564f2a
Bart Teeuwisse

Jonas, I've added test cases and I18n all error messages. Give it try. See if you can force a ::MIME::InvalidContentType error, that is the only one missing.

Trevor Turk
Owner

@bartt I'm seeing "This pull request cannot be automatically merged." so you may need to rebase your commits in order for us to merge. I'd check with @jnicklas before doing more work, though -- I'm not clear on whether or not he's interested in merging.

Bart Teeuwisse Merge branch 'master' of git://github.com/jnicklas/carrierwave
Conflicts:
	spec/orm/activerecord_spec.rb
769622a
Bart Teeuwisse

@trevorturk, conflict resolved. I do hope @jnicklas will merge the changes as the current code doesn't allow for custom error messages. These changes allow for custom error messages while preserving i18n.

Trevor Turk trevorturk merged commit 84b3918 into from
Trevor Turk
Owner

Merged -- thanks @bartt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 6, 2011
  1. - Allow for custom CarrierWave error messages.

    Bart Teeuwisse authored
Commits on Sep 7, 2011
  1. - I18n all error messages.

    Bart Teeuwisse authored
    - Add test for all but mime_types
Commits on Sep 18, 2011
  1. Merge branch 'master' of git://github.com/jnicklas/carrierwave

    Bart Teeuwisse authored
    Conflicts:
    	spec/orm/activerecord_spec.rb
This page is out of date. Refresh to see the latest.
6 lib/carrierwave/locale/en.yml
View
@@ -2,4 +2,8 @@ en:
errors:
messages:
carrierwave_processing_error: failed to be processed
- carrierwave_integrity_error: is not an allowed file type
+ carrierwave_integrity_error: is not of an allowed file type
+ extension_white_list_error: "You are not allowed to upload %{extension} files, allowed types: %{allowed_types}"
+ rmagick_processing_error: "Failed to manipulate with rmagick, maybe it is not an image? Original Error: %{e}"
+ mime_types_processing_error: "Failed to process file with MIME::Types, maybe not valid content-type? Original Error: %{e}"
+ mini_magick_processing_error: "Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: %{e}"
2  lib/carrierwave/processing/mime_types.rb
View
@@ -51,7 +51,7 @@ def set_content_type(override=false)
end
end
rescue ::MIME::InvalidContentType => e
- raise CarrierWave::ProcessingError.new("Failed to process file with MIME::Types, maybe not valid content-type? Original Error: #{e}")
+ raise CarrierWave::ProcessingError, I18n.translate(:"errors.messages.mime_types_processing_error", :e => e)
end
end # MimeTypes
2  lib/carrierwave/processing/mini_magick.rb
View
@@ -245,7 +245,7 @@ def manipulate!
image.write(current_path)
::MiniMagick::Image.open(current_path)
rescue ::MiniMagick::Error, ::MiniMagick::Invalid => e
- raise CarrierWave::ProcessingError.new("Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: #{e}")
+ raise CarrierWave::ProcessingError, I18n.translate(:"errors.messages.mini_magick_processing_error", :e => e)
end
end # MiniMagick
2  lib/carrierwave/processing/rmagick.rb
View
@@ -266,7 +266,7 @@ def manipulate!(options={})
end
destroy_image(frames)
rescue ::Magick::ImageMagickError => e
- raise CarrierWave::ProcessingError.new("Failed to manipulate with rmagick, maybe it is not an image? Original Error: #{e}")
+ raise CarrierWave::ProcessingError, I18n.translate(:"errors.messages.rmagick_processing_error", :e => e)
end
private
2  lib/carrierwave/uploader/extension_whitelist.rb
View
@@ -40,7 +40,7 @@ def extension_white_list; end
def check_whitelist!(new_file)
extension = new_file.extension.to_s
if extension_white_list and not extension_white_list.detect { |item| extension =~ /\A#{item}\z/i }
- raise CarrierWave::IntegrityError, "You are not allowed to upload #{new_file.extension.inspect} files, allowed types: #{extension_white_list.inspect}"
+ raise CarrierWave::IntegrityError, I18n.translate(:"errors.messages.extension_white_list_error", :extension => new_file.extension.inspect, :allowed_types => extension_white_list.inspect)
end
end
29 lib/carrierwave/validations/active_model.rb
View
@@ -14,8 +14,9 @@ module ActiveModel
class ProcessingValidator < ::ActiveModel::EachValidator
def validate_each(record, attribute, value)
- if record.send("#{attribute}_processing_error")
- record.errors.add(attribute, :carrierwave_processing_error)
+ if e = record.send("#{attribute}_processing_error")
+ message = (e.message == e.class.to_s) ? :carrierwave_processing_error : e.message
+ record.errors.add(attribute, message)
end
end
end
@@ -23,8 +24,9 @@ def validate_each(record, attribute, value)
class IntegrityValidator < ::ActiveModel::EachValidator
def validate_each(record, attribute, value)
- if record.send("#{attribute}_integrity_error")
- record.errors.add(attribute, :carrierwave_integrity_error)
+ if e = record.send("#{attribute}_integrity_error")
+ message = (e.message == e.class.to_s) ? :carrierwave_integrity_error : e.message
+ record.errors.add(attribute, message)
end
end
end
@@ -36,14 +38,6 @@ module HelperMethods
#
# Accepts the usual parameters for validations in Rails (:if, :unless, etc...)
#
- # === Note
- #
- # Set this key in your translations file for I18n:
- #
- # carrierwave:
- # errors:
- # integrity: 'Here be an error message'
- #
def validates_integrity_of(*attr_names)
validates_with IntegrityValidator, _merge_attributes(attr_names)
end
@@ -54,14 +48,6 @@ def validates_integrity_of(*attr_names)
#
# Accepts the usual parameters for validations in Rails (:if, :unless, etc...)
#
- # === Note
- #
- # Set this key in your translations file for I18n:
- #
- # carrierwave:
- # errors:
- # processing: 'Here be an error message'
- #
def validates_processing_of(*attr_names)
validates_with ProcessingValidator, _merge_attributes(attr_names)
end
@@ -75,5 +61,4 @@ def validates_processing_of(*attr_names)
end
end
-I18n.load_path << File.join(File.dirname(__FILE__), "..", "locale", 'en.yml')
-
+I18n.load_path << File.join(File.dirname(__FILE__), "..", "locale", 'en.yml')
4 spec/mount_spec.rb
View
@@ -405,7 +405,9 @@ def extension_white_list
end
end
@instance.image = stub_file('test.jpg')
- @instance.image_integrity_error.should be_an_instance_of(CarrierWave::IntegrityError)
+ e = @instance.image_integrity_error
+ e.should be_an_instance_of(CarrierWave::IntegrityError)
+ e.message.grep(/^You are not allowed to upload/).should be_true
end
end
43 spec/orm/activerecord_spec.rb
View
@@ -143,26 +143,52 @@ def extension_white_list
%w(txt)
end
end
+ end
+
+ it "should use I18n for integrity error messages" do
+ # Localize the error message to Dutch
+ change_locale_and_store_translations(:nl, :errors => {
+ :messages => {
+ :extension_white_list_error => "Het opladen van %{extension} bestanden is niet toe gestaan. Geaccepteerde types: %{allowed_types}"
+ }
+ }) do
+ # Assigning image triggers check_whitelist! and thus should be inside change_locale_and_store_translations
+ @event.image = stub_file('test.jpg')
+ @event.should_not be_valid
+ @event.valid?
+ @event.errors[:image].should == ['Het opladen van "jpg" bestanden is niet toe gestaan. Geaccepteerde types: ["txt"]']
+ end
+ end
+ end
+
+ context 'when validating processing' do
+ before do
+ @uploader.class_eval do
+ process :monkey
+ def monkey
+ raise CarrierWave::ProcessingError
+ end
+ end
@event.image = stub_file('test.jpg')
end
- it "should make the record invalid when an integrity error occurs" do
+ it "should make the record invalid when a processing error occurs" do
@event.should_not be_valid
end
- it "should use I18n for integrity error messages" do
+ it "should use I18n for processing errors without messages" do
@event.valid?
- @event.errors[:image].should == ['is not an allowed file type']
+ @event.errors[:image].should == ['failed to be processed']
change_locale_and_store_translations(:pt, :activerecord => {
:errors => {
:messages => {
- :carrierwave_integrity_error => 'tipo de imagem não permitido.'
+ :carrierwave_processing_error => 'falha ao processar imagem.'
}
}
}) do
@event.should_not be_valid
- @event.errors[:image].should == ['tipo de imagem não permitido.']
+ @event.errors[:image].should == ['falha ao processar imagem.']
end
end
end
@@ -182,9 +208,9 @@ def monkey
@event.should_not be_valid
end
- it "should use I18n for processing error messages" do
+ it "should use the error's messages for processing errors with messages" do
@event.valid?
- @event.errors[:image].should == ['failed to be processed']
+ @event.errors[:image].should == ['Ohh noez!']
change_locale_and_store_translations(:pt, :activerecord => {
:errors => {
@@ -194,11 +220,10 @@ def monkey
}
}) do
@event.should_not be_valid
- @event.errors[:image].should == ['falha ao processar imagem.']
+ @event.errors[:image].should == ['Ohh noez!']
end
end
end
-
end
describe '#save' do
12 spec/processing/mime_types_spec.rb
View
@@ -48,4 +48,16 @@
end
+ describe "test errors" do
+ context "invalid mime type" do
+ before do
+ @instance.file.content_type = nil
+ # TODO: somehow force a ::MIME::InvalidContentType error when set_content_type is called.
+ end
+
+ it "should raise a MIME::InvalidContentType error" do
+ # lambda {@instance.set_content_type}.should raise_exception(::MIME::InvalidContentType, /^Failed to process file with MIME::Types, maybe not valid content-type\? Original Error:/)
+ end
+ end
+ end
end
23 spec/processing/mini_magick_spec.rb
View
@@ -74,4 +74,27 @@
end
end
+ describe "test errors" do
+ context "invalid image file" do
+ before do
+ File.open(@instance.current_path, 'w') do |f|
+ f.puts "bogus"
+ end
+ end
+
+ it "should fail to process a non image file" do
+ lambda {@instance.resize_to_limit(200, 200)}.should raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate with MiniMagick, maybe it is not an image\? Original Error:/)
+ end
+
+ it "should use I18n" do
+ change_locale_and_store_translations(:nl, :errors => {
+ :messages => {
+ :mini_magick_processing_error => "Kon bestand niet met MiniMagick bewerken, misschien is het geen beeld bestand? MiniMagick foutmelding: %{e}"
+ }
+ }) do
+ lambda {@instance.resize_to_limit(200, 200)}.should raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet met MiniMagick bewerken, misschien is het geen beeld bestand\? MiniMagick foutmelding:/)
+ end
+ end
+ end
+ end
end
23 spec/processing/rmagick_spec.rb
View
@@ -76,5 +76,28 @@
end
end
+ describe "test errors" do
+ context "invalid image file" do
+ before do
+ File.open(@instance.current_path, 'w') do |f|
+ f.puts "bogus"
+ end
+ end
+
+ it "should fail to process a non image file" do
+ lambda {@instance.resize_to_limit(200, 200)}.should raise_exception(CarrierWave::ProcessingError, /^Failed to manipulate with rmagick, maybe it is not an image\? Original Error:/)
+ end
+
+ it "should use I18n" do
+ change_locale_and_store_translations(:nl, :errors => {
+ :messages => {
+ :rmagick_processing_error => "Kon bestand niet met rmagick bewerken, misschien is het geen beeld bestand? rmagick foutmelding: %{e}"
+ }
+ }) do
+ lambda {@instance.resize_to_limit(200, 200)}.should raise_exception(CarrierWave::ProcessingError, /^Kon bestand niet met rmagick bewerken, misschien is het geen beeld bestand\? rmagick foutmelding:/)
+ end
+ end
+ end
+ end
end
end
Something went wrong with that request. Please try again.