[EE] Present member collection at the controller level

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/controllers/admin/groups_controller.rb

 class Admin::GroupsController < Admin::ApplicationController
+  include MembersPresentation
   prepend EE::Admin::GroupsController
 
   before_action :group, only: [:edit, :update, :destroy, :project_update, :members_update]
@@ -12,8 +13,10 @@ class Admin::GroupsController < Admin::ApplicationController
 
   def show
     @group = Group.with_statistics.joins(:route).group('routes.path').find_by_full_path(params[:id])
-    @members = @group.members.order("access_level DESC").page(params[:members_page])
-    @requesters = AccessRequestsFinder.new(@group).execute(current_user)
+    @members = present_members(
+      @group.members.order("access_level DESC").page(params[:members_page]))
+    @requesters = present_members(
+      AccessRequestsFinder.new(@group).execute(current_user))
     @projects = @group.projects.with_statistics.page(params[:projects_page])
   end
 

app/controllers/admin/projects_controller.rb

 class Admin::ProjectsController < Admin::ApplicationController
+  include MembersPresentation
+
   before_action :project, only: [:show, :transfer, :repository_check]
   before_action :group, only: [:show, :transfer]
 
@@ -19,11 +21,14 @@ class Admin::ProjectsController < Admin::ApplicationController
 
   def show
     if @group
-      @group_members = @group.members.order("access_level DESC").page(params[:group_members_page])
+      @group_members = present_members(
+        @group.members.order("access_level DESC").page(params[:group_members_page]))
     end
 
-    @project_members = @project.members.page(params[:project_members_page])
-    @requesters = AccessRequestsFinder.new(@project).execute(current_user)
+    @project_members = present_members(
+      @project.members.page(params[:project_members_page]))
+    @requesters = present_members(
+      AccessRequestsFinder.new(@project).execute(current_user))
   end
 
   def transfer

app/controllers/concerns/members_presentation.rb

+module MembersPresentation
+  extend ActiveSupport::Concern
+
+  def present_members(members)
+    Gitlab::View::Presenter::Factory.new(
+      members,
+      current_user: current_user,
+      presenter_class: MembersPresenter
+    ).fabricate!
+  end
+end

app/controllers/groups/group_members_controller.rb

@@ -2,6 +2,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
   prepend EE::Groups::GroupMembersController
 
   include MembershipActions
+  include MembersPresentation
   include SortingHelper
 
   # Authorize
@@ -17,15 +18,17 @@ class Groups::GroupMembersController < Groups::ApplicationController
     @members = @members.search(params[:search]) if params[:search].present?
     @members = @members.sort(@sort)
     @members = @members.page(params[:page]).per(50)
-    @members.includes(:user)
+    @members = present_members(@members.includes(:user))
 
-    @requesters = AccessRequestsFinder.new(@group).execute(current_user)
+    @requesters = present_members(
+      AccessRequestsFinder.new(@group).execute(current_user))
 
     @group_member = @group.group_members.new
   end
 
   def update
     @group_member = @group.group_members.find(params[:id])
+      .present(current_user: current_user)
 
     return render_403 unless can?(current_user, :update_group_member, @group_member)
 

app/controllers/projects/project_members_controller.rb

 class Projects::ProjectMembersController < Projects::ApplicationController
   include MembershipActions
+  include MembersPresentation
   include SortingHelper
 
   # Authorize
@@ -20,13 +21,14 @@ class Projects::ProjectMembersController < Projects::ApplicationController
       @group_links = @group_links.where(group_id: @project.invited_groups.search(params[:search]).select(:id))
     end
 
-    @project_members = @project_members.sort(@sort).page(params[:page])
-    @requesters = AccessRequestsFinder.new(@project).execute(current_user)
+    @project_members = present_members(@project_members.sort(@sort).page(params[:page]))
+    @requesters = present_members(AccessRequestsFinder.new(@project).execute(current_user))
     @project_member = @project.project_members.new
   end
 
   def update
     @project_member = @project.project_members.find(params[:id])
