Fix import behavior and specs on conflicting namespaces23932-import-behaviour-on-conflicting-namespaces-doesn-t-match-specs

The specs specify that in the case of conflicting namespaces on an
import, if the existing namespace is not owned by `current_user`, the
project will be created in that user's namespace instead. This change
correctly implements that behvaior and fixes the spec examples to test
it properly.

9 files changed, 84 insertions, 105 deletions

diff --git a/app/controllers/import/base_controller.rb b/app/controllers/import/base_controller.rb
index 9de0297ecfd..644bc031e15 100644
--- a/app/controllers/import/base_controller.rb
+++ b/app/controllers/import/base_controller.rb
@@ -1,16 +1,17 @@
 class Import::BaseController < ApplicationController
   private
 
-  def find_or_create_namespace(names, owner)
-    return current_user.namespace if names == owner
-    return current_user.namespace unless current_user.can_create_group?
+  def find_or_create_namespace
+    path = params[:target_namespace]
+
+    return current_user.namespace if path == current_user.namespace_path
 
-    names = params[:target_namespace].presence || names
-    full_path_namespace = Namespace.find_by_full_path(names)
+    owned_namespace = current_user.owned_groups.find_by_full_path(path)
+    return owned_namespace if owned_namespace
 
-    return full_path_namespace if full_path_namespace
+    return current_user.namespace unless current_user.can_create_group?
 
-    names.split('/').inject(nil) do |parent, name|
+    path.split('/').inject(nil) do |parent, name|
       begin
         namespace = Group.create!(name: name,
                                   path: name,
@@ -20,7 +21,8 @@ class Import::BaseController < ApplicationController
 
         namespace
       rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid
-        Namespace.where(parent: parent).find_by_path_or_name(name)
+        # Namespace.where(parent: parent).find_by_path_or_name(name)
+        current_user.namespace
       end
     end
   end
diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb
index 5ad1e116e4e..04c4a202e3d 100644
--- a/app/controllers/import/bitbucket_controller.rb
+++ b/app/controllers/import/bitbucket_controller.rb
@@ -42,11 +42,7 @@ class Import::BitbucketController < Import::BaseController
     repo = bitbucket_client.repo(name)
     @project_name = params[:new_name].presence || repo.name
 
-    repo_owner = repo.owner
-    repo_owner = current_user.username if repo_owner == bitbucket_client.user.username
-    namespace_path = params[:new_namespace].presence || repo_owner
-
-    @target_namespace = find_or_create_namespace(namespace_path, current_user)
+    @target_namespace = find_or_create_namespace
 
     if current_user.can?(:create_projects, @target_namespace)
       # The token in a session can be expired, we need to get most recent one because
diff --git a/app/controllers/import/github_controller.rb b/app/controllers/import/github_controller.rb
index 53a5981e564..29a350f62d4 100644
--- a/app/controllers/import/github_controller.rb
+++ b/app/controllers/import/github_controller.rb
@@ -39,8 +39,7 @@ class Import::GithubController < Import::BaseController
     @repo_id = params[:repo_id].to_i
     repo = client.repo(@repo_id)
     @project_name = params[:new_name].presence || repo.name
-    namespace_path = params[:target_namespace].presence || current_user.namespace_path
-    @target_namespace = find_or_create_namespace(namespace_path, current_user.namespace_path)
+    @target_namespace = find_or_create_namespace
 
     if can?(current_user, :create_projects, @target_namespace)
       @project = Gitlab::GithubImport::ProjectCreator.new(repo, @project_name, @target_namespace, current_user, access_params, type: provider).execute
diff --git a/app/controllers/import/gitlab_controller.rb b/app/controllers/import/gitlab_controller.rb
index 73837ffbe67..fc8c824a1da 100644
--- a/app/controllers/import/gitlab_controller.rb
+++ b/app/controllers/import/gitlab_controller.rb
@@ -27,7 +27,7 @@ class Import::GitlabController < Import::BaseController
     @repo_id = params[:repo_id].to_i
     repo = client.project(@repo_id)
     @project_name = repo['name']
-    @target_namespace = find_or_create_namespace(repo['namespace']['path'], client.user['username'])
+    @target_namespace = find_or_create_namespace
 
     if current_user.can?(:create_projects, @target_namespace)
       @project = Gitlab::GitlabImport::ProjectCreator.new(repo, @target_namespace, current_user, access_params).execute
diff --git a/app/views/import/gitlab/status.html.haml b/app/views/import/gitlab/status.html.haml
index 7456799ca0e..269037e932a 100644
--- a/app/views/import/gitlab/status.html.haml
+++ b/app/views/import/gitlab/status.html.haml
@@ -45,7 +45,17 @@
           %td
             = link_to repo["path_with_namespace"], "https://gitlab.com/#{repo["path_with_namespace"]}", target: "_blank", rel: 'noopener noreferrer'
           %td.import-target
-            = import_project_target(repo['namespace']['path'], repo['name'])
+            %fieldset.row
+            .input-group
+              .project-path.input-group-btn
+                - if current_user.can_select_namespace?
+                  - selected = params[:namespace_id] || :current_user
+                  - opts = current_user.can_create_group? ? { extra_group: Group.new(name: repo['namespace']['path'], path: repo['namespace']['path']) } : {}
+                  = select_tag :namespace_id, namespaces_options(selected, opts.merge({ display_path: true })), { class: 'select2 js-select-namespace', tabindex: 1 }
+                - else
+                  = text_field_tag :path, current_user.namespace_path, class: "input-large form-control", tabindex: 1, disabled: true
+              %span.input-group-addon /
+              = text_field_tag :path, repo['name'], class: "input-mini form-control", tabindex: 2, autofocus: true, required: true
           %td.import-actions.job-status
             = button_tag class: "btn btn-import js-add-to-import" do
               Import
diff --git a/changelogs/unreleased/fix-import-behaviour.yml b/changelogs/unreleased/fix-import-behaviour.yml
new file mode 100644
index 00000000000..392bd253144
--- /dev/null
+++ b/changelogs/unreleased/fix-import-behaviour.yml
@@ -0,0 +1,4 @@
+---
+title: Fix import behavior on conflicting namespaces
+merge_request: 7166
+author: 
diff --git a/spec/controllers/import/bitbucket_controller_spec.rb b/spec/controllers/import/bitbucket_controller_spec.rb
index 010e3180ea4..b2de7bd566c 100644
--- a/spec/controllers/import/bitbucket_controller_spec.rb
+++ b/spec/controllers/import/bitbucket_controller_spec.rb
@@ -97,19 +97,7 @@ describe Import::BitbucketController do
             to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params).
             and_return(double(execute: true))
 
