1320: refactor convert proforma to task service to use persistence for file-consistency

Author: kkoehn

app/services/proforma_service/convert_proforma_task_to_task.rb

@@ -7,19 +7,19 @@ def initialize(proforma_task:, user:, task: nil)
       @proforma_task = proforma_task
       @user = user
       @task = task || Task.new
-      @all_files = @task.all_files
       @file_xml_ids = []
     end
 
     def execute
-      import_task
+      ActiveRecord::Base.transaction do
+        import_task
+      end
       @task
     end
 
     private
 
     def import_task
-      upsert_files @proforma_task, @task
       @task.assign_attributes(
         user:,
         title: @proforma_task.title,
@@ -33,12 +33,51 @@ def import_task
 
         submission_restrictions: @proforma_task.submission_restrictions,
         external_resources: @proforma_task.external_resources,
-        grading_hints: @proforma_task.grading_hints,
-
-        tests:,
-        model_solutions:
+        grading_hints: @proforma_task.grading_hints
       )
-      delete_removed_files
+      Pundit.authorize @user, @task, :create?
+
+      @task.save!
+
+      manage_objects
+    end
+
+    def manage_objects
+      # delete files that do not exist in imported task
+      unreferenced_files.each(&:destroy)
+      @task.reload
+      # Move only relocated files to the task, avoiding updates to the updated_at of untouched files.
+      move_relocated_files_to_task
+      set_tests
+      set_model_solutions
+
+      @task.reload
+      upsert_files @proforma_task, @task
+      delete_removed_objects
+      @task.save!
+    end
+
+    def move_relocated_files_to_task
+      @task.files = @task.all_files(cached: false).filter do |file|
+        # Move files to the task if they belong directly to it or if the parent's xml_id differs from the current parent's xml_id.
+        parent_id(file.xml_id) == 'task' || (file.fileable.respond_to?(:xml_id) && file.fileable.xml_id) != parent_id(file.xml_id)
+      end
+    end
+
+    def parent_id(xml_id)
+      [@proforma_task, *@proforma_task.model_solutions, *@proforma_task.tests].find do |object|
+        object.files.any? {|file| file.id == xml_id }
+      end.then {|object| object.respond_to?(:id) ? object.id : 'task' }
+    end
+
+    def all_proforma_files
+      [@proforma_task.files + @proforma_task.tests.map(&:files) + @proforma_task.model_solutions.map(&:files)].flatten
+    end
+
+    def unreferenced_files
+      @task.all_files.reject do |f|
+        all_proforma_files.map {|pf| pf.id.to_s }.include?(f.xml_id)
+      end
     end
 
     def parent_uuid
@@ -62,7 +101,7 @@ def upsert_files(collection, fileable)
     end
 
     def upsert_file_from_proforma_file(proforma_task_file, fileable)
-      task_file = ch_record_for(@all_files, proforma_task_file.id) || TaskFile.new
+      task_file = (@file_xml_ids.exclude?(proforma_task_file.id) && ch_record_for(@task.all_files, proforma_task_file.id)) || TaskFile.new
       task_file.assign_attributes(file_attributes(proforma_task_file, fileable))
       attach_file_content(proforma_task_file, task_file)
       task_file
@@ -105,9 +144,10 @@ def file_attributes(proforma_task_file, fileable)
       }
     end
 
-    def tests
-      @proforma_task.tests.map do |test|
-        ch_test = ch_record_for(@task.tests, test.id) || Test.new(xml_id: test.id)
+    def set_tests
+      @proforma_task.tests.each do |test|
+        ch_test = find_or_initialize_ch_test test
+        ch_test.task = @task
         upsert_files test, ch_test
         ch_test.assign_attributes(
           title: test.title,
@@ -117,12 +157,12 @@ def tests
           meta_data: test.meta_data,
           configuration: test.configuration
         )
-        ch_test
+        ch_test.save!
       end
     end
 
-    def object_files(object)
-      object.files.map {|file| files.delete(file.id) }
+    def find_or_initialize_ch_test(test)
+      ch_record_for(@task.tests, test.id) || Test.new(xml_id: test.id)
     end
 
     def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity
@@ -136,24 +176,36 @@ def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticCo
       end
     end
 
-    def model_solutions
-      @proforma_task.model_solutions.map do |model_solution|
+    def set_model_solutions
+      @proforma_task.model_solutions.each do |model_solution|
         ch_model_solution = ch_record_for(@task.model_solutions, model_solution.id) || ModelSolution.new(xml_id: model_solution.id)
+        ch_model_solution.task = @task
         upsert_files model_solution, ch_model_solution
         ch_model_solution.assign_attributes(
           description: model_solution.description,
           internal_description: model_solution.internal_description
         )
-        ch_model_solution
+        ch_model_solution.save!
       end
     end
 
     def ch_record_for(collection, xml_id)
       collection.select {|record| record.xml_id == xml_id }&.first
     end
 
-    def delete_removed_files
-      @all_files.reject {|file| @file_xml_ids.include? file.xml_id }.each(&:destroy)
+    def delete_removed_objects
+      delete_removed_tests
+      delete_removed_model_solutions
+    end
+
+    def delete_removed_tests
+      remaining_test_ids = @proforma_task.tests.map {|t| t.id.to_s }
+      @task.tests.reject {|test| remaining_test_ids.include? test.xml_id }.each(&:destroy)
+    end
+
+    def delete_removed_model_solutions
+      remaining_model_solution_ids = @proforma_task.model_solutions.map {|ms| ms.id.to_s }
+      @task.model_solutions.reject {|model_solution| remaining_model_solution_ids.include? model_solution.xml_id }.each(&:destroy)
     end
   end
 end

app/services/proforma_service/import_task.rb

@@ -9,10 +9,7 @@ def initialize(proforma_task:, user:)
     end
 
     def execute
-      task = ConvertProformaTaskToTask.call(proforma_task: @proforma_task, user: @user, task: base_task)
-      Pundit.authorize @user, task, :create?
-      task.save!
-      task
+      ConvertProformaTaskToTask.call(proforma_task: @proforma_task, user: @user, task: base_task)
     end
 
     private

spec/services/proforma_service/convert_proforma_task_to_task_spec.rb

@@ -24,7 +24,7 @@
   end
 
   describe '#execute' do
-    subject(:convert_to_task_service) { described_class.call(proforma_task:, user:, task:) }
+    subject(:convert_to_task_service) { described_class.call(proforma_task:, user:, task:).reload }
 
     let(:proforma_task) do
       ProformaXML::Task.new(
@@ -237,8 +237,9 @@
         end
 
         it 'creates files with correct attributes' do
-          expect(convert_to_task_service.tests[1].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.merge('xml_id' => 'id-2'))
-          expect(convert_to_task_service.tests[2].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.merge('xml_id' => 'id-3'))
+          attribute_exceptions = %w[id created_at fileable_id updated_at]
+          expect(convert_to_task_service.tests[1].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.except(*attribute_exceptions).merge('xml_id' => 'id-2'))
+          expect(convert_to_task_service.tests[2].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.except(*attribute_exceptions).merge('xml_id' => 'id-3'))
         end
 
         it 'creates separate copies of referenced file' do
@@ -321,10 +322,12 @@
           internal_description: 'internal_description',
           test_type: 'test_type',
           files: test_files,
-          meta_data: test_meta_data
+          meta_data: test_meta_data,
+          configuration:
         )
       end
 
+      let(:configuration) {}
       let(:test_meta_data) {}
       let(:test_files) { [test_file] }
       let(:test_file) do
@@ -402,15 +405,15 @@
       end
 
       context 'when test has custom configuration' do
-        let(:test) { build(:test, :with_unittest) }
+        let(:configuration) { attributes_for(:test, :with_unittest)[:configuration] }
 
         it 'creates a test with the supplied test configuration' do
           expect(convert_to_task_service.tests.first).to have_attributes(configuration: test.configuration)
         end
       end
 
       context 'when test has multiple custom configuration' do
-        let(:test) { build(:test, :with_multiple_custom_configurations) }
+        let(:configuration) { attributes_for(:test, :with_multiple_custom_configurations)[:configuration] }
 
         it 'creates a test with the supplied test configurations' do
           expect(convert_to_task_service.tests.first).to have_attributes(configuration: test.configuration)
@@ -420,7 +423,11 @@
       context 'when proforma_task has multiple tests' do
         let(:tests) { [test, test2] }
         let(:test2) do
-          ProformaXML::Test.new(files: test_files2)
+          ProformaXML::Test.new(
+            id: 'test_file_id',
+            title: 'wild-title',
+            files: test_files2
+          )
         end
         let(:test_files2) { [test_file2] }
         let(:test_file2) do
