diff --git a/app/models/concerns/file_concern.rb b/app/models/concerns/file_concern.rb new file mode 100644 index 000000000..19c9724a3 --- /dev/null +++ b/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 diff --git a/app/models/model_solution.rb b/app/models/model_solution.rb index cbb98b50d..19f7e4891 100644 --- a/app/models/model_solution.rb +++ b/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} diff --git a/app/models/task.rb b/app/models/task.rb index f6eed8f34..0f0801f0b 100644 --- a/app/models/task.rb +++ b/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 diff --git a/app/models/task_file.rb b/app/models/task_file.rb index 387efed40..903718c6e 100644 --- a/app/models/task_file.rb +++ b/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 diff --git a/app/models/test.rb b/app/models/test.rb index 3da6e5d15..adf5dad3e 100644 --- a/app/models/test.rb +++ b/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} 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 38bcafba6..5e7a22426 100644 --- a/app/services/proforma_service/convert_proforma_task_to_task.rb +++ b/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 diff --git a/app/views/tasks/_nested_file_form.html.slim b/app/views/tasks/_nested_file_form.html.slim index 205b2afd4..b21e8cb9b 100644 --- a/app/views/tasks/_nested_file_form.html.slim +++ b/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 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 a62b568c6..3ce29715c 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 @@ -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