add watchers to email recipients list. Add emails for close/merge MR

app/observers/merge_request_observer.rb

@@ -7,6 +7,12 @@ class MergeRequestObserver < BaseObserver
 
   def after_close(merge_request, transition)
     Note.create_status_change_note(merge_request, current_user, merge_request.state)
+
+    notification.close_mr(merge_request, current_user)
+  end
+
+  def after_merge(merge_request, transition)
+    notification.merge_mr(merge_request, current_user)
   end
 
   def after_reopen(merge_request, transition)

app/services/notification_service.rb

@@ -17,7 +17,21 @@ class NotificationService
     end
   end
 
-  # TODO: When we close an issue we should send next emails:
+  # When create an issue we should send next emails:
+  #
+  #  * issue assignee if his notification level is not Disabled
+  #  * project team members with notification level higher then Participating
+  #
+  def new_issue(issue, current_user)
+    recipients = reject_muted_users([issue.assignee])
+    recipients = recipients.concat(project_watchers(issue.project)).uniq
+
+    recipients.each do |recipient|
+      Notify.delay.new_issue_email(recipient.id, issue.id)
+    end
+  end
+
+  # When we close an issue we should send next emails:
   #
   #  * issue author if his notification level is not Disabled
   #  * issue assignee if his notification level is not Disabled
@@ -26,6 +40,9 @@ class NotificationService
   def close_issue(issue, current_user)
     recipients = reject_muted_users([issue.author, issue.assignee])
 
+    # Add watchers to email list
+    recipients = recipients.concat(project_watchers(issue.project)).uniq
+
     # Dont send email to me when I close an issue
     recipients.delete(current_user)
 
@@ -43,30 +60,17 @@ class NotificationService
     reassign_email(issue, current_user, 'reassigned_issue_email')
   end
 
-  # When create an issue we should send next emails:
-  #
-  #  * issue assignee if his notification level is not Disabled
-  #
-  def new_issue(issue, current_user)
-
-    if issue.assignee && issue.assignee != current_user
-      # skip if assignee notification disabled
-      return true if issue.assignee.notification.disabled?
-
-      Notify.delay.new_issue_email(issue.id)
-    end
-  end
 
   # When create a merge request we should send next emails:
   #
   #  * mr assignee if his notification level is not Disabled
   #
   def new_merge_request(merge_request, current_user)
-    if merge_request.assignee && merge_request.assignee != current_user
+    recipients = reject_muted_users([merge_request.assignee])
-      # skip if assignee notification disabled
+    recipients = recipients.concat(project_watchers(merge_request.project)).uniq
-      return true if merge_request.assignee.notification.disabled?
 
-      Notify.delay.new_merge_request_email(merge_request.id)
+    recipients.each do |recipient|
+      Notify.delay.new_merge_request_email(recipient.id, merge_request.id)
     end
   end
 
@@ -79,6 +83,36 @@ class NotificationService
     reassign_email(merge_request, current_user, 'reassigned_merge_request_email')
   end
 
+  # When we close a merge request we should send next emails:
+  #
+  #  * merge_request author if his notification level is not Disabled
+  #  * merge_request assignee if his notification level is not Disabled
+  #  * project team members with notification level higher then Participating
+  #
+  def close_mr(merge_request)
+    recipients = reject_muted_users([merge_request.author, merge_request.assignee])
+    recipients = recipients.concat(project_watchers(merge_request.project)).uniq
+
+    recipients.each do |recipient|
+      Notify.delay.closed_merge_request_email(recipient.id, merge_request.id)
+    end
+  end
+
+  # When we merge a merge request we should send next emails:
+  #
+  #  * merge_request author if his notification level is not Disabled
+  #  * merge_request assignee if his notification level is not Disabled
+  #  * project team members with notification level higher then Participating
+  #
+  def merge_mr(merge_request)
+    recipients = reject_muted_users([merge_request.author, merge_request.assignee])
+    recipients = recipients.concat(project_watchers(merge_request.project)).uniq
+
+    recipients.each do |recipient|
+      Notify.delay.merged_merge_request_email(recipient.id, merge_request.id)
+    end
+  end
+
   # Notify new user with email after creation
   def new_user(user)
     # Dont email omniauth created users
@@ -119,6 +153,11 @@ class NotificationService
 
   protected
 
+  # Get project users with WATCH notification level
+  def project_watchers(project)
+    project.users.where(notification_level: Notification::N_WATCH)
+  end
+
   # Remove users with disabled notifications from array
   # Also remove duplications and nil recipients
   def reject_muted_users(users)
@@ -130,6 +169,9 @@ class NotificationService
   def reassign_email(target, current_user, method)
     recipients = User.where(id: [target.assignee_id, target.assignee_id_was])
 
+    # Add watchers to email list
+    recipients = recipients.concat(project_watchers(target.project))
+
     # reject users with disabled notifications
     recipients = reject_muted_users(recipients)
 

app/views/notify/closed_merge_request_email.html.haml

