Commit 9f97fa4d authored by Mark Fletcher's avatar Mark Fletcher

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
parent 2f736c6d
...@@ -184,7 +184,8 @@ class IssuableBaseService < BaseService ...@@ -184,7 +184,8 @@ class IssuableBaseService < BaseService
old_labels = issuable.labels.to_a old_labels = issuable.labels.to_a
old_mentioned_users = issuable.mentioned_users.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) if params.present? && update_issuable(issuable, params)
# We do not touch as it will affect a update on updated_at field # We do not touch as it will affect a update on updated_at field
...@@ -201,6 +202,10 @@ class IssuableBaseService < BaseService ...@@ -201,6 +202,10 @@ class IssuableBaseService < BaseService
issuable issuable
end end
def labels_changing?(old_label_ids, new_label_ids)
old_label_ids.sort != new_label_ids.sort
end
def change_state(issuable) def change_state(issuable)
case params.delete(:state_event) case params.delete(:state_event)
when 'reopen' when 'reopen'
......
---
title: Ensure issuable state changes only fire webhooks once
merge_request:
author:
...@@ -376,5 +376,10 @@ describe Issues::UpdateService, services: true do ...@@ -376,5 +376,10 @@ describe Issues::UpdateService, services: true do
let(:mentionable) { issue } let(:mentionable) { issue }
include_examples 'updating mentions', Issues::UpdateService include_examples 'updating mentions', Issues::UpdateService
end end
include_examples 'issuable update service' do
let(:open_issuable) { issue }
let(:closed_issuable) { create(:closed_issue, project: project) }
end
end end
end end
...@@ -320,5 +320,10 @@ describe MergeRequests::UpdateService, services: true do ...@@ -320,5 +320,10 @@ describe MergeRequests::UpdateService, services: true do
expect(issue_ids).to be_empty expect(issue_ids).to be_empty
end end
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
end end
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment