From b674ab529ac26169249a91261a070da733cec84e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 20 Sep 2016 15:18:11 +0300 Subject: Add specs for ProjectTeam#fetch_members --- spec/factories/group_members.rb | 6 ++++ spec/models/project_team_spec.rb | 61 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/spec/factories/group_members.rb b/spec/factories/group_members.rb index 2044ebec09a..795df5dfda9 100644 --- a/spec/factories/group_members.rb +++ b/spec/factories/group_members.rb @@ -3,5 +3,11 @@ FactoryGirl.define do access_level { GroupMember::OWNER } group user + + trait(:guest) { access_level GroupMember::GUEST } + trait(:reporter) { access_level GroupMember::REPORTER } + trait(:developer) { access_level GroupMember::DEVELOPER } + trait(:master) { access_level GroupMember::MASTER } + trait(:owner) { access_level GroupMember::OWNER } end end diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 5eaf0d3b7a6..469e306044b 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -73,6 +73,67 @@ describe ProjectTeam, models: true do end end + describe '#fetch_members' do + context 'personal project' do + let(:project) { create(:empty_project) } + + it 'returns project members' do + user = create(:user) + project.team << [user, :guest] + + expect(project.team.members).to contain_exactly(user) + end + + it 'returns project members of a specified level' do + user = create(:user) + project.team << [user, :reporter] + + expect(project.team.guests).to be_empty + expect(project.team.reporters).to contain_exactly(user) + end + + it 'returns invited members of a group' do + group_member = create(:group_member) + + project.project_group_links.create!( + group: group_member.group, + group_access: Gitlab::Access::GUEST + ) + + expect(project.team.members).to contain_exactly(group_member.user) + end + + it 'returns invited members of a group of a specified level' do + group_member = create(:group_member) + + project.project_group_links.create!( + group: group_member.group, + group_access: Gitlab::Access::REPORTER + ) + + expect(project.team.reporters).to contain_exactly(group_member.user) + end + end + + context 'group project' do + let(:group) { create(:group) } + let(:project) { create(:empty_project, group: group) } + + it 'returns project members' do + group_member = create(:group_member, group: group) + + expect(project.team.members).to contain_exactly(group_member.user) + end + + it 'returns project members of a specified level' do + group_member = create(:group_member, :reporter, group: group) + + expect(project.team.guests).to be_empty + expect(project.team.reporters).to contain_exactly(group_member.user) + end + end + end + describe '#find_member' do context 'personal project' do let(:project) { create(:empty_project) } -- cgit v1.2.1 From b7b41d1f68bf30f9d115bc6687f6762a78a546af Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 20 Sep 2016 15:19:20 +0300 Subject: Simplify ProjectTeam#fetch_members to satisfy flog --- app/models/project_team.rb | 70 ++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index ab6ea2aae36..e8606c8c2c6 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -163,7 +163,7 @@ class ProjectTeam # Each group produces a list of maximum access level per user. We take the # max of the values produced by each group. - if project.invited_groups.any? && project.allowed_to_share_with_group? + if project_shared_with_group? project.project_group_links.each do |group_link| invited_access = max_invited_level_for_users(group_link, user_ids) merge_max!(access, invited_access) @@ -200,35 +200,6 @@ class ProjectTeam def fetch_members(level = nil) project_members = project.members group_members = group ? group.members : [] - invited_members = [] - - if project.invited_groups.any? && project.allowed_to_share_with_group? - project.project_group_links.includes(group: [:group_members]).each do |group_link| - invited_group = group_link.group - im = invited_group.members - - if level - int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] - - # Skip group members if we ask for masters - # but max group access is developers - next if int_level > group_link.group_access - - # If we ask for developers and max - # group access is developers we need to provide - # both group master, developers as devs - if int_level == group_link.group_access - im.where("access_level >= ?)", group_link.group_access) - else - im.send(level) - end - end - - invited_members << im - end - - invited_members = invited_members.flatten.compact - end if level project_members = project_members.send(level) @@ -236,7 +207,10 @@ class ProjectTeam end user_ids = project_members.pluck(:user_id) + + invited_members = fetch_invited_members(level) user_ids.push(*invited_members.map(&:user_id)) if invited_members.any? + user_ids.push(*group_members.pluck(:user_id)) if group User.where(id: user_ids) @@ -249,4 +223,40 @@ class ProjectTeam def merge_max!(first_hash, second_hash) first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new } end + + def project_shared_with_group? + project.invited_groups.any? && project.allowed_to_share_with_group? + end + + def fetch_invited_members(level = nil) + invited_members = [] + + return invited_members unless project_shared_with_group? + + project.project_group_links.includes(group: [:group_members]).each do |group_link| + invited_group = group_link.group + im = invited_group.members + + if level + int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] + + # Skip group members if we ask for masters + # but max group access is developers + next if int_level > group_link.group_access + + # If we ask for developers and max + # group access is developers we need to provide + # both group master, developers as devs + if int_level == group_link.group_access + im.where("access_level >= ?)", group_link.group_access) + else + im.send(level) + end + end + + invited_members << im + end + + invited_members.flatten.compact + end end -- cgit v1.2.1 From 6f558121b45f705624b76c2ba39491dc72810c13 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 20 Sep 2016 21:48:58 +0300 Subject: Fix a logic error in ProjectTeam#fetch_invited_members We were calling `.where` and `.send` on the relation, but never doing anything with the return value, resulting in proper access-level filtering never being of any consequence. --- app/models/project_team.rb | 4 ++-- spec/models/project_team_spec.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index e8606c8c2c6..04f4fa849c4 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -248,9 +248,9 @@ class ProjectTeam # group access is developers we need to provide # both group master, developers as devs if int_level == group_link.group_access - im.where("access_level >= ?)", group_link.group_access) + im = im.where("access_level >= ?", group_link.group_access) else - im.send(level) + im = im.send(level) end end diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 469e306044b..f979d66c88c 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -111,6 +111,7 @@ describe ProjectTeam, models: true do group_access: Gitlab::Access::REPORTER ) + expect(project.team.guests).to be_empty expect(project.team.reporters).to contain_exactly(group_member.user) end end -- cgit v1.2.1 From 2908f17371802d570014181033f16f2f3ffd182c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 20 Sep 2016 22:03:35 +0300 Subject: Improve clarity of variable names in ProjectTeam#fetch_invited_members --- app/models/project_team.rb | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 04f4fa849c4..ca331e7645a 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -233,28 +233,26 @@ class ProjectTeam return invited_members unless project_shared_with_group? - project.project_group_links.includes(group: [:group_members]).each do |group_link| - invited_group = group_link.group - im = invited_group.members + project.project_group_links.includes(group: [:group_members]).each do |link| + invited_group_members = link.group.members if level - int_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] + numeric_level = GroupMember.access_level_roles[level.to_s.singularize.titleize] - # Skip group members if we ask for masters - # but max group access is developers - next if int_level > group_link.group_access + # If we're asked for a level that's higher than the group's access, + # there's nothing left to do + next if numeric_level > link.group_access - # If we ask for developers and max - # group access is developers we need to provide - # both group master, developers as devs - if int_level == group_link.group_access - im = im.where("access_level >= ?", group_link.group_access) + # Make sure we include everyone _above_ the requested level as well + if numeric_level == link.group_access + invited_group_members = invited_group_members + .where("access_level >= ?", link.group_access) else - im = im.send(level) + invited_group_members = invited_group_members.send(level) end end - invited_members << im + invited_members << invited_group_members end invited_members.flatten.compact -- cgit v1.2.1 From e4485d7b688e24eb9a9db5b2af0b6372892822da Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 21 Sep 2016 14:53:02 +0300 Subject: Use `public_send` over `send` in ProjectTeam --- app/models/project_team.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index ca331e7645a..3cb11fee07f 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -202,8 +202,8 @@ class ProjectTeam group_members = group ? group.members : [] if level - project_members = project_members.send(level) - group_members = group_members.send(level) if group + project_members = project_members.public_send(level) + group_members = group_members.public_send(level) if group end user_ids = project_members.pluck(:user_id) @@ -248,7 +248,7 @@ class ProjectTeam invited_group_members = invited_group_members .where("access_level >= ?", link.group_access) else - invited_group_members = invited_group_members.send(level) + invited_group_members = invited_group_members.public_send(level) end end -- cgit v1.2.1 From e2c2bd69f2ccb6b8df41685b48259c4cde91eb8b Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 21 Sep 2016 15:28:26 +0300 Subject: Simplify invited_group_members filter-by-level --- app/models/project_team.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 3cb11fee07f..d9ce5088903 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -244,12 +244,12 @@ class ProjectTeam next if numeric_level > link.group_access # Make sure we include everyone _above_ the requested level as well - if numeric_level == link.group_access - invited_group_members = invited_group_members - .where("access_level >= ?", link.group_access) - else - invited_group_members = invited_group_members.public_send(level) - end + invited_group_members = + if numeric_level == link.group_access + invited_group_members.where("access_level >= ?", link.group_access) + else + invited_group_members.public_send(level) + end end invited_members << invited_group_members -- cgit v1.2.1