From 870238eb4129a082f9de313840aa55b66c89bffb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Laur=C3=A9line=20Gu=C3=A9rin?= <zebuline@entrouvert.com>
Date: Mon, 23 Nov 2020 15:48:28 +0100
Subject: [PATCH 2/2] lingo: do not retry notify on 4xx error (#48393)

---
 combo/apps/lingo/models.py       |  41 +++++++--
 tests/test_lingo_payment.py      | 139 +++++++++++++++++++++++++++++++
 tests/test_lingo_remote_regie.py |   6 +-
 3 files changed, 177 insertions(+), 9 deletions(-)

diff --git a/combo/apps/lingo/models.py b/combo/apps/lingo/models.py
index ea4876e2..52cd9bf5 100644
--- a/combo/apps/lingo/models.py
+++ b/combo/apps/lingo/models.py
@@ -253,11 +253,13 @@ class Regie(models.Model):
             return []
         return []
 
-    def get_invoice(self, user, invoice_id, log_errors=True):
+    def get_invoice(self, user, invoice_id, log_errors=True, raise_4xx=False):
         if not self.is_remote():
             return self.basketitem_set.get(pk=invoice_id)
         url = self.webservice_url + '/invoice/%s/' % invoice_id
         response = requests.get(url, user=user, remote_service='auto', cache_duration=0, log_errors=log_errors)
+        if raise_4xx and 400 <= response.status_code < 500:
+            raise ObjectDoesNotExist()
         if response.status_code == 404:
             raise ObjectDoesNotExist()
         response.raise_for_status()
@@ -286,8 +288,22 @@ class Regie(models.Model):
             'transaction_date': transaction_date.strftime('%Y-%m-%dT%H:%M:%S')
         }
         headers = {'content-type': 'application/json'}
