Repository: saleor/saleor
Pull request: Fix payment tracking in checkout
Cancel all existing checkout active payments before creating a new checkout payment.
Fixes #4163 

# Impact

* [ ] New migrations
* [x] New/Updated API fields or mutations
* [ ] Deprecated API fields or mutations
* [ ] Removed API types, fields, or mutations
* [ ] Documentation needs to be updated

# Pull Request Checklist

<!-- Please keep this section. It will make the maintainer's life easier. -->

* [ ] Privileged queries and mutations are guarded by proper permission checks
* [ ] Database queries are optimized and the number of queries is constant
* [ ] Database migration files are up to date
* [x] The changes are tested
* [ ] GraphQL schema and type definitions are up to date
* [ ] Changes are mentioned in the changelog


Problem statement:
Payment tracking is inconsistent in checkout GraphQL API
### What I'm trying to achieve
In certain cases completing an order will only capture partially.

### Steps to reproduce the problem
1. Create a checkout with one item
2. Call `checkoutPaymentCreate`, the checkout object will now have one active payment that covers the entire total
3. Add a line to the order, there is now one payment but it's not enough to cover the total
4. Call `checkoutPaymentCreate` again, there are now two active payments for this checkout
5. Add another line that is worth less than the amount of the first payment
6. The checkout API will let you call `checkoutComplete` but will only capture the last payment

### What I expected to happen
I would expect to get an error when attempting to complete the order if the sum of all payments does not equal the total gross. I would expect either all payments to be captured or previous payments to be canceled when creating a new one if multiple payments are not supported.

Hints from issue discussion:

Commit message: Drop unused import

Patch to review:
diff --git a/saleor/checkout/models.py b/saleor/checkout/models.py
index 2e96e47adc8..cc7e3561a7a 100644
--- a/saleor/checkout/models.py
+++ b/saleor/checkout/models.py
@@ -1,5 +1,4 @@
 """Checkout-related ORM models."""
-from operator import attrgetter
 from typing import TYPE_CHECKING, Optional
 from uuid import uuid4
 
@@ -139,8 +138,7 @@ def get_line(self, variant: "ProductVariant") -> Optional["CheckoutLine"]:
         return next(matching_lines, None)
 
     def get_last_active_payment(self) -> Optional["Payment"]:
-        payments = [payment for payment in self.payments.all() if payment.is_active]
-        return max(payments, default=None, key=attrgetter("pk"))
+        return self.payments.filter(is_active=True).last()
 
     def set_country(
         self, country_code: str, commit: bool = False, replace: bool = True
diff --git a/saleor/checkout/tests/test_checkout.py b/saleor/checkout/tests/test_checkout.py
index 1020a3c1fdf..b7fd562647d 100644
--- a/saleor/checkout/tests/test_checkout.py
+++ b/saleor/checkout/tests/test_checkout.py
@@ -18,6 +18,7 @@
 from ...discount.models import NotApplicable, Voucher
 from ...order import OrderEvents, OrderEventsEmails
 from ...order.models import OrderEvent
+from ...payment.models import Payment
 from ...plugins.manager import get_plugins_manager
 from ...shipping.models import ShippingZone
 from ...tests.utils import flush_post_commit_hooks
@@ -26,12 +27,14 @@
 from ..utils import (
     add_variant_to_checkout,
     add_voucher_to_checkout,
+    cancel_active_payments,
     change_billing_address_in_checkout,
     change_shipping_address_in_checkout,
     clear_shipping_method,
     create_order,
     get_voucher_discount_for_checkout,
     get_voucher_for_checkout,
+    is_fully_paid,
     is_valid_shipping_method,
     prepare_order_data,
     recalculate_checkout_discount,
@@ -1404,3 +1407,87 @@ def test_store_user_address_create_new_address_if_not_associated(address):
 
     assert user.addresses.count() == expected_user_addresses_count
     assert user.default_billing_address_id != address.pk
+
+
+def test_get_last_active_payment(checkout_with_payments):
+    # given
+    payment = Payment.objects.create(
+        gateway="mirumee.payments.dummy",
+        is_active=True,
+        checkout=checkout_with_payments,
+    )
+
+    # when
+    last_payment = checkout_with_payments.get_last_active_payment()
+
+    # then
+    assert last_payment.pk == payment.pk
+
+
+def test_is_fully_paid(checkout_with_item, payment_dummy):
+    checkout = checkout_with_item
+    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
+    payment = payment_dummy
+    payment.is_active = True
+    payment.order = None
+    payment.total = total.gross.amount
+    payment.currency = total.gross.currency
+    payment.checkout = checkout
+    payment.save()
+    is_paid = is_fully_paid(checkout, list(checkout), None)
+    assert is_paid
+
+
+def test_is_fully_paid_many_payments(checkout_with_item, payment_dummy):
+    checkout = checkout_with_item
+    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
+    payment = payment_dummy
+    payment.is_active = True
+    payment.order = None
+    payment.total = total.gross.amount - 1
+    payment.currency = total.gross.currency
+    payment.checkout = checkout
+    payment.save()
+    payment2 = payment_dummy
+    payment2.pk = None
+    payment2.is_active = True
+    payment2.order = None
+    payment2.total = 1
+    payment2.currency = total.gross.currency
+    payment2.checkout = checkout
+    payment2.save()
+    is_paid = is_fully_paid(checkout, list(checkout), None)
+    assert is_paid
+
+
+def test_is_fully_paid_partially_paid(checkout_with_item, payment_dummy):
+    checkout = checkout_with_item
+    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
+    payment = payment_dummy
+    payment.is_active = True
+    payment.order = None
+    payment.total = total.gross.amount - 1
+    payment.currency = total.gross.currency
+    payment.checkout = checkout
+    payment.save()
+    is_paid = is_fully_paid(checkout, list(checkout), None)
+    assert not is_paid
+
+
+def test_is_fully_paid_no_payment(checkout_with_item):
+    checkout = checkout_with_item
+    is_paid = is_fully_paid(checkout, list(checkout), None)
+    assert not is_paid
+
+
+def test_cancel_active_payments(checkout_with_payments):
+    # given
+    checkout = checkout_with_payments
+    count_active = checkout.payments.filter(is_active=True).count()
+    assert count_active != 0
+
+    # when
+    cancel_active_payments(checkout)
+
+    # then
+    assert checkout.payments.filter(is_active=True).count() == 0
diff --git a/saleor/checkout/utils.py b/saleor/checkout/utils.py
index 0a08fbba3d2..1efa5649c16 100644
--- a/saleor/checkout/utils.py
+++ b/saleor/checkout/utils.py
@@ -6,6 +6,7 @@
 from django.core.exceptions import ValidationError
 from django.db import transaction
 from django.db.models import Max, Min, Sum
+from django.db.models.functions import Coalesce
 from django.utils import timezone
 from django.utils.encoding import smart_text
 from django.utils.translation import get_language
@@ -760,8 +761,9 @@ def is_fully_paid(
 
     Note that these payments may not be captured or charged at all.
     """
-    payments = [payment for payment in checkout.payments.all() if payment.is_active]
-    total_paid = sum([p.total for p in payments])
+    total_paid = checkout.payments.filter(is_active=True).aggregate(
+        total_sum=Coalesce(Sum("total"), 0)
+    )["total_sum"]
     checkout_total = (
         calculations.checkout_total(checkout=checkout, lines=lines, discounts=discounts)
         - checkout.get_total_gift_cards_balance()
@@ -804,3 +806,7 @@ def clean_checkout(
             "Provided payment methods can not cover the checkout's total amount",
             code=CheckoutErrorCode.CHECKOUT_NOT_FULLY_PAID.value,
         )
+
+
+def cancel_active_payments(checkout: Checkout):
+    checkout.payments.filter(is_active=True).update(is_active=False)
diff --git a/saleor/graphql/checkout/tests/test_checkout.py b/saleor/graphql/checkout/tests/test_checkout.py
index bb7d9e40907..7661c0a6acc 100644
--- a/saleor/graphql/checkout/tests/test_checkout.py
+++ b/saleor/graphql/checkout/tests/test_checkout.py
@@ -13,7 +13,7 @@
 from ....checkout import calculations
 from ....checkout.error_codes import CheckoutErrorCode
 from ....checkout.models import Checkout
-from ....checkout.utils import add_variant_to_checkout, is_fully_paid
+from ....checkout.utils import add_variant_to_checkout
 from ....core.payments import PaymentInterface
 from ....core.taxes import zero_money
 from ....order.models import Order
@@ -2703,59 +2703,3 @@ def test_clean_checkout_no_payment(checkout_with_item, shipping_method, address)
 
     msg = "Provided payment methods can not cover the checkout's total amount"
     assert e.value.error_list[0].message == msg
-
-
-def test_is_fully_paid(checkout_with_item, payment_dummy):
-    checkout = checkout_with_item
-    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
-    payment = payment_dummy
-    payment.is_active = True
-    payment.order = None
-    payment.total = total.gross.amount
-    payment.currency = total.gross.currency
-    payment.checkout = checkout
-    payment.save()
-    is_paid = is_fully_paid(checkout, list(checkout), None)
-    assert is_paid
-
-
-def test_is_fully_paid_many_payments(checkout_with_item, payment_dummy):
-    checkout = checkout_with_item
-    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
-    payment = payment_dummy
-    payment.is_active = True
-    payment.order = None
-    payment.total = total.gross.amount - 1
-    payment.currency = total.gross.currency
-    payment.checkout = checkout
-    payment.save()
-    payment2 = payment_dummy
-    payment2.pk = None
-    payment2.is_active = True
-    payment2.order = None
-    payment2.total = 1
-    payment2.currency = total.gross.currency
-    payment2.checkout = checkout
-    payment2.save()
-    is_paid = is_fully_paid(checkout, list(checkout), None)
-    assert is_paid
-
-
-def test_is_fully_paid_partially_paid(checkout_with_item, payment_dummy):
-    checkout = checkout_with_item
-    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
-    payment = payment_dummy
-    payment.is_active = True
-    payment.order = None
-    payment.total = total.gross.amount - 1
-    payment.currency = total.gross.currency
-    payment.checkout = checkout
-    payment.save()
-    is_paid = is_fully_paid(checkout, list(checkout), None)
-    assert not is_paid
-
-
-def test_is_fully_paid_no_payment(checkout_with_item):
-    checkout = checkout_with_item
-    is_paid = is_fully_paid(checkout, list(checkout), None)
-    assert not is_paid
diff --git a/saleor/graphql/payment/mutations.py b/saleor/graphql/payment/mutations.py
index 189fcc81e97..82a0bc743fb 100644
--- a/saleor/graphql/payment/mutations.py
+++ b/saleor/graphql/payment/mutations.py
@@ -3,6 +3,7 @@
 from django.core.exceptions import ValidationError
 
 from ...checkout import calculations
+from ...checkout.utils import cancel_active_payments
 from ...core.permissions import OrderPermissions
 from ...core.taxes import zero_taxed_money
 from ...core.utils import get_client_ip
@@ -140,6 +141,8 @@ def perform_mutation(cls, _root, info, checkout_id, **data):
         cls.clean_payment_amount(info, checkout_total, amount)
         extra_data = {"customer_user_agent": info.context.META.get("HTTP_USER_AGENT")}
 
+        cancel_active_payments(checkout)
+
         payment = create_payment(
             gateway=gateway,
             payment_token=data["token"],
@@ -150,7 +153,7 @@ def perform_mutation(cls, _root, info, checkout_id, **data):
             customer_ip_address=get_client_ip(info.context),
             checkout=checkout,
         )
-        return CheckoutPaymentCreate(payment=payment)
+        return CheckoutPaymentCreate(payment=payment, checkout=checkout)
 
 
 class PaymentCapture(BaseMutation):
diff --git a/saleor/graphql/payment/tests/test_payment.py b/saleor/graphql/payment/tests/test_payment.py
index 0ac78af507a..8054b70b7fe 100644
--- a/saleor/graphql/payment/tests/test_payment.py
+++ b/saleor/graphql/payment/tests/test_payment.py
@@ -73,7 +73,7 @@ def test_payment_void_gateway_error(
     assert not txn.is_success
 
 
-CREATE_QUERY = """
+CREATE_PAYMENT_MUTATION = """
     mutation CheckoutPaymentCreate($checkoutId: ID!, $input: PaymentInput!) {
         checkoutPaymentCreate(checkoutId: $checkoutId, input: $input) {
             payment {
@@ -109,7 +109,7 @@ def test_checkout_add_payment_without_shipping_method_and_not_shipping_required(
             "amount": total.gross.amount,
         },
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
     assert not data["paymentErrors"]
@@ -145,7 +145,7 @@ def test_checkout_add_payment_without_shipping_method_with_shipping_required(
             "amount": total.gross.amount,
         },
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
 
@@ -172,7 +172,7 @@ def test_checkout_add_payment_with_shipping_method_and_shipping_required(
             "amount": total.gross.amount,
         },
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
 
@@ -208,7 +208,7 @@ def test_checkout_add_payment(
             "amount": total.gross.amount,
         },
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
 
@@ -241,7 +241,7 @@ def test_checkout_add_payment_default_amount(
         "checkoutId": checkout_id,
         "input": {"gateway": DUMMY_GATEWAY, "token": "sample-token"},
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
     assert not data["paymentErrors"]
@@ -277,7 +277,7 @@ def test_checkout_add_payment_bad_amount(
             ),
         },
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
     assert (
@@ -299,7 +299,7 @@ def test_checkout_add_payment_not_supported_gateways(
         "checkoutId": checkout_id,
         "input": {"gateway": DUMMY_GATEWAY, "token": "sample-token", "amount": "10.0"},
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
     assert (
@@ -322,7 +322,7 @@ def test_use_checkout_billing_address_as_payment_billing(
             "amount": total.gross.amount,
         },
     }
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     content = get_graphql_content(response)
     data = content["data"]["checkoutPaymentCreate"]
 
@@ -338,7 +338,7 @@ def test_use_checkout_billing_address_as_payment_billing(
     address.save()
     checkout.billing_address = address
     checkout.save()
-    response = user_api_client.post_graphql(CREATE_QUERY, variables)
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
     get_graphql_content(response)
 
     checkout.refresh_from_db()
@@ -347,6 +347,49 @@ def test_use_checkout_billing_address_as_payment_billing(
     assert payment.billing_address_1 == address.street_address_1
 
 
+def test_create_payment_for_checkout_with_active_payments(
+    checkout_with_payments, user_api_client, address
+):
+    # given
+    checkout = checkout_with_payments
+    address.street_address_1 = "spanish-inqusition"
+    address.save()
+    checkout.billing_address = address
+    checkout.save()
+
+    total = calculations.checkout_total(checkout=checkout, lines=list(checkout))
+    checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
+    variables = {
+        "checkoutId": checkout_id,
+        "input": {
+            "gateway": DUMMY_GATEWAY,
+            "token": "sample-token",
+            "amount": total.gross.amount,
+        },
+    }
+
+    payments_count = checkout.payments.count()
+    previous_active_payments = checkout.payments.filter(is_active=True)
+    previous_active_payments_ids = list(
+        previous_active_payments.values_list("pk", flat=True)
+    )
+    assert len(previous_active_payments_ids) > 0
+
+    # when
+    response = user_api_client.post_graphql(CREATE_PAYMENT_MUTATION, variables)
+    content = get_graphql_content(response)
+
+    # then
+    data = content["data"]["checkoutPaymentCreate"]
+
+    assert not data["paymentErrors"]
+    checkout.refresh_from_db()
+    assert checkout.payments.all().count() == payments_count + 1
+    active_payments = checkout.payments.all().filter(is_active=True)
+    assert active_payments.count() == 1
+    assert active_payments.first().pk not in previous_active_payments_ids
+
+
 CAPTURE_QUERY = """
     mutation PaymentCapture($paymentId: ID!, $amount: Decimal!) {
         paymentCapture(paymentId: $paymentId, amount: $amount) {
diff --git a/saleor/tests/fixtures.py b/saleor/tests/fixtures.py
index 47840febbe9..09dd76d66d2 100644
--- a/saleor/tests/fixtures.py
+++ b/saleor/tests/fixtures.py
@@ -329,6 +329,21 @@ def checkout_with_voucher_percentage_and_shipping(
     return checkout
 
 
+@pytest.fixture
+def checkout_with_payments(checkout):
+    Payment.objects.bulk_create(
+        [
+            Payment(
+                gateway="mirumee.payments.dummy", is_active=True, checkout=checkout
+            ),
+            Payment(
+                gateway="mirumee.payments.dummy", is_active=False, checkout=checkout
+            ),
+        ]
+    )
+    return checkout
+
+
 @pytest.fixture
 def address(db):  # pylint: disable=W0613
     return Address.objects.create(


Code review comments:
diff_hunk: @@ -139,8 +138,7 @@ def get_line(self, variant: "ProductVariant") -> Optional["CheckoutLine"]:
         return next(matching_lines, None)
 
     def get_last_active_payment(self) -> Optional["Payment"]:
-        payments = [payment for payment in self.payments.all() if payment.is_active]
-        return max(payments, default=None, key=attrgetter("pk"))
+        return self.payments.filter(is_active=True).last()
path: saleor/checkout/models.py
text: @user1:
I think the previous solution was done like that to save some database queries. If payments are prefetched then `all()` won't perform any queries while `filter()` will.

@author:
Good point. I will change it to the previous version.