Proforma: reuse existing files on import, if possible (#2538)

* basic functionality of reusing files based on xml_id.
Use xml_id_path to recreate proforma structure with tests and model_solutions containing multiple files

* refactor xml_id_path to use array

* Update app/services/proforma_service/convert_exercise_to_task.rb

Co-authored-by: Sebastian Serth <[email protected]>

* fix exercise#duplicate when xml_id_path is set

---------

Co-authored-by: Sebastian Serth <[email protected]>

Author: kkoehn

app/controllers/exercises_controller.rb

@@ -56,7 +56,7 @@ def batch_update
   end
 
   def clone
-    exercise = @exercise.duplicate(public: false, token: nil, user: current_user)
+    exercise = @exercise.duplicate(public: false, token: nil, user: current_user, uuid: nil)
     exercise.send(:generate_token)
     if exercise.save
       redirect_to(exercise_path(exercise), notice: t('shared.object_cloned', model: Exercise.model_name.human))

app/models/code_ocean/file.rb

@@ -60,6 +60,11 @@ class File < ApplicationRecord
     validates :weight, absence: true, unless: :teacher_defined_assessment?
     validates :file, presence: true if :context.is_a?(Submission)
     validates :context_type, inclusion: {in: ALLOWED_CONTEXT_TYPES}
+    # xml_id_path must be unique within the scope of an exercise.
+    # Initially, it may match the record’s id (set while exporting),
+    # but it can later diverge as long as uniqueness is preserved.
+    validates :xml_id_path, uniqueness: {scope: %I[context_id context_type]}, allow_blank: true
+    validates :xml_id_path, absence: true, unless: -> { context.is_a?(Exercise) }
 
     validates_with FileNameValidator, fields: %i[name path file_type_id]
 

app/services/proforma_service/convert_exercise_to_task.rb

@@ -67,7 +67,7 @@ def create_task
 
     def proglang
       regex = %r{^openhpi/co_execenv_(?<language>[^:]*):(?<version>[^-]*)(?>-.*)?$}
-      match = regex.match @exercise.execution_environment.docker_image
+      match = regex.match @exercise&.execution_environment&.docker_image
       match ? {name: match[:language], version: match[:version]} : nil
     end
 
@@ -77,62 +77,85 @@ def uuid
     end
 
     def model_solutions
-      @exercise.files.filter {|file| file.role == 'reference_implementation' }.map do |file|
+      @exercise.files.filter(&:reference_implementation?).group_by {|file| xml_id_from_file(file).first }.map do |xml_id, files|
         ProformaXML::ModelSolution.new(
-          id: "ms-#{file.id}",
-          files: model_solution_file(file)
+          id: xml_id,
+          files: files.map {|file| model_solution_file(file) }
         )
       end
     end
 
     def model_solution_file(file)
-      [
-        task_file(file).tap do |ms_file|
-          ms_file.used_by_grader = false
-          ms_file.usage_by_lms = 'display'
-        end,
-      ]
+      task_file(file).tap do |ms_file|
+        ms_file.used_by_grader = false
+        ms_file.usage_by_lms = 'display'
+      end
     end
 
     def tests
-      @exercise.files.filter(&:teacher_defined_assessment?).map do |file|
+      @exercise.files.filter(&:teacher_defined_assessment?).group_by {|file| xml_id_from_file(file).first }.map do |xml_id, files|
         ProformaXML::Test.new(
-          id: file.id,
-          title: file.name,
-          files: test_file(file),
-          meta_data: test_meta_data(file)
+          id: xml_id,
+          title: files.first.name,
+          files: files.map {|file| test_file(file) },
+          meta_data: test_meta_data(files)
         )
       end
     end
 
-    def test_meta_data(file)
+    def xml_id_from_file(file)
+      xml_id_path = file.xml_id_path || []
+      return xml_id_path if xml_id_path&.any?
+
+      type = if file.teacher_defined_assessment?
+               'test'
+             elsif file.reference_implementation?
+               'ms'
+             else
+               'file'
+             end
+
+      xml_id_path << "co-#{type}-#{file.id}" unless type == 'file'
+      xml_id_path << file.id.to_s
+
+      file.update!(xml_id_path: xml_id_from_file(file))
+      xml_id_path
+    end
+
+    def test_meta_data(files)
       {
         '@@order' => %w[test-meta-data],
         'test-meta-data' => {
-          '@@order' => %w[CodeOcean:feedback-message CodeOcean:weight CodeOcean:hidden-feedback],
+          '@@order' => %w[CodeOcean:test-file],
           '@xmlns' => {'CodeOcean' => 'codeocean.openhpi.de'},
-          'CodeOcean:feedback-message' => {
-            '@@order' => %w[$1],
-            '$1' => file.feedback_message,
-          },
-          'CodeOcean:weight' => {
-            '@@order' => %w[$1],
-            '$1' => file.weight,
-          },
-          'CodeOcean:hidden-feedback' => {
-            '@@order' => %w[$1],
-            '$1' => file.hidden_feedback,
-          },
+          'CodeOcean:test-file' => files.map do |file|
+            {
+              '@@order' => %w[CodeOcean:feedback-message CodeOcean:weight CodeOcean:hidden-feedback],
+              '@xmlns' => {'CodeOcean' => 'codeocean.openhpi.de'},
+              '@id' =>  xml_id_from_file(file).last,
+              '@name' => file.name,
+              'CodeOcean:feedback-message' => {
+                '@@order' => %w[$1],
+                '$1' => file.feedback_message,
+              },
+              'CodeOcean:weight' => {
+                '@@order' => %w[$1],
+                '$1' => file.weight,
+              },
+              'CodeOcean:hidden-feedback' => {
+                '@@order' => %w[$1],
+                '$1' => file.hidden_feedback,
+              },
+            }
+          end,
         },
       }
     end
 
     def test_file(file)
-      [
-        task_file(file).tap do |t_file|
-          t_file.used_by_grader = true
-        end,
-      ]
+      task_file(file).tap do |t_file|
+        t_file.used_by_grader = true
+      end
     end
 
     def exercise_files
@@ -168,8 +191,9 @@ def task_files
     end
 
     def task_file(file)
+      xml_id = xml_id_from_file(file).last
       task_file = ProformaXML::TaskFile.new(
-        id: file.id,
+        id: xml_id,
         filename: filename(file),
         usage_by_lms: file.read_only ? 'display' : 'edit',
         visible: file.hidden ? 'no' : 'yes'

app/services/proforma_service/convert_task_to_exercise.rb

@@ -10,13 +10,16 @@ def initialize(task:, user:, exercise: nil)
     end
 
     def execute
-      import_task
+      ActiveRecord::Base.transaction do
+        import_task
+      end
       @exercise
     end
 
     private
 
     def import_task
+      destroy_old_files
       @exercise.assign_attributes(
         user: @user,
         title: @task.title,
@@ -32,9 +35,14 @@ def import_task
       )
     end
 
+    def destroy_old_files
+      file_ids = (@task.files + @task.tests.flat_map(&:files) + @task.model_solutions.flat_map(&:files)).map(&:id)
+      @exercise.files.reject {|file| file_ids.include? file.xml_id_path.last }.each(&:destroy)
+    end
+
     def extract_meta_data(meta_data, *path)
       current_level = meta_data
-      path.each {|attribute| current_level = current_level&.dig("CodeOcean:#{attribute}") }
+      path.each {|attribute| current_level = current_level.is_a?(Hash) ? current_level&.dig("CodeOcean:#{attribute}") : current_level&.find {|entry| entry['@id'] == attribute } } # || current_level
       current_level&.dig('$1')
     end
 
@@ -60,44 +68,55 @@ def string_to_bool(str)
     end
 
     def files
-      model_solution_files + test_files + task_files.values.tap {|array| array.each {|file| file.role ||= 'regular_file' } }
+      model_solution_files + test_files + task_files
     end
 
     def test_files
-      @task.tests.map do |test_object|
-        task_files.delete(test_object.files.first.id).tap do |file|
-          file.weight = extract_meta_data(test_object.meta_data&.dig('test-meta-data'), 'weight').presence || 1.0
-          file.feedback_message = extract_meta_data(test_object.meta_data&.dig('test-meta-data'), 'feedback-message').presence || 'Feedback'
-          file.hidden_feedback = extract_meta_data(test_object.meta_data&.dig('test-meta-data'), 'hidden-feedback').presence || false
-          file.role ||= 'teacher_defined_test'
+      @task.tests.flat_map do |test|
+        test.files.map do |task_file|
+          codeocean_file_from_task_file(task_file, test).tap do |file|
+            file.weight = extract_meta_data(test.meta_data&.dig('test-meta-data'), 'test-file', task_file.id, 'weight').presence || 1.0
+            file.feedback_message = extract_meta_data(test.meta_data&.dig('test-meta-data'), 'test-file', task_file.id, 'feedback-message').presence || 'Feedback'
+            file.hidden_feedback = extract_meta_data(test.meta_data&.dig('test-meta-data'), 'test-file', task_file.id, 'hidden-feedback').presence || false
+            file.role = 'teacher_defined_test' unless file.teacher_defined_assessment?
+          end
         end
       end
     end
 
     def model_solution_files
-      @task.model_solutions.map do |model_solution_object|
-        task_files.delete(model_solution_object.files.first.id).tap do |file|
-          file.role ||= 'reference_implementation'
+      @task.model_solutions.flat_map do |model_solution|
+        model_solution.files.map do |task_file|
+          codeocean_file_from_task_file(task_file, model_solution).tap do |file|
+            file.role ||= 'reference_implementation'
+            file.feedback_message = nil
+          end
         end
       end
     end
 
     def task_files
-      @task_files ||= @task.all_files.reject {|file| file.id == 'ms-placeholder-file' }.to_h do |task_file|
-        [task_file.id, codeocean_file_from_task_file(task_file)]
+      @task.files.reject {|file| file.id == 'ms-placeholder-file' }.map do |task_file|
+        codeocean_file_from_task_file(task_file).tap do |file|
+          file.role ||= 'regular_file'
+          file.feedback_message = nil
+        end
       end
     end
 
-    def codeocean_file_from_task_file(file)
+    def codeocean_file_from_task_file(file, parent_object = nil)
       extension = File.extname(file.filename)
-      codeocean_file = CodeOcean::File.new(
+      # checking the last element of xml_id_path array for file.id
+      codeocean_file = @exercise.files.detect {|f| f.xml_id_path.last == file.id } || @exercise.files.new
+      codeocean_file.assign_attributes(
         context: @exercise,
         file_type: file_type(extension),
         hidden: file.visible != 'yes', # hides 'delayed' and 'no'
         name: File.basename(file.filename, '.*'),
         read_only: file.usage_by_lms != 'edit',
         role: extract_meta_data(@task.meta_data&.dig('meta-data'), 'files', "CO-#{file.id}", 'role'),
-        path: File.dirname(file.filename).in?(['.', '']) ? nil : File.dirname(file.filename)
+        path: File.dirname(file.filename).in?(['.', '']) ? nil : File.dirname(file.filename),
+        xml_id_path: (parent_object.nil? ? [file.id] : [parent_object.id, file.id]).map(&:to_s)
       )
       if file.binary
         codeocean_file.native_file = FileIO.new(file.content.dup.force_encoding('UTF-8'), File.basename(file.filename))

app/services/proforma_service/import.rb

@@ -14,13 +14,7 @@ def execute
         import_result = importer.perform
         @task = import_result
 
-        exercise = base_exercise
-        exercise_files = exercise&.files&.to_a
-
-        exercise = ConvertTaskToExercise.call(task: @task, user: @user, exercise:)
-        exercise_files&.each(&:destroy) # feels suboptimal
-
-        exercise
+        ConvertTaskToExercise.call(task: @task, user: @user, exercise: base_exercise)
       else
         import_multi
       end

db/migrate/20241007204319_add_xml_path_to_files.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddXmlPathToFiles < ActiveRecord::Migration[7.1]
+  def change
+    add_column :files, :xml_id_path, :string, array: true, default: [], null: true
+  end
+end

db/schema.rb

@@ -49,6 +49,15 @@
       expect_redirect(Exercise.last)
     end
 
+    context 'when exercise has uuid' do
+      let(:exercise) { create(:dummy, uuid: SecureRandom.hex) }
+
+      it 'clones the exercise' do
+        expect_any_instance_of(Exercise).to receive(:duplicate).with(hash_including(public: false, user:)).and_call_original
+        expect { perform_request.call }.to change(Exercise, :count).by(1)
+      end
+    end
+
     context 'when saving fails' do
       before do
         allow_any_instance_of(Exercise).to receive(:save).and_return(false)

spec/controllers/exercises_controller_spec.rb

@@ -57,6 +57,46 @@
     end
   end
 
+  context 'with xml_id_path' do
+    let(:exercise) { create(:dummy) }
+    let(:file) { build(:file, context: file_context, xml_id_path: xml_id_path) }
+    let(:file_context) { exercise }
+    let(:xml_id_path) { ['abcde'] }
+
+    before do
+      create(:file, context: exercise, xml_id_path: ['abcde'])
+      file.validate
+    end
+
+    it 'has an error for xml_id_path' do
+      expect(file.errors[:xml_id_path]).to be_present
+    end
+
+    context 'when second file has a different exercise' do
+      let(:file_context) { create(:dummy) }
+
+      it 'has no error for xml_id_path' do
+        expect(file.errors[:xml_id_path]).not_to be_present
+      end
+    end
+
+    context 'when second file has a different xml_id_path' do
+      let(:xml_id_path) { ['foobar'] }
+
+      it 'has no error for xml_id_path' do
+        expect(file.errors[:xml_id_path]).not_to be_present
+      end
+    end
+
+    context 'when file_context is not Exercise' do
+      let(:file_context) { create(:submission) }
+
+      it 'has an error for xml_id_path' do
+        expect(file.errors[:xml_id_path]).to be_present
+      end
+    end
+  end
+
   context 'with a native file' do
     let(:file) { create(:file, :image) }