Merge branch 'ce-to-ee-2018-02-07' into 'master'

CE upstream - 2018-02-07 09:27 UTC

See merge request gitlab-org/gitlab-ee!4419

app/assets/javascripts/boards/components/board_list.vue

@@ -159,7 +159,7 @@ export default {
       this.showIssueForm = !this.showIssueForm;
     },
     onScroll() {
-      if (!this.list.loadingMore && (this.scrollTop() > this.scrollHeight() - this.scrollOffset)) {
+      if (!this.loadingMore && (this.scrollTop() > this.scrollHeight() - this.scrollOffset)) {
         this.loadNextPage();
       }
     },

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

app/models/namespace.rb

@@ -21,6 +21,9 @@ class Namespace < ActiveRecord::Base
 
   has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :project_statistics
+
+  # This should _not_ be `inverse_of: :namespace`, because that would also set
+  # `user.namespace` when this user creates a group with themselves as `owner`.
   belongs_to :owner, class_name: "User"
 
   belongs_to :parent, class_name: "Namespace"
@@ -30,7 +33,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

@@ -251,8 +251,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

@@ -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?

app/models/user.rb

@@ -79,7 +79,7 @@ class User < ActiveRecord::Base
   #
 
   # Namespace for personal projects
-  has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, autosave: true # rubocop:disable Cop/ActiveRecordDependent
+  has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent
 
   # Profile
   has_many :keys, -> do
@@ -153,12 +153,9 @@ class User < ActiveRecord::Base
   validates :projects_limit,
     presence: true,
     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 }
+  validates :username, 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?
@@ -173,7 +170,8 @@ class User < ActiveRecord::Base
   before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? }
   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? }
-  after_save :ensure_namespace_correct
+  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
@@ -523,17 +521,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.')
@@ -902,17 +889,16 @@ class User < ActiveRecord::Base
   end
 
   def ensure_namespace_correct
-    # Ensure user has namespace
-    create_namespace!(path: username, name: username) unless namespace
-
-    if username_changed?
-      unless namespace.update_attributes(path: username, name: username)
-        namespace.errors.each do |attribute, message|
-          self.errors.add(:"namespace_#{attribute}", message)
-        end
-        raise ActiveRecord::RecordInvalid.new(namespace)
+    if namespace
+      namespace.path = namespace.name = username if username_changed?
+    else
+      build_namespace(path: username, name: username)
     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

app/validators/abstract_path_validator.rb

@@ -13,10 +13,6 @@ class AbstractPathValidator < ActiveModel::EachValidator
     raise NotImplementedError
   end
 
-  def self.full_path(record, value)
-    value
-  end
-
   def self.valid_path?(path)
     encode!(path)
     "#{path}/" =~ path_regex
@@ -28,7 +24,7 @@ class AbstractPathValidator < ActiveModel::EachValidator
       return
     end
 
-    full_path = self.class.full_path(record, value)
+    full_path = record.build_full_path
     return unless full_path
 
     unless self.class.valid_path?(full_path)

app/validators/namespace_path_validator.rb

@@ -12,8 +12,4 @@ class NamespacePathValidator < AbstractPathValidator
   def self.format_error_message
     Gitlab::PathRegex.namespace_format_message
   end
-
-  def self.full_path(record, value)
-    record.build_full_path
-  end
 end

app/validators/project_path_validator.rb

@@ -12,8 +12,4 @@ class ProjectPathValidator < AbstractPathValidator
   def self.format_error_message
     Gitlab::PathRegex.project_path_format_message
   end
-
-  def self.full_path(record, value)
-    record.build_full_path
-  end
 end

app/validators/user_path_validator.rb

-class UserPathValidator < AbstractPathValidator
-  extend Gitlab::EncodingHelper
-
-  def self.path_regex
-    Gitlab::PathRegex.root_namespace_path_regex
-  end
-
-  def self.format_regex
-    Gitlab::PathRegex.namespace_format_regex
-  end
-
-  def self.format_error_message
-    Gitlab::PathRegex.namespace_format_message
-  end
-end

changelogs/unreleased/dm-route-path-validation.yml

+---
+title: Validate user, group and project paths consistently, and only once
+merge_request:
+author:
+type: fixed

changelogs/unreleased/dm-user-namespace-route-path-validation.yml

+---
+title: Validate user namespace before saving so that errors persist on model
+merge_request:
+author:
+type: fixed

