Merge branch 'refactoring/services' of /home/git/repositories/gitlab/gitlabhq

app/contexts/commit_load_context.rb

@@ -1,34 +0,0 @@
-class CommitLoadContext < BaseContext
-  def execute
-    result = {
-      commit: nil,
-      suppress_diff: false,
-      line_notes: [],
-      notes_count: 0,
-      note: nil,
-      status: :ok
-    }
-
-    commit = project.repository.commit(params[:id])
-
-    if commit
-      line_notes = project.notes.for_commit_id(commit.id).inline
-
-      result[:commit] = commit
-      result[:note] = project.build_commit_note(commit)
-      result[:line_notes] = line_notes
-      result[:notes_count] = project.notes.for_commit_id(commit.id).count
-      result[:branches] = project.repository.branch_names_contains(commit.id)
-
-      begin
-        result[:suppress_diff] = true if commit.diff_suppress? && !params[:force_show_diff]
-        result[:force_suppress_diff] = commit.diff_force_suppress?
-      rescue Grit::Git::GitTimeout
-        result[:suppress_diff] = true
-        result[:status] = :huge_commit
-      end
-    end
-
-    result
-  end
-end

app/contexts/test_hook_context.rb

@@ -1,7 +0,0 @@
-class TestHookContext < BaseContext
-  def execute
-    hook = project.hooks.find(params[:id])
-    data = GitPushService.new.sample_data(project, current_user)
-    hook.execute(data)
-  end
-end

app/controllers/projects/commit_controller.rb

@@ -6,34 +6,35 @@ class Projects::CommitController < Projects::ApplicationController
   before_filter :authorize_read_project!
   before_filter :authorize_code_access!
   before_filter :require_non_empty_project
+  before_filter :commit
 
   def show
-    result = CommitLoadContext.new(project, current_user, params).execute
+    return git_not_found! unless @commit
 
-    @commit = result[:commit]
+    @line_notes = project.notes.for_commit_id(commit.id).inline
+    @branches = project.repository.branch_names_contains(commit.id)
 
-    if @commit.nil?
-      git_not_found!
-      return
+    begin
+      @suppress_diff = true if commit.diff_suppress? && !params[:force_show_diff]
+      @force_suppress_diff = commit.diff_force_suppress?
+    rescue Grit::Git::GitTimeout
+      @suppress_diff = true
+      @status = :huge_commit
     end
 
-    @suppress_diff = result[:suppress_diff]
-    @force_suppress_diff = result[:force_suppress_diff]
-
-    @note        = result[:note]
-    @line_notes  = result[:line_notes]
-    @branches    = result[:branches]
-    @notes_count = result[:notes_count]
+    @note = project.build_commit_note(commit)
+    @notes_count = project.notes.for_commit_id(commit.id).count
     @notes = project.notes.for_commit_id(@commit.id).not_inline.fresh
     @noteable = @commit
-
     @comments_allowed = @reply_allowed = true
-    @comments_target  = { noteable_type: 'Commit',
-                          commit_id: @commit.id }
+    @comments_target  = {
+      noteable_type: 'Commit',
+      commit_id: @commit.id
+    }
 
     respond_to do |format|
       format.html do
-        if result[:status] == :huge_commit
+        if @status == :huge_commit
           render "huge_commit" and return
         end
       end
@@ -42,4 +43,8 @@ class Projects::CommitController < Projects::ApplicationController
       format.patch { render text: @commit.to_patch }
     end
   end
+
+  def commit
+    @commit ||= project.repository.commit(params[:id])
+  end
 end

app/controllers/projects/hooks_controller.rb

@@ -24,15 +24,20 @@ class Projects::HooksController < Projects::ApplicationController
   end
 
   def test
-    TestHookContext.new(project, current_user, params).execute
+    TestHookService.new.execute(hook, current_user)
 
     redirect_to :back
   end
 
   def destroy
-    @hook = @project.hooks.find(params[:id])
-    @hook.destroy
+    hook.destroy
 
     redirect_to project_hooks_path(@project)
   end
+
+  private
+
+  def hook
+    @hook ||= @project.hooks.find(params[:id])
+  end
 end

app/observers/system_hook_observer.rb

@@ -2,10 +2,16 @@ class SystemHookObserver < BaseObserver
   observe :user, :project, :users_project
 
   def after_create(model)
-    SystemHooksService.execute_hooks_for(model, :create)
+    system_hook_service.execute_hooks_for(model, :create)
   end
 
   def after_destroy(model)
-    SystemHooksService.execute_hooks_for(model, :destroy)
+    system_hook_service.execute_hooks_for(model, :destroy)
+  end
+
+  private
+
+  def system_hook_service
+    SystemHooksService.new
   end
 end

app/services/system_hooks_service.rb

@@ -1,21 +1,21 @@
 class SystemHooksService
-  def self.execute_hooks_for(model, event)
+  def execute_hooks_for(model, event)
     execute_hooks(build_event_data(model, event))
   end
 
   private
 
-  def self.execute_hooks(data)
+  def execute_hooks(data)
     SystemHook.all.each do |sh|
       async_execute_hook sh, data
     end
   end
 
-  def self.async_execute_hook(hook, data)
+  def async_execute_hook(hook, data)
     Sidekiq::Client.enqueue(SystemHookWorker, hook.id, data)
   end
 
-  def self.build_event_data(model, event)
+  def build_event_data(model, event)
     data = {
       event_name: build_event_name(model, event),
       created_at: model.created_at
@@ -51,7 +51,7 @@ class SystemHooksService
     end
   end
 
-  def self.build_event_name(model, event)
+  def build_event_name(model, event)
     case model
     when UsersProject
       return "user_add_to_team"      if event == :create

app/services/test_hook_service.rb

@@ -0,0 +1,6 @@
+class TestHookService
+  def execute(hook, current_user)
+    data = GitPushService.new.sample_data(hook.project, current_user)
+    hook.execute(data)
+  end
+end

features/steps/project/project_hooks.rb

@@ -1,9 +1,12 @@
+require 'webmock'
+
 class ProjectHooks < Spinach::FeatureSteps
   include SharedAuthentication
   include SharedProject
   include SharedPaths
   include RSpec::Matchers
   include RSpec::Mocks::ExampleMethods
+  include WebMock::API
 
   Given 'project has hook' do
     @hook = create(:project_hook, project: current_project)
@@ -25,8 +28,7 @@ class ProjectHooks < Spinach::FeatureSteps
   end
 
   When 'I click test hook button' do
-    test_hook_context = double(execute: true)
-    TestHookContext.should_receive(:new).and_return(test_hook_context)
+    stub_request(:post, @hook.url).to_return(status: 200)
     click_link 'Test Hook'
   end
 

lib/api/repositories.rb

@@ -124,9 +124,9 @@ module API
       #   GET /projects/:id/repository/commits/:sha/diff
       get ":id/repository/commits/:sha/diff" do
         sha = params[:sha]
-        result = CommitLoadContext.new(user_project, current_user, {id: sha}).execute
-        not_found! "Commit" unless result[:commit]
-        result[:commit].diffs
+        commit = user_project.repository.commit(sha)
+        not_found! "Commit" unless commit
+        commit.diffs
       end
 
       # Get a project repository tree

spec/services/system_hooks_service_spec.rb

@@ -24,10 +24,10 @@ describe SystemHooksService do
   end
 
   def event_data(*args)
-    SystemHooksService.build_event_data(*args)
+    SystemHooksService.new.send :build_event_data, *args
   end
 
   def event_name(*args)
-    SystemHooksService.build_event_name(*args)
+    SystemHooksService.new.send :build_event_name, *args
   end
 end

spec/services/test_hook_service_spec.rb

@@ -0,0 +1,14 @@
+require 'spec_helper'
+
+describe TestHookService do
+  let (:user)    { create :user }
+  let (:project) { create :project_with_code }
+  let (:hook)    { create :project_hook, project: project }
+
+  describe :execute do
+    it "should execute successfully" do
+      stub_request(:post, hook.url).to_return(status: 200)
+      TestHookService.new.execute(hook, user).should be_true
+    end
+  end
+end