From e30ea5c08bf2a5e053ff260e06bee839a178f92e Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 6 Mar 2015 14:36:19 -0500 Subject: [PATCH] Switch to api directory and improve error handling. --- common/djangoapps/student/views.py | 18 +- .../verify_student/tests/test_views.py | 4 +- lms/djangoapps/verify_student/views.py | 10 +- .../core/djangoapps/user_api/accounts/api.py | 231 ++++++++++++++ .../user_api/accounts/tests/test_api.py | 177 +++++++++++ .../user_api/accounts/tests/test_views.py | 19 +- .../djangoapps/user_api/accounts/views.py | 298 ++++-------------- .../core/djangoapps/user_api/api/account.py | 20 +- .../core/djangoapps/user_api/api/profile.py | 4 +- .../user_api/tests/test_profile_api.py | 4 +- .../djangoapps/user_api/tests/test_views.py | 6 +- 11 files changed, 532 insertions(+), 259 deletions(-) create mode 100644 openedx/core/djangoapps/user_api/accounts/api.py create mode 100644 openedx/core/djangoapps/user_api/accounts/tests/test_api.py diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index f4dd626e02..ba3ea82ae6 100644 --- a/common/djangoapps/student/views.py +++ b/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() diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 077e35e275..9fb41f4f97 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/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) diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 18d56e3e82..0f37cad32b 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/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 = { diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py new file mode 100644 index 0000000000..206af58fdf --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -0,0 +1,231 @@ +from django.contrib.auth.models import User +from django.utils.translation import ugettext as _ +import datetime +from pytz import UTC +from django.core.exceptions import ObjectDoesNotExist +from django.conf import settings + +from openedx.core.djangoapps.user_api.api.account import ( + AccountUserNotFound, AccountUpdateError, AccountNotAuthorized, AccountValidationError +) +from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer +from student.models import UserProfile +from student.views import validate_new_email, do_email_change_request +from ..models import UserPreference +from . import ACCOUNT_VISIBILITY_PREF_KEY, ALL_USERS_VISIBILITY + + +def get_account_settings(requesting_user, username=None, configuration=None, view=None): + """Returns account information for a user serialized as JSON. + + Note: + If `requesting_user.username` != `username`, this method will return differing amounts of information + based on who `requesting_user` is and the privacy settings of the user associated with `username`. + + Args: + requesting_user (User): The user requesting the account information. Only the user with username + `username` or users with "is_staff" privileges can get full account information. + Other users will get the account fields that the user has elected to share. + username (str): Optional username for the desired account information. If not specified, + `requesting_user.username` is assumed. + configuration (dict): an optional configuration specifying which fields in the account + can be shared, and the default visibility settings. If not present, the setting value with + key ACCOUNT_VISIBILITY_CONFIGURATION is used. + view (str): An optional string allowing "is_staff" users and users requesting their own + account information to get just the fields that are shared with everyone. If view is + "shared", only shared account information will be returned, regardless of `requesting_user`. + + Returns: + A dict containing account fields. + + Raises: + AccountUserNotFound: no user with username `username` exists (or `requesting_user.username` if + `username` is not specified) + """ + if username is None: + username = requesting_user.username + + has_full_access = requesting_user.username == username or requesting_user.is_staff + return_all_fields = has_full_access and view != 'shared' + + existing_user, existing_user_profile = _get_user_and_profile(username) + + user_serializer = AccountUserSerializer(existing_user) + legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile) + + account_settings = dict(user_serializer.data, **legacy_profile_serializer.data) + + if return_all_fields: + return account_settings + + if not configuration: + configuration = settings.ACCOUNT_VISIBILITY_CONFIGURATION + + visible_settings = {} + + profile_privacy = UserPreference.get_preference(existing_user, ACCOUNT_VISIBILITY_PREF_KEY) + privacy_setting = profile_privacy if profile_privacy else configuration.get('default_visibility') + + if privacy_setting == ALL_USERS_VISIBILITY: + field_names = configuration.get('shareable_fields') + else: + field_names = configuration.get('public_fields') + + for field_name in field_names: + visible_settings[field_name] = account_settings.get(field_name, None) + + return visible_settings + + +def update_account_settings(requesting_user, update, username=None): + """Update user account information. + + Note: + It is up to the caller of this method to enforce the contract that this method is only called + with the user who made the request. + + Arguments: + requesting_user (User): The user requesting to modify account information. Only the user with username + 'username' has permissions to modify account information. + update (dict): The updated account field values. + username (string): Optional username specifying which account should be updated. If not specified, + `requesting_user.username` is assumed. + + Raises: + AccountUserNotFound: no user with username `username` exists (or `requesting_user.username` if + `username` is not specified) + AccountNotAuthorized: the requesting_user does not have access to change the account + associated with `username` + AccountValidationError: the update was not attempted because validation errors were found with + the supplied update + AccountUpdateError: the update could not be completed. Note that if multiple fields are updated at the same + time, some parts of the update may have been successful, even if an AccountUpdateError is returned; + in particular, the user account (not including e-mail address) may have successfully been updated, + but then the e-mail change request, which is processed last, may throw an error. + + """ + if username is None: + username = requesting_user.username + + existing_user, existing_user_profile = _get_user_and_profile(username) + + if requesting_user.username != username: + raise AccountNotAuthorized() + + # If user has requested to change email, we must call the multi-step process to handle this. + # It is not handled by the serializer (which considers email to be read-only). + new_email = None + if "email" in update: + new_email = update["email"] + del update["email"] + + # If user has requested to change name, store old name because we must update associated metadata + # after the save process is complete. + old_name = None + if "name" in update: + old_name = existing_user_profile.name + + # Check for fields that are not editable. Marking them read-only causes them to be ignored, but we wish to 400. + read_only_fields = set(update.keys()).intersection( + AccountUserSerializer.Meta.read_only_fields + AccountLegacyProfileSerializer.Meta.read_only_fields + ) + + # Build up all field errors, whether read-only, validation, or email errors. + field_errors = {} + + 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", + "user_message": _("Field '{field_name}' cannot be edited.".format(field_name=read_only_field)) + } + del update[read_only_field] + + user_serializer = AccountUserSerializer(existing_user, data=update) + legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=update) + + for serializer in user_serializer, legacy_profile_serializer: + field_errors = _add_serializer_errors(update, serializer, field_errors) + + # If the user asked to change email, validate it. + if new_email: + try: + validate_new_email(existing_user, new_email) + except ValueError as err: + field_errors["email"] = { + "developer_message": "Error thrown from validate_new_email: '{}'".format(err.message), + "user_message": err.message + } + + # If we have encountered any validation errors, return them to the user. + if field_errors: + raise AccountValidationError(field_errors) + + try: + # If everything validated, go ahead and save the serializers. + for serializer in user_serializer, legacy_profile_serializer: + serializer.save() + + # If the name was changed, store information about the change operation. This is outside of the + # serializer so that we can store who requested the change. + if old_name: + meta = existing_user_profile.get_meta() + if 'old_names' not in meta: + meta['old_names'] = [] + meta['old_names'].append([ + old_name, + "Name change requested through account API by {0}".format(requesting_user.username), + datetime.datetime.now(UTC).isoformat() + ]) + existing_user_profile.set_meta(meta) + existing_user_profile.save() + + except Exception as err: + raise AccountUpdateError( + "Error thrown when saving account updates: '{}'".format(err.message) + ) + + # And try to send the email change request if necessary. + if new_email: + try: + do_email_change_request(existing_user, new_email) + except ValueError as err: + raise AccountUpdateError( + "Error thrown from do_email_change_request: '{}'".format(err.message), + user_message=err.message + ) + + +def _get_user_and_profile(username): + """ + Helper method to return the legacy user and profile objects based on username. + """ + try: + existing_user = User.objects.get(username=username) + existing_user_profile = UserProfile.objects.get(user=existing_user) + except ObjectDoesNotExist: + raise AccountUserNotFound() + + return existing_user, existing_user_profile + + +def _add_serializer_errors(update, serializer, field_errors): + """ + Helper method that adds any validation errors that are present in the serializer to + the supplied field_errors dict. + """ + if not serializer.is_valid(): + errors = serializer.errors + for key, value in errors.iteritems(): + if isinstance(value, list) and len(value) > 0: + developer_message = value[0] + else: + developer_message = "Invalid value: {field_value}'".format(field_value=update[key]) + field_errors[key] = { + "developer_message": developer_message, + "user_message": _("Value '{field_value}' is not valid for field '{field_name}'.".format( + field_value=update[key], field_name=key) + ) + } + + return field_errors diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py new file mode 100644 index 0000000000..fff9493cdf --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -0,0 +1,177 @@ +# -*- 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)) 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 1db5f894fc..918eb6529e 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/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"]) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index b3b6ec8fed..0b150ceac7 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -4,28 +4,17 @@ 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.core.exceptions import ObjectDoesNotExist -from django.contrib.auth.models import User -from django.utils.translation import ugettext as _ -from django.conf import settings -import datetime -from pytz import UTC - from rest_framework.views import APIView from rest_framework.response import Response from rest_framework import status from rest_framework.authentication import OAuth2Authentication, SessionAuthentication from rest_framework import permissions -from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer, AccountUserSerializer -from openedx.core.djangoapps.user_api.api.account import AccountUserNotFound, AccountUpdateError, AccountNotAuthorized +from openedx.core.djangoapps.user_api.api.account import ( + AccountUserNotFound, AccountUpdateError, AccountNotAuthorized, AccountValidationError +) from openedx.core.lib.api.parsers import MergePatchParser -from openedx.core.lib.api.permissions import IsUserInUrlOrStaff -from student.models import UserProfile -from student.views import do_email_change_request -from ..models import UserPreference - -from . import ACCOUNT_VISIBILITY_PREF_KEY, ALL_USERS_VISIBILITY +from .api import get_account_settings, update_account_settings class AccountView(APIView): @@ -36,51 +25,75 @@ class AccountView(APIView): **Example Requests**: - GET /api/user/v0/accounts/{username}/ + GET /api/user/v0/accounts/{username}/[?view=shared] PATCH /api/user/v0/accounts/{username}/ with content_type "application/merge-patch+json" **Response Values for GET** - * username: username associated with the account (not editable) + If the user making the request has username "username", or has "is_staff" access, the following + fields will be returned: - * name: full name of the user (must be at least two characters) + * username: username associated with the account (not editable) - * email: email for the user (the new email address must be confirmed via a confirmation email, so GET will - not reflect the change until the address has been confirmed) + * name: full name of the user (must be at least two characters) - * date_joined: date this account was created (not editable), in the string format provided by - datetime (for example, "2014-08-26T17:52:11Z") + * email: email for the user (the new email address must be confirmed via a confirmation email, so GET will + not reflect the change until the address has been confirmed) - * gender: null (not set), "m", "f", or "o" + * date_joined: date this account was created (not editable), in the string format provided by + datetime (for example, "2014-08-26T17:52:11Z") - * year_of_birth: null or integer year + * gender: null (not set), "m", "f", or "o" - * level_of_education: null (not set), or one of the following choices: + * year_of_birth: null or integer year - * "p" signifying "Doctorate" - * "m" signifying "Master's or professional degree" - * "b" signifying "Bachelor's degree" - * "a" signifying "Associate's degree" - * "hs" signifying "Secondary/high school" - * "jhs" signifying "Junior secondary/junior high/middle school" - * "el" signifying "Elementary/primary school" - * "none" signifying "None" - * "o" signifying "Other" + * level_of_education: null (not set), or one of the following choices: - * language: null or name of preferred language + * "p" signifying "Doctorate" + * "m" signifying "Master's or professional degree" + * "b" signifying "Bachelor's degree" + * "a" signifying "Associate's degree" + * "hs" signifying "Secondary/high school" + * "jhs" signifying "Junior secondary/junior high/middle school" + * "el" signifying "Elementary/primary school" + * "none" signifying "None" + * "o" signifying "Other" - * country: null (not set), or a Country corresponding to one of the ISO 3166-1 countries + * language: null or name of preferred language - * mailing_address: null or textual representation of mailing address + * country: null (not set), or a Country corresponding to one of the ISO 3166-1 countries - * goals: null or textual representation of goals + * mailing_address: null or textual representation of mailing address + + * goals: null or textual representation of goals + + If a user without "is_staff" access has requested account information for a different user, + only a subset of these fields will be returned. The actual fields returned depend on the configuration + setting ACCOUNT_VISIBILITY_CONFIGURATION, and the visibility preference of the user with username + "username". + + Note that a user can view which account fields they have shared with other users by requesting their + own username and providing the url parameter "view=shared". + + This method will return a 404 if no user exists with username "username". **Response for PATCH** - Returns a 204 status if successful, with no additional content. - If "application/merge-patch+json" is not the specified content_type, returns a 415 status. + Users can only modify their own account information. If the requesting user does not have username + "username", this method will return with a status of 404. + This method will also return a 404 if no user exists with username "username". + + If "application/merge-patch+json" is not the specified content_type, this method returns a 415 status. + + If the update could not be completed due to validation errors, this method returns a 400 with all + field-specific error messages in the "field_errors" field of the returned JSON. + + If the update could not be completed due to failure at the time of update, this method returns a 400 with + specific errors in the returned JSON. + + If the updated is successful, a 204 status is returned with no additional content. """ authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated,) @@ -91,76 +104,12 @@ class AccountView(APIView): GET /api/user/v0/accounts/{username}/ """ try: - account_settings = AccountView.get_serialized_account( - request.user, username, - view=request.QUERY_PARAMS.get('view') - ) + account_settings = get_account_settings(request.user, username, view=request.QUERY_PARAMS.get('view')) except AccountUserNotFound: return Response(status=status.HTTP_404_NOT_FOUND) return Response(account_settings) - @staticmethod - def get_serialized_account(requesting_user, username=None, configuration=None, view=None): - """Returns account information for a user serialized as JSON. - - Note: - If `requesting_user.username` != `username`, this method will return differing amounts of information - based on who `requesting_user` is and the privacy settings of the user associated with `username`. - - Args: - requesting_user (User): The user requesting the account information. Only the user with username - `username` or users with "is_staff" privileges can get full account information. - Other users will get the account fields that the user has elected to share. - username (str): Optional username for the desired account information. If not specified, - `requesting_user.username` is assumed. - configuration (dict): an optional configuration specifying which fields in the account - can be shared, and the default visibility settings. If not present, the setting value with - key ACCOUNT_VISIBILITY_CONFIGURATION is used. - view (str): An optional string allowing "is_staff" users and users requesting their own - account information to get just the fields that are shared with everyone. If view is - "shared", only shared account information will be returned, regardless of `requesting_user`. - - Returns: - A dict containing account fields. - - Raises: - AccountUserNotFound: `username` was specified, but no user exists with that username. - """ - if username is None: - username = requesting_user.username - - has_full_access = requesting_user.username == username or requesting_user.is_staff - return_all_fields = has_full_access and view != 'shared' - - existing_user, existing_user_profile = AccountView._get_user_and_profile(username) - - user_serializer = AccountUserSerializer(existing_user) - legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile) - - account_settings = dict(user_serializer.data, **legacy_profile_serializer.data) - - if return_all_fields: - return account_settings - - if not configuration: - configuration = settings.ACCOUNT_VISIBILITY_CONFIGURATION - - visible_settings = {} - - profile_privacy = UserPreference.get_preference(existing_user, ACCOUNT_VISIBILITY_PREF_KEY) - privacy_setting = profile_privacy if profile_privacy else configuration.get('default_visibility') - - if privacy_setting == ALL_USERS_VISIBILITY: - field_names = configuration.get('shareable_fields') - else: - field_names = configuration.get('public_fields') - - for field_name in field_names: - visible_settings[field_name] = account_settings.get(field_name, None) - - return visible_settings - def patch(self, request, username): """ PATCH /api/user/v0/accounts/{username}/ @@ -170,137 +119,18 @@ class AccountView(APIView): else an error response with status code 415 will be returned. """ try: - AccountView.update_account(request.user, request.DATA, username=username) + update_account_settings(request.user, request.DATA, username=username) except (AccountUserNotFound, AccountNotAuthorized): return Response(status=status.HTTP_404_NOT_FOUND) + except AccountValidationError as err: + return Response({"field_errors": err.field_errors}, status=status.HTTP_400_BAD_REQUEST) except AccountUpdateError as err: - return Response(err.error_info, status=status.HTTP_400_BAD_REQUEST) + return Response( + { + "developer_message": err.developer_message, + "user_message": err.user_message + }, + status=status.HTTP_400_BAD_REQUEST + ) return Response(status=status.HTTP_204_NO_CONTENT) - - @staticmethod - def update_account(requesting_user, update, username=None): - """Update user account information. - - Note: - It is up to the caller of this method to enforce the contract that this method is only called - with the user who made the request. - - Arguments: - requesting_user (User): The user requesting to modify account information. Only the user with username - 'username' has permissions to modify account information. - update (dict): The updated account field values. - username (string): Optional username specifying which account should be updated. If not specified, - `requesting_user.username` is assumed. - - Raises: - AccountUserNotFound: `username` was specified, but no user exists with that username. - AccountUpdateError: the update could not be completed, usually due to validation errors - (for example, read-only fields were specified or field values are not legal) - AccountNotAuthorized: the requesting_user does not have access to change the account - associated with `username`. - """ - if username is None: - username = requesting_user.username - - if requesting_user.username != username: - raise AccountNotAuthorized() - - existing_user, existing_user_profile = AccountView._get_user_and_profile(username) - - # If user has requested to change email, we must call the multi-step process to handle this. - # It is not handled by the serializer (which considers email to be read-only). - new_email = None - if "email" in update: - new_email = update["email"] - del update["email"] - - # If user has requested to change name, store old name because we must update associated metadata - # after the save process is complete. - old_name = None - if "name" in update: - old_name = existing_user_profile.name - - # Check for fields that are not editable. Marking them read-only causes them to be ignored, but we wish to 400. - read_only_fields = set(update.keys()).intersection( - AccountUserSerializer.Meta.read_only_fields + AccountLegacyProfileSerializer.Meta.read_only_fields - ) - if read_only_fields: - field_errors = {} - for read_only_field in read_only_fields: - field_errors[read_only_field] = { - "developer_message": "This field is not editable via this API", - "user_message": _("Field '{field_name}' cannot be edited.".format(field_name=read_only_field)) - } - raise AccountUpdateError({"field_errors": field_errors}) - - # If the user asked to change email, send the request now. - if new_email: - try: - do_email_change_request(existing_user, new_email) - except ValueError as err: - response_data = { - "developer_message": "Error thrown from do_email_change_request: '{}'".format(err.message), - "user_message": err.message - } - raise AccountUpdateError(response_data) - - user_serializer = AccountUserSerializer(existing_user, data=update) - legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=update) - - for serializer in user_serializer, legacy_profile_serializer: - validation_errors = AccountView._get_validation_errors(update, serializer) - if validation_errors: - raise AccountUpdateError(validation_errors) - serializer.save() - - # If the name was changed, store information about the change operation. This is outside of the - # serializer so that we can store who requested the change. - if old_name: - meta = existing_user_profile.get_meta() - if 'old_names' not in meta: - meta['old_names'] = [] - meta['old_names'].append([ - old_name, - "Name change requested through account API by {0}".format(requesting_user.username), - datetime.datetime.now(UTC).isoformat() - ]) - existing_user_profile.set_meta(meta) - existing_user_profile.save() - - @staticmethod - def _get_user_and_profile(username): - """ - Helper method to return the legacy user and profile objects based on username. - """ - try: - existing_user = User.objects.get(username=username) - existing_user_profile = UserProfile.objects.get(user=existing_user) - except ObjectDoesNotExist: - raise AccountUserNotFound() - - return existing_user, existing_user_profile - - @staticmethod - def _get_validation_errors(update, serializer): - """ - Helper method that returns any validation errors that are present. - """ - validation_errors = {} - if not serializer.is_valid(): - field_errors = {} - errors = serializer.errors - for key, value in errors.iteritems(): - if isinstance(value, list) and len(value) > 0: - developer_message = value[0] - else: - developer_message = "Invalid value: {field_value}'".format(field_value=update[key]) - field_errors[key] = { - "developer_message": developer_message, - "user_message": _("Value '{field_value}' is not valid for field '{field_name}'.".format( - field_value=update[key], field_name=key) - ) - } - - validation_errors['field_errors'] = field_errors - return validation_errors diff --git a/openedx/core/djangoapps/user_api/api/account.py b/openedx/core/djangoapps/user_api/api/account.py index bdef6cc839..2e797c8160 100644 --- a/openedx/core/djangoapps/user_api/api/account.py +++ b/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]) diff --git a/openedx/core/djangoapps/user_api/api/profile.py b/openedx/core/djangoapps/user_api/api/profile.py index 04b8b2184d..9e19914243 100644 --- a/openedx/core/djangoapps/user_api/api/profile.py +++ b/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. diff --git a/openedx/core/djangoapps/user_api/tests/test_profile_api.py b/openedx/core/djangoapps/user_api/tests/test_profile_api.py index 926a4ae403..530fadd90c 100644 --- a/openedx/core/djangoapps/user_api/tests/test_profile_api.py +++ b/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']) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 62bad76989..e4f2bbe69d 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/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))