Refactoed GroupsFinder into two separate classes

In the previous setup the GroupsFinder class had two distinct tasks:

1. Finding the projects user A could see
2. Finding the projects of user A that user B could see

Task two was actually handled outside of the GroupsFinder (in the
UsersController) by restricting the returned list of groups to those the
viewed user was a member of. Moving all this logic into a single finder
proved to be far too complex and confusing, hence there are now two
finders:

* GroupsFinder: for finding groups a user can see
* JoinedGroupsFinder: for finding groups that user A is a member of,
  restricted to either public groups or groups user B can also see.

4 files changed, 177 insertions, 41 deletions

diff --git a/app/finders/groups_finder.rb b/app/finders/groups_finder.rb
index b5f3176461c..91cb0f228f0 100644
--- a/app/finders/groups_finder.rb
+++ b/app/finders/groups_finder.rb
@@ -1,39 +1,44 @@
 class GroupsFinder
-  def execute(current_user, options = {})
-    all_groups(current_user)
+  # Finds the groups available to the given user.
+  #
+  # current_user - The user to find the groups for.
+  #
+  # Returns an ActiveRecord::Relation.
+  def execute(current_user = nil)
+    if current_user
+      relation = groups_visible_to_user(current_user)
+    else
+      relation = public_groups
+    end
+
+    relation.order_id_desc
   end
 
   private
 
-  def all_groups(current_user)
-    group_ids = if current_user
-                  if current_user.authorized_groups.any?
-                    # User has access to groups
-                    #
-                    # Return only:
-                    #   groups with public projects
-                    #   groups with internal projects
-                    #   groups with joined projects
-                    #
-                    Project.public_and_internal_only.pluck(:namespace_id) +
-                      current_user.authorized_groups.pluck(:id)
-                  else
-                    # User has no group membership
-                    #
-                    # Return only:
-                    #   groups with public projects
-                    #   groups with internal projects
-                    #
-                    Project.public_and_internal_only.pluck(:namespace_id)
-                  end
-                else
-                  # Not authenticated
-                  #
-                  # Return only:
-                  #   groups with public projects
-                  Project.public_only.pluck(:namespace_id)
-                end
-
-    Group.where("public IS TRUE OR id IN(?)", group_ids)
+  # This method returns the groups "current_user" can see.
+  def groups_visible_to_user(current_user)
+    base = groups_for_projects(public_and_internal_projects)
+
+    union = Gitlab::SQL::Union.
+      new([base.select(:id), current_user.authorized_groups.select(:id)])
+
+    Group.where("namespaces.id IN (#{union.to_sql})")
+  end
+
+  def public_groups
+    groups_for_projects(public_projects)
+  end
+
+  def groups_for_projects(projects)
+    Group.public_and_given_groups(projects.select(:namespace_id))
+  end
+
+  def public_projects
+    Project.unscoped.public_only
+  end
+
+  def public_and_internal_projects
+    Project.unscoped.public_and_internal_only
   end
 end
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
new file mode 100644
index 00000000000..e7523136fea
--- /dev/null
+++ b/app/finders/joined_groups_finder.rb
@@ -0,0 +1,49 @@
+# Class for finding the groups a user is a member of.
+class JoinedGroupsFinder
+  def initialize(user = nil)
+    @user = user
+  end
+
+  # Finds the groups of the source user, optionally limited to those visible to
+  # the current user.
+  #
+  # current_user - If given the groups of "@user" will only include the groups
+  #                "current_user" can also see.
+  #
+  # Returns an ActiveRecord::Relation.
+  def execute(current_user = nil)
+    if current_user
+      relation = groups_visible_to_user(current_user)
+    else
+      relation = public_groups
+    end
+
+    relation.order_id_desc
+  end
+
+  private
+
+  # Returns the groups the user in "current_user" can see.
+  #
+  # This list includes all public/internal projects as well as the projects of
+  # "@user" that "current_user" also has access to.
+  def groups_visible_to_user(current_user)
+    base  = @user.authorized_groups.visible_to_user(current_user)
+    extra = public_and_internal_groups
+    union = Gitlab::SQL::Union.new([base.select(:id), extra.select(:id)])
+
+    Group.where("namespaces.id IN (#{union.to_sql})")
+  end
+
+  def public_groups
+    groups_for_projects(@user.authorized_projects.public_only)
+  end
+
+  def public_and_internal_groups
+    groups_for_projects(@user.authorized_projects.public_and_internal_only)
+  end
+
+  def groups_for_projects(projects)
+    @user.groups.public_and_given_groups(projects.select(:namespace_id))
+  end
+end
diff --git a/spec/finders/groups_finder_spec.rb b/spec/finders/groups_finder_spec.rb
index 78dc027837c..4f6a000822e 100644
--- a/spec/finders/groups_finder_spec.rb
+++ b/spec/finders/groups_finder_spec.rb
@@ -1,15 +1,48 @@
 require 'spec_helper'
 
 describe GroupsFinder do
