From 787c6edce99cc5a72d5faa7dd7bbc6e0eb7ca0ce Mon Sep 17 00:00:00 2001 From: Vincent Pochet Date: Thu, 17 Oct 2024 09:19:32 +0200 Subject: [PATCH] fix(gocardless): Treat Gocardless webhooks events in separate jobs (#2695) This pull request focuses on improving the GoCardless webhook events management and refactoring the integration by introducing new services for handling webhooks and events using the standard call/Result pattern. The main changes include replacing direct calls to `GocardlessService` with more specific services and updating the logic to treat each webhook events in isolation to avoid failling a batch if a single event is causing an error. ### Refactoring and Service Extraction: * **Refactored Webhook Handling:** - Replaced `GocardlessService.handle_incoming_webhook` with `Gocardless::HandleIncomingWebhookService.call` in `WebhooksController`. (`app/controllers/webhooks_controller.rb`) - Extracted webhook handling logic into a new service `HandleIncomingWebhookService`. (`app/services/payment_providers/gocardless/handle_incoming_webhook_service.rb`) * **Refactored Event Handling:** - Replaced `GocardlessService.handle_event` with `Gocardless::HandleEventService.call` in `HandleEventJob`. (`app/jobs/payment_providers/gocardless/handle_event_job.rb`) - Extracted event handling logic into a new service `HandleEventService`. (`app/services/payment_providers/gocardless/handle_event_service.rb`) - Keep acception the legacy `events_json` input argument in `HandleEventJob` to avoid issues with enqueued/dead jobs for now --- app/controllers/webhooks_controller.rb | 2 +- .../gocardless/handle_event_job.rb | 14 +- .../gocardless/handle_event_service.rb | 61 +++++++ .../handle_incoming_webhook_service.rb | 48 ++++++ .../payment_providers/gocardless_service.rb | 79 --------- .../gocardless/handle_event_job_spec.rb | 34 ++-- spec/requests/webhooks_controller_spec.rb | 10 +- .../gocardless/handle_event_service_spec.rb | 86 ++++++++++ .../handle_incoming_webhook_service_spec.rb | 66 ++++++++ .../gocardless_service_spec.rb | 152 ------------------ 10 files changed, 299 insertions(+), 253 deletions(-) create mode 100644 app/services/payment_providers/gocardless/handle_event_service.rb create mode 100644 app/services/payment_providers/gocardless/handle_incoming_webhook_service.rb create mode 100644 spec/services/payment_providers/gocardless/handle_event_service_spec.rb create mode 100644 spec/services/payment_providers/gocardless/handle_incoming_webhook_service_spec.rb diff --git a/app/controllers/webhooks_controller.rb b/app/controllers/webhooks_controller.rb index 67fc93b3407..fedac0cbf0f 100644 --- a/app/controllers/webhooks_controller.rb +++ b/app/controllers/webhooks_controller.rb @@ -21,7 +21,7 @@ def stripe end def gocardless - result = PaymentProviders::GocardlessService.new.handle_incoming_webhook( + result = PaymentProviders::Gocardless::HandleIncomingWebhookService.call( organization_id: params[:organization_id], code: params[:code].presence, body: request.body.read, diff --git a/app/jobs/payment_providers/gocardless/handle_event_job.rb b/app/jobs/payment_providers/gocardless/handle_event_job.rb index af29f483f70..190ab4a40dc 100644 --- a/app/jobs/payment_providers/gocardless/handle_event_job.rb +++ b/app/jobs/payment_providers/gocardless/handle_event_job.rb @@ -5,9 +5,17 @@ module Gocardless class HandleEventJob < ApplicationJob queue_as 'providers' - def perform(events_json:) - result = PaymentProviders::GocardlessService.new.handle_event(events_json:) - result.raise_if_error! + def perform(organization: nil, events_json: nil, event_json: nil) + # NOTE: temporary keeps both events_json and event_json to avoid errors during the deployment + if events_json.present? + JSON.parse(events_json)['events'].each do |event| + PaymentProviders::Gocardless::HandleEventJob.perform_later(event_json: event.to_json) + end + + return + end + + PaymentProviders::Gocardless::HandleEventService.call(event_json:).raise_if_error! end end end diff --git a/app/services/payment_providers/gocardless/handle_event_service.rb b/app/services/payment_providers/gocardless/handle_event_service.rb new file mode 100644 index 00000000000..5cb27119bde --- /dev/null +++ b/app/services/payment_providers/gocardless/handle_event_service.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module PaymentProviders + module Gocardless + class HandleEventService < BaseService + PAYMENT_ACTIONS = %w[paid_out failed cancelled customer_approval_denied charged_back].freeze + REFUND_ACTIONS = %w[created funds_returned paid refund_settled failed].freeze + + PAYMENT_SERVICE_CLASS_MAP = { + "Invoice" => Invoices::Payments::GocardlessService, + "PaymentRequest" => PaymentRequests::Payments::GocardlessService + }.freeze + + def initialize(event_json:) + @event_json = event_json + + super + end + + def call + case event.resource_type + when 'payments' + if PAYMENT_ACTIONS.include?(event.action) + payment_service_klass(event) + .new.update_payment_status( + provider_payment_id: event.links.payment, + status: event.action + ).raise_if_error! + end + when 'refunds' + if REFUND_ACTIONS.include?(event.action) + CreditNotes::Refunds::GocardlessService + .new.update_status( + provider_refund_id: event.links.refund, + status: event.action, + metadata: event.metadata + ).raise_if_error! + end + end + + result + end + + private + + attr_reader :organization, :event_json + + def event + @event ||= GoCardlessPro::Resources::Event.new(JSON.parse(event_json)) + end + + def payment_service_klass(event) + payable_type = event.metadata["lago_payable_type"] || "Invoice" + + PAYMENT_SERVICE_CLASS_MAP.fetch(payable_type) do + raise NameError, "Invalid lago_payable_type: #{payable_type}" + end + end + end + end +end diff --git a/app/services/payment_providers/gocardless/handle_incoming_webhook_service.rb b/app/services/payment_providers/gocardless/handle_incoming_webhook_service.rb new file mode 100644 index 00000000000..5e85342ae0d --- /dev/null +++ b/app/services/payment_providers/gocardless/handle_incoming_webhook_service.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module PaymentProviders + module Gocardless + class HandleIncomingWebhookService < BaseService + def initialize(organization_id:, body:, signature:, code: nil) + @organization_id = organization_id + @body = body + @signature = signature + @code = code + + super + end + + def call + payment_provider_result = PaymentProviders::FindService.call( + organization_id:, + code:, + payment_provider_type: 'gocardless' + ) + return payment_provider_result unless payment_provider_result.success? + + result.events = GoCardlessPro::Webhook.parse( + request_body: body, + signature_header: signature, + webhook_endpoint_secret: payment_provider_result.payment_provider&.webhook_secret + ) + + result.events.each do |event| + PaymentProviders::Gocardless::HandleEventJob.perform_later( + organization: payment_provider_result.payment_provider.organization, + event_json: event.to_json + ) + end + + result + rescue JSON::ParserError + result.service_failure!(code: 'webhook_error', message: 'Invalid payload') + rescue GoCardlessPro::Webhook::InvalidSignatureError + result.service_failure!(code: 'webhook_error', message: 'Invalid signature') + end + + private + + attr_reader :organization_id, :body, :signature, :code + end + end +end diff --git a/app/services/payment_providers/gocardless_service.rb b/app/services/payment_providers/gocardless_service.rb index a2e7fd28ae2..518ee55f5ea 100644 --- a/app/services/payment_providers/gocardless_service.rb +++ b/app/services/payment_providers/gocardless_service.rb @@ -3,13 +3,6 @@ module PaymentProviders class GocardlessService < BaseService REDIRECT_URI = "#{ENV["LAGO_OAUTH_PROXY_URL"]}/gocardless/callback".freeze - PAYMENT_ACTIONS = %w[paid_out failed cancelled customer_approval_denied charged_back].freeze - REFUND_ACTIONS = %w[created funds_returned paid refund_settled failed].freeze - - PAYMENT_SERVICE_CLASS_MAP = { - "Invoice" => Invoices::Payments::GocardlessService, - "PaymentRequest" => PaymentRequests::Payments::GocardlessService - }.freeze def create_or_update(**args) access_token = if args[:access_code].present? @@ -53,70 +46,6 @@ def create_or_update(**args) result.service_failure!(code: 'internal_error', message: e.description) end - def handle_incoming_webhook(organization_id:, body:, signature:, code: nil) - payment_provider_result = PaymentProviders::FindService.call( - organization_id:, - code:, - payment_provider_type: 'gocardless' - ) - - return payment_provider_result unless payment_provider_result.success? - - events = GoCardlessPro::Webhook.parse( - request_body: body, - signature_header: signature, - webhook_endpoint_secret: payment_provider_result.payment_provider&.webhook_secret - ) - - PaymentProviders::Gocardless::HandleEventJob.perform_later(events_json: body) - - result.events = events - result - rescue JSON::ParserError - result.service_failure!(code: 'webhook_error', message: 'Invalid payload') - rescue GoCardlessPro::Webhook::InvalidSignatureError - result.service_failure!(code: 'webhook_error', message: 'Invalid signature') - end - - def handle_event(events_json:) - handled_events = [] - events = JSON.parse(events_json)['events'] - parsed_events = events.map { |event| GoCardlessPro::Resources::Event.new(event) } - parsed_events.each do |event| - case event.resource_type - when 'payments' - if PAYMENT_ACTIONS.include?(event.action) - update_payment_status_result = payment_service_klass(event) - .new.update_payment_status( - provider_payment_id: event.links.payment, - status: event.action - ) - - return update_payment_status_result unless update_payment_status_result.success? - - handled_events << event - end - when 'refunds' - if REFUND_ACTIONS.include?(event.action) - status_result = CreditNotes::Refunds::GocardlessService - .new.update_status( - provider_refund_id: event.links.refund, - status: event.action, - metadata: event.metadata - ) - - return status_result unless status_result.success? - - handled_events << event - end - - end - end - - result.handled_events = handled_events - result - end - private def oauth @@ -129,13 +58,5 @@ def oauth auth_scheme: :request_body ) end - - def payment_service_klass(event) - payable_type = event.metadata["lago_payable_type"] || "Invoice" - - PAYMENT_SERVICE_CLASS_MAP.fetch(payable_type) do - raise NameError, "Invalid lago_payable_type: #{payable_type}" - end - end end end diff --git a/spec/jobs/payment_providers/gocardless/handle_event_job_spec.rb b/spec/jobs/payment_providers/gocardless/handle_event_job_spec.rb index fdb1233abf5..ab63aa49296 100644 --- a/spec/jobs/payment_providers/gocardless/handle_event_job_spec.rb +++ b/spec/jobs/payment_providers/gocardless/handle_event_job_spec.rb @@ -9,21 +9,33 @@ let(:result) { BaseService::Result.new } let(:organization) { create(:organization) } - let(:gocardless_events) do - [] + let(:event_json) do + path = Rails.root.join('spec/fixtures/gocardless/events.json') + JSON.parse(File.read(path))['events'].first.to_json end - before do - allow(PaymentProviders::GocardlessService).to receive(:new) - .and_return(gocardless_service) - allow(gocardless_service).to receive(:handle_event) - .and_return(result) + let(:service_result) { BaseService::Result.new } + + it 'delegate to the event service' do + allow(PaymentProviders::Gocardless::HandleEventService).to receive(:call) + .with(event_json:) + .and_return(service_result) + + handle_event_job.perform_now(organization:, event_json:) + + expect(PaymentProviders::Gocardless::HandleEventService).to have_received(:call) end - it 'calls the handle event service' do - handle_event_job.perform_now(events_json: gocardless_events) + context 'with legacy multiple events' do + let(:events_json) do + path = Rails.root.join('spec/fixtures/gocardless/events.json') + File.read(path) + end + + it 'enqueues a job for each event' do + handle_event_job.perform_now(events_json:) - expect(PaymentProviders::GocardlessService).to have_received(:new) - expect(gocardless_service).to have_received(:handle_event) + expect(described_class).to have_been_enqueued.exactly(JSON.parse(events_json)['events'].count).times + end end end diff --git a/spec/requests/webhooks_controller_spec.rb b/spec/requests/webhooks_controller_spec.rb index 0cfdda8f964..9dacce0e9b7 100644 --- a/spec/requests/webhooks_controller_spec.rb +++ b/spec/requests/webhooks_controller_spec.rb @@ -104,9 +104,7 @@ end before do - allow(PaymentProviders::GocardlessService).to receive(:new) - .and_return(gocardless_service) - allow(gocardless_service).to receive(:handle_incoming_webhook) + allow(PaymentProviders::Gocardless::HandleIncomingWebhookService).to receive(:call) .with( organization_id: organization.id, code: nil, @@ -128,8 +126,7 @@ expect(response).to have_http_status(:success) - expect(PaymentProviders::GocardlessService).to have_received(:new) - expect(gocardless_service).to have_received(:handle_incoming_webhook) + expect(PaymentProviders::Gocardless::HandleIncomingWebhookService).to have_received(:call) end context 'when failing to handle gocardless event' do @@ -149,8 +146,7 @@ expect(response).to have_http_status(:bad_request) - expect(PaymentProviders::GocardlessService).to have_received(:new) - expect(gocardless_service).to have_received(:handle_incoming_webhook) + expect(PaymentProviders::Gocardless::HandleIncomingWebhookService).to have_received(:call) end end end diff --git a/spec/services/payment_providers/gocardless/handle_event_service_spec.rb b/spec/services/payment_providers/gocardless/handle_event_service_spec.rb new file mode 100644 index 00000000000..a97380358f9 --- /dev/null +++ b/spec/services/payment_providers/gocardless/handle_event_service_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe PaymentProviders::Gocardless::HandleEventService, type: :service do + subject(:event_service) { described_class.new(event_json:) } + + let(:event_json) do + path = Rails.root.join('spec/fixtures/gocardless/events.json') + JSON.parse(File.read(path))['events'].first.to_json + end + + let(:payment_service) { instance_double(Invoices::Payments::GocardlessService) } + let(:service_result) { BaseService::Result.new } + + describe '#call' do + context 'when succeeded payment event' do + it 'routes the event to an other service' do + allow(Invoices::Payments::GocardlessService).to receive(:new) + .and_return(payment_service) + allow(payment_service).to receive(:update_payment_status) + .and_return(service_result) + + event_service.call + + expect(Invoices::Payments::GocardlessService).to have_received(:new) + expect(payment_service).to have_received(:update_payment_status) + end + end + + context "when event metadata contains payable_type PaymentRequest" do + let(:payment_service) { instance_double(PaymentRequests::Payments::GocardlessService) } + let(:service_result) { BaseService::Result.new } + + let(:event_json) do + path = Rails.root.join("spec/fixtures/gocardless/events_payment_request.json") + JSON.parse(File.read(path))['events'].first.to_json + end + + it "routes the event to an other service" do + allow(PaymentRequests::Payments::GocardlessService).to receive(:new) + .and_return(payment_service) + allow(payment_service).to receive(:update_payment_status) + .and_return(service_result) + + event_service.call + + expect(PaymentRequests::Payments::GocardlessService).to have_received(:new) + expect(payment_service).to have_received(:update_payment_status) + end + end + + context "when event metadata contains invalid payable_type" do + let(:event_json) do + path = Rails.root.join("spec/fixtures/gocardless/events_invalid_payable_type.json") + JSON.parse(File.read(path))['events'].first.to_json + end + + it "routes the event to an other service" do + expect { + event_service.call + }.to raise_error(NameError, "Invalid lago_payable_type: InvalidPayableTypeName") + end + end + + context 'when succeeded refund event' do + let(:refund_service) { instance_double(CreditNotes::Refunds::GocardlessService) } + let(:event_json) do + path = Rails.root.join('spec/fixtures/gocardless/events_refund.json') + JSON.parse(File.read(path))['events'].first.to_json + end + + it 'routes the event to an other service' do + allow(CreditNotes::Refunds::GocardlessService).to receive(:new) + .and_return(refund_service) + allow(refund_service).to receive(:update_status) + .and_return(service_result) + + event_service.call + + expect(CreditNotes::Refunds::GocardlessService).to have_received(:new) + expect(refund_service).to have_received(:update_status) + end + end + end +end diff --git a/spec/services/payment_providers/gocardless/handle_incoming_webhook_service_spec.rb b/spec/services/payment_providers/gocardless/handle_incoming_webhook_service_spec.rb new file mode 100644 index 00000000000..9ca87eb0eaf --- /dev/null +++ b/spec/services/payment_providers/gocardless/handle_incoming_webhook_service_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe PaymentProviders::Gocardless::HandleIncomingWebhookService, type: :service do + let(:webhook_service) { described_class.new(organization_id: organization.id, body:, signature:, code:) } + + let(:organization) { create(:organization) } + let(:gocardless_provider) { create(:gocardless_provider, organization:) } + + let(:events) do + path = Rails.root.join('spec/fixtures/gocardless/events.json') + JSON.parse(File.read(path)) + end + + let(:body) { events.to_json } + let(:events_result) { events['events'].map { |event| GoCardlessPro::Resources::Event.new(event) } } + let(:signature) { 'signature' } + let(:code) { nil } + + before { gocardless_provider } + + describe '#call' do + it 'checks the webhook' do + allow(GoCardlessPro::Webhook).to receive(:parse) + .and_return(events_result) + + result = webhook_service.call + expect(result).to be_success + + expect(result.events).to eq(events_result) + expect(PaymentProviders::Gocardless::HandleEventJob).to have_been_enqueued + end + + context 'when failing to parse payload' do + it 'returns an error' do + allow(GoCardlessPro::Webhook).to receive(:parse).and_raise(JSON::ParserError) + + result = webhook_service.call + + aggregate_failures do + expect(result).not_to be_success + expect(result.error).to be_a(BaseService::ServiceFailure) + expect(result.error.code).to eq('webhook_error') + expect(result.error.error_message).to eq('Invalid payload') + end + end + end + + context 'when failing to validate the signature' do + it 'returns an error' do + allow(GoCardlessPro::Webhook).to receive(:parse) + .and_raise(GoCardlessPro::Webhook::InvalidSignatureError.new('error')) + + result = webhook_service.call + + aggregate_failures do + expect(result).not_to be_success + expect(result.error).to be_a(BaseService::ServiceFailure) + expect(result.error.code).to eq('webhook_error') + expect(result.error.error_message).to eq('Invalid signature') + end + end + end + end +end diff --git a/spec/services/payment_providers/gocardless_service_spec.rb b/spec/services/payment_providers/gocardless_service_spec.rb index 6ac1bed38d5..772cdea5929 100644 --- a/spec/services/payment_providers/gocardless_service_spec.rb +++ b/spec/services/payment_providers/gocardless_service_spec.rb @@ -118,156 +118,4 @@ end end end - - describe '.handle_incoming_webhook' do - let(:gocardless_provider) { create(:gocardless_provider, organization:) } - let(:events_result) { events['events'].map { |event| GoCardlessPro::Resources::Event.new(event) } } - - let(:events) do - path = Rails.root.join('spec/fixtures/gocardless/events.json') - JSON.parse(File.read(path)) - end - - before { gocardless_provider } - - it 'checks the webhook' do - allow(GoCardlessPro::Webhook).to receive(:parse) - .and_return(events_result) - - result = gocardless_service.handle_incoming_webhook( - organization_id: organization.id, - body: events.to_json, - signature: 'signature' - ) - - expect(result).to be_success - - expect(result.events).to eq(events_result) - expect(PaymentProviders::Gocardless::HandleEventJob).to have_been_enqueued - end - - context 'when failing to parse payload' do - it 'returns an error' do - allow(GoCardlessPro::Webhook).to receive(:parse).and_raise(JSON::ParserError) - - result = gocardless_service.handle_incoming_webhook( - organization_id: organization.id, - body: events.to_json, - signature: 'signature' - ) - - aggregate_failures do - expect(result).not_to be_success - expect(result.error).to be_a(BaseService::ServiceFailure) - expect(result.error.code).to eq('webhook_error') - expect(result.error.error_message).to eq('Invalid payload') - end - end - end - - context 'when failing to validate the signature' do - it 'returns an error' do - allow(GoCardlessPro::Webhook).to receive(:parse) - .and_raise(GoCardlessPro::Webhook::InvalidSignatureError.new('error')) - - result = gocardless_service.handle_incoming_webhook( - organization_id: organization.id, - body: events.to_json, - signature: 'signature' - ) - - aggregate_failures do - expect(result).not_to be_success - expect(result.error).to be_a(BaseService::ServiceFailure) - expect(result.error.code).to eq('webhook_error') - expect(result.error.error_message).to eq('Invalid signature') - end - end - end - end - - describe '.handle_event' do - let(:payment_service) { instance_double(Invoices::Payments::GocardlessService) } - let(:service_result) { BaseService::Result.new } - - before do - allow(Invoices::Payments::GocardlessService).to receive(:new) - .and_return(payment_service) - allow(payment_service).to receive(:update_payment_status) - .and_return(service_result) - end - - context 'when succeeded payment event' do - let(:events) do - path = Rails.root.join('spec/fixtures/gocardless/events.json') - File.read(path) - end - - it 'routes the event to an other service' do - gocardless_service.handle_event(events_json: events) - - expect(Invoices::Payments::GocardlessService).to have_received(:new) - expect(payment_service).to have_received(:update_payment_status) - end - end - - context "when event metadata contains payable_type PaymentRequest" do - let(:payment_service) { instance_double(PaymentRequests::Payments::GocardlessService) } - let(:service_result) { BaseService::Result.new } - - let(:events) do - path = Rails.root.join("spec/fixtures/gocardless/events_payment_request.json") - File.read(path) - end - - before do - allow(PaymentRequests::Payments::GocardlessService).to receive(:new) - .and_return(payment_service) - allow(payment_service).to receive(:update_payment_status) - .and_return(service_result) - end - - it "routes the event to an other service" do - gocardless_service.handle_event(events_json: events) - - expect(PaymentRequests::Payments::GocardlessService).to have_received(:new) - expect(payment_service).to have_received(:update_payment_status) - end - end - - context "when event metadata contains invalid payable_type" do - let(:events) do - path = Rails.root.join("spec/fixtures/gocardless/events_invalid_payable_type.json") - File.read(path) - end - - it "routes the event to an other service" do - expect { - gocardless_service.handle_event(events_json: events) - }.to raise_error(NameError, "Invalid lago_payable_type: InvalidPayableTypeName") - end - end - - context 'when succeeded refund event' do - let(:refund_service) { instance_double(CreditNotes::Refunds::GocardlessService) } - let(:events) do - path = Rails.root.join('spec/fixtures/gocardless/events_refund.json') - File.read(path) - end - - before do - allow(CreditNotes::Refunds::GocardlessService).to receive(:new) - .and_return(refund_service) - allow(refund_service).to receive(:update_status) - .and_return(service_result) - end - - it 'routes the event to an other service' do - gocardless_service.handle_event(events_json: events) - - expect(CreditNotes::Refunds::GocardlessService).to have_received(:new) - expect(refund_service).to have_received(:update_status) - end - end - end end