Merge pull request #9237 from edx/bderusha/teams-expand-users-api-fix

Team API include correct info when expanding users

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

@@ -22,84 +22,67 @@ from ..helpers import intercept_errors
 from ..models import UserPreference
 
 from . import (
-    ACCOUNT_VISIBILITY_PREF_KEY, ALL_USERS_VISIBILITY, PRIVATE_VISIBILITY,
+    ACCOUNT_VISIBILITY_PREF_KEY, PRIVATE_VISIBILITY,
     EMAIL_MIN_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MIN_LENGTH, PASSWORD_MAX_LENGTH,
     USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH
 )
-from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer
+from .serializers import (
+    AccountLegacyProfileSerializer, AccountUserSerializer,
+    UserReadOnlySerializer
+)
 
 
 @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError])
-def get_account_settings(requesting_user, username=None, configuration=None, view=None):
+def get_account_settings(request, 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`.
+        If `request.user.username` != `username`, this method will return differing amounts of information
+        based on who `request.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.
+        request (Request): The request object with account information about the requesting user.
+            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.
+            `request.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`.
+            "shared", only shared account information will be returned, regardless of `request.user`.
 
     Returns:
          A dict containing account fields.
 
     Raises:
-         UserNotFound: no user with username `username` exists (or `requesting_user.username` if
+         UserNotFound: no user with username `username` exists (or `request.user.username` if
             `username` is not specified)
          UserAPIInternalError: the operation failed due to an unexpected error.
     """
+    requesting_user = request.user
+
     if username is None:
         username = requesting_user.username
 
+    try:
+        existing_user = User.objects.get(username=username)
+    except ObjectDoesNotExist:
+        raise UserNotFound()
+
     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_visibility = _get_profile_visibility(existing_user_profile, configuration)
-    if profile_visibility == ALL_USERS_VISIBILITY:
-        field_names = configuration.get('shareable_fields')
+    if has_full_access and view != 'shared':
+        admin_fields = settings.ACCOUNT_VISIBILITY_CONFIGURATION.get('admin_fields')
     else:
-        field_names = configuration.get('public_fields')
+        admin_fields = None
 
-    for field_name in field_names:
-        visible_settings[field_name] = account_settings.get(field_name, None)
-
-    return visible_settings
-
-
-def _get_profile_visibility(user_profile, configuration):
-    """Returns the visibility level for the specified user profile."""
-    if user_profile.requires_parental_consent():
-        return PRIVATE_VISIBILITY
-
-    # Calling UserPreference directly because the requesting user may be different from existing_user
-    # (and does not have to be is_staff).
-    profile_privacy = UserPreference.get_value(user_profile.user, ACCOUNT_VISIBILITY_PREF_KEY)
-    return profile_privacy if profile_privacy else configuration.get('default_visibility')
+    return UserReadOnlySerializer(
+        existing_user,
+        configuration=configuration,
+        custom_fields=admin_fields,
+        context={'request': request}
+    ).data
 
 
 @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError])

openedx/core/djangoapps/user_api/accounts/image_helpers.py

@@ -72,7 +72,7 @@ def get_profile_image_names(username):
     return {size: _get_profile_image_filename(name, size) for size in _PROFILE_IMAGE_SIZES}
 
 
-def get_profile_image_urls_for_user(user):
+def get_profile_image_urls_for_user(user, request=None):
     """
     Return a dict {size:url} for each profile image for a given user.
     Notes:
@@ -93,13 +93,19 @@ def get_profile_image_urls_for_user(user):
 
     """
     if user.profile.has_profile_image:
-        return _get_profile_image_urls(
+        urls = _get_profile_image_urls(
             _make_profile_image_name(user.username),
             get_profile_image_storage(),
             version=user.profile.profile_image_uploaded_at.strftime("%s"),
         )
     else:
-        return _get_default_profile_image_urls()
+        urls = _get_default_profile_image_urls()
+
+    if request:
+        for key, value in urls.items():
+            urls[key] = request.build_absolute_uri(value)
+
+    return urls
 
 
 def _get_default_profile_image_urls():

openedx/core/djangoapps/user_api/accounts/serializers.py

@@ -1,10 +1,16 @@
 from rest_framework import serializers
 from django.contrib.auth.models import User
+from django.conf import settings
+from django.core.urlresolvers import reverse
 from openedx.core.djangoapps.user_api.accounts import NAME_MIN_LENGTH
 from openedx.core.djangoapps.user_api.serializers import ReadOnlyFieldsSerializerMixin
 
 from student.models import UserProfile, LanguageProficiency
+from ..models import UserPreference
 from .image_helpers import get_profile_image_urls_for_user
+from . import (
+    ACCOUNT_VISIBILITY_PREF_KEY, ALL_USERS_VISIBILITY, PRIVATE_VISIBILITY,
+)
 
 
 PROFILE_IMAGE_KEY_PREFIX = 'image_url'
@@ -32,6 +38,104 @@ class LanguageProficiencySerializer(serializers.ModelSerializer):
             return None
 
 
+class UserReadOnlySerializer(serializers.Serializer):
+    """
+    Class that serializes the User model and UserProfile model together.
+    """
+    def __init__(self, *args, **kwargs):
+        # Don't pass the 'configuration' arg up to the superclass
+        self.configuration = kwargs.pop('configuration', None)
+        if not self.configuration:
+            self.configuration = settings.ACCOUNT_VISIBILITY_CONFIGURATION
+
+        # Don't pass the 'custom_fields' arg up to the superclass
+        self.custom_fields = kwargs.pop('custom_fields', None)
+
+        super(UserReadOnlySerializer, self).__init__(*args, **kwargs)
+
+    def to_native(self, user):
+        """
+        Overwrite to_native to handle custom logic since we are serializing two models as one here
+        :param user: User object
+        :return: Dict serialized account
+        """
+        profile = user.profile
+
+        data = {
+            "username": user.username,
+            "url": self.context.get('request').build_absolute_uri(
+                reverse('accounts_api', kwargs={'username': user.username})
+            ),
+            "email": user.email,
+            "date_joined": user.date_joined,
+            "is_active": user.is_active,
+            "bio": AccountLegacyProfileSerializer.convert_empty_to_None(profile.bio),
+            "country": AccountLegacyProfileSerializer.convert_empty_to_None(profile.country.code),
+            "profile_image": AccountLegacyProfileSerializer.get_profile_image(
+                profile,
+                user,
+                self.context.get('request')
+            ),
+            "time_zone": None,
+            "language_proficiencies": LanguageProficiencySerializer(
+                profile.language_proficiencies.all(),
+                many=True
+            ).data,
+            "name": profile.name,
+            "gender": AccountLegacyProfileSerializer.convert_empty_to_None(profile.gender),
+            "goals": profile.goals,
+            "year_of_birth": profile.year_of_birth,
+            "level_of_education": AccountLegacyProfileSerializer.convert_empty_to_None(profile.level_of_education),
+            "mailing_address": profile.mailing_address,
+            "requires_parental_consent": profile.requires_parental_consent(),
+        }
+
+        return self._filter_fields(
+            self._visible_fields(profile, user),
+            data
+        )
+
+    def _visible_fields(self, user_profile, user):
+        """
+        Return what fields should be visible based on user settings
+
+        :param user_profile: User profile object
+        :param user: User object
+        :return: whitelist List of fields to be shown
+        """
+
+        if self.custom_fields:
+            return self.custom_fields
+
+        profile_visibility = self._get_profile_visibility(user_profile, user)
+
+        if profile_visibility == ALL_USERS_VISIBILITY:
+            return self.configuration.get('shareable_fields')
+        else:
+            return self.configuration.get('public_fields')
+
+    def _get_profile_visibility(self, user_profile, user):
+        """Returns the visibility level for the specified user profile."""
+        if user_profile.requires_parental_consent():
+            return PRIVATE_VISIBILITY
+
+        # Calling UserPreference directly because the requesting user may be different from existing_user
+        # (and does not have to be is_staff).
+        profile_privacy = UserPreference.get_value(user, ACCOUNT_VISIBILITY_PREF_KEY)
+        return profile_privacy if profile_privacy else self.configuration.get('default_visibility')
+
+    def _filter_fields(self, field_whitelist, serialized_account):
+        """
+        Filter serialized account Dict to only include whitelisted keys
+        """
+        visible_serialized_account = {}
+
+        for field_name in field_whitelist:
+            visible_serialized_account[field_name] = serialized_account.get(field_name, None)
+
+        return visible_serialized_account
+
+
 class AccountUserSerializer(serializers.HyperlinkedModelSerializer, ReadOnlyFieldsSerializerMixin):
     """
     Class that serializes the portion of User model needed for account information.
@@ -47,7 +151,7 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer, Rea
     """
     Class that serializes the portion of UserProfile model needed for account information.
     """
-    profile_image = serializers.SerializerMethodField("get_profile_image")
+    profile_image = serializers.SerializerMethodField("_get_profile_image")
     requires_parental_consent = serializers.SerializerMethodField("get_requires_parental_consent")
     language_proficiencies = LanguageProficiencySerializer(many=True, allow_add_remove=True, required=False)
 
@@ -102,10 +206,11 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer, Rea
         """ Helper method to convert empty string to None (other values pass through). """
         return None if value == "" else value
 
-    def get_profile_image(self, user_profile):
+    @staticmethod
+    def get_profile_image(user_profile, user, request=None):
         """ Returns metadata about a user's profile image. """
         data = {'has_image': user_profile.has_profile_image}
-        urls = get_profile_image_urls_for_user(user_profile.user)
+        urls = get_profile_image_urls_for_user(user, request)
         data.update({
             '{image_key_prefix}_{size}'.format(image_key_prefix=PROFILE_IMAGE_KEY_PREFIX, size=size_display_name): url
             for size_display_name, url in urls.items()
@@ -115,3 +220,13 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer, Rea
     def get_requires_parental_consent(self, user_profile):
         """ Returns a boolean representing whether the user requires parental controls.  """
         return user_profile.requires_parental_consent()
+
+    def _get_profile_image(self, user_profile):
+        """
+        Returns metadata about a user's profile image
+
+        This protected method delegates to the static 'get_profile_image' method
+        because 'serializers.SerializerMethodField("_get_profile_image")' will
+        call the method with a single argument, the user_profile object.
+        """
+        return AccountLegacyProfileSerializer.get_profile_image(user_profile, user_profile.user)

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

@@ -15,6 +15,7 @@ from student.tests.factories import UserFactory
 from django.conf import settings
 from django.contrib.auth.models import User
 from django.core import mail
+from django.test.client import RequestFactory
 from student.models import PendingEmailChange
 from student.tests.tests import UserSettingsEventTestMixin
 from ...errors import (
@@ -43,21 +44,24 @@ class TestAccountApi(UserSettingsEventTestMixin, TestCase):
 
     def setUp(self):
         super(TestAccountApi, self).setUp()
+        self.request_factory = RequestFactory()
         self.table = "student_languageproficiency"
         self.user = UserFactory.create(password=self.password)
+        self.default_request = self.request_factory.get("/api/user/v1/accounts/")
+        self.default_request.user = self.user
         self.different_user = UserFactory.create(password=self.password)
         self.staff_user = UserFactory(is_staff=True, password=self.password)
         self.reset_tracker()
 
     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)
+        account_settings = get_account_settings(self.default_request)
         self.assertEqual(self.user.username, account_settings["username"])
 
-        account_settings = get_account_settings(self.user, username=self.user.username)
+        account_settings = get_account_settings(self.default_request, username=self.user.username)
         self.assertEqual(self.user.username, account_settings["username"])
 
-        account_settings = get_account_settings(self.user, username=self.different_user.username)
+        account_settings = get_account_settings(self.default_request, username=self.different_user.username)
         self.assertEqual(self.different_user.username, account_settings["username"])
 
     def test_get_configuration_provided(self):
@@ -75,29 +79,35 @@ class TestAccountApi(UserSettingsEventTestMixin, TestCase):
         }
 
         # With default configuration settings, email is not shared with other (non-staff) users.
-        account_settings = get_account_settings(self.user, self.different_user.username)
+        account_settings = get_account_settings(self.default_request, self.different_user.username)
         self.assertFalse("email" in account_settings)
 
-        account_settings = get_account_settings(self.user, self.different_user.username, configuration=config)
+        account_settings = get_account_settings(
+            self.default_request,
+            self.different_user.username,
+            configuration=config
+        )
         self.assertEqual(self.different_user.email, account_settings["email"])
 
     def test_get_user_not_found(self):
         """Test that UserNotFound is thrown if there is no user with username."""
         with self.assertRaises(UserNotFound):
-            get_account_settings(self.user, username="does_not_exist")
+            get_account_settings(self.default_request, username="does_not_exist")
 
         self.user.username = "does_not_exist"
+        request = self.request_factory.get("/api/user/v1/accounts/")
+        request.user = self.user
         with self.assertRaises(UserNotFound):
-            get_account_settings(self.user)
+            get_account_settings(request)
 
     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)
+        account_settings = get_account_settings(self.default_request)
         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)
+        account_settings = get_account_settings(self.default_request)
         self.assertEqual("Donald Duck", account_settings["name"])
 
         with self.assertRaises(UserNotAuthorized):
@@ -171,7 +181,7 @@ class TestAccountApi(UserSettingsEventTestMixin, TestCase):
         self.assertIn("Error thrown from do_email_change_request", context_manager.exception.developer_message)
 
         # Verify that the name change happened, even though the attempt to send the email failed.
-        account_settings = get_account_settings(self.user)
+        account_settings = get_account_settings(self.default_request)
         self.assertEqual("Mickey Mouse", account_settings["name"])
 
     @patch('openedx.core.djangoapps.user_api.accounts.serializers.AccountUserSerializer.save')
@@ -229,7 +239,9 @@ class AccountSettingsOnCreationTest(TestCase):
 
         # Retrieve the account settings
         user = User.objects.get(username=self.USERNAME)
-        account_settings = get_account_settings(user)
+        request = RequestFactory().get("/api/user/v1/accounts/")
+        request.user = user
+        account_settings = get_account_settings(request)
 
         # Expect a date joined field but remove it to simplify the following comparison
         self.assertIsNotNone(account_settings['date_joined'])
@@ -250,8 +262,8 @@ class AccountSettingsOnCreationTest(TestCase):
             'bio': None,
             'profile_image': {
                 'has_image': False,
-                'image_url_full': '/static/default_50.png',
-                'image_url_small': '/static/default_10.png',
+                'image_url_full': request.build_absolute_uri('/static/default_50.png'),
+                'image_url_small': request.build_absolute_uri('/static/default_10.png'),
             },
             'requires_parental_consent': True,
             'language_proficiencies': [],
@@ -303,18 +315,22 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase):
         u'a' * (PASSWORD_MAX_LENGTH + 1)
     ]
 
