From 5255a54df9778b107734a84acb85230d62d3cff7 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 17 Feb 2016 10:42:59 +0100 Subject: [PATCH] refactored some stuff based on MR feedback --- app/services/git_push_service.rb | 6 +- app/workers/post_receive.rb | 7 +-- spec/services/git_push_service_spec.rb | 84 +++++++++----------------- 3 files changed, 34 insertions(+), 63 deletions(-) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index b203065e708..a1711d234ff 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -106,7 +106,7 @@ class GitPushService < BaseService def build_push_data @push_data ||= Gitlab::PushDataBuilder. - build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits) + build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits) end def push_to_existing_branch? @@ -128,7 +128,7 @@ class GitPushService < BaseService def is_default_branch? Gitlab::Git.branch_ref?(params[:ref]) && - (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) + (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) end def commit_user(commit) @@ -136,6 +136,6 @@ class GitPushService < BaseService end def branch_name - @_branch_name ||= Gitlab::Git.ref_name(params[:ref]) + @branch_name ||= Gitlab::Git.ref_name(params[:ref]) end end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 9e7934b84d8..14d7813412e 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -38,12 +38,7 @@ class PostReceive if Gitlab::Git.tag_ref?(ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) else - GitPushService.new(project, @user, - { - oldrev: oldrev, - newrev: newrev, - ref: ref - }).execute + GitPushService.new(project, @user, oldrev: oldrev, newrev: newrev, ref: ref).execute end end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index f953f226e67..48e796b6946 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -16,8 +16,7 @@ describe GitPushService, services: true do describe 'Push branches' do context 'new branch' do subject do - service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @blankrev, @newrev, @ref ) end it { is_expected.to be_truthy } @@ -37,8 +36,7 @@ describe GitPushService, services: true do context 'existing branch' do subject do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it { is_expected.to be_truthy } @@ -52,8 +50,7 @@ describe GitPushService, services: true do context 'rm branch' do subject do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @blankrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @blankrev, @ref ) end it { is_expected.to be_truthy } @@ -74,8 +71,7 @@ describe GitPushService, services: true do describe "Git Push Data" do before do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + service = execute_service(project, user, @oldrev, @newrev, @ref ) @push_data = service.push_data @commit = project.commit(@newrev) end @@ -137,8 +133,7 @@ describe GitPushService, services: true do describe "Push Event" do before do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + service = execute_service(project, user, @oldrev, @newrev, @ref ) @event = Event.last @push_data = service.push_data end @@ -152,8 +147,7 @@ describe GitPushService, services: true do it "when pushing a new branch for the first time" do expect(project).to receive(:update_merge_requests). with(@blankrev, 'newrev', 'refs/heads/master', user) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end end end @@ -164,8 +158,7 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end it "when pushing a branch for the first time with default branch protection disabled" do @@ -174,8 +167,7 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).not_to receive(:create) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do @@ -184,14 +176,12 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) end it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - service = described_class.new(project, user, { oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/heads/master' }) - service.execute + execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' ) end end end @@ -214,9 +204,7 @@ describe GitPushService, services: true do it "creates a note if a pushed commit mentions an issue" do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "only creates a cross-reference note if one doesn't already exist" do @@ -224,9 +212,7 @@ describe GitPushService, services: true do expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "defaults to the pushing user if the commit's author is not known" do @@ -236,9 +222,7 @@ describe GitPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "finds references in the first push to a non-default branch" do @@ -247,9 +231,7 @@ describe GitPushService, services: true do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: 'refs/heads/other' }) - - service.execute + execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' ) end end @@ -273,21 +255,18 @@ describe GitPushService, services: true do context "to default branches" do it "closes issues" do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(Issue.find(issue.id)).to be_closed end it "adds a note indicating that the issue is now closed" do expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "doesn't close issues when external issue tracker is in use" do @@ -295,8 +274,7 @@ describe GitPushService, services: true do # The push still shouldn't create cross-reference notes. expect do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: 'refs/heads/hurf' }) - service.execute + execute_service(project, user, @oldrev, @newrev, 'refs/heads/hurf' ) end.not_to change { Note.where(project_id: project.id, system: true).count } end end @@ -309,13 +287,11 @@ describe GitPushService, services: true do it "creates cross-reference notes" do expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) end it "doesn't close issues" do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(Issue.find(issue.id)).to be_opened end end @@ -352,8 +328,7 @@ describe GitPushService, services: true do let(:message) { "this is some work.\n\nrelated to JIRA-1" } it "should initiate one api call to jira server to mention the issue" do - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: /mentioned this issue in/ @@ -371,9 +346,7 @@ describe GitPushService, services: true do } }.to_json - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(WebMock).to have_requested(:post, jira_api_transition_url).with( body: transition_body ).once @@ -384,9 +357,7 @@ describe GitPushService, services: true do body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json - service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) - - service.execute + execute_service(project, user, @oldrev, @newrev, @ref ) expect(WebMock).to have_requested(:post, jira_api_comment_url).with( body: comment_body ).once @@ -405,8 +376,13 @@ describe GitPushService, services: true do end it 'push to first branch updates HEAD' do - service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: new_ref }) - service.execute + execute_service(project, user, @blankrev, @newrev, new_ref ) end end + + def execute_service(project, user, oldrev, newrev, ref) + service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref ) + service.execute + service + end end