Skip to content
Snippets Groups Projects
Verified Commit 7286b1c6 authored by Phil Hughes's avatar Phil Hughes
Browse files

Remove mr_changes_request flag

Changelog: other

#429557
parent 6defcf90
No related branches found
No related tags found
1 merge request!144624Remove mr_changes_request flag
Showing
with 77 additions and 248 deletions
<script>
import {
GlDisclosureDropdown,
GlButton,
GlIcon,
GlForm,
GlFormCheckbox,
GlFormRadioGroup,
} from '@gitlab/ui';
import { GlDisclosureDropdown, GlButton, GlIcon, GlForm, GlFormRadioGroup } from '@gitlab/ui';
// eslint-disable-next-line no-restricted-imports
import { mapGetters, mapActions, mapState } from 'vuex';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { __ } from '~/locale';
import { createAlert } from '~/alert';
import MarkdownEditor from '~/vue_shared/components/markdown/markdown_editor.vue';
......@@ -43,13 +35,11 @@ export default {
GlIcon,
GlForm,
GlFormRadioGroup,
GlFormCheckbox,
MarkdownEditor,
ApprovalPassword: () => import('ee_component/batch_comments/components/approval_password.vue'),
SummarizeMyReview: () =>
import('ee_component/batch_comments/components/summarize_my_review.vue'),
},
mixins: [glFeatureFlagsMixin()],
inject: {
canSummarize: { default: false },
},
......@@ -252,21 +242,11 @@ export default {
/>
</div>
<gl-form-radio-group
v-if="glFeatures.mrRequestChanges"
v-model="noteData.reviewer_state"
:options="radioGroupOptions"
class="gl-mt-4"
data-testid="reviewer_states"
/>
<template v-else-if="userPermissions.canApprove">
<gl-form-checkbox
v-model="noteData.approve"
data-testid="approve_merge_request"
class="gl-mt-4"
>
{{ __('Approve merge request') }}
</gl-form-checkbox>
</template>
<approval-password
v-if="userPermissions.canApprove && getNoteableData.require_password_to_approve"
v-show="noteData.approve || noteData.reviewer_state === 'approved'"
......
......@@ -277,8 +277,6 @@ class GfmAutoComplete {
// eslint-disable-next-line class-methods-use-this
setSubmitReviewStates($input) {
if (!window.gon.features?.mrRequestChanges) return;
const REVIEW_STATES = {
reviewed: {
header: __('Comment'),
......
......@@ -6,7 +6,6 @@ import { createAlert } from '~/alert';
import { TYPE_ISSUE } from '~/issues/constants';
import { __ } from '~/locale';
import { getIdFromGraphQLId } from '~/graphql_shared/utils';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { fetchUserCounts } from '~/super_sidebar/user_counts_fetch';
import eventHub from '../../event_hub';
import getMergeRequestReviewersQuery from '../../queries/get_merge_request_reviewers.query.graphql';
......@@ -27,7 +26,6 @@ export default {
ReviewerTitle,
Reviewers,
},
mixins: [glFeatureFlagsMixin()],
props: {
mediator: {
type: Object,
......@@ -58,7 +56,6 @@ export default {
return {
iid: this.issuableIid,
fullPath: this.projectPath,
mrRequestChanges: this.glFeatures.mrRequestChanges,
};
},
update(data) {
......@@ -77,7 +74,6 @@ export default {
variables() {
return {
issuableId: this.issuable?.id,
mrRequestChanges: this.glFeatures.mrRequestChanges,
};
},
skip() {
......
......@@ -2,7 +2,6 @@
import { GlButton, GlTooltipDirective, GlIcon } from '@gitlab/ui';
import { TYPE_ISSUE } from '~/issues/constants';
import { __, sprintf, s__ } from '~/locale';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import ReviewerAvatarLink from './reviewer_avatar_link.vue';
const LOADING_STATE = 'loading';
......@@ -44,7 +43,6 @@ export default {
directives: {
GlTooltip: GlTooltipDirective,
},
mixins: [glFeatureFlagsMixin()],
props: {
users: {
type: Array,
......@@ -100,14 +98,6 @@ export default {
this.loadingStates[userId] = null;
}, 1500);
},
approveAnimation(userId) {
return {
'merge-request-approved-icon': this.loadingStates[userId] === JUST_APPROVED,
};
},
approvedByTooltipTitle(user) {
return sprintf(s__('MergeRequest|Approved by @%{username}'), user);
},
reviewedButNotApprovedTooltip(user) {
return sprintf(s__('MergeRequest|Reviewed by @%{username} but not yet approved'), user);
},
......@@ -143,7 +133,7 @@ export default {
return REVIEW_STATE_ICONS[user.mergeRequestInteraction.reviewState];
},
showRequestReviewButton(user) {
if (this.glFeatures.mrRequestChanges && !user.mergeRequestInteraction.approved) {
if (!user.mergeRequestInteraction.approved) {
return user.mergeRequestInteraction.reviewState !== 'UNREVIEWED';
}
......@@ -190,42 +180,20 @@ export default {
data-testid="re-request-button"
@click="reRequestReview(user.id)"
/>
<template v-if="glFeatures.mrRequestChanges">
<span
v-gl-tooltip.top.viewport
:title="reviewStateIcon(user).title"
:class="reviewStateIcon(user).class"
class="gl-float-right gl-my-2 gl-ml-auto gl-flex-shrink-0"
>
<gl-icon
:size="reviewStateIcon(user).size || 16"
:name="reviewStateIcon(user).name"
:aria-label="reviewStateIcon(user).title"
data-testid="reviewer-state-icon"
/>
</span>
</template>
<template v-else>
<gl-icon
v-if="user.mergeRequestInteraction.approved"
v-gl-tooltip.left
:size="16"
:title="approvedByTooltipTitle(user)"
name="status-success"
class="gl-float-right gl-my-2 gl-ml-auto gl-text-green-500 gl-flex-shrink-0"
:class="approveAnimation(user.id)"
data-testid="approved"
/>
<span
v-gl-tooltip.top.viewport
:title="reviewStateIcon(user).title"
:class="reviewStateIcon(user).class"
class="gl-float-right gl-my-2 gl-ml-auto gl-flex-shrink-0"
data-testid="reviewer-state-icon-parent"
>
<gl-icon
v-else-if="user.mergeRequestInteraction.reviewed"
v-gl-tooltip.left
:size="16"
:title="reviewedButNotApprovedTooltip(user)"
name="dash-circle"
class="gl-float-right gl-my-2 gl-ml-auto gl-text-gray-400 gl-flex-shrink-0"
data-testid="reviewed-not-approved"
:size="reviewStateIcon(user).size || 16"
:name="reviewStateIcon(user).name"
:aria-label="reviewStateIcon(user).title"
data-testid="reviewer-state-icon"
/>
</template>
</span>
</div>
</div>
</template>
#import "~/graphql_shared/fragments/user.fragment.graphql"
#import "~/graphql_shared/fragments/user_availability.fragment.graphql"
query mergeRequestReviewers($fullPath: ID!, $iid: String!, $mrRequestChanges: Boolean!) {
query mergeRequestReviewers($fullPath: ID!, $iid: String!) {
workspace: project(fullPath: $fullPath) {
id
issuable: mergeRequest(iid: $iid) {
......@@ -14,8 +14,7 @@ query mergeRequestReviewers($fullPath: ID!, $iid: String!, $mrRequestChanges: Bo
canMerge
canUpdate
approved
reviewed @skip(if: $mrRequestChanges)
reviewState @include(if: $mrRequestChanges)
reviewState
}
}
}
......
#import "~/graphql_shared/fragments/user.fragment.graphql"
#import "~/graphql_shared/fragments/user_availability.fragment.graphql"
subscription mergeRequestReviewersUpdated($issuableId: IssuableID!, $mrRequestChanges: Boolean!) {
subscription mergeRequestReviewersUpdated($issuableId: IssuableID!) {
mergeRequestReviewersUpdated(issuableId: $issuableId) {
... on MergeRequest {
id
......@@ -13,8 +13,7 @@ subscription mergeRequestReviewersUpdated($issuableId: IssuableID!, $mrRequestCh
canMerge
canUpdate
approved
reviewed @skip(if: $mrRequestChanges)
reviewState @include(if: $mrRequestChanges)
reviewState
}
}
}
......
......@@ -61,22 +61,7 @@ def publish
merge_request_activity_counter.track_submit_review_comment(user: current_user)
end
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)
.execute(merge_request)
unless success
return render json: { message: _('An error occurred while approving, please try again.') },
status: :internal_server_error
end
end
merge_request_activity_counter.track_submit_review_approve(user: current_user)
end
update_reviewer_state if reviewer_state_params[:reviewer_state]
if result[:status] == :success
head :ok
......@@ -192,6 +177,8 @@ def update_reviewer_state
::MergeRequests::ApprovalService
.new(project: @project, current_user: current_user, params: approve_params)
.execute(merge_request)
merge_request_activity_counter.track_submit_review_approve(user: current_user)
else
::MergeRequests::UpdateReviewerStateService
.new(project: @project, current_user: current_user)
......
......@@ -44,7 +44,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
push_frontend_feature_flag(:ci_job_failures_in_mr, project)
push_frontend_feature_flag(:mr_pipelines_graphql, project)
push_frontend_feature_flag(:notifications_todos_buttons, current_user)
push_frontend_feature_flag(:mr_request_changes, current_user)
push_frontend_feature_flag(:merge_blocked_component, current_user)
push_frontend_feature_flag(:mention_autocomplete_backend_filtering, project)
push_frontend_feature_flag(:pinned_file, project)
......
......@@ -45,7 +45,6 @@ def publish_draft_notes
capture_diff_note_positions(created_notes)
keep_around_commits(created_notes)
draft_notes.delete_all
set_reviewed
notification_service.async.new_review(review)
MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request)
GraphqlTriggers.merge_request_merge_status_updated(merge_request)
......@@ -60,8 +59,7 @@ def create_note_from_draft(draft, skip_capture_diff_note_position: false, skip_k
note_params = draft.publish_params.merge(skip_keep_around_commits: skip_keep_around_commits)
note = Notes::CreateService.new(draft.project, draft.author, note_params).execute(
skip_capture_diff_note_position: skip_capture_diff_note_position,
skip_merge_status_trigger: skip_merge_status_trigger,
skip_set_reviewed: true
skip_merge_status_trigger: skip_merge_status_trigger
)
set_discussion_resolve_status(note, draft)
......@@ -80,12 +78,6 @@ def set_discussion_resolve_status(note, draft_note)
end
end
def set_reviewed
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)
paths = notes.flat_map do |note|
note.diff_file&.paths if note.diff_note?
......
......@@ -14,7 +14,7 @@ def execute(merge_request, user)
trigger_merge_request_reviewers_updated(merge_request)
create_system_note(merge_request, user)
remove_approval(merge_request) if Feature.enabled?(:mr_request_changes, current_user)
remove_approval(merge_request)
success
else
......
......@@ -4,7 +4,7 @@ module Notes
class CreateService < ::Notes::BaseService
include IncidentManagement::UsageData
def execute(skip_capture_diff_note_position: false, skip_merge_status_trigger: false, skip_set_reviewed: false)
def execute(skip_capture_diff_note_position: false, skip_merge_status_trigger: false)
note = Notes::BuildService.new(project, current_user, params.except(:merge_request_diff_head_sha)).execute
# n+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/37440
......@@ -40,8 +40,7 @@ def execute(skip_capture_diff_note_position: false, skip_merge_status_trigger: f
when_saved(
note,
skip_capture_diff_note_position: skip_capture_diff_note_position,
skip_merge_status_trigger: skip_merge_status_trigger,
skip_set_reviewed: skip_set_reviewed
skip_merge_status_trigger: skip_merge_status_trigger
)
end
end
......@@ -83,8 +82,7 @@ def update_discussions(note)
end
def when_saved(
note, skip_capture_diff_note_position: false, skip_merge_status_trigger: false,
skip_set_reviewed: false)
note, skip_capture_diff_note_position: false, skip_merge_status_trigger: false)
todo_service.new_note(note, current_user)
clear_noteable_diffs_cache(note)
Suggestions::CreateService.new(note).execute
......@@ -92,8 +90,6 @@ def when_saved(
track_event(note, current_user)
if note.for_merge_request? && note.start_of_discussion?
set_reviewed(note) unless skip_set_reviewed
if !skip_capture_diff_note_position && note.diff_note?
Discussions::CaptureDiffNotePositionService.new(note.noteable, note.diff_file&.paths).execute(note.discussion)
end
......@@ -224,13 +220,6 @@ def track_note_creation_in_ipynb(note)
def track_note_creation_visual_review(note)
Gitlab::Tracking.event('Notes::CreateService', 'execute', **tracking_data_for(note))
end
def set_reviewed(note)
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
......
......@@ -3477,15 +3477,6 @@
:weight: 1
:idempotent: true
:tags: []
- :name: merge_requests_set_reviewer_reviewed
:worker_name: MergeRequests::SetReviewerReviewedWorker
:feature_category: :code_review_workflow
:has_external_dependencies: false
:urgency: :low
:resource_boundary: :unknown
:weight: 1
:idempotent: true
:tags: []
- :name: merge_requests_update_head_pipeline
:worker_name: MergeRequests::UpdateHeadPipelineWorker
:feature_category: :code_review_workflow
......
# frozen_string_literal: true
module MergeRequests
class SetReviewerReviewedWorker
include Gitlab::EventStore::Subscriber
data_consistency :always
feature_category :code_review_workflow
urgency :low
idempotent!
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)
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)
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
---
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: true
......@@ -483,8 +483,6 @@
- 1
- - merge_requests_resolve_todos_after_approval
- 1
- - merge_requests_set_reviewer_reviewed
- 1
- - merge_requests_stream_approval_audit_event
- 1
- - merge_requests_sync_code_owner_approval_rules
......
......@@ -44,9 +44,6 @@ def self.configure!(store)
store.subscribe ::MergeRequests::CreateApprovalNoteWorker, to: ::MergeRequests::ApprovedEvent
store.subscribe ::MergeRequests::ResolveTodosAfterApprovalWorker, to: ::MergeRequests::ApprovedEvent
store.subscribe ::MergeRequests::ExecuteApprovalHooksWorker, to: ::MergeRequests::ApprovedEvent
store.subscribe ::MergeRequests::SetReviewerReviewedWorker,
to: ::MergeRequests::ApprovedEvent,
if: -> (event) { ::Feature.disabled?(:mr_request_changes, User.find_by_id(event.data[:current_user_id])) }
store.subscribe ::Ml::ExperimentTracking::AssociateMlCandidateToPackageWorker,
to: ::Packages::PackageCreatedEvent,
if: -> (event) { ::Ml::ExperimentTracking::AssociateMlCandidateToPackageWorker.handles_event?(event) }
......
......@@ -174,18 +174,16 @@ module MergeRequestActions
result = DraftNotes::PublishService.new(quick_action_target, current_user).execute
if Feature.enabled?(:mr_request_changes, current_user)
reviewer_state = state.strip.presence
if reviewer_state === 'approve'
::MergeRequests::ApprovalService
.new(project: quick_action_target.project, current_user: current_user)
.execute(quick_action_target)
elsif MergeRequestReviewer.states.key?(reviewer_state)
::MergeRequests::UpdateReviewerStateService
.new(project: quick_action_target.project, current_user: current_user)
.execute(quick_action_target, reviewer_state)
end
reviewer_state = state.strip.presence
if reviewer_state === 'approve'
::MergeRequests::ApprovalService
.new(project: quick_action_target.project, current_user: current_user)
.execute(quick_action_target)
elsif MergeRequestReviewer.states.key?(reviewer_state)
::MergeRequests::UpdateReviewerStateService
.new(project: quick_action_target.project, current_user: current_user)
.execute(quick_action_target, reviewer_state)
end
@execution_message[:submit_review] = if result[:status] == :success
......@@ -199,8 +197,7 @@ module MergeRequestActions
explanation { _('Request changes to the current merge request.') }
types MergeRequest
condition do
Feature.enabled?(:mr_request_changes, current_user) &&
quick_action_target.persisted? &&
quick_action_target.persisted? &&
quick_action_target.find_reviewer(current_user)
end
command :request_changes do
......
......@@ -5081,9 +5081,6 @@ msgstr ""
msgid "An error occurred while adding formatted title for epic"
msgstr ""
 
msgid "An error occurred while approving, please try again."
msgstr ""
msgid "An error occurred while checking group path. Please refresh and try again."
msgstr ""
 
......@@ -6400,9 +6397,6 @@ msgid_plural "Approve %d pending members"
msgstr[0] ""
msgstr[1] ""
 
msgid "Approve merge request"
msgstr ""
msgid "Approve the current merge request."
msgstr ""
 
......@@ -30903,9 +30897,6 @@ msgstr ""
msgid "MergeRequests|started a thread on commit %{linkStart}%{commitDisplay}%{linkEnd}"
msgstr ""
 
msgid "MergeRequest|Approved by @%{username}"
msgstr ""
msgid "MergeRequest|Can't fetch the diff needed to update this view. Please reload this page."
msgstr ""
 
......@@ -499,19 +499,19 @@ def create_reply(discussion_id, resolves: false)
end
it 'approves merge request' do
post :publish, params: params.merge!(approve: true)
post :publish, params: params.merge!(reviewer_state: 'approved')
expect(merge_request.approvals.reload.size).to be(1)
end
it 'does not approve merge request' do
post :publish, params: params.merge!(approve: false)
post :publish, params: params.merge!(reviewer_state: 'reviewed')
expect(merge_request.approvals.reload.size).to be(0)
end
it 'tracks merge request activity' do
post :publish, params: params.merge!(approve: true)
post :publish, params: params.merge!(reviewer_state: 'approved')
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to have_received(:track_submit_review_approve).with(user: user)
......@@ -523,7 +523,7 @@ def create_reply(discussion_id, resolves: false)
end
it 'does return 200' do
post :publish, params: params.merge!(approve: true)
post :publish, params: params.merge!(reviewer_state: 'approved')
expect(response).to have_gitlab_http_status(:ok)
......
......@@ -19,11 +19,7 @@ let wrapper;
let publishReview;
let trackingSpy;
function factory({
canApprove = true,
shouldAnimateReviewButton = false,
mrRequestChanges = false,
} = {}) {
function factory({ canApprove = true, shouldAnimateReviewButton = false } = {}) {
publishReview = jest.fn();
trackingSpy = mockTracking(undefined, null, jest.spyOn);
const requestHandlers = [
......@@ -79,9 +75,6 @@ function factory({
wrapper = mountExtended(SubmitDropdown, {
store,
apolloProvider,
provide: {
glFeatures: { mrRequestChanges },
},
});
}
......@@ -149,9 +142,9 @@ describe('Batch comments submit dropdown', () => {
});
it.each`
canApprove | exists | existsText
${true} | ${true} | ${'shows'}
${false} | ${false} | ${'hides'}
canApprove | exists | existsText
${true} | ${undefined} | ${'shows'}
${false} | ${'disabled'} | ${'hides'}
`(
'$existsText approve checkbox if can_approve is $canApprove',
async ({ canApprove, exists }) => {
......@@ -161,7 +154,7 @@ describe('Batch comments submit dropdown', () => {
await waitForPromises();
expect(wrapper.findByTestId('approve_merge_request').exists()).toBe(exists);
expect(wrapper.findAll('input').at(1).attributes('disabled')).toBe(exists);
},
);
......@@ -180,51 +173,49 @@ describe('Batch comments submit dropdown', () => {
},
);
describe('when mrRequestChanges feature flag is enabled', () => {
it('renders a radio group with review state options', async () => {
factory({ mrRequestChanges: true });
it('renders a radio group with review state options', async () => {
factory();
await waitForPromises();
await waitForPromises();
expect(wrapper.findAll('.gl-form-radio').length).toBe(3);
});
expect(wrapper.findAll('.gl-form-radio').length).toBe(3);
});
it('renders disabled approve radio button when user can not approve', async () => {
factory({ mrRequestChanges: true, canApprove: false });
it('renders disabled approve radio button when user can not approve', async () => {
factory({ mrRequestChanges: true, canApprove: false });
wrapper.findComponent(GlDisclosureDropdown).vm.$emit('shown');
wrapper.findComponent(GlDisclosureDropdown).vm.$emit('shown');
await waitForPromises();
await waitForPromises();
expect(wrapper.find('.custom-control-input[value="approved"]').attributes('disabled')).toBe(
'disabled',
);
});
expect(wrapper.find('.custom-control-input[value="approved"]').attributes('disabled')).toBe(
'disabled',
);
});
it.each`
value
${'approved'}
${'reviewed'}
${'requested_changes'}
`('sends $value review state to api when submitting', async ({ value }) => {
factory({ mrRequestChanges: true });
it.each`
value
${'approved'}
${'reviewed'}
${'requested_changes'}
`('sends $value review state to api when submitting', async ({ value }) => {
factory();
wrapper.findComponent(GlDisclosureDropdown).vm.$emit('shown');
wrapper.findComponent(GlDisclosureDropdown).vm.$emit('shown');
await waitForPromises();
await waitForPromises();
await wrapper.find(`.custom-control-input[value="${value}"]`).trigger('change');
await wrapper.find(`.custom-control-input[value="${value}"]`).trigger('change');
findForm().vm.$emit('submit', { preventDefault: jest.fn() });
findForm().vm.$emit('submit', { preventDefault: jest.fn() });
expect(publishReview).toHaveBeenCalledWith(expect.anything(), {
noteable_type: 'merge_request',
noteable_id: 1,
note: 'Hello world',
approve: false,
approval_password: '',
reviewer_state: value,
});
expect(publishReview).toHaveBeenCalledWith(expect.anything(), {
noteable_type: 'merge_request',
noteable_id: 1,
note: 'Hello world',
approve: false,
approval_password: '',
reviewer_state: value,
});
});
});
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment