Merge branch 'create-email-notifications-for-merge-request-reviewers' into 'master'

Create email notifications for merge request reviewers

See merge request gitlab-org/gitlab!41851

app/helpers/emails_helper.rb

@@ -218,8 +218,28 @@ module EmailsHelper
     _('Please contact your administrator with any questions.')
   end
 
+  def change_reviewer_notification_text(new_reviewers, previous_reviewers, html_tag = nil)
+    new = new_reviewers.any? ? users_to_sentence(new_reviewers) : s_('ChangeReviewer|Unassigned')
+    old = previous_reviewers.any? ? users_to_sentence(previous_reviewers) : nil
+
+    if html_tag.present?
+      new = content_tag(html_tag, new)
+      old = content_tag(html_tag, old) if old.present?
+    end
+
+    if old.present?
+      s_('ChangeReviewer|Reviewer changed from %{old} to %{new}').html_safe % { old: old, new: new }
+    else
+      s_('ChangeReviewer|Reviewer changed to %{new}').html_safe % { new: new }
+    end
+  end
+
   private
 
+  def users_to_sentence(users)
+    sanitize_name(users.map(&:name).to_sentence)
+  end
+
   def generate_link(text, url)
     link_to(text, url, target: :_blank, rel: 'noopener noreferrer')
   end

app/mailers/emails/merge_requests.rb

@@ -34,6 +34,17 @@ module Emails
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    # rubocop: disable CodeReuse/ActiveRecord
+    def changed_reviewer_of_merge_request_email(recipient_id, merge_request_id, previous_reviewer_ids, updated_by_user_id, reason = nil)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
+      @previous_reviewers = []
+      @previous_reviewers = User.where(id: previous_reviewer_ids) if previous_reviewer_ids.any?
+
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
     def relabeled_merge_request_email(recipient_id, merge_request_id, label_names, updated_by_user_id, reason = nil)
       setup_merge_request_mail(merge_request_id, recipient_id)
 

app/models/merge_request.rb

@@ -121,6 +121,8 @@ class MergeRequest < ApplicationRecord
   # when creating new merge request
   attr_accessor :can_be_created, :compare_commits, :diff_options, :compare
 
+  participant :reviewers
+
   # Keep states definition to be evaluated before the state_machine block to avoid spec failures.
   # If this gets evaluated after, the `merged` and `locked` states which are overrided can be nil.
   def self.available_state_names

app/models/notification_reason.rb

@@ -5,6 +5,7 @@
 class NotificationReason
   OWN_ACTIVITY = 'own_activity'
   ASSIGNED = 'assigned'
+  REVIEW_REQUESTED = 'review_requested'
   MENTIONED = 'mentioned'
   SUBSCRIBED = 'subscribed'
 
@@ -12,6 +13,7 @@ class NotificationReason
   REASON_PRIORITY = [
     OWN_ACTIVITY,
     ASSIGNED,
+    REVIEW_REQUESTED,
     MENTIONED,
     SUBSCRIBED
   ].freeze

app/models/notification_setting.rb

@@ -43,6 +43,7 @@ class NotificationSetting < ApplicationRecord
     :reopen_merge_request,
     :close_merge_request,
     :reassign_merge_request,
+    :change_reviewer_merge_request,
     :merge_merge_request,
     :failed_pipeline,
     :fixed_pipeline,

app/services/issuable_base_service.rb

@@ -365,6 +365,7 @@ class IssuableBaseService < BaseService
       }
     associations[:total_time_spent] = issuable.total_time_spent if issuable.respond_to?(:total_time_spent)
     associations[:description] = issuable.description
+    associations[:reviewers] = issuable.reviewers.to_a if issuable.allows_reviewers?
 
     associations
   end

app/services/merge_requests/update_service.rb

@@ -112,6 +112,7 @@ module MergeRequests
     end
 
     def handle_reviewers_change(merge_request, old_reviewers)
