Refactor Participable

There are several changes to this module:

1. The use of an explicit stack in Participable#participants
2. Proc behaviour has been changed
3. Batch permissions checking

== Explicit Stack

Participable#participants no longer uses recursion to process "self" and
all child objects, instead it uses an Array and processes objects in
breadth-first order. This allows us to for example create a single
Gitlab::ReferenceExtractor instance and pass this to any Procs. Re-using
a ReferenceExtractor removes the need for running potentially many SQL
queries every time a Proc is called on a new object.

== Proc Behaviour Changed

Previously a Proc in Participable was expected to return an Array of
User instances. This has been changed and instead it's now expected that
a Proc modifies the Gitlab::ReferenceExtractor passed to it. The return
value of the Proc is ignored.

== Permissions Checking

The method Participable#participants uses
Ability.users_that_can_read_project to check if the returned users have
access to the project of "self" _without_ running multiple SQL queries
for every user.

8 files changed, 372 insertions, 3 deletions

diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
new file mode 100644
index 00000000000..1acb5846fcf
--- /dev/null
+++ b/spec/models/ability_spec.rb
@@ -0,0 +1,117 @@
+require 'spec_helper'
+
+describe Ability, lib: true do
+  describe '.users_that_can_read_project' do
+    context 'using a public project' do
+      it 'returns all the users' do
+        project = create(:project, :public)
+        user = build(:user)
+
+        expect(described_class.users_that_can_read_project([user], project)).
+          to eq([user])
+      end
+    end
+
+    context 'using an internal project' do
+      let(:project) { create(:project, :internal) }
+
+      it 'returns users that are administrators' do
+        user = build(:user, admin: true)
+
+        expect(described_class.users_that_can_read_project([user], project)).
+          to eq([user])
+      end
+
+      it 'returns internal users while skipping external users' do
+        user1 = build(:user)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns external users if they are the project owner' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project).to receive(:owner).twice.and_return(user1)
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns external users if they are project members' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project.team).to receive(:members).twice.and_return([user1])
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns an empty Array if all users are external users without access' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([])
+      end
+    end
+
+    context 'using a private project' do
+      let(:project) { create(:project, :private) }
+
+      it 'returns users that are administrators' do
+        user = build(:user, admin: true)
+
+        expect(described_class.users_that_can_read_project([user], project)).
+          to eq([user])
+      end
+
+      it 'returns external users if they are the project owner' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project).to receive(:owner).twice.and_return(user1)
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns external users if they are project members' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(project.team).to receive(:members).twice.and_return([user1])
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([user1])
+      end
+
+      it 'returns an empty Array if all users are internal users without access' do
+        user1 = build(:user)
+        user2 = build(:user)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([])
+      end
+
+      it 'returns an empty Array if all users are external users without access' do
+        user1 = build(:user, external: true)
+        user2 = build(:user, external: true)
+        users = [user1, user2]
+
+        expect(described_class.users_that_can_read_project(users, project)).
+          to eq([])
+      end
+    end
+  end
+end
diff --git a/spec/models/commit_range_spec.rb b/spec/models/commit_range_spec.rb
index 1cf51d8bab3..6bc496414a3 100644
--- a/spec/models/commit_range_spec.rb
+++ b/spec/models/commit_range_spec.rb
@@ -145,4 +145,27 @@ describe CommitRange, models: true do
       end
     end
   end
+
+  describe '#has_been_reverted?' do
+    it 'returns true if the commit has been reverted' do
+      issue = create(:issue)
+
+      create(:note_on_issue,
+             noteable_id: issue.id,
+             system: true,
+             note: commit1.revert_description)
+
+      expect_any_instance_of(Commit).to receive(:reverts_commit?).
+        with(commit1).
+        and_return(true)
+
+      expect(commit1.has_been_reverted?(nil, issue)).to eq(true)
+    end
+
+    it 'returns false a commit has not been reverted' do
+      issue = create(:issue)
+
+      expect(commit1.has_been_reverted?(nil, issue)).to eq(false)
+    end
+  end
 end
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index ccb100cd96f..beca8708c9d 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Commit, models: true do
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :public) }
   let(:commit)  { project.commit }
 
   describe 'modules' do
