From f49a2ac0df978eaf897a8c8b28a202ae9a01165f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 28 Mar 2013 12:14:53 +0200 Subject: [PATCH] Add close issue/mr methods to Notify. Refactored Notificationservice --- app/mailers/emails/issues.rb | 8 +++ app/mailers/emails/merge_requests.rb | 13 ++++ app/observers/merge_request_observer.rb | 2 +- app/services/notification_service.rb | 61 +++++++++---------- app/views/notify/closed_issue_email.html.haml | 5 ++ .../closed_merge_request_email.html.haml | 2 +- spec/services/notification_service_spec.rb | 6 +- 7 files changed, 59 insertions(+), 38 deletions(-) create mode 100644 app/views/notify/closed_issue_email.html.haml diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index 5b69886f..dc0381f5 100644 --- a/app/mailers/emails/issues.rb +++ b/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 diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 35890460..76a0ae55 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/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 diff --git a/app/observers/merge_request_observer.rb b/app/observers/merge_request_observer.rb index 10f30155..e10e5049 100644 --- a/app/observers/merge_request_observer.rb +++ b/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) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 82ecd25e..c53a6cc5 100644 --- a/app/services/notification_service.rb +++ b/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 diff --git a/app/views/notify/closed_issue_email.html.haml b/app/views/notify/closed_issue_email.html.haml new file mode 100644 index 00000000..23ccc453 --- /dev/null +++ b/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 diff --git a/app/views/notify/closed_merge_request_email.html.haml b/app/views/notify/closed_merge_request_email.html.haml index c0b08fcc..0c6c79e0 100644 --- a/app/views/notify/closed_merge_request_email.html.haml +++ b/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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 6fe18bb7..0c6c0144 100644 --- a/spec/services/notification_service_spec.rb +++ b/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)