Merge pull request #7257 from edx/christina/accounts-cleanup-tasks

Switch to api directory.

common/djangoapps/student/views.py

@@ -1909,7 +1909,8 @@ def reactivation_email_for_user(user):
     return JsonResponse({"success": True})
 
 
-# TODO: delete this method and redirect unit tests to do_email_change_request after accounts page work is done.
+# TODO: delete this method and redirect unit tests to validate_new_email and do_email_change_request
+# after accounts page work is done.
 @ensure_csrf_cookie
 def change_email_request(request):
     """ AJAX call from the profile page. User wants a new e-mail.
@@ -1928,6 +1929,7 @@ def change_email_request(request):
 
     new_email = request.POST['new_email']
     try:
+        validate_new_email(request.user, new_email)
         do_email_change_request(request.user, new_email)
     except ValueError as err:
         return JsonResponse({
@@ -1937,11 +1939,10 @@ def change_email_request(request):
     return JsonResponse({"success": True})
 
 
-def do_email_change_request(user, new_email, activation_key=uuid.uuid4().hex):
+def validate_new_email(user, new_email):
     """
-    Given a new email for a user, does some basic verification of the new address and sends an activation message
-    to the new address. If any issues are encountered with verification or sending the message, a ValueError will
-    be thrown.
+    Given a new email for a user, does some basic verification of the new address If any issues are encountered
+    with verification a ValueError will be thrown.
     """
     try:
         validate_email(new_email)
@@ -1954,6 +1955,13 @@ def do_email_change_request(user, new_email, activation_key=uuid.uuid4().hex):
     if User.objects.filter(email=new_email).count() != 0:
         raise ValueError(_('An account with this e-mail already exists.'))
 
+
+def do_email_change_request(user, new_email, activation_key=uuid.uuid4().hex):
+    """
+    Given a new email for a user, does some basic verification of the new address and sends an activation message
+    to the new address. If any issues are encountered with verification or sending the message, a ValueError will
+    be thrown.
+    """
     pec_list = PendingEmailChange.objects.filter(user=user)
     if len(pec_list) == 0:
         pec = PendingEmailChange()

lms/djangoapps/verify_student/tests/test_views.py

@@ -19,7 +19,7 @@ from django.core.exceptions import ObjectDoesNotExist
 from django.core import mail
 from bs4 import BeautifulSoup
 
-from openedx.core.djangoapps.user_api.accounts.views import AccountView
+from openedx.core.djangoapps.user_api.accounts.api import get_account_settings
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.django import modulestore
@@ -1149,7 +1149,7 @@ class TestSubmitPhotosForVerification(TestCase):
             AssertionError
 
         """
-        account_settings = AccountView.get_serialized_account(self.user)
+        account_settings = get_account_settings(self.user)
         self.assertEqual(account_settings['name'], full_name)
 
 

lms/djangoapps/verify_student/views.py

@@ -27,9 +27,9 @@ from django.utils.translation import ugettext as _, ugettext_lazy
 from django.contrib.auth.decorators import login_required
 from django.core.mail import send_mail
 
-from openedx.core.djangoapps.user_api.accounts.views import AccountView
+from openedx.core.djangoapps.user_api.accounts.api import get_account_settings, update_account_settings
 from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
-from openedx.core.djangoapps.user_api.api.account import AccountUserNotFound, AccountUpdateError
+from openedx.core.djangoapps.user_api.api.account import AccountUserNotFound, AccountValidationError
 
 from course_modes.models import CourseMode
 from student.models import CourseEnrollment
@@ -718,10 +718,10 @@ def submit_photos_for_verification(request):
     # then try to do that before creating the attempt.
     if request.POST.get('full_name'):
         try:
-            AccountView.update_account(request.user, {"name": request.POST.get('full_name')})
+            update_account_settings(request.user, {"name": request.POST.get('full_name')})
         except AccountUserNotFound:
             return HttpResponseBadRequest(_("No profile found for user"))
-        except AccountUpdateError:
+        except AccountValidationError:
             msg = _(
                 "Name must be at least {min_length} characters long."
             ).format(min_length=NAME_MIN_LENGTH)
@@ -741,7 +741,7 @@ def submit_photos_for_verification(request):
     attempt.mark_ready()
     attempt.submit()
 
