Skip to content

Result#generate_print_pdf refactor #7964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
danielrafailov1 wants to merge 5 commits into
base: master
Choose a base branch
from
+139 −143
Open

Result#generate_print_pdf refactor #7964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd31a24
Refactored Result#generate_print_pdf to use Dir.mktmpdir instead of F…
danielrafailov1 May 24, 2026
6a3e4a7
removed commented out lines
danielrafailov1 May 24, 2026
c07e858
altered Dir.mktmpdir call in Result#generate_print_pdf to use the sys…
danielrafailov1 May 25, 2026
a14c96e
added changes to the Changelog file
danielrafailov1 May 26, 2026
59f6143
Merge branch 'MarkUsProject:master' into result-generate-print-pdf-re…
danielrafailov1 May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,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)
Copy link
Copy Markdown
Collaborator

@david-yz-liu david-yz-liu May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, use monospace code font for method names (and other names which are from code)

- 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)
Expand Down
281 changes: 138 additions & 143 deletions app/models/result.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,176 +252,171 @@ 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
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}") 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)

Copy link
Copy Markdown
Collaborator

@david-yz-liu david-yz-liu May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this blank line

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

Copy link
Copy Markdown
Collaborator

@david-yz-liu david-yz-liu May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep one blank line here

Copy link
Copy Markdown
Contributor Author

@danielrafailov1 danielrafailov1 May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, do you want me to insert a blank line after line 418 (combined_pdf) or before line 418?

Copy link
Copy Markdown
Collaborator

@david-yz-liu david-yz-liu May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff is showing that in the version on master, Line 418 was a blank line. This immediately followed the end on Line 417.

In the changed version, the end is now on Line 419. You should insert a blank line after this end.

The diff is indicating that you deleted this line, perhaps without realizing it; so my comment here falls under the category of "revert changes that are unnecessary".

# 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.
Expand Down
Loading