Merge pull request #996 from edx/schen/ECOM-5891

Catch exceptions during paypal payment creation and introduce retry

ecommerce/extensions/payment/admin.py

@@ -3,8 +3,10 @@ from pprint import pformat
 from django.utils.html import format_html
 from oscar.apps.payment.admin import *  # noqa pylint: disable=wildcard-import,unused-wildcard-import,wrong-import-position
 from oscar.core.loading import get_model
+from solo.admin import SingletonModelAdmin
 
 PaymentProcessorResponse = get_model('payment', 'PaymentProcessorResponse')
+PaypalProcessorConfiguration = get_model('payment', 'PaypalProcessorConfiguration')
 
 
 @admin.register(PaymentProcessorResponse)
@@ -23,3 +25,6 @@ class PaymentProcessorResponseAdmin(admin.ModelAdmin):
         return format_html('<br><br><pre>{}</pre>', pretty_response)
 
     formatted_response.allow_tags = True
+
+
+admin.site.register(PaypalProcessorConfiguration, SingletonModelAdmin)

ecommerce/extensions/payment/migrations/0011_paypalprocessorconfiguration.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('payment', '0010_create_client_side_checkout_flag'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='PaypalProcessorConfiguration',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+                ('retry_attempts', models.PositiveSmallIntegerField(default=0, verbose_name='Number of times to retry failing Paypal client actions')),
+            ],
+            options={
+                'verbose_name': 'Paypal Processor Configuration',
+            },
+        ),
+    ]

ecommerce/extensions/payment/models.py

@@ -2,6 +2,7 @@ from django.db import models
 from django.utils.translation import ugettext_lazy as _
 from jsonfield import JSONField
 from oscar.apps.payment.abstract_models import AbstractSource
+from solo.models import SingletonModel
 
 from ecommerce.extensions.payment.constants import CARD_TYPE_CHOICES
 
@@ -32,5 +33,18 @@ class PaypalWebProfile(models.Model):
     name = models.CharField(max_length=255, unique=True)
 
 
+class PaypalProcessorConfiguration(SingletonModel):
+    """ This is a configuration model for PayPal Payment Processor"""
+    retry_attempts = models.PositiveSmallIntegerField(
+        default=0,
+        verbose_name=_(
+            'Number of times to retry failing Paypal client actions (e.g., payment creation, payment execution)'
+        )
+    )
+
+    class Meta(object):
+        verbose_name = "Paypal Processor Configuration"
+
+
 # noinspection PyUnresolvedReferences
-from oscar.apps.payment.models import *  # noqa pylint: disable=wildcard-import,unused-wildcard-import,wrong-import-position,wrong-import-order
+from oscar.apps.payment.models import *  # noqa pylint: disable=ungrouped-imports, wildcard-import,unused-wildcard-import,wrong-import-position,wrong-import-order

ecommerce/extensions/payment/processors/paypal.py

@@ -14,7 +14,7 @@ from oscar.core.loading import get_model
 
 from ecommerce.core.url_utils import get_ecommerce_url
 from ecommerce.extensions.order.constants import PaymentEventTypeName
-from ecommerce.extensions.payment.models import PaypalWebProfile
+from ecommerce.extensions.payment.models import PaypalWebProfile, PaypalProcessorConfiguration
 from ecommerce.extensions.payment.processors import BasePaymentProcessor
 from ecommerce.extensions.payment.utils import middle_truncate
 
@@ -48,7 +48,7 @@ class Paypal(BasePaymentProcessor):
         super(Paypal, self).__init__(site)
 
         # Number of times payment execution is retried after failure.
-        self.retry_attempts = self.configuration.get('retry_attempts', 1)
+        self.retry_attempts = PaypalProcessorConfiguration.get_solo().retry_attempts
 
     @cached_property
     def paypal_api(self):
@@ -132,32 +132,50 @@ class Paypal(BasePaymentProcessor):
             available_attempts = self.retry_attempts
 
         for i in range(1, available_attempts + 1):
-            payment = paypalrestsdk.Payment(data, api=self.paypal_api)
-            payment.create()
-            # Raise an exception for payments that were not successfully created. Consuming code is
-            # responsible for handling the exception.
-            if not payment.success():
+            try:
+                payment = paypalrestsdk.Payment(data, api=self.paypal_api)
+                payment.create()
+                if payment.success():
+                    break
+                else:
+                    if i < available_attempts:
+                        logger.warning(
+                            u"Creating PayPal payment for basket [%d] was unsuccessful. Will retry.",
+                            basket.id,
+                            exc_info=True
+                        )
+                    else:
+                        error = self._get_error(payment)
+                        # pylint: disable=unsubscriptable-object
+                        entry = self.record_processor_response(
+                            error,
+                            transaction_id=error['debug_id'],
+                            basket=basket
+                        )
+                        logger.error(
+                            u"%s [%d], %s [%d].",
+                            "Failed to create PayPal payment for basket",
+                            basket.id,
+                            "PayPal's response recorded in entry",
+                            entry.id,
+                            exc_info=True
+                        )
+                        raise GatewayError(error)
+
+            except:  # pylint: disable=bare-except
                 if i < available_attempts:
                     logger.warning(
-                        u"Creating paypal payment for basket [%d] unsuccessful. Will retry",
+                        u"Creating PayPal payment for basket [%d] resulted in an exception. Will retry.",
                         basket.id,
                         exc_info=True
                     )
                 else:
