Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
1
Merge Requests
1
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
nexedi
gitlab-ce
Commits
de7d507f
Commit
de7d507f
authored
Sep 07, 2020
by
David Kim
Committed by
Sean McGivern
Sep 07, 2020
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Add merge_request_reviewers table and model
It's added to prepare for dedicated reviewers section for MRs on EE
parent
d69d13ea
Changes
19
Hide whitespace changes
Inline
Side-by-side
Showing
19 changed files
with
279 additions
and
6 deletions
+279
-6
app/controllers/projects/merge_requests/application_controller.rb
...rollers/projects/merge_requests/application_controller.rb
+1
-0
app/models/concerns/issuable.rb
app/models/concerns/issuable.rb
+4
-0
app/models/merge_request.rb
app/models/merge_request.rb
+4
-0
app/serializers/merge_request_basic_entity.rb
app/serializers/merge_request_basic_entity.rb
+1
-0
app/serializers/merge_request_reviewer_entity.rb
app/serializers/merge_request_reviewer_entity.rb
+9
-0
app/serializers/merge_request_sidebar_extras_entity.rb
app/serializers/merge_request_sidebar_extras_entity.rb
+4
-0
app/services/issuable_base_service.rb
app/services/issuable_base_service.rb
+5
-5
app/services/merge_requests/base_service.rb
app/services/merge_requests/base_service.rb
+22
-0
config/feature_flags/development/merge_request_reviewers.yml
config/feature_flags/development/merge_request_reviewers.yml
+7
-0
ee/app/serializers/ee/merge_request_reviewer_entity.rb
ee/app/serializers/ee/merge_request_reviewer_entity.rb
+11
-0
spec/fixtures/api/schemas/entities/merge_request_basic.json
spec/fixtures/api/schemas/entities/merge_request_basic.json
+7
-0
spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json
...es/api/schemas/entities/merge_request_sidebar_extras.json
+4
-0
spec/models/issue_spec.rb
spec/models/issue_spec.rb
+10
-0
spec/models/merge_request_spec.rb
spec/models/merge_request_spec.rb
+18
-0
spec/serializers/merge_request_basic_entity_spec.rb
spec/serializers/merge_request_basic_entity_spec.rb
+26
-1
spec/serializers/merge_request_sidebar_extras_entity_spec.rb
spec/serializers/merge_request_sidebar_extras_entity_spec.rb
+57
-0
spec/services/merge_requests/create_service_spec.rb
spec/services/merge_requests/create_service_spec.rb
+4
-0
spec/services/merge_requests/update_service_spec.rb
spec/services/merge_requests/update_service_spec.rb
+23
-0
spec/support/shared_examples/services/merge_request_shared_examples.rb
...shared_examples/services/merge_request_shared_examples.rb
+62
-0
No files found.
app/controllers/projects/merge_requests/application_controller.rb
View file @
de7d507f
...
...
@@ -43,6 +43,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
:discussion_locked
,
label_ids:
[],
assignee_ids:
[],
reviewer_ids:
[],
update_task:
[
:index
,
:checked
,
:line_number
,
:line_source
]
]
end
...
...
app/models/concerns/issuable.rb
View file @
de7d507f
...
...
@@ -177,6 +177,10 @@ module Issuable
assignees
.
count
>
1
end
def
allows_reviewers?
false
end
def
supports_time_tracking?
is_a?
(
TimeTrackable
)
&&
!
incident?
end
...
...
app/models/merge_request.rb
View file @
de7d507f
...
...
@@ -1658,6 +1658,10 @@ class MergeRequest < ApplicationRecord
end
end
def
allows_reviewers?
Feature
.
enabled?
(
:merge_request_reviewers
,
project
)
end
private
def
with_rebase_lock
...
...
app/serializers/merge_request_basic_entity.rb
View file @
de7d507f
...
...
@@ -9,6 +9,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
:task_status
,
:task_status_short
expose
:lock_version
,
:lock_version
end
app/serializers/merge_request_reviewer_entity.rb
0 → 100644
View file @
de7d507f
# frozen_string_literal: true
class
MergeRequestReviewerEntity
<
::
API
::
Entities
::
UserBasic
expose
:can_merge
do
|
reviewer
,
options
|
options
[
:merge_request
]
&
.
can_be_merged_by?
(
reviewer
)
end
end
MergeRequestReviewerEntity
.
prepend_if_ee
(
'EE::MergeRequestReviewerEntity'
)
app/serializers/merge_request_sidebar_extras_entity.rb
View file @
de7d507f
...
...
@@ -4,4 +4,8 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity
expose
:assignees
do
|
merge_request
|
MergeRequestAssigneeEntity
.
represent
(
merge_request
.
assignees
,
merge_request:
merge_request
)
end
expose
:reviewers
,
if:
->
(
m
)
{
m
.
allows_reviewers?
}
do
|
merge_request
|
MergeRequestReviewerEntity
.
represent
(
merge_request
.
reviewers
,
merge_request:
merge_request
)
end
end
app/services/issuable_base_service.rb
View file @
de7d507f
...
...
@@ -46,7 +46,7 @@ class IssuableBaseService < BaseService
params
[
:assignee_ids
]
=
params
[
:assignee_ids
].
first
(
1
)
end
assignee_ids
=
params
[
:assignee_ids
].
select
{
|
assignee_id
|
assignee
_can_read?
(
issuable
,
assignee_id
)
}
assignee_ids
=
params
[
:assignee_ids
].
select
{
|
assignee_id
|
user
_can_read?
(
issuable
,
assignee_id
)
}
if
params
[
:assignee_ids
].
map
(
&
:to_s
)
==
[
IssuableFinder
::
Params
::
NONE
]
params
[
:assignee_ids
]
=
[]
...
...
@@ -57,15 +57,15 @@ class IssuableBaseService < BaseService
end
end
def
assignee_can_read?
(
issuable
,
assignee
_id
)
new_assignee
=
User
.
find_by_id
(
assignee
_id
)
def
user_can_read?
(
issuable
,
user
_id
)
user
=
User
.
find_by_id
(
user
_id
)
return
false
unless
new_assignee
return
false
unless
user
ability_name
=
:"read_
#{
issuable
.
to_ability_name
}
"
resource
=
issuable
.
persisted?
?
issuable
:
project
can?
(
new_assignee
,
ability_name
,
resource
)
can?
(
user
,
ability_name
,
resource
)
end
def
filter_milestone
...
...
app/services/merge_requests/base_service.rb
View file @
de7d507f
...
...
@@ -97,6 +97,28 @@ module MergeRequests
unless
merge_request
.
can_allow_collaboration?
(
current_user
)
params
.
delete
(
:allow_collaboration
)
end
filter_reviewer
(
merge_request
)
end
def
filter_reviewer
(
merge_request
)
return
if
params
[
:reviewer_ids
].
blank?
unless
can_admin_issuable?
(
merge_request
)
&&
merge_request
.
allows_reviewers?
params
.
delete
(
:reviewer_ids
)
return
end
reviewer_ids
=
params
[
:reviewer_ids
].
select
{
|
reviewer_id
|
user_can_read?
(
merge_request
,
reviewer_id
)
}
if
params
[
:reviewer_ids
].
map
(
&
:to_s
)
==
[
IssuableFinder
::
Params
::
NONE
]
params
[
:reviewer_ids
]
=
[]
elsif
reviewer_ids
.
any?
params
[
:reviewer_ids
]
=
reviewer_ids
else
params
.
delete
(
:reviewer_ids
)
end
end
def
merge_request_metrics_service
(
merge_request
)
...
...
config/feature_flags/development/merge_request_reviewers.yml
0 → 100644
View file @
de7d507f
---
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
group
:
group::source code
type
:
development
default_enabled
:
false
ee/app/serializers/ee/merge_request_reviewer_entity.rb
0 → 100644
View file @
de7d507f
# frozen_string_literal: true
module
EE
module
MergeRequestReviewerEntity
extend
ActiveSupport
::
Concern
prepended
do
expose
:gitlab_employee?
,
as: :is_gitlab_employee
,
if:
proc
{
::
Gitlab
.
com?
&&
::
Feature
.
enabled?
(
:gitlab_employee_badge
)
}
end
end
end
spec/fixtures/api/schemas/entities/merge_request_basic.json
View file @
de7d507f
...
...
@@ -15,6 +15,13 @@
"$ref"
:
"../public_api/v4/user/basic.json"
}
},
"reviewers"
:
{
"type"
:
[
"array"
],
"items"
:
{
"type"
:
"object"
,
"$ref"
:
"../public_api/v4/user/basic.json"
}
},
"milestone"
:
{
"type"
:
[
"object"
,
"null"
]
},
...
...
spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json
View file @
de7d507f
...
...
@@ -17,6 +17,10 @@
"assignees"
:
{
"type"
:
"array"
,
"items"
:
{
"$ref"
:
"../public_api/v4/user/basic.json"
}
},
"reviewers"
:
{
"type"
:
"array"
,
"items"
:
{
"$ref"
:
"../public_api/v4/user/basic.json"
}
}
},
"additionalProperties"
:
false
...
...
spec/models/issue_spec.rb
View file @
de7d507f
...
...
@@ -1212,4 +1212,14 @@ RSpec.describe Issue do
end
end
end
describe
'#allows_reviewers?'
do
it
'returns false as issues do not support reviewers feature'
do
stub_feature_flags
(
merge_request_reviewers:
true
)
issue
=
build_stubbed
(
:issue
)
expect
(
issue
.
allows_reviewers?
).
to
be
(
false
)
end
end
end
spec/models/merge_request_spec.rb
View file @
de7d507f
...
...
@@ -4145,4 +4145,22 @@ RSpec.describe MergeRequest, factory_default: :keep do
.
with_arguments
(
allow_nil:
true
)
end
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
)
merge_request
=
build_stubbed
(
:merge_request
)
expect
(
merge_request
.
allows_reviewers?
).
to
be
(
true
)
end
end
end
spec/serializers/merge_request_basic_entity_spec.rb
View file @
de7d507f
...
...
@@ -3,7 +3,8 @@
require
'spec_helper'
RSpec
.
describe
MergeRequestBasicEntity
do
let
(
:resource
)
{
build
(
:merge_request
)
}
let
(
:resource
)
{
build
(
:merge_request
,
params
)
}
let
(
:params
)
{
{}
}
subject
do
described_class
.
new
(
resource
).
as_json
...
...
@@ -14,4 +15,28 @@ RSpec.describe MergeRequestBasicEntity do
expect
(
subject
[
:merge_status
]).
to
eq
'checking'
end
describe
'#reviewers'
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
end
end
end
spec/serializers/merge_request_sidebar_extras_entity_spec.rb
0 → 100644
View file @
de7d507f
# frozen_string_literal: true
require
'spec_helper'
RSpec
.
describe
MergeRequestSidebarExtrasEntity
do
let_it_be
(
:assignee
)
{
build
(
:user
)
}
let_it_be
(
:reviewer
)
{
build
(
:user
)
}
let_it_be
(
:user
)
{
build
(
:user
)
}
let_it_be
(
:project
)
{
create
:project
,
:repository
}
let
(
:params
)
do
{
source_project:
project
,
target_project:
project
,
assignees:
[
assignee
],
reviewers:
[
reviewer
]
}
end
let
(
:resource
)
do
build
(
:merge_request
,
params
)
end
let
(
:request
)
{
double
(
'request'
,
current_user:
user
,
project:
project
)
}
let
(
:entity
)
{
described_class
.
new
(
resource
,
request:
request
).
as_json
}
describe
'#assignees'
do
it
'contains assignees attributes'
do
expect
(
entity
[
:assignees
].
count
).
to
be
1
expect
(
entity
[
:assignees
].
first
.
keys
).
to
include
(
:id
,
:name
,
:username
,
:state
,
:avatar_url
,
:web_url
,
:can_merge
)
end
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
end
end
end
spec/services/merge_requests/create_service_spec.rb
View file @
de7d507f
...
...
@@ -339,6 +339,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
end
end
end
it_behaves_like
'reviewer_ids filter'
do
let
(
:execute
)
{
service
.
execute
}
end
end
it_behaves_like
'issuable record that supports quick actions'
do
...
...
spec/services/merge_requests/update_service_spec.rb
View file @
de7d507f
...
...
@@ -161,6 +161,29 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
expect
(
@merge_request
.
merge_params
[
"force_remove_source_branch"
]).
to
eq
(
"1"
)
end
end
it_behaves_like
'reviewer_ids filter'
do
let
(
:opts
)
{
{}
}
let
(
:execute
)
{
update_merge_request
(
opts
)
}
end
context
'with an existing reviewer'
do
let
(
:merge_request
)
do
create
(
:merge_request
,
:simple
,
source_project:
project
,
reviewer_ids:
[
user2
.
id
])
end
context
'when merge_request_reviewer feature is enabled'
do
before
do
stub_feature_flags
(
merge_request_reviewer:
true
)
end
let
(
:opts
)
{
{
reviewer_ids:
[
IssuableFinder
::
Params
::
NONE
]
}
}
it
'removes reviewers'
do
expect
(
update_merge_request
(
opts
).
reviewers
).
to
eq
[]
end
end
end
end
context
'after_save callback to store_mentions'
do
...
...
spec/support/shared_examples/services/merge_request_shared_examples.rb
0 → 100644
View file @
de7d507f
# frozen_string_literal: true
RSpec
.
shared_examples
'reviewer_ids filter'
do
context
'filter_reviewer'
do
let
(
:opts
)
{
super
().
merge
(
reviewer_ids_param
)
}
context
'without reviewer_ids'
do
let
(
:reviewer_ids_param
)
{
{}
}
it
'contains no reviewer_ids'
do
expect
(
execute
.
reviewers
).
to
eq
[]
end
end
context
'with reviewer_ids'
do
let
(
:reviewer_ids_param
)
{
{
reviewer_ids:
[
reviewer1
.
id
,
reviewer2
.
id
,
reviewer3
.
id
]
}
}
let
(
:reviewer1
)
{
create
(
:user
)
}
let
(
:reviewer2
)
{
create
(
:user
)
}
let
(
:reviewer3
)
{
create
(
:user
)
}
context
'when the current user can admin the merge_request'
do
context
'when merge_request_reviewer feature is enabled'
do
before
do
stub_feature_flags
(
merge_request_reviewer:
true
)
end
context
'with reviewers who can read the merge_request'
do
before
do
project
.
add_developer
(
reviewer1
)
project
.
add_developer
(
reviewer2
)
end
it
'contains reviewers who can read the merge_request'
do
expect
(
execute
.
reviewers
).
to
contain_exactly
(
reviewer1
,
reviewer2
)
end
end
end
context
'when merge_request_reviewer feature is disabled'
do
before
do
stub_feature_flags
(
merge_request_reviewer:
false
)
end
it
'contains no reviewers'
do
expect
(
execute
.
reviewers
).
to
eq
[]
end
end
end
context
'when the current_user cannot admin the merge_request'
do
before
do
project
.
add_developer
(
user
)
end
it
'contains no reviewers'
do
expect
(
execute
.
reviewers
).
to
eq
[]
end
end
end
end
end
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment