Send email notification on change of build status from success to failed and failed to success

13 files changed, 194 insertions, 125 deletions

diff --git a/app/models/build.rb b/app/models/build.rb
index 404738b..44f46e0 100644
--- a/app/models/build.rb
+++ b/app/models/build.rb
@@ -168,9 +168,7 @@ class Build < ActiveRecord::Base
   end
   
   def project_recipients
-    recipients = []
-    return recipients if project.email_only_breaking_build && status != :failed
-    recipients.concat(project.email_recipients.split(' '))
+    recipients = project.email_recipients.split(' ')
     recipients << git_author_email if project.email_add_committer?
     recipients.uniq
   end
diff --git a/app/models/project.rb b/app/models/project.rb
index 3db8ef5..ca35b5b 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -19,14 +19,14 @@
 #  allow_git_fetch           :boolean          default(TRUE), not null
 #  email_recipients          :string(255)
 #  email_add_committer       :boolean          default(TRUE), not null 
-#  email_only_breaking_build :boolean          default(TRUE), not null 
+#  email_all_broken_builds   :boolean          default(TRUE), not null 
 #
 
 class Project < ActiveRecord::Base
   attr_accessible :name, :path, :scripts, :timeout, :token,
     :default_ref, :gitlab_url, :always_build, :polling_interval,
     :public, :ssh_url_to_repo, :gitlab_id, :allow_git_fetch, 
-    :email_recipients, :email_add_committer, :email_only_breaking_build
+    :email_recipients, :email_add_committer, :email_all_broken_builds
 
   has_many :builds, dependent: :destroy
   has_many :runner_projects, dependent: :destroy
@@ -53,14 +53,14 @@ class Project < ActiveRecord::Base
       project = YAML.load(project_yaml)
 
       params = {
-        name: project.name_with_namespace,
-        gitlab_id: project.id,
-        gitlab_url: project.web_url,
-        scripts: 'ls -la',
-        default_ref: project.default_branch || 'master',
-        ssh_url_to_repo: project.ssh_url_to_repo,
-        email_add_committer:       GitlabCi.config.gitlab_ci.add_committer,
-        email_only_breaking_build: GitlabCi.config.gitlab_ci.only_breaking_build,
+        name:                    project.name_with_namespace,
+        gitlab_id:               project.id,
+        gitlab_url:              project.web_url,
+        scripts:                 'ls -la',
+        default_ref:             project.default_branch || 'master',
+        ssh_url_to_repo:         project.ssh_url_to_repo,
+        email_add_committer:     GitlabCi.config.gitlab_ci.add_committer,
+        email_all_broken_builds: GitlabCi.config.gitlab_ci.all_broken_builds,
       }
 
       Project.new(params)
@@ -120,6 +120,18 @@ class Project < ActiveRecord::Base
     last_build.status if last_build
   end
 
+  def broken?
+    last_build.failed? || last_build.canceled? if last_build
+  end
+
+  def success?
+    last_build.success? if last_build
+  end
+  
+  def broken_or_success?
+    broken? || success? 
+  end
+  
   def last_build
     builds.last
   end
@@ -174,7 +186,15 @@ class Project < ActiveRecord::Base
   end
   
   def email_notification?
-    email_add_committer || !email_recipients.blank?
+    (email_add_committer || !email_recipients.blank?) && broken_or_success?
+  end
+  
+  # onlu check for toggling build status within same ref.
+  def last_build_changed_status?
+    ref = last_build.ref 
+    last_builds = builds.where(ref: ref).order('id DESC').limit(2)
+    return false if last_builds.size < 2 
+    return last_builds[0].status != last_builds[1].status
   end
   
 end
diff --git a/app/observers/build_observer.rb b/app/observers/build_observer.rb
index ee6f73f..274497e 100644
--- a/app/observers/build_observer.rb
+++ b/app/observers/build_observer.rb
@@ -1,7 +1,10 @@
 class BuildObserver < BaseObserver
   def after_save(build)
-    if [:failed, :success, :canceled].include? build.status
-      notification.build_ended(build)
+    project = build.project
+    if project.email_notification?
+      if (project.email_all_broken_builds? && project.broken?) || project.last_build_changed_status?
+        notification.build_ended(build)
+      end
     end
   end
 
diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml
index 2a3ee3a..6fcae41 100644
--- a/app/views/admin/projects/show.html.haml
+++ b/app/views/admin/projects/show.html.haml
@@ -41,7 +41,7 @@
         %strong= @project.email_add_committer
       %p
         Send only on broken builds:
-        %strong= @project.email_only_breaking_build
+        %strong= @project.email_all_breaking_builds
 
   .span6
     %fieldset
diff --git a/app/views/projects/_form.html.haml b/app/views/projects/_form.html.haml
index 986cbda..ac070a5 100644
--- a/app/views/projects/_form.html.haml
+++ b/app/views/projects/_form.html.haml
@@ -68,9 +68,9 @@
             %span Add committer to recipients list
         %br
         .field
-          = f.label :email_only_breaking_build do
-            = f.check_box :email_only_breaking_build
-            %span Send notification on breaking builds only
+          = f.label :email_all_breaking_builds do
+            = f.check_box :email_all_breaking_builds
+            %span Send notification on all breaking builds
 
       %br
       %fieldset
diff --git a/config/application.yml.example b/config/application.yml.example
index 24f96f8..1a7d41b 100644
--- a/config/application.yml.example
+++ b/config/application.yml.example
@@ -17,8 +17,8 @@ defaults: &defaults
     # Email address of your support contact (default: same as email_from)
     support_email: support@localhost
   
-    # Only send emails for breaking builds
-    # only_breaking_build: true
+    # Only send emails for all failing builds
+    # all_broken_builds: true
     
     # Add committer to recipients list
     # add_committer: true
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 314ea37..b6105e9 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -37,7 +37,7 @@ Settings.gitlab_ci['relative_url_root']   ||= ENV['RAILS_RELATIVE_URL_ROOT'] ||
 Settings.gitlab_ci['protocol']            ||= Settings.gitlab_ci.https ? "https" : "http"
 Settings.gitlab_ci['email_from']          ||= "gitlab-ci@#{Settings.gitlab_ci.host}"
 Settings.gitlab_ci['support_email']       ||= Settings.gitlab_ci.email_from
-Settings.gitlab_ci['only_breaking_build'] ||= true
+Settings.gitlab_ci['all_broken_builds']   ||= true
 Settings.gitlab_ci['add_committer']       ||= true
 Settings.gitlab_ci['url']                 ||= Settings.send(:build_gitlab_ci_url)
 
diff --git a/db/migrate/20131120155545_add_email_notification_fields_to_project.rb b/db/migrate/20131120155545_add_email_notification_fields_to_project.rb
index ff2e71a..e0f4943 100644
--- a/db/migrate/20131120155545_add_email_notification_fields_to_project.rb
+++ b/db/migrate/20131120155545_add_email_notification_fields_to_project.rb
@@ -2,6 +2,6 @@ class AddEmailNotificationFieldsToProject < ActiveRecord::Migration
   def change
     add_column :projects, :email_recipients, :string, default: '', null: false
     add_column :projects, :email_add_committer, :boolean, default: true, null: false
-    add_column :projects, :email_only_breaking_build, :boolean, default: true, null: false
+    add_column :projects, :email_all_broken_builds, :boolean, default: true, null: false
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 92b5b76..d916f91 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -33,23 +33,23 @@ ActiveRecord::Schema.define(:version => 20131120155545) do
   add_index "builds", ["runner_id"], :name => "index_builds_on_runner_id"
 
   create_table "projects", :force => true do |t|
-    t.string   "name",                                         :null => false
-    t.integer  "timeout",                   :default => 1800,  :null => false
-    t.text     "scripts",                                      :null => false
-    t.datetime "created_at",                                   :null => false
-    t.datetime "updated_at",                                   :null => false
+    t.string   "name",                                       :null => false
+    t.integer  "timeout",                 :default => 1800,  :null => false
+    t.text     "scripts",                                    :null => false
+    t.datetime "created_at",                                 :null => false
+    t.datetime "updated_at",                                 :null => false
     t.string   "token"
     t.string   "default_ref"
     t.string   "gitlab_url"
