diff --git a/app/assets/javascripts/behaviors/shortcuts/keybindings.js b/app/assets/javascripts/behaviors/shortcuts/keybindings.js index e8c486f6e74..941662635ea 100644 --- a/app/assets/javascripts/behaviors/shortcuts/keybindings.js +++ b/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, ], }; diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_navigation.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_navigation.js index d9dc3aae808..4691a4228e6 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_navigation.js +++ b/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')], ]); diff --git a/app/assets/javascripts/issuable/popover/components/mr_popover.vue b/app/assets/javascripts/issuable/popover/components/mr_popover.vue index e2c2181684f..80ae8ed8cf6 100644 --- a/app/assets/javascripts/issuable/popover/components/mr_popover.vue +++ b/app/assets/javascripts/issuable/popover/components/mr_popover.vue @@ -96,14 +96,14 @@ export default {
- + {{ stateHumanName }} {{ __('Opened') }}
- +
{{ title }}
diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index 32735679ded..e269ea68e41 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/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; diff --git a/app/controllers/projects/merge_requests/drafts_controller.rb b/app/controllers/projects/merge_requests/drafts_controller.rb index 74c495261a3..1ec25d44bfa 100644 --- a/app/controllers/projects/merge_requests/drafts_controller.rb +++ b/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 diff --git a/app/graphql/types/merge_request_review_state_enum.rb b/app/graphql/types/merge_request_review_state_enum.rb index 45f97758425..c7c82de2906 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 @@ module Types 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 412b1da55da..e4ee6e7e58e 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 a7a2ad63c1c..5ba7f829c8e 100644 --- a/app/services/draft_notes/publish_service.rb +++ b/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) 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 96747eabcf6..00000000000 --- 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 00000000000..e2252f55fd3 --- /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 1af26377b71..a63b1cf375f 100644 --- a/app/services/notes/create_service.rb +++ b/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) - .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 2f15bf3b879..7e8bc60f6e1 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 @@ 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 - logger.info(structured_payload(message: 'Current user not found.', current_user_id: current_user_id)) - elsif !merge_request + unless merge_request logger.info(structured_payload(message: 'Merge request not found.', merge_request_id: merge_request_id)) - else - project = merge_request.source_project - - ::MergeRequests::MarkReviewerReviewedService.new(project: project, current_user: current_user) - .execute(merge_request) + 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 00000000000..f55e410190a --- /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 1aae2d4ce05..91a1460df18 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 | | ----- | ----------- | -| `REVIEWED` | The merge request is reviewed. | -| `UNREVIEWED` | The merge request is unreviewed. | +| `REQUESTED_CHANGES` | Merge request reviewer has requested changes. | +| `REVIEWED` | Merge request reviewer has reviewed. | +| `UNREVIEWED` | Awaiting review from merge request reviewer. | ### `MergeRequestSort` diff --git a/doc/ci/variables/predefined_variables.md b/doc/ci/variables/predefined_variables.md index a77ba781d7d..ceca0eefe5b 100644 --- a/doc/ci/variables/predefined_variables.md +++ b/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 `[:]`. 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 `[:]/`. 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. | diff --git a/doc/tutorials/build_application.md b/doc/tutorials/build_application.md index 2b1f63874b1..5f4b9da2aa3 100644 --- a/doc/tutorials/build_application.md +++ b/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 diff --git a/doc/user/packages/container_registry/index.md b/doc/user/packages/container_registry/index.md index 1f95d2f9403..786fd0ca658 100644 --- a/doc/user/packages/container_registry/index.md +++ b/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 -/// +//[/] ``` For example, if your project is `gitlab.example.com/mynamespace/myproject`, diff --git a/doc/user/profile/index.md b/doc/user/profile/index.md index f868bef8ece..b55f2411a0a 100644 --- a/doc/user/profile/index.md +++ b/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. diff --git a/doc/user/shortcuts.md b/doc/user/shortcuts.md index 3230d26a5aa..e504ee90821 100644 --- a/doc/user/shortcuts.md +++ b/doc/user/shortcuts.md @@ -135,6 +135,7 @@ These shortcuts are available when browsing the files in a project (go to | Enter | Open selection. | | Escape | Go back to file list screen (only while searching for files, **Code > Repository**, then select **Find File**). | | y | Go to file permalink (only while viewing a file). | +| Shift + c | Go to compare branches view. | | . | Open the [Web IDE](project/web_ide/index.md). | ### Web IDE diff --git a/lib/api/merge_request_approvals.rb b/lib/api/merge_request_approvals.rb index 35fdcfe3ab0..d0c9400039a 100644 --- a/lib/api/merge_request_approvals.rb +++ b/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 diff --git a/lib/gitlab/quick_actions/merge_request_actions.rb b/lib/gitlab/quick_actions/merge_request_actions.rb index 9798b0eca2c..c8dacc5ddd8 100644 --- a/lib/gitlab/quick_actions/merge_request_actions.rb +++ b/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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b01185abfd2..521e52a8592 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -12235,6 +12235,9 @@ msgstr "" msgid "Compare %{oldCommitId}...%{newCommitId}" msgstr "" +msgid "Compare Branches" +msgstr "" + msgid "Compare GitLab editions" msgstr "" diff --git a/package.json b/package.json index c5900e31e33..976c58591df 100644 --- a/package.json +++ b/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", diff --git a/spec/controllers/projects/merge_requests/drafts_controller_spec.rb b/spec/controllers/projects/merge_requests/drafts_controller_spec.rb index 68fbeb00b67..505f9f5b19b 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 @@ require 'spec_helper' 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 @@ 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 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 486e1c4f502..d8de3fcd1d1 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 @@ 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 diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb index a1d6abec97e..df2b20c62c3 100644 --- a/spec/requests/api/merge_request_approvals_spec.rb +++ b/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 diff --git a/spec/services/draft_notes/publish_service_spec.rb b/spec/services/draft_notes/publish_service_spec.rb index e087f2ffc7e..fbc38f93c56 100644 --- a/spec/services/draft_notes/publish_service_spec.rb +++ b/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 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 172c2133168..00000000000 --- 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 00000000000..be24d95d7f1 --- /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 87491515053..c1b15ec7681 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -228,21 +228,35 @@ 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 - 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 2c34d6a59be..d5108bb3988 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/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 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 942cf8e87e9..7341a0dcc5b 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 @@ 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 diff --git a/yarn.lock b/yarn.lock index 0da503168ee..f293cdcead8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1913,14 +1913,14 @@ estree-walker "^2.0.2" picomatch "^2.3.1" -"@sentry-internal/tracing@7.76.0": - version "7.76.0" - resolved "https://registry.yarnpkg.com/@sentry-internal/tracing/-/tracing-7.76.0.tgz#36c54425bc20c08e569e6da52e13d325611cad66" - integrity sha512-QQVIv+LS2sbGf/e5P2dRisHzXpy02dAcLqENLPG4sZ9otRaFNjdFYEqnlJ4qko+ORpJGQEQp/BX7Q/qzZQHlAg== +"@sentry-internal/tracing@7.77.0": + version "7.77.0" + resolved "https://registry.yarnpkg.com/@sentry-internal/tracing/-/tracing-7.77.0.tgz#f3d82486f8934a955b3dd2aa54c8d29586e42a37" + integrity sha512-8HRF1rdqWwtINqGEdx8Iqs9UOP/n8E0vXUu3Nmbqj4p5sQPA7vvCfq+4Y4rTqZFc7sNdFpDsRION5iQEh8zfZw== dependencies: - "@sentry/core" "7.76.0" - "@sentry/types" "7.76.0" - "@sentry/utils" "7.76.0" + "@sentry/core" "7.77.0" + "@sentry/types" "7.77.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": - version "7.76.0" - resolved "https://registry.yarnpkg.com/@sentry/core/-/core-7.76.0.tgz#b0d1dc399a862ea8a1c8a1c60a409e92eaf8e9e1" - integrity sha512-M+ptkCTeCNf6fn7p2MmEb1Wd9/JXUWxIT/0QEc+t11DNR4FYy1ZP2O9Zb3Zp2XacO7ORrlL3Yc+VIfl5JTgjfw== +"@sentry/core@7.77.0": + version "7.77.0" + resolved "https://registry.yarnpkg.com/@sentry/core/-/core-7.77.0.tgz#21100843132beeeff42296c8370cdcc7aa1d8510" + integrity sha512-Tj8oTYFZ/ZD+xW8IGIsU6gcFXD/gfE+FUxUaeSosd9KHwBQNOLhZSsYo/tTVf/rnQI/dQnsd4onPZLiL+27aTg== dependencies: - "@sentry/types" "7.76.0" - "@sentry/utils" "7.76.0" + "@sentry/types" "7.77.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": - version "7.76.0" - resolved "https://registry.yarnpkg.com/@sentry/replay/-/replay-7.76.0.tgz#bccf9ea4a6efc332a79d6a78f923697b9b283371" - integrity sha512-OACT7MfMHC/YGKnKST8SF1d6znr3Yu8fpUpfVVh2t9TNeh3+cQJVTOliHDqLy+k9Ljd5FtitgSn4IHtseCHDLQ== +"@sentry/replay@7.77.0": + version "7.77.0" + resolved "https://registry.yarnpkg.com/@sentry/replay/-/replay-7.77.0.tgz#21d242c9cd70a7235237216174873fd140b6eb80" + integrity sha512-M9Ik2J5ekl+C1Och3wzLRZVaRGK33BlnBwfwf3qKjgLDwfKW+1YkwDfTHbc2b74RowkJbOVNcp4m8ptlehlSaQ== dependencies: - "@sentry-internal/tracing" "7.76.0" - "@sentry/core" "7.76.0" - "@sentry/types" "7.76.0" - "@sentry/utils" "7.76.0" + "@sentry-internal/tracing" "7.77.0" + "@sentry/core" "7.77.0" + "@sentry/types" "7.77.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": - version "7.76.0" - resolved "https://registry.yarnpkg.com/@sentry/types/-/types-7.76.0.tgz#628c9899bfa82ea762708314c50fd82f2138587d" - integrity sha512-vj6z+EAbVrKAXmJPxSv/clpwS9QjPqzkraMFk2hIdE/kii8s8kwnkBwTSpIrNc8GnzV3qYC4r3qD+BXDxAGPaw== +"@sentry/types@7.77.0": + version "7.77.0" + resolved "https://registry.yarnpkg.com/@sentry/types/-/types-7.77.0.tgz#c5d00fe547b89ccde59cdea59143bf145cee3144" + 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": - version "7.76.0" - resolved "https://registry.yarnpkg.com/@sentry/utils/-/utils-7.76.0.tgz#6b540b387d3ac539abd20978f4d3ae235114f6ab" - integrity sha512-40jFD+yfQaKpFYINghdhovzec4IEpB7aAuyH/GtE7E0gLpcqnC72r55krEIVILfqIR2Mlr5OKUzyeoCyWAU/yw== +"@sentry/utils@7.77.0": + version "7.77.0" + resolved "https://registry.yarnpkg.com/@sentry/utils/-/utils-7.77.0.tgz#1f88501f0b8777de31b371cf859d13c82ebe1379" + 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": - version "7.76.0" - resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-7.76.0.tgz#7d73573790023523f7d9c3757b8424b7ad60d664" - integrity sha512-83xA+cWrBhhkNuMllW5ucFsEO2NlUh2iBYtmg07lp3fyVW+6+b1yMKRnc4RFArJ+Wcq6UO+qk2ZEvrSAts1wEw== +"sentrybrowser@npm:@sentry/browser@7.77.0": + version "7.77.0" + resolved "https://registry.yarnpkg.com/@sentry/browser/-/browser-7.77.0.tgz#155440f1a0d3a1bbd5d564c28d6b0c9853a51d72" + integrity sha512-nJ2KDZD90H8jcPx9BysQLiQW+w7k7kISCWeRjrEMJzjtge32dmHA8G4stlUTRIQugy5F+73cOayWShceFP7QJQ== dependencies: - "@sentry-internal/tracing" "7.76.0" - "@sentry/core" "7.76.0" - "@sentry/replay" "7.76.0" - "@sentry/types" "7.76.0" - "@sentry/utils" "7.76.0" + "@sentry-internal/tracing" "7.77.0" + "@sentry/core" "7.77.0" + "@sentry/replay" "7.77.0" + "@sentry/types" "7.77.0" + "@sentry/utils" "7.77.0" serialize-javascript@^2.1.2: version "2.1.2"