diff --git a/lms/djangoapps/teams/tests/test_serializers.py b/lms/djangoapps/teams/tests/test_serializers.py index c30334ad2e..55323d638e 100644 --- a/lms/djangoapps/teams/tests/test_serializers.py +++ b/lms/djangoapps/teams/tests/test_serializers.py @@ -65,7 +65,8 @@ class MembershipSerializerTestCase(SerializerTestCase): 'image_url_medium': 'http://testserver/static/default_50.png', 'image_url_small': 'http://testserver/static/default_30.png', 'has_image': False - } + }, + 'account_privacy': None }) self.assertNotIn('membership', data['team']) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index a498a26f30..4cdab1a37a 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -129,7 +129,7 @@ class TestDashboard(SharedModuleStoreTestCase): team.add_user(self.user) # Check the query count on the dashboard again - with self.assertNumQueries(19): + with self.assertNumQueries(20): self.client.get(self.teams_url) def test_bad_course_id(self): diff --git a/lms/envs/common.py b/lms/envs/common.py index e4ca17c0b1..7b2c053e0e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2544,12 +2544,14 @@ ACCOUNT_VISIBILITY_CONFIGURATION = { 'time_zone', 'language_proficiencies', 'bio', + 'account_privacy', ], # The list of account fields that are always public "public_fields": [ 'username', 'profile_image', + 'account_privacy', ], # The list of account fields that are visible only to staff and users viewing their own profiles @@ -2569,6 +2571,7 @@ ACCOUNT_VISIBILITY_CONFIGURATION = { "level_of_education", "mailing_address", "requires_parental_consent", + "account_privacy", ] } diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 461f5e92dc..d38726d946 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -8,6 +8,8 @@ from pytz import UTC from django.core.exceptions import ObjectDoesNotExist from django.conf import settings from django.core.validators import validate_email, validate_slug, ValidationError +from openedx.core.djangoapps.user_api.preferences.api import update_user_preferences +from openedx.core.djangoapps.user_api.errors import PreferenceValidationError from student.models import User, UserProfile, Registration from student import views as student_views @@ -70,7 +72,7 @@ def get_account_settings(request, username=None, configuration=None, view=None): username = requesting_user.username try: - existing_user = User.objects.get(username=username) + existing_user = User.objects.select_related('profile').get(username=username) except ObjectDoesNotExist: raise UserNotFound() @@ -185,6 +187,13 @@ def update_account_settings(requesting_user, update, username=None): for serializer in user_serializer, legacy_profile_serializer: serializer.save() + # if any exception is raised for user preference (i.e. account_privacy), the entire transaction for user account + # patch is rolled back and the data is not saved + if 'account_privacy' in update: + update_user_preferences( + requesting_user, {'account_privacy': update["account_privacy"]}, existing_user + ) + if "language_proficiencies" in update: new_language_proficiencies = update["language_proficiencies"] emit_setting_changed_event( @@ -209,6 +218,8 @@ def update_account_settings(requesting_user, update, username=None): existing_user_profile.set_meta(meta) existing_user_profile.save() + except PreferenceValidationError as err: + raise AccountValidationError(err.preference_errors) except Exception as err: raise AccountUpdateError( u"Error thrown when saving account updates: '{}'".format(err.message) diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py index fff44bbbf7..c1741634d8 100644 --- a/openedx/core/djangoapps/user_api/accounts/serializers.py +++ b/openedx/core/djangoapps/user_api/accounts/serializers.py @@ -91,6 +91,7 @@ class UserReadOnlySerializer(serializers.Serializer): "level_of_education": AccountLegacyProfileSerializer.convert_empty_to_None(profile.level_of_education), "mailing_address": profile.mailing_address, "requires_parental_consent": profile.requires_parental_consent(), + "account_privacy": UserPreference.get_value(user, 'account_privacy'), } return self._filter_fields( diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index fa0f62f761..534f7626ab 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -150,6 +150,9 @@ class TestAccountApi(UserSettingsEventTestMixin, TestCase): {"language_proficiencies": [{}]} ) + with self.assertRaises(AccountValidationError): + update_account_settings(self.user, {"account_privacy": ""}) + 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. @@ -275,6 +278,7 @@ class AccountSettingsOnCreationTest(TestCase): }, 'requires_parental_consent': True, 'language_proficiencies': [], + 'account_privacy': None }) 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 d161be7989..5bc49ced6b 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -16,6 +16,7 @@ from django.core.urlresolvers import reverse from django.test.testcases import TransactionTestCase from django.test.utils import override_settings from rest_framework.test import APITestCase, APIClient +from openedx.core.djangoapps.user_api.models import UserPreference from student.tests.factories import UserFactory from student.models import UserProfile, LanguageProficiency, PendingEmailChange @@ -151,34 +152,36 @@ class TestAccountAPI(UserAPITestCase): } ) - def _verify_full_shareable_account_response(self, response): + def _verify_full_shareable_account_response(self, response, account_privacy=None): """ Verify that the shareable fields from the account are returned """ data = response.data - self.assertEqual(6, len(data)) + self.assertEqual(7, len(data)) self.assertEqual(self.user.username, data["username"]) self.assertEqual("US", data["country"]) self._verify_profile_image_data(data, True) self.assertIsNone(data["time_zone"]) self.assertEqual([{"code": "en"}], data["language_proficiencies"]) self.assertEqual("Tired mother of twins", data["bio"]) + self.assertEqual(account_privacy, data["account_privacy"]) - def _verify_private_account_response(self, response, requires_parental_consent=False): + def _verify_private_account_response(self, response, requires_parental_consent=False, account_privacy=None): """ 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(3, len(data)) self.assertEqual(self.user.username, data["username"]) self._verify_profile_image_data(data, not requires_parental_consent) + self.assertEqual(account_privacy, data["account_privacy"]) def _verify_full_account_response(self, response, requires_parental_consent=False): """ Verify that all account fields are returned (even those that are not shareable). """ data = response.data - self.assertEqual(15, len(data)) + self.assertEqual(16, 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"]) @@ -194,6 +197,7 @@ class TestAccountAPI(UserAPITestCase): self._verify_profile_image_data(data, not requires_parental_consent) self.assertEquals(requires_parental_consent, data["requires_parental_consent"]) self.assertEqual([{"code": "en"}], data["language_proficiencies"]) + self.assertEqual(UserPreference.get_value(self.user, 'account_privacy'), data["account_privacy"]) def test_anonymous_access(self): """ @@ -235,7 +239,8 @@ class TestAccountAPI(UserAPITestCase): """ self.different_client.login(username=self.different_user.username, password=self.test_password) self.create_mock_profile(self.user) - response = self.send_get(self.different_client) + with self.assertNumQueries(9): + 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. @@ -249,7 +254,8 @@ class TestAccountAPI(UserAPITestCase): """ self.different_client.login(username=self.different_user.username, password=self.test_password) self.create_mock_profile(self.user) - response = self.send_get(self.different_client) + with self.assertNumQueries(9): + response = self.send_get(self.different_client) self._verify_private_account_response(response) @ddt.data( @@ -270,9 +276,9 @@ class TestAccountAPI(UserAPITestCase): Confirms that private fields are private, and public/shareable fields are public/shareable """ if preference_visibility == PRIVATE_VISIBILITY: - self._verify_private_account_response(response) + self._verify_private_account_response(response, account_privacy=PRIVATE_VISIBILITY) else: - self._verify_full_shareable_account_response(response) + self._verify_full_shareable_account_response(response, ALL_USERS_VISIBILITY) client = self.login_client(api_client, requesting_username) @@ -299,9 +305,10 @@ class TestAccountAPI(UserAPITestCase): """ Internal helper to perform the actual assertions """ - response = self.send_get(self.client) + with self.assertNumQueries(8): + response = self.send_get(self.client) data = response.data - self.assertEqual(15, len(data)) + self.assertEqual(16, len(data)) self.assertEqual(self.user.username, data["username"]) self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) for empty_field in ("year_of_birth", "level_of_education", "mailing_address", "bio"): @@ -315,6 +322,7 @@ class TestAccountAPI(UserAPITestCase): self._verify_profile_image_data(data, False) self.assertTrue(data["requires_parental_consent"]) self.assertEqual([], data["language_proficiencies"]) + self.assertEqual(None, data["account_privacy"]) self.client.login(username=self.user.username, password=self.test_password) verify_get_own_information() @@ -336,7 +344,8 @@ class TestAccountAPI(UserAPITestCase): legacy_profile.save() self.client.login(username=self.user.username, password=self.test_password) - response = self.send_get(self.client) + with self.assertNumQueries(8): + response = self.send_get(self.client) for empty_field in ("level_of_education", "gender", "country", "bio"): self.assertIsNone(response.data[empty_field]) @@ -383,6 +392,8 @@ class TestAccountAPI(UserAPITestCase): "bio", u"Lacrosse-playing superhero 壓是進界推日不復女", "z" * 3001, u"Ensure this field has no more than 3000 characters." ), + ("account_privacy", ALL_USERS_VISIBILITY), + ("account_privacy", PRIVATE_VISIBILITY), # Note that email is tested below, as it is not immediately updated. # Note that language_proficiencies is tested below as there are multiple error and success conditions. ) @@ -407,8 +418,9 @@ class TestAccountAPI(UserAPITestCase): ), error_response.data["field_errors"][field]["developer_message"] ) - else: - # If there are no values that would fail validation, then empty string should be supported. + elif field != "account_privacy": + # If there are no values that would fail validation, then empty string should be supported; + # except for account_privacy, which cannot be an empty string. response = self.send_patch(client, {field: ""}) self.assertEqual("", response.data[field]) @@ -662,7 +674,7 @@ class TestAccountAPI(UserAPITestCase): response = self.send_get(client) if has_full_access: data = response.data - self.assertEqual(15, len(data)) + self.assertEqual(16, len(data)) self.assertEqual(self.user.username, data["username"]) self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) self.assertEqual(self.user.email, data["email"]) @@ -675,12 +687,17 @@ class TestAccountAPI(UserAPITestCase): self.assertIsNotNone(data["date_joined"]) self._verify_profile_image_data(data, False) self.assertTrue(data["requires_parental_consent"]) + self.assertEqual(ALL_USERS_VISIBILITY, data["account_privacy"]) else: - self._verify_private_account_response(response, requires_parental_consent=True) + self._verify_private_account_response( + response, requires_parental_consent=True, account_privacy=ALL_USERS_VISIBILITY + ) # Verify that the shared view is still private response = self.send_get(client, query_parameters='view=shared') - self._verify_private_account_response(response, requires_parental_consent=True) + self._verify_private_account_response( + response, requires_parental_consent=True, account_privacy=ALL_USERS_VISIBILITY + ) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 9735c8631e..1d6250ea5d 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -96,6 +96,8 @@ class AccountView(APIView): requiring parental consent. * username: The username associated with the account. * year_of_birth: The year the user was born, as an integer, or null. + * account_privacy: The user's setting for sharing her personal + profile. Possible values are "all_users" or "private". For all text fields, plain text instead of HTML is supported. The data is stored exactly as specified. Clients must HTML escape diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py index e7d0705717..cd4d94b4a0 100644 --- a/openedx/core/djangoapps/user_api/preferences/api.py +++ b/openedx/core/djangoapps/user_api/preferences/api.py @@ -45,7 +45,7 @@ def get_user_preference(requesting_user, preference_key, username=None): UserNotAuthorized: the requesting_user does not have access to the user preference. UserAPIInternalError: the operation failed due to an unexpected error. """ - existing_user = _get_user(requesting_user, username, allow_staff=True) + existing_user = _get_authorized_user(requesting_user, username, allow_staff=True) return UserPreference.get_value(existing_user, preference_key) @@ -68,7 +68,7 @@ def get_user_preferences(requesting_user, username=None): UserNotAuthorized: the requesting_user does not have access to the user preference. UserAPIInternalError: the operation failed due to an unexpected error. """ - existing_user = _get_user(requesting_user, username, allow_staff=True) + existing_user = _get_authorized_user(requesting_user, username, allow_staff=True) # Django Rest Framework V3 uses the current request to version # hyperlinked URLS, so we need to retrieve the request and pass @@ -84,8 +84,8 @@ def get_user_preferences(requesting_user, username=None): @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) -def update_user_preferences(requesting_user, update, username=None): - """Update the user preferences for the given username. +def update_user_preferences(requesting_user, update, user=None): + """Update the user preferences for the given user. Note: It is up to the caller of this method to enforce the contract that this method is only called @@ -98,8 +98,8 @@ def update_user_preferences(requesting_user, update, username=None): Some notes: Values are expected to be strings. Non-string values will be converted to strings. Null values for a preference will be treated as a request to delete the key in question. - username (str): Optional username specifying which account should be updated. If not specified, - `requesting_user.username` is assumed. + user (str/User): Optional, either username string or user object specifying which account should be updated. + If not specified, `requesting_user.username` is assumed. Raises: UserNotFound: no user with username `username` exists (or `requesting_user.username` if @@ -110,7 +110,10 @@ def update_user_preferences(requesting_user, update, username=None): PreferenceUpdateError: the operation failed when performing the update. UserAPIInternalError: the operation failed due to an unexpected error. """ - existing_user = _get_user(requesting_user, username) + if not user or isinstance(user, basestring): + user = _get_authorized_user(requesting_user, user) + else: + _check_authorized(requesting_user, user.username) # First validate each preference setting errors = {} @@ -119,7 +122,7 @@ def update_user_preferences(requesting_user, update, username=None): preference_value = update[preference_key] if preference_value is not None: try: - serializer = create_user_preference_serializer(existing_user, preference_key, preference_value) + serializer = create_user_preference_serializer(user, preference_key, preference_value) validate_user_preference_serializer(serializer, preference_key, preference_value) serializers[preference_key] = serializer except PreferenceValidationError as error: @@ -168,7 +171,7 @@ def set_user_preference(requesting_user, preference_key, preference_value, usern PreferenceUpdateError: the operation failed when performing the update. UserAPIInternalError: the operation failed due to an unexpected error. """ - existing_user = _get_user(requesting_user, username) + existing_user = _get_authorized_user(requesting_user, username) serializer = create_user_preference_serializer(existing_user, preference_key, preference_value) validate_user_preference_serializer(serializer, preference_key, preference_value) try: @@ -203,7 +206,7 @@ def delete_user_preference(requesting_user, preference_key, username=None): PreferenceUpdateError: the operation failed when performing the update. UserAPIInternalError: the operation failed due to an unexpected error. """ - existing_user = _get_user(requesting_user, username) + existing_user = _get_authorized_user(requesting_user, username) try: user_preference = UserPreference.objects.get(user=existing_user, key=preference_key) except ObjectDoesNotExist: @@ -298,25 +301,32 @@ def _track_update_email_opt_in(user_id, organization, opt_in): ) -def _get_user(requesting_user, username=None, allow_staff=False): +def _get_authorized_user(requesting_user, username=None, allow_staff=False): """ - Helper method to return the user for a given username. + Helper method to return the authorized user for a given username. If username is not provided, requesting_user.username is assumed. """ if username is None: username = requesting_user.username - try: existing_user = User.objects.get(username=username) except ObjectDoesNotExist: raise UserNotFound() + _check_authorized(requesting_user, username, allow_staff) + return existing_user + + +def _check_authorized(requesting_user, username, allow_staff=False): + """ + Helper method that raises UserNotAuthorized if requesting user + is not owner user or is not staff if access to staff is given + (i.e. 'allow_staff' = true) + """ if requesting_user.username != username: if not requesting_user.is_staff or not allow_staff: raise UserNotAuthorized() - return existing_user - def create_user_preference_serializer(user, preference_key, preference_value): """Creates a serializer for the specified user preference. diff --git a/openedx/core/djangoapps/user_api/preferences/tests/test_api.py b/openedx/core/djangoapps/user_api/preferences/tests/test_api.py index a3b3fe8b66..f8bc76e9ab 100644 --- a/openedx/core/djangoapps/user_api/preferences/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/preferences/tests/test_api.py @@ -181,6 +181,34 @@ class TestPreferenceAPI(TestCase): "new_value" ) + def test_update_user_preferences_with_username(self): + """ + Verifies the basic behavior of update_user_preferences when passed + username string. + """ + update_data = { + self.test_preference_key: "new_value" + } + update_user_preferences(self.user, update_data, user=self.user.username) + self.assertEqual( + get_user_preference(self.user, self.test_preference_key), + "new_value" + ) + + def test_update_user_preferences_with_user(self): + """ + Verifies the basic behavior of update_user_preferences when passed + user object. + """ + update_data = { + self.test_preference_key: "new_value" + } + update_user_preferences(self.user, update_data, user=self.user) + self.assertEqual( + get_user_preference(self.user, self.test_preference_key), + "new_value" + ) + @patch('openedx.core.djangoapps.user_api.models.UserPreference.delete') @patch('openedx.core.djangoapps.user_api.models.UserPreference.save') def test_update_user_preferences_errors(self, user_preference_save, user_preference_delete): @@ -191,16 +219,16 @@ class TestPreferenceAPI(TestCase): self.test_preference_key: "new_value" } with self.assertRaises(UserNotFound): - update_user_preferences(self.user, update_data, username="no_such_user") + update_user_preferences(self.user, update_data, user="no_such_user") with self.assertRaises(UserNotFound): update_user_preferences(self.no_such_user, update_data) with self.assertRaises(UserNotAuthorized): - update_user_preferences(self.staff_user, update_data, username=self.user.username) + update_user_preferences(self.staff_user, update_data, user=self.user.username) with self.assertRaises(UserNotAuthorized): - update_user_preferences(self.different_user, update_data, username=self.user.username) + update_user_preferences(self.different_user, update_data, user=self.user.username) too_long_key = "x" * 256 with self.assertRaises(PreferenceValidationError) as context_manager: diff --git a/openedx/core/djangoapps/user_api/preferences/views.py b/openedx/core/djangoapps/user_api/preferences/views.py index c6e6690ed4..7a4649e36c 100644 --- a/openedx/core/djangoapps/user_api/preferences/views.py +++ b/openedx/core/djangoapps/user_api/preferences/views.py @@ -118,7 +118,7 @@ class PreferencesView(APIView): ) try: with transaction.commit_on_success(): - update_user_preferences(request.user, request.data, username=username) + update_user_preferences(request.user, request.data, user=username) except UserNotAuthorized: return Response(status=status.HTTP_403_FORBIDDEN) except UserNotFound: