From 44fed816ee510c0aa56406c9221a0ffb2601d8f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 14 Feb 2018 23:09:18 +0000 Subject: Do not validate individual Variables when saving Project/Group --- app/models/group.rb | 2 +- app/models/project.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 75bf013ecd2..45160875dfb 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,7 +26,7 @@ class Group < Namespace has_many :shared_projects, through: :project_group_links, source: :project has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :labels, class_name: 'GroupLabel' - has_many :variables, class_name: 'Ci::GroupVariable' + has_many :variables, class_name: 'Ci::GroupVariable', validate: false has_many :custom_attributes, class_name: 'GroupCustomAttribute' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/project.rb b/app/models/project.rb index 4ad6f025e5c..fd09bb6a4d3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -210,7 +210,7 @@ class Project < ActiveRecord::Base has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName' has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' - has_many :variables, class_name: 'Ci::Variable' + has_many :variables, class_name: 'Ci::Variable', validate: false has_many :triggers, class_name: 'Ci::Trigger' has_many :environments has_many :deployments -- cgit v1.2.1 From 4319b15a78be70ccabb31a25ffba37f77de27b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Feb 2018 13:59:11 +0100 Subject: Condition associated variable validation in Project and Group --- app/models/group.rb | 3 ++- app/models/project.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 45160875dfb..e7640d1c8ec 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,7 +26,7 @@ class Group < Namespace has_many :shared_projects, through: :project_group_links, source: :project has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :labels, class_name: 'GroupLabel' - has_many :variables, class_name: 'Ci::GroupVariable', validate: false + has_many :variables, class_name: 'Ci::GroupVariable' has_many :custom_attributes, class_name: 'GroupCustomAttribute' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -37,6 +37,7 @@ class Group < Namespace validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent validates :variables, variable_duplicates: true + validates_associated :variables, if: proc { |group| group.errors[:variables].nil? } validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/models/project.rb b/app/models/project.rb index fd09bb6a4d3..4ad6f025e5c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -210,7 +210,7 @@ class Project < ActiveRecord::Base has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName' has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' - has_many :variables, class_name: 'Ci::Variable', validate: false + has_many :variables, class_name: 'Ci::Variable' has_many :triggers, class_name: 'Ci::Trigger' has_many :environments has_many :deployments -- cgit v1.2.1 From c65529e8f66bf5367ad2d989a556bf766701d7b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sun, 18 Feb 2018 21:54:55 +0100 Subject: Skip variables duplicates validator if variable is already a duplicate --- app/models/group.rb | 1 - app/validators/variable_duplicates_validator.rb | 2 ++ spec/support/features/variable_list_shared_examples.rb | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/group.rb b/app/models/group.rb index e7640d1c8ec..75bf013ecd2 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -37,7 +37,6 @@ class Group < Namespace validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent validates :variables, variable_duplicates: true - validates_associated :variables, if: proc { |group| group.errors[:variables].nil? } validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/validators/variable_duplicates_validator.rb b/app/validators/variable_duplicates_validator.rb index 4bfa3c45303..72660be6c43 100644 --- a/app/validators/variable_duplicates_validator.rb +++ b/app/validators/variable_duplicates_validator.rb @@ -5,6 +5,8 @@ # - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model class VariableDuplicatesValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) + return if record.errors.include?(:"#{attribute}.key") + if options[:scope] scoped = value.group_by do |variable| Array(options[:scope]).map { |attr| variable.send(attr) } # rubocop:disable GitlabSecurity/PublicSend diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 0d8f7a7aae6..f7f851eb1eb 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -261,6 +261,8 @@ shared_examples 'variable list' do click_button('Save variables') wait_for_requests + expect(all('.js-ci-variable-list-section .js-ci-variable-error-box ul li').count).to eq(1) + # We check the first row because it re-sorts to alphabetical order on refresh page.within('.js-ci-variable-list-section') do expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables have duplicate values \(.+\)/) -- cgit v1.2.1 From c7dac22e52ceb6f47492af0cc57ce3f16894b90f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 22 Feb 2018 21:10:02 +0100 Subject: Add CHANGELOG entry --- changelogs/unreleased/43275-improve-variables-validation-message.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/43275-improve-variables-validation-message.yml diff --git a/changelogs/unreleased/43275-improve-variables-validation-message.yml b/changelogs/unreleased/43275-improve-variables-validation-message.yml new file mode 100644 index 00000000000..88ef93123a0 --- /dev/null +++ b/changelogs/unreleased/43275-improve-variables-validation-message.yml @@ -0,0 +1,5 @@ +--- +title: Remove duplicated error message on duplicate variable validation +merge_request: 17135 +author: +type: fixed -- cgit v1.2.1 From f2695aa84b68cd905eff90c598e452cbf63d98ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 23 Feb 2018 01:19:46 +0100 Subject: Include CI Variable Key in its uniqueness validation error message --- app/models/ci/group_variable.rb | 5 ++++- app/models/ci/variable.rb | 5 ++++- spec/models/ci/group_variable_spec.rb | 2 +- spec/models/ci/variable_spec.rb | 2 +- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb index afeae69ba39..1dd0e050ba9 100644 --- a/app/models/ci/group_variable.rb +++ b/app/models/ci/group_variable.rb @@ -6,7 +6,10 @@ module Ci belongs_to :group - validates :key, uniqueness: { scope: :group_id } + validates :key, uniqueness: { + scope: :group_id, + message: "(%{value}) has already been taken" + } scope :unprotected, -> { where(protected: false) } end diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 67d3ec81b6f..7c71291de84 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -6,7 +6,10 @@ module Ci belongs_to :project - validates :key, uniqueness: { scope: [:project_id, :environment_scope] } + validates :key, uniqueness: { + scope: [:project_id, :environment_scope], + message: "(%{value}) has already been taken" + } scope :unprotected, -> { where(protected: false) } end diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index 145189e7469..1b10501701c 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -5,7 +5,7 @@ describe Ci::GroupVariable do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } - it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id) } + it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) } describe '.unprotected' do subject { described_class.unprotected } diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index e4ff551151e..875e8b2b682 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -6,7 +6,7 @@ describe Ci::Variable do describe 'validations' do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } - it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) } + it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } end describe '.unprotected' do -- cgit v1.2.1