Merge branch 'rs-simplify-fetch_members' into 'master'

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,53 +200,61 @@ 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]
+      project_members = project_members.public_send(level)
+      group_members = group_members.public_send(level) if group
+    end
 
-          # Skip group members if we ask for masters
-          # but max group access is developers
-          next if int_level > group_link.group_access
+    user_ids = project_members.pluck(:user_id)
 
-          # 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)
+    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)
   end
+
+  def group
+    project.group
   end
 
-        invited_members << im
+  def merge_max!(first_hash, second_hash)
+    first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new }
   end
 
-      invited_members = invited_members.flatten.compact
+  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 |link|
+      invited_group_members = link.group.members
+
       if level
-      project_members = project_members.send(level)
-      group_members = group_members.send(level) if group
-    end
+        numeric_level = GroupMember.access_level_roles[level.to_s.singularize.titleize]
 
-    user_ids = project_members.pluck(:user_id)
-    user_ids.push(*invited_members.map(&:user_id)) if invited_members.any?
-    user_ids.push(*group_members.pluck(:user_id)) if group
+        # 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
 
-    User.where(id: user_ids)
+        # Make sure we include everyone _above_ the requested level as well
+        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
 
-  def group
-    project.group
+      invited_members << invited_group_members
     end
 
-  def merge_max!(first_hash, second_hash)
-    first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new }
+    invited_members.flatten.compact
   end
 end

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

spec/models/project_team_spec.rb

@@ -73,6 +73,68 @@ 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.guests).to be_empty
+        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) }