+      .present(current_user: current_user)
 
     return render_403 unless can?(current_user, :update_project_member, @project_member)
 

app/presenters/member_presenter.rb

 class MemberPresenter < Gitlab::View::Presenter::Delegated
-  include Gitlab::Allowable
-
   prepend EE::MemberPresenter
 
   presents :member
 
+  def access_level_roles
+    member.class.access_level_roles
+  end
+
   def can_resend_invite?
     invite? &&
       can?(current_user, admin_member_permission, source)
@@ -14,10 +16,6 @@ class MemberPresenter < Gitlab::View::Presenter::Delegated
     can?(current_user, update_member_permission, member)
   end
 
-  def cannot_update?
-    !can_update?
-  end
-
   def can_remove?
     can?(current_user, destroy_member_permission, member)
   end

app/presenters/members_presenter.rb

+class MembersPresenter < Gitlab::View::Presenter::Delegated
+  include Enumerable
+
+  presents :members
+
+  def to_ary
+    to_a
+  end
+
+  def each
+    members.each do |member|
+      yield member.present(current_user: current_user)
+    end
+  end
+end

app/views/projects/project_members/_team.html.haml

+- project = local_assigns.fetch(:project)
+- members = local_assigns.fetch(:members)
+
 .panel.panel-default
   .panel-heading.flex-project-members-panel
     %span.flex-project-title
       Members of
-      %strong
-        #{@project.name}
-    %span.badge= @project_members.total_count
-    = form_tag project_project_members_path(@project), method: :get, class: 'form-inline member-search-form flex-project-members-form'  do
+      %strong= project.name
+    %span.badge= members.total_count
+    = form_tag project_project_members_path(project), method: :get, class: 'form-inline member-search-form flex-project-members-form'  do
       .form-group
         = search_field_tag :search, params[:search], { placeholder: 'Find existing members by name', class: 'form-control', spellcheck: false }
         %button.member-search-btn{ type: "submit", "aria-label" => "Submit search" }

app/views/projects/project_members/index.html.haml

@@ -39,5 +39,5 @@
         - if @group_links.any?
           = render 'projects/project_members/groups', group_links: @group_links
 
-    = render 'projects/project_members/team', members: @project_members
+    = render 'projects/project_members/team', project: @project, members: @project_members
     = paginate @project_members, theme: "gitlab"

app/views/shared/members/_member.html.haml

 - show_roles = local_assigns.fetch(:show_roles, true)
 - show_controls = local_assigns.fetch(:show_controls, true)
 - force_mobile_view = local_assigns.fetch(:force_mobile_view, false)
+- member = local_assigns.fetch(:member)
 - user = local_assigns.fetch(:user, member.user)
 - source = member.source
-- member_presenter = member.present(current_user: current_user)
 
 %li.member{ class: [dom_class(member), ("is-overriden" if member.override)], id: dom_id(member) }
   %span.list-item-name
@@ -48,21 +48,21 @@
   - if show_roles
     - current_resource = @project || @group
     .controls.member-controls
-      = render 'shared/members/ee/ldap_tag', can_override: member_presenter.can_override?, visible: false
+      = render 'shared/members/ee/ldap_tag', can_override: member.can_override?, visible: false
       - if show_controls && member.source == current_resource
 
-        - if member_presenter.can_resend_invite?
+        - if member.can_resend_invite?
           = link_to icon('paper-plane'), polymorphic_path([:resend_invite, member]),
                     method: :post,
                     class: 'btn btn-default prepend-left-10 hidden-xs',
                     title: 'Resend invite'
 
-        - if user != current_user && (member_presenter.can_update? || member_presenter.can_override?)
+        - if user != current_user && member.can_update?
           = form_for member, remote: true, html: { class: 'form-horizontal js-edit-member-form' } do |f|
             = f.hidden_field :access_level
             .member-form-control.dropdown.append-right-5
               %button.dropdown-menu-toggle.js-member-permissions-dropdown{ type: "button",
-                disabled: member_presenter.cannot_update?,
+                disabled: member.can_override?,
                 data: { toggle: "dropdown", field_name: "#{f.object_name}[access_level]" } }
                 %span.dropdown-toggle-text
                   = member.human_access
