Fix service testing

8 files changed, 45 insertions, 70 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 05c5e2b..83141db 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -13,6 +13,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 b4a3ffc..5d97196 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -204,7 +204,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..32246e0 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..5d2c670 100644
--- a/app/models/project_services/mail_service.rb
+++ b/app/models/project_services/mail_service.rb
@@ -41,34 +41,28 @@ 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?
+  def can_execute?(build)
+    return if build.allow_failure?
 
-      case build.status.to_sym
+    # it doesn't make sense to send emails for retried builds
+    commit = build.commit
+    return unless commit
+    return unless commit.builds_without_retry.include?(build)
+
+    case build.status.to_sym
       when :failed
         true
       when :success
-        !email_only_broken_builds
+        true unless email_only_broken_builds
       else
         false
-      end
     end
   end
 
   def execute(build)
-    return if build.allow_failure?
-
-    # it doesn't make sense to send emails for retried builds
-    commit = build.commit
-    return unless commit
-    return unless commit.builds_without_retry.include?(build)
-
-    commit.project_recipients.each do |recipient|
+    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..7244bcc 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
@@ -64,14 +50,17 @@ class SlackService < Service
     return unless commit.builds_without_retry.include?(build)
 
     case commit.status.to_sym
-    when :failed
-    when :success
-      return if notify_only_broken_builds?
-    else
-      return
+      when :failed
+        true
+      when :success
+        true unless notify_only_broken_builds?
+      else
+        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)