From 2cf3fc18a6e111a820f9842bb1d939790e8625eb Mon Sep 17 00:00:00 2001 From: TM Lee Date: Thu, 2 Mar 2017 14:01:02 +0800 Subject: [PATCH] Refactor member view by using presenter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- app/helpers/members_helper.rb | 7 - app/models/member.rb | 1 + app/presenters/group_member_presenter.rb | 15 ++ app/presenters/member_presenter.rb | 40 ++++++ app/presenters/project_member_presenter.rb | 15 ++ .../members/approve_access_request_service.rb | 11 +- app/services/members/destroy_service.rb | 11 +- app/views/shared/members/_member.html.haml | 16 +-- ...ctoring-member-view-by-using-presenter.yml | 4 + spec/helpers/members_helper_spec.rb | 8 -- .../presenters/group_member_presenter_spec.rb | 136 ++++++++++++++++++ .../project_member_presenter_spec.rb | 136 ++++++++++++++++++ 12 files changed, 375 insertions(+), 25 deletions(-) create mode 100644 app/presenters/group_member_presenter.rb create mode 100644 app/presenters/member_presenter.rb create mode 100644 app/presenters/project_member_presenter.rb create mode 100644 changelogs/unreleased/28004-consider-refactoring-member-view-by-using-presenter.yml create mode 100644 spec/presenters/group_member_presenter_spec.rb create mode 100644 spec/presenters/project_member_presenter_spec.rb diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index 41d471cc92f..a3129cac2b1 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -1,11 +1,4 @@ module MembersHelper - # Returns a `__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) diff --git a/app/models/member.rb b/app/models/member.rb index 2fe5fda985f..c47145667b5 100644 --- a/app/models/member.rb +++ b/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 diff --git a/app/presenters/group_member_presenter.rb b/app/presenters/group_member_presenter.rb new file mode 100644 index 00000000000..8f53dfa105e --- /dev/null +++ b/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 diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb new file mode 100644 index 00000000000..a8732226018 --- /dev/null +++ b/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 diff --git a/app/presenters/project_member_presenter.rb b/app/presenters/project_member_presenter.rb new file mode 100644 index 00000000000..7f42d2b70df --- /dev/null +++ b/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 diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb index c13f289f61e..2a2bb0cae5b 100644 --- a/app/services/members/approve_access_request_service.rb +++ b/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 diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index 46c505baf8b..05b93ac8fdb 100644 --- a/app/services/members/destroy_service.rb +++ b/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 diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 2c27dd638a7..e53cb4eff75 100644 --- a/app/views/shared/members/_member.html.haml +++ b/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, diff --git a/changelogs/unreleased/28004-consider-refactoring-member-view-by-using-presenter.yml b/changelogs/unreleased/28004-consider-refactoring-member-view-by-using-presenter.yml new file mode 100644 index 00000000000..0e91d4ae403 --- /dev/null +++ b/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 diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index 33186cf50d5..45ffbeb27a4 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/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) } diff --git a/spec/presenters/group_member_presenter_spec.rb b/spec/presenters/group_member_presenter_spec.rb new file mode 100644 index 00000000000..05a6dee3196 --- /dev/null +++ b/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 diff --git a/spec/presenters/project_member_presenter_spec.rb b/spec/presenters/project_member_presenter_spec.rb new file mode 100644 index 00000000000..eb93f03a958 --- /dev/null +++ b/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