Merge pull request #120 from pmclanahan/fix-get-involved-et-error-108…

…8779

Fix bug 1088779: Fix ET error handling around views and tasks.

news/backends/common.py

@@ -6,7 +6,11 @@ class UnauthorizedException(Exception):
 
 class NewsletterException(Exception):
     """Error when trying to talk to the the email server."""
-    pass
+
+    def __init__(self, msg=None, error_code=None, status_code=None):
+        self.error_code = error_code
+        self.status_code = status_code
+        super(NewsletterException, self).__init__(msg)
 
 
 class NewsletterNoResultsException(NewsletterException):

news/tasks.py

@@ -476,10 +476,6 @@ def update_user(data, email, token, api_call_type, optin):
             'lang': lang,
             'status': 'ok',
         }
-    elif user_data.get('status', 'error') != 'ok':
-        # Error talking to ET - raise so we retry later
-        msg = "Some error with Exact Target: %r" % user_data
-        raise NewsletterException(msg)
 
     if lang:
         # User asked for a language change. Use the new language from
@@ -744,14 +740,15 @@ def confirm_user(token, user_data):
     if user_data is None:
         from .utils import get_user_data   # Avoid circular import
         user_data = get_user_data(token=token)
+
     if user_data is None:
         raise BasketError(MSG_USER_NOT_FOUND)
-    if user_data['status'] == 'error':
-        raise NewsletterException('error getting user data')
+
     if user_data['confirmed']:
-        log.info("In confirm_user, user with token %s "
-                 "is already confirmed" % token)
+        log.info('In confirm_user, user with token %s '
+                 'is already confirmed' % token)
         return
+
     if not 'email' in user_data or not user_data['email']:
         raise BasketError('token has no email in ET')
 

news/tests/test_confirm.py

@@ -318,18 +318,15 @@ def test_master_non_english_not_required(self):
 @patch('news.tasks.send_welcomes')
 @patch('news.tasks.apply_updates')
 class TestConfirmTask(TestCase):
-    def test_error(self, apply_updates, send_welcomes):
+    @patch('news.utils.lookup_subscriber')
+    def test_error(self, lookup_subscriber, apply_updates, send_welcomes):
         """
         If user_data shows an error talking to ET, the task raises
         an exception so our task logic will retry
         """
-        user_data = {
-            'status': 'error',
-            'desc': 'TESTERROR',
-        }
-        token = "TOKEN"
+        lookup_subscriber.side_effect = NewsletterException('Stuffs broke yo.')
         with self.assertRaises(NewsletterException):
-            confirm_user(token, user_data)
+            confirm_user('token', None)
         self.assertFalse(apply_updates.called)
         self.assertFalse(send_welcomes.called)
 

news/tests/test_tasks.py

@@ -12,6 +12,7 @@
     add_sms_user,
     et_task,
     mogrify_message_id,
+    NewsletterException,
     RECOVERY_MESSAGE_ID,
     send_recovery_message_task,
     SUBSCRIBE,
@@ -79,6 +80,15 @@ def test_unknown_email(self, mock_look_for_user, mock_send):
         send_recovery_message_task(self.email)
         self.assertFalse(mock_send.called)
 
+    def test_et_error(self, mock_look_for_user, mock_send):
+        """Error talking to Basket. I'm shocked, SHOCKED!"""
+        mock_look_for_user.side_effect = NewsletterException('ET has failed to achieve.')
+
+        with self.assertRaises(NewsletterException):
+            send_recovery_message_task(self.email)
+
+        self.assertFalse(mock_send.called)
+
     def test_email_only_in_et(self, mock_look_for_user, mock_send):
         """Email not in basket but in ET"""
         # Should create new subscriber with ET data, then trigger message

news/tests/test_users.py

@@ -129,6 +129,8 @@ def test_existing_user(self):
                                              email, sub.token, 'T')
 
 
+@override_settings(EXACTTARGET_DATA='DATA_FOR_DUDE',
+                   EXACTTARGET_INTERESTS='DUDE_IS_INTERESTED')
 class UpdateGetInvolvedTests(TestCase):
     def setUp(self):
         patcher = patch.object(tasks, 'send_message')
@@ -158,8 +160,6 @@ def setUp(self):
         Newsletter.objects.create(slug='about-mozilla', vendor_id='ABOUT_MOZILLA')
         Newsletter.objects.create(slug='get-involved', vendor_id='GET_INVOLVED')
 
-    @override_settings(EXACTTARGET_DATA='DATA_FOR_DUDE',
-                       EXACTTARGET_INTERESTS='DUDE_IS_INTERESTED')
     def test_new_user_interested(self):
         """Successful submission of the form for new user."""
         email = 'walter@example.com'
@@ -194,14 +194,24 @@ def test_new_user_interested(self):
         self.interest.notify_stewards.assert_called_with('Walter', email, 'en',
                                                          'It really tied the room together.')
 
-    @override_settings(EXACTTARGET_DATA='DATA_FOR_DUDE',
-                       EXACTTARGET_INTERESTS='DUDE_IS_INTERESTED')
+    def test_exact_target_error(self):
+        """Successful submission of the form for new user, but ET has a problem."""
+        self.get_user_data.side_effect = NewsletterException('Stuffs broke yo.')
+        with self.assertRaises(NewsletterException):
+            tasks.update_get_involved('bowling', 'en', 'Walter', 'walter@example.com',
+                                      'US', 'T', 'Y', 'It really tied the room together.', None)
+
+        self.assertFalse(self.send_message.called)
+        self.assertFalse(self.send_welcomes.called)
+        self.assertFalse(self.interest.notify_stewards.called)
+
     def test_existing_user_interested(self):
         """Successful submission of the form for existing newsletter user."""
         email = 'walter@example.com'
         sub = models.Subscriber.objects.create(email=email)
         token = sub.token
         self.get_user_data.return_value = {
+            'status': 'ok',
             'format': 'T',
             'token': token,
             'newsletters': ['about-mozilla', 'mozilla-and-you', 'get-involved'],
@@ -228,8 +238,6 @@ def test_existing_user_interested(self):
         self.interest.notify_stewards.assert_called_with('Walter', email, 'en',
                                                          'It really tied the room together.')
 
-    @override_settings(EXACTTARGET_DATA='DATA_FOR_DUDE',
-                       EXACTTARGET_INTERESTS='DUDE_IS_INTERESTED')
     def test_existing_user_interested_no_newsletter(self):
         """
         Successful submission of the form for existing newsletter user not
@@ -239,6 +247,7 @@ def test_existing_user_interested_no_newsletter(self):
         sub = models.Subscriber.objects.create(email=email)
         token = sub.token
         self.get_user_data.return_value = {
+            'status': 'ok',
             'format': 'T',
             'token': token,
             'newsletters': ['mozilla-and-you'],
@@ -268,8 +277,6 @@ def test_existing_user_interested_no_newsletter(self):
         self.interest.notify_stewards.assert_called_with('Walter', email, 'en',
                                                          'It really tied the room together.')
 
-    @override_settings(EXACTTARGET_DATA='DATA_FOR_DUDE',
-                       EXACTTARGET_INTERESTS='DUDE_IS_INTERESTED')
     def test_new_user_interested_no_sub(self):
         """Successful submission of the form for new user without newsletter subscription."""
         email = 'walter@example.com'
@@ -412,12 +419,7 @@ def test_unset_lang(self):
 
 class TestGetUserData(TestCase):
 
-    def check_get_user(self,
-                       master,
-                       optin,
-                       confirm,
-                       error,
-                       expected_result):
+    def check_get_user(self, master, optin, confirm, error, expected_result):
         """
         Call get_user_data with the given conditions and verify
         that the return value matches the expected result.
@@ -446,8 +448,8 @@ def mock_look_for_user(database, email, token, fields):
                 raise Exception("INVALID INPUT TO mock_look_for_user - "
                                 "database %r unknown" % database)
 
-        with patch('news.utils.look_for_user') as look_for_user:
-            look_for_user.side_effect = mock_look_for_user
+        with patch('news.utils.look_for_user') as look_for_user_mock:
+            look_for_user_mock.side_effect = mock_look_for_user
             result = get_user_data()
 
         self.assertEqual(expected_result, result)
@@ -475,17 +477,18 @@ def test_not_in_et(self):
         # User not in Exact Target, return None
         self.check_get_user(None, None, None, False, None)
 
-    def test_et_error(self):
+    @patch('news.utils.look_for_user')
+    def test_et_error(self, look_for_user_mock):
         # Error calling Exact Target, return error code
-        err_msg = "Mock error for testing"
-        error = NewsletterException(err_msg)
-        expected = {
-            'status': 'error',
-            'desc': err_msg,
-            'status_code': 400,
-            'code': errors.BASKET_NETWORK_FAILURE,
-        }
-        self.check_get_user(ANY, ANY, ANY, error, expected)
+        err_msg = 'Stuffs broke yo.'
+        look_for_user_mock.side_effect = NewsletterException(err_msg)
+        with self.assertRaises(NewsletterException) as exc_manager:
+            get_user_data()
+
+        exc = exc_manager.exception
+        self.assertEqual(str(exc), err_msg)
+        self.assertEqual(exc.error_code, errors.BASKET_NETWORK_FAILURE)
+        self.assertEqual(exc.status_code, 400)
 
     def test_in_master(self):
         """

news/utils.py

@@ -111,13 +111,25 @@ def lookup_subscriber(token=None, email=None):
     return subscriber, user_data, created
 
 
+def newsletter_exception_response(exc):
+    """Convert a NewsletterException into a JSON HTTP response."""
+    return HttpResponseJSON({
+        'status': 'error',
+        'code': exc.error_code or errors.BASKET_UNKNOWN_ERROR,
+        'desc': str(exc),
+    }, exc.status_code or 400)
+
+
 def logged_in(f):
     """Decorator to check if the user has permission to view these
     pages"""
 
     @wraps(f)
     def wrapper(request, token, *args, **kwargs):
-        subscriber, subscriber_data, created = lookup_subscriber(token=token)
+        try:
+            subscriber, subscriber_data, created = lookup_subscriber(token=token)
+        except NewsletterException as e:
+            return newsletter_exception_response(e)
 
         if not subscriber:
             return HttpResponseJSON({
@@ -350,19 +362,13 @@ def get_user_data(token=None, email=None, sync_data=False):
         user_data['pending'] = pending
         user_data['master'] = master
     except NewsletterException as e:
-        return {
-            'status': 'error',
-            'status_code': 400,
-            'desc': str(e),
-            'code': errors.BASKET_NETWORK_FAILURE,
-        }
-    except UnauthorizedException as e:
-        return {
-            'status': 'error',
-            'status_code': 500,
-            'desc': 'Email service provider auth failure',
-            'code': errors.BASKET_EMAIL_PROVIDER_AUTH_FAILURE,
-        }
+        raise NewsletterException(str(e),
+                                  error_code=errors.BASKET_NETWORK_FAILURE,
+                                  status_code=400)
+    except UnauthorizedException:
+        raise NewsletterException('Email service provider auth failure',
+                                  error_code=errors.BASKET_EMAIL_PROVIDER_AUTH_FAILURE,
+                                  status_code=500)
 
     # We did find a user
     if sync_data:
@@ -373,8 +379,19 @@ def get_user_data(token=None, email=None, sync_data=False):
 
 
 def get_user(token=None, email=None, sync_data=False):
-    user_data = get_user_data(token, email, sync_data)
-    status_code = user_data.pop('status_code', 200) if user_data else 400
+    try:
+        user_data = get_user_data(token, email, sync_data)
+        status_code = 200
+    except NewsletterException as e:
+        return newsletter_exception_response(e)
+
+    if user_data is None:
+        user_data = {
+            'status': 'error',
+            'code': errors.BASKET_UNKNOWN_EMAIL if email else errors.BASKET_UNKNOWN_TOKEN,
+            'desc': 'Unable to find user.'
+        }
+        status_code = 400
     return HttpResponseJSON(user_data, status_code)
 
 

news/views.py

@@ -37,9 +37,10 @@
     HttpResponseJSON,
     language_code_is_valid,
     logged_in,
+    NewsletterException,
     update_user_task,
     validate_email,
-)
+    newsletter_exception_response)
 
 
 @require_POST
@@ -312,13 +313,18 @@ def send_recovery_message(request):
         return invalid_email_response(e)
 
     email = request.POST.get('email')
-    user_data = get_user_data(email=email, sync_data=True)
+    try:
+        user_data = get_user_data(email=email, sync_data=True)
+    except NewsletterException as e:
+        return newsletter_exception_response(e)
+
     if not user_data:
         return HttpResponseJSON({
             'status': 'error',
             'desc': 'Email address not known',
             'code': errors.BASKET_UNKNOWN_EMAIL,
         }, 404)  # Note: Bedrock looks for this 404
+
     send_recovery_message_task.delay(email)
     return HttpResponseJSON({'status': 'ok'})
 
@@ -340,8 +346,11 @@ def debug_user(request):
                                 401)
 
     email = request.GET['email']
-    user_data = get_user_data(email=email)
-    status_code = user_data.pop('status_code', 200)
+    try:
+        user_data = get_user_data(email=email)
+    except NewsletterException as e:
+        return newsletter_exception_response(e)
+
     try:
         user = Subscriber.objects.get(email=email)
         user_data['in_basket'] = True
@@ -350,7 +359,7 @@ def debug_user(request):
         user_data['in_basket'] = False
         user_data['basket_token'] = ''
 
-    return HttpResponseJSON(user_data, status_code)
+    return HttpResponseJSON(user_data, 200)
 
 
 # Custom update methods
@@ -476,8 +485,12 @@ def lookup_user(request):
             'code': errors.BASKET_AUTH_ERROR,
         }, 401)
 
+    try:
+        user_data = get_user_data(token=token, email=email)
+    except NewsletterException as e:
+        return newsletter_exception_response(e)
+
     status_code = 200
-    user_data = get_user_data(token=token, email=email)
     if not user_data:
         code = errors.BASKET_UNKNOWN_TOKEN if token else errors.BASKET_UNKNOWN_EMAIL
         user_data = {
@@ -486,8 +499,6 @@ def lookup_user(request):
             'code': code,
         }
         status_code = 404
-    elif user_data['status'] == 'error':
-        status_code = 400
 
     return HttpResponseJSON(user_data, status_code)
 
@@ -498,5 +509,5 @@ def list_newsletters(request):
     """
 
     active_newsletters = Newsletter.objects.filter(active=True)
-    return render(request, "news/newsletters.html",
+    return render(request, 'news/newsletters.html',
                   {'newsletters': active_newsletters})