-          post :create, format: :js
-        end
-      end
-
-      context "when the Bitbucket user and GitLab user's usernames don't match" do
-        let(:bitbucket_username) { "someone_else" }
-
-        it "takes the current user's namespace" do
-          expect(Gitlab::BitbucketImport::ProjectCreator).
-            to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params).
-            and_return(double(execute: true))
-
-          post :create, format: :js
+          post :create, target_namespace: bitbucket_username, format: :js
         end
       end
 
@@ -133,29 +121,27 @@ describe Import::BitbucketController do
       end
 
       context "when a namespace with the Bitbucket user's username already exists" do
-        let!(:existing_namespace) { create(:namespace, name: other_username, owner: user) }
+        let!(:existing_namespace) { create(:group, name: other_username) }
 
         context "when the namespace is owned by the GitLab user" do
+          before { existing_namespace.add_owner(user) }
+
           it "takes the existing namespace" do
             expect(Gitlab::BitbucketImport::ProjectCreator).
               to receive(:new).with(bitbucket_repo, bitbucket_repo.name, existing_namespace, user, access_params).
               and_return(double(execute: true))
 
-            post :create, format: :js
+            post :create, target_namespace: existing_namespace.name, format: :js
           end
         end
 
         context "when the namespace is not owned by the GitLab user" do
-          before do
-            existing_namespace.owner = create(:user)
-            existing_namespace.save
-          end
-
-          it "doesn't create a project" do
+          it "creates a project using user's namespace" do
             expect(Gitlab::BitbucketImport::ProjectCreator).
-              not_to receive(:new)
+              to receive(:new).with(bitbucket_repo, bitbucket_repo.name, user.namespace, user, access_params).
+              and_return(double(execute: true))
 
-            post :create, format: :js
+            post :create, target_namespace: existing_namespace.name, format: :js
           end
         end
       end
@@ -166,7 +152,7 @@ describe Import::BitbucketController do
             expect(Gitlab::BitbucketImport::ProjectCreator).
               to receive(:new).and_return(double(execute: true))
 
-            expect { post :create, format: :js }.to change(Namespace, :count).by(1)
+            expect { post :create, target_namespace: bitbucket_repo.slug, format: :js }.to change(Namespace, :count).by(1)
           end
 
           it "takes the new namespace" do
@@ -174,7 +160,7 @@ describe Import::BitbucketController do
               to receive(:new).with(bitbucket_repo, bitbucket_repo.name, an_instance_of(Group), user, access_params).
               and_return(double(execute: true))
 
-            post :create, format: :js
+            post :create, target_namespace: bitbucket_repo.slug, format: :js
           end
         end
 
