Exclude requesters from Project#members, Group#members and User#members

And create new Project#requesters, Group#requesters scopes.

Signed-off-by: Rémy Coutable <remy@rymai.me>

spec/controllers/projects/project_members_controller_spec.rb

@@ -187,7 +187,7 @@ describe Projects::ProjectMembersController do
           expect(response).to set_flash.to 'Your access request to the project has been withdrawn.'
           expect(response).to redirect_to(namespace_project_path(project.namespace, project))
-          expect(project.members.request).to be_empty
+          expect(project.requesters).to be_empty
           expect(project.users).not_to include user
         end
       end
@@ -210,7 +210,7 @@ describe Projects::ProjectMembersController do
       expect(response).to redirect_to(
         namespace_project_path(project.namespace, project)
       )
-      expect(project.members.request.exists?(user_id: user)).to be_truthy
+      expect(project.requesters.exists?(user_id: user)).to be_truthy
       expect(project.users).not_to include user
     end
   end
@@ -233,7 +233,7 @@ describe Projects::ProjectMembersController do
       let(:team_requester) { create(:user) }
       let(:member) do
         project.request_access(team_requester)
-        project.members.request.find_by(user_id: team_requester.id)
+        project.requesters.find_by(user_id: team_requester.id)
       end
       context 'when user does not have enough rights' do

spec/features/groups/members/owner_manages_access_requests_spec.rb

@@ -41,7 +41,7 @@ feature 'Groups > Members > Owner manages access requests', feature: true do
   def expect_visible_access_request(group, user)
-    expect(group.members.request.exists?(user_id: user)).to be_truthy
+    expect(group.requesters.exists?(user_id: user)).to be_truthy
     expect(page).to have_content "#{group.name} access requests 1"
     expect(page).to have_content user.name
   end

spec/features/groups/members/user_requests_access_spec.rb

@@ -18,7 +18,7 @@ feature 'Groups > Members > User requests access', feature: true do
     expect(ActionMailer::Base.deliveries.last.to).to eq [owner.notification_email]
     expect(ActionMailer::Base.deliveries.last.subject).to match "Request to join the #{group.name} group"
-    expect(group.members.request.exists?(user_id: user)).to be_truthy
+    expect(group.requesters.exists?(user_id: user)).to be_truthy
     expect(page).to have_content 'Your request for access has been queued for review.'
     expect(page).to have_content 'Withdraw Access Request'
@@ -42,7 +42,7 @@ feature 'Groups > Members > User requests access', feature: true do
   scenario 'user is not listed in the group members page' do
     click_link 'Request Access'
-    expect(group.members.request.exists?(user_id: user)).to be_truthy
+    expect(group.requesters.exists?(user_id: user)).to be_truthy
     click_link 'Members'
@@ -54,11 +54,11 @@ feature 'Groups > Members > User requests access', feature: true do
   scenario 'user can withdraw its request for access' do
     click_link 'Request Access'
-    expect(group.members.request.exists?(user_id: user)).to be_truthy
+    expect(group.requesters.exists?(user_id: user)).to be_truthy
     click_link 'Withdraw Access Request'
-    expect(group.members.request.exists?(user_id: user)).to be_falsey
+    expect(group.requesters.exists?(user_id: user)).to be_falsey
     expect(page).to have_content 'Your access request to the group has been withdrawn.'
   end
 end

spec/features/projects/members/master_manages_access_requests_spec.rb

@@ -40,7 +40,7 @@ feature 'Projects > Members > Master manages access requests', feature: true do
   end
   def expect_visible_access_request(project, user)
-    expect(project.members.request.exists?(user_id: user)).to be_truthy
+    expect(project.requesters.exists?(user_id: user)).to be_truthy
     expect(page).to have_content "#{project.name} access requests 1"
     expect(page).to have_content user.name
   end

spec/features/projects/members/user_requests_access_spec.rb

@@ -17,7 +17,7 @@ feature 'Projects > Members > User requests access', feature: true do
     expect(ActionMailer::Base.deliveries.last.to).to eq [master.notification_email]
     expect(ActionMailer::Base.deliveries.last.subject).to eq "Request to join the #{project.name_with_namespace} project"
-    expect(project.members.request.exists?(user_id: user)).to be_truthy
+    expect(project.requesters.exists?(user_id: user)).to be_truthy
     expect(page).to have_content 'Your request for access has been queued for review.'
     expect(page).to have_content 'Withdraw Access Request'
@@ -27,7 +27,7 @@ feature 'Projects > Members > User requests access', feature: true do
   scenario 'user is not listed in the project members page' do
     click_link 'Request Access'
-    expect(project.members.request.exists?(user_id: user)).to be_truthy
+    expect(project.requesters.exists?(user_id: user)).to be_truthy
     open_project_settings_menu
     click_link 'Members'
