Clean up transactional behavior in User API

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

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),

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"])

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:

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(

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):

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:

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