UI and copywriting improvements13948-access-request-to-projects-and-groups

+ Move 'Edit Project/Group' out of membership-related partial
+ Show the access request buttons only to logged-in users
+ Put the request access buttons out of in a more visible button
+ Improve the copy in the #remove_member_message helper

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

7 files changed, 129 insertions, 77 deletions

diff --git a/spec/models/concerns/access_requestable_spec.rb b/spec/models/concerns/access_requestable_spec.rb
index 2dfed1eb4c4..98307876962 100644
--- a/spec/models/concerns/access_requestable_spec.rb
+++ b/spec/models/concerns/access_requestable_spec.rb
@@ -7,8 +7,7 @@ describe AccessRequestable do
       let(:user) { create(:user) }
 
       it { expect(group.request_access(user)).to be_a(GroupMember) }
-      it { expect(group.request_access(user).user).to be_nil }
-      it { expect(group.request_access(user).created_by).to eq(user) }
+      it { expect(group.request_access(user).user).to eq(user) }
     end
 
     describe '#access_requested?' do
@@ -17,7 +16,7 @@ describe AccessRequestable do
 
       before { group.request_access(user) }
 
-      it { expect(group.access_requested?(user)).to be_truthy }
+      it { expect(group.members.request.exists?(user_id: user)).to be_truthy }
     end
   end
 
@@ -35,7 +34,7 @@ describe AccessRequestable do
 
       before { project.request_access(user) }
 
-      it { expect(project.access_requested?(user)).to be_truthy }
+      it { expect(project.members.request.exists?(user_id: user)).to be_truthy }
     end
   end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 52f9d57bc0a..ccdcb29f773 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -10,17 +10,6 @@ describe Group, models: true do
     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 '#group_members' do
-      let(:user) { create(:user) }
-      let(:group) { create(:group) }
-
-      before { group.request_access(user) }
-
-      it 'does not includes membership requests' do
-        expect(user.group_members).to be_empty
-      end
-    end
   end
 
   describe 'modules' do
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index a3d525d8d56..3ed3202ac6c 100644
--- a/spec/models/member_spec.rb
+++ b/spec/models/member_spec.rb
@@ -55,45 +55,78 @@ describe Member, models: true do
     end
   end
 
-  describe 'Scopes' do
+  describe 'Scopes & finders' do
     before do
       project = create(:project)
-      @invited_member = build(:project_member, user: nil).tap { |m| m.generate_invite_token! }
-      @accepted_invite_member = build(:project_member, user: nil).tap { |m| m.generate_invite_token! && m.accept_invite!(build(:user)) }
+      group = create(:group)
+      @owner_user = create(:user).tap { |u| group.add_owner(u) }
+      @owner = group.members.find_by(user_id: @owner_user.id)
+
+      @master_user = create(:user).tap { |u| project.team << [u, :master] }
+      @master = project.members.find_by(user_id: @master_user.id)
+
+      ProjectMember.add_user(project.members, 'toto1@example.com', Gitlab::Access::DEVELOPER, @master_user)
+      @invited_member = project.members.invite.find_by_invite_email('toto1@example.com')
+
+      accepted_invite_user = build(:user)
+      ProjectMember.add_user(project.members, 'toto2@example.com', Gitlab::Access::DEVELOPER, @master_user)
+      @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.project_members.find_by(created_by_id: requested_user.id)
+      @requested_member = project.members.request.find_by(user_id: requested_user.id)
+
       accepted_request_user = create(:user).tap { |u| project.request_access(u) }
-      @accepted_request_member = project.project_members.find_by(created_by_id: accepted_request_user.id).tap { |m| m.accept_request }
+      @accepted_request_member = project.members.request.find_by(user_id: accepted_request_user.id).tap { |m| m.accept_request }
     end
 
-    describe '#invite' do
+    describe '.invite' do
+      it { expect(described_class.invite).not_to include @master }
       it { expect(described_class.invite).to include @invited_member }
       it { expect(described_class.invite).not_to include @accepted_invite_member }
       it { expect(described_class.invite).not_to include @requested_member }
       it { expect(described_class.invite).not_to include @accepted_request_member }
     end
 
