Commit 63cfff7f authored by Igor Drozdov's avatar Igor Drozdov

Merge branch '28154-move-services-outside-ee' into 'master'

[RUN AS-IF-FOSS] Move review related services outside EE

See merge request gitlab-org/gitlab!32645
parents e8bc265c 64d035a6
......@@ -18,6 +18,7 @@ class Notify < ApplicationMailer
include Emails::RemoteMirrors
include Emails::Releases
include Emails::Groups
include Emails::Reviews
helper TimeboxesHelper
helper MergeRequestsHelper
......
......@@ -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')
......@@ -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')
# 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
......@@ -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)
......
......@@ -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
......
......@@ -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|
......
<%= "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| %>
......
......@@ -12,7 +12,6 @@ module EE
include ::Emails::AdminNotification
include ::Emails::ServiceDesk
include ::Emails::Epics
include ::Emails::Reviews
end
attr_reader :group
......
# 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
# 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
# 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
......@@ -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
......@@ -71,14 +66,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
......
......@@ -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
......@@ -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) }
......
# 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
......@@ -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) }
......
......@@ -13495,6 +13495,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 ""
......
......@@ -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
......@@ -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
......
......@@ -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
......@@ -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)
......
......@@ -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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment