Merge branch '4326-one-notification-per-code-review' into 'master'

Sends only one notification per batch review

Closes #4326

See merge request gitlab-org/gitlab-ee!8442

app/services/notification_recipient_service.rb

@@ -390,3 +390,5 @@ module NotificationRecipientService
     end
   end
 end
+
+NotificationRecipientService.prepend(EE::NotificationRecipientService)

app/workers/new_note_worker.rb

@@ -23,3 +23,5 @@ class NewNoteWorker
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end
+
+NewNoteWorker.prepend(EE::NewNoteWorker)

db/schema.rb

@@ -1878,6 +1878,7 @@ ActiveRecord::Schema.define(version: 20181206121340) do
     t.integer "cached_markdown_version"
     t.text "change_position"
     t.boolean "resolved_by_push"
+    t.bigint "review_id"
     t.index ["author_id"], name: "index_notes_on_author_id", using: :btree
     t.index ["commit_id"], name: "index_notes_on_commit_id", using: :btree
     t.index ["created_at"], name: "index_notes_on_created_at", using: :btree
@@ -1887,6 +1888,7 @@ ActiveRecord::Schema.define(version: 20181206121340) do
     t.index ["noteable_id", "noteable_type"], name: "index_notes_on_noteable_id_and_noteable_type", using: :btree
     t.index ["noteable_type"], name: "index_notes_on_noteable_type", using: :btree
     t.index ["project_id", "noteable_type"], name: "index_notes_on_project_id_and_noteable_type", using: :btree
+    t.index ["review_id"], name: "index_notes_on_review_id", using: :btree
   end
 
   create_table "notification_settings", force: :cascade do |t|
@@ -2556,6 +2558,16 @@ ActiveRecord::Schema.define(version: 20181206121340) do
     t.index ["user_id"], name: "index_resource_label_events_on_user_id", using: :btree
   end
 
+  create_table "reviews", id: :bigserial, force: :cascade do |t|
+    t.integer "author_id"
+    t.integer "merge_request_id", null: false
+    t.integer "project_id", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.index ["author_id"], name: "index_reviews_on_author_id", using: :btree
+    t.index ["merge_request_id"], name: "index_reviews_on_merge_request_id", using: :btree
+    t.index ["project_id"], name: "index_reviews_on_project_id", using: :btree
+  end
+
   create_table "routes", force: :cascade do |t|
     t.integer "source_id", null: false
     t.string "source_type", null: false
@@ -3296,6 +3308,7 @@ ActiveRecord::Schema.define(version: 20181206121340) do
   add_foreign_key "namespaces", "projects", column: "file_template_project_id", name: "fk_319256d87a", on_delete: :nullify
   add_foreign_key "note_diff_files", "notes", column: "diff_note_id", on_delete: :cascade
   add_foreign_key "notes", "projects", name: "fk_99e097b079", on_delete: :cascade
+  add_foreign_key "notes", "reviews", name: "fk_2e82291620", on_delete: :nullify
   add_foreign_key "notification_settings", "users", name: "fk_0c95e91db7", on_delete: :cascade
   add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
   add_foreign_key "operations_feature_flags", "projects", on_delete: :cascade
@@ -3360,6 +3373,9 @@ ActiveRecord::Schema.define(version: 20181206121340) do
   add_foreign_key "resource_label_events", "labels", on_delete: :nullify
   add_foreign_key "resource_label_events", "merge_requests", on_delete: :cascade
   add_foreign_key "resource_label_events", "users", on_delete: :nullify
+  add_foreign_key "reviews", "merge_requests", on_delete: :cascade
+  add_foreign_key "reviews", "projects", on_delete: :cascade
+  add_foreign_key "reviews", "users", column: "author_id", on_delete: :nullify
   add_foreign_key "saml_providers", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade
   add_foreign_key "slack_integrations", "services", on_delete: :cascade

doc/user/discussions/index.md

@@ -337,6 +337,12 @@ bottom of the screen with two buttons:
 Alternatively, every pending comment has a button to finish the entire review.
 
 ![Review submission](img/review_preview.png)
