Merge branch 'bvl-rename-routes-after-user-rename' into 'master'

Set the name of a user-namespace to the user name

See merge request gitlab-org/gitlab-ce!23272

11 files changed, 268 insertions, 36 deletions

diff --git a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb
index fdef9bc5638..45328482ad7 100644
--- a/spec/controllers/projects/environments/prometheus_api_controller_spec.rb
+++ b/spec/controllers/projects/environments/prometheus_api_controller_spec.rb
@@ -176,7 +176,7 @@ describe Projects::Environments::PrometheusApiController do
   def environment_params(params = {})
     {
       id: environment.id.to_s,
-      namespace_id: project.namespace.name,
+      namespace_id: project.namespace.full_path,
       project_id: project.name,
       proxy_path: 'query',
       query: '1'
diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb
index d05ef2a8f12..2a70cbb2a72 100644
--- a/spec/features/merge_request/user_creates_merge_request_spec.rb
+++ b/spec/features/merge_request/user_creates_merge_request_spec.rb
@@ -78,7 +78,7 @@ describe "User creates a merge request", :js do
 
       click_button("Submit merge request")
 
-      expect(page).to have_content(title).and have_content("Request to merge #{user.namespace.name}:#{source_branch} into master")
+      expect(page).to have_content(title).and have_content("Request to merge #{user.namespace.path}:#{source_branch} into master")
     end
   end
 end
diff --git a/spec/features/projects/fork_spec.rb b/spec/features/projects/fork_spec.rb
index 7c71b4c52e0..26b94bf58b2 100644
--- a/spec/features/projects/fork_spec.rb
+++ b/spec/features/projects/fork_spec.rb
@@ -50,7 +50,7 @@ describe 'Project fork' do
       click_link('New merge request')
     end
 
-    expect(current_path).to have_content(/#{user.namespace.name}/i)
+    expect(current_path).to have_content(/#{user.namespace.path}/i)
   end
 
   it 'shows avatars when Gravatar is disabled' do
diff --git a/spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb b/spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb
new file mode 100644
index 00000000000..5938ecca459
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/fix_user_namespace_names_spec.rb
@@ -0,0 +1,104 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::FixUserNamespaceNames, :migration, schema: 20190620112608 do
+  let(:namespaces) { table(:namespaces) }
+  let(:users) { table(:users) }
+  let(:user) { users.create(name: "The user's full name", projects_limit: 10, username: 'not-null', email: '1') }
+
+  context 'updating the namespace names' do
+    it 'updates a user namespace within range' do
+      user2 = users.create(name: "Other user's full name", projects_limit: 10, username: 'also-not-null', email: '2')
+      user_namespace1 = namespaces.create(
+        id: 2,
+        owner_id: user.id,
+        name: "Should be the user's name",
+        path: user.username
+      )
+      user_namespace2 = namespaces.create(
+        id: 3,
+        owner_id: user2.id,
+        name: "Should also be the user's name",
+        path: user.username
+      )
+
+      described_class.new.perform(1, 5)
+
+      expect(user_namespace1.reload.name).to eq("The user's full name")
+      expect(user_namespace2.reload.name).to eq("Other user's full name")
+    end
+
+    it 'does not update namespaces out of range' do
+      user_namespace = namespaces.create(
+        id: 6,
+        owner_id: user.id,
+        name: "Should be the user's name",
+        path: user.username
+      )
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { user_namespace.reload.name }
+    end
+
+    it 'does not update groups owned by the users' do
+      user_group = namespaces.create(
+        id: 2,
+        owner_id: user.id,
+        name: 'A group name',
+        path: 'the-path',
+        type: 'Group'
+      )
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { user_group.reload.name }
+    end
+  end
+
+  context 'namespace route names' do
+    let(:routes) { table(:routes) }
+    let(:namespace) do
+      namespaces.create(
+        id: 2,
+        owner_id: user.id,
+        name: "Will be updated to the user's name",
+        path: user.username
+      )
+    end
+
+    it "updates the route name if it didn't match the namespace" do
+      route = routes.create(path: namespace.path, name: 'Incorrect name', source_type: 'Namespace', source_id: namespace.id)
+
+      described_class.new.perform(1, 5)
+
+      expect(route.reload.name).to eq("The user's full name")
+    end
+
+    it 'updates the route name if it was nil match the namespace' do
+      route = routes.create(path: namespace.path, name: nil, source_type: 'Namespace', source_id: namespace.id)
+
+      described_class.new.perform(1, 5)
+
+      expect(route.reload.name).to eq("The user's full name")
+    end
+
+    it "doesn't update group routes" do
+      route = routes.create(path: 'group-path', name: 'Group name', source_type: 'Group', source_id: namespace.id)
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { route.reload.name }
+    end
+
+    it "doesn't touch routes for namespaces out of range" do
+      user_namespace = namespaces.create(
+        id: 6,
+        owner_id: user.id,
+        name: "Should be the user's name",
+        path: user.username
+      )
+
+      expect { described_class.new.perform(1, 5) }
+        .not_to change { user_namespace.reload.name }
+    end
+  end
+end
diff --git a/spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb b/spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb
new file mode 100644
index 00000000000..d1d6d8411d1
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/fix_user_project_route_names_spec.rb
@@ -0,0 +1,98 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::FixUserProjectRouteNames, :migration, schema: 20190620112608 do
+  let(:namespaces) { table(:namespaces) }
+  let(:users) { table(:users) }
+  let(:routes) { table(:routes) }
+  let(:projects) { table(:projects) }
+
+  let(:user) { users.create(name: "The user's full name", projects_limit: 10, username: 'not-null', email: '1') }
+
+  let(:namespace) do
+    namespaces.create(
+      owner_id: user.id,
+      name: "Should eventually be the user's name",
+      path: user.username
+    )
+  end
+
+  let(:project) do
+    projects.create(namespace_id: namespace.id, name: 'Project Name')
+  end
+
+  it "updates the route for a project if it did not match the user's name" do
+    route = routes.create(
+      id: 1,
+      path: "#{user.username}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: 'Completely wrong'
+    )
+
+    described_class.new.perform(1, 5)
+
+    expect(route.reload.name).to eq("The user's full name / Project Name")
+  end
+
+  it 'updates the route for a project if the name was nil' do
+    route = routes.create(
+      id: 1,
+      path: "#{user.username}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: nil
+    )
+
+    described_class.new.perform(1, 5)
+
+    expect(route.reload.name).to eq("The user's full name / Project Name")
+  end
+
+  it 'does not update routes that were are out of the range' do
+    route = routes.create(
+      id: 6,
+      path: "#{user.username}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: 'Completely wrong'
+    )
+
+    expect { described_class.new.perform(1, 5) }
+      .not_to change { route.reload.name }
+  end
+
+  it 'does not update routes for projects in groups owned by the user' do
+    group = namespaces.create(
+      owner_id: user.id,
+      name: 'A group',
+      path: 'a-path',
+      type: ''
+    )
+    project = projects.create(namespace_id: group.id, name: 'Project Name')
+    route = routes.create(
+      id: 1,
+      path: "#{group.path}/#{project.path}",
+      source_id: project.id,
+      source_type: 'Project',
+      name: 'Completely wrong'
+    )
+
+    expect { described_class.new.perform(1, 5) }
+      .not_to change { route.reload.name }
+  end
+
+  it 'does not update routes for namespaces' do
+    route = routes.create(
+      id: 1,
+      path: namespace.path,
+      source_id: namespace.id,
+      source_type: 'Namespace',
+      name: 'Completely wrong'
+    )
+
+    expect { described_class.new.perform(1, 5) }
+      .not_to change { route.reload.name }
+  end
+end
diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb
index 1fb0dd5030c..31163a5bb5c 100644
--- a/spec/models/concerns/routable_spec.rb
+++ b/spec/models/concerns/routable_spec.rb
@@ -15,23 +15,46 @@ describe Group, 'Routable' do
   end
 
   describe 'Callbacks' do
-    it 'creates route record on create' do
-      expect(group.route.path).to eq(group.path)
-      expect(group.route.name).to eq(group.name)
-    end
+    context 'for a group' do
+      it 'creates route record on create' do
+        expect(group.route.path).to eq(group.path)
+        expect(group.route.name).to eq(group.name)
+      end
+
+      it 'updates route record on path change' do
+        group.update(path: 'wow', name: 'much')
 
-    it 'updates route record on path change' do
-      group.update(path: 'wow', name: 'much')
+        expect(group.route.path).to eq('wow')
+        expect(group.route.name).to eq('much')
+      end
+
+      it 'ensure route path uniqueness across different objects' do
+        create(:group, parent: group, path: 'xyz')
+        duplicate = build(:project, namespace: group, path: 'xyz')
 
-      expect(group.route.path).to eq('wow')
-      expect(group.route.name).to eq('much')
+        expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Path has already been taken')
+      end
     end
 
-    it 'ensure route path uniqueness across different objects' do
-      create(:group, parent: group, path: 'xyz')
-      duplicate = build(:project, namespace: group, path: 'xyz')
+    context 'for a user' do
+      let(:user) { create(:user, username: 'jane', name: "Jane Doe") }
 
-      expect { duplicate.save! }.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Path has already been taken')
+      it 'creates the route for a record on create' do
+        expect(user.namespace.name).to eq('Jane Doe')
+        expect(user.namespace.path).to eq('jane')
+      end
+
+      it 'updates routes and nested routes on name change' do
+        project = create(:project, path: 'work-stuff', name: 'Work stuff', namespace: user.namespace)
+
+        user.update!(username: 'jaen', name: 'Jaen Did')
+        project.reload
+
+        expect(user.namespace.name).to eq('Jaen Did')
+        expect(user.namespace.path).to eq('jaen')
+        expect(project.full_name).to eq('Jaen Did / Work stuff')
+        expect(project.full_path).to eq('jaen/work-stuff')
+      end
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a4d177da0be..e4f84172215 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -2931,7 +2931,7 @@ describe User do
       let(:user) { create(:user, username: username) }
 
       context 'when the user is updated' do
-        context 'when the username is changed' do
+        context 'when the username or name is changed' do
           let(:new_username) { 'bar' }
 
           it 'changes the namespace (just to compare to when username is not changed)' do
@@ -2942,16 +2942,24 @@ describe User do
             end.to change { user.namespace.updated_at }
           end
 
-          it 'updates the namespace name' do
+          it 'updates the namespace path when the username was changed' do
             user.update!(username: new_username)
 
-            expect(user.namespace.name).to eq(new_username)
+            expect(user.namespace.path).to eq(new_username)
           end
 
-          it 'updates the namespace path' do
-            user.update!(username: new_username)
+          it 'updates the namespace name if the name was changed' do
+            user.update!(name: 'New name')
 
-            expect(user.namespace.path).to eq(new_username)
+            expect(user.namespace.name).to eq('New name')
+          end
+
+          it 'updates nested routes for the namespace if the name was changed' do
+            project = create(:project, namespace: user.namespace)
+
+            user.update!(name: 'New name')
+
+            expect(project.route.reload.name).to include('New name')
           end
 
           context 'when there is a validation error (namespace name taken) while updating namespace' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index fcbff19bd61..3ab1818bebb 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -474,7 +474,7 @@ describe API::Internal do
           'ssh',
           {
             authentication_abilities: [:read_project, :download_code, :push_code],
-            namespace_path: project.namespace.name,
+            namespace_path: project.namespace.path,
             project_path: project.path,
             redirected_path: nil
           }
@@ -753,7 +753,7 @@ describe API::Internal do
   end
 
   describe 'GET /internal/merge_request_urls' do
-    let(:repo_name) { "#{project.namespace.name}/#{project.path}" }
+    let(:repo_name) { "#{project.full_path}" }
     let(:changes) { URI.escape("#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch") }
 
     before do
@@ -765,7 +765,7 @@ describe API::Internal do
 
       expect(json_response).to match [{
         "branch_name" => "new_branch",
-        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
         "new_merge_request" => true
       }]
     end
@@ -786,7 +786,7 @@ describe API::Internal do
 
         expect(json_response).to match [{
           "branch_name" => "new_branch",
-          "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
+          "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch",
           "new_merge_request" => true
         }]
       end
@@ -927,7 +927,7 @@ describe API::Internal do
 
       expect(json_response['merge_request_urls']).to match [{
         "branch_name" => branch_name,
-        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}",
+        "url" => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name}",
         "new_merge_request" => true
       }]
     end
@@ -970,7 +970,7 @@ describe API::Internal do
 
         expect(json_response['merge_request_urls']).to match [{
           'branch_name' => branch_name,
-          'url' => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/1",
+          'url' => "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/1",
           'new_merge_request' => false
         }]
       end
diff --git a/spec/routing/environments_spec.rb b/spec/routing/environments_spec.rb
index aacbe300966..28b3e79c1ff 100644
--- a/spec/routing/environments_spec.rb
+++ b/spec/routing/environments_spec.rb
@@ -9,7 +9,7 @@ describe 'environments routing' do
   end
 
   let(:environments_route) do
-    "#{project.namespace.name}/#{project.name}/environments/"
+    "#{project.full_path}/environments/"
   end
 
   describe 'routing environment folders' do
@@ -36,13 +36,12 @@ describe 'environments routing' do
   end
 
   def get_folder(folder)
-    get("#{project.namespace.name}/#{project.name}/" \
-        "environments/folders/#{folder}")
+    get("#{project.full_path}/environments/folders/#{folder}")
   end
 
   def folder_action(**opts)
-    options = { namespace_id: project.namespace.name,
-                project_id: project.name }
+    options = { namespace_id: project.namespace.path,
+                project_id: project.path }
 
     ['projects/environments#folder', options.merge(opts)]
   end
diff --git a/spec/serializers/environment_status_entity_spec.rb b/spec/serializers/environment_status_entity_spec.rb
index f421432e8d6..cb4749f019f 100644
--- a/spec/serializers/environment_status_entity_spec.rb
+++ b/spec/serializers/environment_status_entity_spec.rb
@@ -70,7 +70,7 @@ describe EnvironmentStatusEntity do
 
       it 'returns metrics url' do
         expect(subject[:metrics_url])
-          .to eq("/#{project.namespace.name}/#{project.name}/environments/#{environment.id}/deployments/#{deployment.iid}/metrics")
+          .to eq("/#{project.full_path}/environments/#{environment.id}/deployments/#{deployment.iid}/metrics")
       end
     end
 
diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb
index 0933c6d4336..9e7a5260ca4 100644
--- a/spec/services/merge_requests/get_urls_service_spec.rb
+++ b/spec/services/merge_requests/get_urls_service_spec.rb
@@ -8,8 +8,8 @@ describe MergeRequests::GetUrlsService do
   let(:project) { create(:project, :public, :repository) }
   let(:service) { described_class.new(project) }
   let(:source_branch) { "merge-test" }
-  let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" }
-  let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" }
+  let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" }
+  let(:show_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/#{merge_request.iid}" }
   let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
   let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" }
   let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" }
@@ -119,7 +119,7 @@ describe MergeRequests::GetUrlsService do
       let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" }
       let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/markdown" }
       let(:changes) { "#{new_branch_changes}\n#{existing_branch_changes}" }
-      let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" }
+      let(:new_merge_request_url) { "http://#{Gitlab.config.gitlab.host}/#{project.full_path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" }
 
       it 'returns 2 urls for both creating new and showing merge request' do
         result = service.execute(changes)