diff --git a/lms/djangoapps/student_profile/test/test_views.py b/lms/djangoapps/student_profile/test/test_views.py index 7ce8174ee9..5299b51308 100644 --- a/lms/djangoapps/student_profile/test/test_views.py +++ b/lms/djangoapps/student_profile/test/test_views.py @@ -40,8 +40,9 @@ class LearnerProfileViewTest(UrlResetMixin, TestCase): Verify learner profile page context data. """ request = RequestFactory().get('/url') + request.user = self.user - context = learner_profile_context(self.user, self.USERNAME, self.user.is_staff, request.build_absolute_uri) + context = learner_profile_context(request, self.USERNAME, self.user.is_staff) self.assertEqual( context['data']['default_public_account_fields'], diff --git a/lms/djangoapps/student_profile/views.py b/lms/djangoapps/student_profile/views.py index 4020852015..7ef5787f9b 100644 --- a/lms/djangoapps/student_profile/views.py +++ b/lms/djangoapps/student_profile/views.py @@ -40,13 +40,13 @@ def learner_profile(request, username): try: return render_to_response( 'student_profile/learner_profile.html', - learner_profile_context(request.user, username, request.user.is_staff, request.build_absolute_uri) + learner_profile_context(request, username, request.user.is_staff) ) except (UserNotAuthorized, UserNotFound, ObjectDoesNotExist): raise Http404 -def learner_profile_context(logged_in_user, profile_username, user_is_staff, build_absolute_uri_func): +def learner_profile_context(request, profile_username, user_is_staff): """Context for the learner profile page. Args: @@ -62,14 +62,11 @@ def learner_profile_context(logged_in_user, profile_username, user_is_staff, bui ObjectDoesNotExist: the specified profile_username does not exist. """ profile_user = User.objects.get(username=profile_username) + logged_in_user = request.user own_profile = (logged_in_user.username == profile_username) - account_settings_data = get_account_settings(logged_in_user, profile_username) - # Account for possibly relative URLs. - for key, value in account_settings_data['profile_image'].items(): - if key.startswith(PROFILE_IMAGE_KEY_PREFIX): - account_settings_data['profile_image'][key] = build_absolute_uri_func(value) + account_settings_data = get_account_settings(request, profile_username) preferences_data = get_user_preferences(profile_user, profile_username) diff --git a/lms/djangoapps/teams/__init__.py b/lms/djangoapps/teams/__init__.py index e69de29bb2..0aaad373bf 100644 --- a/lms/djangoapps/teams/__init__.py +++ b/lms/djangoapps/teams/__init__.py @@ -0,0 +1,12 @@ +""" +Defines common methods shared by Teams classes +""" + +from django.conf import settings + + +def is_feature_enabled(course): + """ + Returns True if the teams feature is enabled. + """ + return settings.FEATURES.get('ENABLE_TEAMS', False) and course.teams_enabled diff --git a/lms/djangoapps/teams/plugins.py b/lms/djangoapps/teams/plugins.py index a613eb7740..439b693a5b 100644 --- a/lms/djangoapps/teams/plugins.py +++ b/lms/djangoapps/teams/plugins.py @@ -1,10 +1,9 @@ """ Definition of the course team feature. """ - from django.utils.translation import ugettext_noop from courseware.tabs import EnrolledTab -from .views import is_feature_enabled +from teams import is_feature_enabled class TeamsTab(EnrolledTab): diff --git a/lms/djangoapps/teams/serializers.py b/lms/djangoapps/teams/serializers.py index e75912117a..d9262ddbb2 100644 --- a/lms/djangoapps/teams/serializers.py +++ b/lms/djangoapps/teams/serializers.py @@ -1,13 +1,14 @@ """Defines serializers used by the Team API.""" - +from copy import deepcopy from django.contrib.auth.models import User from django.db.models import Count +from django.conf import settings from rest_framework import serializers from openedx.core.lib.api.serializers import CollapsedReferenceSerializer, PaginationSerializer from openedx.core.lib.api.fields import ExpandableField -from openedx.core.djangoapps.user_api.serializers import UserSerializer +from openedx.core.djangoapps.user_api.accounts.serializers import UserReadOnlySerializer from .models import CourseTeam, CourseTeamMembership @@ -17,6 +18,10 @@ class UserMembershipSerializer(serializers.ModelSerializer): Used for listing team members. """ + profile_configuration = deepcopy(settings.ACCOUNT_VISIBILITY_CONFIGURATION) + profile_configuration['shareable_fields'].append('url') + profile_configuration['public_fields'].append('url') + user = ExpandableField( collapsed_serializer=CollapsedReferenceSerializer( model_class=User, @@ -24,7 +29,7 @@ class UserMembershipSerializer(serializers.ModelSerializer): view_name='accounts_api', read_only=True, ), - expanded_serializer=UserSerializer(), + expanded_serializer=UserReadOnlySerializer(configuration=profile_configuration), ) class Meta(object): @@ -87,6 +92,10 @@ class CourseTeamCreationSerializer(serializers.ModelSerializer): class MembershipSerializer(serializers.ModelSerializer): """Serializes CourseTeamMemberships with information about both teams and users.""" + profile_configuration = deepcopy(settings.ACCOUNT_VISIBILITY_CONFIGURATION) + profile_configuration['shareable_fields'].append('url') + profile_configuration['public_fields'].append('url') + user = ExpandableField( collapsed_serializer=CollapsedReferenceSerializer( model_class=User, @@ -94,7 +103,7 @@ class MembershipSerializer(serializers.ModelSerializer): view_name='accounts_api', read_only=True, ), - expanded_serializer=UserSerializer(read_only=True) + expanded_serializer=UserReadOnlySerializer(configuration=profile_configuration) ) team = ExpandableField( collapsed_serializer=CollapsedReferenceSerializer( diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index b8c637a8f7..2e9787405f 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -110,7 +110,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): @classmethod def setUpClass(cls): super(TeamAPITestCase, cls).setUpClass() - teams_configuration = { + teams_configuration_1 = { 'topics': [ { @@ -124,9 +124,30 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): org='TestX', course='TS101', display_name='Test Course', - teams_configuration=teams_configuration + teams_configuration=teams_configuration_1 + ) + + teams_configuration_2 = { + 'topics': + [ + { + 'id': 'topic_5', + 'name': 'Other Interests', + 'description': 'Description for topic 5.' + }, + { + 'id': 'topic_6', + 'name': 'Public Profiles', + 'description': 'Description for topic 6.' + }, + ] + } + cls.test_course_2 = CourseFactory.create( + org='MIT', + course='6.002x', + display_name='Circuits', + teams_configuration=teams_configuration_2 ) - cls.test_course_2 = CourseFactory.create(org='MIT', course='6.002x', display_name='Circuits') def setUp(self): super(TeamAPITestCase, self).setUp() @@ -152,6 +173,15 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): username='student_enrolled_both_courses_other_team' ) + # Make this student have a public profile + self.create_and_enroll_student( + courses=[self.test_course_2], + username='student_enrolled_public_profile' + ) + profile = self.users['student_enrolled_public_profile'].profile + profile.year_of_birth = 1970 + profile.save() + # 'solar team' is intentionally lower case to test case insensitivity in name ordering self.test_team_1 = CourseTeamFactory.create( name=u'sólar team', @@ -162,11 +192,13 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): self.test_team_3 = CourseTeamFactory.create(name='Nuclear Team', course_id=self.test_course_1.id) self.test_team_4 = CourseTeamFactory.create(name='Coal Team', course_id=self.test_course_1.id, is_active=False) self.test_team_5 = CourseTeamFactory.create(name='Another Team', course_id=self.test_course_2.id) + self.test_team_6 = CourseTeamFactory.create( + name='Public Profile Team', + course_id=self.test_course_2.id, + topic_id='topic_6' + ) - for user, course in [ - ('staff', self.test_course_1), - ('course_staff', self.test_course_1), - ]: + for user, course in [('staff', self.test_course_1), ('course_staff', self.test_course_1)]: CourseEnrollment.enroll( self.users[user], course.id, check_access=True ) @@ -174,6 +206,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): self.test_team_1.add_user(self.users['student_enrolled']) self.test_team_3.add_user(self.users['student_enrolled_both_courses_other_team']) self.test_team_5.add_user(self.users['student_enrolled_both_courses_other_team']) + self.test_team_6.add_user(self.users['student_enrolled_public_profile']) def create_and_enroll_student(self, courses=None, username=None): """ Creates a new student and enrolls that student in the course. @@ -305,11 +338,18 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): **kwargs ) - def verify_expanded_user(self, user): + def verify_expanded_public_user(self, user): """Verifies that fields exist on the returned user json indicating that it is expanded.""" - for field in ['id', 'url', 'email', 'name', 'username', 'preferences']: + for field in ['username', 'url', 'bio', 'country', 'profile_image', 'time_zone', 'language_proficiencies']: self.assertIn(field, user) + def verify_expanded_private_user(self, user): + """Verifies that fields exist on the returned user json indicating that it is expanded.""" + for field in ['username', 'url', 'profile_image']: + self.assertIn(field, user) + for field in ['bio', 'country', 'time_zone', 'language_proficiencies']: + self.assertNotIn(field, user) + def verify_expanded_team(self, team): """Verifies that fields exist on the returned team json indicating that it is expanded.""" for field in ['id', 'name', 'is_active', 'course_id', 'topic_id', 'date_created', 'description']: @@ -347,7 +387,12 @@ class TestListTeamsAPI(TeamAPITestCase): self.verify_names({'course_id': 'no_such_course'}, 400) def test_filter_course_id(self): - self.verify_names({'course_id': self.test_course_2.id}, 200, ['Another Team'], user='staff') + self.verify_names( + {'course_id': self.test_course_2.id}, + 200, + ['Another Team', 'Public Profile Team'], + user='staff' + ) def test_filter_topic_id(self): self.verify_names({'course_id': self.test_course_1.id, 'topic_id': 'topic_0'}, 200, [u'sólar team']) @@ -386,9 +431,22 @@ class TestListTeamsAPI(TeamAPITestCase): self.assertIsNone(result['next']) self.assertIsNotNone(result['previous']) - def test_expand_user(self): + def test_expand_private_user(self): + # Use the default user which is already private because to year_of_birth is set result = self.get_teams_list(200, {'expand': 'user', 'topic_id': 'topic_0'}) - self.verify_expanded_user(result['results'][0]['membership'][0]['user']) + self.verify_expanded_private_user(result['results'][0]['membership'][0]['user']) + + def test_expand_public_user(self): + result = self.get_teams_list( + 200, + { + 'expand': 'user', + 'topic_id': 'topic_6', + 'course_id': self.test_course_2.id + }, + user='student_enrolled_public_profile' + ) + self.verify_expanded_public_user(result['results'][0]['membership'][0]['user']) @ddt.ddt @@ -515,9 +573,19 @@ class TestDetailTeamAPI(TeamAPITestCase): def test_does_not_exist(self): self.get_team_detail('no_such_team', 404) - def test_expand_user(self): + def test_expand_private_user(self): + # Use the default user which is already private because to year_of_birth is set result = self.get_team_detail(self.test_team_1.team_id, 200, {'expand': 'user'}) - self.verify_expanded_user(result['membership'][0]['user']) + self.verify_expanded_private_user(result['membership'][0]['user']) + + def test_expand_public_user(self): + result = self.get_team_detail( + self.test_team_6.team_id, + 200, + {'expand': 'user'}, + user='student_enrolled_public_profile' + ) + self.verify_expanded_public_user(result['membership'][0]['user']) @ddt.ddt @@ -715,9 +783,18 @@ class TestListMembershipAPI(TeamAPITestCase): def test_bad_team_id(self): self.get_membership_list(404, {'team_id': 'no_such_team'}) - def test_expand_user(self): + def test_expand_private_user(self): + # Use the default user which is already private because to year_of_birth is set result = self.get_membership_list(200, {'team_id': self.test_team_1.team_id, 'expand': 'user'}) - self.verify_expanded_user(result['results'][0]['user']) + self.verify_expanded_private_user(result['results'][0]['user']) + + def test_expand_public_user(self): + result = self.get_membership_list( + 200, + {'team_id': self.test_team_6.team_id, 'expand': 'user'}, + user='student_enrolled_public_profile' + ) + self.verify_expanded_public_user(result['results'][0]['user']) def test_expand_team(self): result = self.get_membership_list(200, {'team_id': self.test_team_1.team_id, 'expand': 'team'}) @@ -842,14 +919,25 @@ class TestDetailMembershipAPI(TeamAPITestCase): 404 ) - def test_expand_user(self): + def test_expand_private_user(self): + # Use the default user which is already private because to year_of_birth is set result = self.get_membership_detail( self.test_team_1.team_id, self.users['student_enrolled'].username, 200, {'expand': 'user'} ) - self.verify_expanded_user(result['user']) + self.verify_expanded_private_user(result['user']) + + def test_expand_public_user(self): + result = self.get_membership_detail( + self.test_team_6.team_id, + self.users['student_enrolled_public_profile'].username, + 200, + {'expand': 'user'}, + user='student_enrolled_public_profile' + ) + self.verify_expanded_public_user(result['user']) def test_expand_team(self): result = self.get_membership_detail( diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index eecbf62297..6dbab0faec 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -1,13 +1,11 @@ """HTTP endpoints for the Teams API.""" from django.shortcuts import render_to_response -from courseware.courses import get_course_with_access, has_access from django.http import Http404 from django.conf import settings from django.core.paginator import Paginator from django.views.generic.base import View import newrelic.agent - from rest_framework.generics import GenericAPIView from rest_framework.response import Response from rest_framework.reverse import reverse @@ -18,16 +16,11 @@ from rest_framework.authentication import ( ) from rest_framework import status from rest_framework import permissions - from django.db.models import Count from django.contrib.auth.models import User from django_countries import countries from django.utils.translation import ugettext as _ from django.utils.translation import ugettext_noop - -from student.models import CourseEnrollment, CourseAccessRole -from student.roles import CourseStaffRole - from openedx.core.lib.api.parsers import MergePatchParser from openedx.core.lib.api.permissions import IsStaffOrReadOnly from openedx.core.lib.api.view_utils import ( @@ -37,14 +30,15 @@ from openedx.core.lib.api.view_utils import ( ExpandableFieldViewMixin ) from openedx.core.lib.api.serializers import PaginationSerializer - from xmodule.modulestore.django import modulestore - from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from courseware.courses import get_course_with_access, has_access +from student.models import CourseEnrollment, CourseAccessRole +from student.roles import CourseStaffRole from django_comment_client.utils import has_discussion_privileges - +from teams import is_feature_enabled from .models import CourseTeam, CourseTeamMembership from .serializers import ( CourseTeamSerializer, @@ -58,7 +52,6 @@ from .serializers import ( from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam -# Constants TEAM_MEMBERSHIPS_PER_PAGE = 2 TOPICS_PER_PAGE = 12 @@ -120,13 +113,6 @@ class TeamsDashboardView(View): return render_to_response("teams/teams.html", context) -def is_feature_enabled(course): - """ - Returns True if the teams feature is enabled. - """ - return settings.FEATURES.get('ENABLE_TEAMS', False) and course.teams_enabled - - def has_team_api_access(user, course_key, access_username=None): """Returns True if the user has access to the Team API for the course given by `course_key`. The user must either be enrolled in the course, diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index e96ad28349..80b008ede6 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -1482,7 +1482,9 @@ class TestSubmitPhotosForVerification(TestCase): AssertionError """ - account_settings = get_account_settings(self.user) + request = RequestFactory().get('/url') + request.user = self.user + account_settings = get_account_settings(request) self.assertEqual(account_settings['name'], full_name) def _get_post_data(self): diff --git a/lms/envs/common.py b/lms/envs/common.py index efc358446f..53ed67810b 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2517,6 +2517,25 @@ ACCOUNT_VISIBILITY_CONFIGURATION = { 'username', 'profile_image', ], + + # The list of account fields that are visible only to staff and users viewing their own profiles + "admin_fields": [ + "username", + "email", + "date_joined", + "is_active", + "bio", + "country", + "profile_image", + "language_proficiencies", + "name", + "gender", + "goals", + "year_of_birth", + "level_of_education", + "mailing_address", + "requires_parental_consent", + ] } # E-Commerce API Configuration diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index d04709282c..532c139a7a 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/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]) diff --git a/openedx/core/djangoapps/user_api/accounts/image_helpers.py b/openedx/core/djangoapps/user_api/accounts/image_helpers.py index 48dde39941..f5058a0d9c 100644 --- a/openedx/core/djangoapps/user_api/accounts/image_helpers.py +++ b/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(): diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py index dcca026227..180f8ea208 100644 --- a/openedx/core/djangoapps/user_api/accounts/serializers.py +++ b/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) 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 84ed42486f..2cad671c49 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/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): 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 fbc32382c7..8d021d7dd2 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/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( diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index e05c937de5..31c14ac66c 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/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) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 79e8918340..5c53bcb4eb 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/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))