Browse files

Fix thumbnail generation of PDF publications.

We had a problem where thumbnails weren't created correctly if the PDF
was retrieved from the file cache. The problem occurred when creating a
publication with an attachment where the attachment failed validation.

After lots of digging we've determined that by overriding
`#full_original_filename` (as well as `#full_filename`) we can get
CarrierWave to do the right thing (i.e correctly generate our thumbnail
when the attachment uses the file cache). We've added two tests that
demonstrate the broken behaviour without the implementation of

We've also fixed a secondary problem whereby the previous implementation
of `#get_first_page_as_png` caused two thumbnail_ files to be generated
- the first was the version that CarrierWave creates by default when any
version is declared, and the second was the from our use of the
`convert` command.
  • Loading branch information...
chrisroos-and-lazyatom committed Feb 1, 2012
1 parent d5fcd2e commit 114990faa5cf8d722b3f11701ffeed68ef5e861b
Showing with 20 additions and 5 deletions.
  1. +4 −4 app/uploaders/attachment_uploader.rb
  2. +10 −0 test/unit/attachment_test.rb
  3. +6 −1 test/unit/attachment_uploader_test.rb
@@ -14,6 +14,9 @@ class AttachmentUploader < CarrierWave::Uploader::Base
def full_filename(for_file)
super + ".png"
+ def full_original_filename
+ super + ".png"
+ end
process :generate_thumbnail
@@ -30,10 +33,7 @@ def pdf?(file)
def get_first_page_as_png(width, height)
- thumbnail_path = current_path + ".png"
- cmd = %{convert -resize #{width}x#{height} "#{path}[0]" "#{thumbnail_path}"}
- `#{cmd}`
- @file =
+ `convert -resize #{width}x#{height} "#{path}[0]" "#{path}"`
def extension_white_list
@@ -115,6 +115,16 @@ class AttachmentTest < ActiveSupport::TestCase
assert attachment.url(:thumbnail).ends_with?("thumbnail_greenpaper.pdf.png"), "unexpected url ending: #{attachment.url(:thumbnail)}"
+ test "should successfully create PNG thumbnail from the file_cache after a validation failure" do
+ greenpaper_pdf = fixture_file_upload('greenpaper.pdf', 'application/pdf')
+ attachment = build(:attachment, title: nil, file: greenpaper_pdf)
+ refute attachment.valid?
+ second_attempt_attachment = build(:attachment, title: "title", file: nil, file_cache: attachment.file_cache)
+ assert
+ type = `file -b --mime-type "#{second_attempt_attachment.file.thumbnail.path}"`
+ assert_equal "image/png", type.strip
+ end
test "should return nil file extension when no uploader present" do
attachment = build(:attachment)
attachment.stubs(file: nil)
@@ -46,10 +46,15 @@ class AttachmentUploaderPDFTest < ActiveSupport::TestCase
assert_respond_to @uploader, :thumbnail
- test "should store the thumbnail as a PNG" do
+ test "should store the thumbnail with the PNG extension" do
assert @uploader.thumbnail.path.ends_with?(".png"), "should be a png"
+ test "should store an actual PNG" do
+ type = `file -b --mime-type "#{@uploader.thumbnail.path}"`
+ assert_equal "image/png", type.strip
+ end
test "should scale the thumbnail down proportionally to A4" do
identify_details = `identify "#{Rails.root.join("public", @uploader.thumbnail.path)}"`
path, type, geometry, rest = identify_details.split

0 comments on commit 114990f

Please sign in to comment.