Merge branch 'emails-on-push'

Conflicts:
	app/controllers/projects/services_controller.rb
	app/models/project_services/emails_on_push_service.rb

app/controllers/admin/services_controller.rb

@@ -45,7 +45,8 @@ class Admin::ServicesController < Admin::ApplicationController
         :room, :recipients, :project_url, :webhook,
         :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
         :build_key, :server, :teamcity_url, :build_type,
-        :description, :issues_url, :new_issue_url, :restrict_to_branch
+        :description, :issues_url, :new_issue_url, :restrict_to_branch,
+        :send_from_committer_email, :disable_diffs
     ])
   end
 end

app/controllers/projects/services_controller.rb

@@ -53,7 +53,7 @@ class Projects::ServicesController < Projects::ApplicationController
       :description, :issues_url, :new_issue_url, :restrict_to_branch, :channel,
       :colorize_messages, :channels,
       :push_events, :issues_events, :merge_requests_events, :tag_push_events,
-      :note_events
+      :note_events, :send_from_committer_email, :disable_diffs
     )
   end
 end

app/mailers/emails/projects.rb

@@ -16,28 +16,38 @@ module Emails
            subject: subject("Project was moved"))
     end
 
-    def repository_push_email(project_id, recipient, author_id, branch, compare)
+    def repository_push_email(project_id, recipient, author_id, branch, compare, reverse_compare = false, send_from_committer_email = false, disable_diffs = false)
       @project = Project.find(project_id)
       @author  = User.find(author_id)
+      @reverse_compare = reverse_compare
       @compare = compare
       @commits = Commit.decorate(compare.commits)
       @diffs   = compare.diffs
-      @branch  = branch
+      @branch  = branch.gsub("refs/heads/", "")
+      @disable_diffs = disable_diffs
+
+      @subject = "[#{@project.path_with_namespace}][#{@branch}] "
+
       if @commits.length > 1
         @target_url = namespace_project_compare_url(@project.namespace,
                                                     @project,
-                                                    from: @commits.first,
-                                                    to: @commits.last)
-        @subject = "#{@commits.length} new commits pushed to repository"
+                                                    from: Commit.new(@compare.base),
+                                                    to:   Commit.new(@compare.head))
+        @subject << "Deleted " if @reverse_compare
+        @subject << "#{@commits.length} commits: #{@commits.first.title}"
       else
         @target_url = namespace_project_commit_url(@project.namespace,
                                                    @project, @commits.first)
-        @subject = @commits.first.title
+
+        @subject << "Deleted 1 commit: " if @reverse_compare
+        @subject << @commits.first.title
       end
 
-      mail(from: sender(author_id),
+      @disable_footer = true
+
+      mail(from: sender(author_id, send_from_committer_email),
            to: recipient,
-           subject: subject(@subject))
+           subject: @subject)
     end
   end
 end

app/mailers/notify.rb

@@ -34,6 +34,20 @@ class Notify < ActionMailer::Base
     )
   end
 
+  # Splits "gitlab.corp.company.com" up into "gitlab.corp.company.com",
+  # "corp.company.com" and "company.com".
+  # Respects set tld length so "company.co.uk" won't match "somethingelse.uk"
+  def self.allowed_email_domains
+    domain_parts = Gitlab.config.gitlab.host.split(".")
+    allowed_domains = []
+    begin
+      allowed_domains << domain_parts.join(".")
+      domain_parts.shift
+    end while domain_parts.length > ActionDispatch::Http::URL.tld_length
+
+    allowed_domains
+  end
+
   private
 
   # The default email address to send emails from
@@ -45,10 +59,16 @@ class Notify < ActionMailer::Base
 
   # Return an email address that displays the name of the sender.
   # Only the displayed name changes; the actual email address is always the same.
-  def sender(sender_id)
+  def sender(sender_id, send_from_user_email = false)
     if sender = User.find(sender_id)
       address = default_sender_address
       address.display_name = sender.name
+
+      sender_domain = sender.email.split("@").last
+      if send_from_user_email && self.class.allowed_email_domains.include?(sender_domain)
+        address.address = sender.email
+      end
+
       address.format
     end
   end

app/models/project_services/emails_on_push_service.rb

@@ -18,6 +18,8 @@
 #
 
 class EmailsOnPushService < Service
+  prop_accessor :send_from_committer_email
+  prop_accessor :disable_diffs
   prop_accessor :recipients
   validates :recipients, presence: true, if: :activated?
 
@@ -37,14 +39,27 @@ class EmailsOnPushService < Service
     %w(push)
   end
 
-  def execute(data)
-    return unless supported_events.include?(data[:object_kind])
+  def execute(push_data)
+    return unless supported_events.include?(push_data[:object_kind])
 
-    EmailsOnPushWorker.perform_async(project_id, recipients, data)
+    EmailsOnPushWorker.perform_async(project_id, recipients, push_data, send_from_committer_email?, disable_diffs?)
+  end
+
+  def send_from_committer_email?
+    self.send_from_committer_email == "1"
+  end
+
+  def disable_diffs?
+    self.disable_diffs == "1"
   end
 
   def fields
+    domains = Notify.allowed_email_domains.map { |domain| "user@#{domain}" }.join(", ")
     [
+      { type: 'checkbox', name: 'send_from_committer_email', title: "Send from committer",
+        help: "Send notifications from the committer's email address if the domain is part of the domain GitLab is running on (e.g. #{domains})." },
+      { type: 'checkbox', name: 'disable_diffs', title: "Disable code diffs",
+        help: "Don't include possibly sensitive code diffs in notification body." },
       { type: 'textarea', name: 'recipients', placeholder: 'Emails separated by whitespace' },
     ]
   end

app/views/admin/services/_form.html.haml

@@ -53,14 +53,16 @@
 
   - @service.fields.each do |field|
     - name = field[:name]
+    - title = field[:title] || name.humanize
     - value = @service.send(name) unless field[:type] == 'password'
     - type = field[:type]
     - placeholder = field[:placeholder]
     - choices = field[:choices]
     - default_choice = field[:default_choice]
+    - help = field[:help]
 
     .form-group
-      = f.label name, class: "control-label"
+      = f.label name, title, class: "control-label"
       .col-sm-10
         - if type == 'text'
           = f.text_field name, class: "form-control", placeholder: placeholder
@@ -72,6 +74,8 @@
           = f.select name, options_for_select(choices, value ? value : default_choice), {}, { class: "form-control" }
         - elsif type == 'password'
           = f.password_field name, class: 'form-control'
+        - if help
+          %span.help-block= help
 
   .form-actions
     = f.submit 'Save', class: 'btn btn-save'

app/views/layouts/notify.html.haml

@@ -16,6 +16,18 @@
       font-size:small;
       color:#777
     }
+    pre.commit-message {
+      white-space: pre-wrap;
+    }
+    .file-stats a {
+      text-decoration: none;
+    }
+    .file-stats .new-file {
+      color: #090;
+    }
+    .file-stats .deleted-file {
+      color: #B00;
+    }
     #{add_email_highlight_css}
   %body
     %div.content
@@ -27,5 +39,5 @@
         - if @target_url
           #{link_to "View it on GitLab", @target_url}
           = email_action @target_url
-        - if @project
+        - if @project && !@disable_footer
           You're receiving this notification because you are a member of the #{link_to_unless @target_url, @project.name_with_namespace, namespace_project_url(@project.namespace, @project)} project team.

app/views/notify/repository_push_email.html.haml

@@ -1,6 +1,12 @@
 %h3 #{@author.name} pushed to #{@branch} at #{link_to @project.name_with_namespace, namespace_project_url(@project.namespace, @project)}
 
-%h4 Commits:
+- if @reverse_compare
+  %p
+    %strong WARNING:
+    The push did not contain any new commits, but force pushed to delete the commits and changes below.
+
+%h4
+  = @reverse_compare ? "Deleted commits:" : "Commits:"
 
 %ul
   - @commits.each do |commit|
@@ -9,22 +15,52 @@
       %div
         %span by #{commit.author_name}
         %i at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
-      %pre #{commit.safe_message}
+      %pre.commit-message 
+        = commit.safe_message
 