+      notification_service.async.changed_reviewer_of_merge_request(merge_request, current_user, old_reviewers)
       todo_service.reassigned_reviewable(merge_request, current_user, old_reviewers)
     end
 

app/services/notification_recipients/builder/default.rb

@@ -34,6 +34,9 @@ module NotificationRecipients
         when :reassign_merge_request, :reassign_issue
           add_recipients(previous_assignees, :mention, nil)
           add_recipients(target.assignees, :mention, NotificationReason::ASSIGNED)
+        when :change_reviewer_merge_request
+          add_recipients(previous_assignees, :mention, nil)
+          add_recipients(target.reviewers, :mention, NotificationReason::REVIEW_REQUESTED)
         end
 
         add_subscribed_users

app/services/notification_service.rb

@@ -238,6 +238,33 @@ class NotificationService
     end
   end
 
+  # When we change reviewer in a merge_request we should send an email to:
+  #
+  #  * merge_request old reviewers if their notification level is not Disabled
+  #  * merge_request new reviewers if their notification level is not Disabled
+  #  * users with custom level checked with "change reviewer merge request"
+  #
+  def changed_reviewer_of_merge_request(merge_request, current_user, previous_reviewers = [])
+    recipients = NotificationRecipients::BuildService.build_recipients(
+      merge_request,
+      current_user,
+      action: "change_reviewer",
+      previous_assignees: previous_reviewers
+    )
+
+    previous_reviewer_ids = previous_reviewers.map(&:id)
+
+    recipients.each do |recipient|
+      mailer.changed_reviewer_of_merge_request_email(
+        recipient.user.id,
+        merge_request.id,
+        previous_reviewer_ids,
+        current_user.id,
+        recipient.reason
+      ).deliver_later
+    end
+  end
+
   # When we add labels to a merge request we should send an email to:
   #
   #  * watchers of the mr's labels

app/views/notify/changed_reviewer_of_merge_request_email.html.haml

+%p
+  = change_reviewer_notification_text(@merge_request.reviewers, @previous_reviewers, :strong)

app/views/notify/changed_reviewer_of_merge_request_email.text.erb

+<%= change_reviewer_notification_text(@merge_request.reviewers, @previous_reviewers) %>

changelogs/unreleased/add-notification-setting-for-merge-request-reviewers.yml

+---
+title: Add notification setting for merge request reviewers
+merge_request: 41851
+author:
+type: added

db/migrate/20200909083339_add_change_reviewer_merge_request_to_notification_settings.rb

+# frozen_string_literal: true
+
+class AddChangeReviewerMergeRequestToNotificationSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :notification_settings, :change_reviewer_merge_request, :boolean
+  end
+end

db/schema_migrations/20200909083339

+8b2090e953e6205b65555408a88d3da7f6bce28b0baa52d1a43a3a3e8001b7e1
\ No newline at end of file

db/structure.sql

@@ -13727,7 +13727,8 @@ CREATE TABLE notification_settings (
     notification_email character varying,
     fixed_pipeline boolean,
     new_release boolean,
-    moved_project boolean DEFAULT true NOT NULL
+    moved_project boolean DEFAULT true NOT NULL,
+    change_reviewer_merge_request boolean
 );
 
 CREATE SEQUENCE notification_settings_id_seq

ee/spec/models/ee/notification_setting_spec.rb

@@ -24,6 +24,7 @@ RSpec.describe NotificationSetting do
             :reopen_merge_request,
             :close_merge_request,
             :reassign_merge_request,
+            :change_reviewer_merge_request,
             :merge_merge_request,
             :failed_pipeline,
             :fixed_pipeline,
@@ -53,6 +54,7 @@ RSpec.describe NotificationSetting do
             :reopen_merge_request,
             :close_merge_request,
             :reassign_merge_request,
+            :change_reviewer_merge_request,
             :merge_merge_request,
             :failed_pipeline,
             :fixed_pipeline,
@@ -81,6 +83,7 @@ RSpec.describe NotificationSetting do
             :reopen_merge_request,
             :close_merge_request,
             :reassign_merge_request,
