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

kkoehn · kkoehn · commit 0409543a2137 · 2024-12-02T23:32:22.000+01:00
diff --git a/app/models/concerns/file_concern.rb b/app/models/concerns/file_concern.rb
diff --git a/app/models/model_solution.rb b/app/models/model_solution.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 class ModelSolution < ApplicationRecord
-  include FileConcern
   include TransferValues
   include ParentValidation
 
diff --git a/app/models/task.rb b/app/models/task.rb
@@ -3,7 +3,6 @@
 require 'nokogiri'
 require 'zip'
 class Task < ApplicationRecord # rubocop:disable Metrics/ClassLength
-  include FileConcern
   include ParentValidation
   include TransferValues
 
@@ -23,6 +22,9 @@ class Task < ApplicationRecord # rubocop:disable Metrics/ClassLength
   validates :parent, presence: true, if: -> { contribution? }
   validate :no_license_change_on_duplicate, on: :update
 
+  has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy
+  accepts_nested_attributes_for :files, allow_destroy: true
+
   has_many :group_tasks, dependent: :destroy
   has_many :groups, through: :group_tasks
 
diff --git a/app/models/test.rb b/app/models/test.rb
@@ -1,13 +1,14 @@
 # frozen_string_literal: true
 
 class Test < ApplicationRecord
-  include FileConcern
   include TransferValues
   include ParentValidation
 
   belongs_to :task, autosave: true, inverse_of: :tests
   belongs_to :testing_framework, optional: true
   belongs_to :parent, class_name: 'Test', optional: true
+  has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy
+  accepts_nested_attributes_for :files, allow_destroy: true
   validates :title, presence: true
   validates :xml_id, presence: true
   validates :parent_id, uniqueness: {scope: :task}, if: -> { parent_id.present? }
diff --git a/app/services/proforma_service/convert_proforma_task_to_task.rb b/app/services/proforma_service/convert_proforma_task_to_task.rb
@@ -12,14 +12,15 @@ def initialize(proforma_task:, user:, task: nil)
     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 +34,35 @@ 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
       )
+      @task.save!
+      @task.reload
+      @task.files = @all_files # @task.files + unreferenced_files
+      @task.reload
+      tests
+      @task.reload
+      model_solutions
+
+      @task.reload
+      @all_files = @task.files
+      upsert_files @proforma_task, @task
+      # Currently files on task do not get deleted.
+      # find all removed files first and move only those to task (and then further as necessary) that way updated_at does not get set
+      # upsert, then delete unwanted files (by comparing xml_ids)
       delete_removed_files
+      delete_removed_objects
+      @task.save!
+    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.filter do |f|
+        all_proforma_files.map {|pf| pf.id.to_s }.include?(f.xml_id)
+      end
     end
 
     def parent_uuid
@@ -106,8 +130,9 @@ def file_attributes(proforma_task_file, fileable)
     end
 
     def tests
-      @proforma_task.tests.map do |test|
+      @proforma_task.tests.each do |test|
         ch_test = ch_record_for(@task.tests, test.id) || Test.new(xml_id: test.id)
+        ch_test.task = @task
         upsert_files test, ch_test
         ch_test.assign_attributes(
           title: test.title,
@@ -117,7 +142,7 @@ def tests
           meta_data: test.meta_data,
           configuration: test.configuration
         )
-        ch_test
+        ch_test.save!
       end
     end
 
@@ -137,14 +162,15 @@ def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticCo
     end
 
     def model_solutions
-      @proforma_task.model_solutions.map do |model_solution|
+      @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
 
@@ -153,7 +179,14 @@ def ch_record_for(collection, xml_id)
     end
 
     def delete_removed_files
-      @all_files.reject {|file| @file_xml_ids.include? file.xml_id }.each(&:destroy)
+      @all_files.reject {|file| @file_xml_ids.map(&:to_s).include? file.xml_id }.each(&:destroy)
+    end
+
+    def delete_removed_objects
+      @task.tests.reject {|test| @proforma_task.tests.map {|t| t.id.to_s }.include? test.xml_id }.each(&:destroy)
+      @task.model_solutions.reject do |model_solution|
+        @proforma_task.model_solutions.map {|ms| ms.id.to_s }.include? model_solution.xml_id
+      end.each(&:destroy)
     end
   end
 end
diff --git a/spec/services/proforma_service/convert_proforma_task_to_task_spec.rb b/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 = ['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
@@ -605,51 +612,69 @@
         )
       end
 
+      xit '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 +716,17 @@
               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 +762,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)))
               )))
             )
