Add latest changes from gitlab-org/gitlab@master

app/assets/javascripts/behaviors/shortcuts/keybindings.js

@@ -381,6 +381,12 @@ export const PROJECT_FILES_GO_TO_PERMALINK = {
   defaultKeys: ['y'],
 };
 
+export const PROJECT_FILES_GO_TO_COMPARE = {
+  id: 'projectFiles.goToCompare',
+  description: __('Compare Branches'),
+  defaultKeys: ['shift+c'],
+};
+
 export const ISSUABLE_COMMENT_OR_REPLY = {
   id: 'issuables.commentReply',
   description: __('Comment/Reply (quoting selected text)'),
@@ -606,6 +612,7 @@ const PROJECT_FILES_SHORTCUTS_GROUP = {
     PROJECT_FILES_OPEN_SELECTION,
     PROJECT_FILES_GO_BACK,
     PROJECT_FILES_GO_TO_PERMALINK,
+    PROJECT_FILES_GO_TO_COMPARE,
   ],
 };
 

app/assets/javascripts/behaviors/shortcuts/shortcuts_navigation.js

@@ -18,6 +18,7 @@ import {
   GO_TO_PROJECT_KUBERNETES,
   GO_TO_PROJECT_ENVIRONMENTS,
   GO_TO_PROJECT_WEBIDE,
+  PROJECT_FILES_GO_TO_COMPARE,
   NEW_ISSUE,
 } from './keybindings';
 import Shortcuts from './shortcuts';
@@ -43,6 +44,7 @@ export default class ShortcutsNavigation extends Shortcuts {
       [GO_TO_PROJECT_SNIPPETS, () => findAndFollowLink('.shortcuts-snippets')],
       [GO_TO_PROJECT_KUBERNETES, () => findAndFollowLink('.shortcuts-kubernetes')],
       [GO_TO_PROJECT_ENVIRONMENTS, () => findAndFollowLink('.shortcuts-environments')],
+      [PROJECT_FILES_GO_TO_COMPARE, () => findAndFollowLink('.shortcuts-compare')],
       [GO_TO_PROJECT_WEBIDE, ShortcutsNavigation.navigateToWebIDE],
       [NEW_ISSUE, () => findAndFollowLink('.shortcuts-new-issue')],
     ]);

app/assets/javascripts/issuable/popover/components/mr_popover.vue

