From cd31a24308f8450b3603e85df1e9f0a32c786609 Mon Sep 17 00:00:00 2001 From: Daniel Rafailov Date: Sat, 23 May 2026 21:20:52 -0400 Subject: [PATCH 1/4] Refactored Result#generate_print_pdf to use Dir.mktmpdir instead of Fileutils.mkdir_p --- app/models/result.rb | 281 +++++++++++++++++++++---------------------- 1 file changed, 140 insertions(+), 141 deletions(-) diff --git a/app/models/result.rb b/app/models/result.rb index 4466537e2a..932ad711aa 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -253,175 +253,174 @@ def generate_print_pdf assignment = grouping.assignment # Make folder for temporary files - workdir = "tmp/print/#{self.id}" - FileUtils.mkdir_p(workdir) + # workdir = "tmp/print/#{self.id}" + # FileUtils.mkdir_p(workdir) # Constants used for PDF generation logo_width = 80 line_space = 12 annotation_size = 20 - # Generate front page - Prawn::Document.generate("#{workdir}/front.pdf") do - # Add MarkUs logo - image Rails.root.join('app/assets/images/markus_logo_big.png'), - at: [bounds.width - logo_width, bounds.height], - width: logo_width - - font_families.update( - 'Open Sans' => { - normal: Rails.root.join('vendor/assets/stylesheets/fonts/OpenSansEmoji.ttf'), - bold: Rails.root.join('vendor/assets/stylesheets/fonts/OpenSans-Bold.ttf') - } - ) - font 'Open Sans' - - # Title - formatted_text [{ - text: "#{assignment.short_identifier}: #{assignment.description}", size: 20, styles: [:bold] - }] - move_down line_space - - # Group members - grouping.accepted_students.includes(:user).find_each do |student| - text "#{student.user_name} - #{student.first_name} #{student.last_name}" - end - move_down line_space - - # Marks - assignment.ta_criteria.order(:position).find_each do |criterion| - mark = marks.dig(criterion.id, :mark) - if criterion.is_a? RubricCriterion - formatted_text [{ text: "#{criterion.name}:", styles: [:bold] }] - indent(10) do - criterion.levels.order(:mark).find_each do |level| - styles = level.mark == mark ? [:bold] : [:normal] - formatted_text [{ - text: "• #{level.mark} / #{criterion.max_mark} #{level.name}: #{level.description}", - styles: styles - }] + Dir.mktmpdir("print/#{self.id}", 'tmp') do |workdir| + # Generate front page + Prawn::Document.generate("#{workdir}/front.pdf") do + # Add MarkUs logo + image Rails.root.join('app/assets/images/markus_logo_big.png'), + at: [bounds.width - logo_width, bounds.height], + width: logo_width + + font_families.update( + 'Open Sans' => { + normal: Rails.root.join('vendor/assets/stylesheets/fonts/OpenSansEmoji.ttf'), + bold: Rails.root.join('vendor/assets/stylesheets/fonts/OpenSans-Bold.ttf') + } + ) + font 'Open Sans' + + # Title + formatted_text [{ + text: "#{assignment.short_identifier}: #{assignment.description}", size: 20, styles: [:bold] + }] + move_down line_space + + # Group members + grouping.accepted_students.includes(:user).find_each do |student| + text "#{student.user_name} - #{student.first_name} #{student.last_name}" + end + move_down line_space + + # Marks + assignment.ta_criteria.order(:position).find_each do |criterion| + mark = marks.dig(criterion.id, :mark) + if criterion.is_a? RubricCriterion + formatted_text [{ text: "#{criterion.name}:", styles: [:bold] }] + indent(10) do + criterion.levels.order(:mark).find_each do |level| + styles = level.mark == mark ? [:bold] : [:normal] + formatted_text [{ + text: "• #{level.mark} / #{criterion.max_mark} #{level.name}: #{level.description}", + styles: styles + }] + end end + else + formatted_text [{ + text: "#{criterion.name}: #{mark || '-'} / #{criterion.max_mark}", + styles: [:bold] + }] + text criterion.description if criterion.description.present? end - else - formatted_text [{ - text: "#{criterion.name}: #{mark || '-'} / #{criterion.max_mark}", - styles: [:bold] - }] - text criterion.description if criterion.description.present? end - end - extra_marks.each do |extra_mark| - text "#{extra_mark.description}: #{extra_mark.extra_mark}#{extra_mark.unit == 'percentage' ? '%' : ''}" - end - move_down line_space + extra_marks.each do |extra_mark| + text "#{extra_mark.description}: #{extra_mark.extra_mark}#{extra_mark.unit == 'percentage' ? '%' : ''}" + end + move_down line_space - formatted_text [{ text: "#{I18n.t('results.total_mark')}: #{total_mark} / #{assignment.max_mark}", - styles: [:bold] }] - move_down line_space + formatted_text [{ text: "#{I18n.t('results.total_mark')}: #{total_mark} / #{assignment.max_mark}", + styles: [:bold] }] + move_down line_space - # Annotations and overall comments - formatted_text [{ text: Annotation.model_name.human.pluralize, styles: [:bold] }] - submission.annotations.order(:annotation_number).includes(:annotation_text).each do |annotation| - text "#{annotation.annotation_number}. #{annotation.annotation_text.content}" - end - move_down line_space + # Annotations and overall comments + formatted_text [{ text: Annotation.model_name.human.pluralize, styles: [:bold] }] + submission.annotations.order(:annotation_number).includes(:annotation_text).each do |annotation| + text "#{annotation.annotation_number}. #{annotation.annotation_text.content}" + end + move_down line_space - formatted_text [{ text: Result.human_attribute_name(:overall_comment), styles: [:bold] }] - if overall_comment.present? - text overall_comment - else - text I18n.t(:not_applicable) + formatted_text [{ text: Result.human_attribute_name(:overall_comment), styles: [:bold] }] + if overall_comment.present? + text overall_comment + else + text I18n.t(:not_applicable) + end end - end - # Copy all PDF submission files to workspace - input_files = submission.submission_files.where("filename LIKE '%.pdf'").order(:path, :filename) - grouping.access_repo do |repo| - input_files.each do |sf| - contents = sf.retrieve_file(repo: repo) - FileUtils.mkdir_p(File.join(workdir, sf.path)) - File.binwrite(File.join(workdir, sf.path, sf.filename), contents) + # Copy all PDF submission files to workspace + input_files = submission.submission_files.where("filename LIKE '%.pdf'").order(:path, :filename) + grouping.access_repo do |repo| + input_files.each do |sf| + contents = sf.retrieve_file(repo: repo) + FileUtils.mkdir_p(File.join(workdir, sf.path)) + File.binwrite(File.join(workdir, sf.path, sf.filename), contents) + end end - end - combined_pdf = CombinePDF.new - # Simultaneouly do two things: - # 1. Generate combined_pdf, a concatenation of all PDF submission files - # 2. Generate annotations.pdf, a PDF containing only markers for annotations. - # These will be overlaid onto combined_pdf. - Prawn::Document.generate("#{workdir}/annotations.pdf", skip_page_creation: true) do - total_num_pages = 0 - input_files.each do |input_file| - # Process the submission file - input_pdf = CombinePDF.load(File.join(workdir, input_file.path, input_file.filename)) - combined_pdf << input_pdf - - num_pages = input_pdf.pages.size - num_pages.times do - start_new_page - end + combined_pdf = CombinePDF.new + # Simultaneouly do two things: + # 1. Generate combined_pdf, a concatenation of all PDF submission files + # 2. Generate annotations.pdf, a PDF containing only markers for annotations. + # These will be overlaid onto combined_pdf. + Prawn::Document.generate("#{workdir}/annotations.pdf", skip_page_creation: true) do + total_num_pages = 0 + input_files.each do |input_file| + # Process the submission file + input_pdf = CombinePDF.load(File.join(workdir, input_file.path, input_file.filename)) + combined_pdf << input_pdf - # Create markers for the annotations. - # TODO: remove where clause after investigating how PDF annotations might have a nil page attribute - input_file.annotations.where.not(page: nil).order(:annotation_number).each do |annotation| - go_to_page(total_num_pages + annotation.page) - width, height = bounds.width, bounds.height - x1, y1 = annotation.x1 / 1.0e5 * width, annotation.y1 / 1.0e5 * height - - float do - transparent(0.5) do - fill_color 'AAAAAA' - fill_rectangle([x1, height - y1], annotation_size, annotation_size) - end + num_pages = input_pdf.pages.size + num_pages.times do + start_new_page + end - bounding_box([x1, height - y1], width: annotation_size, height: annotation_size) do - move_down 5 - text annotation.annotation_number.to_s, color: '000000', align: :center + # Create markers for the annotations. + # TODO: remove where clause after investigating how PDF annotations might have a nil page attribute + input_file.annotations.where.not(page: nil).order(:annotation_number).each do |annotation| + go_to_page(total_num_pages + annotation.page) + width, height = bounds.width, bounds.height + x1, y1 = annotation.x1 / 1.0e5 * width, annotation.y1 / 1.0e5 * height + + float do + transparent(0.5) do + fill_color 'AAAAAA' + fill_rectangle([x1, height - y1], annotation_size, annotation_size) + end + + bounding_box([x1, height - y1], width: annotation_size, height: annotation_size) do + move_down 5 + text annotation.annotation_number.to_s, color: '000000', align: :center + end end end - end - total_num_pages += num_pages + total_num_pages += num_pages + end end - end - # Combine annotations and submission files - annotations_pdf = CombinePDF.load("#{workdir}/annotations.pdf") - combined_pdf.pages.zip(annotations_pdf.pages) do |combined_page, annotation_page| - combined_page.fix_rotation # Fix rotation metadata, useful for scanned pages - combined_page << annotation_page - end + # Combine annotations and submission files + annotations_pdf = CombinePDF.load("#{workdir}/annotations.pdf") + combined_pdf.pages.zip(annotations_pdf.pages) do |combined_page, annotation_page| + combined_page.fix_rotation # Fix rotation metadata, useful for scanned pages + combined_page << annotation_page + end - input_files = submission.submission_files.where("filename LIKE '%.ipynb'").order(:path, :filename) - grouping.access_repo do |repo| - input_files.each do |sf| - contents = sf.retrieve_file(repo: repo) - tmp_path = File.join(workdir, 'tmp_file.pdf') - FileUtils.rm_rf(tmp_path) - args = [ - Rails.application.config.python, - '-m', 'nbconvert', - '--to', 'webpdf', - '--stdin', - '--output', File.join(workdir, File.basename(tmp_path.to_s, '.pdf')) # Can't include the .pdf extension - ] - _stdout, stderr, status = Open3.capture3(*args, stdin_data: contents) - if status.success? - input_pdf = CombinePDF.load(tmp_path) - combined_pdf << input_pdf - else - raise stderr + input_files = submission.submission_files.where("filename LIKE '%.ipynb'").order(:path, :filename) + grouping.access_repo do |repo| + input_files.each do |sf| + contents = sf.retrieve_file(repo: repo) + tmp_path = File.join(workdir, 'tmp_file.pdf') + FileUtils.rm_rf(tmp_path) + + args = [ + Rails.application.config.python, + '-m', 'nbconvert', + '--to', 'webpdf', + '--stdin', + '--output', File.join(workdir, File.basename(tmp_path, '.pdf')) # Can't include the .pdf extension + ] + _stdout, stderr, status = Open3.capture3(*args, stdin_data: contents) + if status.success? + input_pdf = CombinePDF.load(tmp_path) + combined_pdf << input_pdf + else + raise stderr + end end end + # Finally, insert cover page at the front + combined_pdf >> CombinePDF.load("#{workdir}/front.pdf") + combined_pdf end - - # Finally, insert cover page at the front - combined_pdf >> CombinePDF.load("#{workdir}/front.pdf") - - # Delete old files - FileUtils.rm_rf(workdir) - combined_pdf end # Generate a filename to be used for the printed PDF. From 6a3e4a73bb5280ee4099e5153d55ecf0c87a6cce Mon Sep 17 00:00:00 2001 From: Daniel Rafailov Date: Sun, 24 May 2026 12:43:57 -0400 Subject: [PATCH 2/4] removed commented out lines --- app/models/result.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/result.rb b/app/models/result.rb index 932ad711aa..eb2720a065 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -252,10 +252,6 @@ def generate_print_pdf grouping = submission.grouping assignment = grouping.assignment - # Make folder for temporary files - # workdir = "tmp/print/#{self.id}" - # FileUtils.mkdir_p(workdir) - # Constants used for PDF generation logo_width = 80 line_space = 12 From c07e858ddb8995efed5c1f078bc3767f013d8405 Mon Sep 17 00:00:00 2001 From: Daniel Rafailov Date: Mon, 25 May 2026 19:57:41 -0400 Subject: [PATCH 3/4] altered Dir.mktmpdir call in Result#generate_print_pdf to use the system tmp directory instead of the project tmp directory --- app/models/result.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/result.rb b/app/models/result.rb index eb2720a065..a446ebd143 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -257,7 +257,7 @@ def generate_print_pdf line_space = 12 annotation_size = 20 - Dir.mktmpdir("print/#{self.id}", 'tmp') do |workdir| + Dir.mktmpdir("print-#{self.id}") do |workdir| # Generate front page Prawn::Document.generate("#{workdir}/front.pdf") do # Add MarkUs logo From a14c96e8cd001d70b84c63c095f6b9202cb994df Mon Sep 17 00:00:00 2001 From: Daniel Rafailov Date: Mon, 25 May 2026 20:45:37 -0400 Subject: [PATCH 4/4] added changes to the Changelog file --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index fae105630c..6496d5ab0f 100644 --- a/Changelog.md +++ b/Changelog.md @@ -29,6 +29,7 @@ - Fixed `(hidden)` assignment labeling for assignments with `visible_on` and/or `visible_until` set (#7944) ### 🔧 Internal changes +- Refactored `Result#generate_print_pdf` to use Dir.mktmpdir instead of Fileutils.mkdir_p (#7964) - Added tests for `MarksGradersController` to achieve full test coverage for `randomly_assign` (#7947) - Refactored `AuthenticationHelper#sign_in` to set session values directly instead of going through `MainController#login` (#7962) - Updated `MainController` specs to dispatch `post :login` directly in tests that assert on login's response, instead of relying on `sign_in`'s internal request (#7962)