Handle errors while sending emails around changing user data

The most common error is Amazon SES having blacklisted an email address.
However, we handle any error being raised during user.email_user.

Fixes JIRA LMS-131

common/djangoapps/student/tests/factories.py

 from student.models import (User, UserProfile, Registration,
-                            CourseEnrollmentAllowed, CourseEnrollment)
+                            CourseEnrollmentAllowed, CourseEnrollment,
+                            PendingEmailChange)
 from django.contrib.auth.models import Group
 from datetime import datetime
 from factory import DjangoModelFactory, SubFactory, PostGenerationMethodCall, post_generation, Sequence
 from uuid import uuid4
 
+# Factories don't have __init__ methods, and are self documenting
+# pylint: disable=W0232
+
 
 class GroupFactory(DjangoModelFactory):
     FACTORY_FOR = Group
@@ -72,3 +76,17 @@ class CourseEnrollmentAllowedFactory(DjangoModelFactory):
 
     email = 'test@edx.org'
     course_id = 'edX/test/2012_Fall'
+
+
+class PendingEmailChangeFactory(DjangoModelFactory):
+    """Factory for PendingEmailChange objects
+
+    user: generated by UserFactory
+    new_email: sequence of new+email+{}@edx.org
+    activation_key: sequence of integers, padded to 30 characters
+    """
+    FACTORY_FOR = PendingEmailChange
+
+    user = SubFactory(UserFactory)
+    new_email = Sequence(u'new+email+{0}@edx.org'.format)
+    activation_key = Sequence(u'{:0<30d}'.format)

common/djangoapps/student/tests/test_email.py

