Allow moving ProFormA files between fileables

We recently discovered an edge case where a moved file was not updated correctly. At first, it got removed and recreated, and later on only removed. While these changes look pretty difficult, they are the best and only working approach I could find to fix the problem ;). For now, I'd suggest to go with this solution. See #1606 for more details.

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

app/models/task_file.rb

@@ -1,7 +1,7 @@
 # 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

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
 
@@ -34,9 +35,10 @@ 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
@@ -55,13 +57,13 @@ 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 file_from_proforma_file(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))
+    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
@@ -88,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
       {
@@ -99,22 +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|
-        @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
+        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
+        )
+        ch_test
       end
     end
 
@@ -135,14 +138,22 @@ def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticCo
 
     def model_solutions
       @proforma_task.model_solutions.map do |model_solution|
-        @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
+        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
+        )
+        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/services/proforma_service/convert_proforma_task_to_task_spec.rb

@@ -605,6 +605,50 @@
         )
       end
 
+      context 'when files have been deleted' do
+        context 'when task files have been deleted' do
+          before { 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
+          end
+        end
+
+        context 'when test files have been deleted' do
+          before { 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
+            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 = [] }
+
+          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
+            expect(task.model_solutions.first.files).to be_empty
+          end
+        end
+      end
+
       context 'when files have been move around' do
         before do
           task_file = proforma_task.files.first