Merge pull request #10008 from edx/jia/MA-1281

MA-1281 User Account: added account_privacy field in GET and PATCH endpoints

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

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

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",
     ]
 }
 

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)

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(

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

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"<html>Lacrosse-playing superhero 壓是進界推日不復女</html>",
             "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')

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

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.

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:

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: