From 4cbf1ac09d69b1390d51bab9f26631ebc6fd6bbf Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Fri, 28 Jul 2023 16:13:30 +0000
Subject: [PATCH] Create a review submitted To-Do with summary
When a review has associated review summary generated by AI,
show it in the To-Do item.
Calls the TodoService#review_submitted when a review summary is
generated to create the To-Do.
This is behind automatically_summarize_mr_review feature flag.
---
app/models/todo.rb | 4 ++
app/views/dashboard/todos/_todo.html.haml | 1 +
ee/app/helpers/ee/merge_requests_helper.rb | 8 +++
ee/app/models/ee/merge_request_diff.rb | 4 ++
.../merge_request/review_llm_summary.rb | 2 +
.../dashboard/todos/_diff_summary.html.haml | 7 +-
.../dashboard/todos/_llm_summary.html.haml | 8 +++
.../dashboard/todos/_review_summary.html.haml | 6 ++
.../completions/summarize_submitted_review.rb | 6 ++
ee/spec/features/dashboards/todos_spec.rb | 70 ++++++++++++++++++-
ee/spec/helpers/merge_requests_helper_spec.rb | 45 ++++++++++++
.../summarize_submitted_review_spec.rb | 14 ++++
ee/spec/models/ee/merge_request_diff_spec.rb | 20 ++++++
.../merge_request/review_llm_summary_spec.rb | 11 +++
14 files changed, 198 insertions(+), 8 deletions(-)
create mode 100644 ee/app/views/dashboard/todos/_llm_summary.html.haml
create mode 100644 ee/app/views/dashboard/todos/_review_summary.html.haml
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 2f2e731fc7ef7..d159b51a0eb5f 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -225,6 +225,10 @@ def member_access_requested?
action == MEMBER_ACCESS_REQUESTED
end
+ def review_submitted?
+ action == REVIEW_SUBMITTED
+ end
+
def member_access_type
target.class.name.downcase
end
diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml
index e20fccc218a32..1cd8015934e5f 100644
--- a/app/views/dashboard/todos/_todo.html.haml
+++ b/app/views/dashboard/todos/_todo.html.haml
@@ -48,6 +48,7 @@
= first_line_in_markdown(todo, :body, 125, is_todo: true, project: todo.project, group: todo.group)
= render_if_exists "dashboard/todos/diff_summary", local_assigns: { todo: todo }
+ = render_if_exists "dashboard/todos/review_summary", local_assigns: { todo: todo }
.todo-timestamp.gl-white-space-nowrap.gl-sm-ml-3.gl-mt-2.gl-mb-2.gl-sm-my-0.gl-px-2.gl-sm-px-0
%span.todo-timestamp.gl-font-sm.gl-text-secondary
diff --git a/ee/app/helpers/ee/merge_requests_helper.rb b/ee/app/helpers/ee/merge_requests_helper.rb
index b9e265a0bd5f2..7065ef71abccb 100644
--- a/ee/app/helpers/ee/merge_requests_helper.rb
+++ b/ee/app/helpers/ee/merge_requests_helper.rb
@@ -42,5 +42,13 @@ def review_bar_data(merge_request, user)
def diff_llm_summary(merge_request)
merge_request.latest_merge_request_diff&.merge_request_diff_llm_summary
end
+
+ def review_llm_summary_allowed?(merge_request, user)
+ Ability.allowed?(user, :summarize_submitted_review, merge_request)
+ end
+
+ def review_llm_summary(merge_request, reviewer)
+ merge_request.latest_merge_request_diff&.latest_review_summary_from_reviewer(reviewer)
+ end
end
end
diff --git a/ee/app/models/ee/merge_request_diff.rb b/ee/app/models/ee/merge_request_diff.rb
index d1b9ff80fde25..9fa544c040aac 100644
--- a/ee/app/models/ee/merge_request_diff.rb
+++ b/ee/app/models/ee/merge_request_diff.rb
@@ -104,5 +104,9 @@ def log_geo_deleted_event
# Keep empty for now. Should be addressed in future
# by https://gitlab.com/gitlab-org/gitlab/issues/33817
end
+
+ def latest_review_summary_from_reviewer(reviewer)
+ merge_request_review_llm_summaries.from_reviewer(reviewer).last
+ end
end
end
diff --git a/ee/app/models/merge_request/review_llm_summary.rb b/ee/app/models/merge_request/review_llm_summary.rb
index 3480730975e0a..b08098438c3cb 100644
--- a/ee/app/models/merge_request/review_llm_summary.rb
+++ b/ee/app/models/merge_request/review_llm_summary.rb
@@ -11,6 +11,8 @@ class MergeRequest::ReviewLlmSummary < ApplicationRecord
enum provider: { open_ai: 0, vertex_ai: 1 }
+ scope :from_reviewer, ->(reviewer) { joins(:review).where(review: { author_id: reviewer.id }) }
+
def reviewer
review.author
end
diff --git a/ee/app/views/dashboard/todos/_diff_summary.html.haml b/ee/app/views/dashboard/todos/_diff_summary.html.haml
index c3a8407c3f5f5..5ea6e71320895 100644
--- a/ee/app/views/dashboard/todos/_diff_summary.html.haml
+++ b/ee/app/views/dashboard/todos/_diff_summary.html.haml
@@ -4,9 +4,4 @@
- merge_request = todo.target
- diff_llm_summary = diff_llm_summary(merge_request)
- if summarize_llm_enabled?(merge_request.project, current_user) && diff_llm_summary.present?
- .todo-diff-summary.gl-mt-3.gl-py-3.gl-pl-6.gl-inset-border-l-4-gray-100
- %p.gl-m-0{ class: "gl-text-gray-600!" }= diff_llm_summary.content
- .gl-text-gray-400.gl-mt-4
- = sprite_icon('tanuki-ai', size: 16, css_class: 'gl-text-gray-600!')
- = _('Summary generated by AI')
- = render Pajamas::BadgeComponent.new(_('Experiment'), size: 'sm', variant: 'neutral', class: 'gl-ml-3')
+ = render 'llm_summary', local_assigns: { content: diff_llm_summary.content }
diff --git a/ee/app/views/dashboard/todos/_llm_summary.html.haml b/ee/app/views/dashboard/todos/_llm_summary.html.haml
new file mode 100644
index 0000000000000..f23d9d1aa2569
--- /dev/null
+++ b/ee/app/views/dashboard/todos/_llm_summary.html.haml
@@ -0,0 +1,8 @@
+- content = local_assigns.fetch(:content)
+
+.todo-llm-summary.gl-mt-3.gl-py-3.gl-pl-6.gl-inset-border-l-4-gray-100
+ %p.gl-m-0{ class: "gl-text-gray-600!" }= content
+ .gl-text-gray-400.gl-mt-4
+ = sprite_icon('tanuki-ai', size: 16, css_class: 'gl-text-gray-600!')
+ = _('Summary generated by AI')
+ = render Pajamas::BadgeComponent.new(_('Experiment'), size: 'sm', variant: 'neutral', class: 'gl-ml-3')
diff --git a/ee/app/views/dashboard/todos/_review_summary.html.haml b/ee/app/views/dashboard/todos/_review_summary.html.haml
new file mode 100644
index 0000000000000..15efe15558f00
--- /dev/null
+++ b/ee/app/views/dashboard/todos/_review_summary.html.haml
@@ -0,0 +1,6 @@
+- todo = local_assigns.fetch(:todo)
+
+- if todo.review_submitted? && review_llm_summary_allowed?(todo.target, todo.user)
+ - review_summary = review_llm_summary(todo.target, todo.author)
+ - if review_summary
+ = render 'llm_summary', local_assigns: { content: review_summary.content }
diff --git a/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb b/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb
index b1c110b913aa6..c7f60fb233065 100644
--- a/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb
+++ b/ee/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review.rb
@@ -42,6 +42,12 @@ def store_response(response_modifier, review, mr_diff)
provider: MergeRequest::ReviewLlmSummary.providers[:vertex_ai],
content: response_modifier.response_body
)
+
+ create_todo(review)
+ end
+
+ def create_todo(review)
+ TodoService.new.review_submitted(review)
end
end
end
diff --git a/ee/spec/features/dashboards/todos_spec.rb b/ee/spec/features/dashboards/todos_spec.rb
index 7639f37f7499b..f0251e72a62b5 100644
--- a/ee/spec/features/dashboards/todos_spec.rb
+++ b/ee/spec/features/dashboards/todos_spec.rb
@@ -110,7 +110,7 @@
visit page_path
page.within('.js-todos-all') do
- expect(page).not_to have_selector('.todo-diff-summary')
+ expect(page).not_to have_selector('.todo-llm-summary')
end
end
@@ -125,8 +125,74 @@
it 'shows the todo with diff summary' do
visit page_path
- page.within('.js-todos-all .todo-diff-summary') do
+ page.within('.js-todos-all .todo-llm-summary') do
expect(page).to have_content(diff_summary.content)
+ expect(page).to have_content('Summary generated by AI')
+ end
+ end
+ end
+ end
+
+ context 'when user has review submitted todo', :saas do
+ let_it_be(:namespace) { create(:group_with_plan, plan: :ultimate_plan) }
+ let_it_be(:project) { create(:project, group: namespace) }
+ let_it_be(:merge_request) { create(:merge_request, :skip_diff_creation, source_project: project) }
+ let_it_be(:merge_request_diff) { create(:merge_request_diff, merge_request: merge_request) }
+ let_it_be(:review) { create(:review, merge_request: merge_request) }
+ let_it_be(:todo) do
+ create(
+ :todo,
+ :review_submitted,
+ author: review.author,
+ user: user,
+ project: project,
+ target: merge_request
+ )
+ end
+
+ before_all do
+ project.add_developer(user)
+ end
+
+ before do
+ stub_ee_application_setting(should_check_namespace_plan: true)
+
+ stub_licensed_features(
+ summarize_submitted_review: true,
+ ai_features: true
+ )
+
+ project.reload.root_ancestor.namespace_settings.update!(
+ experiment_features_enabled: true,
+ third_party_ai_features_enabled: true
+ )
+
+ sign_in(user)
+ end
+
+ it 'does not show todo with review summary' do
+ visit page_path
+
+ page.within('.js-todos-all') do
+ expect(page).not_to have_selector('.todo-llm-summary')
+ end
+ end
+
+ context 'when merge request has review summary' do
+ let!(:review_summary) do
+ create(
+ :merge_request_review_llm_summary,
+ merge_request_diff: merge_request_diff,
+ review: review
+ )
+ end
+
+ it 'shows the todo with review summary' do
+ visit page_path
+
+ page.within('.js-todos-all .todo-llm-summary') do
+ expect(page).to have_content(review_summary.content)
+ expect(page).to have_content('Summary generated by AI')
end
end
end
diff --git a/ee/spec/helpers/merge_requests_helper_spec.rb b/ee/spec/helpers/merge_requests_helper_spec.rb
index b7e7e589cb376..195500de99570 100644
--- a/ee/spec/helpers/merge_requests_helper_spec.rb
+++ b/ee/spec/helpers/merge_requests_helper_spec.rb
@@ -108,4 +108,49 @@
it { expect(helper.diff_llm_summary(merge_request)).to eq(nil) }
end
end
+
+ describe '#review_llm_summary_allowed?' do
+ let(:user) { build_stubbed(:user) }
+ let(:merge_request) { build_stubbed(:merge_request) }
+
+ it 'calls Ability.allowed? with summarize_submitted_review' do
+ expect(Ability)
+ .to receive(:allowed?)
+ .with(user, :summarize_submitted_review, merge_request)
+ .and_return(true)
+
+ expect(review_llm_summary_allowed?(merge_request, user)).to eq(true)
+ end
+ end
+
+ describe '#review_llm_summary' do
+ let_it_be(:merge_request) { build_stubbed(:merge_request) }
+ let_it_be(:reviewer) { build_stubbed(:user) }
+ let(:latest_merge_request_diff) { instance_double(MergeRequestDiff) }
+
+ before do
+ allow(merge_request)
+ .to receive(:latest_merge_request_diff)
+ .and_return(latest_merge_request_diff)
+ end
+
+ it 'returns latest review summary from reviewer' do
+ latest_review_summary = instance_double(MergeRequest::ReviewLlmSummary)
+
+ expect(latest_merge_request_diff)
+ .to receive(:latest_review_summary_from_reviewer)
+ .with(reviewer)
+ .and_return(latest_review_summary)
+
+ expect(review_llm_summary(merge_request, reviewer)).to eq(latest_review_summary)
+ end
+
+ context 'when merge request has no latest merge request diff' do
+ let(:latest_merge_request_diff) { nil }
+
+ it 'returns nil' do
+ expect(review_llm_summary(merge_request, reviewer)).to be_nil
+ end
+ end
+ end
end
diff --git a/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb b/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb
index 957faca3d125f..2ab037ab52a30 100644
--- a/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb
+++ b/ee/spec/lib/gitlab/llm/vertex_ai/completions/summarize_submitted_review_spec.rb
@@ -68,6 +68,14 @@
expect(latest_review_llm_summary.content).to eq('Summary generated by AI')
end
end
+
+ it 'create a todo' do
+ expect_next_instance_of(TodoService) do |svc|
+ expect(svc).to receive(:review_submitted).with(review)
+ end
+
+ subject.execute(user, merge_request, options)
+ end
end
context 'when the chat client returns an unsuccessful response' do
@@ -83,6 +91,12 @@
expect { subject.execute(user, merge_request, options) }
.not_to change { mr_diff.merge_request_review_llm_summaries.count }
end
+
+ it 'does not create a todo' do
+ expect(TodoService).not_to receive(:new)
+
+ subject.execute(user, merge_request, options)
+ end
end
end
end
diff --git a/ee/spec/models/ee/merge_request_diff_spec.rb b/ee/spec/models/ee/merge_request_diff_spec.rb
index aaa9445a799a1..92821c1aab119 100644
--- a/ee/spec/models/ee/merge_request_diff_spec.rb
+++ b/ee/spec/models/ee/merge_request_diff_spec.rb
@@ -323,4 +323,24 @@
end
end
end
+
+ describe '#latest_review_summary_from_reviewer' do
+ let(:merge_request) { create(:merge_request, :skip_diff_creation) }
+ let(:merge_request_2) { create(:merge_request, :skip_diff_creation) }
+ let(:reviewer) { merge_request.author }
+ let(:mr_diff_1) { create(:merge_request_diff, merge_request: merge_request) }
+ let(:mr_diff_2) { create(:merge_request_diff, merge_request: merge_request_2) }
+ let(:review_1) { create(:review, merge_request: merge_request, author: reviewer) }
+ let(:review_2) { create(:review, merge_request: merge_request, author: reviewer) }
+ let(:review_3) { create(:review, merge_request: merge_request) }
+ let(:review_4) { create(:review, merge_request: merge_request_2, author: reviewer) }
+ let!(:review_summary_1) { create(:merge_request_review_llm_summary, merge_request_diff: mr_diff_1, review: review_1) }
+ let!(:review_summary_2) { create(:merge_request_review_llm_summary, merge_request_diff: mr_diff_1, review: review_2) }
+ let!(:review_summary_3) { create(:merge_request_review_llm_summary, merge_request_diff: mr_diff_1, review: review_3) }
+ let!(:review_summary_4) { create(:merge_request_review_llm_summary, merge_request_diff: mr_diff_2, review: review_4) }
+
+ it 'returns the latest review summary from reviewer' do
+ expect(mr_diff_1.latest_review_summary_from_reviewer(reviewer)).to eq(review_summary_2)
+ end
+ end
end
diff --git a/ee/spec/models/merge_request/review_llm_summary_spec.rb b/ee/spec/models/merge_request/review_llm_summary_spec.rb
index 0f971accad930..282a95dd95794 100644
--- a/ee/spec/models/merge_request/review_llm_summary_spec.rb
+++ b/ee/spec/models/merge_request/review_llm_summary_spec.rb
@@ -14,6 +14,17 @@
it { is_expected.to validate_presence_of(:provider) }
end
+ describe '.from_reviewer' do
+ let(:review_1) { create(:review) }
+ let(:review_2) { create(:review) }
+ let!(:review_llm_summary_1) { create(:merge_request_review_llm_summary, review: review_1) }
+ let!(:review_llm_summary_2) { create(:merge_request_review_llm_summary, review: review_2) }
+
+ it 'returns review LLM summaries that were generated for the reviews from the reviewer' do
+ expect(described_class.from_reviewer(review_1.author)).to eq([review_llm_summary_1])
+ end
+ end
+
describe '#reviewer' do
it 'returns author of associated review' do
expect(merge_request_review_llm_summary.reviewer).to eq(merge_request_review_llm_summary.review.author)
--
GitLab