Skip to content

PDF conversion patch + Collect a submission patch [corrected] #732

Merged
merged 21 commits into from Apr 1, 2012

4 participants

@nicolasbouillon

New conversion process:

Use of the RubyGem RGhost instead of ImageMagick
Competitive conversion time
Output : a collection of jpg images, one image for each pdf page
(previous process gave only one image for the entire pdf file, leading to limiting the number of pages).
Possibility to grade a pdf file with a great number of pages.

Collect a submission :

Due to Rails 3.0 migration, the collecting of a submissions gave a routing issue.
Adding the required lines in routes.rb resolves the problem.

@jerboaa jerboaa commented on an outdated diff Mar 22, 2012
app/models/submission_file.rb
end
+ # Convert a pdf file into a an array of jpg files (one jpg file = one page of the pdf file)
+ RGhost::Convert.new(File.join(storage_path, self.filename)).to :jpeg, :filename => file_path, :multipage => true, :resolution => 150
@jerboaa
MarkUs member
jerboaa added a note Mar 22, 2012

It would be good to do error checking here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jerboaa jerboaa commented on the diff Mar 22, 2012
config/routes.rb
@@ -129,8 +129,12 @@
resources :results do
collection do
- post 'update_mark'
- post 'expand_criteria'
+ get 'update_mark'
+ get 'expand_criteria'
+ get 'collapse_criteria'
+ get 'expand_unmarked_criteria'
+ get 'edit'
+ get 'download'
@jerboaa
MarkUs member
jerboaa added a note Mar 22, 2012

It would be good to have tests for routes we depend on. Could add tests for this? assert_recognizes should do this for you. There should be some existing tests around using it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jerboaa
MarkUs member
jerboaa commented Mar 22, 2012

@nicolasbouillon Nice work! Thanks. Fix the two concerns and this should be good to go from my point of view.

@benjaminvialle
MarkUs member

I tested the patch and the result is really impressive ! Good work.

I still noticed 8 errors doing functionnal tests. They are on edit function in results_controller.

@nicolasbouillon

@benjaminvialle I don't understand : I have just run the functional tests again, and I get no error at all...

@benjaminvialle
MarkUs member

mmhh, did you push again your branch?

I'm going to checkout again your repository an re start tests.

@benjaminvialle
MarkUs member

Ok, I was not using the good branch. Shame on me!

All tests pass ! That's perfect :)

We will look at the two points Severin asked and this will be ready to be merged!

@NelleV
MarkUs member
NelleV commented Mar 30, 2012

I've pushed a patch.
Thanks,

@jerboaa jerboaa and 1 other commented on an outdated diff Mar 30, 2012
app/models/submission_file.rb
begin
- # This is an ImageMagick command, see http://www.imagemagick.org/script/convert.php for documentation
- # For some reason, ImageMagick seemed to fail silently when not
- # redirecting the output to a log file. Let's redirect the output to
- # "something"
- `convert -limit memory #{MarkusConfigurator.markus_config_pdf_conv_memory_allowance} -limit map 0 -density 150 -resize 66% #{File.join(storage_path, self.filename)} -append #{file_path} >> #{File.join(RAILS_ROOT, "log", "export-pdf.log")}`
- # Sometimes, ImageMagick fails silently
- if File.exists?(file_path)
- m_logger.log("Successfully converted pdf file to jpg")
- else
- m_logger.log("Problem in PDF conversion")
- end
+ RGhost::Convert.new(File.join(storage_path,
@jerboaa
MarkUs member
jerboaa added a note Mar 30, 2012

Does this always raise an exception? I'm asking because of this: https://github.com/shairontoledo/rghost/wiki/Converting-PDF-into-other-formats See the checking for errors section. Thoughts?

@NelleV
MarkUs member
NelleV added a note Mar 30, 2012

I've checked both methods by uninstalling ghostscripts (so the conversion doesn't work). None of the methods seems to work (the flag isn't set, and no exception is raised.). We could check manually that the pdf files aren't converted but that seems like a terrible hack to me.

@jerboaa
MarkUs member
jerboaa added a note Mar 31, 2012

Unless the RGhost doc is way out of date, there has to be a reason why they suggest to do error checking that way. I could see that it may report errors if the conversion failed due to other reasons such as out-of-memory or corrupted PDFs? I suggest to do the error checking as suggested by the RGhost documentation anyway. Feel free to keep the rescue block as well :) Otherwise this is good to go from my point of view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@NelleV
MarkUs member
NelleV commented Mar 31, 2012

I've modified the way we detect errors in PDF conversion, as RGhost doesn't raise errors, but uses a boolean flag.

@jerboaa
MarkUs member
jerboaa commented Mar 31, 2012

Looks good. Feel free to merge.

@benjaminvialle benjaminvialle merged commit dc2c0f1 into MarkUsProject:master Apr 1, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.