Merge branch 'service-testing' into 'master'

Fix service testing

Fixes:
- https://gitlab.com/gitlab-org/gitlab-ci/issues/248
- https://gitlab.com/gitlab-org/gitlab-ci/issues/208

/cc @vsizov 

See merge request !221

CHANGELOG

@@ -20,6 +20,7 @@ v7.13.0
   - Fix inline edit runner-description
   - Allow to specify image and services in yml that can be used with docker
   - Fix: No runner notification can see managers only
+  - Fix service testing for slack
   - Ability to cancel all builds in commit at once
   - Disable colors in rake tasks automatically (if IO is not a TTY)
   - Implemented "rake env:info". Rake task to receive system information

app/controllers/services_controller.rb

@@ -31,7 +31,7 @@ class ServicesController < ApplicationController
     if @service.execute(last_build)
       message = { notice: 'We successfully tested the service' }
     else
-      message = { alert: 'We tried to test the service but error occured' }
+      message = { alert: 'We tried to test the service but error occurred' }
     end
     redirect_to :back, message

app/models/project.rb

@@ -199,7 +199,7 @@ ls -la
       # Call service hook only if it is active
       begin
-        service.execute(data) if service.active
+        service.execute(data) if service.active && service.can_execute?(data)
       rescue => e
         logger.error(e)
       end

app/models/project_services/hip_chat_service.rb

@@ -43,15 +43,24 @@ class HipChatService < Service
     ]
   end
-  def execute build
+  def can_execute?(build)
     return if build.allow_failure?
     commit = build.commit
     return unless commit
     return unless commit.builds_without_retry.include? build
-    return if commit.success? and notify_only_broken_builds?
-    return if commit.running?
+    case commit.status.to_sym
+    when :failed
+      true
+    when :success
+      true unless notify_only_broken_builds?
+    else
+      false
+    end
+  end
+
+  def execute(build)
     msg = HipChatMessage.new(build)
     opts = default_options.merge(
       token: hipchat_token,

app/models/project_services/mail_service.rb

@@ -41,23 +41,7 @@ class MailService < Service
     ]
   end
-  def can_test?
-    # e-mail notification is useful only for builds either successful or failed
-    project.builds.order(id: :desc).any? do |build|
-      return false unless build.commit.project_recipients.any?
-
-      case build.status.to_sym
-      when :failed
-        true
-      when :success
-        !email_only_broken_builds
-      else
-        false
-      end
-    end
-  end
-
-  def execute(build)
+  def can_execute?(build)
     return if build.allow_failure?
     # it doesn't make sense to send emails for retried builds
@@ -65,10 +49,20 @@ class MailService < Service
     return unless commit
     return unless commit.builds_without_retry.include?(build)
-    commit.project_recipients.each do |recipient|
+    case build.status.to_sym
+    when :failed
+      true
+    when :success
+      true unless email_only_broken_builds
+    else
+      false
+    end
+  end
+
+  def execute(build)
+    build.commit.project_recipients.each do |recipient|
       case build.status.to_sym
       when :success
-        return if email_only_broken_builds
         mailer.build_success_email(build.id, recipient)
       when :failed
         mailer.build_fail_email(build.id, recipient)

app/models/project_services/slack_service.rb

@@ -42,21 +42,7 @@ class SlackService < Service
     ]
   end
-  def can_test?
-    # slack notification is useful only for builds either successful or failed
-    project.commits.order(id: :desc).any? do |commit|
-      case commit.status.to_sym
-      when :failed
-        true
-      when :success
-        !notify_only_broken_builds?
-      else
-        false
-      end
-    end
-  end
-
-  def execute(build)
+  def can_execute?(build)
     return if build.allow_failure?
     commit = build.commit
@@ -65,13 +51,16 @@ class SlackService < Service
     case commit.status.to_sym
     when :failed
+      true
     when :success
-      return if notify_only_broken_builds?
+      true unless notify_only_broken_builds?
     else
-      return
+      false
     end
-    message = SlackMessage.new(commit)
+  end
+  def execute(build)
+    message = SlackMessage.new(build.commit)
     options = default_options.merge(
       color: message.color,
       fallback: message.fallback,

app/models/service.rb

@@ -58,14 +58,18 @@ class Service < ActiveRecord::Base
     []
   end
-  def execute
-    # implement inside child
-  end
-
   def can_test?
     project.builds.any?
   end
+  def can_execute?(build)
+    true
+  end
+
+  def execute(build)
+    # implement inside child
+  end
+
   # Provide convenient accessor methods
   # for each serialized property.
   def self.prop_accessor(*args)

spec/models/mail_service_spec.rb

@@ -121,7 +121,7 @@ describe MailService do
       it do
         should_email(commit.git_author_email)
         should_email("jeroen@example.com")
-        mail.execute(build)
+        mail.execute(build) if mail.can_execute?(build)
       end
       def should_email(email)
@@ -152,28 +152,6 @@ describe MailService do
       end
     end
-    describe 'successful build and cannot test service' do
-      let(:project) {
-        FactoryGirl.create(:project,
-                           email_add_pusher: true,
-                           email_only_broken_builds: true,
-                           email_recipients: "jeroen@example.com")
-      }
-      let(:commit) { FactoryGirl.create(:commit, project: project) }
-      let(:build) { FactoryGirl.create(:build, status: :success, commit: commit) }
-
-      before do
-        mail.stub(
-          project: project
-        )
-        build
-      end
-
-      it do
-        mail.can_test?.should == false
-      end
-    end
-
     describe 'retried build should not receive email' do
       let(:project) {
         FactoryGirl.create(:project,
@@ -194,7 +172,7 @@ describe MailService do
         Build.retry(build)
         should_email(commit.git_author_email)
         should_email("jeroen@example.com")
-        mail.execute(build)
+        mail.execute(build) if mail.can_execute?(build)
       end
       def should_email(email)