diff --git a/Changelog.md b/Changelog.md index 01b7372a02..3662376b5e 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ ### 🐛 Bug fixes - Ensures row selection for peer reviewer unassigning has the same validation checks as individual selections (#7274) +- Ensures mark levels on a rubric criterion are properly scaled when its max mark is updated (#7311) - Refactor contributors list in About section to read from markus-contributors.txt (#7374) ### 🔧 Internal changes diff --git a/app/controllers/criteria_controller.rb b/app/controllers/criteria_controller.rb index 459cb60c9c..1b911c6bb0 100644 --- a/app/controllers/criteria_controller.rb +++ b/app/controllers/criteria_controller.rb @@ -76,11 +76,7 @@ def update end if @criterion.is_a? RubricCriterion # update everything except levels and assignments - properly_updated = @criterion.update(rubric_criterion_params.except(:levels_attributes, :assignment_files)) - # update levels - if rubric_criterion_params[:levels_attributes] - properly_updated &&= @criterion.update_levels(rubric_criterion_params[:levels_attributes]) - end + properly_updated = @criterion.update(rubric_criterion_params.except(:assignment_files)) unless rubric_criterion_params[:assignment_files].nil? assignment_files = AssignmentFile.find(rubric_criterion_params[:assignment_files].reject(&:empty?)) end diff --git a/app/models/rubric_criterion.rb b/app/models/rubric_criterion.rb index e0b60f782f..059fc86ddb 100644 --- a/app/models/rubric_criterion.rb +++ b/app/models/rubric_criterion.rb @@ -5,29 +5,6 @@ class RubricCriterion < Criterion validates :levels, presence: true DEFAULT_MAX_MARK = 4 - # Checks whether the passed in param's level_attributes have unique name and marks. - # Skips the uniqueness validation if true. - def update_levels(levels_attributes) - s1, s2 = Set[], Set[] - should_skip = true - - # Check if there's a dup in marks or names - levels_attributes.each_value do |val| - if s1.include?(val[:mark]) || s2.include?(val[:name]) - should_skip = false - break - end - s1.add(val[:mark]) - s2.add(val[:name]) - end - - if should_skip - levels_attributes.each_value do |val| - val[:skip_uniqueness_validation] = true - end - end - self.update(levels_attributes: levels_attributes) - end def level_with_mark_closest_to(mark) self.levels.min_by { |m| (m.mark - mark).abs } diff --git a/spec/models/rubric_criterion_spec.rb b/spec/models/rubric_criterion_spec.rb index 1aeaf89987..c3e546ce40 100644 --- a/spec/models/rubric_criterion_spec.rb +++ b/spec/models/rubric_criterion_spec.rb @@ -5,120 +5,6 @@ it_behaves_like 'a criterion' end - describe '#update_levels' do - # At creation, rubric already has 5 levels - let(:rubric) { create(:rubric_criterion) } - - context 'when the properties with uniqueness validations are changed in a way -that two properties A and B have switched values' do - context 'when the names are changed and the marks are not changed' do - it 'updates the associated levels if all the levels after update will have unique names' do - level0_mark, level0_name, level0_id = rubric.levels[0].mark, rubric.levels[0].name, rubric.levels[0].id - level1_mark, level1_name, level1_id = rubric.levels[1].mark, rubric.levels[1].name, rubric.levels[1].id - - params = { - '0' => { mark: level0_mark, name: level1_name, id: level0_id }, - '1' => { mark: level1_mark, name: level0_name, id: level1_id } - } - rubric.update_levels(params) - - expect(rubric.levels.find_by(id: level0_id).name).to eq(level1_name) - expect(rubric.levels.find_by(id: level1_id).name).to eq(level0_name) - end - end - - context 'when the marks are changed and the names are not changed' do - it 'updates the associated levels if all the levels after update will have unique marks' do - level0_mark, level0_name, level0_id = rubric.levels[0].mark, rubric.levels[0].name, rubric.levels[0].id - level1_mark, level1_name, level1_id = rubric.levels[1].mark, rubric.levels[1].name, rubric.levels[1].id - - params = { - '0' => { mark: level1_mark, name: level0_name, id: level0_id }, - '1' => { mark: level0_mark, name: level1_name, id: level1_id } - } - rubric.update_levels(params) - - expect(rubric.levels.find_by(id: level0_id).mark).to eq(level1_mark) - expect(rubric.levels.find_by(id: level1_id).mark).to eq(level0_mark) - end - end - - context 'when both the marks and the names are changed' do - it 'updates the associated levels if all the levels after update will have unique marks' do - level0_mark, level0_name, level0_id = rubric.levels[0].mark, rubric.levels[0].name, rubric.levels[0].id - level1_mark, level1_name, level1_id = rubric.levels[1].mark, rubric.levels[1].name, rubric.levels[1].id - - params = { - '0' => { mark: level1_mark, name: level1_name, id: level0_id }, - '1' => { mark: level0_mark, name: level0_name, id: level1_id } - } - rubric.update_levels(params) - - expect(rubric.levels.find_by(id: level0_id).mark).to eq(level1_mark) - expect(rubric.levels.find_by(id: level1_id).mark).to eq(level0_mark) - expect(rubric.levels.find_by(id: level0_id).name).to eq(level1_name) - expect(rubric.levels.find_by(id: level1_id).name).to eq(level0_name) - end - end - end - - context 'when the properties with uniqueness validations are changed in a way that the validation is be violated' do - context 'when the names are changed and the marks are not changed' do - it 'does not update associated levels' do - level0_mark, level0_id = rubric.levels[0].mark, rubric.levels[0].id - level1_mark, level1_name, level1_id = rubric.levels[1].mark, rubric.levels[1].name, rubric.levels[1].id - - params = { - '0' => { name: level1_name, mark: level0_mark, id: level0_id }, - '1' => { name: level1_name, mark: level1_mark, id: level1_id } - } - level0_before = rubric.levels.find_by(id: level0_id) - level1_before = rubric.levels.find_by(id: level1_id) - rubric.update_levels(params) - - expect(rubric.levels.find_by(id: level0_id)).to eq(level0_before) - expect(rubric.levels.find_by(id: level1_id)).to eq(level1_before) - end - end - - context 'when the marks are changed and the names are not changed' do - it 'does not update associated levels' do - level0_name, level0_id = rubric.levels[0].name, rubric.levels[0].id - level1_mark, level1_name, level1_id = rubric.levels[1].mark, rubric.levels[1].name, rubric.levels[1].id - - params = { - '0' => { name: level0_name, mark: level1_mark, id: level0_id }, - '1' => { name: level1_name, mark: level1_mark, id: level1_id } - } - level0_before = rubric.levels.find_by(id: level0_id) - level1_before = rubric.levels.find_by(id: level1_id) - rubric.update_levels(params) - - expect(rubric.levels.find_by(id: level0_id)).to eq(level0_before) - expect(rubric.levels.find_by(id: level1_id)).to eq(level1_before) - end - end - - context 'when both the marks and the names are changed' do - it 'does not update associated levels' do - level0_id = rubric.levels[0].id - level1_mark, level1_name, level1_id = rubric.levels[1].mark, rubric.levels[1].name, rubric.levels[1].id - - params = { - '0' => { name: level1_name, mark: level1_mark, id: level0_id }, - '1' => { name: level1_name, mark: level1_mark, id: level1_id } - } - level0_before = rubric.levels.find_by(id: level0_id) - level1_before = rubric.levels.find_by(id: level1_id) - rubric.update_levels(params) - - expect(rubric.levels.find_by(id: level0_id)).to eq(level0_before) - expect(rubric.levels.find_by(id: level1_id)).to eq(level1_before) - end - end - end - end - context 'A good rubric criterion model' do before do @rubric = create(:rubric_criterion)