diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2016-10-12 15:01:34 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2016-11-07 14:49:24 +0300 |
commit | f694f94c491452a50035c2ff43c8ba595c0e73aa (patch) | |
tree | 125d88eff69df031b590f589ae107f609e2f95db | |
parent | 89bb29b247b57e3b4ba053a5fd17f2087ac4414f (diff) |
Added IssueCollection
This class can be used to reduce a list of issues down to a subset based
on user permissions. This class operates in such a way that it can
reduce issues using as few queries as possible, if any at all.
-rw-r--r-- | app/models/concerns/issuable.rb | 5 | ||||
-rw-r--r-- | app/models/issue_collection.rb | 40 | ||||
-rw-r--r-- | app/policies/issuable_policy.rb | 2 | ||||
-rw-r--r-- | app/policies/issue_policy.rb | 9 | ||||
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 21 | ||||
-rw-r--r-- | spec/models/issue_collection_spec.rb | 58 |
6 files changed, 127 insertions, 8 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 613444e0d70..93a6b3122e0 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -286,6 +286,11 @@ module Issuable false end + def assignee_or_author?(user) + # We're comparing IDs here so we don't need to load any associations. + author_id == user.id || assignee_id == user.id + end + def record_metrics metrics = self.metrics || create_metrics metrics.record! diff --git a/app/models/issue_collection.rb b/app/models/issue_collection.rb new file mode 100644 index 00000000000..df36252a5b1 --- /dev/null +++ b/app/models/issue_collection.rb @@ -0,0 +1,40 @@ +# IssueCollection can be used to reduce a list of issues down to a subset. +# +# IssueCollection is not meant to be some sort of Enumerable, instead it's meant +# to take a list of issues and return a new list of issues based on some +# criteria. For example, given a list of issues you may want to return a list of +# issues that can be read or updated by a given user. +class IssueCollection + attr_reader :collection + + def initialize(collection) + @collection = collection + end + + # Returns all the issues that can be updated by the user. + def updatable_by_user(user) + return collection if user.admin? + + # Given all the issue projects we get a list of projects that the current + # user has at least reporter access to. + projects_with_reporter_access = user. + projects_with_reporter_access_limited_to(project_ids). + pluck(:id) + + collection.select do |issue| + if projects_with_reporter_access.include?(issue.project_id) + true + elsif issue.is_a?(Issue) + issue.assignee_or_author?(user) + else + false + end + end + end + + private + + def project_ids + @project_ids ||= collection.map(&:project_id).uniq + end +end diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index c253f9a9399..9501e499507 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -4,7 +4,7 @@ class IssuablePolicy < BasePolicy end def rules - if @user && (@subject.author == @user || @subject.assignee == @user) + if @user && @subject.assignee_or_author?(@user) can! :"read_#{action_name}" can! :"update_#{action_name}" end diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index bd1811a3c54..f3ede58a001 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -8,9 +8,8 @@ class IssuePolicy < IssuablePolicy if @subject.confidential? && !can_read_confidential? cannot! :read_issue - cannot! :admin_issue cannot! :update_issue - cannot! :read_issue + cannot! :admin_issue end end @@ -18,11 +17,7 @@ class IssuePolicy < IssuablePolicy def can_read_confidential? return false unless @user - return true if @user.admin? - return true if @subject.author == @user - return true if @subject.assignee == @user - return true if @subject.project.team.member?(@user, Gitlab::Access::REPORTER) - false + IssueCollection.new([@subject]).updatable_by_user(@user).any? end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index a59d30687f6..a9603074c32 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -341,4 +341,25 @@ describe Issue, "Issuable" do expect(Issue.with_label([bug.title, enhancement.title])).to match_array([issue2]) end end + + describe '#assignee_or_author?' do + let(:user) { build(:user, id: 1) } + let(:issue) { build(:issue) } + + it 'returns true for a user that is assigned to an issue' do + issue.assignee = user + + expect(issue.assignee_or_author?(user)).to eq(true) + end + + it 'returns true for a user that is the author of an issue' do + issue.author = user + + expect(issue.assignee_or_author?(user)).to eq(true) + end + + it 'returns false for a user that is not the assignee or author' do + expect(issue.assignee_or_author?(user)).to eq(false) + end + end end diff --git a/spec/models/issue_collection_spec.rb b/spec/models/issue_collection_spec.rb new file mode 100644 index 00000000000..d9ab397c302 --- /dev/null +++ b/spec/models/issue_collection_spec.rb @@ -0,0 +1,58 @@ +require 'spec_helper' + +describe IssueCollection do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:issue1) { create(:issue, project: project) } + let(:issue2) { create(:issue, project: project) } + let(:collection) { described_class.new([issue1, issue2]) } + + describe '#collection' do + it 'returns the issues in the same order as the input Array' do + expect(collection.collection).to eq([issue1, issue2]) + end + end + + describe '#updatable_by_user' do + context 'using an admin user' do + it 'returns all issues' do + user = create(:admin) + + expect(collection.updatable_by_user(user)).to eq([issue1, issue2]) + end + end + + context 'using a user that has no access to the project' do + it 'returns no issues when the user is not an assignee or author' do + expect(collection.updatable_by_user(user)).to be_empty + end + + it 'returns the issues the user is assigned to' do + issue1.assignee = user + + expect(collection.updatable_by_user(user)).to eq([issue1]) + end + + it 'returns the issues for which the user is the author' do + issue1.author = user + + expect(collection.updatable_by_user(user)).to eq([issue1]) + end + end + + context 'using a user that has reporter access to the project' do + it 'returns the issues of the project' do + project.team << [user, :reporter] + + expect(collection.updatable_by_user(user)).to eq([issue1, issue2]) + end + end + + context 'using a user that is the owner of a project' do + it 'returns the issues of the project' do + expect(collection.updatable_by_user(project.namespace.owner)). + to eq([issue1, issue2]) + end + end + end +end |