+import json
+import django.db
+
+from student.tests.factories import UserFactory, RegistrationFactory, PendingEmailChangeFactory
+from student.views import reactivation_email_for_user, change_email_request, confirm_email_change
+from student.models import UserProfile, PendingEmailChange
+from django.contrib.auth.models import User
+from django.test import TestCase, TransactionTestCase
+from django.test.client import RequestFactory
+from mock import Mock, patch
+from django.http import Http404, HttpResponse
+from django.conf import settings
+from nose.plugins.skip import SkipTest
+
+
+class TestException(Exception):
+    """Exception used for testing that nothing will catch explicitly"""
+    pass
+
+
+def mock_render_to_string(template_name, context):
+    """Return a string that encodes template_name and context"""
+    return str((template_name, sorted(context.iteritems())))
+
+
+def mock_render_to_response(template_name, context):
+    """Return an HttpResponse with content that encodes template_name and context"""
+    return HttpResponse(mock_render_to_string(template_name, context))
+
+
+class EmailTestMixin(object):
+    """Adds useful assertions for testing `email_user`"""
+
+    def assertEmailUser(self, email_user, subject_template, subject_context, body_template, body_context):
+        """Assert that `email_user` was used to send and email with the supplied subject and body
+
+        `email_user`: The mock `django.contrib.auth.models.User.email_user` function
+            to verify
+        `subject_template`: The template to have been used for the subject
+        `subject_context`: The context to have been used for the subject
+        `body_template`: The template to have been used for the body
+        `body_context`: The context to have been used for the body
+        """
+        email_user.assert_called_with(
+            mock_render_to_string(subject_template, subject_context),
+            mock_render_to_string(body_template, body_context),
+            settings.DEFAULT_FROM_EMAIL
+        )
+
+
+@patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True))
+@patch('django.contrib.auth.models.User.email_user')
+class ReactivationEmailTests(EmailTestMixin, TestCase):
+    """Test sending a reactivation email to a user"""
+
+    def setUp(self):
+        self.user = UserFactory.create()
+        self.registration = RegistrationFactory.create(user=self.user)
+
+    def reactivation_email(self):
+        """Send the reactivation email, and return the response as json data"""
+        return json.loads(reactivation_email_for_user(self.user).content)
+
+    def assertReactivateEmailSent(self, email_user):
+        """Assert that the correct reactivation email has been sent"""
+        context = {
+            'name': self.user.profile.name,
+            'key': self.registration.activation_key
+        }
+
+        self.assertEmailUser(
+            email_user,
+            'emails/activation_email_subject.txt',
+            context,
+            'emails/activation_email.txt',
+            context
+        )
+
+    def test_reactivation_email_failure(self, email_user):
+        self.user.email_user.side_effect = Exception
+        response_data = self.reactivation_email()
+
+        self.assertReactivateEmailSent(email_user)
+        self.assertFalse(response_data['success'])
+
+    def test_reactivation_email_success(self, email_user):
+        response_data = self.reactivation_email()
+
+        self.assertReactivateEmailSent(email_user)
+        self.assertTrue(response_data['success'])
+
+
+class EmailChangeRequestTests(TestCase):
+    """Test changing a user's email address"""
+
+    def setUp(self):
+        self.user = UserFactory.create()
+        self.new_email = 'new.email@edx.org'
+        self.req_factory = RequestFactory()
+        self.request = self.req_factory.post('unused_url', data={
+            'password': 'test',
+            'new_email': self.new_email
+        })
+        self.request.user = self.user
+        self.user.email_user = Mock()
+
+    def run_request(self, request=None):
+        """Execute request and return result parsed as json
+
+        If request isn't passed in, use self.request instead
+        """
+        if request is None:
+            request = self.request
+
+        response = change_email_request(self.request)
+        return json.loads(response.content)
+
+    def assertFailedRequest(self, response_data, expected_error):
+        """Assert that `response_data` indicates a failed request that returns `expected_error`"""
+        self.assertFalse(response_data['success'])
+        self.assertEquals(expected_error, response_data['error'])
+        self.assertFalse(self.user.email_user.called)
+
+    def test_unauthenticated(self):
+        self.user.is_authenticated = False
+        with self.assertRaises(Http404):
+            change_email_request(self.request)
+        self.assertFalse(self.user.email_user.called)
+
+    def test_invalid_password(self):
+        self.request.POST['password'] = 'wrong'
+        self.assertFailedRequest(self.run_request(), 'Invalid password')
+
+    def test_invalid_emails(self):
+        for email in ('bad_email', 'bad_email@', '@bad_email'):
+            self.request.POST['new_email'] = email
+            self.assertFailedRequest(self.run_request(), 'Valid e-mail address required.')
+
+    def check_duplicate_email(self, email):
+        """Test that a request to change a users email to `email` fails"""
+        request = self.req_factory.post('unused_url', data={
+            'new_email': email,
+            'password': 'test',
+        })
+        request.user = self.user
+        self.assertFailedRequest(self.run_request(request), 'An account with this e-mail already exists.')
+
+    def test_duplicate_email(self):
+        UserFactory.create(email=self.new_email)
+        self.check_duplicate_email(self.new_email)
+
+    def test_capitalized_duplicate_email(self):
+        raise SkipTest("We currently don't check for emails in a case insensitive way, but we should")
+        UserFactory.create(email=self.new_email)
+        self.check_duplicate_email(self.new_email.capitalize())
+
+    # TODO: Finish testing the rest of change_email_request
+
+
+@patch('django.contrib.auth.models.User.email_user')
+@patch('student.views.render_to_response', Mock(side_effect=mock_render_to_response, autospec=True))
+@patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True))
+class EmailChangeConfirmationTests(EmailTestMixin, TransactionTestCase):
+    """Test that confirmation of email change requests function even in the face of exceptions thrown while sending email"""
+    def setUp(self):
+        self.user = UserFactory.create()
+        self.profile = UserProfile.objects.get(user=self.user)
+        self.req_factory = RequestFactory()
+        self.request = self.req_factory.get('unused_url')
+        self.request.user = self.user
+        self.user.email_user = Mock()
+        self.pending_change_request = PendingEmailChangeFactory.create(user=self.user)
+        self.key = self.pending_change_request.activation_key
+
+    def assertRolledBack(self):
+        """Assert that no changes to user, profile, or pending email have been made to the db"""
+        self.assertEquals(self.user.email, User.objects.get(username=self.user.username).email)
+        self.assertEquals(self.profile.meta, UserProfile.objects.get(user=self.user).meta)
+        self.assertEquals(1, PendingEmailChange.objects.count())
+
+    def assertFailedBeforeEmailing(self, email_user):
+        """Assert that the function failed before emailing a user"""
+        self.assertRolledBack()
+        self.assertFalse(email_user.called)
+
+    def check_confirm_email_change(self, expected_template, expected_context):
+        """Call `confirm_email_change` and assert that the content was generated as expected
+
+        `expected_template`: The name of the template that should have been used
+            to generate the content
+        `expected_context`: The context dictionary that should have been used to
+            generate the content
+        """
+        response = confirm_email_change(self.request, self.key)
+        self.assertEquals(
+            mock_render_to_response(expected_template, expected_context).content,
+            response.content
+        )
+
+    def assertChangeEmailSent(self, email_user):
+        """Assert that the correct email was sent to confirm an email change"""
+        context = {
+            'old_email': self.user.email,
+            'new_email': self.pending_change_request.new_email,
+        }
+        self.assertEmailUser(
+            email_user,
+            'emails/email_change_subject.txt',
+            context,
+            'emails/confirm_email_change.txt',
+            context
+        )
+
+    def test_not_pending(self, email_user):
+        self.key = 'not_a_key'
+        self.check_confirm_email_change('invalid_email_key.html', {})
+        self.assertFailedBeforeEmailing(email_user)
+
+    def test_duplicate_email(self, email_user):
+        UserFactory.create(email=self.pending_change_request.new_email)
+        self.check_confirm_email_change('email_exists.html', {})
+        self.assertFailedBeforeEmailing(email_user)
+
+    def test_old_email_fails(self, email_user):
+        email_user.side_effect = [Exception, None]
+        self.check_confirm_email_change('email_change_failed.html', {
+            'email': self.user.email,
+        })
+        self.assertRolledBack()
+        self.assertChangeEmailSent(email_user)
+
+    def test_new_email_fails(self, email_user):
+        email_user.side_effect = [None, Exception]
+        self.check_confirm_email_change('email_change_failed.html', {
+            'email': self.pending_change_request.new_email
+        })
+        self.assertRolledBack()
+        self.assertChangeEmailSent(email_user)
+
+    def test_successful_email_change(self, email_user):
+        self.check_confirm_email_change('email_change_successful.html', {
+            'old_email': self.user.email,
+            'new_email': self.pending_change_request.new_email
+        })
+        self.assertChangeEmailSent(email_user)
+        meta = json.loads(UserProfile.objects.get(user=self.user).meta)
+        self.assertIn('old_emails', meta)
+        self.assertEquals(self.user.email, meta['old_emails'][0][0])
+        self.assertEquals(
+            self.pending_change_request.new_email,
+            User.objects.get(username=self.user.username).email
+        )
+        self.assertEquals(0, PendingEmailChange.objects.count())
+
+    @patch('student.views.PendingEmailChange.objects.get', Mock(side_effect=TestException))
+    @patch('student.views.transaction.rollback', wraps=django.db.transaction.rollback)
+    def test_always_rollback(self, rollback, _email_user):
+        with self.assertRaises(TestException):
+            confirm_email_change(self.request, self.key)
+
+        rollback.assert_called_with()

