misc(PaymentRequest): Apply payment idempotency refactor (#2986)

## Context This PR follow #2962 in the refactor of the payment processing. It applied the logic that was applied to payment for invoices to payment requests The main goal of this refactor will be to: - Avoid double payment at all cost - Avoid code duplication between payment processor integration to make maintenance easier ## Description This PR applies the following logic for payment processing of payment requests: - Creates a payment (or retrieve the pending one in case of retry) - Delegate the payment processing to the dedicated `PaymentProviders::*::Payments::CreateService` responsible only for the payment creation and the error handling (including idempotency). The payment provider payment creation logic will be shared between types of payment and will rely only on the Lago Payment record
getlago · Jan 13, 2025 · 4a0f0c4 · 4a0f0c4
1 parent 8f21a6a
commit 4a0f0c4
Show file tree

Hide file tree

Showing 40 changed files with 785 additions and 1,471 deletions.
diff --git a/app/jobs/invoices/payments/adyen_create_job.rb b/app/jobs/invoices/payments/adyen_create_job.rb
@@ -10,7 +10,7 @@ class AdyenCreateJob < ApplicationJob
       retry_on Faraday::ConnectionFailed, wait: :polynomially_longer, attempts: 6
 
       def perform(invoice)
-        # NOTE: Legacy job, kept only to avoid existing jobs
+        # NOTE: Legacy job, kept only to avoid failure with existing jobs
 
         Invoices::Payments::CreateService.call!(invoice:, payment_provider: :adyen)
       end

diff --git a/app/jobs/invoices/payments/gocardless_create_job.rb b/app/jobs/invoices/payments/gocardless_create_job.rb
@@ -8,7 +8,7 @@ class GocardlessCreateJob < ApplicationJob
       unique :until_executed, on_conflict: :log
 
       def perform(invoice)
-        # NOTE: Legacy job, kept only to avoid existing jobs
+        # NOTE: Legacy job, kept only to avoid faileure with existing jobs
 
         Invoices::Payments::CreateService.call!(invoice:, payment_provider: :gocardless)
       end

diff --git a/app/jobs/invoices/payments/stripe_create_job.rb b/app/jobs/invoices/payments/stripe_create_job.rb
@@ -13,7 +13,7 @@ class StripeCreateJob < ApplicationJob
       retry_on Invoices::Payments::RateLimitError, wait: :polynomially_longer, attempts: 6
 
       def perform(invoice)
-        # NOTE: Legacy job, kept only to avoid existing jobs
+        # NOTE: Legacy job, kept only to avoid faileure with existing jobs
 
         Invoices::Payments::CreateService.call!(invoice:, payment_provider: :stripe)
       end

diff --git a/app/jobs/payment_requests/payments/adyen_create_job.rb b/app/jobs/payment_requests/payments/adyen_create_job.rb
@@ -10,11 +10,9 @@ class AdyenCreateJob < ApplicationJob
       retry_on Faraday::ConnectionFailed, wait: :polynomially_longer, attempts: 6
 
       def perform(payable)
-        result = PaymentRequests::Payments::AdyenService.new(payable).create
+        # NOTE: Legacy job, kept only to avoid faileure with existing jobs
 
-        PaymentRequestMailer.with(payment_request: payable).requested.deliver_later if result.payable&.payment_failed?
-
-        result.raise_if_error!
+        PaymentRequests::Payments::CreateService.call!(payable:, payment_provider: 'adyen')
       end
     end
   end

diff --git a/app/jobs/payment_requests/payments/create_job.rb b/app/jobs/payment_requests/payments/create_job.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module PaymentRequests
+  module Payments
+    class CreateJob < ApplicationJob
+      queue_as 'providers'
+
+      unique :until_executed, on_conflict: :log
+
+      retry_on Faraday::ConnectionFailed, wait: :polynomially_longer, attempts: 6
+      retry_on ::Stripe::RateLimitError, wait: :polynomially_longer, attempts: 6
+      retry_on ::Stripe::APIConnectionError, wait: :polynomially_longer, attempts: 6
+
+      def perform(payable:, payment_provider:)
+        PaymentRequests::Payments::CreateService.call!(payable:, payment_provider:)
+      end
+    end
+  end
+end
diff --git a/app/jobs/payment_requests/payments/gocardless_create_job.rb b/app/jobs/payment_requests/payments/gocardless_create_job.rb
@@ -8,11 +8,9 @@ class GocardlessCreateJob < ApplicationJob
       unique :until_executed, on_conflict: :log
 
       def perform(payable)
-        result = PaymentRequests::Payments::GocardlessService.new(payable).create
+        # NOTE: Legacy job, kept only to avoid faileure with existing jobs
 
-        PaymentRequestMailer.with(payment_request: payable).requested.deliver_later if result.payable&.payment_failed?
-
-        result.raise_if_error!
+        PaymentRequests::Payments::CreateService.call!(payable:, payment_provider: 'gocardless')
       end
     end
   end

diff --git a/app/jobs/payment_requests/payments/stripe_create_job.rb b/app/jobs/payment_requests/payments/stripe_create_job.rb
@@ -11,11 +11,9 @@ class StripeCreateJob < ApplicationJob
       retry_on ::Stripe::APIConnectionError, wait: :polynomially_longer, attempts: 6
 
       def perform(payable)
-        result = PaymentRequests::Payments::StripeService.new(payable).create
+        # NOTE: Legacy job, kept only to avoid faileure with existing jobs
 
-        PaymentRequestMailer.with(payment_request: payable).requested.deliver_later if result.payable&.payment_failed?
-
-        result.raise_if_error!
+        PaymentRequests::Payments::CreateService.call!(payable:, payment_provider: 'stripe')
       end
     end
   end

diff --git a/app/models/payment_providers/adyen_provider.rb b/app/models/payment_providers/adyen_provider.rb
@@ -4,7 +4,7 @@ module PaymentProviders
   class AdyenProvider < BaseProvider
     SUCCESS_REDIRECT_URL = 'https://www.adyen.com/'
 
-    PENDING_STATUSES = %w[AuthorisedPending Received].freeze
+    PROCESSING_STATUSES = %w[AuthorisedPending Received].freeze
     SUCCESS_STATUSES = %w[Authorised SentForSettle SettleScheduled Settled Refunded].freeze
     FAILED_STATUSES = %w[Cancelled CaptureFailed Error Expired Refused].freeze
 

diff --git a/app/models/payment_providers/base_provider.rb b/app/models/payment_providers/base_provider.rb
@@ -28,7 +28,7 @@ class BaseProvider < ApplicationRecord
     settings_accessors :webhook_secret, :success_redirect_url
 
     def determine_payment_status(payment_status)
-      return :pending if self.class::PENDING_STATUSES.include?(payment_status)
+      return :processing if self.class::PROCESSING_STATUSES.include?(payment_status)
       return :succeeded if self.class::SUCCESS_STATUSES.include?(payment_status)
       return :failed if self.class::FAILED_STATUSES.include?(payment_status)
 

diff --git a/app/models/payment_providers/gocardless_provider.rb b/app/models/payment_providers/gocardless_provider.rb
@@ -4,7 +4,7 @@ module PaymentProviders
   class GocardlessProvider < BaseProvider
     SUCCESS_REDIRECT_URL = 'https://gocardless.com/'
 
-    PENDING_STATUSES = %w[pending_customer_approval pending_submission submitted confirmed].freeze
+    PROCESSING_STATUSES = %w[pending_customer_approval pending_submission submitted confirmed].freeze
     SUCCESS_STATUSES = %w[paid_out].freeze
     FAILED_STATUSES = %w[cancelled customer_approval_denied failed charged_back].freeze
 

diff --git a/app/models/payment_providers/stripe_provider.rb b/app/models/payment_providers/stripe_provider.rb
@@ -18,7 +18,7 @@ class StripeProvider < BaseProvider
       charge.dispute.closed
     ].freeze
 
