Commit e64594ac authored by Robert Speicher's avatar Robert Speicher

Merge branch...

Merge branch '21983-member-add_user-doesn-t-detect-existing-members-that-have-requested-access' into 'master'

Resolve "`Member.add_user`doesn't detect existing members that have requested access"

## What does this MR do?

This merge request handle the case when an access requester is added to a group or project (via the members page or the API).

In `Member.add_user`, if an access requester already exists, we simply accept their request (and set the `created_by`, `access_level` and `expires_at` attributes if given).

## Are there points in the code the reviewer needs to double check?

I've taken the opportunity to cleanup the whole `{Group,Project}Member.add_user*` methods since it was quite a mess.

## What are the relevant issue numbers?

Closes #21983

See merge request !6393
parents 076e0406 60790cac
...@@ -102,40 +102,44 @@ class Group < Namespace ...@@ -102,40 +102,44 @@ class Group < Namespace
self[:lfs_enabled] self[:lfs_enabled]
end end
def add_users(user_ids, access_level, current_user: nil, expires_at: nil) def add_users(users, access_level, current_user: nil, expires_at: nil)
user_ids.each do |user_id| GroupMember.add_users_to_group(
Member.add_user( self,
self.group_members, users,
user_id, access_level,
access_level, current_user: current_user,
current_user: current_user, expires_at: expires_at
expires_at: expires_at )
)
end
end end
def add_user(user, access_level, current_user: nil, expires_at: nil) def add_user(user, access_level, current_user: nil, expires_at: nil)
add_users([user], access_level, current_user: current_user, expires_at: expires_at) GroupMember.add_user(
self,
user,
access_level,
current_user: current_user,
expires_at: expires_at
)
end end
def add_guest(user, current_user = nil) def add_guest(user, current_user = nil)
add_user(user, Gitlab::Access::GUEST, current_user: current_user) add_user(user, :guest, current_user: current_user)
end end
def add_reporter(user, current_user = nil) def add_reporter(user, current_user = nil)
add_user(user, Gitlab::Access::REPORTER, current_user: current_user) add_user(user, :reporter, current_user: current_user)
end end
def add_developer(user, current_user = nil) def add_developer(user, current_user = nil)
add_user(user, Gitlab::Access::DEVELOPER, current_user: current_user) add_user(user, :developer, current_user: current_user)
end end
def add_master(user, current_user = nil) def add_master(user, current_user = nil)
add_user(user, Gitlab::Access::MASTER, current_user: current_user) add_user(user, :master, current_user: current_user)
end end
def add_owner(user, current_user = nil) def add_owner(user, current_user = nil)
add_user(user, Gitlab::Access::OWNER, current_user: current_user) add_user(user, :owner, current_user: current_user)
end end
def has_owner?(user) def has_owner?(user)
......
...@@ -80,49 +80,70 @@ class Member < ActiveRecord::Base ...@@ -80,49 +80,70 @@ class Member < ActiveRecord::Base
find_by(invite_token: invite_token) find_by(invite_token: invite_token)
end end
# This method is used to find users that have been entered into the "Add members" field. def add_user(source, user, access_level, current_user: nil, expires_at: nil)
# These can be the User objects directly, their IDs, their emails, or new emails to be invited. user = retrieve_user(user)
def user_for_id(user_id) access_level = retrieve_access_level(access_level)
return user_id if user_id.is_a?(User)
user = User.find_by(id: user_id)
user ||= User.find_by(email: user_id)
user ||= user_id
user
end
def add_user(members, user_id, access_level, current_user: nil, expires_at: nil)
user = user_for_id(user_id)
# `user` can be either a User object or an email to be invited # `user` can be either a User object or an email to be invited
if user.is_a?(User) member =
member = members.find_or_initialize_by(user_id: user.id) if user.is_a?(User)
source.members.find_by(user_id: user.id) ||
source.requesters.find_by(user_id: user.id) ||
source.members.build(user_id: user.id)
else
source.members.build(invite_email: user)
end
return member unless can_update_member?(current_user, member)
member.attributes = {
created_by: member.created_by || current_user,
access_level: access_level,
expires_at: expires_at
}
if member.request?
::Members::ApproveAccessRequestService.new(source, current_user, id: member.id).execute
else else
member = members.build member.save
member.invite_email = user
end end
if can_update_member?(current_user, member) || project_creator?(member, access_level) member
member.created_by ||= current_user end
member.access_level = access_level
member.expires_at = expires_at
member.save def access_levels
end Gitlab::Access.sym_options
end end
private private
# This method is used to find users that have been entered into the "Add members" field.
# These can be the User objects directly, their IDs, their emails, or new emails to be invited.
def retrieve_user(user)
return user if user.is_a?(User)
User.find_by(id: user) || User.find_by(email: user) || user
end
def retrieve_access_level(access_level)
access_levels.fetch(access_level) { access_level.to_i }
end
def can_update_member?(current_user, member) def can_update_member?(current_user, member)
# There is no current user for bulk actions, in which case anything is allowed # There is no current user for bulk actions, in which case anything is allowed
!current_user || !current_user || current_user.can?(:"update_#{member.type.underscore}", member)
current_user.can?(:update_group_member, member) ||
current_user.can?(:update_project_member, member)
end end
def project_creator?(member, access_level) def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil)
member.new_record? && member.owner? && users.each do |user|
access_level.to_i == ProjectMember::MASTER add_user(
source,
user,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
end end
end end
......
...@@ -12,6 +12,22 @@ class GroupMember < Member ...@@ -12,6 +12,22 @@ class GroupMember < Member
Gitlab::Access.options_with_owner Gitlab::Access.options_with_owner
end end
def self.access_levels
Gitlab::Access.sym_options_with_owner
end
def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil)
self.transaction do
add_users_to_source(
group,
users,
access_level,
current_user: current_user,
expires_at: expires_at
)
end
end
def group def group
source source
end end
......
...@@ -34,36 +34,20 @@ class ProjectMember < Member ...@@ -34,36 +34,20 @@ class ProjectMember < Member
# :master # :master
# ) # )
# #
def add_users_to_projects(project_ids, user_ids, access, current_user: nil, expires_at: nil) def add_users_to_projects(project_ids, users, access_level, current_user: nil, expires_at: nil)
access_level = if roles_hash.has_key?(access) self.transaction do
roles_hash[access]
elsif roles_hash.values.include?(access.to_i)
access
else
raise "Non valid access"
end
users = user_ids.map { |user_id| Member.user_for_id(user_id) }
ProjectMember.transaction do
project_ids.each do |project_id| project_ids.each do |project_id|
project = Project.find(project_id) project = Project.find(project_id)
users.each do |user| add_users_to_source(
Member.add_user( project,
project.project_members, users,
user, access_level,
access_level, current_user: current_user,
current_user: current_user, expires_at: expires_at
expires_at: expires_at )
)
end
end end
end end
true
rescue
false
end end
def truncate_teams(project_ids) def truncate_teams(project_ids)
...@@ -84,13 +68,15 @@ class ProjectMember < Member ...@@ -84,13 +68,15 @@ class ProjectMember < Member
truncate_teams [project.id] truncate_teams [project.id]
end end
def roles_hash
Gitlab::Access.sym_options
end
def access_level_roles def access_level_roles
Gitlab::Access.options Gitlab::Access.options
end end
private
def can_update_member?(current_user, member)
super || (member.owner? && member.new_record?)
end
end end
def access_field def access_field
......
...@@ -146,6 +146,7 @@ class Project < ActiveRecord::Base ...@@ -146,6 +146,7 @@ class Project < ActiveRecord::Base
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
delegate :members, to: :team, prefix: true delegate :members, to: :team, prefix: true
delegate :add_user, to: :team
# Validations # Validations
validates :creator, presence: true, on: :create validates :creator, presence: true, on: :create
...@@ -1016,10 +1017,6 @@ class Project < ActiveRecord::Base ...@@ -1016,10 +1017,6 @@ class Project < ActiveRecord::Base
project_members.find_by(user_id: user) project_members.find_by(user_id: user)
end end
def add_user(user, access_level, current_user: nil, expires_at: nil)
team.add_user(user, access_level, current_user: current_user, expires_at: expires_at)
end
def default_branch def default_branch
@default_branch ||= repository.root_ref if repository.exists? @default_branch ||= repository.root_ref if repository.exists?
end end
......
...@@ -33,18 +33,24 @@ class ProjectTeam ...@@ -33,18 +33,24 @@ class ProjectTeam
member member
end end
def add_users(users, access, current_user: nil, expires_at: nil) def add_users(users, access_level, current_user: nil, expires_at: nil)
ProjectMember.add_users_to_projects( ProjectMember.add_users_to_projects(
[project.id], [project.id],
users, users,
access, access_level,
current_user: current_user, current_user: current_user,
expires_at: expires_at expires_at: expires_at
) )
end end
def add_user(user, access, current_user: nil, expires_at: nil) def add_user(user, access_level, current_user: nil, expires_at: nil)
add_users([user], access, current_user: current_user, expires_at: expires_at) ProjectMember.add_user(
project,
user,
access_level,
current_user: current_user,
expires_at: expires_at
)
end end
# Remove all users from project team # Remove all users from project team
......
Gitlab::Seeder.quiet do Gitlab::Seeder.quiet do
Group.all.each do |group| Group.all.each do |group|
User.all.sample(4).each do |user| User.all.sample(4).each do |user|
if group.add_users([user.id], Gitlab::Access.values.sample) if group.add_user(user, Gitlab::Access.values.sample).persisted?
print '.' print '.'
else else
print 'F' print 'F'
......
...@@ -59,13 +59,6 @@ module API ...@@ -59,13 +59,6 @@ module API
authorize_admin_source!(source_type, source) authorize_admin_source!(source_type, source)
required_attributes! [:user_id, :access_level] required_attributes! [:user_id, :access_level]
access_requester = source.requesters.find_by(user_id: params[:user_id])
if access_requester
# We pass current_user = access_requester so that the requester doesn't
# receive a "access denied" email
::Members::DestroyService.new(access_requester, access_requester.user).execute
end
member = source.members.find_by(user_id: params[:user_id]) member = source.members.find_by(user_id: params[:user_id])
# This is to ensure back-compatibility but 409 behavior should be used # This is to ensure back-compatibility but 409 behavior should be used
...@@ -73,18 +66,12 @@ module API ...@@ -73,18 +66,12 @@ module API
conflict!('Member already exists') if source_type == 'group' && member conflict!('Member already exists') if source_type == 'group' && member
unless member unless member
source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at])
member = source.members.find_by(user_id: params[:user_id])
end end
if member if member.persisted? && member.valid?
present member.user, with: Entities::Member, member: member present member.user, with: Entities::Member, member: member
else else
# Since `source.add_user` doesn't return a member object, we have to
# build a new one and populate its errors in order to render them.
member = source.members.build(attributes_for_keys([:user_id, :access_level, :expires_at]))
member.valid? # populate the errors
# This is to ensure back-compatibility but 400 behavior should be used # This is to ensure back-compatibility but 400 behavior should be used
# for all validation errors in 9.0! # for all validation errors in 9.0!
render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level) render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level)
......
...@@ -53,6 +53,10 @@ module Gitlab ...@@ -53,6 +53,10 @@ module Gitlab
} }
end end
def sym_options_with_owner
sym_options.merge(owner: OWNER)
end
def protection_options def protection_options
{ {
"Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE, "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE,
......
...@@ -13,7 +13,7 @@ describe Projects::TemplatesController do ...@@ -13,7 +13,7 @@ describe Projects::TemplatesController do
end end
before do before do
project.team.add_user(user, Gitlab::Access::MASTER) project.add_user(user, Gitlab::Access::MASTER)
project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
end end
......
...@@ -4,24 +4,9 @@ FactoryGirl.define do ...@@ -4,24 +4,9 @@ FactoryGirl.define do
project project
master master
trait :guest do trait(:guest) { access_level ProjectMember::GUEST }
access_level ProjectMember::GUEST trait(:reporter) { access_level ProjectMember::REPORTER }
end trait(:developer) { access_level ProjectMember::DEVELOPER }
trait(:master) { access_level ProjectMember::MASTER }
trait :reporter do
access_level ProjectMember::REPORTER
end
trait :developer do
access_level ProjectMember::DEVELOPER
end
trait :master do
access_level ProjectMember::MASTER
end
trait :owner do
access_level ProjectMember::OWNER
end
end end
end end
require 'spec_helper' require 'spec_helper'
feature 'Projects > Members > Owner cannot leave project', feature: true do feature 'Projects > Members > Owner cannot leave project', feature: true do
let(:owner) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
background do background do
project.team << [owner, :owner] login_as(project.owner)
login_as(owner)
visit namespace_project_path(project.namespace, project) visit namespace_project_path(project.namespace, project)
end end
......
require 'spec_helper' require 'spec_helper'
feature 'Projects > Members > Owner cannot request access to his project', feature: true do feature 'Projects > Members > Owner cannot request access to his project', feature: true do
let(:owner) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project) }
background do background do
project.team << [owner, :owner] login_as(project.owner)
login_as(owner)
visit namespace_project_path(project.namespace, project) visit namespace_project_path(project.namespace, project)
end end
......
...@@ -82,7 +82,7 @@ feature 'Project', feature: true do ...@@ -82,7 +82,7 @@ feature 'Project', feature: true do
before do before do
login_with(user) login_with(user)
project.team.add_user(user, Gitlab::Access::MASTER) project.add_user(user, Gitlab::Access::MASTER)
visit namespace_project_path(project.namespace, project) visit namespace_project_path(project.namespace, project)
end end
...@@ -101,8 +101,8 @@ feature 'Project', feature: true do ...@@ -101,8 +101,8 @@ feature 'Project', feature: true do
context 'on issues page', js: true do context 'on issues page', js: true do
before do before do
login_with(user) login_with(user)
project.team.add_user(user, Gitlab::Access::MASTER) project.add_user(user, Gitlab::Access::MASTER)
project2.team.add_user(user, Gitlab::Access::MASTER) project2.add_user(user, Gitlab::Access::MASTER)
visit namespace_project_issue_path(project.namespace, project, issue) visit namespace_project_issue_path(project.namespace, project, issue)
end end
......
...@@ -43,7 +43,7 @@ describe JoinedGroupsFinder do ...@@ -43,7 +43,7 @@ describe JoinedGroupsFinder do
context 'if profile visitor is in one of the private group projects' do context 'if profile visitor is in one of the private group projects' do
before do before do
project = create(:project, :private, group: private_group, name: 'B', path: 'B') project = create(:project, :private, group: private_group, name: 'B', path: 'B')
project.team.add_user(profile_visitor, Gitlab::Access::DEVELOPER) project.add_user(profile_visitor, Gitlab::Access::DEVELOPER)
end end
it 'shows group' do it 'shows group' do
......
...@@ -38,7 +38,7 @@ describe ProjectsFinder do ...@@ -38,7 +38,7 @@ describe ProjectsFinder do
describe 'with private projects' do describe 'with private projects' do
before do before do
private_project.team.add_user(user, Gitlab::Access::MASTER) private_project.add_user(user, Gitlab::Access::MASTER)
end end
it do it do
......
...@@ -10,7 +10,7 @@ describe Gitlab::Template::IssueTemplate do ...@@ -10,7 +10,7 @@ describe Gitlab::Template::IssueTemplate do
let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' } let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' }
before do before do
project.team.add_user(user, Gitlab::Access::MASTER) project.add_user(user, Gitlab::Access::MASTER)
project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
...@@ -53,7 +53,7 @@ describe Gitlab::Template::IssueTemplate do ...@@ -53,7 +53,7 @@ describe Gitlab::Template::IssueTemplate do
context 'when repo is bare or empty' do context 'when repo is bare or empty' do
let(:empty_project) { create(:empty_project) } let(:empty_project) { create(:empty_project) }
before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } before { empty_project.add_user(user, Gitlab::Access::MASTER) }
it "returns empty array" do it "returns empty array" do
templates = subject.by_category('', empty_project) templates = subject.by_category('', empty_project)
...@@ -78,7 +78,7 @@ describe Gitlab::Template::IssueTemplate do ...@@ -78,7 +78,7 @@ describe Gitlab::Template::IssueTemplate do
context "when repo is empty" do context "when repo is empty" do
let(:empty_project) { create(:empty_project) } let(:empty_project) { create(:empty_project) }
before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } before { empty_project.add_user(user, Gitlab::Access::MASTER) }
it "raises file not found" do it "raises file not found" do
issue_template = subject.new('.gitlab/issue_templates/not_existent.md', empty_project) issue_template = subject.new('.gitlab/issue_templates/not_existent.md', empty_project)
......
...@@ -10,7 +10,7 @@ describe Gitlab::Template::MergeRequestTemplate do ...@@ -10,7 +10,7 @@ describe Gitlab::Template::MergeRequestTemplate do
let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' } let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' }
before do before do
project.team.add_user(user, Gitlab::Access::MASTER) project.add_user(user, Gitlab::Access::MASTER)
project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
...@@ -53,7 +53,7 @@ describe Gitlab::Template::MergeRequestTemplate do ...@@ -53,7 +53,7 @@ describe Gitlab::Template::MergeRequestTemplate do
context 'when repo is bare or empty' do context 'when repo is bare or empty' do
let(:empty_project) { create(:empty_project) } let(:empty_project) { create(:empty_project) }
before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } before { empty_project.add_user(user, Gitlab::Access::MASTER) }
it "returns empty array" do it "returns empty array" do
templates = subject.by_category('', empty_project) templates = subject.by_category('', empty_project)
...@@ -78,7 +78,7 @@ describe Gitlab::Template::MergeRequestTemplate do ...@@ -78,7 +78,7 @@ describe Gitlab::Template::MergeRequestTemplate do
context "when repo is empty" do context "when repo is empty" do
let(:empty_project) { create(:empty_project) } let(:empty_project) { create(:empty_project) }
before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } before { empty_project.add_user(user, Gitlab::Access::MASTER) }
it "raises file not found" do it "raises file not found" do
issue_template = subject.new('.gitlab/merge_request_templates/not_existent.md', empty_project) issue_template = subject.new('.gitlab/merge_request_templates/not_existent.md', empty_project)
......
...@@ -492,21 +492,22 @@ describe Notify do ...@@ -492,21 +492,22 @@ describe Notify do
end end
end end
def invite_to_project(project:, email:, inviter:) def invite_to_project(project, inviter:)
Member.add_user( create(
project.project_members, :project_member,
'toto@example.com', :developer,
Gitlab::Access::DEVELOPER, project: project,
current_user: inviter invite_token: '1234',
invite_email: 'toto@example.com',
user: nil,
created_by: inviter
) )
project.project_members.invite.last
end end
describe 'project invitation' do describe 'project invitation' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
let(:project_member) { invite_to_project(project: project, email: 'toto@example.com', inviter: master) } let(:project_member) { invite_to_project(project, inviter: master) }
subject { Notify.member_invited_email('project', project_member.id, project_member.invite_token) } subject { Notify.member_invited_email('project', project_member.id, project_member.invite_token) }
...@@ -525,10 +526,10 @@ describe Notify do ...@@ -525,10 +526,10 @@ describe Notify do
describe 'project invitation accepted' do describe 'project invitation accepted' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:invited_user) { create(:user) } let(:invited_user) { create(:user, name: 'invited user') }
let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
let(:project_member) do let(:project_member) do
invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master) invitee = invite_to_project(project, inviter: master)
invitee.accept_invite!(invited_user) invitee.accept_invite!(invited_user)
invitee invitee
end end
...@@ -552,7 +553,7 @@ describe Notify do ...@@ -552,7 +553,7 @@ describe Notify do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
let(:project_member) do let(:project_member) do
invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master) invitee = invite_to_project(project, inviter: master)
invitee.decline_invite! invitee.decline_invite!
invitee invitee
end end
...@@ -744,21 +745,22 @@ describe Notify do ...@@ -744,21 +745,22 @@ describe Notify do
end end
end end
def invite_to_group(group:, email:, inviter:) def invite_to_group(group, inviter:)
Member.add_user( create(
group.group_members, :group_member,
'toto@example.com', :developer,
Gitlab::Access::DEVELOPER, group: group,
current_user: inviter invite_token: '1234',
invite_email: 'toto@example.com',
user: nil,
created_by: inviter
) )
group.group_members.invite.last
end end
describe 'group invitation' do describe 'group invitation' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } }
let(:group_member) { invite_to_group(group: group, email: 'toto@example.com', inviter: owner) } let(:group_member) { invite_to_group(group, inviter: owner) }
subject { Notify.member_invited_email('group', group_member.id, group_member.invite_token) } subject { Notify.member_invited_email('group', group_member.id, group_member.invite_token) }
...@@ -777,10 +779,10 @@ describe Notify do ...@@ -777,10 +779,10 @@ describe Notify do
describe 'group invitation accepted' do describe 'group invitation accepted' do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:invited_user) { create(:user) } let(:invited_user) { create(:user, name: 'invited user') }
let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } }
let(:group_member) do let(:group_member) do
invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner) invitee = invite_to_group(group, inviter: owner)
invitee.accept_invite!(invited_user) invitee.accept_invite!(invited_user)
invitee invitee
end end
...@@ -804,7 +806,7 @@ describe Notify do ...@@ -804,7 +806,7 @@ describe Notify do
let(:group) { create(:group) } let(:group) { create(:group) }
let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } }
let(:group_member) do let(:group_member) do
invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner) invitee = invite_to_group(group, inviter: owner)
invitee.decline_invite! invitee.decline_invite!
invitee invitee
end end
......
...@@ -494,7 +494,7 @@ describe Issue, models: true do ...@@ -494,7 +494,7 @@ describe Issue, models: true do
context 'with an admin user' do context 'with an admin user' do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:user) { create(:user, admin: true) } let(:user) { create(:admin) }
it 'returns true for a regular issue' do it 'returns true for a regular issue' do
issue = build(:issue, project: project) issue = build(:issue, project: project)
......
...@@ -74,22 +74,17 @@ describe Member, models: true do ...@@ -74,22 +74,17 @@ describe Member, models: true do
@blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER) @blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER)
@blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER) @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER)
Member.add_user( @invited_member = create(:project_member, :developer,
project.members, project: project,
'toto1@example.com', invite_token: '1234',
Gitlab::Access::DEVELOPER, invite_email: 'toto1@example.com')
current_user: @master_user
)
@invited_member = project.members.invite.find_by_invite_email('toto1@example.com')
accepted_invite_user = build(:user, state: :active) accepted_invite_user = build(:user, state: :active)
Member.add_user( @accepted_invite_member = create(:project_member, :developer,
project.members, project: project,
'toto2@example.com', invite_token: '1234',
Gitlab::Access::DEVELOPER, invite_email: 'toto2@example.com').
current_user: @master_user tap { |u| u.accept_invite!(accepted_invite_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_user = create(:user).tap { |u| project.request_access(u) }
@requested_member = project.requesters.find_by(user_id: requested_user.id) @requested_member = project.requesters.find_by(user_id: requested_user.id)
...@@ -176,39 +171,209 @@ describe Member, models: true do ...@@ -176,39 +171,209 @@ describe Member, models: true do
it { is_expected.to respond_to(:user_email) } it { is_expected.to respond_to(:user_email) }
end end
describe ".add_user" do describe '.add_user' do
let!(:user) { create(:user) } %w[project group].each do |source_type|
let(:project) { create(:project) } context "when source is a #{source_type}" do
let!(:source) { create(source_type) }
let!(:user) { create(:user) }
let!(:admin) { create(:admin) }
context "when called with a user id" do it 'returns a <Source>Member object' do
it "adds the user as a member" do member = described_class.add_user(source, user, :master)
Member.add_user(project.project_members, user.id, ProjectMember::MASTER)
expect(project.users).to include(user) expect(member).to be_a "#{source_type.classify}Member".constantize
end expect(member).to be_persisted
end end
context "when called with a user object" do it 'sets members.created_by to the given current_user' do
it "adds the user as a member" do member = described_class.add_user(source, user, :master, current_user: admin)
Member.add_user(project.project_members, user, ProjectMember::MASTER)
expect(project.users).to include(user) expect(member.created_by).to eq(admin)
end end
end
context "when called with a known user email" do it 'sets members.expires_at to the given expires_at' do
it "adds the user as a member" do member = described_class.add_user(source, user, :master, expires_at: Date.new(2016, 9, 22))
Member.add_user(project.project_members, user.email, ProjectMember::MASTER)
expect(project.users).to include(user) expect(member.expires_at).to eq(Date.new(2016, 9, 22))
end end
end
described_class.access_levels.each do |sym_key, int_access_level|
it "accepts the :#{sym_key} symbol as access level" do
expect(source.users).not_to include(user)
member = described_class.add_user(source, user.id, sym_key)
expect(member.access_level).to eq(int_access_level)
expect(source.users.reload).to include(user)
end
it "accepts the #{int_access_level} integer as access level" do
expect(source.users).not_to include(user)
member = described_class.add_user(source, user.id, int_access_level)
expect(member.access_level).to eq(int_access_level)
expect(source.users.reload).to include(user)
end
end
context 'with no current_user' do
context 'when called with a known user id' do
it 'adds the user as a member' do
expect(source.users).not_to include(user)
described_class.add_user(source, user.id, :master)
expect(source.users.reload).to include(user)
end
end
context 'when called with an unknown user id' do
it 'adds the user as a member' do
expect(source.users).not_to include(user)
described_class.add_user(source, 42, :master)
expect(source.users.reload).not_to include(user)
end
end
context 'when called with a user object' do
it 'adds the user as a member' do
expect(source.users).not_to include(user)
described_class.add_user(source, user, :master)
expect(source.users.reload).to include(user)
end
end
context 'when called with a requester user object' do
before do
source.request_access(user)
end
it 'adds the requester as a member' do
expect(source.users).not_to include(user)
expect(source.requesters.exists?(user_id: user)).to be_truthy
expect { described_class.add_user(source, user, :master) }.
to raise_error(Gitlab::Access::AccessDeniedError)
expect(source.users.reload).not_to include(user)
expect(source.requesters.reload.exists?(user_id: user)).to be_truthy
end
end
context 'when called with a known user email' do
it 'adds the user as a member' do
expect(source.users).not_to include(user)
described_class.add_user(source, user.email, :master)
expect(source.users.reload).to include(user)
end
end
context 'when called with an unknown user email' do
it 'creates an invited member' do
expect(source.users).not_to include(user)
described_class.add_user(source, 'user@example.com', :master)
expect(source.members.invite.pluck(:invite_email)).to include('user@example.com')
end
end
end
context 'when current_user can update member' do
it 'creates the member' do
expect(source.users).not_to include(user)
described_class.add_user(source, user, :master, current_user: admin)
expect(source.users.reload).to include(user)
end
context 'when called with a requester user object' do
before do
source.request_access(user)
end
it 'adds the requester as a member' do
expect(source.users).not_to include(user)
expect(source.requesters.exists?(user_id: user)).to be_truthy
described_class.add_user(source, user, :master, current_user: admin)
expect(source.users.reload).to include(user)
expect(source.requesters.reload.exists?(user_id: user)).to be_falsy
end
end
end
context 'when current_user cannot update member' do
it 'does not create the member' do
expect(source.users).not_to include(user)
member = described_class.add_user(source, user, :master, current_user: user)
expect(source.users.reload).not_to include(user)
expect(member).not_to be_persisted
end
context 'when called with a requester user object' do
before do
source.request_access(user)
end
it 'does not destroy the requester' do
expect(source.users).not_to include(user)
expect(source.requesters.exists?(user_id: user)).to be_truthy
described_class.add_user(source, user, :master, current_user: user)
expect(source.users.reload).not_to include(user)
expect(source.requesters.exists?(user_id: user)).to be_truthy
end
end
end
context 'when member already exists' do
before do
source.add_user(user, :developer)
end
context 'with no current_user' do
it 'updates the member' do
expect(source.users).to include(user)
described_class.add_user(source, user, :master)
expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER)
end
end
context 'when current_user can update member' do
it 'updates the member' do
expect(source.users).to include(user)
described_class.add_user(source, user, :master, current_user: admin)
expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER)
end
end
context 'when current_user cannot update member' do
it 'does not update the member' do
expect(source.users).to include(user)
context "when called with an unknown user email" do described_class.add_user(source, user, :master, current_user: user)
it "adds a member invite" do
Member.add_user(project.project_members, "user@example.com", ProjectMember::MASTER)
expect(project.project_members.invite.pluck(:invite_email)).to include("user@example.com") expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::DEVELOPER)
end
end
end
end end
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe GroupMember, models: true do describe GroupMember, models: true do
describe '.access_level_roles' do
it 'returns Gitlab::Access.options_with_owner' do
expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner)
end
end
describe '.access_levels' do
it 'returns Gitlab::Access.options_with_owner' do
expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner)
end
end
describe '.add_users_to_group' do
it 'adds the given users to the given group' do
group = create(:group)
users = create_list(:user, 2)
described_class.add_users_to_group(
group,
[users.first.id, users.second],
described_class::MASTER
)
expect(group.users).to include(users.first, users.second)
end
end
describe 'notifications' do describe 'notifications' do
describe "#after_create" do describe "#after_create" do
it "sends email to user" do it "sends email to user" do
......
...@@ -15,6 +15,26 @@ describe ProjectMember, models: true do ...@@ -15,6 +15,26 @@ describe ProjectMember, models: true do
it { is_expected.to include_module(Gitlab::ShellAdapter) } it { is_expected.to include_module(Gitlab::ShellAdapter) }
end end
describe '.access_level_roles' do
it 'returns Gitlab::Access.options' do
expect(described_class.access_level_roles).to eq(Gitlab::Access.options)
end
end
describe '.add_user' do
context 'when called with the project owner' do
it 'adds the user as a member' do
project = create(:empty_project)
expect(project.users).not_to include(project.owner)
described_class.add_user(project, project.owner, :master, current_user: project.owner)
expect(project.users.reload).to include(project.owner)
end
end
end
describe '#real_source_type' do describe '#real_source_type' do
subject { create(:project_member).real_source_type } subject { create(:project_member).real_source_type }
...@@ -50,7 +70,7 @@ describe ProjectMember, models: true do ...@@ -50,7 +70,7 @@ describe ProjectMember, models: true do
end end
end end
describe :import_team do describe '.import_team' do
before do before do
@project_1 = create :project @project_1 = create :project
@project_2 = create :project @project_2 = create :project
...@@ -81,25 +101,21 @@ describe ProjectMember, models: true do ...@@ -81,25 +101,21 @@ describe ProjectMember, models: true do
end end
describe '.add_users_to_projects' do describe '.add_users_to_projects' do
before do it 'adds the given users to the given projects' do
@project_1 = create :project projects = create_list(:empty_project, 2)
@project_2 = create :project users = create_list(:user, 2)
@user_1 = create :user described_class.add_users_to_projects(
@user_2 = create :user [projects.first.id, projects.second],
[users.first.id, users.second],
ProjectMember.add_users_to_projects( described_class::MASTER)
[@project_1.id, @project_2.id],
[@user_1.id, @user_2.id],
ProjectMember::MASTER
)
end
it { expect(@project_1.users).to include(@user_1) } expect(projects.first.users).to include(users.first)
it { expect(@project_1.users).to include(@user_2) } expect(projects.first.users).to include(users.second)
it { expect(@project_2.users).to include(@user_1) } expect(projects.second.users).to include(users.first)
it { expect(@project_2.users).to include(@user_2) } expect(projects.second.users).to include(users.second)
end
end end
describe '.truncate_teams' do describe '.truncate_teams' do
......
...@@ -836,7 +836,7 @@ describe Project, models: true do ...@@ -836,7 +836,7 @@ describe Project, models: true do
describe 'when a user has access to a project' do describe 'when a user has access to a project' do
before do before do
project.team.add_user(user, Gitlab::Access::MASTER) project.add_user(user, Gitlab::Access::MASTER)
end end
it { is_expected.to eq([project]) } it { is_expected.to eq([project]) }
......
...@@ -15,7 +15,7 @@ describe API::API, api: true do ...@@ -15,7 +15,7 @@ describe API::API, api: true do
let(:milestone) { create(:milestone, title: '1.0.0', project: project) } let(:milestone) { create(:milestone, title: '1.0.0', project: project) }
before do before do
project.team << [user, :reporters] project.team << [user, :reporter]
end end
describe "GET /projects/:id/merge_requests" do describe "GET /projects/:id/merge_requests" do
...@@ -299,7 +299,7 @@ describe API::API, api: true do ...@@ -299,7 +299,7 @@ describe API::API, api: true do
let!(:unrelated_project) { create(:project, namespace: create(:user).namespace, creator_id: user2.id) } let!(:unrelated_project) { create(:project, namespace: create(:user).namespace, creator_id: user2.id) }
before :each do |each| before :each do |each|
fork_project.team << [user2, :reporters] fork_project.team << [user2, :reporter]
end end
it "returns merge_request" do it "returns merge_request" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment