newsletters: manage http errors (#48562)

combo/apps/newsletters/models.py

@@ -98,20 +98,24 @@ class NewslettersCell(CellBase):
 
     def get_newsletters(self):
         endpoint = self.url + 'newsletters/'
-        response = requests.get(endpoint, remote_service='auto', cache_duration=60, without_user=True)
-        if response.ok:
-            json_response = response.json()
-            return self.filter_data(json_response['data'])
-        return []
+        try:
+            response = requests.get(endpoint, remote_service='auto', cache_duration=60, without_user=True)
+            response.raise_for_status()
+        except RequestException:
+            return []
+        json_response = response.json()
+        return self.filter_data(json_response['data'])
 
     def get_subscriptions(self, user, **kwargs):
         endpoint = self.url + 'subscriptions/'
-        response = requests.get(endpoint, remote_service='auto',
-                                user=user, cache_duration=0, params=kwargs)
-        if response.ok:
-            json_response = response.json()
-            return self.filter_data(json_response['data'])
-        return []
+        try:
+            response = requests.get(endpoint, remote_service='auto',
+                                    user=user, cache_duration=0, params=kwargs)
+            response.raise_for_status()
+        except RequestException:
+            return []
+        json_response = response.json()
+        return self.filter_data(json_response['data'])
 
     def set_subscriptions(self, subscriptions, user, **kwargs):
         logger = logging.getLogger(__name__)
@@ -123,11 +127,12 @@ class NewslettersCell(CellBase):
         try:
             response = requests.post(endpoint, remote_service='auto', data=json.dumps(subscriptions),
                                      user=user, federation_key='email', params=kwargs, headers=headers)
+            response.raise_for_status()
             if not response.json()['data']:
                 raise SubscriptionsSaveError
-        except HTTPError:
+        except HTTPError as e:
             logger.error(u'set subscriptions on %s returned an HTTP error code: %s',
-                         response.request.url, response.status_code)
+                         e.response.request.url, e.response.status_code)
             raise SubscriptionsSaveError
         except RequestException as e:
             logger.error(u'set subscriptions on %s failed with exception: %s',

tests/test_newsletters_cell.py

@@ -3,6 +3,7 @@
 import pytest
 import mock
 import requests
+import json
 
 from django.template.defaultfilters import slugify
 from django.contrib.auth.models import User
@@ -102,6 +103,10 @@ def test_get_newsletters_by_transports(mock_get, cell):
     assert mock_get.call_args[1]['without_user']
     assert 'user' not in mock_get.call_args[1]
 
+    mock_get.side_effect = requests.RequestException
+    assert cell.get_newsletters() == []
+
+
 @mock.patch('combo.apps.newsletters.models.requests.get')
 def test_get_newsletters_by_unrestricted_transports(mock_get, cell):
     cell.transports_restrictions = ''
@@ -148,6 +153,10 @@ def test_get_subscriptions(mock_get, cell, user):
     assert cell.get_subscriptions(user) == expected_subscriptions
     assert mock_get.call_args[1]['user'].email == USER_EMAIL
 
+    mock_get.side_effect = requests.RequestException
+    assert cell.get_subscriptions(user) == []
+
+
 @mock.patch('combo.utils.requests_wrapper.RequestsSession.send')
 def test_get_subscriptions_signature_check(mock_send, cell, user):
     restrictions = ('mail', 'sms')
@@ -167,11 +176,7 @@ def test_get_subscriptions_signature_check(mock_send, cell, user):
     assert check_query(url.split('?', 1)[-1], 'combo')
 
 
-def mocked_requests_connection_error(*args, **kwargs):
-    raise requests.ConnectionError()
-
-@mock.patch('combo.apps.newsletters.models.requests.post',
-            side_effect=mocked_requests_connection_error)
+@mock.patch('combo.apps.newsletters.models.requests.post')
 def test_failed_set_subscriptions(mock_post, cell, user):
     restrictions = ('sms', 'mail')
     cell.transports_restrictions = ','.join(restrictions)
@@ -179,21 +184,25 @@ def test_failed_set_subscriptions(mock_post, cell, user):
                      {'id': '7', 'transports': [{'id': 'sms', 'text': 'sms'}]},
                      {'id': '8', 'transports': [{'id': 'sms', 'text': 'sms'},
                                                 {'id': 'mail', 'text': 'mail'}]}]
+    mock_post.side_effect = requests.ConnectionError
     with pytest.raises(SubscriptionsSaveError):
         cell.set_subscriptions(subscriptions, user, uuid='useruuid')
 
-@mock.patch('combo.apps.newsletters.models.requests.post')
-def test_set_subscriptions_with_no_uuid(mocked_post, cell, user):
+    mock_post.side_effect = requests.HTTPError(response=mock.MagicMock())
+    with pytest.raises(SubscriptionsSaveError):
+        cell.set_subscriptions(subscriptions, user, uuid='useruuid')
+
+
+def test_set_subscriptions_with_no_uuid(cell, user):
     restrictions = ('sms', 'mail')
     cell.transports_restrictions = ','.join(restrictions)
     subscriptions = [{'id': '1', 'transports': [{'id': 'mail', 'text': 'mail'}]},
                      {'id': '8', 'transports': [{'id': 'sms', 'text': 'sms'},
                                                 {'id': 'mail', 'text': 'mail'}]}]
-    mock_json = mock.Mock()
-    mock_json.json.return_value = {'err': 0, 'data': True}
     with pytest.raises(SubscriptionsSaveError):
         cell.set_subscriptions(subscriptions, user)
 
+
 @mock.patch('combo.apps.newsletters.models.requests.post')
 def test_set_subscriptions(mock_post, cell, user):
     restrictions = ('sms', 'mail')
@@ -230,22 +239,21 @@ def test_get_subscriptions_with_name_id_and_mobile(mock_get, cell, user):
     args, kwargs = mock_get.call_args
     assert kwargs['params'] == {'uuid': 'nameid', 'mobile': '0607080900'}
 
+
 def mocked_requests_get(*args, **kwargs):
     url = args[0]
-    class MockResponse:
-        def __init__(self, json_data):
-            self.json_data = json_data
 
-        def ok(self):
-            return True
+    class MockResponse(mock.Mock):
+        status_code = 200
 
         def json(self):
-            return self.json_data
+            return json.loads(self.content)
 
     if 'newsletters' in url:
-        return MockResponse({'data': NEWSLETTERS})
+        return MockResponse(content=json.dumps({'data': NEWSLETTERS}))
     else:
-        return MockResponse({'data': SUBSCRIPTIONS})
+        return MockResponse(content=json.dumps({'data': SUBSCRIPTIONS}))
+
 
 @mock.patch('combo.apps.newsletters.models.requests.get',
             side_effect=mocked_requests_get)