-    account_settings = AccountView.get_serialized_account(request.user)
+    account_settings = get_account_settings(request.user)
 
     # Send a confirmation email to the user
     context = {

openedx/core/djangoapps/user_api/accounts/tests/test_api.py

+# -*- coding: utf-8 -*-
+"""
+Unit tests for behavior that is specific to the api methods (vs. the view methods).
+Most of the functionality is covered in test_views.py.
+"""
+
+from mock import Mock, patch
+from django.test import TestCase
+import unittest
+from student.tests.factories import UserFactory
+from django.conf import settings
+from student.models import PendingEmailChange
+from openedx.core.djangoapps.user_api.api.account import (
+    AccountUserNotFound, AccountUpdateError, AccountNotAuthorized, AccountValidationError
+)
+from ..api import get_account_settings, update_account_settings
+from ..serializers import AccountUserSerializer
+
+
+def mock_render_to_string(template_name, context):
+    """Return a string that encodes template_name and context"""
+    return str((template_name, sorted(context.iteritems())))
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
+class TestAccountApi(TestCase):
+    """
+    These tests specifically cover the parts of the API methods that are not covered by test_views.py.
+    This includes the specific types of error raised, and default behavior when optional arguments
+    are not specified.
+    """
+    password = "test"
+
+    def setUp(self):
+        super(TestAccountApi, self).setUp()
+        self.user = UserFactory.create(password=self.password)
+        self.different_user = UserFactory.create(password=self.password)
+        self.staff_user = UserFactory(is_staff=True, password=self.password)
+
+    def test_get_username_provided(self):
+        """Test the difference in behavior when a username is supplied to get_account_settings."""
+        account_settings = get_account_settings(self.user)
+        self.assertEqual(self.user.username, account_settings["username"])
+
+        account_settings = get_account_settings(self.user, username=self.user.username)
+        self.assertEqual(self.user.username, account_settings["username"])
+
+        account_settings = get_account_settings(self.user, username=self.different_user.username)
+        self.assertEqual(self.different_user.username, account_settings["username"])
+
+    def test_get_configuration_provided(self):
+        """Test the difference in behavior when a configuration is supplied to get_account_settings."""
+        config = {
+            "default_visibility": "private",
+
+            "shareable_fields": [
+                'name',
+            ],
+
+            "public_fields": [
+                'email',
+            ],
+        }
+
+        # With default configuration settings, email is not shared with other (non-staff) users.
+        account_settings = get_account_settings(self.user, self.different_user.username)
+        self.assertFalse("email" in account_settings)
+
+        account_settings = get_account_settings(self.user, self.different_user.username, configuration=config)
+        self.assertEqual(self.different_user.email, account_settings["email"])
+
+    def test_get_user_not_found(self):
+        """Test that AccountUserNotFound is thrown if there is no user with username."""
+        with self.assertRaises(AccountUserNotFound):
+            get_account_settings(self.user, username="does_not_exist")
+
+        self.user.username = "does_not_exist"
+        with self.assertRaises(AccountUserNotFound):
+            get_account_settings(self.user)
+
+    def test_update_username_provided(self):
+        """Test the difference in behavior when a username is supplied to update_account_settings."""
+        update_account_settings(self.user, {"name": "Mickey Mouse"})
+        account_settings = get_account_settings(self.user)
+        self.assertEqual("Mickey Mouse", account_settings["name"])
+
+        update_account_settings(self.user, {"name": "Donald Duck"}, username=self.user.username)
+        account_settings = get_account_settings(self.user)
+        self.assertEqual("Donald Duck", account_settings["name"])
+
+        with self.assertRaises(AccountNotAuthorized):
+            update_account_settings(self.different_user, {"name": "Pluto"}, username=self.user.username)
+
+    def test_update_user_not_found(self):
+        """Test that AccountUserNotFound is thrown if there is no user with username."""
+        with self.assertRaises(AccountUserNotFound):
+            update_account_settings(self.user, {}, username="does_not_exist")
+
+        self.user.username = "does_not_exist"
+        with self.assertRaises(AccountUserNotFound):
+            update_account_settings(self.user, {})
+
+    def test_update_error_validating(self):
+        """Test that AccountValidationError is thrown if incorrect values are supplied."""
+        with self.assertRaises(AccountValidationError):
+            update_account_settings(self.user, {"username": "not_allowed"})
+
+        with self.assertRaises(AccountValidationError):
+            update_account_settings(self.user, {"gender": "undecided"})
+
+    def test_update_multiple_validation_errors(self):
+        """Test that all validation errors are built up and returned at once"""
+        # Send a read-only error, serializer error, and email validation error.
+        naughty_update = {
+            "username": "not_allowed",
+            "gender": "undecided",
+            "email": "not an email address"
+        }
+
+        error_thrown = False
+        try:
+            update_account_settings(self.user, naughty_update)
+        except AccountValidationError as response:
+            error_thrown = True
+            field_errors = response.field_errors
+            self.assertEqual(3, len(field_errors))
+            self.assertEqual("This field is not editable via this API", field_errors["username"]["developer_message"])
+            self.assertIn("Select a valid choice", field_errors["gender"]["developer_message"])
+            self.assertIn("Valid e-mail address required.", field_errors["email"]["developer_message"])
+
+        self.assertTrue(error_thrown, "No AccountValidationError was thrown")
+
+    @patch('django.core.mail.send_mail')
+    @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True))
+    def test_update_sending_email_fails(self, send_mail):
+        """Test what happens if all validation checks pass, but sending the email for email change fails."""
+        send_mail.side_effect = [Exception, None]
+        less_naughty_update = {
+            "name": "Mickey Mouse",
+            "email": "seems_ok@sample.com"
+        }
+        error_thrown = False
+        try:
+            update_account_settings(self.user, less_naughty_update)
+        except AccountUpdateError as response:
+            error_thrown = True
+            self.assertIn("Error thrown from do_email_change_request", response.developer_message)
+
+        self.assertTrue(error_thrown, "No AccountUpdateError was thrown")
+
+        # Verify that the name change happened, even though the attempt to send the email failed.
+        account_settings = get_account_settings(self.user)
+        self.assertEqual("Mickey Mouse", account_settings["name"])
+
+    @patch('openedx.core.djangoapps.user_api.accounts.serializers.AccountUserSerializer.save')
+    def test_serializer_save_fails(self, serializer_save):
+        """
+        Test the behavior of one of the serializers failing to save. Note that email request change
+        won't be processed in this case.
+        """
+        serializer_save.side_effect = [Exception, None]
+        update_will_fail = {
+            "name": "Mickey Mouse",
+            "email": "ok@sample.com"
+        }
+        error_thrown = False
+        try:
+            update_account_settings(self.user, update_will_fail)
+        except AccountUpdateError as response:
+            error_thrown = True
+            self.assertIn("Error thrown when saving account updates", response.developer_message)
+
+        self.assertTrue(error_thrown, "No AccountUpdateError was thrown")
+
+        # Verify that no email change request was initiated.
+        pending_change = PendingEmailChange.objects.filter(user=self.user)
+        self.assertEqual(0, len(pending_change))

