From 512185c61a2f9a664eda020c4ae2a71ecabd65fc Mon Sep 17 00:00:00 2001 From: Karol Date: Thu, 3 Oct 2024 00:04:22 +0200 Subject: [PATCH 1/3] 1320: add example tests for file overwriting --- .../convert_proforma_task_to_task.rb | 2 +- .../convert_proforma_task_to_task_spec.rb | 56 +++++++++---------- 2 files changed, 29 insertions(+), 29 deletions(-) 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 5e7a22426..42b7a2ba9 100644 --- a/app/services/proforma_service/convert_proforma_task_to_task.rb +++ b/app/services/proforma_service/convert_proforma_task_to_task.rb @@ -19,6 +19,7 @@ def execute private def import_task + upsert_files @proforma_task, @task @task.assign_attributes( user:, title: @proforma_task.title, @@ -37,7 +38,6 @@ def import_task tests:, model_solutions: ) - upsert_files @proforma_task, @task delete_removed_files 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 3ce29715c..7e9b88907 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 @@ -579,7 +579,7 @@ context 'when proforma_task has been exported from task' do let(:proforma_task) { ProformaService::ConvertTaskToProformaTask.call(task:) } - let(:task) { create(:task, files: task_files, tests:, model_solutions:, title: 'title') } + let!(:task) { create(:task, files: task_files, tests:, model_solutions:, title: 'title') } let(:task_files) { build_list(:task_file, 1, :exportable, internal_description: 'original task file') } let(:tests) { build_list(:test, 1, files: test_files) } let(:test_files) { build_list(:task_file, 1, :exportable, internal_description: 'original test file') } @@ -596,11 +596,11 @@ ))), 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)) + files: have(1).item.and(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)) + files: have(1).item.and(include(have_attributes(id: tests.first.files.first.id))) ))) ) end @@ -654,29 +654,29 @@ task_file = proforma_task.files.first test_file = proforma_task.tests.first.files.first model_solution_file = proforma_task.model_solutions.first.files.first - proforma_task.files = [model_solution_file] - proforma_task.tests.first.files = [task_file] - proforma_task.model_solutions.first.files = [test_file] + proforma_task.files = [test_file] + proforma_task.tests = [ProformaXML::Test.new(title: 'replacement Test', id: 987_654_325, files: [model_solution_file])] + proforma_task.model_solutions.first.files = [task_file] end it 'imports taskfiles correctly' do expect(convert_to_task_service.files.first).to have_attributes( - id: model_solution_files.first.id, - internal_description: model_solution_files.first.internal_description + id: test_files.first.id, + internal_description: test_files.first.internal_description ) end it 'imports testfiles correctly' do expect(convert_to_task_service.tests.first.files.first).to have_attributes( - id: task_files.first.id, - internal_description: task_files.first.internal_description + id: model_solution_files.first.id, + internal_description: model_solution_files.first.internal_description ) end it 'imports msfiles correctly' do expect(convert_to_task_service.model_solutions.first.files.first).to have_attributes( - id: test_files.first.id, - internal_description: test_files.first.internal_description + id: task_files.first.id, + internal_description: task_files.first.internal_description ) end @@ -684,15 +684,15 @@ expect(convert_to_task_service).to have_attributes( id: task.id, files: have(1).item.and(include(have_attributes( - id: model_solution_files.first.id - ))), + id: test_files.first.id + ))), model_solutions: have(1).item.and(include(have_attributes( id: model_solutions.first.id, - files: include(have_attributes(id: test_files.first.id)) + files: have(1).item.and(include(have_attributes(id: task_files.first.id))) ))), tests: have(1).item.and(include(have_attributes( - id: tests.first.id, - files: include(have_attributes(id: task_files.first.id)) + id: 987_654_325, + files: have(1).item.and(include(have_attributes(id: model_solution_files.first.id))) ))) ) end @@ -705,22 +705,22 @@ it 'imports taskfiles correctly' do expect(task.files.first).to have_attributes( - id: model_solution_files.first.id, - internal_description: model_solution_files.first.internal_description + id: test_files.first.id, + internal_description: test_files.first.internal_description ) end it 'imports testfiles correctly' do expect(task.tests.first.files.first).to have_attributes( - id: task_files.first.id, - internal_description: task_files.first.internal_description + id: model_solution_files.first.id, + internal_description: model_solution_files.first.internal_description ) end it 'imports msfiles correctly' do expect(task.model_solutions.first.files.first).to have_attributes( - id: test_files.first.id, - internal_description: test_files.first.internal_description + id: task_files.first.id, + internal_description: task_files.first.internal_description ) end @@ -728,15 +728,15 @@ expect(task).to have_attributes( id: task.id, files: have(1).item.and(include(have_attributes( - id: model_solution_files.first.id - ))), + id: test_files.first.id + ))), model_solutions: have(1).item.and(include(have_attributes( id: model_solutions.first.id, - files: include(have_attributes(id: test_files.first.id)) + files: have(1).item.and(include(have_attributes(id: task_files.first.id))) ))), tests: have(1).item.and(include(have_attributes( - id: tests.first.id, - files: include(have_attributes(id: task_files.first.id)) + id: 987_654_325, + files: have(1).item.and(include(have_attributes(id: model_solution_files.first.id))) ))) ) end From cc487bdb571f5ce4cda4cf42132a76f85e72ef11 Mon Sep 17 00:00:00 2001 From: Karol Date: Mon, 2 Dec 2024 23:32:22 +0100 Subject: [PATCH 2/3] 1320: refactor convert proforma to task service to use persistence for file-consistency --- .../convert_proforma_task_to_task.rb | 92 +++++++++++++++---- app/services/proforma_service/import_task.rb | 5 +- .../convert_proforma_task_to_task_spec.rb | 68 +++++++++----- spec/services/proforma_service/import_spec.rb | 5 +- 4 files changed, 123 insertions(+), 47 deletions(-) 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 42b7a2ba9..4883ad6ed 100644 --- a/app/services/proforma_service/convert_proforma_task_to_task.rb +++ b/app/services/proforma_service/convert_proforma_task_to_task.rb @@ -7,19 +7,19 @@ 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 def execute - import_task + ActiveRecord::Base.transaction do + import_task + end @task end private def import_task - upsert_files @proforma_task, @task @task.assign_attributes( user:, title: @proforma_task.title, @@ -33,12 +33,51 @@ def import_task submission_restrictions: @proforma_task.submission_restrictions, external_resources: @proforma_task.external_resources, - grading_hints: @proforma_task.grading_hints, - - tests:, - model_solutions: + grading_hints: @proforma_task.grading_hints ) - delete_removed_files + Pundit.authorize @user, @task, :create? + + @task.save! + + manage_objects + end + + def manage_objects + # delete files that do not exist in imported task + unreferenced_files.each(&:destroy) + @task.reload + # Move only relocated files to the task, avoiding updates to the updated_at of untouched files. + move_relocated_files_to_task + set_tests + set_model_solutions + + @task.reload + upsert_files @proforma_task, @task + delete_removed_objects + @task.save! + end + + def move_relocated_files_to_task + @task.files = @task.all_files(cached: false).filter do |file| + # Move files to the task if they belong directly to it or if the parent's xml_id differs from the current parent's xml_id. + parent_id(file.xml_id) == 'task' || (file.fileable.respond_to?(:xml_id) && file.fileable.xml_id) != parent_id(file.xml_id) + end + end + + def parent_id(xml_id) + [@proforma_task, *@proforma_task.model_solutions, *@proforma_task.tests].find do |object| + object.files.any? {|file| file.id == xml_id } + end.then {|object| object.respond_to?(:id) ? object.id : 'task' } + end + + def all_proforma_files + [@proforma_task.files + @proforma_task.tests.map(&:files) + @proforma_task.model_solutions.map(&:files)].flatten + end + + def unreferenced_files + @task.all_files.reject do |f| + all_proforma_files.map {|pf| pf.id.to_s }.include?(f.xml_id) + end end def parent_uuid @@ -62,7 +101,7 @@ def upsert_files(collection, 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 = (@file_xml_ids.exclude?(proforma_task_file.id) && ch_record_for(@task.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 @@ -105,9 +144,10 @@ def file_attributes(proforma_task_file, fileable) } end - def tests - @proforma_task.tests.map do |test| - ch_test = ch_record_for(@task.tests, test.id) || Test.new(xml_id: test.id) + def set_tests + @proforma_task.tests.each do |test| + ch_test = find_or_initialize_ch_test test + ch_test.task = @task upsert_files test, ch_test ch_test.assign_attributes( title: test.title, @@ -117,12 +157,12 @@ def tests meta_data: test.meta_data, configuration: test.configuration ) - ch_test + ch_test.save! end end - def object_files(object) - object.files.map {|file| files.delete(file.id) } + def find_or_initialize_ch_test(test) + ch_record_for(@task.tests, test.id) || Test.new(xml_id: test.id) end def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity @@ -136,15 +176,16 @@ def programming_language # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticCo end end - def model_solutions - @proforma_task.model_solutions.map do |model_solution| + def set_model_solutions + @proforma_task.model_solutions.each do |model_solution| ch_model_solution = ch_record_for(@task.model_solutions, model_solution.id) || ModelSolution.new(xml_id: model_solution.id) + ch_model_solution.task = @task 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 + ch_model_solution.save! end end @@ -152,8 +193,19 @@ 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) + def delete_removed_objects + delete_removed_tests + delete_removed_model_solutions + end + + def delete_removed_tests + remaining_test_ids = @proforma_task.tests.map {|t| t.id.to_s } + @task.tests.reject {|test| remaining_test_ids.include? test.xml_id }.each(&:destroy) + end + + def delete_removed_model_solutions + remaining_model_solution_ids = @proforma_task.model_solutions.map {|ms| ms.id.to_s } + @task.model_solutions.reject {|model_solution| remaining_model_solution_ids.include? model_solution.xml_id }.each(&:destroy) end end end diff --git a/app/services/proforma_service/import_task.rb b/app/services/proforma_service/import_task.rb index ed189d6da..e45942c1e 100644 --- a/app/services/proforma_service/import_task.rb +++ b/app/services/proforma_service/import_task.rb @@ -9,10 +9,7 @@ def initialize(proforma_task:, user:) end def execute - task = ConvertProformaTaskToTask.call(proforma_task: @proforma_task, user: @user, task: base_task) - Pundit.authorize @user, task, :create? - task.save! - task + ConvertProformaTaskToTask.call(proforma_task: @proforma_task, user: @user, task: base_task) end private 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 7e9b88907..84452599d 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 @@ -24,7 +24,7 @@ end describe '#execute' do - subject(:convert_to_task_service) { described_class.call(proforma_task:, user:, task:) } + subject(:convert_to_task_service) { described_class.call(proforma_task:, user:, task:).reload } let(:proforma_task) do ProformaXML::Task.new( @@ -237,8 +237,9 @@ end it 'creates files with correct attributes' do - expect(convert_to_task_service.tests[1].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.merge('xml_id' => 'id-2')) - expect(convert_to_task_service.tests[2].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.merge('xml_id' => 'id-3')) + attribute_exceptions = %w[id created_at fileable_id updated_at] + expect(convert_to_task_service.tests[1].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.except(*attribute_exceptions).merge('xml_id' => 'id-2')) + expect(convert_to_task_service.tests[2].files[0]).to have_attributes(convert_to_task_service.tests[0].files[0].attributes.except(*attribute_exceptions).merge('xml_id' => 'id-3')) end it 'creates separate copies of referenced file' do @@ -321,10 +322,12 @@ internal_description: 'internal_description', test_type: 'test_type', files: test_files, - meta_data: test_meta_data + meta_data: test_meta_data, + configuration: ) end + let(:configuration) {} let(:test_meta_data) {} let(:test_files) { [test_file] } let(:test_file) do @@ -402,7 +405,7 @@ end context 'when test has custom configuration' do - let(:test) { build(:test, :with_unittest) } + let(:configuration) { attributes_for(:test, :with_unittest)[:configuration] } it 'creates a test with the supplied test configuration' do expect(convert_to_task_service.tests.first).to have_attributes(configuration: test.configuration) @@ -410,7 +413,7 @@ end context 'when test has multiple custom configuration' do - let(:test) { build(:test, :with_multiple_custom_configurations) } + let(:configuration) { attributes_for(:test, :with_multiple_custom_configurations)[:configuration] } it 'creates a test with the supplied test configurations' do expect(convert_to_task_service.tests.first).to have_attributes(configuration: test.configuration) @@ -420,7 +423,11 @@ context 'when proforma_task has multiple tests' do let(:tests) { [test, test2] } let(:test2) do - ProformaXML::Test.new(files: test_files2) + ProformaXML::Test.new( + id: 'test_file_id', + title: 'wild-title', + files: test_files2 + ) end let(:test_files2) { [test_file2] } let(:test_file2) do @@ -473,6 +480,7 @@ let(:task) do create( :task, + user:, title: 'task-title', description: 'task-description', internal_description: 'task-internal_description' @@ -579,7 +587,7 @@ context 'when proforma_task has been exported from task' do let(:proforma_task) { ProformaService::ConvertTaskToProformaTask.call(task:) } - let!(:task) { create(:task, files: task_files, tests:, model_solutions:, title: 'title') } + let!(:task) { create(:task, user:, files: task_files, tests:, model_solutions:, title: 'title') } let(:task_files) { build_list(:task_file, 1, :exportable, internal_description: 'original task file') } let(:tests) { build_list(:test, 1, files: test_files) } let(:test_files) { build_list(:task_file, 1, :exportable, internal_description: 'original test file') } @@ -605,51 +613,67 @@ ) end + it 'does not update updated_at of unchanged files' do + task_file_updated_at = task_files.first.updated_at + test_file_updated_at = test_files.first.updated_at + ms_file_update_at = model_solution_files.first.updated_at + + expect(convert_to_task_service).to have_attributes( + files: have(1).item.and(include(have_attributes( + updated_at: task_file_updated_at + ))), + model_solutions: have(1).item.and(include(have_attributes( + files: have(1).item.and(include(have_attributes(updated_at: ms_file_update_at))) + ))), + tests: have(1).item.and(include(have_attributes( + id: tests.first.id, + files: have(1).item.and(include(have_attributes(updated_at: test_file_updated_at))) + ))) + ) + end + context 'when files have been deleted' do context 'when task files have been deleted' do - before { task.files = [] } + before { proforma_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 + convert_to_task_service + expect(task.reload.files).to be_empty end end context 'when test files have been deleted' do - before { task.tests.first.files = [] } + before { proforma_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 + convert_to_task_service 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 = [] } + before { proforma_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 + convert_to_task_service expect(task.model_solutions.first.files).to be_empty end end end - context 'when files have been move around' do + context 'when files have been moved around' do before do task_file = proforma_task.files.first test_file = proforma_task.tests.first.files.first @@ -691,7 +715,7 @@ files: have(1).item.and(include(have_attributes(id: task_files.first.id))) ))), tests: have(1).item.and(include(have_attributes( - id: 987_654_325, + xml_id: '987654325', files: have(1).item.and(include(have_attributes(id: model_solution_files.first.id))) ))) ) @@ -699,7 +723,7 @@ context 'when imported task is persisted' do before do - convert_to_task_service.save + convert_to_task_service.save! task.reload end @@ -735,7 +759,7 @@ files: have(1).item.and(include(have_attributes(id: task_files.first.id))) ))), tests: have(1).item.and(include(have_attributes( - id: 987_654_325, + xml_id: '987654325', files: have(1).item.and(include(have_attributes(id: model_solution_files.first.id))) ))) ) diff --git a/spec/services/proforma_service/import_spec.rb b/spec/services/proforma_service/import_spec.rb index 65abdfebf..438a2dd7b 100644 --- a/spec/services/proforma_service/import_spec.rb +++ b/spec/services/proforma_service/import_spec.rb @@ -53,6 +53,7 @@ before do zip_file.write(exporter) zip_file.rewind + task.reload end it { is_expected.to be_an_equal_task_as task } @@ -75,7 +76,7 @@ it { is_expected.to be_valid } it 'sets the correct user as owner of the task' do - expect(imported_task.user).to be user + expect(imported_task.user).to eq user end it 'sets the uuid' do @@ -199,6 +200,8 @@ user:) end + before { task2.reload } + it 'imports the tasks from zip containing multiple zips' do expect(imported_task).to all be_an(Task) end From 2952b5631a9b301a02893d750e449fd20f781c63 Mon Sep 17 00:00:00 2001 From: Karol Date: Mon, 27 Jan 2025 20:10:52 +0100 Subject: [PATCH 3/3] 1320: Remove autosave causing unnecessary N+1 queries --- app/models/concerns/file_concern.rb | 2 +- app/models/model_solution.rb | 2 +- app/models/task_file.rb | 2 +- app/models/test.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/file_concern.rb b/app/models/concerns/file_concern.rb index 515a0aba5..73384b5f9 100644 --- a/app/models/concerns/file_concern.rb +++ b/app/models/concerns/file_concern.rb @@ -4,7 +4,7 @@ module FileConcern extend ActiveSupport::Concern included do - has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy, autosave: true, inverse_of: :fileable + has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy, inverse_of: :fileable accepts_nested_attributes_for :files, allow_destroy: true end diff --git a/app/models/model_solution.rb b/app/models/model_solution.rb index d15fd53e6..7f96c2d38 100644 --- a/app/models/model_solution.rb +++ b/app/models/model_solution.rb @@ -5,7 +5,7 @@ class ModelSolution < ApplicationRecord include TransferValues include ParentValidation - belongs_to :task, autosave: true, inverse_of: :model_solutions + belongs_to :task, inverse_of: :model_solutions belongs_to :parent, class_name: 'ModelSolution', optional: true has_many :files, as: :fileable, class_name: 'TaskFile', dependent: :destroy accepts_nested_attributes_for :files, allow_destroy: true diff --git a/app/models/task_file.rb b/app/models/task_file.rb index 9536b377f..1c0b87d85 100644 --- a/app/models/task_file.rb +++ b/app/models/task_file.rb @@ -4,7 +4,7 @@ class TaskFile < ApplicationRecord include ParentValidation include TransferValues - belongs_to :fileable, polymorphic: true, autosave: true, inverse_of: :files + belongs_to :fileable, polymorphic: true, inverse_of: :files belongs_to :parent, class_name: 'TaskFile', optional: true has_one_attached :attachment validates :name, presence: true diff --git a/app/models/test.rb b/app/models/test.rb index 808b8261e..aaf1ac179 100644 --- a/app/models/test.rb +++ b/app/models/test.rb @@ -5,7 +5,7 @@ class Test < ApplicationRecord include TransferValues include ParentValidation - belongs_to :task, autosave: true, inverse_of: :tests + belongs_to :task, inverse_of: :tests belongs_to :testing_framework, optional: true belongs_to :parent, class_name: 'Test', optional: true validates :title, presence: true