-    t.boolean  "always_build",              :default => false, :null => false
+    t.boolean  "always_build",            :default => false, :null => false
     t.integer  "polling_interval"
-    t.boolean  "public",                    :default => false, :null => false
+    t.boolean  "public",                  :default => false, :null => false
     t.string   "ssh_url_to_repo"
     t.integer  "gitlab_id"
-    t.boolean  "allow_git_fetch",           :default => true,  :null => false
-    t.string   "email_recipients",          :default => "",    :null => false
-    t.boolean  "email_add_committer",       :default => true,  :null => false
-    t.boolean  "email_only_breaking_build", :default => true,  :null => false
+    t.boolean  "allow_git_fetch",         :default => true,  :null => false
+    t.string   "email_recipients",        :default => "",    :null => false
+    t.boolean  "email_add_committer",     :default => true,  :null => false
+    t.boolean  "email_all_broken_builds", :default => true,  :null => false
   end
 
   create_table "runner_projects", :force => true do |t|
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index f254fe4..17c3ba5 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -52,16 +52,10 @@ describe Build do
   end
   
   describe '#project_recipients' do
-    # before(:each) do
-    #   @settings = double("settings")
-    #   @settings.stub(:only_breaking_build) { true }
-    #   stub_const("Settings", Class.new)
-    #   Settings.stub_chain(:gitlab_ci).and_return(@settings)
-    # end
+
     context 'always sending notification' do
       it 'should return git_author_email as only recipient when no additional recipients are given' do
         project = FactoryGirl.create :project, 
-                                           email_only_breaking_build: false,
                                            email_add_committer: true,
                                            email_recipients: '' 
         build =  FactoryGirl.create :build, 
@@ -74,7 +68,6 @@ describe Build do
 
       it 'should return git_author_email and additional recipients' do
         project = FactoryGirl.create :project, 
-                                           email_only_breaking_build: false,
                                            email_add_committer: true,
                                            email_recipients: 'rec1 rec2' 
         build = FactoryGirl.create :build, 
@@ -87,7 +80,6 @@ describe Build do
 
       it 'should return recipients' do
         project = FactoryGirl.create :project, 
-                                           email_only_breaking_build: false,
                                            email_add_committer: false,
                                            email_recipients: 'rec1 rec2' 
         build = FactoryGirl.create :build, 
@@ -100,7 +92,6 @@ describe Build do
 
       it 'should return unique recipients only' do
         project = FactoryGirl.create :project, 
-                                           email_only_breaking_build: false,
                                            email_add_committer: true,
                                            email_recipients: 'rec1 rec1 rec2' 
         build = FactoryGirl.create :build, 
@@ -111,35 +102,6 @@ describe Build do
         build.project_recipients.should == ['rec1', 'rec2']
       end
     end
-
-    describe 'only on breaking build sending notification' do
-      it 'should return git_author_email and additional recipients' do
-        project = FactoryGirl.create :project, 
-                                           email_only_breaking_build: true,
-                                           email_add_committer: true,
-                                           email_recipients: 'rec1 rec2' 
-        build = FactoryGirl.create :build, 
-                                         status: :failed, 
-                                         project: project 
-        expected = 'git_author_email'
-        build.stub(:git_author_email) { expected }
-        build.project_recipients.should == ['rec1', 'rec2', expected]
-      end
-
-      it 'should return an empty array because the build was successful' do
-        project = FactoryGirl.create :project, 
-                                           email_only_breaking_build: true,
-                                           email_add_committer: true,
-                                           email_recipients: 'rec1 rec2' 
-        build = FactoryGirl.create :build, 
-                                         status: :success, 
-                                         project: project 
-        expected = 'git_author_email'
-        build.stub(:git_author_email) { expected }
-        build.project_recipients.should == []
-      end
-    end
-  
   end
   
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9778dc8..6a63a7e 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -70,26 +70,61 @@ describe Project do
   
   describe '#email_notification?' do
     it { 
-      project = FactoryGirl.create :project, email_add_committer: true 
+      project = FactoryGirl.create :project, email_add_committer: true
+      project.stub(:broken_or_success?).and_return(true)
       project.email_notification?.should == true 
     }
 
     it { 
-      project = FactoryGirl.create :project, email_add_committer: false 
-      project.email_notification?.should == false 
+      project = FactoryGirl.create :project, email_add_committer: true
+      project.stub(:broken_or_success?).and_return(false)
+      project.email_notification?.should == false
     }
 
+
     it { 
       project = FactoryGirl.create :project, email_add_committer: false, email_recipients: 'test tesft' 
+      project.stub(:broken_or_success?).and_return(true)
       project.email_notification?.should == true 
     }
 
     it { 
       project = FactoryGirl.create :project, email_add_committer: false, email_recipients: ''  
+      project.stub(:broken_or_success?).and_return(true)
       project.email_notification?.should == false 
     }
   end
   
+  describe '#broken_or_success?' do
+
+    it {
+      project = FactoryGirl.create :project, email_add_committer: true
+      project.stub(:broken?).and_return(true)
+      project.stub(:success?).and_return(true)
+      project.broken_or_success?.should == true
+    }
+
+    it {
+      project = FactoryGirl.create :project, email_add_committer: true
+      project.stub(:broken?).and_return(true)
+      project.stub(:success?).and_return(false)
+      project.broken_or_success?.should == true
+    }
+
+    it {
+      project = FactoryGirl.create :project, email_add_committer: true
+      project.stub(:broken?).and_return(false)
+      project.stub(:success?).and_return(true)
+      project.broken_or_success?.should == true
+    }
+
+    it {
+      project = FactoryGirl.create :project, email_add_committer: true
+      project.stub(:broken?).and_return(false)
+      project.stub(:success?).and_return(false)
+      project.broken_or_success?.should == false
+    }
+   end 
 end
 
 # == Schema Information
diff --git a/spec/observers/build_observer_spec.rb b/spec/observers/build_observer_spec.rb
index d9795e4..5439ac3 100644
--- a/spec/observers/build_observer_spec.rb
+++ b/spec/observers/build_observer_spec.rb
@@ -4,49 +4,101 @@ describe BuildObserver do
   subject { BuildObserver.instance }
   before { subject.stub(notification: double('NotificationService').as_null_object) }
 
-  # let(:build) { double(:build).as_null_object }
-  let(:build) { FactoryGirl.build(:build, status: 'pending')}
-
   describe '#after_save' do
-    it 'is called after a build is saved' do
-      subject.should_receive(:after_save)
-
-      Build.observers.enable :build_observer do
-        build.status = :success
-        build.save!
+    
+    context "only one build per project" do
+      let(:project) { FactoryGirl.build(:project) }
+      let(:build) { FactoryGirl.build(:build, status: 'success', project: project)}
+    
+      it 'is called after a build is saved' do
+        subject.should_receive(:after_save)
+  
+        Build.observers.enable :build_observer do
+          build.status = :success
+          build.save!
+        end
       end
-    end
+      
+      describe "sends out notification" do
+        it 'is a failure' do
+          subject.should_receive(:notification)
+          build.status = :failed
+          build.save!
+          subject.after_save(build)
+        end
     
-    it 'sends out notifications when build is a success' do
-      subject.should_receive(:notification)
-
-      build.status = :success
-      build.save!
-      subject.after_save(build)
-    end
-
-    it 'sends out notifications when build is a failure' do
-      subject.should_receive(:notification)
-
-      build.status = :failed
-      build.save!
-      subject.after_save(build)
-    end
-
-    it 'sends out notifications when build is cancelled' do
-      subject.should_receive(:notification)
-
-      build.status = :canceled
-      build.save!
-      subject.after_save(build)
+        it 'is cancelled' do
+          subject.should_receive(:notification)
+          build.status = :canceled
+          build.save!
+          subject.after_save(build)
+        end
+      end
+    
+      describe "sends no notification" do
+        it 'is a success' do
+          subject.should_not_receive(:notification)
+          build.status = :success
+          build.save!
+          subject.after_save(build)
+        end
+      
+        it "sends no notification when status is pending" do
+          subject.should_not_receive(:notification)
+          build.status = :pending
+          build.save!
+          subject.after_save(build)
+        end
+      end
     end
