Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
0
Merge Requests
0
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
Boxiang Sun
gitlab-ce
Commits
75144b1e
Commit
75144b1e
authored
Jan 31, 2018
by
Douwe Maan
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Validate path uniqueness only on Route, and bubble up appropriately
parent
8d69436c
Changes
12
Hide whitespace changes
Inline
Side-by-side
Showing
12 changed files
with
26 additions
and
37 deletions
+26
-37
app/models/concerns/routable.rb
app/models/concerns/routable.rb
+7
-1
app/models/namespace.rb
app/models/namespace.rb
+0
-1
app/models/project.rb
app/models/project.rb
+1
-2
app/models/route.rb
app/models/route.rb
+1
-1
app/models/user.rb
app/models/user.rb
+8
-14
spec/models/concerns/routable_spec.rb
spec/models/concerns/routable_spec.rb
+1
-1
spec/models/group_spec.rb
spec/models/group_spec.rb
+0
-1
spec/models/namespace_spec.rb
spec/models/namespace_spec.rb
+0
-1
spec/models/project_spec.rb
spec/models/project_spec.rb
+0
-1
spec/models/route_spec.rb
spec/models/route_spec.rb
+3
-3
spec/models/user_spec.rb
spec/models/user_spec.rb
+3
-9
spec/services/users/update_service_spec.rb
spec/services/users/update_service_spec.rb
+2
-2
No files found.
app/models/concerns/routable.rb
View file @
75144b1e
...
...
@@ -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
...
...
app/models/namespace.rb
View file @
75144b1e
...
...
@@ -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
...
...
app/models/project.rb
View file @
75144b1e
...
...
@@ -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
}
...
...
app/models/route.rb
View file @
75144b1e
...
...
@@ -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 on
e"
)
if
conflicting_redirect_exists?
errors
.
add
(
:path
,
"
has been taken befor
e"
)
if
conflicting_redirect_exists?
end
def
conflicting_redirect_exists?
...
...
app/models/user.rb
View file @
75144b1e
...
...
@@ -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?
validate
s
: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
...
...
spec/models/concerns/routable_spec.rb
View file @
75144b1e
...
...
@@ -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
...
...
spec/models/group_spec.rb
View file @
75144b1e
...
...
@@ -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
}
...
...
spec/models/namespace_spec.rb
View file @
75144b1e
...
...
@@ -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
)
}
...
...
spec/models/project_spec.rb
View file @
75144b1e
...
...
@@ -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
)
}
...
...
spec/models/route_spec.rb
View file @
75144b1e
...
...
@@ -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 on
e'
)
expect
(
new_route
.
errors
.
first
[
1
]).
to
eq
(
'
has been taken befor
e'
)
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 on
e'
)
expect
(
route
.
errors
.
first
[
1
]).
to
eq
(
'
has been taken befor
e'
)
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
...
...
spec/models/user_spec.rb
View file @
75144b1e
...
...
@@ -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
(
'
User
name has already been taken'
)
end
end
end
...
...
spec/services/users/update_service_spec.rb
View file @
75144b1e
...
...
@@ -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
(
'
User
name has already been taken'
)
end
def
update_user
(
user
,
opts
)
...
...
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment