Merge branch 'notify_based_on_pusher_email' into 'master'

Notify only pusher instead of commiter

Fixes #161

!!! Dependent MR on GitLab - https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/1630

See merge request !126

16 files changed, 56 insertions, 46 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 33f4087..c38703b 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -11,6 +11,7 @@ v7.9.0
 
 v7.8.2
   - Fix the broken build failed email
+  - Notify only pusher instead of commiter
 
 v7.8.0
   - Fix OAuth login with GitLab installed in relative URL
diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb
index 99b4d65..39cf306 100644
--- a/app/controllers/services_controller.rb
+++ b/app/controllers/services_controller.rb
@@ -50,7 +50,7 @@ class ServicesController < ApplicationController
   def service_params
     params.require(:service).permit(
       :type, :active, :webhook, :notify_only_broken_builds,
-      :email_recipients, :email_only_broken_builds, :email_add_committer
+      :email_recipients, :email_only_broken_builds, :email_add_pusher
     )
   end
 end
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 11b61d4..2e1bd52 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -92,7 +92,11 @@ class Commit < ActiveRecord::Base
 
   def project_recipients
     recipients = project.email_recipients.split(' ')
-    recipients << git_author_email if project.email_add_committer?
+
+    if project.email_add_pusher? && push_data[:user_email].present?
+      recipients << push_data[:user_email]
+    end
+
     recipients.uniq
   end
 
diff --git a/app/models/project.rb b/app/models/project.rb
index bf14dc5..d615f7e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -17,7 +17,7 @@
 #  gitlab_id                :integer
 #  allow_git_fetch          :boolean          default(TRUE), not null
 #  email_recipients         :string(255)      default(""), not null
-#  email_add_committer      :boolean          default(TRUE), not null
+#  email_add_pusher         :boolean          default(TRUE), not null
 #  email_only_broken_builds :boolean          default(TRUE), not null
 #  skip_refs                :string(255)
 #  coverage_regex           :string(255)
@@ -29,7 +29,7 @@ class Project < ActiveRecord::Base
   attr_accessible :name, :path, :timeout, :token, :timeout_in_minutes,
     :default_ref, :gitlab_url, :always_build, :polling_interval,
     :public, :ssh_url_to_repo, :gitlab_id, :allow_git_fetch, :skip_refs,
-    :email_recipients, :email_add_committer, :email_only_broken_builds, :coverage_regex,
+    :email_recipients, :email_add_pusher, :email_only_broken_builds, :coverage_regex,
     :jobs_attributes
 
   has_many :commits, dependent: :destroy
@@ -81,7 +81,7 @@ ls -la
         gitlab_url:              project.web_url,
         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_add_pusher:        GitlabCi.config.gitlab_ci.add_pusher,
         email_only_broken_builds: GitlabCi.config.gitlab_ci.all_broken_builds,
       }
 
@@ -137,7 +137,7 @@ ls -la
   end
 
   def email_notification?
-    email_add_committer || email_recipients.present?
+    email_add_pusher || email_recipients.present?
   end
 
   def web_hooks?
diff --git a/app/models/project_services/mail_service.rb b/app/models/project_services/mail_service.rb
index 380d2b6..a920968 100644
--- a/app/models/project_services/mail_service.rb
+++ b/app/models/project_services/mail_service.rb
@@ -14,7 +14,7 @@
 
 class MailService < Service
   delegate :email_recipients, :email_recipients=,
-           :email_add_committer, :email_add_committer=,
+           :email_add_pusher, :email_add_pusher=,
            :email_only_broken_builds, :email_only_broken_builds=, to: :project, prefix: false
 
   before_save :update_project
@@ -36,7 +36,7 @@ class MailService < Service
   def fields
     [
       { type: 'text', name: 'email_recipients', label: 'Recipients', help: 'Whitespace-separated list of recipient addresses' },
-      { type: 'checkbox', name: 'email_add_committer', label: 'Add committer to recipients list' },
+      { type: 'checkbox', name: 'email_add_pusher', label: 'Add pusher to recipients list' },
       { type: 'checkbox', name: 'email_only_broken_builds', label: 'Notify only broken builds' }
     ]
   end
