Merge branch '8.0.x' into new/scorm

CHANGELOG.md

@@ -2,6 +2,41 @@
 
 All notable changes to this project will be documented in this file. See [standard-version](https://github.com/conventional-changelog/standard-version) for commit guidelines.
 
+### [8.0.36](https://github.com/macite/doubtfire-deploy/compare/v8.0.35...v8.0.36) (2024-09-24)
+
+
+### Bug Fixes
+
+* markdown in ipynb ([4fd4ed3](https://github.com/macite/doubtfire-deploy/commit/4fd4ed3c633e7aa4ee801e5d274c82aa840f31ac))
+
+### [8.0.35](https://github.com/doubtfire-lms/doubtfire-deploy/compare/v8.0.34...v8.0.35) (2024-09-23)
+
+
+### Bug Fixes
+
+* ensure default lsr task def can be updated ([491691f](https://github.com/doubtfire-lms/doubtfire-deploy/commit/491691f2b7b5d51c838d22299099f3725d2ee47e))
+
+### [8.0.34](https://github.com/doubtfire-lms/doubtfire-deploy/compare/v8.0.33...v8.0.34) (2024-09-21)
+
+
+### Features
+
+* send latex log on convert failure ([166690f](https://github.com/doubtfire-lms/doubtfire-deploy/commit/166690fffe8160146f734d6a7df42dda4174866b))
+
+
+### Bug Fixes
+
+* correct error reporting with AAF failure ([58b6391](https://github.com/doubtfire-lms/doubtfire-deploy/commit/58b6391c5658b80ef4e803cd955bdde90c79d87c))
+* ensure unicode control characters work in pdf gen ([cd32b06](https://github.com/doubtfire-lms/doubtfire-deploy/commit/cd32b0672a4491bdc9b1d0ebef192abc4726ccf0))
+* migration to fix invalid task upload filenames ([ac80fec](https://github.com/doubtfire-lms/doubtfire-deploy/commit/ac80fec11a6748671821b8d82beae016464eec0b))
+
+### [8.0.33](https://github.com/macite/doubtfire-deploy/compare/v8.0.32...v8.0.33) (2024-09-13)
+
+
+### Bug Fixes
+
+* correct access to moss and devise secrets ([8fae9c6](https://github.com/macite/doubtfire-deploy/commit/8fae9c6f02a08e27daa141d448a4dae0ef62b241))
+
 ### [8.0.32](https://github.com/macite/doubtfire-deploy/compare/v8.0.31...v8.0.32) (2024-09-05)
 
 

app/api/task_definitions_api.rb

@@ -162,18 +162,23 @@ class TaskDefinitionsApi < Grape::API
                                                 :upload_requirements
                                               )
 
-    task_params[:upload_requirements] = params[:task_def][:upload_requirements].present? ? JSON.parse(params[:task_def][:upload_requirements]) : []
+    if params[:task_def][:upload_requirements].present?
+      upload_reqs = JSON.parse(params[:task_def][:upload_requirements])
+      task_params[:upload_requirements] = upload_reqs
+
+      # Ensure we permit all of the passed in upload requirements
+      if task_params[:upload_requirements].is_a? Array
+        # Force permit - the model validates the details
+        task_params[:upload_requirements].each(&:permit!)
+      end
 
-    # Ensure changes to a TD defined as a 'draft task definition' are validated
-    if unit.draft_task_definition_id == params[:id]
-      if task_params[:upload_requirements]
-        requirements = task_params[:upload_requirements]
-        if requirements.length != 1 || requirements[0]['type'] != 'document'
-          error!({ error: 'Task is marked as the draft learning summary task definition. A draft learning summary task can only contain a single document upload.' }, 403)
-        end
+      # Ensure changes to a TD defined as a 'draft task definition' are validated
+      if unit.draft_task_definition_id == params[:id] && (upload_reqs.length != 1 || upload_reqs[0]['type'] != 'document')
+        error!({ error: 'Task is marked as the draft learning summary. A draft learning summary task can only contain a single document upload.' }, 403)
       end
     end
 
+    # Bulk update task definition with permitted parameters
     task_def.update!(task_params)
 
     # Set the tutorial stream

app/helpers/file_helper.rb

@@ -547,12 +547,50 @@ def move_compressed_task_to_new(task)
     task.extract_file_from_done student_work_dir(:new), '*', ->(_task, to_path, name) { "#{to_path}#{name}" }
   end
 
+  REPLACEMENTS_SED_COMMAND = [
+    ['[\\]u0000','␀'],
+    ['[\\]u0001','␁'],
+    ['[\\]u0002','␂'],
+    ['[\\]u0003','␃'],
+    ['[\\]u0004','␄'],
+    ['[\\]u0005','␅'],
+    ['[\\]u0006','␆'],
+    ['[\\]u0007','␇'],
+    ['[\\]u0008','␈'],
+    ['[\\]b','␈'],
+    ['[\\]u0009','␉'],
+    ['[\\]u000A','␊'],
+    ['[\\]u000B','␋'],
+    ['[\\]u000C','␌'],
+    ['[\\]f','␌'],
+    ['[\\]u000D','␍'],
+    ['[\\]r','␍'],
+    ['[\\]u000E','␎'],
+    ['[\\]u000F','␏'],
+    ['[\\]u0010','␐'],
+    ['[\\]u0011','␑'],
+    ['[\\]u0012','␒'],
+    ['[\\]u0013','␓'],
+    ['[\\]u0014','␔'],
+    ['[\\]u0015','␕'],
+    ['[\\]u0016','␖'],
+    ['[\\]u0017','␗'],
+    ['[\\]u0018','␘'],
+    ['[\\]u0019','␙'],
+    ['[\\]u001A','␚'],
+    ['[\\]u001B','␛'],
+    ['[\\]u001C','␜'],
+    ['[\\]u001D','␝'],
+    ['[\\]u001E','␞'],
+    ['[\\]u001F','␟']
+  ].map { |r| "s/#{r[0]}/#{r[1]}/gI" }.join('; ').freeze
+
   #
   # Ensure that the contents of a file appear to be valid UTF8, on retry convert to ASCII to ensure
   #
   def ensure_utf8_code(output_filename, force_ascii)
     # puts "Converting #{output_filename} to utf8"
-    tmp_filename = Dir::Tmpname.create(["new", ".code"]) { |name| raise Errno::EEXIST if File.exist?(name)  }
+    tmp_filename = Dir::Tmpname.create(["new", ".code"]) { |name| raise Errno::EEXIST if File.exist?(name) }
 
     # Convert to utf8 from read encoding
     if force_ascii
@@ -561,6 +599,9 @@ def ensure_utf8_code(output_filename, force_ascii)
       `iconv -c -t UTF-8 "#{output_filename}" > "#{tmp_filename}"`
     end
 
+    # Remove utf8 control character sequences
+    `sed -i '#{FileHelper::REPLACEMENTS_SED_COMMAND}' "#{tmp_filename}"`
+
     # Move into place
     FileUtils.mv(tmp_filename, output_filename)
   end

app/mailers/error_log_mailer.rb

@@ -3,6 +3,10 @@ def error_message(subject, message, exception)
     email = Doubtfire::Application.config.email_errors_to
     return nil if email.blank?
 
+    if exception.instance_of?(Task::LatexError)
+      attachments['log.txt'] = exception.log_message
+    end
+
     @doubtfire_product_name = Doubtfire::Application.config.institution[:product_name]
     @error_log = "#{message}\n\n#{exception.message}\n\n#{exception.backtrace.join("\n")}"
 

app/models/similarity/unit_similarity_module.rb

@@ -51,7 +51,7 @@ def check_moss_similarity(force: false)
         logger.debug 'Contacting MOSS for new checks'
 
         # Create the MossRuby object
-        moss_key = Doubtfire::Application.secrets.secret_key_moss
+        moss_key = Doubtfire::Application.credentials.secret_key_moss
         raise "No moss key set. Check ENV['DF_SECRET_KEY_MOSS'] first." if moss_key.nil?
 
         moss = MossRuby.new(moss_key)
@@ -99,7 +99,7 @@ def check_moss_similarity(force: false)
   end
 
   def update_plagiarism_stats
-    moss_key = Doubtfire::Application.secrets.secret_key_moss
+    moss_key = Doubtfire::Application.credentials.secret_key_moss
     raise "No moss key set. Check ENV['DF_SECRET_KEY_MOSS'] first." if moss_key.nil?
 
     moss = MossRuby.new(moss_key)

app/models/task.rb

@@ -1141,6 +1141,16 @@ def final_pdf_path
     end
   end
 
+  # A custom error to capture the log message from the latex error
+  class LatexError < StandardError
+    attr_reader :log_message
+
+    def initialize(log_message)
+      super
+      @log_message = log_message
+    end
+  end
+
   # Convert a submission to pdf - the source folder is the root folder in which the submission folder will be found (not the submission folder itself)
   def convert_submission_to_pdf(source_folder: FileHelper.student_work_dir(:new), log_to_stdout: true)
     logger.info "Converting task #{self.id} to pdf"
@@ -1168,20 +1178,24 @@ def convert_submission_to_pdf(source_folder: FileHelper.student_work_dir(:new),
           logger.error "Failed to create PDF for task #{log_details}. Error: #{e.message}"
 
           log_file = e.message.scan(/\/.*\.log/).first
-          # puts "log file is ... #{log_file}"
-          if log_to_stdout && log_file && File.exist?(log_file)
-            # puts "exists"
-            begin
-              # rubocop:disable Rails/Output
-              puts "--- Latex Log ---\n"
-              puts File.read(log_file)
-              puts "---    End    ---\n\n"
-              # rubocop:enable Rails/Output
-            rescue
+          if log_file && File.exist?(log_file)
+            log_message = File.read(log_file)
+
+            # puts "log file is ... #{log_file}"
+            if log_to_stdout
+              # puts "exists"
+              begin
+                # rubocop:disable Rails/Output
+                puts "--- Latex Log ---\n"
+                puts log_message
+                puts "---    End    ---\n\n"
+                # rubocop:enable Rails/Output
+              rescue
+              end
             end
           end
 
-          raise 'Failed to convert your submission to PDF. Check code files submitted for invalid characters, that documents are valid pdfs, and that images are valid.'
+          raise LatexError.new(log_message), 'Failed to convert your submission to PDF. Check code files submitted for invalid characters, that documents are valid pdfs, and that images are valid.'
         end
       end
 

app/views/layouts/jupynotex.py

@@ -25,7 +25,7 @@
 
 # markdown start/end
 MARKDOWN_BEGIN = [r"\begin{markdown}"]
-MARKDOWN_END = [r"\end{markdown}"]
+MARKDOWN_END = [r"\end{markdown}"+"\n"]
 
 # highlighers for different languages (block beginning and ending)
 HIGHLIGHTERS = {

config/application.rb

@@ -151,7 +151,7 @@ def self.fetch_boolean_env(name)
               "DF_AAF_CALLBACK_URL          => #{!ENV['DF_AAF_CALLBACK_URL'].nil?}\n  " \
               "DF_AAF_IDENTITY_PROVIDER_URL => #{!ENV['DF_AAF_IDENTITY_PROVIDER_URL'].nil?}\n  " \
               "DF_AAF_UNIQUE_URL            => #{!ENV['DF_AAF_UNIQUE_URL'].nil?}\n  " \
-              "DF_SECRET_KEY_AAF            => #{!secrets.secret_key_aaf.nil?}\n"
+              "DF_SECRET_KEY_AAF            => #{!credentials.secret_key_aaf.nil?}\n"
       end
     end
     # Check secrets set for DF_SECRET_KEY_BASE, DF_SECRET_KEY_ATTR, DF_SECRET_KEY_DEVISE

config/initializers/devise.rb

@@ -252,7 +252,7 @@
   # ==> AAF via JWT OmniAuth
   # Devise method for JWT
   # if Doubtfire::Application.config.auth_method == :jwt
-  #   aaf_secret = Doubtfire::Application.secrets.secret_key_aaf
+  #   aaf_secret = Doubtfire::Application.credentials.secret_key_aaf
   #   aaf_config = Doubtfire::Application.config.aaf
   #   config.omniauth :jwt,
   #                   aaf_secret,
@@ -268,7 +268,7 @@
 
   # ==> Devise secret key
   # Secret key to be used by devise in prod.
-  config.secret_key = Doubtfire::Application.secrets.secret_key_devise if Rails.env.production?
+  config.secret_key = Doubtfire::Application.credentials.secret_key_devise if Rails.env.production?
 
   config.ldap_use_admin_to_bind = ENV.fetch('DF_LDAP_USE_ADMIN_TO_BIND', 'false').to_s.downcase != 'false'
 

db/migrate/20240920052508_convert_task_def_filenames.rb

@@ -0,0 +1,32 @@
+class ConvertTaskDefFilenames < ActiveRecord::Migration[7.1]
+
+  # Check filenames in the upload requirements for each task definition
+  # and replace any invalid characters using sanitize filename
+  def change
+    TaskDefinition.find_in_batches do |group|
+      group.each do |task_def|
+        next if task_def.valid?
+
+        upload_req = task_def.upload_requirements
+
+        change = false
+        upload_req.each do |req|
+          unless req['name'].match?(/^[a-zA-Z0-9_\- \.]+$/)
+            req['name'] = FileHelper.sanitized_filename(req['name'])
+            change = true
+          end
+
+          if req['name'].blank?
+            req['name'] = 'file'
+            change = true
+          end
+        end
+
+        unless change && task_def.valid? && task_def.save
+          puts "Remaining issue with task definition #{task_def.id}"
+        end
+        puts '.'
+      end
+    end
+  end
+end

test/api/tasks_api_test.rb

@@ -406,6 +406,47 @@ def test_can_submit_ipynb
     unit.destroy
   end
 
+  def test_invalid_latex_in_ipynb
+    unit = FactoryBot.create(:unit, student_count: 1, task_count: 0)
+    td = TaskDefinition.create!({
+        unit_id: unit.id,
+        tutorial_stream: unit.tutorial_streams.first,
+        name: 'Code task',
+        description: 'Code task',
+        weighting: 4,
+        target_grade: 0,
+        start_date: Time.zone.now - 2.weeks,
+        target_date: Time.zone.now + 1.week,
+        abbreviation: 'CodeTask',
+        restrict_status_updates: false,
+        upload_requirements: [ { "key" => 'file0', "name" => 'Shape Class', "type" => 'code' } ],
+        plagiarism_warn_pct: 0.8,
+        is_graded: true,
+        max_quality_pts: 0
+      })
+
+    project = unit.active_projects.first
+
+    # Add username and auth_token to Header
+    add_auth_header_for(user: project.user)
+
+    data_to_post = {
+      trigger: 'ready_for_feedback'
+    }
+
+    data_to_post = with_file('test_files/submissions/invalid_notebook.ipynb', 'application/json', data_to_post)
+
+    post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data_to_post
+
+    assert_equal 201, last_response.status, last_response_body
+
+    task = project.task_for_task_definition(td)
+    task.convert_submission_to_pdf(log_to_stdout: true)
+    assert File.exist? task.final_pdf_path
+
+    unit.destroy
+  end
+
   def test_download_task_pdf
     unit = FactoryBot.create(:unit, student_count: 1, task_count: 0)
     td = TaskDefinition.create!({

test/mailers/error_log_mailer_test.rb

@@ -16,4 +16,18 @@ def test_can_send_error_log_mail
     assert mail.body.include? e.message
     assert mail.body.include? e.backtrace.join("\n")
   end
+
+  def test_latex_error_logs_are_attached
+    Doubtfire::Application.config.email_errors_to = 'test <[email protected]>'
+    begin
+      raise Task::LatexError.new('this is the content of the log'), 'test'
+    rescue StandardError => e
+      mail = ErrorLogMailer.error_message('test', 'test message', e)
+    end
+
+    assert mail.present?
+    assert mail.to.include? '[email protected]'
+    assert mail.attachments['log.txt'].present?
+    assert mail.attachments['log.txt'].body.include? 'this is the content of the log'
+  end
 end