From b20c8f9248e9761d5e685ac8a529cf659ac449cf Mon Sep 17 00:00:00 2001
From: jsa <jsa@edx.org>
Date: Fri, 14 Mar 2014 11:02:36 -0400
Subject: [PATCH] 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
---
.../management/commands/create_user.py | 15 ++---
common/djangoapps/student/models.py | 10 +--
.../student/tests/test_create_account.py | 46 ++++++++++++-
common/djangoapps/student/views.py | 64 +++++++++++--------
lms/lib/comment_client/user.py | 11 +++-
5 files changed, 103 insertions(+), 43 deletions(-)
diff --git a/common/djangoapps/student/management/commands/create_user.py b/common/djangoapps/student/management/commands/create_user.py
index d375dedc54a..cacc5dea8ea 100644
--- a/common/djangoapps/student/management/commands/create_user.py
+++ b/common/djangoapps/student/management/commands/create_user.py
@@ -1,7 +1,7 @@
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'])
diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py
index c2fcdea3547..8f2952fdfe4 100644
--- a/common/djangoapps/student/models.py
+++ b/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
diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py
index 1f5f4943063..bc9503d1d05 100644
--- a/common/djangoapps/student/tests/test_create_account.py
+++ b/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)
diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py
index 61d2aa22d6c..54f3e2bfb3d 100644
--- a/common/djangoapps/student/views.py
+++ b/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(
diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py
index 014fe0a810e..77e8b0d0116 100644
--- a/lms/lib/comment_client/user.py
+++ b/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)
--
GitLab