Commit 14326c88 authored by Rydkin Maxim's avatar Rydkin Maxim

refactor merge request build service

add changelog entry

replace local variables `merge_request` with instance ones

modify `MergeRequests::BuildService` to fix failed tests

extract `assign_target_project` method

remove unnecessary instance variables

remove exclamation marks and rewrite conditionals to improve readability

extract `params_does_not_contain_branches?` method, rename `unprepared_merge_request` method to `uncreatable_merge_request`

replace instance variables `merge_request` and `error_messages` with getters and setters; divide `set_title_and_description` method on two separate ones

refactor `execute` method

return `set_title_and_description` method

rename `branches_selected?` method to `branches_present?` to make it more different from `branches_specified?`

fixes after discussion

renamed method branches_valid?

fix space

return assigning methods into `execute` method

simplify `find_target_branch` and `find_source_project` methods

fix spec `merge request issuable record that supports slash commands in its description and notes`
parent adc0e41f
module MergeRequests module MergeRequests
class BuildService < MergeRequests::BaseService class BuildService < MergeRequests::BaseService
def execute def execute
merge_request = MergeRequest.new(params) self.merge_request = MergeRequest.new(params)
merge_request.can_be_created = true
# Set MR attributes
merge_request.can_be_created = true
merge_request.compare_commits = [] merge_request.compare_commits = []
merge_request.source_project = project unless merge_request.source_project merge_request.source_project = find_source_project
merge_request.target_project = find_target_project
merge_request.target_branch = find_target_branch
if branches_specified? && branches_valid?
compare_branches
assign_title_and_description
else
merge_request.can_be_created = false
end
merge_request
end
merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project) private
merge_request.target_project ||= (project.forked_from_project || project) attr_accessor :merge_request
merge_request.target_branch ||= merge_request.target_project.default_branch
messages = validate_branches(merge_request) delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request
return build_failed(merge_request, messages) unless messages.empty?
def find_source_project
source_project || project
end
def find_target_project
return target_project if target_project.present? && can?(current_user, :read_project, target_project)
project.forked_from_project || project
end
def find_target_branch
target_branch || target_project.default_branch
end
def branches_specified?
params[:source_branch] && params[:target_branch]
end
def branches_valid?
validate_branches
errors.blank?
end
def compare_branches
compare = CompareService.new.execute( compare = CompareService.new.execute(
merge_request.source_project, source_project,
merge_request.source_branch, source_branch,
merge_request.target_project, target_project,
merge_request.target_branch, target_branch
) )
merge_request.compare_commits = compare.commits merge_request.compare_commits = compare.commits
merge_request.compare = compare merge_request.compare = compare
set_title_and_description(merge_request)
end end
private def validate_branches
add_error('You must select source and target branch') unless branches_present?
def validate_branches(merge_request) add_error('You must select different branches') if same_source_and_target?
messages = [] add_error("Source branch \"#{source_branch}\" does not exist") unless source_branch_exists?
add_error("Target branch \"#{target_branch}\" does not exist") unless target_branch_exists?
if merge_request.target_branch.blank? || merge_request.source_branch.blank? end
messages <<
if params[:source_branch] || params[:target_branch]
"You must select source and target branch"
end
end
if merge_request.source_project == merge_request.target_project && def add_error(message)
merge_request.target_branch == merge_request.source_branch errors.add(:base, message)
end
messages << 'You must select different branches' def branches_present?
end target_branch.present? && source_branch.present?
end
# See if source and target branches exist def same_source_and_target?
if merge_request.source_branch.present? && !merge_request.source_project.commit(merge_request.source_branch) source_project == target_project && target_branch == source_branch
messages << "Source branch \"#{merge_request.source_branch}\" does not exist" end
end
if merge_request.target_branch.present? && !merge_request.target_project.commit(merge_request.target_branch) def source_branch_exists?
messages << "Target branch \"#{merge_request.target_branch}\" does not exist" source_branch.blank? || source_project.commit(source_branch)
end end
messages def target_branch_exists?
target_branch.blank? || target_project.commit(target_branch)
end end
# When your branch name starts with an iid followed by a dash this pattern will be # When your branch name starts with an iid followed by a dash this pattern will be
...@@ -71,17 +97,17 @@ module MergeRequests ...@@ -71,17 +97,17 @@ module MergeRequests
# - Setting the title as 'Resolves "Emoji don't show up in commit title"' if there is # - Setting the title as 'Resolves "Emoji don't show up in commit title"' if there is
# more than one commit in the MR # more than one commit in the MR
# #
def set_title_and_description(merge_request) def assign_title_and_description
if match = merge_request.source_branch.match(/\A(\d+)-/) if match = source_branch.match(/\A(\d+)-/)
iid = match[1] iid = match[1]
end end
commits = merge_request.compare_commits commits = compare_commits
if commits && commits.count == 1 if commits && commits.count == 1
commit = commits.first commit = commits.first
merge_request.title = commit.title merge_request.title = commit.title
merge_request.description ||= commit.description.try(:strip) merge_request.description ||= commit.description.try(:strip)
elsif iid && issue = merge_request.target_project.get_issue(iid, current_user) elsif iid && issue = target_project.get_issue(iid, current_user)
case issue case issue
when Issue when Issue
merge_request.title = "Resolve \"#{issue.title}\"" merge_request.title = "Resolve \"#{issue.title}\""
...@@ -89,31 +115,20 @@ module MergeRequests ...@@ -89,31 +115,20 @@ module MergeRequests
merge_request.title = "Resolve #{issue.title}" merge_request.title = "Resolve #{issue.title}"
end end
else else
merge_request.title = merge_request.source_branch.titleize.humanize merge_request.title = source_branch.titleize.humanize
end end
if iid if iid
closes_issue = "Closes ##{iid}" closes_issue = "Closes ##{iid}"
if merge_request.description.present? if description.present?
merge_request.description += closes_issue.prepend("\n\n") merge_request.description += closes_issue.prepend("\n\n")
else else
merge_request.description = closes_issue merge_request.description = closes_issue
end end
end end
merge_request.title = merge_request.wip_title if commits.empty? merge_request.title = wip_title if commits.empty?
merge_request
end
def build_failed(merge_request, messages)
messages.compact.each do |message|
merge_request.errors.add(:base, message)
end
merge_request.compare_commits = []
merge_request.can_be_created = false
merge_request
end end
end end
end end
---
title: Refactor MergeRequests::BuildService
merge_request: 8462
author: Rydkin Maxim
...@@ -11,7 +11,7 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do ...@@ -11,7 +11,7 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
it_behaves_like 'issuable record that supports slash commands in its description and notes', :merge_request do it_behaves_like 'issuable record that supports slash commands in its description and notes', :merge_request do
let(:issuable) { create(:merge_request, source_project: project) } let(:issuable) { create(:merge_request, source_project: project) }
let(:new_url_opts) { { merge_request: { source_branch: 'feature' } } } let(:new_url_opts) { { merge_request: { source_branch: 'feature', target_branch: 'master' } } }
end end
describe 'merge-request-only commands' do describe 'merge-request-only commands' do
......
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