-                    error = self._get_error(payment)
-                    # pylint: disable=unsubscriptable-object
-                    entry = self.record_processor_response(
-                        error,
-                        transaction_id=error['debug_id'],
-                        basket=basket
-                    )
-                    logger.error(
-                        u"Failed to create PayPal payment for basket [%d]. PayPal's response recorded in entry [%d].",
-                        basket.id,
-                        entry.id,
-                        exc_info=True
+                    logger.exception(
+                        u"After %d retries, creating PayPal payment for basket [%d] still experienced exception.",
+                        i,
+                        basket.id
                     )
-                    raise GatewayError(error)
+                    raise
 
         entry = self.record_processor_response(payment.to_dict(), transaction_id=payment.id, basket=basket)
         logger.info("Successfully created PayPal payment [%s] for basket [%d].", payment.id, basket.id)

ecommerce/extensions/payment/tests/mixins.py

@@ -282,21 +282,21 @@ class PaypalMixin(object):
             status=200
         )
 
-    def mock_api_responses(self, path, body_array, post=True):
+    def mock_api_responses(self, path, response_array, post=True):
         assert httpretty.is_enabled()
 
         url = self._create_api_url(path)
 
-        response_array = []
-        for body in body_array:
-            response_array.append(
-                httpretty.Response(body=json.dumps(body), status=200)
+        httpretty_response_array = []
+        for response in response_array:
+            httpretty_response_array.append(
+                httpretty.Response(body=json.dumps(response['body']), status=response['status'])
             )
 
         httpretty.register_uri(
             httpretty.POST if post else httpretty.GET,
             url,
-            responses=response_array,
+            responses=httpretty_response_array,
             status=200
         )
 

ecommerce/extensions/payment/tests/processors/test_paypal.py

@@ -101,27 +101,7 @@ class PaypalTests(PaypalMixin, PaymentProcessorTestCaseMixin, TestCase):
         amount = self.basket.total_incl_tax
         self.assert_valid_payment_event_fields(payment_event, amount, paid_type, self.processor.NAME, reference)
 
-    @httpretty.activate
-    def test_get_transaction_parameters(self):
-        """Verify the processor returns the appropriate parameters required to complete a transaction."""
-        self.mock_oauth2_response()
-        response = self.mock_payment_creation_response(self.basket)
-
-        self._assert_transaction_parameters()
-        self.assert_processor_response_recorded(self.processor.NAME, self.PAYMENT_ID, response, basket=self.basket)
-
-        last_request_body = json.loads(httpretty.last_request().body)
-        expected = urljoin(self.site.siteconfiguration.build_ecommerce_url(), reverse('paypal_execute'))
-        self.assertEqual(last_request_body['redirect_urls']['return_url'], expected)
-
-    @httpretty.activate
-    def test_get_transaction_parameters_with_retry(self):
-        """Verify the processor returns the appropriate parameters required to complete a transaction after a retry"""
-        toggle_switch('PAYPAL_RETRY_ATTEMPTS', True)
-        self.mock_oauth2_response()
-        response_error = self.get_payment_creation_error_response_mock()
-        response_success = self.get_payment_creation_response_mock(self.basket)
-        self.mock_api_responses('/v1/payments/payment', [response_error, response_success])
+    def _assert_transaction_parameters_retry(self, response_success, failure_log_message):
         self.processor.retry_attempts = 2
         logger_name = 'ecommerce.extensions.payment.processors.paypal'
 
@@ -141,7 +121,7 @@ class PaypalTests(PaypalMixin, PaymentProcessorTestCaseMixin, TestCase):
                 (
                     logger_name,
                     'WARNING',
-                    'Creating paypal payment for basket [{}] unsuccessful. Will retry'.format(self.basket.id)
+                    failure_log_message,
                 ),
                 (
                     logger_name,
@@ -150,6 +130,60 @@ class PaypalTests(PaypalMixin, PaymentProcessorTestCaseMixin, TestCase):
                 )
             )
 
+    @httpretty.activate
+    def test_get_transaction_parameters(self):
+        """Verify the processor returns the appropriate parameters required to complete a transaction."""
+        self.mock_oauth2_response()
+        response = self.mock_payment_creation_response(self.basket)
+
+        self._assert_transaction_parameters()
+        self.assert_processor_response_recorded(self.processor.NAME, self.PAYMENT_ID, response, basket=self.basket)
+
+        last_request_body = json.loads(httpretty.last_request().body)
+        expected = urljoin(self.site.siteconfiguration.build_ecommerce_url(), reverse('paypal_execute'))
+        self.assertEqual(last_request_body['redirect_urls']['return_url'], expected)
+
+    @httpretty.activate
+    def test_get_transaction_parameters_with_retry(self):
+        """Verify the processor returns the appropriate parameters required to complete a transaction after a retry"""
+        toggle_switch('PAYPAL_RETRY_ATTEMPTS', True)
+        self.mock_oauth2_response()
+        response_error = self.get_payment_creation_error_response_mock()
+        response_success = self.get_payment_creation_response_mock(self.basket)
+        self.mock_api_responses(
+            '/v1/payments/payment',
+            [
+                {'body': response_error, 'status': 200},
+                {'body': response_success, 'status': 200}
+            ]
+        )
+        self._assert_transaction_parameters_retry(
+            response_success,
+            'Creating PayPal payment for basket [{}] was unsuccessful. Will retry.'.format(self.basket.id)
+        )
+
+    @httpretty.activate
+    def test_get_transaction_parameters_server_error_with_retry(self):
+        """
+        Verify the processor returns the appropriate parameters required
+        to complete a transaction after a retry with server error
+        """
+        toggle_switch('PAYPAL_RETRY_ATTEMPTS', True)
+        self.mock_oauth2_response()
+        response_error = self.get_payment_creation_error_response_mock()
+        response_success = self.get_payment_creation_response_mock(self.basket)
+        self.mock_api_responses(
+            '/v1/payments/payment',
+            [
+                {'body': response_error, 'status': 500},
+                {'body': response_success, 'status': 200}
+            ]
+        )
+        self._assert_transaction_parameters_retry(
+            response_success,
+            'Creating PayPal payment for basket [{}] resulted in an exception. Will retry.'.format(self.basket.id)
+        )
+
     def test_switch_enabled_otto_url(self):
         """
         Ensures that when the otto_receipt_page waffle switch is enabled, the processor uses the new receipt page.
@@ -207,6 +241,33 @@ class PaypalTests(PaypalMixin, PaymentProcessorTestCaseMixin, TestCase):
             )
 
     @httpretty.activate
+    def test_payment_creation_exception_state(self):
+        """Verify that an exception is thrown while create a payment results in paypal exception."""
+        self.mock_oauth2_response()
+        response_error = self.get_payment_creation_error_response_mock()
+        self.mock_api_responses(
+            '/v1/payments/payment',
+            [{'body': response_error, 'status': 500}]
+        )
+        logger_name = 'ecommerce.extensions.payment.processors.paypal'
+
+        with LogCapture(logger_name) as paypal_logger:
+            self.assertRaises(
+                paypalrestsdk.exceptions.ServerError,
+                self.processor.get_transaction_parameters,
+                self.basket,
+                request=self.request
+            )
+            paypal_logger.check(
+                (
+                    logger_name,
+                    'ERROR',
+                    'After {} retries, creating PayPal payment for basket [{}] still experienced exception.'
+                    .format(self.processor.retry_attempts + 1, self.basket.id)
+                )
+            )
+
+    @httpretty.activate
     def test_approval_url_missing(self):
         """Verify that a missing approval URL results in a GatewayError."""
         self.mock_oauth2_response()
@@ -272,6 +333,7 @@ class PaypalTests(PaypalMixin, PaymentProcessorTestCaseMixin, TestCase):
         GatewayError is raised.
         """
         toggle_switch('PAYPAL_RETRY_ATTEMPTS', True)
+        self.processor.retry_attempts = 1
         self.mock_oauth2_response()
         self.mock_payment_creation_response(self.basket, find=True)
         self.mock_payment_execution_response(self.basket)

ecommerce/settings/base.py

@@ -261,6 +261,7 @@ DJANGO_APPS = [
     'rest_framework_swagger',
     'release_util',
     'crispy_forms',
+    'solo',
 ]
 
 # Apps specific to this project go here.

requirements/base.txt

@@ -9,6 +9,7 @@ django-libsass==0.5
 django-oscar==1.1.1
 django-rest-swagger==0.3.5
 django_simple_history==1.6.3
+django-solo==1.1.2
 django-threadlocals==0.8
 django-waffle==0.11.1
 djangorestframework==3.2.3