Commit baccb9a4 authored by Dmitriy Zaporozhets's avatar Dmitriy Zaporozhets

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
parents d1d13856 1338c7dd
...@@ -50,17 +50,26 @@ class MergeRequest < ActiveRecord::Base ...@@ -50,17 +50,26 @@ class MergeRequest < ActiveRecord::Base
end end
event :merge do event :merge do
transition [:reopened, :opened] => :merged transition [:reopened, :opened, :locked] => :merged
end end
event :reopen do event :reopen do
transition closed: :reopened transition closed: :reopened
end end
event :lock do
transition [:reopened, :opened] => :locked
end
event :unlock do
transition locked: :reopened
end
state :opened state :opened
state :reopened state :reopened
state :closed state :closed
state :merged state :merged
state :locked
end end
state_machine :merge_status, initial: :unchecked do state_machine :merge_status, initial: :unchecked do
...@@ -117,7 +126,9 @@ class MergeRequest < ActiveRecord::Base ...@@ -117,7 +126,9 @@ class MergeRequest < ActiveRecord::Base
end end
def reload_code def reload_code
merge_request_diff.reload_content if opened? if merge_request_diff && opened?
merge_request_diff.reload_content
end
end end
def check_if_can_be_merged def check_if_can_be_merged
...@@ -136,19 +147,8 @@ class MergeRequest < ActiveRecord::Base ...@@ -136,19 +147,8 @@ class MergeRequest < ActiveRecord::Base
self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
end end
def merge!(user_id)
self.author_id_of_changes = user_id
self.merge
end
def automerge!(current_user, commit_message = nil) def automerge!(current_user, commit_message = nil)
if Gitlab::Satellite::MergeAction.new(current_user, self).merge!(commit_message) MergeRequests::AutoMergeService.new.execute(self, current_user, commit_message)
self.merge!(current_user.id)
true
end
rescue
mark_as_unmergeable
false
end end
def mr_and_commit_notes def mr_and_commit_notes
......
...@@ -350,7 +350,7 @@ class Project < ActiveRecord::Base ...@@ -350,7 +350,7 @@ class Project < ActiveRecord::Base
# Close merge requests # Close merge requests
mrs = self.merge_requests.opened.where(target_branch: branch_name).to_a 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 = 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 true
end end
......
...@@ -18,23 +18,6 @@ class MergeRequestObserver < ActivityObserver ...@@ -18,23 +18,6 @@ class MergeRequestObserver < ActivityObserver
execute_hooks(merge_request) execute_hooks(merge_request)
end 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) def after_reopen(merge_request, transition)
create_event(merge_request, Event::REOPENED) create_event(merge_request, Event::REOPENED)
create_note(merge_request) create_note(merge_request)
......
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
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
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment