Ensure account creation succeeds before creating comments service user

Also improves handling in cases where account creation did succeed, but
comments service user creation did not.

Depends on commit 88abdd8a0b20bc9816f487b25abdea1953f5661d in https://github.com/edx/cs_comments_service

JIRA: FOR-522

common/djangoapps/student/management/commands/create_user.py

 from optparse import make_option
 from django.core.management.base import BaseCommand
-from student.models import CourseEnrollment, Registration
-from student.views import _do_create_account
+from student.models import CourseEnrollment, Registration, create_comments_service_user
+from student.views import _do_create_account, AccountValidationError
 from django.contrib.auth.models import User
 
 from track.management.tracked_command import TrackedCommand
@@ -74,17 +74,16 @@ class Command(TrackedCommand):
             'honor_code': u'true',
             'terms_of_service': u'true',
         }
-        create_account = _do_create_account(post_data)
-        if isinstance(create_account, tuple):
-            user = create_account[0]
+        try:
+            user, profile, reg = _do_create_account(post_data)
             if options['staff']:
                 user.is_staff = True
                 user.save()
-            reg = Registration.objects.get(user=user)
             reg.activate()
             reg.save()
-        else:
-            print create_account
+            create_comments_service_user(user)
+        except AccountValidationError as e:
+            print e.message
             user = User.objects.get(email=options['email'])
         if options['course']:
             CourseEnrollment.enroll(user, options['course'], mode=options['mode'])

common/djangoapps/student/models.py

@@ -831,18 +831,18 @@ def add_user_to_default_group(user, group):
     utg.save()
 
 
-@receiver(post_save, sender=User)
-def update_user_information(sender, instance, created, **kwargs):
+def create_comments_service_user(user):
     if not settings.FEATURES['ENABLE_DISCUSSION_SERVICE']:
         # Don't try--it won't work, and it will fill the logs with lots of errors
         return
     try:
-        cc_user = cc.User.from_django_user(instance)
+        cc_user = cc.User.from_django_user(user)
         cc_user.save()
     except Exception as e:
         log = logging.getLogger("edx.discussion")
-        log.error(unicode(e))
-        log.error("update user info to discussion failed for user with id: " + str(instance.id))
+        log.error(
+            "Could not create comments service user with id {}".format(user.id),
+            exc_info=True)
 
 # Define login and logout handlers here in the models file, instead of the views file,
 # so that they are more likely to be loaded when a Studio user brings up the Studio admin

common/djangoapps/student/tests/test_create_account.py

@@ -3,13 +3,17 @@
 import ddt
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
-from django.test import TestCase
+from django.db.transaction import rollback
+from django.test import TestCase, TransactionTestCase
 from django.test.utils import override_settings
 import mock
 
 from user_api.models import UserPreference
 from lang_pref import LANGUAGE_KEY
 
+import student
+
+TEST_CS_URL = 'https://comments.service.test:123/'
 
 @ddt.ddt
 class TestCreateAccount(TestCase):
@@ -41,3 +45,43 @@ class TestCreateAccount(TestCase):
         self.assertEqual(response.status_code, 200)
         user = User.objects.get(username=self.username)
         self.assertEqual(UserPreference.get_preference(user, LANGUAGE_KEY), lang)
+
+
+@mock.patch.dict("student.models.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
+@mock.patch("lms.lib.comment_client.User.base_url", TEST_CS_URL)
+@mock.patch("lms.lib.comment_client.utils.requests.request", return_value=mock.Mock(status_code=200, text='{}'))
+class TestCreateCommentsServiceUser(TransactionTestCase):
+
+    def setUp(self):
+        self.username = "test_user"
+        self.url = reverse("create_account")
+        self.params = {
+            "username": self.username,
+            "email": "test@example.org",
+            "password": "testpass",
+            "name": "Test User",
+            "honor_code": "true",
+            "terms_of_service": "true",
+        }
+
+    def test_cs_user_created(self, request):
+        "If user account creation succeeds, we should create a comments service user"
+        response = self.client.post(self.url, self.params)
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(request.called)
+        args, kwargs = request.call_args
+        self.assertEqual(args[0], 'put')
+        self.assertTrue(args[1].startswith(TEST_CS_URL))
+        self.assertEqual(kwargs['data']['username'], self.params['username'])
+
+    @mock.patch("student.models.Registration.register", side_effect=Exception)
+    def test_cs_user_not_created(self, register, request):
+        "If user account creation fails, we should not create a comments service user"
+        try:
+            response = self.client.post(self.url, self.params)
+        except:
+            pass
+        with self.assertRaises(User.DoesNotExist):
+            User.objects.get(username=self.username)
+        self.assertTrue(register.called)
+        self.assertFalse(request.called)

common/djangoapps/student/views.py

@@ -38,7 +38,8 @@ from course_modes.models import CourseMode
 from student.models import (
     Registration, UserProfile, PendingNameChange,
     PendingEmailChange, CourseEnrollment, unique_id_for_user,
-    CourseEnrollmentAllowed, UserStanding, LoginFailures
+    CourseEnrollmentAllowed, UserStanding, LoginFailures,
+    create_comments_service_user
 )
 from student.forms import PasswordResetFormNoActive
 from student.firebase_token_generator import create_token
@@ -951,6 +952,11 @@ def change_setting(request):
     })
 
 
