Refactor member view by using presenter

- Create MemberPresenter alongside with GroupMemberPresenter and ProjectMemberPresenter
- Make Member model Presentable
- Move action_member_permission from MembersHelper into the MemberPresenter
- Added rspec using double, separate specs for GroupMemberPresenter and ProjectMemberPresenter

Fixes #28004.

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

app/helpers/members_helper.rb

@@ -1,11 +1,4 @@
 module MembersHelper
-  # Returns a `<action>_<source>_member` association, e.g.:
-  # - admin_project_member, update_project_member, destroy_project_member
-  # - admin_group_member, update_group_member, destroy_group_member
-  def action_member_permission(action, member)
-    "#{action}_#{member.type.underscore}".to_sym
-  end
-
   def remove_member_message(member, user: nil)
     user = current_user if defined?(current_user)
 

app/models/member.rb

@@ -4,6 +4,7 @@ class Member < ActiveRecord::Base
   include Importable
   include Expirable
   include Gitlab::Access
+  include Presentable
 
   attr_accessor :raw_invite_token
 

app/presenters/group_member_presenter.rb

@@ -0,0 +1,15 @@
+class GroupMemberPresenter < MemberPresenter
+  private
+
+  def admin_member_permission
+    :admin_group_member
+  end
+
+  def update_member_permission
+    :update_group_member
+  end
+
+  def destroy_member_permission
+    :destroy_group_member
+  end
+end

app/presenters/member_presenter.rb

@@ -0,0 +1,40 @@
+class MemberPresenter < Gitlab::View::Presenter::Delegated
+  include Gitlab::Allowable
+
+  presents :member
+
+  def can_resend_invite?
+    invite? &&
+      can?(current_user, admin_member_permission, source)
+  end
+
+  def can_update?
+    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
+
+  def can_approve?
+    request? && can_update?
+  end
+
+  private
+
+  def admin_member_permission
+    raise NotImplementedError
+  end
+
+  def update_member_permission
+    raise NotImplementedError
+  end
+
+  def destroy_member_permission
+    raise NotImplementedError
+  end
+end

app/presenters/project_member_presenter.rb

@@ -0,0 +1,15 @@
+class ProjectMemberPresenter < MemberPresenter
+  private
+
+  def admin_member_permission
+    :admin_project_member
+  end
+
+  def update_member_permission
+    :update_project_member
+  end
+
+  def destroy_member_permission
+    :destroy_project_member
+  end
+end

app/services/members/approve_access_request_service.rb

@@ -35,8 +35,17 @@ module Members
     def can_update_access_requester?(access_requester, opts = {})
       access_requester && (
         opts[:force] ||
-        can?(current_user, action_member_permission(:update, access_requester), access_requester)
+        can?(current_user, update_member_permission(access_requester), access_requester)
       )
     end
+
+    def update_member_permission(member)
+      case member
+      when GroupMember
+        :update_group_member
+      when ProjectMember
+        :update_project_member
+      end
+    end
   end
 end

app/services/members/destroy_service.rb

@@ -36,7 +36,16 @@ module Members
     end
 
     def can_destroy_member?(member)
-      member && can?(current_user, action_member_permission(:destroy, member), member)
+      member && can?(current_user, destroy_member_permission(member), member)
+    end
+
+    def destroy_member_permission(member)
+      case member
+      when GroupMember
+        :destroy_group_member
+      when ProjectMember
+        :destroy_project_member
+      end
     end
   end
 end

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

@@ -3,7 +3,7 @@
 - force_mobile_view = local_assigns.fetch(:force_mobile_view, false)
 - user = local_assigns.fetch(:user, member.user)
 - source = member.source
-- can_admin_member = can?(current_user, action_member_permission(:update, member), member)
+- member_presenter = member.present(current_user: current_user)
 
 %li.member{ class: dom_class(member), id: dom_id(member) }
   %span.list-item-name
@@ -50,18 +50,18 @@
     .controls.member-controls
       - if show_controls && member.source == current_resource
 
-        - if member.invite? && can?(current_user, action_member_permission(:admin, member), member.source)
+        - if member_presenter.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 && can_admin_member
+        - if user != current_user && member_presenter.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: !can_admin_member,
+                disabled: member_presenter.cannot_update?,
                 data: { toggle: "dropdown", field_name: "#{f.object_name}[access_level]" } }
                 %span.dropdown-toggle-text
                   = member.human_access
@@ -76,17 +76,17 @@
                           class: ("is-active" if member.access_level == role_id),
                           data: { id: role_id, el_id: dom_id(member) }
             .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: !can_admin_member, data: { el_id: dom_id(member) }
+              = 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) }
               %i.clear-icon.js-clear-input
         - else
           %span.member-access-text= member.human_access
 
-        - if member.invite? && can?(current_user, action_member_permission(:admin, member), member.source)
+        - 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.request? && can_admin_member
+        - elsif member_presenter.can_approve?
           = link_to polymorphic_path([:approve_access_request, member]),
                     method: :post,
                     class: 'btn btn-success prepend-left-10',
@@ -96,7 +96,7 @@
             - unless force_mobile_view
               = icon('check inverse', class: 'hidden-xs')
 
-        - if can?(current_user, action_member_permission(:destroy, member), member)
+        - if member_presenter.can_remove?
           - if current_user == user
             = link_to icon('sign-out', text: 'Leave'), polymorphic_path([:leave, member.source, :members]),
                       method: :delete,

changelogs/unreleased/28004-consider-refactoring-member-view-by-using-presenter.yml

@@ -0,0 +1,4 @@
+---
+title: Refactor member view using a Presenter
+merge_request: 9645
+author: TM Lee

spec/helpers/members_helper_spec.rb

@@ -1,14 +1,6 @@
 require 'spec_helper'
 
 describe MembersHelper do
-  describe '#action_member_permission' do
-    let(:project_member) { build(:project_member) }
-    let(:group_member) { build(:group_member) }
-
-    it { expect(action_member_permission(:admin, project_member)).to eq :admin_project_member }
-    it { expect(action_member_permission(:admin, group_member)).to eq :admin_group_member }
-  end
-
   describe '#remove_member_message' do
     let(:requester) { create(:user) }
     let(:project) { create(:project, :public, :access_requestable) }

spec/presenters/group_member_presenter_spec.rb

@@ -0,0 +1,136 @@
+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_resend_invite?' do
+    context 'when group_member is invited' do
+      before do
+        expect(group_member).to receive(:invite?).and_return(true)
+      end
+
+      context 'and user can admin_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_group_member, group).and_return(true)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(true) }
+      end
+
+      context 'and user cannot admin_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_group_member, group).and_return(false)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(false) }
+      end
+    end
+
+    context 'when group_member is not invited' do
+      before do
+        expect(group_member).to receive(:invite?).and_return(false)
+      end
+
+      context 'and user can admin_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_group_member, group).and_return(true)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(false) }
+      end
+
+      context 'and user cannot admin_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_group_member, group).and_return(false)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(false) }
+      end
+    end
+  end
+
+  describe '#can_update?' do
+    context 'when user can update_group_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(true)
+      end
+
+      it { expect(presenter.can_update?).to eq(true) }
+    end
+
+    context 'when user cannot update_group_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+      end
+
+      it { expect(presenter.can_update?).to eq(false) }
+    end
+  end
+
+  describe '#can_remove?' do
+    context 'when user can destroy_group_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :destroy_group_member, presenter).and_return(true)
+      end
+
+      it { expect(presenter.can_remove?).to eq(true) }
+    end
+
+    context 'when user cannot destroy_group_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :destroy_group_member, presenter).and_return(false)
+      end
+
+      it { expect(presenter.can_remove?).to eq(false) }
+    end
+  end
+
+  describe '#can_approve?' do
+    context 'when group_member has request an invite' do
+      before do
+        expect(group_member).to receive(:request?).and_return(true)
+      end
+
+      context 'when user can update_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(true)
+        end
+
+        it { expect(presenter.can_approve?).to eq(true) }
+      end
+
+      context 'when user cannot update_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+        end
+
+        it { expect(presenter.can_approve?).to eq(false) }
+      end
+    end
+
+    context 'when group_member did not request an invite' do
+      before do
+        expect(group_member).to receive(:request?).and_return(false)
+      end
+
+      context 'when user can update_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(true)
+        end
+
+        it { expect(presenter.can_approve?).to eq(false) }
+      end
+
+      context 'when user cannot update_group_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_group_member, presenter).and_return(false)
+        end
+
+        it { expect(presenter.can_approve?).to eq(false) }
+      end
+    end
+  end
+end

spec/presenters/project_member_presenter_spec.rb

@@ -0,0 +1,136 @@
+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_resend_invite?' do
+    context 'when project_member is invited' do
+      before do
+        expect(project_member).to receive(:invite?).and_return(true)
+      end
+
+      context 'and user can admin_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_project_member, project).and_return(true)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(true) }
+      end
+
+      context 'and user cannot admin_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_project_member, project).and_return(false)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(false) }
+      end
+    end
+
+    context 'when project_member is not invited' do
+      before do
+        expect(project_member).to receive(:invite?).and_return(false)
+      end
+
+      context 'and user can admin_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_project_member, project).and_return(true)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(false) }
+      end
+
+      context 'and user cannot admin_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :admin_project_member, project).and_return(false)
+        end
+
+        it { expect(presenter.can_resend_invite?).to eq(false) }
+      end
+    end
+  end
+
+  describe '#can_update?' do
+    context 'when user can update_project_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(true)
+      end
+
+      it { expect(presenter.can_update?).to eq(true) }
+    end
+
+    context 'when user cannot update_project_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+      end
+
+      it { expect(presenter.can_update?).to eq(false) }
+    end
+  end
+
+  describe '#can_remove?' do
+    context 'when user can destroy_project_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :destroy_project_member, presenter).and_return(true)
+      end
+
+      it { expect(presenter.can_remove?).to eq(true) }
+    end
+
+    context 'when user cannot destroy_project_member' do
+      before do
+        allow(presenter).to receive(:can?).with(user, :destroy_project_member, presenter).and_return(false)
+      end
+
+      it { expect(presenter.can_remove?).to eq(false) }
+    end
+  end
+
+  describe '#can_approve?' do
+    context 'when project_member has request an invite' do
+      before do
+        expect(project_member).to receive(:request?).and_return(true)
+      end
+
+      context 'and user can update_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(true)
+        end
+
+        it { expect(presenter.can_approve?).to eq(true) }
+      end
+
+      context 'and user cannot update_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+        end
+
+        it { expect(presenter.can_approve?).to eq(false) }
+      end
+    end
+
+    context 'when project_member did not request an invite' do
+      before do
+        expect(project_member).to receive(:request?).and_return(false)
+      end
+
+      context 'and user can update_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(true)
+        end
+
+        it { expect(presenter.can_approve?).to eq(false) }
+      end
+
+      context 'and user cannot update_project_member' do
+        before do
+          allow(presenter).to receive(:can?).with(user, :update_project_member, presenter).and_return(false)
+        end
+
+        it { expect(presenter.can_approve?).to eq(false) }
+      end
+    end
+  end
+end