Add close issue/mr methods to Notify. Refactored Notificationservice

app/mailers/emails/issues.rb

@@ -13,6 +13,14 @@ module Emails
       mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title))
     end
 
+    def close_issue_email(recipient_id, issue_id, updated_by_user_id)
+      @issue = Issue.find issue_id
+      @project = @issue.project
+      @updated_by = User.find updated_by_user_id
+      mail(to: recipient(recipient_id),
+           subject: subject("Closed issue ##{@issue.id}", @issue.title))
+    end
+
     def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
       @issue = Issue.find issue_id
       @issue_status = status

app/mailers/emails/merge_requests.rb

@@ -12,5 +12,18 @@ module Emails
       @project = @merge_request.project
       mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title))
     end
+
+    def closed_merge_request_email(recipient_id, merge_request_id, updated_by_user_id)
+      @merge_request = MergeRequest.find(merge_request_id)
+      @project = @merge_request.project
+      @updated_by = User.find updated_by_user_id
+      mail(to: recipient(recipient_id), subject: subject("Closed merge request !#{@merge_request.id}", @merge_request.title))
+    end
+
+    def merged_merge_request_email(recipient_id, merge_request_id)
+      @merge_request = MergeRequest.find(merge_request_id)
+      @project = @merge_request.project
+      mail(to: recipient(recipient_id), subject: subject("Accepted merge request !#{@merge_request.id}", @merge_request.title))
+    end
   end
 end

app/observers/merge_request_observer.rb

@@ -12,7 +12,7 @@ class MergeRequestObserver < BaseObserver
   end
 
   def after_merge(merge_request, transition)
-    notification.merge_mr(merge_request, current_user)
+    notification.merge_mr(merge_request)
   end
 
   def after_reopen(merge_request, transition)

app/services/notification_service.rb

@@ -23,12 +23,7 @@ class NotificationService
   #  * 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
+    new_resource_email(issue, 'new_issue_email')
   end
 
   # When we close an issue we should send next emails:
@@ -38,17 +33,7 @@ class NotificationService
   #  * project team members with notification level higher then Participating
   #
   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)
-
-    recipients.each do |recipient|
-      Notify.delay.issue_status_changed_email(recipient.id, issue.id, issue.state, current_user.id)
-    end
+    close_resource_email(issue, current_user, 'close_issue_email')
   end
 
   # When we reassign an issue we should send next emails:
@@ -57,7 +42,7 @@ class NotificationService
   #  * issue new assignee if his notification level is not Disabled
   #
   def reassigned_issue(issue, current_user)
-    reassign_email(issue, current_user, 'reassigned_issue_email')
+    reassign_resource_email(issue, current_user, 'reassigned_issue_email')
   end
 
 
@@ -66,12 +51,7 @@ class NotificationService
   #  * mr assignee if his notification level is not Disabled
   #
   def new_merge_request(merge_request, current_user)
-    recipients = reject_muted_users([merge_request.assignee])
-    recipients = recipients.concat(project_watchers(merge_request.project)).uniq
-
-    recipients.each do |recipient|
-      Notify.delay.new_merge_request_email(recipient.id, merge_request.id)
-    end
+    new_resource_email(merge_request, 'new_merge_request_email')
   end
 
   # When we reassign a merge_request we should send next emails:
@@ -80,7 +60,7 @@ class NotificationService
   #  * merge_request assignee if his notification level is not Disabled
   #
   def reassigned_merge_request(merge_request, current_user)
-    reassign_email(merge_request, current_user, 'reassigned_merge_request_email')
+    reassign_resource_email(merge_request, current_user, 'reassigned_merge_request_email')
   end
 
   # When we close a merge request we should send next emails:
@@ -89,13 +69,8 @@ class NotificationService
   #  * 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
+  def close_mr(merge_request, current_user)
+    close_resource_email(merge_request, current_user, 'closed_merge_request_email')
   end
 
   # When we merge a merge request we should send next emails:
@@ -166,7 +141,27 @@ class NotificationService
     end
   end
 
-  def reassign_email(target, current_user, method)
+  def new_resource_email(target, method)
+    recipients = reject_muted_users([target.assignee])
+    recipients = recipients.concat(project_watchers(target.project)).uniq
+    recipients.delete(target.author)
+
+    recipients.each do |recipient|
+      Notify.delay.send(method, recipient.id, target.id)
+    end
+  end
+
+  def close_resource_email(target, current_user, method)
+    recipients = reject_muted_users([target.author, target.assignee])
+    recipients = recipients.concat(project_watchers(target.project)).uniq
+    recipients.delete(current_user)
+
+    recipients.each do |recipient|
+      Notify.delay.send(method, recipient.id, target.id, current_user.id)
+    end
+  end
+
+  def reassign_resource_email(target, current_user, method)
     recipients = User.where(id: [target.assignee_id, target.assignee_id_was])
 
     # Add watchers to email list

app/views/notify/closed_issue_email.html.haml

@@ -0,0 +1,5 @@
+%p
+  = "Issue was closed by #{@updated_by.name}"
+%p
+  = "Issue ##{@issue.id}"
+  = link_to_gfm truncate(@issue.title, length: 45), project_issue_url(@issue.project, @issue), title: @issue.title

app/views/notify/closed_merge_request_email.html.haml

@@ -1,5 +1,5 @@
 %p
-  = "Merge Request #{@merge_request.id} was closed"
+  = "Merge Request #{@merge_request.id} was closed by #{@updated_by.name}"
 %p
   = link_to_gfm truncate(@merge_request.title, length: 40), project_merge_request_url(@merge_request.project, @merge_request)
 %p

spec/services/notification_service_spec.rb

@@ -65,11 +65,11 @@ describe NotificationService do
       end
 
       def should_email(user_id)
-        Notify.should_receive(:issue_status_changed_email).with(user_id, issue.id, issue.assignee_id)
+        Notify.should_receive(:closed_issue_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)
+        Notify.should_not_receive(:closed_issue_email).with(user_id, issue.id, issue.assignee_id)
       end
     end
   end
@@ -123,7 +123,7 @@ describe NotificationService do
         should_email(@u_watcher.id)
         should_not_email(@u_participating.id)
         should_not_email(@u_disabled.id)
-        notification.close_mr(merge_request)
+        notification.close_mr(merge_request, @u_disabled)
       end
 
       def should_email(user_id)