From 3c0d3b58c6e1478bc9bc02e61846804cccec3a2d Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Mon, 24 Jul 2023 08:14:17 +0000
Subject: [PATCH] Include review summary in new review email
When a review is created we send an email to notify users and we
want to include the review summary generated by AI.
To do that, we delay the sending of the `new_review_email` by
2 minutes as we make a request to AI to generate the review summary
asynchronously.
If after 2 minutes the review summary is still not available for
a specific review, then the new review email will be sent without
it.
We are still generating review summary behind
`automatically_summarize_mr_review` feature flag.
---
app/mailers/emails/reviews.rb | 15 +++++-----
app/mailers/previews/notify_preview.rb | 7 +++++
app/models/review.rb | 2 ++
app/services/notification_service.rb | 10 ++++++-
app/views/notify/new_review_email.html.haml | 1 +
app/views/notify/new_review_email.text.erb | 2 ++
ee/app/models/ee/review.rb | 11 +++++++
ee/app/services/ee/notification_service.rb | 12 ++++++++
ee/app/views/notify/_review_summary.html.haml | 7 +++++
ee/app/views/notify/_review_summary.text.erb | 8 +++++
ee/spec/mailers/notify_spec.rb | 11 +++++++
ee/spec/models/ee/review_spec.rb | 9 ++++++
.../services/ee/notification_service_spec.rb | 29 +++++++++++++++++++
locale/gitlab.pot | 3 ++
spec/mailers/previews_spec.rb | 1 +
15 files changed, 119 insertions(+), 9 deletions(-)
create mode 100644 ee/app/models/ee/review.rb
create mode 100644 ee/app/views/notify/_review_summary.html.haml
create mode 100644 ee/app/views/notify/_review_summary.text.erb
create mode 100644 ee/spec/models/ee/review_spec.rb
diff --git a/app/mailers/emails/reviews.rb b/app/mailers/emails/reviews.rb
index b98fa8aa6c9fa..ed1166509a58b 100644
--- a/app/mailers/emails/reviews.rb
+++ b/app/mailers/emails/reviews.rb
@@ -19,17 +19,16 @@ def review_thread_options(recipient_id)
end
def setup_review_email(review_id, recipient_id)
- review = Review.find_by_id(review_id)
-
- @notes = review.notes
- @discussions = Discussion.build_discussions(review.discussion_ids, preload_note_diff_file: true)
+ @review = Review.find_by_id(review_id)
+ @notes = @review.notes
+ @discussions = Discussion.build_discussions(@review.discussion_ids, preload_note_diff_file: true)
@include_diff_discussion_stylesheet = @discussions.values.any? do |discussion|
discussion.diff_discussion? && discussion.on_text?
end
- @author = review.author
- @author_name = review.author_name
- @project = review.project
- @merge_request = review.merge_request
+ @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
diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb
index 576dbdd8b52a7..93d4625c34421 100644
--- a/app/mailers/previews/notify_preview.rb
+++ b/app/mailers/previews/notify_preview.rb
@@ -284,6 +284,13 @@ def request_review_merge_request_email
Notify.request_review_merge_request_email(user.id, merge_request.id, user.id).message
end
+ def new_review_email
+ review = Review.last
+ mr_author = review.merge_request.author
+
+ Notify.new_review_email(mr_author.id, review.id).message
+ end
+
def project_was_moved_email
Notify.project_was_moved_email(project.id, user.id, "gitlab/gitlab").message
end
diff --git a/app/models/review.rb b/app/models/review.rb
index c621da3b03c54..d47aaf027ce7c 100644
--- a/app/models/review.rb
+++ b/app/models/review.rb
@@ -32,3 +32,5 @@ def user_mentions
merge_request.user_mentions.where.not(note_id: nil)
end
end
+
+Review.prepend_mod
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index b93b44ce79705..d305c8c03cfd4 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -723,9 +723,12 @@ def group_was_not_exported(group, current_user, errors)
# Notify users on new review in system
def new_review(review)
recipients = NotificationRecipients::BuildService.build_new_review_recipients(review)
+ deliver_options = new_review_deliver_options(review)
recipients.each do |recipient|
- mailer.new_review_email(recipient.user.id, review.id).deliver_later
+ mailer
+ .new_review_email(recipient.user.id, review.id)
+ .deliver_later(deliver_options)
end
end
@@ -946,6 +949,11 @@ def deliver_access_request_email(recipient, member)
def warn_skipping_notifications(user, object)
Gitlab::AppLogger.warn(message: "Skipping sending notifications", user: user.id, klass: object.class.to_s, object_id: object.id)
end
+
+ def new_review_deliver_options(review)
+ # Overridden in EE
+ {}
+ end
end
NotificationService.prepend_mod_with('NotificationService')
diff --git a/app/views/notify/new_review_email.html.haml b/app/views/notify/new_review_email.html.haml
index afc1bd682156b..8a184aa969675 100644
--- a/app/views/notify/new_review_email.html.haml
+++ b/app/views/notify/new_review_email.html.haml
@@ -22,3 +22,4 @@
- discussion.first_note.project = @project if discussion&.first_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; padding-bottom: 1em;", include_stylesheet_link: false, discussion: discussion, author: @author
+ = render_if_exists 'notify/review_summary'
diff --git a/app/views/notify/new_review_email.text.erb b/app/views/notify/new_review_email.text.erb
index 69cb33b05dfdc..e974c8b6be871 100644
--- a/app/views/notify/new_review_email.text.erb
+++ b/app/views/notify/new_review_email.text.erb
@@ -12,3 +12,5 @@
--
<% end %>
<% end %>
+
+<%= render_if_exists 'notify/review_summary' %>
diff --git a/ee/app/models/ee/review.rb b/ee/app/models/ee/review.rb
new file mode 100644
index 0000000000000..fa48aa0c9cc51
--- /dev/null
+++ b/ee/app/models/ee/review.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module EE
+ module Review
+ extend ActiveSupport::Concern
+
+ prepended do
+ has_one :merge_request_review_llm_summary, class_name: 'MergeRequest::ReviewLlmSummary'
+ end
+ end
+end
diff --git a/ee/app/services/ee/notification_service.rb b/ee/app/services/ee/notification_service.rb
index 6ad77f1b66200..9b6e910e4195a 100644
--- a/ee/app/services/ee/notification_service.rb
+++ b/ee/app/services/ee/notification_service.rb
@@ -182,5 +182,17 @@ def send_account_validation_email(pipeline)
email = user.notification_email_or_default
mailer.account_validation_email(pipeline, email).deliver_later
end
+
+ override :new_review_deliver_options
+ def new_review_deliver_options(review)
+ options = super
+
+ # We delay delivery by 2 minutes since we generate review summary asynchronously.
+ # We expect that in 2 minutes, we already have the summary generated so we
+ # can include it in the email.
+ options[:wait] = 2.minutes if Ability.allowed?(review.author, :summarize_submitted_review, review.merge_request)
+
+ options
+ end
end
end
diff --git a/ee/app/views/notify/_review_summary.html.haml b/ee/app/views/notify/_review_summary.html.haml
new file mode 100644
index 0000000000000..ac068f1333a87
--- /dev/null
+++ b/ee/app/views/notify/_review_summary.html.haml
@@ -0,0 +1,7 @@
+- review_llm_summary = @review.merge_request_review_llm_summary
+- if review_llm_summary.present?
+ .md{ style: 'border-bottom:1px solid #ededed; padding-bottom: 1em; padding-top: 1em;' }
+ %h4= _('Summary')
+ = markdown(review_llm_summary.content, pipeline: :email, author: review_llm_summary.reviewer, current_user: @recipient, issuable_reference_expansion_enabled: true)
+ %p{ style: 'color: #737278;' }
+ = _('Summary generated by AI (Experiment)')
diff --git a/ee/app/views/notify/_review_summary.text.erb b/ee/app/views/notify/_review_summary.text.erb
new file mode 100644
index 0000000000000..57fff56e3cb9e
--- /dev/null
+++ b/ee/app/views/notify/_review_summary.text.erb
@@ -0,0 +1,8 @@
+<% review_llm_summary = @review.merge_request_review_llm_summary %>
+<% if review_llm_summary.present? %>
+--
+
+<%= review_llm_summary.content %>
+
+<%= _('Summary generated by AI (Experiment)') %>
+<% end %>
diff --git a/ee/spec/mailers/notify_spec.rb b/ee/spec/mailers/notify_spec.rb
index 01e42dbd30969..0d04c92a8178c 100644
--- a/ee/spec/mailers/notify_spec.rb
+++ b/ee/spec/mailers/notify_spec.rb
@@ -402,6 +402,17 @@
end
end
+ describe 'merge request reviews' do
+ let!(:review) { create(:review, project: project, merge_request: merge_request) }
+ let!(:review_summary) { create(:merge_request_review_llm_summary, review: review) }
+
+ subject { described_class.new_review_email(recipient.id, review.id) }
+
+ it 'includes review summary' do
+ expect(subject.text_part.body.raw_source).to include(review_summary.content)
+ end
+ end
+
def expect_sender(user)
sender = subject.header[:from].addrs[0]
expect(sender.display_name).to eq("#{user.name} (@#{user.username})")
diff --git a/ee/spec/models/ee/review_spec.rb b/ee/spec/models/ee/review_spec.rb
new file mode 100644
index 0000000000000..fce8a253108ad
--- /dev/null
+++ b/ee/spec/models/ee/review_spec.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Review, feature_category: :code_review_workflow do
+ describe 'associations' do
+ it { is_expected.to have_one(:merge_request_review_llm_summary).class_name('MergeRequest::ReviewLlmSummary') }
+ end
+end
diff --git a/ee/spec/services/ee/notification_service_spec.rb b/ee/spec/services/ee/notification_service_spec.rb
index b34e77ab9d460..28959adc4fb0c 100644
--- a/ee/spec/services/ee/notification_service_spec.rb
+++ b/ee/spec/services/ee/notification_service_spec.rb
@@ -59,6 +59,35 @@
subject.new_review(review)
end
+ context 'when review author is allowed to summarize_submitted_review' do
+ before do
+ allow(Ability).to receive(:allowed?).and_call_original
+ allow(Ability)
+ .to receive(:allowed?)
+ .with(review.author, :summarize_submitted_review, review.merge_request)
+ .and_return(true)
+ end
+
+ it 'delays the sending of new_review_email by 2 minutes' do
+ new_review_email = double
+
+ 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_return(new_review_email)
+ end
+ expect(Notify).to receive(:new_review_email).with(merge_request.author.id, review.id).and_return(new_review_email)
+ expect(Notify).to receive(:new_review_email).with(@u_watcher.id, review.id).and_return(new_review_email)
+ expect(Notify).to receive(:new_review_email).with(@u_mentioned.id, review.id).and_return(new_review_email)
+ expect(Notify).to receive(:new_review_email).with(@subscriber.id, review.id).and_return(new_review_email)
+ expect(Notify).to receive(:new_review_email).with(@watcher_and_subscriber.id, review.id).and_return(new_review_email)
+ expect(Notify).to receive(:new_review_email).with(@subscribed_participant.id, review.id).and_return(new_review_email)
+ expect(new_review_email).to receive(:deliver_later).with({ wait: 2.minutes }).exactly(8).times
+
+ subject.new_review(review)
+ end
+ end
+
it_behaves_like 'project emails are disabled' do
let(:notification_target) { review }
let(:notification_trigger) { subject.new_review(review) }
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 1ba69edc3bf11..d39561ec7a69c 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -45170,6 +45170,9 @@ msgstr ""
msgid "Summary generated by AI"
msgstr ""
+msgid "Summary generated by AI (Experiment)"
+msgstr ""
+
msgid "Summary will be generated with the next push to this merge request and will appear here."
msgstr ""
diff --git a/spec/mailers/previews_spec.rb b/spec/mailers/previews_spec.rb
index 14bd56e5d40f2..e1af0d7ef7731 100644
--- a/spec/mailers/previews_spec.rb
+++ b/spec/mailers/previews_spec.rb
@@ -13,6 +13,7 @@
let_it_be(:issue) { create(:issue, project: project, milestone: milestone) }
let_it_be(:remote_mirror) { create(:remote_mirror, project: project) }
let_it_be(:member) { create(:project_member, :maintainer, project: project, created_by: user) }
+ let_it_be(:review) { create(:review, project: project, merge_request: merge_request, author: user) }
Gitlab.ee do
let_it_be(:epic) { create(:epic, group: group) }
--
GitLab