Merge branch 'dz-refactor-project-members-controller' into 'master'

Refactor ProjectMembers controller

See merge request gitlab-org/gitlab!23960

app/controllers/projects/project_members_controller.rb

@@ -8,27 +8,26 @@ class Projects::ProjectMembersController < Projects::ApplicationController
   # Authorize
   before_action :authorize_admin_project_member!, except: [:index, :leave, :request_access]
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def index
     @sort = params[:sort].presence || sort_value_name
+
+    @skip_groups = @project.invited_group_ids
+    @skip_groups += @project.group.self_and_ancestors_ids if @project.group
+
     @group_links = @project.project_group_links
+    @group_links = @group_links.search(params[:search]) if params[:search].present?
 
-    @skip_groups = @group_links.pluck(:group_id)
-    @skip_groups << @project.namespace_id unless @project.personal?
-    @skip_groups += @project.group.ancestors.pluck(:id) if @project.group
+    @project_members = MembersFinder.new(@project, current_user)
+      .execute(include_relations: requested_relations, params: params.merge(sort: @sort))
 
-    @project_members = MembersFinder.new(@project, current_user).execute(include_relations: requested_relations)
+    @project_members = present_members(@project_members.page(params[:page]))
 
-    if params[:search].present?
-      @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
+    @requesters = present_members(
+      AccessRequestsFinder.new(@project).execute(current_user)
+    )
 
-    @project_members = present_members(@project_members.sort_by_attribute(@sort).page(params[:page]))
-    @requesters = present_members(AccessRequestsFinder.new(@project).execute(current_user))
     @project_member = @project.project_members.new
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def import
     @projects = current_user.authorized_projects.order_id_desc

app/finders/members_finder.rb

 # frozen_string_literal: true
 
 class MembersFinder
-  attr_reader :project, :current_user, :group
+  # Params can be any of the following:
+  #   sort:       string
+  #   search:     string
 
   def initialize(project, current_user)
     @project = project
@@ -9,29 +11,40 @@ class MembersFinder
     @group = project.group
   end
 
-  def execute(include_relations: [:inherited, :direct])
+  def execute(include_relations: [:inherited, :direct], params: {})
+    members = find_members(include_relations, params)
+
+    filter_members(members, params)
+  end
+
+  def can?(*args)
+    Ability.allowed?(*args)
+  end
+
+  private
+
+  attr_reader :project, :current_user, :group
+
+  def find_members(include_relations, params)
     project_members = project.project_members
     project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
 
     return project_members if include_relations == [:direct]
 
     union_members = group_union_members(include_relations)
-
     union_members << project_members if include_relations.include?(:direct)
 
-    if union_members.any?
+    return project_members unless union_members.any?
+
     distinct_union_of_members(union_members)
-    else
-      project_members
-    end
   end
 
-  def can?(*args)
-    Ability.allowed?(*args)
+  def filter_members(members, params)
+    members = members.search(params[:search]) if params[:search].present?
+    members = members.sort_by_attribute(params[:sort]) if params[:sort].present?
+    members
   end
 
-  private
-
   def group_union_members(include_relations)
     [].tap do |members|
       members << direct_group_members(include_relations.include?(:descendants)) if group

app/models/project_group_link.rb

@@ -31,6 +31,10 @@ class ProjectGroupLink < ApplicationRecord
     DEVELOPER
   end
 
+  def self.search(query)
+    joins(:group).merge(Group.search(query))
+  end
+
   def human_access
     self.class.access_options.key(self.group_access)
   end

spec/finders/members_finder_spec.rb

@@ -75,6 +75,15 @@ describe MembersFinder, '#execute' do
     expect(result).to contain_exactly(member2, member3)
   end
 
+  it 'returns only inherited members of a personal project' do
+    project = create(:project, namespace: user1.namespace)
+    member = project.members.first
+
+    result = described_class.new(project, user1).execute(include_relations: [:inherited])
+
+    expect(result).to contain_exactly(member)
+  end
+
   it 'returns the members.access_level when the user is invited', :nested_groups do
     member_invite = create(:project_member, :invited, project: project, invite_email: create(:user).email)
     member1 = group.add_maintainer(user2)
@@ -96,6 +105,26 @@ describe MembersFinder, '#execute' do
     expect(result.first.access_level).to eq(Gitlab::Access::DEVELOPER)
   end
 
+  it 'returns searched members if requested' do
+    project.add_maintainer(user2)
+    project.add_maintainer(user3)
+    member3 = project.add_maintainer(user4)
+
+    result = described_class.new(project, user2).execute(params: { search: user4.name })
+
+    expect(result).to contain_exactly(member3)
+  end
+
+  it 'returns members sorted by id_desc' do
+    member1 = project.add_maintainer(user2)
+    member2 = project.add_maintainer(user3)
+    member3 = project.add_maintainer(user4)
+
+    result = described_class.new(project, user2).execute(params: { sort: 'id_desc' })
+
+    expect(result).to eq([member3, member2, member1])
+  end
+
   context 'when include_invited_groups_members == true' do
     subject { described_class.new(project, user2).execute(include_relations: [:inherited, :direct, :invited_groups_members]) }
 

spec/models/project_group_link_spec.rb

@@ -47,4 +47,12 @@ describe ProjectGroupLink do
       group_users.each { |user| expect(user.authorized_projects).not_to include(project) }
     end
   end
+
+  describe 'search by group name' do
+    let_it_be(:project_group_link) { create(:project_group_link) }
+    let_it_be(:group) { project_group_link.group }
+
+    it { expect(described_class.search(group.name)).to eq([project_group_link]) }
+    it { expect(described_class.search('not-a-group-name')).to be_empty }
+  end
 end