@@ -202,10 +188,15 @@ describe Import::BitbucketController do
     end
 
     context 'user has chosen an existing nested namespace and name for the project' do
-      let(:parent_namespace) { create(:namespace, name: 'foo', owner: user) }
-      let(:nested_namespace) { create(:namespace, name: 'bar', parent: parent_namespace, owner: user) }
+      let(:parent_namespace) { create(:group, name: 'foo') }
+      let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
       let(:test_name) { 'test_name' }
 
+      before do
+        parent_namespace.add_owner(user)
+        nested_namespace.add_owner(user)
+      end
+
       it 'takes the selected namespace and name' do
         expect(Gitlab::BitbucketImport::ProjectCreator).
           to receive(:new).with(bitbucket_repo, test_name, nested_namespace, user, access_params).
diff --git a/spec/controllers/import/gitlab_controller_spec.rb b/spec/controllers/import/gitlab_controller_spec.rb
index 3270ea059fa..54ea19a2969 100644
--- a/spec/controllers/import/gitlab_controller_spec.rb
+++ b/spec/controllers/import/gitlab_controller_spec.rb
@@ -82,19 +82,7 @@ describe Import::GitlabController do
             to receive(:new).with(gitlab_repo, user.namespace, user, access_params).
             and_return(double(execute: true))
 
-          post :create, format: :js
-        end
-      end
-
-      context "when the GitLab.com user and GitLab server user's usernames don't match" do
-        let(:gitlab_username) { "someone_else" }
-
-        it "takes the current user's namespace" do
-          expect(Gitlab::GitlabImport::ProjectCreator).
-            to receive(:new).with(gitlab_repo, user.namespace, user, access_params).
-            and_return(double(execute: true))
-
-          post :create, format: :js
+          post :create, target_namespace: gitlab_username, format: :js
         end
       end
     end
@@ -108,29 +96,27 @@ describe Import::GitlabController do
       end
 
       context "when a namespace with the GitLab.com user's username already exists" do
-        let!(:existing_namespace) { create(:namespace, name: other_username, owner: user) }
+        let!(:existing_namespace) { create(:group, name: other_username) }
 
         context "when the namespace is owned by the GitLab server user" do
+          before { existing_namespace.add_owner(user) }
+
           it "takes the existing namespace" do
             expect(Gitlab::GitlabImport::ProjectCreator).
               to receive(:new).with(gitlab_repo, existing_namespace, user, access_params).
               and_return(double(execute: true))
 
-            post :create, format: :js
+            post :create, target_namespace: existing_namespace.name, format: :js
           end
         end
 
         context "when the namespace is not owned by the GitLab server user" do
-          before do
-            existing_namespace.owner = create(:user)
-            existing_namespace.save
-          end
-
-          it "doesn't create a project" do
+          it "creates a project using user's namespace" do
             expect(Gitlab::GitlabImport::ProjectCreator).
-              not_to receive(:new)
+              to receive(:new).with(gitlab_repo, user.namespace, user, access_params).
+              and_return(double(execute: true))
 
-            post :create, format: :js
+            post :create, target_namespace: existing_namespace.name, format: :js
           end
         end
       end
@@ -141,7 +127,7 @@ describe Import::GitlabController do
             expect(Gitlab::GitlabImport::ProjectCreator).
               to receive(:new).and_return(double(execute: true))
 
-            expect { post :create, format: :js }.to change(Namespace, :count).by(1)
+            expect { post :create, target_namespace: gitlab_repo[:path], format: :js }.to change(Namespace, :count).by(1)
           end
 
           it "takes the new namespace" do
@@ -149,7 +135,7 @@ describe Import::GitlabController do
               to receive(:new).with(gitlab_repo, an_instance_of(Group), user, access_params).
               and_return(double(execute: true))
 
-            post :create, format: :js
+            post :create, target_namespace: gitlab_repo[:path], format: :js
           end
         end
 
@@ -176,8 +162,13 @@ describe Import::GitlabController do
       end
 
       context 'user has chosen an existing nested namespace for the project' do
-        let(:parent_namespace) { create(:namespace, name: 'foo', owner: user) }
-        let(:nested_namespace) { create(:namespace, name: 'bar', parent: parent_namespace, owner: user) }
+        let(:parent_namespace) { create(:group, name: 'foo') }
+        let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
+
+        before do
+          parent_namespace.add_owner(user)
+          nested_namespace.add_owner(user)
+        end
 
         it 'takes the selected namespace and name' do
           expect(Gitlab::GitlabImport::ProjectCreator).
