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