-    PENDING_STATUSES = %w[
+    PROCESSING_STATUSES = %w[
       processing
       requires_capture
       requires_action

diff --git a/app/services/invoices/payments/adyen_service.rb b/app/services/invoices/payments/adyen_service.rb
@@ -6,10 +6,6 @@ class AdyenService < BaseService
       include Lago::Adyen::ErrorHandlable
       include Customers::PaymentProviderFinder
 
-      PENDING_STATUSES = %w[AuthorisedPending Received].freeze
-      SUCCESS_STATUSES = %w[Authorised SentForSettle SettleScheduled Settled Refunded].freeze
-      FAILED_STATUSES = %w[Cancelled CaptureFailed Error Expired Refused].freeze
-
       def initialize(invoice = nil)
         @invoice = invoice
 
@@ -30,7 +26,7 @@ def update_payment_status(provider_payment_id:, status:, metadata: {})
 
         payment.update!(status:)
 
-        invoice_payment_status = invoice_payment_status(status)
+        invoice_payment_status = payment.payment_provider&.determine_payment_status(status)
         update_invoice_payment_status(payment_status: invoice_payment_status)
 
         result
@@ -175,14 +171,6 @@ def payment_url_params
         prms
       end
 
-      def invoice_payment_status(payment_status)
-        return :pending if PENDING_STATUSES.include?(payment_status)
-        return :succeeded if SUCCESS_STATUSES.include?(payment_status)
-        return :failed if FAILED_STATUSES.include?(payment_status)
-
-        payment_status
-      end
-
       def update_invoice_payment_status(payment_status:, deliver_webhook: true)
         result = Invoices::UpdateService.call(
           invoice:,

diff --git a/app/services/invoices/payments/create_service.rb b/app/services/invoices/payments/create_service.rb
@@ -43,13 +43,20 @@ def call
 
         result.payment = payment
 
-        payment_result = ::PaymentProviders::CreatePaymentFactory.new_instance(provider:, payment:).call!
+        payment_result = ::PaymentProviders::CreatePaymentFactory.new_instance(
+          provider:,
+          payment:,
+          reference: "#{invoice.organization.name} - Invoice #{invoice.number}",
+          metadata: {
+            lago_invoice_id: invoice.id,
+            lago_customer_id: invoice.customer_id,
+            invoice_issuing_date: invoice.issuing_date.iso8601,
+            invoice_type: invoice.invoice_type
+          }
+        ).call!
 
         payment_status = payment_result.payment.payable_payment_status
-        update_invoice_payment_status(
-          payment_status: (payment_status == "processing") ? :pending : payment_status,
-          processing: payment_status == "processing"
-        )
+        update_invoice_payment_status(payment_status:)
 
         Integrations::Aggregator::Payments::CreateJob.perform_later(payment:) if result.payment.should_sync_payment?
 
@@ -58,10 +65,12 @@ def call
         result.payment = e.result.payment
 
         if e.result.payment.payable_payment_status&.to_sym != :pending
+          # Avoid notification for amount_too_small errors
           deliver_error_webhook(e.result)
-          update_invoice_payment_status(payment_status: e.result.payment.payable_payment_status)
         end
 
+        update_invoice_payment_status(payment_status: e.result.payment.payable_payment_status)
+
         # Some errors should be investigated and need to be raised
         raise if e.result.reraise
 
@@ -103,13 +112,13 @@ def current_payment_provider_customer
           .find_by(payment_provider_id: current_payment_provider.id)
       end
 
-      def update_invoice_payment_status(payment_status:, processing: false)
+      def update_invoice_payment_status(payment_status:)
         Invoices::UpdateService.call!(
           invoice: invoice,
           params: {
-            payment_status:,
             # NOTE: A proper `processing` payment status should be introduced for invoices
-            ready_for_payment_processing: !processing && payment_status.to_sym != :succeeded
+            payment_status: (payment_status.to_s == "processing") ? :pending : payment_status,
+            ready_for_payment_processing: %w[pending failed].include?(payment_status.to_s)
           },
           webhook_notification: payment_status.to_sym == :succeeded
         )

diff --git a/app/services/invoices/payments/gocardless_service.rb b/app/services/invoices/payments/gocardless_service.rb
@@ -5,11 +5,6 @@ module Payments
     class GocardlessService < BaseService
       include Customers::PaymentProviderFinder
 
-      PENDING_STATUSES = %w[pending_customer_approval pending_submission submitted confirmed]
-        .freeze
-      SUCCESS_STATUSES = %w[paid_out].freeze
-      FAILED_STATUSES = %w[cancelled customer_approval_denied failed charged_back].freeze
-
       def initialize(invoice = nil)
         @invoice = invoice
 
@@ -26,7 +21,7 @@ def update_payment_status(provider_payment_id:, status:)
 
         payment.update!(status:)
 
-        invoice_payment_status = invoice_payment_status(status)
+        invoice_payment_status = payment.payment_provider&.determine_payment_status(status)
         update_invoice_payment_status(payment_status: invoice_payment_status)
 
         result
@@ -40,14 +35,6 @@ def update_payment_status(provider_payment_id:, status:)
 
       delegate :organization, :customer, to: :invoice
 
-      def invoice_payment_status(payment_status)
-        return :pending if PENDING_STATUSES.include?(payment_status)
-        return :succeeded if SUCCESS_STATUSES.include?(payment_status)
-        return :failed if FAILED_STATUSES.include?(payment_status)
-
-        payment_status
-      end
-
       def update_invoice_payment_status(payment_status:, deliver_webhook: true)
         update_invoice_result = Invoices::UpdateService.call(
           invoice: result.invoice,

diff --git a/app/services/invoices/payments/stripe_service.rb b/app/services/invoices/payments/stripe_service.rb
@@ -5,11 +5,6 @@ module Payments
     class StripeService < BaseService
       include Customers::PaymentProviderFinder
 
-      PENDING_STATUSES = %w[processing requires_capture requires_action requires_confirmation requires_payment_method]
-        .freeze
-      SUCCESS_STATUSES = %w[succeeded].freeze
-      FAILED_STATUSES = %w[canceled].freeze
-
       def initialize(invoice = nil)
         @invoice = invoice
 
@@ -37,7 +32,7 @@ def update_payment_status(organization_id:, status:, stripe_payment:)
         payment.update!(status:)
 
         update_invoice_payment_status(
-          payment_status: invoice_payment_status(status),
+          payment_status: payment.payment_provider&.determine_payment_status(status),
           processing: status == "processing"
         )
 
@@ -89,7 +84,7 @@ def create_payment(stripe_payment, invoice: nil)
           status: "pending"
         )
 
-        status = invoice_payment_status(stripe_payment.status)
+        status = payment.payment_provider&.determine_payment_status(stripe_payment.status)
         status = (status.to_sym == :pending) ? :processing : status
 
         payment.provider_payment_id = stripe_payment.id
@@ -150,14 +145,6 @@ def description
         "#{organization.name} - Invoice #{invoice.number}"
       end
 
-      def invoice_payment_status(payment_status)
-        return :pending if PENDING_STATUSES.include?(payment_status)
-        return :succeeded if SUCCESS_STATUSES.include?(payment_status)
-        return :failed if FAILED_STATUSES.include?(payment_status)
-
-        payment_status&.to_sym
-      end
-
       def update_invoice_payment_status(payment_status:, deliver_webhook: true, processing: false)
         result = Invoices::UpdateService.call(
           invoice: invoice.presence || @result.invoice,

diff --git a/app/services/payment_providers/adyen/payments/create_service.rb b/app/services/payment_providers/adyen/payments/create_service.rb
@@ -4,12 +4,10 @@ module PaymentProviders
   module Adyen
     module Payments
       class CreateService < BaseService
-        PROCESSING_STATUSES = %w[AuthorisedPending Received].freeze
-        SUCCESS_STATUSES = %w[Authorised SentForSettle SettleScheduled Settled Refunded].freeze
-        FAILED_STATUSES = %w[Cancelled CaptureFailed Error Expired Refused].freeze
-
-        def initialize(payment:)
+        def initialize(payment:, reference:, metadata:)
           @payment = payment
+          @reference = reference
+          @metadata = metadata
           @invoice = payment.payable
           @provider_customer = payment.payment_provider_customer
 
@@ -29,7 +27,7 @@ def call
 
           payment.provider_payment_id = adyen_result.response["pspReference"]
           payment.status = adyen_result.response["resultCode"]
-          payment.payable_payment_status = payment_status_mapping(payment.status)
+          payment.payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
           payment.save!
 
           result.payment = payment
@@ -45,7 +43,7 @@ def call
 
         private
 
-        attr_reader :payment, :invoice, :provider_customer
+        attr_reader :payment, :reference, :metadata, :invoice, :provider_customer
 
         delegate :payment_provider, :customer, to: :provider_customer
 
@@ -92,7 +90,7 @@ def payment_params
               currency: payment.amount_currency.upcase,
               value: payment.amount_cents
             },
-            reference: invoice.number,
+            reference: reference,
             paymentMethod: {
               type: "scheme",
               storedPaymentMethodId: provider_customer.payment_method_id
@@ -106,14 +104,6 @@ def payment_params
           prms
         end
 
-        def payment_status_mapping(payment_status)
-          return :processing if PROCESSING_STATUSES.include?(payment_status)
-          return :succeeded if SUCCESS_STATUSES.include?(payment_status)
-          return :failed if FAILED_STATUSES.include?(payment_status)
-
-          payment_status
-        end
-
         def prepare_failed_result(error, reraise: false)
           result.error_message = error.msg
           result.error_code = error.code

diff --git a/app/services/payment_providers/create_payment_factory.rb b/app/services/payment_providers/create_payment_factory.rb
@@ -2,8 +2,8 @@
 
 module PaymentProviders
   class CreatePaymentFactory
-    def self.new_instance(provider:, payment:)
-      service_class(provider:).new(payment:)
+    def self.new_instance(provider:, payment:, reference:, metadata:)
+      service_class(provider:).new(payment:, reference:, metadata:)
     end
 
     def self.service_class(provider:)