Merge branch 'dz-refactor-member-controllers' into 'master'

Refactor ProjectMembers and GroupMembers controllers

Closes #118630

See merge request gitlab-org/gitlab!29501

app/controllers/concerns/members_presentation.rb

@@ -5,6 +5,7 @@ module MembersPresentation
 
   def present_members(members)
     preload_associations(members)
+
     Gitlab::View::Presenter::Factory.new(
       members,
       current_user: current_user,

app/controllers/groups/group_members_controller.rb

@@ -21,19 +21,26 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   def index
     @sort = params[:sort].presence || sort_value_name
+
     @project = @group.projects.find(params[:project_id]) if params[:project_id]
-    @members = find_members
+
+    @members = GroupMembersFinder
+      .new(@group, current_user, params: filter_params)
+      .execute(include_relations: requested_relations)
 
     if can_manage_members
       @skip_groups = @group.related_group_ids
-      @invited_members = present_invited_members(@members)
+
+      @invited_members = @members.invite
+      @invited_members = @invited_members.search_invite_email(params[:search_invited]) if params[:search_invited].present?
+      @invited_members = present_invited_members(@invited_members)
     end
 
-    @members = @members.non_invite
-    @members = present_group_members(@members)
+    @members = present_group_members(@members.non_invite)
 
     @requesters = present_members(
-      AccessRequestsFinder.new(@group).execute(current_user))
+      AccessRequestsFinder.new(@group).execute(current_user)
+    )
 
     @group_member = @group.group_members.new
   end
@@ -43,30 +50,24 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   private
 
-  def present_invited_members(members)
-    invited_members = members.invite
-
-    if params[:search_invited].present?
-      invited_members = invited_members.search_invite_email(params[:search_invited])
-    end
-
-    present_members(invited_members
-          .page(params[:invited_members_page])
-          .per(MEMBER_PER_PAGE_LIMIT))
+  def can_manage_members
+    can?(current_user, :admin_group_member, @group)
   end
 
-  def find_members
-    filter_params = params.slice(:two_factor, :search).merge(sort: @sort)
-    GroupMembersFinder.new(@group, current_user, params: filter_params).execute(include_relations: requested_relations)
+  def present_invited_members(invited_members)
+    present_members(invited_members
+      .page(params[:invited_members_page])
+      .per(MEMBER_PER_PAGE_LIMIT))
   end
 
-  def can_manage_members
-    can?(current_user, :admin_group_member, @group)
+  def present_group_members(members)
+    present_members(members
+      .page(params[:page])
+      .per(MEMBER_PER_PAGE_LIMIT))
   end
 
-  def present_group_members(original_members)
-    members = original_members.page(params[:page]).per(MEMBER_PER_PAGE_LIMIT)
-    present_members(members)
+  def filter_params
+    params.permit(:two_factor, :search).merge(sort: @sort)
   end
 end
 

app/controllers/projects/project_members_controller.rb

@@ -17,8 +17,9 @@ class Projects::ProjectMembersController < Projects::ApplicationController
     @group_links = @project.project_group_links
     @group_links = @group_links.search(params[:search]) if params[:search].present?
 
-    @project_members = MembersFinder.new(@project, current_user)
-      .execute(include_relations: requested_relations, params: params.merge(sort: @sort))
+    @project_members = MembersFinder
+      .new(@project, current_user, params: filter_params)
+      .execute(include_relations: requested_relations)
 
     @project_members = present_members(@project_members.page(params[:page]))
 
@@ -43,12 +44,17 @@ class Projects::ProjectMembersController < Projects::ApplicationController
       return render_404
     end
 
-    redirect_to(project_project_members_path(project),
-                notice: notice)
+    redirect_to(project_project_members_path(project), notice: notice)
   end
 
   # MembershipActions concern
   alias_method :membershipable, :project
+
+  private
+
+  def filter_params
+    params.permit(:search).merge(sort: @sort)
+  end
 end
 
 Projects::ProjectMembersController.prepend_if_ee('EE::Projects::ProjectMembersController')

app/finders/group_members_finder.rb

@@ -9,7 +9,6 @@ class GroupMembersFinder < UnionFinder
   #   search:     string
   #   created_after: datetime
   #   created_before: datetime
-
   attr_reader :params
 
   def initialize(group, user = nil, params: {})
@@ -22,7 +21,6 @@ class GroupMembersFinder < UnionFinder
   def execute(include_relations: [:inherited, :direct])
     group_members = group.members
     relations = []
-    @params = params
 
     return group_members if include_relations == [:direct]
 

app/finders/members_finder.rb

@@ -4,17 +4,19 @@ class MembersFinder
   # Params can be any of the following:
   #   sort:       string
   #   search:     string
+  attr_reader :params
 
-  def initialize(project, current_user)
+  def initialize(project, current_user, params: {})
     @project = project
-    @current_user = current_user
     @group = project.group
+    @current_user = current_user
+    @params = params
   end
 
-  def execute(include_relations: [:inherited, :direct], params: {})
-    members = find_members(include_relations, params)
+  def execute(include_relations: [:inherited, :direct])
+    members = find_members(include_relations)
 
-    filter_members(members, params)
+    filter_members(members)
   end
 
   def can?(*args)
@@ -25,7 +27,7 @@ class MembersFinder
 
   attr_reader :project, :current_user, :group
 
-  def find_members(include_relations, params)
+  def find_members(include_relations)
     project_members = project.project_members
     project_members = project_members.non_invite unless can?(current_user, :admin_project, project)
 
@@ -39,7 +41,7 @@ class MembersFinder
     distinct_union_of_members(union_members)
   end
 
-  def filter_members(members, params)
+  def filter_members(members)
     members = members.search(params[:search]) if params[:search].present?
     members = members.sort_by_attribute(params[:sort]) if params[:sort].present?
     members

spec/finders/members_finder_spec.rb

@@ -110,7 +110,7 @@ describe MembersFinder, '#execute' do
     project.add_maintainer(user3)
     member3 = project.add_maintainer(user4)
 
-    result = described_class.new(project, user2).execute(params: { search: user4.name })
+    result = described_class.new(project, user2, params: { search: user4.name }).execute
 
     expect(result).to contain_exactly(member3)
   end
@@ -120,7 +120,7 @@ describe MembersFinder, '#execute' do
     member2 = project.add_maintainer(user3)
     member3 = project.add_maintainer(user4)
 
-    result = described_class.new(project, user2).execute(params: { sort: 'id_desc' })
+    result = described_class.new(project, user2, params: { sort: 'id_desc' }).execute
 
     expect(result).to eq([member3, member2, member1])
   end