@@ -96,14 +96,14 @@ export default {
       </gl-skeleton-loader>
       <div v-else-if="showDetails" class="d-flex align-items-center justify-content-between">
         <div class="d-inline-flex align-items-center">
-          <gl-badge class="gl-mr-3" :variant="badgeVariant">
+          <gl-badge class="gl-mr-2" :variant="badgeVariant">
             {{ stateHumanName }}
           </gl-badge>
           <span class="gl-text-secondary">
             {{ __('Opened') }} <time v-text="formattedTime"></time
           ></span>
         </div>
-        <ci-icon v-if="detailedStatus" :status="detailedStatus" />
+        <ci-icon v-if="detailedStatus" :status="detailedStatus" class="gl-ml-2" />
       </div>
       <h5 v-if="!$apollo.queries.mergeRequest.loading" class="my-2">{{ title }}</h5>
       <!-- eslint-disable @gitlab/vue-require-i18n-strings -->

app/assets/stylesheets/framework/header.scss

@@ -630,10 +630,17 @@ $search-input-field-x-min-width: 200px;
 header.navbar-gitlab.super-sidebar-logged-out {
   background-color: $brand-charcoal !important;
 
+  li.nav-item > button,
   li.nav-item > a {
-    @include gl-text-white;
+    @include gl-text-gray-100;
     @include gl-font-weight-normal;
 
+    &:hover,
+    &:focus,
+    &:active {
+      @include gl-text-white
+    }
+
     &:hover,
     &:focus {
       background-color: $brand-gray-04;

app/controllers/projects/merge_requests/drafts_controller.rb

@@ -61,7 +61,9 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
       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 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
     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 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
   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

app/graphql/types/merge_request_review_state_enum.rb

@@ -5,7 +5,11 @@ module Types
     graphql_name 'MergeRequestReviewState'
     description 'State of a review of a GitLab merge request.'
 
-    from_rails_enum(::MergeRequestReviewer.states,
+    value 'UNREVIEWED', value: 'unreviewed',
-                    description: "The merge request is %{name}.")
+      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

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,

app/services/draft_notes/publish_service.rb

@@ -81,7 +81,9 @@ module DraftNotes
     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)

app/services/merge_requests/mark_reviewer_reviewed_service.rb

@@ -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

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

app/services/notes/create_service.rb

@@ -226,8 +226,10 @@ module Notes
     end
 
     def set_reviewed(note)
-      ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user)
+      return if Feature.enabled?(:mr_request_changes, current_user)
-        .execute(note.noteable)
+
+      ::MergeRequests::UpdateReviewerStateService.new(project: project, current_user: current_user)
+        .execute(note.noteable, "reviewed")
     end
   end
 end

app/workers/merge_requests/set_reviewer_reviewed_worker.rb

@@ -13,18 +13,23 @@ module MergeRequests
       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)
+
+      unless current_user
+        logger.info(structured_payload(message: 'Current user not found.', current_user_id: current_user_id))
+        return
+      end
+
       merge_request = MergeRequest.find_by_id(merge_request_id)
 
-      if !current_user
+      unless merge_request
-        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
+        return
+      end
+
       project = merge_request.source_project
 
-        ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user)
+      ::MergeRequests::UpdateReviewerStateService.new(project: project, current_user: current_user)
-          .execute(merge_request)
+              .execute(merge_request, "reviewed")
-      end
     end
   end
 end

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

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="mergerequestreviewstaterequested_changes"></a>`REQUESTED_CHANGES` | Merge request reviewer has requested changes. |
-| <a id="mergerequestreviewstateunreviewed"></a>`UNREVIEWED` | The merge request is unreviewed. |
+| <a id="mergerequestreviewstatereviewed"></a>`REVIEWED` | Merge request reviewer has reviewed. |
+| <a id="mergerequestreviewstateunreviewed"></a>`UNREVIEWED` | Awaiting review from merge request reviewer. |
 
 ### `MergeRequestSort`
 

doc/ci/variables/predefined_variables.md

@@ -107,10 +107,10 @@ as it can cause the pipeline to behave unexpectedly.
 | `CI_PROJECT_URL`                         | 8.10   | 0.5    | The HTTP(S) address of the project. |
 | `CI_PROJECT_VISIBILITY`                  | 10.3   | all    | The project visibility. Can be `internal`, `private`, or `public`. |
 | `CI_PROJECT_CLASSIFICATION_LABEL`        | 14.2   | all    | The project [external authorization classification label](../../administration/settings/external_authorization.md). |
-| `CI_REGISTRY_IMAGE`                      | 8.10   | 0.5    | The address of the project's Container Registry. Only available if the Container Registry is enabled for the project. |
+| `CI_REGISTRY`                            | 8.10   | 0.5    | Address of the [Container Registry](../../user/packages/container_registry/index.md) server, formatted as `<host>[:<port>]`. For example: `registry.gitlab.example.com`. Only available if the Container Registry is enabled for the GitLab instance. |
+| `CI_REGISTRY_IMAGE`                      | 8.10   | 0.5    | Base address for the container registry to push, pull, or tag project's images, formatted as `<host>[:<port>]/<project_full_path>`. For example: `registry.gitlab.example.com/my_group/my_project`. Image names must follow the [container registry naming convention](../../user/packages/container_registry/index.md#naming-convention-for-your-container-images). Only available if the Container Registry is enabled for the project. |
 | `CI_REGISTRY_PASSWORD`                   | 9.0    | all    | The password to push containers to the project's GitLab Container Registry. Only available if the Container Registry is enabled for the project. This password value is the same as the `CI_JOB_TOKEN` and is valid only as long as the job is running. Use the `CI_DEPLOY_PASSWORD` for long-lived access to the registry |
 | `CI_REGISTRY_USER`                       | 9.0    | all    | The username to push containers to the project's GitLab Container Registry. Only available if the Container Registry is enabled for the project. |
