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>

3 files changed, 101 insertions, 108 deletions

diff --git a/spec/helpers/gitlab_routing_helper_spec.rb b/spec/helpers/gitlab_routing_helper_spec.rb
new file mode 100644
index 00000000000..14847d0a49e
--- /dev/null
+++ b/spec/helpers/gitlab_routing_helper_spec.rb
@@ -0,0 +1,79 @@
+require 'spec_helper'
+
+describe GitlabRoutingHelper do
+  describe 'Project URL helpers' do
+    describe '#project_members_url' do
+      let(:project) { build_stubbed(:empty_project) }
+
+      it { expect(project_members_url(project)).to eq namespace_project_project_members_url(project.namespace, project) }
+    end
+
+    describe '#project_member_path' do
+      let(:project_member) { create(:project_member) }
+
+      it { expect(project_member_path(project_member)).to eq namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) }
+    end
+
+    describe '#request_access_project_members_path' do
+      let(:project) { build_stubbed(:empty_project) }
+
+      it { expect(request_access_project_members_path(project)).to eq request_access_namespace_project_project_members_path(project.namespace, project) }
+    end
+
+    describe '#leave_project_members_path' do
+      let(:project) { build_stubbed(:empty_project) }
+
+      it { expect(leave_project_members_path(project)).to eq leave_namespace_project_project_members_path(project.namespace, project) }
+    end
+
+    describe '#approve_access_request_project_member_path' do
+      let(:project_member) { create(:project_member) }
+
+      it { expect(approve_access_request_project_member_path(project_member)).to eq approve_access_request_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) }
+    end
+
+    describe '#resend_invite_project_member_path' do
+      let(:project_member) { create(:project_member) }
+
+      it { expect(resend_invite_project_member_path(project_member)).to eq resend_invite_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) }
+    end
+  end
+
+  describe 'Group URL helpers' do
+    describe '#group_members_url' do
+      let(:group) { build_stubbed(:group) }
+
+      it { expect(group_members_url(group)).to eq group_group_members_url(group) }
+    end
+
+    describe '#group_member_path' do
+      let(:group_member) { create(:group_member) }
+
+      it { expect(group_member_path(group_member)).to eq group_group_member_path(group_member.source, group_member) }
+    end
+
+    describe '#request_access_group_members_path' do
+      let(:group) { build_stubbed(:group) }
+
+      it { expect(request_access_group_members_path(group)).to eq request_access_group_group_members_path(group) }
+    end
+
+    describe '#leave_group_members_path' do
+      let(:group) { build_stubbed(:group) }
+
+      it { expect(leave_group_members_path(group)).to eq leave_group_group_members_path(group) }
+    end
+
+    describe '#approve_access_request_group_member_path' do
+      let(:group_member) { create(:group_member) }
+
+      it { expect(approve_access_request_group_member_path(group_member)).to eq approve_access_request_group_group_member_path(group_member.source, group_member) }
+    end
+
+    describe '#resend_invite_group_member_path' do
+      let(:group_member) { create(:group_member) }
+
+      it { expect(resend_invite_group_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) }
+    end
+  end
+end
diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb
index c2f10e1db75..0b1a76156e0 100644
--- a/spec/helpers/members_helper_spec.rb
+++ b/spec/helpers/members_helper_spec.rb
@@ -1,22 +1,6 @@
 require 'spec_helper'
 
 describe MembersHelper do
-  describe '#member_class' do
-    let(:project_member) { build(:project_member) }
-    let(:group_member) { build(:group_member) }
-
-    it { expect(member_class(project_member)).to eq ProjectMember }
-    it { expect(member_class(group_member)).to eq GroupMember }
-  end
-
-  describe '#members_association' do
-    let(:project) { build_stubbed(:project) }
-    let(:group) { build_stubbed(:group) }
-
-    it { expect(members_association(project)).to eq :project_members }
-    it { expect(members_association(group)).to eq :group_members }
-  end
-
   describe '#action_member_permission' do
     let(:project_member) { build(:project_member) }
     let(:group_member) { build(:group_member) }
@@ -25,73 +9,20 @@ describe MembersHelper do
     it { expect(action_member_permission(:admin, group_member)).to eq :admin_group_member }
   end
 
-  describe '#can_see_entity_roles?' do
-    let(:project) { create(:project) }
+  describe '#can_see_member_roles?' do
+    let(:project) { create(:empty_project) }
     let(:group) { create(:group) }
     let(:user) { build(:user) }
     let(:admin) { build(:user, :admin) }
     let(:project_member) { create(:project_member, project: project) }
     let(:group_member) { create(:group_member, group: group) }
 
-    it { expect(can_see_entity_roles?(nil, project)).to be_falsy }
-    it { expect(can_see_entity_roles?(nil, group)).to be_falsy }
-    it { expect(can_see_entity_roles?(admin, project)).to be_truthy }
-    it { expect(can_see_entity_roles?(admin, group)).to be_truthy }
-    it { expect(can_see_entity_roles?(project_member.user, project)).to be_truthy }
-    it { expect(can_see_entity_roles?(group_member.user, group)).to be_truthy }
-  end
-
-  describe '#member_path' do
-    let(:project_member) { create(:project_member) }
-    let(:group_member) { create(:group_member) }
-
-    it { expect(member_path(project_member)).to eq namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) }
-    it { expect(member_path(group_member)).to eq group_group_member_path(group_member.source, group_member) }
-    it { expect { member_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' }
-  end
-
-  describe '#resend_invite_member_path' do
-    let(:project_member) { create(:project_member) }
-    let(:group_member) { create(:group_member) }
-
-    it { expect(resend_invite_member_path(project_member)).to eq resend_invite_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) }
-    it { expect(resend_invite_member_path(group_member)).to eq resend_invite_group_group_member_path(group_member.source, group_member) }
-    it { expect { resend_invite_member_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' }
-  end
-
-  describe '#request_access_path' do
-    let(:project) { build_stubbed(:project) }
-    let(:group) { build_stubbed(:group) }
-
-    it { expect(request_access_path(project)).to eq request_access_namespace_project_project_members_path(project.namespace, project) }
-    it { expect(request_access_path(group)).to eq request_access_group_group_members_path(group) }
-    it { expect { request_access_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' }
-  end
-
-  describe '#approve_request_member_path' do
-    let(:project_member) { create(:project_member) }
-    let(:group_member) { create(:group_member) }
-
-    it { expect(approve_request_member_path(project_member)).to eq approve_access_request_namespace_project_project_member_path(project_member.source.namespace, project_member.source, project_member) }
-    it { expect(approve_request_member_path(group_member)).to eq approve_access_request_group_group_member_path(group_member.source, group_member) }
-    it { expect { approve_request_member_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' }
-  end
-
-  describe '#leave_path' do
-    let(:project) { build_stubbed(:project) }
-    let(:group) { build_stubbed(:group) }
-
-    it { expect(leave_path(project)).to eq leave_namespace_project_project_members_path(project.namespace, project) }
-    it { expect(leave_path(group)).to eq leave_group_group_members_path(group) }
-    it { expect { leave_path(double(:member, source: 'foo')) }.to raise_error ArgumentError, 'Unknown object class' }
-  end
-
-  describe '#withdraw_request_message' do
-    let(:project) { build_stubbed(:project) }
-    let(:group) { build_stubbed(:group) }
-
-    it { expect(withdraw_request_message(project)).to eq "Are you sure you want to withdraw your access request for the \"#{project.name_with_namespace}\" project?" }
-    it { expect(withdraw_request_message(group)).to eq "Are you sure you want to withdraw your access request for the \"#{group.name}\" group?" }
+    it { expect(can_see_member_roles?(source: project, user: nil)).to be_falsy }
+    it { expect(can_see_member_roles?(source: group, user: nil)).to be_falsy }
+    it { expect(can_see_member_roles?(source: project, user: admin)).to be_truthy }
+    it { expect(can_see_member_roles?(source: group, user: admin)).to be_truthy }
+    it { expect(can_see_member_roles?(source: project, user: project_member.user)).to be_truthy }
+    it { expect(can_see_member_roles?(source: group, user: group_member.user)).to be_truthy }
   end
 
   describe '#remove_member_message' do
@@ -105,12 +36,14 @@ describe MembersHelper do
     let(:group_member_invite) { build(:group_member, group: group).tap { |m| m.generate_invite_token! } }
     let(:group_member_request) { group.request_access(requester) }
 
-    it { expect(remove_member_message(project_member)).to eq "You are going to remove #{project_member.user.name} from the #{project.name_with_namespace} project. Are you sure?" }
-    it { expect(remove_member_message(project_member_invite)).to eq "You are going to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.name_with_namespace} project. Are you sure?" }
-    it { expect(remove_member_message(project_member_request)).to eq "You are going to deny #{requester.name}'s request to join the #{project.name_with_namespace} project. Are you sure?" }
-    it { expect(remove_member_message(group_member)).to eq "You are going to remove #{group_member.user.name} from the #{group.name} group. Are you sure?" }
-    it { expect(remove_member_message(group_member_invite)).to eq "You are going to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group. Are you sure?" }
-    it { expect(remove_member_message(group_member_request)).to eq "You are going to deny #{requester.name}'s request to join the #{group.name} group. Are you sure?" }
+    it { expect(remove_member_message(project_member)).to eq "Are you sure you want to remove #{project_member.user.name} from the #{project.name_with_namespace} project?" }
+    it { expect(remove_member_message(project_member_invite)).to eq "Are you sure you want to revoke the invitation for #{project_member_invite.invite_email} to join the #{project.name_with_namespace} project?" }
+    it { expect(remove_member_message(project_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{project.name_with_namespace} project?" }
+    it { expect(remove_member_message(project_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{project.name_with_namespace} project?" }
+    it { expect(remove_member_message(group_member)).to eq "Are you sure you want to remove #{group_member.user.name} from the #{group.name} group?" }
+    it { expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group?" }
+    it { expect(remove_member_message(group_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{group.name} group?" }
+    it { expect(remove_member_message(group_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{group.name} group?" }
   end
 
   describe '#remove_member_title' do
@@ -122,10 +55,10 @@ describe MembersHelper do
     let(:group_member) { build(:group_member, group: group) }
     let(:group_member_request) { group.request_access(requester) }
 
-    it { expect(remove_member_title(project_member)).to eq 'Remove user' }
-    it { expect(remove_member_title(project_member_request)).to eq 'Deny access request' }
-    it { expect(remove_member_title(group_member)).to eq 'Remove user' }
-    it { expect(remove_member_title(group_member_request)).to eq 'Deny access request' }
+    it { expect(remove_member_title(project_member)).to eq 'Remove user from project' }
+    it { expect(remove_member_title(project_member_request)).to eq 'Deny access request from project' }
+    it { expect(remove_member_title(group_member)).to eq 'Remove user from group' }
+    it { expect(remove_member_title(group_member_request)).to eq 'Deny access request from group' }
   end
 
   describe '#leave_confirmation_message' do
@@ -133,7 +66,7 @@ describe MembersHelper do
     let(:group) { build_stubbed(:group) }
     let(:user) { build_stubbed(:user) }
 
-    it { expect(leave_confirmation_message(project)).to eq "Are you sure you want to leave \"#{project.name_with_namespace}\" project?" }
-    it { expect(leave_confirmation_message(group)).to eq "Are you sure you want to leave \"#{group.name}\" group?" }
+    it { expect(leave_confirmation_message(project)).to eq "Are you sure you want to leave the \"#{project.name_with_namespace}\" project?" }
+    it { expect(leave_confirmation_message(group)).to eq "Are you sure you want to leave the \"#{group.name}\" group?" }
   end
 end
diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index fa81c28849e..09e0bbfd00b 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -1,25 +1,6 @@
 require 'spec_helper'
 
 describe ProjectsHelper do
-  describe '#max_access_level' do
-    let(:master) { create(:user) }
-    let(:owner) { create(:user) }
-    let(:reporter) { create(:user) }
-    let(:group) { create(:group) }
-    let(:project) { build_stubbed(:empty_project, namespace: group) }
-
-    before do
-      group.add_master(master)
-      group.add_owner(owner)
-      group.add_reporter(reporter)
-    end
-
-    it { expect(max_access_level(project, master)).to eq 'Master' }
-    it { expect(max_access_level(project, owner)).to eq 'Owner' }
-    it { expect(max_access_level(project, reporter)).to eq 'Reporter' }
-    it { expect(max_access_level(project, build_stubbed(:user))).to be_nil }
-  end
-
   describe "#project_status_css_class" do
     it "returns appropriate class" do
       expect(project_status_css_class("started")).to eq("active")