+            :change_reviewer_merge_request,
             :merge_merge_request,
             :failed_pipeline,
             :fixed_pipeline,

locale/gitlab.pot

@@ -4621,6 +4621,15 @@ msgstr ""
 msgid "Change your password or recover your current one"
 msgstr ""
 
+msgid "ChangeReviewer|Reviewer changed from %{old} to %{new}"
+msgstr ""
+
+msgid "ChangeReviewer|Reviewer changed to %{new}"
+msgstr ""
+
+msgid "ChangeReviewer|Unassigned"
+msgstr ""
+
 msgid "ChangeTypeActionLabel|Pick into branch"
 msgstr ""
 
@@ -17416,6 +17425,9 @@ msgstr ""
 msgid "Notification settings saved"
 msgstr ""
 
+msgid "NotificationEvent|Change reviewer merge request"
+msgstr ""
+
 msgid "NotificationEvent|Close issue"
 msgstr ""
 

locale/unfound_translations.rb

@@ -16,3 +16,4 @@ N_('NotificationEvent|Merge merge request')
 N_('NotificationEvent|Failed pipeline')
 N_('NotificationEvent|Fixed pipeline')
 N_('NotificationEvent|New release')
+N_('NotificationEvent|Change reviewer merge request')

spec/helpers/emails_helper_spec.rb

@@ -361,4 +361,116 @@ RSpec.describe EmailsHelper do
       end
     end
   end
+
+  describe '#change_reviewer_notification_text' do
+    let(:mary) { build(:user, name: 'Mary') }
+    let(:john) { build(:user, name: 'John') }
+    let(:ted) { build(:user, name: 'Ted') }
+
+    context 'to new reviewers only' do
+      let(:previous_reviewers) { [] }
+      let(:new_reviewers) { [john] }
+
+      context 'with no html tag' do
+        let(:expected_output) do
+          'Reviewer changed to John'
+        end
+
+        it 'returns the expected output' do
+          expect(change_reviewer_notification_text(new_reviewers, previous_reviewers)).to eq(expected_output)
+        end
+      end
+
+      context 'with <strong> tag' do
+        let(:expected_output) do
+          'Reviewer changed to <strong>John</strong>'
+        end
+
+        it 'returns the expected output' do
+          expect(change_reviewer_notification_text(new_reviewers, previous_reviewers, :strong)).to eq(expected_output)
+        end
+      end
+    end
+
+    context 'from previous reviewers to new reviewers' do
+      let(:previous_reviewers) { [john, mary] }
+      let(:new_reviewers) { [ted] }
+
+      context 'with no html tag' do
+        let(:expected_output) do
+          'Reviewer changed from John and Mary to Ted'
+        end
+
+        it 'returns the expected output' do
+          expect(change_reviewer_notification_text(new_reviewers, previous_reviewers)).to eq(expected_output)
+        end
+      end
+
+      context 'with <strong> tag' do
+        let(:expected_output) do
+          'Reviewer changed from <strong>John and Mary</strong> to <strong>Ted</strong>'
+        end
+
+        it 'returns the expected output' do
+          expect(change_reviewer_notification_text(new_reviewers, previous_reviewers, :strong)).to eq(expected_output)
+        end
+      end
+    end
+
+    context 'from previous reviewers to no reviewers' do
+      let(:previous_reviewers) { [john, mary] }
+      let(:new_reviewers) { [] }
+
+      context 'with no html tag' do
+        let(:expected_output) do
+          'Reviewer changed from John and Mary to Unassigned'
+        end
+
+        it 'returns the expected output' do
+          expect(change_reviewer_notification_text(new_reviewers, previous_reviewers)).to eq(expected_output)
+        end
+      end
+
+      context 'with <strong> tag' do
+        let(:expected_output) do
+          'Reviewer changed from <strong>John and Mary</strong> to <strong>Unassigned</strong>'
+        end
+
+        it 'returns the expected output' do
+          expect(change_reviewer_notification_text(new_reviewers, previous_reviewers, :strong)).to eq(expected_output)
+        end
+      end
+    end
+
+    context "with a <script> tag in user's name" do
+      let(:previous_reviewers) { [] }
+      let(:new_reviewers) { [fishy_user] }
+      let(:fishy_user) { build(:user, name: "<script>alert('hi')</script>") }
+
+      let(:expected_output) do
+        'Reviewer changed to <strong>&lt;script&gt;alert(&#39;hi&#39;)&lt;/script&gt;</strong>'
+      end
+
+      it 'escapes the html tag' do
+        expect(change_reviewer_notification_text(new_reviewers, previous_reviewers, :strong)).to eq(expected_output)
+      end
+    end
+
+    context "with url in user's name" do
+      subject(:email_helper) { Object.new.extend(described_class) }
+
+      let(:previous_reviewers) { [] }
+      let(:new_reviewers) { [fishy_user] }
+      let(:fishy_user) { build(:user, name: "example.com") }
+
+      let(:expected_output) do
+        'Reviewer changed to example_com'
+      end
+
+      it "sanitizes user's name" do
+        expect(email_helper).to receive(:sanitize_name).and_call_original
+        expect(email_helper.change_reviewer_notification_text(new_reviewers, previous_reviewers)).to eq(expected_output)
+      end
+    end
+  end
 end

spec/models/merge_request_spec.rb

@@ -4256,24 +4256,6 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
-  describe '#allows_reviewers?' do
-    it 'returns false without merge_request_reviewers feature' do
-      stub_feature_flags(merge_request_reviewers: false)
-
-      merge_request = build_stubbed(:merge_request)
-
-      expect(merge_request.allows_reviewers?).to be(false)
-    end
-
-    it 'returns true with merge_request_reviewers feature' do
-      stub_feature_flags(merge_request_reviewers: true)
-
-      merge_request = build_stubbed(:merge_request)
-
-      expect(merge_request.allows_reviewers?).to be(true)
-    end
-  end
-
   describe '#merge_ref_head' do
     let(:merge_request) { create(:merge_request) }
 
@@ -4299,4 +4281,22 @@ RSpec.describe MergeRequest, factory_default: :keep do
       end
     end
   end
+
+  describe '#allows_reviewers?' do
+    it 'returns false without merge_request_reviewers feature' do
+      stub_feature_flags(merge_request_reviewers: false)
+
+      merge_request = build_stubbed(:merge_request)
+
+      expect(merge_request.allows_reviewers?).to be(false)
+    end
+
+    it 'returns true with merge_request_reviewers feature' do
+      stub_feature_flags(merge_request_reviewers: true)
+
+      merge_request = build_stubbed(:merge_request)
+
+      expect(merge_request.allows_reviewers?).to be(true)
+    end
+  end
 end

spec/models/notification_setting_spec.rb

@@ -175,6 +175,7 @@ RSpec.describe NotificationSetting do
         :reopen_merge_request,
         :close_merge_request,
         :reassign_merge_request,
+        :change_reviewer_merge_request,
         :merge_merge_request,
         :failed_pipeline,
         :success_pipeline,

spec/services/merge_requests/update_service_spec.rb

@@ -52,7 +52,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           title: 'New title',
           description: 'Also please fix',
           assignee_ids: [user.id],
-          reviewer_ids: [user.id],
           state_event: 'close',
           label_ids: [label.id],
           target_branch: 'target',
@@ -76,7 +75,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         expect(@merge_request).to be_valid
         expect(@merge_request.title).to eq('New title')
         expect(@merge_request.assignees).to match_array([user])
-        expect(@merge_request.reviewers).to match_array([user])
+        expect(@merge_request.reviewers).to match_array([])
         expect(@merge_request).to be_closed
         expect(@merge_request.labels.count).to eq(1)
         expect(@merge_request.labels.first.title).to eq(label.name)