-| `CI_REGISTRY`                            | 8.10   | 0.5    | The address of the GitLab Container Registry. Only available if the Container Registry is enabled for the project. This variable includes a `:port` value if one is specified in the registry configuration. |
 | `CI_REPOSITORY_URL`                      | 9.0    | all    | The full path to Git clone (HTTP) the repository with a [CI/CD job token](../jobs/ci_job_token.md), in the format `https://gitlab-ci-token:$CI_JOB_TOKEN@gitlab.example.com/my-group/my-project.git`. |
 | `CI_RUNNER_DESCRIPTION`                  | 8.10   | 0.5    | The description of the runner. |
 | `CI_RUNNER_EXECUTABLE_ARCH`              | all    | 10.6   | The OS/architecture of the GitLab Runner executable. Might not be the same as the environment of the executor. |

doc/tutorials/build_application.md

@@ -31,7 +31,7 @@ Set up runners to run jobs in a pipeline.
 |-------|-------------|--------------------|
 | [Create, register, and run your own project runner](create_register_first_runner/index.md) | Learn the basics of how to create and register a project runner that runs jobs for your project. | **{star}** |
 | [Configure GitLab Runner to use the Google Kubernetes Engine](configure_gitlab_runner_to_use_gke/index.md) | Learn how to configure GitLab Runner to use the GKE to run jobs. | |
