From 7266b1e6c4beff34a4b92d05c895335104890fec Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Tue, 5 Sep 2017 08:13:19 +1000 Subject: [PATCH] feat(webhook status): implement PUT for webhook resource --- lib/pact_broker/api/resources/webhook.rb | 34 ++++++++- lib/pact_broker/webhooks/execution.rb | 2 +- lib/pact_broker/webhooks/repository.rb | 10 +++ lib/pact_broker/webhooks/service.rb | 4 + lib/pact_broker/webhooks/webhook.rb | 27 +++++-- spec/features/edit_webhook_spec.rb | 61 +++++++++++++++ .../pact_broker/api/resources/webhook_spec.rb | 74 +++++++++++-------- .../pact_broker/webhooks/repository_spec.rb | 45 +++++++++++ 8 files changed, 214 insertions(+), 43 deletions(-) create mode 100644 spec/features/edit_webhook_spec.rb diff --git a/lib/pact_broker/api/resources/webhook.rb b/lib/pact_broker/api/resources/webhook.rb index 31959ea5d..dab908552 100644 --- a/lib/pact_broker/api/resources/webhook.rb +++ b/lib/pact_broker/api/resources/webhook.rb @@ -7,18 +7,38 @@ module Resources class Webhook < BaseResource + def content_types_accepted + [["application/json", :from_json]] + end + def content_types_provided [["application/hal+json", :to_json]] end def allowed_methods - ["GET", "DELETE"] + ["GET", "PUT", "DELETE"] end def resource_exists? webhook end + def malformed_request? + if request.put? + return invalid_json? || validation_errors?(webhook) + end + false + end + + def from_json + if webhook + @webhook = webhook_service.update_by_uuid uuid, new_webhook + response.body = to_json + else + 404 + end + end + def to_json Decorators::WebhookDecorator.new(webhook).to_json(user_options: { base_url: base_url }) end @@ -30,16 +50,24 @@ def delete_resource private + def validation_errors? webhook + errors = webhook_service.errors(new_webhook) + set_json_validation_error_messages(errors.messages) if !errors.empty? + !errors.empty? + end + def webhook @webhook ||= webhook_service.find_by_uuid uuid end + def new_webhook + @new_webhook ||= Decorators::WebhookDecorator.new(PactBroker::Domain::Webhook.new).from_json(request_body) + end + def uuid identifier_from_path[:uuid] end - end end - end end diff --git a/lib/pact_broker/webhooks/execution.rb b/lib/pact_broker/webhooks/execution.rb index c612f9a18..8498d84dc 100644 --- a/lib/pact_broker/webhooks/execution.rb +++ b/lib/pact_broker/webhooks/execution.rb @@ -5,7 +5,7 @@ module PactBroker module Webhooks - class Execution < Sequel::Model(::DB::PACT_BROKER_DB[:webhook_executions].select(Sequel[:webhook_executions][:id], :triggered_webhook_id, :success, :logs)) + class Execution < Sequel::Model(PactBroker::DB.connection[:webhook_executions].select(Sequel[:webhook_executions][:id], :triggered_webhook_id, :success, :logs)) dataset_module do include PactBroker::Repositories::Helpers diff --git a/lib/pact_broker/webhooks/repository.rb b/lib/pact_broker/webhooks/repository.rb index ba77237d0..c817ff42a 100644 --- a/lib/pact_broker/webhooks/repository.rb +++ b/lib/pact_broker/webhooks/repository.rb @@ -28,6 +28,16 @@ def find_by_uuid uuid Webhook.where(uuid: uuid).limit(1).collect(&:to_domain)[0] end + def update_by_uuid uuid, webhook + existing_webhook = Webhook.find(uuid: uuid) + existing_webhook.update_from_domain(webhook).save + existing_webhook.headers.collect(&:delete) + webhook.request.headers.each_pair do | name, value | + existing_webhook.add_header PactBroker::Webhooks::WebhookHeader.from_domain(name, value, existing_webhook.id) + end + find_by_uuid uuid + end + def delete_by_uuid uuid Webhook.where(uuid: uuid).destroy end diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index e5bffb6d0..2e59e23bd 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -35,6 +35,10 @@ def self.find_by_uuid uuid webhook_repository.find_by_uuid uuid end + def self.update_by_uuid uuid, webhook + webhook_repository.update_by_uuid uuid, webhook + end + def self.delete_by_uuid uuid webhook_repository.unlink_triggered_webhooks_by_webhook_uuid uuid webhook_repository.delete_by_uuid uuid diff --git a/lib/pact_broker/webhooks/webhook.rb b/lib/pact_broker/webhooks/webhook.rb index 2c0689e37..0584cc4c9 100644 --- a/lib/pact_broker/webhooks/webhook.rb +++ b/lib/pact_broker/webhooks/webhook.rb @@ -15,16 +15,13 @@ def before_destroy WebhookHeader.where(webhook_id: id).destroy end + def update_from_domain webhook + set(self.class.properties_hash_from_domain(webhook)) + end + def self.from_domain webhook, consumer, provider - is_json_request_body = !(String === webhook.request.body || webhook.request.body.nil?) # Can't rely on people to set content type new( - uuid: webhook.uuid, - method: webhook.request.method, - url: webhook.request.url, - username: webhook.request.username, - password: not_plain_text_password(webhook.request.password), - body: (is_json_request_body ? webhook.request.body.to_json : webhook.request.body), - is_json_request_body: is_json_request_body + properties_hash_from_domain(webhook).merge(uuid: webhook.uuid) ).tap do | db_webhook | db_webhook.consumer_id = consumer.id db_webhook.provider_id = provider.id @@ -67,6 +64,20 @@ def parsed_body end end + private + + def self.properties_hash_from_domain webhook + is_json_request_body = !(String === webhook.request.body || webhook.request.body.nil?) # Can't rely on people to set content type + { + method: webhook.request.method, + url: webhook.request.url, + username: webhook.request.username, + password: not_plain_text_password(webhook.request.password), + body: (is_json_request_body ? webhook.request.body.to_json : webhook.request.body), + is_json_request_body: is_json_request_body + } + end + end Webhook.plugin :timestamps, :update_on_create=>true diff --git a/spec/features/edit_webhook_spec.rb b/spec/features/edit_webhook_spec.rb new file mode 100644 index 000000000..95d499091 --- /dev/null +++ b/spec/features/edit_webhook_spec.rb @@ -0,0 +1,61 @@ +require 'support/test_data_builder' + +describe "Creating a webhook" do + + let(:td) { TestDataBuilder.new } + + before do + td.create_pact_with_hierarchy("Some Consumer", "1", "Some Provider") + .create_webhook + end + + let(:path) { "/webhooks/#{td.webhook.uuid}" } + let(:headers) { {'CONTENT_TYPE' => 'application/json'} } + let(:response_body) { JSON.parse(last_response.body, symbolize_names: true)} + let(:webhook_json) do + h = load_json_fixture('webhook_valid.json') + h['request']['method'] = 'GET' + h.to_json + end + + let(:reloaded_webhook) { PactBroker::Webhooks::Repository.new.find_by_uuid(td.webhook.uuid) } + + subject { put path, webhook_json, headers; last_response } + + context "with invalid attributes" do + let(:webhook_json) { '{}' } + + it "returns a 400" do + subject + expect(last_response.status).to be 400 + end + + it "returns the validation errors" do + subject + expect(response_body[:errors]).to_not be_empty + end + + it "does not update the webhook" do + expect(reloaded_webhook.request.method).to eq 'POST' + end + end + + context "with valid attributes" do + let(:webhook_hash) { JSON.parse(webhook_json, symbolize_names: true) } + + it "returns a 200 response" do + subject + expect(last_response.status).to be 200 + end + + it "returns a JSON Content Type" do + subject + expect(last_response.headers['Content-Type']).to eq 'application/hal+json;charset=utf-8' + end + + it "updates the webhook" do + subject + expect(reloaded_webhook.request.method).to eq 'GET' + end + end +end diff --git a/spec/lib/pact_broker/api/resources/webhook_spec.rb b/spec/lib/pact_broker/api/resources/webhook_spec.rb index 6cbd9bb45..cffd3966c 100644 --- a/spec/lib/pact_broker/api/resources/webhook_spec.rb +++ b/spec/lib/pact_broker/api/resources/webhook_spec.rb @@ -11,50 +11,62 @@ module Resources allow(PactBroker::Webhooks::Service).to receive(:find_by_uuid).and_return(webhook) end - subject { get '/webhooks/some-uuid' } + describe "GET" do + subject { get '/webhooks/some-uuid'; last_response } - context "when the webhook does not exist" do - let(:webhook) { nil } + context "when the webhook does not exist" do + let(:webhook) { nil } - it "returns a 404" do - subject - expect(last_response).to be_a_404_response + it "returns a 404" do + expect(subject).to be_a_404_response + end end - end - context "when the webhook exists" do + context "when the webhook exists" do - let(:webhook) { double("webhook") } - let(:decorator) { double(Decorators::WebhookDecorator, to_json: json)} - let(:json) { {some: 'json'}.to_json } + let(:webhook) { double("webhook") } + let(:decorator) { double(Decorators::WebhookDecorator, to_json: json)} + let(:json) { {some: 'json'}.to_json } - before do - allow(Decorators::WebhookDecorator).to receive(:new).and_return(decorator) - end + before do + allow(Decorators::WebhookDecorator).to receive(:new).and_return(decorator) + end - it "finds the webhook by UUID" do - expect(PactBroker::Webhooks::Service).to receive(:find_by_uuid).with('some-uuid') - subject - end + it "finds the webhook by UUID" do + expect(PactBroker::Webhooks::Service).to receive(:find_by_uuid).with('some-uuid') + subject + end - it "returns a 200 JSON response" do - subject - expect(last_response).to be_a_hal_json_success_response - end + it "returns a 200 JSON response" do + subject + expect(last_response).to be_a_hal_json_success_response + end - it "generates a JSON representation of the webhook" do - expect(Decorators::WebhookDecorator).to receive(:new).with(webhook) - expect(decorator).to receive(:to_json).with(user_options: { base_url: 'http://example.org'}) - subject - end + it "generates a JSON representation of the webhook" do + expect(Decorators::WebhookDecorator).to receive(:new).with(webhook) + expect(decorator).to receive(:to_json).with(user_options: { base_url: 'http://example.org'}) + subject + end - it "includes the JSON representation in the response body" do - subject - expect(last_response.body).to eq json + it "includes the JSON representation in the response body" do + subject + expect(last_response.body).to eq json + end end end + describe "PUT" do + context "when the webhook does not exist" do + let(:webhook) { nil } + let(:webhook_json) { load_fixture('webhook_valid.json') } + + subject { put '/webhooks/some-uuid', webhook_json, {'CONTENT_TYPE' => 'application/json'}; last_response } + + it "returns a 404" do + expect(subject).to be_a_404_response + 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 b9350f3cf..d513edf1e 100644 --- a/spec/lib/pact_broker/webhooks/repository_spec.rb +++ b/spec/lib/pact_broker/webhooks/repository_spec.rb @@ -188,6 +188,51 @@ module Webhooks end + describe "update_by_uuid" do + let(:uuid) { '1234' } + let(:td) { TestDataBuilder.new } + let(:old_webhook_params) do + { + uuid: uuid, + method: 'POST', + url: 'http://example.org', + body: '{"foo":1}', + headers: {'Content-Type' => 'application/json'}, + username: 'username', + password: 'password' + } + end + let(:new_webhook_params) do + { + method: 'GET', + url: 'http://example.com', + body: 'foo', + headers: {'Content-Type' => 'text/plain'} + } + end + before do + td.create_consumer + .create_provider + .create_webhook(old_webhook_params) + end + let(:new_webhook) do + PactBroker::Domain::Webhook.new(request: PactBroker::Domain::WebhookRequest.new(new_webhook_params)) + end + + subject { Repository.new.update_by_uuid uuid, new_webhook } + + it "updates the webhook" do + updated_webhook = subject + expect(updated_webhook.uuid).to eq uuid + expect(updated_webhook.request.method).to eq 'GET' + expect(updated_webhook.request.url).to eq 'http://example.com' + expect(updated_webhook.request.body).to eq 'foo' + expect(updated_webhook.request.headers).to eq 'Content-Type' => 'text/plain' + expect(updated_webhook.request.username).to eq nil + expect(updated_webhook.request.password).to eq nil + end + end + describe "find_all" do before do Repository.new.create uuid, webhook, consumer, provider