ISSUE-7687: Remove criteria UPDATE conflicts

Changelog.md

@@ -10,6 +10,7 @@
 ### 🐛 Bug fixes
 - Fix name column search in graders table (#7693)
 - Check against mtime instead of atime for clean up of git repo directories in tmp folder (#7706)
+- Update Model: Fix level validation checks through use of a custom validator (#7696)
 
 ### 🔧 Internal changes
 - Updated Github Actions CI to use cache-apt-pkgs to speed up workflow runs (#7645)

app/models/level.rb

@@ -6,19 +6,37 @@ class Level < ApplicationRecord
   attr_accessor :skip_uniqueness_validation
 
   validates :name, presence: true
-  validates :name, uniqueness: { scope: :criterion_id }, unless: :skip_uniqueness_validation
 
   validates :description, exclusion: { in: [nil] }
 
   validates :mark, presence: true
-  validates :mark, uniqueness: { scope: :criterion_id }, unless: :skip_uniqueness_validation
   validates :mark, numericality: { greater_than_or_equal_to: 0 }
 
   validate :only_update_if_results_unreleased
+  validate -> { unique_within_criterion(:name) }, unless: :skip_uniqueness_validation
+  validate -> { unique_within_criterion(:mark) }, unless: :skip_uniqueness_validation
 
   before_update :update_associated_marks
   before_destroy :destroy_associated_marks
 
+  # Validates that an attr (i.e. name, or mark) is unique
+  #
+  # Custom validators have access to in memory data and can be used to validate final state changes.
+  # By contrast, built-in rails uniqueness validators query the database,
+  # their information limited, to pre-updated records
+  #
+  # Built in validators run into the issue of constraint infractions.
+  # This happens when an attempt is made to swap two values within the same transaction.
+  # Each record is compared against the database before the second has had a chance to update.
+  def unique_within_criterion(attr)
+    return unless criterion
+    return unless new_record? || public_send(:"will_save_change_to_#{attr}?")
+
+    siblings = criterion.levels.reject { |level| level.id == id || level.marked_for_destruction? }
+    duplicate = siblings.find { |level| level.public_send(attr) == public_send(attr) }
+    errors.add(attr, :taken) if duplicate
+  end
+
   def only_update_if_results_unreleased
     return if self.criterion.nil? # When the level is first being created
     unless self.criterion.results_unreleased?

spec/models/level_spec.rb

@@ -7,11 +7,4 @@
   it { is_expected.to have_one(:course) }
 
   it { is_expected.to validate_numericality_of(:mark).is_greater_than_or_equal_to(0) }
-
-  describe 'uniqueness validations' do
-    subject { create(:level, mark: 0.5) }
-
-    it { is_expected.to validate_uniqueness_of(:mark).scoped_to(:criterion_id) }
-    it { is_expected.to validate_uniqueness_of(:name).scoped_to(:criterion_id) }
-  end
 end

spec/models/rubric_criterion_spec.rb

@@ -190,6 +190,22 @@
     end
 
     context 'has basic levels functionality' do
+      describe 'uniqueness validations' do
+        it 'raise error on duplicate mark' do
+          @criterion.levels.create(name: 'A', description: 'A', mark: 5)
+          expect do
+            @criterion.levels.create!(name: 'B', description: 'B', mark: 5)
+          end.to raise_error(ActiveRecord::RecordInvalid, /Mark has already been taken/)
+        end
+
+        it 'raise error on duplicate name' do
+          @criterion.levels.create(name: 'A', description: 'A', mark: 5)
+          expect do
+            @criterion.levels.create!(name: 'A', description: 'B', mark: 6)
+          end.to raise_error(ActiveRecord::RecordInvalid, /Name has already been taken/)
+        end
+      end
+
       describe 'can add levels' do
         it 'not raise error' do
           expect(@criterion.levels.length).to eq(5)
@@ -199,6 +215,23 @@
         end
       end
 
+      describe 'can swap values in a single transaction' do
+        it 'not raise error' do
+          # Create records
+          a = @criterion.levels.create(name: 'testname', description: 'Description for level', mark: '5')
+          b = @criterion.levels.create(name: 'sillyname', description: 'Description for level 2', mark: '6')
+
+          @criterion.update!(
+            levels_attributes: [
+              { id: a.id, name: 'placeholder' },
+              { id: b.id, name: 'testname' }
+            ]
+          )
+          expect(@criterion.levels.find(a.id).name).to eq 'placeholder'
+          expect(@criterion.levels.find(b.id).name).to eq 'testname'
+        end
+      end
+
       describe 'can delete levels' do
         it 'not raise error' do
           expect(@criterion.levels.length).to eq(5)