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.

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