diff --git a/app/services/create_commit_service.rb b/app/services/create_commit_service.rb
index 6526445..b7a5903 100644
--- a/app/services/create_commit_service.rb
+++ b/app/services/create_commit_service.rb
@@ -32,6 +32,7 @@ class CreateCommitService
           after: sha,
           ref: ref,
           user_name: params[:user_name],
+          user_email: params[:user_email],
           repository: params[:repository],
           commits: params[:commits],
           total_commits_count: params[:total_commits_count]
diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml
index 8204a90..f8e6b9f 100644
--- a/app/views/admin/projects/show.html.haml
+++ b/app/views/admin/projects/show.html.haml
@@ -42,7 +42,7 @@
         %strong= @project.email_recipients
       %p
         Send mail to commiter:
-        %strong= @project.email_add_committer
+        %strong= @project.email_add_pusher
       %p
         Send on all broken builds:
         %strong= @project.email_only_broken_builds
diff --git a/config/application.yml.example b/config/application.yml.example
index acdc993..124d147 100644
--- a/config/application.yml.example
+++ b/config/application.yml.example
@@ -23,8 +23,8 @@ defaults: &defaults
     # Send emails only on broken builds (default: true)
     # all_broken_builds: true
     # 
-    # Add committer to recipients list (default: false)
-    # add_committer: true
+    # Add pusher to recipients list (default: false)
+    # add_pusher: true
     
   gravatar:
     enabled: true
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 8b9703a..8e77da8 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -38,7 +38,7 @@ Settings.gitlab_ci['protocol']            ||= Settings.gitlab_ci.https ? "https"
 Settings.gitlab_ci['email_from']          ||= "gitlab-ci@#{Settings.gitlab_ci.host}"
 Settings.gitlab_ci['support_email']       ||= Settings.gitlab_ci.email_from
 Settings.gitlab_ci['all_broken_builds'] = true if Settings.gitlab_ci['all_broken_builds'].nil?
-Settings.gitlab_ci['add_committer']     = false if Settings.gitlab_ci['add_committer'].nil?
+Settings.gitlab_ci['add_pusher']     = false if Settings.gitlab_ci['add_pusher'].nil?
 Settings.gitlab_ci['url']                 ||= Settings.send(:build_gitlab_ci_url)
 
 # Compatibility with old config
diff --git a/db/migrate/20150310001733_rename_committer_to_pusher.rb b/db/migrate/20150310001733_rename_committer_to_pusher.rb
new file mode 100644
index 0000000..7e18367
--- /dev/null
+++ b/db/migrate/20150310001733_rename_committer_to_pusher.rb
@@ -0,0 +1,5 @@
+class RenameCommitterToPusher < ActiveRecord::Migration
+  def change
+    rename_column :projects, :email_add_committer, :email_add_pusher
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 804984c..b76eaa0 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150306135341) do
+ActiveRecord::Schema.define(version: 20150310001733) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -87,7 +87,7 @@ ActiveRecord::Schema.define(version: 20150306135341) do
     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_add_pusher",         default: true,  null: false
     t.boolean  "email_only_broken_builds", default: true,  null: false
     t.string   "skip_refs"
     t.string   "coverage_regex"
diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb
index 156fabe..3674643 100644
--- a/spec/factories/commits.rb
+++ b/spec/factories/commits.rb
@@ -25,6 +25,7 @@ FactoryGirl.define do
         before: '76de212e80737a608d939f648d959671fb0a0142',
         after: '97de212e80737a608d939f648d959671fb0a0142',
         user_name: 'Git User',
+        user_email: 'git@example.com',
         repository: {
           name: 'test-data',
           url: 'ssh://git@gitlab.com/test/test-data.git',
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index 552cd4e..55e2cdd 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -17,7 +17,7 @@
 #  gitlab_id                :integer
 #  allow_git_fetch          :boolean          default(TRUE), not null
 #  email_recipients         :string(255)      default(""), not null
-#  email_add_committer      :boolean          default(TRUE), not null
+#  email_add_pusher      :boolean          default(TRUE), not null
 #  email_only_broken_builds :boolean          default(TRUE), not null
 #  skip_refs                :string(255)
 #  coverage_regex           :string(255)
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 891cd52..da57109 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -79,43 +79,41 @@ describe Commit do
   describe :project_recipients do
 
     context 'always sending notification' do
-      it 'should return git_author_email as only recipient when no additional recipients are given' do
+      it 'should return commit_pusher_email as only recipient when no additional recipients are given' do
         project = FactoryGirl.create :project,
-          email_add_committer: true,
+          email_add_pusher: true,
           email_recipients: ''
         commit =  FactoryGirl.create :commit, project: project
-        expected = 'git_author_email'
-        commit.stub(:git_author_email) { expected }
+        expected = 'commit_pusher_email'
+        commit.stub(:push_data) { { user_email: expected } }
         commit.project_recipients.should == [expected]
       end
 
-      it 'should return git_author_email and additional recipients' do
+      it 'should return commit_pusher_email and additional recipients' do
         project = FactoryGirl.create :project,
-          email_add_committer: true,
+          email_add_pusher: true,
           email_recipients: 'rec1 rec2'
         commit = FactoryGirl.create :commit, project: project
-        expected = 'git_author_email'
-        commit.stub(:git_author_email) { expected }
+        expected = 'commit_pusher_email'
+        commit.stub(:push_data) { { user_email: expected } }
         commit.project_recipients.should == ['rec1', 'rec2', expected]
       end
 
       it 'should return recipients' do
         project = FactoryGirl.create :project,
-          email_add_committer: false,
+          email_add_pusher: false,
           email_recipients: 'rec1 rec2'
         commit = FactoryGirl.create :commit, project: project
-        expected = 'git_author_email'
-        commit.stub(:git_author_email) { expected }
         commit.project_recipients.should == ['rec1', 'rec2']
       end
 
       it 'should return unique recipients only' do
         project = FactoryGirl.create :project,
-          email_add_committer: true,
+          email_add_pusher: true,
           email_recipients: 'rec1 rec1 rec2'
         commit = FactoryGirl.create :commit, project: project
         expected = 'rec2'
-        commit.stub(:git_author_email) { expected }
+        commit.stub(:push_data) { { user_email: expected } }
         commit.project_recipients.should == ['rec1', 'rec2']
       end
     end
diff --git a/spec/models/mail_service_spec.rb b/spec/models/mail_service_spec.rb
index 7a43978..a24fca2 100644
--- a/spec/models/mail_service_spec.rb
+++ b/spec/models/mail_service_spec.rb
@@ -17,7 +17,7 @@ describe MailService do
     let(:mail)   { MailService.new }
 
     describe 'failed build' do
-      let(:project) { FactoryGirl.create(:project, email_add_committer: true) }
+      let(:project) { FactoryGirl.create(:project, email_add_pusher: true) }
       let(:commit) { FactoryGirl.create(:commit, project: project) }
       let(:build) { FactoryGirl.create(:build, status: :failed, commit: commit) }
 
@@ -28,7 +28,7 @@ describe MailService do
       end
 
       it do
-        should_email(commit.git_author_email)
+        should_email("git@example.com")
         mail.execute(build)
       end
 
@@ -39,7 +39,7 @@ describe MailService do
     end
 
     describe 'successfull build' do
-      let(:project) { FactoryGirl.create(:project, email_add_committer: true, email_only_broken_builds: false) }
+      let(:project) { FactoryGirl.create(:project, email_add_pusher: true, email_only_broken_builds: false) }
       let(:commit) { FactoryGirl.create(:commit, project: project) }
       let(:build) { FactoryGirl.create(:build, status: :success, commit: commit) }
 
@@ -50,7 +50,7 @@ describe MailService do
       end
 
       it do
-        should_email(commit.git_author_email)
+        should_email("git@example.com")
         mail.execute(build)
       end
 
@@ -63,7 +63,7 @@ describe MailService do
     describe 'successfull build and project has email_recipients' do
       let(:project) {
         FactoryGirl.create(:project,
-                           email_add_committer: true,
+                           email_add_pusher: true,
                            email_only_broken_builds: false,
                            email_recipients: "jeroen@example.com")
       }
@@ -77,7 +77,7 @@ describe MailService do
       end
 
       it do
-        should_email(commit.git_author_email)
+        should_email("git@example.com")
         should_email("jeroen@example.com")
         mail.execute(build)
       end
@@ -91,7 +91,7 @@ describe MailService do
     describe 'successful build and notify only broken builds' do
       let(:project) {
         FactoryGirl.create(:project,
-                           email_add_committer: true,
+                           email_add_pusher: true,
                            email_only_broken_builds: true,
                            email_recipients: "jeroen@example.com")
       }
@@ -119,7 +119,7 @@ describe MailService do
     describe 'successful build and can test service' do
       let(:project) {
         FactoryGirl.create(:project,
-                           email_add_committer: true,
+                           email_add_pusher: true,
                            email_only_broken_builds: false,
                            email_recipients: "jeroen@example.com")
       }
@@ -141,7 +141,7 @@ describe MailService do
     describe 'successful build and cannot test service' do
       let(:project) {
         FactoryGirl.create(:project,
-                           email_add_committer: true,
+                           email_add_pusher: true,
                            email_only_broken_builds: true,
                            email_recipients: "jeroen@example.com")
       }
@@ -163,7 +163,7 @@ describe MailService do
     describe 'retried build should not receive email' do
       let(:project) {
         FactoryGirl.create(:project,
-                           email_add_committer: true,
+                           email_add_pusher: true,
                            email_only_broken_builds: true,
                            email_recipients: "jeroen@example.com")
       }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4f7f299..af7d162 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -17,7 +17,7 @@
 #  gitlab_id                :integer
 #  allow_git_fetch          :boolean          default(TRUE), not null
 #  email_recipients         :string(255)      default(""), not null
-#  email_add_committer      :boolean          default(TRUE), not null
+#  email_add_pusher         :boolean          default(TRUE), not null
 #  email_only_broken_builds :boolean          default(TRUE), not null
 #  skip_refs                :string(255)
 #  coverage_regex           :string(255)
@@ -63,45 +63,45 @@ describe Project do
 
   describe '#email_notification?' do
     it do
-      project = FactoryGirl.create :project, email_add_committer: true
+      project = FactoryGirl.create :project, email_add_pusher: true
       project.email_notification?.should == true
     end
 
     it do
-      project = FactoryGirl.create :project, email_add_committer: false, email_recipients: 'test tesft'
+      project = FactoryGirl.create :project, email_add_pusher: false, email_recipients: 'test tesft'
       project.email_notification?.should == true
     end
 
     it do
-      project = FactoryGirl.create :project, email_add_committer: false, email_recipients: ''
+      project = FactoryGirl.create :project, email_add_pusher: false, email_recipients: ''
       project.email_notification?.should == false
     end
   end
 
   describe '#broken_or_success?' do
     it {
-      project = FactoryGirl.create :project, email_add_committer: true
+      project = FactoryGirl.create :project, email_add_pusher: 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 = FactoryGirl.create :project, email_add_pusher: 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 = FactoryGirl.create :project, email_add_pusher: 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 = FactoryGirl.create :project, email_add_pusher: true
       project.stub(:broken?).and_return(false)
       project.stub(:success?).and_return(false)
       project.broken_or_success?.should == false