diff --git a/CHANGELOG b/CHANGELOG index 09b60e8e54a..7216676d5e8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.10.0 (unreleased) - Add a service to support external wikis (Hannes Rosenögger) + - Send EmailsOnPush email when branch or tag is created or deleted. v 7.9.0 (unreleased) - Add HipChat integration documentation (Stan Hu) diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index b55129de292..d2165c9f764 100644 --- a/app/mailers/emails/projects.rb +++ b/app/mailers/emails/projects.rb @@ -16,31 +16,59 @@ 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, 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 - 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}" + @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) + + @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 diff --git a/app/models/project_services/emails_on_push_service.rb b/app/models/project_services/emails_on_push_service.rb index acb5e7f1af5..3e465ab1a38 100644 --- a/app/models/project_services/emails_on_push_service.rb +++ b/app/models/project_services/emails_on_push_service.rb @@ -36,7 +36,7 @@ class EmailsOnPushService < Service end def supported_events - %w(push) + %w(push tag_push) end def execute(push_data) diff --git a/app/views/notify/repository_push_email.html.haml b/app/views/notify/repository_push_email.html.haml index 039b92df2be..bbf7004c906 100644 --- a/app/views/notify/repository_push_email.html.haml +++ b/app/views/notify/repository_push_email.html.haml @@ -1,66 +1,67 @@ -%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 + - elsif diff.new_file + %span.new-file + + + = diff.new_path + - else = 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 + - 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 + - if @compare.timeout + %h5 Huge diff. To prevent performance issues changes are hidden diff --git a/app/views/notify/repository_push_email.text.haml b/app/views/notify/repository_push_email.text.haml index 8d67a42234e..97a176ed2a3 100644 --- a/app/views/notify/repository_push_email.text.haml +++ b/app/views/notify/repository_push_email.text.haml @@ -1,47 +1,49 @@ -#{@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 - \ - \ - Changes: - - @diffs.each do |diff| + - 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"}: + \ + - @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} diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb index e59ca81defe..429b29525dc 100644 --- a/app/workers/emails_on_push_worker.rb +++ b/app/workers/emails_on_push_worker.rb @@ -5,24 +5,32 @@ class EmailsOnPushWorker 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| @@ -30,7 +38,8 @@ class EmailsOnPushWorker project_id, recipient, author_id, - branch, + ref, + action, compare, reverse_compare, send_from_committer_email, diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index e3a3b542358..aeb0e14d836 100644 --- a/spec/mailers/notify_spec.rb +++ b/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', user.id, 'refs/heads/master', :create, nil) } + + 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', user.id, 'refs/tags/v1.0', :create, nil) } + + 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', user.id, 'refs/heads/master', :delete, nil) } + + 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', user.id, 'refs/tags/v1.0', :delete, nil) } + + 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', user.id, 'refs/heads/master', :push, compare, false, 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', user.id, 'refs/heads/master', :push, compare) } it 'is sent as the author' do sender = subject.header[:from].addrs[0]