Merge branch 'ph/requestReviewBackend' into 'master'

Added backend for requesting a new review

See merge request gitlab-org/gitlab!52321

app/controllers/autocomplete_controller.rb

@@ -18,7 +18,7 @@ class AutocompleteController < ApplicationController
       .new(params: params, current_user: current_user, project: project, group: group)
       .execute
 
-    render json: UserSerializer.new(params).represent(users, project: project)
+    render json: UserSerializer.new(params.merge({ current_user: current_user })).represent(users, project: project)
   end
 
   def user

app/graphql/mutations/merge_requests/reviewer_rereview.rb

+# frozen_string_literal: true
+
+module Mutations
+  module MergeRequests
+    class ReviewerRereview < Base
+      graphql_name 'MergeRequestReviewerRereview'
+
+      argument :user_id, ::Types::GlobalIDType[::User],
+               loads: Types::UserType,
+               required: true,
+               description: <<~DESC
+                            The user ID for the user that has been requested for a new review.
+               DESC
+
+      def resolve(project_path:, iid:, user:)
+        merge_request = authorized_find!(project_path: project_path, iid: iid)
+
+        result = ::MergeRequests::RequestReviewService.new(merge_request.project, current_user).execute(merge_request, user)
+
+        {
+          merge_request: merge_request,
+          errors: Array(result[:message])
+        }
+      end
+    end
+  end
+end

app/graphql/types/mutation_type.rb

@@ -51,6 +51,7 @@ module Types
     mount_mutation Mutations::MergeRequests::SetSubscription
     mount_mutation Mutations::MergeRequests::SetWip, calls_gitaly: true
     mount_mutation Mutations::MergeRequests::SetAssignees
+    mount_mutation Mutations::MergeRequests::ReviewerRereview
     mount_mutation Mutations::Metrics::Dashboard::Annotations::Create
     mount_mutation Mutations::Metrics::Dashboard::Annotations::Delete
     mount_mutation Mutations::Notes::Create::Note, calls_gitaly: true

app/mailers/emails/merge_requests.rb

@@ -82,6 +82,13 @@ module Emails
       mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
     end
 
+    def request_review_merge_request_email(recipient_id, merge_request_id, updated_by_user_id, reason = nil)
+      setup_merge_request_mail(merge_request_id, recipient_id)
+
+      @updated_by = User.find(updated_by_user_id)
+      mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id, reason))
+    end
+
     def merge_request_status_email(recipient_id, merge_request_id, status, updated_by_user_id, reason = nil)
       setup_merge_request_mail(merge_request_id, recipient_id)
 

app/models/merge_request.rb

@@ -1777,6 +1777,10 @@ class MergeRequest < ApplicationRecord
     true
   end
 
+  def find_reviewer(user)
+    merge_request_reviewers.find_by(user_id: user.id)
+  end
+
   private
 
   def with_rebase_lock

app/models/merge_request_reviewer.rb

 # frozen_string_literal: true
 
 class MergeRequestReviewer < ApplicationRecord
+  enum state: {
+    unreviewed: 0,
+    reviewed: 1
+  }
+
+  validates :state,
+    presence: true,
+    inclusion: { in: MergeRequestReviewer.states.keys }
+
   belongs_to :merge_request
   belongs_to :reviewer, class_name: 'User', foreign_key: :user_id, inverse_of: :merge_request_reviewers
 end

app/serializers/merge_request_sidebar_extras_entity.rb

 # frozen_string_literal: true
 
 class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity
-  expose :assignees do |merge_request|
-    MergeRequestUserEntity.represent(merge_request.assignees, merge_request: merge_request)
+  expose :assignees do |merge_request, options|
+    MergeRequestUserEntity.represent(merge_request.assignees, options.merge(merge_request: merge_request))
   end
 
-  expose :reviewers, if: -> (m) { m.allows_reviewers? } do |merge_request|
-    MergeRequestUserEntity.represent(merge_request.reviewers, merge_request: merge_request)
+  expose :reviewers, if: -> (m) { m.allows_reviewers? } do |merge_request, options|
+    MergeRequestUserEntity.represent(merge_request.reviewers, options.merge(merge_request: merge_request))
   end
 end

app/serializers/merge_request_user_entity.rb

@@ -2,10 +2,21 @@
 
 class MergeRequestUserEntity < ::API::Entities::UserBasic
   include UserStatusTooltip
+  include RequestAwareEntity
 
   expose :can_merge do |reviewer, options|
     options[:merge_request]&.can_be_merged_by?(reviewer)
   end
+
+  expose :can_update_merge_request do |reviewer, options|
+    request.current_user&.can?(:update_merge_request, options[:merge_request])
+  end
+
+  expose :reviewed, if: -> (_, options) { options[:merge_request] && options[:merge_request].allows_reviewers? } do |reviewer, options|
+    reviewer = options[:merge_request].find_reviewer(reviewer)
+
+    reviewer&.reviewed?
+  end
 end
 
 MergeRequestUserEntity.prepend_if_ee('EE::MergeRequestUserEntity')

app/services/draft_notes/publish_service.rb

@@ -38,6 +38,8 @@ module DraftNotes
       end
       draft_notes.delete_all
 
+      set_reviewed
+
       notification_service.async.new_review(review)
       MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
     end
@@ -64,5 +66,9 @@ module DraftNotes
         discussion.unresolve!
       end
     end
+
+    def set_reviewed
+      ::MergeRequests::MarkReviewerReviewedService.new(project, current_user).execute(merge_request)
+    end
   end
 end

app/services/merge_requests/mark_reviewer_reviewed_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class MarkReviewerReviewedService < MergeRequests::BaseService
+    def execute(merge_request)
+      return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request)
+
+      reviewer = merge_request.find_reviewer(current_user)
+
+      if reviewer
+        return error("Failed to update reviewer") unless reviewer.update(state: :reviewed)
+
+        success
+      else
+        error("Reviewer not found")
+      end
+    end
+  end
+end

app/services/merge_requests/request_review_service.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class RequestReviewService < MergeRequests::BaseService
+    def execute(merge_request, user)
+      return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request)
+
+      reviewer = merge_request.find_reviewer(user)
+
+      if reviewer
+        return error("Failed to update reviewer") unless reviewer.update(state: :unreviewed)
+
+        notify_reviewer(merge_request, user)
+
+        success
+      else
+        error("Reviewer not found")
+      end
+    end
+
+    private
+
+    def notify_reviewer(merge_request, reviewer)
+      notification_service.async.review_requested_of_merge_request(merge_request, current_user, reviewer)
+      todo_service.create_request_review_todo(merge_request, current_user, reviewer)
+    end
+  end
+end

app/services/notification_recipients/build_service.rb

@@ -36,5 +36,9 @@ module NotificationRecipients
     def self.build_new_review_recipients(*args)
       ::NotificationRecipients::Builder::NewReview.new(*args).notification_recipients
     end
+
+    def self.build_requested_review_recipients(*args)
+      ::NotificationRecipients::Builder::RequestReview.new(*args).notification_recipients
+    end
   end
 end

app/services/notification_recipients/builder/request_review.rb

+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class RequestReview < Base
+      attr_reader :merge_request, :current_user, :reviewer
+
+      def initialize(merge_request, current_user, reviewer)
+        @merge_request, @current_user, @reviewer = merge_request, current_user, reviewer
+      end
+
+      def target
+        merge_request
+      end
+
+      def build!
+        add_recipients(reviewer, :mention, NotificationReason::REVIEW_REQUESTED)
+      end
+    end
+  end
+end

app/services/notification_service.rb

@@ -265,6 +265,14 @@ class NotificationService
     end
   end
 
