diff options
author | James Lopez <james@jameslopez.es> | 2016-02-15 17:51:00 +0300 |
---|---|---|
committer | James Lopez <james@jameslopez.es> | 2016-02-15 17:51:00 +0300 |
commit | 20e79f714a4b98526d5eeffc82c0de89c8369c4e (patch) | |
tree | 62c4c06036a2afb612e39778be812e34b05767ab | |
parent | 83a81ffc5df04de74b8a93737ddc54d26981baef (diff) |
refactored GitPushService and updated spec
-rw-r--r-- | app/services/git_push_service.rb | 47 | ||||
-rw-r--r-- | app/workers/post_receive.rb | 7 | ||||
-rw-r--r-- | spec/services/git_push_service_spec.rb | 48 |
3 files changed, 49 insertions, 53 deletions
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 5362dd401be..b203065e708 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -1,19 +1,10 @@ -class GitPushService - attr_accessor :project, :user, :push_data, :push_commits +class GitPushService < BaseService + attr_accessor :push_data, :push_commits include Gitlab::CurrentSettings include Gitlab::Access - def initialize(project, user, oldrev, newrev, ref) - # TODO: Consider passing an options hash instead https://github.com/bbatsov/ruby-style-guide#too-many-params - @project = project - @user = user - @oldrev = oldrev - @newrev = newrev - @ref = ref - end - # This method will be called after each git update - # and only if the provided user and project is present in GitLab. + # and only if the provided user and project are present in GitLab. # # All callbacks for post receive action should be placed here. # @@ -41,14 +32,14 @@ class GitPushService # Use the pushed commits that aren't reachable by the default branch # as a heuristic. This may include more commits than are actually pushed, but # that shouldn't matter because we check for existing cross-references later. - @push_commits = @project.repository.commits_between(@project.default_branch, @newrev) + @push_commits = @project.repository.commits_between(@project.default_branch, params[:newrev]) # don't process commits for the initial push to the default branch process_commit_messages end elsif push_to_existing_branch? # Collect data for this git push - @push_commits = @project.repository.commits_between(@oldrev, @newrev) + @push_commits = @project.repository.commits_between(params[:oldrev], params[:newrev]) process_commit_messages end # Update merge requests that may be affected by this push. A new branch @@ -59,17 +50,17 @@ class GitPushService protected def update_merge_requests - @project.update_merge_requests(@oldrev, @newrev, @ref, @user) + @project.update_merge_requests(params[:oldrev], params[:newrev], params[:ref], current_user) - EventCreateService.new.push(@project, @user, build_push_data) + EventCreateService.new.push(@project, current_user, build_push_data) @project.execute_hooks(build_push_data.dup, :push_hooks) @project.execute_services(build_push_data.dup, :push_hooks) - CreateCommitBuildsService.new.execute(@project, @user, build_push_data) + CreateCommitBuildsService.new.execute(@project, current_user, build_push_data) ProjectCacheWorker.perform_async(@project.id) end def process_default_branch - @push_commits = project.repository.commits(@newrev) + @push_commits = project.repository.commits(params[:newrev]) # Ensure HEAD points to the default branch in case it is not master project.change_head(branch_name) @@ -103,7 +94,7 @@ class GitPushService # Close issues if these commits were pushed to the project's default branch and the commit message matches the # closing regex. Exclude any mentioned Issues from cross-referencing even if the commits are being pushed to # a different branch. - closed_issues = commit.closes_issues(user) + closed_issues = commit.closes_issues(current_user) closed_issues.each do |issue| Issues::CloseService.new(project, authors[commit], {}).execute(issue, commit) end @@ -115,36 +106,36 @@ class GitPushService def build_push_data @push_data ||= Gitlab::PushDataBuilder. - build(@project, @user, @oldrev, @newrev, @ref, push_commits) + build(@project, current_user, params[:oldrev], params[:newrev], params[:ref], push_commits) end def push_to_existing_branch? # Return if this is not a push to a branch (e.g. new commits) - Gitlab::Git.branch_ref?(@ref) && !Gitlab::Git.blank_ref?(@oldrev) + Gitlab::Git.branch_ref?(params[:ref]) && !Gitlab::Git.blank_ref?(params[:oldrev]) end def push_to_new_branch? - Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@oldrev) + Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:oldrev]) end def push_remove_branch? - Gitlab::Git.branch_ref?(@ref) && Gitlab::Git.blank_ref?(@newrev) + Gitlab::Git.branch_ref?(params[:ref]) && Gitlab::Git.blank_ref?(params[:newrev]) end def push_to_branch? - Gitlab::Git.branch_ref?(@ref) + Gitlab::Git.branch_ref?(params[:ref]) end def is_default_branch? - Gitlab::Git.branch_ref?(@ref) && - (Gitlab::Git.ref_name(@ref) == project.default_branch || project.default_branch.nil?) + Gitlab::Git.branch_ref?(params[:ref]) && + (Gitlab::Git.ref_name(params[:ref]) == project.default_branch || project.default_branch.nil?) end def commit_user(commit) - commit.author || user + commit.author || current_user end def branch_name - @_branch_name ||= Gitlab::Git.ref_name(@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 80081ac5f49..9e7934b84d8 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -38,7 +38,12 @@ class PostReceive if Gitlab::Git.tag_ref?(ref) GitTagPushService.new.execute(project, @user, oldrev, newrev, ref) else - GitPushService.new(project, @user, oldrev, newrev, 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 ad18f48ff91..f953f226e67 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -16,7 +16,7 @@ describe GitPushService, services: true do describe 'Push branches' do context 'new branch' do subject do - service = described_class.new(project, user, @blankrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: @ref }) service.execute end @@ -37,7 +37,7 @@ describe GitPushService, services: true do context 'existing branch' do subject do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -52,7 +52,7 @@ describe GitPushService, services: true do context 'rm branch' do subject do - service = described_class.new(project, user, @oldrev, @blankrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @blankrev, ref: @ref }) service.execute end @@ -74,7 +74,7 @@ describe GitPushService, services: true do describe "Git Push Data" do before do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute @push_data = service.push_data @commit = project.commit(@newrev) @@ -137,7 +137,7 @@ describe GitPushService, services: true do describe "Push Event" do before do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute @event = Event.last @push_data = service.push_data @@ -152,7 +152,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, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end end @@ -164,7 +164,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, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end @@ -174,7 +174,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, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end @@ -184,13 +184,13 @@ 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, @blankrev, 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end it "when pushing new commits to existing branch" do expect(project).to receive(:execute_hooks) - service = described_class.new(project, user, 'oldrev', 'newrev', 'refs/heads/master') + service = described_class.new(project, user, { oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/heads/master' }) service.execute end end @@ -214,7 +214,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, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -224,7 +224,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, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -236,7 +236,7 @@ describe GitPushService, services: true do ) expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -247,7 +247,7 @@ describe GitPushService, services: true do expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author) - service = described_class.new(project, user, @blankrev, @newrev, 'refs/heads/other') + service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: 'refs/heads/other' }) service.execute end @@ -273,20 +273,20 @@ describe GitPushService, services: true do context "to default branches" do it "closes issues" do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute 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, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end it "doesn't create additional cross-reference notes" do expect(SystemNoteService).not_to receive(:cross_reference) - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end @@ -295,7 +295,7 @@ describe GitPushService, services: true do # The push still shouldn't create cross-reference notes. expect do - service = described_class.new(project, user, @oldrev, @newrev, 'refs/heads/hurf') + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: 'refs/heads/hurf' }) service.execute end.not_to change { Note.where(project_id: project.id, system: true).count } end @@ -309,12 +309,12 @@ 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, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute end it "doesn't close issues" do - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(Issue.find(issue.id)).to be_opened end @@ -352,7 +352,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, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( @@ -371,7 +371,7 @@ describe GitPushService, services: true do } }.to_json - service = described_class.new(project, user, @oldrev, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(WebMock).to have_requested(:post, jira_api_transition_url).with( @@ -384,7 +384,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, @newrev, @ref) + service = described_class.new(project, user, { oldrev: @oldrev, newrev: @newrev, ref: @ref }) service.execute expect(WebMock).to have_requested(:post, jira_api_comment_url).with( @@ -405,7 +405,7 @@ describe GitPushService, services: true do end it 'push to first branch updates HEAD' do - service = described_class.new(project, user, @blankrev, @newrev, new_ref) + service = described_class.new(project, user, { oldrev: @blankrev, newrev: @newrev, ref: new_ref }) service.execute end end |