openedx/core/djangoapps/user_api/accounts/tests/test_views.py

@@ -440,8 +440,23 @@ class TestAccountAPI(UserAPITestCase):
 
         # Finally, try changing to an invalid email just to make sure error messages are appropriately returned.
         error_response = self.send_patch(client, {"email": "not_an_email"}, expected_status=400)
+        field_errors = error_response.data["field_errors"]
         self.assertEqual(
-            "Error thrown from do_email_change_request: 'Valid e-mail address required.'",
+            "Error thrown from validate_new_email: 'Valid e-mail address required.'",
+            field_errors["email"]["developer_message"]
+        )
+        self.assertEqual("Valid e-mail address required.", field_errors["email"]["user_message"])
+
+    @patch('openedx.core.djangoapps.user_api.accounts.serializers.AccountUserSerializer.save')
+    def test_patch_serializer_save_fails(self, serializer_save):
+        """
+        Test that AccountUpdateErrors are passed through to the response.
+        """
+        serializer_save.side_effect = [Exception("bummer"), None]
+        self.client.login(username=self.user.username, password=TEST_PASSWORD)
+        error_response = self.send_patch(self.client, {"goals": "save an account field"}, expected_status=400)
+        self.assertEqual(
+            "Error thrown when saving account updates: 'bummer'",
             error_response.data["developer_message"]
         )
