refactor import to reuse task_files based on xml_id

app/models/concerns/file_concern.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module FileConcern
+  extend ActiveSupport::Concern
+
+  included do
+    has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy, autosave: true, inverse_of: :fileable
+    accepts_nested_attributes_for :files, allow_destroy: true
+  end
+
+  def files
+    # We need to overwrite the `files` association method to allow moving files between the polymorphic associations.
+    # In the default case, a moved file would still be associated with the old record until saved (despite a correct inverse relationship).
+    # Therefore, we need to filter the files by the fileable type and only return those are belong to the current record.
+    # To minimize the impact and still allow method chaining, we filter only files if the association is already loaded.
+    # See https://github.com/openHPI/codeharbor/pull/1606 for more details.
+    return super unless association(:files).loaded?
+
+    association(:files).reader.records.filter {|file| file.fileable == self }
+  end
+end

app/models/model_solution.rb

@@ -1,9 +1,8 @@
 # frozen_string_literal: true
 
 class ModelSolution < ApplicationRecord
-  belongs_to :task
-  has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy
-  accepts_nested_attributes_for :files, allow_destroy: true
+  include FileConcern
+  belongs_to :task, autosave: true, inverse_of: :model_solutions
   validates :xml_id, presence: true
   validates :xml_id, uniqueness: {scope: :task_id}
 

app/models/task.rb

@@ -3,6 +3,7 @@
 require 'nokogiri'
 require 'zip'
 class Task < ApplicationRecord
+  include FileConcern
   acts_as_taggable_on :state
 
   before_validation :lowercase_language
@@ -15,16 +16,14 @@ class Task < ApplicationRecord
   validates :language, format: {with: /\A[a-zA-Z]{1,8}(-[a-zA-Z0-9]{1,8})*\z/, message: :not_de_or_us}
   validate :primary_language_tag_in_iso639?
 
-  has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy
-
   has_many :group_tasks, dependent: :destroy
   has_many :groups, through: :group_tasks
 
   has_many :task_labels, dependent: :destroy
   has_many :labels, through: :task_labels
 
-  has_many :tests, dependent: :destroy
-  has_many :model_solutions, dependent: :destroy
+  has_many :tests, dependent: :destroy, inverse_of: :task
+  has_many :model_solutions, dependent: :destroy, inverse_of: :task
 
   has_many :collection_tasks, dependent: :destroy
   has_many :collections, through: :collection_tasks
@@ -36,7 +35,6 @@ class Task < ApplicationRecord
   belongs_to :programming_language, optional: true
   belongs_to :license, optional: true
 
-  accepts_nested_attributes_for :files, allow_destroy: true
   accepts_nested_attributes_for :tests, allow_destroy: true
   accepts_nested_attributes_for :model_solutions, allow_destroy: true
   accepts_nested_attributes_for :group_tasks, allow_destroy: true
@@ -161,8 +159,10 @@ def overall_rating
     average_rating[:overall_rating]
   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)

app/models/task_file.rb

@@ -1,15 +1,15 @@
 # frozen_string_literal: true
 
 class TaskFile < ApplicationRecord
-  belongs_to :fileable, polymorphic: true
+  belongs_to :fileable, polymorphic: true, autosave: true, inverse_of: :files
 
   has_one_attached :attachment
   validates :name, presence: true
   validates :attachment, presence: true, if: -> { use_attached_file == 'true' }, on: :force_validations
   validates :xml_id, presence: true
   validates :visible, inclusion: {in: %w[yes no delayed]}
   validates :used_by_grader, inclusion: {in: [true, false]}
-  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
 
@@ -52,7 +52,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

app/models/test.rb

@@ -1,10 +1,9 @@
 # frozen_string_literal: true
 
 class Test < ApplicationRecord
-  belongs_to :task
+  include FileConcern
+  belongs_to :task, autosave: true, inverse_of: :tests
   belongs_to :testing_framework, 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 :xml_id, uniqueness: {scope: :task_id}

app/services/proforma_service/convert_proforma_task_to_task.rb

@@ -7,6 +7,7 @@ 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
 
@@ -19,13 +20,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,
 
@@ -34,9 +35,18 @@ def import_task
         grading_hints: @proforma_task.grading_hints,
 
         tests:,
-        model_solutions:,
-        files:
+        model_solutions:
       )
+      upsert_files @proforma_task, @task
+      delete_removed_files
+    end
+
+    def parent_uuid
+      @proforma_task.parent_uuid || @task.parent_uuid
+    end
+
+    def user
+      @task.user || @user
     end
 
     def uuid
@@ -47,20 +57,29 @@ def description
       Kramdown::Document.new(@proforma_task.description || '', html_to_native: true, line_width: -1).to_kramdown.strip
     end
 
-    def files
-      @proforma_task.files.map {|task_file| file_from_proforma_file(task_file) }
+    def upsert_files(collection, fileable)
+      collection.files.map {|task_file| upsert_file_from_proforma_file(task_file, 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.assign_attributes(file_attributes(proforma_task_file, fileable))
+      attach_file_content(proforma_task_file, task_file)
+      task_file
     end
 
-    def file_from_proforma_file(proforma_task_file)
-      task_file = TaskFile.new(file_attributes(proforma_task_file))
+    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)
@@ -71,7 +90,7 @@ def xml_file_id(xml_id)
       "#{xml_id}-#{offset}"
     end
 
-    def file_attributes(proforma_task_file)
+    def file_attributes(proforma_task_file, fileable)
       xml_id = xml_file_id proforma_task_file.id
       @file_xml_ids << xml_id
       {
@@ -82,21 +101,23 @@ def file_attributes(proforma_task_file)
         usage_by_lms: proforma_task_file.usage_by_lms,
         mime_type: proforma_task_file.mimetype,
         xml_id:,
+        fileable:,
       }
     end
 
     def tests
       @proforma_task.tests.map do |test|
-        Test.new(
-          xml_id: test.id,
+        ch_test = ch_record_for(@task.tests, test.id) || Test.new(xml_id: test.id)
+        upsert_files test, 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) }
+          configuration: test.configuration
         )
+        ch_test
       end
     end
 
@@ -117,13 +138,22 @@ 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,
+        ch_model_solution = ch_record_for(@task.model_solutions, model_solution.id) || ModelSolution.new(xml_id: model_solution.id)
+        upsert_files model_solution, 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) }
+          internal_description: model_solution.internal_description
         )
+        ch_model_solution
       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)
+    end
   end
 end

app/views/tasks/_nested_file_form.html.slim

@@ -1,5 +1,5 @@
 .field-element
-  = f.nested_fields_for :files do |file|
+  = f.nested_fields_for :files, class_name: TaskFile do |file|
     .show-table.exercise-show
       - if file.object.new_record?
         - display_container_class = '' # rubocop:disable Lint/UselessAssignment

spec/models/task_spec.rb

@@ -284,4 +284,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