Merge branch...

Merge branch 'YarNayar/gitlab-ce-23460-send-email-when-pushing-more-commits-to-the-merge-request' into 'master'

Send notification emails when push to a merge request

Closes #23460

See merge request gitlab-org/gitlab-ce!17865

app/mailers/emails/merge_requests.rb

@@ -11,6 +11,14 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end
+    def push_to_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil, new_commits: [], existing_commits: [])
+      setup_merge_request_mail(merge_request_id, recipient_id)
+      @new_commits = new_commits
+      @existing_commits = existing_commits
+
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
+    end
+
     def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id, updated_by_user_id, reason = nil)
       setup_merge_request_mail(merge_request_id, recipient_id)

app/models/commit.rb

@@ -175,7 +175,7 @@ class Commit
       if safe_message.blank?
         no_commit_message
       else
-        safe_message.split("\n", 2).first
+        safe_message.split(/[\r\n]/, 2).first
       end
   end

app/models/notification_recipient.rb

@@ -48,7 +48,7 @@ class NotificationRecipient
     when :custom
       custom_enabled? || %i[participating mention].include?(@type)
     when :watch, :participating
-      !excluded_watcher_action?
+      !action_excluded?
     when :mention
       @type == :mention
     else
@@ -96,13 +96,22 @@ class NotificationRecipient
     end
   end
+  def action_excluded?
+    excluded_watcher_action? || excluded_participating_action?
+  end
+
   def excluded_watcher_action?
-    return false unless @custom_action
-    return false if notification_level == :custom
+    return false unless @custom_action && notification_level == :watch
     NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action)
   end
+  def excluded_participating_action?
+    return false unless @custom_action && notification_level == :participating
+
+    NotificationSetting::EXCLUDED_PARTICIPATING_EVENTS.include?(@custom_action)
+  end
+
   private
   def read_ability

app/models/notification_setting.rb

@@ -33,6 +33,7 @@ class NotificationSetting < ActiveRecord::Base
     :close_issue,
     :reassign_issue,
     :new_merge_request,
+    :push_to_merge_request,
     :reopen_merge_request,
     :close_merge_request,
     :reassign_merge_request,
@@ -41,10 +42,14 @@ class NotificationSetting < ActiveRecord::Base
     :success_pipeline
   ].freeze
-  EXCLUDED_WATCHER_EVENTS = [
+  EXCLUDED_PARTICIPATING_EVENTS = [
     :success_pipeline
   ].freeze
+  EXCLUDED_WATCHER_EVENTS = [
+    :push_to_merge_request
+  ].push(*EXCLUDED_PARTICIPATING_EVENTS).freeze
+
   def self.find_or_create_for(source)
     setting = find_or_initialize_by(source: source)

app/services/merge_requests/refresh_service.rb

@@ -21,7 +21,7 @@ module MergeRequests
         comment_mr_branch_presence_changed
       end
-      comment_mr_with_commits
+      notify_about_push
       mark_mr_as_wip_from_commits
       execute_mr_web_hooks
@@ -141,8 +141,8 @@ module MergeRequests
       end
     end
-    # Add comment about pushing new commits to merge requests
-    def comment_mr_with_commits
+    # Add comment about pushing new commits to merge requests and send nofitication emails
+    def notify_about_push
       return unless @commits.present?
       merge_requests_for_source_branch.each do |merge_request|
@@ -155,6 +155,8 @@ module MergeRequests
         SystemNoteService.add_commits(merge_request, merge_request.project,
                                       @current_user, new_commits,
                                       existing_commits, @oldrev)
+
+        notification_service.push_to_merge_request(merge_request, @current_user, new_commits: new_commits, existing_commits: existing_commits)
       end
     end

app/services/notification_service.rb

@@ -113,6 +113,16 @@ class NotificationService
     new_resource_email(merge_request, :new_merge_request_email)
   end
+  def push_to_merge_request(merge_request, current_user, new_commits: [], existing_commits: [])
+    new_commits = new_commits.map { |c| { short_id: c.short_id, title: c.title } }
+    existing_commits = existing_commits.map { |c| { short_id: c.short_id, title: c.title } }
+    recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: "push_to")
+
+    recipients.each do |recipient|
+      mailer.send(:push_to_merge_request_email, recipient.user.id, merge_request.id, current_user.id, recipient.reason, new_commits: new_commits, existing_commits: existing_commits).deliver_later
+    end
+  end
+
   # When merge request text is updated, we should send an email to:
   #
   #  * newly mentioned project team members with notification level higher than Participating

app/views/notify/push_to_merge_request_email.html.haml

+%h3
+  New commits were pushed to the merge request
+  = link_to(@merge_request.to_reference, project_merge_request_url(@merge_request.target_project, @merge_request))
+  by #{@current_user.name}
+
+- if @existing_commits.any?
+  - count = @existing_commits.size
+  %ul
+    %li
+      - if count.one?
+        - commit_id = @existing_commits.first[:short_id]
+        = link_to(commit_id, project_commit_url(@merge_request.target_project, commit_id))
+      - else
+        = link_to(project_compare_url(@merge_request.target_project, from: @existing_commits.first[:short_id], to: @existing_commits.last[:short_id])) do
+          #{@existing_commits.first[:short_id]}...#{@existing_commits.last[:short_id]}
+      = precede ' - ' do
+        - commits_text = "#{count} commit".pluralize(count)
+        #{commits_text} from branch `#{@merge_request.target_branch}`
+
+- if @new_commits.any?
+  %ul
+    - @new_commits.each do |commit|
+      %li
+        = link_to(commit[:short_id], project_commit_url(@merge_request.target_project, commit[:short_id]))
+        = precede ' - ' do
+          #{commit[:title]}

app/views/notify/push_to_merge_request_email.text.haml

+New commits were pushed to the merge request #{@merge_request.to_reference} by #{@current_user.name}
+\
+#{url_for(project_merge_request_url(@merge_request.target_project, @merge_request))}
+\
+- if @existing_commits.any?
+  - count = @existing_commits.size
+  - commits_id = count.one? ? @existing_commits.first[:short_id] : "#{@existing_commits.first[:short_id]}...#{@existing_commits.last[:short_id]}"
+  - commits_text = "#{count} commit".pluralize(count)
+
+  * #{commits_id} - #{commits_text} from branch `#{@merge_request.target_branch}`
+\
+- @new_commits.each do |commit|
+  * #{commit[:short_id]} - #{raw commit[:title]}

changelogs/unreleased/23460-send-email-when-pushing-more-commits-to-the-merge-request.yml

+---
+title: Send notification emails when push to a merge request
+merge_request: 7610
+author: YarNayar
+type: feature

db/migrate/20180323150945_add_push_to_merge_request_to_notification_settings.rb

+class AddPushToMergeRequestToNotificationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :notification_settings, :push_to_merge_request, :boolean
+  end
+end

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: 20180320182229) do
+ActiveRecord::Schema.define(version: 20180323150945) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1296,6 +1296,7 @@ ActiveRecord::Schema.define(version: 20180320182229) do
     t.boolean "merge_merge_request"
     t.boolean "failed_pipeline"
     t.boolean "success_pipeline"
+    t.boolean "push_to_merge_request"
   end
   add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree

doc/api/notification_settings.md

@@ -24,6 +24,7 @@ reopen_issue
 close_issue
 reassign_issue
 new_merge_request
+push_to_merge_request
 reopen_merge_request
 close_merge_request
 reassign_merge_request
@@ -75,6 +76,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab
 | `close_issue` | boolean | no | Enable/disable this notification |
 | `reassign_issue` | boolean | no | Enable/disable this notification |
 | `new_merge_request` | boolean | no | Enable/disable this notification |
+| `push_to_merge_request` | boolean | no | Enable/disable this notification |
 | `reopen_merge_request` | boolean | no | Enable/disable this notification |
 | `close_merge_request` | boolean | no | Enable/disable this notification |
 | `reassign_merge_request` | boolean | no | Enable/disable this notification |
@@ -141,6 +143,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab
 | `close_issue` | boolean | no | Enable/disable this notification |
 | `reassign_issue` | boolean | no | Enable/disable this notification |
 | `new_merge_request` | boolean | no | Enable/disable this notification |
+| `push_to_merge_request` | boolean | no | Enable/disable this notification |
 | `reopen_merge_request` | boolean | no | Enable/disable this notification |
 | `close_merge_request` | boolean | no | Enable/disable this notification |
 | `reassign_merge_request` | boolean | no | Enable/disable this notification |
@@ -164,6 +167,7 @@ Example responses:
     "close_issue": false,
     "reassign_issue": false,
     "new_merge_request": false,
+    "push_to_merge_request": false,
     "reopen_merge_request": false,
     "close_merge_request": false,
     "reassign_merge_request": false,

doc/workflow/notifications.md

@@ -67,7 +67,7 @@ Below is the table of events users can be notified of:
 ### Issue / Merge request events
-In all of the below cases, the notification will be sent to:
+In most of the below cases, the notification will be sent to:
 - Participants:
   - the author and assignee of the issue/merge request
   - authors of comments on the issue/merge request
