Commit c2a219d1 authored by James Fargher's avatar James Fargher

Stop recursively submitting reviews

Previously submitting a review that used `/submit_review` would
recursively submit reviews until timeout
parent 926546a9
...@@ -4,9 +4,7 @@ module Notes ...@@ -4,9 +4,7 @@ module Notes
class CreateService < ::Notes::BaseService class CreateService < ::Notes::BaseService
# rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/CyclomaticComplexity
def execute def execute
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha) note = Notes::BuildService.new(project, current_user, params.except(:merge_request_diff_head_sha)).execute
note = Notes::BuildService.new(project, current_user, params).execute
# n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37440 # n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37440
note_valid = Gitlab::GitalyClient.allow_n_plus_1_calls do note_valid = Gitlab::GitalyClient.allow_n_plus_1_calls do
...@@ -23,8 +21,7 @@ module Notes ...@@ -23,8 +21,7 @@ module Notes
quick_actions_service = QuickActionsService.new(project, current_user) quick_actions_service = QuickActionsService.new(project, current_user)
if quick_actions_service.supported?(note) if quick_actions_service.supported?(note)
options = { merge_request_diff_head_sha: merge_request_diff_head_sha } content, update_params, message = quick_actions_service.execute(note, quick_action_options)
content, update_params, message = quick_actions_service.execute(note, options)
only_commands = content.empty? only_commands = content.empty?
...@@ -74,6 +71,11 @@ module Notes ...@@ -74,6 +71,11 @@ module Notes
private private
# EE::Notes::CreateService would override this method
def quick_action_options
{ merge_request_diff_head_sha: params[:merge_request_diff_head_sha] }
end
def tracking_data_for(note) def tracking_data_for(note)
label = Gitlab.ee? && note.author == User.visual_review_bot ? 'anonymous_visual_review_note' : 'note' label = Gitlab.ee? && note.author == User.visual_review_bot ? 'anonymous_visual_review_note' : 'note'
...@@ -84,3 +86,5 @@ module Notes ...@@ -84,3 +86,5 @@ module Notes
end end
end end
end end
Notes::CreateService.prepend_if_ee('EE::Notes::CreateService')
# 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
...@@ -28,6 +28,8 @@ module EE ...@@ -28,6 +28,8 @@ module EE
quick_action_target.persisted? && quick_action_target.project.feature_available?(:batch_comments, current_user) quick_action_target.persisted? && quick_action_target.project.feature_available?(:batch_comments, current_user)
end end
command :submit_review do command :submit_review do
next if params[:review_id]
result = DraftNotes::PublishService.new(quick_action_target, current_user).execute result = DraftNotes::PublishService.new(quick_action_target, current_user).execute
@execution_message[:submit_review] = if result[:status] == :success @execution_message[:submit_review] = if result[:status] == :success
_('Submitted the current review.') _('Submitted the current review.')
......
...@@ -711,17 +711,32 @@ describe QuickActions::InterpretService do ...@@ -711,17 +711,32 @@ describe QuickActions::InterpretService do
context 'submit_review command' do context 'submit_review command' do
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
let(:content) { '/submit_review' } let(:content) { '/submit_review' }
let!(:draft_note) { create(:draft_note, merge_request: merge_request, author: current_user) } let!(:draft_note) { create(:draft_note, note: note, merge_request: merge_request, author: current_user) }
before do before do
stub_licensed_features(batch_comments: true) stub_licensed_features(batch_comments: true)
end end
it 'submits the users current review' do context 'note has normal text' do
_, _, message = service.execute(content, merge_request) let(:note) { 'I like it' }
expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound) it 'submits the users current review' do
expect(message).to eq('Submitted the current review.') _, _, message = service.execute(content, merge_request)
expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect(message).to eq('Submitted the current review.')
end
end
context 'note has /submit_review' do
let(:note) { '/submit_review' }
it 'submits the users current review' do
_, _, message = service.execute(content, merge_request)
expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect(message).to eq('Submitted the current review.')
end
end end
end end
......
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