Handle an edge-case whith invitees

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 <remy@rymai.me>

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,12 @@ 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)
+    wheres = ["id IN (#{@project_members.select(:id).to_sql})"]
+    wheres << "id IN (#{group_members.select(:id).to_sql})" if group_members
 
-    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])
+    @project_members = Member.
+      where(wheres.join(' OR ')).
+      order(access_level: :desc).page(params[:page])
 
     @requesters = AccessRequestsFinder.new(@project).execute(current_user)
 

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
+
+    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)
 
-    page.within first('.content-list .member') do
-      expect(page).to have_content(group.name)
+        # Group owner
+        expect(page).to have_content(user.name)
+        expect(page).to have_content(group.name)
+      end
     end
   end
 end