-%h4 Changes:
-- @diffs.each do |diff|
-  %li
-    %strong
-      - if diff.old_path == diff.new_path
-        = diff.new_path
-      - elsif diff.new_path && diff.old_path
-        #{diff.old_path} → #{diff.new_path}
-      - else
-        = diff.new_path || diff.old_path
-    %hr
-    %pre
-      = color_email_diff(diff.diff)
-    %br
+%h4 #{pluralize @diffs.count, "changed file"}:
+
+%ul
+  - @diffs.each_with_index do |diff, i|
+    %li.file-stats
+      %a{href: "#{@target_url if @disable_diffs}#diff-#{i}" }
+        - if diff.deleted_file
+          %span.deleted-file
+            −
+            = diff.old_path
+        - elsif diff.renamed_file
+          = diff.old_path
+          →
+          = diff.new_path
+        - elsif diff.new_file
+          %span.new-file
+            +
+            = diff.new_path
+        - else
+          = diff.new_path
+
+- unless @disable_diffs
+  %h4 Changes:
+  - @diffs.each_with_index do |diff, i|
+    %li{id: "diff-#{i}"}
+      %a{href: @target_url + "#diff-#{i}"}
+        - if diff.deleted_file
+          %strong
+            = diff.old_path
+          deleted
+        - elsif diff.renamed_file
+          %strong
+            = diff.old_path
+          →
+          %strong
+            = diff.new_path
+        - else
+          %strong
+            = diff.new_path
+      %hr
+      %pre
+        = color_email_diff(diff.diff)
+      %br
 
 - if @compare.timeout
   %h5 Huge diff. To prevent performance issues changes are hidden

app/views/notify/repository_push_email.text.haml

@@ -1,25 +1,47 @@
-#{@author.name} pushed to #{@branch} at #{link_to @project.name_with_namespace, namespace_project_url(@project.namespace, @project)}
-
+#{@author.name} pushed to #{@branch} at #{@project.name_with_namespace}
 \
-Commits:
+\
+- if @reverse_compare
+  WARNING: The push did not contain any new commits, but force pushed to delete the commits and changes below.
+  \
+  \
+= @reverse_compare ? "Deleted commits:" : "Commits:"
 - @commits.each do |commit|
-  #{link_to commit.short_id, namespace_project_commit_url(@project.namespace, @project, commit)} by #{commit.author_name}
+  #{commit.short_id} by #{commit.author_name} at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
   #{commit.safe_message}
   \- - - - -
 \
 \
-Changes:
-- @diffs.each do |diff|
-  \
-  \=====================================
-  - if diff.old_path == diff.new_path
-    = diff.new_path
-  - elsif diff.new_path && diff.old_path
-    #{diff.old_path} → #{diff.new_path}
-  - else
-    = diff.new_path || diff.old_path
-  \=====================================
-  != diff.diff
+#{pluralize @diffs.count, "changed file"}:
 \
+- @diffs.each do |diff|
+  - if diff.deleted_file
+    \- − #{diff.old_path}
+  - elsif diff.renamed_file
+    \- #{diff.old_path} → #{diff.new_path}
+  - elsif diff.new_file
+    \- + #{diff.new_path}
+  - else
+    \- #{diff.new_path}
+- unless @disable_diffs
+  \
+  \
+  Changes:
+  - @diffs.each do |diff|
+    \
+    \=====================================
+    - if diff.deleted_file
+      #{diff.old_path} deleted
+    - elsif diff.renamed_file
+      #{diff.old_path} → #{diff.new_path}
+    - else
+      = diff.new_path
+    \=====================================
+    != diff.diff
 - if @compare.timeout
+  \
+  \
   Huge diff. To prevent performance issues it was hidden
+\
+\
+View it on GitLab: #{@target_url}

app/views/projects/diffs/_stats.html.haml

@@ -26,7 +26,7 @@
               %a{href: "#diff-#{i}"}
                 %i.fa.fa-minus
                 = diff.old_path
-                \->
+                →
                 = diff.new_path
           - elsif diff.new_file
             %span.new-file

app/views/projects/services/_form.html.haml

@@ -74,14 +74,16 @@
 
   - @service.fields.each do |field|
     - name = field[:name]
+    - title = field[:title] || name.humanize
     - value = @service.send(name) unless field[:type] == 'password'
     - type = field[:type]
     - placeholder = field[:placeholder]
     - choices = field[:choices]
     - default_choice = field[:default_choice]
+    - help = field[:help]
 
     .form-group
