diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index b24ef618c7ce..625b5b5a9c07 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -488,9 +488,7 @@ def test_certificate_exception_missing_username_and_email_error(self): assert not res_json['success'] # Assert Error Message - assert res_json['message'] ==\ - 'Student username/email field is required and can not be empty.' \ - ' Kindly fill in username/email and then press "Add to Exception List" button.' + assert res_json['message'] == {'user': ['This field may not be blank.']} def test_certificate_exception_duplicate_user_error(self): """ @@ -604,6 +602,34 @@ def test_certificate_exception_removed_successfully(self): # Verify that certificate exception does not exist assert not certs_api.is_on_allowlist(self.user2, self.course.id) + def test_certificate_exception_removed_successfully_form_url(self): + """ + In case of deletion front-end is sending content-type x-www-form-urlencoded. + Just to handle that some logic added in api and this test is for that part. + Test certificates exception removal api endpoint returns success status + when called with valid course key and certificate exception id + """ + GeneratedCertificateFactory.create( + user=self.user2, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + grade='1.0' + ) + # Verify that certificate exception exists + assert certs_api.is_on_allowlist(self.user2, self.course.id) + + response = self.client.post( + self.url, + data=json.dumps(self.certificate_exception_in_db), + content_type='application/x-www-form-urlencoded', + REQUEST_METHOD='DELETE' + ) + # Assert successful request processing + assert response.status_code == 204 + + # Verify that certificate exception does not exist + assert not certs_api.is_on_allowlist(self.user2, self.course.id) + def test_remove_certificate_exception_invalid_request_error(self): """ Test certificates exception removal api endpoint returns error diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index ccab674d5986..6c1b1a014fdc 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -22,7 +22,7 @@ from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist, PermissionDenied, ValidationError from django.core.validators import validate_email from django.db import IntegrityError, transaction -from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotFound +from django.http import QueryDict, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotFound from django.shortcuts import redirect from django.urls import reverse from django.utils.decorators import method_decorator @@ -30,7 +30,7 @@ from django.utils.translation import gettext as _ from django.views.decorators.cache import cache_control from django.views.decorators.csrf import ensure_csrf_cookie -from django.views.decorators.http import require_POST, require_http_methods +from django.views.decorators.http import require_POST from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from edx_when.api import get_date_for_block @@ -3350,42 +3350,93 @@ def start_certificate_regeneration(request, course_id): return JsonResponse(response_payload) -@transaction.non_atomic_requests -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.CERTIFICATE_EXCEPTION_VIEW) -@require_http_methods(['POST', 'DELETE']) -def certificate_exception_view(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +@method_decorator(transaction.non_atomic_requests, name='dispatch') +class CertificateExceptionView(DeveloperErrorViewMixin, APIView): """ Add/Remove students to/from the certificate allowlist. - - :param request: HttpRequest object - :param course_id: course identifier of the course for whom to add/remove certificates exception. - :return: JsonResponse object with success/error message or certificate exception data. """ - course_key = CourseKey.from_string(course_id) - # Validate request data and return error response in case of invalid data - try: - certificate_exception, student = parse_request_data_and_get_user(request) - except ValueError as error: - return JsonResponse({'success': False, 'message': str(error)}, status=400) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.CERTIFICATE_EXCEPTION_VIEW + serializer_class = CertificateSerializer + http_method_names = ['post', 'delete'] + + @method_decorator(transaction.non_atomic_requests, name='dispatch') + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """ + Add certificate exception for a student. + """ + return self._handle_certificate_exception(request, course_id, action="post") + + @method_decorator(ensure_csrf_cookie) + @method_decorator(transaction.non_atomic_requests) + def delete(self, request, course_id): + """ + Remove certificate exception for a student. + """ + return self._handle_certificate_exception(request, course_id, action="delete") + + def _handle_certificate_exception(self, request, course_id, action): + """ + Handles adding or removing certificate exceptions. + """ + course_key = CourseKey.from_string(course_id) + try: + data = request.data + except Exception: # pylint: disable=broad-except + return JsonResponse( + { + 'success': False, + 'message': + _('The record is not in the correct format. Please add a valid username or email address.')}, + status=400 + ) + + # Extract and validate the student information + student, error_response = self._get_and_validate_user(data) + + if error_response: + return error_response - # Add new Certificate Exception for the student passed in request data - if request.method == 'POST': try: - exception = add_certificate_exception(course_key, student, certificate_exception) + if action == "post": + exception = add_certificate_exception(course_key, student, data) + return JsonResponse(exception) + elif action == "delete": + remove_certificate_exception(course_key, student) + return JsonResponse({}, status=204) except ValueError as error: return JsonResponse({'success': False, 'message': str(error)}, status=400) - return JsonResponse(exception) - # Remove Certificate Exception for the student passed in request data - elif request.method == 'DELETE': + def _get_and_validate_user(self, raw_data): + """ + Extracts the user data from the request and validates the student. + """ + # This is only happening in case of delete. + # because content-type is coming as x-www-form-urlencoded from front-end. + if isinstance(raw_data, QueryDict): + raw_data = list(raw_data.keys())[0] + try: + raw_data = json.loads(raw_data) + except Exception as error: # pylint: disable=broad-except + return None, JsonResponse({'success': False, 'message': str(error)}, status=400) + try: - remove_certificate_exception(course_key, student) + user_data = raw_data.get('user_name', '') or raw_data.get('user_email', '') except ValueError as error: - return JsonResponse({'success': False, 'message': str(error)}, status=400) + return None, JsonResponse({'success': False, 'message': str(error)}, status=400) - return JsonResponse({}, status=204) + serializer_data = self.serializer_class(data={'user': user_data}) + if not serializer_data.is_valid(): + return None, JsonResponse({'success': False, 'message': serializer_data.errors}, status=400) + + student = serializer_data.validated_data.get('user') + if not student: + response_payload = f'{user_data} does not exist in the LMS. Please check your spelling and retry.' + return None, JsonResponse({'success': False, 'message': response_payload}, status=400) + + return student, None def add_certificate_exception(course_key, student, certificate_exception): diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 9d4162afae0e..72b93ba12766 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -84,7 +84,7 @@ path('enable_certificate_generation', api.enable_certificate_generation, name='enable_certificate_generation'), path('start_certificate_generation', api.StartCertificateGeneration.as_view(), name='start_certificate_generation'), path('start_certificate_regeneration', api.start_certificate_regeneration, name='start_certificate_regeneration'), - path('certificate_exception_view/', api.certificate_exception_view, name='certificate_exception_view'), + path('certificate_exception_view/', api.CertificateExceptionView.as_view(), name='certificate_exception_view'), re_path(r'^generate_certificate_exceptions/(?P[^/]*)', api.GenerateCertificateExceptions.as_view(), name='generate_certificate_exceptions'), path('generate_bulk_certificate_exceptions', api.generate_bulk_certificate_exceptions, diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 2ac794bc2943..48c5f7edec1c 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -232,7 +232,11 @@ def __init__(self, *args, **kwargs): class CertificateSerializer(serializers.Serializer): """ - Serializer for resetting a students attempts counter or starts a task to reset all students + Serializer for multiple operations related with certificates. + resetting a students attempts counter or starts a task to reset all students + attempts counters + Also Add/Remove students to/from the certificate allowlist. + Also For resetting a students attempts counter or starts a task to reset all students attempts counters. """ user = serializers.CharField(