common/djangoapps/student/views.py

@@ -19,7 +19,7 @@ from django.core.context_processors import csrf
 from django.core.mail import send_mail
 from django.core.urlresolvers import reverse
 from django.core.validators import validate_email, validate_slug, ValidationError
-from django.db import IntegrityError
+from django.db import IntegrityError, transaction
 from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotAllowed, HttpResponseRedirect, Http404
 from django.shortcuts import redirect
 from django_future.csrf import ensure_csrf_cookie, csrf_exempt
@@ -655,7 +655,7 @@ def create_account(request, post_override=None):
         elif not settings.GENERATE_RANDOM_USER_CREDENTIALS:
             res = user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
     except:
-        log.exception(sys.exc_info())
+        log.warning('Unable to send activation email to user', exc_info=True)
         js['value'] = 'Could not send activation e-mail.'
         return HttpResponse(json.dumps(js))
 
@@ -975,7 +975,11 @@ def reactivation_email_for_user(user):
     subject = ''.join(subject.splitlines())
     message = render_to_string('emails/activation_email.txt', d)
 
-    res = user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
+    try:
+        res = user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
+    except:
+        log.warning('Unable to send reactivation email', exc_info=True)
+        return HttpResponse(json.dumps({'success': False, 'error': 'Unable to send reactivation email'}))
 
     return HttpResponse(json.dumps({'success': True}))
 
@@ -1001,7 +1005,7 @@ def change_email_request(request):
         return HttpResponse(json.dumps({'success': False,
                                         'error': 'Valid e-mail address required.'}))
 
-    if len(User.objects.filter(email=new_email)) != 0:
+    if User.objects.filter(email=new_email).count() != 0:
         ## CRITICAL TODO: Handle case sensitivity for e-mails
         return HttpResponse(json.dumps({'success': False,
                                         'error': 'An account with this e-mail already exists.'}))
@@ -1036,41 +1040,63 @@ def change_email_request(request):
 
 
 @ensure_csrf_cookie
+@transaction.commit_manually
 def confirm_email_change(request, key):
     ''' User requested a new e-mail. This is called when the activation
     link is clicked. We confirm with the old e-mail, and update
     '''
     try:
-        pec = PendingEmailChange.objects.get(activation_key=key)
-    except PendingEmailChange.DoesNotExist:
-        return render_to_response("invalid_email_key.html", {})
-
-    user = pec.user
-    d = {'old_email': user.email,
-         'new_email': pec.new_email}
+        try:
+            pec = PendingEmailChange.objects.get(activation_key=key)
+        except PendingEmailChange.DoesNotExist:
+            transaction.rollback()
+            return render_to_response("invalid_email_key.html", {})
+
+        user = pec.user
+        address_context = {
+            'old_email': user.email,
+            'new_email': pec.new_email
+        }
 
-    if len(User.objects.filter(email=pec.new_email)) != 0:
-        return render_to_response("email_exists.html", d)
+        if len(User.objects.filter(email=pec.new_email)) != 0:
+            transaction.rollback()
+            return render_to_response("email_exists.html", {})
+
+        subject = render_to_string('emails/email_change_subject.txt', address_context)
+        subject = ''.join(subject.splitlines())
+        message = render_to_string('emails/confirm_email_change.txt', address_context)
+        up = UserProfile.objects.get(user=user)
+        meta = up.get_meta()
+        if 'old_emails' not in meta:
+            meta['old_emails'] = []
+        meta['old_emails'].append([user.email, datetime.datetime.now().isoformat()])
+        up.set_meta(meta)
+        up.save()
+        # Send it to the old email...
+        try:
+            user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
+        except Exception:
+            transaction.rollback()
+            log.warning('Unable to send confirmation email to old address', exc_info=True)
+            return render_to_response("email_change_failed.html", {'email': user.email})
 
-    subject = render_to_string('emails/email_change_subject.txt', d)
-    subject = ''.join(subject.splitlines())
-    message = render_to_string('emails/confirm_email_change.txt', d)
-    up = UserProfile.objects.get(user=user)
-    meta = up.get_meta()
-    if 'old_emails' not in meta:
-        meta['old_emails'] = []
-    meta['old_emails'].append([user.email, datetime.datetime.now().isoformat()])
-    up.set_meta(meta)
-    up.save()
-    # Send it to the old email...
-    user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
-    user.email = pec.new_email
-    user.save()
-    pec.delete()
-    # And send it to the new email...
-    user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
-
-    return render_to_response("email_change_successful.html", d)
+        user.email = pec.new_email
+        user.save()
+        pec.delete()
+        # And send it to the new email...
+        try:
+            user.email_user(subject, message, settings.DEFAULT_FROM_EMAIL)
+        except Exception:
+            transaction.rollback()
+            log.warning('Unable to send confirmation email to new address', exc_info=True)
+            return render_to_response("email_change_failed.html", {'email': pec.new_email})
+
+        transaction.commit()
+        return render_to_response("email_change_successful.html", address_context)
+    except Exception:
+        # If we get an unexpected exception, be sure to rollback the transaction
+        transaction.rollback()
+        raise
 
 
 @ensure_csrf_cookie

lms/templates/email_change_failed.html

+<h1>E-mail change failed.</h1>
+
+<p>We were unable to send a confirmation email to ${email}</p>

pylintrc

@@ -110,7 +110,9 @@ generated-members=
     get_url,
     size,
     content,
-    status_code
+    status_code,
+# For factory_body factories
+    create
 
 
 [BASIC]

requirements/edx/base.txt

@@ -71,7 +71,7 @@ transifex-client==0.8
 coverage==3.6
 factory_boy==2.0.2
 lettuce==0.2.16
-mock==0.8.0
+mock==1.0.1
 nosexcover==1.0.7
 pep8==1.4.5
 pylint==0.28