feat: remove stored pdf path and allow move to archive

- refactor to allow different file roots
- remove need for portfolio evidence attribute, while still working with existing values

Author: macite

README.md

@@ -32,6 +32,7 @@ Doubtfire requires multiple environment variables that help define settings abou
 | `DF_AUTH_METHOD`               | The authentication method you would like Doubtfire to use. Possible values are `database` for standard authentication with the database, `ldap`                                                                                                                             | `database`                     |
 |                                |   for [LDAP](https://www.freebsd.org/doc/en/articles/ldap-auth/), `aaf` for [AAF Rapid Connect](https://rapid.aaf.edu.au/), or `SAML2` for [SAML2.0 auth](https://en.wikipedia.org/wiki/SAML_2.0).                                                                          |                                |
 | `DF_STUDENT_WORK_DIR`          | The directory to store uploaded student work for processing.                                                                                                                                                                                                                | `student_work`                 |
+| `DF_ARCHIVE_DIR`               | The directory to move archived unit files to, and access from.                                                                                                                                                                                                                | `DF_STUDENT_WORK_DIR/archive`     |
 | `DF_INSTITUTION_NAME`          | The name of your institution running Doubtfire.                                                                                                                                                                                                                             | _University of Foo_            |
 | `DF_INSTITUTION_EMAIL_DOMAIN`  | The email domain from which emails are sent to and from in your institution.                                                                                                                                                                                                | `doubtfire.com`                |
 | `DF_INSTITUTION_HOST`          | The host running the Doubtfire instance.                                                                                                                                                                                                                                    | `localhost:3000`               |

app/api/submission/portfolio_evidence_api.rb

@@ -76,8 +76,8 @@ def self.logger
       optional :as_attachment, type: Boolean, desc: 'Whether or not to download file as attachment. Default is false.'
     end
     get '/projects/:id/task_def_id/:task_definition_id/submission' do
-      project = Project.find(params[:id])
-      task_definition = project.unit.task_definitions.find(params[:task_definition_id])
+      project = Project.eager_load(:unit).find(params[:id])
+      task_definition = project.unit.task_definitions.select(:id, :name, :abbreviation).find(params[:task_definition_id])
 
       # check the user can put this task
       unless authorise? current_user, project, :get_submission
@@ -86,14 +86,12 @@ def self.logger
 
       task = project.task_for_task_definition(task_definition)
 
-      evidence_loc = task.portfolio_evidence_path
-      student = task.project.student
-      unit = task.project.unit
+      evidence_loc = task.final_pdf_path
 
       if task.processing_pdf?
         evidence_loc = Rails.root.join('public/resources/AwaitingProcessing.pdf')
         filename = 'AwaitingProcessing.pdf'
-      elsif evidence_loc.nil?
+      elsif evidence_loc.nil? || !File.exist?(evidence_loc)
         evidence_loc = Rails.root.join('public/resources/FileNotFound.pdf')
         filename = 'FileNotFound.pdf'
       else

app/helpers/file_helper.rb

@@ -125,9 +125,7 @@ def sanitized_filename(filename)
   end
 
   def task_file_dir_for_unit(unit, create = true)
-    file_server = Doubtfire::Application.config.student_work_dir
-    dst = "#{file_server}/" # trust the server config and passed in type for paths
-    dst << sanitized_path("#{unit.code}-#{unit.id}", 'TaskFiles') << '/'
+    dst = unit_work_root(unit) << 'TaskFiles/'
 
     FileUtils.mkdir_p dst if create && (!Dir.exist? dst)
 
@@ -176,6 +174,30 @@ def student_work_root
     Doubtfire::Application.config.student_work_dir
   end
 
+  def archive_root
+    Doubtfire::Application.config.archive_dir
+  end
+
+  # Get the path to the unit root - will take into consideration if archived
+  #
+  # @param [Unit] unit - the unit to get the root path for
+  # @param [Boolean] archived - whether to use the archived property (true/false)
+  #                             or force it to be the archived path (:force)
+  def unit_work_root(unit, archived: true)
+    dst = if (unit.archived && archived) || (archived == :force)
+            "#{archive_root}/"
+          else
+            "#{student_work_root}/"
+          end
+
+    dst << sanitized_path("#{unit.code}-#{unit.id}") << '/'
+  end
+
+  def project_work_root(project, archived: true, username: nil)
+    username = project.student.username.to_s if username.nil?
+    unit_work_root(project.unit, archived: archived) << sanitized_path(username) << '/'
+  end
+
   #
   # Generates a path for storing student work
   # type = [:new, :in_process, :done, :pdf, :plagarism]
@@ -187,18 +209,17 @@ def student_work_dir(type = nil, task = nil, create = true)
       file_server = Doubtfire::Application.config.student_work_dir
       dst = "#{file_server}/" # trust the server config and passed in type for paths
 
-      if !(type.nil? || task.nil?)
+      if !(type.nil? || task.nil?) # we have task and type
         if [:discussion, :pdf, :comment].include? type
-          dst << sanitized_path("#{task.project.unit.code}-#{task.project.unit.id}", task.project.student.username.to_s, type.to_s) << '/'
+          dst = project_work_root(task.project) << sanitized_path(type.to_s) << '/'
         elsif [:done, :plagarism].include? type
-          dst << sanitized_path("#{task.project.unit.code}-#{task.project.unit.id}", task.project.student.username.to_s, type.to_s, task.id.to_s) << '/'
+          dst = project_work_root(task.project) << sanitized_path(type.to_s, task.id.to_s) << '/'
         else # new and in_process -- just have task id
           # Add task id to dst if we want task
           dst << "#{type}/#{task.id}/"
         end
-      elsif !type.nil?
+      elsif !type.nil? # have type but not task
         if [:in_process, :new].include? type
-          # Add task id to dst if we want task
           dst << "#{type}/"
         else
           raise 'Error in request to student work directory'
@@ -211,27 +232,40 @@ def student_work_dir(type = nil, task = nil, create = true)
     dst
   end
 
-  def dir_for_unit_code_and_id(unit_code, unit_id, create = true)
-    file_server = Doubtfire::Application.config.student_work_dir
-    dst = "#{file_server}/" # trust the server config and passed in type for paths
+  def dir_for_unit_code_and_id(unit_code, unit_id, create: true, archived: false)
+    dst = if archived
+            "#{archive_root}/"
+          else
+            "#{student_work_root}/"
+          end
+
     dst << sanitized_path("#{unit_code}-#{unit_id}")
 
     FileUtils.mkdir_p dst if create && !Dir.exist?(dst)
 
     dst
   end
 
-  def unit_dir(unit, create = true)
-    dir_for_unit_code_and_id(unit.code, unit.id, create)
+  def unit_dir(unit, create: true, archived: true)
+    dir_for_unit_code_and_id(unit.code, unit.id, create: create, archived: archived == :force || (archived && unit.archived))
   end
 
-  def root_portfolio_dir
-    file_server = Doubtfire::Application.config.student_work_dir
+  def root_portfolio_dir(archived: false)
+    file_server = if archived
+                    archive_root
+                  else
+                    student_work_root
+                  end
+
     "#{file_server}/portfolio/" # trust the server config and passed in type for paths
   end
 
-  def unit_portfolio_dir(unit, create = true)
-    dst = root_portfolio_dir
+  def unit_portfolio_dir(unit, create: true, archived: true)
+    dst = if (unit.archived && archived) || (archived == :force)
+            "#{archive_root}/portfolio/"
+          else
+            "#{student_work_root}/portfolio/"
+          end
 
     dst << sanitized_path("#{unit.code}-#{unit.id}") << '/'
 
@@ -243,8 +277,8 @@ def unit_portfolio_dir(unit, create = true)
   #
   # Generates a path for storing student portfolios
   #
-  def student_portfolio_dir(unit, username, create = true)
-    dst = unit_portfolio_dir(unit, create)
+  def student_portfolio_dir(unit, username, create: true, archived: true)
+    dst = unit_portfolio_dir(unit, create: create, archived: archived)
 
     dst << sanitized_path(username.to_s)
 
@@ -253,8 +287,8 @@ def student_portfolio_dir(unit, username, create = true)
     dst
   end
 
-  def student_portfolio_path(unit, username, create = true)
-    File.join(student_portfolio_dir(unit, username, create), FileHelper.sanitized_filename("#{username}-portfolio.pdf"))
+  def student_portfolio_path(unit, username, create: true, archived: true)
+    File.join(student_portfolio_dir(unit, username, create: create, archived: archived), FileHelper.sanitized_filename("#{username}-portfolio.pdf"))
   end
 
   def comment_attachment_path(task_comment, attachment_extension)
@@ -679,10 +713,13 @@ def line_wrap(path, width: 160)
   module_function :student_group_work_dir
   module_function :student_work_dir
   module_function :student_work_root
+  module_function :archive_root
   module_function :dir_for_unit_code_and_id
   module_function :unit_dir
   module_function :root_portfolio_dir
   module_function :unit_portfolio_dir
+  module_function :unit_work_root
+  module_function :project_work_root
   module_function :student_portfolio_dir
   module_function :student_portfolio_path
   module_function :comment_attachment_path

app/mailers/d2l_result_mailer.rb

@@ -1,5 +1,5 @@
 class D2lResultMailer < ApplicationMailer
-  def result_message(unit, user, result_message = 'completed', success = true)
+  def result_message(unit, user, result_message: 'completed', success: true)
     email = user.email
     return nil if email.blank?
 

app/models/group_submission.rb

@@ -18,10 +18,9 @@ class GroupSubmission < ApplicationRecord
       FileHelper.delete_group_submission(group_submission)
 
       # also remove evidence from group members
-      tasks.each do |t|
-        t.portfolio_evidence_path = nil
-        t.save
-      end
+      # rubocop:disable Rails/SkipsModelValidations
+      tasks.where('portfolio_evidence IS NOT NULL').update_all(portfolio_evidence: nil)
+      # rubocop:enable Rails/SkipsModelValidations
     rescue => e
       logger.error "Failed to delete group submission #{group_submission.id}. Error: #{e.message}"
     end

app/models/pdf_generation/project_compile_portfolio_module.rb

@@ -177,8 +177,8 @@ def learning_summary_report_exists?
     # Portfolio production code
     #
     def portfolio_temp_path
-      portfolio_dir = FileHelper.student_portfolio_dir(self.unit, self.student.username, false)
-      portfolio_tmp_dir = File.join(portfolio_dir, 'tmp')
+      portfolio_dir = FileHelper.student_portfolio_dir(self.unit, self.student.username, create: false)
+      File.join(portfolio_dir, 'tmp')
     end
 
     def portfolio_tmp_file_name(dict)
@@ -270,7 +270,7 @@ def remove_portfolio_file(idx, kind, name)
     end
 
     def portfolio_path
-      FileHelper.student_portfolio_path(self.unit, self.student.username, true)
+      FileHelper.student_portfolio_path(self.unit, self.student.username, create: true)
     end
 
     def portfolio_exists?

app/models/task.rb

@@ -431,7 +431,7 @@ def status
   end
 
   def has_pdf
-    !portfolio_evidence_path.nil? && File.exist?(portfolio_evidence_path) && !processing_pdf?
+    !final_pdf_path.nil? && File.exist?(final_pdf_path) && !processing_pdf?
   end
 
   def log_details
@@ -1003,6 +1003,17 @@ def move_files_to_in_process(source_folder = FileHelper.student_work_dir(:new))
     end
   end
 
+  def move_files_on_abbreviation_change(old_abbreviation)
+    # Move files from old abbreviation to new abbreviation
+    old_path = final_pdf_path(abbr: old_abbreviation)
+    new_path = final_pdf_path(ignore_portfolio_evidence: true)
+
+    return if old_path == new_path || !File.exist?(old_path)
+
+    FileUtils.mv(old_path, new_path)
+    update(portfolio_evidence: nil) unless portfolio_evidence.nil?
+  end
+
   def __output_filename__(in_dir, idx, type)
     pwd = FileUtils.pwd
     Dir.chdir(in_dir)
@@ -1117,28 +1128,56 @@ def self.pygments_lang(extn)
     end
   end
 
-  def portfolio_evidence_path
-    # Add the student work dir to the start of the portfolio evidence
-    File.join(FileHelper.student_work_dir, self.portfolio_evidence) if self.portfolio_evidence.present?
+  def move_to_final_pdf_path
+    if portfolio_evidence.present?
+      # Move the portfolio evidence to the final pdf path
+      if File.exist?(portfolio_evidence_path)
+        new_path = final_pdf_path(ignore_portfolio_evidence: true)
+        FileUtils.mv(portfolio_evidence_path, new_path)
+      end
+      update(portfolio_evidence: nil)
+    end
   end
 
-  def portfolio_evidence_path=(value)
-    # Strip the student work directory to store in database as relative path
-    self.portfolio_evidence = value.present? ? value.sub(FileHelper.student_work_dir, '') : nil
+  def portfolio_evidence_path
+    # Add the student work dir to the start of the portfolio evidence
+    if unit.archived
+      base = FileHelper.archive_root
+    else
+      base = FileHelper.student_work_dir
+    end
+    File.join(base, self.portfolio_evidence) if self.portfolio_evidence.present?
   end
 
   # The path to the PDF for this task's submission
-  def final_pdf_path
-    if group_task?
-      return nil if group_submission.nil? || group_submission.task_definition.nil?
-
-      File.join(
-        FileHelper.student_group_work_dir(:pdf, group_submission, task = nil, create = true),
-        FileHelper.sanitized_filename(FileHelper.sanitized_path("#{group_submission.task_definition.abbreviation}-#{group_submission.id}") + '.pdf')
-      )
-    else
-      File.join(student_work_dir(:pdf), FileHelper.sanitized_filename(FileHelper.sanitized_path("#{task_definition.abbreviation}-#{id}") + '.pdf'))
+  def final_pdf_path(abbr: nil, ignore_portfolio_evidence: false)
+    result = if group_task?
+               return nil if group_submission.nil? || group_submission.task_definition.nil?
+
+               abbr = group_submission.task_definition.abbreviation if abbr.nil?
+
+               File.join(
+                 FileHelper.student_group_work_dir(:pdf, group_submission, task = nil, create = true),
+                 FileHelper.sanitized_filename(FileHelper.sanitized_path("#{abbr}-#{group_submission.id}") + '.pdf')
+               )
+             else
+               abbr = task_definition.abbreviation if abbr.nil?
+               File.join(student_work_dir(:pdf), FileHelper.sanitized_filename(FileHelper.sanitized_path("#{abbr}-#{id}") + '.pdf'))
+             end
+
+    # see if we need to use the portfolio evidence
+    if portfolio_evidence.present? && !ignore_portfolio_evidence
+      evidence_loc = portfolio_evidence_path
+
+      # Remove portfolio evidence if possible
+      if evidence_loc == result || !File.exist?(evidence_loc)
+        update(portfolio_evidence: nil)
+      else
+        result = evidence_loc
+      end
     end
+
+    result
   end
 
   # A custom error to capture the log message from the latex error
@@ -1202,31 +1241,20 @@ def convert_submission_to_pdf(source_folder: FileHelper.student_work_dir(:new),
         end
       end
 
-      # save the final pdf path to portfolio evidence - relative to student work folder
-      if group_task?
-        group_submission.tasks.each do |t|
-          t.portfolio_evidence_path = final_pdf_path
-          t.save
-        end
-        reload
-      else
-        self.portfolio_evidence_path = final_pdf_path
-      end
-
       # Save the file... now using the full path!
-      File.open(portfolio_evidence_path, 'w') do |fout|
+      File.open(final_pdf_path, 'w') do |fout|
         fout.puts pdf_text
       end
 
-      FileHelper.compress_pdf(portfolio_evidence_path)
+      FileHelper.compress_pdf(final_pdf_path)
 
       logger.info("PDF created for task #{self.id}")
 
       # if the task is the draft learning summary task
       if task_definition_id == unit.draft_task_definition_id
         # if there is a learning summary, execute, if there isn't and a learning summary exists, don't execute
         if project.uses_draft_learning_summary || !project.learning_summary_report_exists?
-          project.save_as_learning_summary_report portfolio_evidence_path
+          project.save_as_learning_summary_report final_pdf_path
         end
       end
 
@@ -1371,7 +1399,10 @@ def accept_submission(current_user, files, ui, contributions, trigger, alignment
     #
     # Set portfolio_evidence_path to nil while it gets processed
     #
-    self.portfolio_evidence_path = nil
+    if portfolio_evidence.present?
+      FileUtils.rm_f(portfolio_evidence_path)
+      update(portfolio_evidence: nil)
+    end
 
     files.each_with_index.map do |file, idx|
       output_filename = File.join(tmp_dir, "#{idx.to_s.rjust(3, '0')}-#{file[:type]}#{File.extname(file[:filename]).downcase}")
@@ -1465,7 +1496,7 @@ def read_file_from_done(idx)
   end
 
   def archive_submission
-    FileUtils.rm_f(portfolio_evidence_path) if has_pdf
+    FileUtils.rm_f(final_pdf_path) if has_pdf
   end
 
   def overseer_enabled?
@@ -1484,8 +1515,8 @@ def delete_associated_files
       zip_file = zip_file_path_for_done_task
 
       FileUtils.rm(zip_file) if zip_file && File.exist?(zip_file)
-
-      FileUtils.rm(portfolio_evidence_path) if portfolio_evidence_path.present? && File.exist?(portfolio_evidence_path)
+      path = final_pdf_path
+      FileUtils.rm(path) if path.present? && File.exist?(path)
 
       new_path = FileHelper.student_work_dir(:new, self, false)
       FileUtils.rm_rf(new_path) if new_path.present? && File.directory?(new_path)