+  def review_requested_of_merge_request(merge_request, current_user, reviewer)
+    recipients = NotificationRecipients::BuildService.build_requested_review_recipients(merge_request, current_user, reviewer)
+
+    recipients.each do |recipient|
+      mailer.request_review_merge_request_email(recipient.user.id, merge_request.id, 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/services/todo_service.rb

@@ -212,6 +212,11 @@ class TodoService
     current_user.update_todos_count_cache
   end
 
+  def create_request_review_todo(target, author, reviewers)
+    attributes = attributes_for_todo(target.project, target, author, Todo::REVIEW_REQUESTED)
+    create_todos(reviewers, attributes)
+  end
+
   private
 
   def create_todos(users, attributes)
@@ -266,8 +271,7 @@ class TodoService
   def create_reviewer_todo(target, author, old_reviewers = [])
     if target.reviewers.any?
       reviewers = target.reviewers - old_reviewers
-      attributes = attributes_for_todo(target.project, target, author, Todo::REVIEW_REQUESTED)
-      create_todos(reviewers, attributes)
+      create_request_review_todo(target, author, reviewers)
     end
   end
 

app/views/notify/request_review_merge_request_email.html.haml

+%p
+  #{sanitize_name(@updated_by.name)} requested a new review on #{merge_request_reference_link(@merge_request)}.

app/views/notify/request_review_merge_request_email.text.erb

+<%= sanitize_name(@updated_by.name) %> requested a new review on <%= merge_request_reference_link(@merge_request) %>.

changelogs/unreleased/ph-requestReviewBackend.yml

+---
+title: Added ability to re-request a review from a reviewer
+merge_request: 52321
+author:
+type: added

db/migrate/20210122153259_add_state_to_merge_request_reviewers.rb

+# frozen_string_literal: true
+
+class AddStateToMergeRequestReviewers < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  REVIEW_DEFAULT_STATE = 0
+
+  def change
+    add_column :merge_request_reviewers, :state, :smallint, default: REVIEW_DEFAULT_STATE, null: false
+  end
+end

db/schema_migrations/20210122153259

+4c697cc183a000ee8c18b516e4b1d77d0f8d2d3d7abe11121f2240a60c03216c
\ No newline at end of file

db/structure.sql

@@ -14069,7 +14069,8 @@ CREATE TABLE merge_request_reviewers (
     id bigint NOT NULL,
     user_id bigint NOT NULL,
     merge_request_id bigint NOT NULL,
-    created_at timestamp with time zone NOT NULL
+    created_at timestamp with time zone NOT NULL,
+    state smallint DEFAULT 0 NOT NULL
 );
 
 CREATE SEQUENCE merge_request_reviewers_id_seq

doc/api/graphql/reference/gitlab_schema.graphql

@@ -15106,6 +15106,51 @@ type MergeRequestPermissions {
   updateMergeRequest: Boolean!
 }
 
+"""
+Autogenerated input type of MergeRequestReviewerRereview
+"""
+input MergeRequestReviewerRereviewInput {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  The IID of the merge request to mutate.
+  """
+  iid: String!
+
+  """
+  The project the merge request to mutate is in.
+  """
+  projectPath: ID!
+
+  """
+  The user ID for the user that has been requested for a new review.
+  """
+  userId: UserID!
+}
+
+"""
+Autogenerated return type of MergeRequestReviewerRereview
+"""
+type MergeRequestReviewerRereviewPayload {
+  """
+  A unique identifier for the client performing the mutation.
+  """
+  clientMutationId: String
+
+  """
+  Errors encountered during execution of the mutation.
+  """
+  errors: [String!]!
+
+  """
+  The merge request after mutation.
+  """
+  mergeRequest: MergeRequest
+}
+
 """
 Autogenerated input type of MergeRequestSetAssignees
 """
@@ -15945,6 +15990,7 @@ type Mutation {
   labelCreate(input: LabelCreateInput!): LabelCreatePayload
   markAsSpamSnippet(input: MarkAsSpamSnippetInput!): MarkAsSpamSnippetPayload
   mergeRequestCreate(input: MergeRequestCreateInput!): MergeRequestCreatePayload
+  mergeRequestReviewerRereview(input: MergeRequestReviewerRereviewInput!): MergeRequestReviewerRereviewPayload
   mergeRequestSetAssignees(input: MergeRequestSetAssigneesInput!): MergeRequestSetAssigneesPayload
   mergeRequestSetLabels(input: MergeRequestSetLabelsInput!): MergeRequestSetLabelsPayload
   mergeRequestSetLocked(input: MergeRequestSetLockedInput!): MergeRequestSetLockedPayload

doc/api/graphql/reference/gitlab_schema.json

@@ -41585,6 +41585,136 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "INPUT_OBJECT",
+          "name": "MergeRequestReviewerRereviewInput",
+          "description": "Autogenerated input type of MergeRequestReviewerRereview",
+          "fields": null,
+          "inputFields": [
+            {
+              "name": "projectPath",
+              "description": "The project the merge request to mutate is in.",
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "ID",
+                  "ofType": null
+                }
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "iid",
+              "description": "The IID of the merge request to mutate.",
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "String",
+                  "ofType": null
+                }
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "userId",
+              "description": "The user ID for the user that has been requested for a new review.\n",
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "SCALAR",
+                  "name": "UserID",
+                  "ofType": null
+                }
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "clientMutationId",
+              "description": "A unique identifier for the client performing the mutation.",
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "defaultValue": null
+            }
+          ],
+          "interfaces": null,
+          "enumValues": null,
+          "possibleTypes": null
+        },
+        {
+          "kind": "OBJECT",
+          "name": "MergeRequestReviewerRereviewPayload",
+          "description": "Autogenerated return type of MergeRequestReviewerRereview",
+          "fields": [
+            {
+              "name": "clientMutationId",
+              "description": "A unique identifier for the client performing the mutation.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "errors",
+              "description": "Errors encountered during execution of the mutation.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "NON_NULL",
+                "name": null,
+                "ofType": {
+                  "kind": "LIST",
+                  "name": null,
+                  "ofType": {
+                    "kind": "NON_NULL",
+                    "name": null,
+                    "ofType": {
+                      "kind": "SCALAR",
+                      "name": "String",
+                      "ofType": null
+                    }
+                  }
+                }
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
+            {
+              "name": "mergeRequest",
+              "description": "The merge request after mutation.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "MergeRequest",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            }
+          ],
+          "inputFields": null,
+          "interfaces": [
+
+          ],
+          "enumValues": null,
+          "possibleTypes": null
+        },
         {
           "kind": "INPUT_OBJECT",
           "name": "MergeRequestSetAssigneesInput",
@@ -45874,6 +46004,33 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "mergeRequestReviewerRereview",
+              "description": null,
+              "args": [
+                {
+                  "name": "input",
+                  "description": null,
+                  "type": {
+                    "kind": "NON_NULL",
+                    "name": null,
+                    "ofType": {
+                      "kind": "INPUT_OBJECT",
+                      "name": "MergeRequestReviewerRereviewInput",
+                      "ofType": null
+                    }
+                  },
+                  "defaultValue": null
+                }
+              ],
+              "type": {
+                "kind": "OBJECT",
+                "name": "MergeRequestReviewerRereviewPayload",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "mergeRequestSetAssignees",
               "description": null,

doc/api/graphql/reference/index.md

@@ -2287,6 +2287,16 @@ Check permissions for the current user on a merge request.
 | `revertOnCurrentMergeRequest` | Boolean! | Indicates the user can perform `revert_on_current_merge_request` on this resource |
 | `updateMergeRequest` | Boolean! | Indicates the user can perform `update_merge_request` on this resource |
 
+### MergeRequestReviewerRereviewPayload
+
+Autogenerated return type of MergeRequestReviewerRereview.
+
+| Field | Type | Description |
+| ----- | ---- | ----------- |
+| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
+| `errors` | String! => Array | Errors encountered during execution of the mutation. |
+| `mergeRequest` | MergeRequest | The merge request after mutation. |
+
 ### MergeRequestSetAssigneesPayload
 
 Autogenerated return type of MergeRequestSetAssignees.

ee/spec/serializers/ee/user_serializer_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe UserSerializer do
     create(:approval_project_rule, name: 'Project Rule', project: project, users: [user1], protected_branches: [protected_branch])
   end
 
-  let(:serializer) { described_class.new(options) }
+  let(:serializer) { described_class.new(options.merge({ current_user: user1 })) }
 
   shared_examples 'user without applicable_approval_rules' do
     it 'returns a user without applicable_approval_rules' do

spec/requests/api/graphql/mutations/merge_requests/reviewer_rereview_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Setting assignees of a merge request' do
+  include GraphqlHelpers
+
+  let(:current_user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [user]) }
+  let(:project) { merge_request.project }
+  let(:user) { create(:user) }
+  let(:input) { { user_id: global_id_of(user) } }
+
+  let(:mutation) do
+    variables = {
+      project_path: project.full_path,
+      iid: merge_request.iid.to_s
+    }
+    graphql_mutation(:merge_request_reviewer_rereview, variables.merge(input),
+                     <<-QL.strip_heredoc
+                       clientMutationId
+                       errors
+    QL
+    )
+  end
+
+  def mutation_response
+    graphql_mutation_response(:merge_request_reviewer_rereview)
+  end
+
+  def mutation_errors
+    mutation_response['errors']
+  end
+
+  before do
+    project.add_developer(current_user)
+    project.add_developer(user)
+  end
+
+  it 'returns an error if the user is not allowed to update the merge request' do
+    post_graphql_mutation(mutation, current_user: create(:user))
+
+    expect(graphql_errors).not_to be_empty
+  end
+
+  describe 'reviewer does not exist' do
+    let(:input) { { user_id: global_id_of(create(:user)) } }
+
+    it 'returns an error' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(mutation_errors).not_to be_empty
+    end
+  end
+
+  describe 'reviewer exists' do
+    it 'does not return an error' do
+      post_graphql_mutation(mutation, current_user: current_user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(mutation_errors).to be_empty
+    end
+  end
+end

spec/serializers/user_serializer_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe UserSerializer do
   context 'serializer with merge request context' do
     let(:merge_request) { create(:merge_request) }
     let(:project) { merge_request.project }
-    let(:serializer) { described_class.new(merge_request_iid: merge_request.iid) }
+    let(:serializer) { described_class.new(current_user: user1, merge_request_iid: merge_request.iid) }
 
     before do
       allow(project).to(

spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::MarkReviewerReviewedService do
+  let(:current_user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [current_user]) }
+  let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) }
+  let(:project) { merge_request.project }
+  let(:service) { described_class.new(project, current_user) }
+  let(:result) { service.execute(merge_request) }
+
+  before do
+    project.add_developer(current_user)
+  end
+
+  describe '#execute' do
+    describe 'invalid permissions' do
+      let(:service) { described_class.new(project, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer does not exist' do
+      let(:service) { described_class.new(project, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer exists' do
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates reviewers state' do
+        expect(result[:status]).to eq :success
+        expect(reviewer.state).to eq 'reviewed'
+      end
+    end
+  end
+end

spec/services/merge_requests/request_review_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::RequestReviewService do
+  let(:current_user) { create(:user) }
+  let(:user) { create(:user) }
+  let(:merge_request) { create(:merge_request, reviewers: [user]) }
+  let(:reviewer) { merge_request.find_reviewer(user) }
+  let(:project) { merge_request.project }
+  let(:service) { described_class.new(project, current_user) }
+  let(:result) { service.execute(merge_request, user) }
+  let(:todo_service) { spy('todo service') }
+  let(:notification_service) { spy('notification service') }
+
+  before do
+    allow(NotificationService).to receive(:new) { notification_service }
+    allow(service).to receive(:todo_service).and_return(todo_service)
+    allow(service).to receive(:notification_service).and_return(notification_service)
+
+    reviewer.update!(state: MergeRequestReviewer.states[:reviewed])
+
+    project.add_developer(current_user)
+    project.add_developer(user)
+  end
+
+  describe '#execute' do
+    describe 'invalid permissions' do
+      let(:service) { described_class.new(project, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer does not exist' do
+      let(:result) { service.execute(merge_request, create(:user)) }
+
+      it 'returns an error' do
+        expect(result[:status]).to eq :error
+      end
+    end
+
+    describe 'reviewer exists' do
+      it 'returns success' do
+        expect(result[:status]).to eq :success
+      end
+
+      it 'updates reviewers state' do
+        service.execute(merge_request, user)
+        reviewer.reload
+
+        expect(reviewer.state).to eq 'unreviewed'
+      end
+
+      it 'sends email to reviewer' do
+        expect(notification_service).to receive_message_chain(:async, :review_requested_of_merge_request).with(merge_request, current_user, user)
+
+        service.execute(merge_request, user)
+      end
+
+      it 'creates a new todo for the reviewer' do
+        expect(todo_service).to receive(:create_request_review_todo).with(merge_request, current_user, user)
+
+        service.execute(merge_request, user)
+      end
+    end
+  end
+end

spec/services/notification_recipients/build_service_spec.rb

@@ -110,4 +110,28 @@ RSpec.describe NotificationRecipients::BuildService do
       end
     end
   end
+
+  describe '#build_requested_review_recipients' do
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    before do
+      merge_request.reviewers.push(assignee)
+    end
+
+    shared_examples 'no N+1 queries' do
+      it 'avoids N+1 queries', :request_store do
+        create_user
+
+        service.build_requested_review_recipients(note)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          service.build_requested_review_recipients(note)
+        end
+
+        create_user
+
+        expect { service.build_requested_review_recipients(note) }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
 end

spec/services/notification_service_spec.rb

@@ -2177,6 +2177,46 @@ RSpec.describe NotificationService, :mailer do
         let(:notification_trigger) { notification.merge_when_pipeline_succeeds(merge_request, @u_disabled) }
       end
     end
+
+    describe '#review_requested_of_merge_request' do
+      let(:merge_request) { create(:merge_request, author: author, source_project: project, reviewers: [reviewer]) }
+
+      let_it_be(:current_user) { create(:user) }
+      let_it_be(:reviewer) { create(:user) }
+
+      it 'sends email to reviewer', :aggregate_failures do
+        notification.review_requested_of_merge_request(merge_request, current_user, reviewer)
+
+        merge_request.reviewers.each { |reviewer| should_email(reviewer) }
+        should_not_email(merge_request.author)
+        should_not_email(@u_watcher)
+        should_not_email(@u_participant_mentioned)
+        should_not_email(@subscriber)
+        should_not_email(@watcher_and_subscriber)
+        should_not_email(@u_guest_watcher)
+        should_not_email(@u_guest_custom)
+        should_not_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.review_requested_of_merge_request(merge_request, current_user, [reviewer])
+
+        merge_request.reviewers.each do |reviewer|
+          email = find_email_for(reviewer)
+
+          expect(email).to have_header('X-GitLab-NotificationReason', NotificationReason::REVIEW_REQUESTED)
+        end
+      end
+
+      it_behaves_like 'project emails are disabled' do
+        let(:notification_target)  { merge_request }
+        let(:notification_trigger) { notification.review_requested_of_merge_request(merge_request, current_user, reviewer) }
+      end
+    end
   end
 
   describe 'Projects', :deliver_mails_inline do

spec/services/todo_service_spec.rb

@@ -1193,6 +1193,17 @@ RSpec.describe TodoService do
     end
   end
 
+  describe '#create_request_review_todo' do
+    let(:target) { create(:merge_request, author: author, source_project: project) }
+    let(:reviewer) { create(:user) }
+
+    it 'creates a todo for reviewer' do
+      service.create_request_review_todo(target, author, reviewer)
+
+      should_create_todo(user: reviewer, target: target, action: Todo::REVIEW_REQUESTED)
+    end
+  end
+
   def should_create_todo(attributes = {})
     attributes.reverse_merge!(
       project: project,