Commit 5255a54d authored by James Lopez's avatar James Lopez

refactored some stuff based on MR feedback

parent 20e79f71
...@@ -106,7 +106,7 @@ class GitPushService < BaseService ...@@ -106,7 +106,7 @@ class GitPushService < BaseService
def build_push_data def build_push_data
@push_data ||= Gitlab::PushDataBuilder. @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 end
def push_to_existing_branch? def push_to_existing_branch?
...@@ -128,7 +128,7 @@ class GitPushService < BaseService ...@@ -128,7 +128,7 @@ class GitPushService < BaseService
def is_default_branch? def is_default_branch?
Gitlab::Git.branch_ref?(params[:ref]) && 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 end
def commit_user(commit) def commit_user(commit)
...@@ -136,6 +136,6 @@ class GitPushService < BaseService ...@@ -136,6 +136,6 @@ class GitPushService < BaseService
end end
def branch_name def branch_name
@_branch_name ||= Gitlab::Git.ref_name(params[:ref]) @branch_name ||= Gitlab::Git.ref_name(params[:ref])
end end
end end
...@@ -38,12 +38,7 @@ class PostReceive ...@@ -38,12 +38,7 @@ class PostReceive
if Gitlab::Git.tag_ref?(ref) if Gitlab::Git.tag_ref?(ref)
GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref)
else else
GitPushService.new(project, @user, GitPushService.new(project, @user, oldrev: oldrev, newrev: newrev, ref: ref).execute
{
oldrev: oldrev,
newrev: newrev,
ref: ref
}).execute
end end
end end
end end
......
...@@ -16,8 +16,7 @@ describe GitPushService, services: true do ...@@ -16,8 +16,7 @@ describe GitPushService, services: true do
describe 'Push branches' do describe 'Push branches' do
context 'new branch' do context 'new branch' do
subject do subject do
service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @blankrev, @newrev, @ref )
service.execute
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -37,8 +36,7 @@ describe GitPushService, services: true do ...@@ -37,8 +36,7 @@ describe GitPushService, services: true do
context 'existing branch' do context 'existing branch' do
subject do subject do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -52,8 +50,7 @@ describe GitPushService, services: true do ...@@ -52,8 +50,7 @@ describe GitPushService, services: true do
context 'rm branch' do context 'rm branch' do
subject do subject do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @blankrev, ref: @ref }) execute_service(project, user, @oldrev, @blankrev, @ref )
service.execute
end end
it { is_expected.to be_truthy } it { is_expected.to be_truthy }
...@@ -74,8 +71,7 @@ describe GitPushService, services: true do ...@@ -74,8 +71,7 @@ describe GitPushService, services: true do
describe "Git Push Data" do describe "Git Push Data" do
before do before do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service = execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
@push_data = service.push_data @push_data = service.push_data
@commit = project.commit(@newrev) @commit = project.commit(@newrev)
end end
...@@ -137,8 +133,7 @@ describe GitPushService, services: true do ...@@ -137,8 +133,7 @@ describe GitPushService, services: true do
describe "Push Event" do describe "Push Event" do
before do before do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service = execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
@event = Event.last @event = Event.last
@push_data = service.push_data @push_data = service.push_data
end end
...@@ -152,8 +147,7 @@ describe GitPushService, services: true do ...@@ -152,8 +147,7 @@ describe GitPushService, services: true do
it "when pushing a new branch for the first time" do it "when pushing a new branch for the first time" do
expect(project).to receive(:update_merge_requests). expect(project).to receive(:update_merge_requests).
with(@blankrev, 'newrev', 'refs/heads/master', user) with(@blankrev, 'newrev', 'refs/heads/master', user)
service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
service.execute
end end
end end
end end
...@@ -164,8 +158,7 @@ describe GitPushService, services: true do ...@@ -164,8 +158,7 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master") expect(project.default_branch).to eq("master")
expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false }) 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' }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
service.execute
end end
it "when pushing a branch for the first time with default branch protection disabled" do it "when pushing a branch for the first time with default branch protection disabled" do
...@@ -174,8 +167,7 @@ describe GitPushService, services: true do ...@@ -174,8 +167,7 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master") expect(project.default_branch).to eq("master")
expect(project.protected_branches).not_to receive(:create) expect(project.protected_branches).not_to receive(:create)
service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
service.execute
end end
it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do 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 ...@@ -184,14 +176,12 @@ describe GitPushService, services: true do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
expect(project.default_branch).to eq("master") expect(project.default_branch).to eq("master")
expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true }) 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' }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
service.execute
end end
it "when pushing new commits to existing branch" do it "when pushing new commits to existing branch" do
expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_hooks)
service = described_class.new(project, user, { oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/heads/master' }) execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' )
service.execute
end end
end end
end end
...@@ -214,9 +204,7 @@ describe GitPushService, services: true do ...@@ -214,9 +204,7 @@ describe GitPushService, services: true do
it "creates a note if a pushed commit mentions an issue" do it "creates a note if a pushed commit mentions an issue" do
expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author)
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it "only creates a cross-reference note if one doesn't already exist" do it "only creates a cross-reference note if one doesn't already exist" do
...@@ -224,9 +212,7 @@ describe GitPushService, services: true do ...@@ -224,9 +212,7 @@ describe GitPushService, services: true do
expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author) expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author)
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it "defaults to the pushing user if the commit's author is not known" do it "defaults to the pushing user if the commit's author is not known" do
...@@ -236,9 +222,7 @@ describe GitPushService, services: true do ...@@ -236,9 +222,7 @@ describe GitPushService, services: true do
) )
expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user)
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it "finds references in the first push to a non-default branch" do it "finds references in the first push to a non-default branch" do
...@@ -247,9 +231,7 @@ describe GitPushService, services: true do ...@@ -247,9 +231,7 @@ describe GitPushService, services: true do
expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) 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' }) execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' )
service.execute
end end
end end
...@@ -273,21 +255,18 @@ describe GitPushService, services: true do ...@@ -273,21 +255,18 @@ describe GitPushService, services: true do
context "to default branches" do context "to default branches" do
it "closes issues" do it "closes issues" do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
expect(Issue.find(issue.id)).to be_closed expect(Issue.find(issue.id)).to be_closed
end end
it "adds a note indicating that the issue is now closed" do 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) 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 }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it "doesn't create additional cross-reference notes" do it "doesn't create additional cross-reference notes" do
expect(SystemNoteService).not_to receive(:cross_reference) expect(SystemNoteService).not_to receive(:cross_reference)
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it "doesn't close issues when external issue tracker is in use" do it "doesn't close issues when external issue tracker is in use" do
...@@ -295,8 +274,7 @@ describe GitPushService, services: true do ...@@ -295,8 +274,7 @@ describe GitPushService, services: true do
# The push still shouldn't create cross-reference notes. # The push still shouldn't create cross-reference notes.
expect do expect do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: 'refs/heads/hurf' }) execute_service(project, user, @oldrev, @newrev, 'refs/heads/hurf' )
service.execute
end.not_to change { Note.where(project_id: project.id, system: true).count } end.not_to change { Note.where(project_id: project.id, system: true).count }
end end
end end
...@@ -309,13 +287,11 @@ describe GitPushService, services: true do ...@@ -309,13 +287,11 @@ describe GitPushService, services: true do
it "creates cross-reference notes" do it "creates cross-reference notes" do
expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author) expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author)
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
end end
it "doesn't close issues" do it "doesn't close issues" do
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
expect(Issue.find(issue.id)).to be_opened expect(Issue.find(issue.id)).to be_opened
end end
end end
...@@ -352,8 +328,7 @@ describe GitPushService, services: true do ...@@ -352,8 +328,7 @@ describe GitPushService, services: true do
let(:message) { "this is some work.\n\nrelated to JIRA-1" } 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 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 }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
expect(WebMock).to have_requested(:post, jira_api_comment_url).with( expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
body: /mentioned this issue in/ body: /mentioned this issue in/
...@@ -371,9 +346,7 @@ describe GitPushService, services: true do ...@@ -371,9 +346,7 @@ describe GitPushService, services: true do
} }
}.to_json }.to_json
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
expect(WebMock).to have_requested(:post, jira_api_transition_url).with( expect(WebMock).to have_requested(:post, jira_api_transition_url).with(
body: transition_body body: transition_body
).once ).once
...@@ -384,9 +357,7 @@ describe GitPushService, services: true do ...@@ -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}]." body: "Issue solved with [#{closing_commit.id}|http://localhost/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
}.to_json }.to_json
service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) execute_service(project, user, @oldrev, @newrev, @ref )
service.execute
expect(WebMock).to have_requested(:post, jira_api_comment_url).with( expect(WebMock).to have_requested(:post, jira_api_comment_url).with(
body: comment_body body: comment_body
).once ).once
...@@ -405,8 +376,13 @@ describe GitPushService, services: true do ...@@ -405,8 +376,13 @@ describe GitPushService, services: true do
end end
it 'push to first branch updates HEAD' do it 'push to first branch updates HEAD' do
service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: new_ref }) execute_service(project, user, @blankrev, @newrev, new_ref )
service.execute
end end
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 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