refactor import to reuse task_files based on xml_id

app/models/task_file.rb

@@ -13,6 +13,13 @@ class TaskFile < ApplicationRecord
 
   before_save :remove_attachment
 
+  scope :all_files_for_task, lambda {|task|
+    joins("LEFT JOIN tests ON fileable_id = tests.id AND fileable_type = 'Test'")
+      .joins("LEFT JOIN model_solutions ON fileable_id = model_solutions.id AND fileable_type = 'ModelSolution'")
+      .joins("LEFT JOIN tasks ON fileable_id = tasks.id AND fileable_type = 'Task'")
+      .where('tasks.id = :task_id OR tests.task_id = :task_id OR model_solutions.task_id = :task_id', task_id: task.id)
+  }
+
   def full_file_name
     path.present? ? File.join(path.to_s, name) : name
   end

app/services/proforma_service/convert_proforma_task_to_task.rb

@@ -19,13 +19,13 @@ def execute
 
     def import_task
       @task.assign_attributes(
-        user: @user,
+        user:,
         title: @proforma_task.title,
         description:,
         internal_description: @proforma_task.internal_description,
         programming_language:,
         uuid:,
-        parent_uuid: @proforma_task.parent_uuid,
+        parent_uuid:,
         language: @proforma_task.language,
         meta_data: @proforma_task.meta_data,
 
@@ -39,6 +39,14 @@ def import_task
       )
     end
 
+    def parent_uuid
+      @proforma_task.parent_uuid || @task.parent_uuid
+    end
+
+    def user
+      @task.user || @user
+    end
+
     def uuid
       @task.uuid || @proforma_task.uuid
     end
@@ -52,15 +60,24 @@ def files
     end
 
     def file_from_proforma_file(proforma_task_file)
-      task_file = TaskFile.new(file_attributes(proforma_task_file))
+      task_file = (@task.id && TaskFile.all_files_for_task(@task).find_by(xml_id: proforma_task_file.id)) || TaskFile.new
+      task_file.assign_attributes(file_attributes(proforma_task_file))
+      attach_file_content(proforma_task_file, task_file)
+      task_file
+    end
+
+    def attach_file_content(proforma_task_file, task_file)
       if proforma_task_file.binary
-        task_file.attachment.attach(io: StringIO.new(proforma_task_file.content), filename: proforma_task_file.filename,
-          content_type: proforma_task_file.mimetype)
+        task_file.attachment.attach(
+          io: StringIO.new(proforma_task_file.content),
+          filename: proforma_task_file.filename,
+          content_type: proforma_task_file.mimetype
+        )
         task_file.use_attached_file = 'true'
       else
         task_file.content = proforma_task_file.content
+        task_file.use_attached_file = 'false'
       end
-      task_file
     end
 
     def xml_file_id(xml_id)
@@ -87,16 +104,17 @@ def file_attributes(proforma_task_file)
 
     def tests
       @proforma_task.tests.map do |test|
-        Test.new(
-          xml_id: test.id,
-          title: test.title,
-          description: test.description,
-          internal_description: test.internal_description,
-          test_type: test.test_type,
-          meta_data: test.meta_data,
-          configuration: test.configuration,
-          files: test.files.map {|task_file| file_from_proforma_file(task_file) }
-        )
+        @task.tests.find_or_initialize_by(xml_id: test.id).tap do |ch_test|
+          ch_test.assign_attributes(
+            title: test.title,
+            description: test.description,
+            internal_description: test.internal_description,
+            test_type: test.test_type,
+            meta_data: test.meta_data,
+            configuration: test.configuration,
+            files: test.files.map {|task_file| file_from_proforma_file(task_file) }
+          )
+        end
       end
     end
 
@@ -117,12 +135,13 @@ def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticCo
 
     def model_solutions
       @proforma_task.model_solutions.map do |model_solution|
-        ModelSolution.new(
-          xml_id: model_solution.id,
-          description: model_solution.description,
-          internal_description: model_solution.internal_description,
-          files: model_solution.files.map {|task_file| file_from_proforma_file(task_file) }
-        )
+        @task.model_solutions.find_or_initialize_by(xml_id: model_solution.id).tap do |ch_model_solution|
+          ch_model_solution.assign_attributes(
+            description: model_solution.description,
+            internal_description: model_solution.internal_description,
+            files: model_solution.files.map {|task_file| file_from_proforma_file(task_file) }
+          )
+        end
       end
     end
   end

spec/models/task_file_spec.rb

@@ -116,4 +116,33 @@
       it { is_expected.to be true }
     end
   end
+
+  describe '.all_files_for_task' do
+    subject { described_class.all_files_for_task(task) }
+
+    let(:task) { create(:task, files:, model_solutions:, tests:) }
+    let(:files) { build_list(:task_file, 1) }
+    let(:model_solutions) do
+      build_list(:model_solution, 2) do |model_solution|
+        model_solution.files = build_list(:task_file, 3)
+      end
+    end
+    let(:tests) do
+      build_list(:test, 4) do |test|
+        test.files = build_list(:task_file, 5)
+      end
+    end
+
+    it { is_expected.to have_attributes(size: 27) }
+
+    context 'with specific task_files' do
+      let(:model_solutions) { build_list(:model_solution, 1, files: model_solution_files) }
+      let(:model_solution_files) { build_list(:task_file, 3) }
+      let(:tests) { build_list(:test, 1, files: test_files) }
+      let(:test_files) { build_list(:task_file, 4) }
+
+      it { is_expected.to match_array(files + model_solution_files + test_files) }
+
+    end
+  end
 end

spec/services/proforma_service/convert_proforma_task_to_task_spec.rb

@@ -576,5 +576,31 @@
         end
       end
     end
+
+    context 'when proforma_task has been exported from task' do
+      let(:proforma_task) { ProformaService::ConvertTaskToProformaTask.call(task:) }
+      let(:task) { create(:task, files: build_list(:task_file, 1, :exportable), tests:, model_solutions:, title: 'title') }
+      let(:tests) { build_list(:test, 1, files: build_list(:task_file, 1, :exportable)) }
+      let(:model_solutions) { build_list(:model_solution, 1, files: build_list(:task_file, 1, :exportable)) }
+
+      it { is_expected.to be_an_equal_task_as task }
+
+      it 'reuses existing objects' do
+        expect(convert_to_task_service).to have_attributes(
+          id: task.id,
+          files: have(1).item.and(include(have_attributes(
+            id: task.files.first.id
+          ))),
+          model_solutions: have(1).item.and(include(have_attributes(
+            id: model_solutions.first.id,
+            files: include(have_attributes(id: model_solutions.first.files.first.id))
+          ))),
+          tests: have(1).item.and(include(have_attributes(
+            id: tests.first.id,
+            files: include(have_attributes(id: tests.first.files.first.id))
+          )))
+        )
+      end
+    end
   end
 end

spec/services/proforma_service/import_spec.rb

@@ -4,22 +4,22 @@
 
 RSpec.describe ProformaService::Import do
   describe '.new' do
-    subject(:import_service) { described_class.new(zip:, user:) }
+    subject(:imported_task) { described_class.new(zip:, user:) }
 
     let(:zip) { Tempfile.new('proforma_test_zip_file') }
     let(:user) { build(:user) }
 
     it 'assigns zip' do
-      expect(import_service.instance_variable_get(:@zip)).to be zip
+      expect(imported_task.instance_variable_get(:@zip)).to be zip
     end
 
     it 'assigns user' do
-      expect(import_service.instance_variable_get(:@user)).to be user
+      expect(imported_task.instance_variable_get(:@user)).to be user
     end
   end
 
   describe '#execute' do
-    subject(:import_service) { described_class.call(zip: zip_file, user: import_user) }
+    subject(:imported_task) { described_class.call(zip: zip_file, user: import_user) }
 
     let(:user) { build(:user) }
     let(:import_user) { user }
@@ -57,12 +57,16 @@
 
     it { is_expected.to be_an_equal_task_as task }
 
-    it 'sets the correct user as owner of the task' do
-      expect(import_service.user).to be user
+    it 'sets the uuid' do
+      expect(imported_task.uuid).not_to be_blank
     end
 
-    it 'sets the uuid' do
-      expect(import_service.uuid).not_to be_blank
+    context 'when task is imported by a different user, but is admin' do
+      let(:import_user) { build(:admin) }
+
+      it 'does not overwrite existing owner' do
+        expect(imported_task.user).to eql user
+      end
     end
 
     context 'when no task exists' do
@@ -71,18 +75,18 @@
       it { is_expected.to be_valid }
 
       it 'sets the correct user as owner of the task' do
-        expect(import_service.user).to be user
+        expect(imported_task.user).to be user
       end
 
       it 'sets the uuid' do
-        expect(import_service.uuid).not_to be_blank
+        expect(imported_task.uuid).not_to be_blank
       end
 
       context 'when task has a uuid' do
         let(:uuid) { SecureRandom.uuid }
 
         it 'sets the uuid' do
-          expect(import_service.uuid).to eql uuid
+          expect(imported_task.uuid).to eql uuid
         end
       end
     end
@@ -91,31 +95,31 @@
       let(:meta_data) { attributes_for(:task, :with_meta_data)[:meta_data] }
 
       it 'sets the meta_data' do
-        expect(import_service.meta_data).to eql meta_data
+        expect(imported_task.meta_data).to eql meta_data
       end
     end
 
     context 'when task has submission_restrictions' do
       let(:submission_restrictions) { attributes_for(:task, :with_submission_restrictions)[:submission_restrictions] }
 
       it 'sets the submission_restrictions' do
-        expect(import_service.submission_restrictions).to eql submission_restrictions
+        expect(imported_task.submission_restrictions).to eql submission_restrictions
       end
     end
 
     context 'when task has external_resources' do
       let(:external_resources) { attributes_for(:task, :with_external_resources)[:external_resources] }
 
       it 'sets the external_resources' do
-        expect(import_service.external_resources).to eql external_resources
+        expect(imported_task.external_resources).to eql external_resources
       end
     end
 
     context 'when task has grading_hints' do
       let(:grading_hints) { attributes_for(:task, :with_grading_hints)[:grading_hints] }
 
       it 'sets the grading_hints' do
-        expect(import_service.grading_hints).to eql grading_hints
+        expect(imported_task.grading_hints).to eql grading_hints
       end
     end
 
@@ -163,7 +167,7 @@
         let(:test_meta_data) { attributes_for(:test, :with_meta_data)[:meta_data] }
 
         it 'sets the meta_data' do
-          expect(import_service.tests.first.meta_data).to eql test_meta_data
+          expect(imported_task.tests.first.meta_data).to eql test_meta_data
         end
       end
 
@@ -173,7 +177,7 @@
         end
 
         it 'sets the configuration' do
-          expect(import_service.tests.first.configuration).to eql test_configuration
+          expect(imported_task.tests.first.configuration).to eql test_configuration
         end
       end
     end
@@ -196,19 +200,19 @@
       end
 
       it 'imports the tasks from zip containing multiple zips' do
-        expect(import_service).to all be_an(Task)
+        expect(imported_task).to all be_an(Task)
       end
 
       it 'imports the zip exactly how they were exported' do
-        expect(import_service).to all be_an_equal_task_as(task).or be_an_equal_task_as(task2)
+        expect(imported_task).to all be_an_equal_task_as(task).or be_an_equal_task_as(task2)
       end
 
       context 'when a task has files and tests' do
         let(:files) { build_list(:task_file, 2, :exportable) }
         let(:tests) { build_list(:test, 2, test_type: 'test_type') }
 
         it 'imports the zip exactly how the were exported' do
-          expect(import_service).to all be_an_equal_task_as(task).or be_an_equal_task_as(task2)
+          expect(imported_task).to all be_an_equal_task_as(task).or be_an_equal_task_as(task2)
         end
       end
     end
@@ -222,22 +226,22 @@
       end
 
       it 'creates a new task' do
-        expect(import_service.id).not_to be task.id
+        expect(imported_task.id).not_to be task.id
       end
     end
 
     context 'when task in zip has the same uuid and nothing has changed' do
       let(:uuid) { SecureRandom.uuid }
 
       it 'updates the old task' do
-        expect(import_service.id).to be task.id
+        expect(imported_task.id).to be task.id
       end
 
       context 'when another user imports the task' do
         let(:import_user) { create(:user) }
 
         it 'creates a new task' do
-          expect(import_service.id).not_to be task.id
+          expect(imported_task.id).not_to be task.id
         end
       end
     end