+
+Submitting the review will send a single email to every notifiable user of the
+merge request with all the comments associated to it.
+
+Replying to this email will, consequentially, create a new comment on the associated merge request.
+
 ## Filtering notes
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/26723) in GitLab 11.5. 

ee/app/controllers/projects/merge_requests/drafts_controller.rb

@@ -41,9 +41,13 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
   end
 
   def publish
-    DraftNotes::PublishService.new(merge_request, current_user).execute(draft_note(allow_nil: true))
+    result = DraftNotes::PublishService.new(merge_request, current_user).execute(draft_note(allow_nil: true))
 
-    head :ok
+    if result[:status] == :success
+      head :ok
+    else
+      render json: { message: result[:message] }, status: result[:status]
+    end
   end
 
   def discard

ee/app/mailers/ee/notify.rb

@@ -13,6 +13,7 @@ module EE
       include ::Emails::CsvExport
       include ::Emails::ServiceDesk
       include ::Emails::Epics
+      include ::Emails::Reviews
     end
 
     attr_reader :group

ee/app/mailers/emails/reviews.rb

+# frozen_string_literal: true
+
+module Emails
+  module Reviews
+    def new_review_email(recipient_id, review_id)
+      setup_review_email(review_id, recipient_id)
+
+      mail_answer_thread(@merge_request, review_thread_options(recipient_id))
+    end
+
+    private
+
+    def review_thread_options(recipient_id)
+      {
+        from: sender(@author.id),
+        to: recipient(recipient_id),
+        subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})")
+      }
+    end
+
+    def setup_review_email(review_id, recipient_id)
+      review = Review.find_by_id(review_id)
+
+      @notes = review.notes
+      @author = review.author
+      @author_name = review.author_name
+      @project = review.project
+      @merge_request = review.merge_request
+      @target_url = project_merge_request_url(@project, @merge_request)
+      @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key)
+    end
+  end
+end

ee/app/models/draft_note.rb

@@ -13,6 +13,8 @@ class DraftNote < ActiveRecord::Base
   # Text with quick actions filtered out
   attr_accessor :rendered_note
 
+  attr_accessor :review
+
   belongs_to :author, class_name: 'User'
   belongs_to :merge_request
 
@@ -91,6 +93,7 @@ class DraftNote < ActiveRecord::Base
     attrs.concat(DIFF_ATTRS) if on_diff?
     params = slice(*attrs)
     params[:in_reply_to_discussion_id] = discussion_id if discussion_id.present?
+    params[:review_id] = review.id if review.present?
 
     params
   end

ee/app/models/ee/merge_request.rb

@@ -11,6 +11,7 @@ module EE
     prepended do
       include Elastic::MergeRequestsSearch
 
+      has_many :reviews, inverse_of: :merge_request
       has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent

ee/app/models/ee/note.rb

@@ -9,6 +9,8 @@ module EE
       include ::ObjectStorage::BackgroundMove
       include Elastic::NotesSearch
 
+      belongs_to :review, inverse_of: :notes
+
       scope :searchable, -> { where(system: false) }
     end
 

ee/app/models/ee/project.rb

@@ -41,6 +41,7 @@ module EE
       has_one :gitlab_slack_application_service
       has_one :tracing_setting, class_name: 'ProjectTracingSetting'
 
+      has_many :reviews, inverse_of: :project
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :audit_events, as: :entity

ee/app/models/ee/user.rb

@@ -27,6 +27,7 @@ module EE
       delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
                to: :namespace
 
+      has_many :reviews,                  foreign_key: :author_id, inverse_of: :author
       has_many :epics,                    foreign_key: :author_id
       has_many :assigned_epics,           foreign_key: :assignee_id, class_name: "Epic"
       has_many :path_locks,               dependent: :destroy # rubocop: disable Cop/ActiveRecordDependent

ee/app/models/review.rb

+# frozen_string_literal: true
+
+class Review < ApplicationRecord
+  include Participable
+  include Mentionable
+
+  belongs_to :author, class_name: 'User', foreign_key: :author_id, inverse_of: :reviews
+  belongs_to :merge_request, inverse_of: :reviews
+  belongs_to :project, inverse_of: :reviews
+
+  has_many :notes, -> { order(:id) }, inverse_of: :review
+
+  delegate :name, to: :author, prefix: true
+
+  participant :author
+
+  def all_references(current_user = nil, extractor: nil)
+    ext = super
+
+    notes.each do |note|
+      note.all_references(current_user, extractor: ext)
+    end
+
+    ext
+  end
+end

ee/app/services/draft_notes/base_service.rb

 # frozen_string_literal: true
 
 module DraftNotes
-  class BaseService
+  class BaseService < ::BaseService
     attr_accessor :merge_request, :current_user, :params
 
     def initialize(merge_request, current_user, params = nil)

ee/app/services/draft_notes/publish_service.rb

@@ -8,6 +8,11 @@ module DraftNotes
       else
         publish_draft_notes
       end
+
+      success
+    rescue ActiveRecord::RecordInvalid => e
+      message = "Unable to save #{e.record.class.name}: #{e.record.errors.full_messages.join(", ")} "
+      error(message)
     end
 
     private
@@ -20,7 +25,21 @@ module DraftNotes
     end
 
     def publish_draft_notes
-      draft_notes.each(&method(:create_note_from_draft))
+      return if draft_notes.empty?
+
+      if Feature.enabled?(:batch_review_notification, project)
+        review = Review.create!(author: current_user, merge_request: merge_request, project: project)
+
+        draft_notes.map do |draft_note|
+          draft_note.review = review
+          create_note_from_draft(draft_note)
+        end
+
+        notification_service.async.new_review(review)
+      else
+        draft_notes.each(&method(:create_note_from_draft))
+      end
+
       draft_notes.delete_all
 
       MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
@@ -33,6 +52,8 @@ module DraftNotes
 
       note = Notes::CreateService.new(draft.project, draft.author, draft.publish_params).execute
       set_discussion_resolve_status(note, draft)
+
+      note
     end
 
     def set_discussion_resolve_status(note, draft_note)

ee/app/services/ee/notification_recipient_service.rb

+# frozen_string_literal: true
+
+module EE
+  module NotificationRecipientService
+    extend ActiveSupport::Concern
+
+    class_methods do
+      def build_new_review_recipients(*args)
+        NotificationRecipientService::Builder::NewReview.new(*args).notification_recipients
+      end
+    end
+
+    prepended do
+      module Builder
+        class NewReview < ::NotificationRecipientService::Builder::Base
+          attr_reader :review
+          def initialize(review)
+            @review = review
+          end
+
+          def target
+            review.merge_request
+          end
+
+          def project
+            review.project
+          end
+
+          def group
+            project.group
+          end
+
+          def build!
+            add_participants(review.author)
+            add_mentions(review.author, target: review)
+            add_project_watchers
+            add_custom_notifications
+            add_subscribed_users
+          end
+
+          # A new review is a batch of new notes
+          # therefore new_note subscribers should also
+          # receive incoming new reviews
+          def custom_action
+            :new_note
+          end
+
+          def acting_user
+            review.author
+          end
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/notification_service.rb

@@ -6,6 +6,11 @@ module EE
   module NotificationService
     extend ::Gitlab::Utils::Override
 
+    # Notify users on new review in system
+    def new_review(review)
+      send_new_review_notification(review)
+    end
+
     # When we add approvers to a merge request we should send an email to:
     #
     #  * the new approvers
@@ -70,6 +75,14 @@ module EE
 
     private
 
+    def send_new_review_notification(review)
+      recipients = ::NotificationRecipientService.build_new_review_recipients(review)
+
+      recipients.each do |recipient|
+        mailer.new_review_email(recipient.user.id, review.id).deliver_later
+      end
+    end
+
     def add_mr_approvers_email(merge_request, approvers, current_user)
       approvers.each do |approver|
         mailer.add_merge_request_approver_email(approver.id, merge_request.id, current_user.id).deliver_later
@@ -77,7 +90,7 @@ module EE
     end
 
     def approve_mr_email(merge_request, project, current_user)
-      recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: 'approve')
+      recipients = ::NotificationRecipientService.build_recipients(merge_request, current_user, action: 'approve')
 
       recipients.each do |recipient|
         mailer.approved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
@@ -85,7 +98,7 @@ module EE
     end
 
     def unapprove_mr_email(merge_request, project, current_user)
-      recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: 'unapprove')
+      recipients = ::NotificationRecipientService.build_recipients(merge_request, current_user, action: 'unapprove')
 
       recipients.each do |recipient|
         mailer.unapproved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later
@@ -110,7 +123,7 @@ module EE
     def epic_status_change_email(target, current_user, status)
       action = status == 'reopened' ? 'reopen' : 'close'
 
-      recipients = NotificationRecipientService.build_recipients(
+      recipients = ::NotificationRecipientService.build_recipients(
         target,
         current_user,
         action: action

ee/app/services/ee/users/migrate_to_ghost_user_service.rb

@@ -8,6 +8,7 @@ module EE
       def migrate_records
         migrate_epics
         migrate_vulnerabilities_feedback
+        migrate_reviews
         super
       end
 
@@ -21,6 +22,10 @@ module EE
       def migrate_vulnerabilities_feedback
         user.vulnerability_feedback.update_all(author_id: ghost_user.id)
       end
+
+      def migrate_reviews
+        user.reviews.update_all(author_id: ghost_user.id)
+      end
     end
   end
 end

ee/app/views/notify/new_review_email.html.haml

+%table{ border: "0", cellpadding:"0", cellspacing: "0", style: "width:100%;margin:0 auto;border-collapse:separate;border-spacing:0;" }
+  %tbody
+    %tr
+      %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;background-color:#ffffff;text-align:left;overflow:hidden;" }
+        %table{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;border-collapse:separate;border-spacing:0;" }
+          %tbody
+            %tr
+              %td{ style: "color:#333333;border-bottom:1px solid #ededed;font-size:15px;font-weight:bold;line-height:1.4;padding: 20px 0;" }
+                Merge request
+                = link_to(@merge_request.to_reference(@project), project_merge_request_url(@project, @merge_request))
+                was reviewed by
+                = link_to(@author_name, user_url(@author))
+            %tr
+              %td{ style: "overflow:hidden;font-size:14px;line-height:1.4;display:grid;" }
+                - @notes.each do |note|
+                  - target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{note.id}")
+                  = render 'note_email', note: note, diff_limit: 3, target_url: target_url, note_style: "border-bottom:1px solid #ededed;"

ee/app/views/notify/new_review_email.text.erb

+<%= "Merge request #{merge_request_url(@merge_request)} was reviewed by #{@author_name}" %>
+
+--
+<% @notes.each_with_index do |note, index| %>
+  <%= render 'note_email', note: note, diff_limit: 3 %>
+
+  <% if index != @notes.length-1 %>
+--
+  <% end %>
+<% end %>

ee/app/workers/ee/new_note_worker.rb

+# frozen_string_literal: true
+
+module EE
+  module NewNoteWorker
+    extend ActiveSupport::Concern
+
+    private
+
+    # If a note belongs to a review
+    # We do not want to send a standalone
+    # notification
+    def skip_notification?(note)
+      note.review.present?
+    end
+  end
+end

ee/db/migrate/20181127130125_create_reviews.rb

+# frozen_string_literal: true
+
+class CreateReviews < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def up
+    create_table :reviews, id: :bigserial do |t|
+      t.references :author, index: true, references: :users
+      t.references :merge_request, index: true, null: false, foreign_key: { on_delete: :cascade }
+      t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade }
+      t.datetime_with_timezone :created_at, null: false
+    end
+
+    add_foreign_key :reviews, :users, column: :author_id, on_delete: :nullify # rubocop:disable Migration/AddConcurrentForeignKey
+  end
+
+  def down
+    remove_foreign_key :reviews, column: :author_id
+
+    drop_table :reviews
+  end
+end

ee/db/migrate/20181127133629_add_review_id_to_notes.rb

+# frozen_string_literal: true
+
+class AddReviewIdToNotes < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def change
+    add_column :notes, :review_id, :bigint
+  end
+end

ee/db/migrate/20181205093951_add_review_foreign_key_to_notes.rb

+# frozen_string_literal: true
+
+class AddReviewForeignKeyToNotes < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key(:notes, :reviews, column: :review_id, on_delete: :nullify)
+    add_concurrent_index :notes, :review_id
+  end
+
+  def down
+    remove_foreign_key :notes, column: :review_id
+    remove_concurrent_index(:notes, :review_id)
+  end
+end

ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb

@@ -193,6 +193,22 @@ describe Projects::MergeRequests::DraftsController do
       end
     end
 
+    context 'when PublishService errors' do
+      it 'returns message and 500 response' do
+        create(:draft_note, merge_request: merge_request, author: user)
+        error_message = "Something went wrong"
+
+        expect_next_instance_of(DraftNotes::PublishService) do |service|
+          allow(service).to receive(:execute).and_return({ message: error_message, status: :error })
+        end
+
+        post :publish, params
+
+        expect(response).to have_gitlab_http_status(:error)
+        expect(json_response["message"]).to include(error_message)
+      end
+    end
+
     it 'publishes draft notes with position' do
       diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs)
 

ee/spec/factories/reviews.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :review do
+    merge_request
+    association :project, :repository
+    author factory: :user
+  end
+end

ee/spec/lib/gitlab/import_export/all_models.yml

@@ -5,6 +5,7 @@ issues:
 milestone:
 - boards
 merge_requests:
+- reviews
 - approvals
 - approvers
 - approver_groups
@@ -69,6 +70,7 @@ project:
 - packages
 - tracing_setting
 - webide_pipelines
+- reviews
 prometheus_metrics:
 - project
 - prometheus_alerts
@@ -82,3 +84,10 @@ epic_issues:
 - epic
 tracing_setting:
 - project
+notes:
+- review
+reviews:
+- project
+- merge_request
+- author
+- notes

ee/spec/lib/gitlab/import_export/safe_model_attributes.yml

 ---
 ProjectTracingSetting:
 - external_url
+Note:
+- review_id

ee/spec/mailers/notify_spec.rb

@@ -227,6 +227,47 @@ describe Notify do
     end
   end
 
+  describe 'merge request reviews' do
+    let(:review) { create(:review, project: project, merge_request: merge_request) }
+    let(:notes) { create_list(:notes, 3, review: review, project: project, author: review.author, noteable: merge_request) }
+
+    subject { described_class.new_review_email(recipient.id, review.id) }
+
+    it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+      let(:model) { review.merge_request }
+    end
+    it_behaves_like 'it should show Gmail Actions View Merge request link'
+    it_behaves_like 'an unsubscribeable thread'
+
+    it 'is sent to the given recipient as the author' do
+      sender = subject.header[:from].addrs[0]
+
+      aggregate_failures do
+        expect(sender.display_name).to eq(review.author_name)
+        expect(sender.address).to eq(gitlab_sender)
+        expect(subject).to deliver_to(recipient.notification_email)
+      end
+    end
+
+    it 'contains the message from the notes of the review' do
+      review.notes.each do |note|
+        is_expected.to have_body_text note.note
+      end
+    end
+
+    it 'contains review author name' do
+      is_expected.to have_body_text review.author_name
+    end
+
+    it 'has the correct subject and body' do
+      aggregate_failures do
+        is_expected.to have_subject "Re: #{project.name} | #{merge_request.title} (#{merge_request.to_reference})"
+
+        is_expected.to have_body_text project_merge_request_path(project, merge_request)
+      end
+    end
+  end
+
   describe 'mirror was hard failed' do
     let(:project) { create(:project, :mirror, :import_hard_failed) }
 

ee/spec/models/ee/note_spec.rb

@@ -3,6 +3,10 @@ require 'spec_helper'
 describe Note do
   include ::EE::GeoHelpers
 
+  describe 'associations' do
+    it { is_expected.to belong_to(:review).inverse_of(:notes) }
+  end
+
   context 'object storage with background upload' do
     before do
       stub_uploads_object_storage(AttachmentUploader, background_upload: true)

ee/spec/models/ee/user_spec.rb

@@ -4,6 +4,7 @@ describe EE::User do
   describe 'associations' do
     subject { build(:user) }
 
+    it { is_expected.to have_many(:reviews) }
     it { is_expected.to have_many(:vulnerability_feedback) }
   end
 

ee/spec/models/merge_request_spec.rb

@@ -8,6 +8,7 @@ describe MergeRequest do
   subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
 
   describe 'associations' do
+    it { is_expected.to have_many(:reviews).inverse_of(:merge_request) }
     it { is_expected.to have_many(:approvals).dependent(:delete_all) }
     it { is_expected.to have_many(:approvers).dependent(:delete_all) }
     it { is_expected.to have_many(:approver_groups).dependent(:delete_all) }

ee/spec/models/project_spec.rb

@@ -17,6 +17,7 @@ describe Project do
     it { is_expected.to have_one(:import_state).class_name('ProjectImportState') }
     it { is_expected.to have_one(:repository_state).class_name('ProjectRepositoryState').inverse_of(:project) }
 
+    it { is_expected.to have_many(:reviews).inverse_of(:project) }
     it { is_expected.to have_many(:path_locks) }
     it { is_expected.to have_many(:vulnerability_feedback) }
     it { is_expected.to have_many(:sourced_pipelines) }

ee/spec/models/review_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Review do
+  describe 'associations' do
+    it { is_expected.to belong_to(:author).class_name('User').with_foreign_key(:author_id).inverse_of(:reviews) }
+    it { is_expected.to belong_to(:merge_request).inverse_of(:reviews).touch(false) }
+    it { is_expected.to belong_to(:project).inverse_of(:reviews) }
+
+    it { is_expected.to have_many(:notes).order(:id).inverse_of(:review) }
+  end
+
+  describe 'modules' do
+    it { is_expected.to include_module(Participable) }
+    it { is_expected.to include_module(Mentionable) }
+  end
+
+  describe '#all_references' do
+    it 'returns an extractor with the correct referenced users' do
+      user1 = create(:user, username: "foo")
+      user2 = create(:user, username: "bar")
+      review = create(:review)
+      project = review.project
+      author = review.author
+
+      create(:note, review: review, project: project, author: author, note: "cc @foo @non_existent")
+      create(:note, review: review, project: project, author: author, note: "cc @bar")
+
+      expect(review.all_references(author).users).to match_array([user1, user2])
+    end
+  end
+
+  describe '#participants' do
+    it 'includes the review author' do
+      project = create(:project, :public)
+      merge_request = create(:merge_request, source_project: project)
+      review = create(:review, project: project, merge_request: merge_request)
+      create(:note, review: review, noteable: merge_request, project: project, author: review.author)
+
+      expect(review.participants).to include(review.author)
+    end
+  end
+end

ee/spec/services/draft_notes/publish_service_spec.rb

@@ -10,18 +10,84 @@ describe DraftNotes::PublishService do
     DraftNotes::PublishService.new(merge_request, user).execute(draft)
   end
 
-  it 'publishes a single draft note' do
-    drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user)
+  context 'single draft note' do
+    let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user) }
 
