From 864bd12be4a91aadaed9674c3f454015dfda4edd Mon Sep 17 00:00:00 2001 From: Jacopo Date: Wed, 19 Jun 2019 11:51:15 +0200 Subject: [PATCH] Uses projects_authorizations.access_level in MembersFinder --- app/finders/members_finder.rb | 35 +++++++++++---- ...oject-group-members-returns-duplicates.yml | 5 +++ spec/finders/members_finder_spec.rb | 44 ++++++++++++++++--- 3 files changed, 69 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/62284-follow-up-from-resolve-api-to-get-all-project-group-members-returns-duplicates.yml diff --git a/app/finders/members_finder.rb b/app/finders/members_finder.rb index f730b015c0a..e8c7f9622a9 100644 --- a/app/finders/members_finder.rb +++ b/app/finders/members_finder.rb @@ -60,15 +60,32 @@ class MembersFinder # We're interested in a list of members without duplicates by user_id. # We prefer project members over group members, project members should go first. <<~SQL - SELECT DISTINCT ON (user_id, invite_email) member_union.* - FROM (#{union.to_sql}) AS member_union - ORDER BY user_id, - invite_email, - CASE - WHEN type = 'ProjectMember' THEN 1 - WHEN type = 'GroupMember' THEN 2 - ELSE 3 - END + SELECT DISTINCT ON (user_id, invite_email) #{member_columns} + FROM (#{union.to_sql}) AS #{member_union_table} + LEFT JOIN users on users.id = member_union.user_id + LEFT JOIN project_authorizations on project_authorizations.user_id = users.id + AND + project_authorizations.project_id = #{project.id} + ORDER BY user_id, + invite_email, + CASE + WHEN type = 'ProjectMember' THEN 1 + WHEN type = 'GroupMember' THEN 2 + ELSE 3 + END SQL end + + def member_union_table + 'member_union' + end + + def member_columns + Member.column_names.map do |column_name| + # fallback to members.access_level when project_authorizations.access_level is missing + next "COALESCE(#{ProjectAuthorization.table_name}.access_level, #{member_union_table}.access_level) access_level" if column_name == 'access_level' + + "#{member_union_table}.#{column_name}" + end.join(',') + end end diff --git a/changelogs/unreleased/62284-follow-up-from-resolve-api-to-get-all-project-group-members-returns-duplicates.yml b/changelogs/unreleased/62284-follow-up-from-resolve-api-to-get-all-project-group-members-returns-duplicates.yml new file mode 100644 index 00000000000..0c73f73c297 --- /dev/null +++ b/changelogs/unreleased/62284-follow-up-from-resolve-api-to-get-all-project-group-members-returns-duplicates.yml @@ -0,0 +1,5 @@ +--- +title: Uses projects_authorizations.access_level in MembersFinder +merge_request: 28887 +author: Jacopo Beschi @jacopo-beschi +type: fixed diff --git a/spec/finders/members_finder_spec.rb b/spec/finders/members_finder_spec.rb index 4203f58fe81..6920fb4e572 100644 --- a/spec/finders/members_finder_spec.rb +++ b/spec/finders/members_finder_spec.rb @@ -17,11 +17,10 @@ describe MembersFinder, '#execute' do result = described_class.new(project, user2).execute - expect(result.to_a).to match_array([member1, member2, member3]) + expect(result).to contain_exactly(member1, member2, member3) end - it 'includes nested group members if asked' do - project = create(:project, namespace: group) + it 'includes nested group members if asked', :nested_groups do nested_group.request_access(user1) member1 = group.add_maintainer(user2) member2 = nested_group.add_maintainer(user3) @@ -29,7 +28,28 @@ describe MembersFinder, '#execute' do result = described_class.new(project, user2).execute(include_descendants: true) - expect(result.to_a).to match_array([member1, member2, member3]) + expect(result).to contain_exactly(member1, member2, member3) + end + + it 'returns the members.access_level when the user is invited', :nested_groups do + member_invite = create(:project_member, :invited, project: project, invite_email: create(:user).email) + member1 = group.add_maintainer(user2) + + result = described_class.new(project, user2).execute(include_descendants: true) + + expect(result).to contain_exactly(member1, member_invite) + expect(result.last.access_level).to eq(member_invite.access_level) + end + + it 'returns the highest access_level for the user', :nested_groups do + member1 = project.add_guest(user1) + group.add_developer(user1) + nested_group.add_reporter(user1) + + result = described_class.new(project, user1).execute(include_descendants: true) + + expect(result).to contain_exactly(member1) + expect(result.first.access_level).to eq(Gitlab::Access::DEVELOPER) end context 'when include_invited_groups_members == true' do @@ -37,8 +57,8 @@ describe MembersFinder, '#execute' do set(:linked_group) { create(:group, :public, :access_requestable) } set(:nested_linked_group) { create(:group, parent: linked_group) } - set(:linked_group_member) { linked_group.add_developer(user1) } - set(:nested_linked_group_member) { nested_linked_group.add_developer(user2) } + set(:linked_group_member) { linked_group.add_guest(user1) } + set(:nested_linked_group_member) { nested_linked_group.add_guest(user2) } it 'includes all the invited_groups members including members inherited from ancestor groups' do create(:project_group_link, project: project, group: nested_linked_group) @@ -60,5 +80,17 @@ describe MembersFinder, '#execute' do expect(subject).to contain_exactly(linked_group_member) end + + context 'when the user is a member of invited group and ancestor groups' do + it 'returns the highest access_level for the user limited by project_group_link.group_access', :nested_groups do + create(:project_group_link, project: project, group: nested_linked_group, group_access: Gitlab::Access::REPORTER) + nested_linked_group.add_developer(user1) + + result = subject + + expect(result).to contain_exactly(linked_group_member, nested_linked_group_member) + expect(result.first.access_level).to eq(Gitlab::Access::REPORTER) + end + end end end