-| [Automate the creation of runners](https://about.gitlab.com/blog/2023/07/06/how-to-automate-creation-of-runners/) | Learn how to automate runner creation as an authenticated user to optimize your runner fleet.  | |
+| [Automate runner creation and registration](automate_runner_creation/index.md) | Learn how to automate runner creation as an authenticated user to optimize your runner fleet.  | |
 
 ## Publish a static website
 

doc/user/packages/container_registry/index.md

@@ -79,7 +79,7 @@ For more information on running container images, see the [Docker documentation]
 Your container images must follow this naming convention:
 
 ```plaintext
-<registry URL>/<namespace>/<project>/<image>
+<registry server>/<namespace>/<project>[/<optional path>]
 ```
 
 For example, if your project is `gitlab.example.com/mynamespace/myproject`,

doc/user/profile/index.md

@@ -62,6 +62,9 @@ To add new email to your account:
 1. Select **Add email address**.
 1. Verify your email address with the verification email received.
 
+NOTE:
+[Making your email non-public](#set-your-public-email) does not prevent it from being utilised for commit matching.
+
 ## Make your user profile page private
 
 You can make your user profile visible to only you and GitLab administrators.

doc/user/shortcuts.md

@@ -135,6 +135,7 @@ These shortcuts are available when browsing the files in a project (go to
 | <kbd>Enter</kbd>  | Open selection. |
 | <kbd>Escape</kbd> | Go back to file list screen (only while searching for files, **Code > Repository**, then select **Find File**). |
 | <kbd>y</kbd>      | Go to file permalink (only while viewing a file). |
+| <kbd>Shift</kbd> + <kbd>c</kbd>  | Go to compare branches view. |
 | <kbd>.</kbd>      | Open the [Web IDE](project/web_ide/index.md). |
 
 ### Web IDE

lib/api/merge_request_approvals.rb

@@ -86,6 +86,10 @@ module API
 
           not_found! unless success
 
+          ::MergeRequests::UpdateReviewerStateService
+            .new(project: user_project, current_user: current_user)
+            .execute(merge_request, "unreviewed")
+
           present_approval(merge_request)
         end
 

lib/gitlab/quick_actions/merge_request_actions.rb

@@ -197,6 +197,10 @@ module Gitlab
 
           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
 

locale/gitlab.pot

@@ -12235,6 +12235,9 @@ msgstr ""
 msgid "Compare %{oldCommitId}...%{newCommitId}"
 msgstr ""
 
+msgid "Compare Branches"
+msgstr ""
+
 msgid "Compare GitLab editions"
 msgstr ""
 

package.json

@@ -191,7 +191,7 @@
     "remark-rehype": "^10.1.0",
     "scrollparent": "^2.0.1",
     "semver": "^7.3.4",
-    "sentrybrowser": "npm:@sentry/browser@7.76.0",
+    "sentrybrowser": "npm:@sentry/browser@7.77.0",
     "sentrybrowser5": "npm:@sentry/browser@5.30.0",
     "sortablejs": "^1.10.2",
     "string-hash": "1.1.3",

spec/controllers/projects/merge_requests/drafts_controller_spec.rb

@@ -4,10 +4,10 @@ require 'spec_helper'
 RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :code_review_workflow do
   include RepoHelpers
 
-  let(:project)       { create(:project, :repository) }
+  let_it_be(:project) { create(:project, :repository) }
-  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project, author: create(:user)) }
+  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(:user2)         { create(:user) }
+  let_it_be(:user2) { create(:user) }
 
   let(:params) do
     {
@@ -18,6 +18,8 @@ RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :cod
   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 @@ RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :cod
     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 @@ RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :cod
       end
 
       context 'when note belongs to someone else' do
-        before do
+        before_all do
           project.add_developer(user2)
         end
 
@@ -465,6 +470,24 @@ RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :cod
       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 @@ RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :cod
     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 @@ RSpec.describe Projects::MergeRequests::DraftsController, feature_category: :cod
     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

spec/graphql/types/merge_request_review_state_enum_spec.rb

@@ -6,12 +6,16 @@ RSpec.describe GitlabSchema.types['MergeRequestReviewState'] do
   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

spec/requests/api/merge_request_approvals_spec.rb

@@ -87,6 +87,28 @@ RSpec.describe API::MergeRequestApprovals, feature_category: :source_code_manage
 
         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
 

spec/services/draft_notes/publish_service_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 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 @@ RSpec.describe DraftNotes::PublishService, feature_category: :code_review_workfl
         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

spec/services/merge_requests/mark_reviewer_reviewed_service_spec.rb

@@ -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

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

spec/services/notes/create_service_spec.rb

@@ -228,22 +228,36 @@ RSpec.describe Notes::CreateService, feature_category: :team_planning do
 
         let(:new_opts) { opts.merge(noteable_type: 'MergeRequest', noteable_id: merge_request.id) }
 
-        it 'calls MergeRequests::MarkReviewerReviewedService service' do
+        context 'when mr_request_changes feature flag is disabled' do
+          before do
+            stub_feature_flags(mr_request_changes: false)
+          end
+
+          it 'calls MergeRequests::UpdateReviewerStateService service' do
             expect_next_instance_of(
-            MergeRequests::MarkReviewerReviewedService,
+              MergeRequests::UpdateReviewerStateService,
               project: project_with_repo, current_user: user
             ) do |service|
-            expect(service).to receive(:execute).with(merge_request)
+              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::MarkReviewerReviewedService service when skip_set_reviewed is true' do
+          it 'does not call MergeRequests::UpdateReviewerStateService service when skip_set_reviewed is true' do
-          expect(MergeRequests::MarkReviewerReviewedService).not_to receive(:new)
+            expect(MergeRequests::UpdateReviewerStateService).not_to receive(:new)
 
             described_class.new(project_with_repo, user, new_opts).execute(skip_set_reviewed: true)
           end
+        end
+
+        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)
+          end
+        end
 
         context 'noteable highlight cache clearing' do
           let(:position) do

spec/services/quick_actions/interpret_service_spec.rb

@@ -2422,6 +2422,17 @@ RSpec.describe QuickActions::InterpretService, feature_category: :team_planning
         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

spec/workers/merge_requests/set_reviewer_reviewed_worker_spec.rb

@@ -14,21 +14,21 @@ RSpec.describe MergeRequests::SetReviewerReviewedWorker, feature_category: :sour
     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

yarn.lock

@@ -1913,14 +1913,14 @@
     estree-walker "^2.0.2"
     picomatch "^2.3.1"
 
-"@sentry-internal/tracing@7.76.0":
+"@sentry-internal/tracing@7.77.0":
-  version "7.76.0"
+  version "7.77.0"
-  resolved "https://registry.yarnpkg.com/@sentry-internal/tracing/-/tracing-7.76.0.tgz#36c54425bc20c08e569e6da52e13d325611cad66"
+  resolved "https://registry.yarnpkg.com/@sentry-internal/tracing/-/tracing-7.77.0.tgz#f3d82486f8934a955b3dd2aa54c8d29586e42a37"
-  integrity sha512-QQVIv+LS2sbGf/e5P2dRisHzXpy02dAcLqENLPG4sZ9otRaFNjdFYEqnlJ4qko+ORpJGQEQp/BX7Q/qzZQHlAg==
+  integrity sha512-8HRF1rdqWwtINqGEdx8Iqs9UOP/n8E0vXUu3Nmbqj4p5sQPA7vvCfq+4Y4rTqZFc7sNdFpDsRION5iQEh8zfZw==
   dependencies:
-    "@sentry/core" "7.76.0"
+    "@sentry/core" "7.77.0"
-    "@sentry/types" "7.76.0"
+    "@sentry/types" "7.77.0"
-    "@sentry/utils" "7.76.0"
+    "@sentry/utils" "7.77.0"
 
 "@sentry/core@5.30.0":
   version "5.30.0"
@@ -1933,13 +1933,13 @@
     "@sentry/utils" "5.30.0"
     tslib "^1.9.3"
 
-"@sentry/core@7.76.0":
+"@sentry/core@7.77.0":
-  version "7.76.0"
+  version "7.77.0"
-  resolved "https://registry.yarnpkg.com/@sentry/core/-/core-7.76.0.tgz#b0d1dc399a862ea8a1c8a1c60a409e92eaf8e9e1"
+  resolved "https://registry.yarnpkg.com/@sentry/core/-/core-7.77.0.tgz#21100843132beeeff42296c8370cdcc7aa1d8510"
-  integrity sha512-M+ptkCTeCNf6fn7p2MmEb1Wd9/JXUWxIT/0QEc+t11DNR4FYy1ZP2O9Zb3Zp2XacO7ORrlL3Yc+VIfl5JTgjfw==
+  integrity sha512-Tj8oTYFZ/ZD+xW8IGIsU6gcFXD/gfE+FUxUaeSosd9KHwBQNOLhZSsYo/tTVf/rnQI/dQnsd4onPZLiL+27aTg==
   dependencies:
-    "@sentry/types" "7.76.0"
+    "@sentry/types" "7.77.0"
-    "@sentry/utils" "7.76.0"
+    "@sentry/utils" "7.77.0"
 
 "@sentry/hub@5.30.0":
   version "5.30.0"
@@ -1959,25 +1959,25 @@
     "@sentry/types" "5.30.0"
     tslib "^1.9.3"
 
-"@sentry/replay@7.76.0":
+"@sentry/replay@7.77.0":
-  version "7.76.0"
+  version "7.77.0"
-  resolved "https://registry.yarnpkg.com/@sentry/replay/-/replay-7.76.0.tgz#bccf9ea4a6efc332a79d6a78f923697b9b283371"
+  resolved "https://registry.yarnpkg.com/@sentry/replay/-/replay-7.77.0.tgz#21d242c9cd70a7235237216174873fd140b6eb80"
-  integrity sha512-OACT7MfMHC/YGKnKST8SF1d6znr3Yu8fpUpfVVh2t9TNeh3+cQJVTOliHDqLy+k9Ljd5FtitgSn4IHtseCHDLQ==
+  integrity sha512-M9Ik2J5ekl+C1Och3wzLRZVaRGK33BlnBwfwf3qKjgLDwfKW+1YkwDfTHbc2b74RowkJbOVNcp4m8ptlehlSaQ==
   dependencies:
-    "@sentry-internal/tracing" "7.76.0"
+    "@sentry-internal/tracing" "7.77.0"
-    "@sentry/core" "7.76.0"
+    "@sentry/core" "7.77.0"
-    "@sentry/types" "7.76.0"
+    "@sentry/types" "7.77.0"
-    "@sentry/utils" "7.76.0"
+    "@sentry/utils" "7.77.0"
 
 "@sentry/types@5.30.0":
   version "5.30.0"
   resolved "https://registry.yarnpkg.com/@sentry/types/-/types-5.30.0.tgz#19709bbe12a1a0115bc790b8942917da5636f402"
   integrity sha512-R8xOqlSTZ+htqrfteCWU5Nk0CDN5ApUTvrlvBuiH1DyP6czDZ4ktbZB0hAgBlVcK0U+qpD3ag3Tqqpa5Q67rPw==
 
-"@sentry/types@7.76.0":
+"@sentry/types@7.77.0":
-  version "7.76.0"
+  version "7.77.0"
-  resolved "https://registry.yarnpkg.com/@sentry/types/-/types-7.76.0.tgz#628c9899bfa82ea762708314c50fd82f2138587d"
+  resolved "https://registry.yarnpkg.com/@sentry/types/-/types-7.77.0.tgz#c5d00fe547b89ccde59cdea59143bf145cee3144"
-  integrity sha512-vj6z+EAbVrKAXmJPxSv/clpwS9QjPqzkraMFk2hIdE/kii8s8kwnkBwTSpIrNc8GnzV3qYC4r3qD+BXDxAGPaw==
+  integrity sha512-nfb00XRJVi0QpDHg+JkqrmEBHsqBnxJu191Ded+Cs1OJ5oPXEW6F59LVcBScGvMqe+WEk1a73eH8XezwfgrTsA==
 
 "@sentry/utils@5.30.0":
   version "5.30.0"
@@ -1987,12 +1987,12 @@
     "@sentry/types" "5.30.0"
     tslib "^1.9.3"
 
-"@sentry/utils@7.76.0":
+"@sentry/utils@7.77.0":
-  version "7.76.0"
+  version "7.77.0"
-  resolved "https://registry.yarnpkg.com/@sentry/utils/-/utils-7.76.0.tgz#6b540b387d3ac539abd20978f4d3ae235114f6ab"
+  resolved "https://registry.yarnpkg.com/@sentry/utils/-/utils-7.77.0.tgz#1f88501f0b8777de31b371cf859d13c82ebe1379"
-  integrity sha512-40jFD+yfQaKpFYINghdhovzec4IEpB7aAuyH/GtE7E0gLpcqnC72r55krEIVILfqIR2Mlr5OKUzyeoCyWAU/yw==
+  integrity sha512-NmM2kDOqVchrey3N5WSzdQoCsyDkQkiRxExPaNI2oKQ/jMWHs9yt0tSy7otPBcXs0AP59ihl75Bvm1tDRcsp5g==
   dependencies:
-    "@sentry/types" "7.76.0"
+    "@sentry/types" "7.77.0"
 
 "@sinclair/typebox@^0.24.1":
   version "0.24.40"
@@ -11775,16 +11775,16 @@ send@0.17.2:
     "@sentry/utils" "5.30.0"
     tslib "^1.9.3"
 
-"sentrybrowser@npm:@sentry/browser@7.76.0":
+"sentrybrowser@npm:@sentry/browser@7.77.0":
-  version "7.76.0"
+  version "7.77.0"
-  resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-7.76.0.tgz#7d73573790023523f7d9c3757b8424b7ad60d664"
+  resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-7.77.0.tgz#155440f1a0d3a1bbd5d564c28d6b0c9853a51d72"
-  integrity sha512-83xA+cWrBhhkNuMllW5ucFsEO2NlUh2iBYtmg07lp3fyVW+6+b1yMKRnc4RFArJ+Wcq6UO+qk2ZEvrSAts1wEw==
+  integrity sha512-nJ2KDZD90H8jcPx9BysQLiQW+w7k7kISCWeRjrEMJzjtge32dmHA8G4stlUTRIQugy5F+73cOayWShceFP7QJQ==
   dependencies:
-    "@sentry-internal/tracing" "7.76.0"
+    "@sentry-internal/tracing" "7.77.0"
-    "@sentry/core" "7.76.0"
+    "@sentry/core" "7.77.0"
-    "@sentry/replay" "7.76.0"
+    "@sentry/replay" "7.77.0"
-    "@sentry/types" "7.76.0"
+    "@sentry/types" "7.77.0"
-    "@sentry/utils" "7.76.0"
+    "@sentry/utils" "7.77.0"
 
 serialize-javascript@^2.1.2:
   version "2.1.2"