From c656998e8db52a4bb036823d79bad42b588c6e39 Mon Sep 17 00:00:00 2001 From: Piotr Mitros Date: Mon, 23 Mar 2015 11:55:27 -0400 Subject: [PATCH] Make e-mail activation optional --- .../student/tests/test_create_account.py | 13 ++++++-- common/djangoapps/student/views.py | 33 ++++++++++++++++--- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index 88f574bd74..8119f75826 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/common/djangoapps/student/tests/test_create_account.py @@ -175,7 +175,7 @@ class TestCreateAccount(TestCase): profile = self.create_account_and_fetch_profile() self.assertIsNone(profile.year_of_birth) - def base_extauth_bypass_sending_activation_email(self, bypass_activation_email_for_extauth_setting): + def base_extauth_bypass_sending_activation_email(self, bypass_activation_email): """ Tests user creation without sending activation email when doing external auth @@ -196,7 +196,7 @@ class TestCreateAccount(TestCase): student.views.create_account(request) # check that send_mail is called - if bypass_activation_email_for_extauth_setting: + if bypass_activation_email: self.assertFalse(mock_send_mail.called) else: self.assertTrue(mock_send_mail.called) @@ -219,6 +219,15 @@ class TestCreateAccount(TestCase): """ self.base_extauth_bypass_sending_activation_email(False) + @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") + @mock.patch.dict(settings.FEATURES, {'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': False, 'AUTOMATIC_AUTH_FOR_TESTING': False, 'SKIP_EMAIL_VALIDATION': True}) + def test_extauth_bypass_sending_activation_email_without_bypass(self): + """ + Tests user creation without sending activation email when + settings.FEATURES['BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH']=False and doing external auth + """ + self.base_extauth_bypass_sending_activation_email(True) + @ddt.data(True, False) def test_discussions_email_digest_pref(self, digest_enabled): with mock.patch.dict("student.models.settings.FEATURES", {"ENABLE_DISCUSSION_EMAIL_DIGEST": digest_enabled}): diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index b3f76781a0..f990ca6060 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -23,7 +23,7 @@ from django.core.urlresolvers import reverse from django.core.validators import validate_email, ValidationError from django.db import IntegrityError, transaction from django.http import (HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, - Http404) + HttpResponseServerError, Http404) from django.shortcuts import redirect from django.utils.translation import ungettext from django_future.csrf import ensure_csrf_cookie @@ -1408,6 +1408,19 @@ def create_account_with_params(request, params): Raises AccountValidationError if an account with the username or email specified by params already exists, or ValidationError if any of the given parameters is invalid for any other reason. + + Issues with this code: + * It is not transactional. If there is a failure part-way, an incomplete + account will be created and left in the database. + * Third-party auth passwords are not verified. There is a comment that + they are unused, but it would be helpful to have a sanity check that + they are sane. + * It is over 300 lines long (!) and includes disprate functionality, from + registration e-mails to all sorts of other things. It should be broken + up into semantically meaningful functions. + * The user-facing text is rather unfriendly (e.g. "Username must be a + minimum of two characters long" rather than "Please use a username of + at least two characters"). """ # Copy params so we can modify it; we can't just do dict(params) because if # params is request.POST, that results in a dict containing lists of values @@ -1555,9 +1568,19 @@ def create_account_with_params(request, params): subject = ''.join(subject.splitlines()) message = render_to_string('emails/activation_email.txt', context) - # don't send email if we are doing load testing or random user generation for some reason - # or external auth with bypass activated + # Don't send email if we are: + # + # 1. Doing load testing. + # 2. Random user generation for other forms of testing. + # 3. External auth bypassing activation. + # 4. Have the platform configured to not require e-mail activation. + # + # Note that this feature is only tested as a flag set one way or + # the other for *new* systems. we need to be careful about + # changing settings on a running system to make sure no users are + # left in an inconsistent state (or doing a migration if they are). send_email = ( + not settings.FEATURES.get('SKIP_EMAIL_VALIDATION', None) and not settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING') and not (do_external_auth and settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH')) ) @@ -1576,6 +1599,8 @@ def create_account_with_params(request, params): user.email_user(subject, message, from_address) except Exception: # pylint: disable=broad-except log.error(u'Unable to send activation email to user from "%s"', from_address, exc_info=True) + else: + registration.activate() # Immediately after a user creates an account, we log them in. They are only # logged in until they close the browser. They can't log in again until they click @@ -1794,7 +1819,7 @@ def activate_account(request, key): "registration/activation_invalid.html", {'csrf': csrf(request)['csrf_token']} ) - return HttpResponse(_("Unknown error. Please e-mail us to let us know how it happened.")) + return HttpResponseServerError(_("Unknown error. Please e-mail us to let us know how it happened.")) @csrf_exempt