diff --git a/app/controllers/projects/merge_requests/drafts_controller.rb b/app/controllers/projects/merge_requests/drafts_controller.rb
index 74c495261a31e2291e67d03a5f171cd55aab5118..1ec25d44bfa1df077172ccc14bb1ebff31c448af 100644
--- a/app/controllers/projects/merge_requests/drafts_controller.rb
+++ b/app/controllers/projects/merge_requests/drafts_controller.rb
@@ -61,7 +61,9 @@ def publish
merge_request_activity_counter.track_submit_review_comment(user: current_user)
end
- if Gitlab::Utils.to_boolean(approve_params[:approve])
+ if Feature.enabled?(:mr_request_changes, current_user) && reviewer_state_params[:reviewer_state]
+ update_reviewer_state
+ elsif Gitlab::Utils.to_boolean(approve_params[:approve])
unless merge_request.approved_by?(current_user)
success = ::MergeRequests::ApprovalService
.new(project: @project, current_user: current_user, params: approve_params)
@@ -144,6 +146,10 @@ def approve_params
params.permit(:approve)
end
+ def reviewer_state_params
+ params.permit(:reviewer_state)
+ end
+
def prepare_notes_for_rendering(notes)
return [] unless notes
@@ -180,6 +186,18 @@ def authorize_create_note!
def merge_request_activity_counter
Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter
end
+
+ def update_reviewer_state
+ if reviewer_state_params[:reviewer_state] === 'approved'
+ ::MergeRequests::ApprovalService
+ .new(project: @project, current_user: current_user)
+ .execute(merge_request)
+ else
+ ::MergeRequests::UpdateReviewerStateService
+ .new(project: @project, current_user: current_user)
+ .execute(merge_request, reviewer_state_params[:reviewer_state])
+ end
+ end
end
Projects::MergeRequests::DraftsController.prepend_mod
diff --git a/app/graphql/types/merge_request_review_state_enum.rb b/app/graphql/types/merge_request_review_state_enum.rb
index 45f977584255e199388be59159e29cc5281462ae..c7c82de29066bac1e62237e5c544b3952b1ef246 100644
--- a/app/graphql/types/merge_request_review_state_enum.rb
+++ b/app/graphql/types/merge_request_review_state_enum.rb
@@ -5,7 +5,11 @@ class MergeRequestReviewStateEnum < BaseEnum
graphql_name 'MergeRequestReviewState'
description 'State of a review of a GitLab merge request.'
- from_rails_enum(::MergeRequestReviewer.states,
- description: "The merge request is %{name}.")
+ value 'UNREVIEWED', value: 'unreviewed',
+ description: 'Awaiting review from merge request reviewer.'
+ value 'REVIEWED', value: 'reviewed',
+ description: 'Merge request reviewer has reviewed.'
+ value 'REQUESTED_CHANGES', value: 'requested_changes',
+ description: 'Merge request reviewer has requested changes.'
end
end
diff --git a/app/models/concerns/merge_request_reviewer_state.rb b/app/models/concerns/merge_request_reviewer_state.rb
index 412b1da55da5ea3dcd04b9bdaa0762be39c65126..e4ee6e7e58e1a13c27ba1019059f1407d380669b 100644
--- a/app/models/concerns/merge_request_reviewer_state.rb
+++ b/app/models/concerns/merge_request_reviewer_state.rb
@@ -6,7 +6,8 @@ module MergeRequestReviewerState
included do
enum state: {
unreviewed: 0,
- reviewed: 1
+ reviewed: 1,
+ requested_changes: 2
}
validates :state,
diff --git a/app/services/draft_notes/publish_service.rb b/app/services/draft_notes/publish_service.rb
index a7a2ad63c1cd8363fb1206746801d25d089af060..5ba7f829c8e955f1a6df0cec37df9077e5414425 100644
--- a/app/services/draft_notes/publish_service.rb
+++ b/app/services/draft_notes/publish_service.rb
@@ -81,7 +81,9 @@ def set_discussion_resolve_status(note, draft_note)
end
def set_reviewed
- ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user).execute(merge_request)
+ return if Feature.enabled?(:mr_request_changes, current_user)
+
+ ::MergeRequests::UpdateReviewerStateService.new(project: project, current_user: current_user).execute(merge_request, "reviewed")
end
def capture_diff_note_positions(notes)
diff --git a/app/services/merge_requests/mark_reviewer_reviewed_service.rb b/app/services/merge_requests/mark_reviewer_reviewed_service.rb
deleted file mode 100644
index 96747eabcf605fb981afa48f0a48717cb8e9f16f..0000000000000000000000000000000000000000
--- a/app/services/merge_requests/mark_reviewer_reviewed_service.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-module MergeRequests
- class MarkReviewerReviewedService < MergeRequests::BaseService
- def execute(merge_request)
- return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request)
-
- reviewer = merge_request.find_reviewer(current_user)
-
- if reviewer
- return error("Failed to update reviewer") unless reviewer.update(state: :reviewed)
-
- trigger_merge_request_reviewers_updated(merge_request)
-
- success
- else
- error("Reviewer not found")
- end
- end
- end
-end
diff --git a/app/services/merge_requests/update_reviewer_state_service.rb b/app/services/merge_requests/update_reviewer_state_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..e2252f55fd3f18f6b91422ac9c1cab54bb1ff4f0
--- /dev/null
+++ b/app/services/merge_requests/update_reviewer_state_service.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+module MergeRequests
+ class UpdateReviewerStateService < MergeRequests::BaseService
+ def execute(merge_request, state)
+ return error("Invalid permissions") unless can?(current_user, :update_merge_request, merge_request)
+
+ reviewer = merge_request.find_reviewer(current_user)
+
+ if reviewer
+ return error("Failed to update reviewer") unless reviewer.update(state: state)
+
+ trigger_merge_request_reviewers_updated(merge_request)
+
+ return success if state != 'requested_changes'
+
+ if merge_request.approved_by?(current_user) && !remove_approval(merge_request)
+ return error("Failed to remove approval")
+ end
+
+ success
+ else
+ error("Reviewer not found")
+ end
+ end
+
+ private
+
+ def remove_approval(merge_request)
+ MergeRequests::RemoveApprovalService.new(project: project, current_user: current_user)
+ .execute(merge_request)
+ end
+ end
+end
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 1af26377b7173f138fe8e85a957de49ad394f4e0..a63b1cf375fe95f3fac8026c3394410ed5025384 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -226,8 +226,10 @@ def track_note_creation_visual_review(note)
end
def set_reviewed(note)
- ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user)
- .execute(note.noteable)
+ return if Feature.enabled?(:mr_request_changes, current_user)
+
+ ::MergeRequests::UpdateReviewerStateService.new(project: project, current_user: current_user)
+ .execute(note.noteable, "reviewed")
end
end
end
diff --git a/app/workers/merge_requests/set_reviewer_reviewed_worker.rb b/app/workers/merge_requests/set_reviewer_reviewed_worker.rb
index 2f15bf3b8795c261a6cf1e8c1f177acb64ae9e85..7e8bc60f6e1c4cd349d4840fd63b44f7afe14099 100644
--- a/app/workers/merge_requests/set_reviewer_reviewed_worker.rb
+++ b/app/workers/merge_requests/set_reviewer_reviewed_worker.rb
@@ -13,18 +13,23 @@ def handle_event(event)
current_user_id = event.data[:current_user_id]
merge_request_id = event.data[:merge_request_id]
current_user = User.find_by_id(current_user_id)
- merge_request = MergeRequest.find_by_id(merge_request_id)
- if !current_user
+ unless current_user
logger.info(structured_payload(message: 'Current user not found.', current_user_id: current_user_id))
- elsif !merge_request
- logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id))
- else
- project = merge_request.source_project
+ return
+ end
+
+ merge_request = MergeRequest.find_by_id(merge_request_id)
- ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user)
- .execute(merge_request)
+ unless merge_request
+ logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id))
+ return
end
+
+ project = merge_request.source_project
+
+ ::MergeRequests::UpdateReviewerStateService.new(project: project, current_user: current_user)
+ .execute(merge_request, "reviewed")
end
end
end
diff --git a/config/feature_flags/development/mr_request_changes.yml b/config/feature_flags/development/mr_request_changes.yml
new file mode 100644
index 0000000000000000000000000000000000000000..f55e410190a9589be849c3381a522513c7efaf27
--- /dev/null
+++ b/config/feature_flags/development/mr_request_changes.yml
@@ -0,0 +1,8 @@
+---
+name: mr_request_changes
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134766
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/429557
+milestone: '16.6'
+type: development
+group: group::code review
+default_enabled: false
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index 1aae2d4ce05b3224be086f10d34a5959e6a2961e..91a1460df18d23adfbd2a65cd90c649673c461ea 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -29229,8 +29229,9 @@ State of a review of a GitLab merge request.
| Value | Description |
| ----- | ----------- |
-| <a id="mergerequestreviewstatereviewed"></a>`REVIEWED` | The merge request is reviewed. |
-| <a id="mergerequestreviewstateunreviewed"></a>`UNREVIEWED` | The merge request is unreviewed. |
+| <a id="mergerequestreviewstaterequested_changes"></a>`REQUESTED_CHANGES` | Merge request reviewer has requested changes. |
+| <a id="mergerequestreviewstatereviewed"></a>`REVIEWED` | Merge request reviewer has reviewed. |
+| <a id="mergerequestreviewstateunreviewed"></a>`UNREVIEWED` | Awaiting review from merge request reviewer. |
### `MergeRequestSort`
diff --git a/lib/api/merge_request_approvals.rb b/lib/api/merge_request_approvals.rb
index 35fdcfe3ab05627a68d3a3653aa76aa6abae90a3..d0c9400039af7d0ab36eb6c03df3a90e5055b604 100644
--- a/lib/api/merge_request_approvals.rb
+++ b/lib/api/merge_request_approvals.rb
@@ -86,6 +86,10 @@ def present_approval(merge_request)
not_found! unless success
+ ::MergeRequests::UpdateReviewerStateService
+ .new(project: user_project, current_user: current_user)
+ .execute(merge_request, "unreviewed")
+
present_approval(merge_request)
end
diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb
index 9798b0eca2cf971403d7a9a1c91874f1c5be4efa..c8dacc5ddd8feec2858906f13fe5e17d7444b968 100644
--- a/lib/gitlab/quick_actions/merge_request_actions.rb
+++ b/lib/gitlab/quick_actions/merge_request_actions.rb
@@ -197,6 +197,10 @@ module MergeRequestActions
next unless success
+ ::MergeRequests::UpdateReviewerStateService
+ .new(project: quick_action_target.project, current_user: current_user)
+ .execute(quick_action_target, "unreviewed")
+
@execution_message[:unapprove] = _('Unapproved the current merge request.')
end
diff --git a/spec/controllers/projects/merge_requests/drafts_controller_spec.rb b/spec/controllers/projects/merge_requests/drafts_controller_spec.rb
index 68fbeb00b676d34a7f09e39215355c760fd70f02..505f9f5b19b70c69397f686d51d8da6f9eafe919 100644
--- a/spec/controllers/projects/merge_requests/drafts_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests/drafts_controller_spec.rb
@@ -4,10 +4,10 @@
RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :code_review_workflow do
include RepoHelpers
- let(:project) { create(:project, :repository) }
- let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, author: create(:user)) }
- let(:user) { project.first_owner }
- let(:user2) { create(:user) }
+ let_it_be(:project) { create(:project, :repository) }
+ let_it_be_with_reload(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, author: create(:user)) }
+ let(:user) { project.first_owner }
+ let_it_be(:user2) { create(:user) }
let(:params) do
{
@@ -18,6 +18,8 @@
end
before do
+ create(:merge_request_reviewer, merge_request: merge_request, reviewer: user)
+
sign_in(user)
stub_licensed_features(multiple_merge_request_assignees: true)
stub_commonmark_sourcepos_disabled
@@ -216,9 +218,12 @@ def update_draft_note(overrides = {})
end
context 'without permissions' do
+ before_all do
+ project.add_developer(user2)
+ end
+
before do
sign_in(user2)
- project.add_developer(user2)
end
it 'does not allow editing draft note belonging to someone else' do
@@ -282,7 +287,7 @@ def update_draft_note(overrides = {})
end
context 'when note belongs to someone else' do
- before do
+ before_all do
project.add_developer(user2)
end
@@ -465,6 +470,24 @@ def create_reply(discussion_id, resolves: false)
end
end
+ context 'reviewer state' do
+ before do
+ create(:draft_note, merge_request: merge_request, author: user)
+ end
+
+ it 'updates reviewers state' do
+ post :publish, params: params.merge!(reviewer_state: 'requested_changes')
+
+ expect(merge_request.merge_request_reviewers.reload[0].state).to eq('requested_changes')
+ end
+
+ it 'approves merge request' do
+ post :publish, params: params.merge!(reviewer_state: 'approved')
+
+ expect(merge_request.approvals.reload.size).to eq(1)
+ end
+ end
+
context 'approve merge request' do
before do
allow(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
@@ -517,9 +540,12 @@ def create_draft
end
context 'without permissions' do
+ before_all do
+ project.add_developer(user2)
+ end
+
before do
sign_in(user2)
- project.add_developer(user2)
end
it 'does not allow destroying a draft note belonging to someone else' do
@@ -562,9 +588,12 @@ def create_draft
end
context 'without permissions' do
+ before_all do
+ project.add_developer(user2)
+ end
+
before do
sign_in(user2)
- project.add_developer(user2)
end
it 'does not destroys a draft note belonging to someone else' do
diff --git a/spec/graphql/types/merge_request_review_state_enum_spec.rb b/spec/graphql/types/merge_request_review_state_enum_spec.rb
index 486e1c4f502d8a8c57f1468d0ff2eadb6a7f632e..d8de3fcd1d125801315e13bf74d1dce8fa5e06ba 100644
--- a/spec/graphql/types/merge_request_review_state_enum_spec.rb
+++ b/spec/graphql/types/merge_request_review_state_enum_spec.rb
@@ -6,12 +6,16 @@
it 'the correct enum members' do
expect(described_class.values).to match(
'REVIEWED' => have_attributes(
- description: 'The merge request is reviewed.',
+ description: 'Merge request reviewer has reviewed.',
value: 'reviewed'
),
'UNREVIEWED' => have_attributes(
- description: 'The merge request is unreviewed.',
+ description: 'Awaiting review from merge request reviewer.',
value: 'unreviewed'
+ ),
+ 'REQUESTED_CHANGES' => have_attributes(
+ description: 'Merge request reviewer has requested changes.',
+ value: 'requested_changes'
)
)
end
diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb
index a1d6abec97e6e6d94db9e46dd5aec1c58d82f26b..df2b20c62c3dd869651d963a741775c1cbfa45ff 100644
--- a/spec/requests/api/merge_request_approvals_spec.rb
+++ b/spec/requests/api/merge_request_approvals_spec.rb
@@ -87,6 +87,28 @@ def approve(extra_params = {})
expect(response).to have_gitlab_http_status(:created)
end
+
+ it 'calls MergeRequests::UpdateReviewerStateService' do
+ unapprover = create(:user)
+
+ project.add_developer(approver)
+ project.add_developer(unapprover)
+ project.add_developer(create(:user))
+
+ create(:approval, user: approver, merge_request: merge_request)
+ create(:approval, user: unapprover, merge_request: merge_request)
+
+ expect_next_instance_of(
+ MergeRequests::UpdateReviewerStateService,
+ project: project, current_user: unapprover
+ ) do |service|
+ expect(service).to receive(:execute).with(merge_request, "unreviewed")
+ end
+
+ post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
+
+ expect(response).to have_gitlab_http_status(:created)
+ end
end
end
diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb
index e087f2ffc7ec181d03bdb26d504c33bcafe01ca6..fbc38f93c56a9433be07067f0ffccf2719b8cf96 100644
--- a/spec/services/draft_notes/publish_service_spec.rb
+++ b/spec/services/draft_notes/publish_service_spec.rb
@@ -4,7 +4,7 @@
RSpec.describe DraftNotes::PublishService, feature_category: :code_review_workflow do
include RepoHelpers
- let(:merge_request) { create(:merge_request) }
+ let_it_be(:merge_request) { create(:merge_request, reviewers: create_list(:user, 1)) }
let(:project) { merge_request.target_project }
let(:user) { merge_request.author }
let(:commit) { project.commit(sample_commit.id) }
@@ -198,6 +198,29 @@ def publish(draft: nil)
end
end
end
+
+ it 'does not call UpdateReviewerStateService' do
+ publish
+
+ expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new)
+ end
+
+ context 'when `mr_request_changes` feature flag is disabled' do
+ before do
+ stub_feature_flags(mr_request_changes: false)
+ end
+
+ it 'calls UpdateReviewerStateService' do
+ expect_next_instance_of(
+ MergeRequests::UpdateReviewerStateService,
+ project: project, current_user: user
+ ) do |service|
+ expect(service).to receive(:execute).with(merge_request, "reviewed")
+ end
+
+ publish
+ end
+ end
end
context 'draft notes with suggestions' do
diff --git a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb b/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb
deleted file mode 100644
index 172c2133168cfe7c901ad580f3227d812041bf29..0000000000000000000000000000000000000000
--- a/spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb
+++ /dev/null
@@ -1,55 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe MergeRequests::MarkReviewerReviewedService, feature_category: :code_review_workflow do
- let(:current_user) { create(:user) }
- let(:merge_request) { create(:merge_request, reviewers: [current_user]) }
- let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) }
- let(:project) { merge_request.project }
- let(:service) { described_class.new(project: project, current_user: current_user) }
- let(:result) { service.execute(merge_request) }
-
- before do
- project.add_developer(current_user)
- end
-
- describe '#execute' do
- shared_examples_for 'failed service execution' do
- it 'returns an error' do
- expect(result[:status]).to eq :error
- end
-
- it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do
- let(:action) { result }
- end
- end
-
- describe 'invalid permissions' do
- let(:service) { described_class.new(project: project, current_user: create(:user)) }
-
- it_behaves_like 'failed service execution'
- end
-
- describe 'reviewer does not exist' do
- let(:service) { described_class.new(project: project, current_user: create(:user)) }
-
- it_behaves_like 'failed service execution'
- end
-
- describe 'reviewer exists' do
- it 'returns success' do
- expect(result[:status]).to eq :success
- end
-
- it 'updates reviewers state' do
- expect(result[:status]).to eq :success
- expect(reviewer.state).to eq 'reviewed'
- end
-
- it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do
- let(:action) { result }
- end
- end
- end
-end
diff --git a/spec/services/merge_requests/update_reviewer_state_service_spec.rb b/spec/services/merge_requests/update_reviewer_state_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..be24d95d7f17486ec0fca8c9644f4b22d73bef83
--- /dev/null
+++ b/spec/services/merge_requests/update_reviewer_state_service_spec.rb
@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::UpdateReviewerStateService, feature_category: :code_review_workflow do
+ let_it_be(:current_user) { create(:user) }
+ let_it_be(:merge_request) { create(:merge_request, reviewers: [current_user]) }
+ let(:reviewer) { merge_request.merge_request_reviewers.find_by(user_id: current_user.id) }
+ let(:project) { merge_request.project }
+ let(:service) { described_class.new(project: project, current_user: current_user) }
+ let(:state) { 'requested_changes' }
+ let(:result) { service.execute(merge_request, state) }
+
+ before do
+ project.add_developer(current_user)
+ end
+
+ describe '#execute' do
+ shared_examples_for 'failed service execution' do
+ it 'returns an error' do
+ expect(result[:status]).to eq :error
+ end
+
+ it_behaves_like 'does not trigger GraphQL subscription mergeRequestReviewersUpdated' do
+ let(:action) { result }
+ end
+ end
+
+ describe 'invalid permissions' do
+ let(:service) { described_class.new(project: project, current_user: create(:user)) }
+
+ it_behaves_like 'failed service execution'
+ end
+
+ describe 'reviewer exists' do
+ it 'returns success' do
+ expect(result[:status]).to eq :success
+ end
+
+ it 'updates reviewers state' do
+ expect(result[:status]).to eq :success
+ expect(reviewer.state).to eq 'requested_changes'
+ end
+
+ it 'does not call MergeRequests::RemoveApprovalService' do
+ expect(MergeRequests::RemoveApprovalService).not_to receive(:new)
+
+ expect(result[:status]).to eq :success
+ end
+
+ it_behaves_like 'triggers GraphQL subscription mergeRequestReviewersUpdated' do
+ let(:action) { result }
+ end
+
+ context 'when reviewer has approved' do
+ before do
+ create(:approval, user: current_user, merge_request: merge_request)
+ end
+
+ it 'removes approval when state is requested_changes' do
+ expect_next_instance_of(
+ MergeRequests::RemoveApprovalService,
+ project: project, current_user: current_user
+ ) do |service|
+ expect(service).to receive(:execute).with(merge_request).and_return({ success: true })
+ end
+
+ expect(result[:status]).to eq :success
+ end
+
+ it 'renders error when remove approval service fails' do
+ expect_next_instance_of(
+ MergeRequests::RemoveApprovalService,
+ project: project, current_user: current_user
+ ) do |service|
+ expect(service).to receive(:execute).with(merge_request).and_return(nil)
+ end
+
+ expect(result[:status]).to eq :error
+ expect(result[:message]).to eq "Failed to remove approval"
+ end
+ end
+ end
+ end
+end
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index 874915150537d5f8e6c350c39067a6aa6cc1fe0d..c1b15ec7681d702a321fe86043fa0703c4af5e95 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -228,21 +228,35 @@
let(:new_opts) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) }
- it 'calls MergeRequests::MarkReviewerReviewedService service' do
- expect_next_instance_of(
- MergeRequests::MarkReviewerReviewedService,
- project: project_with_repo, current_user: user
- ) do |service|
- expect(service).to receive(:execute).with(merge_request)
+ context 'when mr_request_changes feature flag is disabled' do
+ before do
+ stub_feature_flags(mr_request_changes: false)
end
- described_class.new(project_with_repo, user, new_opts).execute
+ it 'calls MergeRequests::UpdateReviewerStateService service' do
+ expect_next_instance_of(
+ MergeRequests::UpdateReviewerStateService,
+ project: project_with_repo, current_user: user
+ ) do |service|
+ expect(service).to receive(:execute).with(merge_request, "reviewed")
+ end
+
+ described_class.new(project_with_repo, user, new_opts).execute
+ end
+
+ it 'does not call MergeRequests::UpdateReviewerStateService service when skip_set_reviewed is true' do
+ expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new)
+
+ described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true)
+ end
end
- it 'does not call MergeRequests::MarkReviewerReviewedService service when skip_set_reviewed is true' do
- expect(MergeRequests::MarkReviewerReviewedService).not_to receive(:new)
+ context 'when mr_request_changes feature flag is enabled' do
+ it 'does not call MergeRequests::UpdateReviewerStateService service when skip_set_reviewed is true' do
+ expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new)
- described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true)
+ described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true)
+ end
end
context 'noteable highlight cache clearing' do
diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb
index 2c34d6a59be7f210b611adcd71b6d3890a3805ea..d5108bb39886a1d6bcef4efb920e133bd134ceaf 100644
--- a/spec/services/quick_actions/interpret_service_spec.rb
+++ b/spec/services/quick_actions/interpret_service_spec.rb
@@ -2422,6 +2422,17 @@
expect(merge_request.approved_by_users).to be_empty
end
+ it 'calls MergeRequests::UpdateReviewerStateService' do
+ expect_next_instance_of(
+ MergeRequests::UpdateReviewerStateService,
+ project: project, current_user: current_user
+ ) do |service|
+ expect(service).to receive(:execute).with(merge_request, "unreviewed")
+ end
+
+ service.execute(content, merge_request)
+ end
+
context "when the user can't unapprove" do
before do
project.team.truncate
diff --git a/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb b/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb
index 942cf8e87e9e52822658a1fdc213bc420883a963..7341a0dcc5b1b00369408a1227cdeca3db914a82 100644
--- a/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb
+++ b/spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb
@@ -14,21 +14,21 @@
let(:event) { approved_event }
end
- it 'calls MergeRequests::MarkReviewerReviewedService' do
+ it 'calls MergeRequests::UpdateReviewerStateService' do
expect_next_instance_of(
- MergeRequests::MarkReviewerReviewedService,
+ MergeRequests::UpdateReviewerStateService,
project: project, current_user: user
) do |service|
- expect(service).to receive(:execute).with(merge_request)
+ expect(service).to receive(:execute).with(merge_request, "reviewed")
end
consume_event(subscriber: described_class, event: approved_event)
end
shared_examples 'when object does not exist' do
- it 'logs and does not call MergeRequests::MarkReviewerReviewedService' do
+ it 'logs and does not call MergeRequests::UpdateReviewerStateService' do
expect(Sidekiq.logger).to receive(:info).with(hash_including(log_payload))
- expect(MergeRequests::MarkReviewerReviewedService).not_to receive(:new)
+ expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new)
expect { consume_event(subscriber: described_class, event: approved_event) }
.not_to raise_exception