From 89c22ed5af490f250800030ee4342c185dbc5358 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 2 Dec 2016 14:34:08 +0000 Subject: [PATCH 1/4] Shows group members in the project members list Closes #24122 --- .../projects/project_members_controller.rb | 20 +++++++++++++++++- app/views/shared/members/_member.html.haml | 6 +++++- .../group-members-in-project-members-view.yml | 4 ++++ .../projects/members/group_members_spec.rb | 21 +++++++++++++++++++ 4 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/group-members-in-project-members-view.yml create mode 100644 spec/features/projects/members/group_members_spec.rb diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 699a56ae2f8..ccf5ff35171 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -10,14 +10,32 @@ class Projects::ProjectMembersController < Projects::ApplicationController @project_members = @project.project_members @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project) + @group = @project.group + + if @group + @group_members = @group.group_members + @group_members = @group_members.non_invite unless can?(current_user, :admin_group, @group) + end + if params[:search].present? users = @project.users.search(params[:search]).to_a @project_members = @project_members.where(user_id: users) + if @group_members + users = @group.users.search(params[:search]).to_a + @group_members = @group_members.where(user_id: users) + end + @group_links = @project.project_group_links.where(group_id: @project.invited_groups.search(params[:search]).select(:id)) end - @project_members = @project_members.order(access_level: :desc).page(params[:page]) + members_id = @project_members.pluck(:id) + + if @group_members + members_id << @group_members.select{ |member| !@project_members.find_by(user_id: member.user_id) }.select(&:id) + end + + @project_members = Member.where(id: members_id.flatten).order(access_level: :desc).page(params[:page]) @requesters = AccessRequestsFinder.new(@project).execute(current_user) diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 432047a1c4e..bf42c9080a6 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -12,6 +12,10 @@ = link_to user.name, user_path(user) %span.cgray= user.to_reference + - if member.real_source_type == 'Group' + · + %span.cblue=member.group.name + - if user == current_user %span.label.label-success.prepend-left-5 It's you @@ -45,7 +49,7 @@ = time_ago_with_tooltip(member.created_at) - if show_roles .controls.member-controls - - if show_controls + - if show_controls && member.real_source_type == 'Project' - if user != current_user = form_for member, remote: true, html: { class: 'form-horizontal js-edit-member-form' } do |f| = f.select :access_level, options_for_select(member.class.access_level_roles, member.access_level), {}, class: 'form-control member-form-control append-right-5 js-member-update-control', id: "member_access_level_#{member.id}", disabled: !can_admin_member diff --git a/changelogs/unreleased/group-members-in-project-members-view.yml b/changelogs/unreleased/group-members-in-project-members-view.yml new file mode 100644 index 00000000000..415e2b6b1e2 --- /dev/null +++ b/changelogs/unreleased/group-members-in-project-members-view.yml @@ -0,0 +1,4 @@ +--- +title: Shows group members in project members list +merge_request: +author: diff --git a/spec/features/projects/members/group_members_spec.rb b/spec/features/projects/members/group_members_spec.rb new file mode 100644 index 00000000000..39235a1cd4f --- /dev/null +++ b/spec/features/projects/members/group_members_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +feature 'Projects members', feature: true do + let(:user) { create(:user) } + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + + background do + group.add_owner(user) + login_as(user) + visit namespace_project_project_members_path(project.namespace, project) + end + + it 'shows group members in list' do + expect(page).to have_selector('.group_member') + + page.within first('.content-list .member') do + expect(page).to have_content(group.name) + end + end +end From cc66ec2b73d6fa581f5300957597615ed1b58c55 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 2 Dec 2016 16:48:34 +0000 Subject: [PATCH 2/4] Fixed Ruby to be better for performance Fixed controls not showing in groups which fixes tests --- .../projects/project_members_controller.rb | 22 +++++++++---------- app/views/shared/members/_member.html.haml | 10 +++------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index ccf5ff35171..10bc75b4c5e 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -10,20 +10,20 @@ class Projects::ProjectMembersController < Projects::ApplicationController @project_members = @project.project_members @project_members = @project_members.non_invite unless can?(current_user, :admin_project, @project) - @group = @project.group + group = @project.group - if @group - @group_members = @group.group_members - @group_members = @group_members.non_invite unless can?(current_user, :admin_group, @group) + if group + group_members = group.group_members.where.not(user_id: @project_members.select(:user_id)) + group_members = group_members.non_invite unless can?(current_user, :admin_group, @group) end if params[:search].present? - users = @project.users.search(params[:search]).to_a - @project_members = @project_members.where(user_id: users) + user_ids = @project.users.search(params[:search]).select(:id) + @project_members = @project_members.where(user_id: user_ids) - if @group_members - users = @group.users.search(params[:search]).to_a - @group_members = @group_members.where(user_id: users) + if group_members + user_ids = group.users.search(params[:search]).select(:id) + group_members = group_members.where(user_id: user_ids) end @group_links = @project.project_group_links.where(group_id: @project.invited_groups.search(params[:search]).select(:id)) @@ -31,8 +31,8 @@ class Projects::ProjectMembersController < Projects::ApplicationController members_id = @project_members.pluck(:id) - if @group_members - members_id << @group_members.select{ |member| !@project_members.find_by(user_id: member.user_id) }.select(&:id) + if group_members + members_id << group_members.pluck(:id) end @project_members = Member.where(id: members_id.flatten).order(access_level: :desc).page(params[:page]) diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index bf42c9080a6..aa5b39151e6 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -12,10 +12,6 @@ = link_to user.name, user_path(user) %span.cgray= user.to_reference - - if member.real_source_type == 'Group' - · - %span.cblue=member.group.name - - if user == current_user %span.label.label-success.prepend-left-5 It's you @@ -24,8 +20,8 @@ %strong Blocked - if source.instance_of?(Group) && !@group - = link_to source, class: "member-group-link prepend-left-5" do - = "· #{source.name}" + · + = link_to source.name, source, class: "member-group-link" .hidden-xs.cgray - if member.request? @@ -49,7 +45,7 @@ = time_ago_with_tooltip(member.created_at) - if show_roles .controls.member-controls - - if show_controls && member.real_source_type == 'Project' + - if show_controls && (member.respond_to?(:group) && @members) || (member.respond_to?(:project) && @project_members) - if user != current_user = form_for member, remote: true, html: { class: 'form-horizontal js-edit-member-form' } do |f| = f.select :access_level, options_for_select(member.class.access_level_roles, member.access_level), {}, class: 'form-control member-form-control append-right-5 js-member-update-control', id: "member_access_level_#{member.id}", disabled: !can_admin_member From cae90506fb3bdf8412d241a21a1f0fe2d96393b6 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 5 Dec 2016 16:24:56 +0000 Subject: [PATCH 3/4] Fixed if statement to use global group & project variables --- app/views/shared/members/_member.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index aa5b39151e6..e67f7d5a352 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -45,7 +45,7 @@ = time_ago_with_tooltip(member.created_at) - if show_roles .controls.member-controls - - if show_controls && (member.respond_to?(:group) && @members) || (member.respond_to?(:project) && @project_members) + - if show_controls && (member.respond_to?(:group) && @group) || (member.respond_to?(:project) && @project) - if user != current_user = form_for member, remote: true, html: { class: 'form-horizontal js-edit-member-form' } do |f| = f.select :access_level, options_for_select(member.class.access_level_roles, member.access_level), {}, class: 'form-control member-form-control append-right-5 js-member-update-control', id: "member_access_level_#{member.id}", disabled: !can_admin_member From 96f162125dabb3d3ff21cb95abf97e5af6ee5589 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 7 Dec 2016 09:59:52 +0100 Subject: [PATCH 4/4] Handle an edge-case whith invitees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the project has invitees, no group members were returned due to a `user_id NOT IN (42, NULL)` query which always returned [] since a `user_id` would be NULL, thus the condition could never match. Signed-off-by: Rémy Coutable --- .../projects/project_members_controller.rb | 14 +++- .../projects/members/group_members_spec.rb | 83 +++++++++++++++++-- 2 files changed, 86 insertions(+), 11 deletions(-) diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 10bc75b4c5e..3fb8bba3cd0 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -13,7 +13,13 @@ class Projects::ProjectMembersController < Projects::ApplicationController group = @project.group if group - group_members = group.group_members.where.not(user_id: @project_members.select(:user_id)) + # We need `.where.not(user_id: nil)` here otherwise when a group has an + # invitee, it would make the following query return 0 rows since a NULL + # user_id would be present in the subquery + # See http://stackoverflow.com/questions/129077/not-in-clause-and-null-values + # FIXME: This whole logic should be moved to a finder! + non_null_user_ids = @project_members.where.not(user_id: nil).select(:user_id) + group_members = group.group_members.where.not(user_id: non_null_user_ids) group_members = group_members.non_invite unless can?(current_user, :admin_group, @group) end @@ -29,13 +35,13 @@ class Projects::ProjectMembersController < Projects::ApplicationController @group_links = @project.project_group_links.where(group_id: @project.invited_groups.search(params[:search]).select(:id)) end - members_id = @project_members.pluck(:id) + member_ids = @project_members.pluck(:id) if group_members - members_id << group_members.pluck(:id) + member_ids += group_members.pluck(:id) end - @project_members = Member.where(id: members_id.flatten).order(access_level: :desc).page(params[:page]) + @project_members = Member.where(id: member_ids).order(access_level: :desc).page(params[:page]) @requesters = AccessRequestsFinder.new(@project).execute(current_user) diff --git a/spec/features/projects/members/group_members_spec.rb b/spec/features/projects/members/group_members_spec.rb index 39235a1cd4f..7d0065ee2c4 100644 --- a/spec/features/projects/members/group_members_spec.rb +++ b/spec/features/projects/members/group_members_spec.rb @@ -2,20 +2,89 @@ require 'spec_helper' feature 'Projects members', feature: true do let(:user) { create(:user) } - let(:group) { create(:group) } - let(:project) { create(:project, group: group) } + let(:developer) { create(:user) } + let(:group) { create(:group, :public, :access_requestable) } + let(:project) { create(:empty_project, :public, :access_requestable, creator: user, group: group) } + let(:project_invitee) { create(:project_member, project: project, invite_token: '123', invite_email: 'test1@abc.com', user: nil) } + let(:group_invitee) { create(:group_member, group: group, invite_token: '123', invite_email: 'test2@abc.com', user: nil) } + let(:project_requester) { create(:user) } + let(:group_requester) { create(:user) } background do + project.team << [developer, :developer] group.add_owner(user) login_as(user) - visit namespace_project_project_members_path(project.namespace, project) end - it 'shows group members in list' do - expect(page).to have_selector('.group_member') + context 'with a group invitee' do + before do + group_invitee + visit namespace_project_project_members_path(project.namespace, project) + end - page.within first('.content-list .member') do - expect(page).to have_content(group.name) + scenario 'does not appear in the project members page' do + page.within first('.content-list') do + expect(page).not_to have_content('test2@abc.com') + end + end + end + + context 'with a group and a project invitee' do + before do + group_invitee + project_invitee + visit namespace_project_project_members_path(project.namespace, project) + end + + scenario 'shows the project invitee, the project developer, and the group owner' do + page.within first('.content-list') do + expect(page).to have_content('test1@abc.com') + expect(page).not_to have_content('test2@abc.com') + + # Project developer + expect(page).to have_content(developer.name) + + # Group owner + expect(page).to have_content(user.name) + expect(page).to have_content(group.name) + end + end + end + + context 'with a group requester' do + before do + group.request_access(group_requester) + visit namespace_project_project_members_path(project.namespace, project) + end + + scenario 'does not appear in the project members page' do + page.within first('.content-list') do + expect(page).not_to have_content(group_requester.name) + end + end + end + + context 'with a group and a project requesters' do + before do + group.request_access(group_requester) + project.request_access(project_requester) + visit namespace_project_project_members_path(project.namespace, project) + end + + scenario 'shows the project requester, the project developer, and the group owner' do + page.within first('.content-list') do + expect(page).to have_content(project_requester.name) + expect(page).not_to have_content(group_requester.name) + end + + page.within all('.content-list').last do + # Project developer + expect(page).to have_content(developer.name) + + # Group owner + expect(page).to have_content(user.name) + expect(page).to have_content(group.name) + end end end end