Scale rubric criterion levels on max_mark update (#7311)

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

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

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 }

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)