-      = f.label name, class: "control-label"
+      = f.label name, title, class: "control-label"
       .col-sm-10
         - if type == 'text'
           = f.text_field name, class: "form-control", placeholder: placeholder
@@ -93,6 +95,8 @@
           = f.select name, options_for_select(choices, value ? value : default_choice), {}, { class: "form-control" }
         - elsif type == 'password'
           = f.password_field name, class: 'form-control'
+        - if help
+          %span.help-block= help
 
   .form-actions
     = f.submit 'Save', class: 'btn btn-save'

app/workers/emails_on_push_worker.rb

@@ -1,7 +1,7 @@
 class EmailsOnPushWorker
   include Sidekiq::Worker
 
-  def perform(project_id, recipients, push_data)
+  def perform(project_id, recipients, push_data, send_from_committer_email = false, disable_diffs = false)
     project = Project.find(project_id)
     before_sha = push_data["before"]
     after_sha = push_data["after"]
@@ -15,11 +15,27 @@ class EmailsOnPushWorker
 
     compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
 
-    # Do not send emails if git compare failed
-    return false unless compare && compare.commits.present?
+    return false if compare.same
+
+    if compare.commits.empty?
+      compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
+
+      reverse_compare = true
+
+      return false if compare.commits.empty?
+    end
 
     recipients.split(" ").each do |recipient|
-      Notify.repository_push_email(project_id, recipient, author_id, branch, compare).deliver
+      Notify.repository_push_email(
+        project_id, 
+        recipient, 
+        author_id, 
+        branch, 
+        compare, 
+        reverse_compare,
+        send_from_committer_email,
+        disable_diffs
+      ).deliver
     end
   ensure
     compare = nil

spec/mailers/notify_spec.rb

@@ -568,9 +568,10 @@ describe Notify do
     let(:user) { create(:user) }
     let(:compare) { Gitlab::Git::Compare.new(project.repository.raw_repository, sample_image_commit.id, sample_commit.id) }
     let(:commits) { Commit.decorate(compare.commits) }
-    let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: commits.first, to: commits.last) }
+    let(:diff_path) { namespace_project_compare_path(project.namespace, project, from: Commit.new(compare.base), to: Commit.new(compare.head)) }
+    let(:send_from_committer_email) { false }
 
-    subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) }
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare, false, send_from_committer_email) }
 
     it 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -583,7 +584,7 @@ describe Notify do
     end
 
     it 'has the correct subject' do
-      is_expected.to have_subject /#{commits.length} new commits pushed to repository/
+      is_expected.to have_subject /\[#{project.path_with_namespace}\]\[master\] #{commits.length} commits:/
     end
 
     it 'includes commits list' do
@@ -597,6 +598,58 @@ describe Notify do
     it 'contains a link to the diff' do
       is_expected.to have_body_text /#{diff_path}/
     end
+
+    it 'doesn not contain the misleading footer' do
+      is_expected.not_to have_body_text /you are a member of/
+    end
+
+    context "when set to send from committer email if domain matches" do
+
+      let(:send_from_committer_email) { true }
+
+      before do
+        allow(Gitlab.config.gitlab).to receive(:host).and_return("gitlab.corp.company.com")
+      end
+
+      context "when the committer email domain is within the GitLab domain" do
+
+        before do
+          user.update_attribute(:email, "user@company.com")
+          user.confirm!
+        end
+
+        it "is sent from the committer email" do
+          sender = subject.header[:from].addrs[0]
+          expect(sender.address).to eq(user.email)
+        end
+      end
+
+      context "when the committer email domain is not completely within the GitLab domain" do
+
+        before do
+          user.update_attribute(:email, "user@something.company.com")
+          user.confirm!
+        end
+
+        it "is sent from the default email" do
+          sender = subject.header[:from].addrs[0]
+          expect(sender.address).to eq(gitlab_sender)
+        end
+      end
+
+      context "when the committer email domain is outside the GitLab domain" do
+
+        before do
+          user.update_attribute(:email, "user@mpany.com")
+          user.confirm!
+        end
+
+        it "is sent from the default email" do
+          sender = subject.header[:from].addrs[0]
+          expect(sender.address).to eq(gitlab_sender)
+        end
+      end
+    end
   end
 
   describe 'email on push with a single commit' do