From 456128c3bd616e96af562a74a11df327b102cd74 Mon Sep 17 00:00:00 2001 From: Armin Kirchner Date: Tue, 29 Jul 2025 11:18:56 +0200 Subject: [PATCH] refactor: Files feedback_message without NULL state Column type string should not have a null state to avoid logical distinction between NULL and '' case. User defined tests are created without feedback_message. Removing the NULL case allows the new record to render the empty feedback_message. An edge case is resolved when a test for assessment with a feedback_message is transformed to a user defined test. The feedback_message is overwritten, and no validation error for invisible fields is displayed. Resolves #3014 --- app/models/code_ocean/file.rb | 7 +++- .../convert_task_to_exercise.rb | 2 -- ...remove_null_from_files_feedback_message.rb | 15 ++++++++ db/schema.rb | 4 +-- spec/models/code_ocean/file_spec.rb | 36 +++++++++++++------ 5 files changed, 49 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20250729085843_remove_null_from_files_feedback_message.rb diff --git a/app/models/code_ocean/file.rb b/app/models/code_ocean/file.rb index 0d9327531..c76d83e61 100644 --- a/app/models/code_ocean/file.rb +++ b/app/models/code_ocean/file.rb @@ -16,6 +16,7 @@ class File < ApplicationRecord after_initialize :set_default_values before_validation :clear_weight, unless: :teacher_defined_assessment? + before_validation :clear_feedback_message, unless: :teacher_defined_assessment? before_validation :hash_content, if: :content_present? before_validation :set_ancestor_values, if: :incomplete_descendent? @@ -49,7 +50,6 @@ class File < ApplicationRecord default_scope { order(path: :asc, name: :asc) } validates :feedback_message, if: :teacher_defined_assessment?, presence: true - validates :feedback_message, absence: true, unless: :teacher_defined_assessment? validates :hashed_content, if: :content_present?, presence: true validates :hidden, inclusion: [true, false] validates :hidden_feedback, inclusion: [true, false] @@ -153,6 +153,11 @@ def set_default_values end private :set_default_values + def clear_feedback_message + self.feedback_message = '' + end + private :clear_feedback_message + def visible? !hidden end diff --git a/app/services/proforma_service/convert_task_to_exercise.rb b/app/services/proforma_service/convert_task_to_exercise.rb index 238ce4eb3..1c3c2ac62 100644 --- a/app/services/proforma_service/convert_task_to_exercise.rb +++ b/app/services/proforma_service/convert_task_to_exercise.rb @@ -89,7 +89,6 @@ def model_solution_files model_solution.files.map do |task_file| codeocean_file_from_task_file(task_file, model_solution).tap do |file| file.role ||= 'reference_implementation' - file.feedback_message = nil end end end @@ -99,7 +98,6 @@ def task_files @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' - file.feedback_message = nil end end end diff --git a/db/migrate/20250729085843_remove_null_from_files_feedback_message.rb b/db/migrate/20250729085843_remove_null_from_files_feedback_message.rb new file mode 100644 index 000000000..3bb25f18f --- /dev/null +++ b/db/migrate/20250729085843_remove_null_from_files_feedback_message.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class RemoveNullFromFilesFeedbackMessage < ActiveRecord::Migration[8.0] + def up + CodeOcean::File.where(feedback_message: nil).in_batches.update_all(feedback_message: '') # rubocop:disable Rails/SkipsModelValidations + + change_column_default :files, :feedback_message, '' + change_column_null :files, :feedback_message, false + end + + def down + change_column_null :files, :feedback_message, true + change_column_default :files, :feedback_message, nil + end +end diff --git a/db/schema.rb b/db/schema.rb index f95ae3f2d..fdaefd546 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[8.0].define(version: 2025_07_13_114125) do +ActiveRecord::Schema[8.0].define(version: 2025_07_29_085843) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" enable_extension "pgcrypto" @@ -312,7 +312,7 @@ t.string "native_file" t.string "role" t.string "hashed_content" - t.string "feedback_message" + t.string "feedback_message", default: "", null: false t.float "weight" t.string "path" t.integer "file_template_id" diff --git a/spec/models/code_ocean/file_spec.rb b/spec/models/code_ocean/file_spec.rb index e031da404..a5ba54053 100644 --- a/spec/models/code_ocean/file_spec.rb +++ b/spec/models/code_ocean/file_spec.rb @@ -3,7 +3,12 @@ require 'rails_helper' RSpec.describe CodeOcean::File do - let(:file) { described_class.create.tap {|file| file.update(content: nil, hidden: nil, read_only: nil) } } + let(:file) do + described_class.new.tap do |file| + file.assign_attributes(content: nil, hidden: nil, read_only: nil) + file.validate + end + end it 'validates the presence of a file type' do expect(file.errors[:file_type]).to be_present @@ -11,7 +16,8 @@ it 'validates the presence of the hidden flag' do expect(file.errors[:hidden]).to be_present - file.update(hidden: false) + file.hidden = false + file.validate expect(file.errors[:hidden]).to be_blank end @@ -21,19 +27,24 @@ it 'validates the presence of the read-only flag' do expect(file.errors[:read_only]).to be_present - file.update(read_only: false) + file.read_only = false + file.validate expect(file.errors[:read_only]).to be_blank end context 'with a teacher-defined test' do - before { file.update(role: 'teacher_defined_test') } + before do + file.role = 'teacher_defined_test' + file.validate + end it 'validates the presence of a feedback message' do expect(file.errors[:feedback_message]).to be_present end it 'validates the numericality of a weight' do - file.update(weight: 'heavy') + file.weight = 'heavy' + file.validate expect(file.errors[:weight]).to be_present end @@ -43,16 +54,21 @@ end context 'with another file type' do - before { file.update(role: 'regular_file') } + before do + file.role = 'regular_file' + end - it 'validates the absence of a feedback message' do - file.update(feedback_message: 'Your solution is not correct yet.') - expect(file.errors[:feedback_message]).to be_present + it 'removes the feedback message' do + file.feedback_message = 'Your solution is not correct yet.' + + expect { file.validate } + .to change(file, :feedback_message).to('') end it 'validates the absence of a weight' do allow(file).to receive(:clear_weight) - file.update(weight: 1) + file.weight = 1 + file.validate expect(file.errors[:weight]).to be_present end end