From 9af7d71bd6621721d6b027d4c50adf395f9afef7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 22 Jun 2016 01:15:29 +0000 Subject: Merge branch '18755-fix-destroy-project-causes-post_decline_request-to-be-executed' into 'master' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve "Destroying a project causes post_decline_request to be executed" ## What does this MR do? Ensure we don't send "access request declined" to access requesters when a project is deleted. ## Are there points in the code the reviewer needs to double check? I've created a service to decouple the notification sending from the AR model. ## Why was this MR needed? Because there was an issue. ## What are the relevant issue numbers? Fixes #18755, #18750. ## Does this MR meet the acceptance criteria? - [x] No CHANGELOG needed. - [x] Tests - [x] Added for this feature/bug - [x] All builds are passing - [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides) - [x] Branch has no merge conflicts with `master` (if you do - rebase it please) - [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits) See merge request !4744 Signed-off-by: Rémy Coutable --- .../groups/group_members_controller_spec.rb | 6 +- .../projects/project_members_controller_spec.rb | 8 +-- .../members/last_owner_cannot_leave_group_spec.rb | 16 +++++ .../groups/members/member_leaves_group_spec.rb | 21 +++++++ .../groups/members/user_requests_access_spec.rb | 1 + .../projects/members/member_leaves_project_spec.rb | 19 ++++++ .../members/owner_cannot_leave_project_spec.rb | 16 +++++ .../projects/members/user_requests_access_spec.rb | 1 + spec/features/projects_spec.rb | 16 ----- spec/models/member_spec.rb | 12 ---- spec/models/members/group_member_spec.rb | 10 --- spec/models/members/project_member_spec.rb | 10 --- spec/services/members/destroy_service_spec.rb | 71 ++++++++++++++++++++++ 13 files changed, 149 insertions(+), 58 deletions(-) create mode 100644 spec/features/groups/members/last_owner_cannot_leave_group_spec.rb create mode 100644 spec/features/groups/members/member_leaves_group_spec.rb create mode 100644 spec/features/projects/members/member_leaves_project_spec.rb create mode 100644 spec/features/projects/members/owner_cannot_leave_project_spec.rb create mode 100644 spec/services/members/destroy_service_spec.rb (limited to 'spec') diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 89c2c26a367..c8601341d54 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -118,9 +118,7 @@ describe Groups::GroupMembersController do it 'cannot removes himself from the group' do delete :leave, group_id: group - expect(response).to redirect_to(group_path(group)) - expect(response).to set_flash[:alert].to "You can not leave the \"#{group.name}\" group. Transfer or delete the group." - expect(group.users).to include user + expect(response.status).to eq(403) end end @@ -134,7 +132,7 @@ describe Groups::GroupMembersController do delete :leave, group_id: group expect(response).to set_flash.to 'Your access request to the group has been withdrawn.' - expect(response).to redirect_to(dashboard_groups_path) + expect(response).to redirect_to(group_path(group)) expect(group.members.request).to be_empty expect(group.users).not_to include user end diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index fc5f458e795..e5e750c855f 100644 --- a/spec/controllers/projects/project_members_controller_spec.rb +++ b/spec/controllers/projects/project_members_controller_spec.rb @@ -171,11 +171,7 @@ describe Projects::ProjectMembersController do delete :leave, namespace_id: project.namespace, project_id: project - expect(response).to redirect_to( - namespace_project_path(project.namespace, project) - ) - expect(response).to set_flash[:alert].to "You can not leave the \"#{project.human_name}\" project. Transfer or delete the project." - expect(project.users).to include user + expect(response.status).to eq(403) end end @@ -190,7 +186,7 @@ describe Projects::ProjectMembersController do project_id: project expect(response).to set_flash.to 'Your access request to the project has been withdrawn.' - expect(response).to redirect_to(dashboard_projects_path) + expect(response).to redirect_to(namespace_project_path(project.namespace, project)) expect(project.members.request).to be_empty expect(project.users).not_to include user end diff --git a/spec/features/groups/members/last_owner_cannot_leave_group_spec.rb b/spec/features/groups/members/last_owner_cannot_leave_group_spec.rb new file mode 100644 index 00000000000..33bf6d3752f --- /dev/null +++ b/spec/features/groups/members/last_owner_cannot_leave_group_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +feature 'Groups > Members > Last owner cannot leave group', feature: true do + let(:owner) { create(:user) } + let(:group) { create(:group) } + + background do + group.add_owner(owner) + login_as(owner) + visit group_path(group) + end + + scenario 'user does not see a "Leave Group" link' do + expect(page).not_to have_content 'Leave Group' + end +end diff --git a/spec/features/groups/members/member_leaves_group_spec.rb b/spec/features/groups/members/member_leaves_group_spec.rb new file mode 100644 index 00000000000..3185ff924b9 --- /dev/null +++ b/spec/features/groups/members/member_leaves_group_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +feature 'Groups > Members > Member leaves group', feature: true do + let(:user) { create(:user) } + let(:owner) { create(:user) } + let(:group) { create(:group, :public) } + + background do + group.add_owner(owner) + group.add_developer(user) + login_as(user) + visit group_path(group) + end + + scenario 'user leaves group' do + click_link 'Leave Group' + + expect(current_path).to eq(dashboard_groups_path) + expect(group.users.exists?(user.id)).to be_falsey + end +end diff --git a/spec/features/groups/members/user_requests_access_spec.rb b/spec/features/groups/members/user_requests_access_spec.rb index a878a96b6ee..1ea607cbca0 100644 --- a/spec/features/groups/members/user_requests_access_spec.rb +++ b/spec/features/groups/members/user_requests_access_spec.rb @@ -21,6 +21,7 @@ feature 'Groups > Members > User requests access', feature: true do expect(page).to have_content 'Your request for access has been queued for review.' expect(page).to have_content 'Withdraw Access Request' + expect(page).not_to have_content 'Leave Group' end scenario 'user is not listed in the group members page' do diff --git a/spec/features/projects/members/member_leaves_project_spec.rb b/spec/features/projects/members/member_leaves_project_spec.rb new file mode 100644 index 00000000000..79dec442818 --- /dev/null +++ b/spec/features/projects/members/member_leaves_project_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +feature 'Projects > Members > Member leaves project', feature: true do + let(:user) { create(:user) } + let(:project) { create(:project) } + + background do + project.team << [user, :developer] + login_as(user) + visit namespace_project_path(project.namespace, project) + end + + scenario 'user leaves project' do + click_link 'Leave Project' + + expect(current_path).to eq(dashboard_projects_path) + expect(project.users.exists?(user.id)).to be_falsey + end +end diff --git a/spec/features/projects/members/owner_cannot_leave_project_spec.rb b/spec/features/projects/members/owner_cannot_leave_project_spec.rb new file mode 100644 index 00000000000..67811b1048e --- /dev/null +++ b/spec/features/projects/members/owner_cannot_leave_project_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +feature 'Projects > Members > Owner cannot leave project', feature: true do + let(:owner) { create(:user) } + let(:project) { create(:project) } + + background do + project.team << [owner, :owner] + login_as(owner) + visit namespace_project_path(project.namespace, project) + end + + scenario 'user does not see a "Leave Project" link' do + expect(page).not_to have_content 'Leave Project' + end +end diff --git a/spec/features/projects/members/user_requests_access_spec.rb b/spec/features/projects/members/user_requests_access_spec.rb index fd92a3a2f0c..af420c170ef 100644 --- a/spec/features/projects/members/user_requests_access_spec.rb +++ b/spec/features/projects/members/user_requests_access_spec.rb @@ -21,6 +21,7 @@ feature 'Projects > Members > User requests access', feature: true do expect(page).to have_content 'Your request for access has been queued for review.' expect(page).to have_content 'Withdraw Access Request' + expect(page).not_to have_content 'Leave Project' end scenario 'user is not listed in the project members page' do diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 9dd0378d165..6fa8298d489 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -70,22 +70,6 @@ feature 'Project', feature: true do end end - describe 'leave project link' do - let(:user) { create(:user) } - let(:project) { create(:project, namespace: user.namespace) } - - before do - login_with(user) - project.team.add_user(user, Gitlab::Access::MASTER) - visit namespace_project_path(project.namespace, project) - end - - it 'click project-settings and find leave project' do - find('#project-settings-button').click - expect(page).to have_link('Leave Project') - end - end - describe 'project title' do include WaitForAjax diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 3ed3202ac6c..e9134a3d283 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -134,18 +134,6 @@ 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) } diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index eeb74a462ac..18439cac2a4 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -61,16 +61,6 @@ describe GroupMember, models: true do end end - 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) - - expect_any_instance_of(NotificationService).to receive(:decline_group_access_request) - - member.__send__(:post_decline_request) - end - end - describe '#real_source_type' do subject { create(:group_member).real_source_type } diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 1e466f9c620..bbf65edb27c 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -152,15 +152,5 @@ describe ProjectMember, models: true do member.__send__(:after_accept_request) end end - - 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) - - expect_any_instance_of(NotificationService).to receive(:decline_project_access_request) - - member.__send__(:post_decline_request) - end - end end end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb new file mode 100644 index 00000000000..2395445e7fd --- /dev/null +++ b/spec/services/members/destroy_service_spec.rb @@ -0,0 +1,71 @@ +require 'spec_helper' + +describe Members::DestroyService, services: true do + let(:user) { create(:user) } + let(:project) { create(:project) } + let!(:member) { create(:project_member, source: project) } + + context 'when member is nil' do + before do + project.team << [user, :developer] + end + + it 'does not destroy the member' do + expect { destroy_member(nil, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + context 'when current user cannot destroy the given member' do + before do + project.team << [user, :developer] + end + + it 'does not destroy the member' do + expect { destroy_member(member, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + context 'when current user can destroy the given member' do + before do + project.team << [user, :master] + end + + it 'destroys the member' do + destroy_member(member, user) + + expect(member).to be_destroyed + end + + context 'when the given member is a requester' do + before do + member.update_column(:requested_at, Time.now) + end + + it 'calls Member#after_decline_request' do + expect_any_instance_of(NotificationService).to receive(:decline_access_request).with(member) + + destroy_member(member, user) + end + + context 'when current user is the member' do + it 'does not call Member#after_decline_request' do + expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) + + destroy_member(member, member.user) + end + end + + context 'when current user is the member and ' do + it 'does not call Member#after_decline_request' do + expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) + + destroy_member(member, member.user) + end + end + end + end + + def destroy_member(member, user) + Members::DestroyService.new(member, user).execute + end +end -- cgit v1.2.3