feat: name downloaded submission files after upload requirements

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')

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'

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(?<index>\d{3})-(?<type>document|code|image|zip|archive)(?<extension>\..+)\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?

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

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,
@@ -2505,7 +2512,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 +2533,21 @@ def get_task_submissions_zip(current_user, td, progress_callback: nil)
       entries[path_part] = task
     end
 
-    if File.exist?(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? && 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)
+                    end
+
+    if cache_current
       progress_callback&.call(
         message: "Initialising submission files download",
         total_rows: submissions.count,
@@ -2554,7 +2575,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)
@@ -2569,6 +2590,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

test/models/task_submission_download_test.rb

@@ -0,0 +1,94 @@
+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"
+    ]
+
+    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
+    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)
+    Zip::File.open(path) do |zip|
+      assert_equal expected.sort, zip.entries.reject(&:name_is_directory?).map(&:name).sort
+    end
+  end
+end