Merge pull request #2090 from edx/fix/cale/amazon-ses-errors

Fix/cale/amazon ses errors

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, Factory, SubFactory, PostGenerationMethodCall, post_generation
+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
 
-    name = 'staff_MITx/999/Robot_Super_Course'
+    name = u'staff_MITx/999/Robot_Super_Course'
 
 
 class UserProfileFactory(DjangoModelFactory):
     FACTORY_FOR = UserProfile
 
     user = None
-    name = 'Robot Test'
+    name = u'Robot Test'
     level_of_education = None
-    gender = 'm'
+    gender = u'm'
     mailing_address = None
-    goals = 'World domination'
+    goals = u'World domination'
 
 
 class RegistrationFactory(DjangoModelFactory):
     FACTORY_FOR = Registration
 
     user = None
-    activation_key = uuid4().hex
+    activation_key = uuid4().hex.decode('ascii')
 
 
 class UserFactory(DjangoModelFactory):
     FACTORY_FOR = User
 
-    username = 'robot'
-    email = 'robot+test@edx.org'
+    username = Sequence(u'robot{0}'.format)
+    email = Sequence(u'robot+test+{0}@edx.org'.format)
     password = PostGenerationMethodCall('set_password',
                                         'test')
-    first_name = 'Robot'
+    first_name = Sequence(u'Robot{0}'.format)
     last_name = 'Test'
     is_staff = False
     is_active = True
@@ -64,7 +68,7 @@ class CourseEnrollmentFactory(DjangoModelFactory):
     FACTORY_FOR = CourseEnrollment
 
     user = SubFactory(UserFactory)
-    course_id = 'edX/toy/2012_Fall'
+    course_id = u'edX/toy/2012_Fall'
 
 
 class CourseEnrollmentAllowedFactory(DjangoModelFactory):
@@ -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/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

doc/testing.md

@@ -8,7 +8,7 @@ and acceptance tests.
 ### Unit Tests
 
 * Each test case should be concise: setup, execute, check, and teardown.
-If you find yourself writing tests with many steps, consider refactoring 
+If you find yourself writing tests with many steps, consider refactoring
 the unit under tests into smaller units, and then testing those individually.
 
 * As a rule of thumb, your unit tests should cover every code branch.