@@ -87,6 +87,7 @@ In all of the below cases, the notification will be sent to:
 | Reassign issue         | The above, plus the old assignee |
 | Reopen issue           | |
 | New merge request      | |
+| Push to merge request  | Participants and Custom notification level with this event selected |
 | Reassign merge request | The above, plus the old assignee |
 | Close merge request    | |
 | Reopen merge request   | |

spec/services/merge_requests/refresh_service_spec.rb

@@ -24,6 +24,14 @@ describe MergeRequests::RefreshService do
                               merge_when_pipeline_succeeds: true,
                               merge_user: @user)
+      @another_merge_request = create(:merge_request,
+                                      source_project: @project,
+                                      source_branch: 'master',
+                                      target_branch: 'test',
+                                      target_project: @project,
+                                      merge_when_pipeline_succeeds: true,
+                                      merge_user: @user)
+
       @fork_merge_request = create(:merge_request,
                                    source_project: @fork_project,
                                    source_branch: 'master',
@@ -52,9 +60,11 @@ describe MergeRequests::RefreshService do
     context 'push to origin repo source branch' do
       let(:refresh_service) { service.new(@project, @user) }
+      let(:notification_service) { spy('notification_service') }
       before do
         allow(refresh_service).to receive(:execute_hooks)
+        allow(NotificationService).to receive(:new) { notification_service }
       end
       it 'executes hooks with update action' do
@@ -64,6 +74,11 @@ describe MergeRequests::RefreshService do
         expect(refresh_service).to have_received(:execute_hooks)
           .with(@merge_request, 'update', old_rev: @oldrev)
+        expect(notification_service).to have_received(:push_to_merge_request)
+          .with(@merge_request, @user, new_commits: anything, existing_commits: anything)
+        expect(notification_service).to have_received(:push_to_merge_request)
+          .with(@another_merge_request, @user, new_commits: anything, existing_commits: anything)
+
         expect(@merge_request.notes).not_to be_empty
         expect(@merge_request).to be_open
         expect(@merge_request.merge_when_pipeline_succeeds).to be_falsey
@@ -119,11 +134,13 @@ describe MergeRequests::RefreshService do
     context 'push to origin repo source branch when an MR was reopened' do
       let(:refresh_service) { service.new(@project, @user) }
+      let(:notification_service) { spy('notification_service') }
       before do
         @merge_request.update(state: :reopened)
         allow(refresh_service).to receive(:execute_hooks)
+        allow(NotificationService).to receive(:new) { notification_service }
         refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
         reload_mrs
       end
@@ -131,6 +148,10 @@ describe MergeRequests::RefreshService do
       it 'executes hooks with update action' do
         expect(refresh_service).to have_received(:execute_hooks)
           .with(@merge_request, 'update', old_rev: @oldrev)
+        expect(notification_service).to have_received(:push_to_merge_request)
+          .with(@merge_request, @user, new_commits: anything, existing_commits: anything)
+        expect(notification_service).to have_received(:push_to_merge_request)
+          .with(@another_merge_request, @user, new_commits: anything, existing_commits: anything)
         expect(@merge_request.notes).not_to be_empty
         expect(@merge_request).to be_open

spec/services/notification_service_spec.rb

@@ -1090,6 +1090,36 @@ describe NotificationService, :mailer do
       end
     end
+    describe '#push_to_merge_request' do
+      before do
+        update_custom_notification(:push_to_merge_request, @u_guest_custom, resource: project)
+        update_custom_notification(:push_to_merge_request, @u_custom_global)
+      end
+
+      it do
+        notification.push_to_merge_request(merge_request, @u_disabled)
+
+        should_email(merge_request.assignee)
+        should_email(@u_guest_custom)
+        should_email(@u_custom_global)
+        should_email(@u_participant_mentioned)
+        should_email(@subscriber)
+        should_email(@watcher_and_subscriber)
+        should_not_email(@u_watcher)
+        should_not_email(@u_guest_watcher)
+        should_not_email(@unsubscriber)
+        should_not_email(@u_participating)
+        should_not_email(@u_disabled)
+        should_not_email(@u_lazy_participant)
+      end
+
+      it_behaves_like 'participating notifications' do
+        let(:participant) { create(:user, username: 'user-participant') }
+        let(:issuable) { merge_request }
+        let(:notification_trigger) { notification.push_to_merge_request(merge_request, @u_disabled) }
+      end
+    end
+
     describe '#relabel_merge_request' do
       let(:group_label_1) { create(:group_label, group: group, title: 'Group Label 1', merge_requests: [merge_request]) }
       let(:group_label_2) { create(:group_label, group: group, title: 'Group Label 2') }