1247: add manual xml_id-uniqueness validations for tests and model_solutions

Author: kkoehn

app/models/model_solution.rb

@@ -12,11 +12,7 @@ class ModelSolution < ApplicationRecord
   validates :xml_id, presence: true
   validates :parent_id, uniqueness: {scope: :task}, if: -> { parent_id.present? }
   validate :parent_validation_check
-  # TODO: For new tasks, this validation is currently useless, because the validation is performed
-  # before the task is saved (and thus the task_id is not yet known, i.e., is NULL). Therefore,
-  # one can create a **new task** with a test that has the same xml_id as another test of the same task.
-  # TODO: This validation is currently useless on new records, because the uuid is generated after validation
-  validates :xml_id, uniqueness: {scope: :task_id}
+  validate :unique_xml_id, if: -> { !task.nil? && xml_id_changed? }
 
   def duplicate(set_parent_id: true)
     dup.tap do |model_solution|
@@ -26,4 +22,11 @@ def duplicate(set_parent_id: true)
       end
     end
   end
+
+  private
+
+  def unique_xml_id
+    xml_ids = (task.model_solutions - [self]).map(&:xml_id)
+    errors.add(:xml_id, :not_unique) if xml_ids.include? xml_id
+  end
 end

app/models/test.rb

@@ -12,10 +12,7 @@ class Test < ApplicationRecord
   validates :xml_id, presence: true
   validates :parent_id, uniqueness: {scope: :task}, if: -> { parent_id.present? }
   validate :parent_validation_check
-  # TODO: For new tasks, this validation is currently useless, because the validation is performed
-  # before the task is saved (and thus the task_id is not yet known, i.e., is NULL). Therefore,
-  # one can create a **new task** with a test that has the same xml_id as another test of the same task.
-  validates :xml_id, uniqueness: {scope: :task_id}
+  validate :unique_xml_id, if: -> { !task.nil? && xml_id_changed? }
 
   def configuration_as_xml
     Dachsfisch::JSON2XMLConverter.perform(json: configuration.to_json)
@@ -29,4 +26,11 @@ def duplicate(set_parent_id: true)
       end
     end
   end
+
+  private
+
+  def unique_xml_id
+    xml_ids = (task.tests - [self]).map(&:xml_id)
+    errors.add(:xml_id, :not_unique) if xml_ids.include? xml_id
+  end
 end

config/locales/de/models.yml

@@ -101,6 +101,8 @@ de:
         omniauth_provider: OmniAuth-Anbieter
         provider_uid: Anbieter-UID
     errors:
+      messages:
+        not_unique: is not unique
       models:
         collection:
           attributes:
@@ -120,10 +122,6 @@ de:
               not_iso639: ist kein zweistelliger ISO-639-1- oder dreistelliger ISO-639-2-Sprachcode
             task_contribution:
               duplicated: wurde bereits erzeugt und wartet auf Genehmigung. Bitte bearbeiten Sie Ihren vorhandenen Änderungsvorschlag, anstatt einen neuen anzulegen.
-        task_file:
-          attributes:
-            xml_id:
-              not_unique: ist nicht eindeutig
         user:
           attributes:
             avatar:

config/locales/en/models.yml

@@ -101,6 +101,8 @@ en:
         omniauth_provider: OmniAuth Provider
         provider_uid: Provider UID
     errors:
+      messages:
+        not_unique: is not unique
       models:
         collection:
           attributes:
@@ -120,10 +122,6 @@ en:
               not_iso639: is not a two letter ISO 639-1 or three letter ISO-639-2 language code
             task_contribution:
               duplicated: has been created already and is currently waiting for approval. Please edit your existing contribution instead of creating a new one
-        task_file:
-          attributes:
-            xml_id:
-              not_unique: is not unique
         user:
           attributes:
             avatar:

spec/models/model_solution_spec.rb

@@ -6,7 +6,17 @@
   describe 'validations' do
     it { is_expected.to belong_to(:task) }
     it { is_expected.to validate_presence_of(:xml_id) }
-    it { is_expected.to validate_uniqueness_of(:xml_id).scoped_to(:task_id) }
+
+    context 'when a task is created with multiple model_solutions' do
+      before { build(:task, model_solutions: [model_solution, build(:model_solution, xml_id: 'same')]) }
+
+      let(:model_solution) { build(:model_solution, xml_id: 'same') }
+
+      it 'validates xml_id correctly' do
+        model_solution.validate
+        expect(model_solution.errors.full_messages).to include "#{described_class.human_attribute_name('xml_id')} #{I18n.t('activerecord.errors.messages.not_unique')}"
+      end
+    end
 
     it_behaves_like 'parent validation with parent_id', :model_solution
   end

spec/models/task_file_spec.rb

@@ -18,7 +18,7 @@
 
       it 'has correct error' do
         file.validate
-        expect(file.errors.full_messages).to include "#{described_class.human_attribute_name('xml_id')} #{I18n.t('activerecord.errors.models.task_file.attributes.xml_id.not_unique')}"
+        expect(file.errors.full_messages).to include "#{described_class.human_attribute_name('xml_id')} #{I18n.t('activerecord.errors.messages.not_unique')}"
       end
     end
 
@@ -96,7 +96,7 @@
 
       it 'has correct error' do
         file.validate
-        expect(file.errors.full_messages).to include "#{described_class.human_attribute_name('xml_id')} #{I18n.t('activerecord.errors.models.task_file.attributes.xml_id.not_unique')}"
+        expect(file.errors.full_messages).to include "#{described_class.human_attribute_name('xml_id')} #{I18n.t('activerecord.errors.messages.not_unique')}"
       end
     end
 

spec/models/test_spec.rb

@@ -7,7 +7,17 @@
     it { is_expected.to belong_to(:task) }
     it { is_expected.to validate_presence_of(:title) }
     it { is_expected.to validate_presence_of(:xml_id) }
-    it { is_expected.to validate_uniqueness_of(:xml_id).scoped_to(:task_id) }
+
+    context 'when a task is created with multiple tests' do
+      before { build(:task, tests: [test, build(:test, xml_id: 'same')]) }
+
+      let(:test) { build(:test, xml_id: 'same') }
+
+      it 'validates xml_id correctly' do
+        test.validate
+        expect(test.errors.full_messages).to include "#{described_class.human_attribute_name('xml_id')} #{I18n.t('activerecord.errors.messages.not_unique')}"
+      end
+    end
 
     it_behaves_like 'parent validation with parent_id', :test
   end