diff options
author | Douwe Maan <douwe@selenight.nl> | 2018-01-31 10:52:09 -0600 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2018-02-06 12:06:38 -0600 |
commit | 75144b1e03db342730535f1f49b0e7cd2987d755 (patch) | |
tree | 30295c514e238b48a646d77c6b453e2ec44319f4 | |
parent | 8d69436c901a3eee674e72c67d91de3994d30e0c (diff) | |
download | gitlab-ce-75144b1e03db342730535f1f49b0e7cd2987d755.tar.gz |
Validate path uniqueness only on Route, and bubble up appropriately
-rw-r--r-- | app/models/concerns/routable.rb | 8 | ||||
-rw-r--r-- | app/models/namespace.rb | 1 | ||||
-rw-r--r-- | app/models/project.rb | 3 | ||||
-rw-r--r-- | app/models/route.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 22 | ||||
-rw-r--r-- | spec/models/concerns/routable_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/group_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/namespace_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 1 | ||||
-rw-r--r-- | spec/models/route_spec.rb | 6 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 12 | ||||
-rw-r--r-- | spec/services/users/update_service_spec.rb | 4 |
12 files changed, 26 insertions, 37 deletions
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 5c1cce98ad4..dfd7d94450b 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -7,11 +7,12 @@ module Routable has_one :route, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - validates_associated :route validates :route, presence: true scope :with_route, -> { includes(:route) } + after_validation :set_path_errors + before_validation do if full_path_changed? || full_name_changed? prepare_route @@ -125,6 +126,11 @@ module Routable private + def set_path_errors + route_path_errors = self.errors.delete(:"route.path") + self.errors[:path].concat(route_path_errors) if route_path_errors + end + def uncached_full_path if route && route.path.present? @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 44c5bb13a59..d95489ee9f2 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -32,7 +32,6 @@ class Namespace < ActiveRecord::Base validates :owner, presence: true, unless: ->(n) { n.type == "Group" } validates :name, presence: true, - uniqueness: { scope: :parent_id }, length: { maximum: 255 }, namespace_name: true diff --git a/app/models/project.rb b/app/models/project.rb index 12d5f28f5ea..dbd908670fd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -245,8 +245,7 @@ class Project < ActiveRecord::Base validates :path, presence: true, project_path: true, - length: { maximum: 255 }, - uniqueness: { scope: :namespace_id } + length: { maximum: 255 } validates :namespace, presence: true validates :name, uniqueness: { scope: :namespace_id } diff --git a/app/models/route.rb b/app/models/route.rb index 3d4b5a8b5ee..07d96c21cf1 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -75,7 +75,7 @@ class Route < ActiveRecord::Base def ensure_permanent_paths return if path.nil? - errors.add(:path, "#{path} has been taken before. Please use another one") if conflicting_redirect_exists? + errors.add(:path, "has been taken before") if conflicting_redirect_exists? end def conflicting_redirect_exists? diff --git a/app/models/user.rb b/app/models/user.rb index 1df3772e63f..da56446b6ab 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -153,10 +153,9 @@ class User < ActiveRecord::Base numericality: { greater_than_or_equal_to: 0, less_than_or_equal_to: Gitlab::Database::MAX_INT_VALUE } validates :username, user_path: true, - presence: true, - uniqueness: { case_sensitive: false } + presence: true - validate :namespace_uniq, if: :username_changed? + validates :namespace, presence: true validate :namespace_move_dir_allowed, if: :username_changed? validate :unique_email, if: :email_changed? @@ -172,6 +171,7 @@ class User < ActiveRecord::Base before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) } before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? } before_validation :ensure_namespace_correct + after_validation :set_username_errors after_update :username_changed_hook, if: :username_changed? after_destroy :post_destroy_hook after_destroy :remove_key_cache @@ -505,17 +505,6 @@ class User < ActiveRecord::Base end end - def namespace_uniq - # Return early if username already failed the first uniqueness validation - return if errors.key?(:username) && - errors[:username].include?('has already been taken') - - existing_namespace = Namespace.by_path(username) - if existing_namespace && existing_namespace != namespace - errors.add(:username, 'has already been taken') - end - end - def namespace_move_dir_allowed if namespace&.any_project_has_container_registry_tags? errors.add(:username, 'cannot be changed if a personal project has container registry tags.') @@ -891,6 +880,11 @@ class User < ActiveRecord::Base end end + def set_username_errors + namespace_path_errors = self.errors.delete(:"namespace.path") + self.errors[:username].concat(namespace_path_errors) if namespace_path_errors + end + def username_changed_hook system_hook_service.execute_hooks_for(self, :rename) end diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index 3106207811a..8cb50d7465c 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -39,7 +39,7 @@ describe Group, 'Routable' do create(:group, parent: group, path: 'xyz') duplicate = build(:project, namespace: group, path: 'xyz') - expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Route path has already been taken, Route is invalid') + expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Path has already been taken') end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 5ea4acb6687..338fb314ee9 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -41,7 +41,6 @@ describe Group do describe 'validations' do it { is_expected.to validate_presence_of :name } - it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) } it { is_expected.to validate_presence_of :path } it { is_expected.not_to validate_presence_of :owner } it { is_expected.to validate_presence_of :two_factor_grace_period } diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 5e126bc4bea..191b60e4383 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -15,7 +15,6 @@ describe Namespace do describe 'validations' do it { is_expected.to validate_presence_of(:name) } - it { is_expected.to validate_uniqueness_of(:name).scoped_to(:parent_id) } it { is_expected.to validate_length_of(:name).is_at_most(255) } it { is_expected.to validate_length_of(:description).is_at_most(255) } it { is_expected.to validate_presence_of(:path) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index da940571bc1..a63f5d6d5a1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -129,7 +129,6 @@ describe Project do it { is_expected.to validate_length_of(:name).is_at_most(255) } it { is_expected.to validate_presence_of(:path) } - it { is_expected.to validate_uniqueness_of(:path).scoped_to(:namespace_id) } it { is_expected.to validate_length_of(:path).is_at_most(255) } it { is_expected.to validate_length_of(:description).is_at_most(2000) } diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index 88f54fd10e5..dfac82b327a 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -27,7 +27,7 @@ describe Route do redirect.save!(validate: false) expect(new_route.valid?).to be_falsey - expect(new_route.errors.first[1]).to eq('foo has been taken before. Please use another one') + expect(new_route.errors.first[1]).to eq('has been taken before') end end @@ -49,7 +49,7 @@ describe Route do redirect.save!(validate: false) expect(route.valid?).to be_falsey - expect(route.errors.first[1]).to eq('foo has been taken before. Please use another one') + expect(route.errors.first[1]).to eq('has been taken before') end end @@ -368,7 +368,7 @@ describe Route do group2.path = 'foo' group2.valid? - expect(group2.errors["route.path"].first).to eq('foo has been taken before. Please use another one') + expect(group2.errors[:path]).to eq(['has been taken before']) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 24d4d8f1741..a52dc93befe 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -116,12 +116,6 @@ describe User do expect(user).to be_valid end - it 'validates uniqueness' do - user = build(:user) - - expect(user).to validate_uniqueness_of(:username).case_insensitive - end - context 'when username is changed' do let(:user) { build_stubbed(:user, username: 'old_path', namespace: build_stubbed(:namespace)) } @@ -2287,17 +2281,17 @@ describe User do end context 'when there is a validation error (namespace name taken) while updating namespace' do - let!(:conflicting_namespace) { create(:group, name: new_username, path: 'quz') } + let!(:conflicting_namespace) { create(:group, path: new_username) } it 'causes the user save to fail' do expect(user.update_attributes(username: new_username)).to be_falsey - expect(user.namespace.errors.messages[:name].first).to eq('has already been taken') + expect(user.namespace.errors.messages[:path].first).to eq('has already been taken') end it 'adds the namespace errors to the user' do user.update_attributes(username: new_username) - expect(user.errors.full_messages.first).to eq('Namespace name has already been taken') + expect(user.errors.full_messages.first).to eq('Username has already been taken') end end end diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index f8d4a47b212..a4b7fe4674f 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -21,13 +21,13 @@ describe Users::UpdateService do end it 'includes namespace error messages' do - create(:group, name: 'taken', path: 'something_else') + create(:group, path: 'taken') result = {} expect do result = update_user(user, { username: 'taken' }) end.not_to change { user.reload.username } expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Namespace name has already been taken') + expect(result[:message]).to eq('Username has already been taken') end def update_user(user, opts) |