@@ -16,19 +16,19 @@ the unit under tests into smaller units, and then testing those individually.
 * Mock or patch external dependencies.
 We use [voidspace mock](http://www.voidspace.org.uk/python/mock/).
 
-* We unit test Python code (using [unittest](http://docs.python.org/2/library/unittest.html)) and 
+* We unit test Python code (using [unittest](http://docs.python.org/2/library/unittest.html)) and
 Javascript (using [Jasmine](http://pivotal.github.io/jasmine/))
 
 ### Integration Tests
 * Test several units at the same time.
 Note that you can still mock or patch dependencies
-that are not under test!  For example, you might test that 
-`LoncapaProblem`, `NumericalResponse`, and `CorrectMap` in the 
+that are not under test!  For example, you might test that
+`LoncapaProblem`, `NumericalResponse`, and `CorrectMap` in the
 `capa` package work together, while still mocking out template rendering.
 
 * Use integration tests to ensure that units are hooked up correctly.
-You do not need to test every possible input--that's what unit 
-tests are for.  Instead, focus on testing the "happy path" 
+You do not need to test every possible input--that's what unit
+tests are for.  Instead, focus on testing the "happy path"
 to verify that the components work together correctly.
 
 * Many of our tests use the [Django test client](https://docs.djangoproject.com/en/dev/topics/testing/overview/) to simulate
@@ -43,8 +43,8 @@ these tests simulate user interactions through the browser using
 
 Overall, you want to write the tests that **maximize coverage**
 while **minimizing maintenance**.
-In practice, this usually means investing heavily 
-in unit tests, which tend to be the most robust to changes in the code base.  
+In practice, this usually means investing heavily
+in unit tests, which tend to be the most robust to changes in the code base.
 
 ![Test Pyramid](test_pyramid.png)
 
@@ -53,13 +53,13 @@ and acceptance tests.  Most of our tests are unit tests or integration tests.
 
 ## Test Locations
 
-* Python unit and integration tests: Located in 
+* Python unit and integration tests: Located in
 subpackages called `tests`.
-For example, the tests for the `capa` package are located in 
+For example, the tests for the `capa` package are located in
 `common/lib/capa/capa/tests`.
 
 * Javascript unit tests: Located in `spec` folders.  For example,
-`common/lib/xmodule/xmodule/js/spec` and `{cms,lms}/static/coffee/spec`  
+`common/lib/xmodule/xmodule/js/spec` and `{cms,lms}/static/coffee/spec`
 For consistency, you should use the same directory structure for implementation
 and test.  For example, the test for `src/views/module.coffee`
 should be written in `spec/views/module_spec.coffee`.
@@ -101,7 +101,7 @@ You can run tests using `rake` commands.  For example,
 
     rake test
 
-runs all the tests.  It also runs `collectstatic`, which prepares the static files used by the site (for example, compiling Coffeescript to Javascript).  
+runs all the tests.  It also runs `collectstatic`, which prepares the static files used by the site (for example, compiling Coffeescript to Javascript).
 
 You can also run the tests without `collectstatic`, which tends to be faster:
 
@@ -117,12 +117,11 @@ xmodule can be tested independently, with this:
 
 To run a single django test class:
 
-    django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/courseware/tests/tests.py:TestViewAuth
+    rake test_lms[courseware.tests.tests:testViewAuth]
 
 To run a single django test:
 
-    django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/courseware/tests/tests.py:TestViewAuth.test_dark_launch
-
+    rake test_lms[courseware.tests.tests:TestViewAuth.test_dark_launch]
 
 To run a single nose test file:
 
@@ -150,7 +149,7 @@ If the `phantomjs` binary is not on the path, set the `PHANTOMJS_PATH` environme
 
     PHANTOMJS_PATH=/path/to/phantomjs rake phantomjs_jasmine_{lms,cms}
 
-Once you have run the `rake` command, your browser should open to 
+Once you have run the `rake` command, your browser should open to
 to `http://localhost/_jasmine/`, which displays the test results.
 
 **Troubleshooting**: If you get an error message while running the `rake` task,
@@ -163,7 +162,7 @@ Most of our tests use [Splinter](http://splinter.cobrateam.info/)
 to simulate UI browser interactions.  Splinter, in turn,
 uses [Selenium](http://docs.seleniumhq.org/) to control the Chrome browser.
 
-**Prerequisite**: You must have [ChromeDriver](https://code.google.com/p/selenium/wiki/ChromeDriver) 
+**Prerequisite**: You must have [ChromeDriver](https://code.google.com/p/selenium/wiki/ChromeDriver)
 installed to run the tests in Chrome.  The tests are confirmed to run
 with Chrome (not Chromium) version 26.0.0.1410.63 with ChromeDriver
 version r195636.
@@ -190,7 +189,7 @@ Try running:
 
     pip install -r requirements.txt
 
-**Note**: The acceptance tests can *not* currently run in parallel.  
+**Note**: The acceptance tests can *not* currently run in parallel.
 
 ## Viewing Test Coverage
 

jenkins/test.sh

@@ -73,8 +73,8 @@ rake pylint > pylint.log || cat pylint.log
 TESTS_FAILED=0
 
 # Run the python unit tests
-rake test_cms[false] || TESTS_FAILED=1
-rake test_lms[false] || TESTS_FAILED=1
+rake test_cms || TESTS_FAILED=1
+rake test_lms || TESTS_FAILED=1
 rake test_common/lib/capa || TESTS_FAILED=1
 rake test_common/lib/xmodule || TESTS_FAILED=1
 

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]

rakefiles/tests.rake

@@ -12,10 +12,11 @@ def run_under_coverage(cmd, root)
     return cmd
 end
 
-def run_tests(system, report_dir, stop_on_failure=true)
+def run_tests(system, report_dir, test_id=nil, stop_on_failure=true)
     ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml")
     dirs = Dir["common/djangoapps/*"] + Dir["#{system}/djangoapps/*"]
-    cmd = django_admin(system, :test, 'test', '--logging-clear-handlers', *dirs.each)
+    test_id = dirs.join(' ') if test_id.nil? or test_id == ''
+    cmd = django_admin(system, :test, 'test', '--logging-clear-handlers', test_id)
     sh(run_under_coverage(cmd, system)) do |ok, res|
         if !ok and stop_on_failure
             abort "Test failed!"
@@ -44,13 +45,13 @@ TEST_TASK_DIRS = []
 
     # Per System tasks
     desc "Run all django tests on our djangoapps for the #{system}"
-    task "test_#{system}", [:stop_on_failure] => ["clean_test_files", :predjango, "#{system}:gather_assets:test", "fasttest_#{system}"]
+    task "test_#{system}", [:test_id, :stop_on_failure] => ["clean_test_files", :predjango, "#{system}:gather_assets:test", "fasttest_#{system}"]
 
     # Have a way to run the tests without running collectstatic -- useful when debugging without
     # messing with static files.
-    task "fasttest_#{system}", [:stop_on_failure] => [report_dir, :install_prereqs, :predjango] do |t, args|
-        args.with_defaults(:stop_on_failure => 'true')
-        run_tests(system, report_dir, args.stop_on_failure)
+    task "fasttest_#{system}", [:test_id, :stop_on_failure] => [report_dir, :install_prereqs, :predjango] do |t, args|
+        args.with_defaults(:stop_on_failure => 'true', :test_id => nil)
+        run_tests(system, report_dir, args.test_id, args.stop_on_failure)
     end
 
     # Run acceptance tests
@@ -100,7 +101,7 @@ end
 
 task :test do
     TEST_TASK_DIRS.each do |dir|
-        Rake::Task["test_#{dir}"].invoke(false)
+        Rake::Task["test_#{dir}"].invoke(nil, false)
     end
 
     if $failed_tests > 0

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