diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 51fbb5a3cc..ecc840f153 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -407,7 +407,7 @@ class CommentSerializer(_ContentSerializer): "parent_id does not identify a comment in the thread identified by thread_id." ) if is_comment_too_deep(parent): - raise ValidationError({"parent_id": ["Comment level is too deep."]}) + raise ValidationError("Comment level is too deep.") return attrs def create(self, validated_data): diff --git a/lms/djangoapps/experiments/routers.py b/lms/djangoapps/experiments/routers.py index 36b69842f3..72077fdd65 100644 --- a/lms/djangoapps/experiments/routers.py +++ b/lms/djangoapps/experiments/routers.py @@ -4,7 +4,7 @@ Experimentation routers from rest_framework import routers -from rest_framework.routers import DynamicDetailRoute, DynamicListRoute, Route +from rest_framework.routers import DynamicRoute, Route class DefaultRouter(routers.DefaultRouter): @@ -19,14 +19,16 @@ class DefaultRouter(routers.DefaultRouter): 'put': 'create_or_update', }, name='{basename}-list', + detail=False, initkwargs={'suffix': 'List'} ), # Dynamically generated list routes. # Generated using @list_route decorator # on methods of the viewset. - DynamicListRoute( - url=r'^{prefix}/{methodname}{trailing_slash}$', - name='{basename}-{methodnamehyphen}', + DynamicRoute( + url=r'^{prefix}/{lookup}{trailing_slash}$', + name='{basename}-list', + detail=False, initkwargs={} ), # Detail route. @@ -39,13 +41,15 @@ class DefaultRouter(routers.DefaultRouter): 'delete': 'destroy' }, name='{basename}-detail', + detail=True, initkwargs={'suffix': 'Instance'} ), # Dynamically generated detail routes. # Generated using @detail_route decorator on methods of the viewset. - DynamicDetailRoute( - url=r'^{prefix}/{lookup}/{methodname}{trailing_slash}$', - name='{basename}-{methodnamehyphen}', + DynamicRoute( + url=r'^{prefix}/{lookup}{trailing_slash}$', + name='{basename}-detail', + detail=True, initkwargs={} ), ] diff --git a/lms/djangoapps/experiments/tests/test_views.py b/lms/djangoapps/experiments/tests/test_views.py index 510316d789..616cf58201 100644 --- a/lms/djangoapps/experiments/tests/test_views.py +++ b/lms/djangoapps/experiments/tests/test_views.py @@ -165,65 +165,6 @@ class ExperimentDataViewSetTests(APITestCase): response = self.client.patch(url, data) self.assertEqual(response.status_code, 404) - def test_bulk_upsert_permissions(self): - """ Only staff users can access the bulk upsert endpoint. """ - url = reverse('api_experiments:v0:data-bulk-upsert') - data = [] - - # Authentication is required - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 401) - - user = UserFactory() - self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) - - # No access to non-staff users - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 403) - - user.is_staff = True - user.save() - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 200) - - def test_bulk_upsert(self): - """ The endpoint should support creating/updating multiple ExperimentData objects with a single call. """ - url = reverse('api_experiments:v0:data-bulk-upsert') - experiment_id = 1 - user = UserFactory(is_staff=True) - other_user = UserFactory() - self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) - - data = [ - { - 'experiment_id': experiment_id, - 'key': 'foo', - 'value': 'bar', - 'user': user.username, - }, - { - 'experiment_id': experiment_id, - 'key': 'foo', - 'value': 'bar', - 'user': other_user.username, - }, - ] - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 200) - kwargs = { - 'experiment_id': experiment_id, - 'key': 'foo', - 'value': 'bar', - } - ExperimentData.objects.get(user=user, **kwargs) - ExperimentData.objects.get(user=other_user, **kwargs) - - # Subsequent calls should update the existing data rather than create more - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 200) - ExperimentData.objects.get(user=user, **kwargs) - ExperimentData.objects.get(user=other_user, **kwargs) - def cross_domain_config(func): """Decorator for configuring a cross-domain request. """ @@ -347,54 +288,3 @@ class ExperimentKeyValueViewSetTests(APITestCase): response = self.client.delete(url) self.assertEqual(response.status_code, 403) - - def test_bulk_upsert_permissions(self): - """ Non-staff users should not be allowed to access the endpoint. """ - data = [] - url = reverse('api_experiments:v0:key_value-bulk-upsert') - user = UserFactory(is_staff=False) - - # Authentication required - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 401) - - # Staff permission required - self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 403) - - def test_bulk_upsert(self): - """ The endpoint should support creating/updating multiple ExperimentData objects with a single call. """ - url = reverse('api_experiments:v0:key_value-bulk-upsert') - experiment_id = 1 - user = UserFactory(is_staff=True) - data = [ - { - 'experiment_id': experiment_id, - 'key': 'foo', - 'value': 'bar', - }, - { - 'experiment_id': experiment_id, - 'key': 'foo1', - 'value': 'bar', - }, - ] - - self.client.login(username=user.username, password=UserFactory._DEFAULT_PASSWORD) - - # New data should be created - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 200) - kwargs = { - 'experiment_id': experiment_id, - 'value': 'bar', - } - ExperimentKeyValue.objects.get(key='foo', **kwargs) - ExperimentKeyValue.objects.get(key='foo1', **kwargs) - - # Subsequent calls should update the existing data rather than create more - response = self.client.put(url, data, format='json') - self.assertEqual(response.status_code, 200) - ExperimentKeyValue.objects.get(key='foo', **kwargs) - ExperimentKeyValue.objects.get(key='foo1', **kwargs) diff --git a/lms/djangoapps/experiments/views.py b/lms/djangoapps/experiments/views.py index 43300d4df1..8add1c135b 100644 --- a/lms/djangoapps/experiments/views.py +++ b/lms/djangoapps/experiments/views.py @@ -9,7 +9,6 @@ from django_filters.rest_framework import DjangoFilterBackend from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from rest_framework import permissions, viewsets -from rest_framework.decorators import list_route from rest_framework.response import Response from experiments import filters, serializers @@ -77,21 +76,6 @@ class ExperimentDataViewSet(viewsets.ModelViewSet): return user - @list_route(methods=['put'], permission_classes=[permissions.IsAdminUser]) - def bulk_upsert(self, request): - upserted = [] - self._cache_users([datum['user'] for datum in request.data]) - - with transaction.atomic(): - for item in request.data: - user = self._get_user(username=item['user']) - datum, __ = ExperimentData.objects.update_or_create( - user=user, experiment_id=item['experiment_id'], key=item['key'], defaults={'value': item['value']}) - upserted.append(datum) - - serializer = self.get_serializer(upserted, many=True) - return Response(serializer.data) - class ExperimentKeyValueViewSet(viewsets.ModelViewSet): authentication_classes = (JwtAuthentication, ExperimentCrossDomainSessionAuth,) @@ -100,16 +84,3 @@ class ExperimentKeyValueViewSet(viewsets.ModelViewSet): permission_classes = (IsStaffOrReadOnly,) queryset = ExperimentKeyValue.objects.all() serializer_class = serializers.ExperimentKeyValueSerializer - - @list_route(methods=['put'], permission_classes=[permissions.IsAdminUser]) - def bulk_upsert(self, request): - upserted = [] - - with transaction.atomic(): - for item in request.data: - datum, __ = ExperimentKeyValue.objects.update_or_create( - experiment_id=item['experiment_id'], key=item['key'], defaults={'value': item['value']}) - upserted.append(datum) - - serializer = self.get_serializer(upserted, many=True) - return Response(serializer.data) diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py index 1223bdde44..9b16a3b5d8 100644 --- a/openedx/core/djangoapps/user_api/preferences/api.py +++ b/openedx/core/djangoapps/user_api/preferences/api.py @@ -13,7 +13,6 @@ from django.utils.translation import ugettext as _ from django.utils.translation import ugettext_noop from django_countries import countries from pytz import common_timezones, common_timezones_set, country_timezones -from six import text_type from openedx.core.lib.time_zone_utils import get_display_time_zone from student.models import User, UserProfile @@ -279,7 +278,9 @@ def update_email_opt_in(user, org, opt_in): if hasattr(settings, 'LMS_SEGMENT_KEY') and settings.LMS_SEGMENT_KEY: _track_update_email_opt_in(user.id, org, opt_in) except IntegrityError as err: - log.warning(u"Could not update organization wide preference due to IntegrityError: {}".format(text_type(err))) + log.warning( + u"Could not update organization wide preference due to IntegrityError: {}".format(six.text_type(err)) + ) def _track_update_email_opt_in(user_id, organization, opt_in): @@ -386,8 +387,13 @@ def validate_user_preference_serializer(serializer, preference_key, preference_v } }) if not serializer.is_valid(): + errors = serializer.errors + # DRF error messages are of type ErrorDetail and serialize out as such. We want to coerce those + # messages into the strings only. + for key in errors: + errors[key] = [six.text_type(el) for el in errors[key]] developer_message = u"Value '{preference_value}' not valid for preference '{preference_key}': {error}".format( - preference_key=preference_key, preference_value=preference_value, error=serializer.errors + preference_key=preference_key, preference_value=preference_value, error=errors ) if "key" in serializer.errors: user_message = _(u"Invalid user preference key '{preference_key}'.").format( diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py index 9067d878dc..47cc8e3a7a 100644 --- a/openedx/core/lib/api/view_utils.py +++ b/openedx/core/lib/api/view_utils.py @@ -13,7 +13,7 @@ from edx_rest_framework_extensions.auth.session.authentication import SessionAut from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from rest_framework import status -from rest_framework.exceptions import APIException +from rest_framework.exceptions import APIException, ErrorDetail from rest_framework.generics import GenericAPIView from rest_framework.mixins import RetrieveModelMixin, UpdateModelMixin from rest_framework.permissions import IsAuthenticated @@ -132,11 +132,28 @@ def view_auth_classes(is_user=False, is_authenticated=True): return _decorator +def clean_errors(error): + """ + DRF error messages are of type ErrorDetail and serialize out as such. + We want to coerce the strings into the message only. + + This cursively handles the nesting of errors. + """ + if isinstance(error, ErrorDetail): + return text_type(error) + if isinstance(error, list): + return [clean_errors(el) for el in error] + else: + # We assume that it's a nested dictionary if it's not a list. + return {key: clean_errors(value) for key, value in error.items()} + + def add_serializer_errors(serializer, data, field_errors): """Adds errors from serializer validation to field_errors. data is the original data to deserialize.""" if not serializer.is_valid(): errors = serializer.errors for key, error in iteritems(errors): + error = clean_errors(error) field_errors[key] = { 'developer_message': u"Value '{field_value}' is not valid for field '{field_name}': {error}".format( field_value=data.get(key, ''), field_name=key, error=error diff --git a/requirements/edx/base.in b/requirements/edx/base.in index 035f468ece..54e3575f90 100644 --- a/requirements/edx/base.in +++ b/requirements/edx/base.in @@ -64,7 +64,7 @@ django-storages django-user-tasks django-waffle==0.12.0 django-webpack-loader # Used to wire webpack bundles into the django asset pipeline -djangorestframework==3.7.7 +djangorestframework==3.9.4 djangorestframework-jwt drf-yasg # Replacement for django-rest-swagger edx-ace diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index b6b24add20..ebd3083c9d 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -89,7 +89,7 @@ django==1.11.27 djangorestframework-jwt==1.11.0 git+https://github.com/edx/django-rest-framework-oauth.git@0a43e8525f1e3048efe4bc70c03de308a277197c#egg=djangorestframework-oauth==1.1.1 djangorestframework-xml==1.4.0 # via edx-enterprise -djangorestframework==3.7.7 +djangorestframework==3.9.4 docopt==0.6.2 docutils==0.15.2 # via botocore drf-yasg==1.16 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index b942a06c48..cda9e58b8e 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -102,7 +102,7 @@ django==1.11.27 djangorestframework-jwt==1.11.0 git+https://github.com/edx/django-rest-framework-oauth.git@0a43e8525f1e3048efe4bc70c03de308a277197c#egg=djangorestframework-oauth==1.1.1 djangorestframework-xml==1.4.0 -djangorestframework==3.7.7 +djangorestframework==3.9.4 docker==4.1.0 docopt==0.6.2 docutils==0.15.2 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 2e5bf1f60e..b5fc5f56fe 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -99,7 +99,7 @@ django-webpack-loader==0.6.0 djangorestframework-jwt==1.11.0 git+https://github.com/edx/django-rest-framework-oauth.git@0a43e8525f1e3048efe4bc70c03de308a277197c#egg=djangorestframework-oauth==1.1.1 djangorestframework-xml==1.4.0 -djangorestframework==3.7.7 +djangorestframework==3.9.4 docker==4.1.0 # via moto docopt==0.6.2 docutils==0.15.2