From 14326c88f7710bd0ffc7b148a87c6dc2188e2683 Mon Sep 17 00:00:00 2001 From: Rydkin Maxim Date: Thu, 5 Jan 2017 18:25:45 +0300 Subject: [PATCH] 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` --- app/services/merge_requests/build_service.rb | 137 ++++++++++-------- ...5_refactor_merge_request_build_service.yml | 4 + .../user_uses_slash_commands_spec.rb | 2 +- 3 files changed, 81 insertions(+), 62 deletions(-) create mode 100644 changelogs/unreleased/24795_refactor_merge_request_build_service.yml diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 6a7393a9921..1d6d2754559 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -1,62 +1,88 @@ module MergeRequests class BuildService < MergeRequests::BaseService def execute - merge_request = MergeRequest.new(params) - - # Set MR attributes - merge_request.can_be_created = true + self.merge_request = MergeRequest.new(params) + merge_request.can_be_created = true 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 - merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project) + if branches_specified? && branches_valid? + compare_branches + assign_title_and_description + else + merge_request.can_be_created = false + end - merge_request.target_project ||= (project.forked_from_project || project) - merge_request.target_branch ||= merge_request.target_project.default_branch - - messages = validate_branches(merge_request) - return build_failed(merge_request, messages) unless messages.empty? - - compare = CompareService.new.execute( - merge_request.source_project, - merge_request.source_branch, - merge_request.target_project, - merge_request.target_branch, - ) - - merge_request.compare_commits = compare.commits - merge_request.compare = compare - - set_title_and_description(merge_request) + merge_request end private - def validate_branches(merge_request) - messages = [] + attr_accessor :merge_request - if merge_request.target_branch.blank? || merge_request.source_branch.blank? - messages << - if params[:source_branch] || params[:target_branch] - "You must select source and target branch" - end - end + delegate :target_branch, :source_branch, :source_project, :target_project, :compare_commits, :wip_title, :description, :errors, to: :merge_request - if merge_request.source_project == merge_request.target_project && - merge_request.target_branch == merge_request.source_branch + def find_source_project + source_project || project + end - messages << 'You must select different branches' - 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 - # See if source and target branches exist - if merge_request.source_branch.present? && !merge_request.source_project.commit(merge_request.source_branch) - messages << "Source branch \"#{merge_request.source_branch}\" does not exist" - end + def find_target_branch + target_branch || target_project.default_branch + end - if merge_request.target_branch.present? && !merge_request.target_project.commit(merge_request.target_branch) - messages << "Target branch \"#{merge_request.target_branch}\" does not exist" - end + def branches_specified? + params[:source_branch] && params[:target_branch] + end - messages + def branches_valid? + validate_branches + errors.blank? + end + + def compare_branches + compare = CompareService.new.execute( + source_project, + source_branch, + target_project, + target_branch + ) + + merge_request.compare_commits = compare.commits + merge_request.compare = compare + end + + def validate_branches + add_error('You must select source and target branch') unless branches_present? + add_error('You must select different branches') if same_source_and_target? + 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? + end + + def add_error(message) + errors.add(:base, message) + end + + def branches_present? + target_branch.present? && source_branch.present? + end + + def same_source_and_target? + source_project == target_project && target_branch == source_branch + end + + def source_branch_exists? + source_branch.blank? || source_project.commit(source_branch) + end + + def target_branch_exists? + target_branch.blank? || target_project.commit(target_branch) end # When your branch name starts with an iid followed by a dash this pattern will be @@ -71,17 +97,17 @@ module MergeRequests # - Setting the title as 'Resolves "Emoji don't show up in commit title"' if there is # more than one commit in the MR # - def set_title_and_description(merge_request) - if match = merge_request.source_branch.match(/\A(\d+)-/) + def assign_title_and_description + if match = source_branch.match(/\A(\d+)-/) iid = match[1] end - commits = merge_request.compare_commits + commits = compare_commits if commits && commits.count == 1 commit = commits.first merge_request.title = commit.title 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 when Issue merge_request.title = "Resolve \"#{issue.title}\"" @@ -89,31 +115,20 @@ module MergeRequests merge_request.title = "Resolve #{issue.title}" end else - merge_request.title = merge_request.source_branch.titleize.humanize + merge_request.title = source_branch.titleize.humanize end if iid closes_issue = "Closes ##{iid}" - if merge_request.description.present? + if description.present? merge_request.description += closes_issue.prepend("\n\n") else merge_request.description = closes_issue end end - merge_request.title = merge_request.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 + merge_request.title = wip_title if commits.empty? end end end diff --git a/changelogs/unreleased/24795_refactor_merge_request_build_service.yml b/changelogs/unreleased/24795_refactor_merge_request_build_service.yml new file mode 100644 index 00000000000..b735fb57649 --- /dev/null +++ b/changelogs/unreleased/24795_refactor_merge_request_build_service.yml @@ -0,0 +1,4 @@ +--- +title: Refactor MergeRequests::BuildService +merge_request: 8462 +author: Rydkin Maxim diff --git a/spec/features/merge_requests/user_uses_slash_commands_spec.rb b/spec/features/merge_requests/user_uses_slash_commands_spec.rb index b13674b4db9..2582a540240 100644 --- a/spec/features/merge_requests/user_uses_slash_commands_spec.rb +++ b/spec/features/merge_requests/user_uses_slash_commands_spec.rb @@ -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 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 describe 'merge-request-only commands' do