Merge branch 'improve/mr_service' into 'master'

Improve merge request accepting logic

Add locked state to merge request during `accept` action.
Prevents mr being `triggered as merged` twice (hooks, emails etc)
Fixes #998 #729

6 files changed, 93 insertions, 32 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 712d01626b5..c6d2f74f2b8 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -50,17 +50,26 @@ class MergeRequest < ActiveRecord::Base
     end
 
     event :merge do
-      transition [:reopened, :opened] => :merged
+      transition [:reopened, :opened, :locked] => :merged
     end
 
     event :reopen do
       transition closed: :reopened
     end
 
+    event :lock do
+      transition [:reopened, :opened] => :locked
+    end
+
+    event :unlock do
+      transition locked: :reopened
+    end
+
     state :opened
     state :reopened
     state :closed
     state :merged
+    state :locked
   end
 
   state_machine :merge_status, initial: :unchecked do
@@ -117,7 +126,9 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def reload_code
-    merge_request_diff.reload_content if opened?
+    if merge_request_diff && opened?
+      merge_request_diff.reload_content
+    end
   end
 
   def check_if_can_be_merged
@@ -136,19 +147,8 @@ class MergeRequest < ActiveRecord::Base
     self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
   end
 
-  def merge!(user_id)
-    self.author_id_of_changes = user_id
-    self.merge
-  end
-
   def automerge!(current_user, commit_message = nil)
-    if Gitlab::Satellite::MergeAction.new(current_user, self).merge!(commit_message)
-      self.merge!(current_user.id)
-      true
-    end
-  rescue
-    mark_as_unmergeable
-    false
+    MergeRequests::AutoMergeService.new.execute(self, current_user, commit_message)
   end
 
   def mr_and_commit_notes
diff --git a/app/models/project.rb b/app/models/project.rb
index 579644d1119..d9da2c377c8 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -350,7 +350,7 @@ class Project < ActiveRecord::Base
     # Close merge requests
     mrs = self.merge_requests.opened.where(target_branch: branch_name).to_a
     mrs = mrs.select(&:last_commit).select { |mr| c_ids.include?(mr.last_commit.id) }
-    mrs.each { |merge_request| merge_request.merge!(user.id) }
+    mrs.each { |merge_request| MergeRequests::MergeService.new.execute(merge_request, user, nil) }
 
     true
   end
diff --git a/app/observers/merge_request_observer.rb b/app/observers/merge_request_observer.rb
index 0ac555fce7c..ef31498e7d0 100644
--- a/app/observers/merge_request_observer.rb
+++ b/app/observers/merge_request_observer.rb
@@ -18,23 +18,6 @@ class MergeRequestObserver < ActivityObserver
     execute_hooks(merge_request)
   end
 
-  def after_merge(merge_request, transition)
-    notification.merge_mr(merge_request)
-    # Since MR can be merged via sidekiq
-    # to prevent event duplication do this check
-    return true if merge_request.merge_event
-
-    Event.create(
-      project: merge_request.target_project,
-      target_id: merge_request.id,
-      target_type: merge_request.class.name,
-      action: Event::MERGED,
-      author_id: merge_request.author_id_of_changes
-    )
-
-    execute_hooks(merge_request)
-  end
-
   def after_reopen(merge_request, transition)
     create_event(merge_request, Event::REOPENED)
     create_note(merge_request)
diff --git a/app/services/merge_requests/auto_merge_service.rb b/app/services/merge_requests/auto_merge_service.rb
new file mode 100644
index 00000000000..d60d61ed54a
--- /dev/null
+++ b/app/services/merge_requests/auto_merge_service.rb
@@ -0,0 +1,30 @@
+module MergeRequests
+  # AutoMergeService class
+  #
+  # Do git merge in satellite and in case of success
+  # mark merge request as merged and execute all hooks and notifications
+  # Called when you do merge via GitLab UI
+  class AutoMergeService < BaseMergeService
+    def execute(merge_request, current_user, commit_message)
+      merge_request.lock
+
+      if Gitlab::Satellite::MergeAction.new(current_user, merge_request).merge!(commit_message)
+        merge_request.author_id_of_changes = current_user.id
+        merge_request.merge
+
+        notification.merge_mr(merge_request)
+        create_merge_event(merge_request)
+        execute_project_hooks(merge_request)
+
+        true
+      else
+        merge_request.unlock
+        false
+      end
+    rescue
+      merge_request.unlock if merge_request.locked?
+      merge_request.mark_as_unmergeable
+      false
+    end
+  end
+end
diff --git a/app/services/merge_requests/base_merge_service.rb b/app/services/merge_requests/base_merge_service.rb
new file mode 100644
index 00000000000..dbdb0063074
--- /dev/null
+++ b/app/services/merge_requests/base_merge_service.rb
@@ -0,0 +1,26 @@
+module MergeRequests
+  class BaseMergeService
+
+    private
+
+    def notification
+      NotificationService.new
+    end
+
+    def create_merge_event(merge_request)
+      Event.create(
+        project: merge_request.target_project,
+        target_id: merge_request.id,
+        target_type: merge_request.class.name,
+        action: Event::MERGED,
+        author_id: merge_request.author_id_of_changes
+      )
+    end
+
+    def execute_project_hooks(merge_request)
+      if merge_request.project
+        merge_request.project.execute_hooks(merge_request.to_hook_data, :merge_request_hooks)
+      end
+    end
+  end
+end
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
new file mode 100644
index 00000000000..1d5af04cdbb
--- /dev/null
+++ b/app/services/merge_requests/merge_service.rb
@@ -0,0 +1,22 @@
+module MergeRequests
+  # MergeService class
+  #
+  # Mark existing merge request as merged
+  # and execute all hooks and notifications
+  # Called when you do merge via command line and push code
+  # to target branch
+  class MergeService < BaseMergeService
+    def execute(merge_request, current_user, commit_message)
+      merge_request.author_id_of_changes = current_user.id
+      merge_request.merge
+
+      notification.merge_mr(merge_request)
+      create_merge_event(merge_request)
+      execute_project_hooks(merge_request)
+
+      true
+    rescue
+      false
+    end
+  end
+end