From 47071e23a14c68a10de73747aaedec7ab61d160c Mon Sep 17 00:00:00 2001 From: Martin Artnik Date: Thu, 17 Jun 2021 14:54:30 +0200 Subject: [PATCH] Added specs for webhooks --- .../users/settings/webhooks_controller.rb | 2 +- ...hooks_jobs.rb => dispatch_webhooks_job.rb} | 2 +- app/jobs/activities/send_webhook_job.rb | 2 +- app/models/activity.rb | 2 +- app/models/webhook.rb | 4 +- .../activities/activity_webhook_service.rb | 21 +++++++ app/services/activities/webhook_service.rb | 56 ------------------ app/services/webhook_service.rb | 59 +++++++++++++++++++ .../settings/webhooks/_webhook_form.html.erb | 2 +- .../users/settings/webhooks/index.html.erb | 2 +- ...0617111749_rename_webhook_method_column.rb | 5 ++ db/structure.sql | 5 +- spec/factories/activity_filters.rb | 8 +++ spec/factories/webhooks.rb | 9 +++ .../activities/dispatch_webhooks_job_spec.rb | 25 ++++++++ spec/jobs/activities/send_webhook_job_spec.rb | 14 +++++ spec/models/activity_spec.rb | 12 ++++ spec/services/webhook_service_spec.rb | 57 ++++++++++++++++++ 18 files changed, 221 insertions(+), 66 deletions(-) rename app/jobs/activities/{dispatch_webhooks_jobs.rb => dispatch_webhooks_job.rb} (89%) create mode 100644 app/services/activities/activity_webhook_service.rb delete mode 100644 app/services/activities/webhook_service.rb create mode 100644 app/services/webhook_service.rb create mode 100644 db/migrate/20210617111749_rename_webhook_method_column.rb create mode 100644 spec/factories/activity_filters.rb create mode 100644 spec/factories/webhooks.rb create mode 100644 spec/jobs/activities/dispatch_webhooks_job_spec.rb create mode 100644 spec/jobs/activities/send_webhook_job_spec.rb create mode 100644 spec/services/webhook_service_spec.rb diff --git a/app/controllers/users/settings/webhooks_controller.rb b/app/controllers/users/settings/webhooks_controller.rb index b08ea3acd..ed1733ec2 100644 --- a/app/controllers/users/settings/webhooks_controller.rb +++ b/app/controllers/users/settings/webhooks_controller.rb @@ -72,7 +72,7 @@ module Users end def webhook_params - params.require(:webhook).permit(:method, :url, :active) + params.require(:webhook).permit(:http_method, :url, :active) end def load_filter_elements(filter) diff --git a/app/jobs/activities/dispatch_webhooks_jobs.rb b/app/jobs/activities/dispatch_webhooks_job.rb similarity index 89% rename from app/jobs/activities/dispatch_webhooks_jobs.rb rename to app/jobs/activities/dispatch_webhooks_job.rb index 19f5f8810..610602487 100644 --- a/app/jobs/activities/dispatch_webhooks_jobs.rb +++ b/app/jobs/activities/dispatch_webhooks_job.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Activities - class DispatchWebhooksJobs < ApplicationJob + class DispatchWebhooksJob < ApplicationJob queue_as :high_priority def perform(activity) diff --git a/app/jobs/activities/send_webhook_job.rb b/app/jobs/activities/send_webhook_job.rb index a7dbf3bcb..73c5969dc 100644 --- a/app/jobs/activities/send_webhook_job.rb +++ b/app/jobs/activities/send_webhook_job.rb @@ -5,7 +5,7 @@ module Activities queue_as :high_priority def perform(webhook, activity) - Activities::WebhookService.new(webhook, activity).send_webhook + Activities::ActivityWebhookService.new(webhook, activity).send_webhook end end end diff --git a/app/models/activity.rb b/app/models/activity.rb index fd950d061..ddb72dc69 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -155,6 +155,6 @@ class Activity < ApplicationRecord end def dispatch_webhooks - Activities::DispatchWebhooksJobs.perform_later(self) + Activities::DispatchWebhooksJob.perform_later(self) end end diff --git a/app/models/webhook.rb b/app/models/webhook.rb index 9661f8613..2388f0ebc 100644 --- a/app/models/webhook.rb +++ b/app/models/webhook.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true class Webhook < ApplicationRecord - enum method: { get: 0, post: 1, patch: 2 } + enum http_method: { get: 0, post: 1, patch: 2 } belongs_to :activity_filter - validates :method, presence: true + validates :http_method, presence: true validates :url, presence: true validate :valid_url diff --git a/app/services/activities/activity_webhook_service.rb b/app/services/activities/activity_webhook_service.rb new file mode 100644 index 000000000..7abd797f4 --- /dev/null +++ b/app/services/activities/activity_webhook_service.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Activities + class ActivityWebhookService + def initialize(webhook, activity) + @webhook = webhook + @activity = activity + end + + def send_webhook + WebhookService.new(@webhook, activity_payload).send_webhook + end + + def activity_payload + @activity.values.merge( + type: @activity.type_of, + created_at: @activity.created_at + ) + end + end +end diff --git a/app/services/activities/webhook_service.rb b/app/services/activities/webhook_service.rb deleted file mode 100644 index e1d32be5f..000000000 --- a/app/services/activities/webhook_service.rb +++ /dev/null @@ -1,56 +0,0 @@ -# frozen_string_literal: true - -module Activities - class WebhookService - DISABLE_WEBHOOK_ERROR_THRESHOLD = 10 - - def initialize(webhook, activity) - @webhook = webhook - @activity = activity - end - - def send_webhook - raise "Cannot send inactive webhook." unless @webhook.active? - - response = HTTParty.send( - @webhook.method, - @webhook.url, - { - headers: { 'Content-Type' => 'application/json' }, - body: activity_payload - } - ) - - unless response.success? - log_error!("#{response.status}: #{response.message}") - end - rescue Net::ReadTimeout, SocketError => error - log_error!(error) - ensure - disable_webhook_if_broken! - end - - private - - def activity_payload - @activity.values.merge( - type: @activity.type_of, - created_at: @activity.created_at - ) - end - - def log_error!(message) - error_count = @webhook.error_count + 1 - - @webhook.update( - error_count: error_count, - last_error: message - ) - end - - def disable_webhook_if_broken! - return if @webhook.error_count < DISABLE_WEBHOOK_ERROR_THRESHOLD - @webhook.update(active: false) - end - end -end diff --git a/app/services/webhook_service.rb b/app/services/webhook_service.rb new file mode 100644 index 000000000..9ab12701d --- /dev/null +++ b/app/services/webhook_service.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +class WebhookService + class InactiveWebhookSendException < StandardError; end + + DISABLE_WEBHOOK_ERROR_THRESHOLD = 10 + + def initialize(webhook, payload) + @webhook = webhook + @payload = payload + end + + def send_webhook + unless @webhook.active? + raise( + Activities::WebhooksService::InactiveWebhookSendException.new( + "Refused to send inactive webhook." + ) + ) + end + + response = HTTParty.send( + @webhook.http_method, + @webhook.url, + { + headers: { 'Content-Type' => 'application/json' }, + body: @payload + } + ) + + unless response.success? + log_error!("#{response.code}: #{response.message}") + end + + response + rescue Net::ReadTimeout, Net::OpenTimeout, SocketError => error + log_error!(error) + raise error + ensure + disable_webhook_if_broken! + end + + private + + + def log_error!(message) + error_count = @webhook.error_count + 1 + + @webhook.update( + error_count: error_count, + last_error: message + ) + end + + def disable_webhook_if_broken! + return if @webhook.error_count < DISABLE_WEBHOOK_ERROR_THRESHOLD + @webhook.update(active: false) + end +end diff --git a/app/views/users/settings/webhooks/_webhook_form.html.erb b/app/views/users/settings/webhooks/_webhook_form.html.erb index c3ac7a4bf..0c5febbdc 100644 --- a/app/views/users/settings/webhooks/_webhook_form.html.erb +++ b/app/views/users/settings/webhooks/_webhook_form.html.erb @@ -1,6 +1,6 @@ <%= t("webhooks.index.webhook_trigger") %>
- <%= f.select :method, options_for_select(Webhook.methods.map{ |k,_v| [k.upcase, k] }, f.object.method) %> + <%= f.select :http_method, options_for_select(Webhook.http_methods.map{ |k,_v| [k.upcase, k] }, f.object.http_method) %>
<%= t("webhooks.index.target") %>
diff --git a/app/views/users/settings/webhooks/index.html.erb b/app/views/users/settings/webhooks/index.html.erb index 1fe510fe9..435e2e810 100644 --- a/app/views/users/settings/webhooks/index.html.erb +++ b/app/views/users/settings/webhooks/index.html.erb @@ -63,7 +63,7 @@
  • - <%= t('webhooks.index.webhook_text_html', method: webhook.method.upcase, url: webhook.url) %> + <%= t('webhooks.index.webhook_text_html', method: webhook.http_method.upcase, url: webhook.url) %> <% if webhook.active? %> diff --git a/db/migrate/20210617111749_rename_webhook_method_column.rb b/db/migrate/20210617111749_rename_webhook_method_column.rb new file mode 100644 index 000000000..58cce6dbb --- /dev/null +++ b/db/migrate/20210617111749_rename_webhook_method_column.rb @@ -0,0 +1,5 @@ +class RenameWebhookMethodColumn < ActiveRecord::Migration[6.1] + def change + rename_column :webhooks, :method, :http_method + end +end diff --git a/db/structure.sql b/db/structure.sql index 10eb89fdc..7bd13e5ec 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2799,7 +2799,7 @@ CREATE TABLE public.webhooks ( activity_filter_id bigint NOT NULL, active boolean DEFAULT true NOT NULL, url character varying NOT NULL, - method integer NOT NULL, + http_method integer NOT NULL, created_at timestamp(6) without time zone NOT NULL, updated_at timestamp(6) without time zone NOT NULL, error_count integer DEFAULT 0 NOT NULL, @@ -7351,6 +7351,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20210506125657'), ('20210531114633'), ('20210603152345'), -('20210616071836'); +('20210616071836'), +('20210617111749'); diff --git a/spec/factories/activity_filters.rb b/spec/factories/activity_filters.rb new file mode 100644 index 000000000..1726cdb08 --- /dev/null +++ b/spec/factories/activity_filters.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :activity_filter do + name { "type filter 1" } + filter { {"types" => ["0"], "from_date" => "", "to_date" => ""} } + end +end diff --git a/spec/factories/webhooks.rb b/spec/factories/webhooks.rb new file mode 100644 index 000000000..624648eb7 --- /dev/null +++ b/spec/factories/webhooks.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :webhook do + activity_filter + http_method { "post" } + url { "https://www.example.com" } + end +end diff --git a/spec/jobs/activities/dispatch_webhooks_job_spec.rb b/spec/jobs/activities/dispatch_webhooks_job_spec.rb new file mode 100644 index 000000000..1a515f945 --- /dev/null +++ b/spec/jobs/activities/dispatch_webhooks_job_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Activities::DispatchWebhooksJob do + let!(:activity_filter_1) { create :activity_filter } + let!(:activity_filter_2) { create :activity_filter } + let!(:non_matching_activity_filter) do + create(:activity_filter, + filter: {"types" => ["163"], "from_date" => "", "to_date" => ""} + ) + end + let!(:webhook_1) { create :webhook, activity_filter: activity_filter_1 } + let!(:webhook_2) { create :webhook, activity_filter: activity_filter_2 } + let!(:webhook_3) { create :webhook, activity_filter: non_matching_activity_filter } + let!(:activity) { create :activity } + + it 'enqueues webhook jobs' do + ActiveJob::Base.queue_adapter = :test + + expect { + Activities::DispatchWebhooksJob.new(activity).perform_now + }.to have_enqueued_job(Activities::SendWebhookJob).exactly(2).times + end +end diff --git a/spec/jobs/activities/send_webhook_job_spec.rb b/spec/jobs/activities/send_webhook_job_spec.rb new file mode 100644 index 000000000..f67090969 --- /dev/null +++ b/spec/jobs/activities/send_webhook_job_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Activities::SendWebhookJob do + let!(:webhook) { create :webhook } + let!(:activity) { create :activity } + + it 'sends the webhook' do + stub_request(:post, webhook.url).to_return(status: 200, body: "", headers: {}) + + expect(Activities::SendWebhookJob.new(webhook, activity).perform_now.response.code).to eq("200") + end +end diff --git a/spec/models/activity_spec.rb b/spec/models/activity_spec.rb index 7dc06219a..f517d4bd2 100644 --- a/spec/models/activity_spec.rb +++ b/spec/models/activity_spec.rb @@ -4,6 +4,9 @@ require 'rails_helper' describe Activity, type: :model do let(:activity) { build :activity } + let(:user) { create :user } + let(:team) { create :team } + let(:old_activity) { build :activity, :old } it 'should be of class Activity' do @@ -61,6 +64,15 @@ describe Activity, type: :model do end end + describe '.create' do + it 'enqueues webhook dispatch job' do + ActiveJob::Base.queue_adapter = :test + expect { + Activity.create(owner: user, team: team, type_of: "generate_pdf_report") + }.to have_enqueued_job(Activities::DispatchWebhooksJob) + end + end + describe '.save' do it 'adds user to message items' do activity.save diff --git a/spec/services/webhook_service_spec.rb b/spec/services/webhook_service_spec.rb new file mode 100644 index 000000000..89c3aafa9 --- /dev/null +++ b/spec/services/webhook_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Activities::CreateActivityService do + let(:webhook) { create :webhook } + + context 'when webhook is valid' do + it 'executes webhook' do + stub_request(:post, webhook.url).to_return(status: 200, body: "", headers: {}) + + expect(WebhookService.new(webhook, { payload: "payload" }).send_webhook.response.code).to eq("200") + end + end + + context 'when webhook gets non-success HTTP response' do + it 'logs error' do + stub_request(:post, webhook.url).to_return(status: 500, body: "", headers: {}) + + expect(WebhookService.new(webhook, { payload: "payload" }).send_webhook.response.code).to eq("500") + expect(webhook.error_count).to eq(1) + expect(webhook.last_error).to eq("500: ") + end + end + + context 'when webhook times out' do + it 'logs error' do + stub_request(:post, webhook.url).to_timeout + + expect { WebhookService.new(webhook, { payload: "payload" }).send_webhook }.to raise_error(Net::OpenTimeout) + expect(webhook.error_count).to eq(1) + expect(webhook.last_error).to eq("execution expired") + end + end + + context 'when webhook url cannot be resolved' do + it 'logs error' do + stub_request(:post, webhook.url).to_raise(SocketError) + + expect { WebhookService.new(webhook, { payload: "payload" }).send_webhook }.to raise_error(SocketError) + expect(webhook.error_count).to eq(1) + expect(webhook.last_error).to eq("Exception from WebMock") + end + end + + context 'when webhook failed too many times' do + it 'disables webhook' do + stub_request(:post, webhook.url).to_raise(SocketError) + + webhook.update_columns(error_count: WebhookService::DISABLE_WEBHOOK_ERROR_THRESHOLD - 1, active: true) + + expect { WebhookService.new(webhook, { payload: "payload" }).send_webhook }.to raise_error(SocketError) + expect(webhook.error_count).to eq(WebhookService::DISABLE_WEBHOOK_ERROR_THRESHOLD) + expect(webhook.active).to be false + end + end +end