-    expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
-    expect(DraftNote.count).to eq(1)
+    it 'publishes' do
+      expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1)
+      expect(DraftNote.count).to eq(1)
+    end
+
+    it 'does not skip notification' do
+      expect(Notes::CreateService).to receive(:new).with(project, user, drafts.first.publish_params).and_call_original
+      expect_next_instance_of(NotificationService) do |notification_service|
+        expect(notification_service).to receive(:new_note)
+      end
+
+      result = publish(draft: drafts.first)
+
+      expect(result[:status]).to eq(:success)
+    end
   end
 
-  it 'publishes all draft notes for a user in a merge request' do
-    create_list(:draft_note, 2, merge_request: merge_request, author: user)
+  context 'multiple draft notes' do
+    before do
+      create_list(:draft_note, 2, merge_request: merge_request, author: user)
+    end
 
-    expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2)
-    expect(DraftNote.count).to eq(0)
+    context 'when review fails to create' do
+      before do
+        expect_next_instance_of(Review) do |review|
+          allow(review).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(review))
+        end
+      end
+
+      it 'does not publish any draft note' do
+        expect { publish }.not_to change { DraftNote.count }
+      end
+
+      it 'returns an error' do
+        result = publish
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to match(/Unable to save Review/)
+      end
+    end
+
+    it 'returns success' do
+      result = publish
+
+      expect(result[:status]).to eq(:success)
+    end
+
+    it 'publishes all draft notes for a user in a merge request' do
+      expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2).and change { Review.count }.by(1)
+      expect(DraftNote.count).to eq(0)
+    end
+
+    context 'when batch_review_notification feature is enabled' do
+      it 'sends batch notification' do
+        expect_next_instance_of(NotificationService) do |notification_service|
+          expect(notification_service).to receive_message_chain(:async, :new_review).with(kind_of(Review))
+        end
+
+        publish
+      end
+    end
+
+    context 'when batch_review_notification feature is disabled' do
+      it 'send a notification for each note' do
+        stub_feature_flags(batch_review_notification: false)
+
+        2.times do
+          expect_next_instance_of(NotificationService) do |notification_service|
+            expect(notification_service).to receive(:new_note).with(kind_of(Note))
+          end
+        end
+
+        publish
+      end
+    end
   end
 
   it 'only publishes the draft notes belonging to the current user' do