db/migrate/20180206200543_reset_events_primary_key_sequence.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ResetEventsPrimaryKeySequence < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  class Event < ActiveRecord::Base
+    self.table_name = 'events'
+  end
+
+  def up
+    if Gitlab::Database.postgresql?
+      reset_primary_key_for_postgresql
+    else
+      reset_primary_key_for_mysql
+    end
+  end
+
+  def down
+    # No-op
+  end
+
+  def reset_primary_key_for_postgresql
+    reset_pk_sequence!(Event.table_name)
+  end
+
+  def reset_primary_key_for_mysql
+    amount = Event.pluck('COALESCE(MAX(id), 1)').first
+
+    execute "ALTER TABLE #{Event.table_name} AUTO_INCREMENT = #{amount}"
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180204200836) do
+ActiveRecord::Schema.define(version: 20180206200543) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

doc/install/kubernetes/gitlab_chart.md

 # GitLab Helm Chart
-> **Note**:
-* This chart is deprecated, and is being replaced by the [cloud native GitLab chart](https://gitlab.com/charts/helm.gitlab.io/blob/master/README.md). For more information on available charts, please see our [overview](index.md#chart-overview).
-* These charts have been tested on Google Kubernetes Engine and Azure Container Service. Other Kubernetes installations may work as well, if not please [open an issue](https://gitlab.com/charts/charts.gitlab.io/issues).
+> **Note:**
+* This chart has been tested on Google Kubernetes Engine and Azure Container Service.
+
+**This chart is deprecated.** For small installations on Kubernetes today, we recommend the beta [`gitlab-omnibus` Helm chart](gitlab_omnibus.md).
 
+A new [cloud native GitLab chart](index.md#cloud-native-gitlab-chart) is in development with increased scalability and resilience, among other benefits. The cloud native chart will replace both the `gitlab` and `gitlab-omnibus` charts when available later this year.
 
-For more information on available GitLab Helm Charts, please see our [overview](index.md#chart-overview).
+Due to the significant architectural changes, migrating will require backing up data out of this instance and restoring it into the new deployment. For more information on available GitLab Helm Charts, please see our [overview](index.md#chart-overview).
 
 ## Introduction
 
 The `gitlab` Helm chart deploys just GitLab into your Kubernetes cluster, and offers extensive configuration options. This chart requires advanced knowledge of Kubernetes to successfully use. We **strongly recommend** the [gitlab-omnibus](gitlab_omnibus.md) chart.
 
-This chart is deprecated, and will be replaced by the [cloud native GitLab chart](https://gitlab.com/charts/helm.gitlab.io/blob/master/README.md). Due to the difficulty in supporting upgrades, migrating will require exporting data out of this instance and importing it into the new deployment.
-
 This chart includes the following:
 
 - Deployment using the [gitlab-ce](https://hub.docker.com/r/gitlab/gitlab-ce) or [gitlab-ee](https://hub.docker.com/r/gitlab/gitlab-ee) container image

doc/install/kubernetes/gitlab_omnibus.md

 # GitLab-Omnibus Helm Chart
-> **Note:**
-* This Helm chart is in beta, and will be deprecated by the [cloud native GitLab chart](https://gitlab.com/charts/helm.gitlab.io/blob/master/README.md).
-* These charts have been tested on Google Kubernetes Engine and Azure Container Service. Other Kubernetes installations may work as well, if not please [open an issue](https://gitlab.com/charts/charts.gitlab.io/issues).
+> **Note:**.
+* This chart has been tested on Google Kubernetes Engine and Azure Container Service.
 
-This work is based partially on: https://github.com/lwolf/kubernetes-gitlab/. GitLab would like to thank Sergey Nuzhdin for his work.
+**[This chart is beta](#limitations), and is the best way to install GitLab on Kubernetes today.** A new [cloud native GitLab chart](index.md#cloud-native-gitlab-chart) is in development with increased scalability and resilience, among other benefits. Once available, the cloud native chart will be the recommended installation method for Kubernetes, and this chart will be deprecated.
 
 For more information on available GitLab Helm Charts, please see our [overview](index.md#chart-overview).
 
+This work is based partially on: https://github.com/lwolf/kubernetes-gitlab/. GitLab would like to thank Sergey Nuzhdin for his work.
+
 ## Introduction
 
 This chart provides an easy way to get started with GitLab, provisioning an installation with nearly all functionality enabled. SSL is automatically provisioned via [Let's Encrypt](https://letsencrypt.org/).
 
-This Helm chart is in beta, and will be deprecated by the [cloud native GitLab chart](https://gitlab.com/charts/helm.gitlab.io/blob/master/README.md) once available. Due to the difficulty in supporting upgrades, migrating will require exporting data out of this instance and importing it into the new deployment.
+This Helm chart is in beta, and is suited for small to medium deployments. It will be deprecated by the [cloud native GitLab chart](https://gitlab.com/charts/helm.gitlab.io/blob/master/README.md) once available. Due to the significant architectural changes, migrating will require backing up data out of this instance and importing it into the new deployment.
 
 The deployment includes:
 

lib/constraints/user_url_constrainer.rb

@@ -2,7 +2,7 @@ class UserUrlConstrainer
   def matches?(request)
     full_path = request.params[:username]
 
-    return false unless UserPathValidator.valid_path?(full_path)
+    return false unless NamespacePathValidator.valid_path?(full_path)
 
     User.find_by_full_path(full_path, follow_redirects: request.get?).present?
   end

lib/gitlab/git/hook.rb

@@ -82,15 +82,21 @@ module Gitlab
       end
 
       def call_update_hook(gl_id, gl_username, oldrev, newrev, ref)
-        Dir.chdir(repo_path) do
         env = {
           'GL_ID' => gl_id,
-            'GL_USERNAME' => gl_username
+          'GL_USERNAME' => gl_username,
+          'PWD' => repo_path
+        }
+
+        options = {
+          chdir: repo_path
         }
-          stdout, stderr, status = Open3.capture3(env, path, ref, oldrev, newrev)
+
+        args = [ref, oldrev, newrev]
+
+        stdout, stderr, status = Open3.capture3(env, path, *args, options)
         [status.success?, (stderr.presence || stdout).gsub(/\R/, "<br>").html_safe]
       end
-      end
 
       def retrieve_error_message(stderr, stdout)
         err_message = stderr.read

lib/gitlab/o_auth/user.rb

@@ -180,7 +180,7 @@ module Gitlab
         valid_username = ::Namespace.clean_path(username)
 
         uniquify = Uniquify.new
-        valid_username = uniquify.string(valid_username) { |s| !UserPathValidator.valid_path?(s) }
+        valid_username = uniquify.string(valid_username) { |s| !NamespacePathValidator.valid_path?(s) }
 
         name = auth_hash.name
         name = valid_username if name.strip.empty?

lib/gitlab/path_regex.rb

@@ -173,18 +173,10 @@ module Gitlab
       @project_git_route_regex ||= /#{project_route_regex}\.git/.freeze
     end
 
-    def root_namespace_path_regex
-      @root_namespace_path_regex ||= %r{\A#{root_namespace_route_regex}/\z}
-    end
-
     def full_namespace_path_regex
       @full_namespace_path_regex ||= %r{\A#{full_namespace_route_regex}/\z}
     end
 
-    def project_path_regex
-      @project_path_regex ||= %r{\A#{project_route_regex}/\z}
-    end
-
     def full_project_path_regex
       @full_project_path_regex ||= %r{\A#{full_namespace_route_regex}/#{project_route_regex}/\z}
     end

lib/gitlab/sidekiq_middleware/memory_killer.rb

@@ -45,7 +45,7 @@ module Gitlab
       private
 
       def get_rss
-        output, status = Gitlab::Popen.popen(%W(ps -o rss= -p #{pid}))
+        output, status = Gitlab::Popen.popen(%W(ps -o rss= -p #{pid}), Rails.root.to_s)
         return 0 unless status.zero?
 
         output.to_i

qa/README.md

@@ -34,9 +34,6 @@ You can use GitLab QA to exercise tests on any live instance! For example, the
 following call would login to a local [GDK] instance and run all specs in
 `qa/specs/features`:
 
-First, `cd` into the `$gdk/gitlab/qa` directory.
-The `bin/qa` script expects you to be in the `qa` folder of the app.
-
 ```
 bin/qa Test::Instance http://localhost:3000
 ```

qa/qa.rb

@@ -64,6 +64,7 @@ module QA
       autoload :Instance, 'qa/scenario/test/instance'
 
       module Integration
+        autoload :LDAP, 'qa/scenario/test/integration/ldap'
         autoload :Mattermost, 'qa/scenario/test/integration/mattermost'
       end
 

qa/qa/page/main/login.rb

@@ -14,12 +14,32 @@ module QA
           element :sign_in_button, 'submit "Sign in"'
         end
 
+        view 'app/views/devise/sessions/_new_ldap.html.haml' do
+          element :username_field, 'text_field_tag :username'
+          element :password_field, 'password_field_tag :password'
+          element :sign_in_button, 'submit_tag "Sign in"'
+        end
+
+        view 'app/views/devise/shared/_tabs_ldap.html.haml' do
+          element :ldap_tab, "link_to server['label']"
+          element :standard_tab, "link_to 'Standard'"
+        end
+
         def initialize
           wait(max: 500) do
             page.has_css?('.application')
           end
         end
 
+        def sign_in_using_ldap_credentials
+          click_link 'LDAP'
+
+          fill_in :username, with: Runtime::User.name
+          fill_in :password, with: Runtime::User.password
+
+          click_button 'Sign in'
+        end
+
         def sign_in_using_credentials
           using_wait_time 0 do
             if page.has_content?('Change your password')
@@ -28,6 +48,8 @@ module QA
               click_button 'Change your password'
             end
 
+            click_link 'Standard' if page.has_content?('LDAP')
+
             fill_in :user_login, with: Runtime::User.name
             fill_in :user_password, with: Runtime::User.password
             click_button 'Sign in'

qa/qa/scenario/test/instance.rb

@@ -22,7 +22,12 @@ module QA
           Specs::Runner.perform do |specs|
             specs.tty = true
             specs.tags = self.class.focus
-            specs.files = files.any? ? files : 'qa/specs/features'
+            specs.files =
+              if files.any?
+                files
+              else
+                File.expand_path('../../specs/features', __dir__)
+              end
           end
         end
       end

qa/qa/scenario/test/integration/ldap.rb

+module QA
+  module Scenario
+    module Test
+      module Integration
+        class LDAP < Test::Instance
+          tags :ldap
+        end
+      end
+    end
+  end
+end

qa/qa/specs/features/login/ldap_spec.rb

+module QA
+  feature 'LDAP user login', :ldap do
+    scenario 'user logs in using LDAP credentials' do
+      Runtime::Browser.visit(:gitlab, Page::Main::Login)
+      Page::Main::Login.act { sign_in_using_ldap_credentials }
+
+      # TODO, since `Signed in successfully` message was removed
+      # this is the only way to tell if user is signed in correctly.
+      #
+      Page::Menu::Main.perform do |menu|
+        expect(menu).to have_personal_area
+      end
+    end
+  end
+end

qa/qa/specs/runner.rb

@@ -8,7 +8,7 @@ module QA
       def initialize
         @tty = false
         @tags = []
-        @files = ['qa/specs/features']
+        @files = [File.expand_path('./features', __dir__)]
       end
 
       def perform

qa/spec/scenario/test/instance_spec.rb

@@ -29,7 +29,8 @@ describe QA::Scenario::Test::Instance do
       it 'should call runner with default arguments' do
         subject.perform("test")
 
-        expect(runner).to have_received(:files=).with('qa/specs/features')
+        expect(runner).to have_received(:files=)
+          .with(File.expand_path('../../../qa/specs/features', __dir__))
       end
     end
 

spec/ee/spec/features/profiles/pipeline_quota_spec.rb

 require 'spec_helper'
 
-feature 'Profile > Pipeline Quota' do
+feature 'Profile > Pipeline Quota', :postgresql do
   before do
-    # This would make sure that the user won't try to create another namespace
-    allow_any_instance_of(User).to receive(:ensure_namespace_correct)
     gitlab_sign_in(user)
   end
 

spec/lib/gitlab/closing_issue_extractor_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Gitlab::ClosingIssueExtractor do
   let(:project) { create(:project) }
   let(:project2) { create(:project) }
-  let(:forked_project) { Projects::ForkService.new(project, project.creator).execute }
+  let(:forked_project) { Projects::ForkService.new(project, project2.creator).execute }
   let(:issue) { create(:issue, project: project) }
   let(:issue2) { create(:issue, project: project2) }
   let(:reference) { issue.to_reference }
@@ -14,6 +14,7 @@ describe Gitlab::ClosingIssueExtractor do
 
   before do
     project.add_developer(project.creator)
+    project.add_developer(project2.creator)
     project2.add_master(project.creator)
   end
 

spec/lib/gitlab/git_access_spec.rb

@@ -964,19 +964,6 @@ describe Gitlab::GitAccess do
       end
     end
 
-    context "when license blocks changes" do
-      before do
-        create(:protected_branch, name: 'feature', project: project)
-        allow(License).to receive(:block_changes?).and_return(true)
-      end
-
-      # Only check admin; if an admin can't do it, other roles can't either
-      matrix = permissions_matrix[:admin].dup
-      matrix.each { |key, _| matrix[key] = false }
-
-      run_permission_checks(admin: matrix)
-    end
-
     describe "push_rule_check" do
       let(:start_sha) { '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' }
       let(:end_sha)   { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }

spec/lib/gitlab/path_regex_spec.rb

@@ -194,8 +194,8 @@ describe Gitlab::PathRegex do
     end
   end
 
-  describe '.root_namespace_path_regex' do
-    subject { described_class.root_namespace_path_regex }
+  describe '.root_namespace_route_regex' do
+    subject { %r{\A#{described_class.root_namespace_route_regex}/\z} }
 
     it 'rejects top level routes' do
       expect(subject).not_to match('admin/')
@@ -318,8 +318,8 @@ describe Gitlab::PathRegex do
     end
   end
 
-  describe '.project_path_regex' do
-    subject { described_class.project_path_regex }
+  describe '.project_route_regex' do
+    subject { %r{\A#{described_class.project_route_regex}/\z} }
 
     it 'accepts top level routes' do
       expect(subject).to match('admin/')

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
 

spec/models/group_spec.rb

@@ -42,7 +42,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

@@ -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

@@ -134,7 +134,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

@@ -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
 

spec/models/user_spec.rb

@@ -110,7 +110,7 @@ describe User do
         user = build(:user, username: 'dashboard')
 
         expect(user).not_to be_valid
-        expect(user.errors.values).to eq [['dashboard is a reserved name']]
+        expect(user.errors.messages[:username]).to eq ['dashboard is a reserved name']
       end
 
       it 'allows child names' do
@@ -125,12 +125,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)) }
 
@@ -141,6 +135,35 @@ describe User do
           expect(user.errors.messages[:username].first).to match('cannot be changed if a personal project has container registry tags')
         end
       end
+
+      context 'when the username was used by another user before' do
+        let(:username) { 'foo' }
+        let!(:other_user) { create(:user, username: username) }
+
+        before do
+          other_user.username = 'bar'
+          other_user.save!
+        end
+
+        it 'is invalid' do
+          user = build(:user, username: username)
+
+          expect(user).not_to be_valid
+          expect(user.errors.full_messages).to eq(['Username has been taken before'])
+        end
+      end
+
+      context 'when the username is in use by another user' do
+        let(:username) { 'foo' }
+        let!(:other_user) { create(:user, username: username) }
+
+        it 'is invalid' do
+          user = build(:user, username: username)
+
+          expect(user).not_to be_valid
+          expect(user.errors.full_messages).to eq(['Username has already been taken'])
+        end
+      end
     end
 
     it 'has a DB-level NOT NULL constraint on projects_limit' do
@@ -2373,17 +2396,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
@@ -2726,7 +2749,7 @@ describe User do
 
       it 'should raise an ActiveRecord::RecordInvalid exception' do
         user2 = build(:user, username: 'foo')
-        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Route path foo has been taken before. Please use another one, Route is invalid/)
+        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Username has been taken before/)
       end
     end
 

spec/services/groups/transfer_service_spec.rb

@@ -177,7 +177,7 @@ describe Groups::TransferService, :postgresql do
 
         it 'should add an error on group' do
           transfer_service.execute(new_parent_group)
-          expect(transfer_service.error).to eq('Transfer failed: Validation failed: Route path has already been taken, Route is invalid')
+          expect(transfer_service.error).to eq('Transfer failed: Validation failed: Path has already been taken')
         end
       end
 

spec/services/projects/gitlab_projects_import_service_spec.rb

 require 'spec_helper'
 
 describe Projects::GitlabProjectsImportService do
-  set(:namespace) { build(:namespace) }
+  set(:namespace) { create(:namespace) }
   let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
   subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) }
 

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)

spec/validators/user_path_validator_spec.rb

-require 'spec_helper'
-
-describe UserPathValidator do
-  let(:validator) { described_class.new(attributes: [:username]) }
-
-  describe '.valid_path?' do
-    it 'handles invalid utf8' do
-      expect(described_class.valid_path?("a\0weird\255path")).to be_falsey
-    end
-  end
-
-  describe '#validates_each' do
-    it 'adds a message when the path is not in the correct format' do
-      user = build(:user)
-
-      validator.validate_each(user, :username, "Path with spaces, and comma's!")
-
-      expect(user.errors[:username]).to include(Gitlab::PathRegex.namespace_format_message)
-    end
-
-    it 'adds a message when the path is reserved when creating' do
-      user = build(:user, username: 'help')
-
-      validator.validate_each(user, :username, 'help')
-
-      expect(user.errors[:username]).to include('help is a reserved name')
-    end
-
-    it 'adds a message when the path is reserved when updating' do
-      user = create(:user)
-      user.username = 'help'
-
-      validator.validate_each(user, :username, 'help')
-
-      expect(user.errors[:username]).to include('help is a reserved name')
-    end
-  end
-end