New Members::DestroyService

This is to ensure we don't send unwanted notifications when deleting a
project. In other words, stop abusing AR callbacks and use services.

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

10 files changed, 88 insertions, 78 deletions

diff --git a/app/controllers/concerns/membership_actions.rb b/app/controllers/concerns/membership_actions.rb
index a24273fad0b..b1343576b3c 100644
--- a/app/controllers/concerns/membership_actions.rb
+++ b/app/controllers/concerns/membership_actions.rb
@@ -23,22 +23,24 @@ module MembershipActions
     @member = membershipable.members.find_by(user_id: current_user)
     return render_403 unless @member
 
+    @member = Members::DestroyService.new(@member, current_user).execute
+
     source_type = @member.real_source_type.humanize(capitalize: false)
 
-    if can?(current_user, action_member_permission(:destroy, @member), @member)
+    if @member.destroyed?
       notice =
         if @member.request?
           "Your access request to the #{source_type} has been withdrawn."
         else
           "You left the \"#{@member.source.human_name}\" #{source_type}."
         end
-      @member.destroy
 
       redirect_to [:dashboard, @member.real_source_type.tableize], notice: notice
     else
       if cannot_leave?
         alert = "You can not leave the \"#{@member.source.human_name}\" #{source_type}."
         alert << " Transfer or delete the #{source_type}."
+
         redirect_to polymorphic_url(membershipable), alert: alert
       else
         render_403
diff --git a/app/models/member.rb b/app/models/member.rb
index 4ee3f1bb5c2..c74a16367db 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -48,7 +48,6 @@ class Member < ActiveRecord::Base
   after_create :post_create_hook, unless: [:pending?, :importing?]
   after_update :post_update_hook, unless: [:pending?, :importing?]
   after_destroy :post_destroy_hook, unless: :pending?
-  after_destroy :post_decline_request, if: :request?
 
   delegate :name, :username, :email, to: :user, prefix: true
 
@@ -188,7 +187,7 @@ class Member < ActiveRecord::Base
   end
 
   def send_request
-    # override in subclass
+    notification_service.new_access_request(self)
   end
 
   def post_create_hook
@@ -215,10 +214,6 @@ class Member < ActiveRecord::Base
     post_create_hook
   end
 
-  def post_decline_request
-    # override in subclass
-  end
-
   def system_hook_service
     SystemHooksService.new
   end
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 363db877968..2f13d339c89 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -33,12 +33,6 @@ class GroupMember < Member
     super
   end
 
-  def send_request
-    notification_service.new_group_access_request(self)
-
-    super
-  end
-
   def post_create_hook
     notification_service.new_group_member(self)
 
@@ -64,10 +58,4 @@ class GroupMember < Member
 
     super
   end
-
-  def post_decline_request
-    notification_service.decline_group_access_request(self)
-
-    super
-  end
 end
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 250ee04fd1d..e9d3a82ba15 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -111,12 +111,6 @@ class ProjectMember < Member
     super
   end
 
-  def send_request
-    notification_service.new_project_access_request(self)
-
-    super
-  end
-
   def post_create_hook
     unless owner?
       event_service.join_project(self.project, self.user)
@@ -152,12 +146,6 @@ class ProjectMember < Member
     super
   end
 
-  def post_decline_request
-    notification_service.decline_project_access_request(self)
-
-    super
-  end
-
   def event_service
     EventCreateService.new
   end
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
new file mode 100644
index 00000000000..e32eb47b846
--- /dev/null
+++ b/app/services/members/destroy_service.rb
@@ -0,0 +1,33 @@
+module Members
+  class DestroyService < BaseService
+    attr_accessor :member, :current_user
+
+    def initialize(member, user)
+      @member, @current_user = member, user
+    end
+
+    def execute
+      if can?(current_user, "destroy_#{member.type.underscore}".to_sym, member)
+        member.destroy
+
+        notification_service.decline_access_request(member) if member.request?
+      end
+
+      member
+    end
+
+    private
+
+    def abilities
+      Ability.abilities
+    end
+
+    def can?(object, action, subject)
+      abilities.allowed?(object, action, subject)
+    end
+
+    def notification_service
+      NotificationService.new
+    end
+  end
+end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 19832a19b2b..590350a11e5 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -181,15 +181,16 @@ class NotificationService
     end
   end
 
-  # Project access request
-  def new_project_access_request(project_member)
-    mailer.member_access_requested_email(project_member.real_source_type, project_member.id).deliver_later
+  # Members
+  def new_access_request(member)
+    mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
   end
 
-  def decline_project_access_request(project_member)
-    mailer.member_access_denied_email(project_member.real_source_type, project_member.project.id, project_member.user.id).deliver_later
+  def decline_access_request(member)
+    mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later
   end
 
+  # Project invite
   def invite_project_member(project_member, token)
     mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later
   end
@@ -216,15 +217,7 @@ class NotificationService
     mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later
   end
 
-  # Group access request
-  def new_group_access_request(group_member)
-    mailer.member_access_requested_email(group_member.real_source_type, group_member.id).deliver_later
-  end
-
-  def decline_group_access_request(group_member)
-    mailer.member_access_denied_email(group_member.real_source_type, group_member.group.id, group_member.user.id).deliver_later
-  end
-
+  # Group invite
   def invite_group_member(group_member, token)
     mailer.member_invited_email(group_member.real_source_type, group_member.id, token).deliver_later
   end
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..aa002b4bd22
--- /dev/null
+++ b/spec/services/members/destroy_service_spec.rb
@@ -0,0 +1,43 @@
+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 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)).not_to be_destroyed
+    end
+  end
+
+  context 'when current user can destroy the given member' do
+    before do
+      project.team << [user, :master]
+    end
+
+    it 'destroys the member' do
+      expect(destroy_member(member, user)).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
+    end
+  end
+
+  def destroy_member(member, user)
+    Members::DestroyService.new(member, user).execute
+  end
+end