@@ -54,8 +120,8 @@ describe DraftNotes::PublishService do
   end
 
   context 'with drafts that resolve discussions' do
-    let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
-    let(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) }
+    let!(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+    let!(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) }
 
     it 'resolves the discussion' do
       publish(draft: draft_note)

ee/spec/services/ee/notification_recipient_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe NotificationRecipientService do
+  subject(:service) { described_class }
+
+  let(:project) { create(:project, :public) }
+  let(:other_projects) { create_list(:project, 5, :public) }
+
+  describe '#build_new_review_recipients' do
+    let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+    let(:review) { create(:review, merge_request: merge_request, project: project, author: merge_request.author) }
+    let(:notes) { create_list(:note_on_merge_request, 3, review: review, noteable: review.merge_request, project: review.project) }
+
+    shared_examples 'no N+1 queries' do
+      it 'avoids N+1 queries', :request_store do
+        create_user
+
+        service.build_new_review_recipients(review)
+
+        control_count = ActiveRecord::QueryRecorder.new do
+          service.build_new_review_recipients(review)
+        end
+
+        create_user
+
+        expect { service.build_new_review_recipients(review) }.not_to exceed_query_limit(control_count)
+      end
+    end
+
+    context 'when there are multiple watchers' do
+      def create_user
+        watcher = create(:user)
+        create(:notification_setting, source: project, user: watcher, level: :watch)
+
+        other_projects.each do |other_project|
+          create(:notification_setting, source: other_project, user: watcher, level: :watch)
+        end
+      end
+
+      include_examples 'no N+1 queries'
+    end
+
+    context 'when there are multiple subscribers' do
+      def create_user
+        subscriber = create(:user)
+        merge_request.subscriptions.create(user: subscriber, project: project, subscribed: true)
+      end
+
+      include_examples 'no N+1 queries'
+
+      context 'when the project is private' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+        end
+
+        include_examples 'no N+1 queries'
+      end
+    end
+  end
+end

ee/spec/services/ee/notification_service_spec.rb

@@ -121,6 +121,90 @@ describe EE::NotificationService, :mailer do
     end
   end
 
+  context 'new review' do
+    let(:project) { create(:project, :repository) }
+    let(:user) { create(:user) }
+    let(:reviewer) { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: project, assignee: user, author: create(:user)) }
+    let(:review) { create(:review, merge_request: merge_request, project: project, author: reviewer) }
+    let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, author: reviewer, review: review) }
+
+    before do
+      build_team(review.project, merge_request)
+      project.add_maintainer(merge_request.author)
+      project.add_maintainer(reviewer)
+      project.add_maintainer(merge_request.assignee)
+
+      create(:diff_note_on_merge_request,
+             project: project,
+             noteable: merge_request,
+             author: reviewer,
+             review: review,
+             note: "cc @mention")
+    end
+
+    it 'sends emails' do
+      expect(Notify).not_to receive(:new_review_email).with(review.author.id, review.id)
+      expect(Notify).not_to receive(:new_review_email).with(@unsubscriber.id, review.id)
+      expect(Notify).to receive(:new_review_email).with(merge_request.assignee.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(merge_request.author.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@u_watcher.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@u_mentioned.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@subscriber.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@watcher_and_subscriber.id, review.id).and_call_original
+      expect(Notify).to receive(:new_review_email).with(@subscribed_participant.id, review.id).and_call_original
+
+      subject.new_review(review)
+    end
+
+    def build_team(project, merge_request)
+      @u_watcher               = create_global_setting_for(create(:user), :watch)
+      @u_participating         = create_global_setting_for(create(:user), :participating)
+      @u_participant_mentioned = create_global_setting_for(create(:user, username: 'participant'), :participating)
+      @u_disabled              = create_global_setting_for(create(:user), :disabled)
+      @u_mentioned             = create_global_setting_for(create(:user, username: 'mention'), :mention)
+      @u_committer             = create(:user, username: 'committer')
+      @u_not_mentioned         = create_global_setting_for(create(:user, username: 'regular'), :participating)
+      @u_outsider_mentioned    = create(:user, username: 'outsider')
+      @u_custom_global         = create_global_setting_for(create(:user, username: 'custom_global'), :custom)
+
+      # subscribers
+      @subscriber = create :user
+      @unsubscriber = create :user
+      @subscribed_participant = create_global_setting_for(create(:user, username: 'subscribed_participant'), :participating)
+      @watcher_and_subscriber = create_global_setting_for(create(:user), :watch)
+
+      # User to be participant by default
+      # This user does not contain any record in notification settings table
+      # It should be treated with a :participating notification_level
+      @u_lazy_participant = create(:user, username: 'lazy-participant')
+
+      @u_guest_watcher = create_user_with_notification(:watch, 'guest_watching')
+      @u_guest_custom = create_user_with_notification(:custom, 'guest_custom')
+
+      project.add_maintainer(@u_watcher)
+      project.add_maintainer(@u_participating)
+      project.add_maintainer(@u_participant_mentioned)
+      project.add_maintainer(@u_disabled)
+      project.add_maintainer(@u_mentioned)
+      project.add_maintainer(@u_committer)
+      project.add_maintainer(@u_not_mentioned)
+      project.add_maintainer(@u_lazy_participant)
+      project.add_maintainer(@u_custom_global)
+      project.add_maintainer(@subscriber)
+      project.add_maintainer(@unsubscriber)
+      project.add_maintainer(@subscribed_participant)
+      project.add_maintainer(@watcher_and_subscriber)
+
+      merge_request.subscriptions.create(user: @unsubscribed_mentioned, subscribed: false)
+      merge_request.subscriptions.create(user: @subscriber, subscribed: true)
+      merge_request.subscriptions.create(user: @subscribed_participant, subscribed: true)
+      merge_request.subscriptions.create(user: @unsubscriber, subscribed: false)
+      # Make the watcher a subscriber to detect dupes
+      merge_request.subscriptions.create(user: @watcher_and_subscriber, subscribed: true)
+    end
+  end
+
   describe 'mirror hard failed' do
     let(:user) { create(:user) }
 

ee/spec/services/users/migrate_to_ghost_user_service_spec.rb

@@ -28,4 +28,13 @@ describe Users::MigrateToGhostUserService do
       let(:created_record) { create(:vulnerability_feedback, author: user) }
     end
   end
+
+  context 'reviews' do
+    let!(:user)      { create(:user) }
+    let(:service)    { described_class.new(user) }
+
+    include_examples "migrating a deleted user's associated records to the ghost user", Review, [:author] do
+      let(:created_record) { create(:review, author: user) }
+    end
+  end
 end

ee/spec/workers/new_note_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe NewNoteWorker do
+  context 'when skip_notification' do
+    it 'does not create a new note notification' do
+      note = create(:note, :with_review)
+
+      expect_any_instance_of(NotificationService).not_to receive(:new_note)
+
+      subject.perform(note.id)
+    end
+  end
+end

spec/factories/notes.rb

@@ -122,6 +122,10 @@ FactoryBot.define do
       system true
     end
 
+    trait :with_review do
+      review
+    end
+
     trait :downvote do
       note "thumbsdown"
     end