diff options
| -rw-r--r-- | app/models/group.rb | 1 | ||||
| -rw-r--r-- | app/models/member.rb | 62 | ||||
| -rw-r--r-- | app/models/project.rb | 1 | ||||
| -rw-r--r-- | spec/lib/gitlab/import_export/all_models.yml | 1 | ||||
| -rw-r--r-- | spec/models/group_spec.rb | 19 | ||||
| -rw-r--r-- | spec/models/member_spec.rb | 9 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 19 | ||||
| -rw-r--r-- | spec/support/group_members_shared_example.rb | 27 |
8 files changed, 90 insertions, 49 deletions
diff --git a/app/models/group.rb b/app/models/group.rb index 190b27cf66b..e746e4a12c9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -16,6 +16,7 @@ class Group < Namespace source: :user has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent + has_many :members_and_requesters, as: :source, class_name: 'GroupMember' has_many :milestones has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/member.rb b/app/models/member.rb index ee2cb13697b..cbbd58f2eaf 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -126,20 +126,11 @@ class Member < ActiveRecord::Base find_by(invite_token: invite_token) end - def add_user(source, user, access_level, current_user: nil, expires_at: nil) - user = retrieve_user(user) + def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil) + # `user` can be either a User object, User ID or an email to be invited + member = retrieve_member(source, user, existing_members) access_level = retrieve_access_level(access_level) - # `user` can be either a User object or an email to be invited - member = - 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 = { @@ -165,17 +156,15 @@ class Member < ActiveRecord::Base def add_users(source, users, access_level, current_user: nil, expires_at: nil) return [] unless users.present? - # Collect all user ids into separate array - # so we can use single sql query to get user objects - user_ids = users.select { |user| user =~ /\A\d+\Z/ } - users = users - user_ids + User.where(id: user_ids) + emails, users, existing_members = parse_users_list(source, users) self.transaction do - users.map do |user| + (emails + users).map! do |user| add_user( source, user, access_level, + existing_members: existing_members, current_user: current_user, expires_at: expires_at ) @@ -189,6 +178,31 @@ class Member < ActiveRecord::Base private + def parse_users_list(source, list) + emails, user_ids, users = [], [], [] + existing_members = {} + + list.each do |item| + case item + when User + users << item + when Integer + user_ids << item + when /\A\d+\Z/ + user_ids << item.to_i + when Devise.email_regexp + emails << item + end + end + + if user_ids.present? + users.concat(User.where(id: user_ids)) + existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id) + end + + [emails, users, existing_members] + end + # 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) @@ -197,6 +211,20 @@ class Member < ActiveRecord::Base User.find_by(id: user) || User.find_by(email: user) || user end + def retrieve_member(source, user, existing_members) + user = retrieve_user(user) + + if user.is_a?(User) + if existing_members + existing_members[user.id] || source.members.build(user_id: user.id) + else + source.members_and_requesters.find_or_initialize_by(user_id: user.id) + end + else + source.members.build(invite_email: user) + end + end + def retrieve_access_level(access_level) access_levels.fetch(access_level) { access_level.to_i } end diff --git a/app/models/project.rb b/app/models/project.rb index 051c4c8e2ec..3d89dabd96f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -144,6 +144,7 @@ class Project < ActiveRecord::Base has_many :requesters, -> { where.not(requested_at: nil) }, as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent + has_many :members_and_requesters, as: :source, class_name: 'ProjectMember' has_many :deploy_keys_projects has_many :deploy_keys, through: :deploy_keys_projects diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 8da02b0cf00..beed4e77e8b 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -264,6 +264,7 @@ project: - statistics - container_repositories - uploads +- members_and_requesters award_emoji: - awardable - user diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index f9cd12c0ff3..f36d6eeb327 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -9,6 +9,7 @@ describe Group do it { is_expected.to have_many(:users).through(:group_members) } it { is_expected.to have_many(:owners).through(:group_members) } it { is_expected.to have_many(:requesters).dependent(:destroy) } + it { is_expected.to have_many(:members_and_requesters) } 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) } @@ -25,22 +26,8 @@ describe Group do group.add_developer(developer) end - describe '#members' do - it 'includes members and exclude requesters' do - member_user_ids = group.members.pluck(:user_id) - - expect(member_user_ids).to include(developer.id) - expect(member_user_ids).not_to include(requester.id) - end - end - - describe '#requesters' do - it 'does not include requesters' do - requester_user_ids = group.requesters.pluck(:user_id) - - expect(requester_user_ids).to include(requester.id) - expect(requester_user_ids).not_to include(developer.id) - end + it_behaves_like 'members and requesters associations' do + let(:namespace) { group } end end end diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 87513e18b25..a07ce05a865 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -409,6 +409,15 @@ describe Member do expect(members).to be_a Array expect(members).to be_empty end + + it 'supports differents formats' do + list = ['joe@local.test', admin, user1.id, user2.id.to_s] + + members = described_class.add_users(source, list, :master) + + expect(members.size).to eq(4) + expect(members.first).to be_invite + end end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index be1ae295f75..1f7c6a82b91 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -74,6 +74,7 @@ describe Project do it { is_expected.to have_many(:forks).through(:forked_project_links) } it { is_expected.to have_many(:uploads).dependent(:destroy) } it { is_expected.to have_many(:pipeline_schedules) } + it { is_expected.to have_many(:members_and_requesters) } context 'after initialized' do it "has a project_feature" do @@ -90,22 +91,8 @@ describe Project do project.team << [developer, :developer] end - describe '#members' do - it 'includes members and exclude requesters' do - member_user_ids = project.members.pluck(:user_id) - - expect(member_user_ids).to include(developer.id) - expect(member_user_ids).not_to include(requester.id) - end - end - - describe '#requesters' do - it 'does not include requesters' do - requester_user_ids = project.requesters.pluck(:user_id) - - expect(requester_user_ids).to include(requester.id) - expect(requester_user_ids).not_to include(developer.id) - end + it_behaves_like 'members and requesters associations' do + let(:namespace) { project } end end diff --git a/spec/support/group_members_shared_example.rb b/spec/support/group_members_shared_example.rb new file mode 100644 index 00000000000..547c83c7955 --- /dev/null +++ b/spec/support/group_members_shared_example.rb @@ -0,0 +1,27 @@ +RSpec.shared_examples 'members and requesters associations' do + describe '#members_and_requesters' do + it 'includes members and requesters' do + member_and_requester_user_ids = namespace.members_and_requesters.pluck(:user_id) + + expect(member_and_requester_user_ids).to include(requester.id, developer.id) + end + end + + describe '#members' do + it 'includes members and exclude requesters' do + member_user_ids = namespace.members.pluck(:user_id) + + expect(member_user_ids).to include(developer.id) + expect(member_user_ids).not_to include(requester.id) + end + end + + describe '#requesters' do + it 'does not include requesters' do + requester_user_ids = namespace.requesters.pluck(:user_id) + + expect(requester_user_ids).to include(requester.id) + expect(requester_user_ids).not_to include(developer.id) + end + end +end |