Commit 690c47d0 authored by David Kim's avatar David Kim Committed by Marcia Ramos

[RUN AS-IF-FOSS] Remove merge_request_reviewers feature flag [RUN ALL RSPEC]

parent 26043c95
......@@ -12,7 +12,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
before_action :build_merge_request, except: [:create]
before_action do
push_frontend_feature_flag(:merge_request_reviewers, @project, default_enabled: true)
push_frontend_feature_flag(:mr_collapsed_approval_rules, @project)
push_frontend_feature_flag(:reviewer_approval_rules, @project, default_enabled: :yaml)
end
......
......@@ -50,7 +50,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
end
before_action do
push_frontend_feature_flag(:merge_request_reviewers, @project, default_enabled: true)
push_frontend_feature_flag(:mr_collapsed_approval_rules, @project)
push_frontend_feature_flag(:reviewer_approval_rules, @project, default_enabled: :yaml)
end
......
......@@ -243,7 +243,7 @@ module Types
end
def reviewers
object.reviewers if object.allows_reviewers?
object.reviewers
end
end
end
......
......@@ -174,15 +174,9 @@ module MergeRequestsHelper
end
end
def merge_request_reviewers_enabled?
Feature.enabled?(:merge_request_reviewers, default_enabled: :yaml)
end
private
def review_requested_merge_requests_count
return 0 unless merge_request_reviewers_enabled?
current_user.review_requested_open_merge_requests_count
end
......
......@@ -1784,7 +1784,7 @@ class MergeRequest < ApplicationRecord
end
def allows_reviewers?
Feature.enabled?(:merge_request_reviewers, project, default_enabled: true)
true
end
def allows_multiple_reviewers?
......
......@@ -10,7 +10,7 @@ class MergeRequestBasicEntity < Grape::Entity
expose :milestone, using: API::Entities::Milestone
expose :labels, using: LabelEntity
expose :assignees, using: API::Entities::UserBasic
expose :reviewers, if: -> (m) { m.allows_reviewers? }, using: API::Entities::UserBasic
expose :reviewers, using: API::Entities::UserBasic
expose :task_status, :task_status_short
expose :lock_version, :lock_version
end
......@@ -5,7 +5,7 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity
MergeRequestUserEntity.represent(merge_request.assignees, options.merge(merge_request: merge_request))
end
expose :reviewers, if: -> (m) { m.allows_reviewers? } do |merge_request, options|
expose :reviewers do |merge_request, options|
MergeRequestUserEntity.represent(merge_request.reviewers, options.merge(merge_request: merge_request))
end
end
......@@ -108,7 +108,7 @@ module MergeRequests
def filter_reviewer(merge_request)
return if params[:reviewer_ids].blank?
unless can_admin_issuable?(merge_request) && merge_request.allows_reviewers?
unless can_admin_issuable?(merge_request)
params.delete(:reviewer_ids)
return
......
......@@ -47,11 +47,10 @@
%span.badge.badge-pill.issues-count.green-badge{ class: ('hidden' if issues_count == 0) }
= number_with_delimiter(issues_count)
- if header_link?(:merge_requests)
- reviewers_enabled = merge_request_reviewers_enabled?
= nav_link(path: 'dashboard#merge_requests', html_options: { class: "user-counter #{reviewers_enabled ? 'dropdown' : ''}" }) do
= nav_link(path: 'dashboard#merge_requests', html_options: { class: "user-counter dropdown" }) do
= link_to assigned_mrs_dashboard_path, class: 'dashboard-shortcuts-merge_requests', title: _('Merge requests'), aria: { label: _('Merge requests') },
data: { qa_selector: 'merge_requests_shortcut_button',
toggle: reviewers_enabled ? "dropdown" : "tooltip",
toggle: "dropdown",
placement: 'bottom',
track_label: 'main_navigation',
track_event: 'click_merge_link',
......@@ -60,23 +59,21 @@
= sprite_icon('git-merge')
%span.badge.badge-pill.merge-requests-count.js-merge-requests-count{ class: ('hidden' if user_merge_requests_counts[:total] == 0) }
= number_with_delimiter(user_merge_requests_counts[:total])
- if reviewers_enabled
= sprite_icon('chevron-down', css_class: 'caret-down gl-mx-0!')
- if reviewers_enabled
.dropdown-menu.dropdown-menu-right
%ul
%li.dropdown-header
= _('Merge requests')
%li
= link_to assigned_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center' do
= _('Assigned to you')
%span.badge.gl-badge.badge-pill.badge-muted.merge-request-badge.gl-ml-auto.js-assigned-mr-count{ class: "" }
= user_merge_requests_counts[:assigned]
%li
= link_to reviewer_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center' do
= _('Review requests for you')
%span.badge.gl-badge.badge-pill.badge-muted.merge-request-badge.gl-ml-auto.js-reviewer-mr-count{ class: "" }
= user_merge_requests_counts[:review_requested]
= sprite_icon('chevron-down', css_class: 'caret-down gl-mx-0!')
.dropdown-menu.dropdown-menu-right
%ul
%li.dropdown-header
= _('Merge requests')
%li
= link_to assigned_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center' do
= _('Assigned to you')
%span.badge.gl-badge.badge-pill.badge-muted.merge-request-badge.gl-ml-auto.js-assigned-mr-count{ class: "" }
= user_merge_requests_counts[:assigned]
%li
= link_to reviewer_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center' do
= _('Review requests for you')
%span.badge.gl-badge.badge-pill.badge-muted.merge-request-badge.gl-ml-auto.js-reviewer-mr-count{ class: "" }
= user_merge_requests_counts[:review_requested]
- if header_link?(:todos)
= nav_link(controller: 'dashboard/todos', html_options: { class: "user-counter" }) do
= link_to dashboard_todos_path, title: _('To-Do List'), aria: { label: _('To-Do List') }, class: 'shortcuts-todos',
......
......@@ -55,7 +55,7 @@
- if merge_request.assignees.any?
%li.gl-display-flex.gl-align-items-center
= render 'shared/issuable/assignees', project: merge_request.project, issuable: merge_request
- if Feature.enabled?(:merge_request_reviewers, @project, default_enabled: true) && merge_request.reviewers.any?
- if merge_request.reviewers.any?
%li.gl-display-flex.issuable-reviewers
= render 'shared/issuable/reviewers', project: merge_request.project, issuable: merge_request
= render 'projects/merge_requests/approvals_count', merge_request: merge_request
......
......@@ -26,7 +26,7 @@
.block.assignee.qa-assignee-block
= render "shared/issuable/sidebar_assignees", issuable_sidebar: issuable_sidebar, assignees: assignees, signed_in: signed_in
- if Feature.enabled?(:merge_request_reviewers, @project, default_enabled: true) && reviewers
- if reviewers
.block.reviewer.qa-reviewer-block
= render "shared/issuable/sidebar_reviewers", issuable_sidebar: issuable_sidebar, reviewers: reviewers, signed_in: signed_in
......
---
title: Remove merge_request_reviewers feature flag
merge_request: 52468
author:
type: removed
---
name: merge_request_reviewers
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40488
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/245190
milestone: '13.4'
type: development
group: group::code review
default_enabled: true
......@@ -454,10 +454,10 @@ Parameters:
| `author_id` | integer | no | Returns merge requests created by the given user `id`. Mutually exclusive with `author_username`. _([Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/13060) in GitLab 9.5)_. |
| `author_username` | string | no | Returns merge requests created by the given `username`. Mutually exclusive with `author_id`. _([Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/13060) in GitLab 12.10)_. |
| `assignee_id` | integer | no | Returns merge requests assigned to the given user `id`. `None` returns unassigned merge requests. `Any` returns merge requests with an assignee. _([Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/13060) in GitLab 9.5)_. |
| `approver_ids` **(PREMIUM)** | integer array | no | Returns merge requests which have specified all the users with the given `id`s as individual approvers. `None` returns merge requests without approvers. `Any` returns merge requests with an approver. |
| `approver_ids` **(PREMIUM))** | integer array | no | Returns merge requests which have specified all the users with the given `id`s as individual approvers. `None` returns merge requests without approvers. `Any` returns merge requests with an approver. |
| `approved_by_ids` **(PREMIUM)** | integer array | no | Returns merge requests which have been approved by all the users with the given `id`s (Max: 5). `None` returns merge requests with no approvals. `Any` returns merge requests with an approval. |
| `reviewer_id` | integer | no | Returns merge requests which have the user as a [reviewer](../user/project/merge_requests/getting_started.md#enable-or-disable-merge-request-reviewers) with the given user `id`. `None` returns merge requests with no reviewers. `Any` returns merge requests with any reviewer. Mutually exclusive with `reviewer_username`. |
| `reviewer_username` | string | no | Returns merge requests which have the user as a [reviewer](../user/project/merge_requests/getting_started.md#enable-or-disable-merge-request-reviewers) with the given `username`. `None` returns merge requests with no reviewers. `Any` returns merge requests with any reviewer. Mutually exclusive with `reviewer_id`. |
| `reviewer_id` | integer | no | Returns merge requests which have the user as a [reviewer](../user/project/merge_requests/getting_started.md#reviewer) with the given user `id`. `None` returns merge requests with no reviewers. `Any` returns merge requests with any reviewer. Mutually exclusive with `reviewer_username`. |
| `reviewer_username` | string | no | Returns merge requests which have the user as a [reviewer](../user/project/merge_requests/getting_started.md#reviewer) with the given `username`. `None` returns merge requests with no reviewers. `Any` returns merge requests with any reviewer. Mutually exclusive with `reviewer_id`. |
| `my_reaction_emoji` | string | no | Return merge requests reacted by the authenticated user by the given `emoji`. `None` returns issues not given a reaction. `Any` returns issues given at least one reaction. _([Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/14016) in GitLab 10.0)_. |
| `source_branch` | string | no | Return merge requests with the given source branch. |
| `target_branch` | string | no | Return merge requests with the given target branch. |
......
......@@ -115,16 +115,9 @@ It is also possible to manage multiple assignees:
### Reviewer
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/216054) in GitLab 13.5.
> - It was [deployed behind a feature flag](../../../user/feature_flags.md), disabled by default.
> - [Became enabled by default](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49787) on GitLab 13.7.
> - It's enabled on GitLab.com.
> - It's recommended for production use.
> - It can be enabled or disabled for a single project.
> - For GitLab self-managed instances, GitLab administrators can opt to [disable it](#enable-or-disable-merge-request-reviewers). **(FREE SELF)**
> - [Feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/245190) in GitLab 13.9.
WARNING:
This feature might not be available to you. Check the **version history** note above for details.
Requesting a code review is an important part of contributing code. However, deciding who should review
your code and asking for a review are no easy tasks. Using the "assignee" field for both authors and
reviewers makes it hard for others to determine who's doing what on a merge request.
......@@ -137,31 +130,6 @@ This makes it easy to determine the relevant roles for the users involved in the
To request it, open the **Reviewers** drop-down box to search for the user you wish to get a review from.
#### Enable or disable Merge Request Reviewers **(FREE SELF)**
Merge Request Reviewers is under development but ready for production use.
It is deployed behind a feature flag that is **enabled by default**.
[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md)
can opt to disable it.
To enable it:
```ruby
# For the instance
Feature.enable(:merge_request_reviewers)
# For a single project
Feature.enable(:merge_request_reviewers, Project.find(<project id>))
```
To disable it:
```ruby
# For the instance
Feature.disable(:merge_request_reviewers)
# For a single project
Feature.disable(:merge_request_reviewers, Project.find(<project id>))
```
#### Approval Rule information for Reviewers **(PREMIUM)**
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/233736) in GitLab 13.8.
......
......@@ -28,7 +28,7 @@ export default {
return s__('ApprovalRule|Approval rules');
},
isCollapseFeatureEnabled() {
return this.glFeatures.mergeRequestReviewers && this.glFeatures.mrCollapsedApprovalRules;
return this.glFeatures.mrCollapsedApprovalRules;
},
hasOptionalRules() {
return this.rules.every((r) => r.approvalsRequired === 0);
......
......@@ -3,7 +3,7 @@
- return unless issuable.is_a?(MergeRequest)
- return unless issuable.approval_feature_available?
- if !Feature.enabled?(:merge_request_reviewers, @project, default_enabled: true) || !Feature.enabled?(:mr_collapsed_approval_rules, @project)
- if !Feature.enabled?(:mr_collapsed_approval_rules, @project)
.form-group.row
.col-sm-2.col-form-label
= form.label :approver_ids, "Approval rules"
......
......@@ -15,7 +15,6 @@ module EE
types MergeRequest
condition do
quick_action_target.allows_multiple_reviewers? &&
::Feature.enabled?(:merge_request_reviewers, project, default_enabled: :yaml) &&
quick_action_target.persisted? &&
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
end
......
......@@ -14,8 +14,8 @@ RSpec.describe 'Merge request > User sets approvers', :js do
context 'with feature flag off' do
let!(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
def visit_mr(merge_request_reviewers: false, mr_collapsed_approval_rules: false)
stub_feature_flags(merge_request_reviewers: merge_request_reviewers, mr_collapsed_approval_rules: mr_collapsed_approval_rules)
def visit_mr(mr_collapsed_approval_rules: false)
stub_feature_flags(mr_collapsed_approval_rules: mr_collapsed_approval_rules)
project.add_developer(user)
sign_in(user)
visit edit_project_merge_request_path(project, merge_request)
......@@ -25,18 +25,8 @@ RSpec.describe 'Merge request > User sets approvers', :js do
expect(page).to have_button('Add approval rule')
end
it 'does not hide approval rules inside collapse when only merge_request_reviewers is off' do
visit_mr(merge_request_reviewers: false, mr_collapsed_approval_rules: true)
non_collapse_approval_rules
end
it 'does not hide approval rules inside collapse when mr_collapsed_approval_rules is off' do
visit_mr(merge_request_reviewers: true, mr_collapsed_approval_rules: false)
non_collapse_approval_rules
end
it 'does not hide approval rules inside collapse when merge_request_reviewers and mr_collapsed_approval_rules are off' do
visit_mr(merge_request_reviewers: false, mr_collapsed_approval_rules: false)
visit_mr(mr_collapsed_approval_rules: false)
non_collapse_approval_rules
end
end
......
......@@ -16,13 +16,12 @@ describe('EE Approvals MREditApp', () => {
let store;
let axiosMock;
const factory = (mergeRequestReviewers = false, mrCollapsedApprovalRules = false) => {
const factory = (mrCollapsedApprovalRules = false) => {
wrapper = mount(MREditApp, {
localVue,
store: new Vuex.Store(store),
provide: {
glFeatures: {
mergeRequestReviewers,
mrCollapsedApprovalRules,
},
},
......
......@@ -276,38 +276,24 @@ RSpec.describe QuickActions::InterpretService do
context 'reassign_reviewer command' do
let(:content) { "/reassign_reviewer @#{current_user.username}" }
context "if the 'merge_request_reviewers' feature flag is on" do
context 'unlicensed' do
before do
stub_licensed_features(multiple_merge_request_reviewers: false)
end
it 'does not recognize /reassign_reviewer @user' do
content = "/reassign_reviewer @#{current_user.username}"
_, updates = service.execute(content, merge_request)
expect(updates).to be_empty
end
end
it 'reassigns reviewer if content contains /reassign_reviewer @user' do
_, updates = service.execute("/reassign_reviewer @#{current_user.username}", merge_request)
expect(updates[:reviewer_ids]).to match_array([current_user.id])
end
end
context "if the 'merge_request_reviewers' feature flag is off" do
context 'unlicensed' do
before do
stub_feature_flags(merge_request_reviewers: false)
stub_licensed_features(multiple_merge_request_reviewers: false)
end
it 'does not recognize /reassign_reviewer @user' do
content = "/reassign_reviewer @#{current_user.username}"
_, updates = service.execute(content, merge_request)
expect(updates).to be_empty
end
end
it 'reassigns reviewer if content contains /reassign_reviewer @user' do
_, updates = service.execute("/reassign_reviewer @#{current_user.username}", merge_request)
expect(updates[:reviewer_ids]).to match_array([current_user.id])
end
end
context 'iteration command' do
......
......@@ -27,7 +27,7 @@ module API
expose(:downvotes) { |merge_request, options| issuable_metadata.downvotes }
expose :author, :assignees, :assignee, using: Entities::UserBasic
expose :reviewers, if: -> (merge_request, _) { merge_request.allows_reviewers? }, using: Entities::UserBasic
expose :reviewers, using: Entities::UserBasic
expose :source_project_id, :target_project_id
expose :labels do |merge_request, options|
if options[:with_labels_details]
......
......@@ -181,8 +181,7 @@ module Gitlab
end
types MergeRequest
condition do
Feature.enabled?(:merge_request_reviewers, project, default_enabled: :yaml) &&
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
end
parse_params do |reviewer_param|
extract_users(reviewer_param)
......@@ -221,7 +220,6 @@ module Gitlab
types MergeRequest
condition do
quick_action_target.persisted? &&
Feature.enabled?(:merge_request_reviewers, project, default_enabled: :yaml) &&
quick_action_target.reviewers.any? &&
current_user.can?(:"admin_#{quick_action_target.to_ability_name}", project)
end
......
......@@ -20,14 +20,4 @@ RSpec.describe 'Merge request > User edits MR' do
include_context 'merge request edit context'
it_behaves_like 'an editable merge request'
end
context 'when merge_request_reviewers is turned off' do
before do
stub_feature_flags(merge_request_reviewers: false)
end
it 'does not render reviewers dropdown' do
expect(page).not_to have_selector('.js-reviewer-search')
end
end
end
......@@ -30,19 +30,6 @@ RSpec.describe 'User views an open merge request' do
end
end
context 'when merge_request_reviewers is turned off' do
let(:project) { create(:project, :public, :repository) }
before do
stub_feature_flags(merge_request_reviewers: false)
visit(merge_request_path(merge_request))
end
it 'has reviewers in sidebar' do
expect(page).not_to have_css('.reviewer')
end
end
context 'when a merge request has repository', :js do
let(:project) { create(:project, :public, :repository) }
......
......@@ -40,9 +40,8 @@ RSpec.describe 'Merge requests > User lists merge requests' do
updated_at: 10.seconds.ago)
end
context 'when merge_request_reviewers is turned on' do
context 'merge request reviewers' do
before do
stub_feature_flags(merge_request_reviewers: true)
visit_merge_requests(project, reviewer_id: user.id)
end
......@@ -62,15 +61,6 @@ RSpec.describe 'Merge requests > User lists merge requests' do
end
end
context 'when merge_request_reviewers is turned false' do
it 'has no reviewers in MR list' do
stub_feature_flags(merge_request_reviewers: false)
visit_merge_requests(project, reviewer_id: user.id)
expect(page).not_to have_css('.issuable-reviewers')
end
end
it 'filters on no assignee' do
visit_merge_requests(project, assignee_id: IssuableFinder::Params::FILTER_NONE)
......
......@@ -89,15 +89,5 @@ RSpec.describe MergeRequestsHelper do
total: user.assigned_open_merge_requests_count + user.review_requested_open_merge_requests_count
)
end
context 'when merge_request_reviewers is disabled' do
before do
stub_feature_flags(merge_request_reviewers: false)
end
it 'returns review_requested as 0' do
expect(subject[:review_requested]).to eq(0)
end
end
end
end
......@@ -42,29 +42,14 @@ RSpec.describe ::API::Entities::MergeRequestBasic do
end
context 'reviewers' do
context "when merge_request_reviewers FF is enabled" do
before do
stub_feature_flags(merge_request_reviewers: true)
merge_request.reviewers = [user]
end
it 'includes assigned reviewers' do
result = Gitlab::Json.parse(present(merge_request).to_json)
expect(result['reviewers'][0]['username']).to eq user.username
end
before do
merge_request.reviewers = [user]
end
context "when merge_request_reviewers FF is disabled" do
before do
stub_feature_flags(merge_request_reviewers: false)
end
it 'does not include reviewers' do
result = Gitlab::Json.parse(present(merge_request).to_json)
it 'includes assigned reviewers' do
result = Gitlab::Json.parse(present(merge_request).to_json)
expect(result.keys).not_to include('reviewers')
end
expect(result['reviewers'][0]['username']).to eq user.username
end
end
end
......@@ -1244,9 +1244,7 @@ RSpec.describe Issue do
end
describe '#allows_reviewers?' do
it 'returns false as issues do not support reviewers feature' do
stub_feature_flags(merge_request_reviewers: true)
it 'returns false as we do not support reviewers on issues yet' do
issue = build_stubbed(:issue)
expect(issue.allows_reviewers?).to be(false)
......
......@@ -4567,17 +4567,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
end
describe '#allows_reviewers?' do
it 'returns false without merge_request_reviewers feature' do
stub_feature_flags(merge_request_reviewers: false)
merge_request = build_stubbed(:merge_request)
expect(merge_request.allows_reviewers?).to be(false)
end
it 'returns true with merge_request_reviewers feature' do
stub_feature_flags(merge_request_reviewers: true)
it 'returns true' do
merge_request = build_stubbed(:merge_request)
expect(merge_request.allows_reviewers?).to be(true)
......
......@@ -66,14 +66,6 @@ RSpec.describe 'getting merge request information nested in a project' do
expect(graphql_data_at(:project, :merge_request, :reviewers, :nodes)).to match_array(expected)
expect(graphql_data_at(:project, :merge_request, :participants, :nodes)).to include(*expected)
end
it 'suppresses reviewers if reviewers are not allowed' do
stub_feature_flags(merge_request_reviewers: false)
post_graphql(query, current_user: current_user)
expect(graphql_data_at(:project, :merge_request, :reviewers)).to be_nil
end
end
it 'includes diff stats' do
......
......@@ -265,18 +265,6 @@ RSpec.describe 'getting merge request listings nested in a project' do
})
end
context 'the feature flag is disabled' do
before do
stub_feature_flags(merge_request_reviewers: false)
end
it 'does not return reviewers' do
execute_query
expect(results).to all(match a_hash_including('reviewers' => be_nil))
end
end
include_examples 'N+1 query check'
end
end
......
......@@ -20,23 +20,11 @@ RSpec.describe MergeRequestBasicEntity do
let(:params) { { reviewers: [reviewer] } }
let(:reviewer) { build(:user) }
context 'when merge_request_reviewers feature is disabled' do
it 'does not contain assignees attributes' do
stub_feature_flags(merge_request_reviewers: false)
expect(subject[:reviewers]).to be_nil
end
end
context 'when merge_request_reviewers feature is enabled' do
it 'contains reviewers attributes' do
stub_feature_flags(merge_request_reviewers: true)
expect(subject[:reviewers].count).to be 1
expect(subject[:reviewers].first.keys).to include(
:id, :name, :username, :state, :avatar_url, :web_url
)
end
it 'contains reviewers attributes' do
expect(subject[:reviewers].count).to be 1
expect(subject[:reviewers].first.keys).to include(
:id, :name, :username, :state, :avatar_url, :web_url
)
end
end
end
......@@ -35,23 +35,11 @@ RSpec.describe MergeRequestSidebarExtrasEntity do
end
describe '#reviewers' do
context 'when merge_request_reviewers feature is disabled' do
it 'does not contain reviewers attributes' do
stub_feature_flags(merge_request_reviewers: false)
expect(entity[:reviewers]).to be_nil
end
end
context 'when merge_request_reviewers feature is enabled' do
it 'contains reviewers attributes' do
stub_feature_flags(merge_request_reviewers: true)
expect(entity[:reviewers].count).to be 1
expect(entity[:reviewers].first.keys).to include(
:id, :name, :username, :state, :avatar_url, :web_url, :can_merge
)
end
it 'contains reviewers attributes' do
expect(entity[:reviewers].count).to be 1
expect(entity[:reviewers].first.keys).to include(
:id, :name, :username, :state, :avatar_url, :web_url, :can_merge
)
end
end
end
......@@ -142,44 +142,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
context 'with reviewers' do
let(:opts) { { reviewer_ids: [user2.id] } }
context 'when merge_request_reviewers feature is disabled' do
before do
stub_feature_flags(merge_request_reviewers: false)
end
it 'creates system note about merge_request review request' do
note = find_note('requested review from')
it 'does not create a system note about merge_request review request' do
note = find_note('review requested from')
expect(note).to be_nil
end
it 'does not update the tracking' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.not_to receive(:track_users_review_requested)
update_merge_request(reviewer_ids: [user.id])
end
expect(note).not_to be_nil
expect(note.note).to include "requested review from #{user2.to_reference}"
end
context 'when merge_request_reviewers feature is enabled' do
before(:context) do
stub_feature_flags(merge_request_reviewers: true)
end
it 'creates system note about merge_request review request' do
note = find_note('requested review from')
expect(note).not_to be_nil
expect(note.note).to include "requested review from #{user2.to_reference}"
end
it 'updates the tracking' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_users_review_requested)
.with(users: [user])
it 'updates the tracking' do
expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter)
.to receive(:track_users_review_requested)
.with(users: [user])
update_merge_request(reviewer_ids: [user.id])
end
update_merge_request(reviewer_ids: [user.id])
end
end
......
......@@ -879,145 +879,123 @@ RSpec.describe QuickActions::InterpretService do
let(:issuable) { issue }
end
context 'when the merge_request_reviewers flag is enabled' do
describe 'assign_reviewer command' do
let(:content) { "/assign_reviewer @#{developer.username}" }
let(:issuable) { merge_request }
describe 'assign_reviewer command' do
let(:content) { "/assign_reviewer @#{developer.username}" }
let(:issuable) { merge_request }
context 'with one user' do
it_behaves_like 'assign_reviewer command'
end
context 'with one user' do
it_behaves_like 'assign_reviewer command'
end
context 'with an issue instead of a merge request' do
let(:issuable) { issue }
context 'with an issue instead of a merge request' do
let(:issuable) { issue }
it_behaves_like 'empty command'
end
it_behaves_like 'empty command'
end
# CE does not have multiple reviewers
context 'assign command with multiple assignees' do
before do
project.add_developer(developer2)
end
# CE does not have multiple reviewers
context 'assign command with multiple assignees' do
before do
project.add_developer(developer2)
end
# There's no guarantee that the reference extractor will preserve
# the order of the mentioned users since this is dependent on the
# order in which rows are returned. We just ensure that at least
# one of the mentioned users is assigned.
context 'assigns to one of the two users' do
let(:content) { "/assign_reviewer @#{developer.username} @#{developer2.username}" }
# There's no guarantee that the reference extractor will preserve
# the order of the mentioned users since this is dependent on the
# order in which rows are returned. We just ensure that at least
# one of the mentioned users is assigned.
context 'assigns to one of the two users' do
let(:content) { "/assign_reviewer @#{developer.username} @#{developer2.username}" }
it 'assigns to a single reviewer' do
_, updates, message = service.execute(content, issuable)
it 'assigns to a single reviewer' do
_, updates, message = service.execute(content, issuable)
expect(updates[:reviewer_ids].count).to eq(1)
reviewer = updates[:reviewer_ids].first
expect([developer.id, developer2.id]).to include(reviewer)
expect(updates[:reviewer_ids].count).to eq(1)
reviewer = updates[:reviewer_ids].first
expect([developer.id, developer2.id]).to include(reviewer)
user = reviewer == developer.id ? developer : developer2
user = reviewer == developer.id ? developer : developer2
expect(message).to match("Assigned #{user.to_reference} as reviewer.")
end
expect(message).to match("Assigned #{user.to_reference} as reviewer.")
end
end
end
context 'with "me" alias' do
let(:content) { '/assign_reviewer me' }
it_behaves_like 'assign_reviewer command'
end
context 'with an alias and whitespace' do
let(:content) { '/assign_reviewer me ' }
it_behaves_like 'assign_reviewer command'
end
context 'with an incorrect user' do
let(:content) { '/assign_reviewer @abcd1234' }
context 'with "me" alias' do
let(:content) { '/assign_reviewer me' }
it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found."
end
it_behaves_like 'assign_reviewer command'
end
context 'with the "reviewer" alias' do
let(:content) { "/reviewer @#{developer.username}" }
context 'with an alias and whitespace' do
let(:content) { '/assign_reviewer me ' }
it_behaves_like 'assign_reviewer command'
end
it_behaves_like 'assign_reviewer command'
end
context 'with the "request_review" alias' do
let(:content) { "/request_review @#{developer.username}" }
context 'with an incorrect user' do
let(:content) { '/assign_reviewer @abcd1234' }
it_behaves_like 'assign_reviewer command'
end
it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found."
end
context 'with no user' do
let(:content) { '/assign_reviewer' }
context 'with the "reviewer" alias' do
let(:content) { "/reviewer @#{developer.username}" }
it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found."
end
it_behaves_like 'assign_reviewer command'
end
context 'includes only the user reference with extra text' do
let(:content) { "/assign_reviewer @#{developer.username} do it!" }
context 'with the "request_review" alias' do
let(:content) { "/request_review @#{developer.username}" }
it_behaves_like 'assign_reviewer command'
end
it_behaves_like 'assign_reviewer command'
end
describe 'unassign_reviewer command' do
# CE does not have multiple reviewers, so basically anything
# after /unassign_reviewer (including whitespace) will remove
# all the current reviewers.
let(:issuable) { create(:merge_request, reviewers: [developer]) }
let(:content) { "/unassign_reviewer @#{developer.username}" }
context 'with no user' do
let(:content) { '/assign_reviewer' }
context 'with one user' do
it_behaves_like 'unassign_reviewer command'
end
it_behaves_like 'empty command', "Failed to assign a reviewer because no user was found."
end
context 'with an issue instead of a merge request' do
let(:issuable) { issue }
context 'includes only the user reference with extra text' do
let(:content) { "/assign_reviewer @#{developer.username} do it!" }
it_behaves_like 'empty command'
end
it_behaves_like 'assign_reviewer command'
end
end
context 'with anything after the command' do
let(:content) { '/unassign_reviewer supercalifragilisticexpialidocious' }
describe 'unassign_reviewer command' do
# CE does not have multiple reviewers, so basically anything
# after /unassign_reviewer (including whitespace) will remove
# all the current reviewers.
let(:issuable) { create(:merge_request, reviewers: [developer]) }
let(:content) { "/unassign_reviewer @#{developer.username}" }
it_behaves_like 'unassign_reviewer command'
end
context 'with one user' do
it_behaves_like 'unassign_reviewer command'
end
context 'with the "remove_reviewer" alias' do
let(:content) { "/remove_reviewer @#{developer.username}" }
context 'with an issue instead of a merge request' do
let(:issuable) { issue }
it_behaves_like 'unassign_reviewer command'
end
it_behaves_like 'empty command'
end
context 'with no user' do
let(:content) { '/unassign_reviewer' }
context 'with anything after the command' do
let(:content) { '/unassign_reviewer supercalifragilisticexpialidocious' }
it_behaves_like 'unassign_reviewer command'
end
it_behaves_like 'unassign_reviewer command'
end
end
context 'when the merge_request_reviewers flag is disabled' do
before do
stub_feature_flags(merge_request_reviewers: false)
end
context 'with the "remove_reviewer" alias' do
let(:content) { "/remove_reviewer @#{developer.username}" }
describe 'assign_reviewer command' do
it_behaves_like 'empty command' do
let(:content) { "/assign_reviewer @#{developer.username}" }
let(:issuable) { merge_request }
end
it_behaves_like 'unassign_reviewer command'
end
describe 'unassign_reviewer command' do
it_behaves_like 'empty command' do
let(:content) { "/unassign_reviewer @#{developer.username}" }
let(:issuable) { merge_request }
end
context 'with no user' do
let(:content) { '/unassign_reviewer' }
it_behaves_like 'unassign_reviewer command'
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment