diff options
author | Rémy Coutable <remy@rymai.me> | 2016-06-02 19:05:06 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-06-14 14:18:14 +0300 |
commit | 515205d3c1c6655302ed0ae44cc5954dead7ae79 (patch) | |
tree | 5ac24884eb0e6faf56e1e460260303fbdb14b9cf | |
parent | 6d103a2f4764441b1650ba6d790732056c9a8516 (diff) |
UI and copywriting improvements
+ Move 'Edit Project/Group' out of membership-related partial
+ Show the access request buttons only to logged-in users
+ Put the request access buttons out of in a more visible button
+ Improve the copy in the #remove_member_message helper
Signed-off-by: Rémy Coutable <remy@rymai.me>
74 files changed, 775 insertions, 839 deletions
diff --git a/app/assets/stylesheets/pages/groups.scss b/app/assets/stylesheets/pages/groups.scss index ec6c099df5b..ac7721cbe15 100644 --- a/app/assets/stylesheets/pages/groups.scss +++ b/app/assets/stylesheets/pages/groups.scss @@ -39,3 +39,20 @@ } } } + +.groups-cover-block { + + .container-fluid { + position: relative; + } + + .access-request-button { + @include btn-gray; + position: absolute; + right: 16px; + bottom: 32px; + padding: 3px 10px; + text-transform: none; + background-color: $background-color; + } +} diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 2505deaf757..0e4cefc55c2 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -229,13 +229,20 @@ right: 16px; bottom: 0; - .btn { - padding: 3px 10px; - background-color: $background-color; + @media (max-width: $screen-lg-min) { + top: 0; } - @media (max-width: 1304px) { - top: 0; + .access-request-button { + position: absolute; + right: 0; + bottom: 61px; + + @media (max-width: $screen-lg-min) { + position: relative; + bottom: 0; + margin-right: 10px; + } } } diff --git a/app/controllers/concerns/access_request_actions.rb b/app/controllers/concerns/access_request_actions.rb deleted file mode 100644 index c4d22749d6a..00000000000 --- a/app/controllers/concerns/access_request_actions.rb +++ /dev/null @@ -1,38 +0,0 @@ -module AccessRequestActions - extend ActiveSupport::Concern - - def request_access - access_requestable_resource.request_access(current_user) - - redirect_to access_requestable_resource_path, - notice: 'Your request for access has been queued for review.' - end - - def approve_access_request - @member = access_requestable_resource.public_send(member_entity_name.pluralize).request.find(params[:id]) - - return render_403 unless can?(current_user, :"update_#{member_entity_name}", @member) - - @member.accept_request - - redirect_to access_requestable_resource_members_path - end - - protected - - def access_requestable_resource - raise NotImplementedError - end - - def access_requestable_resource_path - access_requestable_resource - end - - def access_requestable_resource_members_path - [access_requestable_resource, 'members'] - end - - def member_entity_name - "#{access_requestable_resource.class.to_s.underscore}_member" - end -end diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb new file mode 100644 index 00000000000..a24273fad0b --- /dev/null +++ b/app/controllers/concerns/membership_actions.rb @@ -0,0 +1,58 @@ +module MembershipActions + extend ActiveSupport::Concern + include MembersHelper + + def request_access + membershipable.request_access(current_user) + + redirect_to polymorphic_path(membershipable), + notice: 'Your request for access has been queued for review.' + end + + def approve_access_request + @member = membershipable.members.request.find(params[:id]) + + return render_403 unless can?(current_user, action_member_permission(:update, @member), @member) + + @member.accept_request + + redirect_to polymorphic_url([membershipable, :members]) + end + + def leave + @member = membershipable.members.find_by(user_id: current_user) + return render_403 unless @member + + source_type = @member.real_source_type.humanize(capitalize: false) + + if can?(current_user, action_member_permission(:destroy, @member), @member) + notice = + if @member.request? + "Your access request to the #{source_type} has been withdrawn." + else + "You left the \"#{@member.source.human_name}\" #{source_type}." + end + @member.destroy + + redirect_to [:dashboard, @member.real_source_type.tableize], notice: notice + else + if cannot_leave? + alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}." + alert << " Transfer or delete the #{source_type}." + redirect_to polymorphic_url(membershipable), alert: alert + else + render_403 + end + end + end + + protected + + def membershipable + raise NotImplementedError + end + + def cannot_leave? + raise NotImplementedError + end +end diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index a37129062f9..d0f2e2949f0 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -1,5 +1,5 @@ class Groups::GroupMembersController < Groups::ApplicationController - include AccessRequestActions + include MembershipActions # Authorize before_action :authorize_admin_group_member!, except: [:index, :leave, :request_access] @@ -38,7 +38,7 @@ class Groups::GroupMembersController < Groups::ApplicationController return render_403 unless can?(current_user, :destroy_group_member, @group_member) - @group_member.request? ? @group_member.decline_request : @group_member.destroy + @group_member.destroy respond_to do |format| format.html { redirect_to group_group_members_path(@group), notice: 'User was successfully removed from group.' } @@ -60,46 +60,16 @@ class Groups::GroupMembersController < Groups::ApplicationController end end - def leave - @group_member = - @group.group_members.find_by(user_id: current_user.id) || - @group.group_members.find_by(created_by_id: current_user.id) - - if can?(current_user, :destroy_group_member, @group_member) - notice = - if @group_member.request? - 'You withdrawn your access request to the group.' - else - "You left #{@group.name} group." - end - @group_member.destroy - - redirect_to dashboard_groups_path, notice: notice - else - if @group.last_owner?(current_user) - redirect_to(dashboard_groups_path, alert: "You can not leave #{group.name} group because you're the last owner. Transfer or delete the group.") - else - return render_403 - end - end - end - protected def member_params params.require(:group_member).permit(:access_level, :user_id) end - # AccessRequestActions concern - def access_requestable_resource - @group - end - - def access_requestable_resource_path - group_path(@group) - end + # MembershipActions concern + alias_method :membershipable, :group - def access_requestable_resource_members_path - group_group_members_path(@group) + def cannot_leave? + @group.last_owner?(current_user) end end diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index c61eda95bc7..35d067cd029 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -1,5 +1,5 @@ class Projects::ProjectMembersController < Projects::ApplicationController - include AccessRequestActions + include MembershipActions # Authorize before_action :authorize_admin_project_member!, except: [:index, :leave, :request_access] @@ -52,7 +52,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController return render_403 unless can?(current_user, :destroy_project_member, @project_member) - @project_member.request? ? @project_member.decline_request : @project_member.destroy + @project_member.destroy respond_to do |format| format.html do @@ -76,31 +76,6 @@ class Projects::ProjectMembersController < Projects::ApplicationController end end - def leave - @project_member = - @project.project_members.find_by(user_id: current_user.id) || - @project.project_members.find_by(created_by_id: current_user.id) - - if can?(current_user, :destroy_project_member, @project_member) - notice = - if @project_member.request? - 'You withdrawn your access request to the project.' - else - 'You left the project.' - end - @project_member.destroy - - redirect_to dashboard_projects_path, notice: notice - else - if current_user == @project.owner - message = 'You can not leave your own project. Transfer or delete the project.' - redirect_back_or_default(default: { action: 'index' }, options: { alert: message }) - else - render_403 - end - end - end - def apply_import source_project = Project.find(params[:source_project_id]) @@ -121,16 +96,10 @@ class Projects::ProjectMembersController < Projects::ApplicationController params.require(:project_member).permit(:user_id, :access_level) end - # AccessRequestActions concern - def access_requestable_resource - @project - end - - def access_requestable_resource_path - namespace_project_path(@project.namespace, @project) - end + # MembershipActions concern + alias_method :membershipable, :project - def access_requestable_resource_members_path - namespace_project_project_members_path(@project.namespace, @project) + def cannot_leave? + current_user == @project.owner end end diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index 2ce2d4e694f..3a43e936aee 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -13,10 +13,23 @@ # merge_request_path(merge_request) # module GitlabRoutingHelper + # Project def project_path(project, *args) namespace_project_path(project.namespace, project, *args) end + def project_url(project, *args) + namespace_project_url(project.namespace, project, *args) + end + + def edit_project_path(project, *args) + edit_namespace_project_path(project.namespace, project, *args) + end + + def edit_project_url(project, *args) + edit_namespace_project_url(project.namespace, project, *args) + end + def project_files_path(project, *args) namespace_project_tree_path(project.namespace, project, @ref || project.repository.root_ref) end @@ -41,10 +54,6 @@ module GitlabRoutingHelper activity_namespace_project_path(project.namespace, project, *args) end - def edit_project_path(project, *args) - edit_namespace_project_path(project.namespace, project, *args) - end - def runners_path(project, *args) namespace_project_runners_path(project.namespace, project, *args) end @@ -65,14 +74,6 @@ module GitlabRoutingHelper namespace_project_milestone_path(entity.project.namespace, entity.project, entity, *args) end - def project_url(project, *args) - namespace_project_url(project.namespace, project, *args) - end - - def edit_project_url(project, *args) - edit_namespace_project_url(project.namespace, project, *args) - end - def issue_url(entity, *args) namespace_project_issue_url(entity.project.namespace, entity.project, entity, *args) end @@ -92,4 +93,56 @@ module GitlabRoutingHelper toggle_subscription_namespace_project_merge_request_path(entity.project.namespace, entity.project, entity) end end + + ## Members + def project_members_url(project, *args) + namespace_project_project_members_url(project.namespace, project) + end + + def project_member_path(project_member, *args) + namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) + end + + def request_access_project_members_path(project, *args) + request_access_namespace_project_project_members_path(project.namespace, project) + end + + def leave_project_members_path(project, *args) + leave_namespace_project_project_members_path(project.namespace, project) + end + + def approve_access_request_project_member_path(project_member, *args) + approve_access_request_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) + end + + def resend_invite_project_member_path(project_member, *args) + resend_invite_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) + end + + # Groups + + ## Members + def group_members_url(group, *args) + group_group_members_url(group, *args) + end + + def group_member_path(group_member, *args) + group_group_member_path(group_member.source, group_member) + end + + def request_access_group_members_path(group, *args) + request_access_group_group_members_path(group) + end + + def leave_group_members_path(group, *args) + leave_group_group_members_path(group) + end + + def approve_access_request_group_member_path(group_member, *args) + approve_access_request_group_group_member_path(group_member.source, group_member) + end + + def resend_invite_group_member_path(group_member, *args) + resend_invite_group_group_member_path(group_member.source, group_member) + end end diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index bd84b8b239f..a53828ef4e7 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -1,117 +1,45 @@ module MembersHelper - def member_class(member) - "#{member.source.class.to_s}Member".constantize - end - - def members_association(entity) - "#{entity.class.to_s.underscore}_members".to_sym - end - + # Returns a `<action>_<source>_member` association, e.g.: + # - admin_project_member, update_project_member, destroy_project_member + # - admin_group_member, update_group_member, destroy_group_member def action_member_permission(action, member) - "#{action}_#{member.source.class.to_s.underscore}_member".to_sym + "#{action}_#{member.type.underscore}".to_sym end - def can_see_entity_roles?(user, entity) + def can_see_member_roles?(source:, user: nil) return false unless user - user.is_admin? || entity.send(members_association(entity)).exists?(user_id: user.id) - end - - def member_path(member) - case member.source - when Project - namespace_project_project_member_path(member.source.namespace, member.source, member) - when Group - group_group_member_path(member.source, member) - else - raise ArgumentError.new('Unknown object class') - end + user.is_admin? || source.members.exists?(user_id: user.id) end - def resend_invite_member_path(member) - case member.source - when Project - resend_invite_namespace_project_project_member_path(member.source.namespace, member.source, member) - when Group - resend_invite_group_group_member_path(member.source, member) - else - raise ArgumentError.new('Unknown object class') - end - end + def remove_member_message(member, user: nil) + user = current_user if defined?(current_user) - def request_access_path(entity) - case entity - when Project - request_access_namespace_project_project_members_path(entity.namespace, entity) - when Group - request_access_group_group_members_path(entity) - else - raise ArgumentError.new('Unknown object class') - end - end - - def approve_request_member_path(member) - case member.source - when Project - approve_access_request_namespace_project_project_member_path(member.source.namespace, member.source, member) - when Group - approve_access_request_group_group_member_path(member.source, member) - else - raise ArgumentError.new('Unknown object class') - end - end + text = 'Are you sure you want to ' + action = + if member.request? + if member.user == user + 'withdraw your access request for' + else + "deny #{member.user.name}'s request to join" + end + elsif member.invite? + "revoke the invitation for #{member.invite_email} to join" + else + "remove #{member.user.name} from" + end - def leave_path(entity) - case entity - when Project - leave_namespace_project_project_members_path(entity.namespace, entity) - when Group - leave_group_group_members_path(entity) - else - raise ArgumentError.new('Unknown object class') - end - end - - def withdraw_request_message(entity) - "Are you sure you want to withdraw your access request for the \"#{entity_name(entity)}\" #{entity_type(entity)}?" - end - - def remove_member_message(member) - entity = member.source - entity_type = entity_type(entity) - entity_name = entity_name(entity) - - if member.request? - "You are going to deny #{member.created_by.name}'s request to join the #{entity_name} #{entity_type}. Are you sure?" - elsif member.invite? - "You are going to revoke the invitation for #{member.invite_email} to join the #{entity_name} #{entity_type}. Are you sure?" - else - "You are going to remove #{member.user.name} from the #{entity_name} #{entity_type}. Are you sure?" - end + text << action << " the #{member.source.human_name} #{member.real_source_type.humanize(capitalize: false)}?" end def remove_member_title(member) - member.request? ? 'Deny access request' : 'Remove user' - end - - def leave_confirmation_message(entity) - "Are you sure you want to leave \"#{entity_name(entity)}\" #{entity_type(entity)}?" - end - - private + text = " from #{member.real_source_type.humanize(capitalize: false)}" - def entity_type(entity) - entity.class.to_s.underscore + text.prepend(member.request? ? 'Deny access request' : 'Remove user') end - def entity_name(entity) - case entity - when Project - entity.name_with_namespace - when Group - entity.name - else - raise ArgumentError.new('Unknown object class') - end + def leave_confirmation_message(member_source) + "Are you sure you want to leave the " \ + "\"#{member_source.human_name}\" #{member_source.class.to_s.humanize(capitalize: false)}?" end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 03941f87b13..d30dd66202b 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -1,8 +1,4 @@ module ProjectsHelper - def max_access_level(project, user) - Gitlab::Access.options_with_owner.key(project.team.max_member_access(user.id)) - end - def link_to_project(project) link_to [project.namespace.becomes(Namespace), project], title: h(project.name) do title = content_tag(:span, project.name, class: 'project-name') diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 5fd55c149df..6dde2e9847d 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -1,104 +1,81 @@ module Emails module Members extend ActiveSupport::Concern + include MembersHelper included do - attr_reader :member_target_type - helper_method :member, :access_requester, :member_target_type, :member_target_name, :member_target_url + helper_method :member_source, :member end - def member_access_requested_email(member_target_type, member_id) - @member_target_type = member_target_type + def member_access_requested_email(member_source_type, member_id) + @member_source_type = member_source_type @member_id = member_id - admins = User.where(id: target.public_send(members_association).admins.pluck(:user_id)).pluck(:notification_email) + admins = member_source.members.owners_and_masters.includes(:user).pluck(:notification_email) mail(to: admins, - subject: subject("Request to join the #{member_target_name} #{member_target_type}")) + subject: subject("Request to join the #{member_source.human_name} #{member_source.model_name.singular}")) end - def member_access_granted_email(member_target_type, member_id) - @member_target_type = member_target_type + def member_access_granted_email(member_source_type, member_id) + @member_source_type = member_source_type @member_id = member_id mail(to: member.user.notification_email, - subject: subject("Access to the #{member_target_name} #{member_target_type} was granted")) + subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was granted")) end - def member_access_denied_email(member_target_type, target_id, user_id) - @member_target_type = member_target_type - @target = target_class.find(target_id) + def member_access_denied_email(member_source_type, source_id, user_id) + @member_source_type = member_source_type + @member_source = member_source_class.find(source_id) + requester = User.find(user_id) - mail(to: User.find(user_id).notification_email, - subject: subject("Access to the #{member_target_name} #{member_target_type} was denied")) + mail(to: requester.notification_email, + subject: subject("Access to the #{member_source.human_name} #{member_source.model_name.singular} was denied")) end - def member_invited_email(member_target_type, member_id, token) - @member_target_type = member_target_type + def member_invited_email(member_source_type, member_id, token) + @member_source_type = member_source_type @member_id = member_id @token = token mail(to: member.invite_email, - subject: "Invitation to join the #{member_target_name} #{member_target_type}") + subject: "Invitation to join the #{member_source.human_name} #{member_source.model_name.singular}") end - def member_invite_accepted_email(member_target_type, member_id) - @member_target_type = member_target_type + def member_invite_accepted_email(member_source_type, member_id) + @member_source_type = member_source_type @member_id = member_id - return if access_requester.nil? + return unless member.created_by - mail(to: access_requester.notification_email, + mail(to: member.created_by.notification_email, subject: subject('Invitation accepted')) end - def member_invite_declined_email(member_target_type, target_id, invite_email, created_by_id) - return if created_by_id.nil? + def member_invite_declined_email(member_source_type, source_id, invite_email, created_by_id) + return unless created_by_id - @member_target_type = member_target_type - @target = target_class.find(target_id) + @member_source_type = member_source_type + @member_source = member_source_class.find(source_id) @invite_email = invite_email + inviter = User.find(created_by_id) - mail(to: User.find(created_by_id).notification_email, + mail(to: inviter.notification_email, subject: subject('Invitation declined')) end def member - @member ||= member_class.find(@member_id) + @member ||= Member.find(@member_id) end - def access_requester - @access_requester ||= member.created_by - end - - def member_target_name - case member_target_type - when 'project' - target.name_with_namespace - else - target.name - end - end - - def member_target_url - @member_target_url ||= target.web_url + def member_source + @member_source ||= member.source end private - def target - @target ||= member.public_send(member_target_type) - end - - def target_class - @target_class ||= member_target_type.classify.constantize - end - - def member_class - @member_class ||= "#{member_target_type.classify}Member".constantize - end - - def members_association - @members_association ||= member_class.to_s.tableize + def member_source_class + @member_source_type.classify.constantize end end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index bd5c6788cce..0cc709f68e4 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -13,6 +13,8 @@ class Notify < BaseMailer add_template_helper DiffHelper add_template_helper BlobHelper add_template_helper EmailsHelper + add_template_helper MembersHelper + add_template_helper GitlabRoutingHelper def test_email(recipient_email, subject, body) mail(to: recipient_email, diff --git a/app/models/ability.rb b/app/models/ability.rb index 90156bf130c..647a73aa1ce 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -460,8 +460,6 @@ class Ability rules << :destroy_group_member elsif user == target_user rules << :destroy_group_member - elsif subject.request? && user == subject.created_by - rules << :destroy_group_member end end @@ -481,8 +479,6 @@ class Ability rules << :destroy_project_member elsif user == target_user rules << :destroy_project_member - elsif subject.request? && user == subject.created_by - rules << :destroy_project_member end end diff --git a/app/models/concerns/access_requestable.rb b/app/models/concerns/access_requestable.rb index cf37284e31a..eedd32a729f 100644 --- a/app/models/concerns/access_requestable.rb +++ b/app/models/concerns/access_requestable.rb @@ -10,18 +10,7 @@ module AccessRequestable def request_access(user) members.create( access_level: Gitlab::Access::DEVELOPER, - created_by: user, + user: user, requested_at: Time.now.utc) end - - def access_requested?(user) - members.where(created_by_id: user.id).where.not(requested_at: nil).any? - end - - private - - # Returns a `<entities>_members` association, e.g.: project_members, group_members - def members - @members ||= send("#{self.class.to_s.underscore}_members".to_sym) - end end diff --git a/app/models/group.rb b/app/models/group.rb index 520cbd0283c..b8dffe9f5b9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -8,7 +8,7 @@ class Group < Namespace has_many :group_members, dependent: :destroy, as: :source, class_name: 'GroupMember' alias_method :members, :group_members - has_many :users, through: :group_members + has_many :users, -> { where(members: { requested_at: nil }) }, through: :group_members 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 5c3a5eab406..cea6d259760 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -8,7 +8,7 @@ class Member < ActiveRecord::Base belongs_to :user belongs_to :source, polymorphic: true - validates :user, presence: true, unless: :pending? + validates :user, presence: true, unless: :invite? validates :source, presence: true validates :user_id, uniqueness: { scope: [:source_type, :source_id], message: "already exists in source", @@ -27,16 +27,17 @@ 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, -> { where.not(user_id: nil) } + scope :non_pending, -> { non_request.non_invite } scope :guests, -> { where(access_level: GUEST) } scope :reporters, -> { where(access_level: REPORTER) } scope :developers, -> { where(access_level: DEVELOPER) } scope :masters, -> { where(access_level: MASTER) } scope :owners, -> { where(access_level: OWNER) } - scope :admins, -> { where(access_level: [OWNER, MASTER]) } + scope :owners_and_masters, -> { where(access_level: [OWNER, MASTER]) } before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? } @@ -46,6 +47,7 @@ class Member < ActiveRecord::Base after_create :post_create_hook, unless: :pending? after_update :post_update_hook, unless: :pending? after_destroy :post_destroy_hook, unless: :pending? + after_destroy :post_decline_request, if: :request? delegate :name, :username, :email, to: :user, prefix: true @@ -102,36 +104,31 @@ class Member < ActiveRecord::Base end end - def pending? - request? || invite? + def real_source_type + source_type + end + + def invite? + self.invite_token.present? end def request? - user.nil? && created_by.present? && requested_at.present? + requested_at.present? end - def invite? - self.invite_token.present? + def pending? + invite? || request? end def accept_request return false unless request? - updated = self.update(user: created_by, requested_at: nil) + updated = self.update(requested_at: nil) after_accept_request if updated updated end - def decline_request - return false unless request? - - self.destroy - after_decline_request if destroyed? - - destroyed? - end - def accept_invite!(new_user) return false unless invite? @@ -217,7 +214,7 @@ class Member < ActiveRecord::Base post_create_hook end - def after_decline_request + def post_decline_request # override in subclass end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 476b4816b90..363db877968 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -20,6 +20,11 @@ class GroupMember < Member access_level end + # Because source_type is `Namespace`... + def real_source_type + 'Group' + end + private def send_invite @@ -60,8 +65,8 @@ class GroupMember < Member super end - def after_decline_request - notification_service.decline_group_access_request(group, created_by) + def post_decline_request + notification_service.decline_group_access_request(self) super end diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index c6fd1a5c3d1..250ee04fd1d 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -152,8 +152,8 @@ class ProjectMember < Member super end - def after_decline_request - notification_service.decline_project_access_request(project, created_by) + def post_decline_request + notification_service.decline_project_access_request(self) super end diff --git a/app/models/project.rb b/app/models/project.rb index ef665373495..0d2e612436a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -104,7 +104,8 @@ class Project < ActiveRecord::Base 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 :users, through: :project_members + alias_method :members, :project_members + has_many :users, -> { where(members: { requested_at: nil }) }, through: :project_members has_many :deploy_keys_projects, dependent: :destroy has_many :deploy_keys, through: :deploy_keys_projects has_many :users_star_projects, dependent: :destroy @@ -690,6 +691,7 @@ class Project < ActiveRecord::Base end end end + alias_method :human_name, :name_with_namespace def path_with_namespace if namespace diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 7fb17df0e96..73e736820af 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.project_members.find_by(user_id: user_id) + member = project.members.non_request.find_by(user_id: user_id) # If user is not in project members # we should check for group membership if group && !member - member = group.group_members.find_by(user_id: user_id) + member = group.members.non_request.find_by(user_id: user_id) end member @@ -128,12 +128,16 @@ class ProjectTeam end end + def human_max_access(user_id) + Gitlab::Access.options_with_owner.key(max_member_access(user_id)) + end + # This method assumes project and group members are eager loaded for optimal # performance. def max_member_access(user_id) access = [] - project.project_members.each do |member| + project.members.non_request.each do |member| if member.user_id == user_id access << member.access_field if member.access_field break @@ -141,7 +145,7 @@ class ProjectTeam end if group - group.group_members.each do |member| + group.members.non_request.each do |member| if member.user_id == user_id access << member.access_field if member.access_field break @@ -174,14 +178,14 @@ class ProjectTeam end def fetch_members(level = nil) - project_members = project.project_members - group_members = group ? group.group_members : [] + project_members = project.members.non_request + group_members = group ? group.members.non_request : [] 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.group_members + im = invited_group.members.non_request if level int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 259199f6e2b..f804ac171c4 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -175,24 +175,24 @@ class NotificationService # Project access request def new_project_access_request(project_member) - mailer.member_access_requested_email('project', project_member.id).deliver_later + mailer.member_access_requested_email(project_member.real_source_type, project_member.id).deliver_later end - def decline_project_access_request(project, user) - mailer.member_access_denied_email('project', project.id, user.id).deliver_later + def decline_project_access_request(project_member) + mailer.member_access_denied_email(project_member.real_source_type, project_member.project.id, project_member.user.id).deliver_later end def invite_project_member(project_member, token) - mailer.member_invited_email('project', project_member.id, token).deliver_later + mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later end def accept_project_invite(project_member) - mailer.member_invite_accepted_email('project', project_member.id).deliver_later + mailer.member_invite_accepted_email(project_member.real_source_type, project_member.id).deliver_later end def decline_project_invite(project_member) mailer.member_invite_declined_email( - 'project', + project_member.real_source_type, project_member.project.id, project_member.invite_email, project_member.access_level, @@ -201,24 +201,24 @@ class NotificationService end def new_project_member(project_member) - mailer.member_access_granted_email('project', project_member.id).deliver_later + mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end def update_project_member(project_member) - mailer.member_access_granted_email('project', project_member.id).deliver_later + mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end # Group access request def new_group_access_request(group_member) - mailer.member_access_requested_email('group', group_member.id).deliver_later + mailer.member_access_requested_email(group_member.real_source_type, group_member.id).deliver_later end - def decline_group_access_request(group, user) - mailer.member_access_denied_email('group', group.id, user.id).deliver_later + def decline_group_access_request(group_member) + mailer.member_access_denied_email(group_member.real_source_type, group_member.group.id, group_member.user.id).deliver_later end def invite_group_member(group_member, token) - mailer.member_invited_email('group', group_member.id, token).deliver_later + mailer.member_invited_email(group_member.real_source_type, group_member.id, token).deliver_later end def accept_group_invite(group_member) @@ -227,7 +227,7 @@ class NotificationService def decline_group_invite(group_member) mailer.member_invite_declined_email( - 'group', + group_member.real_source_type, group_member.group.id, group_member.invite_email, group_member.access_level, @@ -236,11 +236,11 @@ class NotificationService end def new_group_member(group_member) - mailer.member_access_granted_email('group', group_member.id).deliver_later + mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later end def update_group_member(group_member) - mailer.member_access_granted_email('group', group_member.id).deliver_later + mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later end def project_was_moved(project, old_path_with_namespace) diff --git a/app/views/groups/group_members/index.html.haml b/app/views/groups/group_members/index.html.haml index a39d5d3d0f0..a36531e095a 100644 --- a/app/views/groups/group_members/index.html.haml +++ b/app/views/groups/group_members/index.html.haml @@ -11,7 +11,7 @@ .new-group-member-holder = render "new_group_member" - = render "shared/members/requests", entity: @group, members: @members + = render 'shared/members/requests', membership_source: @group, members: @members.request .panel.panel-default .panel-heading diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml index 85635bc4616..62ebd69485c 100644 --- a/app/views/groups/show.html.haml +++ b/app/views/groups/show.html.haml @@ -19,6 +19,9 @@ .cover-desc.description = markdown(@group.description, pipeline: :description) + - if current_user + = render 'shared/members/access_request_buttons', source: @group + %div{ class: container_class } .top-area %ul.nav-links diff --git a/app/views/layouts/nav/_group_settings.html.haml b/app/views/layouts/nav/_group_settings.html.haml index b461772b87e..dac46648b9f 100644 --- a/app/views/layouts/nav/_group_settings.html.haml +++ b/app/views/layouts/nav/_group_settings.html.haml @@ -1,3 +1,16 @@ - if current_user - .controls - = render 'shared/group_or_project_home_dropdown', entity: @group + - if access = @group.users.find_by(id: current_user.id) + .controls + .dropdown.group-settings-dropdown + %a.dropdown-new.btn.btn-default#group-settings-button{href: '#', 'data-toggle' => 'dropdown'} + = icon('cog') + = icon('caret-down') + %ul.dropdown-menu.dropdown-menu-align-right + - if can?(current_user, :admin_group, @group) + = nav_link(path: 'groups#projects') do + = link_to projects_group_path(@group), title: 'Projects' do + Projects + %li.divider + %li + = link_to edit_group_path(@group) do + Edit Group diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml index 3398794302f..ad019710830 100644 --- a/app/views/layouts/nav/_project.html.haml +++ b/app/views/layouts/nav/_project.html.haml @@ -1,33 +1,25 @@ - if current_user .controls - - access = user_max_access_in_project(current_user.id, @project) - - can_edit = can?(current_user, :admin_project, @project) .dropdown.project-settings-dropdown %a.dropdown-new.btn.btn-default#project-settings-button{href: '#', 'data-toggle' => 'dropdown'} = icon('cog') = icon('caret-down') %ul.dropdown-menu.dropdown-menu-align-right = render 'layouts/nav/project_settings' - %li.divider - - if can_edit - %li - = link_to edit_project_path(@project) do - Edit Project - - if access - %li - = link_to leave_path(@project), - data: { confirm: leave_confirmation_message(@project) }, method: :delete do - Leave Project - - elsif @project.access_requested?(current_user) - %li - = link_to leave_path(@project), - data: { confirm: withdraw_request_message(@project) }, method: :delete do - Withdraw Request - - else - %li - = link_to request_access_path(@project), - class: 'btn btn-gray', style: 'margin-left: 10px', method: :post do - Request Access + + - access = @project.team.max_member_access(current_user.id) + - can_edit = can?(current_user, :admin_project, @project) + - if can_edit || access + %li.divider + - if can_edit + %li + = link_to edit_project_path(@project) do + Edit Project + - if access + %li + = link_to polymorphic_path([:leave, @project, :members]), + data: { confirm: leave_confirmation_message(@project) }, method: :delete do + Leave Project %div{ class: nav_control_class } %ul.nav-links.scrolling-tabs diff --git a/app/views/notify/group_access_granted_email.html.haml b/app/views/notify/group_access_granted_email.html.haml deleted file mode 100644 index 1283758c576..00000000000 --- a/app/views/notify/group_access_granted_email.html.haml +++ /dev/null @@ -1,3 +0,0 @@ -%p - You have been granted #{@group_member.human_access} access to group - #{link_to @group.name, @target_url}. diff --git a/app/views/notify/group_access_granted_email.text.erb b/app/views/notify/group_access_granted_email.text.erb deleted file mode 100644 index c7568350075..00000000000 --- a/app/views/notify/group_access_granted_email.text.erb +++ /dev/null @@ -1,3 +0,0 @@ -You have been granted <%= @group_member.human_access %> access to group <%= @group.name %>. - -<%= @target_url %> diff --git a/app/views/notify/group_invite_accepted_email.html.haml b/app/views/notify/group_invite_accepted_email.html.haml deleted file mode 100644 index 55efad384a7..00000000000 --- a/app/views/notify/group_invite_accepted_email.html.haml +++ /dev/null @@ -1,6 +0,0 @@ -%p - #{@group_member.invite_email}, now known as - #{link_to @group_member.user.name, user_url(@group_member.user)}, - has accepted your invitation to join group - #{link_to @group.name, group_url(@group)}. - diff --git a/app/views/notify/group_invite_accepted_email.text.erb b/app/views/notify/group_invite_accepted_email.text.erb deleted file mode 100644 index f8b70f7a5a6..00000000000 --- a/app/views/notify/group_invite_accepted_email.text.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= @group_member.invite_email %>, now known as <%= @group_member.user.name %>, has accepted your invitation to join group <%= @group.name %>. - -<%= group_url(@group) %> diff --git a/app/views/notify/group_invite_declined_email.html.haml b/app/views/notify/group_invite_declined_email.html.haml deleted file mode 100644 index f9525d84fac..00000000000 --- a/app/views/notify/group_invite_declined_email.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -%p - #{@invite_email} - has declined your invitation to join group - #{link_to @group.name, group_url(@group)}. - diff --git a/app/views/notify/group_invite_declined_email.text.erb b/app/views/notify/group_invite_declined_email.text.erb deleted file mode 100644 index 6c19a288d15..00000000000 --- a/app/views/notify/group_invite_declined_email.text.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= @invite_email %> has declined your invitation to join group <%= @group.name %>. - -<%= group_url(@group) %> diff --git a/app/views/notify/group_member_invited_email.html.haml b/app/views/notify/group_member_invited_email.html.haml deleted file mode 100644 index 163e88bfea3..00000000000 --- a/app/views/notify/group_member_invited_email.html.haml +++ /dev/null @@ -1,14 +0,0 @@ -%p - You have been invited - - if inviter = @group_member.created_by - by - = link_to inviter.name, user_url(inviter) - to join group - = link_to @group.name, group_url(@group) - as #{@group_member.human_access}. - -%p - = link_to 'Accept invitation', invite_url(@token) - or - = link_to 'decline', decline_invite_url(@token) - diff --git a/app/views/notify/group_member_invited_email.text.erb b/app/views/notify/group_member_invited_email.text.erb deleted file mode 100644 index 28ce4819b14..00000000000 --- a/app/views/notify/group_member_invited_email.text.erb +++ /dev/null @@ -1,4 +0,0 @@ -You have been invited <%= "by #{@group_member.created_by.name} " if @group_member.created_by %>to join group <%= @group.name %> as <%= @group_member.human_access %>. - -Accept invitation: <%= invite_url(@token) %> -Decline invitation: <%= decline_invite_url(@token) %> diff --git a/app/views/notify/member_access_denied_email.html.haml b/app/views/notify/member_access_denied_email.html.haml index a25af24d783..71c9c50071a 100644 --- a/app/views/notify/member_access_denied_email.html.haml +++ b/app/views/notify/member_access_denied_email.html.haml @@ -1,4 +1,4 @@ %p Your request to join the - #{link_to member_target_name, member_target_url} #{member_target_type} + #{link_to member_source.human_name, member_source.web_url} #{member_source.model_name.singular} has been denied. diff --git a/app/views/notify/member_access_denied_email.text.erb b/app/views/notify/member_access_denied_email.text.erb index eb204458d9d..87f2ef817ee 100644 --- a/app/views/notify/member_access_denied_email.text.erb +++ b/app/views/notify/member_access_denied_email.text.erb @@ -1,3 +1,3 @@ -Your request to join the <%= member_target_name %> <%= member_target_type %> has been denied. +Your request to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> has been denied. -<%= member_target_url %> +<%= member_source.web_url %> diff --git a/app/views/notify/member_access_granted_email.html.haml b/app/views/notify/member_access_granted_email.html.haml index 62837d74555..18dec806539 100644 --- a/app/views/notify/member_access_granted_email.html.haml +++ b/app/views/notify/member_access_granted_email.html.haml @@ -1,3 +1,3 @@ %p You have been granted #{member.human_access} access to the - #{link_to member_target_name, member_target_url} #{member_target_type}. + #{link_to member_source.human_name, member_source.web_url} #{member_source.model_name.singular}. diff --git a/app/views/notify/member_access_granted_email.text.erb b/app/views/notify/member_access_granted_email.text.erb index be9bb5ee948..a9fb3a589a5 100644 --- a/app/views/notify/member_access_granted_email.text.erb +++ b/app/views/notify/member_access_granted_email.text.erb @@ -1,3 +1,3 @@ -You have been granted <%= member.human_access %> access to the <%= member_target_name %> <%= member_target_type %>. +You have been granted <%= member.human_access %> access to the <%= member_source.human_name %> <%= member_source.model_name.singular %>. -<%= member_target_url %> +<%= member_source.web_url %> diff --git a/app/views/notify/member_access_requested_email.html.haml b/app/views/notify/member_access_requested_email.html.haml index 96e92a069f2..76f1f08a0cb 100644 --- a/app/views/notify/member_access_requested_email.html.haml +++ b/app/views/notify/member_access_requested_email.html.haml @@ -1,3 +1,3 @@ %p - #{link_to access_requester.name, access_requester} requested #{member.human_access} - access to the #{link_to member_target_name, member_target_url} #{member_target_type}. + #{link_to member.user.name, member.user} requested #{member.human_access} + access to the #{link_to member_source.human_name, polymorphic_url([member_source, :members])} #{member_source.model_name.singular}. diff --git a/app/views/notify/member_access_requested_email.text.erb b/app/views/notify/member_access_requested_email.text.erb index 3b5de8c2abe..9c5ee0eaf26 100644 --- a/app/views/notify/member_access_requested_email.text.erb +++ b/app/views/notify/member_access_requested_email.text.erb @@ -1,3 +1,3 @@ -<%= access_requester.name %> (<%= user_url(access_requester) %>) requested <%= member.human_access %> access to the <%= member_target_name %> <%= member_target_type %>. +<%= member.user.name %> (<%= user_url(member.user) %>) requested <%= member.human_access %> access to the <%= member_source.human_name %> <%= member_source.model_name.singular %>. -<%= member_target_url %> +<%= polymorphic_url([member_source, :members]) %> diff --git a/app/views/notify/member_invite_accepted_email.html.haml b/app/views/notify/member_invite_accepted_email.html.haml index c420a8a7b3c..2d1d40881eb 100644 --- a/app/views/notify/member_invite_accepted_email.html.haml +++ b/app/views/notify/member_invite_accepted_email.html.haml @@ -2,4 +2,4 @@ #{member.invite_email}, now known as #{link_to member.user.name, user_url(member.user)}, has accepted your invitation to join the - #{link_to member_target_name, member_target_url} #{member_target_type}. + #{link_to member_source.human_name, member_source.web_url} #{member_source.model_name.singular}. diff --git a/app/views/notify/member_invite_accepted_email.text.erb b/app/views/notify/member_invite_accepted_email.text.erb index a1616163ceb..cef87101427 100644 --- a/app/views/notify/member_invite_accepted_email.text.erb +++ b/app/views/notify/member_invite_accepted_email.text.erb @@ -1,3 +1,3 @@ -<%= member.invite_email %>, now known as <%= member.user.name %>, has accepted your invitation to join the <%= member_target_name %> <%= member_target_type %>. +<%= member.invite_email %>, now known as <%= member.user.name %>, has accepted your invitation to join the <%= member_source.human_name %> <%= member_source.model_name.singular %>. -<%= member_target_url %> +<%= member_source.web_url %> diff --git a/app/views/notify/member_invite_declined_email.html.haml b/app/views/notify/member_invite_declined_email.html.haml index 5a30ac31b3c..aa1b373d1a6 100644 --- a/app/views/notify/member_invite_declined_email.html.haml +++ b/app/views/notify/member_invite_declined_email.html.haml @@ -1,4 +1,4 @@ %p #{@invite_email} has declined your invitation to join the - #{link_to member_target_name, member_target_url} #{member_target_type}. + #{link_to member_source.human_name, member_source.web_url} #{member_source.model_name.singular}. diff --git a/app/views/notify/member_invite_declined_email.text.erb b/app/views/notify/member_invite_declined_email.text.erb index 301287946d4..8bc305910c4 100644 --- a/app/views/notify/member_invite_declined_email.text.erb +++ b/app/views/notify/member_invite_declined_email.text.erb @@ -1,3 +1,3 @@ -<%= @invite_email %> has declined your invitation to join the <%= member_target_name %> <%= member_target_type %>. +<%= @invite_email %> has declined your invitation to join the <%= member_source.human_name %> <%= member_source.model_name.singular %>. -<%= member_target_url %> +<%= member_source.web_url %> diff --git a/app/views/notify/member_invited_email.html.haml b/app/views/notify/member_invited_email.html.haml index a8e58df9ac8..b8b75da3f2f 100644 --- a/app/views/notify/member_invited_email.html.haml +++ b/app/views/notify/member_invited_email.html.haml @@ -1,11 +1,11 @@ %p You have been invited - - if access_requester + - if member.created_by by - = link_to access_requester.name, user_url(access_requester) + = link_to member.created_by.name, user_url(member.created_by) to join the - = link_to member_target_name, member_target_url - #{member_target_type} as #{member.human_access}. + = link_to member_source.human_name, member_source.web_url + #{member_source.model_name.singular} as #{member.human_access}. %p = link_to 'Accept invitation', invite_url(@token) diff --git a/app/views/notify/member_invited_email.text.erb b/app/views/notify/member_invited_email.text.erb index 1b6897ee2ec..0a6393355be 100644 --- a/app/views/notify/member_invited_email.text.erb +++ b/app/views/notify/member_invited_email.text.erb @@ -1,4 +1,4 @@ -You have been invited <%= "by #{access_requester.name} " if access_requester %>to join the <%= member_target_name %> <%= member_target_type %> as <%= member.human_access %>. +You have been invited <%= "by #{member.created_by.name} " if member.created_by %>to join the <%= member_source.human_name %> <%= member_source.model_name.singular %> as <%= member.human_access %>. Accept invitation: <%= invite_url(@token) %> Decline invitation: <%= decline_invite_url(@token) %> diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml index f5bc1b4e409..2b19ee93eea 100644 --- a/app/views/projects/_home_panel.html.haml +++ b/app/views/projects/_home_panel.html.haml @@ -29,10 +29,13 @@ .project-clone-holder = render "shared/clone_panel" - .project-repo-buttons.btn-group.project-right-buttons - = render "projects/buttons/download" - = render 'projects/buttons/dropdown' - = render 'projects/buttons/notifications' + .project-repo-buttons.project-right-buttons + - if current_user + = render 'shared/members/access_request_buttons', source: @project + .btn-group + = render "projects/buttons/download" + = render 'projects/buttons/dropdown' + = render 'projects/buttons/notifications' :javascript new Star(); diff --git a/app/views/projects/buttons/_notifications.html.haml b/app/views/projects/buttons/_notifications.html.haml index 3b97dc9328f..a7a97181096 100644 --- a/app/views/projects/buttons/_notifications.html.haml +++ b/app/views/projects/buttons/_notifications.html.haml @@ -1,7 +1,7 @@ - if @notification_setting = form_for @notification_setting, url: namespace_project_notification_setting_path(@project.namespace.becomes(Namespace), @project), method: :patch, remote: true, html: { class: 'inline', id: 'notification-form' } do |f| = f.hidden_field :level - .dropdown + .dropdown.hidden-sm %button.btn.btn-default.notifications-btn#notifications-button{ data: { toggle: "dropdown" }, aria: { haspopup: "true", expanded: "false" } } = icon('bell') = notification_title(@notification_setting.level) diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 112a532f9d3..bcdbff08011 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -17,7 +17,7 @@ %a{ href: "##{dom_id(note)}" } = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago') .note-actions - - access = max_access_level(note.project, note.author) + - access = note.project.team.human_max_access(note.author.id) - if access %span.note-role.hidden-xs= access - if current_user diff --git a/app/views/projects/project_members/_group_members.html.haml b/app/views/projects/project_members/_group_members.html.haml index 78c12d52a78..cb6136c215a 100644 --- a/app/views/projects/project_members/_group_members.html.haml +++ b/app/views/projects/project_members/_group_members.html.haml @@ -6,8 +6,9 @@ (#{members.count}) - if can?(current_user, :admin_group_member, @group) .controls - = link_to group_group_members_path(@group), class: 'btn' do - Manage group members + = link_to 'Manage group members', + group_group_members_path(@group), + class: 'btn' %ul.content-list = render partial: 'shared/members/member', collection: members.limit(20), @@ -15,7 +16,4 @@ locals: { show_controls: false } - if members.size > 20 %li - and - = members.size - 20 - more. For full list visit - = link_to 'group members page', group_group_members_path(@group) + and #{members.count - 20} more. For full list visit #{link_to 'group members page', group_group_members_path(@group)} diff --git a/app/views/projects/project_members/index.html.haml b/app/views/projects/project_members/index.html.haml index 61a82724d69..357ccccaf1d 100644 --- a/app/views/projects/project_members/index.html.haml +++ b/app/views/projects/project_members/index.html.haml @@ -13,9 +13,9 @@ Users with access to this project are listed below. = render "new_project_member" - = render "shared/members/requests", entity: @project, members: @project_members + = render 'shared/members/requests', membership_source: @project, members: @project_members.request - = render "team", members: @project_members.non_request + = render 'team', members: @project_members.non_request - if @group = render "group_members", members: @group_members diff --git a/app/views/shared/_group_or_project_home_dropdown.html.haml b/app/views/shared/_group_or_project_home_dropdown.html.haml deleted file mode 100644 index fb9e63f2bd4..00000000000 --- a/app/views/shared/_group_or_project_home_dropdown.html.haml +++ /dev/null @@ -1,30 +0,0 @@ -- member = entity.send(members_association(entity)).find_by(user_id: current_user.id) -- can_edit = can?(current_user, "admin_#{entity.class.to_s.underscore}".to_sym, entity) - -- if member || can_edit - .dropdown.project-settings-dropdown - %a.dropdown-new.btn.btn-gray{ href: '#', id: "#{entity.class.to_s.underscore}-settings-button", data: { toggle: 'dropdown' } } - = icon('cog') - = icon('caret-down') - %ul.dropdown-menu.dropdown-menu-align-right - - if can_edit - %li - = link_to "Edit #{entity.class.to_s}", [:edit, entity] - - - if member - %li - = link_to "Leave #{entity.class.to_s}", - leave_path(entity), - method: :delete, - data: { confirm: leave_confirmation_message(entity) } -- elsif entity.access_requested?(current_user) - = link_to 'Withdraw Request', - leave_path(entity), - data: { confirm: withdraw_request_message(entity) }, - method: :delete, - class: 'btn btn-grouped btn-gray' -- else - = link_to 'Request Access', - request_access_path(entity), - method: :post, - class: 'btn btn-grouped btn-gray' diff --git a/app/views/shared/members/_access_request_buttons.html.haml b/app/views/shared/members/_access_request_buttons.html.haml new file mode 100644 index 00000000000..ed0a6ebcf84 --- /dev/null +++ b/app/views/shared/members/_access_request_buttons.html.haml @@ -0,0 +1,12 @@ +- member = source.members.find_by(user_id: current_user.id) + +- 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 access-request-button hidden-xs' +- else + = link_to 'Request Access', polymorphic_path([:request_access, source, :members]), + method: :post, + class: 'btn access-request-button hidden-xs' diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 7e119155a6c..c69d4cbfbe3 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -1,6 +1,6 @@ - show_roles = local_assigns.fetch(:show_roles, true) - show_controls = local_assigns.fetch(:show_controls, true) -- user = member.request? ? member.created_by : member.user +- user = member.user %li.js-toggle-container{ class: dom_class(member), id: dom_id(member) } %span{ class: ("list-item-name" if show_controls) } @@ -18,25 +18,25 @@ %strong Blocked - if member.request? - %small + %span.cgray – Requested = time_ago_with_tooltip(member.requested_at) - else = image_tag avatar_icon(member.invite_email, 24), class: "avatar s24", alt: '' %strong= member.invite_email %span.cgray - invited + – Invited - if member.created_by by = link_to member.created_by.name, user_path(member.created_by) = time_ago_with_tooltip(member.created_at) - if show_controls && can?(current_user, action_member_permission(:admin, member), member.source) - = link_to 'Resend invite', resend_invite_member_path(member), + = link_to 'Resend invite', polymorphic_path([:resend_invite, member]), method: :post, class: 'btn-xs btn' - - if show_roles && can_see_entity_roles?(current_user, member.source) + - if show_roles && can_see_member_roles?(source: member.source, user: current_user) %span.pull-right %strong= member.human_access - if show_controls @@ -48,30 +48,30 @@ - if member.request? - = link_to icon('check inverse'), approve_request_member_path(member), + = link_to icon('check inverse'), polymorphic_path([:approve_access_request, member]), method: :post, - type: 'button', class: 'btn-xs btn btn-success', title: 'Grant access' - if can?(current_user, action_member_permission(:destroy, member), member) - if current_user == user - = link_to leave_path(member.source), data: { confirm: leave_confirmation_message(member.source)}, method: :delete, class: "btn-xs btn btn-remove", title: 'Remove user from group' do - = icon("sign-out") - Leave - - else - = link_to icon('trash'), member_path(member), + = link_to icon('sign-out', text: 'Leave'), polymorphic_path([:leave, member.source, :members]), method: :delete, + data: { confirm: leave_confirmation_message(member.source) }, + class: 'btn-xs btn btn-remove' + - else + = link_to icon('trash'), member, remote: true, + method: :delete, data: { confirm: remove_member_message(member) }, class: 'btn-xs btn btn-remove', title: remove_member_title(member) .edit-member.hide.js-toggle-content %br - = form_for member_path(member), as: "#{member.source.class.to_s.underscore}_member".to_sym, remote: true do |f| + = form_for member, remote: true do |f| .prepend-top-10 - = f.select :access_level, options_for_select(member_class(member).access_level_roles, member.access_level), {}, class: 'form-control' + = f.select :access_level, options_for_select(member.class.access_level_roles, member.access_level), {}, class: 'form-control' .prepend-top-10 = f.submit 'Save', class: 'btn btn-save btn-sm' diff --git a/app/views/shared/members/_requests.html.haml b/app/views/shared/members/_requests.html.haml index ffbb380f794..b5963876034 100644 --- a/app/views/shared/members/_requests.html.haml +++ b/app/views/shared/members/_requests.html.haml @@ -1,10 +1,8 @@ -- requesters = members.request - -- if requesters.any? +- if members.any? .panel.panel-default .panel-heading - %strong= entity.name + %strong= membership_source.name access requests - %small= "(#{requesters.size})" + %small= "(#{members.size})" %ul.content-list - = render partial: 'shared/members/member', collection: requesters, as: :member + = render partial: 'shared/members/member', collection: members, as: :member diff --git a/features/steps/dashboard/group.rb b/features/steps/dashboard/group.rb index 0c6a0ae3725..9b79a3be49b 100644 --- a/features/steps/dashboard/group.rb +++ b/features/steps/dashboard/group.rb @@ -62,6 +62,6 @@ class Spinach::Features::DashboardGroup < Spinach::FeatureSteps end step 'I should see the "Can not leave message"' do - expect(page).to have_content "You can not leave Owned group because you're the last owner" + expect(page).to have_content "You can not leave the \"Owned\" group." end end diff --git a/features/steps/group/members.rb b/features/steps/group/members.rb index 9de82765df1..dfa2fa75def 100644 --- a/features/steps/group/members.rb +++ b/features/steps/group/members.rb @@ -53,7 +53,7 @@ class Spinach::Features::GroupMembers < Spinach::FeatureSteps step 'I should see "sjobs@apple.com" in team list as invited "Reporter"' do page.within '.content-list' do expect(page).to have_content('sjobs@apple.com') - expect(page).to have_content('invited') + expect(page).to have_content('Invited') expect(page).to have_content('Reporter') end end @@ -116,11 +116,9 @@ class Spinach::Features::GroupMembers < Spinach::FeatureSteps member = mary_jane_member page.within "#group_member_#{member.id}" do - find(".js-toggle-button").click - page.within "#edit_group_member_#{member.id}" do - select 'Developer', from: 'group_member_access_level' - click_on 'Save' - end + click_button "Edit access level" + select 'Developer', from: 'group_member_access_level' + click_on 'Save' end end diff --git a/features/steps/project/team_management.rb b/features/steps/project/team_management.rb index c6ced747370..f32576d2cb1 100644 --- a/features/steps/project/team_management.rb +++ b/features/steps/project/team_management.rb @@ -26,8 +26,11 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps end step 'I should see "Mike" in team list as "Reporter"' do - page.within ".access-reporter" do + user = User.find_by(name: 'Mike') + project_member = project.project_members.find_by(user_id: user.id) + page.within "#project_member_#{project_member.id}" do expect(page).to have_content('Mike') + expect(page).to have_content('Reporter') end end @@ -40,16 +43,20 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps end step 'I should see "sjobs@apple.com" in team list as invited "Reporter"' do - page.within ".access-reporter" do + project_member = project.project_members.find_by(invite_email: 'sjobs@apple.com') + page.within "#project_member_#{project_member.id}" do expect(page).to have_content('sjobs@apple.com') - expect(page).to have_content('invited') + expect(page).to have_content('Invited') expect(page).to have_content('Reporter') end end step 'I should see "Dmitriy" in team list as "Developer"' do - page.within ".access-developer" do + user = User.find_by(name: 'Dmitriy') + project_member = project.project_members.find_by(user_id: user.id) + page.within "#project_member_#{project_member.id}" do expect(page).to have_content('Dmitriy') + expect(page).to have_content('Developer') end end @@ -65,15 +72,14 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps end step 'I should see "Dmitriy" in team list as "Reporter"' do - page.within ".access-reporter" do + user = User.find_by(name: 'Dmitriy') + project_member = project.project_members.find_by(user_id: user.id) + page.within "#project_member_#{project_member.id}" do expect(page).to have_content('Dmitriy') + expect(page).to have_content('Reporter') end end - step 'I click link "Remove from team"' do - click_link "Remove from team" - end - step 'I should not see "Dmitriy" in team list' do user = User.find_by(name: "Dmitriy") expect(page).not_to have_content(user.name) @@ -120,7 +126,7 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps user = User.find_by(name: 'Dmitriy') project_member = project.project_members.find_by(user_id: user.id) page.within "#project_member_#{project_member.id}" do - click_link('Remove user from team') + click_link('Remove user from project') end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 14370ac218d..cc29c7ef428 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -88,10 +88,7 @@ module API class Group < Grape::Entity expose :id, :name, :path, :description, :visibility_level expose :avatar_url - - expose :web_url do |group, options| - Gitlab::Routing.url_helpers.group_url(group) - end + expose :web_url end class GroupDetail < Group diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 0ca8a656f63..89c2c26a367 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -35,7 +35,7 @@ describe Groups::GroupMembersController do let(:group_user) { create(:user) } let(:member) do group.add_developer(group_user) - group.group_members.find_by(user_id: group_user.id) + group.members.find_by(user_id: group_user) end context 'when user does not have enough rights' do @@ -103,7 +103,7 @@ describe Groups::GroupMembersController do it 'removes user from members' do delete :leave, group_id: group - expect(response).to set_flash.to "You left #{group.name} group." + expect(response).to set_flash.to "You left the \"#{group.name}\" group." expect(response).to redirect_to(dashboard_groups_path) expect(group.users).not_to include user end @@ -118,8 +118,8 @@ describe Groups::GroupMembersController do it 'cannot removes himself from the group' do delete :leave, group_id: group - expect(response).to redirect_to(dashboard_groups_path) - expect(response).to set_flash[:alert].to "You can not leave #{group.name} group because you're the last owner. Transfer or delete the group." + expect(response).to redirect_to(group_path(group)) + expect(response).to set_flash[:alert].to "You can not leave the \"#{group.name}\" group. Transfer or delete the group." expect(group.users).to include user end end @@ -133,9 +133,9 @@ describe Groups::GroupMembersController do it 'removes user from members' do delete :leave, group_id: group - expect(response).to set_flash.to 'You withdrawn your access request to the group.' + expect(response).to set_flash.to 'Your access request to the group has been withdrawn.' expect(response).to redirect_to(dashboard_groups_path) - expect(group.group_members.request).to be_empty + expect(group.members.request).to be_empty expect(group.users).not_to include user end end @@ -155,18 +155,18 @@ 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.group_members.request.find_by(created_by_id: user.id).created_by).to eq user + expect(group.members.request.exists?(user_id: user)).to be_truthy expect(group.users).not_to include user end end - describe '#approve' do + describe '#approve_access_request' do let(:group) { create(:group, :public) } context 'when member is not found' do it 'returns 403' do post :approve_access_request, group_id: group, - id: 42 + id: 42 expect(response.status).to eq(403) end @@ -177,7 +177,7 @@ describe Groups::GroupMembersController do let(:group_requester) { create(:user) } let(:member) do group.request_access(group_requester) - group.group_members.request.find_by(created_by_id: group_requester.id) + group.members.request.find_by(user_id: group_requester) end context 'when user does not have enough rights' do @@ -188,7 +188,7 @@ describe Groups::GroupMembersController do it 'returns 403' do post :approve_access_request, group_id: group, - id: member + id: member expect(response.status).to eq(403) expect(group.users).not_to include group_requester @@ -203,7 +203,7 @@ describe Groups::GroupMembersController do it 'adds user to members' do post :approve_access_request, group_id: group, - id: member + id: member expect(response).to redirect_to(group_group_members_path(group)) expect(group.users).to include group_requester diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index d3bd2d0bbba..fc5f458e795 100644 --- a/spec/controllers/projects/project_members_controller_spec.rb +++ b/spec/controllers/projects/project_members_controller_spec.rb @@ -80,7 +80,7 @@ describe Projects::ProjectMembersController do let(:team_user) { create(:user) } let(:member) do project.team << [team_user, :developer] - project.project_members.find_by(user_id: team_user.id) + project.members.find_by(user_id: team_user.id) end context 'when user does not have enough rights' do @@ -154,7 +154,7 @@ describe Projects::ProjectMembersController do delete :leave, namespace_id: project.namespace, project_id: project - expect(response).to set_flash.to 'You left the project.' + expect(response).to set_flash.to "You left the \"#{project.human_name}\" project." expect(response).to redirect_to(dashboard_projects_path) expect(project.users).not_to include user end @@ -167,14 +167,14 @@ describe Projects::ProjectMembersController do sign_in(user) end - it 'cannot removes himself from the project' do + it 'cannot remove himself from the project' do delete :leave, namespace_id: project.namespace, project_id: project expect(response).to redirect_to( - namespace_project_project_members_path(project.namespace, project) + namespace_project_path(project.namespace, project) ) - expect(response).to set_flash[:alert].to 'You can not leave your own project. Transfer or delete the project.' + expect(response).to set_flash[:alert].to "You can not leave the \"#{project.human_name}\" project. Transfer or delete the project." expect(project.users).to include user end end @@ -189,9 +189,9 @@ describe Projects::ProjectMembersController do delete :leave, namespace_id: project.namespace, project_id: project - expect(response).to set_flash.to 'You withdrawn your access request to the project.' + expect(response).to set_flash.to 'Your access request to the project has been withdrawn.' expect(response).to redirect_to(dashboard_projects_path) - expect(project.project_members.request).to be_empty + expect(project.members.request).to be_empty expect(project.users).not_to include user end end @@ -214,7 +214,7 @@ describe Projects::ProjectMembersController do expect(response).to redirect_to( namespace_project_path(project.namespace, project) ) - expect(project.project_members.request.find_by(created_by_id: user.id).created_by).to eq user + expect(project.members.request.exists?(user_id: user)).to be_truthy expect(project.users).not_to include user end end @@ -225,8 +225,8 @@ describe Projects::ProjectMembersController do context 'when member is not found' do it 'returns 404' do post :approve_access_request, namespace_id: project.namespace, - project_id: project, - id: 42 + project_id: project, + id: 42 expect(response.status).to eq(404) end @@ -237,7 +237,7 @@ describe Projects::ProjectMembersController do let(:team_requester) { create(:user) } let(:member) do project.request_access(team_requester) - project.project_members.request.find_by(created_by_id: team_requester.id) + project.members.request.find_by(user_id: team_requester.id) end context 'when user does not have enough rights' do @@ -248,8 +248,8 @@ describe Projects::ProjectMembersController do it 'returns 404' do post :approve_access_request, namespace_id: project.namespace, - project_id: project, - id: member + project_id: project, + id: member expect(response.status).to eq(404) expect(project.users).not_to include team_requester @@ -264,8 +264,8 @@ describe Projects::ProjectMembersController do it 'adds user to members' do post :approve_access_request, namespace_id: project.namespace, - project_id: project, - id: member + project_id: project, + id: member expect(response).to redirect_to( namespace_project_project_members_path(project.namespace, project) 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 d5b5e0e35ea..22525ce530b 100644 --- a/spec/features/groups/members/owner_manages_access_requests_spec.rb +++ b/spec/features/groups/members/owner_manages_access_requests_spec.rb @@ -22,12 +22,10 @@ feature 'Groups > Members > Owner manages access requests', feature: true do expect_visible_access_request(group, user) - perform_enqueued_jobs do - click_on 'Grant access' - end + perform_enqueued_jobs { click_on 'Grant access' } expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email] - expect(ActionMailer::Base.deliveries.last.subject).to match /Access to #{group.name} group was granted/ + expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{group.name} group was granted" end scenario 'master can deny access' do @@ -35,17 +33,15 @@ feature 'Groups > Members > Owner manages access requests', feature: true do expect_visible_access_request(group, user) - perform_enqueued_jobs do - click_on 'Deny access' - end + perform_enqueued_jobs { click_on 'Deny access' } expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email] - expect(ActionMailer::Base.deliveries.last.subject).to match /Access to #{group.name} group was denied/ + expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{group.name} group was denied" end def expect_visible_access_request(group, user) - expect(group.access_requested?(user)).to be_truthy + expect(group.members.request.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 9b8492807fa..a878a96b6ee 100644 --- a/spec/features/groups/members/user_requests_access_spec.rb +++ b/spec/features/groups/members/user_requests_access_spec.rb @@ -8,47 +8,41 @@ feature 'Groups > Members > User requests access', feature: true do background do group.add_owner(owner) login_as(user) + visit group_path(group) end scenario 'user can request access to a group' do - visit group_path(group) - - perform_enqueued_jobs do - click_link 'Request Access' - end + perform_enqueued_jobs { click_link 'Request Access' } expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email] - expect(ActionMailer::Base.deliveries.last.subject).to match /Request to join #{group.name} group/ + expect(ActionMailer::Base.deliveries.last.subject).to match "Request to join the #{group.name} group" - expect(group.access_requested?(user)).to be_truthy + expect(group.members.request.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 Request' + + expect(page).to have_content 'Withdraw Access Request' end scenario 'user is not listed in the group members page' do - visit group_path(group) - click_link 'Request Access' - expect(group.access_requested?(user)).to be_truthy + expect(group.members.request.exists?(user_id: user)).to be_truthy click_link 'Members' - visit group_group_members_path(group) page.within('.content') do expect(page).not_to have_content(user.name) end end scenario 'user can withdraw its request for access' do - visit group_path(group) click_link 'Request Access' - expect(group.access_requested?(user)).to be_truthy + expect(group.members.request.exists?(user_id: user)).to be_truthy - click_link 'Withdraw Request' + click_link 'Withdraw Access Request' - expect(group.access_requested?(user)).to be_falsey - expect(page).to have_content 'You withdrawn your access request to the group.' + expect(group.members.request.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 1b5490ba97f..5fe4caa12f0 100644 --- a/spec/features/projects/members/master_manages_access_requests_spec.rb +++ b/spec/features/projects/members/master_manages_access_requests_spec.rb @@ -22,12 +22,10 @@ feature 'Projects > Members > Master manages access requests', feature: true do expect_visible_access_request(project, user) - perform_enqueued_jobs do - click_on 'Grant access' - end + perform_enqueued_jobs { click_on 'Grant access' } expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email] - expect(ActionMailer::Base.deliveries.last.subject).to match /Access to #{project.name_with_namespace} project was granted/ + expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{project.name_with_namespace} project was granted" end scenario 'master can deny access' do @@ -35,16 +33,14 @@ feature 'Projects > Members > Master manages access requests', feature: true do expect_visible_access_request(project, user) - perform_enqueued_jobs do - click_on 'Deny access' - end + perform_enqueued_jobs { click_on 'Deny access' } expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email] - expect(ActionMailer::Base.deliveries.last.subject).to match /Access to #{project.name_with_namespace} project was denied/ + expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{project.name_with_namespace} project was denied" end def expect_visible_access_request(project, user) - expect(project.access_requested?(user)).to be_truthy + expect(project.members.request.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 58a7ec1880d..fd92a3a2f0c 100644 --- a/spec/features/projects/members/user_requests_access_spec.rb +++ b/spec/features/projects/members/user_requests_access_spec.rb @@ -8,30 +8,27 @@ feature 'Projects > Members > User requests access', feature: true do background do project.team << [master, :master] login_as(user) + visit namespace_project_path(project.namespace, project) end scenario 'user can request access to a project' do - visit namespace_project_path(project.namespace, project) - - perform_enqueued_jobs do - click_link 'Request Access' - end + perform_enqueued_jobs { click_link 'Request Access' } expect(ActionMailer::Base.deliveries.last.to).to eq [master.notification_email] - expect(ActionMailer::Base.deliveries.last.subject).to match /Request to join #{project.name_with_namespace} project/ + expect(ActionMailer::Base.deliveries.last.subject).to eq "Request to join the #{project.name_with_namespace} project" - expect(project.access_requested?(user)).to be_truthy + expect(project.members.request.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 Request' + + expect(page).to have_content 'Withdraw Access Request' end scenario 'user is not listed in the project members page' do - visit namespace_project_path(project.namespace, project) - click_link 'Request Access' - expect(project.access_requested?(user)).to be_truthy + expect(project.members.request.exists?(user_id: user)).to be_truthy + open_project_settings_menu click_link 'Members' visit namespace_project_project_members_path(project.namespace, project) @@ -41,14 +38,17 @@ feature 'Projects > Members > User requests access', feature: true do end scenario 'user can withdraw its request for access' do - visit namespace_project_path(project.namespace, project) click_link 'Request Access' - expect(project.access_requested?(user)).to be_truthy + expect(project.members.request.exists?(user_id: user)).to be_truthy - click_link 'Withdraw Request' + click_link 'Withdraw Access Request' + + expect(project.members.request.exists?(user_id: user)).to be_falsey + expect(page).to have_content 'Your access request to the project has been withdrawn.' + end - expect(project.access_requested?(user)).to be_falsey - expect(page).to have_content 'You withdrawn your access request to the project.' + def open_project_settings_menu + find('#project-settings-button').click end end diff --git a/spec/helpers/gitlab_routing_helper_spec.rb b/spec/helpers/gitlab_routing_helper_spec.rb new file mode 100644 index 00000000000..14847d0a49e --- /dev/null +++ b/spec/helpers/gitlab_routing_helper_spec.rb @@ -0,0 +1,79 @@ +require 'spec_helper' + +describe GitlabRoutingHelper do + describe 'Project URL helpers' do + describe '#project_members_url' do + let(:project) { build_stubbed(:empty_project) } + + it { expect(project_members_url(project)).to eq namespace_project_project_members_url(project.namespace, project) } + end + + describe '#project_member_path' do + let(:project_member) { create(:project_member) } + + it { expect(project_member_path(project_member)).to eq namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) } + end + + describe '#request_access_project_members_path' do + let(:project) { build_stubbed(:empty_project) } + + it { expect(request_access_project_members_path(project)).to eq request_access_namespace_project_project_members_path(project.namespace, project) } + end + + describe '#leave_project_members_path' do + let(:project) { build_stubbed(:empty_project) } + + it { expect(leave_project_members_path(project)).to eq leave_namespace_project_project_members_path(project.namespace, project) } + end + + describe '#approve_access_request_project_member_path' do + let(:project_member) { create(:project_member) } + + it { expect(approve_access_request_project_member_path(project_member)).to eq approve_access_request_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) } + end + + describe '#resend_invite_project_member_path' do + let(:project_member) { create(:project_member) } + + it { expect(resend_invite_project_member_path(project_member)).to eq resend_invite_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) } + end + end + + describe 'Group URL helpers' do + describe '#group_members_url' do + let(:group) { build_stubbed(:group) } + + it { expect(group_members_url(group)).to eq group_group_members_url(group) } + end + + describe '#group_member_path' do + let(:group_member) { create(:group_member) } + + it { expect(group_member_path(group_member)).to eq group_group_member_path(group_member.source, group_member) } + end + + describe '#request_access_group_members_path' do + let(:group) { build_stubbed(:group) } + + it { expect(request_access_group_members_path(group)).to eq request_access_group_group_members_path(group) } + end + + describe '#leave_group_members_path' do + let(:group) { build_stubbed(:group) } + + it { expect(leave_group_members_path(group)).to eq leave_group_group_members_path(group) } + end + + describe '#approve_access_request_group_member_path' do + let(:group_member) { create(:group_member) } + + it { expect(approve_access_request_group_member_path(group_member)).to eq approve_access_request_group_group_member_path(group_member.source, group_member) } + end + + describe '#resend_invite_group_member_path' do + let(:group_member) { create(:group_member) } + + it { expect(resend_invite_group_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) } + end + end +end diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index c2f10e1db75..0b1a76156e0 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/spec/helpers/members_helper_spec.rb @@ -1,22 +1,6 @@ require 'spec_helper' describe MembersHelper do - describe '#member_class' do - let(:project_member) { build(:project_member) } - let(:group_member) { build(:group_member) } - - it { expect(member_class(project_member)).to eq ProjectMember } - it { expect(member_class(group_member)).to eq GroupMember } - end - - describe '#members_association' do - let(:project) { build_stubbed(:project) } - let(:group) { build_stubbed(:group) } - - it { expect(members_association(project)).to eq :project_members } - it { expect(members_association(group)).to eq :group_members } - end - describe '#action_member_permission' do let(:project_member) { build(:project_member) } let(:group_member) { build(:group_member) } @@ -25,73 +9,20 @@ describe MembersHelper do it { expect(action_member_permission(:admin, group_member)).to eq :admin_group_member } end - describe '#can_see_entity_roles?' do - let(:project) { create(:project) } + describe '#can_see_member_roles?' do + let(:project) { create(:empty_project) } let(:group) { create(:group) } let(:user) { build(:user) } let(:admin) { build(:user, :admin) } let(:project_member) { create(:project_member, project: project) } let(:group_member) { create(:group_member, group: group) } - it { expect(can_see_entity_roles?(nil, project)).to be_falsy } - it { expect(can_see_entity_roles?(nil, group)).to be_falsy } - it { expect(can_see_entity_roles?(admin, project)).to be_truthy } - it { expect(can_see_entity_roles?(admin, group)).to be_truthy } - it { expect(can_see_entity_roles?(project_member.user, project)).to be_truthy } - it { expect(can_see_entity_roles?(group_member.user, group)).to be_truthy } - end - - describe '#member_path' do - let(:project_member) { create(:project_member) } - let(:group_member) { create(:group_member) } - - it { expect(member_path(project_member)).to eq namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) } - it { expect(member_path(group_member)).to eq group_group_member_path(group_member.source, group_member) } - it { expect { member_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' } - end - - describe '#resend_invite_member_path' do - let(:project_member) { create(:project_member) } - let(:group_member) { create(:group_member) } - - it { expect(resend_invite_member_path(project_member)).to eq resend_invite_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) } - it { expect(resend_invite_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) } - it { expect { resend_invite_member_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' } - end - - describe '#request_access_path' do - let(:project) { build_stubbed(:project) } - let(:group) { build_stubbed(:group) } - - it { expect(request_access_path(project)).to eq request_access_namespace_project_project_members_path(project.namespace, project) } - it { expect(request_access_path(group)).to eq request_access_group_group_members_path(group) } - it { expect { request_access_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' } - end - - describe '#approve_request_member_path' do - let(:project_member) { create(:project_member) } - let(:group_member) { create(:group_member) } - - it { expect(approve_request_member_path(project_member)).to eq approve_access_request_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) } - it { expect(approve_request_member_path(group_member)).to eq approve_access_request_group_group_member_path(group_member.source, group_member) } - it { expect { approve_request_member_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' } - end - - describe '#leave_path' do - let(:project) { build_stubbed(:project) } - let(:group) { build_stubbed(:group) } - - it { expect(leave_path(project)).to eq leave_namespace_project_project_members_path(project.namespace, project) } - it { expect(leave_path(group)).to eq leave_group_group_members_path(group) } - it { expect { leave_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' } - end - - describe '#withdraw_request_message' do - let(:project) { build_stubbed(:project) } - let(:group) { build_stubbed(:group) } - - it { expect(withdraw_request_message(project)).to eq "Are you sure you want to withdraw your access request for the \"#{project.name_with_namespace}\" project?" } - it { expect(withdraw_request_message(group)).to eq "Are you sure you want to withdraw your access request for the \"#{group.name}\" group?" } + it { expect(can_see_member_roles?(source: project, user: nil)).to be_falsy } + it { expect(can_see_member_roles?(source: group, user: nil)).to be_falsy } + it { expect(can_see_member_roles?(source: project, user: admin)).to be_truthy } + it { expect(can_see_member_roles?(source: group, user: admin)).to be_truthy } + it { expect(can_see_member_roles?(source: project, user: project_member.user)).to be_truthy } + it { expect(can_see_member_roles?(source: group, user: group_member.user)).to be_truthy } end describe '#remove_member_message' do @@ -105,12 +36,14 @@ describe MembersHelper do let(:group_member_invite) { build(:group_member, group: group).tap { |m| m.generate_invite_token! } } let(:group_member_request) { group.request_access(requester) } - it { expect(remove_member_message(project_member)).to eq "You are going to remove #{project_member.user.name} from the #{project.name_with_namespace} project. Are you sure?" } - it { expect(remove_member_message(project_member_invite)).to eq "You are going to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.name_with_namespace} project. Are you sure?" } - it { expect(remove_member_message(project_member_request)).to eq "You are going to deny #{requester.name}'s request to join the #{project.name_with_namespace} project. Are you sure?" } - it { expect(remove_member_message(group_member)).to eq "You are going to remove #{group_member.user.name} from the #{group.name} group. Are you sure?" } - it { expect(remove_member_message(group_member_invite)).to eq "You are going to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group. Are you sure?" } - it { expect(remove_member_message(group_member_request)).to eq "You are going to deny #{requester.name}'s request to join the #{group.name} group. Are you sure?" } + it { expect(remove_member_message(project_member)).to eq "Are you sure you want to remove #{project_member.user.name} from the #{project.name_with_namespace} project?" } + it { expect(remove_member_message(project_member_invite)).to eq "Are you sure you want to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.name_with_namespace} project?" } + it { expect(remove_member_message(project_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{project.name_with_namespace} project?" } + it { expect(remove_member_message(project_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{project.name_with_namespace} project?" } + it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group?" } + it { expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group?" } + it { expect(remove_member_message(group_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{group.name} group?" } + it { expect(remove_member_message(group_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{group.name} group?" } end describe '#remove_member_title' do @@ -122,10 +55,10 @@ describe MembersHelper do let(:group_member) { build(:group_member, group: group) } let(:group_member_request) { group.request_access(requester) } - it { expect(remove_member_title(project_member)).to eq 'Remove user' } - it { expect(remove_member_title(project_member_request)).to eq 'Deny access request' } - it { expect(remove_member_title(group_member)).to eq 'Remove user' } - it { expect(remove_member_title(group_member_request)).to eq 'Deny access request' } + it { expect(remove_member_title(project_member)).to eq 'Remove user from project' } + it { expect(remove_member_title(project_member_request)).to eq 'Deny access request from project' } + it { expect(remove_member_title(group_member)).to eq 'Remove user from group' } + it { expect(remove_member_title(group_member_request)).to eq 'Deny access request from group' } end describe '#leave_confirmation_message' do @@ -133,7 +66,7 @@ describe MembersHelper do let(:group) { build_stubbed(:group) } let(:user) { build_stubbed(:user) } - it { expect(leave_confirmation_message(project)).to eq "Are you sure you want to leave \"#{project.name_with_namespace}\" project?" } - it { expect(leave_confirmation_message(group)).to eq "Are you sure you want to leave \"#{group.name}\" group?" } + it { expect(leave_confirmation_message(project)).to eq "Are you sure you want to leave the \"#{project.name_with_namespace}\" project?" } + it { expect(leave_confirmation_message(group)).to eq "Are you sure you want to leave the \"#{group.name}\" group?" } end end diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index fa81c28849e..09e0bbfd00b 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -1,25 +1,6 @@ require 'spec_helper' describe ProjectsHelper do - describe '#max_access_level' do - let(:master) { create(:user) } - let(:owner) { create(:user) } - let(:reporter) { create(:user) } - let(:group) { create(:group) } - let(:project) { build_stubbed(:empty_project, namespace: group) } - - before do - group.add_master(master) - group.add_owner(owner) - group.add_reporter(reporter) - end - - it { expect(max_access_level(project, master)).to eq 'Master' } - it { expect(max_access_level(project, owner)).to eq 'Owner' } - it { expect(max_access_level(project, reporter)).to eq 'Reporter' } - it { expect(max_access_level(project, build_stubbed(:user))).to be_nil } - end - describe "#project_status_css_class" do it "returns appropriate class" do expect(project_status_css_class("started")).to eq("active") diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index a86ec865b5d..1e6eb20ab39 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -405,7 +405,7 @@ describe Notify do let(:user) { create(:user) } let(:project_member) do project.request_access(user) - project.project_members.find_by(created_by_id: user.id) + project.members.request.find_by(user_id: user.id) end subject { Notify.member_access_requested_email('project', project_member.id) } @@ -413,10 +413,12 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Request to join the #{project.name_with_namespace} project" } - it { is_expected.to have_body_text /#{project.name_with_namespace}/ } - it { is_expected.to have_body_text /#{project.web_url}/ } - it { is_expected.to have_body_text /#{project_member.human_access}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Request to join the #{project.name_with_namespace} project" + is_expected.to have_body_text /#{project.name_with_namespace}/ + is_expected.to have_body_text /#{namespace_project_project_members_url(project.namespace, project)}/ + is_expected.to have_body_text /#{project_member.human_access}/ + end end describe 'project access denied' do @@ -424,7 +426,7 @@ describe Notify do let(:user) { create(:user) } let(:project_member) do project.request_access(user) - project.project_members.find_by(created_by_id: user.id) + project.members.request.find_by(user_id: user.id) end subject { Notify.member_access_denied_email('project', project.id, user.id) } @@ -432,9 +434,11 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Access to the #{project.name_with_namespace} project was denied" } - it { is_expected.to have_body_text /#{project.name_with_namespace}/ } - it { is_expected.to have_body_text /#{project.web_url}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Access to the #{project.name_with_namespace} project was denied" + is_expected.to have_body_text /#{project.name_with_namespace}/ + is_expected.to have_body_text /#{project.web_url}/ + end end describe 'project access changed' do @@ -447,10 +451,12 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Access to the #{project.name_with_namespace} project was granted" } - it { is_expected.to have_body_text /#{project.name_with_namespace}/ } - it { is_expected.to have_body_text /#{project.web_url}/ } - it { is_expected.to have_body_text /#{project_member.human_access}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Access to the #{project.name_with_namespace} project was granted" + is_expected.to have_body_text /#{project.name_with_namespace}/ + is_expected.to have_body_text /#{project.web_url}/ + is_expected.to have_body_text /#{project_member.human_access}/ + end end def invite_to_project(project:, email:, inviter:) @@ -470,11 +476,13 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Invitation to join the #{project.name_with_namespace} project" } - it { is_expected.to have_body_text /#{project.name_with_namespace}/ } - it { is_expected.to have_body_text /#{project.web_url}/ } - it { is_expected.to have_body_text /#{project_member.human_access}/ } - it { is_expected.to have_body_text /#{project_member.invite_token}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Invitation to join the #{project.name_with_namespace} project" + is_expected.to have_body_text /#{project.name_with_namespace}/ + is_expected.to have_body_text /#{project.web_url}/ + is_expected.to have_body_text /#{project_member.human_access}/ + is_expected.to have_body_text /#{project_member.invite_token}/ + end end describe 'project invitation accepted' do @@ -493,11 +501,13 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject 'Invitation accepted' } - it { is_expected.to have_body_text /#{project.name_with_namespace}/ } - it { is_expected.to have_body_text /#{project.web_url}/ } - it { is_expected.to have_body_text /#{project_member.invite_email}/ } - it { is_expected.to have_body_text /#{invited_user.name}/ } + it 'contains all the useful information' do + is_expected.to have_subject 'Invitation accepted' + is_expected.to have_body_text /#{project.name_with_namespace}/ + is_expected.to have_body_text /#{project.web_url}/ + is_expected.to have_body_text /#{project_member.invite_email}/ + is_expected.to have_body_text /#{invited_user.name}/ + end end describe 'project invitation declined' do @@ -515,10 +525,12 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject 'Invitation declined' } - it { is_expected.to have_body_text /#{project.name_with_namespace}/ } - it { is_expected.to have_body_text /#{project.web_url}/ } - it { is_expected.to have_body_text /#{project_member.invite_email}/ } + it 'contains all the useful information' do + is_expected.to have_subject 'Invitation declined' + is_expected.to have_body_text /#{project.name_with_namespace}/ + is_expected.to have_body_text /#{project.web_url}/ + is_expected.to have_body_text /#{project_member.invite_email}/ + end end context 'items that are noteable, the email for a note' do @@ -639,7 +651,7 @@ describe Notify do let(:user) { create(:user) } let(:group_member) do group.request_access(user) - group.group_members.find_by(created_by_id: user.id) + group.members.request.find_by(user_id: user.id) end subject { Notify.member_access_requested_email('group', group_member.id) } @@ -647,10 +659,12 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Request to join the #{group.name} group" } - it { is_expected.to have_body_text /#{group.name}/ } - it { is_expected.to have_body_text /#{group.web_url}/ } - it { is_expected.to have_body_text /#{group_member.human_access}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Request to join the #{group.name} group" + is_expected.to have_body_text /#{group.name}/ + is_expected.to have_body_text /#{group_group_members_url(group)}/ + is_expected.to have_body_text /#{group_member.human_access}/ + end end describe 'group access denied' do @@ -658,7 +672,7 @@ describe Notify do let(:user) { create(:user) } let(:group_member) do group.request_access(user) - group.group_members.find_by(created_by_id: user.id) + group.members.request.find_by(user_id: user.id) end subject { Notify.member_access_denied_email('group', group.id, user.id) } @@ -666,9 +680,11 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Access to the #{group.name} group was denied" } - it { is_expected.to have_body_text /#{group.name}/ } - it { is_expected.to have_body_text /#{group.web_url}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Access to the #{group.name} group was denied" + is_expected.to have_body_text /#{group.name}/ + is_expected.to have_body_text /#{group.web_url}/ + end end describe 'group access changed' do @@ -682,10 +698,12 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Access to the #{group.name} group was granted" } - it { is_expected.to have_body_text /#{group.name}/ } - it { is_expected.to have_body_text /#{group.web_url}/ } - it { is_expected.to have_body_text /#{group_member.human_access}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Access to the #{group.name} group was granted" + is_expected.to have_body_text /#{group.name}/ + is_expected.to have_body_text /#{group.web_url}/ + is_expected.to have_body_text /#{group_member.human_access}/ + end end def invite_to_group(group:, email:, inviter:) @@ -705,11 +723,13 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject "Invitation to join the #{group.name} group" } - it { is_expected.to have_body_text /#{group.name}/ } - it { is_expected.to have_body_text /#{group.web_url}/ } - it { is_expected.to have_body_text /#{group_member.human_access}/ } - it { is_expected.to have_body_text /#{group_member.invite_token}/ } + it 'contains all the useful information' do + is_expected.to have_subject "Invitation to join the #{group.name} group" + is_expected.to have_body_text /#{group.name}/ + is_expected.to have_body_text /#{group.web_url}/ + is_expected.to have_body_text /#{group_member.human_access}/ + is_expected.to have_body_text /#{group_member.invite_token}/ + end end describe 'group invitation accepted' do @@ -728,11 +748,13 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject 'Invitation accepted' } - it { is_expected.to have_body_text /#{group.name}/ } - it { is_expected.to have_body_text /#{group.web_url}/ } - it { is_expected.to have_body_text /#{group_member.invite_email}/ } - it { is_expected.to have_body_text /#{invited_user.name}/ } + it 'contains all the useful information' do + is_expected.to have_subject 'Invitation accepted' + is_expected.to have_body_text /#{group.name}/ + is_expected.to have_body_text /#{group.web_url}/ + is_expected.to have_body_text /#{group_member.invite_email}/ + is_expected.to have_body_text /#{invited_user.name}/ + end end describe 'group invitation declined' do @@ -750,10 +772,12 @@ describe Notify do it_behaves_like 'it should not have Gmail Actions links' it_behaves_like "a user cannot unsubscribe through footer link" - it { is_expected.to have_subject 'Invitation declined' } - it { is_expected.to have_body_text /#{group.name}/ } - it { is_expected.to have_body_text /#{group.web_url}/ } - it { is_expected.to have_body_text /#{group_member.invite_email}/ } + it 'contains all the useful information' do + is_expected.to have_subject 'Invitation declined' + is_expected.to have_body_text /#{group.name}/ + is_expected.to have_body_text /#{group.web_url}/ + is_expected.to have_body_text /#{group_member.invite_email}/ + end end end diff --git a/spec/models/concerns/access_requestable_spec.rb b/spec/models/concerns/access_requestable_spec.rb index 2dfed1eb4c4..98307876962 100644 --- a/spec/models/concerns/access_requestable_spec.rb +++ b/spec/models/concerns/access_requestable_spec.rb @@ -7,8 +7,7 @@ describe AccessRequestable do let(:user) { create(:user) } it { expect(group.request_access(user)).to be_a(GroupMember) } - it { expect(group.request_access(user).user).to be_nil } - it { expect(group.request_access(user).created_by).to eq(user) } + it { expect(group.request_access(user).user).to eq(user) } end describe '#access_requested?' do @@ -17,7 +16,7 @@ describe AccessRequestable do before { group.request_access(user) } - it { expect(group.access_requested?(user)).to be_truthy } + it { expect(group.members.request.exists?(user_id: user)).to be_truthy } end end @@ -35,7 +34,7 @@ describe AccessRequestable do before { project.request_access(user) } - it { expect(project.access_requested?(user)).to be_truthy } + it { expect(project.members.request.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 52f9d57bc0a..ccdcb29f773 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -10,17 +10,6 @@ describe Group, models: true do 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 '#group_members' do - let(:user) { create(:user) } - let(:group) { create(:group) } - - before { group.request_access(user) } - - it 'does not includes membership requests' do - expect(user.group_members).to be_empty - end - end end describe 'modules' do diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index a3d525d8d56..3ed3202ac6c 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -55,45 +55,78 @@ describe Member, models: true do end end - describe 'Scopes' do + describe 'Scopes & finders' do before do project = create(:project) - @invited_member = build(:project_member, user: nil).tap { |m| m.generate_invite_token! } - @accepted_invite_member = build(:project_member, user: nil).tap { |m| m.generate_invite_token! && m.accept_invite!(build(:user)) } + group = create(:group) + @owner_user = create(:user).tap { |u| group.add_owner(u) } + @owner = group.members.find_by(user_id: @owner_user.id) + + @master_user = create(:user).tap { |u| project.team << [u, :master] } + @master = project.members.find_by(user_id: @master_user.id) + + ProjectMember.add_user(project.members, 'toto1@example.com', Gitlab::Access::DEVELOPER, @master_user) + @invited_member = project.members.invite.find_by_invite_email('toto1@example.com') + + accepted_invite_user = build(:user) + ProjectMember.add_user(project.members, 'toto2@example.com', Gitlab::Access::DEVELOPER, @master_user) + @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.project_members.find_by(created_by_id: requested_user.id) + @requested_member = project.members.request.find_by(user_id: requested_user.id) + accepted_request_user = create(:user).tap { |u| project.request_access(u) } - @accepted_request_member = project.project_members.find_by(created_by_id: accepted_request_user.id).tap { |m| m.accept_request } + @accepted_request_member = project.members.request.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request } end - describe '#invite' do + describe '.invite' do + it { expect(described_class.invite).not_to include @master } it { expect(described_class.invite).to include @invited_member } it { expect(described_class.invite).not_to include @accepted_invite_member } it { expect(described_class.invite).not_to include @requested_member } it { expect(described_class.invite).not_to include @accepted_request_member } end - describe '#request' do + describe '.non_invite' do + it { expect(described_class.non_invite).to include @master } + it { expect(described_class.non_invite).not_to include @invited_member } + it { expect(described_class.non_invite).to include @accepted_invite_member } + it { expect(described_class.non_invite).to include @requested_member } + it { expect(described_class.non_invite).to include @accepted_request_member } + end + + describe '.request' do + it { expect(described_class.request).not_to include @master } it { expect(described_class.request).not_to include @invited_member } it { expect(described_class.request).not_to include @accepted_invite_member } it { expect(described_class.request).to include @requested_member } it { expect(described_class.request).not_to include @accepted_request_member } end - describe '#non_request' do + 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 + 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 } + it { expect(described_class.owners_and_masters).not_to include @invited_member } + it { expect(described_class.owners_and_masters).not_to include @accepted_invite_member } + it { expect(described_class.owners_and_masters).not_to include @requested_member } + it { expect(described_class.owners_and_masters).not_to include @accepted_request_member } + end end describe "Delegate methods" do @@ -101,6 +134,18 @@ describe Member, models: true do it { is_expected.to respond_to(:user_email) } end + describe 'Callbacks' do + describe 'after_destroy :post_decline_request, if: :request?' do + let(:member) { create(:project_member, requested_at: Time.now.utc) } + + it 'calls #post_decline_request' do + expect(member).to receive(:post_decline_request) + + member.destroy + end + end + end + describe ".add_user" do let!(:user) { create(:user) } let(:project) { create(:project) } @@ -139,18 +184,9 @@ describe Member, models: true do end describe '#accept_request' do - let(:user) { create(:user) } - let(:member) { create(:project_member, requested_at: Time.now.utc, user: nil, created_by: user) } - - it 'returns true' do - expect(member.accept_request).to be_truthy - end + let(:member) { create(:project_member, requested_at: Time.now.utc) } - it 'sets the user' do - member.accept_request - - expect(member.user).to eq(user) - end + it { expect(member.accept_request).to be_truthy } it 'clears requested_at' do member.accept_request @@ -165,25 +201,24 @@ describe Member, models: true do end end - describe '#decline_request' do - let(:user) { create(:user) } - let(:member) { create(:project_member, requested_at: Time.now.utc, user: nil, created_by: user) } + describe '#invite?' do + subject { create(:project_member, invite_email: "user@example.com", user: nil) } - it 'returns true' do - expect(member.decline_request).to be_truthy - end + it { is_expected.to be_invite } + end - it 'destroys the member' do - member.decline_request + describe '#request?' do + subject { create(:project_member, requested_at: Time.now.utc) } - expect(member).to be_destroyed - end + it { is_expected.to be_request } + end - it 'calls #after_decline_request' do - expect(member).to receive(:after_decline_request) + describe '#pending?' do + let(:invited_member) { create(:project_member, invite_email: "user@example.com", user: nil) } + let(:requester) { create(:project_member, requested_at: Time.now.utc) } - member.decline_request - end + it { expect(invited_member).to be_invite } + it { expect(requester).to be_pending } end describe "#accept_invite!" do diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index c3070d4cb78..eeb74a462ac 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -51,24 +51,30 @@ describe GroupMember, models: true do end end - describe 'after accept_request' do - let(:member) { create(:group_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) } + describe '#after_accept_request' do + it 'calls NotificationService.accept_group_access_request' do + member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now) - it "calls #accept_group_access_request" do expect_any_instance_of(NotificationService).to receive(:new_group_member) - member.accept_request + member.__send__(:after_accept_request) end end - describe 'after decline_request' do - let(:member) { create(:group_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) } + describe '#post_decline_request' do + it 'calls NotificationService.decline_group_access_request' do + member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now) - it "calls #decline_group_access_request" do expect_any_instance_of(NotificationService).to receive(:decline_group_access_request) - member.decline_request + member.__send__(:post_decline_request) end end + + describe '#real_source_type' do + subject { create(:group_member).real_source_type } + + it { is_expected.to eq 'Group' } + end end end diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 99b3c77c6cd..1e466f9c620 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -33,6 +33,12 @@ describe ProjectMember, models: true do it { is_expected.to include_module(Gitlab::ShellAdapter) } end + describe '#real_source_type' do + subject { create(:project_member).real_source_type } + + it { is_expected.to eq 'Project' } + end + describe "#destroy" do let(:owner) { create(:project_member, access_level: ProjectMember::OWNER) } let(:project) { owner.project } @@ -137,23 +143,23 @@ describe ProjectMember, models: true do end describe 'notifications' do - describe 'after accept_request' do - let(:member) { create(:project_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) } + describe '#after_accept_request' do + it 'calls NotificationService.new_project_member' do + member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now) - it 'calls #accept_project_access_request' do expect_any_instance_of(NotificationService).to receive(:new_project_member) - member.accept_request + member.__send__(:after_accept_request) end end - describe 'after decline_request' do - let(:member) { create(:project_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) } + describe '#post_decline_request' do + it 'calls NotificationService.decline_project_access_request' do + member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now) - it 'calls #decline_project_access_request' do expect_any_instance_of(NotificationService).to receive(:decline_project_access_request) - member.decline_request + member.__send__(:post_decline_request) end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index d5a4b73affd..30aa2b70c8d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -29,17 +29,6 @@ describe Project, models: true do it { is_expected.to have_many(:variables) } it { is_expected.to have_many(:triggers) } it { is_expected.to have_many(:todos).dependent(:destroy) } - - describe '#project_members' do - let(:user) { create(:user) } - let(:project) { create(:project) } - - before { project.request_access(user) } - - it 'does not includes membership requests' do - expect(user.project_members).to be_empty - end - end end describe 'modules' do @@ -100,11 +89,17 @@ describe Project, models: true do it { is_expected.to respond_to(:repo_exists?) } it { is_expected.to respond_to(:update_merge_requests) } it { is_expected.to respond_to(:execute_hooks) } - it { is_expected.to respond_to(:name_with_namespace) } it { is_expected.to respond_to(:owner) } it { is_expected.to respond_to(:path_with_namespace) } end + describe '#name_with_namespace' do + let(:project) { build_stubbed(:empty_project) } + + it { expect(project.name_with_namespace).to eq "#{project.namespace.human_name} / #{project.name}" } + it { expect(project.human_name).to eq project.name_with_namespace } + end + describe '#to_reference' do let(:project) { create(:empty_project) } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 36b1f439955..9262aeb6ed8 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -112,6 +112,28 @@ describe ProjectTeam, models: true do end end + describe "#human_max_access" do + it 'returns Master role' do + user = create(:user) + group = create(:group) + group.add_master(user) + + project = build_stubbed(:empty_project, namespace: group) + + expect(project.team.human_max_access(user.id)).to eq 'Master' + end + + it 'returns Owner role' do + user = create(:user) + group = create(:group) + group.add_owner(user) + + project = build_stubbed(:empty_project, namespace: group) + + expect(project.team.human_max_access(user.id)).to eq 'Owner' + end + end + describe '#max_member_access' do let(:requester) { create(:user) } |