@@ -0,0 +1,9 @@
+%p
+  = "Merge Request #{@merge_request.id} was closed"
+%p
+  = link_to_gfm truncate(@merge_request.title, length: 40), project_merge_request_url(@merge_request.project, @merge_request)
+%p
+  Branches: #{@merge_request.source_branch} → #{@merge_request.target_branch}
+%p
+  Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
+

app/views/notify/merged_merge_request_email.html.haml

@@ -0,0 +1,9 @@
+%p
+  = "Merge Request #{@merge_request.id} was merged"
+%p
+  = link_to_gfm truncate(@merge_request.title, length: 40), project_merge_request_url(@merge_request.project, @merge_request)
+%p
+  Branches: #{@merge_request.source_branch} → #{@merge_request.target_branch}
+%p
+  Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
+

spec/services/notification_service_spec.rb

@@ -23,6 +23,10 @@ describe NotificationService do
   describe 'Issues' do
     let(:issue) { create :issue, assignee: create(:user) }
 
+    before do
+      build_team(issue.project)
+    end
+
     describe :new_issue do
       it 'should sent email to issue assignee' do
         Notify.should_receive(:new_issue_email).with(issue.id)
@@ -31,16 +35,41 @@ describe NotificationService do
     end
 
     describe :reassigned_issue do
-      it 'should sent email to issue old assignee and new issue assignee' do
+      it 'should email new assignee' do
-        Notify.should_receive(:reassigned_issue_email)
+        should_email(issue.assignee_id)
-        notification.reassigned_issue(issue, issue.author)
+        should_email(@u_watcher.id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+
+        notification.reassigned_issue(issue, @u_disabled)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id)
+      end
+
+      def should_not_email(user_id)
+        Notify.should_not_receive(:reassigned_issue_email).with(user_id, issue.id, issue.assignee_id)
       end
     end
 
     describe :close_issue do
       it 'should sent email to issue assignee and issue author' do
-        Notify.should_receive(:issue_status_changed_email)
+        should_email(issue.assignee_id)
-        notification.close_issue(issue, issue.author)
+        should_email(issue.author_id)
+        should_email(@u_watcher.id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+
+        notification.close_issue(issue, @u_disabled)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:issue_status_changed_email).with(user_id, issue.id, issue.assignee_id)
+      end
+
+      def should_not_email(user_id)
+        Notify.should_not_receive(:issue_status_changed_email).with(user_id, issue.id, issue.assignee_id)
       end
     end
   end
@@ -48,16 +77,90 @@ describe NotificationService do
   describe 'Merge Requests' do
     let(:merge_request) { create :merge_request, assignee: create(:user) }
 
-    describe :new_merge_request do
+    before do
-      it 'should send email to merge_request assignee' do
+      build_team(merge_request.project)
-        Notify.should_receive(:new_merge_request_email).with(merge_request.id)
-        notification.new_merge_request(merge_request, merge_request.author)
     end
 
-      it 'should not send email to merge_request assignee if he is current_user' do
+    describe :new_merge_request do
+      it do
+        should_email(merge_request.assignee_id)
+        should_email(@u_watcher.id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+        notification.new_merge_request(merge_request, @u_disabled)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:new_merge_request_email).with(merge_request.id)
+      end
+
+      def should_not_email(user_id)
         Notify.should_not_receive(:new_merge_request_email).with(merge_request.id)
-        notification.new_merge_request(merge_request, merge_request.assignee)
+      end
+    end
+
+    describe :reassigned_merge_request do
+      it do
+        should_email(merge_request.assignee_id)
+        should_email(@u_watcher.id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+        notification.reassigned_merge_request(merge_request, merge_request.author)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id)
+      end
+
+      def should_not_email(user_id)
+        Notify.should_not_receive(:reassigned_merge_request_email).with(user_id, merge_request.id, merge_request.assignee_id)
+      end
+    end
+
+    describe :closed_merge_request do
+      it do
+        should_email(merge_request.assignee_id)
+        should_email(@u_watcher.id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+        notification.close_mr(merge_request)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:closed_merge_request_email).with(user_id, merge_request.id)
+      end
+
+      def should_not_email(user_id)
+        Notify.should_not_receive(:closed_merge_request_email).with(user_id, merge_request.id)
+      end
+    end
+
+    describe :merged_merge_request do
+      it do
+        should_email(merge_request.assignee_id)
+        should_email(@u_watcher.id)
+        should_not_email(@u_participating.id)
+        should_not_email(@u_disabled.id)
+        notification.merge_mr(merge_request)
+      end
+
+      def should_email(user_id)
+        Notify.should_receive(:merged_merge_request_email).with(user_id, merge_request.id)
+      end
+
+      def should_not_email(user_id)
+        Notify.should_not_receive(:merged_merge_request_email).with(user_id, merge_request.id)
       end
     end
   end
+
+  def build_team(project)
+    @u_watcher = create(:user, notification_level: Notification::N_WATCH)
+    @u_participating = create(:user, notification_level: Notification::N_PARTICIPATING)
+    @u_disabled = create(:user, notification_level: Notification::N_DISABLED)
+
+    project.team << [@u_watcher, :master]
+    project.team << [@u_participating, :master]
+    project.team << [@u_disabled, :master]
+  end
 end