Use Projects::CreateService to fork projects so that after-create hooks are run.forked-event

6 files changed, 42 insertions, 69 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 293ee04f228..397232e98d8 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -686,11 +686,21 @@ class Project < ActiveRecord::Base
   end
 
   def create_repository
-    if gitlab_shell.add_repository(path_with_namespace)
-      true
+    if forked?
+      if gitlab_shell.fork_repository(forked_from_project.path_with_namespace, self.namespace.path)
+        ensure_satellite_exists
+        true
+      else
+        errors.add(:base, 'Failed to fork repository')
+        false
+      end
     else
-      errors.add(:base, 'Failed to create repository')
-      false
+      if gitlab_shell.add_repository(path_with_namespace)
+        true
+      else
+        errors.add(:base, 'Failed to create repository')
+        false
+      end
     end
   end
 
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index a7afcf8f64b..011f6f6145e 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -5,6 +5,8 @@ module Projects
     end
 
     def execute
+      forked_from_project_id = params.delete(:forked_from_project_id)
+
       @project = Project.new(params)
 
       # Make sure that the user is allowed to use the specified visibility
@@ -45,10 +47,14 @@ module Projects
 
       @project.creator = current_user
 
+      if forked_from_project_id
+        @project.build_forked_project_link(forked_from_project_id: forked_from_project_id)
+      end
+
       Project.transaction do
         @project.save
 
-        unless @project.import?
+        if @project.persisted? && !@project.import?
           unless @project.create_repository
             raise 'Failed to create repository'
           end
diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb
index 1e4deb6ed39..50f208b11d1 100644
--- a/app/services/projects/fork_service.rb
+++ b/app/services/projects/fork_service.rb
@@ -1,66 +1,28 @@
 module Projects
   class ForkService < BaseService
-    include Gitlab::ShellAdapter
-
     def execute
-      @from_project = @project
-
-      project_params = {
-        visibility_level: @from_project.visibility_level,
-        description: @from_project.description,
+      new_params = {
+        forked_from_project_id: @project.id,
+        visibility_level:       @project.visibility_level,
+        description:            @project.description,
+        name:                   @project.name,
+        path:                   @project.path,
+        namespace_id:           @params[:namespace].try(:id) || current_user.namespace.id
       }
 
-      project = Project.new(project_params)
-      project.name = @from_project.name
-      project.path = @from_project.path
-      project.creator = @current_user
-      if @from_project.avatar.present? && @from_project.avatar.image?
-        project.avatar = @from_project.avatar
-      end
-
-      if namespace = @params[:namespace]
-        project.namespace = namespace
-      else
-        project.namespace = @current_user.namespace
+      if @project.avatar.present? && @project.avatar.image?
+        new_params[:avatar] = @project.avatar
       end
 
-      unless @current_user.can?(:create_projects, project.namespace)
-        project.errors.add(:namespace, 'insufficient access rights')
-        return project
-      end
-
-      # If the project cannot save, we do not want to trigger the project destroy
-      # as this can have the side effect of deleting a repo attached to an existing
-      # project with the same name and namespace
-      if project.valid?
-        begin
-          Project.transaction do
-            #First save the DB entries as they can be rolled back if the repo fork fails
-            project.build_forked_project_link(forked_to_project_id: project.id, forked_from_project_id: @from_project.id)
-            if project.save
-              project.team << [@current_user, :master, @current_user]
-            end
-
-            #Now fork the repo
-            unless gitlab_shell.fork_repository(@from_project.path_with_namespace, project.namespace.path)
-              raise 'forking failed in gitlab-shell'
-            end
-
-            project.ensure_satellite_exists
-          end
+      new_project = CreateService.new(current_user, new_params).execute
 
-          if @from_project.gitlab_ci?
-            ForkRegistrationWorker.perform_async(@from_project.id, project.id, @current_user.private_token)
-          end
-        rescue => ex
-          project.errors.add(:base, 'Fork transaction failed.')
-          project.destroy
+      if new_project.persisted?
+        if @project.gitlab_ci?
+          ForkRegistrationWorker.perform_async(@project.id, new_project.id, @current_user.private_token)
         end
-      else
-        project.errors.add(:base, 'Invalid fork destination')
       end
 
-      project
+      new_project
     end
   end
 end
diff --git a/app/views/events/event/_created_project.html.haml b/app/views/events/event/_created_project.html.haml
index 552525f4a07..c2577a24982 100644
--- a/app/views/events/event/_created_project.html.haml
+++ b/app/views/events/event/_created_project.html.haml
@@ -18,7 +18,7 @@
           %a.twitter-share-button{ |
             href: "https://twitter.com/share", |
             "data-url" => event.project.web_url, |
-            "data-text" => "I just #{event.project.imported? ? "imported" : "created"} a new project in GitLab! GitLab is version control on your server.", |
+            "data-text" => "I just #{event.action_name} a new project on GitLab! GitLab is version control on your server.", |
             "data-size" => "medium", |
             "data-related" => "gitlab", |
             "data-hashtags" => "gitlab", |
diff --git a/spec/requests/api/fork_spec.rb b/spec/requests/api/fork_spec.rb
index fb3ff552c8d..7a784796031 100644
--- a/spec/requests/api/fork_spec.rb
+++ b/spec/requests/api/fork_spec.rb
@@ -50,7 +50,6 @@ describe API::API, api: true  do
       it 'should fail if forked project exists in the user namespace' do
         post api("/projects/fork/#{project.id}", user)
         expect(response.status).to eq(409)
-        expect(json_response['message']['base']).to eq(['Invalid fork destination'])
         expect(json_response['message']['name']).to eq(['has already been taken'])
         expect(json_response['message']['path']).to eq(['has already been taken'])
       end
diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb
index c9025bdf133..f158ac87e2b 100644
--- a/spec/services/projects/fork_service_spec.rb
+++ b/spec/services/projects/fork_service_spec.rb
@@ -27,7 +27,7 @@ describe Projects::ForkService do
       it "fails due to transaction failure" do
         @to_project = fork_project(@from_project, @to_user, false)
         expect(@to_project.errors).not_to be_empty
-        expect(@to_project.errors[:base]).to include("Fork transaction failed.")
+        expect(@to_project.errors[:base]).to include("Failed to fork repository")
       end
     end
 
@@ -36,8 +36,8 @@ describe Projects::ForkService do
         @existing_project = create(:project, creator_id: @to_user.id, name: @from_project.name, namespace: @to_namespace)
         @to_project = fork_project(@from_project, @to_user)
         expect(@existing_project.persisted?).to be_truthy
-        expect(@to_project.errors[:base]).to include("Invalid fork destination")
-        expect(@to_project.errors[:base]).not_to include("Fork transaction failed.")
+        expect(@to_project.errors[:name]).to eq(['has already been taken'])
+        expect(@to_project.errors[:path]).to eq(['has already been taken'])
       end
     end
 
@@ -81,7 +81,7 @@ describe Projects::ForkService do
     context 'fork project for group when user not owner' do
       it 'group developer should fail to fork project into the group' do
         to_project = fork_project(@project, @developer, true, @opts)
-        expect(to_project.errors[:namespace]).to eq(['insufficient access rights'])
+        expect(to_project.errors[:namespace]).to eq(['is not valid'])
       end
     end
 
@@ -91,7 +91,6 @@ describe Projects::ForkService do
                                             namespace: @group)
         to_project = fork_project(@project, @group_owner, true, @opts)
         expect(existing_project.persisted?).to be_truthy
-        expect(to_project.errors[:base]).to eq(['Invalid fork destination'])
         expect(to_project.errors[:name]).to eq(['has already been taken'])
         expect(to_project.errors[:path]).to eq(['has already been taken'])
       end
@@ -99,10 +98,7 @@ describe Projects::ForkService do
   end
 
   def fork_project(from_project, user, fork_success = true, params = {})
-    context = Projects::ForkService.new(from_project, user, params)
-    shell = double('gitlab_shell')
-    shell.stub(fork_repository: fork_success)
-    context.stub(gitlab_shell: shell)
-    context.execute
+    allow_any_instance_of(Gitlab::Shell).to receive(:fork_repository).and_return(fork_success)
+    Projects::ForkService.new(from_project, user, params).execute
   end
 end