diff --git a/spec/support/controllers/githubish_import_controller_shared_examples.rb b/spec/support/controllers/githubish_import_controller_shared_examples.rb
index c59b30c772d..cd70126deb8 100644
--- a/spec/support/controllers/githubish_import_controller_shared_examples.rb
+++ b/spec/support/controllers/githubish_import_controller_shared_examples.rb
@@ -114,19 +114,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do
           to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider).
             and_return(double(execute: true))
 
-        post :create, format: :js
-      end
-    end
-
-    context "when the provider user and GitLab user's usernames don't match" do
-      let(:provider_username) { "someone_else" }
-
-      it "takes the current user's namespace" do
-        expect(Gitlab::GithubImport::ProjectCreator).
-          to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider).
-            and_return(double(execute: true))
-
-        post :create, format: :js
+        post :create, target_namespace: provider_username, format: :js
       end
     end
   end
@@ -140,30 +128,27 @@ shared_examples 'a GitHub-ish import controller: POST create' do
     end
 
     context "when a namespace with the provider user's username already exists" do
-      let!(:existing_namespace) { create(:namespace, name: other_username, owner: user) }
+      let!(:existing_namespace) { create(:group, name: other_username) }
 
       context "when the namespace is owned by the GitLab user" do
+        before { existing_namespace.add_owner(user) }
+
         it "takes the existing namespace" do
           expect(Gitlab::GithubImport::ProjectCreator).
             to receive(:new).with(provider_repo, provider_repo.name, existing_namespace, user, access_params, type: provider).
               and_return(double(execute: true))
 
-          post :create, format: :js
+          post :create, target_namespace: existing_namespace.name, format: :js
         end
       end
 
       context "when the namespace is not owned by the GitLab user" do
-        before do
-          existing_namespace.owner = create(:user)
-          existing_namespace.save
-        end
-
         it "creates a project using user's namespace" do
           expect(Gitlab::GithubImport::ProjectCreator).
             to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider).
               and_return(double(execute: true))
 
-          post :create, format: :js
+          post :create, target_namespace: existing_namespace.name, format: :js
         end
       end
     end
@@ -203,15 +188,17 @@ shared_examples 'a GitHub-ish import controller: POST create' do
             to receive(:new).with(provider_repo, provider_repo.name, user.namespace, user, access_params, type: provider).
               and_return(double(execute: true))
 
-          post :create, format: :js
+          post :create, target_namespace: provider_username, format: :js
         end
       end
     end
 
     context 'user has chosen a namespace and name for the project' do
-      let(:test_namespace) { create(:namespace, name: 'test_namespace', owner: user) }
+      let(:test_namespace) { create(:group, name: 'test_namespace') }
       let(:test_name) { 'test_name' }
 
+      before { test_namespace.add_owner(user) }
+
       it 'takes the selected namespace and name' do
         expect(Gitlab::GithubImport::ProjectCreator).
           to receive(:new).with(provider_repo, test_name, test_namespace, user, access_params, type: provider).
@@ -219,21 +206,18 @@ shared_examples 'a GitHub-ish import controller: POST create' do
 
         post :create, { target_namespace: test_namespace.name, new_name: test_name, format: :js }
       end
-
-      it 'takes the selected name and default namespace' do
-        expect(Gitlab::GithubImport::ProjectCreator).
-          to receive(:new).with(provider_repo, test_name, user.namespace, user, access_params, type: provider).
-            and_return(double(execute: true))
-
-        post :create, { new_name: test_name, format: :js }
-      end
     end
 
     context 'user has chosen an existing nested namespace and name for the project' do
-      let(:parent_namespace) { create(:namespace, name: 'foo', owner: user) }
-      let(:nested_namespace) { create(:namespace, name: 'bar', parent: parent_namespace, owner: user) }
+      let(:parent_namespace) { create(:group, name: 'foo') }
+      let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
       let(:test_name) { 'test_name' }
 
+      before do
+        parent_namespace.add_owner(user)
+        nested_namespace.add_owner(user)
+      end
+
       it 'takes the selected namespace and name' do
         expect(Gitlab::GithubImport::ProjectCreator).
           to receive(:new).with(provider_repo, test_name, nested_namespace, user, access_params, type: provider).
@@ -276,7 +260,9 @@ shared_examples 'a GitHub-ish import controller: POST create' do
 
     context 'user has chosen existent and non-existent nested namespaces and name for the project' do
       let(:test_name) { 'test_name' }
-      let!(:parent_namespace) { create(:namespace, name: 'foo', owner: user) }
+      let!(:parent_namespace) { create(:group, name: 'foo') }
+
+      before { parent_namespace.add_owner(user) }
 
       it 'takes the selected namespace and name' do
         expect(Gitlab::GithubImport::ProjectCreator).