Merge branch 'create-system-notes-for-merge-request-reviewers' into 'master'

Add system note for review requests

See merge request gitlab-org/gitlab!42589

app/helpers/system_note_helper.rb

@@ -11,6 +11,7 @@ module SystemNoteHelper
     'closed' => 'issue-close',
     'time_tracking' => 'timer',
     'assignee' => 'user',
+    'reviewer' => 'user',
     'title' => 'pencil-square',
     'task' => 'task-done',
     'label' => 'label',

app/models/system_note_metadata.rb

@@ -18,7 +18,7 @@ class SystemNoteMetadata < ApplicationRecord
     commit description merge confidential visible label assignee cross_reference
     designs_added designs_modified designs_removed designs_discussion_added
     title time_tracking branch milestone discussion task moved
-    opened closed merged duplicate locked unlocked outdated
+    opened closed merged duplicate locked unlocked outdated reviewer
     tag due_date pinned_embed cherry_pick health_status approved unapproved
     status alert_issue_added relate unrelate new_alert_added severity
   ].freeze

app/services/merge_requests/base_service.rb

@@ -130,6 +130,11 @@ module MergeRequests
         merge_request, merge_request.project, current_user, old_assignees)
     end
 
+    def create_reviewer_note(merge_request, old_reviewers)
+      SystemNoteService.change_issuable_reviewers(
+        merge_request, merge_request.project, current_user, old_reviewers)
+    end
+
     def create_pipeline_for(merge_request, user)
       MergeRequests::CreatePipelineService.new(project, user).execute(merge_request)
     end

app/services/merge_requests/update_service.rb

@@ -112,6 +112,7 @@ module MergeRequests
     end
 
     def handle_reviewers_change(merge_request, old_reviewers)
+      create_reviewer_note(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/system_note_service.rb

@@ -41,6 +41,10 @@ module SystemNoteService
     ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_assignees(old_assignees)
   end
 
+  def change_issuable_reviewers(issuable, project, author, old_reviewers)
+    ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_reviewers(old_reviewers)
+  end
+
   def relate_issue(noteable, noteable_ref, user)
     ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issue(noteable_ref)
   end

app/services/system_notes/issuables_service.rb

@@ -81,6 +81,32 @@ module SystemNotes
       create_note(NoteSummary.new(noteable, project, author, body, action: 'assignee'))
     end
 
+    # Called when the reviewers of an issuable is changed or removed
+    #
+    # reviewers - Users being requested to review, or nil
+    #
+    # Example Note text:
+    #
+    #   "requested review from @user1 and @user2"
+    #
+    #   "requested review from @user1, @user2 and @user3 and removed review request for @user4 and @user5"
+    #
+    # Returns the created Note object
+    def change_issuable_reviewers(old_reviewers)
+      unassigned_users = old_reviewers - noteable.reviewers
+      added_users = noteable.reviewers - old_reviewers
+      text_parts = []
+
+      Gitlab::I18n.with_default_locale do
+        text_parts << "requested review from #{added_users.map(&:to_reference).to_sentence}" if added_users.any?
+        text_parts << "removed review request for #{unassigned_users.map(&:to_reference).to_sentence}" if unassigned_users.any?
+      end
+
+      body = text_parts.join(' and ')
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'reviewer'))
+    end
+
     # Called when the title of a Noteable is changed
     #
     # old_title - Previous String title

spec/services/merge_requests/update_service_spec.rb

@@ -52,6 +52,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           title: 'New title',
           description: 'Also please fix',
           assignee_ids: [user.id],
+          reviewer_ids: [],
           state_event: 'close',
           label_ids: [label.id],
           target_branch: 'target',
@@ -116,6 +117,35 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
         expect(note.note).to include "assigned to #{user.to_reference} and unassigned #{user3.to_reference}"
       end
 
