From ac0fe42f0e2794476b2712fe6231797e952f8f14 Mon Sep 17 00:00:00 2001 From: Karol Date: Mon, 16 Sep 2024 21:16:57 +0200 Subject: [PATCH] basic functionality of reusing files based on xml_id. Use xml_id_path to recreate proforma structure with tests and model_solutions containing multiple files --- .../convert_exercise_to_task.rb | 95 ++++++++++++------- .../convert_task_to_exercise.rb | 39 +++++--- app/services/proforma_service/import.rb | 8 +- .../20240903204319_add_xml_path_to_files.rb | 7 ++ db/schema.rb | 3 +- .../convert_exercise_to_task_spec.rb | 79 +++++++++++++-- .../convert_task_to_exercise_spec.rb | 40 ++++++-- spec/services/proforma_service/import_spec.rb | 16 ++-- 8 files changed, 206 insertions(+), 81 deletions(-) create mode 100644 db/migrate/20240903204319_add_xml_path_to_files.rb diff --git a/app/services/proforma_service/convert_exercise_to_task.rb b/app/services/proforma_service/convert_exercise_to_task.rb index 3eba296f6..03bad6d8c 100644 --- a/app/services/proforma_service/convert_exercise_to_task.rb +++ b/app/services/proforma_service/convert_exercise_to_task.rb @@ -77,62 +77,86 @@ def uuid end def model_solutions - @exercise.files.filter {|file| file.role == 'reference_implementation' }.map do |file| + model_solutions_files = @exercise.files.filter {|file| file.role == 'reference_implementation' }.group_by {|file| xml_id_from_file(file).first } + model_solutions_files.map do |xml_id, files| ProformaXML::ModelSolution.new( - id: "ms-#{file.id}", - files: model_solution_file(file) + id: xml_id, + files: files.map {|file| model_solution_file(file) } ) end end def model_solution_file(file) - [ - task_file(file).tap do |ms_file| - ms_file.used_by_grader = false - ms_file.usage_by_lms = 'display' - end, - ] + task_file(file).tap do |ms_file| + ms_file.used_by_grader = false + ms_file.usage_by_lms = 'display' + end end def tests - @exercise.files.filter(&:teacher_defined_assessment?).map do |file| + @exercise.files.filter(&:teacher_defined_assessment?).group_by {|file| xml_id_from_file(file).first }.map do |xml_id, files| ProformaXML::Test.new( - id: file.id, - title: file.name, - files: test_file(file), - meta_data: test_meta_data(file) + id: xml_id, + title: files.first.name, + files: files.map {|file| test_file(file) }, + meta_data: test_meta_data(files) ) end end - def test_meta_data(file) + def xml_id_from_file(file) + type = if file.teacher_defined_assessment? + 'test' + elsif file.reference_implementation? + 'ms' + else + 'file' + end + xml_id_path_parts = file.xml_id_path&.split('/') + xml_id_path = [] + + xml_id_path << (xml_id_path_parts&.first || "co-#{type}-#{file.id}") unless type == 'file' + xml_id_path << (xml_id_path_parts&.last || file.id) + + xml_id_path + end + + def test_meta_data(files) { '@@order' => %w[test-meta-data], 'test-meta-data' => { - '@@order' => %w[CodeOcean:feedback-message CodeOcean:weight CodeOcean:hidden-feedback], + '@@order' => %w[CodeOcean:test-file], '@xmlns' => {'CodeOcean' => 'codeocean.openhpi.de'}, - 'CodeOcean:feedback-message' => { - '@@order' => %w[$1], - '$1' => file.feedback_message, - }, - 'CodeOcean:weight' => { - '@@order' => %w[$1], - '$1' => file.weight, - }, - 'CodeOcean:hidden-feedback' => { - '@@order' => %w[$1], - '$1' => file.hidden_feedback, - }, + 'CodeOcean:test-file' => files.to_h do |file| + [ + "CodeOcean:#{xml_id_from_file(file).last}", { + '@@order' => %w[CodeOcean:feedback-message CodeOcean:weight CodeOcean:hidden-feedback], + '@xmlns' => {'CodeOcean' => 'codeocean.openhpi.de'}, + '@id' => xml_id_from_file(file).last, + '@name' => file.name, + 'CodeOcean:feedback-message' => { + '@@order' => %w[$1], + '$1' => file.feedback_message, + }, + 'CodeOcean:weight' => { + '@@order' => %w[$1], + '$1' => file.weight, + }, + 'CodeOcean:hidden-feedback' => { + '@@order' => %w[$1], + '$1' => file.hidden_feedback, + }, + } + ] + end, }, } end def test_file(file) - [ - task_file(file).tap do |t_file| - t_file.used_by_grader = true - end, - ] + task_file(file).tap do |t_file| + t_file.used_by_grader = true + end end def exercise_files @@ -168,8 +192,11 @@ def task_files end def task_file(file) + file.update(xml_id_path: xml_id_from_file(file).join('/')) if file.xml_id_path.blank? + + xml_id = xml_id_from_file(file).last task_file = ProformaXML::TaskFile.new( - id: file.id, + id: xml_id, filename: filename(file), usage_by_lms: file.read_only ? 'display' : 'edit', visible: file.hidden ? 'no' : 'yes' diff --git a/app/services/proforma_service/convert_task_to_exercise.rb b/app/services/proforma_service/convert_task_to_exercise.rb index 6eda6e4ce..880915e36 100644 --- a/app/services/proforma_service/convert_task_to_exercise.rb +++ b/app/services/proforma_service/convert_task_to_exercise.rb @@ -60,44 +60,53 @@ def string_to_bool(str) end def files - model_solution_files + test_files + task_files.values.tap {|array| array.each {|file| file.role ||= 'regular_file' } } + model_solution_files + test_files + task_files end def test_files - @task.tests.map do |test_object| - task_files.delete(test_object.files.first.id).tap do |file| - file.weight = extract_meta_data(test_object.meta_data&.dig('test-meta-data'), 'weight').presence || 1.0 - file.feedback_message = extract_meta_data(test_object.meta_data&.dig('test-meta-data'), 'feedback-message').presence || 'Feedback' - file.hidden_feedback = extract_meta_data(test_object.meta_data&.dig('test-meta-data'), 'hidden-feedback').presence || false - file.role ||= 'teacher_defined_test' + @task.tests.flat_map do |test| + test.files.map do |task_file| + codeocean_file_from_task_file(task_file, test).tap do |file| + file.weight = extract_meta_data(test.meta_data&.dig('test-meta-data'), 'test-file', task_file.id, 'weight').presence || 1.0 + file.feedback_message = extract_meta_data(test.meta_data&.dig('test-meta-data'), 'test-file', task_file.id, 'feedback-message').presence || 'Feedback' + file.hidden_feedback = extract_meta_data(test.meta_data&.dig('test-meta-data'), 'test-file', task_file.id, 'hidden-feedback').presence || false + file.role = 'teacher_defined_test' unless file.teacher_defined_assessment? + end end end end def model_solution_files - @task.model_solutions.map do |model_solution_object| - task_files.delete(model_solution_object.files.first.id).tap do |file| - file.role ||= 'reference_implementation' + @task.model_solutions.flat_map do |model_solution| + model_solution.files.map do |task_file| + codeocean_file_from_task_file(task_file, model_solution).tap do |file| + file.role ||= 'reference_implementation' + end end end end def task_files - @task_files ||= @task.all_files.reject {|file| file.id == 'ms-placeholder-file' }.to_h do |task_file| - [task_file.id, codeocean_file_from_task_file(task_file)] + @task.files.reject {|file| file.id == 'ms-placeholder-file' }.map do |task_file| + codeocean_file_from_task_file(task_file).tap do |file| + file.role ||= 'regular_file' + end end end - def codeocean_file_from_task_file(file) + def codeocean_file_from_task_file(file, parent_object = nil) extension = File.extname(file.filename) - codeocean_file = CodeOcean::File.new( + + codeocean_file = CodeOcean::File.where(context: @exercise).where('xml_id_path = ? OR xml_id_path LIKE ?', file.id, "%/#{file.id}").first_or_initialize(context: @exercise) + codeocean_file.assign_attributes( context: @exercise, file_type: file_type(extension), hidden: file.visible != 'yes', # hides 'delayed' and 'no' name: File.basename(file.filename, '.*'), read_only: file.usage_by_lms != 'edit', role: extract_meta_data(@task.meta_data&.dig('meta-data'), 'files', "CO-#{file.id}", 'role'), - path: File.dirname(file.filename).in?(['.', '']) ? nil : File.dirname(file.filename) + path: File.dirname(file.filename).in?(['.', '']) ? nil : File.dirname(file.filename), + xml_id_path: (parent_object.nil? ? '' : "#{parent_object.id}/") + file.id.to_s ) if file.binary codeocean_file.native_file = FileIO.new(file.content.dup.force_encoding('UTF-8'), File.basename(file.filename)) diff --git a/app/services/proforma_service/import.rb b/app/services/proforma_service/import.rb index 0547f8bcb..86e11d5fd 100644 --- a/app/services/proforma_service/import.rb +++ b/app/services/proforma_service/import.rb @@ -14,13 +14,7 @@ def execute import_result = importer.perform @task = import_result - exercise = base_exercise - exercise_files = exercise&.files&.to_a - - exercise = ConvertTaskToExercise.call(task: @task, user: @user, exercise:) - exercise_files&.each(&:destroy) # feels suboptimal - - exercise + ConvertTaskToExercise.call(task: @task, user: @user, exercise: base_exercise) else import_multi end diff --git a/db/migrate/20240903204319_add_xml_path_to_files.rb b/db/migrate/20240903204319_add_xml_path_to_files.rb new file mode 100644 index 000000000..680624c15 --- /dev/null +++ b/db/migrate/20240903204319_add_xml_path_to_files.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddXmlPathToFiles < ActiveRecord::Migration[7.1] + def change + add_column :files, :xml_id_path, :string, null: true, default: nil + end +end diff --git a/db/schema.rb b/db/schema.rb index 01938a6da..85221e599 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2023_12_08_194632) do +ActiveRecord::Schema[7.1].define(version: 2024_09_03_204319) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" enable_extension "pgcrypto" @@ -315,6 +315,7 @@ t.string "path" t.integer "file_template_id" t.boolean "hidden_feedback", default: false, null: false + t.string "xml_id_path" t.index ["context_id", "context_type"], name: "index_files_on_context_id_and_context_type" end diff --git a/spec/services/proforma_service/convert_exercise_to_task_spec.rb b/spec/services/proforma_service/convert_exercise_to_task_spec.rb index 73e68dc03..c832c832c 100644 --- a/spec/services/proforma_service/convert_exercise_to_task_spec.rb +++ b/spec/services/proforma_service/convert_exercise_to_task_spec.rb @@ -79,11 +79,11 @@ context 'when exercise has a mainfile' do let(:files) { [file] } - let(:file) { build(:file) } + let(:file) { create(:file) } it 'creates a task-file with the correct attributes' do expect(task.files.first).to have_attributes( - id: file.id, + id: file.id.to_s, content: file.content, filename: file.name_with_extension, used_by_grader: true, @@ -109,6 +109,18 @@ ) ) end + + it 'sets xml_id_path to default' do + expect { convert_to_task.execute }.to change(file.reload, :xml_id_path).from(nil).to(file.id.to_s) + end + + context 'when xml_id_path is set for file' do + let(:file) { create(:file, xml_id_path: 'foobar') } + + it 'does not change xml_path_id' do + expect { convert_to_task.execute }.not_to change(file.reload, :xml_id_path) + end + end end context 'when exercise has a regular file' do @@ -119,7 +131,7 @@ it 'creates a task-file with the correct attributes' do expect(task.files.first).to have_attributes( - id: file.id, + id: file.id.to_s, content: file.content, filename: file.name_with_extension, used_by_grader: true, @@ -185,14 +197,14 @@ it 'creates a model-solution with one file' do expect(task.model_solutions.first).to have_attributes( - id: "ms-#{file.id}", + id: "co-ms-#{file.id}", files: have(1).item ) end it 'creates a model-solution with one file with correct attributes' do expect(task.model_solutions.first.files.first).to have_attributes( - id: file.id, + id: file.id.to_s, content: file.content, filename: file.name_with_extension, used_by_grader: false, @@ -210,6 +222,18 @@ it 'creates a task with two model-solutions' do expect(task.model_solutions).to have(2).items end + + context 'when reference_implementations belong to the same proforma model_solution' do + let(:files) { build_list(:file, 2, role: 'reference_implementation') {|file, i| file.xml_id_path = "proforma_ms/xml_id_#{i}" } } + + it 'creates a task with one model-solution' do + expect(task.model_solutions).to have(1).items + end + + it 'creates a task with a model-solution with two files' do + expect(task.model_solutions.first.files).to have(2).items + end + end end context 'when exercise has a test' do @@ -223,14 +247,19 @@ it 'creates a test with one file' do expect(task.tests.first).to have_attributes( - id: test_file.id, + id: "co-test-#{test_file.id}", title: test_file.name, files: have(1).item, meta_data: a_hash_including( 'test-meta-data' => a_hash_including( - 'CodeOcean:feedback-message' => {'$1' => 'feedback_message', '@@order' => ['$1']}, - 'CodeOcean:weight' => {'$1' => test_file.weight, '@@order' => ['$1']}, - 'CodeOcean:hidden-feedback' => {'$1' => test_file.hidden_feedback, '@@order' => ['$1']} + 'CodeOcean:test-file' => a_hash_including( + "CodeOcean:#{test_file.id}" => a_hash_including( + '@name' => test_file.name, + 'CodeOcean:feedback-message' => {'$1' => 'feedback_message', '@@order' => ['$1']}, + 'CodeOcean:weight' => {'$1' => test_file.weight, '@@order' => ['$1']}, + 'CodeOcean:hidden-feedback' => {'$1' => test_file.hidden_feedback, '@@order' => ['$1']} + ) + ) ) ) ) @@ -238,7 +267,7 @@ it 'creates a test with one file with correct attributes' do expect(task.tests.first.files.first).to have_attributes( - id: test_file.id, + id: test_file.id.to_s, content: test_file.content, filename: test_file.name_with_extension, used_by_grader: true, @@ -263,6 +292,36 @@ it 'creates a task with two tests' do expect(task.tests).to have(2).items end + + context 'when test_files belong to the same proforma test' do + let(:tests) { build_list(:test_file, 2) {|file, i| file.xml_id_path = "proforma_test/xml_id_#{i}" } } + + it 'creates a test with two file' do + expect(task.tests.first).to have_attributes( + id: 'proforma_test', + title: tests.first.name, + files: have(2).item, + meta_data: a_hash_including( + 'test-meta-data' => a_hash_including( + 'CodeOcean:test-file' => a_hash_including( + 'CodeOcean:xml_id_0' => a_hash_including( + '@name' => tests.first.name, + 'CodeOcean:feedback-message' => {'$1' => 'feedback_message', '@@order' => ['$1']}, + 'CodeOcean:weight' => {'$1' => 1, '@@order' => ['$1']}, + 'CodeOcean:hidden-feedback' => {'$1' => tests.first.hidden_feedback, '@@order' => ['$1']} + ), + 'CodeOcean:xml_id_1' => a_hash_including( + '@name' => tests.last.name, + 'CodeOcean:feedback-message' => {'$1' => 'feedback_message', '@@order' => ['$1']}, + 'CodeOcean:weight' => {'$1' => 1, '@@order' => ['$1']}, + 'CodeOcean:hidden-feedback' => {'$1' => tests.last.hidden_feedback, '@@order' => ['$1']} + ) + ) + ) + ) + ) + end + end end end end diff --git a/spec/services/proforma_service/convert_task_to_exercise_spec.rb b/spec/services/proforma_service/convert_task_to_exercise_spec.rb index 12438d9b3..400d35820 100644 --- a/spec/services/proforma_service/convert_task_to_exercise_spec.rb +++ b/spec/services/proforma_service/convert_task_to_exercise_spec.rb @@ -361,10 +361,18 @@ files: test_files, meta_data: { 'test-meta-data' => { - '@@order' => %w[CodeOcean:feedback-message CodeOcean:weight CodeOcean:hidden-feedback], - 'CodeOcean:feedback-message' => {'$1' => 'feedback-message', '@@order' => ['$1']}, - 'CodeOcean:weight' => {'$1' => '0.7', '@@order' => ['$1']}, - 'CodeOcean:hidden-feedback' => {'$1' => 'true', '@@order' => ['$1']}, + '@@order' => %w[CodeOcean:test-file], + '@xmlns' => {'CodeOcean' => 'codeocean.openhpi.de'}, + 'CodeOcean:test-file' => { + 'CodeOcean:test_file_id' => + { + '@@order' => %w[CodeOcean:feedback-message CodeOcean:weight CodeOcean:hidden-feedback], + '@name' => 'test_file_name', + 'CodeOcean:feedback-message' => {'$1' => 'feedback-message', '@@order' => ['$1']}, + 'CodeOcean:weight' => {'$1' => '0.7', '@@order' => ['$1']}, + 'CodeOcean:hidden-feedback' => {'$1' => 'true', '@@order' => ['$1']}, + }, + }, }, } ) @@ -506,7 +514,7 @@ let(:test_files) { [test_file] } let(:test_file) do ProformaXML::TaskFile.new( - id: 'test_file_id', + id: 'test-file-id', content: 'testfile-content', filename: 'testfile.txt', used_by_grader: 'yes', @@ -526,7 +534,7 @@ let(:ms_files) { [ms_file] } let(:ms_file) do ProformaXML::TaskFile.new( - id: 'ms-file', + id: 'ms-file-id', content: 'ms-content', filename: 'filename.txt', used_by_grader: 'used_by_grader', @@ -546,6 +554,26 @@ .and(include(have_attributes(content: 'testfile-content', role: 'teacher_defined_test', hidden: true))) ) end + + context 'when the files with correct xml_id_paths already exist' do + let(:exercise) do + create(:dummy, + files: co_files, + title: 'exercise-title', + description: 'exercise-description') + end + let(:co_files) do + [create(:file, xml_id_path: 'id', role: 'regular_file'), + create(:file, xml_id_path: 'ms-id/ms-file-id', role: 'reference_implementation'), + create(:test_file, xml_id_path: 'test-id/test-file-id')] + end + + it 'reuses existing file' do + convert_to_exercise_service + + expect(exercise.reload.files).to match_array(co_files) + end + end end end end diff --git a/spec/services/proforma_service/import_spec.rb b/spec/services/proforma_service/import_spec.rb index 286d0a45f..9809988c2 100644 --- a/spec/services/proforma_service/import_spec.rb +++ b/spec/services/proforma_service/import_spec.rb @@ -84,12 +84,12 @@ let(:files) { [file] } let(:file) { build(:file) } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } context 'when the mainfile is very large' do let(:file) { build(:file, content: 'test' * (10**5)) } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } end end @@ -97,12 +97,12 @@ let(:files) { [file] } let(:file) { build(:file, role: 'regular_file') } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } context 'when file has an attachment' do let(:file) { build(:file, :image, role: 'regular_file') } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } end end @@ -110,26 +110,26 @@ let(:files) { [file] } let(:file) { build(:file, role: 'reference_implementation', read_only: true) } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } end context 'when exercise has multiple files with role reference implementation' do let(:files) { build_list(:file, 2, role: 'reference_implementation', read_only: true) } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } end context 'when exercise has a test' do let(:tests) { [test] } let(:test) { build(:test_file) } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } end context 'when exercise has multiple tests' do let(:tests) { build_list(:test_file, 2) } - it { is_expected.to be_an_equal_exercise_as exercise } + it { is_expected.to be_an_equal_exercise_as exercise.reload } end context 'when task in zip has a different uuid' do