From 160ae6d1fea681163220d6639c13a5f216a23f68 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Wed, 5 Dec 2018 11:51:48 -0500 Subject: [PATCH 1/8] Add new hooks for all approvals and unapprovals, not just when the threshold is met. The new hooks are using 'approval' and 'unapproval' to ensure backwards compatibility. --- ee/app/services/merge_requests/approval_service.rb | 2 ++ ee/app/services/merge_requests/remove_approval_service.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/ee/app/services/merge_requests/approval_service.rb b/ee/app/services/merge_requests/approval_service.rb index c09628155c5f0c..b2177bdf0e3107 100644 --- a/ee/app/services/merge_requests/approval_service.rb +++ b/ee/app/services/merge_requests/approval_service.rb @@ -12,6 +12,8 @@ def execute(merge_request) if merge_request.approvals_left.zero? notification_service.async.approve_mr(merge_request, current_user) execute_hooks(merge_request, 'approved') + else + execute_hooks(merge_request, 'approval') end end end diff --git a/ee/app/services/merge_requests/remove_approval_service.rb b/ee/app/services/merge_requests/remove_approval_service.rb index b4d31f7dccd817..20ea8f9d980c27 100644 --- a/ee/app/services/merge_requests/remove_approval_service.rb +++ b/ee/app/services/merge_requests/remove_approval_service.rb @@ -18,6 +18,8 @@ def execute(merge_request) notification_service.async.unapprove_mr(merge_request, current_user) execute_hooks(merge_request, 'unapproved') end + else + execute_hooks(merge_request, 'unapproval') end end # rubocop: enable CodeReuse/ActiveRecord -- GitLab From 6ee763833be3069d2c55c5bd3edc028be37fe181 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Wed, 12 Dec 2018 15:04:18 +0000 Subject: [PATCH 2/8] Correct tabing --- ee/app/services/merge_requests/approval_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/merge_requests/approval_service.rb b/ee/app/services/merge_requests/approval_service.rb index 7cd8f45cc73b22..3bad19262e0c60 100644 --- a/ee/app/services/merge_requests/approval_service.rb +++ b/ee/app/services/merge_requests/approval_service.rb @@ -14,7 +14,7 @@ def execute(merge_request) if merge_request.approvals_left.zero? notification_service.async.approve_mr(merge_request, current_user) execute_hooks(merge_request, 'approved') - else + else execute_hooks(merge_request, 'approval') end end -- GitLab From 141dd643fcac625709f36c160ff5714fbaa067d8 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Wed, 12 Dec 2018 15:04:55 +0000 Subject: [PATCH 3/8] Correct tabbing --- ee/app/services/merge_requests/remove_approval_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/merge_requests/remove_approval_service.rb b/ee/app/services/merge_requests/remove_approval_service.rb index fc0e6be1b177fd..7cb83ab3bdccd2 100644 --- a/ee/app/services/merge_requests/remove_approval_service.rb +++ b/ee/app/services/merge_requests/remove_approval_service.rb @@ -21,7 +21,7 @@ def execute(merge_request) execute_hooks(merge_request, 'unapproved') end else - execute_hooks(merge_request, 'unapproval') + execute_hooks(merge_request, 'unapproval') end end # rubocop: enable CodeReuse/ActiveRecord -- GitLab From 248b5f1827fdbb0f84331c07211b08ce5c88b1ab Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Wed, 12 Dec 2018 11:08:06 -0500 Subject: [PATCH 4/8] Added changelog and updated specs --- .../unreleased/approval-unapproval-webhooks.yml | 6 ++++++ .../unreleased/approval-unapproval-webhooks.yml | 5 +++++ .../services/merge_requests/approval_service_spec.rb | 6 +++--- .../merge_requests/remove_approval_service_spec.rb | 12 ++++++++++++ 4 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/approval-unapproval-webhooks.yml create mode 100644 ee/changelogs/unreleased/approval-unapproval-webhooks.yml diff --git a/changelogs/unreleased/approval-unapproval-webhooks.yml b/changelogs/unreleased/approval-unapproval-webhooks.yml new file mode 100644 index 00000000000000..a276359a15f34a --- /dev/null +++ b/changelogs/unreleased/approval-unapproval-webhooks.yml @@ -0,0 +1,6 @@ +--- +title: "Add new webhook for approval and unapproval outside of approval threshold" +merge_request: +author: Zachary Brady +type: added + diff --git a/ee/changelogs/unreleased/approval-unapproval-webhooks.yml b/ee/changelogs/unreleased/approval-unapproval-webhooks.yml new file mode 100644 index 00000000000000..34074bbbb71e4c --- /dev/null +++ b/ee/changelogs/unreleased/approval-unapproval-webhooks.yml @@ -0,0 +1,5 @@ +--- +title: Add approval and unapproval webhooks +merge_request: 8742 +author: +type: added diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb index 97aa9337362eb2..95a09480feeb7b 100644 --- a/ee/spec/services/merge_requests/approval_service_spec.rb +++ b/ee/spec/services/merge_requests/approval_service_spec.rb @@ -51,9 +51,9 @@ end context 'with remaining approvals' do - it 'does not fire a webhook' do + it 'fires a approval webhook' do expect(merge_request).to receive(:approvals_left).and_return(5) - expect(service).not_to receive(:execute_hooks) + expect(service).to receive(:execute_hooks).with(merge_request, 'approval') service.execute(merge_request) end @@ -75,7 +75,7 @@ allow(service).to receive(:notification_service).and_return(notification_service) end - it 'fires a webhook' do + it 'fires a approved webhook' do expect(service).to receive(:execute_hooks).with(merge_request, 'approved') service.execute(merge_request) diff --git a/ee/spec/services/merge_requests/remove_approval_service_spec.rb b/ee/spec/services/merge_requests/remove_approval_service_spec.rb index c8bde170cc7439..974a4476a7b098 100644 --- a/ee/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/ee/spec/services/merge_requests/remove_approval_service_spec.rb @@ -31,6 +31,12 @@ def execute! execute! end + it 'fires a unapproval webhook' do + expect(service).to receive(:execute_hooks).with(merge_request, 'unapproval') + + execute! + end + it 'does not send a notification' do expect(service).not_to receive(:notification_service) @@ -52,6 +58,12 @@ def execute! allow(service).to receive(:notification_service).and_return(notification_service) end + it 'fires a unapproved webhook' do + expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved') + + execute! + end + it 'sends a notification' do expect(notification_service).to receive_message_chain(:async, :unapprove_mr).with(merge_request, user) -- GitLab From 9c50021ba420a7897c05bb6f31389ffcf6fe9c89 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Wed, 12 Dec 2018 11:23:25 -0500 Subject: [PATCH 5/8] Removed CE changelog as this is an EE only change. --- changelogs/unreleased/approval-unapproval-webhooks.yml | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 changelogs/unreleased/approval-unapproval-webhooks.yml diff --git a/changelogs/unreleased/approval-unapproval-webhooks.yml b/changelogs/unreleased/approval-unapproval-webhooks.yml deleted file mode 100644 index a276359a15f34a..00000000000000 --- a/changelogs/unreleased/approval-unapproval-webhooks.yml +++ /dev/null @@ -1,6 +0,0 @@ ---- -title: "Add new webhook for approval and unapproval outside of approval threshold" -merge_request: -author: Zachary Brady -type: added - -- GitLab From 122da99a5dbe4a477b5ca55fc66363c69eb63688 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Wed, 12 Dec 2018 14:24:02 -0500 Subject: [PATCH 6/8] Updated to handle both not destroy all and not currently approved --- .../services/merge_requests/remove_approval_service.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ee/app/services/merge_requests/remove_approval_service.rb b/ee/app/services/merge_requests/remove_approval_service.rb index 7cb83ab3bdccd2..fdbefa0b6953f7 100644 --- a/ee/app/services/merge_requests/remove_approval_service.rb +++ b/ee/app/services/merge_requests/remove_approval_service.rb @@ -13,13 +13,12 @@ def execute(merge_request) if approval.destroy_all # rubocop: disable DestroyAll merge_request.reset_approval_cache! - create_note(merge_request) + end - if currently_approved - notification_service.async.unapprove_mr(merge_request, current_user) - execute_hooks(merge_request, 'unapproved') - end + if currently_approved + notification_service.async.unapprove_mr(merge_request, current_user) + execute_hooks(merge_request, 'unapproved') else execute_hooks(merge_request, 'unapproval') end -- GitLab From 3192a5a89587421eaeca837dee15b8a1a2e92756 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Mon, 14 Jan 2019 15:39:21 -0500 Subject: [PATCH 7/8] Updated to address comments on merge request modified: ../../../app/services/merge_requests/remove_approval_service.rb modified: approval_service_spec.rb modified: remove_approval_service_spec.rb --- .../merge_requests/remove_approval_service.rb | 12 ++++++------ .../services/merge_requests/approval_service_spec.rb | 4 ++-- .../merge_requests/remove_approval_service_spec.rb | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ee/app/services/merge_requests/remove_approval_service.rb b/ee/app/services/merge_requests/remove_approval_service.rb index fdbefa0b6953f7..2a510cbb1bb8ae 100644 --- a/ee/app/services/merge_requests/remove_approval_service.rb +++ b/ee/app/services/merge_requests/remove_approval_service.rb @@ -14,14 +14,14 @@ def execute(merge_request) if approval.destroy_all # rubocop: disable DestroyAll merge_request.reset_approval_cache! create_note(merge_request) + if currently_approved + notification_service.async.unapprove_mr(merge_request, current_user) + execute_hooks(merge_request, 'unapproved') + else + execute_hooks(merge_request, 'unapproval') + end end - if currently_approved - notification_service.async.unapprove_mr(merge_request, current_user) - execute_hooks(merge_request, 'unapproved') - else - execute_hooks(merge_request, 'unapproval') - end end # rubocop: enable CodeReuse/ActiveRecord diff --git a/ee/spec/services/merge_requests/approval_service_spec.rb b/ee/spec/services/merge_requests/approval_service_spec.rb index 95a09480feeb7b..a971c146d0cc25 100644 --- a/ee/spec/services/merge_requests/approval_service_spec.rb +++ b/ee/spec/services/merge_requests/approval_service_spec.rb @@ -51,7 +51,7 @@ end context 'with remaining approvals' do - it 'fires a approval webhook' do + it 'fires an approval webhook' do expect(merge_request).to receive(:approvals_left).and_return(5) expect(service).to receive(:execute_hooks).with(merge_request, 'approval') @@ -75,7 +75,7 @@ allow(service).to receive(:notification_service).and_return(notification_service) end - it 'fires a approved webhook' do + it 'fires an approved webhook' do expect(service).to receive(:execute_hooks).with(merge_request, 'approved') service.execute(merge_request) diff --git a/ee/spec/services/merge_requests/remove_approval_service_spec.rb b/ee/spec/services/merge_requests/remove_approval_service_spec.rb index 974a4476a7b098..c0088e27a658f8 100644 --- a/ee/spec/services/merge_requests/remove_approval_service_spec.rb +++ b/ee/spec/services/merge_requests/remove_approval_service_spec.rb @@ -31,7 +31,7 @@ def execute! execute! end - it 'fires a unapproval webhook' do + it 'fires an unapproval webhook' do expect(service).to receive(:execute_hooks).with(merge_request, 'unapproval') execute! @@ -58,7 +58,7 @@ def execute! allow(service).to receive(:notification_service).and_return(notification_service) end - it 'fires a unapproved webhook' do + it 'fires an unapproved webhook' do expect(service).to receive(:execute_hooks).with(merge_request, 'unapproved') execute! -- GitLab From 600854b0dfd41924f34a2e1a1afcb721d26a58c6 Mon Sep 17 00:00:00 2001 From: Zachary Brady Date: Thu, 21 Mar 2019 08:41:26 -0400 Subject: [PATCH 8/8] Making the static-analysis happy modified: ee/app/services/merge_requests/remove_approval_service.rb --- ee/app/services/merge_requests/remove_approval_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/merge_requests/remove_approval_service.rb b/ee/app/services/merge_requests/remove_approval_service.rb index 2a510cbb1bb8ae..a353a25922e223 100644 --- a/ee/app/services/merge_requests/remove_approval_service.rb +++ b/ee/app/services/merge_requests/remove_approval_service.rb @@ -14,6 +14,7 @@ def execute(merge_request) if approval.destroy_all # rubocop: disable DestroyAll merge_request.reset_approval_cache! create_note(merge_request) + if currently_approved notification_service.async.unapprove_mr(merge_request, current_user) execute_hooks(merge_request, 'unapproved') @@ -21,7 +22,6 @@ def execute(merge_request) execute_hooks(merge_request, 'unapproval') end end - end # rubocop: enable CodeReuse/ActiveRecord -- GitLab