+    @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
     def test_activate_account(self):
         # Create the account, which is initially inactive
         activation_key = create_account(self.USERNAME, self.PASSWORD, self.EMAIL)
         user = User.objects.get(username=self.USERNAME)
-        account = get_account_settings(user)
+
+        request = RequestFactory().get("/api/user/v1/accounts/")
+        request.user = user
+        account = get_account_settings(request)
         self.assertEqual(self.USERNAME, account["username"])
         self.assertEqual(self.EMAIL, account["email"])
         self.assertFalse(account["is_active"])
 
         # Activate the account and verify that it is now active
         activate_account(activation_key)
-        account = get_account_settings(user)
+        account = get_account_settings(request)
         self.assertTrue(account['is_active'])
 
     def test_create_account_duplicate_username(self):

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

@@ -321,11 +321,12 @@ class TestAccountAPI(UserAPITestCase):
         legacy_profile.country = ""
         legacy_profile.level_of_education = ""
         legacy_profile.gender = ""
+        legacy_profile.bio = ""
         legacy_profile.save()
 
         self.client.login(username=self.user.username, password=self.test_password)
         response = self.send_get(self.client)
-        for empty_field in ("level_of_education", "gender", "country"):
+        for empty_field in ("level_of_education", "gender", "country", "bio"):
             self.assertIsNone(response.data[empty_field])
 
     @ddt.data(

openedx/core/djangoapps/user_api/accounts/views.py

@@ -146,11 +146,7 @@ class AccountView(APIView):
         GET /api/user/v1/accounts/{username}/
         """
         try:
-            account_settings = get_account_settings(request.user, username, view=request.QUERY_PARAMS.get('view'))
-            # Account for possibly relative URLs.
-            for key, value in account_settings['profile_image'].items():
-                if key.startswith(PROFILE_IMAGE_KEY_PREFIX):
-                    account_settings['profile_image'][key] = request.build_absolute_uri(value)
+            account_settings = get_account_settings(request, username, view=request.QUERY_PARAMS.get('view'))
         except UserNotFound:
             return Response(status=status.HTTP_403_FORBIDDEN if request.user.is_staff else status.HTTP_404_NOT_FOUND)
 

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

@@ -18,6 +18,7 @@ from django.contrib.auth.models import User
 from django.test import TestCase
 from django.test.testcases import TransactionTestCase
 from django.test.utils import override_settings
+from django.test.client import RequestFactory
 
 from social.apps.django_app.default.models import UserSocialAuth
 
@@ -1269,7 +1270,9 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase):
         self.assertIn(settings.EDXMKTG_USER_INFO_COOKIE_NAME, self.client.cookies)
 
         user = User.objects.get(username=self.USERNAME)
-        account_settings = get_account_settings(user)
+        request = RequestFactory().get('/url')
+        request.user = user
+        account_settings = get_account_settings(request)
 
         self.assertEqual(self.USERNAME, account_settings["username"])
         self.assertEqual(self.EMAIL, account_settings["email"])
@@ -1307,7 +1310,10 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase):
 
         # Verify the user's account
         user = User.objects.get(username=self.USERNAME)
-        account_settings = get_account_settings(user)
+        request = RequestFactory().get('/url')
+        request.user = user
+        account_settings = get_account_settings(request)
+
         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))