fix(payment): Handle payment and invoice status update (#3088)

# Context This PR is related to the recent changes introduced by #2962 and #2986 to rely on payment provider idem-potency logic A new `payable_payment_status` was added to the Payment model, but this status is not updated when a webhook is received from a payment provider # Description This PR makes sure that the payment, payment request or all other payment related statuses are updated accordingly
getlago · Jan 24, 2025 · 46a25d1 · 46a25d1
1 parent 98d336b
commit 46a25d1
Show file tree

Hide file tree

Showing 10 changed files with 71 additions and 142 deletions.
diff --git a/app/services/invoices/payments/adyen_service.rb b/app/services/invoices/payments/adyen_service.rb
@@ -24,10 +24,15 @@ def update_payment_status(provider_payment_id:, status:, metadata: {})
         result.invoice = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
+        payment.status = status
 
-        invoice_payment_status = payment.payment_provider&.determine_payment_status(status)
-        update_invoice_payment_status(payment_status: invoice_payment_status)
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
+
+        update_invoice_payment_status(payment_status: payable_payment_status)
 
         result
       rescue BaseService::FailedResult => e

diff --git a/app/services/invoices/payments/cashfree_service.rb b/app/services/invoices/payments/cashfree_service.rb
@@ -23,9 +23,15 @@ def update_payment_status(organization_id:, status:, cashfree_payment:)
         result.invoice = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
-        invoice_payment_status = payment.payment_provider&.determine_payment_status(status)
-        update_invoice_payment_status(payment_status: invoice_payment_status)
+        payment.status = status
+
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
+
+        update_invoice_payment_status(payment_status: payable_payment_status)
 
         result
       rescue BaseService::FailedResult => e

diff --git a/app/services/invoices/payments/gocardless_service.rb b/app/services/invoices/payments/gocardless_service.rb
@@ -19,10 +19,15 @@ def update_payment_status(provider_payment_id:, status:)
         result.invoice = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
+        payment.status = status
 
-        invoice_payment_status = payment.payment_provider&.determine_payment_status(status)
-        update_invoice_payment_status(payment_status: invoice_payment_status)
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
+
+        update_invoice_payment_status(payment_status: payable_payment_status)
 
         result
       rescue BaseService::FailedResult => e

diff --git a/app/services/invoices/payments/stripe_service.rb b/app/services/invoices/payments/stripe_service.rb
@@ -29,10 +29,16 @@ def update_payment_status(organization_id:, status:, stripe_payment:)
         result.invoice = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
+        payment.status = status
+
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
 
         update_invoice_payment_status(
-          payment_status: payment.payment_provider&.determine_payment_status(status),
+          payment_status: payable_payment_status,
           processing: status == "processing"
         )
 

diff --git a/app/services/payment_requests/payments/adyen_service.rb b/app/services/payment_requests/payments/adyen_service.rb
@@ -43,9 +43,14 @@ def update_payment_status(provider_payment_id:, status:, metadata: {})
         result.payable = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
+        payment.status = status
+
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
 
-        payable_payment_status = payment.payment_provider&.determine_payment_status(status)
         update_payable_payment_status(payment_status: payable_payment_status)
         update_invoices_payment_status(payment_status: payable_payment_status)
         reset_customer_dunning_campaign_status(payable_payment_status)

diff --git a/app/services/payment_requests/payments/cashfree_service.rb b/app/services/payment_requests/payments/cashfree_service.rb
@@ -15,38 +15,6 @@ def initialize(payable = nil)
         super
       end
 
-      def call
-        result.payable = payable
-        return result unless should_process_payment?
-
-        unless payable.total_amount_cents.positive?
-          update_payable_payment_status(payment_status: :succeeded)
-          return result
-        end
-
-        payable.increment_payment_attempts!
-
-        payment = Payment.new(
-          payable: payable,
-          payment_provider_id: cashfree_payment_provider.id,
-          payment_provider_customer_id: customer.cashfree_customer.id,
-          amount_cents: payable.total_amount_cents,
-          amount_currency: payable.currency.upcase,
-          provider_payment_id: payable.id, # NOTE: We are not creating a resource on cashfree's sude.
-          status: :pending
-        )
-        payment.save!
-
-        payable_payment_status = payable_payment_status(payment.status)
-        update_payable_payment_status(payment_status: payable_payment_status)
-        update_invoices_payment_status(payment_status: payable_payment_status)
-
-        Integrations::Aggregator::Payments::CreateJob.perform_later(payment:) if payment.should_sync_payment?
-
-        result.payment = payment
-        result
-      end
-
       def generate_payment_url
         return result unless should_process_payment?
 
@@ -71,9 +39,14 @@ def update_payment_status(organization_id:, status:, cashfree_payment:)
         result.payable = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
+        payment.status = status
+
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
 
-        payable_payment_status = payable_payment_status(status)
         update_payable_payment_status(payment_status: payable_payment_status)
         update_invoices_payment_status(payment_status: payable_payment_status)
         reset_customer_dunning_campaign_status(payable_payment_status)
@@ -152,14 +125,6 @@ def payment_url_params
         }
       end
 
-      def payable_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_payable_payment_status(payment_status:, deliver_webhook: true)
         UpdateService.call(
           payable: result.payable,

diff --git a/app/services/payment_requests/payments/gocardless_service.rb b/app/services/payment_requests/payments/gocardless_service.rb
@@ -32,9 +32,14 @@ def update_payment_status(provider_payment_id:, status:)
         result.payable = payment.payable
         return result if payment.payable.payment_succeeded?
 
-        payment.update!(status:)
+        payment.status = status
+
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
 
-        payable_payment_status = payment.payment_provider.determine_payment_status(status)
         update_payable_payment_status(payment_status: payable_payment_status)
         update_invoices_payment_status(payment_status: payable_payment_status)
         reset_customer_dunning_campaign_status(payable_payment_status)

diff --git a/app/services/payment_requests/payments/stripe_service.rb b/app/services/payment_requests/payments/stripe_service.rb
@@ -52,10 +52,17 @@ def update_payment_status(organization_id:, status:, stripe_payment:)
         payment.update!(status:)
 
         processing = status == "processing"
-        payment_status = payment.payment_provider.determine_payment_status(status)
-        update_payable_payment_status(payment_status:, processing:)
-        update_invoices_payment_status(payment_status:, processing:)
-        reset_customer_dunning_campaign_status(payment_status)
+        payment.status = status
+
+        payable_payment_status = payment.payment_provider&.determine_payment_status(payment.status)
+        if Payment::PAYABLE_PAYMENT_STATUS.include?(payable_payment_status)
+          payment.payable_payment_status = payable_payment_status
+        end
+        payment.save!
+
+        update_payable_payment_status(payment_status: payable_payment_status, processing:)
+        update_invoices_payment_status(payment_status: payable_payment_status, processing:)
+        reset_customer_dunning_campaign_status(payable_payment_status)
 
         PaymentRequestMailer.with(payment_request: payment.payable).requested.deliver_later if result.payable.payment_failed?
 

diff --git a/spec/factories/payments.rb b/spec/factories/payments.rb
@@ -22,6 +22,11 @@
       association :payment_provider_customer, factory: :gocardless_customer
     end
 
+    trait :cashfree_payment do
+      association :payment_provider, factory: :cashfree_provider
+      association :payment_provider_customer, factory: :cashfree_customer
+    end
+
     trait :requires_action do
       status { 'requires_action' }
       provider_payment_data do

diff --git a/spec/services/payment_requests/payments/cashfree_service_spec.rb b/spec/services/payment_requests/payments/cashfree_service_spec.rb
@@ -46,91 +46,11 @@
     )
   end
 
-  describe ".call" do
-    before do
-      cashfree_payment_provider
-      cashfree_customer
-    end
-
-    it "creates a cashfree payment", aggregate_failure: true do
-      result = cashfree_service.call
-
-      expect(result).to be_success
-
-      expect(result.payable).to be_payment_pending
-      expect(result.payable.payment_attempts).to eq(1)
-      expect(result.payable.reload.ready_for_payment_processing).to eq(true)
-
-      expect(result.payment.id).to be_present
-      expect(result.payment.payable).to eq(payment_request)
-      expect(result.payment.payment_provider).to eq(cashfree_payment_provider)
-      expect(result.payment.payment_provider_customer).to eq(cashfree_customer)
-      expect(result.payment.amount_cents).to eq(payment_request.total_amount_cents)
-      expect(result.payment.amount_currency).to eq(payment_request.currency)
-      expect(result.payment.status).to eq("pending")
-    end
-
-    context "with no payment provider" do
-      let(:cashfree_payment_provider) { nil }
-
-      it "does not creates a payment", aggregate_failure: true do
-        result = cashfree_service.call
-
-        expect(result).to be_success
-        expect(result.payable).to eq(payment_request)
-        expect(result.payment).to be_nil
-      end
-    end
-
-    context "with 0 amount" do
-      let(:payment_request) do
-        create(
-          :payment_request,
-          organization:,
-          customer:,
-          amount_cents: 0,
-          amount_currency: "EUR",
-          invoices: [invoice]
-        )
-      end
-
-      let(:invoice) do
-        create(
-          :invoice,
-          organization:,
-          customer:,
-          total_amount_cents: 0,
-          currency: "EUR"
-        )
-      end
-
-      it "does not creates a payment", aggregate_failure: true do
-        result = cashfree_service.call
-
-        expect(result).to be_success
-        expect(result.payable).to eq(payment_request)
-        expect(result.payment).to be_nil
-        expect(result.payable).to be_payment_succeeded
-      end
-    end
-
-    context "when customer does not exists" do
-      let(:cashfree_customer) { nil }
-
-      it "does not creates a adyen payment", aggregate_failure: true do
-        result = cashfree_service.call
-
-        expect(result).to be_success
-        expect(result.payable).to eq(payment_request)
-        expect(result.payment).to be_nil
-      end
-    end
-  end
-
   describe ".update_payment_status" do
     let(:payment) do
       create(
         :payment,
+        :cashfree_payment,
         payable: payment_request,
         provider_payment_id: payment_request.id,
         status: "pending"