diff --git a/CHANGELOG b/CHANGELOG index cee6804bfa7f842d3c5b803624edc2cfe05ed1d3..9ca05156a03395c84fa5c9d5135a6e96e0c27be8 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 8.4.0 (unreleased) - Ensure Gravatar host looks like an actual host + - Consider re-assign as a mention from a notification point of view - Add pagination headers to already paginated API resources - Properly generate diff of orphan commits, like the first commit in a repository - Improve the consistency of commit titles, branch names, tag names, issue/MR titles, on their respective project pages diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index e4edc55bf696a0a860d5f27a423c9a7baa04b6c1..ca8a41d93b89c3199f7daa530f15f36f708c88f2 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -376,10 +376,10 @@ class NotificationService end def reassign_resource_email(target, project, current_user, method) - previous_assignee_id = previous_record(target, "assignee_id") + previous_assignee_id = previous_record(target, 'assignee_id') previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id - recipients = build_recipients(target, project, current_user, [previous_assignee]) + recipients = build_recipients(target, project, current_user, action: :reassign, previous_assignee: previous_assignee) recipients.each do |recipient| mailer.send( @@ -400,22 +400,27 @@ class NotificationService end end - def build_recipients(target, project, current_user, extra_recipients = nil) + def build_recipients(target, project, current_user, action: nil, previous_assignee: nil) recipients = target.participants(current_user) - recipients = recipients.concat(extra_recipients).compact.uniq if extra_recipients - recipients = add_project_watchers(recipients, project) recipients = reject_mention_users(recipients, project) - recipients = reject_muted_users(recipients, project) + # Re-assign is considered as a mention of the new assignee so we add the + # new assignee to the list of recipients after we rejected users with + # the "on mention" notification level + if action == :reassign + recipients << previous_assignee if previous_assignee + recipients << target.assignee + end + + recipients = reject_muted_users(recipients, project) recipients = add_subscribed_users(recipients, target) recipients = reject_unsubscribed_users(recipients, target) recipients.delete(current_user) - recipients = recipients.uniq - recipients + recipients.uniq end def mailer diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 6d219f35895bf13af9ff59b6e9bfe646089756b5..2d0b5df422462fc6398d88e5e3a683dc16c6b223 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -227,7 +227,7 @@ describe NotificationService, services: true do end describe :reassigned_issue do - it 'should email new assignee' do + it 'emails new assignee' do notification.reassigned_issue(issue, @u_disabled) should_email(issue.assignee) @@ -238,6 +238,62 @@ describe NotificationService, services: true do should_not_email(@u_participating) should_not_email(@u_disabled) end + + it 'emails previous assignee even if he has the "on mention" notif level' do + issue.update_attribute(:assignee, @u_mentioned) + issue.update_attributes(assignee: @u_watcher) + notification.reassigned_issue(issue, @u_disabled) + + should_email(@u_mentioned) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end + + it 'emails new assignee even if he has the "on mention" notif level' do + issue.update_attributes(assignee: @u_mentioned) + notification.reassigned_issue(issue, @u_disabled) + + expect(issue.assignee).to be @u_mentioned + should_email(issue.assignee) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end + + it 'emails new assignee' do + issue.update_attribute(:assignee, @u_mentioned) + notification.reassigned_issue(issue, @u_disabled) + + expect(issue.assignee).to be @u_mentioned + should_email(issue.assignee) + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end + + it 'does not email new assignee if they are the current user' do + issue.update_attribute(:assignee, @u_mentioned) + notification.reassigned_issue(issue, @u_mentioned) + + expect(issue.assignee).to be @u_mentioned + should_email(@u_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_not_email(issue.assignee) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + end end describe :close_issue do