diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 2ce26de1768..a94726887d9 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -1,4 +1,6 @@ class Admin::GroupsController < Admin::ApplicationController + include MembersPresentation + before_action :group, only: [:edit, :update, :destroy, :project_update, :members_update] def index @@ -10,8 +12,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 diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb index 50cf2643390..3afe66c3566 100644 --- a/app/controllers/admin/projects_controller.rb +++ b/app/controllers/admin/projects_controller.rb @@ -1,4 +1,6 @@ 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 diff --git a/app/controllers/concerns/members_presentation.rb b/app/controllers/concerns/members_presentation.rb new file mode 100644 index 00000000000..c0622516fd3 --- /dev/null +++ b/app/controllers/concerns/members_presentation.rb @@ -0,0 +1,11 @@ +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 diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 5919bf54468..21e77431176 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -1,5 +1,6 @@ class Groups::GroupMembersController < Groups::ApplicationController include MembershipActions + include MembersPresentation include SortingHelper # Authorize @@ -14,15 +15,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.members_and_requesters.find(params[:id]) + .present(current_user: current_user) return render_403 unless can?(current_user, :update_group_member, @group_member) diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 5a01a59481b..d7372beb9d3 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -1,5 +1,6 @@ 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.members_and_requesters.find(params[:id]) + .present(current_user: current_user) return render_403 unless can?(current_user, :update_project_member, @project_member) diff --git a/app/presenters/member_presenter.rb b/app/presenters/member_presenter.rb index a8732226018..7d2f9303b8f 100644 --- a/app/presenters/member_presenter.rb +++ b/app/presenters/member_presenter.rb @@ -1,8 +1,10 @@ class MemberPresenter < Gitlab::View::Presenter::Delegated - include Gitlab::Allowable - presents :member + def access_level_roles + member.class.access_level_roles + end + def can_resend_invite? invite? && can?(current_user, admin_member_permission, source) @@ -12,10 +14,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 diff --git a/app/presenters/members_presenter.rb b/app/presenters/members_presenter.rb new file mode 100644 index 00000000000..e4aba37b69e --- /dev/null +++ b/app/presenters/members_presenter.rb @@ -0,0 +1,15 @@ +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 diff --git a/app/views/projects/project_members/_team.html.haml b/app/views/projects/project_members/_team.html.haml index e71d58ec26d..16bcf671c25 100644 --- a/app/views/projects/project_members/_team.html.haml +++ b/app/views/projects/project_members/_team.html.haml @@ -1,11 +1,13 @@ +- 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" } diff --git a/app/views/projects/project_members/index.html.haml b/app/views/projects/project_members/index.html.haml index fd5d3ec56da..d81103c3a92 100644 --- a/app/views/projects/project_members/index.html.haml +++ b/app/views/projects/project_members/index.html.haml @@ -37,5 +37,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" diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index e53cb4eff75..71878e93255 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -1,9 +1,9 @@ - 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), id: dom_id(member) } %span.list-item-name @@ -50,18 +50,17 @@ .controls.member-controls - 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? + - 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?, data: { toggle: "dropdown", field_name: "#{f.object_name}[access_level]" } } %span.dropdown-toggle-text = member.human_access @@ -70,23 +69,22 @@ = 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) } .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, + 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', @@ -96,7 +94,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, diff --git a/app/views/shared/members/_requests.html.haml b/app/views/shared/members/_requests.html.haml index 09b9944082f..1fbd6bcc4cb 100644 --- a/app/views/shared/members/_requests.html.haml +++ b/app/views/shared/members/_requests.html.haml @@ -1,10 +1,13 @@ +- 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 } diff --git a/lib/gitlab/view/presenter/factory.rb b/lib/gitlab/view/presenter/factory.rb index d172d61e2c9..570f0723e39 100644 --- a/lib/gitlab/view/presenter/factory.rb +++ b/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 diff --git a/spec/lib/gitlab/view/presenter/factory_spec.rb b/spec/lib/gitlab/view/presenter/factory_spec.rb index 70d2e22b48f..6120bafb2e3 100644 --- a/spec/lib/gitlab/view/presenter/factory_spec.rb +++ b/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 diff --git a/spec/presenters/group_member_presenter_spec.rb b/spec/presenters/group_member_presenter_spec.rb index 05a6dee3196..c00e41725d9 100644 --- a/spec/presenters/group_member_presenter_spec.rb +++ b/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) } diff --git a/spec/presenters/project_member_presenter_spec.rb b/spec/presenters/project_member_presenter_spec.rb index eb93f03a958..83db5c56cdf 100644 --- a/spec/presenters/project_member_presenter_spec.rb +++ b/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) }