From 09e2c6a4d6b9b074ac8f371dd2b8bf11b568f051 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Radek=20Hol=C3=BD?= <ahoj@radekholy24.cz>
Date: Sat, 4 May 2024 14:25:29 +0200
Subject: [PATCH] Fix not refunding the total amount by default

Fixes https://github.com/jazzband/django-payments/issues/401
---
 CHANGELOG.rst         |  1 +
 payments/models.py    | 29 ++++++++++++++++++++---------
 payments/test_core.py | 18 +++++++++---------
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 24a56695d..c14c8b243 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -14,6 +14,7 @@ v3.0.0
 - Stripe backends now supports webhooks
 - New :ref:`webhook settings <webhooks>`
 - Fixed PayPal backends not saving captured_amount when processing data.
+- Fixed ``base_payment.refund()`` not making any refund
 
 v2.0.0
 ------
diff --git a/payments/models.py b/payments/models.py
index 52e7e76da..fe5203aee 100644
--- a/payments/models.py
+++ b/payments/models.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import json
+import logging
 from typing import Iterable
 from uuid import uuid4
 
@@ -14,6 +15,8 @@
 from . import PurchasedItem
 from .core import provider_factory
 
+logger = logging.getLogger(__name__)
+
 
 class PaymentAttributeProxy:
     def __init__(self, payment):
@@ -212,15 +215,23 @@ def release(self):
     def refund(self, amount=None):
         if self.status != PaymentStatus.CONFIRMED:
             raise ValueError("Only charged payments can be refunded.")
-        if amount:
-            if amount > self.captured_amount:
-                raise ValueError(
-                    "Refund amount can not be greater then captured amount"
-                )
-            provider = provider_factory(self.variant, self)
-            amount = provider.refund(self, amount)
-            self.captured_amount -= amount
-        if self.captured_amount == 0 and self.status != PaymentStatus.REFUNDED:
+        if amount and amount > self.captured_amount:
+            raise ValueError("Refund amount can not be greater then captured amount")
+        provider = provider_factory(self.variant, self)
+        amount = provider.refund(self, amount)
+        # If the initial amount is None, the code above has no chance to check whether
+        # the actual amount is greater than the captured amount before actually
+        # performing the refund. But since the refund has been performed already,
+        # raising an exception would just cause inconsistencies. Thus, logging an error.
+        if amount > self.captured_amount:
+            logger.error(
+                "Refund amount of payment %s greater than captured amount: %f > %f",
+                self.id,
+                amount,
+                self.captured_amount,
+            )
+        self.captured_amount -= amount
+        if self.captured_amount <= 0 and self.status != PaymentStatus.REFUNDED:
             self.change_status(PaymentStatus.REFUNDED)
         self.save()
 
diff --git a/payments/test_core.py b/payments/test_core.py
index 8c9190dd6..0ff939452 100644
--- a/payments/test_core.py
+++ b/payments/test_core.py
@@ -113,20 +113,20 @@ def test_refund_too_high_amount(self):
 
     @patch("payments.dummy.DummyProvider.refund")
     def test_refund_without_amount(self, mocked_refund_method):
-        refund_amount = None
+        captured_amount = Decimal("200")
         with patch.object(BasePayment, "save") as mocked_save_method:
             mocked_save_method.return_value = None
-            mocked_refund_method.return_value = refund_amount
+            mocked_refund_method.return_value = captured_amount
 
-            captured_amount = Decimal("200")
-            status = PaymentStatus.CONFIRMED
             payment = Payment(
-                variant="default", status=status, captured_amount=captured_amount
+                variant="default",
+                status=PaymentStatus.CONFIRMED,
+                captured_amount=captured_amount,
             )
-            payment.refund(refund_amount)
-            self.assertEqual(payment.status, status)
-            self.assertEqual(payment.captured_amount, captured_amount)
-        self.assertEqual(mocked_refund_method.call_count, 0)
+            payment.refund()
+            self.assertEqual(payment.status, PaymentStatus.REFUNDED)
+            self.assertEqual(payment.captured_amount, Decimal(0))
+        self.assertEqual(mocked_refund_method.call_count, 1)
 
     @patch("payments.dummy.DummyProvider.refund")
     def test_refund_partial_success(self, mocked_refund_method):