From c6f4f5ac78d5146c9b9ff1a215fb77ed5e13263f Mon Sep 17 00:00:00 2001
From: Karol <git@koehn.pro>
Date: Wed, 21 Aug 2024 21:57:39 +0200
Subject: [PATCH] refactor import to reuse task_files based on xml_id

---
 app/models/task.rb                            |  6 +-
 app/models/task_file.rb                       |  4 +-
 .../convert_proforma_task_to_task.rb          | 63 ++++++++++++-------
 spec/models/task_spec.rb                      | 46 ++++++++++++++
 .../convert_proforma_task_to_task_spec.rb     | 26 ++++++++
 spec/services/proforma_service/import_spec.rb | 50 ++++++++-------
 6 files changed, 146 insertions(+), 49 deletions(-)

diff --git a/app/models/task.rb b/app/models/task.rb
index 28fe2d2df..604f12a89 100644
--- a/app/models/task.rb
+++ b/app/models/task.rb
@@ -151,8 +151,10 @@ def rating_star
     (average_rating * 2).round / 2.0
   end
 
-  def all_files
-    (files + tests.map(&:files) + model_solutions.map(&:files)).flatten
+  def all_files(cached: true)
+    return @all_files if cached && defined?(@all_files)
+
+    @all_files = (files + tests.map(&:files) + model_solutions.map(&:files)).flatten
   end
 
   def label_names=(label_names)
diff --git a/app/models/task_file.rb b/app/models/task_file.rb
index b79918597..d2d5537c5 100644
--- a/app/models/task_file.rb
+++ b/app/models/task_file.rb
@@ -7,7 +7,7 @@ class TaskFile < ApplicationRecord
   validates :name, presence: true
   validates :attachment, presence: true, if: -> { use_attached_file == 'true' }, on: :force_validations
   validates :xml_id, presence: true
-  validate :unique_xml_id, if: -> { !fileable.nil? }
+  validate :unique_xml_id, if: -> { !fileable.nil? && xml_id_changed? }
 
   attr_accessor :use_attached_file, :file_marked_for_deletion
 
@@ -50,7 +50,7 @@ def remove_attachment
 
   def unique_xml_id
     task = fileable.is_a?(Task) ? fileable : fileable.task
-    xml_ids = (task.all_files - [self]).map(&:xml_id)
+    xml_ids = (task.all_files(cached: false) - [self]).map(&:xml_id)
     errors.add(:xml_id, :not_unique) if xml_ids.include? xml_id
   end
 end
diff --git a/app/services/proforma_service/convert_proforma_task_to_task.rb b/app/services/proforma_service/convert_proforma_task_to_task.rb
index 0f97597bb..38bcafba6 100644
--- a/app/services/proforma_service/convert_proforma_task_to_task.rb
+++ b/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&.all_files&.select {|file| file.xml_id == proforma_task_file.id }&.first || 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
diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb
index ea8f22102..67bd9aed5 100644
--- a/spec/models/task_spec.rb
+++ b/spec/models/task_spec.rb
@@ -270,4 +270,50 @@
       end
     end
   end
+
+  describe '#all_files' do
+    subject(:all_files) { task.all_files }
+
+    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
+
+    context 'when all_files is called again' do
+      before do
+        all_files
+        task.tests.destroy_all
+        task.model_solutions.destroy_all
+      end
+
+      it 'returns the cached result' do
+        expect(all_files).to have_attributes(size: 27)
+      end
+
+      context 'with cache: false' do
+        it 'returns the correct result' do
+          expect(task.all_files(cached: false)).to have_attributes(size: 1)
+        end
+      end
+    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
index 6c8cf4f7c..4eb88211d 100644
--- a/spec/services/proforma_service/convert_proforma_task_to_task_spec.rb
+++ b/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
diff --git a/spec/services/proforma_service/import_spec.rb b/spec/services/proforma_service/import_spec.rb
index 09da8f5a4..65abdfebf 100644
--- a/spec/services/proforma_service/import_spec.rb
+++ b/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,7 +95,7 @@
       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
 
@@ -99,7 +103,7 @@
       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
 
@@ -107,7 +111,7 @@
       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
 
@@ -115,7 +119,7 @@
       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,11 +200,11 @@
       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
@@ -208,7 +212,7 @@
         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,7 +226,7 @@
       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
 
@@ -230,14 +234,14 @@
       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