diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index ecfab72331..6bd38ed934 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/common/djangoapps/student/tests/test_enrollment.py @@ -130,7 +130,7 @@ class EnrollmentTest(UrlResetMixin, ModuleStoreTestCase): # Verify that the profile API has been called as expected if email_opt_in is not None: opt_in = email_opt_in == 'true' - mock_update_email_opt_in.assert_called_once_with(self.USERNAME, self.course.org, opt_in) + mock_update_email_opt_in.assert_called_once_with(self.user, self.course.org, opt_in) else: self.assertFalse(mock_update_email_opt_in.called) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 88778e0806..ba3ea82ae6 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -797,7 +797,7 @@ def try_change_enrollment(request): log.exception(u"Exception automatically enrolling after login: %s", exc) -def _update_email_opt_in(request, username, org): +def _update_email_opt_in(request, org): """Helper function used to hit the profile API if email opt-in is enabled.""" # TODO: remove circular dependency on openedx from common @@ -806,7 +806,7 @@ def _update_email_opt_in(request, username, org): email_opt_in = request.POST.get('email_opt_in') if email_opt_in is not None: email_opt_in_boolean = email_opt_in == 'true' - profile_api.update_email_opt_in(username, org, email_opt_in_boolean) + profile_api.update_email_opt_in(request.user, org, email_opt_in_boolean) @require_POST @@ -878,7 +878,7 @@ def change_enrollment(request, check_access=True): # Record the user's email opt-in preference if settings.FEATURES.get('ENABLE_MKTG_EMAIL_OPT_IN'): - _update_email_opt_in(request, user.username, course_id.org) + _update_email_opt_in(request, course_id.org) available_modes = CourseMode.modes_for_course_dict(course_id) @@ -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/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 28081c1b8b..97c905fac0 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -672,7 +672,7 @@ def change_enrollment(strategy, user=None, is_dashboard=False, *args, **kwargs): # TODO: remove circular dependency on openedx from common from openedx.core.djangoapps.user_api.api import profile opt_in = email_opt_in.lower() == 'true' - profile.update_email_opt_in(user.username, course_id.org, opt_in) + profile.update_email_opt_in(user, course_id.org, opt_in) # Check whether we're blocked from enrolling by a # country access rule. diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index bf965263fc..fb94fefba7 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 @@ -1167,7 +1167,7 @@ class TestSubmitPhotosForVerification(TestCase): AssertionError """ - account_settings = AccountView.get_serialized_account(self.user.username) + 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 a245b30658..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 @@ -714,16 +714,14 @@ def submit_photos_for_verification(request): if SoftwareSecurePhotoVerification.user_has_valid_or_pending(request.user): return HttpResponseBadRequest(_("You already have a valid or pending verification.")) - username = request.user.username - # If the user wants to change his/her full name, # then try to do that before creating the attempt. if request.POST.get('full_name'): try: - AccountView.update_account(request.user, username, {"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) @@ -743,7 +741,7 @@ def submit_photos_for_verification(request): attempt.mark_ready() attempt.submit() - account_settings = AccountView.get_serialized_account(username) + account_settings = get_account_settings(request.user) # Send a confirmation email to the user context = { diff --git a/lms/envs/common.py b/lms/envs/common.py index 7747cf67ce..8e45c63293 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2053,14 +2053,14 @@ SEARCH_ENGINE = None # Use the LMS specific result processor SEARCH_RESULT_PROCESSOR = "lms.lib.courseware_search.lms_result_processor.LmsSearchResultProcessor" -# The configuration for learner profiles -PROFILE_CONFIGURATION = { +# The configuration visibility of account fields. +ACCOUNT_VISIBILITY_CONFIGURATION = { # Default visibility level for accounts without a specified value # The value is one of: 'all_users', 'private' "default_visibility": "private", - # The list of all fields that can be shown on a learner's profile - "all_fields": [ + # The list of all fields that can be shared with other users + "shareable_fields": [ 'username', 'profile_image', 'country', @@ -2069,7 +2069,7 @@ PROFILE_CONFIGURATION = { 'bio', ], - # The list of fields that are always public on a learner's profile + # The list of account fields that are always public "public_fields": [ 'username', 'profile_image', diff --git a/openedx/core/djangoapps/user_api/accounts/__init__.py b/openedx/core/djangoapps/user_api/accounts/__init__.py index 1a655b2835..4db70bee44 100644 --- a/openedx/core/djangoapps/user_api/accounts/__init__.py +++ b/openedx/core/djangoapps/user_api/accounts/__init__.py @@ -1,2 +1,14 @@ +""" +Account constants +""" + # The minimum acceptable length for the name account field NAME_MIN_LENGTH = 2 + +ACCOUNT_VISIBILITY_PREF_KEY = 'account_privacy' + +# Indicates the user's preference that all users can view the shareable fields in their account information. +ALL_USERS_VISIBILITY = 'all_users' + +# Indicates the user's preference that all their account information be private. +PRIVATE_VISIBILITY = 'private' 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 cec245f01d..918eb6529e 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -2,6 +2,7 @@ import unittest import ddt import json +from mock import patch from django.conf import settings from django.core.urlresolvers import reverse @@ -9,6 +10,9 @@ from rest_framework.test import APITestCase, APIClient from student.tests.factories import UserFactory from student.models import UserProfile, PendingEmailChange +from openedx.core.djangoapps.user_api.accounts import ACCOUNT_VISIBILITY_PREF_KEY +from openedx.core.djangoapps.user_api.models import UserPreference +from .. import PRIVATE_VISIBILITY, ALL_USERS_VISIBILITY TEST_PASSWORD = "test" @@ -73,24 +77,66 @@ class TestAccountAPI(UserAPITestCase): """ Unit tests for the Account API. """ - def setUp(self): super(TestAccountAPI, self).setUp() self.url = reverse("accounts_api", kwargs={'username': self.user.username}) - def test_get_account_anonymous_user(self): + def _verify_full_shareable_account_response(self, response): """ - Test that an anonymous client (not logged in) cannot call get. + Verify that the shareable fields from the account are returned + """ + data = response.data + self.assertEqual(6, len(data)) + self.assertEqual(self.user.username, data["username"]) + self.assertEqual("US", data["country"]) + self.assertIsNone(data["profile_image"]) + self.assertIsNone(data["time_zone"]) + self.assertIsNone(data["languages"]) + self.assertIsNone(data["bio"]) + + def _verify_private_account_response(self, response): + """ + Verify that only the public fields are returned if a user does not want to share account fields + """ + data = response.data + self.assertEqual(2, len(data)) + self.assertEqual(self.user.username, data["username"]) + self.assertIsNone(data["profile_image"]) + + def _verify_full_account_response(self, response): + """ + Verify that all account fields are returned (even those that are not shareable). + """ + data = response.data + self.assertEqual(11, len(data)) + self.assertEqual(self.user.username, data["username"]) + self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) + self.assertEqual("US", data["country"]) + self.assertEqual("", data["language"]) + self.assertEqual("m", data["gender"]) + self.assertEqual(1900, data["year_of_birth"]) + self.assertEqual("m", data["level_of_education"]) + self.assertEqual("world peace", data["goals"]) + self.assertEqual("Park Ave", data['mailing_address']) + self.assertEqual(self.user.email, data["email"]) + self.assertIsNotNone(data["date_joined"]) + + def test_anonymous_access(self): + """ + Test that an anonymous client (not logged in) cannot call GET or PATCH. """ self.send_get(self.anonymous_client, expected_status=401) + self.send_patch(self.anonymous_client, {}, expected_status=401) - def test_get_account_different_user(self): + def test_unsupported_methods(self): """ - Test that a client (logged in) cannot get the account information for a different client. + Test that DELETE, POST, and PUT are not supported. """ - self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) - self.send_get(self.different_client, expected_status=404) + self.client.login(username=self.user.username, password=TEST_PASSWORD) + self.assertEqual(405, self.client.put(self.url).status_code) + self.assertEqual(405, self.client.post(self.url).status_code) + self.assertEqual(405, self.client.delete(self.url).status_code) @ddt.data( ("client", "user"), @@ -105,6 +151,69 @@ class TestAccountAPI(UserAPITestCase): response = client.get(reverse("accounts_api", kwargs={'username': "does_not_exist"})) self.assertEqual(404, response.status_code) + # Note: using getattr so that the patching works even if there is no configuration. + # This is needed when testing CMS as the patching is still executed even though the + # suite is skipped. + @patch.dict(getattr(settings, "ACCOUNT_VISIBILITY_CONFIGURATION", {}), {"default_visibility": "all_users"}) + def test_get_account_different_user_visible(self): + """ + Test that a client (logged in) can only get the shareable fields for a different user. + This is the case when default_visibility is set to "all_users". + """ + self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) + self.create_mock_profile(self.user) + response = self.send_get(self.different_client) + self._verify_full_shareable_account_response(response) + + # Note: using getattr so that the patching works even if there is no configuration. + # This is needed when testing CMS as the patching is still executed even though the + # suite is skipped. + @patch.dict(getattr(settings, "ACCOUNT_VISIBILITY_CONFIGURATION", {}), {"default_visibility": "private"}) + def test_get_account_different_user_private(self): + """ + Test that a client (logged in) can only get the shareable fields for a different user. + This is the case when default_visibility is set to "private". + """ + self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) + self.create_mock_profile(self.user) + response = self.send_get(self.different_client) + self._verify_private_account_response(response) + + @ddt.data( + ("client", "user", PRIVATE_VISIBILITY), + ("different_client", "different_user", PRIVATE_VISIBILITY), + ("staff_client", "staff_user", PRIVATE_VISIBILITY), + ("client", "user", ALL_USERS_VISIBILITY), + ("different_client", "different_user", ALL_USERS_VISIBILITY), + ("staff_client", "staff_user", ALL_USERS_VISIBILITY), + ) + @ddt.unpack + def test_get_account_private_visibility(self, api_client, requesting_username, preference_visibility): + """ + Test the return from GET based on user visibility setting. + """ + def verify_fields_visible_to_all_users(response): + if preference_visibility == PRIVATE_VISIBILITY: + self._verify_private_account_response(response) + else: + self._verify_full_shareable_account_response(response) + + client = self.login_client(api_client, requesting_username) + + # Update user account visibility setting. + UserPreference.set_preference(self.user, ACCOUNT_VISIBILITY_PREF_KEY, preference_visibility) + self.create_mock_profile(self.user) + response = self.send_get(client) + + if requesting_username == "different_user": + verify_fields_visible_to_all_users(response) + else: + self._verify_full_account_response(response) + + # Verify how the view parameter changes the fields that are returned. + response = self.send_get(client, query_parameters='view=shared') + verify_fields_visible_to_all_users(response) + def test_get_account_default(self): """ Test that a client (logged in) can get her own account information (using default legacy profile information, @@ -126,33 +235,6 @@ class TestAccountAPI(UserAPITestCase): self.assertEqual(self.user.email, data["email"]) self.assertIsNotNone(data["date_joined"]) - @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - def test_get_account(self, api_client, user): - """ - Test that a client (logged in) can get her own account information. Also verifies that a "is_staff" - user can get the account information for other users. - """ - self.create_mock_profile(self.user) - client = self.login_client(api_client, user) - response = self.send_get(client) - data = response.data - self.assertEqual(11, len(data)) - self.assertEqual(self.user.username, data["username"]) - self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) - self.assertEqual("US", data["country"]) - self.assertEqual("", data["language"]) - self.assertEqual("m", data["gender"]) - self.assertEqual(1900, data["year_of_birth"]) - self.assertEqual("m", data["level_of_education"]) - self.assertEqual("world peace", data["goals"]) - self.assertEqual("Park Ave", data['mailing_address']) - self.assertEqual(self.user.email, data["email"]) - self.assertIsNotNone(data["date_joined"]) - def test_get_account_empty_string(self): """ Test the conversion of empty strings to None for certain fields. @@ -168,18 +250,18 @@ class TestAccountAPI(UserAPITestCase): for empty_field in ("level_of_education", "gender", "country"): self.assertIsNone(response.data[empty_field]) - def test_patch_account_anonymous_user(self): + @ddt.data( + ("different_client", "different_user"), + ("staff_client", "staff_user"), + ) + @ddt.unpack + def test_patch_account_disallowed_user(self, api_client, user): """ - Test that an anonymous client (not logged in) cannot call patch. + Test that a client cannot call PATCH on a different client's user account (even with + is_staff access). """ - self.send_patch(self.anonymous_client, {}, expected_status=401) - - def test_patch_account_different_user(self): - """ - Test that a client (logged in) cannot update the account information for a different client. - """ - self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) - self.send_patch(self.different_client, {}, expected_status=404) + client = self.login_client(api_client, user) + self.send_patch(client, {}, expected_status=404) @ddt.data( ("client", "user"), @@ -198,34 +280,23 @@ class TestAccountAPI(UserAPITestCase): self.assertEqual(404, response.status_code) @ddt.data( - ( - "client", "user", "gender", "f", "not a gender", - "Select a valid choice. not a gender is not one of the available choices." - ), - ( - "client", "user", "level_of_education", "none", "x", - "Select a valid choice. x is not one of the available choices." - ), - ("client", "user", "country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."), - ("client", "user", "year_of_birth", 2009, "not_an_int", "Enter a whole number."), - ("client", "user", "name", "bob", "z" * 256, "Ensure this value has at most 255 characters (it has 256)."), - ("client", "user", "name", u"ȻħȺɍłɇs", "z ", "The name field must be at least 2 characters long."), - ("client", "user", "language", "Creole"), - ("client", "user", "goals", "Smell the roses"), - ("client", "user", "mailing_address", "Sesame Street"), - # All of the fields can be edited by is_staff, but iterating through all of them again seems like overkill. - # Just test a representative field. - ("staff_client", "staff_user", "goals", "Smell the roses"), + ("gender", "f", "not a gender", "Select a valid choice. not a gender is not one of the available choices."), + ("level_of_education", "none", "x", "Select a valid choice. x is not one of the available choices."), + ("country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."), + ("year_of_birth", 2009, "not_an_int", "Enter a whole number."), + ("name", "bob", "z" * 256, "Ensure this value has at most 255 characters (it has 256)."), + ("name", u"ȻħȺɍłɇs", "z ", "The name field must be at least 2 characters long."), + ("language", "Creole"), + ("goals", "Smell the roses"), + ("mailing_address", "Sesame Street"), # Note that email is tested below, as it is not immediately updated. ) @ddt.unpack - def test_patch_account( - self, api_client, user, field, value, fails_validation_value=None, developer_validation_message=None - ): + def test_patch_account(self, field, value, fails_validation_value=None, developer_validation_message=None): """ Test the behavior of patch, when using the correct content_type. """ - client = self.login_client(api_client, user) + client = self.login_client("client", "user") self.send_patch(client, {field: value}) get_response = self.send_get(client) @@ -248,16 +319,12 @@ class TestAccountAPI(UserAPITestCase): get_response = self.send_get(client) self.assertEqual("", get_response.data[field]) - @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), - ) @ddt.unpack - def test_patch_account_noneditable(self, api_client, user): + def test_patch_account_noneditable(self): """ Tests the behavior of patch when a read-only field is attempted to be edited. """ - client = self.login_client(api_client, user) + client = self.login_client("client", "user") def verify_error_response(field_name, data): self.assertEqual( @@ -336,25 +403,19 @@ class TestAccountAPI(UserAPITestCase): name_change_info = get_name_change_info(1) verify_change_info(name_change_info[0], old_name, self.user.username, "Mickey Mouse") - # Now change the name as a different (staff) user and verify meta information. - self.staff_client.login(username=self.staff_user.username, password=TEST_PASSWORD) - self.send_patch(self.staff_client, {"name": "Donald Duck"}) + # Now change the name again and verify meta information. + self.send_patch(self.client, {"name": "Donald Duck"}) name_change_info = get_name_change_info(2) verify_change_info(name_change_info[0], old_name, self.user.username, "Donald Duck",) - verify_change_info(name_change_info[1], "Mickey Mouse", self.staff_user.username, "Donald Duck") + verify_change_info(name_change_info[1], "Mickey Mouse", self.user.username, "Donald Duck") - @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - def test_patch_email(self, api_client, user): + def test_patch_email(self): """ - Test that the user (and anyone with an is_staff account) can request an email change through the accounts API. + Test that the user can request an email change through the accounts API. Full testing of the helper method used (do_email_change_request) exists in the package with the code. Here just do minimal smoke testing. """ - client = self.login_client(api_client, user) + client = self.login_client("client", "user") old_email = self.user.email new_email = "newemail@example.com" self.send_patch(client, {"email": new_email, "goals": "change my email"}) @@ -379,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 358ed3cb36..0b150ceac7 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -4,24 +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 _ -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 +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 .api import get_account_settings, update_account_settings class AccountView(APIView): @@ -32,54 +25,78 @@ 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, IsUserInUrlOrStaff) + permission_classes = (permissions.IsAuthenticated,) parser_classes = (MergePatchParser,) def get(self, request, username): @@ -87,35 +104,12 @@ class AccountView(APIView): GET /api/user/v0/accounts/{username}/ """ try: - account_settings = AccountView.get_serialized_account(username) + 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(username): - """Returns the user's account information serialized as JSON. - - Note: - This method does not perform authentication so it is up to the caller - to ensure that only the user themselves or staff can access the account. - - Args: - username (str): The username for the desired account. - - Returns: - A dict containing each of the account's fields. - - Raises: - AccountUserNotFound: raised if there is no account for the specified username. - """ - existing_user, existing_user_profile = AccountView._get_user_and_profile(username) - user_serializer = AccountUserSerializer(existing_user) - legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile) - - return dict(user_serializer.data, **legacy_profile_serializer.data) - def patch(self, request, username): """ PATCH /api/user/v0/accounts/{username}/ @@ -125,128 +119,18 @@ class AccountView(APIView): else an error response with status code 415 will be returned. """ try: - AccountView.update_account(request.user, username, request.DATA) - except AccountUserNotFound: + 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, username, update): - """Update the account for the given username. - - Note: - No authorization or permissions checks are done in this method. It is up to the caller - of this method to enforce the contract that this method is only called if - requesting_user.username == username or requesting_user.is_staff == True. - - Arguments: - requesting_user (User): the user who initiated the request - username (string): the username associated with the account to change - update (dict): the updated account field values - - Raises: - AccountUserNotFound: no user exists with the specified 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) - """ - 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 371e4a4186..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 @@ -90,14 +90,14 @@ def update_preferences(username, **kwargs): @intercept_errors(ProfileInternalError, ignore_errors=[ProfileRequestError]) -def update_email_opt_in(username, org, optin): +def update_email_opt_in(user, org, optin): """Updates a user's preference for receiving org-wide emails. Sets a User Org Tag defining the choice to opt in or opt out of organization-wide emails. Arguments: - username (str): The user to set a preference for. + user (User): The user to set a preference for. org (str): The org is used to determine the organization this setting is related to. optin (Boolean): True if the user is choosing to receive emails for this organization. If the user is not the correct age to receive emails, email-optin is set to False regardless. @@ -105,11 +105,8 @@ def update_email_opt_in(username, org, optin): Returns: None - Raises: - AccountUserNotFound: Raised when the username specified is not associated with a user. - """ - account_settings = AccountView.get_serialized_account(username) + 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. @@ -118,7 +115,6 @@ def update_email_opt_in(username, org, optin): ) try: - user = User.objects.get(username=username) preference, _ = UserOrgTag.objects.get_or_create( user=user, org=org, key='email-optin' ) diff --git a/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py b/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py index 8bf2b295ff..f7c43fe863 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_email_opt_in_list.py @@ -297,7 +297,7 @@ class EmailOptInListTest(ModuleStoreTestCase): None """ - profile_api.update_email_opt_in(user.username, org, is_opted_in) + profile_api.update_email_opt_in(user, org, is_opted_in) def _latest_pref_set_datetime(self, user): """Retrieve the latest opt-in preference for the user, diff --git a/openedx/core/djangoapps/user_api/profiles/__init__.py b/openedx/core/djangoapps/user_api/profiles/__init__.py deleted file mode 100644 index b3d5698513..0000000000 --- a/openedx/core/djangoapps/user_api/profiles/__init__.py +++ /dev/null @@ -1,11 +0,0 @@ -""" -Profile constants -""" - -PROFILE_VISIBILITY_PREF_KEY = 'profile_privacy' - -# Indicates the user's preference that all users can view their profile. -ALL_USERS_VISIBILITY = 'all_users' - -# Indicates the user's preference that their profile be private. -PRIVATE_VISIBILITY = 'private' diff --git a/openedx/core/djangoapps/user_api/profiles/tests/__init__.py b/openedx/core/djangoapps/user_api/profiles/tests/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/openedx/core/djangoapps/user_api/profiles/tests/test_views.py b/openedx/core/djangoapps/user_api/profiles/tests/test_views.py deleted file mode 100644 index 28e4377861..0000000000 --- a/openedx/core/djangoapps/user_api/profiles/tests/test_views.py +++ /dev/null @@ -1,137 +0,0 @@ -""" -Unit tests for profile APIs. -""" - -import ddt -import unittest - -from django.conf import settings -from django.core.urlresolvers import reverse -from mock import patch - -from openedx.core.djangoapps.user_api.accounts.tests.test_views import UserAPITestCase -from openedx.core.djangoapps.user_api.models import UserPreference -from openedx.core.djangoapps.user_api.profiles import PROFILE_VISIBILITY_PREF_KEY -from .. import PRIVATE_VISIBILITY - - -@ddt.ddt -@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile APIs are only supported in LMS') -class TestProfileAPI(UserAPITestCase): - """ - Unit tests for the profile API. - """ - - def setUp(self): - super(TestProfileAPI, self).setUp() - self.url = reverse("profiles_api", kwargs={'username': self.user.username}) - - def test_get_profile_anonymous_user(self): - """ - Test that an anonymous client (not logged in) cannot call get. - """ - self.send_get(self.anonymous_client, expected_status=401) - - def _verify_full_profile_response(self, response): - """ - Verify that all of the profile's fields are returned - """ - data = response.data - self.assertEqual(6, len(data)) - self.assertEqual(self.user.username, data["username"]) - self.assertEqual("US", data["country"]) - self.assertIsNone(data["profile_image"]) - self.assertIsNone(data["time_zone"]) - self.assertIsNone(data["languages"]) - self.assertIsNone(data["bio"]) - - def _verify_private_profile_response(self, response): - """ - Verify that only the public fields are returned for a private user's profile - """ - data = response.data - self.assertEqual(2, len(data)) - self.assertEqual(self.user.username, data["username"]) - self.assertIsNone(data["profile_image"]) - - @ddt.data( - ("client", "user"), - ("different_client", "different_user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - # Note: using getattr so that the patching works even if there is no configuration. - # This is needed when testing CMS as the patching is still executed even though the - # suite is skipped. - @patch.dict(getattr(settings, "PROFILE_CONFIGURATION", {}), {"default_visibility": "all_users"}) - def test_get_default_profile(self, api_client, username): - """ - Test that any logged in user can get the main test user's public profile information. - """ - client = self.login_client(api_client, username) - self.create_mock_profile(self.user) - response = self.send_get(client) - self._verify_full_profile_response(response) - - @ddt.data( - ("client", "user"), - ("different_client", "different_user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - # Note: using getattr so that the patching works even if there is no configuration. - # This is needed when testing CMS as the patching is still executed even though the - # suite is skipped. - @patch.dict(getattr(settings, "PROFILE_CONFIGURATION", {}), {"default_visibility": "private"}) - def test_get_default_private_profile(self, api_client, username): - """ - Test that any logged in user gets only the public fields for a profile - if the default visibility is 'private'. - """ - client = self.login_client(api_client, username) - self.create_mock_profile(self.user) - response = self.send_get(client) - self._verify_private_profile_response(response) - - @ddt.data( - ("client", "user"), - ("different_client", "different_user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - def test_get_private_profile(self, api_client, requesting_username): - """ - Test that private profile information is only available to the test user themselves. - """ - client = self.login_client(api_client, requesting_username) - - # Verify that a user with a private profile only returns the public fields - UserPreference.set_preference(self.user, PROFILE_VISIBILITY_PREF_KEY, PRIVATE_VISIBILITY) - self.create_mock_profile(self.user) - response = self.send_get(client) - self._verify_private_profile_response(response) - - # Verify that only the public fields are returned if 'include_all' parameter is specified as false - response = self.send_get(client, query_parameters='include_all=false') - self._verify_private_profile_response(response) - - # Verify that all fields are returned for the user themselves if - # the 'include_all' parameter is specified as true. - response = self.send_get(client, query_parameters='include_all=true') - if requesting_username == "user": - self._verify_full_profile_response(response) - else: - self._verify_private_profile_response(response) - - @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), - ) - @ddt.unpack - def test_get_profile_unknown_user(self, api_client, username): - """ - Test that requesting a user who does not exist returns a 404. - """ - client = self.login_client(api_client, username) - response = client.get(reverse("profiles_api", kwargs={'username': "does_not_exist"})) - self.assertEqual(404, response.status_code) diff --git a/openedx/core/djangoapps/user_api/profiles/views.py b/openedx/core/djangoapps/user_api/profiles/views.py deleted file mode 100644 index 2d4038e276..0000000000 --- a/openedx/core/djangoapps/user_api/profiles/views.py +++ /dev/null @@ -1,123 +0,0 @@ -""" -NOTE: this API is WIP and has not yet been approved. Do not use this API without talking to Christina or Andy. - -For more information, see: -https://openedx.atlassian.net/wiki/display/TNL/User+API -""" -from django.conf import settings -from django.contrib.auth.models import User - -from rest_framework import status -from rest_framework.views import APIView -from rest_framework.response import Response -from rest_framework.authentication import OAuth2Authentication, SessionAuthentication -from rest_framework import permissions - -from ..accounts.views import AccountView -from ..api.account import AccountUserNotFound -from ..models import UserPreference - -from . import PROFILE_VISIBILITY_PREF_KEY, ALL_USERS_VISIBILITY - - -class ProfileView(APIView): - """ - **Use Cases** - - Get the user's public profile information. - - **Example Requests**: - - GET /api/user/v0/profiles/{username}/[?include_all={true | false}] - - **Response Values for GET** - - Returns the same responses as for the AccountView API for the - subset of fields that have been configured to be in a profile. - The fields are additionally filtered based upon the user's - specified privacy permissions. - - If the parameter 'include_all' is passed as 'true' then a user - can receive all fields for their own account, ignoring - any field visibility preferences. If the parameter is not - specified or if the user is requesting information for a - different account then the privacy filtering will be applied. - - """ - authentication_classes = (OAuth2Authentication, SessionAuthentication) - permission_classes = (permissions.IsAuthenticated,) - - def get(self, request, username): - """ - GET /api/user/v0/profiles/{username}/[?include_all={true | false}] - - Note: - The include_all query parameter will only be honored if the user is making - the request for their own username. It defaults to false, but if true - then all the profile fields will be returned even for a user with - a private profile. - """ - if request.user.username == username: - include_all_fields = self.request.QUERY_PARAMS.get('include_all') == 'true' - else: - include_all_fields = False - try: - profile_settings = ProfileView.get_serialized_profile( - username, - settings.PROFILE_CONFIGURATION, - include_all_fields=include_all_fields, - ) - except AccountUserNotFound: - return Response(status=status.HTTP_404_NOT_FOUND) - return Response(profile_settings) - - @staticmethod - def get_serialized_profile(username, configuration=None, include_all_fields=False): - """Returns the user's public profile settings serialized as JSON. - - The fields returned are by default governed by the user's privacy preference. - If the user has a private profile, then only the fields that are always - public are returned. If the user is sharing their profile with all users - then all profile fields are returned. - - Note: - This method does not perform authentication so it is up to the caller - to ensure that only edX users can access the profile. In addition, only - the user themselves should be able to access all fields of a private - profile through 'include_all_fields' being true. - - Args: - username (str): The username for the desired account. - configuration (dict): A dictionary specifying three profile configuration settings: - default_visibility: the default visibility level for user's with no preference - all_fields: the list of all fields that can be shown on a profile - public_fields: the list of profile fields that are public - include_all_fields (bool): If true, ignores the user's privacy setting. - - Returns: - A dict containing each of the user's profile fields. - - Raises: - AccountUserNotFound: raised if there is no account for the specified username. - """ - if not configuration: - configuration = settings.PROFILE_CONFIGURATION - account_settings = AccountView.get_serialized_account(username) - profile = {} - privacy_setting = ProfileView._get_user_profile_privacy(username, configuration) - if include_all_fields or privacy_setting == ALL_USERS_VISIBILITY: - field_names = configuration.get('all_fields') - else: - field_names = configuration.get('public_fields') - for field_name in field_names: - profile[field_name] = account_settings.get(field_name, None) - return profile - - @staticmethod - def _get_user_profile_privacy(username, configuration): - """ - Returns the profile privacy preference for the specified user. - """ - user = User.objects.get(username=username) - profile_privacy = UserPreference.get_preference(user, PROFILE_VISIBILITY_PREF_KEY) - return profile_privacy if profile_privacy else configuration.get('default_visibility') 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 c5cab80744..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 @@ -29,7 +29,8 @@ class ProfileApiTest(ModuleStoreTestCase): account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) # Retrieve the account settings - account_settings = AccountView.get_serialized_account(self.USERNAME) + user = User.objects.get(username=self.USERNAME) + 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']) @@ -87,7 +88,7 @@ class ProfileApiTest(ModuleStoreTestCase): profile.year_of_birth = year_of_birth profile.save() - profile_api.update_email_opt_in(self.USERNAME, course.id.org, option) + profile_api.update_email_opt_in(user, course.id.org, option) result_obj = UserOrgTag.objects.get(user=user, org=course.id.org, key='email-optin') self.assertEqual(result_obj.value, expected_result) @@ -99,7 +100,7 @@ class ProfileApiTest(ModuleStoreTestCase): user = User.objects.get(username=self.USERNAME) - profile_api.update_email_opt_in(self.USERNAME, course.id.org, True) + profile_api.update_email_opt_in(user, course.id.org, True) result_obj = UserOrgTag.objects.get(user=user, org=course.id.org, key='email-optin') self.assertEqual(result_obj.value, u"True") @@ -130,8 +131,8 @@ class ProfileApiTest(ModuleStoreTestCase): profile.year_of_birth = year_of_birth profile.save() - profile_api.update_email_opt_in(self.USERNAME, course.id.org, option) - profile_api.update_email_opt_in(self.USERNAME, course.id.org, second_option) + profile_api.update_email_opt_in(user, course.id.org, option) + profile_api.update_email_opt_in(user, course.id.org, second_option) result_obj = UserOrgTag.objects.get(user=user, org=course.id.org, key='email-optin') self.assertEqual(result_obj.value, expected_result) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 3d11542e38..e4f2bbe69d 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -8,6 +8,7 @@ import re from django.conf import settings from django.core.urlresolvers import reverse from django.core import mail +from django.contrib.auth.models import User from django.test import TestCase from django.test.utils import override_settings from unittest import skipUnless @@ -23,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 @@ -1247,7 +1248,8 @@ class RegistrationViewTest(ApiTestCase): ) # Verify that the user's full name is set - account_settings = AccountView.get_serialized_account(self.USERNAME) + user = User.objects.get(username=self.USERNAME) + account_settings = get_account_settings(user) self.assertEqual(account_settings["name"], self.NAME) # Verify that we've been logged in @@ -1280,7 +1282,8 @@ class RegistrationViewTest(ApiTestCase): self.assertHttpOK(response) # Verify the user's account - account_settings = AccountView.get_serialized_account(self.USERNAME) + user = User.objects.get(username=self.USERNAME) + 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)) diff --git a/openedx/core/djangoapps/user_api/urls.py b/openedx/core/djangoapps/user_api/urls.py index 795deffb73..4d4ed03731 100644 --- a/openedx/core/djangoapps/user_api/urls.py +++ b/openedx/core/djangoapps/user_api/urls.py @@ -3,7 +3,6 @@ Defines the URL routes for this app. """ from .accounts.views import AccountView -from .profiles.views import ProfileView from django.conf.urls import patterns, url @@ -16,9 +15,4 @@ urlpatterns = patterns( AccountView.as_view(), name="accounts_api" ), - url( - r'^v0/profiles/' + USERNAME_PATTERN + '$', - ProfileView.as_view(), - name="profiles_api" - ), )