Commit 031b64bf authored by Robert Speicher's avatar Robert Speicher

Merge branch...

Merge branch 'ee-tmlee/gitlab-ce-28004-consider-refactoring-member-view-by-using-presenter' into 'master'

[EE] Refactor member view by using presenter

See merge request gitlab-org/gitlab-ee!3620
parents 3e1190cf d578a542
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
......
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
......
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
......@@ -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)
......
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)
......
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, override_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)
......
......@@ -4,6 +4,7 @@ class Member < ActiveRecord::Base
include Importable
include Expirable
include Gitlab::Access
include Presentable
attr_accessor :raw_invite_token
attr_accessor :skip_notification
......
class GroupMemberPresenter < MemberPresenter
prepend EE::GroupMemberPresenter
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
class MemberPresenter < Gitlab::View::Presenter::Delegated
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)
end
def can_update?
can?(current_user, update_member_permission, member)
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
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
class ProjectMemberPresenter < MemberPresenter
prepend EE::ProjectMemberPresenter
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
......@@ -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
......@@ -41,7 +41,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
- 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" }
......
......@@ -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"
- 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
- can_admin_member = can?(current_user, action_member_permission(:update, member), member)
-# EE-only
- can_override_member = can?(current_user, action_member_permission(:override, member), member)
%li.member{ class: [dom_class(member), ("is-overriden" if member.override)], id: dom_id(member) }
%span.list-item-name
......@@ -51,21 +48,21 @@
- if show_roles
- current_resource = @project || @group
.controls.member-controls
= render 'shared/members/ee/ldap_tag', can_override: can_override_member, visible: false
= render 'shared/members/ee/ldap_tag', can_override: member.can_override?, visible: false
- if show_controls && member.source == current_resource
- if member.invite? && can?(current_user, action_member_permission(:admin, member), member.source)
- 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 && (can_admin_member || can_override_member)
- 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: !can_admin_member,
disabled: member.can_override?,
data: { toggle: "dropdown", field_name: "#{f.object_name}[access_level]" } }
%span.dropdown-toggle-text
= member.human_access
......@@ -74,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: can_override_member
= 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: !can_admin_member, 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.invite? && can?(current_user, action_member_permission(:admin, member), member.source)
= 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
- if member.can_approve?
= link_to polymorphic_path([:approve_access_request, member]),
method: :post,
class: 'btn btn-success prepend-left-10',
......@@ -101,7 +101,7 @@
- unless force_mobile_view
= icon('check inverse', class: 'hidden-xs')
- if can?(current_user, action_member_permission(:destroy, member), member)
- if member.can_remove?
- if current_user == user
= link_to icon('sign-out', text: 'Leave'), polymorphic_path([:leave, member.source, :members]),
method: :delete,
......@@ -117,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: can_override_member
= 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: can_override_member
= render 'shared/members/ee/override_member_buttons', group: @group, member: member, user: user, action: :confirm, can_override: member.can_override?
- 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 }
---
title: Refactor member view using a Presenter
merge_request: 9645
author: TM Lee
module EE
module GroupMemberPresenter
private
def override_member_permission
:override_group_member
end
end
end
module EE
module MemberPresenter
def can_update?
super || can_override?
end
def can_override?
can?(current_user, override_member_permission, member)
end
private
def override_member_permission
raise NotImplementedError
end
end
end
module EE
module ProjectMemberPresenter
private
def override_member_permission
:override_project_member
end
end
end
......@@ -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
......
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
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
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) }
......
......@@ -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
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)
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
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)
allow(presenter).to receive(:can?).with(user, :override_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
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)
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
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)
allow(presenter).to receive(:can?).with(user, :override_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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment