diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py
index 863dcd0096da45530421d1a1f032515f7b9b4ff2..bac42c7f6321d377858023833254a106e742214f 100644
--- a/openedx/core/djangoapps/course_groups/cohorts.py
+++ b/openedx/core/djangoapps/course_groups/cohorts.py
@@ -175,9 +175,7 @@ def get_cohorted_commentables(course_key):
@transaction.commit_on_success
def get_cohort(user, course_key, assign=True, use_cached=False):
- """
- Given a Django user and a CourseKey, return the user's cohort in that
- cohort.
+ """Returns the user's cohort for the specified course.
The cohort for the user is cached for the duration of a request. Pass
use_cached=True to use the cached value instead of fetching from the
diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py
index d625d7817a6b9b17601c79a49a758ccac064fe97..a11eab68e88a75d73feac6503d444d7788e439a9 100644
--- a/openedx/core/djangoapps/user_api/accounts/api.py
+++ b/openedx/core/djangoapps/user_api/accounts/api.py
@@ -7,7 +7,7 @@ from django.conf import settings
from django.core.validators import validate_email, validate_slug, ValidationError
from student.models import User, UserProfile, Registration
-from student.views import validate_new_email, do_email_change_request
+from student import views as student_views
from ..errors import (
AccountUpdateError, AccountValidationError, AccountUsernameInvalid, AccountPasswordInvalid,
@@ -92,7 +92,6 @@ def get_account_settings(requesting_user, username=None, configuration=None, vie
return visible_settings
-@transaction.commit_on_success
@intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError])
def update_account_settings(requesting_user, update, username=None):
"""Update user account information.
@@ -154,7 +153,7 @@ def update_account_settings(requesting_user, update, username=None):
if read_only_fields:
for read_only_field in read_only_fields:
field_errors[read_only_field] = {
- "developer_message": "This field is not editable via this API",
+ "developer_message": u"This field is not editable via this API",
"user_message": _(u"Field '{field_name}' cannot be edited.").format(field_name=read_only_field)
}
del update[read_only_field]
@@ -168,7 +167,7 @@ def update_account_settings(requesting_user, update, username=None):
# If the user asked to change email, validate it.
if changing_email:
try:
- validate_new_email(existing_user, new_email)
+ student_views.validate_new_email(existing_user, new_email)
except ValueError as err:
field_errors["email"] = {
"developer_message": u"Error thrown from validate_new_email: '{}'".format(err.message),
@@ -206,7 +205,7 @@ def update_account_settings(requesting_user, update, username=None):
# And try to send the email change request if necessary.
if changing_email:
try:
- do_email_change_request(existing_user, new_email)
+ student_views.do_email_change_request(existing_user, new_email)
except ValueError as err:
raise AccountUpdateError(
u"Error thrown from do_email_change_request: '{}'".format(err.message),
diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py
index e516fc419968bade164823840e91c4105ec269dc..09c406779a5f94cef4273a842d661b16164569f5 100644
--- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py
@@ -6,6 +6,7 @@ from mock import patch
from django.conf import settings
from django.core.urlresolvers import reverse
+from django.test.testcases import TransactionTestCase
from rest_framework.test import APITestCase, APIClient
from student.tests.factories import UserFactory
@@ -509,3 +510,39 @@ class TestAccountAPI(UserAPITestCase):
error_response.data["developer_message"]
)
self.assertIsNone(error_response.data["user_message"])
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class TestAccountAPITransactions(TransactionTestCase):
+ """
+ Tests the transactional behavior of the account API
+ """
+ test_password = "test"
+
+ def setUp(self):
+ super(TestAccountAPITransactions, self).setUp()
+ self.client = APIClient()
+ self.user = UserFactory.create(password=self.test_password)
+ self.url = reverse("accounts_api", kwargs={'username': self.user.username})
+
+ @patch('student.views.do_email_change_request')
+ def test_update_account_settings_rollback(self, mock_email_change):
+ """
+ Verify that updating account settings is transactional when a failure happens.
+ """
+ # Send a PATCH request with updates to both profile information and email.
+ # Throw an error from the method that is used to process the email change request
+ # (this is the last thing done in the api method). Verify that the profile did not change.
+ mock_email_change.side_effect = [ValueError, "mock value error thrown"]
+ self.client.login(username=self.user.username, password=self.test_password)
+ old_email = self.user.email
+
+ json_data = {"email": "foo@bar.com", "gender": "o"}
+ response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json")
+ self.assertEqual(400, response.status_code)
+
+ # Verify that GET returns the original preferences
+ response = self.client.get(self.url)
+ data = response.data
+ self.assertEqual(old_email, data["email"])
+ self.assertEqual(u"m", data["gender"])
diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py
index f749cfad49b35d63e5d855b3c6587dab7486f8a2..3865742d1a5104f910f802f56122b8cb085ac8bc 100644
--- a/openedx/core/djangoapps/user_api/accounts/views.py
+++ b/openedx/core/djangoapps/user_api/accounts/views.py
@@ -4,6 +4,7 @@ NOTE: this API is WIP and has not yet been approved. Do not use this API without
For more information, see:
https://openedx.atlassian.net/wiki/display/TNL/User+API
"""
+from django.db import transaction
from rest_framework.views import APIView
from rest_framework.response import Response
from rest_framework import status
@@ -117,7 +118,8 @@ class AccountView(APIView):
else an error response with status code 415 will be returned.
"""
try:
- update_account_settings(request.user, request.DATA, username=username)
+ with transaction.commit_on_success():
+ update_account_settings(request.user, request.DATA, username=username)
except (UserNotFound, UserNotAuthorized):
return Response(status=status.HTTP_404_NOT_FOUND)
except AccountValidationError as err:
diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py
index c3a2b3805d116ffdef436967cc3360081c9cb5a8..447282a4bd05276b15651ed1a7ee39cfad24a45d 100644
--- a/openedx/core/djangoapps/user_api/preferences/api.py
+++ b/openedx/core/djangoapps/user_api/preferences/api.py
@@ -11,8 +11,9 @@ from pytz import UTC
from django.conf import settings
from django.contrib.auth.models import User
from django.core.exceptions import ObjectDoesNotExist
-from django.db import transaction, IntegrityError
+from django.db import IntegrityError
from django.utils.translation import ugettext as _
+from django.utils.translation import ugettext_noop
from student.models import UserProfile
@@ -77,7 +78,6 @@ def get_user_preferences(requesting_user, username=None):
@intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError])
-@transaction.commit_on_success
def update_user_preferences(requesting_user, update, username=None):
"""Update the user preferences for the given username.
@@ -138,7 +138,6 @@ def update_user_preferences(requesting_user, update, username=None):
@intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError])
-@transaction.commit_on_success
def set_user_preference(requesting_user, preference_key, preference_value, username=None):
"""Update a user preference for the given username.
@@ -173,7 +172,6 @@ def set_user_preference(requesting_user, preference_key, preference_value, usern
@intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError])
-@transaction.commit_on_success
def delete_user_preference(requesting_user, preference_key, username=None):
"""Deletes a user preference on behalf of a requesting user.
@@ -353,11 +351,12 @@ def validate_user_preference_serializer(serializer, preference_key, preference_v
PreferenceValidationError: the supplied key and/or value for a user preference are invalid.
"""
if preference_value is None or unicode(preference_value).strip() == '':
- message = _(u"Preference '{preference_key}' cannot be set to an empty value.").format(
- preference_key=preference_key
- )
+ format_string = ugettext_noop(u"Preference '{preference_key}' cannot be set to an empty value.")
raise PreferenceValidationError({
- preference_key: {"developer_message": message, "user_message": message}
+ preference_key: {
+ "developer_message": format_string.format(preference_key=preference_key),
+ "user_message": _(format_string).format(preference_key=preference_key)
+ }
})
if not serializer.is_valid():
developer_message = u"Value '{preference_value}' not valid for preference '{preference_key}': {error}".format(
diff --git a/openedx/core/djangoapps/user_api/preferences/tests/test_views.py b/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
index 3a9fac142379e73cb96933231bf05264a061c420..d3f4830edc7a72153875475a58097611c201bb4b 100644
--- a/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/preferences/tests/test_views.py
@@ -6,9 +6,13 @@ Unit tests for preference APIs.
import unittest
import ddt
import json
+from mock import patch
from django.core.urlresolvers import reverse
from django.conf import settings
+from django.test.testcases import TransactionTestCase
+from rest_framework.test import APIClient
+from student.tests.factories import UserFactory
from ...accounts.tests.test_views import UserAPITestCase
from ..api import set_user_preference
@@ -288,6 +292,52 @@ class TestPreferencesAPI(UserAPITestCase):
)
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+class TestPreferencesAPITransactions(TransactionTestCase):
+ """
+ Tests the transactional behavior of the preferences API
+ """
+ test_password = "test"
+
+ def setUp(self):
+ super(TestPreferencesAPITransactions, self).setUp()
+ self.client = APIClient()
+ self.user = UserFactory.create(password=self.test_password)
+ self.url = reverse("preferences_api", kwargs={'username': self.user.username})
+
+ @patch('openedx.core.djangoapps.user_api.models.UserPreference.delete')
+ def test_update_preferences_rollback(self, delete_user_preference):
+ """
+ Verify that updating preferences is transactional when a failure happens.
+ """
+ # Create some test preferences values.
+ set_user_preference(self.user, "a", "1")
+ set_user_preference(self.user, "b", "2")
+ set_user_preference(self.user, "c", "3")
+
+ # Send a PATCH request with two updates and a delete. The delete should fail
+ # after one of the updates has happened, in which case the whole operation
+ # should be rolled back.
+ delete_user_preference.side_effect = [Exception, None]
+ self.client.login(username=self.user.username, password=self.test_password)
+ json_data = {
+ "a": "2",
+ "b": None,
+ "c": "1",
+ }
+ response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json")
+ self.assertEqual(400, response.status_code)
+
+ # Verify that GET returns the original preferences
+ response = self.client.get(self.url)
+ expected_preferences = {
+ "a": "1",
+ "b": "2",
+ "c": "3",
+ }
+ self.assertEqual(expected_preferences, response.data)
+
+
@ddt.ddt
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
class TestPreferencesDetailAPI(UserAPITestCase):
diff --git a/openedx/core/djangoapps/user_api/preferences/views.py b/openedx/core/djangoapps/user_api/preferences/views.py
index b9f7544874333bc13e13914c6cd41c71bb61a3b9..e76f735a743a8b619d418c47016917e3921e6ab7 100644
--- a/openedx/core/djangoapps/user_api/preferences/views.py
+++ b/openedx/core/djangoapps/user_api/preferences/views.py
@@ -10,6 +10,7 @@ from rest_framework import status
from util.authentication import SessionAuthenticationAllowInactiveUser, OAuth2AuthenticationAllowInactiveUser
from rest_framework import permissions
+from django.db import transaction
from django.utils.translation import ugettext as _
from openedx.core.lib.api.parsers import MergePatchParser
@@ -88,7 +89,8 @@ class PreferencesView(APIView):
status=status.HTTP_400_BAD_REQUEST
)
try:
- update_user_preferences(request.user, request.DATA, username=username)
+ with transaction.commit_on_success():
+ update_user_preferences(request.user, request.DATA, username=username)
except (UserNotFound, UserNotAuthorized):
return Response(status=status.HTTP_404_NOT_FOUND)
except PreferenceValidationError as error:
diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py
index 91d189e7641640c635a5a69096c22786fd1a6b6e..bb6d22a7a36ef8a61a9e2f2c70c779b474d10f90 100644
--- a/openedx/core/djangoapps/user_api/tests/test_views.py
+++ b/openedx/core/djangoapps/user_api/tests/test_views.py
@@ -1686,7 +1686,6 @@ class TestFacebookRegistrationView(
self._verify_user_existence(user_exists=False, social_link_exists=False)
-
@skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled")
class TestGoogleRegistrationView(
ThirdPartyRegistrationTestMixin, ThirdPartyOAuthTestMixinGoogle, TransactionTestCase