@@ -71,24 +71,27 @@
                 = dropdown_title("Change permissions")
                 .dropdown-content
                   %ul
-                    - member.class.access_level_roles.each do |role, role_id|
+                    - member.access_level_roles.each do |role, role_id|
                       %li
                         = link_to role, "javascript:void(0)",
                           class: ("is-active" if member.access_level == role_id),
                           data: { id: role_id, el_id: dom_id(member) }
-                    = render 'shared/members/ee/revert_ldap_group_sync_option', group: @group, member: member, can_override: member_presenter.can_override?
+                    = render 'shared/members/ee/revert_ldap_group_sync_option',
+                      group: @group,
+                      member: member,
+                      can_override: member.can_override?
             .prepend-left-5.clearable-input.member-form-control
-              = f.text_field :expires_at, class: 'form-control js-access-expiration-date js-member-update-control', placeholder: 'Expiration date', id: "member_expires_at_#{member.id}", disabled: member_presenter.cannot_update?, data: { el_id: dom_id(member) }
+              = f.text_field :expires_at,
+                disabled: member.can_override?,
+                class: 'form-control js-access-expiration-date js-member-update-control',
+                placeholder: 'Expiration date',
+                id: "member_expires_at_#{member.id}",
+                data: { el_id: dom_id(member) }
               %i.clear-icon.js-clear-input
         - else
           %span.member-access-text= member.human_access
 
-        - if member_presenter.can_resend_invite?
-          = link_to 'Resend invite', polymorphic_path([:resend_invite, member]),
-                    method: :post,
-                    class: 'btn btn-default  prepend-left-10 visible-xs-block'
-
-        - elsif member_presenter.can_approve?
+        - if member.can_approve?
           = link_to polymorphic_path([:approve_access_request, member]),
                     method: :post,
                     class: 'btn btn-success prepend-left-10',
@@ -98,7 +101,7 @@
             - unless force_mobile_view
               = icon('check inverse', class: 'hidden-xs')
 
-        - if member_presenter.can_remove?
+        - if member.can_remove?
           - if current_user == user
             = link_to icon('sign-out', text: 'Leave'), polymorphic_path([:leave, member.source, :members]),
                       method: :delete,
@@ -114,8 +117,8 @@
                 Delete
               - unless force_mobile_view
                 = icon('trash', class: 'hidden-xs')
-        = render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :edit, can_override: member_presenter.can_override?
+        = render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :edit, can_override: member.can_override?
       - else
         %span.member-access-text= member.human_access
 
-= render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :confirm, can_override: member_presenter.can_override?
+= render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :confirm, can_override: member.can_override?

app/views/shared/members/_requests.html.haml

+- membership_source = local_assigns.fetch(:membership_source)
+- requesters = local_assigns.fetch(:requesters)
 - force_mobile_view = local_assigns.fetch(:force_mobile_view, false)
 
-- if requesters.any?
-  .panel.panel-default.prepend-top-default{ class: ('panel-mobile' if force_mobile_view ) }
-    .panel-heading
-      Users requesting access to
-      %strong= membership_source.name
-      %span.badge= requesters.size
-    %ul.content-list.members-list
-      = render partial: 'shared/members/member', collection: requesters, as: :member, locals: { force_mobile_view: force_mobile_view }
+- return if requesters.empty?
+
+.panel.panel-default.prepend-top-default{ class: ('panel-mobile' if force_mobile_view ) }
+  .panel-heading
+    Users requesting access to
+    %strong= membership_source.name
+    %span.badge= requesters.size
+  %ul.content-list.members-list
+    = render partial: 'shared/members/member', collection: requesters, as: :member, locals: { force_mobile_view: force_mobile_view }

ee/app/presenters/ee/member_presenter.rb

 module EE
   module MemberPresenter
+    def can_update?
+      super || can_override?
+    end
+
     def can_override?
       can?(current_user, override_member_permission, member)
     end