+    
+    context "more than 1 build per project" do
+      let!(:project) { FactoryGirl.build(:project) }
+      let(:build) { FactoryGirl.build(:build, project: project)}
+    
+      before { project.stub(:email_notification?).and_return(true) }
+      
+      describe "sends out notification" do
+        it 'is a failure and all broken builds should send an email' do
+          subject.should_receive(:notification)
+          project.stub(:broken?).and_return(true)
+          project.stub(:email_all_broken_builds?).and_return(true)
+          subject.after_save(build)
+        end
+    
+        it 'is build status success but changed sinced last build' do
+          subject.should_receive(:notification)
+          project.stub(:broken?).and_return(false)
+          project.stub(:last_build_changed_status?).and_return(true)
+          subject.after_save(build)
+        end
 
-    it 'does not sends out notifications when build is pending' do
-      subject.should_receive(:notification).never
-
-      build.status = :pending
-      build.save!
-      subject.after_save(build)
+        it 'is build status broken but changed sinced last build' do
+          subject.should_receive(:notification)
+          project.stub(:broken?).and_return(true)
+          project.stub(:last_build_changed_status).and_return(true)
+          subject.after_save(build)
+        end
+      end
+    
+      describe "sends no notification" do
+        it 'is build status broken but not changed sinced last build' do
+          subject.should_not_receive(:notification)
+          project.stub(:broken?).and_return(true)
+          project.stub(:email_all_broken_builds?).and_return(false)
+          project.stub(:last_build_changed_status?).and_return(false)
+          subject.after_save(build)
+        end
+      
+        it 'is build status success but not changed sinced last build' do
+          subject.should_not_receive(:notification)
+          project.stub(:broken?).and_return(false)
+          project.stub(:email_all_broken_builds?).and_return(false)
+          project.stub(:last_build_changed_status?).and_return(false)
+          subject.after_save(build)
+        end
+      end
+      
     end
   end
 end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 841763a..1f2038e 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -6,7 +6,7 @@ describe NotificationService do
   describe 'Builds' do
 
     describe 'failed build' do
-      let(:project) { FactoryGirl.create(:project, :email_only_breaking_build => true)}
+      let(:project) { FactoryGirl.create(:project)}
       let(:build) { FactoryGirl.create(:build, :status => :failed, :project => project) }
 
       it do
@@ -20,26 +20,27 @@ describe NotificationService do
       end
     end
 
-    describe 'successfull build when only breaking builds should trigger email' do
-      let(:project) { FactoryGirl.create(:project, :email_only_breaking_build => true)}
+    describe 'successfull build' do
+      let(:project) { FactoryGirl.create(:project)}
       let(:build) { FactoryGirl.create(:build, :status => :success, :project => project) }
       it do
-        should_not_email(build.git_author_email)
+        should_email(build.git_author_email)
         notification.build_ended(build)
       end
 
-      def should_not_email(email)
-        Notify.should_not_receive(:build_success_email).with(build.id, email)
+      def should_email(email)
+        Notify.should_receive(:build_success_email).with(build.id, email)
         Notify.should_not_receive(:build_fail_email).with(build.id, email)
       end
     end
 
-    describe 'successfull build when all builds should trigger email' do
-      let(:project) { FactoryGirl.create(:project, :email_only_breaking_build => false)}
+    describe 'successfull build and project has email_recipients' do
+      let(:project) { FactoryGirl.create(:project, :email_recipients => "jeroen@example.com")}
       let(:build) { FactoryGirl.create(:build, :status => :success, :project => project) }
       
       it do
         should_email(build.git_author_email)
+        should_email("jeroen@example.com")
         notification.build_ended(build)
       end
 
@@ -48,8 +49,6 @@ describe NotificationService do
         Notify.should_not_receive(:build_fail_email).with(build.id, email)
       end
     end
-
-
   end
 
 end