From bd78f5733ca546bf940438b84aefa2fa3abacb36 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Mon, 27 Jun 2016 16:20:57 +0200
Subject: [PATCH] Exclude requesters from Project#members, Group#members and
User#members
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
And create new Project#requesters, Group#requesters scopes.
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
app/controllers/admin/groups_controller.rb | 1 +
app/controllers/admin/projects_controller.rb | 3 +-
.../concerns/membership_actions.rb | 5 +-
.../groups/group_members_controller.rb | 6 +-
.../projects/project_members_controller.rb | 9 ++-
app/helpers/members_helper.rb | 11 ++++
app/models/group.rb | 9 +--
app/models/member.rb | 2 -
app/models/project.rb | 8 ++-
app/models/project_team.rb | 14 ++--
app/views/admin/groups/show.html.haml | 8 +--
app/views/admin/projects/show.html.haml | 12 ++--
.../groups/group_members/index.html.haml | 10 +--
.../layouts/nav/_group_settings.html.haml | 2 +-
app/views/layouts/nav/_project.html.haml | 2 +-
.../_shared_group_members.html.haml | 2 +-
.../projects/project_members/index.html.haml | 6 +-
.../members/_access_request_buttons.html.haml | 16 ++---
app/views/shared/members/_requests.html.haml | 6 +-
.../groups/group_members_controller_spec.rb | 6 +-
.../project_members_controller_spec.rb | 6 +-
.../owner_manages_access_requests_spec.rb | 2 +-
.../members/user_requests_access_spec.rb | 8 +--
.../master_manages_access_requests_spec.rb | 2 +-
.../members/user_requests_access_spec.rb | 8 +--
spec/helpers/members_helper_spec.rb | 66 +++++++++++++++++++
spec/mailers/notify_spec.rb | 10 +--
.../concerns/access_requestable_spec.rb | 4 +-
spec/models/group_spec.rb | 29 ++++++++
spec/models/member_spec.rb | 20 +-----
spec/models/project_spec.rb | 30 +++++++++
31 files changed, 225 insertions(+), 98 deletions(-)
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index a6db4690df0..94b5aaa71d0 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -10,6 +10,7 @@ class Admin::GroupsController < Admin::ApplicationController
def show
@members = @group.members.order("access_level DESC").page(params[:members_page])
+ @requesters = @group.requesters
@projects = @group.projects.page(params[:projects_page])
end
diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb
index 87986fdf8b1..4c9c6362ffc 100644
--- a/app/controllers/admin/projects_controller.rb
+++ b/app/controllers/admin/projects_controller.rb
@@ -20,7 +20,8 @@ class Admin::ProjectsController < Admin::ApplicationController
@group_members = @group.members.order("access_level DESC").page(params[:group_members_page])
end
- @project_members = @project.project_members.page(params[:project_members_page])
+ @project_members = @project.members.page(params[:project_members_page])
+ @requesters = @project.requesters
end
def transfer
diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index 52dc396af6a..52682ef9dc9 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -10,7 +10,7 @@ module MembershipActions
end
def approve_access_request
- @member = membershipable.members.request.find(params[:id])
+ @member = membershipable.requesters.find(params[:id])
return render_403 unless can?(current_user, action_member_permission(:update, @member), @member)
@@ -20,7 +20,8 @@ module MembershipActions
end
def leave
- @member = membershipable.members.find_by(user_id: current_user)
+ @member = membershipable.members.find_by(user_id: current_user) ||
+ membershipable.requesters.find_by(user_id: current_user)
Members::DestroyService.new(@member, current_user).execute
source_type = @member.real_source_type.humanize(capitalize: false)
diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb
index 2c49fe3833e..9fc41a12536 100644
--- a/app/controllers/groups/group_members_controller.rb
+++ b/app/controllers/groups/group_members_controller.rb
@@ -7,7 +7,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
def index
@project = @group.projects.find(params[:project_id]) if params[:project_id]
@members = @group.group_members
- @members = @members.non_pending unless can?(current_user, :admin_group, @group)
+ @members = @members.non_invite unless can?(current_user, :admin_group, @group)
if params[:search].present?
users = @group.users.search(params[:search]).to_a
@@ -15,6 +15,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
end
@members = @members.order('access_level DESC').page(params[:page]).per(50)
+ @requesters = @group.requesters if can?(current_user, :admin_group, @group)
@group_member = @group.group_members.new
end
@@ -34,7 +35,8 @@ class Groups::GroupMembersController < Groups::ApplicationController
end
def destroy
- @group_member = @group.group_members.find(params[:id])
+ @group_member = @group.members.find_by(id: params[:id]) ||
+ @group.requesters.find_by(id: params[:id])
Members::DestroyService.new(@group_member, current_user).execute
diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb
index 6ba32d33403..3435a118964 100644
--- a/app/controllers/projects/project_members_controller.rb
+++ b/app/controllers/projects/project_members_controller.rb
@@ -6,7 +6,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
def index
@project_members = @project.project_members
- @project_members = @project_members.non_pending unless can?(current_user, :admin_project, @project)
+ @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project)
if params[:search].present?
users = @project.users.search(params[:search]).to_a
@@ -19,7 +19,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController
if @group
@group_members = @group.group_members
- @group_members = @group_members.non_pending unless can?(current_user, :admin_group, @group)
+ @group_members = @group_members.non_invite unless can?(current_user, :admin_group, @group)
if params[:search].present?
users = @group.users.search(params[:search]).to_a
@@ -29,6 +29,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController
@group_members = @group_members.order('access_level DESC')
end
+ @requesters = @project.requesters if can?(current_user, :admin_project, @project)
+
@project_member = @project.project_members.new
@project_group_links = @project.project_group_links
end
@@ -48,7 +50,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController
end
def destroy
- @project_member = @project.project_members.find(params[:id])
+ @project_member = @project.members.find_by(id: params[:id]) ||
+ @project.requesters.find_by(id: params[:id])
Members::DestroyService.new(@project_member, current_user).execute
diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb
index ec106418f2d..c70cd19b587 100644
--- a/app/helpers/members_helper.rb
+++ b/app/helpers/members_helper.rb
@@ -12,6 +12,17 @@ module MembersHelper
can?(current_user, action_member_permission(:admin, member), member.source)
end
+ def can_see_request_access_button?(source)
+ source_parent = source.respond_to?(:group) && source.group
+
+ return false if source_parent && source.group.members.exists?(user_id: current_user.id)
+ return false if source_parent && source.group.requesters.exists?(user_id: current_user.id)
+ return false if source.members.exists?(user_id: current_user.id)
+ return true if source.requesters.exists?(user_id: current_user.id)
+
+ true
+ end
+
def remove_member_message(member, user: nil)
user = current_user if defined?(current_user)
diff --git a/app/models/group.rb b/app/models/group.rb
index c70c719e338..a8be7004ee8 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -6,15 +6,16 @@ class Group < Namespace
include AccessRequestable
include Referable
- has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember'
+ has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember'
alias_method :members, :group_members
- has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members
-
+ has_many :users, through: :group_members
has_many :owners,
- -> { where(members: { requested_at: nil, access_level: Gitlab::Access::OWNER }) },
+ -> { where(members: { access_level: Gitlab::Access::OWNER }) },
through: :group_members,
source: :user
+ has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember'
+
has_many :project_group_links, dependent: :destroy
has_many :shared_projects, through: :project_group_links, source: :project
has_many :notification_settings, dependent: :destroy, as: :source
diff --git a/app/models/member.rb b/app/models/member.rb
index 57161397e2b..44db3d977fa 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -30,8 +30,6 @@ class Member < ActiveRecord::Base
scope :invite, -> { where.not(invite_token: nil) }
scope :non_invite, -> { where(invite_token: nil) }
scope :request, -> { where.not(requested_at: nil) }
- scope :non_request, -> { where(requested_at: nil) }
- scope :non_pending, -> { non_request.non_invite }
scope :has_access, -> { where('access_level > 0') }
scope :guests, -> { where(access_level: GUEST) }
diff --git a/app/models/project.rb b/app/models/project.rb
index 6a950ee830d..ae96f00a705 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -108,9 +108,13 @@ class Project < ActiveRecord::Base
has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet'
has_many :hooks, dependent: :destroy, class_name: 'ProjectHook'
has_many :protected_branches, dependent: :destroy
- has_many :project_members, dependent: :destroy, as: :source, class_name: 'ProjectMember'
+
+ has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'ProjectMember'
alias_method :members, :project_members
- has_many :users, -> { where(members: { requested_at: nil }) }, through: :project_members
+ has_many :users, through: :project_members
+
+ has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'ProjectMember'
+
has_many :deploy_keys_projects, dependent: :destroy
has_many :deploy_keys, through: :deploy_keys_projects
has_many :users_star_projects, dependent: :destroy
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 0865b979ce0..0b700930641 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -22,12 +22,12 @@ class ProjectTeam
end
def find_member(user_id)
- member = project.members.non_request.find_by(user_id: user_id)
+ member = project.members.find_by(user_id: user_id)
# If user is not in project members
# we should check for group membership
if group && !member
- member = group.members.non_request.find_by(user_id: user_id)
+ member = group.members.find_by(user_id: user_id)
end
member
@@ -137,10 +137,10 @@ class ProjectTeam
def max_member_access(user_id)
access = []
- access += project.members.non_request.where(user_id: user_id).has_access.pluck(:access_level)
+ access += project.members.where(user_id: user_id).has_access.pluck(:access_level)
if group
- access += group.members.non_request.where(user_id: user_id).has_access.pluck(:access_level)
+ access += group.members.where(user_id: user_id).has_access.pluck(:access_level)
end
if project.invited_groups.any? && project.allowed_to_share_with_group?
@@ -168,14 +168,14 @@ class ProjectTeam
end
def fetch_members(level = nil)
- project_members = project.members.non_request
- group_members = group ? group.members.non_request : []
+ project_members = project.members
+ group_members = group ? group.members : []
invited_members = []
if project.invited_groups.any? && project.allowed_to_share_with_group?
project.project_group_links.each do |group_link|
invited_group = group_link.group
- im = invited_group.members.non_request
+ im = invited_group.members
if level
int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize]
diff --git a/app/views/admin/groups/show.html.haml b/app/views/admin/groups/show.html.haml
index 50770465f07..522153b37e3 100644
--- a/app/views/admin/groups/show.html.haml
+++ b/app/views/admin/groups/show.html.haml
@@ -89,16 +89,16 @@
%hr
= button_tag 'Add users to group', class: "btn btn-create"
- = render 'shared/members/requests', membership_source: @group, members: @members.request
+ = render 'shared/members/requests', membership_source: @group, requesters: @requesters
.panel.panel-default
.panel-heading
%strong= @group.name
group members
- %span.badge= @group.members.non_request.size
+ %span.badge= @group.members.size
.pull-right
= link_to icon('pencil-square-o', text: 'Manage Access'), polymorphic_url([@group, :members]), class: "btn btn-xs"
%ul.well-list.group-users-list.content-list
- = render partial: 'shared/members/member', collection: @members.non_request, as: :member, locals: { show_controls: false }
+ = render partial: 'shared/members/member', collection: @members, as: :member, locals: { show_controls: false }
.panel-footer
- = paginate @members.non_request, param_name: 'members_page', theme: 'gitlab'
+ = paginate @members, param_name: 'members_page', theme: 'gitlab'
diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml
index 461d588415d..82d3169c6f9 100644
--- a/app/views/admin/projects/show.html.haml
+++ b/app/views/admin/projects/show.html.haml
@@ -137,16 +137,16 @@
.panel-heading
%strong= @group.name
group members
- %span.badge= @group_members.non_request.size
+ %span.badge= @group_members.size
.pull-right
= link_to admin_group_path(@group), class: 'btn btn-xs' do
= icon('pencil-square-o', text: 'Manage Access')
%ul.well-list.content-list
- = render partial: 'shared/members/member', collection: @group_members.non_request, as: :member, locals: { show_controls: false }
+ = render partial: 'shared/members/member', collection: @group_members, as: :member, locals: { show_controls: false }
.panel-footer
- = paginate @group_members.non_request, param_name: 'group_members_page', theme: 'gitlab'
+ = paginate @group_members, param_name: 'group_members_page', theme: 'gitlab'
- = render 'shared/members/requests', membership_source: @project, members: @project_members.request
+ = render 'shared/members/requests', membership_source: @project, requesters: @requesters
.panel.panel-default
.panel-heading
@@ -156,6 +156,6 @@
.pull-right
= link_to icon('pencil-square-o', text: 'Manage Access'), polymorphic_url([@project, :members]), class: "btn btn-xs"
%ul.well-list.project_members.content-list
- = render partial: 'shared/members/member', collection: @project_members.non_request, as: :member, locals: { show_controls: false }
+ = render partial: 'shared/members/member', collection: @project_members, as: :member, locals: { show_controls: false }
.panel-footer
- = paginate @project_members.non_request, param_name: 'project_members_page', theme: 'gitlab'
+ = paginate @project_members, param_name: 'project_members_page', theme: 'gitlab'
diff --git a/app/views/groups/group_members/index.html.haml b/app/views/groups/group_members/index.html.haml
index d6acade84f1..90f362c052b 100644
--- a/app/views/groups/group_members/index.html.haml
+++ b/app/views/groups/group_members/index.html.haml
@@ -1,7 +1,7 @@
- page_title "Members"
.group-members-page.prepend-top-default
- - if current_user && current_user.can?(:admin_group_member, @group)
+ - if can?(current_user, :admin_group_member, @group)
.panel.panel-default
.panel-heading
Add new user to group
@@ -11,13 +11,13 @@
.new-group-member-holder
= render "new_group_member"
- = render 'shared/members/requests', membership_source: @group, members: @members.request
+ = render 'shared/members/requests', membership_source: @group, requesters: @requesters
.panel.panel-default
.panel-heading
%strong #{@group.name}
group members
- %span.badge= @members.non_request.size
+ %span.badge= @members.size
.controls
= form_tag group_group_members_path(@group), method: :get, class: 'form-inline member-search-form' do
.form-group
@@ -25,8 +25,8 @@
= button_tag class: 'btn', title: 'Search' do
= icon("search")
%ul.content-list
- = render partial: 'shared/members/member', collection: @members.non_request, as: :member
- = paginate @members.non_request, theme: 'gitlab'
+ = render partial: 'shared/members/member', collection: @members, as: :member
+ = paginate @members, theme: 'gitlab'
:javascript
$('form.member-search-form').on('submit', function(event) {
diff --git a/app/views/layouts/nav/_group_settings.html.haml b/app/views/layouts/nav/_group_settings.html.haml
index 3a24b09ab7e..bf9a7ecb786 100644
--- a/app/views/layouts/nav/_group_settings.html.haml
+++ b/app/views/layouts/nav/_group_settings.html.haml
@@ -1,6 +1,6 @@
- if current_user
- can_edit = can?(current_user, :admin_group, @group)
- - member = @group.members.non_request.find_by(user_id: current_user.id)
+ - member = @group.members.find_by(user_id: current_user.id)
- can_leave = member && can?(current_user, :destroy_group_member, member)
.controls
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index dcef427cda3..9e65d94186b 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -7,7 +7,7 @@
%ul.dropdown-menu.dropdown-menu-align-right
- can_edit = can?(current_user, :admin_project, @project)
-# We don't use @project.team.find_member because it searches for group members too...
- - member = @project.members.non_request.find_by(user_id: current_user.id)
+ - member = @project.members.find_by(user_id: current_user.id)
- can_leave = member && can?(current_user, :destroy_project_member, member)
= render 'layouts/nav/project_settings', can_edit: can_edit
diff --git a/app/views/projects/project_members/_shared_group_members.html.haml b/app/views/projects/project_members/_shared_group_members.html.haml
index 840b57c2e63..77370c14def 100644
--- a/app/views/projects/project_members/_shared_group_members.html.haml
+++ b/app/views/projects/project_members/_shared_group_members.html.haml
@@ -1,6 +1,6 @@
- @project_group_links.each do |group_links|
- shared_group = group_links.group
- - shared_group_members = shared_group.members.non_request
+ - shared_group_members = shared_group.members
- shared_group_users_count = shared_group_members.size
.panel.panel-default
.panel-heading
diff --git a/app/views/projects/project_members/index.html.haml b/app/views/projects/project_members/index.html.haml
index a2026c41d01..9031f01b496 100644
--- a/app/views/projects/project_members/index.html.haml
+++ b/app/views/projects/project_members/index.html.haml
@@ -13,12 +13,12 @@
Users with access to this project are listed below.
= render "new_project_member"
- = render 'shared/members/requests', membership_source: @project, members: @project_members.request
+ = render 'shared/members/requests', membership_source: @project, requesters: @requesters
- = render 'team', members: @project_members.non_request
+ = render 'team', members: @project_members
- if @group
- = render "group_members", members: @group_members.non_request
+ = render "group_members", members: @group_members
- if @project_group_links.any? && @project.allowed_to_share_with_group?
= render "shared_group_members"
diff --git a/app/views/shared/members/_access_request_buttons.html.haml b/app/views/shared/members/_access_request_buttons.html.haml
index c56418f052a..35dcdccc921 100644
--- a/app/views/shared/members/_access_request_buttons.html.haml
+++ b/app/views/shared/members/_access_request_buttons.html.haml
@@ -1,13 +1,9 @@
-- member = source.members.find_by(user_id: current_user.id)
-- group_member = source.group.members.find_by(user_id: current_user.id) if source.respond_to?(:group) && source.group
-
-- unless group_member
- - if member
- - if member.request?
- = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]),
- method: :delete,
- data: { confirm: remove_member_message(member) },
- class: 'btn'
+- if can_see_request_access_button?(source)
+ - if requester = source.requesters.find_by(user_id: current_user.id)
+ = link_to 'Withdraw Access Request', polymorphic_path([:leave, source, :members]),
+ method: :delete,
+ data: { confirm: remove_member_message(requester) },
+ class: 'btn'
- else
= link_to 'Request Access', polymorphic_path([:request_access, source, :members]),
method: :post,
diff --git a/app/views/shared/members/_requests.html.haml b/app/views/shared/members/_requests.html.haml
index e4bd2bdc265..40b39e850b0 100644
--- a/app/views/shared/members/_requests.html.haml
+++ b/app/views/shared/members/_requests.html.haml
@@ -1,8 +1,8 @@
-- if members.any?
+- if requesters.any?
.panel.panel-default
.panel-heading
%strong= membership_source.name
access requests
- %span.badge= members.size
+ %span.badge= requesters.size
%ul.content-list
- = render partial: 'shared/members/member', collection: members, as: :member
+ = render partial: 'shared/members/member', collection: requesters, as: :member
diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb
index ddc54108a7b..c34475976c6 100644
--- a/spec/controllers/groups/group_members_controller_spec.rb
+++ b/spec/controllers/groups/group_members_controller_spec.rb
@@ -133,7 +133,7 @@ describe Groups::GroupMembersController do
expect(response).to set_flash.to 'Your access request to the group has been withdrawn.'
expect(response).to redirect_to(group_path(group))
- expect(group.members.request).to be_empty
+ expect(group.requesters).to be_empty
expect(group.users).not_to include user
end
end
@@ -153,7 +153,7 @@ describe Groups::GroupMembersController do
expect(response).to set_flash.to 'Your request for access has been queued for review.'
expect(response).to redirect_to(group_path(group))
- expect(group.members.request.exists?(user_id: user)).to be_truthy
+ expect(group.requesters.exists?(user_id: user)).to be_truthy
expect(group.users).not_to include user
end
end
@@ -175,7 +175,7 @@ describe Groups::GroupMembersController do
let(:group_requester) { create(:user) }
let(:member) do
group.request_access(group_requester)
- group.members.request.find_by(user_id: group_requester)
+ group.requesters.find_by(user_id: group_requester)
end
context 'when user does not have enough rights' do
diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb
index 29aaceb2302..5e2a8cf3849 100644
--- a/spec/controllers/projects/project_members_controller_spec.rb
+++ b/spec/controllers/projects/project_members_controller_spec.rb
@@ -187,7 +187,7 @@ describe Projects::ProjectMembersController do
expect(response).to set_flash.to 'Your access request to the project has been withdrawn.'
expect(response).to redirect_to(namespace_project_path(project.namespace, project))
- expect(project.members.request).to be_empty
+ expect(project.requesters).to be_empty
expect(project.users).not_to include user
end
end
@@ -210,7 +210,7 @@ describe Projects::ProjectMembersController do
expect(response).to redirect_to(
namespace_project_path(project.namespace, project)
)
- expect(project.members.request.exists?(user_id: user)).to be_truthy
+ expect(project.requesters.exists?(user_id: user)).to be_truthy
expect(project.users).not_to include user
end
end
@@ -233,7 +233,7 @@ describe Projects::ProjectMembersController do
let(:team_requester) { create(:user) }
let(:member) do
project.request_access(team_requester)
- project.members.request.find_by(user_id: team_requester.id)
+ project.requesters.find_by(user_id: team_requester.id)
end
context 'when user does not have enough rights' do
diff --git a/spec/features/groups/members/owner_manages_access_requests_spec.rb b/spec/features/groups/members/owner_manages_access_requests_spec.rb
index 321c9bad7d0..2aae3e00261 100644
--- a/spec/features/groups/members/owner_manages_access_requests_spec.rb
+++ b/spec/features/groups/members/owner_manages_access_requests_spec.rb
@@ -41,7 +41,7 @@ feature 'Groups > Members > Owner manages access requests', feature: true do
def expect_visible_access_request(group, user)
- expect(group.members.request.exists?(user_id: user)).to be_truthy
+ expect(group.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content "#{group.name} access requests 1"
expect(page).to have_content user.name
end
diff --git a/spec/features/groups/members/user_requests_access_spec.rb b/spec/features/groups/members/user_requests_access_spec.rb
index 4944301c938..d1a6a98ab72 100644
--- a/spec/features/groups/members/user_requests_access_spec.rb
+++ b/spec/features/groups/members/user_requests_access_spec.rb
@@ -18,7 +18,7 @@ feature 'Groups > Members > User requests access', feature: true do
expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email]
expect(ActionMailer::Base.deliveries.last.subject).to match "Request to join the #{group.name} group"
- expect(group.members.request.exists?(user_id: user)).to be_truthy
+ expect(group.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content 'Your request for access has been queued for review.'
expect(page).to have_content 'Withdraw Access Request'
@@ -42,7 +42,7 @@ feature 'Groups > Members > User requests access', feature: true do
scenario 'user is not listed in the group members page' do
click_link 'Request Access'
- expect(group.members.request.exists?(user_id: user)).to be_truthy
+ expect(group.requesters.exists?(user_id: user)).to be_truthy
click_link 'Members'
@@ -54,11 +54,11 @@ feature 'Groups > Members > User requests access', feature: true do
scenario 'user can withdraw its request for access' do
click_link 'Request Access'
- expect(group.members.request.exists?(user_id: user)).to be_truthy
+ expect(group.requesters.exists?(user_id: user)).to be_truthy
click_link 'Withdraw Access Request'
- expect(group.members.request.exists?(user_id: user)).to be_falsey
+ expect(group.requesters.exists?(user_id: user)).to be_falsey
expect(page).to have_content 'Your access request to the group has been withdrawn.'
end
end
diff --git a/spec/features/projects/members/master_manages_access_requests_spec.rb b/spec/features/projects/members/master_manages_access_requests_spec.rb
index aa2d906fa2e..f7fcd9b6731 100644
--- a/spec/features/projects/members/master_manages_access_requests_spec.rb
+++ b/spec/features/projects/members/master_manages_access_requests_spec.rb
@@ -40,7 +40,7 @@ feature 'Projects > Members > Master manages access requests', feature: true do
end
def expect_visible_access_request(project, user)
- expect(project.members.request.exists?(user_id: user)).to be_truthy
+ expect(project.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content "#{project.name} access requests 1"
expect(page).to have_content user.name
end
diff --git a/spec/features/projects/members/user_requests_access_spec.rb b/spec/features/projects/members/user_requests_access_spec.rb
index af420c170ef..f2fe3ef364d 100644
--- a/spec/features/projects/members/user_requests_access_spec.rb
+++ b/spec/features/projects/members/user_requests_access_spec.rb
@@ -17,7 +17,7 @@ feature 'Projects > Members > User requests access', feature: true do
expect(ActionMailer::Base.deliveries.last.to).to eq [master.notification_email]
expect(ActionMailer::Base.deliveries.last.subject).to eq "Request to join the #{project.name_with_namespace} project"
- expect(project.members.request.exists?(user_id: user)).to be_truthy
+ expect(project.requesters.exists?(user_id: user)).to be_truthy
expect(page).to have_content 'Your request for access has been queued for review.'
expect(page).to have_content 'Withdraw Access Request'
@@ -27,7 +27,7 @@ feature 'Projects > Members > User requests access', feature: true do
scenario 'user is not listed in the project members page' do
click_link 'Request Access'
- expect(project.members.request.exists?(user_id: user)).to be_truthy
+ expect(project.requesters.exists?(user_id: user)).to be_truthy
open_project_settings_menu
click_link 'Members'
@@ -41,11 +41,11 @@ feature 'Projects > Members > User requests access', feature: true do
scenario 'user can withdraw its request for access' do
click_link 'Request Access'
- expect(project.members.request.exists?(user_id: user)).to be_truthy
+ expect(project.requesters.exists?(user_id: user)).to be_truthy
click_link 'Withdraw Access Request'
- expect(project.members.request.exists?(user_id: user)).to be_falsey
+ expect(project.requesters.exists?(user_id: user)).to be_falsey
expect(page).to have_content 'Your access request to the project has been withdrawn.'
end
diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb
index f75fdb739f6..7b2155e9a4e 100644
--- a/spec/helpers/members_helper_spec.rb
+++ b/spec/helpers/members_helper_spec.rb
@@ -57,6 +57,72 @@ describe MembersHelper do
end
end
+ describe '#can_see_request_access_button?' do
+ let(:user) { create(:user) }
+ let(:group) { create(:group, :public) }
+ let(:project) { create(:project, :public, group: group) }
+
+ before do
+ allow(helper).to receive(:current_user).and_return(user)
+ end
+
+ context 'source is a group' do
+ context 'current_user is not a member' do
+ it 'returns true' do
+ expect(helper.can_see_request_access_button?(group)).to be_truthy
+ end
+ end
+
+ context 'current_user is a member' do
+ it 'returns false' do
+ group.add_owner(user)
+
+ expect(helper.can_see_request_access_button?(group)).to be_falsy
+ end
+ end
+
+ context 'current_user is a requester' do
+ it 'returns true' do
+ group.request_access(user)
+
+ expect(helper.can_see_request_access_button?(group)).to be_truthy
+ end
+ end
+ end
+
+ context 'source is a project' do
+ context 'current_user is not a member' do
+ it 'returns true' do
+ expect(helper.can_see_request_access_button?(project)).to be_truthy
+ end
+ end
+
+ context 'current_user is a group member' do
+ it 'returns false' do
+ group.add_owner(user)
+
+ expect(helper.can_see_request_access_button?(project)).to be_falsy
+ end
+ end
+
+ context 'current_user is a group requester' do
+ it 'returns false' do
+ group.request_access(user)
+
+ expect(helper.can_see_request_access_button?(project)).to be_falsy
+ end
+ end
+
+ context 'current_user is a member' do
+ it 'returns false' do
+ project.team << [user, :master]
+
+ expect(helper.can_see_request_access_button?(project)).to be_falsy
+ end
+ end
+ end
+ end
+
describe '#remove_member_message' do
let(:requester) { build(:user) }
let(:project) { create(:project) }
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index ae55a01ebea..e527d649e34 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -406,7 +406,7 @@ describe Notify do
let(:user) { create(:user) }
let(:project_member) do
project.request_access(user)
- project.members.request.find_by(user_id: user.id)
+ project.requesters.find_by(user_id: user.id)
end
subject { Notify.member_access_requested_email('project', project_member.id) }
@@ -433,7 +433,7 @@ describe Notify do
let(:user) { create(:user) }
let(:project_member) do
project.request_access(user)
- project.members.request.find_by(user_id: user.id)
+ project.requesters.find_by(user_id: user.id)
end
subject { Notify.member_access_requested_email('project', project_member.id) }
@@ -459,7 +459,7 @@ describe Notify do
let(:user) { create(:user) }
let(:project_member) do
project.request_access(user)
- project.members.request.find_by(user_id: user.id)
+ project.requesters.find_by(user_id: user.id)
end
subject { Notify.member_access_denied_email('project', project.id, user.id) }
@@ -684,7 +684,7 @@ describe Notify do
let(:user) { create(:user) }
let(:group_member) do
group.request_access(user)
- group.members.request.find_by(user_id: user.id)
+ group.requesters.find_by(user_id: user.id)
end
subject { Notify.member_access_requested_email('group', group_member.id) }
@@ -705,7 +705,7 @@ describe Notify do
let(:user) { create(:user) }
let(:group_member) do
group.request_access(user)
- group.members.request.find_by(user_id: user.id)
+ group.requesters.find_by(user_id: user.id)
end
subject { Notify.member_access_denied_email('group', group.id, user.id) }
diff --git a/spec/models/concerns/access_requestable_spec.rb b/spec/models/concerns/access_requestable_spec.rb
index 98307876962..96eee0e8bdd 100644
--- a/spec/models/concerns/access_requestable_spec.rb
+++ b/spec/models/concerns/access_requestable_spec.rb
@@ -16,7 +16,7 @@ describe AccessRequestable do
before { group.request_access(user) }
- it { expect(group.members.request.exists?(user_id: user)).to be_truthy }
+ it { expect(group.requesters.exists?(user_id: user)).to be_truthy }
end
end
@@ -34,7 +34,7 @@ describe AccessRequestable do
before { project.request_access(user) }
- it { expect(project.members.request.exists?(user_id: user)).to be_truthy }
+ it { expect(project.requesters.exists?(user_id: user)).to be_truthy }
end
end
end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 2c19aa3f67f..a878ff1b227 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -7,9 +7,38 @@ describe Group, models: true do
it { is_expected.to have_many :projects }
it { is_expected.to have_many(:group_members).dependent(:destroy) }
it { is_expected.to have_many(:users).through(:group_members) }
+ it { is_expected.to have_many(:owners).through(:group_members) }
+ it { is_expected.to have_many(:requesters).dependent(:destroy) }
it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
+
+ describe '#members & #requesters' do
+ let(:requester) { create(:user) }
+ let(:developer) { create(:user) }
+ before do
+ group.request_access(requester)
+ group.add_developer(developer)
+ end
+
+ describe '#members' do
+ it 'includes members and exclude requesters' do
+ member_user_ids = group.members.pluck(:user_id)
+
+ expect(member_user_ids).to include(developer.id)
+ expect(member_user_ids).not_to include(requester.id)
+ end
+ end
+
+ describe '#requesters' do
+ it 'does not include requesters' do
+ requester_user_ids = group.requesters.pluck(:user_id)
+
+ expect(requester_user_ids).to include(requester.id)
+ expect(requester_user_ids).not_to include(developer.id)
+ end
+ end
+ end
end
describe 'modules' do
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index e9134a3d283..40181a8b906 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -73,10 +73,10 @@ describe Member, models: true do
@accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) }
requested_user = create(:user).tap { |u| project.request_access(u) }
- @requested_member = project.members.request.find_by(user_id: requested_user.id)
+ @requested_member = project.requesters.find_by(user_id: requested_user.id)
accepted_request_user = create(:user).tap { |u| project.request_access(u) }
- @accepted_request_member = project.members.request.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
+ @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
end
describe '.invite' do
@@ -103,22 +103,6 @@ describe Member, models: true do
it { expect(described_class.request).not_to include @accepted_request_member }
end
- describe '.non_request' do
- it { expect(described_class.non_request).to include @master }
- it { expect(described_class.non_request).to include @invited_member }
- it { expect(described_class.non_request).to include @accepted_invite_member }
- it { expect(described_class.non_request).not_to include @requested_member }
- it { expect(described_class.non_request).to include @accepted_request_member }
- end
-
- describe '.non_pending' do
- it { expect(described_class.non_pending).to include @master }
- it { expect(described_class.non_pending).not_to include @invited_member }
- it { expect(described_class.non_pending).to include @accepted_invite_member }
- it { expect(described_class.non_pending).not_to include @requested_member }
- it { expect(described_class.non_pending).to include @accepted_request_member }
- end
-
describe '.owners_and_masters' do
it { expect(described_class.owners_and_masters).to include @owner }
it { expect(described_class.owners_and_masters).to include @master }
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 42308035d8c..a8c777d1e3e 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -11,6 +11,8 @@ describe Project, models: true do
it { is_expected.to have_many(:issues).dependent(:destroy) }
it { is_expected.to have_many(:milestones).dependent(:destroy) }
it { is_expected.to have_many(:project_members).dependent(:destroy) }
+ it { is_expected.to have_many(:users).through(:project_members) }
+ it { is_expected.to have_many(:requesters).dependent(:destroy) }
it { is_expected.to have_many(:notes).dependent(:destroy) }
it { is_expected.to have_many(:snippets).class_name('ProjectSnippet').dependent(:destroy) }
it { is_expected.to have_many(:deploy_keys_projects).dependent(:destroy) }
@@ -31,6 +33,34 @@ describe Project, models: true do
it { is_expected.to have_many(:environments).dependent(:destroy) }
it { is_expected.to have_many(:deployments).dependent(:destroy) }
it { is_expected.to have_many(:todos).dependent(:destroy) }
+
+ describe '#members & #requesters' do
+ let(:project) { create(:project) }
+ let(:requester) { create(:user) }
+ let(:developer) { create(:user) }
+ before do
+ project.request_access(requester)
+ project.team << [developer, :developer]
+ end
+
+ describe '#members' do
+ it 'includes members and exclude requesters' do
+ member_user_ids = project.members.pluck(:user_id)
+
+ expect(member_user_ids).to include(developer.id)
+ expect(member_user_ids).not_to include(requester.id)
+ end
+ end
+
+ describe '#requesters' do
+ it 'does not include requesters' do
+ requester_user_ids = project.requesters.pluck(:user_id)
+
+ expect(requester_user_ids).to include(requester.id)
+ expect(requester_user_ids).not_to include(developer.id)
+ end
+ end
+ end
end
describe 'modules' do
--
GitLab