Merge branch '39601-create-issuable-destroy-service' into 'master'

Create issue and merge request destroy services

Closes #39601

See merge request gitlab-org/gitlab-ce!15604

8 files changed, 59 insertions, 5 deletions

diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 072dffaff7a..f6d9f88032f 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -54,7 +54,7 @@ module IssuableActions
   end
 
   def destroy
-    issuable.destroy
+    Issuable::DestroyService.new(project, current_user).execute(issuable)
     TodoService.new.destroy_issuable(issuable, current_user)
 
     name = issuable.human_class_name
diff --git a/app/models/issue.rb b/app/models/issue.rb
index a9863a50d84..d6ef58d150b 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -49,7 +49,6 @@ class Issue < ActiveRecord::Base
   scope :public_only, -> { where(confidential: false) }
 
   after_save :expire_etag_cache
-  after_commit :update_project_counter_caches, on: :destroy
 
   attr_spammable :title, spam_title: true
   attr_spammable :description, spam_description: true
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2753e4b16e5..e4d8f486c77 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -52,7 +52,6 @@ class MergeRequest < ActiveRecord::Base
 
   after_create :ensure_merge_request_diff, unless: :importing?
   after_update :reload_diff_if_branch_changed
-  after_commit :update_project_counter_caches, on: :destroy
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
diff --git a/app/services/issuable/destroy_service.rb b/app/services/issuable/destroy_service.rb
new file mode 100644
index 00000000000..0610b401213
--- /dev/null
+++ b/app/services/issuable/destroy_service.rb
@@ -0,0 +1,9 @@
+module Issuable
+  class DestroyService < IssuableBaseService
+    def execute(issuable)
+      if issuable.destroy
+        issuable.update_project_counter_caches
+      end
+    end
+  end
+end
diff --git a/changelogs/unreleased/39601-create-issuable-destroy-service.yml b/changelogs/unreleased/39601-create-issuable-destroy-service.yml
new file mode 100644
index 00000000000..b0463f02eba
--- /dev/null
+++ b/changelogs/unreleased/39601-create-issuable-destroy-service.yml
@@ -0,0 +1,5 @@
+---
+title: Create issuable destroy service
+merge_request: 15604
+author: George Andrinopoulos
+type: other
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 74dfd9f96de..e60e00d7956 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -255,7 +255,9 @@ module API
 
         authorize!(:destroy_issue, issue)
 
-        destroy_conditionally!(issue)
+        destroy_conditionally!(issue) do |issue|
+          Issuable::DestroyService.new(user_project, current_user).execute(issue)
+        end
       end
 
       desc 'List merge requests closing issue'  do
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 5b4642a2f57..d34886fca2e 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -167,7 +167,9 @@ module API
 
         authorize!(:destroy_merge_request, merge_request)
 
-        destroy_conditionally!(merge_request)
+        destroy_conditionally!(merge_request) do |merge_request|
+          Issuable::DestroyService.new(user_project, current_user).execute(merge_request)
+        end
       end
 
       params do
diff --git a/spec/services/issuable/destroy_service_spec.rb b/spec/services/issuable/destroy_service_spec.rb
new file mode 100644
index 00000000000..d74d98c6079
--- /dev/null
+++ b/spec/services/issuable/destroy_service_spec.rb
@@ -0,0 +1,38 @@
+require 'spec_helper'
+
+describe Issuable::DestroyService do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+
+  subject(:service) { described_class.new(project, user) }
+
+  describe '#execute' do
+    context 'when issuable is an issue' do
+      let!(:issue) { create(:issue, project: project, author: user) }
+
+      it 'destroys the issue' do
+        expect { service.execute(issue) }.to change { project.issues.count }.by(-1)
+      end
+
+      it 'updates open issues count cache' do
+        expect_any_instance_of(Projects::OpenIssuesCountService).to receive(:refresh_cache)
+
+        service.execute(issue)
+      end
+    end
+
+    context 'when issuable is a merge request' do
+      let!(:merge_request) { create(:merge_request, target_project: project, source_project: project, author: user) }
+
+      it 'destroys the merge request' do
+        expect { service.execute(merge_request) }.to change { project.merge_requests.count }.by(-1)
+      end
+
+      it 'updates open merge requests count cache' do
+        expect_any_instance_of(Projects::OpenMergeRequestsCountService).to receive(:refresh_cache)
+
+        service.execute(merge_request)
+      end
+    end
+  end
+end