From 023e6ec8cdbca25e1d3dec662302be2e5c1b8a72 Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 20 Feb 2015 13:35:29 -0500 Subject: [PATCH 1/7] GET method for user account API. --- lms/djangoapps/mobile_api/testutils.py | 4 +- lms/djangoapps/mobile_api/utils.py | 10 +- lms/urls.py | 2 + .../djangoapps/user_api/accounts/__init__.py | 0 .../user_api/accounts/serializers.py | 26 ++++ .../user_api/accounts/tests/test_views.py | 111 ++++++++++++++++++ .../core/djangoapps/user_api/accounts/urls.py | 14 +++ .../djangoapps/user_api/accounts/views.py | 101 ++++++++++++++++ openedx/core/lib/api/permissions.py | 24 ++++ 9 files changed, 282 insertions(+), 10 deletions(-) create mode 100644 openedx/core/djangoapps/user_api/accounts/__init__.py create mode 100644 openedx/core/djangoapps/user_api/accounts/serializers.py create mode 100644 openedx/core/djangoapps/user_api/accounts/tests/test_views.py create mode 100644 openedx/core/djangoapps/user_api/accounts/urls.py create mode 100644 openedx/core/djangoapps/user_api/accounts/views.py diff --git a/lms/djangoapps/mobile_api/testutils.py b/lms/djangoapps/mobile_api/testutils.py index c5bc3f2872..09e2cab339 100644 --- a/lms/djangoapps/mobile_api/testutils.py +++ b/lms/djangoapps/mobile_api/testutils.py @@ -105,7 +105,7 @@ class MobileAuthUserTestMixin(MobileAuthTestMixin): """ def test_invalid_user(self): self.login_and_enroll() - self.api_response(expected_response_code=403, username='no_user') + self.api_response(expected_response_code=404, username='no_user') def test_other_user(self): # login and enroll as the test user @@ -120,7 +120,7 @@ class MobileAuthUserTestMixin(MobileAuthTestMixin): # now login and call the API as the test user self.login() - self.api_response(expected_response_code=403, username=other.username) + self.api_response(expected_response_code=404, username=other.username) @ddt.ddt diff --git a/lms/djangoapps/mobile_api/utils.py b/lms/djangoapps/mobile_api/utils.py index cd7720f8a3..0b15facf48 100644 --- a/lms/djangoapps/mobile_api/utils.py +++ b/lms/djangoapps/mobile_api/utils.py @@ -11,6 +11,7 @@ from rest_framework.authentication import OAuth2Authentication, SessionAuthentic from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore from courseware.courses import get_course_with_access +from openedx.core.lib.api.permissions import IsUserInUrl def mobile_course_access(depth=0, verify_enrolled=True): @@ -43,13 +44,6 @@ def mobile_view(is_user=False): """ Function and class decorator that abstracts the authentication and permission checks for mobile api views. """ - class IsUser(permissions.BasePermission): - """ - Permission that checks to see if the request user matches the user in the URL. - """ - def has_permission(self, request, view): - return request.user.username == request.parser_context.get('kwargs', {}).get('username', None) - def _decorator(func_or_class): """ Requires either OAuth2 or Session-based authentication. @@ -58,6 +52,6 @@ def mobile_view(is_user=False): func_or_class.authentication_classes = (OAuth2Authentication, SessionAuthentication) func_or_class.permission_classes = (permissions.IsAuthenticated,) if is_user: - func_or_class.permission_classes += (IsUser,) + func_or_class.permission_classes += (IsUserInUrl,) return func_or_class return _decorator diff --git a/lms/urls.py b/lms/urls.py index a9ac601513..af4a6071b8 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -61,6 +61,8 @@ urlpatterns = ('', # nopep8 url(r'^user_api/', include('openedx.core.djangoapps.user_api.urls')), + url(r'^api/user/', include('openedx.core.djangoapps.user_api.accounts.urls')), + url(r'^notifier_api/', include('notifier_api.urls')), url(r'^lang_pref/', include('lang_pref.urls')), diff --git a/openedx/core/djangoapps/user_api/accounts/__init__.py b/openedx/core/djangoapps/user_api/accounts/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py new file mode 100644 index 0000000000..710450af67 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/serializers.py @@ -0,0 +1,26 @@ +from rest_framework import serializers +from django.contrib.auth.models import User +from student.models import UserProfile + + +class AccountUserSerializer(serializers.HyperlinkedModelSerializer): + """ + Class that serializes the portion of User model needed for account information. + """ + class Meta: + model = User + fields = ("username", "email", "date_joined") + read_only_fields = ("username", "email", "date_joined") + + +class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer): + """ + Class that serializes the portion of UserProfile model needed for account information. + """ + class Meta: + model = UserProfile + fields = ( + "name", "gender", "goals", "year_of_birth", "level_of_education", "language", "city", "country", + "mailing_address" + ) + read_only_fields = ("name",) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py new file mode 100644 index 0000000000..ebb9e85d22 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -0,0 +1,111 @@ +import unittest +import ddt + +from django.test import TestCase +from django.core.urlresolvers import reverse +from django.conf import settings +from rest_framework.test import APITestCase, APIClient + +from student.tests.factories import UserFactory +from student.models import UserProfile + +TEST_PASSWORD = "test" + +@ddt.ddt +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +class TestAccountAPI(APITestCase): + USERNAME = "Christina" + EMAIL = "christina@example.com" + PASSWORD = TEST_PASSWORD + + BAD_USERNAME = "Bad" + BAD_EMAIL = "bad@example.com" + BAD_PASSWORD = TEST_PASSWORD + + STAFF_USERNAME = "Staff" + STAFF_EMAIL = "staff@example.com" + STAFF_PASSWORD = TEST_PASSWORD + + def setUp(self): + super(TestAccountAPI, self).setUp() + + self.anonymous_client = APIClient() + + self.bad_user = UserFactory.create(password=TEST_PASSWORD) + self.bad_client = APIClient() + + self.staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD) + self.staff_client = APIClient() + + self.user = UserFactory.create(password=TEST_PASSWORD) + # Create some test profile values. + legacy_profile = UserProfile.objects.get(id=self.user.id) + legacy_profile.city = "Indi" + legacy_profile.country = "US" + legacy_profile.year_of_birth = 1900 + legacy_profile.level_of_education = "m" + legacy_profile.goals = "world peace" + legacy_profile.mailing_address = "North Pole" + legacy_profile.save() + + self.accounts_base_uri = reverse("accounts_api", kwargs={'username': self.user.username}) + + def test_get_account_anonymous_user(self): + response = self.anonymous_client.get(self.accounts_base_uri) + self.assert_status_code(401, response) + + def test_get_account_bad_user(self): + self.bad_client.login(username=self.bad_user.username, password=TEST_PASSWORD) + response = self.bad_client.get(self.accounts_base_uri) + self.assert_status_code(404, response) + + @ddt.data( + ("client", "user"), + ("staff_client", "staff_user"), + ) + @ddt.unpack + def test_get_account(self, api_client, user): + client = self.login_client(api_client, user) + + response = client.get(self.accounts_base_uri) + self.assert_status_code(200, response) + data = response.data + self.assertEqual(12, len(data)) + self.assertEqual(self.user.username, data["username"]) + # TODO: should we rename this "full_name"? + self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) + self.assertEqual("Indi", data["city"]) + self.assertEqual("US", data["country"]) + # TODO: what should the format of this be? + self.assertEqual("", data["language"]) + self.assertEqual("m", data["gender"]) + self.assertEqual(1900, data["year_of_birth"]) + self.assertEqual("m", data["level_of_education"]) + self.assertEqual("world peace", data["goals"]) + self.assertEqual("North Pole", data['mailing_address']) + self.assertEqual(self.user.email, data["email"]) + self.assertIsNotNone(data["date_joined"]) + + @ddt.data( + ("client", "user"), + ("staff_client", "staff_user"), + ) + @ddt.unpack + def test_patch_account(self, api_client, user): + client = self.login_client(api_client, user) + response = client.patch(self.accounts_base_uri, data={"usernamae": "willbeignored", "gender": "f"}) + self.assert_status_code(200, response) + data = response.data + # Note that username is read-only, so passing it in patch is ignored. We want to change this behavior so it throws an exception. + self.assertEqual(self.user.username, data["username"]) + self.assertEqual("f", data["gender"]) + + def assert_status_code(self, expected_status_code, response): + """Assert that the given response has the expected status code""" + self.assertEqual(expected_status_code, response.status_code) + + def login_client(self, api_client, user): + client = getattr(self, api_client) + user = getattr(self, user) + client.login(username=user.username, password=TEST_PASSWORD) + return client diff --git a/openedx/core/djangoapps/user_api/accounts/urls.py b/openedx/core/djangoapps/user_api/accounts/urls.py new file mode 100644 index 0000000000..05d94ec7a2 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/urls.py @@ -0,0 +1,14 @@ +from .views import AccountView + +from django.conf.urls import include, patterns, url + +USERNAME_PATTERN = r'(?P[\w.+-]+)' + +urlpatterns = patterns( + '', + url( + r'^v0/accounts/' + USERNAME_PATTERN + '$', + AccountView.as_view(), + name="accounts_api" + ) +) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py new file mode 100644 index 0000000000..b3f2564041 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -0,0 +1,101 @@ +from rest_framework.views import APIView +from django.core.exceptions import ObjectDoesNotExist +from django.contrib.auth.models import User +from rest_framework.response import Response +from rest_framework import status + +from student.models import UserProfile +from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer, AccountUserSerializer +from openedx.core.lib.api.permissions import IsUserInUrlOrStaff + +from rest_framework.authentication import OAuth2Authentication, SessionAuthentication +from rest_framework import permissions + + +class AccountView(APIView): + """ + **Use Cases** + + Get the user's account information. + + **Example Requests**: + + GET /api/user/v0/accounts/{username}/ + + **Response Values** + + * username: The username associated with the account (not editable). + + * name: The full name of the user (not editable through this API). + + * email: The email for the user (not editable through this API). + + * date_joined: The date this account was created (not editable). + + * gender: null, "m", "f", or "o": + + * year_of_birth: null or integer year: + + * level_of_education: null or one of the following choices: + + * "p" signifying "Doctorate" + * "m" signifying "Master's or professional degree" + * "b" signifying "Bachelor's degree" + * "a" signifying "Associate's degree" + * "hs" signifying "Secondary/high school" + * "jhs" signifying "Junior secondary/junior high/middle school" + * "el" signifying "Elementary/primary school" + * "none" signifying "None" + * "o" signifying "Other" + + * language: null or name of preferred language + + * city: null or name of city + + * country: null or a Country corresponding to one of the ISO 3166-1 countries + + * mailing_address: null or textual representation of mailing address + + * goals: null or textual representation of goals + + """ + authentication_classes = (OAuth2Authentication, SessionAuthentication) + permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff) + + def get(self, request, username): + """ + GET /api/user/v0/accounts/{username}/ + """ + existing_user, existing_user_profile = self._get_user_and_profile(username) + user_serializer = AccountUserSerializer(existing_user) + legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile) + + return Response(dict(user_serializer.data, **legacy_profile_serializer.data)) + + def patch(self, request, username): + """ + PATCH /api/user/v0/accounts/{username}/ + """ + existing_user, existing_user_profile = self._get_user_and_profile(username) + + user_serializer = AccountUserSerializer(existing_user, data=request.DATA) + user_serializer.is_valid() + user_serializer.save() + + legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=request.DATA) + legacy_profile_serializer.is_valid() + legacy_profile_serializer.save() + + return Response(dict(user_serializer.data, **legacy_profile_serializer.data)) + + def _get_user_and_profile(self, username): + """ + Helper method to return the legacy user and profile objects based on username. + """ + try: + existing_user = User.objects.get(username=username) + except ObjectDoesNotExist: + return Response({}, status=status.HTTP_404_NOT_FOUND) + existing_user_profile = UserProfile.objects.get(id=existing_user.id) + + return existing_user, existing_user_profile diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py index dca589d88b..2b7c122665 100644 --- a/openedx/core/lib/api/permissions.py +++ b/openedx/core/lib/api/permissions.py @@ -1,6 +1,7 @@ from django.conf import settings from rest_framework import permissions from rest_framework.exceptions import PermissionDenied +from django.http import Http404 class ApiKeyHeaderPermission(permissions.BasePermission): @@ -31,3 +32,26 @@ class IsAuthenticatedOrDebug(permissions.BasePermission): user = getattr(request, 'user', None) return user and user.is_authenticated() + + +class IsUserInUrl(permissions.BasePermission): + """ + Permission that checks to see if the request user matches the user in the URL. + """ + def has_permission(self, request, view): + # Return a 404 instead of a 403 (Unauthorized). If one user is looking up + # other users, do not let them deduce the existence of an account. + if request.user.username != request.parser_context.get('kwargs', {}).get('username', None): + raise Http404() + return True + + +class IsUserInUrlOrStaff(IsUserInUrl): + """ + Permission that checks to see if the request user matches the user in the URL or has is_staff access. + """ + def has_permission(self, request, view): + if request.user.is_staff: + return True + + return super(IsUserInUrlOrStaff, self).has_permission(request, view) From 35e80a3a60551d678b9249848af9df5c76da7df9 Mon Sep 17 00:00:00 2001 From: cahrens Date: Fri, 20 Feb 2015 16:44:10 -0500 Subject: [PATCH 2/7] Throw 400 in patch if read-only fields specified. --- .../user_api/accounts/tests/test_views.py | 27 ++++++++++++++++--- .../djangoapps/user_api/accounts/views.py | 15 +++++++++-- 2 files changed, 37 insertions(+), 5 deletions(-) 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 ebb9e85d22..5e35971a6e 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -93,13 +93,34 @@ class TestAccountAPI(APITestCase): @ddt.unpack def test_patch_account(self, api_client, user): client = self.login_client(api_client, user) - response = client.patch(self.accounts_base_uri, data={"usernamae": "willbeignored", "gender": "f"}) + response = client.patch(self.accounts_base_uri, data={"gender": "f"}) self.assert_status_code(200, response) data = response.data - # Note that username is read-only, so passing it in patch is ignored. We want to change this behavior so it throws an exception. - self.assertEqual(self.user.username, data["username"]) self.assertEqual("f", data["gender"]) + @ddt.data( + ("client", "user"), + ("staff_client", "staff_user"), + ) + @ddt.unpack + def test_patch_account_noneditable(self, api_client, user): + client = self.login_client(api_client, user) + + for field_name in ["username", "email", "date_joined", "name"]: + response = client.patch(self.accounts_base_uri, data={field_name: "willbeignored", "gender": "f"}) + self.assert_status_code(400, response) + data = response.data + self.assertEqual("The following fields are not editable: " + field_name, data["message"]) + + # Make sure that gender did not change. + response = client.get(self.accounts_base_uri) + self.assertEqual("m", response.data["gender"]) + + # Test error message with multiple read-only items + response = client.patch(self.accounts_base_uri, data={"username": "willbeignored", "email": "xx"}) + self.assert_status_code(400, response) + self.assertEqual("The following fields are not editable: username, email", response.data["message"]) + def assert_status_code(self, expected_status_code, response): """Assert that the given response has the expected status code""" self.assertEqual(expected_status_code, response.status_code) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index b3f2564041..39174fcc3b 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -10,6 +10,7 @@ from openedx.core.lib.api.permissions import IsUserInUrlOrStaff from rest_framework.authentication import OAuth2Authentication, SessionAuthentication from rest_framework import permissions +from rest_framework import status class AccountView(APIView): @@ -78,11 +79,21 @@ class AccountView(APIView): """ existing_user, existing_user_profile = self._get_user_and_profile(username) - user_serializer = AccountUserSerializer(existing_user, data=request.DATA) + # Check for fields that are not editable. Marking them read-only causes them to be ignored, but we wish to 400. + update = request.DATA + read_only_fields = set(update.keys()).intersection( + AccountUserSerializer.Meta.read_only_fields + AccountLegacyProfileSerializer.Meta.read_only_fields + ) + if read_only_fields: + response_data = {} + response_data['message'] = "The following fields are not editable: " + ", ".join(str(e) for e in read_only_fields) + return Response(response_data, status=status.HTTP_400_BAD_REQUEST) + + user_serializer = AccountUserSerializer(existing_user, data=update) user_serializer.is_valid() user_serializer.save() - legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=request.DATA) + legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=update) legacy_profile_serializer.is_valid() legacy_profile_serializer.save() From ef8b4394cfdc3d41e2848f5d2171360d2f4d4e86 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 23 Feb 2015 14:33:14 -0500 Subject: [PATCH 3/7] Rest of PATCH implementation. --- .../user_api/accounts/tests/test_views.py | 87 +++++++++++++------ .../djangoapps/user_api/accounts/views.py | 65 ++++++++++---- 2 files changed, 112 insertions(+), 40 deletions(-) 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 5e35971a6e..a0ecabd9c4 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -14,17 +14,6 @@ TEST_PASSWORD = "test" @ddt.ddt @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class TestAccountAPI(APITestCase): - USERNAME = "Christina" - EMAIL = "christina@example.com" - PASSWORD = TEST_PASSWORD - - BAD_USERNAME = "Bad" - BAD_EMAIL = "bad@example.com" - BAD_PASSWORD = TEST_PASSWORD - - STAFF_USERNAME = "Staff" - STAFF_EMAIL = "staff@example.com" - STAFF_PASSWORD = TEST_PASSWORD def setUp(self): super(TestAccountAPI, self).setUp() @@ -45,7 +34,6 @@ class TestAccountAPI(APITestCase): legacy_profile.year_of_birth = 1900 legacy_profile.level_of_education = "m" legacy_profile.goals = "world peace" - legacy_profile.mailing_address = "North Pole" legacy_profile.save() self.accounts_base_uri = reverse("accounts_api", kwargs={'username': self.user.username}) @@ -82,21 +70,61 @@ class TestAccountAPI(APITestCase): self.assertEqual(1900, data["year_of_birth"]) self.assertEqual("m", data["level_of_education"]) self.assertEqual("world peace", data["goals"]) - self.assertEqual("North Pole", data['mailing_address']) + # Default value for mailing address is None, nothing assigned in setup. + self.assertIsNone(None, data['mailing_address']) self.assertEqual(self.user.email, data["email"]) self.assertIsNotNone(data["date_joined"]) @ddt.data( - ("client", "user"), - ("staff_client", "staff_user"), + ( + "client", "user", "gender", "f", "not a gender", + "Select a valid choice. not a gender is not one of the available choices." + ), + ( + "client", "user", "level_of_education", "none", "x", + "Select a valid choice. x is not one of the available choices." + ), + ("client", "user", "country", "GB", "UK", "Select a valid choice. UK is not one of the available choices."), + ("client", "user", "year_of_birth", 2009, "not_an_int", "Enter a whole number."), + ("client", "user", "city", "Knoxville"), + ("client", "user", "language", "Creole"), + ("client", "user", "goals", "Smell the roses"), + ("client", "user", "mailing_address", "Sesame Street"), + # All of the fields can be edited by is_staff, but iterating through all of them again seems like overkill. + # Just test a representative field. + ("staff_client", "staff_user", "goals", "Smell the roses"), ) @ddt.unpack - def test_patch_account(self, api_client, user): + def test_patch_account( + self, api_client, user, field, value, fails_validation_value=None, developer_validation_message=None + ): client = self.login_client(api_client, user) - response = client.patch(self.accounts_base_uri, data={"gender": "f"}) - self.assert_status_code(200, response) - data = response.data - self.assertEqual("f", data["gender"]) + patch_response = client.patch(self.accounts_base_uri, data={field: value}) + self.assert_status_code(204, patch_response) + + get_response = client.get(self.accounts_base_uri) + self.assert_status_code(200, get_response) + self.assertEqual(value, get_response.data[field]) + + if fails_validation_value: + error_response = client.patch(self.accounts_base_uri, data={field: fails_validation_value}) + self.assert_status_code(400, error_response) + self.assertEqual( + "Value '{0}' is not valid for field '{1}'.".format(fails_validation_value, field), + error_response.data["field_errors"][field]["user_message"] + ) + self.assertEqual( + developer_validation_message, + error_response.data["field_errors"][field]["developer_message"] + ) + else: + # If there are no values that would fail validation, then empty string should be supported. + patch_response = client.patch(self.accounts_base_uri, data={field: ""}) + self.assert_status_code(204, patch_response) + + get_response = client.get(self.accounts_base_uri) + self.assert_status_code(200, get_response) + self.assertEqual("", get_response.data[field]) @ddt.data( ("client", "user"), @@ -106,20 +134,29 @@ class TestAccountAPI(APITestCase): def test_patch_account_noneditable(self, api_client, user): client = self.login_client(api_client, user) + def verify_error_response(field_name, data): + self.assertEqual( + "This field is not editable via this API", data["field_errors"][field_name]["developer_message"] + ) + self.assertEqual( + "Field '{0}' cannot be edited.".format(field_name), data["field_errors"][field_name]["user_message"] + ) + for field_name in ["username", "email", "date_joined", "name"]: - response = client.patch(self.accounts_base_uri, data={field_name: "willbeignored", "gender": "f"}) + response = client.patch(self.accounts_base_uri, data={field_name: "will_error", "gender": "f"}) self.assert_status_code(400, response) - data = response.data - self.assertEqual("The following fields are not editable: " + field_name, data["message"]) + verify_error_response(field_name, response.data) # Make sure that gender did not change. response = client.get(self.accounts_base_uri) self.assertEqual("m", response.data["gender"]) # Test error message with multiple read-only items - response = client.patch(self.accounts_base_uri, data={"username": "willbeignored", "email": "xx"}) + response = client.patch(self.accounts_base_uri, data={"username": "will_error", "email": "xx"}) self.assert_status_code(400, response) - self.assertEqual("The following fields are not editable: username, email", response.data["message"]) + self.assertEqual(2, len(response.data["field_errors"])) + verify_error_response("username", response.data) + verify_error_response("email", response.data) def assert_status_code(self, expected_status_code, response): """Assert that the given response has the expected status code""" diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 39174fcc3b..79912a7bcc 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1,29 +1,31 @@ -from rest_framework.views import APIView from django.core.exceptions import ObjectDoesNotExist from django.contrib.auth.models import User +from django.utils.translation import ugettext as _ + +from rest_framework.views import APIView from rest_framework.response import Response from rest_framework import status +from rest_framework.authentication import OAuth2Authentication, SessionAuthentication +from rest_framework import permissions from student.models import UserProfile from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer, AccountUserSerializer from openedx.core.lib.api.permissions import IsUserInUrlOrStaff -from rest_framework.authentication import OAuth2Authentication, SessionAuthentication -from rest_framework import permissions -from rest_framework import status - class AccountView(APIView): """ **Use Cases** - Get the user's account information. + Get or update the user's account information. **Example Requests**: GET /api/user/v0/accounts/{username}/ - **Response Values** + PATCH /api/user/v0/accounts/{username}/ + + **Response Values for GET** * username: The username associated with the account (not editable). @@ -59,6 +61,10 @@ class AccountView(APIView): * goals: null or textual representation of goals + **Response for PATCH** + + Returns a 204 status if successful, with no additional content. + """ authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff) @@ -85,19 +91,25 @@ class AccountView(APIView): AccountUserSerializer.Meta.read_only_fields + AccountLegacyProfileSerializer.Meta.read_only_fields ) if read_only_fields: - response_data = {} - response_data['message'] = "The following fields are not editable: " + ", ".join(str(e) for e in read_only_fields) + field_errors = {} + for read_only_field in read_only_fields: + field_errors[read_only_field] = { + "developer_message": "This field is not editable via this API", + "user_message": _("Field '{field_name}' cannot be edited.".format(field_name=read_only_field)) + } + response_data = {"field_errors": field_errors} return Response(response_data, status=status.HTTP_400_BAD_REQUEST) user_serializer = AccountUserSerializer(existing_user, data=update) - user_serializer.is_valid() - user_serializer.save() - legacy_profile_serializer = AccountLegacyProfileSerializer(existing_user_profile, data=update) - legacy_profile_serializer.is_valid() - legacy_profile_serializer.save() - return Response(dict(user_serializer.data, **legacy_profile_serializer.data)) + for serializer in user_serializer, legacy_profile_serializer: + validation_errors = self._get_validation_errors(update, serializer) + if validation_errors: + return Response(validation_errors, status=status.HTTP_400_BAD_REQUEST) + serializer.save() + + return Response(status=status.HTTP_204_NO_CONTENT) def _get_user_and_profile(self, username): """ @@ -110,3 +122,26 @@ class AccountView(APIView): existing_user_profile = UserProfile.objects.get(id=existing_user.id) return existing_user, existing_user_profile + + def _get_validation_errors(self, update, serializer): + """ + Helper method that returns any validation errors that are present. + """ + validation_errors = {} + if not serializer.is_valid(): + field_errors = {} + errors = serializer.errors + for key, value in errors.iteritems(): + if isinstance(value, list) and len(value) > 0: + developer_message = value[0] + else: + developer_message = "Invalid value: {field_value}'".format(field_value=update[key]) + field_errors[key] = { + "developer_message": developer_message, + "user_message": _("Value '{field_value}' is not valid for field '{field_name}'.".format( + field_value=update[key], field_name=key) + ) + } + + validation_errors['field_errors'] = field_errors + return validation_errors \ No newline at end of file From 8d030974ec2db0d2200c7f44581c9d77ece3c314 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 23 Feb 2015 14:41:53 -0500 Subject: [PATCH 4/7] Typo (squash) --- openedx/core/djangoapps/user_api/accounts/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a0ecabd9c4..ab823d927a 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -71,7 +71,7 @@ class TestAccountAPI(APITestCase): self.assertEqual("m", data["level_of_education"]) self.assertEqual("world peace", data["goals"]) # Default value for mailing address is None, nothing assigned in setup. - self.assertIsNone(None, data['mailing_address']) + self.assertIsNone(data['mailing_address']) self.assertEqual(self.user.email, data["email"]) self.assertIsNotNone(data["date_joined"]) From 09c607c67c9bc1043b1b1a8aa0a9028bd13a0296 Mon Sep 17 00:00:00 2001 From: cahrens Date: Mon, 23 Feb 2015 15:18:15 -0500 Subject: [PATCH 5/7] Get profile object from user, clarify null or "". --- .../core/djangoapps/user_api/accounts/views.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 79912a7bcc..47c15d9734 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -27,19 +27,19 @@ class AccountView(APIView): **Response Values for GET** - * username: The username associated with the account (not editable). + * username: username associated with the account (not editable) - * name: The full name of the user (not editable through this API). + * name: full name of the user (not editable through this API) - * email: The email for the user (not editable through this API). + * email: email for the user (not editable through this API) - * date_joined: The date this account was created (not editable). + * date_joined: date this account was created (not editable) - * gender: null, "m", "f", or "o": + * gender: null or "" (not set), "m", "f", or "o" - * year_of_birth: null or integer year: + * year_of_birth: null or integer year - * level_of_education: null or one of the following choices: + * level_of_education: null or "" (not set), or one of the following choices: * "p" signifying "Doctorate" * "m" signifying "Master's or professional degree" @@ -55,7 +55,7 @@ class AccountView(APIView): * city: null or name of city - * country: null or a Country corresponding to one of the ISO 3166-1 countries + * country: null or "" (not set), or a Country corresponding to one of the ISO 3166-1 countries * mailing_address: null or textual representation of mailing address @@ -119,7 +119,7 @@ class AccountView(APIView): existing_user = User.objects.get(username=username) except ObjectDoesNotExist: return Response({}, status=status.HTTP_404_NOT_FOUND) - existing_user_profile = UserProfile.objects.get(id=existing_user.id) + existing_user_profile = UserProfile.objects.get(user=existing_user) return existing_user, existing_user_profile From 450c0e34f57b1ed2f230612a52d4d213e9146048 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 24 Feb 2015 11:32:28 -0500 Subject: [PATCH 6/7] Enforce content_type of 'application/merge-patch+json' for merge patch. --- .../user_api/accounts/tests/test_views.py | 60 ++++++++++++++----- .../djangoapps/user_api/accounts/views.py | 12 +++- openedx/core/lib/api/parsers.py | 8 +++ 3 files changed, 63 insertions(+), 17 deletions(-) create mode 100644 openedx/core/lib/api/parsers.py 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 ab823d927a..2adc4d4076 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -1,5 +1,6 @@ import unittest import ddt +import json from django.test import TestCase from django.core.urlresolvers import reverse @@ -20,8 +21,8 @@ class TestAccountAPI(APITestCase): self.anonymous_client = APIClient() - self.bad_user = UserFactory.create(password=TEST_PASSWORD) - self.bad_client = APIClient() + self.different_user = UserFactory.create(password=TEST_PASSWORD) + self.different_client = APIClient() self.staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD) self.staff_client = APIClient() @@ -39,12 +40,18 @@ class TestAccountAPI(APITestCase): self.accounts_base_uri = reverse("accounts_api", kwargs={'username': self.user.username}) def test_get_account_anonymous_user(self): + """ + Test that an anonymous client (not logged in) cannot call get. + """ response = self.anonymous_client.get(self.accounts_base_uri) self.assert_status_code(401, response) - def test_get_account_bad_user(self): - self.bad_client.login(username=self.bad_user.username, password=TEST_PASSWORD) - response = self.bad_client.get(self.accounts_base_uri) + def test_get_account_different_user(self): + """ + Test that a client (logged in) cannot get the account information for a different client. + """ + self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) + response = self.different_client.get(self.accounts_base_uri) self.assert_status_code(404, response) @ddt.data( @@ -53,6 +60,10 @@ class TestAccountAPI(APITestCase): ) @ddt.unpack def test_get_account(self, api_client, user): + """ + Test that a client (logged in) can get her own account information. Also verifies that a "is_staff" + user can get the account information for other users. + """ client = self.login_client(api_client, user) response = client.get(self.accounts_base_uri) @@ -98,17 +109,18 @@ class TestAccountAPI(APITestCase): def test_patch_account( self, api_client, user, field, value, fails_validation_value=None, developer_validation_message=None ): + """ + Test the behavior of patch, when using the correct content_type. + """ client = self.login_client(api_client, user) - patch_response = client.patch(self.accounts_base_uri, data={field: value}) - self.assert_status_code(204, patch_response) + self.send_patch(client, {field: value}) get_response = client.get(self.accounts_base_uri) self.assert_status_code(200, get_response) self.assertEqual(value, get_response.data[field]) if fails_validation_value: - error_response = client.patch(self.accounts_base_uri, data={field: fails_validation_value}) - self.assert_status_code(400, error_response) + error_response = self.send_patch(client, {field: fails_validation_value}, expected_status=400) self.assertEqual( "Value '{0}' is not valid for field '{1}'.".format(fails_validation_value, field), error_response.data["field_errors"][field]["user_message"] @@ -119,8 +131,7 @@ class TestAccountAPI(APITestCase): ) else: # If there are no values that would fail validation, then empty string should be supported. - patch_response = client.patch(self.accounts_base_uri, data={field: ""}) - self.assert_status_code(204, patch_response) + self.send_patch(client, {field: ""}) get_response = client.get(self.accounts_base_uri) self.assert_status_code(200, get_response) @@ -132,6 +143,9 @@ class TestAccountAPI(APITestCase): ) @ddt.unpack def test_patch_account_noneditable(self, api_client, user): + """ + Tests the behavior of patch when a read-only field is attempted to be edited. + """ client = self.login_client(api_client, user) def verify_error_response(field_name, data): @@ -143,8 +157,7 @@ class TestAccountAPI(APITestCase): ) for field_name in ["username", "email", "date_joined", "name"]: - response = client.patch(self.accounts_base_uri, data={field_name: "will_error", "gender": "f"}) - self.assert_status_code(400, response) + response = self.send_patch(client, {field_name: "will_error", "gender": "f"}, expected_status=400) verify_error_response(field_name, response.data) # Make sure that gender did not change. @@ -152,18 +165,35 @@ class TestAccountAPI(APITestCase): self.assertEqual("m", response.data["gender"]) # Test error message with multiple read-only items - response = client.patch(self.accounts_base_uri, data={"username": "will_error", "email": "xx"}) - self.assert_status_code(400, response) + response = self.send_patch(client, {"username": "will_error", "email": "xx"}, expected_status=400) self.assertEqual(2, len(response.data["field_errors"])) verify_error_response("username", response.data) verify_error_response("email", response.data) + def test_patch_bad_content_type(self): + """ + Test the behavior of patch when an incorrect content_type is specified. + """ + self.client.login(username=self.user.username, password=TEST_PASSWORD) + self.send_patch(self.client, {}, content_type="application/json", expected_status=415) + self.send_patch(self.client, {}, content_type="application/xml", expected_status=415) + def assert_status_code(self, expected_status_code, response): """Assert that the given response has the expected status code""" self.assertEqual(expected_status_code, response.status_code) def login_client(self, api_client, user): + """Helper method for getting the client and user and logging in. Returns client. """ client = getattr(self, api_client) user = getattr(self, user) client.login(username=user.username, password=TEST_PASSWORD) return client + + def send_patch(self, client, json_data, content_type="application/merge-patch+json", expected_status=204): + """ + Helper method for sending a patch to the server, defaulting to application/merge-patch+json content_type. + Verifies the expected status and returns the response. + """ + response = client.patch(self.accounts_base_uri, data=json.dumps(json_data), content_type=content_type) + self.assert_status_code(expected_status, response) + return response diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 47c15d9734..11fd0b871a 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -7,23 +7,25 @@ from rest_framework.response import Response from rest_framework import status from rest_framework.authentication import OAuth2Authentication, SessionAuthentication from rest_framework import permissions +from rest_framework import parsers from student.models import UserProfile from openedx.core.djangoapps.user_api.accounts.serializers import AccountLegacyProfileSerializer, AccountUserSerializer from openedx.core.lib.api.permissions import IsUserInUrlOrStaff +from openedx.core.lib.api.parsers import MergePatchParser class AccountView(APIView): """ **Use Cases** - Get or update the user's account information. + Get or update the user's account information. Updates are only supported through merge patch. **Example Requests**: GET /api/user/v0/accounts/{username}/ - PATCH /api/user/v0/accounts/{username}/ + PATCH /api/user/v0/accounts/{username}/ with content_type "application/merge-patch+json" **Response Values for GET** @@ -64,10 +66,12 @@ class AccountView(APIView): **Response for PATCH** Returns a 204 status if successful, with no additional content. + If "application/merge-patch+json" is not the specified content_type, returns a 415 status. """ authentication_classes = (OAuth2Authentication, SessionAuthentication) permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff) + parser_classes = (MergePatchParser,) def get(self, request, username): """ @@ -82,6 +86,10 @@ class AccountView(APIView): def patch(self, request, username): """ PATCH /api/user/v0/accounts/{username}/ + + Note that this implementation is the "merge patch" implementation proposed in + https://tools.ietf.org/html/rfc7396. The content_type must be "application/merge-patch+json" or + else an error response with status code 415 will be returned. """ existing_user, existing_user_profile = self._get_user_and_profile(username) diff --git a/openedx/core/lib/api/parsers.py b/openedx/core/lib/api/parsers.py new file mode 100644 index 0000000000..bec80da9a0 --- /dev/null +++ b/openedx/core/lib/api/parsers.py @@ -0,0 +1,8 @@ +from rest_framework import parsers + + +class MergePatchParser(parsers.JSONParser): + """ + Custom parser to be used with the "merge patch" implementation (https://tools.ietf.org/html/rfc7396). + """ + media_type = 'application/merge-patch+json' From ae0333cb61108386e16c7acbe3a9c6af483619e6 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 25 Feb 2015 15:59:02 -0500 Subject: [PATCH 7/7] Convert empty string to None for "select" fields, other cleanup. --- .../user_api/accounts/serializers.py | 20 ++- .../user_api/accounts/tests/test_views.py | 117 +++++++++++++----- .../djangoapps/user_api/accounts/views.py | 17 ++- 3 files changed, 112 insertions(+), 42 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py index 710450af67..6583599ff1 100644 --- a/openedx/core/djangoapps/user_api/accounts/serializers.py +++ b/openedx/core/djangoapps/user_api/accounts/serializers.py @@ -20,7 +20,23 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer): class Meta: model = UserProfile fields = ( - "name", "gender", "goals", "year_of_birth", "level_of_education", "language", "city", "country", - "mailing_address" + "name", "gender", "goals", "year_of_birth", "level_of_education", "language", "country", "mailing_address" ) read_only_fields = ("name",) + + def transform_gender(self, obj, value): + """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ + return AccountLegacyProfileSerializer.convert_empty_to_None(value) + + def transform_country(self, obj, value): + """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ + return AccountLegacyProfileSerializer.convert_empty_to_None(value) + + def transform_level_of_education(self, obj, value): + """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ + return AccountLegacyProfileSerializer.convert_empty_to_None(value) + + @staticmethod + def convert_empty_to_None(value): + """ Helper method to convert empty string to None (other values pass through). """ + return None if value == "" else value 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 2adc4d4076..0d91b86da3 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -1,6 +1,7 @@ import unittest import ddt import json +from datetime import datetime from django.test import TestCase from django.core.urlresolvers import reverse @@ -28,31 +29,42 @@ class TestAccountAPI(APITestCase): self.staff_client = APIClient() self.user = UserFactory.create(password=TEST_PASSWORD) - # Create some test profile values. - legacy_profile = UserProfile.objects.get(id=self.user.id) - legacy_profile.city = "Indi" - legacy_profile.country = "US" - legacy_profile.year_of_birth = 1900 - legacy_profile.level_of_education = "m" - legacy_profile.goals = "world peace" - legacy_profile.save() - self.accounts_base_uri = reverse("accounts_api", kwargs={'username': self.user.username}) + self.url = reverse("accounts_api", kwargs={'username': self.user.username}) def test_get_account_anonymous_user(self): """ Test that an anonymous client (not logged in) cannot call get. """ - response = self.anonymous_client.get(self.accounts_base_uri) - self.assert_status_code(401, response) + self.send_get(self.anonymous_client, expected_status=401) def test_get_account_different_user(self): """ Test that a client (logged in) cannot get the account information for a different client. """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) - response = self.different_client.get(self.accounts_base_uri) - self.assert_status_code(404, response) + self.send_get(self.different_client, expected_status=404) + + def test_get_account_default(self): + """ + Test that a client (logged in) can get her own account information (using default legacy profile information, + as created by the test UserFactory). + """ + self.client.login(username=self.user.username, password=TEST_PASSWORD) + response = self.send_get(self.client) + data = response.data + self.assertEqual(11, len(data)) + self.assertEqual(self.user.username, data["username"]) + self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) + for empty_field in ("year_of_birth", "level_of_education", "mailing_address"): + self.assertIsNone(data[empty_field]) + self.assertIsNone(data["country"]) + # TODO: what should the format of this be? + self.assertEqual("", data["language"]) + self.assertEqual("m", data["gender"]) + self.assertEqual("World domination", data["goals"]) + self.assertEqual(self.user.email, data["email"]) + self.assertIsNotNone(data["date_joined"]) @ddt.data( ("client", "user"), @@ -64,28 +76,46 @@ class TestAccountAPI(APITestCase): Test that a client (logged in) can get her own account information. Also verifies that a "is_staff" user can get the account information for other users. """ - client = self.login_client(api_client, user) + # Create some test profile values. + legacy_profile = UserProfile.objects.get(id=self.user.id) + legacy_profile.country = "US" + legacy_profile.level_of_education = "m" + legacy_profile.year_of_birth = 1900 + legacy_profile.goals = "world peace" + legacy_profile.mailing_address = "Park Ave" + legacy_profile.save() - response = client.get(self.accounts_base_uri) - self.assert_status_code(200, response) + client = self.login_client(api_client, user) + response = self.send_get(client) data = response.data - self.assertEqual(12, len(data)) + self.assertEqual(11, len(data)) self.assertEqual(self.user.username, data["username"]) - # TODO: should we rename this "full_name"? self.assertEqual(self.user.first_name + " " + self.user.last_name, data["name"]) - self.assertEqual("Indi", data["city"]) self.assertEqual("US", data["country"]) - # TODO: what should the format of this be? self.assertEqual("", data["language"]) self.assertEqual("m", data["gender"]) self.assertEqual(1900, data["year_of_birth"]) self.assertEqual("m", data["level_of_education"]) self.assertEqual("world peace", data["goals"]) - # Default value for mailing address is None, nothing assigned in setup. - self.assertIsNone(data['mailing_address']) + self.assertEqual("Park Ave", data['mailing_address']) self.assertEqual(self.user.email, data["email"]) self.assertIsNotNone(data["date_joined"]) + def test_get_account_empty_string(self): + """ + Test the conversion of empty strings to None for certain fields. + """ + legacy_profile = UserProfile.objects.get(id=self.user.id) + legacy_profile.country = "" + legacy_profile.level_of_education = "" + legacy_profile.gender = "" + legacy_profile.save() + + self.client.login(username=self.user.username, password=TEST_PASSWORD) + response = self.send_get(self.client) + for empty_field in ("level_of_education", "gender", "country"): + self.assertIsNone(response.data[empty_field]) + @ddt.data( ( "client", "user", "gender", "f", "not a gender", @@ -95,9 +125,8 @@ class TestAccountAPI(APITestCase): "client", "user", "level_of_education", "none", "x", "Select a valid choice. x is not one of the available choices." ), - ("client", "user", "country", "GB", "UK", "Select a valid choice. UK is not one of the available choices."), + ("client", "user", "country", "GB", "XY", "Select a valid choice. XY is not one of the available choices."), ("client", "user", "year_of_birth", 2009, "not_an_int", "Enter a whole number."), - ("client", "user", "city", "Knoxville"), ("client", "user", "language", "Creole"), ("client", "user", "goals", "Smell the roses"), ("client", "user", "mailing_address", "Sesame Street"), @@ -115,8 +144,7 @@ class TestAccountAPI(APITestCase): client = self.login_client(api_client, user) self.send_patch(client, {field: value}) - get_response = client.get(self.accounts_base_uri) - self.assert_status_code(200, get_response) + get_response = self.send_get(client) self.assertEqual(value, get_response.data[field]) if fails_validation_value: @@ -133,8 +161,7 @@ class TestAccountAPI(APITestCase): # If there are no values that would fail validation, then empty string should be supported. self.send_patch(client, {field: ""}) - get_response = client.get(self.accounts_base_uri) - self.assert_status_code(200, get_response) + get_response = self.send_get(client) self.assertEqual("", get_response.data[field]) @ddt.data( @@ -161,7 +188,7 @@ class TestAccountAPI(APITestCase): verify_error_response(field_name, response.data) # Make sure that gender did not change. - response = client.get(self.accounts_base_uri) + response = self.send_get(client) self.assertEqual("m", response.data["gender"]) # Test error message with multiple read-only items @@ -178,9 +205,23 @@ class TestAccountAPI(APITestCase): self.send_patch(self.client, {}, content_type="application/json", expected_status=415) self.send_patch(self.client, {}, content_type="application/xml", expected_status=415) - def assert_status_code(self, expected_status_code, response): - """Assert that the given response has the expected status code""" - self.assertEqual(expected_status_code, response.status_code) + def test_patch_account_empty_string(self): + """ + Tests the behavior of patch when attempting to set fields with a select list of options to the empty string. + Also verifies the behaviour when setting to None. + """ + self.client.login(username=self.user.username, password=TEST_PASSWORD) + for field_name in ["gender", "level_of_education", "country"]: + self.send_patch(self.client, {field_name: ""}) + response = self.send_get(self.client) + # Although throwing a 400 might be reasonable, the default DRF behavior with ModelSerializer + # is to convert to None, which also seems acceptable (and is difficult to override). + self.assertIsNone(response.data[field_name]) + + # Verify that the behavior is the same for sending None. + self.send_patch(self.client, {field_name: ""}) + response = self.send_get(self.client) + self.assertIsNone(response.data[field_name]) def login_client(self, api_client, user): """Helper method for getting the client and user and logging in. Returns client. """ @@ -194,6 +235,14 @@ class TestAccountAPI(APITestCase): Helper method for sending a patch to the server, defaulting to application/merge-patch+json content_type. Verifies the expected status and returns the response. """ - response = client.patch(self.accounts_base_uri, data=json.dumps(json_data), content_type=content_type) - self.assert_status_code(expected_status, response) + response = client.patch(self.url, data=json.dumps(json_data), content_type=content_type) + self.assertEqual(expected_status, response.status_code) + return response + + def send_get(self, client, expected_status=200): + """ + Helper method for sending a GET to the server. Verifies the expected status and returns the response. + """ + response = client.get(self.url) + self.assertEqual(expected_status, response.status_code) return response diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 11fd0b871a..58759e062b 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1,3 +1,9 @@ +""" +NOTE: this API is WIP and has not yet been approved. Do not use this API without talking to Christina or Andy. + +For more information, see: +https://openedx.atlassian.net/wiki/display/TNL/User+API +""" from django.core.exceptions import ObjectDoesNotExist from django.contrib.auth.models import User from django.utils.translation import ugettext as _ @@ -35,13 +41,14 @@ class AccountView(APIView): * email: email for the user (not editable through this API) - * date_joined: date this account was created (not editable) + * date_joined: date this account was created (not editable), in the string format provided by + datetime (for example, "2014-08-26T17:52:11Z") - * gender: null or "" (not set), "m", "f", or "o" + * gender: null (not set), "m", "f", or "o" * year_of_birth: null or integer year - * level_of_education: null or "" (not set), or one of the following choices: + * level_of_education: null (not set), or one of the following choices: * "p" signifying "Doctorate" * "m" signifying "Master's or professional degree" @@ -55,9 +62,7 @@ class AccountView(APIView): * language: null or name of preferred language - * city: null or name of city - - * country: null or "" (not set), or a Country corresponding to one of the ISO 3166-1 countries + * country: null (not set), or a Country corresponding to one of the ISO 3166-1 countries * mailing_address: null or textual representation of mailing address