@@ -94,6 +93,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
               labels: [],
               mentioned_users: [user2],
               assignees: [user3],
+              reviewers: [],
               milestone: nil,
               total_time_spent: 0,
               description: "FYI #{user2.to_reference}"
@@ -405,15 +405,15 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       context 'when reviewers gets changed' do
-        before do
+        it 'marks pending todo as done' do
           update_merge_request({ reviewer_ids: [user2.id] })
-        end
 
-        it 'marks pending todo as done' do
           expect(pending_todo.reload).to be_done
         end
 
         it 'creates a pending todo for new review request' do
+          update_merge_request({ reviewer_ids: [user2.id] })
+
           attributes = {
             project: project,
             author: user,
@@ -426,6 +426,17 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
 
           expect(Todo.where(attributes).count).to eq 1
         end
+
+        it 'sends email reviewer change notifications to old and new reviewers', :sidekiq_might_not_need_inline do
+          merge_request.reviewers = [user2]
+
+          perform_enqueued_jobs do
+            update_merge_request({ reviewer_ids: [user3.id] })
+          end
+
+          should_email(user2)
+          should_email(user3)
+        end
       end
 
       context 'when the milestone is removed' do

spec/services/notification_service_spec.rb

@@ -150,6 +150,16 @@ RSpec.describe NotificationService, :mailer do
     end
   end
 
+  shared_examples 'participating by reviewer notification' do
+    it 'emails the participant' do
+      issuable.reviewers << participant
+
+      notification_trigger
+
+      should_email(participant)
+    end
+  end
+
   shared_examples_for 'participating notifications' do
     it_behaves_like 'participating by note notification'
     it_behaves_like 'participating by author notification'
@@ -1778,6 +1788,60 @@ RSpec.describe NotificationService, :mailer do
       end
     end
 
+    describe '#changed_reviewer_of_merge_request' do
+      let(:merge_request) { create(:merge_request, author: author, source_project: project, reviewers: [reviewer], description: 'cc @participant') }
+
+      let_it_be(:current_user) { create(:user) }
+      let_it_be(:reviewer) { create(:user) }
+
+      before do
+        update_custom_notification(:change_reviewer_merge_request, @u_guest_custom, resource: project)
+        update_custom_notification(:change_reviewer_merge_request, @u_custom_global)
+      end
+
+      it 'sends emails to relevant users only', :aggregate_failures do
+        notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer])
+
+        merge_request.reviewers.each { |reviewer| should_email(reviewer) }
+        should_email(merge_request.author)
+        should_email(@u_watcher)
+        should_email(@u_participant_mentioned)
+        should_email(@subscriber)
+        should_email(@watcher_and_subscriber)
+        should_email(@u_guest_watcher)
+        should_email(@u_guest_custom)
+        should_email(@u_custom_global)
+        should_not_email(@unsubscriber)
+        should_not_email(@u_participating)
+        should_not_email(@u_disabled)
+        should_not_email(@u_lazy_participant)
+      end
+
+      it 'adds "review requested" reason for new reviewer' do
+        notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer])
+
+        merge_request.reviewers.each do |assignee|
+          email = find_email_for(assignee)
+
+          expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::REVIEW_REQUESTED)
+        end
+      end
+
+      context 'participating notifications with reviewers' do
+        let(:participant) { create(:user, username: 'user-participant') }
+        let(:issuable) { merge_request }
+        let(:notification_trigger) { notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer]) }
+
+        it_behaves_like 'participating notifications'
+        it_behaves_like 'participating by reviewer notification'
+      end
+
+      it_behaves_like 'project emails are disabled' do
+        let(:notification_target)  { merge_request }
+        let(:notification_trigger) { notification.changed_reviewer_of_merge_request(merge_request, current_user, [reviewer]) }
+      end
+    end
+
     describe '#push_to_merge_request' do
       before do
         update_custom_notification(:push_to_merge_request, @u_guest_custom, resource: project)