@@ -171,4 +171,40 @@ eos
   describe '#status' do
     # TODO: kamil
   end
+
+  describe '#participants' do
+    let(:user1) { build(:user) }
+    let(:user2) { build(:user) }
+
+    let!(:note1) do
+      create(:note_on_commit,
+             commit_id: commit.id,
+             project: project,
+             note: 'foo')
+    end
+
+    let!(:note2) do
+      create(:note_on_commit,
+             commit_id: commit.id,
+             project: project,
+             note: 'bar')
+    end
+
+    before do
+      allow(commit).to receive(:author).and_return(user1)
+      allow(commit).to receive(:committer).and_return(user2)
+    end
+
+    it 'includes the commit author' do
+      expect(commit.participants).to include(commit.author)
+    end
+
+    it 'includes the committer' do
+      expect(commit.participants).to include(commit.committer)
+    end
+
+    it 'includes the authors of the commit notes' do
+      expect(commit.participants).to include(note1.author, note2.author)
+    end
+  end
 end
diff --git a/spec/models/concerns/participable_spec.rb b/spec/models/concerns/participable_spec.rb
new file mode 100644
index 00000000000..7e4ea0f2d66
--- /dev/null
+++ b/spec/models/concerns/participable_spec.rb
@@ -0,0 +1,83 @@
+require 'spec_helper'
+
+describe Participable, models: true do
+  let(:model) do
+    Class.new do
+      include Participable
+    end
+  end
+
+  describe '.participant' do
+    it 'adds the participant attributes to the existing list' do
+      model.participant(:foo)
+      model.participant(:bar)
+
+      expect(model.participant_attrs).to eq([:foo, :bar])
+    end
+  end
+
+  describe '#participants' do
+    it 'returns the list of participants' do
+      model.participant(:foo)
+      model.participant(:bar)
+
+      user1 = build(:user)
+      user2 = build(:user)
+      user3 = build(:user)
+      project = build(:project, :public)
+      instance = model.new
+
+      expect(instance).to receive(:foo).and_return(user2)
+      expect(instance).to receive(:bar).and_return(user3)
+      expect(instance).to receive(:project).twice.and_return(project)
+
+      participants = instance.participants(user1)
+
+      expect(participants).to include(user2)
+      expect(participants).to include(user3)
+    end
+
+    it 'supports attributes returning another Participable' do
+      other_model = Class.new { include Participable }
+
+      other_model.participant(:bar)
+      model.participant(:foo)
+
+      instance = model.new
+      other = other_model.new
+      user1 = build(:user)
+      user2 = build(:user)
+      project = build(:project, :public)
+
+      expect(instance).to receive(:foo).and_return(other)
+      expect(other).to receive(:bar).and_return(user2)
+      expect(instance).to receive(:project).twice.and_return(project)
+
+      expect(instance.participants(user1)).to eq([user2])
+    end
+
+    context 'when using a Proc as an attribute' do
+      it 'calls the supplied Proc' do
+        user1 = build(:user)
+        project = build(:project, :public)
+
+        user_arg = nil
+        ext_arg = nil
+
+        model.participant -> (user, ext) do
+          user_arg = user
+          ext_arg = ext
+        end
+
+        instance = model.new
+
+        expect(instance).to receive(:project).twice.and_return(project)
+
+        instance.participants(user1)
+
+        expect(user_arg).to eq(user1)
+        expect(ext_arg).to be_an_instance_of(Gitlab::ReferenceExtractor)
+      end
+    end
+  end
+end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 6540d77fbc0..87b3d8d650a 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -231,4 +231,42 @@ describe Issue, models: true do
       expect(issue.to_branch_name).to match /confidential-issue\z/
     end
   end