@@ -41,11 +41,11 @@ feature 'Projects > Members > User requests access', feature: true do
   scenario 'user can withdraw its request for access' do
     click_link 'Request Access'
-    expect(project.members.request.exists?(user_id: user)).to be_truthy
+    expect(project.requesters.exists?(user_id: user)).to be_truthy
     click_link 'Withdraw Access Request'
-    expect(project.members.request.exists?(user_id: user)).to be_falsey
+    expect(project.requesters.exists?(user_id: user)).to be_falsey
     expect(page).to have_content 'Your access request to the project has been withdrawn.'
   end

spec/helpers/members_helper_spec.rb

@@ -57,6 +57,72 @@ describe MembersHelper do
     end
   end
+  describe '#can_see_request_access_button?' do
+    let(:user) { create(:user) }
+    let(:group) { create(:group, :public) }
+    let(:project) { create(:project, :public, group: group) }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    context 'source is a group' do
+      context 'current_user is not a member' do
+        it 'returns true' do
+          expect(helper.can_see_request_access_button?(group)).to be_truthy
+        end
+      end
+
+      context 'current_user is a member' do
+        it 'returns false' do
+          group.add_owner(user)
+
+          expect(helper.can_see_request_access_button?(group)).to be_falsy
+        end
+      end
+
+      context 'current_user is a requester' do
+        it 'returns true' do
+          group.request_access(user)
+
+          expect(helper.can_see_request_access_button?(group)).to be_truthy
+        end
+      end
+    end
+
+    context 'source is a project' do
+      context 'current_user is not a member' do
+        it 'returns true' do
+          expect(helper.can_see_request_access_button?(project)).to be_truthy
+        end
+      end
+
+      context 'current_user is a group member' do
+        it 'returns false' do
+          group.add_owner(user)
+
+          expect(helper.can_see_request_access_button?(project)).to be_falsy
+        end
+      end
+
+      context 'current_user is a group requester' do
+        it 'returns false' do
+          group.request_access(user)
+
+          expect(helper.can_see_request_access_button?(project)).to be_falsy
+        end
+      end
+
+      context 'current_user is a member' do
+        it 'returns false' do
+          project.team << [user, :master]
+
+          expect(helper.can_see_request_access_button?(project)).to be_falsy
+        end
+      end
+    end
+  end
+
   describe '#remove_member_message' do
     let(:requester) { build(:user) }
     let(:project) { create(:project) }

spec/mailers/notify_spec.rb

@@ -406,7 +406,7 @@ describe Notify do
         let(:user) { create(:user) }
         let(:project_member) do
           project.request_access(user)
-          project.members.request.find_by(user_id: user.id)
+          project.requesters.find_by(user_id: user.id)
         end
         subject { Notify.member_access_requested_email('project', project_member.id) }
@@ -433,7 +433,7 @@ describe Notify do
         let(:user) { create(:user) }
         let(:project_member) do
           project.request_access(user)
-          project.members.request.find_by(user_id: user.id)
+          project.requesters.find_by(user_id: user.id)
         end
         subject { Notify.member_access_requested_email('project', project_member.id) }
@@ -459,7 +459,7 @@ describe Notify do
       let(:user) { create(:user) }
       let(:project_member) do
         project.request_access(user)
-        project.members.request.find_by(user_id: user.id)
+        project.requesters.find_by(user_id: user.id)
       end
       subject { Notify.member_access_denied_email('project', project.id, user.id) }
@@ -684,7 +684,7 @@ describe Notify do
       let(:user) { create(:user) }
       let(:group_member) do
         group.request_access(user)
-        group.members.request.find_by(user_id: user.id)
+        group.requesters.find_by(user_id: user.id)
       end
       subject { Notify.member_access_requested_email('group', group_member.id) }
@@ -705,7 +705,7 @@ describe Notify do
       let(:user) { create(:user) }
       let(:group_member) do
         group.request_access(user)
-        group.members.request.find_by(user_id: user.id)
+        group.requesters.find_by(user_id: user.id)
       end
       subject { Notify.member_access_denied_email('group', group.id, user.id) }

spec/models/concerns/access_requestable_spec.rb

@@ -16,7 +16,7 @@ describe AccessRequestable do
       before { group.request_access(user) }
-      it { expect(group.members.request.exists?(user_id: user)).to be_truthy }
+      it { expect(group.requesters.exists?(user_id: user)).to be_truthy }
     end
   end
@@ -34,7 +34,7 @@ describe AccessRequestable do
       before { project.request_access(user) }
-      it { expect(project.members.request.exists?(user_id: user)).to be_truthy }
+      it { expect(project.requesters.exists?(user_id: user)).to be_truthy }
     end
   end
 end