@@ -473,6 +480,7 @@
       let(:task) do
         create(
           :task,
+          user:,
           title: 'task-title',
           description: 'task-description',
           internal_description: 'task-internal_description'
@@ -579,7 +587,7 @@
 
     context 'when proforma_task has been exported from task' do
       let(:proforma_task) { ProformaService::ConvertTaskToProformaTask.call(task:) }
-      let!(:task) { create(:task, files: task_files, tests:, model_solutions:, title: 'title') }
+      let!(:task) { create(:task, user:, files: task_files, tests:, model_solutions:, title: 'title') }
       let(:task_files) { build_list(:task_file, 1, :exportable, internal_description: 'original task file') }
       let(:tests) { build_list(:test, 1, files: test_files) }
       let(:test_files) { build_list(:task_file, 1, :exportable, internal_description: 'original test file') }
@@ -605,51 +613,67 @@
         )
       end
 
+      it 'does not update updated_at of unchanged files' do
+        task_file_updated_at = task_files.first.updated_at
+        test_file_updated_at = test_files.first.updated_at
+        ms_file_update_at = model_solution_files.first.updated_at
+
+        expect(convert_to_task_service).to have_attributes(
+          files: have(1).item.and(include(have_attributes(
+            updated_at: task_file_updated_at
+          ))),
+          model_solutions: have(1).item.and(include(have_attributes(
+            files: have(1).item.and(include(have_attributes(updated_at: ms_file_update_at)))
+          ))),
+          tests: have(1).item.and(include(have_attributes(
+            id: tests.first.id,
+            files: have(1).item.and(include(have_attributes(updated_at: test_file_updated_at)))
+          )))
+        )
+      end
+
       context 'when files have been deleted' do
         context 'when task files have been deleted' do
-          before { task.files = [] }
+          before { proforma_task.files = [] }
 
           it 'imports task files correctly' do
             expect(convert_to_task_service.files).to be_empty
           end
 
           it 'saves the task correctly' do
-            convert_to_task_service.save
-            task.reload
-            expect(task.files).to be_empty
+            convert_to_task_service
+            expect(task.reload.files).to be_empty
           end
         end
 
         context 'when test files have been deleted' do
-          before { task.tests.first.files = [] }
+          before { proforma_task.tests.first.files = [] }
 
           it 'imports test files correctly' do
             expect(convert_to_task_service.tests.first.files).to be_empty
           end
 
           it 'saves the task correctly' do
-            convert_to_task_service.save
-            task.reload
+            convert_to_task_service
             expect(task.tests.first.files).to be_empty
           end
         end
 
         context 'when model solution files have been deleted' do
-          before { task.model_solutions.first.files = [] }
+          before { proforma_task.model_solutions.first.files = [] }
 
           it 'imports model solution files correctly' do
             expect(convert_to_task_service.model_solutions.first.files).to be_empty
           end
 
           it 'saves the task correctly' do
-            convert_to_task_service.save
-            task.reload
+            convert_to_task_service
             expect(task.model_solutions.first.files).to be_empty
           end
         end
       end
 
-      context 'when files have been move around' do
+      context 'when files have been moved around' do
         before do
           task_file = proforma_task.files.first
           test_file = proforma_task.tests.first.files.first
@@ -691,15 +715,15 @@
               files: have(1).item.and(include(have_attributes(id: task_files.first.id)))
             ))),
             tests: have(1).item.and(include(have_attributes(
-              id: 987_654_325,
+              xml_id: '987654325',
               files: have(1).item.and(include(have_attributes(id: model_solution_files.first.id)))
             )))
           )
         end
 
         context 'when imported task is persisted' do
           before do
-            convert_to_task_service.save
+            convert_to_task_service.save!
             task.reload
           end
 
@@ -735,7 +759,7 @@
                 files: have(1).item.and(include(have_attributes(id: task_files.first.id)))
               ))),
               tests: have(1).item.and(include(have_attributes(
-                id: 987_654_325,
+                xml_id: '987654325',
                 files: have(1).item.and(include(have_attributes(id: model_solution_files.first.id)))
               )))
             )

spec/services/proforma_service/import_spec.rb

@@ -53,6 +53,7 @@
     before do
       zip_file.write(exporter)
       zip_file.rewind
+      task.reload
     end
 
     it { is_expected.to be_an_equal_task_as task }
@@ -75,7 +76,7 @@
       it { is_expected.to be_valid }
 
       it 'sets the correct user as owner of the task' do
-        expect(imported_task.user).to be user
+        expect(imported_task.user).to eq user
       end
 
       it 'sets the uuid' do
@@ -199,6 +200,8 @@
           user:)
       end
 
+      before { task2.reload }
+
       it 'imports the tasks from zip containing multiple zips' do
         expect(imported_task).to all be_an(Task)
       end