Merge branch 'emailsonpush-create-delete' into 'master'

Send EmailsOnPush email when branch or tag is created or deleted.

Addresses #1951, #1957 and #1925.

![Screen_Shot_2015-03-17_at_13.58.15](https://dev.gitlab.org/gitlab/gitlabhq/uploads/16ff25adb4b4a7e1923612e0652442b4/Screen_Shot_2015-03-17_at_13.58.15.png)

![Screen_Shot_2015-03-17_at_13.58.22](https://dev.gitlab.org/gitlab/gitlabhq/uploads/e346c1d84aba3a093b722d0a4167e289/Screen_Shot_2015-03-17_at_13.58.22.png)

![Screen_Shot_2015-03-17_at_13.58.28](https://dev.gitlab.org/gitlab/gitlabhq/uploads/720437ecc13f317c6d20eff82ac60bd7/Screen_Shot_2015-03-17_at_13.58.28.png)

![Screen_Shot_2015-03-17_at_13.58.34](https://dev.gitlab.org/gitlab/gitlabhq/uploads/2b302bb6cdbe27c96a8dff1375236602/Screen_Shot_2015-03-17_at_13.58.34.png)

See merge request !1709

CHANGELOG

@@ -33,6 +33,9 @@ v 7.10.0 (unreleased)
   - Don't show commit comment button when user is not signed in.
 
 v 7.9.0
+  - Send EmailsOnPush email when branch or tag is created or deleted.
+
+v 7.9.0 (unreleased)
   - Add HipChat integration documentation (Stan Hu)
   - Update documentation for object_kind field in Webhook push and tag push Webhooks (Stan Hu)
   - Fix broken email images (Hannes Rosenögger)

app/mailers/emails/projects.rb

@@ -16,31 +16,65 @@ module Emails
            subject: subject("Project was moved"))
     end
 
-    def repository_push_email(project_id, recipient, author_id, branch, compare, reverse_compare = false, send_from_committer_email = false, disable_diffs = false)
+    def repository_push_email(project_id, recipient,  author_id:, 
+                                                      ref:, 
+                                                      action:, 
+                                                      compare: nil, 
+                                                      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  = Gitlab::Git.ref_name(branch)
+      @ref_name  = Gitlab::Git.ref_name(ref)
+      @ref_type  = Gitlab::Git.tag_ref?(ref) ? "tag" : "branch"
+      @action  = action
       @disable_diffs = disable_diffs
 
-      @subject = "[#{@project.path_with_namespace}][#{@branch}] "
+      if @compare
+        @commits = Commit.decorate(compare.commits)
+        @diffs   = compare.diffs
+      end
+
+      @action_name = 
+        case action
+        when :create
+          "pushed new"
+        when :delete
+          "deleted"
+        else
+          "pushed to"
+        end
+
+      @subject = "[#{@project.path_with_namespace}]"
+      @subject << "[#{@ref_name}]" if action == :push
+      @subject << " "
+
+      if action == :push
+        if @commits.length > 1
+          @target_url = namespace_project_compare_url(@project.namespace,
+                                                      @project,
+                                                      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)
 
-      if @commits.length > 1
-        @target_url = namespace_project_compare_url(@project.namespace,
-                                                    @project,
-                                                    from: Commit.new(@compare.base),
-                                                    to:   Commit.new(@compare.head))
-        @subject << "Deleted " if @reverse_compare
-        @subject << "#{@commits.length} commits: #{@commits.first.title}"
+          @subject << "Deleted 1 commit: " if @reverse_compare
+          @subject << @commits.first.title
+        end
       else
-        @target_url = namespace_project_commit_url(@project.namespace,
-                                                   @project, @commits.first)
+        unless action == :delete
+          @target_url = namespace_project_tree_url(@project.namespace,
+                                                   @project, @ref_name)
+        end
 
-        @subject << "Deleted 1 commit: " if @reverse_compare
-        @subject << @commits.first.title
+        subject_action = @action_name.dup
+        subject_action[0] = subject_action[0].capitalize
+        @subject << "#{subject_action} #{@ref_type} #{@ref_name}"
       end
 
       @disable_footer = true

app/models/project_services/emails_on_push_service.rb

@@ -36,13 +36,19 @@ class EmailsOnPushService < Service
   end
 
   def supported_events
-    %w(push)
+    %w(push tag_push)
   end
 
   def execute(push_data)
     return unless supported_events.include?(push_data[:object_kind])
 
-    EmailsOnPushWorker.perform_async(project_id, recipients, push_data, send_from_committer_email?, disable_diffs?)
+    EmailsOnPushWorker.perform_async(
+      project_id, 
+      recipients, 
+      push_data, 
+      send_from_committer_email:  send_from_committer_email?, 
+      disable_diffs:              disable_diffs?
+    )
   end
 
   def send_from_committer_email?

app/views/notify/repository_push_email.html.haml

-%h3 #{@author.name} pushed to #{@branch} at #{link_to @project.name_with_namespace, namespace_project_url(@project.namespace, @project)}
+%h3 #{@author.name} #{@action_name} #{@ref_type} #{@ref_name} at #{link_to @project.name_with_namespace, namespace_project_url(@project.namespace, @project)}
 
-- if @reverse_compare
-  %p
-    %strong WARNING:
-    The push did not contain any new commits, but force pushed to delete the commits and changes below.
+- if @compare
+  - 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:"
+  %h4
+    = @reverse_compare ? "Deleted commits:" : "Commits:"
 
-%ul
-  - @commits.each do |commit|
-    %li
-      %strong #{link_to commit.short_id, namespace_project_commit_url(@project.namespace, @project, commit)}
-      %div
-        %span by #{commit.author_name}
-        %i at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
-      %pre.commit-message 
-        = commit.safe_message
+  %ul
+    - @commits.each do |commit|
+      %li
+        %strong #{link_to commit.short_id, namespace_project_commit_url(@project.namespace, @project, commit)}
+        %div
+          %span by #{commit.author_name}
+          %i at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
+        %pre.commit-message 
+          = commit.safe_message
 
-%h4 #{pluralize @diffs.count, "changed file"}:
+  %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
-            −
+  %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
-        - 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
+          - elsif diff.new_file
+            %span.new-file
+              +
+              = diff.new_path
+          - else
             = 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
+  - 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

-#{@author.name} pushed to #{@branch} at #{@project.name_with_namespace}
-\
-\
-- if @reverse_compare
-  WARNING: The push did not contain any new commits, but force pushed to delete the commits and changes below.
+#{@author.name} #{@action_name} #{@ref_type} #{@ref_name} at #{@project.name_with_namespace}
+- if @compare
   \
   \
-= @reverse_compare ? "Deleted commits:" : "Commits:"
-- @commits.each do |commit|
-  #{commit.short_id} by #{commit.author_name} at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
-  #{commit.safe_message}
-  \- - - - -
-\
-\
-#{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
+  - 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|
+    #{commit.short_id} by #{commit.author_name} at #{commit.committed_date.strftime("%Y-%m-%dT%H:%M:%SZ")}
+    #{commit.safe_message}
+    \- - - - -
+  \
   \
+  #{pluralize @diffs.count, "changed file"}:
   \
-  Changes:
   - @diffs.each do |diff|
-    \
-    \=====================================
     - if diff.deleted_file
-      #{diff.old_path} deleted
+      \- − #{diff.old_path}
     - elsif diff.renamed_file
-      #{diff.old_path} → #{diff.new_path}
+      \- #{diff.old_path} → #{diff.new_path}
+    - elsif diff.new_file
+      \- + #{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}
+      \- #{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
+  - if @target_url
+    \
+    \
+    View it on GitLab: #{@target_url}

app/workers/emails_on_push_worker.rb

 class EmailsOnPushWorker
   include Sidekiq::Worker
 
-  def perform(project_id, recipients, push_data, send_from_committer_email = false, disable_diffs = false)
+  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"]
-    branch = push_data["ref"]
+    ref = push_data["ref"]
     author_id = push_data["user_id"]
 
-    if Gitlab::Git.blank_ref?(before_sha) || Gitlab::Git.blank_ref?(after_sha)
-      # skip if new branch was pushed or branch was removed
-      return true
-    end
+    action = 
+      if Gitlab::Git.blank_ref?(before_sha)
+        :create 
+      elsif Gitlab::Git.blank_ref?(after_sha)
+        :delete
+      else
+        :push
+      end
 
-    compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
+    compare = nil
+    reverse_compare = false
+    if action == :push
+      compare = Gitlab::Git::Compare.new(project.repository.raw_repository, before_sha, after_sha)
 
-    return false if compare.same
+      return false if compare.same
 
-    if compare.commits.empty?
-      compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
+      if compare.commits.empty?
+        compare = Gitlab::Git::Compare.new(project.repository.raw_repository, after_sha, before_sha)
 
-      reverse_compare = true
+        reverse_compare = true
 
-      return false if compare.commits.empty?
+        return false if compare.commits.empty?
+      end
     end
 
     recipients.split(" ").each do |recipient|
       Notify.repository_push_email(
         project_id, 
         recipient, 
-        author_id, 
-        branch, 
-        compare, 
-        reverse_compare,
-        send_from_committer_email,
-        disable_diffs
+        author_id:                  author_id, 
+        ref:                        ref, 
+        action:                     action,
+        compare:                    compare, 
+        reverse_compare:            reverse_compare,
+        send_from_committer_email:  send_from_committer_email,
+        disable_diffs:              disable_diffs
       ).deliver
     end
   ensure

spec/mailers/notify_spec.rb

@@ -640,6 +640,100 @@ describe Notify do
     end
   end
 
+  describe 'email on push for a created branch' do
+    let(:example_site_path) { root_path }
+    let(:user) { create(:user) }
+    let(:tree_path) { namespace_project_tree_path(project.namespace, project, "master") }
+
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :create) }
+
+    it 'is sent as the author' do
+      sender = subject.header[:from].addrs[0]
+      expect(sender.display_name).to eq(user.name)
+      expect(sender.address).to eq(gitlab_sender)
+    end
+
+    it 'is sent to recipient' do
+      is_expected.to deliver_to 'devs@company.name'
+    end
+
+    it 'has the correct subject' do
+      is_expected.to have_subject /Pushed new branch master/
+    end
+
+    it 'contains a link to the branch' do
+      is_expected.to have_body_text /#{tree_path}/
+    end
+  end
+
+  describe 'email on push for a created tag' do
+    let(:example_site_path) { root_path }
+    let(:user) { create(:user) }
+    let(:tree_path) { namespace_project_tree_path(project.namespace, project, "v1.0") }
+
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :create) }
+
+    it 'is sent as the author' do
+      sender = subject.header[:from].addrs[0]
+      expect(sender.display_name).to eq(user.name)
+      expect(sender.address).to eq(gitlab_sender)
+    end
+
+    it 'is sent to recipient' do
+      is_expected.to deliver_to 'devs@company.name'
+    end
+
+    it 'has the correct subject' do
+      is_expected.to have_subject /Pushed new tag v1\.0/
+    end
+
+    it 'contains a link to the tag' do
+      is_expected.to have_body_text /#{tree_path}/
+    end
+  end
+
+  describe 'email on push for a deleted branch' do
+    let(:example_site_path) { root_path }
+    let(:user) { create(:user) }
+
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :delete) }
+
+    it 'is sent as the author' do
+      sender = subject.header[:from].addrs[0]
+      expect(sender.display_name).to eq(user.name)
+      expect(sender.address).to eq(gitlab_sender)
+    end
+
+    it 'is sent to recipient' do
+      is_expected.to deliver_to 'devs@company.name'
+    end
+
+    it 'has the correct subject' do
+      is_expected.to have_subject /Deleted branch master/
+    end
+  end
+
+  describe 'email on push for a deleted tag' do
+    let(:example_site_path) { root_path }
+    let(:user) { create(:user) }
+
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) }
+
+    it 'is sent as the author' do
+      sender = subject.header[:from].addrs[0]
+      expect(sender.display_name).to eq(user.name)
+      expect(sender.address).to eq(gitlab_sender)
+    end
+
+    it 'is sent to recipient' do
+      is_expected.to deliver_to 'devs@company.name'
+    end
+
+    it 'has the correct subject' do
+      is_expected.to have_subject /Deleted tag v1\.0/
+    end
+  end
+
   describe 'email on push with multiple commits' do
     let(:example_site_path) { root_path }
     let(:user) { create(:user) }
@@ -648,7 +742,7 @@ describe Notify do
     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, false, send_from_committer_email) }
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, send_from_committer_email: send_from_committer_email) }
 
     it 'is sent as the author' do
       sender = subject.header[:from].addrs[0]
@@ -736,7 +830,7 @@ describe Notify do
     let(:commits) { Commit.decorate(compare.commits) }
     let(:diff_path) { namespace_project_commit_path(project.namespace, project, commits.first) }
 
-    subject { Notify.repository_push_email(project.id, 'devs@company.name', user.id, 'master', compare) }
+    subject { Notify.repository_push_email(project.id, 'devs@company.name', author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare) }
 
     it 'is sent as the author' do
       sender = subject.header[:from].addrs[0]