-    describe '#request' do
+    describe '.non_invite' do
+      it { expect(described_class.non_invite).to include @master }
+      it { expect(described_class.non_invite).not_to include @invited_member }
+      it { expect(described_class.non_invite).to include @accepted_invite_member }
+      it { expect(described_class.non_invite).to include @requested_member }
+      it { expect(described_class.non_invite).to include @accepted_request_member }
+    end
+
+    describe '.request' do
+      it { expect(described_class.request).not_to include @master }
       it { expect(described_class.request).not_to include @invited_member }
       it { expect(described_class.request).not_to include @accepted_invite_member }
       it { expect(described_class.request).to include @requested_member }
       it { expect(described_class.request).not_to include @accepted_request_member }
     end
 
-    describe '#non_request' do
+    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
+    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 }
+      it { expect(described_class.owners_and_masters).not_to include @invited_member }
+      it { expect(described_class.owners_and_masters).not_to include @accepted_invite_member }
+      it { expect(described_class.owners_and_masters).not_to include @requested_member }
+      it { expect(described_class.owners_and_masters).not_to include @accepted_request_member }
+    end
   end
 
   describe "Delegate methods" do
@@ -101,6 +134,18 @@ describe Member, models: true do
     it { is_expected.to respond_to(:user_email) }
   end
 
+  describe 'Callbacks' do
+    describe 'after_destroy :post_decline_request, if: :request?' do
+      let(:member) { create(:project_member, requested_at: Time.now.utc) }
+
+      it 'calls #post_decline_request' do
+        expect(member).to receive(:post_decline_request)
+
+        member.destroy
+      end
+    end
+  end
+
   describe ".add_user" do
     let!(:user)    { create(:user) }
     let(:project) { create(:project) }
@@ -139,18 +184,9 @@ describe Member, models: true do
   end
 
   describe '#accept_request' do
-    let(:user) { create(:user) }
-    let(:member) { create(:project_member, requested_at: Time.now.utc, user: nil, created_by: user) }
-
-    it 'returns true' do
-      expect(member.accept_request).to be_truthy
-    end
+    let(:member) { create(:project_member, requested_at: Time.now.utc) }
 
-    it 'sets the user' do
-      member.accept_request
-
-      expect(member.user).to eq(user)
-    end
+    it { expect(member.accept_request).to be_truthy }
 
     it 'clears requested_at' do
       member.accept_request
@@ -165,25 +201,24 @@ describe Member, models: true do
     end
   end
 
-  describe '#decline_request' do
-    let(:user) { create(:user) }
-    let(:member) { create(:project_member, requested_at: Time.now.utc, user: nil, created_by: user) }
+  describe '#invite?' do
+    subject { create(:project_member, invite_email: "user@example.com", user: nil) }
 
-    it 'returns true' do
-      expect(member.decline_request).to be_truthy
-    end
+    it { is_expected.to be_invite }
+  end
 
-    it 'destroys the member' do
-      member.decline_request
+  describe '#request?' do
+    subject { create(:project_member, requested_at: Time.now.utc) }
 
-      expect(member).to be_destroyed
-    end
+    it { is_expected.to be_request }
+  end
 
-    it 'calls #after_decline_request' do
-      expect(member).to receive(:after_decline_request)
+  describe '#pending?' do
+    let(:invited_member) { create(:project_member, invite_email: "user@example.com", user: nil) }
+    let(:requester) { create(:project_member, requested_at: Time.now.utc) }
 
-      member.decline_request
-    end
+    it { expect(invited_member).to be_invite }
+    it { expect(requester).to be_pending }
   end
 
   describe "#accept_invite!" do
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index c3070d4cb78..eeb74a462ac 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -51,24 +51,30 @@ describe GroupMember, models: true do
       end
     end
 
-    describe 'after accept_request' do
-      let(:member) { create(:group_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) }
+    describe '#after_accept_request' do
+      it 'calls NotificationService.accept_group_access_request' do
+        member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now)
 
-      it "calls #accept_group_access_request" do
         expect_any_instance_of(NotificationService).to receive(:new_group_member)
 
-        member.accept_request
+        member.__send__(:after_accept_request)
       end
     end
 
-    describe 'after decline_request' do
-      let(:member) { create(:group_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) }
+    describe '#post_decline_request' do
+      it 'calls NotificationService.decline_group_access_request' do
+        member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now)
 
