From 9f97fa4d9f4e86e8f1ff1db4621bcf81390936da Mon Sep 17 00:00:00 2001
From: Mark Fletcher <mark@gitlab.com>
Date: Wed, 14 Dec 2016 20:45:39 +0000
Subject: [PATCH] Ensure issuable state changes only fire webhooks once

* Webhooks for close and reopen events now fired in respective services only
* Prevents generic 'update' webhooks firing too
---
 app/services/issuable_base_service.rb           |  7 ++++++-
 ...ooks-fired-for-issue-closed-and-reopened.yml |  4 ++++
 spec/services/issues/update_service_spec.rb     |  5 +++++
 .../merge_requests/update_service_spec.rb       |  5 +++++
 .../issuable_update_service_shared_examples.rb  | 17 +++++++++++++++++
 5 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/25339-2-webhooks-fired-for-issue-closed-and-reopened.yml
 create mode 100644 spec/support/services/issuable_update_service_shared_examples.rb

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index b5f63cc5a1a..ab3d2a9a0cd 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -184,7 +184,8 @@ class IssuableBaseService < BaseService
     old_labels = issuable.labels.to_a
     old_mentioned_users = issuable.mentioned_users.to_a
 
-    params[:label_ids] = process_label_ids(params, existing_label_ids: issuable.label_ids)
+    label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)
+    params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids)
 
     if params.present? && update_issuable(issuable, params)
       # We do not touch as it will affect a update on updated_at field
@@ -201,6 +202,10 @@ class IssuableBaseService < BaseService
     issuable
   end
 
+  def labels_changing?(old_label_ids, new_label_ids)
+    old_label_ids.sort != new_label_ids.sort
+  end
+
   def change_state(issuable)
     case params.delete(:state_event)
     when 'reopen'
diff --git a/changelogs/unreleased/25339-2-webhooks-fired-for-issue-closed-and-reopened.yml b/changelogs/unreleased/25339-2-webhooks-fired-for-issue-closed-and-reopened.yml
new file mode 100644
index 00000000000..b12eab26b67
--- /dev/null
+++ b/changelogs/unreleased/25339-2-webhooks-fired-for-issue-closed-and-reopened.yml
@@ -0,0 +1,4 @@
+---
+title: Ensure issuable state changes only fire webhooks once
+merge_request: 
+author: 
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 500d224ff98..eafbea46905 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -376,5 +376,10 @@ describe Issues::UpdateService, services: true do
       let(:mentionable) { issue }
       include_examples 'updating mentions', Issues::UpdateService
     end
+
+    include_examples 'issuable update service' do
+      let(:open_issuable) { issue }
+      let(:closed_issuable) { create(:closed_issue, project: project) }
+    end
   end
 end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 790ef765f3a..88c786947d3 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -320,5 +320,10 @@ describe MergeRequests::UpdateService, services: true do
         expect(issue_ids).to be_empty
       end
     end
+
+    include_examples 'issuable update service' do
+      let(:open_issuable) { merge_request }
+      let(:closed_issuable) { create(:closed_merge_request, source_project: project) }
+    end
   end
 end
diff --git a/spec/support/services/issuable_update_service_shared_examples.rb b/spec/support/services/issuable_update_service_shared_examples.rb
new file mode 100644
index 00000000000..a3336755773
--- /dev/null
+++ b/spec/support/services/issuable_update_service_shared_examples.rb
@@ -0,0 +1,17 @@
+shared_examples 'issuable update service' do
+  context 'changing state' do
+    before { expect(project).to receive(:execute_hooks).once }
+
+    context 'to reopened' do
+      it 'executes hooks only once' do
+        described_class.new(project, user, state_event: 'reopen').execute(closed_issuable)
+      end
+    end
+
+    context 'to closed' do
+      it 'executes hooks only once' do
+        described_class.new(project, user, state_event: 'close').execute(open_issuable)
+      end
+    end
+  end
+end