Move review related services outside EE

Another step to moving Review feature to Core.

This focuses on moving all `DraftNotes` services, services that
interact with `DraftNote` or `Review`, and related logic to review
notifications/emails only.

app/mailers/notify.rb

@@ -18,6 +18,7 @@ class Notify < ApplicationMailer
   include Emails::RemoteMirrors
   include Emails::Releases
   include Emails::Groups
+  include Emails::Reviews
 
   helper MilestonesHelper
   helper MergeRequestsHelper

app/services/notes/create_service.rb

@@ -88,9 +88,11 @@ module Notes
       end
     end
 
-    # EE::Notes::CreateService would override this method
     def quick_action_options
-      { merge_request_diff_head_sha: params[:merge_request_diff_head_sha] }
+      {
+        merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
+        review_id: params[:review_id]
+      }
     end
 
     def tracking_data_for(note)
@@ -103,5 +105,3 @@ module Notes
     end
   end
 end
-
-Notes::CreateService.prepend_if_ee('EE::Notes::CreateService')

app/services/notification_recipients/build_service.rb

@@ -32,7 +32,9 @@ module NotificationRecipients
     def self.build_new_release_recipients(*args)
       ::NotificationRecipients::Builder::NewRelease.new(*args).notification_recipients
     end
+
+    def self.build_new_review_recipients(*args)
+      ::NotificationRecipients::Builder::NewReview.new(*args).notification_recipients
+    end
   end
 end
-
-NotificationRecipients::BuildService.prepend_if_ee('EE::NotificationRecipients::BuildService')

app/services/notification_recipients/builder/new_review.rb

+# frozen_string_literal: true
+
+module NotificationRecipients
+  module Builder
+    class NewReview < 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

app/services/notification_service.rb

@@ -557,6 +557,15 @@ class NotificationService
     mailer.group_was_not_exported_email(current_user, group, errors).deliver_later
   end
 
+  # Notify users on new review in system
+  def new_review(review)
+    recipients = NotificationRecipients::BuildService.build_new_review_recipients(review)
+
+    recipients.each do |recipient|
+      mailer.new_review_email(recipient.user.id, review.id).deliver_later
+    end
+  end
+
   protected
 
   def new_resource_email(target, method)

app/services/users/migrate_to_ghost_user_service.rb

@@ -53,6 +53,7 @@ module Users
       migrate_abuse_reports
       migrate_award_emoji
       migrate_snippets
+      migrate_reviews
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
@@ -85,6 +86,10 @@ module Users
       snippets = user.snippets.only_project_snippets
       snippets.update_all(author_id: ghost_user.id)
     end
+
+    def migrate_reviews
+      user.reviews.update_all(author_id: ghost_user.id)
+    end
   end
 end
 

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

@@ -6,10 +6,9 @@
           %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))
+                - mr_link = link_to(@merge_request.to_reference(@project), project_merge_request_url(@project, @merge_request))
+                - mr_author_link = link_to(@author_name, user_url(@author))
+                = _('Merge request %{mr_link} was reviewed by %{mr_author}').html_safe % { mr_link: mr_link, mr_author: mr_author_link }
             %tr
               %td{ style: "overflow:hidden;font-size:14px;line-height:1.4;display:grid;" }
                 - @notes.each do |note|

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

-<%= "Merge request #{merge_request_url(@merge_request)} was reviewed by #{sanitize_name(@author_name)}" %>
+<% mr_url = merge_request_url(@merge_request) %>
+<% mr_author_name = sanitize_name(@author_name) %>
+<%= _('Merge request %{mr_link} was reviewed by %{mr_author}') % { mr_link: mr_url, mr_author: mr_author_name } %>
 
 --
 <% @notes.each_with_index do |note, index| %>

ee/app/mailers/ee/notify.rb

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

ee/app/services/ee/notes/create_service.rb

-# frozen_string_literal: true
-
-module EE
-  module Notes
-    module CreateService
-      extend ::Gitlab::Utils::Override
-
-      override :quick_action_options
-      def quick_action_options
-        super.merge(review_id: params[:review_id])
-      end
-    end
-  end
-end

ee/app/services/ee/notification_recipients/build_service.rb

-# frozen_string_literal: true
-
-module EE
-  module NotificationRecipients
-    module BuildService
-      extend ActiveSupport::Concern
-
-      class_methods do
-        def build_new_review_recipients(*args)
-          NotificationRecipients::Builder::NewReview.new(*args).notification_recipients
-        end
-      end
-    end
-  end
-end

ee/app/services/ee/notification_recipients/builder/new_review.rb

-# frozen_string_literal: true
-
-module EE
-  module NotificationRecipients
-    module Builder
-      class NewReview < ::NotificationRecipients::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

ee/app/services/ee/notification_service.rb

@@ -6,11 +6,6 @@ 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
@@ -63,14 +58,6 @@ module EE
 
     private
 
-    def send_new_review_notification(review)
-      recipients = ::NotificationRecipients::BuildService.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

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

@@ -9,7 +9,6 @@ module EE
         migrate_epics
         migrate_requirements_management_requirements
         migrate_vulnerabilities_feedback
-        migrate_reviews
         super
       end
 
@@ -28,10 +27,6 @@ module EE
         user.vulnerability_feedback.update_all(author_id: ghost_user.id)
         user.commented_vulnerability_feedback.update_all(comment_author_id: ghost_user.id)
       end
-
-      def migrate_reviews
-        user.reviews.update_all(author_id: ghost_user.id)
-      end
     end
   end
 end

ee/spec/mailers/notify_spec.rb

@@ -356,61 +356,6 @@ describe Notify do
     end
   end
 
-  describe 'merge request reviews' do
-    let!(:review) { create(:review, project: project, merge_request: merge_request) }
-    let!(:notes) { create_list(:note, 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
-
-    context 'when diff note' do
-      let!(:notes) { create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request) }
-
-      it 'links to notes' do
-        review.notes.each do |note|
-          # Text part
-          expect(subject.text_part.body.raw_source).to include(
-            project_merge_request_url(project, merge_request, anchor: "note_#{note.id}")
-          )
-        end
-      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/services/ee/notification_recipients/build_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe NotificationRecipients::BuildService 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/users/migrate_to_ghost_user_service_spec.rb

@@ -40,15 +40,6 @@ describe Users::MigrateToGhostUserService do
     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
-
   context 'requirements' do
     let!(:user)   { create(:user) }
     let(:service) { described_class.new(user) }

locale/gitlab.pot

@@ -13460,6 +13460,9 @@ msgstr ""
 msgid "Merge request %{iid} authored by %{authorName}"
 msgstr ""
 
+msgid "Merge request %{mr_link} was reviewed by %{mr_author}"
+msgstr ""
+
 msgid "Merge request approvals"
 msgstr ""
 

spec/mailers/notify_spec.rb

@@ -1726,4 +1726,59 @@ describe Notify do
       is_expected.to have_body_text target_url
     end
   end
+
+  describe 'merge request reviews' do
+    let!(:review) { create(:review, project: project, merge_request: merge_request) }
+    let!(:notes) { create_list(:note, 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
+
+    context 'when diff note' do
+      let!(:notes) { create_list(:diff_note_on_merge_request, 3, review: review, project: project, author: review.author, noteable: merge_request) }
+
+      it 'links to notes' do
+        review.notes.each do |note|
+          # Text part
+          expect(subject.text_part.body.raw_source).to include(
+            project_merge_request_url(project, merge_request, anchor: "note_#{note.id}")
+          )
+        end
+      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
 end

ee/spec/services/draft_notes/publish_service_spec.rb → spec/services/draft_notes/publish_service_spec.rb

@@ -214,10 +214,10 @@ describe DraftNotes::PublishService do
 
       create(:draft_note, merge_request: merge_request,
                           author: user,
-                          note: "thanks\n/assign #{user.to_reference} #{other_user.to_reference}")
+                          note: "thanks\n/assign #{other_user.to_reference}")
 
       expect { publish }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(2)
-      expect(merge_request.reload.assignees).to match_array([user, other_user])
+      expect(merge_request.reload.assignees).to match_array([other_user])
       expect(merge_request.notes.last).to be_system
     end
 

spec/services/notification_recipients/build_service_spec.rb

@@ -58,4 +58,56 @@ describe NotificationRecipients::BuildService do
       end
     end
   end
+
+  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

spec/services/notification_service_spec.rb

@@ -2863,6 +2863,57 @@ describe NotificationService, :mailer do
     end
   end
 
+  describe '#new_review' do
+    let(:project) { create(:project, :repository) }
+    let(:user) { create(:user) }
+    let(:user2) { create(:user) }
+    let(:reviewer) { create(:user) }
+    let(:merge_request) { create(:merge_request, source_project: project, assignees: [user, user2], 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)
+      add_users(review.project)
+      add_user_subscriptions(merge_request)
+      project.add_maintainer(merge_request.author)
+      project.add_maintainer(reviewer)
+      merge_request.assignees.each { |assignee| project.add_maintainer(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)
+      merge_request.assignee_ids.each do |assignee_id|
+        expect(Notify).to receive(:new_review_email).with(assignee_id, review.id).and_call_original
+      end
+      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
+
+    it_behaves_like 'project emails are disabled' do
+      let(:notification_target)  { review }
+      let(:notification_trigger) { subject.new_review(review) }
+
+      around do |example|
+        perform_enqueued_jobs { example.run }
+      end
+    end
+  end
+
   def build_team(project)
     @u_watcher               = create_global_setting_for(create(:user), :watch)
     @u_participating         = create_global_setting_for(create(:user), :participating)

spec/services/users/migrate_to_ghost_user_service_spec.rb

@@ -84,6 +84,15 @@ describe Users::MigrateToGhostUserService do
       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
+
     context "when record migration fails with a rollback exception" do
       before do
         expect_any_instance_of(ActiveRecord::Associations::CollectionProxy)