-        self.assertEqual("Valid e-mail address required.", error_response.data["user_message"])
+        self.assertIsNone(error_response.data["user_message"])

openedx/core/djangoapps/user_api/api/account.py

@@ -67,11 +67,23 @@ class AccountNotAuthorized(AccountRequestError):
 
 class AccountUpdateError(AccountRequestError):
     """
-    An update to the account failed. More detailed information is present in error_info (a dict
-    with at least a developer_message, though possibly also a nested field_errors dict).
+    An update to the account failed. More detailed information is present in developer_message,
+    and depending on the type of error encountered, there may also be a non-null user_message field.
     """
-    def __init__(self, error_info):
-        self.error_info = error_info
+    def __init__(self, developer_message, user_message=None):
+        self.developer_message = developer_message
+        self.user_message = user_message
+
+
+class AccountValidationError(AccountRequestError):
+    """
+    Validation issues were found with the supplied data. More detailed information is present in field_errors,
+    a dict with specific information about each field that failed validation. For each field,
+    there will be at least a developer_message describing the validation issue, and possibly
+    also a user_message.
+    """
+    def __init__(self, field_errors):
+        self.field_errors = field_errors
 
 
 @intercept_errors(AccountInternalError, ignore_errors=[AccountRequestError])

openedx/core/djangoapps/user_api/api/profile.py

@@ -15,7 +15,7 @@ import analytics
 
 from eventtracking import tracker
 from ..accounts import NAME_MIN_LENGTH
-from ..accounts.views import AccountView
+from ..accounts.api import get_account_settings
 from ..models import User, UserPreference, UserOrgTag
 from ..helpers import intercept_errors
 
@@ -106,7 +106,7 @@ def update_email_opt_in(user, org, optin):
         None
 
     """
-    account_settings = AccountView.get_serialized_account(user)
+    account_settings = get_account_settings(user)
     year_of_birth = account_settings['year_of_birth']
     of_age = (
         year_of_birth is None or  # If year of birth is not set, we assume user is of age.

openedx/core/djangoapps/user_api/tests/test_profile_api.py

@@ -11,7 +11,7 @@ from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 import datetime
 
-from ..accounts.views import AccountView
+from ..accounts.api import get_account_settings
 from ..api import account as account_api
 from ..api import profile as profile_api
 from ..models import UserProfile, UserOrgTag
@@ -30,7 +30,7 @@ class ProfileApiTest(ModuleStoreTestCase):
 
         # Retrieve the account settings
         user = User.objects.get(username=self.USERNAME)
-        account_settings = AccountView.get_serialized_account(user)
+        account_settings = get_account_settings(user)
 
         # Expect a date joined field but remove it to simplify the following comparison
         self.assertIsNotNone(account_settings['date_joined'])

openedx/core/djangoapps/user_api/tests/test_views.py

@@ -24,7 +24,7 @@ from django_comment_common import models
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from third_party_auth.tests.testutil import simulate_running_pipeline
 
-from ..accounts.views import AccountView
+from ..accounts.api import get_account_settings
 from ..api import account as account_api, profile as profile_api
 from ..models import UserOrgTag
 from ..tests.factories import UserPreferenceFactory
@@ -1249,7 +1249,7 @@ class RegistrationViewTest(ApiTestCase):
 
         # Verify that the user's full name is set
         user = User.objects.get(username=self.USERNAME)
-        account_settings = AccountView.get_serialized_account(user)
+        account_settings = get_account_settings(user)
         self.assertEqual(account_settings["name"], self.NAME)
 
         # Verify that we've been logged in
@@ -1283,7 +1283,7 @@ class RegistrationViewTest(ApiTestCase):
 
         # Verify the user's account
         user = User.objects.get(username=self.USERNAME)
-        account_settings = AccountView.get_serialized_account(user)
+        account_settings = get_account_settings(user)
         self.assertEqual(account_settings["level_of_education"], self.EDUCATION)
         self.assertEqual(account_settings["mailing_address"], self.ADDRESS)
         self.assertEqual(account_settings["year_of_birth"], int(self.YEAR_OF_BIRTH))