+class AccountValidationError(Exception):
+    def __init__(self, message, field):
+        super(AccountValidationError, self).__init__(message)
+        self.field = field
+
 def _do_create_account(post_vars):
     """
     Given cleaned post variables, create the User and UserProfile objects, as well as the
@@ -970,19 +976,19 @@ def _do_create_account(post_vars):
     try:
         user.save()
     except IntegrityError:
-        js = {'success': False}
         # Figure out the cause of the integrity error
         if len(User.objects.filter(username=post_vars['username'])) > 0:
-            js['value'] = _("An account with the Public Username '{username}' already exists.").format(username=post_vars['username'])
-            js['field'] = 'username'
-            return JsonResponse(js, status=400)
-
-        if len(User.objects.filter(email=post_vars['email'])) > 0:
-            js['value'] = _("An account with the Email '{email}' already exists.").format(email=post_vars['email'])
-            js['field'] = 'email'
-            return JsonResponse(js, status=400)
-
-        raise
+            raise AccountValidationError(
+                _("An account with the Public Username '{username}' already exists.").format(username=post_vars['username']),
+                field="username"
+                )
+        elif len(User.objects.filter(email=post_vars['email'])) > 0:
+            raise AccountValidationError(
+                _("An account with the Email '{email}' already exists.").format(email=post_vars['email']),
+                field="email"
+                )
+        else:
+            raise
 
     registration.register(user)
 
@@ -1005,6 +1011,7 @@ def _do_create_account(post_vars):
         profile.save()
     except Exception:
         log.exception("UserProfile creation failed for user {id}.".format(id=user.id))
+        raise
 
     UserPreference.set_preference(user, LANGUAGE_KEY, get_language())
 
@@ -1150,11 +1157,17 @@ def create_account(request, post_override=None):
             return JsonResponse(js, status=400)
 
     # Ok, looks like everything is legit.  Create the account.
-    ret = _do_create_account(post_vars)
-    if isinstance(ret, HttpResponse):  # if there was an error then return that
-        return ret
+    try:
+        with transaction.commit_on_success():
+            ret = _do_create_account(post_vars)
+    except AccountValidationError as e:
+        return JsonResponse({'success': False, 'value': e.message, 'field': e.field}, status=400)
+
     (user, profile, registration) = ret
 
+    dog_stats_api.increment("common.student.account_created")
+    create_comments_service_user(user)
+
     context = {
         'name': post_vars['name'],
         'key': registration.activation_key,
@@ -1213,8 +1226,6 @@ def create_account(request, post_override=None):
             login_user.save()
             AUDIT_LOG.info(u"Login activated on extauth account - {0} ({1})".format(login_user.username, login_user.email))
 
-    dog_stats_api.increment("common.student.account_created")
-
     response = JsonResponse({
         'success': True,
         'redirect_url': try_change_enrollment(request),
@@ -1283,20 +1294,16 @@ def auto_auth(request):
 
     # Attempt to create the account.
     # If successful, this will return a tuple containing
-    # the new user object; otherwise it will return an error
-    # message.
-    result = _do_create_account(post_data)
-
-    if isinstance(result, tuple):
-        user = result[0]
-
-    # If we did not create a new account, the user might already
-    # exist.  Attempt to retrieve it.
-    else:
+    # the new user object.
+    try:
+        user, profile, reg = _do_create_account(post_data)
+    except AccountValidationError:
+        # Attempt to retrieve the existing user.
         user = User.objects.get(username=username)
         user.email = email
         user.set_password(password)
         user.save()
+        reg = Registration.objects.get(user=user)
 
     # Set the user's global staff bit
     if is_staff is not None:
@@ -1304,7 +1311,6 @@ def auto_auth(request):
         user.save()
 
     # Activate the user
-    reg = Registration.objects.get(user=user)
     reg.activate()
     reg.save()
 
@@ -1321,6 +1327,8 @@ def auto_auth(request):
     user = authenticate(username=username, password=password)
     login(request, user)
 
+    create_comments_service_user(user)
+
     # Provide the user with a valid CSRF token
     # then return a 200 response
     success_msg = u"Logged in user {0} ({1}) with password {2} and user_id {3}".format(

lms/lib/comment_client/user.py

@@ -80,7 +80,16 @@ class User(models.Model):
         retrieve_params = self.default_retrieve_params
         if self.attributes.get('course_id'):
             retrieve_params['course_id'] = self.course_id
-        response = perform_request('get', url, retrieve_params)
+        try:
+            response = perform_request('get', url, retrieve_params)
+        except CommentClientRequestError as e:
+            if e.status_code == 404:
+                # attempt to gracefully recover from a previous failure
+                # to sync this user to the comments service.
+                self.save()
+                response = perform_request('get', url, retrieve_params)
+            else:
+                raise
         self.update_attributes(**response)