lib/gitlab/view/presenter/factory.rb

@@ -16,7 +16,7 @@ module Gitlab
         attr_reader :subject, :attributes
 
         def presenter_class
-          "#{subject.class.name}Presenter".constantize
+          attributes.delete(:presenter_class) { "#{subject.class.name}Presenter".constantize }
         end
       end
     end

spec/ee/spec/presenters/group_member_presenter_spec.rb

+require 'spec_helper'
+
+describe GroupMemberPresenter do
+  let(:user) { double(:user) }
+  let(:group) { double(:group) }
+  let(:group_member) { double(:group_member, source: group) }
+  let(:presenter) { described_class.new(group_member, current_user: user) }
+
+  describe '#can_update?' do
+    context 'when user cannot update_group_member but can override_group_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+        allow(presenter).to receive(:can?).with(user, :override_group_member, presenter).and_return(true)
+      end
+
+      it { expect(presenter.can_update?).to eq(true) }
+    end
+
+    context 'when user cannot update_group_member and cannot override_group_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+        allow(presenter).to receive(:can?).with(user, :override_group_member, presenter).and_return(false)
+      end
+
+      it { expect(presenter.can_update?).to eq(false) }
+    end
+  end
+end

spec/ee/spec/presenters/project_member_presenter_spec.rb

+require 'spec_helper'
+
+describe ProjectMemberPresenter do
+  let(:user) { double(:user) }
+  let(:project) { double(:project) }
+  let(:project_member) { double(:project_member, source: project) }
+  let(:presenter) { described_class.new(project_member, current_user: user) }
+
+  describe '#can_update?' do
+    context 'when user cannot update_project_member but can override_project_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+        allow(presenter).to receive(:can?).with(user, :override_project_member, presenter).and_return(true)
+      end
+
+      it { expect(presenter.can_update?).to eq(true) }
+    end
+
+    context 'when user cannot update_project_member and cannot override_project_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+        allow(presenter).to receive(:can?).with(user, :override_project_member, presenter).and_return(false)
+      end
+
+      it { expect(presenter.can_update?).to eq(false) }
+    end
+  end
+end

spec/lib/gitlab/view/presenter/factory_spec.rb

@@ -27,5 +27,13 @@ describe Gitlab::View::Presenter::Factory do
 
       expect(presenter).to be_a(Ci::BuildPresenter)
     end
+
+    it 'uses the presenter_class if given on #initialize' do
+      MyCustomPresenter = Class.new(described_class)
+
+      presenter = described_class.new(build, presenter_class: MyCustomPresenter).fabricate!
+
+      expect(presenter).to be_a(MyCustomPresenter)
+    end
   end
 end

spec/presenters/group_member_presenter_spec.rb

@@ -64,6 +64,7 @@ describe GroupMemberPresenter do
     context 'when user cannot update_group_member' do
       before do
         allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+        allow(presenter).to receive(:can?).with(user, :override_group_member, presenter).and_return(false)
       end
 
       it { expect(presenter.can_update?).to eq(false) }
@@ -105,6 +106,7 @@ describe GroupMemberPresenter do
       context 'when user cannot update_group_member' do
         before do
           allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+          allow(presenter).to receive(:can?).with(user, :override_group_member, presenter).and_return(false)
         end
 
         it { expect(presenter.can_approve?).to eq(false) }

spec/presenters/project_member_presenter_spec.rb

@@ -64,6 +64,7 @@ describe ProjectMemberPresenter do
     context 'when user cannot update_project_member' do
       before do
         allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+        allow(presenter).to receive(:can?).with(user, :override_project_member, presenter).and_return(false)
       end
 
       it { expect(presenter.can_update?).to eq(false) }
@@ -105,6 +106,7 @@ describe ProjectMemberPresenter do
       context 'and user cannot update_project_member' do
         before do
           allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+          allow(presenter).to receive(:can?).with(user, :override_project_member, presenter).and_return(false)
         end
 
         it { expect(presenter.can_approve?).to eq(false) }