-  let(:user) { create :user }
-  let!(:group) { create :group }
-  let!(:public_group) { create :group, public: true }
-  
-  describe :execute do
-    it 'finds public group' do
-      groups = GroupsFinder.new.execute(user)
-      expect(groups.size).to eq(1)
-      expect(groups.first).to eq(public_group)
+  describe '#execute' do
+    let(:user) { create(:user) }
+
+    let(:group1) { create(:group) }
+    let(:group2) { create(:group) }
+    let(:group3) { create(:group) }
+    let(:group4) { create(:group, public: true) }
+
+    let!(:public_project)   { create(:project, :public, group: group1) }
+    let!(:internal_project) { create(:project, :internal, group: group2) }
+    let!(:private_project)  { create(:project, :private, group: group3) }
+
+    let(:finder) { described_class.new }
+
+    describe 'with a user' do
+      subject { finder.execute(user) }
+
+      describe 'when the user is not a member of any groups' do
+        it { is_expected.to eq([group4, group2, group1]) }
+      end
+
+      describe 'when the user is a member of a group' do
+        before do
+          group3.add_user(user, Gitlab::Access::DEVELOPER)
+        end
+
+        it { is_expected.to eq([group4, group3, group2, group1]) }
+      end
+
+      describe 'when the user is a member of a private project' do
+        before do
+          private_project.team.add_user(user, Gitlab::Access::DEVELOPER)
+        end
+
+        it { is_expected.to eq([group4, group3, group2, group1]) }
+      end
+    end
+
+    describe 'without a user' do
+      subject { finder.execute }
+
+      it { is_expected.to eq([group4, group1]) }
     end
   end
 end
diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb
new file mode 100644
index 00000000000..2d9068cc720
--- /dev/null
+++ b/spec/finders/joined_groups_finder_spec.rb
@@ -0,0 +1,49 @@
+require 'spec_helper'
+
+describe JoinedGroupsFinder do
+  describe '#execute' do
+    let(:source_user) { create(:user) }
+    let(:current_user) { create(:user) }
+
+    let(:group1) { create(:group) }
+    let(:group2) { create(:group) }
+    let(:group3) { create(:group) }
+    let(:group4) { create(:group, public: true) }
+
+    let!(:public_project)   { create(:project, :public, group: group1) }
+    let!(:internal_project) { create(:project, :internal, group: group2) }
+    let!(:private_project)  { create(:project, :private, group: group3) }
+
+    let(:finder) { described_class.new(source_user) }
+
+    before do
+      [group1, group2, group3, group4].each do |group|
+        group.add_user(source_user, Gitlab::Access::MASTER)
+      end
+    end
+
+    describe 'with a current user' do
+      describe 'when the current user has access to the projects of the source user' do
+        before do
+          private_project.team.add_user(current_user, Gitlab::Access::DEVELOPER)
+        end
+
+        subject { finder.execute(current_user) }
+
+        it { is_expected.to eq([group4, group3, group2, group1]) }
+      end
+
+      describe 'when the current user does not have access to the projects of the source user' do
+        subject { finder.execute(current_user) }
+
+        it { is_expected.to eq([group4, group2, group1]) }
+      end
+    end
+
+    describe 'without a current user' do
+      subject { finder.execute }
+
+      it { is_expected.to eq([group4, group1]) }
+    end
+  end
+end