+      context 'with reviewers' do
+        let(:opts) { { reviewer_ids: [user2.id] } }
+
+        context 'when merge_request_reviewers feature is disabled' do
+          before(:context) do
+            stub_feature_flags(merge_request_reviewers: false)
+          end
+
+          it 'does not create a system note about merge_request review request' do
+            note = find_note('review requested from')
+
+            expect(note).to be_nil
+          end
+        end
+
+        context 'when merge_request_reviewers feature is enabled' do
+          before(:context) do
+            stub_feature_flags(merge_request_reviewers: true)
+          end
+
+          it 'creates system note about merge_request review request' do
+            note = find_note('requested review from')
+
+            expect(note).not_to be_nil
+            expect(note.note).to include "requested review from #{user2.to_reference}"
+          end
+        end
+      end
+
       it 'creates a resource label event' do
         event = merge_request.resource_label_events.last
 

spec/services/system_note_service_spec.rb

@@ -64,6 +64,18 @@ RSpec.describe SystemNoteService do
     end
   end
 
+  describe '.change_issuable_reviewers' do
+    let(:reviewers) { [double, double] }
+
+    it 'calls IssuableService' do
+      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
+        expect(service).to receive(:change_issuable_reviewers).with(reviewers)
+      end
+
+      described_class.change_issuable_reviewers(noteable, project, author, reviewers)
+    end
+  end
+
   describe '.close_after_error_tracking_resolve' do
     it 'calls IssuableService' do
       expect_next_instance_of(::SystemNotes::IssuablesService) do |service|

spec/services/system_notes/issuables_service_spec.rb

@@ -128,6 +128,65 @@ RSpec.describe ::SystemNotes::IssuablesService do
     end
   end
 
+  describe '#change_issuable_reviewers' do
+    subject { service.change_issuable_reviewers([reviewer]) }
+
+    let_it_be(:noteable) { create(:merge_request, :simple, source_project: project) }
+    let_it_be(:reviewer) { create(:user) }
+    let_it_be(:reviewer1) { create(:user) }
+    let_it_be(:reviewer2) { create(:user) }
+    let_it_be(:reviewer3) { create(:user) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'reviewer' }
+    end
+
+    def build_note(old_reviewers, new_reviewers)
+      noteable.reviewers = new_reviewers
+      service.change_issuable_reviewers(old_reviewers).note
+    end
+
+    it 'builds a correct phrase when a reviewer is added to a non-assigned merge request' do
+      expect(build_note([], [reviewer1])).to eq "requested review from @#{reviewer1.username}"
+    end
+
+    it 'builds a correct phrase when reviewer is removed' do
+      expect(build_note([reviewer], [])).to eq "removed review request for @#{reviewer.username}"
+    end
+
+    it 'builds a correct phrase when reviewers changed' do
+      expect(build_note([reviewer1], [reviewer2])).to(
+        eq("requested review from @#{reviewer2.username} and removed review request for @#{reviewer1.username}")
+      )
+    end
+
+    it 'builds a correct phrase when three reviewers removed and one added' do
+      expect(build_note([reviewer, reviewer1, reviewer2], [reviewer3])).to(
+        eq("requested review from @#{reviewer3.username} and removed review request for @#{reviewer.username}, @#{reviewer1.username}, and @#{reviewer2.username}")
+      )
+    end
+
+    it 'builds a correct phrase when one reviewer is changed from a set' do
+      expect(build_note([reviewer, reviewer1], [reviewer, reviewer2])).to(
+        eq("requested review from @#{reviewer2.username} and removed review request for @#{reviewer1.username}")
+      )
+    end
+
+    it 'builds a correct phrase when one reviewer removed from a set' do
+      expect(build_note([reviewer, reviewer1, reviewer2], [reviewer, reviewer1])).to(
+        eq( "removed review request for @#{reviewer2.username}")
+      )
+    end
+
+    it 'builds a correct phrase when the locale is different' do
+      Gitlab::I18n.with_locale('pt-BR') do
+        expect(build_note([reviewer, reviewer1, reviewer2], [reviewer3])).to(
+          eq("requested review from @#{reviewer3.username} and removed review request for @#{reviewer.username}, @#{reviewer1.username}, and @#{reviewer2.username}")
+        )
+      end
+    end
+  end
+
   describe '#change_status' do
     subject { service.change_status(status, source) }