refactored GitPushService and updated spec

app/services/git_push_service.rb

-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

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

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