+
+  describe '#participants' do
+    context 'using a public project' do
+      let(:project) { create(:project, :public) }
+      let(:issue) { create(:issue, project: project) }
+
+      let!(:note1) do
+        create(:note_on_issue, noteable: issue, project: project, note: 'a')
+      end
+
+      let!(:note2) do
+        create(:note_on_issue, noteable: issue, project: project, note: 'b')
+      end
+
+      it 'includes the issue author' do
+        expect(issue.participants).to include(issue.author)
+      end
+
+      it 'includes the authors of the notes' do
+        expect(issue.participants).to include(note1.author, note2.author)
+      end
+    end
+
+    context 'using a private project' do
+      it 'does not include mentioned users that do not have access to the project' do
+        project = create(:project)
+        user = create(:user)
+        issue = create(:issue, project: project)
+
+        create(:note_on_issue,
+               noteable: issue,
+               project: project,
+               note: user.to_reference)
+
+        expect(issue.participants).not_to include(user)
+      end
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index e269ff26a04..30a8e3ec059 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -411,4 +411,28 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe '#participants' do
+    let(:project) { create(:project, :public) }
+
+    let(:mr) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
+
+    let!(:note1) do
+      create(:note_on_merge_request, noteable: mr, project: project, note: 'a')
+    end
+
+    let!(:note2) do
+      create(:note_on_merge_request, noteable: mr, project: project, note: 'b')
+    end
+
+    it 'includes the merge request author' do
+      expect(mr.participants).to include(mr.author)
+    end
+
+    it 'includes the authors of the notes' do
+      expect(mr.participants).to include(note1.author, note2.author)
+    end
+  end
 end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 5d916f0e6a6..d158de6b967 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -93,8 +93,19 @@ describe Note, models: true do
     let!(:note2) { create(:note) }
 
     it "reads the rendered note body from the cache" do
-      expect(Banzai::Renderer).to receive(:render).with(note1.note, pipeline: :note, cache_key: [note1, "note"], project: note1.project)
-      expect(Banzai::Renderer).to receive(:render).with(note2.note, pipeline: :note, cache_key: [note2, "note"], project: note2.project)
+      expect(Banzai::Renderer).to receive(:render).
+        with(note1.note,
+             pipeline: :note,
+             cache_key: [note1, "note"],
+             project: note1.project,
+             author: note1.author)
+
+      expect(Banzai::Renderer).to receive(:render).
+        with(note2.note,
+             pipeline: :note,
+             cache_key: [note2, "note"],
+             project: note2.project,
+             author: note2.author)
 
       note1.all_references
       note2.all_references
@@ -195,4 +206,14 @@ describe Note, models: true do
       expect { note.valid? }.to change(note, :line_code).to(nil)
     end
   end
+
+  describe '#participants' do
+    it 'includes the note author' do
+      project = create(:project, :public)
+      issue = create(:issue, project: project)
+      note = create(:note_on_issue, noteable: issue, project: project)
+
+      expect(note.participants).to include(note.author)
+    end
+  end
 end
diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb
index 7a613e360d4..789816bf2c7 100644
--- a/spec/models/snippet_spec.rb
+++ b/spec/models/snippet_spec.rb
@@ -87,4 +87,31 @@ describe Snippet, models: true do
       expect(described_class.search_code('FOO')).to eq([snippet])
     end
   end
+
+  describe '#participants' do
+    let(:project) { create(:project, :public) }
+    let(:snippet) { create(:snippet, content: 'foo', project: project) }
+
+    let!(:note1) do
+      create(:note_on_project_snippet,
+             noteable: snippet,
+             project: project,
+             note: 'a')
+    end
+
+    let!(:note2) do
+      create(:note_on_project_snippet,
+             noteable: snippet,
+             project: project,
+             note: 'b')
+    end
+
+    it 'includes the snippet author' do
+      expect(snippet.participants).to include(snippet.author)
+    end
+
+    it 'includes the note authors' do
+      expect(snippet.participants).to include(note1.author, note2.author)
+    end
+  end
 end