refactor xml_id_path to use array

Author: kkoehn

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
 
@@ -105,19 +105,21 @@ def tests
     end
 
     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_parts = file.xml_id_path&.split('/')
-      xml_id_path  = []
 
-      xml_id_path << (xml_id_path_parts&.first || "co-#{type}-#{file.id}") unless type == 'file'
-      xml_id_path << (xml_id_path_parts&.last || file.id)
+      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
 
@@ -190,8 +192,6 @@ def task_files
     end
 
     def task_file(file)
-      file.update(xml_id_path: xml_id_from_file(file).join('/')) if file.xml_id_path.blank?
-
       xml_id = xml_id_from_file(file).last
       task_file = ProformaXML::TaskFile.new(
         id: xml_id,

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
 
@@ -81,6 +89,7 @@ def model_solution_files
         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
@@ -90,14 +99,15 @@ def task_files
       @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, parent_object = nil)
       extension = File.extname(file.filename)
-
-      codeocean_file = CodeOcean::File.where(context: @exercise).where('xml_id_path = ? OR xml_id_path LIKE ?', file.id, "%/#{file.id}").first_or_initialize(context: @exercise)
+      # checking the last element of xml_id_path array for file.id
+      codeocean_file = @exercise.files.where('array_length(xml_id_path, 1) IS NOT NULL AND xml_id_path[array_length(xml_id_path, 1)] = ?', file.id).first_or_initialize
       codeocean_file.assign_attributes(
         context: @exercise,
         file_type: file_type(extension),
@@ -106,7 +116,7 @@ def codeocean_file_from_task_file(file, parent_object = nil)
         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),
-        xml_id_path: (parent_object.nil? ? '' : "#{parent_object.id}/") + file.id.to_s
+        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))

…/20240903204319_add_xml_path_to_files.rb …/20241007204319_add_xml_path_to_files.rbdb/migrate/20240903204319_add_xml_path_to_files.rb renamed to db/migrate/20241007204319_add_xml_path_to_files.rb db/migrate/20240903204319_add_xml_path_to_files.rb renamed to db/migrate/20241007204319_add_xml_path_to_files.rb

@@ -2,6 +2,6 @@
 
 class AddXmlPathToFiles < ActiveRecord::Migration[7.1]
   def change
-    add_column :files, :xml_id_path, :string, null: true, default: nil
+    add_column :files, :xml_id_path, :string, array: true, default: [], null: true
   end
 end

db/schema.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_exercise) { 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) }
 

spec/models/code_ocean/file_spec.rb

@@ -22,8 +22,10 @@
         execution_environment:,
         instructions: 'instruction',
         uuid: SecureRandom.uuid,
-        files: files + tests)
+        files: files + tests,
+        unpublished:)
     end
+    let(:unpublished) { false }
     let(:files) { [] }
     let(:tests) { [] }
     let(:execution_environment) { create(:java) }
@@ -77,6 +79,15 @@
       end
     end
 
+    context 'when exercise has no execution_environment' do
+      let(:execution_environment) { nil }
+      let(:unpublished) { true }
+
+      it 'creates a task with the correct proglang attribute' do
+        expect(task).to have_attributes(proglang: nil)
+      end
+    end
+
     context 'when exercise has a mainfile' do
       let(:files) { [file] }
       let(:file) { create(:file) }
@@ -111,13 +122,13 @@
       end
 
       it 'sets xml_id_path to default' do
-        expect { convert_to_task.execute }.to change(file.reload, :xml_id_path).from(nil).to(file.id.to_s)
+        expect { convert_to_task.execute }.to change(file.reload, :xml_id_path).from([]).to([file.id.to_s])
       end
 
       context 'when xml_id_path is set for file' do
-        let(:file) { create(:file, xml_id_path: 'foobar') }
+        let(:file) { create(:file, xml_id_path: ['foobar']) }
 
-        it 'does not change xml_path_id' do
+        it 'does not change xml_id_path' do
           expect { convert_to_task.execute }.not_to change(file.reload, :xml_id_path)
         end
       end
@@ -224,7 +235,7 @@
       end
 
       context 'when reference_implementations belong to the same proforma model_solution' do
-        let(:files) { build_list(:file, 2, role: 'reference_implementation') {|file, i| file.xml_id_path = "proforma_ms/xml_id_#{i}" } }
+        let(:files) { build_list(:file, 2, role: 'reference_implementation') {|file, i| file.xml_id_path = ['proforma_ms', "xml_id_#{i}"] } }
 
         it 'creates a task with one model-solution' do
           expect(task.model_solutions).to have(1).items
@@ -294,7 +305,7 @@
       end
 
       context 'when test_files belong to the same proforma test' do
-        let(:tests) { build_list(:test_file, 2) {|file, i| file.xml_id_path = "proforma_test/xml_id_#{i}" } }
+        let(:tests) { build_list(:test_file, 2) {|file, i| file.xml_id_path = ['proforma_test', "xml_id_#{i}"] } }
 
         it 'creates a test with two file' do
           expect(task.tests.first).to have_attributes(

spec/services/proforma_service/convert_exercise_to_task_spec.rb

@@ -558,21 +558,73 @@
         context 'when the files with correct xml_id_paths already exist' do
           let(:exercise) do
             create(:dummy,
+              unpublished: true,
               files: co_files,
               title: 'exercise-title',
               description: 'exercise-description')
           end
           let(:co_files) do
-            [create(:file, xml_id_path: 'id', role: 'regular_file'),
-             create(:file, xml_id_path: 'ms-id/ms-file-id', role: 'reference_implementation'),
-             create(:test_file, xml_id_path: 'test-id/test-file-id')]
+            [create(:file, xml_id_path: ['id'], role: 'regular_file'),
+             create(:file, xml_id_path: %w[ms-id ms-file-id], role: 'reference_implementation'),
+             create(:test_file, xml_id_path: %w[test-id test-file-id])]
           end
 
           it 'reuses existing file' do
             convert_to_exercise_service
-
+            exercise.save!
             expect(exercise.reload.files).to match_array(co_files)
           end
+
+          context 'when files are move around' do
+            let(:files) { [test_file] }
+            let(:test_files) { [ms_file] }
+            let(:ms_files) { [file] }
+
+            it 'reuses existing file' do
+              convert_to_exercise_service
+              exercise.save!
+              expect(exercise.reload.files).to match_array(co_files)
+            end
+          end
+
+          context 'when some files are removed' do
+            let(:test_files) { [] }
+            let(:ms_files) { [] }
+
+            it 'removes files from exercise' do
+              convert_to_exercise_service
+              exercise.save!
+              expect(exercise.reload.files.count).to be 1
+            end
+
+            it 'destroys removed files' do
+              expect { convert_to_exercise_service }.to change(CodeOcean::File, :count).by(-2)
+            end
+          end
+
+          context 'when all files are removed' do
+            let(:files) { [] }
+            let(:test_files) { [] }
+            let(:ms_files) { [] }
+
+            it 'removes files from exercise' do
+              convert_to_exercise_service
+              exercise.save!
+              expect(exercise.reload.files).to be_empty
+            end
+
+            it 'destroys removed files' do
+              expect { convert_to_exercise_service }.to change(CodeOcean::File, :count).by(-3)
+            end
+
+            context 'when the import errors after the file deletion' do
+              before { allow(exercise).to receive(:assign_attributes).and_raise(StandardError) }
+
+              it 'rolls back deletion of files' do
+                expect { convert_to_exercise_service }.to raise_error(StandardError).and(avoid_change(CodeOcean::File, :count))
+              end
+            end
+          end
         end
       end
     end

spec/services/proforma_service/convert_task_to_exercise_spec.rb

@@ -127,7 +127,7 @@
     end
 
     context 'when exercise has multiple tests' do
-      let(:tests) { build_list(:test_file, 2) }
+      let(:tests) { [build(:test_file, xml_id_path: %w[test file1]), build(:test_file, xml_id_path: %w[test file2])] }
 
       it { is_expected.to be_an_equal_exercise_as exercise }
     end