spec/models/group_spec.rb

@@ -7,9 +7,38 @@ describe Group, models: true do
     it { is_expected.to have_many :projects }
     it { is_expected.to have_many(:group_members).dependent(:destroy) }
     it { is_expected.to have_many(:users).through(:group_members) }
+    it { is_expected.to have_many(:owners).through(:group_members) }
+    it { is_expected.to have_many(:requesters).dependent(:destroy) }
     it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
     it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
+
+    describe '#members & #requesters' do
+      let(:requester) { create(:user) }
+      let(:developer) { create(:user) }
+      before do
+        group.request_access(requester)
+        group.add_developer(developer)
+      end
+
+      describe '#members' do
+        it 'includes members and exclude requesters' do
+          member_user_ids = group.members.pluck(:user_id)
+
+          expect(member_user_ids).to include(developer.id)
+          expect(member_user_ids).not_to include(requester.id)
+        end
+      end
+
+      describe '#requesters' do
+        it 'does not include requesters' do
+          requester_user_ids = group.requesters.pluck(:user_id)
+
+          expect(requester_user_ids).to include(requester.id)
+          expect(requester_user_ids).not_to include(developer.id)
+        end
+      end
+    end
   end
   describe 'modules' do

spec/models/member_spec.rb

@@ -73,10 +73,10 @@ describe Member, models: true do
       @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) }
       requested_user = create(:user).tap { |u| project.request_access(u) }
-      @requested_member = project.members.request.find_by(user_id: requested_user.id)
+      @requested_member = project.requesters.find_by(user_id: requested_user.id)
       accepted_request_user = create(:user).tap { |u| project.request_access(u) }
-      @accepted_request_member = project.members.request.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
+      @accepted_request_member = project.requesters.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
     end
     describe '.invite' do
@@ -103,22 +103,6 @@ describe Member, models: true do
       it { expect(described_class.request).not_to include @accepted_request_member }
     end
-    describe '.non_request' do
-      it { expect(described_class.non_request).to include @master }
-      it { expect(described_class.non_request).to include @invited_member }
-      it { expect(described_class.non_request).to include @accepted_invite_member }
-      it { expect(described_class.non_request).not_to include @requested_member }
-      it { expect(described_class.non_request).to include @accepted_request_member }
-    end
-
-    describe '.non_pending' do
-      it { expect(described_class.non_pending).to include @master }
-      it { expect(described_class.non_pending).not_to include @invited_member }
-      it { expect(described_class.non_pending).to include @accepted_invite_member }
-      it { expect(described_class.non_pending).not_to include @requested_member }
-      it { expect(described_class.non_pending).to include @accepted_request_member }
-    end
-
     describe '.owners_and_masters' do
       it { expect(described_class.owners_and_masters).to include @owner }
       it { expect(described_class.owners_and_masters).to include @master }

spec/models/project_spec.rb

@@ -11,6 +11,8 @@ describe Project, models: true do
     it { is_expected.to have_many(:issues).dependent(:destroy) }
     it { is_expected.to have_many(:milestones).dependent(:destroy) }
     it { is_expected.to have_many(:project_members).dependent(:destroy) }
+    it { is_expected.to have_many(:users).through(:project_members) }
+    it { is_expected.to have_many(:requesters).dependent(:destroy) }
     it { is_expected.to have_many(:notes).dependent(:destroy) }
     it { is_expected.to have_many(:snippets).class_name('ProjectSnippet').dependent(:destroy) }
     it { is_expected.to have_many(:deploy_keys_projects).dependent(:destroy) }
@@ -31,6 +33,34 @@ describe Project, models: true do
     it { is_expected.to have_many(:environments).dependent(:destroy) }
     it { is_expected.to have_many(:deployments).dependent(:destroy) }
     it { is_expected.to have_many(:todos).dependent(:destroy) }
+
+    describe '#members & #requesters' do
+      let(:project) { create(:project) }
+      let(:requester) { create(:user) }
+      let(:developer) { create(:user) }
+      before do
+        project.request_access(requester)
+        project.team << [developer, :developer]
+      end
+
+      describe '#members' do
+        it 'includes members and exclude requesters' do
+          member_user_ids = project.members.pluck(:user_id)
+
+          expect(member_user_ids).to include(developer.id)
+          expect(member_user_ids).not_to include(requester.id)
+        end
+      end
+
+      describe '#requesters' do
+        it 'does not include requesters' do
+          requester_user_ids = project.requesters.pluck(:user_id)
+
+          expect(requester_user_ids).to include(requester.id)
+          expect(requester_user_ids).not_to include(developer.id)
+        end
+      end
+    end
   end
   describe 'modules' do