From c792c16aaf328f652b910885a8bffc911d24f8c9 Mon Sep 17 00:00:00 2001 From: Tiago-Salles Date: Sun, 17 May 2026 17:52:07 +0000 Subject: [PATCH] feat: place a pending order for asynchronous payments PayGate does not confirm every payment while the user is still in the browser. The Multibanco reference (REFMB) hands the learner an entity/reference pair that can be paid at an ATM days later. The success callback used to run handle_payment_and_create_order, which asks PayGate whether the transaction is complete. For an unpaid reference the answer is "not yet", so a GatewayError was raised and the learner was shown a payment error page even though nothing had gone wrong. The success callback now never takes payment. It places the order in the Pending status -- unpaid, unfulfilled, no PaymentSource, no PaymentEvent and no post_checkout signal, so the learner is not enrolled and nothing reaches the financial manager -- and redirects to the orders MFE thank-you page. The payment is confirmed later, either by the server-to-server callback when the reference is paid, or lazily from the Order History page. handle_payment_and_create_order now recognises an existing Pending order and confirms it, instead of bailing out with "the basket already has an order" and dropping the payment on the floor. Pending and Payment Error are not an invention of this plugin: upstream ecommerce already declares them and already allows the Pending -> (Open, Payment Error) transitions. Nothing shipped ever set them because OSCAR_INITIAL_ORDER_STATUS is Open. This fills in a slot upstream left open, which is what lets NAU support asynchronous payments without patching ecommerce itself. For a card payment the server callback lands at almost the same instant as the browser redirect. If it wins, our insert fails on the unique constraint on Order.number; that IntegrityError is caught and the existing order is picked up, so a learner who has just paid is never sent to the error page. thank_you_url is optional. Without it the success callback falls back to the receipt page, so a deployment that has not configured it does not regress. Known limitation, accepted deliberately: nothing expires a Pending order. An unpaid reference leaves a submitted basket and a Pending order in place indefinitely and the learner cannot retry without support. See the module docstring of paygate/pending_orders.py for what closing it would take. Related to: fccn/nau-technical#923 --- paygate/pending_orders.py | 224 +++++++++++++++ paygate/processors.py | 23 ++ paygate/tests/test_views.py | 537 ++++++++++++++++++++++++++++++++++++ paygate/utils.py | 10 +- paygate/views.py | 165 +++++++++-- 5 files changed, 938 insertions(+), 21 deletions(-) create mode 100644 paygate/pending_orders.py diff --git a/paygate/pending_orders.py b/paygate/pending_orders.py new file mode 100644 index 0000000..70ddf02 --- /dev/null +++ b/paygate/pending_orders.py @@ -0,0 +1,224 @@ +""" +Support for asynchronous PayGate payment methods. + +Some PayGate payment types are not confirmed while the user is still in front of +the browser. The most relevant one on NAU is the Multibanco reference (`REFMB`): +PayGate hands the user an entity/reference pair that can be paid at an ATM or on +a home-banking site at any point during the following days. + +For those payment types the previous design was wrong: the "success" callback +(the URL PayGate redirects the user to after he presses "Continuar") ran +`handle_payment` immediately, which asks PayGate whether the transaction is +completed. For a Multibanco reference the answer is "not yet", so a `GatewayError` +was raised and the user was shown a payment error page even though nothing had +gone wrong. + +This module implements the two halves of the asynchronous flow: + +* :func:`place_pending_order` creates the `Order` in the `Pending` status as soon + as the user comes back from PayGate. The order is *not* paid and *not* + fulfilled -- `Order.is_fulfillable` is False for `Pending`, no `PaymentSource` + or `PaymentEvent` is attached and the `post_checkout` signal is not sent, so + the learner is not enrolled and nothing is sent to the financial manager. + Its only purpose is to make the in-flight payment visible to the user on the + Order History page. + +* :func:`confirm_pending_order` asks PayGate whether the payment has landed. When + it has, the payment is recorded against the existing order, the order moves to + `Open` and the regular Open edX fulfilment path runs, exactly as it does for a + synchronous card payment. + +Known limitation, accepted deliberately: nothing expires a `Pending` order. An +unpaid Multibanco reference leaves a submitted basket and a `Pending` order in +place indefinitely, and the learner has no self-service way to retry -- support +has to intervene. This is considered acceptable at NAU's REFMB volume. Closing +it would mean sending `REFMB_START_DATE`/`REFMB_END_DATE` to PayGate (see +`PayGate.get_transaction_parameters`, where they are currently commented out) so +both sides agree on a deadline, plus a management command that moves expired +`Pending` orders to `Payment Error` and reopens their basket. + +The `Pending` and `Payment Error` statuses used here are not an invention of this +plugin: upstream ecommerce already declares them in +`ecommerce.extensions.fulfillment.status.ORDER` and already allows the +`Pending -> (Open, Payment Error)` transitions in `OSCAR_ORDER_STATUS_PIPELINE`. +Nothing in the shipped code ever set them, because `OSCAR_INITIAL_ORDER_STATUS` +is `Open`. We are filling in a slot that upstream left open, which is what lets +NAU support asynchronous payments without patching ecommerce itself. +""" + +import logging + +from django.db import transaction +from oscar.core.loading import get_class, get_model + +from ecommerce.extensions.checkout.mixins import EdxOrderPlacementMixin +from ecommerce.extensions.fulfillment.status import ORDER + +from .processors import PayGate + +logger = logging.getLogger(__name__) + +Order = get_model("order", "Order") +NoShippingRequired = get_class("shipping.methods", "NoShippingRequired") +OrderTotalCalculator = get_class("checkout.calculators", "OrderTotalCalculator") + + +class PayGateOrderPlacement(EdxOrderPlacementMixin): + """ + Exposes the `EdxOrderPlacementMixin` order placement machinery outside of a + view. + + `EdxOrderPlacementMixin` is normally mixed into a Django view, which is where + it picks up `self.request` and `self.payment_processor` from. The lazy + resolution done from the Order History page has a request but is not a + checkout view, so this small adapter supplies both. + """ + + def __init__(self, request): + self.request = request + + @property + def payment_processor(self): + """ + An instance of the PayGate payment processor bound to the current site. + """ + return PayGate(self.request.site) + + +def place_pending_order(request, basket): + """ + Place an `Order` for `basket` in the `Pending` status, without taking payment + and without fulfilling it. + + This deliberately calls `place_order` instead of the more usual + `EdxOrderPlacementMixin.create_order`. `create_order` goes through + `handle_order_placement`, which ends in `handle_successful_order` and + therefore sends `post_checkout` -- that would enrol the learner and invoice + the purchase before a single cent has been paid. + + Arguments: + request (HttpRequest): the current request, used for the site and for the + order placement audit trail. + basket (Basket): the basket to place the order for. It must have a + strategy assigned. + + Returns: + Order: the newly created order, in the `Pending` status. + """ + placement = PayGateOrderPlacement(request) + + shipping_method = NoShippingRequired() + shipping_charge = shipping_method.calculate(basket) + order_total = OrderTotalCalculator().calculate(basket, shipping_charge) + + order = placement.place_order( + order_number=basket.order_number, + user=basket.owner, + basket=basket, + shipping_address=None, + shipping_method=shipping_method, + shipping_charge=shipping_charge, + billing_address=None, + order_total=order_total, + status=ORDER.PENDING, + request=request, + ) + basket.submit() + + logger.info( + "PayGate placed pending order [%s] for basket [%d]", + order.number, + basket.id, + ) + return order + + +def confirm_pending_order(request, basket, order, response=None): + """ + Ask PayGate whether `basket` has actually been paid and, if it has, record the + payment against `order` and fulfil it. + + This runs exactly the same `handle_payment` call the synchronous flow runs, so + a payment confirmed lazily from the Order History page is recorded in the same + way as one confirmed by the server-to-server callback. + + Arguments: + request (HttpRequest): the current request. + basket (Basket): the basket being paid. It must have a strategy assigned. + order (Order): the existing `Pending` order to confirm. + response (dict): the raw PayGate response to hand to the processor. + PayGate's `handle_processor_response` ignores it and re-queries the + gateway by order number, but it is passed through for the audit trail. + + Returns: + Order: the refreshed order. + + Raises: + GatewayError: PayGate has not confirmed the payment yet. The order is left + in `Pending` so it can be retried later. This is the expected outcome + for an unpaid Multibanco reference and is not an error. + PaymentError: the payment failed in a way that will not resolve itself. + """ + placement = PayGateOrderPlacement(request) + + # The lazy resolution done from the Order History page and the + # server-to-server callback can both reach this point for the same order at + # the same time. Without a lock both would pass the "still Pending" check and + # both would record a `PaymentSource`/`PaymentEvent`, so take a row lock on + # the order and re-check its status while holding it. The loser of the race + # returns the already confirmed order untouched. + with transaction.atomic(): + order = Order.objects.select_for_update().get(pk=order.pk) + if order.status != ORDER.PENDING: + logger.info( + "PayGate order [%s] is already in status [%s], nothing to confirm", + order.number, + order.status, + ) + return order + + # Raises GatewayError when PayGate has not confirmed the payment yet, which + # the caller is expected to treat as "still pending". + placement.handle_payment(response or {}, basket) + + # `handle_payment` only caches the payment source and event, because in the + # normal flow the order does not exist yet. Here it does, so attach them. + placement.save_payment_details(order) + + order.set_status(ORDER.OPEN) + + logger.info( + "PayGate confirmed payment for order [%s], moved to [%s]", + order.number, + order.status, + ) + + # Runs the regular post-placement path: audit log, offer assignments and the + # `post_checkout` signal that enrols the learner and sends the transaction to + # the financial manager. + placement.handle_successful_order(order, request) + + try: + placement.handle_post_order(order) + except Exception: # pylint: disable=broad-except + placement.log_order_placement_exception(order.number, basket.id) + + order.refresh_from_db() + return order + + +def mark_order_as_payment_error(order): + """ + Move `order` to the `Payment Error` status, if the pipeline allows it. + + Only `Pending` orders can reach `Payment Error`, so this is a no-op for an + order that has already been paid or fulfilled. + """ + if order.status != ORDER.PENDING: + return order + + order.set_status(ORDER.PAYMENT_ERROR) + logger.warning( + "PayGate marked order [%s] as [%s]", order.number, order.status + ) + return order diff --git a/paygate/processors.py b/paygate/processors.py index 727b0cf..1e82ceb 100644 --- a/paygate/processors.py +++ b/paygate/processors.py @@ -211,6 +211,29 @@ def cancel_url(self): ) ) + @property + def thank_you_url(self): + """ + The destination Thank-You page URL where the user is redirected after the PayGate + redirects him back to the Open edX Ecommerce success callback. + + This URL replaces the previous behaviour of synchronously running + `handle_payment_and_create_order` on the success callback, which was failing for + asynchronous payments (MB) because the upstream payment is not yet confirmed + when the user comes back. The Thank-You page is part of the ecommerce + micro-frontend and links to the user's Order History page, where the payment + status is then lazily resolved per order. + + Configure it by setting `thank_you_url` on the payment processor configuration, + for example: + + thank_you_url: https://lms.example.com/orders/thank-you + + When it is not set this returns ``None`` and the success callback falls back to + the previous receipt page behaviour, so existing deployments do not regress. + """ + return self.configuration.get("thank_you_url", None) + def get_transaction_parameters( self, basket, request=None, use_client_side_checkout=False, **kwargs ): # pylint: disable=unused-argument, too-many-locals diff --git a/paygate/tests/test_views.py b/paygate/tests/test_views.py index 524a49f..a9c1114 100644 --- a/paygate/tests/test_views.py +++ b/paygate/tests/test_views.py @@ -1,14 +1,24 @@ +import copy import json +from decimal import Decimal +from urllib.parse import parse_qsl, urlparse import mock from django.conf import settings +from django.db import IntegrityError from django.test import override_settings from django.urls import reverse +from oscar.apps.payment.exceptions import GatewayError from oscar.core.loading import get_model +from paygate.pending_orders import (confirm_pending_order, + mark_order_as_payment_error, + place_pending_order) from paygate.processors import PayGate from paygate.utils import get_receipt_page_url from ecommerce.courses.tests.factories import CourseFactory +from ecommerce.extensions.fulfillment.status import ORDER +from ecommerce.extensions.payment.processors import HandledProcessorResponse from ecommerce.extensions.test.factories import create_basket, create_order from ecommerce.tests.factories import UserFactory from ecommerce.tests.testcases import TestCase @@ -412,3 +422,530 @@ def test_callback_server_duc(self, mock__make_api_json_request): order = Order.objects.all().first() self.assertEqual(order.basket.id, basket.id) self.assertTrue(len(Order.objects.all()) == 1) + + +PAYGATE_CONFIG_WITH_THANK_YOU = { + "edx": { + **settings.PAYMENT_PROCESSOR_CONFIG["edx"], + **{ + "paygate": { + "access_token": "PwdX_XXXX_YYYY", + "merchant_code": "NAU", + "api_checkout_url": "https://test.optimistic.blue/paygateWS/api/CheckOut", + "api_back_search_transactions": ( + "https://test.optimistic.blue/paygateWS/api/BackOfficeSearchTransactions" + ), + "api_basic_auth_user": "NAU", + "api_basic_auth_pass": "APassword", + "thank_you_url": "https://orders.example.com/thank-you", + } + }, + } +} + + +def _thank_you_config(thank_you_url): + """ + A copy of `PAYGATE_CONFIG_WITH_THANK_YOU` with a different `thank_you_url`. + """ + config = copy.deepcopy(PAYGATE_CONFIG_WITH_THANK_YOU) + config["edx"]["paygate"]["thank_you_url"] = thank_you_url + return config + + +class PayGateAsynchronousPaymentTests(TestCase): + """ + Tests for the asynchronous payment flow. + + An asynchronous payment method -- on NAU the Multibanco reference (`REFMB`) -- + is not confirmed while the user is still in the browser. The success callback + must therefore never take payment: it places a `Pending` order so the user can + see the in-flight payment, and the payment is confirmed later, either by the + server-to-server callback or lazily from the Order History page. + """ + + def create_basket_with_seat(self): + course = CourseFactory(id='a/b/c', name='Demo Course', partner=self.partner) + product = course.create_or_update_seat('test-certificate-type', False, 20) + basket = create_basket(site=self.site, owner=UserFactory(), empty=True) + basket.add_product(product) + basket.save() + return basket + + @staticmethod + def callback_data(basket): + return { + "is_paid": False, + "StatusCode": "P", + 'payment_ref': basket.order_number, + "paymentValue": "20.00EUR", + "payment_type_code": "REFMB", + } + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch.object(PayGate, "_make_api_json_request") + def test_success_callback_places_a_pending_order_and_redirects_to_thank_you( + self, mock__make_api_json_request, + ): + """ + With `thank_you_url` configured the success callback must: + * NOT call the PayGate BackOfficeSearchTransactions API, because an + asynchronous payment is not confirmed yet and asking would raise a + GatewayError and show the user a bogus payment error. + * create the order in the `Pending` status, so the user can see it. + * NOT record any payment against it. + * redirect the user to the configured Thank-You URL with the order number. + """ + basket = self.create_basket_with_seat() + + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(response.status_code, 302) + self.assertIn("https://orders.example.com/thank-you", response['Location']) + self.assertIn(f"order_number={basket.order_number}", response['Location']) + + order = Order.objects.get(number=basket.order_number) + self.assertEqual(order.status, ORDER.PENDING) + # A pending order must never be fulfilled: no enrolment, no invoice. + self.assertFalse(order.is_fulfillable) + self.assertEqual(order.sources.count(), 0) + self.assertEqual(order.payment_events.count(), 0) + + mock__make_api_json_request.assert_not_called() + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch.object(PayGate, "_make_api_json_request") + def test_success_callback_is_idempotent(self, mock__make_api_json_request): + """ + The user may reload the success callback. That must not create a second + order nor change the status of the one already placed. + """ + basket = self.create_basket_with_seat() + + self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(Order.objects.filter(number=basket.order_number).count(), 1) + self.assertEqual( + Order.objects.get(number=basket.order_number).status, ORDER.PENDING + ) + mock__make_api_json_request.assert_not_called() + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch.object(PayGate, "handle_processor_response") + def test_server_callback_confirms_a_pending_order(self, mock_handle_processor_response): + """ + When the learner finally pays the Multibanco reference, PayGate calls the + server-to-server callback. That must confirm the existing `Pending` order + rather than bail out because "the basket already has an order", and it must + record the payment and fulfil the order. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="MB-TXN-1", + total=Decimal("20.00"), + currency="EUR", + card_number="REFMB", + card_type="REFMB", + ) + basket = self.create_basket_with_seat() + + # The user came back from PayGate first: a pending order exists. + self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + self.assertEqual( + Order.objects.get(number=basket.order_number).status, ORDER.PENDING + ) + + # PayGate now notifies that the reference has been paid. + response = self.client.post( + reverse("ecommerce_plugin_paygate:callback_server"), + json.dumps(self.callback_data(basket)), + content_type="application/json", + ) + + self.assertEqual(response.status_code, 200) + order = Order.objects.get(number=basket.order_number) + self.assertNotEqual(order.status, ORDER.PENDING) + self.assertEqual(order.sources.count(), 1) + self.assertEqual(order.payment_events.count(), 1) + self.assertEqual(order.sources.first().reference, "MB-TXN-1") + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch.object(PayGate, "handle_processor_response") + def test_server_callback_ignores_an_already_paid_order(self, mock_handle_processor_response): + """ + Duplicated server callbacks for an order that is already paid must be + ignored, and must not record the payment twice. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="MB-TXN-1", + total=Decimal("20.00"), + currency="EUR", + card_number="REFMB", + card_type="REFMB", + ) + basket = self.create_basket_with_seat() + self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + for __ in range(2): + self.client.post( + reverse("ecommerce_plugin_paygate:callback_server"), + json.dumps(self.callback_data(basket)), + content_type="application/json", + ) + + order = Order.objects.get(number=basket.order_number) + self.assertEqual(order.sources.count(), 1) + self.assertEqual(order.payment_events.count(), 1) + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch.object(PayGate, "handle_processor_response") + def test_success_callback_uses_thank_you_for_an_already_fulfilled_order( + self, mock_handle_processor_response, + ): + """ + For a synchronous method the server-to-server callback normally fulfils the + order before the browser redirect arrives. The user must still reach the + Thank-You page, carrying the number of the order that already exists -- no + second order, no receipt page and no extra payment recorded. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="CARD-TXN-4", + total=Decimal("20.00"), + currency="EUR", + card_number="xxxx-1111", + card_type="VISA", + ) + basket = self.create_basket_with_seat() + + # The server callback lands first and fulfils the order. + self.client.post( + reverse("ecommerce_plugin_paygate:callback_server"), + json.dumps(self.callback_data(basket)), + content_type="application/json", + ) + order = Order.objects.get(number=basket.order_number) + self.assertNotEqual(order.status, ORDER.PENDING) + + # The user is only now redirected back from PayGate. + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(response.status_code, 302) + self.assertIn("https://orders.example.com/thank-you", response['Location']) + self.assertIn(f"order_number={basket.order_number}", response['Location']) + self.assertEqual(Order.objects.filter(number=basket.order_number).count(), 1) + order.refresh_from_db() + self.assertEqual(order.sources.count(), 1) + self.assertEqual(order.payment_events.count(), 1) + + @mock.patch.object(PayGate, "handle_processor_response") + def test_success_callback_without_thank_you_url_keeps_the_synchronous_flow( + self, mock_handle_processor_response, + ): + """ + `thank_you_url` gates the whole asynchronous flow, not just the redirect + destination. Until a deployment configures it the previous behaviour is kept + untouched: the payment is handled synchronously, the order is created and + fulfilled, and the user is sent to the receipt page. No `Pending` order is + placed and nothing relies on the server-to-server callback landing later. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="CARD-TXN-3", + total=Decimal("20.00"), + currency="EUR", + card_number="xxxx-1111", + card_type="VISA", + ) + basket = self.create_basket_with_seat() + + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(response.status_code, 302) + self.assertIn("receipt", response['Location']) + mock_handle_processor_response.assert_called_once() + order = Order.objects.get(number=basket.order_number) + self.assertNotEqual(order.status, ORDER.PENDING) + self.assertEqual(order.sources.count(), 1) + self.assertEqual(order.payment_events.count(), 1) + + @mock.patch.object(PayGate, "handle_processor_response") + def test_success_callback_without_thank_you_url_still_shows_the_error_page( + self, mock_handle_processor_response, + ): + """ + The other half of the preserved behaviour: when PayGate cannot confirm the + payment the user reaches the error page, exactly as before. This is the known + bad outcome for an asynchronous payment method, and it is what configuring + `thank_you_url` fixes. + """ + mock_handle_processor_response.side_effect = GatewayError( + "PayGate couldn't double check if basket has been payed" + ) + basket = self.create_basket_with_seat() + + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(response.status_code, 302) + self.assertEqual(Order.objects.filter(number=basket.order_number).count(), 0) + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch("paygate.views.place_pending_order") + @mock.patch.object(PayGate, "handle_processor_response") + def test_success_callback_survives_losing_the_race_to_the_server_callback( + self, mock_handle_processor_response, mock_place_pending_order, + ): + """ + For a card payment the server-to-server callback lands at almost the same + instant as the browser redirect. If it creates the order first, the unique + constraint on `Order.number` makes our insert fail -- and the user, who has + just paid successfully, must NOT be sent to the payment error page. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="CARD-TXN-1", + total=Decimal("20.00"), + currency="EUR", + card_number="xxxx-1111", + card_type="VISA", + ) + basket = self.create_basket_with_seat() + + # The server callback wins the race and fulfils the order. + self.client.post( + reverse("ecommerce_plugin_paygate:callback_server"), + json.dumps(self.callback_data(basket)), + content_type="application/json", + ) + # Our insert then loses on the unique constraint. + mock_place_pending_order.side_effect = IntegrityError("duplicate order number") + + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + # The user reaches the Thank-You page, not the error page. + self.assertEqual(response.status_code, 302) + self.assertIn("https://orders.example.com/thank-you", response['Location']) + self.assertIn(f"order_number={basket.order_number}", response['Location']) + # And there is exactly one order, the paid one. + self.assertEqual(Order.objects.filter(number=basket.order_number).count(), 1) + self.assertEqual(Order.objects.get(number=basket.order_number).sources.count(), 1) + + @override_settings(PAYMENT_PROCESSOR_CONFIG=PAYGATE_CONFIG_WITH_THANK_YOU) + @mock.patch("paygate.views.place_pending_order") + @mock.patch.object(PayGate, "handle_processor_response") + def test_success_callback_survives_a_non_integrity_placement_error( + self, mock_handle_processor_response, mock_place_pending_order, + ): + """ + The losing side of the race does not always fail on the `Order.number` + unique constraint: `basket.submit()` rejects an already submitted basket + with a different exception. The user has still paid, so he must reach the + Thank-You page and not the error page. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="CARD-TXN-2", + total=Decimal("20.00"), + currency="EUR", + card_number="xxxx-1111", + card_type="VISA", + ) + basket = self.create_basket_with_seat() + + # The server callback wins the race and fulfils the order. + self.client.post( + reverse("ecommerce_plugin_paygate:callback_server"), + json.dumps(self.callback_data(basket)), + content_type="application/json", + ) + mock_place_pending_order.side_effect = ValueError("basket already submitted") + + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(response.status_code, 302) + self.assertIn("https://orders.example.com/thank-you", response['Location']) + self.assertIn(f"order_number={basket.order_number}", response['Location']) + self.assertEqual(Order.objects.filter(number=basket.order_number).count(), 1) + self.assertEqual(Order.objects.get(number=basket.order_number).sources.count(), 1) + + @override_settings( + PAYMENT_PROCESSOR_CONFIG=_thank_you_config( + "https://orders.example.com/thank-you?locale=pt&" + ) + ) + @mock.patch.object(PayGate, "_make_api_json_request") + def test_success_callback_builds_a_valid_thank_you_url( + self, mock__make_api_json_request, # pylint: disable=unused-argument + ): + """ + A `thank_you_url` that already carries a query string, and even a stray + trailing separator, must still produce a well formed redirect. + """ + basket = self.create_basket_with_seat() + + response = self.client.get( + reverse("ecommerce_plugin_paygate:callback_success"), + self.callback_data(basket), + ) + + self.assertEqual(response.status_code, 302) + parsed = urlparse(response['Location']) + self.assertEqual(parsed.path, "/thank-you") + self.assertEqual( + dict(parse_qsl(parsed.query)), + {"locale": "pt", "order_number": basket.order_number}, + ) + + +class PayGatePendingOrderTests(TestCase): + """ + Tests for `paygate.pending_orders`, the module that places and confirms the + orders of asynchronous payments. + """ + + def setUp(self): + super().setUp() + course = CourseFactory(id='a/b/c', name='Demo Course', partner=self.partner) + product = course.create_or_update_seat('test-certificate-type', False, 20) + self.basket = create_basket(site=self.site, owner=UserFactory(), empty=True) + self.basket.add_product(product) + self.basket.save() + # `self.request` comes from SiteMixin, already bound to the test site and + # registered with crum, which the order placement machinery relies on. + self.request.user = self.basket.owner + + def test_place_pending_order_does_not_fulfil(self): + order = place_pending_order(self.request, self.basket) + + self.assertEqual(order.status, ORDER.PENDING) + self.assertFalse(order.is_fulfillable) + self.assertEqual(order.sources.count(), 0) + self.assertEqual(order.payment_events.count(), 0) + self.basket.refresh_from_db() + self.assertEqual(self.basket.status, Basket.SUBMITTED) + + @mock.patch.object(PayGate, "handle_processor_response") + def test_confirm_pending_order_records_payment_and_fulfils( + self, mock_handle_processor_response, + ): + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="MB-TXN-2", + total=Decimal("20.00"), + currency="EUR", + card_number="REFMB", + card_type="REFMB", + ) + order = place_pending_order(self.request, self.basket) + + order = confirm_pending_order(self.request, self.basket, order, {}) + + # The order left the Pending status and carries the payment. + self.assertNotEqual(order.status, ORDER.PENDING) + self.assertEqual(order.sources.count(), 1) + self.assertEqual(order.payment_events.count(), 1) + source = order.sources.first() + self.assertEqual(source.reference, "MB-TXN-2") + self.assertEqual(source.amount_debited, Decimal("20.00")) + + @mock.patch.object(PayGate, "handle_processor_response") + def test_confirm_pending_order_leaves_it_pending_when_not_payed_yet( + self, mock_handle_processor_response, + ): + """ + An unpaid Multibanco reference makes PayGate raise a GatewayError. That is + the normal, expected outcome and must leave the order untouched so it can be + retried on the next visit to the Order History page. + """ + mock_handle_processor_response.side_effect = GatewayError( + "PayGate couldn't double check if basket has been payed" + ) + order = place_pending_order(self.request, self.basket) + + with self.assertRaises(GatewayError): + confirm_pending_order(self.request, self.basket, order, {}) + + order.refresh_from_db() + self.assertEqual(order.status, ORDER.PENDING) + self.assertEqual(order.sources.count(), 0) + self.assertEqual(order.payment_events.count(), 0) + + @mock.patch.object(PayGate, "handle_processor_response") + def test_confirm_pending_order_is_idempotent( + self, mock_handle_processor_response, + ): + """ + The lazy resolution from the Order History page and the server-to-server + callback can both confirm the same order. The second confirmation must be + a no-op instead of recording a second payment source and event. + """ + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="MB-TXN-4", + total=Decimal("20.00"), + currency="EUR", + card_number="REFMB", + card_type="REFMB", + ) + order = place_pending_order(self.request, self.basket) + order = confirm_pending_order(self.request, self.basket, order, {}) + status_after_first = order.status + + order = confirm_pending_order(self.request, self.basket, order, {}) + + self.assertEqual(order.status, status_after_first) + self.assertEqual(order.sources.count(), 1) + self.assertEqual(order.payment_events.count(), 1) + mock_handle_processor_response.assert_called_once() + + def test_mark_order_as_payment_error(self): + order = place_pending_order(self.request, self.basket) + + mark_order_as_payment_error(order) + + order.refresh_from_db() + self.assertEqual(order.status, ORDER.PAYMENT_ERROR) + + @mock.patch.object(PayGate, "handle_processor_response") + def test_mark_order_as_payment_error_leaves_a_paid_order_alone( + self, mock_handle_processor_response, + ): + mock_handle_processor_response.return_value = HandledProcessorResponse( + transaction_id="MB-TXN-3", + total=Decimal("20.00"), + currency="EUR", + card_number="REFMB", + card_type="REFMB", + ) + order = place_pending_order(self.request, self.basket) + order = confirm_pending_order(self.request, self.basket, order, {}) + status_before = order.status + + mark_order_as_payment_error(order) + + order.refresh_from_db() + self.assertEqual(order.status, status_before) diff --git a/paygate/utils.py b/paygate/utils.py index b4e1490..6e2dd3e 100644 --- a/paygate/utils.py +++ b/paygate/utils.py @@ -13,11 +13,19 @@ OrderNumberGenerator = get_class("order.utils", "OrderNumberGenerator") +def get_order(basket: Basket): + """ + Utility method that returns the Order for the Basket, or None if the basket + has not been ordered yet. + """ + return Order.objects.filter(number=basket.order_number).first() + + def order_exist(basket: Basket) -> bool: """ Utility method that check if there is an Order for the Basket """ - return Order.objects.filter(number=basket.order_number).exists() + return get_order(basket) is not None def get_basket(basket_id, request=None): diff --git a/paygate/views.py b/paygate/views.py index 80c1912..e8f604a 100644 --- a/paygate/views.py +++ b/paygate/views.py @@ -7,8 +7,9 @@ import logging import traceback from json.decoder import JSONDecodeError +from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse -from django.db import transaction +from django.db import IntegrityError, transaction from django.http import (HttpResponse, HttpResponseNotAllowed, HttpResponseServerError) from django.shortcuts import redirect @@ -20,10 +21,12 @@ from paygate.utils import get_receipt_page_url from ecommerce.extensions.checkout.mixins import EdxOrderPlacementMixin +from ecommerce.extensions.fulfillment.status import ORDER from .ip import allowed_client_ip, get_client_ip +from .pending_orders import confirm_pending_order, place_pending_order from .processors import PayGate -from .utils import get_basket_from_payment_ref, order_exist +from .utils import get_basket_from_payment_ref, get_order logger = logging.getLogger(__name__) @@ -101,12 +104,25 @@ def get_basket_and_record_response(self, request): logger.warning("Missing 'payment_ref' parameter from request") return basket, ppr - def handle_payment_and_create_order(self, request, basket, payment_processor_response): + def handle_payment_and_create_order(self, basket, payment_processor_response): """ - Handle payment and if need create_order + Handle payment and, if needed, create the order. + + Three cases are handled: + + * There is already a `Pending` order for the basket, because the user came + back from PayGate through the success callback before the payment was + confirmed. The payment is recorded against that existing order and the + order is fulfilled. + * There is already an order in any other status. This is a duplicated + server callback and is ignored. + * There is no order yet, which is the classic synchronous flow: handle the + payment, create the order and fulfil it. """ - if order_exist(basket): - # the basket already contains an order. + existing_order = get_order(basket) + + if existing_order and existing_order.status != ORDER.PENDING: + # the basket already contains a fulfilled order. # we could receive duplicated server callbacks. logger.warning( "PayGate callback the basket already has an order for basket [%d]", @@ -115,9 +131,24 @@ def handle_payment_and_create_order(self, request, basket, payment_processor_res return False try: + if existing_order: + # An asynchronous payment (e.g. a Multibanco reference) that has + # now been paid. Confirm the pending order that was placed when + # the user returned from PayGate. + confirm_pending_order( + self.request, + basket, + existing_order, + payment_processor_response.response, + ) + return True + # This method have to be invoked in order to handle a payment, # this method could raise an PaymentError exception. self.handle_payment(payment_processor_response.response, basket) + + order = self.create_new_order(self.request, basket) + self.run_post_order(basket, order) except PaymentError as exc: logger.exception( "PayGate server callback error while handling payment with a payment error for basket [%d]", @@ -126,6 +157,10 @@ def handle_payment_and_create_order(self, request, basket, payment_processor_res raise PayGateCallbackException( "Error while handling payment - payment error" ) from exc + except PayGateCallbackException: + # Already logged and already the right exception type; let it through + # untouched instead of wrapping it in itself. + raise except Exception as exc: # pylint: disable=broad-except logger.exception( "PayGate server callback error while handling payment with another error for basket [%d]", @@ -134,9 +169,14 @@ def handle_payment_and_create_order(self, request, basket, payment_processor_res logger.error(traceback.format_exc()) raise PayGateCallbackException("Error while handling payment - other error") from exc - # create an order for the basket + return True + + def create_new_order(self, request, basket): + """ + Create an order for the basket. + """ try: - order = self.create_order(request, basket) + return self.create_order(request, basket) except Exception as exc: # pylint: disable=broad-except logger.exception( "PayGate server callback error while creating order for basket [%d]", @@ -144,7 +184,14 @@ def handle_payment_and_create_order(self, request, basket, payment_processor_res ) raise PayGateCallbackException("Error while creating order") from exc - # post order + def run_post_order(self, basket, order): + """ + Run the post-order actions, swallowing any error. + + Note that this is deliberately *not* called `handle_post_order`: that name + belongs to `EdxOrderPlacementMixin` and overriding it here would shadow the + implementation this method calls. + """ try: self.handle_post_order(order) except Exception: # pylint: disable=broad-except @@ -211,7 +258,7 @@ def post(self, request, *args, **kwargs): # pylint: disable=unused-argument ) try: - self.handle_payment_and_create_order(request, basket, payment_processor_response) + self.handle_payment_and_create_order(basket, payment_processor_response) except PayGateCallbackException as exp: return HttpResponseServerError(str(exp)) @@ -222,11 +269,25 @@ class PayGateCallbackSuccessResponseView(PayGateCallbackBaseResponseView): """ This view is used by the PayGate frontend to redirect the user after he has payed with success. - This callback should NOT be used to fullfill the order. - Internally this method will call the BackOfficeSearchTransactions to double check that the - transaction is really payed. With this design decision we don't need to protect the - callbacks URLs by IP. + Its behaviour depends on whether ``thank_you_url`` is configured for the PayGate + payment processor, so that the asynchronous flow is only enabled on a deployment + that has already rolled out the Thank-You page. + + Without ``thank_you_url`` (the previous behaviour, unchanged): the payment is handled + synchronously, the order is created and fulfilled, and the user is sent to the + receipt page. This calls ``handle_processor_response``, so for an asynchronous + payment method it still raises a ``GatewayError`` and shows the payment error page -- + that is the bug this PR fixes, and it is deliberately kept until the flag is on. + + With ``thank_you_url`` configured: this view never takes payment. It records the + callback response and makes sure an order exists for the basket: if the + server-to-server callback has not already created a fulfilled one, a ``Pending`` + order is placed. A ``Pending`` order is not fulfilled and carries no payment + record -- it exists so the user can see the in-flight payment on the Order History + page, where the status is then lazily resolved (see ``nau_extensions`` + ``OrderPaymentStatusView``). The user is then redirected to the ecommerce + micro-frontend Thank-You page. """ def get( @@ -242,17 +303,81 @@ def get( logger.warning("PayGate no basket found on the callback success") return redirect(self.payment_processor.failure_url) - receipt_url = get_receipt_page_url( - self.request, - order_number=basket.order_number, - ) + thank_you_url = self.payment_processor.thank_you_url + if not thank_you_url: + return self.fulfil_synchronously(basket, payment_processor_response) + + order = self.get_or_place_pending_order(basket) + if order is None: + return redirect(self.payment_processor.error_url) + parsed = urlparse(thank_you_url) + query = dict(parse_qsl(parsed.query)) + query["order_number"] = order.number + return redirect(urlunparse(parsed._replace(query=urlencode(query)))) + + def fulfil_synchronously(self, basket, payment_processor_response): + """ + The behaviour of this view before the asynchronous flow existed, kept for the + deployments that have not configured a `thank_you_url` yet: handle the payment, + create and fulfil the order, and send the user to the receipt page. + """ try: - self.handle_payment_and_create_order(request, basket, payment_processor_response) + self.handle_payment_and_create_order(basket, payment_processor_response) except PayGateCallbackException: return redirect(self.payment_processor.error_url) - return redirect(receipt_url) + return redirect( + get_receipt_page_url(self.request, order_number=basket.order_number) + ) + + def get_or_place_pending_order(self, basket): + """ + Return the `Order` of `basket`, placing a `Pending` one if it does not exist yet. + + Returns: + Order: the existing or newly placed order, or None when it could not be + placed and the caller should send the user to the error page. + """ + order = get_order(basket) + if order: + logger.info( + "PayGate success callback for basket [%d]: order [%s] already exists with status [%s]", + basket.id, + order.number, + order.status, + ) + return order + + try: + return place_pending_order(self.request, basket) + except IntegrityError: + logger.info( + "PayGate success callback for basket [%d]: the order was created " + "concurrently by the server callback", + basket.id, + ) + except Exception: # pylint: disable=broad-except + logger.exception( + "PayGate success callback could not place a pending order for basket [%d]", + basket.id, + ) + + order = get_order(basket) + if order is None: + logger.error( + "PayGate success callback has no order for basket [%d]", basket.id + ) + return None + + logger.info( + "PayGate success callback for basket [%d]: continuing with order [%s] " + "in status [%s]", + basket.id, + order.number, + order.status, + ) + return order class PayGateCallbackRedirectResponseView(PayGateCallbackBaseResponseView):