From a065b0e8af8879f38bc52ca2eb0f9aec52087817 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:37:19 +1100 Subject: [PATCH 1/3] feat: name downloaded submission files after upload requirements --- app/api/tasks_api.rb | 2 +- ...sion_download_authorizations_controller.rb | 2 +- app/models/task.rb | 52 +++++++++++++++++++ app/models/unit.rb | 13 +++-- test/models/task_submission_download_test.rb | 47 +++++++++++++++++ 5 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 test/models/task_submission_download_test.rb diff --git a/app/api/tasks_api.rb b/app/api/tasks_api.rb index b95b144cfe..bd5f0552e8 100644 --- a/app/api/tasks_api.rb +++ b/app/api/tasks_api.rb @@ -345,7 +345,7 @@ class TasksApi < Grape::API ) # Find the file - file_loc = FileHelper.zip_file_path_for_done_task(task) + file_loc = task.downloadable_submission_zip_path if file_loc.nil? || !File.exist?(file_loc) file_loc = Rails.root.join('public/resources/FileNotFound.pdf') diff --git a/app/controllers/submission_download_authorizations_controller.rb b/app/controllers/submission_download_authorizations_controller.rb index 36b2f8b014..2dc6b7510b 100644 --- a/app/controllers/submission_download_authorizations_controller.rb +++ b/app/controllers/submission_download_authorizations_controller.rb @@ -43,7 +43,7 @@ def locate_download(route) def submission_files_download(task, task_definition, project) { - path: FileHelper.zip_file_path_for_done_task(task), + path: task.downloadable_submission_zip_path, filename: FileHelper.sanitized_filename("#{project.student.username}-#{task_definition.abbreviation}.zip"), content_type: 'application/octet-stream', disposition: 'attachment' diff --git a/app/models/task.rb b/app/models/task.rb index a690571330..2747b9e097 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'date' +require 'securerandom' class Task < ApplicationRecord include ApplicationHelper @@ -1882,6 +1883,57 @@ def filename_for_upload(idx) "#{upload_requirements[idx]['name']}#{extension_for_upload(idx)}" end + # Keep the stored archive's numbered filenames for internal processing, but + # expose the upload requirement names when staff download its contents. + def download_filename_for(entry_name) + relative_path = entry_name.split('/', 2).last || entry_name + filename = File.basename(relative_path) + match = /\A(?\d{3})-(?document|code|image|zip|archive)(?\..+)\z/.match(filename) + return relative_path unless match + + requirement = upload_requirements[match[:index].to_i] + return relative_path unless requirement && requirement['type'] == match[:type] + + extension = match[:extension] + name = requirement['name'].to_s.strip + return relative_path if name.empty? + + name = name[0...-extension.length] if name.downcase.end_with?(extension.downcase) + renamed_file = "#{match[:index]}-#{FileHelper.sanitized_filename(name)}#{extension}" + directory = File.dirname(relative_path) + directory == '.' ? renamed_file : File.join(directory, renamed_file) + end + + def downloadable_submission_zip_path + source = zip_file_path_for_done_task + return source unless source && File.file?(source) + + result = FileHelper.tmp_file("submission-#{id}-named.zip") + if File.file?(result) && File.mtime(result) >= [File.mtime(source), task_definition.updated_at].max + return result + end + + partial_result = "#{result}.partial-#{SecureRandom.hex(8)}" + begin + Zip::OutputStream.open(partial_result) do |output_zip| + Zip::File.open(source) do |submission_zip| + submission_zip.each do |entry| + next if entry.name_is_directory? + + renamed_entry = entry.dup + renamed_entry.name = File.join(entry.name.split('/', 2).first, download_filename_for(entry.name)) + output_zip.copy_raw_entry(renamed_entry) + end + end + end + FileUtils.mv(partial_result, result) + ensure + FileUtils.rm_f(partial_result) + end + + result + end + def extension_for_upload(idx) filename = filename_in_zip(idx) return nil if filename.nil? diff --git a/app/models/unit.rb b/app/models/unit.rb index 67d9494a07..cab45bdac4 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2505,7 +2505,7 @@ def get_task_submissions_pdf_zip(current_user, td, progress_callback: nil) # Create a temp zip file with all submissions for a task # def task_submissions_zip_path(current_user, td) - FileHelper.tmp_file("submissions-#{code}-#{td.abbreviation}-#{current_user.username}-files.zip") + FileHelper.tmp_file("submissions-#{code}-#{td.abbreviation}-#{current_user.username}-named-files.zip") end def get_task_submissions_zip(current_user, td, progress_callback: nil) @@ -2526,7 +2526,14 @@ def get_task_submissions_zip(current_user, td, progress_callback: nil) entries[path_part] = task end - if File.exist?(result) + cache_current = File.file?(result) && + File.mtime(result) >= td.updated_at && + submissions.values.all? do |task| + source = task.zip_file_path_for_done_task + File.file?(source) && File.mtime(result) >= File.mtime(source) + end + + if cache_current progress_callback&.call( message: "Initialising submission files download", total_rows: submissions.count, @@ -2554,7 +2561,7 @@ def get_task_submissions_zip(current_user, td, progress_callback: nil) # Done archives store files below a task-id folder. The aggregate # archive replaces that folder with the student or group name. - relative_path = entry.name.split('/', 2).last || entry.name + relative_path = task.download_filename_for(entry.name) aggregate_entry = entry.dup aggregate_entry.name = File.join(zip_root_path, path_part, relative_path) output_zip.copy_raw_entry(aggregate_entry) diff --git a/test/models/task_submission_download_test.rb b/test/models/task_submission_download_test.rb new file mode 100644 index 0000000000..ba17392db3 --- /dev/null +++ b/test/models/task_submission_download_test.rb @@ -0,0 +1,47 @@ +require 'test_helper' + +class TaskSubmissionDownloadTest < ActiveSupport::TestCase + def test_individual_and_bulk_downloads_use_upload_requirement_names + project = FactoryBot.create(:project) + unit = project.unit + task_definition = unit.task_definitions.first + task_definition.update!(upload_requirements: [ + { 'key' => 'file0', 'name' => 'Design report', 'type' => 'document' }, + { 'key' => 'file1', 'name' => 'source.py', 'type' => 'code' } + ]) + task = project.task_for_task_definition(task_definition) + source = task.zip_file_path_for_done_task + FileUtils.mkdir_p(File.dirname(source)) + + Zip::File.open(source, Zip::File::CREATE) do |zip| + zip.get_output_stream("#{task.id}/000-document.pdf") { |stream| stream.write('report') } + zip.get_output_stream("#{task.id}/001-code.py") { |stream| stream.write('code') } + end + + individual = task.downloadable_submission_zip_path + assert_archive_entries individual, [ + "#{task.id}/000-Design_report.pdf", + "#{task.id}/001-source.py" + ] + + bulk = task_definition.stub(:related_tasks_with_files, [task]) do + unit.get_task_submissions_zip(unit.main_convenor_user, task_definition) + end + assert_archive_entries bulk, [ + "#{task_definition.abbreviation}-submissions/#{project.student.username}/000-Design_report.pdf", + "#{task_definition.abbreviation}-submissions/#{project.student.username}/001-source.py" + ] + ensure + FileUtils.rm_f(source) if source + FileUtils.rm_f(individual) if individual + FileUtils.rm_f(bulk) if bulk + end + + private + + def assert_archive_entries(path, expected) + Zip::File.open(path) do |zip| + assert_equal expected.sort, zip.entries.reject(&:name_is_directory?).map(&:name).sort + end + end +end From 7887181ff5d393693c5d7f9881e1f29d016212f7 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:38:34 +1100 Subject: [PATCH 2/3] fix: skip missing submissions --- app/models/unit.rb | 15 ++++++++++++++- test/models/task_submission_download_test.rb | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/app/models/unit.rb b/app/models/unit.rb index cab45bdac4..e2d849e1bf 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2526,7 +2526,15 @@ def get_task_submissions_zip(current_user, td, progress_callback: nil) entries[path_part] = task end - cache_current = File.file?(result) && + missing_submissions = [] + submissions.reject! do |path_part, task| + missing = !File.file?(task.zip_file_path_for_done_task) + missing_submissions << "#{path_part} (task #{task.id})" if missing + missing + end + logger.warn "Skipping missing submission archives: #{missing_submissions.join(', ')}" if missing_submissions.any? + + cache_current = missing_submissions.empty? && File.file?(result) && File.mtime(result) >= td.updated_at && submissions.values.all? do |task| source = task.zip_file_path_for_done_task @@ -2576,6 +2584,11 @@ def get_task_submissions_zip(current_user, td, progress_callback: nil) ) end end + + if missing_submissions.any? + output_zip.put_next_entry(File.join(zip_root_path, 'missing-submissions.txt')) + output_zip.write("Submission files were unavailable for:\n#{missing_submissions.join("\n")}\n") + end end # ZIP entries are copied in their existing compressed form. Closing only diff --git a/test/models/task_submission_download_test.rb b/test/models/task_submission_download_test.rb index ba17392db3..7874a3f7f3 100644 --- a/test/models/task_submission_download_test.rb +++ b/test/models/task_submission_download_test.rb @@ -31,6 +31,21 @@ def test_individual_and_bulk_downloads_use_upload_requirement_names "#{task_definition.abbreviation}-submissions/#{project.student.username}/000-Design_report.pdf", "#{task_definition.abbreviation}-submissions/#{project.student.username}/001-source.py" ] + + missing_project = FactoryBot.create(:project, unit: unit) + missing_task = missing_project.task_for_task_definition(task_definition) + bulk = task_definition.stub(:related_tasks_with_files, [task, missing_task]) do + unit.get_task_submissions_zip(unit.main_convenor_user, task_definition) + end + manifest = "#{task_definition.abbreviation}-submissions/missing-submissions.txt" + assert_archive_entries bulk, [ + "#{task_definition.abbreviation}-submissions/#{project.student.username}/000-Design_report.pdf", + "#{task_definition.abbreviation}-submissions/#{project.student.username}/001-source.py", + manifest + ] + Zip::File.open(bulk) do |zip| + assert_includes zip.read(manifest), missing_project.student.username + end ensure FileUtils.rm_f(source) if source FileUtils.rm_f(individual) if individual From 43e11e72f79af4c8a2aff88b2a54b3d32aa4aba2 Mon Sep 17 00:00:00 2001 From: b0ink <40929320+b0ink@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:41:52 +1100 Subject: [PATCH 3/3] fix: skip withdrawn projects --- app/models/task_definition.rb | 2 +- app/models/unit.rb | 12 ++++++-- test/models/task_submission_download_test.rb | 32 ++++++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/app/models/task_definition.rb b/app/models/task_definition.rb index 18f884c0c1..a4c90d3e9f 100644 --- a/app/models/task_definition.rb +++ b/app/models/task_definition.rb @@ -1056,7 +1056,7 @@ def jplag_report end def related_tasks_with_files(consolidate_groups = true) - tasks_with_files = tasks.select(&:has_pdf) + tasks_with_files = tasks.joins(:project).where(projects: { enrolled: true }).select(&:has_pdf) if is_group_task? && consolidate_groups # group task so only select one member of each group diff --git a/app/models/unit.rb b/app/models/unit.rb index e2d849e1bf..64a509c753 100644 --- a/app/models/unit.rb +++ b/app/models/unit.rb @@ -2442,12 +2442,19 @@ def task_submissions_pdf_zip_path(current_user, td) FileHelper.tmp_file("submissions-#{code}-#{td.abbreviation}-#{current_user.username}-pdfs.zip") end + def task_submissions_cache_current?(path, td) + return false unless File.file?(path) + + latest_project_update = td.tasks.joins(:project).maximum('projects.updated_at') + File.mtime(path) >= [td.updated_at, latest_project_update].compact.max + end + def get_task_submissions_pdf_zip(current_user, td, progress_callback: nil) # Get a temp file path result = task_submissions_pdf_zip_path(current_user, td) tasks_with_files = td.related_tasks_with_files - if File.exist?(result) + if task_submissions_cache_current?(result, td) progress_callback&.call( message: "Initialising submission pdfs download", total_rows: tasks_with_files.count, @@ -2534,8 +2541,7 @@ def get_task_submissions_zip(current_user, td, progress_callback: nil) end logger.warn "Skipping missing submission archives: #{missing_submissions.join(', ')}" if missing_submissions.any? - cache_current = missing_submissions.empty? && File.file?(result) && - File.mtime(result) >= td.updated_at && + cache_current = missing_submissions.empty? && task_submissions_cache_current?(result, td) && submissions.values.all? do |task| source = task.zip_file_path_for_done_task File.file?(source) && File.mtime(result) >= File.mtime(source) diff --git a/test/models/task_submission_download_test.rb b/test/models/task_submission_download_test.rb index 7874a3f7f3..2b95b925e4 100644 --- a/test/models/task_submission_download_test.rb +++ b/test/models/task_submission_download_test.rb @@ -52,6 +52,38 @@ def test_individual_and_bulk_downloads_use_upload_requirement_names FileUtils.rm_f(bulk) if bulk end + def test_bulk_downloads_exclude_projects_after_withdrawal + unit = FactoryBot.create(:unit, with_students: false, task_count: 1) + project = FactoryBot.create(:project, unit: unit) + task_definition = unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + pdf_path = task.final_pdf_path + source = task.zip_file_path_for_done_task + FileUtils.mkdir_p(File.dirname(pdf_path)) + FileUtils.mkdir_p(File.dirname(source)) + File.write(pdf_path, '%PDF-1.4 submission') + Zip::File.open(source, Zip::File::CREATE) do |zip| + zip.get_output_stream("#{task.id}/000-code.rb") { |stream| stream.write('code') } + end + + assert_includes task_definition.related_tasks_with_files.map(&:id), task.id + pdf_zip = unit.get_task_submissions_pdf_zip(unit.main_convenor_user, task_definition) + files_zip = unit.get_task_submissions_zip(unit.main_convenor_user, task_definition) + assert_archive_entries pdf_zip, ["#{task_definition.abbreviation}-pdfs/#{project.student.username}.pdf"] + assert_archive_entries files_zip, ["#{task_definition.abbreviation}-submissions/#{project.student.username}/000-Imported_Code.rb"] + + project.update!(enrolled: false) + + assert_not_includes task_definition.related_tasks_with_files.map(&:id), task.id + assert_archive_entries unit.get_task_submissions_pdf_zip(unit.main_convenor_user, task_definition), [] + assert_archive_entries unit.get_task_submissions_zip(unit.main_convenor_user, task_definition), [] + ensure + FileUtils.rm_f(pdf_path) if pdf_path + FileUtils.rm_f(source) if source + FileUtils.rm_f(pdf_zip) if pdf_zip + FileUtils.rm_f(files_zip) if files_zip + end + private def assert_archive_entries(path, expected)