diff --git a/app/models/model_solution.rb b/app/models/model_solution.rb index d15fd53e6..d0bf41814 100644 --- a/app/models/model_solution.rb +++ b/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 diff --git a/app/models/test.rb b/app/models/test.rb index 808b8261e..295113ef5 100644 --- a/app/models/test.rb +++ b/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 diff --git a/config/locales/de/models.yml b/config/locales/de/models.yml index 89adba73d..6bbcd13ac 100644 --- a/config/locales/de/models.yml +++ b/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: diff --git a/config/locales/en/models.yml b/config/locales/en/models.yml index 8b7d02002..f026d850b 100644 --- a/config/locales/en/models.yml +++ b/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: diff --git a/spec/models/model_solution_spec.rb b/spec/models/model_solution_spec.rb index 9f8a9a9ce..bb7a34b47 100644 --- a/spec/models/model_solution_spec.rb +++ b/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 diff --git a/spec/models/task_file_spec.rb b/spec/models/task_file_spec.rb index c7e08ae72..4437003eb 100644 --- a/spec/models/task_file_spec.rb +++ b/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 diff --git a/spec/models/test_spec.rb b/spec/models/test_spec.rb index f15d1bade..f55243246 100644 --- a/spec/models/test_spec.rb +++ b/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