Commit 4e28b60f authored by Alexandru Croitor's avatar Alexandru Croitor

Expose merge requests count based on user access

Count issues related merge requests based on user access level. And
issue can have related MRs from projects where user does not have
access so the number of related merge requests should be adjusted
based on user's ability to access the related MRs.

https://gitlab.com/gitlab-org/gitlab-ce/issues/59581
parent e3eeb779
...@@ -41,7 +41,7 @@ module IssuableCollections ...@@ -41,7 +41,7 @@ module IssuableCollections
return if pagination_disabled? return if pagination_disabled?
@issuables = @issuables.page(params[:page]) @issuables = @issuables.page(params[:page])
@issuable_meta_data = issuable_meta_data(@issuables, collection_type) @issuable_meta_data = issuable_meta_data(@issuables, collection_type, current_user)
@total_pages = issuable_page_count @total_pages = issuable_page_count
end end
# rubocop:enable Gitlab/ModuleWithInstanceVariables # rubocop:enable Gitlab/ModuleWithInstanceVariables
......
...@@ -11,7 +11,7 @@ module IssuableCollectionsAction ...@@ -11,7 +11,7 @@ module IssuableCollectionsAction
.non_archived .non_archived
.page(params[:page]) .page(params[:page])
@issuable_meta_data = issuable_meta_data(@issues, collection_type) @issuable_meta_data = issuable_meta_data(@issues, collection_type, current_user)
respond_to do |format| respond_to do |format|
format.html format.html
...@@ -22,7 +22,7 @@ module IssuableCollectionsAction ...@@ -22,7 +22,7 @@ module IssuableCollectionsAction
def merge_requests def merge_requests
@merge_requests = issuables_collection.page(params[:page]) @merge_requests = issuables_collection.page(params[:page])
@issuable_meta_data = issuable_meta_data(@merge_requests, collection_type) @issuable_meta_data = issuable_meta_data(@merge_requests, collection_type, current_user)
end end
# rubocop:enable Gitlab/ModuleWithInstanceVariables # rubocop:enable Gitlab/ModuleWithInstanceVariables
......
...@@ -298,7 +298,7 @@ class ProjectsController < Projects::ApplicationController ...@@ -298,7 +298,7 @@ class ProjectsController < Projects::ApplicationController
elsif @project.feature_available?(:issues, current_user) elsif @project.feature_available?(:issues, current_user)
@issues = issuables_collection.page(params[:page]) @issues = issuables_collection.page(params[:page])
@collection_type = 'Issue' @collection_type = 'Issue'
@issuable_meta_data = issuable_meta_data(@issues, @collection_type) @issuable_meta_data = issuable_meta_data(@issues, @collection_type, current_user)
end end
render :show render :show
......
...@@ -280,7 +280,7 @@ module IssuablesHelper ...@@ -280,7 +280,7 @@ module IssuablesHelper
initialTaskStatus: issuable.task_status initialTaskStatus: issuable.task_status
} }
data[:hasClosingMergeRequest] = issuable.merge_requests_count != 0 if issuable.is_a?(Issue) data[:hasClosingMergeRequest] = issuable.merge_requests_count(current_user) != 0 if issuable.is_a?(Issue)
if parent.is_a?(Group) if parent.is_a?(Group)
data[:groupPath] = parent.path data[:groupPath] = parent.path
......
...@@ -29,7 +29,11 @@ module Issuable ...@@ -29,7 +29,11 @@ module Issuable
# This object is used to gather issuable meta data for displaying # This object is used to gather issuable meta data for displaying
# upvotes, downvotes, notes and closing merge requests count for issues and merge requests # upvotes, downvotes, notes and closing merge requests count for issues and merge requests
# lists avoiding n+1 queries and improving performance. # lists avoiding n+1 queries and improving performance.
IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :merge_requests_count) IssuableMeta = Struct.new(:upvotes, :downvotes, :user_notes_count, :mrs_count) do
def merge_requests_count(user = nil)
mrs_count
end
end
included do included do
cache_markdown_field :title, pipeline: :single_line cache_markdown_field :title, pipeline: :single_line
......
...@@ -248,8 +248,8 @@ class Issue < ApplicationRecord ...@@ -248,8 +248,8 @@ class Issue < ApplicationRecord
end end
# rubocop: enable CodeReuse/ServiceClass # rubocop: enable CodeReuse/ServiceClass
def merge_requests_count def merge_requests_count(user = nil)
merge_requests_closing_issues.count ::MergeRequestsClosingIssues.count_for_issue(self.id, user)
end end
private private
......
...@@ -7,11 +7,38 @@ class MergeRequestsClosingIssues < ApplicationRecord ...@@ -7,11 +7,38 @@ class MergeRequestsClosingIssues < ApplicationRecord
validates :merge_request_id, uniqueness: { scope: :issue_id }, presence: true validates :merge_request_id, uniqueness: { scope: :issue_id }, presence: true
validates :issue_id, presence: true validates :issue_id, presence: true
scope :with_issues, ->(ids) { where(issue_id: ids) }
scope :with_merge_requests_enabled, -> do
joins(:merge_request)
.joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id')
.where('project_features.merge_requests_access_level >= :access', access: ProjectFeature::ENABLED)
end
scope :accessible_by, ->(user) do
joins(:merge_request)
.joins('INNER JOIN project_features ON merge_requests.target_project_id = project_features.project_id')
.where('project_features.merge_requests_access_level >= :access OR EXISTS(:authorizations)',
access: ProjectFeature::ENABLED,
authorizations: user.authorizations_for_projects(min_access_level: Gitlab::Access::REPORTER, related_project_column: "merge_requests.target_project_id")
)
end
class << self class << self
def count_for_collection(ids) def count_for_collection(ids, current_user)
group(:issue_id) closing_merge_requests(ids, current_user).group(:issue_id).pluck('issue_id', 'COUNT(*) as count')
.where(issue_id: ids) end
.pluck('issue_id', 'COUNT(*) as count')
def count_for_issue(id, current_user)
closing_merge_requests(id, current_user).count
end
private
def closing_merge_requests(ids, current_user)
return with_issues(ids) if current_user&.admin?
return with_issues(ids).with_merge_requests_enabled if current_user.blank?
with_issues(ids).accessible_by(current_user)
end end
end end
end end
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
- issue_votes = @issuable_meta_data[issuable.id] - issue_votes = @issuable_meta_data[issuable.id]
- upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes - upvotes, downvotes = issue_votes.upvotes, issue_votes.downvotes
- issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes') - issuable_url = @collection_type == "Issue" ? issue_path(issuable, anchor: 'notes') : merge_request_path(issuable, anchor: 'notes')
- issuable_mr = @issuable_meta_data[issuable.id].merge_requests_count - issuable_mr = @issuable_meta_data[issuable.id].merge_requests_count(current_user)
- if issuable_mr > 0 - if issuable_mr > 0
%li.issuable-mr.d-none.d-sm-block.has-tooltip{ title: _('Related merge requests') } %li.issuable-mr.d-none.d-sm-block.has-tooltip{ title: _('Related merge requests') }
......
---
title: Expose merge requests count based on user access
merge_request:
author:
type: security
...@@ -493,9 +493,9 @@ module API ...@@ -493,9 +493,9 @@ module API
expose :state, :created_at, :updated_at expose :state, :created_at, :updated_at
# Avoids an N+1 query when metadata is included # Avoids an N+1 query when metadata is included
def issuable_metadata(subject, options, method) def issuable_metadata(subject, options, method, args = nil)
cached_subject = options.dig(:issuable_metadata, subject.id) cached_subject = options.dig(:issuable_metadata, subject.id)
(cached_subject || subject).public_send(method) # rubocop: disable GitlabSecurity/PublicSend (cached_subject || subject).public_send(method, *args) # rubocop: disable GitlabSecurity/PublicSend
end end
end end
...@@ -554,7 +554,7 @@ module API ...@@ -554,7 +554,7 @@ module API
end end
expose(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) } expose(:user_notes_count) { |issue, options| issuable_metadata(issue, options, :user_notes_count) }
expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count) } expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count, options[:current_user]) }
expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) } expose(:upvotes) { |issue, options| issuable_metadata(issue, options, :upvotes) }
expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) } expose(:downvotes) { |issue, options| issuable_metadata(issue, options, :downvotes) }
expose :due_date expose :due_date
......
...@@ -93,7 +93,7 @@ module API ...@@ -93,7 +93,7 @@ module API
options = { options = {
with: Entities::IssueBasic, with: Entities::IssueBasic,
current_user: current_user, current_user: current_user,
issuable_metadata: issuable_meta_data(issues, 'Issue') issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
} }
present issues, options present issues, options
...@@ -120,7 +120,7 @@ module API ...@@ -120,7 +120,7 @@ module API
options = { options = {
with: Entities::IssueBasic, with: Entities::IssueBasic,
current_user: current_user, current_user: current_user,
issuable_metadata: issuable_meta_data(issues, 'Issue') issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
} }
present issues, options present issues, options
...@@ -150,7 +150,7 @@ module API ...@@ -150,7 +150,7 @@ module API
with: Entities::IssueBasic, with: Entities::IssueBasic,
current_user: current_user, current_user: current_user,
project: user_project, project: user_project,
issuable_metadata: issuable_meta_data(issues, 'Issue') issuable_metadata: issuable_meta_data(issues, 'Issue', current_user)
} }
present issues, options present issues, options
......
...@@ -72,7 +72,7 @@ module API ...@@ -72,7 +72,7 @@ module API
if params[:view] == 'simple' if params[:view] == 'simple'
options[:with] = Entities::MergeRequestSimple options[:with] = Entities::MergeRequestSimple
else else
options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest') options[:issuable_metadata] = issuable_meta_data(merge_requests, 'MergeRequest', current_user)
end end
options options
......
...@@ -65,7 +65,7 @@ module API ...@@ -65,7 +65,7 @@ module API
next unless collection next unless collection
targets = collection.map(&:target) targets = collection.map(&:target)
options[type] = { issuable_metadata: issuable_meta_data(targets, type) } options[type] = { issuable_metadata: issuable_meta_data(targets, type, current_user) }
end end
end end
end end
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module Gitlab module Gitlab
module IssuableMetadata module IssuableMetadata
def issuable_meta_data(issuable_collection, collection_type) def issuable_meta_data(issuable_collection, collection_type, user = nil)
# ActiveRecord uses Object#extend for null relations. # ActiveRecord uses Object#extend for null relations.
if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) && if !(issuable_collection.singleton_class < ActiveRecord::NullRelation) &&
issuable_collection.respond_to?(:limit_value) && issuable_collection.respond_to?(:limit_value) &&
...@@ -23,7 +23,7 @@ module Gitlab ...@@ -23,7 +23,7 @@ module Gitlab
issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type) issuable_votes_count = ::AwardEmoji.votes_for_collection(issuable_ids, collection_type)
issuable_merge_requests_count = issuable_merge_requests_count =
if collection_type == 'Issue' if collection_type == 'Issue'
::MergeRequestsClosingIssues.count_for_collection(issuable_ids) ::MergeRequestsClosingIssues.count_for_collection(issuable_ids, user)
else else
[] []
end end
......
...@@ -7,11 +7,11 @@ describe Gitlab::IssuableMetadata do ...@@ -7,11 +7,11 @@ describe Gitlab::IssuableMetadata do
subject { Class.new { include Gitlab::IssuableMetadata }.new } subject { Class.new { include Gitlab::IssuableMetadata }.new }
it 'returns an empty Hash if an empty collection is provided' do it 'returns an empty Hash if an empty collection is provided' do
expect(subject.issuable_meta_data(Issue.none, 'Issue')).to eq({}) expect(subject.issuable_meta_data(Issue.none, 'Issue', user)).to eq({})
end end
it 'raises an error when given a collection with no limit' do it 'raises an error when given a collection with no limit' do
expect { subject.issuable_meta_data(Issue.all, 'Issue') }.to raise_error(/must have a limit/) expect { subject.issuable_meta_data(Issue.all, 'Issue', user) }.to raise_error(/must have a limit/)
end end
context 'issues' do context 'issues' do
...@@ -23,7 +23,7 @@ describe Gitlab::IssuableMetadata do ...@@ -23,7 +23,7 @@ describe Gitlab::IssuableMetadata do
let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) } let!(:closing_issues) { create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request) }
it 'aggregates stats on issues' do it 'aggregates stats on issues' do
data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue') data = subject.issuable_meta_data(Issue.all.limit(10), 'Issue', user)
expect(data.count).to eq(2) expect(data.count).to eq(2)
expect(data[issue.id].upvotes).to eq(1) expect(data[issue.id].upvotes).to eq(1)
...@@ -46,7 +46,7 @@ describe Gitlab::IssuableMetadata do ...@@ -46,7 +46,7 @@ describe Gitlab::IssuableMetadata do
let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") } let!(:note) { create(:note_on_merge_request, author: user, project: project, noteable: merge_request, note: "a comment on a MR") }
it 'aggregates stats on merge requests' do it 'aggregates stats on merge requests' do
data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest') data = subject.issuable_meta_data(MergeRequest.all.limit(10), 'MergeRequest', user)
expect(data.count).to eq(2) expect(data.count).to eq(2)
expect(data[merge_request.id].upvotes).to eq(1) expect(data[merge_request.id].upvotes).to eq(1)
......
def get_issue
json_response.is_a?(Array) ? json_response.detect {|issue| issue['id'] == target_issue.id} : json_response
end
shared_examples 'accessible merge requests count' do
it 'returns anonymous accessible merge requests count' do
get api(api_url), params: { scope: 'all' }
issue = get_issue
expect(issue).not_to be_nil
expect(issue['merge_requests_count']).to eq(1)
end
it 'returns guest accessible merge requests count' do
get api(api_url, guest), params: { scope: 'all' }
issue = get_issue
expect(issue).not_to be_nil
expect(issue['merge_requests_count']).to eq(1)
end
it 'returns reporter accessible merge requests count' do
get api(api_url, user), params: { scope: 'all' }
issue = get_issue
expect(issue).not_to be_nil
expect(issue['merge_requests_count']).to eq(2)
end
it 'returns admin accessible merge requests count' do
get api(api_url, admin), params: { scope: 'all' }
issue = get_issue
expect(issue).not_to be_nil
expect(issue['merge_requests_count']).to eq(2)
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