CommandNotFoundError is rescued and added to model errors

[adamniedzielski]

I spent some time debugging when I pushed the code to my CI server and one spec was failing. The spec was a Capybara feature which was submitting form with attachment and checking if the model got saved. Finally I found the error among validation errors - it was CommandNotFoundError - "Could not run the identify command. Please install ImageMagick."

This was quite surprising, because it is a missing dependency and certainly not validation error. I cannot imagine a case when I want to present it to the end-user.

Looking through the code I found the reason of this behaviour:

https://github.com/thoughtbot/paperclip/blob/adddf87cf30d416d71c7b2a80a004d170d10dc2c/lib/paperclip/attachment.rb#L434-437

In this case I would expect that the exception is not rescued at all because it is a matter of overlooked dependency and should be clearly visible. Visible to the dev, not the user.

What do you think?

[maclover7]

Hi @adamniedzielski ! Is this still an issue for you in Paperclip; I know this issue is from approximately 2 years ago. If it is still an issue, can you please provide the code that's causing you the error? Thanks!

[adamniedzielski]

Looking at the code I can tell that nothing has changed here:

https://github.com/thoughtbot/paperclip/blob/cf60549e4354ce0d41fc7bea093a41ce24574021/lib/paperclip/attachment.rb#L531-533

[maclover7]

How would you like to see errors resolved - feel free to send in a PR with your approach to solve the problem.

[adamniedzielski]

After looking at exceptions inheriting from Paperclip::Error, I think that there is only one which should be rescued and go to the end user - NotIdentifiedByImageMagickError. All others are programmer's errors and should not be rescued. What do you think about it?

[maclover7]

Sounds fine to me. Would you be willing to send in a PR?