Merge branch 'dz-nested-groups-members' into 'master'

Show members of parent groups on project members page

Closes #28786 and #27962

See merge request !9764

6 files changed, 69 insertions, 44 deletions

diff --git a/app/controllers/projects/settings/members_controller.rb b/app/controllers/projects/settings/members_controller.rb
index 5735e281f66..cbfa2afa959 100644
--- a/app/controllers/projects/settings/members_controller.rb
+++ b/app/controllers/projects/settings/members_controller.rb
@@ -7,47 +7,18 @@ module Projects
         @sort = params[:sort].presence || sort_value_name
         @group_links = @project.project_group_links
 
-        @project_members = @project.project_members
-        @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project)
-
-        group = @project.group
-
-        # group links
-        @group_links = @project.project_group_links.all
-
         @skip_groups = @group_links.pluck(:group_id)
         @skip_groups << @project.namespace_id unless @project.personal?
 
-        if group
-          # We need `.where.not(user_id: nil)` here otherwise when a group has an
-          # invitee, it would make the following query return 0 rows since a NULL
-          # user_id would be present in the subquery
-          # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values
-          group_members = MembersFinder.new(@project_members, group).execute(current_user)
-        end
+        @project_members = MembersFinder.new(@project, current_user).execute
 
         if params[:search].present?
-          user_ids = @project.users.search(params[:search]).select(:id)
-          @project_members = @project_members.where(user_id: user_ids)
-
-          if group_members
-            user_ids = group.users.search(params[:search]).select(:id)
-            group_members = group_members.where(user_id: user_ids)
-          end
-
-          @group_links = @project.project_group_links.where(group_id: @project.invited_groups.search(params[:search]).select(:id))
+          @project_members = @project_members.joins(:user).merge(User.search(params[:search]))
+          @group_links = @group_links.where(group_id: @project.invited_groups.search(params[:search]).select(:id))
         end
 
-        wheres = ["members.id IN (#{@project_members.select(:id).to_sql})"]
-        wheres << "members.id IN (#{group_members.select(:id).to_sql})" if group_members
-
-        @project_members = Member.
-          where(wheres.join(' OR ')).
-          sort(@sort).
-          page(params[:page])
-
+        @project_members = @project_members.sort(@sort).page(params[:page])
         @requesters = AccessRequestsFinder.new(@project).execute(current_user)
-
         @project_member = @project.project_members.new
       end
     end
diff --git a/app/finders/group_members_finder.rb b/app/finders/group_members_finder.rb
index 9f2206346ce..fce3775f40e 100644
--- a/app/finders/group_members_finder.rb
+++ b/app/finders/group_members_finder.rb
@@ -1,4 +1,4 @@
-class GroupMembersFinder < Projects::ApplicationController
+class GroupMembersFinder
   def initialize(group)
     @group = group
   end
diff --git a/app/finders/members_finder.rb b/app/finders/members_finder.rb
index 702944404f5..af24045886e 100644
--- a/app/finders/members_finder.rb
+++ b/app/finders/members_finder.rb
@@ -1,13 +1,35 @@
-class MembersFinder < Projects::ApplicationController
-  def initialize(project_members, project_group)
-    @project_members = project_members
-    @project_group = project_group
+class MembersFinder
+  attr_reader :project, :current_user, :group
+
+  def initialize(project, current_user)
+    @project = project
+    @current_user = current_user
+    @group = project.group
+  end
+
+  def execute
+    project_members = project.project_members
+    project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
+    wheres = ["members.id IN (#{project_members.select(:id).to_sql})"]
+
+    if group
+      # We need `.where.not(user_id: nil)` here otherwise when a group has an
+      # invitee, it would make the following query return 0 rows since a NULL
+      # user_id would be present in the subquery
+      # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values
+      non_null_user_ids = project_members.where.not(user_id: nil).select(:user_id)
+
+      group_members = GroupMembersFinder.new(group).execute
+      group_members = group_members.where.not(user_id: non_null_user_ids)
+      group_members = group_members.non_invite unless can?(current_user, :admin_group, group)
+
+      wheres << "members.id IN (#{group_members.select(:id).to_sql})"
+    end
+
+    Member.where(wheres.join(' OR '))
   end
 
-  def execute(current_user)
-    non_null_user_ids = @project_members.where.not(user_id: nil).select(:user_id)
-    group_members = @project_group.group_members.where.not(user_id: non_null_user_ids)
-    group_members = group_members.non_invite unless can?(current_user, :admin_group,  @project_group)
-    group_members
+  def can?(*args)
+    Ability.allowed?(*args)
   end
 end
diff --git a/changelogs/unreleased/dz-nested-groups-members.yml b/changelogs/unreleased/dz-nested-groups-members.yml
new file mode 100644
index 00000000000..bab0c8465c2
--- /dev/null
+++ b/changelogs/unreleased/dz-nested-groups-members.yml
@@ -0,0 +1,4 @@
+---
+title: Show members of parent groups on project members page
+merge_request:
+author:
diff --git a/lib/gitlab/import_export/project_tree_saver.rb b/lib/gitlab/import_export/project_tree_saver.rb
index b79be62245b..3473b466936 100644
--- a/lib/gitlab/import_export/project_tree_saver.rb
+++ b/lib/gitlab/import_export/project_tree_saver.rb
@@ -47,7 +47,13 @@ module Gitlab
       def group_members
         return [] unless @current_user.can?(:admin_group, @project.group)
 
-        MembersFinder.new(@project.project_members, @project.group).execute(@current_user)
+        # We need `.where.not(user_id: nil)` here otherwise when a group has an
+        # invitee, it would make the following query return 0 rows since a NULL
+        # user_id would be present in the subquery
+        # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values
+        non_null_user_ids = @project.project_members.where.not(user_id: nil).select(:user_id)
+
+        GroupMembersFinder.new(@project.group).execute.where.not(user_id: non_null_user_ids)
       end
     end
   end
diff --git a/spec/finders/members_finder_spec.rb b/spec/finders/members_finder_spec.rb
new file mode 100644
index 00000000000..cf691cf684b
--- /dev/null
+++ b/spec/finders/members_finder_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe MembersFinder, '#execute' do
+  let(:group)        { create(:group) }
+  let(:nested_group) { create(:group, :access_requestable, parent: group) }
+  let(:project)      { create(:project, namespace: nested_group) }
+  let(:user1)        { create(:user) }
+  let(:user2)        { create(:user) }
+  let(:user3)        { create(:user) }
+  let(:user4)        { create(:user) }
+
+  it 'returns members for project and parent groups' do
+    nested_group.request_access(user1)
+    member1 = group.add_master(user2)
+    member2 = nested_group.add_master(user3)
+    member3 = project.add_master(user4)
+
+    result = described_class.new(project, user2).execute
+
+    expect(result.to_a).to eq([member3, member2, member1])
+  end
+end