-        return requests.post(url, remote_service='auto',
-                             data=json.dumps(data), headers=headers).json()
+        try:
+            response = requests.post(
+                url, remote_service='auto',
+                data=json.dumps(data), headers=headers)
+            if 400 <= response.status_code < 500:
+                raise ObjectDoesNotExist()
+            response.raise_for_status()
+        except RequestException as e:
+            raise RemoteInvoiceException from e
+        try:
+            resp = response.json()
+        except ValueError as e:
+            raise RemoteInvoiceException from e
+        if resp.get('err'):
+            raise RemoteInvoiceException
+        return resp
 
     def as_api_dict(self):
         return {'id': self.slug,
@@ -620,12 +636,25 @@ class Transaction(models.Model):
         to_be_paid_remote_items = []
         for item_id in items.split(','):
             try:
-                remote_item = regie.get_invoice(user=self.user, invoice_id=item_id)
+                remote_item = regie.get_invoice(user=self.user, invoice_id=item_id, raise_4xx=True)
                 regie.pay_invoice(item_id, self.order_id, self.bank_transaction_date or self.end_date)
+            except ObjectDoesNotExist:
+                # 4xx error
+                logger.error(
+                    'unable to retrieve or pay remote item %s from transaction %s, ignore it',
+                    item_id, self)
+            except (RequestException, RemoteInvoiceException):
+                # 5xx, err or requests error
+                to_be_paid_remote_items.append(item_id)
+                logger.warning(
+                    'unable to notify payment for remote item %s from transaction %s, retry later',
+                    item_id, self)
             except Exception:
+                # unknown error
                 to_be_paid_remote_items.append(item_id)
-                logger.exception(u'unable to notify payment for remote item %s from transaction %s',
-                                 item_id, self)
+                logger.exception(
+                    'unable to notify payment for remote item %s from transaction %s',
+                    item_id, self)
             else:
                 logger.info(u'notified payment for remote item %s from transaction %s',
                             item_id, self)
diff --git a/tests/test_lingo_payment.py b/tests/test_lingo_payment.py
index 5b808c49..ef42492f 100644
--- a/tests/test_lingo_payment.py
+++ b/tests/test_lingo_payment.py
@@ -8,6 +8,8 @@ import mock
 import uuid
 
 from mellon.models import UserSAMLIdentifier
+from requests.exceptions import ConnectionError
+from requests.models import Response
 
 from django.apps import apps
 from django.contrib.auth.models import User
@@ -1073,6 +1075,143 @@ def test_transaction_retry(mock_request, remote_regie):
     assert transaction.to_be_paid_remote_items == '42,35'
 
 
+@mock.patch('combo.utils.requests_wrapper.RequestsSession.request')
+def test_transaction_retry_failure(mock_request, remote_regie):
+    transaction = Transaction.objects.create(
+        status=eopayment.PAID, regie=remote_regie,
+        end_date=timezone.now(),
+        to_be_paid_remote_items='42,35')
+
+    appconfig = apps.get_app_config('lingo')
+
+    mock_json_item = mock.Mock(status_code=200)
+    mock_json_item.json.return_value = {'err': 0, 'data': {
+        'created': '2020-11-23', 'pay_limit_date': '2021-11-23',
+        'total_amount': '123.45', 'amount': '123.45'}}
+
+    mock_json_paid = mock.Mock(status_code=200)
+    mock_json_paid.json.return_value = {'err': 0}
+
+    mock_5xx = mock.Mock(status_code=500)
+
+    mock_4xx = mock.Mock(status_code=400)
+
+    mock_err = mock.Mock(status_code=200)
+    mock_err.json.return_url = {'err': 1}
+
+    # error on get invoice
+    mock_request.side_effect = [
+        ConnectionError('where is my hostname?'),  # get invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # error on pay invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_json_item,  # get invoice 42
+        ConnectionError('where is my hostname?'),  # pay invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # unknown error on get invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        Exception,  # get invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # unknown error on pay invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_json_item,  # get invoice 42
+        Exception,  # pay invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # 5xx on get invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_5xx,  # get invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # 5xx on pay invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_json_item,  # get invoice 42
+        mock_5xx,  # pay invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # err on get invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_err,  # get invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # err on pay invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_json_item,  # get invoice 42
+        mock_err,  # pay invoice 42
+        mock_json_item,  # get invoice 35
+        mock_json_paid,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '42'  # retry for first one
+
+    # 4xx on get invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_4xx,  # get invoice 42
+        mock_5xx,  # get invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '35'  # retry for the second only
+
+    # 4xx on pay invoice
+    Transaction.objects.update(to_be_paid_remote_items='42,35')
+    mock_request.side_effect = [
+        mock_json_item,  # get invoice 42
+        mock_4xx,  # get invoice 35
+        mock_5xx,  # pay invoice 35
+    ]
+    appconfig.update_transactions()
+    transaction.refresh_from_db()
+    assert transaction.to_be_paid_remote_items == '35'  # retry for the second only
+
+
 def test_transaction_validate(app, key, regie, user):
     t1 = Transaction(regie=regie, bank_data={'bank': 'data'}, amount=12,
             status=eopayment.PAID)
diff --git a/tests/test_lingo_remote_regie.py b/tests/test_lingo_remote_regie.py
index b1cdb4be..0fa2e8ae 100644
--- a/tests/test_lingo_remote_regie.py
+++ b/tests/test_lingo_remote_regie.py
@@ -315,7 +315,7 @@ def test_anonymous_successful_item_payment(mock_get, mock_pay_invoice, app, remo
     invoices = copy.deepcopy(INVOICES)
     invoices[0]['amount'] = '100.00'
     invoices[0]['amount_paid'] = '23.45'
-    mock_json = mock.Mock()
+    mock_json = mock.Mock(status_code=200)
     mock_json.json.return_value = {'err': 0, 'data': invoices[0]}
     mock_get.return_value = mock_json
     mock_pay_invoice.return_value = mock.Mock(status_code=200)
@@ -325,7 +325,7 @@ def test_anonymous_successful_item_payment(mock_get, mock_pay_invoice, app, remo
     assert 'Amount already paid: <span class="amount">23.45€</span>' in resp.text
 
     # invoice without amount_paid
-    mock_json = mock.Mock()
+    mock_json = mock.Mock(status_code=200)
     mock_json.json.return_value = {'err': 0, 'data': INVOICES[0]}
     mock_get.return_value = mock_json
     mock_pay_invoice.return_value = mock.Mock(status_code=200)
@@ -491,7 +491,7 @@ def test_remote_item_payment_failure(mock_post, mock_get, mock_pay_invoice, app,
     page.save()
     assert remote_regie.is_remote()
     encrypt_id = aes_hex_encrypt(settings.SECRET_KEY, force_bytes('F201601'))
-    mock_json = mock.Mock()
+    mock_json = mock.Mock(status_code=200)
     mock_json.json.return_value = {'err': 0, 'data': INVOICES[0]}
     mock_get.return_value = mock_json
     mock_pay_invoice.return_value = mock.Mock(status_code=200)
-- 
2.28.0

