From ec18943e42cf8040c3d0ee40aefd32c551788931 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 24 Nov 2017 19:59:05 +1100 Subject: [PATCH] feat(verification webhooks): alter logic to select only the relevant webhooks when the pact has changed --- .../20171118_create_webhook_events.rb | 2 +- lib/pact_broker/webhooks/repository.rb | 9 ++++++++ lib/pact_broker/webhooks/service.rb | 7 +++--- lib/pact_broker/webhooks/webhook_event.rb | 6 ++++- .../api/decorators/webhook_decorator_spec.rb | 2 +- .../api/resources/pact_webhooks_spec.rb | 17 +++++++------- .../pact_broker/webhooks/repository_spec.rb | 23 +++++++++++++++++++ spec/lib/pact_broker/webhooks/service_spec.rb | 8 ++++--- spec/support/test_data_builder.rb | 3 ++- 9 files changed, 58 insertions(+), 19 deletions(-) diff --git a/db/migrations/20171118_create_webhook_events.rb b/db/migrations/20171118_create_webhook_events.rb index acb4ea90c..3584d00a6 100644 --- a/db/migrations/20171118_create_webhook_events.rb +++ b/db/migrations/20171118_create_webhook_events.rb @@ -5,7 +5,7 @@ from(:webhooks).each do | webhook | from(:webhook_events).insert( webhook_id: webhook[:id], - name: 'contract_changed', + name: 'contract_content_changed', created_at: DateTime.now, updated_at: DateTime.now ) diff --git a/lib/pact_broker/webhooks/repository.rb b/lib/pact_broker/webhooks/repository.rb index 6c7bfac89..007eb01c3 100644 --- a/lib/pact_broker/webhooks/repository.rb +++ b/lib/pact_broker/webhooks/repository.rb @@ -63,6 +63,15 @@ def find_by_consumer_and_provider consumer, provider Webhook.where(consumer_id: consumer.id, provider_id: provider.id).collect(&:to_domain) end + def find_by_consumer_and_provider_and_event_name consumer, provider, event_name + Webhook + .select_all_qualified + .where(consumer_id: consumer.id, provider_id: provider.id) + .join(:webhook_events, { webhook_id: :id }) + .where(Sequel[:webhook_events][:name] => event_name) + .collect(&:to_domain) + end + def find_by_consumer_and_provider_existing_at consumer, provider, date_time Webhook.where(consumer_id: consumer.id, provider_id: provider.id) .where(Sequel.lit("created_at < ?", date_time)) diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index 6bfd7a251..df4c6d829 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -1,10 +1,11 @@ require 'pact_broker/repositories' require 'pact_broker/logging' -require 'pact_broker/webhooks/job' require 'base64' require 'securerandom' +require 'pact_broker/webhooks/job' require 'pact_broker/webhooks/triggered_webhook' require 'pact_broker/webhooks/status' +require 'pact_broker/webhooks/webhook_event' module PactBroker @@ -83,8 +84,8 @@ def self.find_by_consumer_and_provider consumer, provider webhook_repository.find_by_consumer_and_provider consumer, provider end - def self.execute_webhooks pact - webhooks = webhook_repository.find_by_consumer_and_provider pact.consumer, pact.provider + def self.execute_webhooks pact, event_name = PactBroker::Webhooks::WebhookEvent::DEFAULT_EVENT_NAME + webhooks = webhook_repository.find_by_consumer_and_provider_and_event_name pact.consumer, pact.provider, event_name if webhooks.any? run_later(webhooks, pact) diff --git a/lib/pact_broker/webhooks/webhook_event.rb b/lib/pact_broker/webhooks/webhook_event.rb index 4b473c3a7..1ccecdc3d 100644 --- a/lib/pact_broker/webhooks/webhook_event.rb +++ b/lib/pact_broker/webhooks/webhook_event.rb @@ -5,7 +5,11 @@ module PactBroker module Webhooks class WebhookEvent < Sequel::Model - DEFAULT_EVENT_NAME = 'contract_changed' + CONTRACT_CONTENT_CHANGED = 'contract_content_changed' + CONTRACT_VERIFIABLE_CONTENT_CHANGED = 'contract_verifiable_content_changed' + VERIFICATION_PUBLISHED = 'verification_published' + VERIFICATION_STATUS_CHANGED = 'verification_status_changed' + DEFAULT_EVENT_NAME = CONTRACT_CONTENT_CHANGED dataset_module do include PactBroker::Repositories::Helpers diff --git a/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb b/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb index 6f0aa317c..f28957315 100644 --- a/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb +++ b/spec/lib/pact_broker/api/decorators/webhook_decorator_spec.rb @@ -124,7 +124,7 @@ module Decorators it "defaults to a single contract_changed event for backwards compatibility" do expect(parsed_object.events.size).to eq 1 - expect(parsed_object.events.first.name).to eq 'contract_changed' + expect(parsed_object.events.first.name).to eq PactBroker::Webhooks::WebhookEvent::DEFAULT_EVENT_NAME end end end diff --git a/spec/lib/pact_broker/api/resources/pact_webhooks_spec.rb b/spec/lib/pact_broker/api/resources/pact_webhooks_spec.rb index a9e14a497..68b58ade0 100644 --- a/spec/lib/pact_broker/api/resources/pact_webhooks_spec.rb +++ b/spec/lib/pact_broker/api/resources/pact_webhooks_spec.rb @@ -13,12 +13,14 @@ module Resources let(:headers) { {'CONTENT_TYPE' => 'application/json'} } let(:webhook) { double('webhook')} let(:saved_webhook) { double('saved_webhook')} - let(:provider) { instance_double(PactBroker::Domain::Pacticipant)} - let(:consumer) { instance_double(PactBroker::Domain::Pacticipant)} + let(:provider) { instance_double(PactBroker::Domain::Pacticipant) } + let(:consumer) { instance_double(PactBroker::Domain::Pacticipant) } + let(:webhook_decorator) { instance_double(Decorators::WebhookDecorator, from_json: webhook) } before do allow(PactBroker::Pacticipants::Service).to receive(:find_pacticipant_by_name).with("Some Provider").and_return(provider) allow(PactBroker::Pacticipants::Service).to receive(:find_pacticipant_by_name).with("Some Consumer").and_return(consumer) + allow(Decorators::WebhookDecorator).to receive(:new).and_return(webhook_decorator) end describe "GET" do @@ -83,6 +85,7 @@ module Resources context "when the provider is not found" do let(:provider) { nil } + it "returns a 404 status" do subject expect(last_response.status).to eq 404 @@ -142,10 +145,10 @@ module Resources context "with valid attributes" do let(:webhook_response_json) { {some: 'webhook'}.to_json } - let(:decorator) { instance_double(Decorators::WebhookDecorator) } before do allow_any_instance_of(Decorators::WebhookDecorator).to receive(:to_json).and_return(webhook_response_json) + allow(webhook_decorator).to receive(:to_json).and_return(webhook_response_json) end it "saves the webhook" do @@ -169,9 +172,8 @@ module Resources end it "generates the JSON response body" do - allow(Decorators::WebhookDecorator).to receive(:new).and_call_original #Deserialise - expect(Decorators::WebhookDecorator).to receive(:new).with(saved_webhook).and_return(decorator) #Serialize - expect(decorator).to receive(:to_json).with(user_options: { base_url: 'http://example.org' }) + expect(Decorators::WebhookDecorator).to receive(:new).with(saved_webhook).and_return(webhook_decorator) + expect(webhook_decorator).to receive(:to_json).with(user_options: { base_url: 'http://example.org' }) subject end @@ -180,10 +182,7 @@ module Resources expect(last_response.body).to eq webhook_response_json end end - end - end end - end diff --git a/spec/lib/pact_broker/webhooks/repository_spec.rb b/spec/lib/pact_broker/webhooks/repository_spec.rb index bdca3c1ec..8d4532cd4 100644 --- a/spec/lib/pact_broker/webhooks/repository_spec.rb +++ b/spec/lib/pact_broker/webhooks/repository_spec.rb @@ -303,6 +303,29 @@ module Webhooks end end + describe "find_by_consumer_and_provider_and_event_name" do + let(:test_data_builder) { TestDataBuilder.new } + subject { Repository.new.find_by_consumer_and_provider_and_event_name test_data_builder.consumer, test_data_builder.provider, 'something_happened' } + + context "when a webhook exists with a matching consumer and provider and event name" do + + before do + test_data_builder + .create_consumer("Consumer") + .create_provider("Another Provider") + .create_webhook + .create_provider("Provider") + .create_webhook(uuid: '1', events: [{ name: 'something_happened' }]) + .create_webhook(uuid: '2', events: [{ name: 'something_happened' }]) + .create_webhook(uuid: '3', events: [{ name: 'something_else_happened' }]) + end + + it "returns an array of webhooks" do + expect(subject.collect(&:uuid).sort).to eq ['1', '2'] + end + end + end + describe "create_triggered_webhook" do before do td.create_consumer diff --git a/spec/lib/pact_broker/webhooks/service_spec.rb b/spec/lib/pact_broker/webhooks/service_spec.rb index a0d158a5e..c507bd6d5 100644 --- a/spec/lib/pact_broker/webhooks/service_spec.rb +++ b/spec/lib/pact_broker/webhooks/service_spec.rb @@ -37,7 +37,7 @@ module Webhooks let(:triggered_webhook) { instance_double(PactBroker::Webhooks::TriggeredWebhook) } before do - allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:find_by_consumer_and_provider).and_return(webhooks) + allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:find_by_consumer_and_provider_and_event_name).and_return(webhooks) allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:create_triggered_webhook).and_return(triggered_webhook) allow(Job).to receive(:perform_async) end @@ -45,7 +45,7 @@ module Webhooks subject { Service.execute_webhooks pact } it "finds the webhooks" do - expect_any_instance_of(PactBroker::Webhooks::Repository).to receive(:find_by_consumer_and_provider).with(consumer, provider) + expect_any_instance_of(PactBroker::Webhooks::Repository).to receive(:find_by_consumer_and_provider_and_event_name).with(consumer, provider, PactBroker::Webhooks::WebhookEvent::DEFAULT_EVENT_NAME) subject end @@ -126,12 +126,14 @@ module Webhooks to_return(:status => 200) end + let(:events) { [{ name: PactBroker::Webhooks::WebhookEvent::DEFAULT_EVENT_NAME }] } + let(:pact) do td.create_consumer .create_provider .create_consumer_version .create_pact - .create_webhook(method: 'GET', url: 'http://example.org') + .create_webhook(method: 'GET', url: 'http://example.org', events: events) .and_return(:pact) end diff --git a/spec/support/test_data_builder.rb b/spec/support/test_data_builder.rb index 479f9028a..0690eeef0 100644 --- a/spec/support/test_data_builder.rb +++ b/spec/support/test_data_builder.rb @@ -200,7 +200,8 @@ def revise_pact json_content = nil def create_webhook params = {} uuid = params[:uuid] || PactBroker::Webhooks::Service.next_uuid - events = (params[:events] || [{ name: 'pact_changed' }]).collect{ |e| PactBroker::Webhooks::WebhookEvent.new(e) } + event_params = params[:events] || [{ name: PactBroker::Webhooks::WebhookEvent::DEFAULT_EVENT_NAME }] + events = event_params.collect{ |e| PactBroker::Webhooks::WebhookEvent.new(e) } default_params = { method: 'POST', url: 'http://example.org', headers: {'Content-Type' => 'application/json'}} request = PactBroker::Domain::WebhookRequest.new(default_params.merge(params)) @webhook = PactBroker::Webhooks::Repository.new.create uuid, PactBroker::Domain::Webhook.new(request: request, events: events), @consumer, @provider