From 9d9ac2311d951932f72c76436258fad912626c9f Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Wed, 11 Sep 2024 19:26:54 +0200 Subject: [PATCH] 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 https://github.com/openHPI/codeharbor/pull/1606 for more details. --- app/models/concerns/file_concern.rb | 21 +++++++ app/models/model_solution.rb | 5 +- app/models/task.rb | 8 +-- app/models/task_file.rb | 2 +- app/models/test.rb | 5 +- .../convert_proforma_task_to_task.rb | 63 +++++++++++-------- app/views/tasks/_nested_file_form.html.slim | 2 +- .../convert_proforma_task_to_task_spec.rb | 44 +++++++++++++ 8 files changed, 111 insertions(+), 39 deletions(-) create mode 100644 app/models/concerns/file_concern.rb 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