Merge pull request #2957 from edx/jsa/forum-user-sync

Ensure account creation succeeds before creating comments service user

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)