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

8 files changed, 45 insertions, 70 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6b0a44f..bf965a4 100644
--- a/CHANGELOG
+++ b/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
diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb
index 64bc698..c400776 100644
--- a/app/controllers/services_controller.rb
+++ b/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
diff --git a/app/models/project.rb b/app/models/project.rb
index 373fe84..55d8445 100644
--- a/app/models/project.rb
+++ b/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
diff --git a/app/models/project_services/hip_chat_service.rb b/app/models/project_services/hip_chat_service.rb
index 70f6a67..aa33633 100644
--- a/app/models/project_services/hip_chat_service.rb
+++ b/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,
diff --git a/app/models/project_services/mail_service.rb b/app/models/project_services/mail_service.rb
index 8520786..49a6311 100644
--- a/app/models/project_services/mail_service.rb
+++ b/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)
diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb
index a1f701b..ee51f71 100644
--- a/app/models/project_services/slack_service.rb
+++ b/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
+  end
 
-    message = SlackMessage.new(commit)
+  def execute(build)
+    message = SlackMessage.new(build.commit)
     options = default_options.merge(
       color: message.color,
       fallback: message.fallback,
diff --git a/app/models/service.rb b/app/models/service.rb
index 9586060..714d4a9 100644
--- a/app/models/service.rb
+++ b/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)
diff --git a/spec/models/mail_service_spec.rb b/spec/models/mail_service_spec.rb
index 97f29f0..d66a659 100644
--- a/spec/models/mail_service_spec.rb
+++ b/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)