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):