-      it "calls #decline_group_access_request" do
         expect_any_instance_of(NotificationService).to receive(:decline_group_access_request)
 
-        member.decline_request
+        member.__send__(:post_decline_request)
       end
     end
+
+    describe '#real_source_type' do
+      subject { create(:group_member).real_source_type }
+
+      it { is_expected.to eq 'Group' }
+    end
   end
 end
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index 99b3c77c6cd..1e466f9c620 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -33,6 +33,12 @@ describe ProjectMember, models: true do
     it { is_expected.to include_module(Gitlab::ShellAdapter) }
   end
 
+  describe '#real_source_type' do
+    subject { create(:project_member).real_source_type }
+
+    it { is_expected.to eq 'Project' }
+  end
+
   describe "#destroy" do
     let(:owner)   { create(:project_member, access_level: ProjectMember::OWNER) }
     let(:project) { owner.project }
@@ -137,23 +143,23 @@ describe ProjectMember, models: true do
   end
 
   describe 'notifications' do
-    describe 'after accept_request' do
-      let(:member) { create(:project_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) }
+    describe '#after_accept_request' do
+      it 'calls NotificationService.new_project_member' do
+        member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now)
 
-      it 'calls #accept_project_access_request' do
         expect_any_instance_of(NotificationService).to receive(:new_project_member)
 
-        member.accept_request
+        member.__send__(:after_accept_request)
       end
     end
 
-    describe 'after decline_request' do
-      let(:member) { create(:project_member, user: nil, created_by: build_stubbed(:user), requested_at: Time.now) }
+    describe '#post_decline_request' do
+      it 'calls NotificationService.decline_project_access_request' do
+        member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now)
 
-      it 'calls #decline_project_access_request' do
         expect_any_instance_of(NotificationService).to receive(:decline_project_access_request)
 
-        member.decline_request
+        member.__send__(:post_decline_request)
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d5a4b73affd..30aa2b70c8d 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -29,17 +29,6 @@ describe Project, models: true do
     it { is_expected.to have_many(:variables) }
     it { is_expected.to have_many(:triggers) }
     it { is_expected.to have_many(:todos).dependent(:destroy) }
-
-    describe '#project_members' do
-      let(:user) { create(:user) }
-      let(:project) { create(:project) }
-
-      before { project.request_access(user) }
-
-      it 'does not includes membership requests' do
-        expect(user.project_members).to be_empty
-      end
-    end
   end
 
   describe 'modules' do
@@ -100,11 +89,17 @@ describe Project, models: true do
     it { is_expected.to respond_to(:repo_exists?) }
     it { is_expected.to respond_to(:update_merge_requests) }
     it { is_expected.to respond_to(:execute_hooks) }
-    it { is_expected.to respond_to(:name_with_namespace) }
     it { is_expected.to respond_to(:owner) }
     it { is_expected.to respond_to(:path_with_namespace) }
   end
 
+  describe '#name_with_namespace' do
+    let(:project) { build_stubbed(:empty_project) }
+
+    it { expect(project.name_with_namespace).to eq "#{project.namespace.human_name} / #{project.name}" }
+    it { expect(project.human_name).to eq project.name_with_namespace }
+  end
+
   describe '#to_reference' do
     let(:project) { create(:empty_project) }
 
diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb
index 36b1f439955..9262aeb6ed8 100644
--- a/spec/models/project_team_spec.rb
+++ b/spec/models/project_team_spec.rb
@@ -112,6 +112,28 @@ describe ProjectTeam, models: true do
     end
   end
 
+  describe "#human_max_access" do
+    it 'returns Master role' do
+      user = create(:user)
+      group = create(:group)
+      group.add_master(user)
+
+      project = build_stubbed(:empty_project, namespace: group)
+
+      expect(project.team.human_max_access(user.id)).to eq 'Master'
+    end
+
+    it 'returns Owner role' do
+      user = create(:user)
+      group = create(:group)
+      group.add_owner(user)
+
+      project = build_stubbed(:empty_project, namespace: group)
+
+      expect(project.team.human_max_access(user.id)).to eq 'Owner'
+    end
+  end
+
   describe '#max_member_access' do
     let(:requester) { create(:user) }