From a83c1aad1e4d041e70b1582ddd7230b36a734220 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Fri, 20 Sep 2024 14:22:44 +0500 Subject: [PATCH 01/20] feat!: fixing quality. --- lms/djangoapps/instructor/views/api.py | 368 ++++++++++--------------- 1 file changed, 150 insertions(+), 218 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 45c460d32908..5dc89b48c3f7 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1676,31 +1676,18 @@ def get_proctored_exam_results(request, course_id): return JsonResponse({"status": success_status}) -@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -@method_decorator(transaction.non_atomic_requests, name='dispatch') -class GetAnonIds(APIView): +@transaction.non_atomic_requests +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_course_permission(permissions.CAN_RESEARCH) +def get_anon_ids(request, course_id): """ - Respond with 2-column CSV output of user-id, anonymized-user-id. - This API processes the incoming request to generate a CSV file containing - two columns: `user-id` and `anonymized-user-id`. The CSV is returned as a - response to the client. + Respond with 2-column CSV output of user-id, anonymized-user-id """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.CAN_RESEARCH - - @method_decorator(ensure_csrf_cookie) - @method_decorator(transaction.non_atomic_requests) - def post(self, request, course_id): - """ - Handle POST request to generate a CSV output. - - Returns: - Response: A CSV file with two columns: `user-id` and `anonymized-user-id`. - """ - report_type = _('Anonymized User IDs') - success_status = SUCCESS_MESSAGE_TEMPLATE.format(report_type=report_type) - task_api.generate_anonymous_ids(request, course_id) - return JsonResponse({"status": success_status}) + report_type = _('Anonymized User IDs') + success_status = SUCCESS_MESSAGE_TEMPLATE.format(report_type=report_type) + task_api.generate_anonymous_ids(request, course_id) + return JsonResponse({"status": success_status}) @require_POST @@ -1818,24 +1805,23 @@ def post(self, request, course_id): return Response(serializer.data) -@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -@method_decorator(transaction.non_atomic_requests, name='dispatch') -class ResetStudentAttempts(DeveloperErrorViewMixin, APIView): +@transaction.non_atomic_requests +@require_POST +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_course_permission(permissions.GIVE_STUDENT_EXTENSION) +@require_post_params( + problem_to_reset="problem urlname to reset" +) +@common_exceptions_400 +def reset_student_attempts(request, course_id): """ + Resets a students attempts counter or starts a task to reset all students attempts counters. Optionally deletes student state for a problem. Limited to staff access. Some sub-methods limited to instructor access. - """ - http_method_names = ['post'] - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.GIVE_STUDENT_EXTENSION - serializer_class = StudentAttemptsSerializer - @method_decorator(ensure_csrf_cookie) - @transaction.non_atomic_requests - def post(self, request, course_id): - """ - Takes some of the following query parameters + Takes some of the following query parameters - problem_to_reset is a urlname of a problem - unique_student_identifier is an email or username - all_students is a boolean @@ -1845,74 +1831,65 @@ def post(self, request, course_id): - delete_module is a boolean requires instructor access mutually exclusive with all_students - """ - course_id = CourseKey.from_string(course_id) - serializer_data = self.serializer_class(data=request.data) - - if not serializer_data.is_valid(): - return HttpResponseBadRequest(reason=serializer_data.errors) + """ + course_id = CourseKey.from_string(course_id) + course = get_course_with_access( + request.user, 'staff', course_id, depth=None + ) + all_students = _get_boolean_param(request, 'all_students') - course = get_course_with_access( - request.user, 'staff', course_id, depth=None - ) + if all_students and not has_access(request.user, 'instructor', course): + return HttpResponseForbidden("Requires instructor access.") - all_students = serializer_data.validated_data.get('all_students') + problem_to_reset = strip_if_string(request.POST.get('problem_to_reset')) + student_identifier = request.POST.get('unique_student_identifier', None) + student = None + if student_identifier is not None: + student = get_student_from_identifier(student_identifier) + delete_module = _get_boolean_param(request, 'delete_module') - if all_students and not has_access(request.user, 'instructor', course): - return HttpResponseForbidden("Requires instructor access.") + # parameter combinations + if all_students and student: + return HttpResponseBadRequest( + "all_students and unique_student_identifier are mutually exclusive." + ) + if all_students and delete_module: + return HttpResponseBadRequest( + "all_students and delete_module are mutually exclusive." + ) - problem_to_reset = strip_if_string(serializer_data.validated_data.get('problem_to_reset')) - student_identifier = request.POST.get('unique_student_identifier', None) - student = serializer_data.validated_data.get('unique_student_identifier') - delete_module = serializer_data.validated_data.get('delete_module') + try: + module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) + except InvalidKeyError: + return HttpResponseBadRequest() - # parameter combinations - if all_students and student: - return HttpResponseBadRequest( - "all_students and unique_student_identifier are mutually exclusive." - ) - if all_students and delete_module: - return HttpResponseBadRequest( - "all_students and delete_module are mutually exclusive." - ) + response_payload = {} + response_payload['problem_to_reset'] = problem_to_reset + if student: try: - module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) - except InvalidKeyError: - return HttpResponseBadRequest() - - response_payload = {} - response_payload['problem_to_reset'] = problem_to_reset - - if student: - try: - enrollment.reset_student_attempts( - course_id, - student, - module_state_key, - requesting_user=request.user, - delete_module=delete_module - ) - except StudentModule.DoesNotExist: - return HttpResponseBadRequest(_("Module does not exist.")) - except sub_api.SubmissionError: - # Trust the submissions API to log the error - error_msg = _("An error occurred while deleting the score.") - return HttpResponse(error_msg, status=500) - response_payload['student'] = student_identifier - - elif all_students: - try: - task_api.submit_reset_problem_attempts_for_all_students(request, module_state_key) - response_payload['task'] = TASK_SUBMISSION_OK - response_payload['student'] = 'All Students' - except Exception: # pylint: disable=broad-except - error_msg = _("An error occurred while attempting to reset for all students.") - return HttpResponse(error_msg, status=500) - else: - return HttpResponseBadRequest() + enrollment.reset_student_attempts( + course_id, + student, + module_state_key, + requesting_user=request.user, + delete_module=delete_module + ) + except StudentModule.DoesNotExist: + return HttpResponseBadRequest(_("Module does not exist.")) + except sub_api.SubmissionError: + # Trust the submissions API to log the error + error_msg = _("An error occurred while deleting the score.") + return HttpResponse(error_msg, status=500) + response_payload['student'] = student_identifier + elif all_students: + task_api.submit_reset_problem_attempts_for_all_students(request, module_state_key) + response_payload['task'] = TASK_SUBMISSION_OK + response_payload['student'] = 'All Students' + else: + return HttpResponseBadRequest() - return JsonResponse(response_payload) + return JsonResponse(response_payload) @transaction.non_atomic_requests @@ -1949,10 +1926,8 @@ def reset_student_attempts_for_entrance_exam(request, course_id): student_identifier = request.POST.get('unique_student_identifier', None) student = None - if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = _get_boolean_param(request, 'all_students') delete_module = _get_boolean_param(request, 'delete_module') @@ -2374,8 +2349,9 @@ def get(self, request, course_id): return _list_instructor_tasks(request=request, course_id=course_id) -@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -class ListInstructorTasks(APIView): +@require_POST +@ensure_csrf_cookie +def list_instructor_tasks(request, course_id): """ List instructor tasks. @@ -2385,44 +2361,21 @@ class ListInstructorTasks(APIView): - `problem_location_str` and `unique_student_identifier` lists task history for problem AND student (intersection) """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.SHOW_TASKS - serializer_class = ListInstructorTaskInputSerializer - - @method_decorator(ensure_csrf_cookie) - def post(self, request, course_id): - """ - List instructor tasks. - """ - serializer = self.serializer_class(data=request.data) - serializer.is_valid(raise_exception=True) - - return _list_instructor_tasks( - request=request, course_id=course_id, serialize_data=serializer.validated_data - ) + return _list_instructor_tasks(request=request, course_id=course_id) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_course_permission(permissions.SHOW_TASKS) -def _list_instructor_tasks(request, course_id, serialize_data=None): +def _list_instructor_tasks(request, course_id): """ List instructor tasks. Internal function with common code for both DRF and and tradition views. """ - # This method is also used by other APIs with the GET method. - # The query_params attribute is utilized for GET requests, - # where parameters are passed as query strings. - course_id = CourseKey.from_string(course_id) - if serialize_data is not None: - problem_location_str = strip_if_string(serialize_data.get('problem_location_str', False)) - student = serialize_data.get('unique_student_identifier', None) - else: - params = getattr(request, 'query_params', request.POST) - problem_location_str = strip_if_string(params.get('problem_location_str', False)) - student = params.get('unique_student_identifier', None) - + params = getattr(request, 'query_params', request.POST) + problem_location_str = strip_if_string(params.get('problem_location_str', False)) + student = params.get('unique_student_identifier', None) if student is not None: student = get_student_from_identifier(student) @@ -2576,22 +2529,16 @@ def get(self, request, course_id): return _list_report_downloads(request=request, course_id=course_id) -@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -class ListReportDownloads(APIView): - +@require_POST +@ensure_csrf_cookie +def list_report_downloads(request, course_id): """ List grade CSV files that are available for download for this course. Takes the following query parameters: - (optional) report_name - name of the report """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.CAN_RESEARCH - - @method_decorator(ensure_csrf_cookie) - def post(self, request, course_id): - - return _list_report_downloads(request=request, course_id=course_id) + return _list_report_downloads(request=request, course_id=course_id) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -2805,96 +2752,81 @@ def extract_user_info(user): return JsonResponse(response_payload) -@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -@method_decorator(transaction.non_atomic_requests, name='dispatch') -class SendEmail(DeveloperErrorViewMixin, APIView): +@transaction.non_atomic_requests +@require_POST +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_course_permission(permissions.EMAIL) +@require_post_params(send_to="sending to whom", subject="subject line", message="message text") +@common_exceptions_400 +def send_email(request, course_id): """ Send an email to self, staff, cohorts, or everyone involved in a course. + Query Parameters: + - 'send_to' specifies what group the email should be sent to + Options are defined by the CourseEmail model in + lms/djangoapps/bulk_email/models.py + - 'subject' specifies email's subject + - 'message' specifies email's content """ - http_method_names = ['post'] - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.EMAIL - serializer_class = SendEmailSerializer - - @method_decorator(ensure_csrf_cookie) - @method_decorator(transaction.non_atomic_requests) - def post(self, request, course_id): - """ - Query Parameters: - - 'send_to' specifies what group the email should be sent to - Options are defined by the CourseEmail model in - lms/djangoapps/bulk_email/models.py - - 'subject' specifies email's subject - - 'message' specifies email's content - """ - course_id = CourseKey.from_string(course_id) - course_overview = CourseOverview.get_from_id(course_id) - - if not is_bulk_email_feature_enabled(course_id): - log.warning(f"Email is not enabled for course {course_id}") - return HttpResponseForbidden("Email is not enabled for this course.") - - serializer_data = self.serializer_class(data=request.data) - if not serializer_data.is_valid(): - return HttpResponseBadRequest(reason=serializer_data.errors) - - # Skipping serializer validation to avoid potential disruptions. - # The API handles numerous input variations, and changes here could introduce breaking issues. - - targets = json.loads(request.POST.get("send_to")) + course_id = CourseKey.from_string(course_id) + course_overview = CourseOverview.get_from_id(course_id) - subject = serializer_data.validated_data.get("subject") - message = serializer_data.validated_data.get("message") - # optional, this is a date and time in the form of an ISO8601 string - schedule = serializer_data.validated_data.get("schedule", "") + if not is_bulk_email_feature_enabled(course_id): + log.warning(f"Email is not enabled for course {course_id}") + return HttpResponseForbidden("Email is not enabled for this course.") - schedule_dt = None - if schedule: - try: - # convert the schedule from a string to a datetime, then check if its a - # valid future date and time, dateutil - # will throw a ValueError if the schedule is no good. - schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) - if schedule_dt < datetime.datetime.now(pytz.utc): - raise ValueError("the requested schedule is in the past") - except ValueError as value_error: - error_message = ( - f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " - f"{value_error}" - ) - log.error(error_message) - return HttpResponseBadRequest(error_message) + targets = json.loads(request.POST.get("send_to")) + subject = request.POST.get("subject") + message = request.POST.get("message") + # optional, this is a date and time in the form of an ISO8601 string + schedule = request.POST.get("schedule", "") - # Retrieve the customized email "from address" and email template from site configuration for the c - # ourse/partner. - # If there is no site configuration enabled for the current site then we use system defaults for both. - from_addr = _get_branded_email_from_address(course_overview) - template_name = _get_branded_email_template(course_overview) - - # Create the CourseEmail object. This is saved immediately so that any transaction that has been - # pending up to this point will also be committed. + schedule_dt = None + if schedule: try: - email = create_course_email( - course_id, - request.user, - targets, - subject, - message, - template_name=template_name, - from_addr=from_addr, + # convert the schedule from a string to a datetime, then check if its a valid future date and time, dateutil + # will throw a ValueError if the schedule is no good. + schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) + if schedule_dt < datetime.datetime.now(pytz.utc): + raise ValueError("the requested schedule is in the past") + except ValueError as value_error: + error_message = ( + f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " + f"{value_error}" ) - except ValueError as err: - return HttpResponseBadRequest(repr(err)) + log.error(error_message) + return HttpResponseBadRequest(error_message) - # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) - task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) + # Retrieve the customized email "from address" and email template from site configuration for the course/partner. If + # there is no site configuration enabled for the current site then we use system defaults for both. + from_addr = _get_branded_email_from_address(course_overview) + template_name = _get_branded_email_template(course_overview) - response_payload = { - 'course_id': str(course_id), - 'success': True, - } + # Create the CourseEmail object. This is saved immediately so that any transaction that has been pending up to this + # point will also be committed. + try: + email = create_course_email( + course_id, + request.user, + targets, + subject, + message, + template_name=template_name, + from_addr=from_addr, + ) + except ValueError as err: + return HttpResponseBadRequest(repr(err)) - return JsonResponse(response_payload) + # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) + task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) + + response_payload = { + 'course_id': str(course_id), + 'success': True, + } + + return JsonResponse(response_payload) @require_POST @@ -3054,7 +2986,7 @@ def post(self, request, course_id): student (str): The email or username of the student whose access is being modified. reason (str): Optional param. """ - serializer_data = self.serializer_class(data=request.data, context={'disable_due_datetime': True}) + serializer_data = self.serializer_class(data=request.data, context={'make_due_datetime': True}) if not serializer_data.is_valid(): return HttpResponseBadRequest(reason=serializer_data.errors) From 7ab88ed4611dba12680f3142e3e74428d8fcfe54 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 23 Sep 2024 14:50:35 +0500 Subject: [PATCH 02/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 61 +++++++++++++++------ lms/djangoapps/instructor/views/api_urls.py | 6 +- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 5dc89b48c3f7..906e5df2f5a8 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3539,12 +3539,9 @@ def build_row_errors(key, _user, row_count): return JsonResponse(results) -@transaction.non_atomic_requests -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.CERTIFICATE_INVALIDATION_VIEW) -@require_http_methods(['POST', 'DELETE']) -def certificate_invalidation_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 CertificateInvalidationView(APIView): """ Invalidate/Re-Validate students to/from certificate. @@ -3552,17 +3549,22 @@ def certificate_invalidation_view(request, course_id): :param course_id: course identifier of the course for whom to add/remove certificates exception. :return: JsonResponse object with success/error message or certificate invalidation data. """ - course_key = CourseKey.from_string(course_id) - # Validate request data and return error response in case of invalid data - try: - certificate_invalidation_data = parse_request_data(request) - student = _get_student_from_request_data(certificate_invalidation_data) - certificate = _get_certificate_for_user(course_key, student) - except ValueError as error: - return JsonResponse({'message': str(error)}, status=400) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.CERTIFICATE_INVALIDATION_VIEW + http_method_names = ['post', 'delete'] - # Invalidate certificate of the given student for the course course - if request.method == 'POST': + @method_decorator(ensure_csrf_cookie) + # @method_decorator(transaction.non_atomic_requests) + def post(self, request, course_id): + course_key = CourseKey.from_string(course_id) + validation_response = self.validate_and_get_data(request, course_key) + # If validation fails, return the error response + if isinstance(validation_response, JsonResponse): + return validation_response + # Otherwise, extract the validated data + + student, certificate, certificate_invalidation_data = validation_response + # Invalidate certificate of the given student for the course course try: if certs_api.is_on_allowlist(student, course_key): log.warning(f"Invalidating certificate for student {student.id} in course {course_key} failed. " @@ -3582,8 +3584,20 @@ def certificate_invalidation_view(request, course_id): return JsonResponse({'message': str(error)}, status=400) return JsonResponse(certificate_invalidation) - # Re-Validate student certificate for the course course - elif request.method == 'DELETE': + @method_decorator(ensure_csrf_cookie) + # @method_decorator(transaction.non_atomic_requests) + def delete(self, request, course_id): + # Re-Validate student certificate for the course course + course_key = CourseKey.from_string(course_id) + validation_response = self.validate_and_get_data(request, course_key) + + # If validation fails, return the error response + if isinstance(validation_response, JsonResponse): + return validation_response + + # Otherwise, extract the validated data + student, certificate, certificate_invalidation_data = validation_response + try: re_validate_certificate(request, course_key, certificate, student) except ValueError as error: @@ -3591,6 +3605,17 @@ def certificate_invalidation_view(request, course_id): return JsonResponse({}, status=204) + def validate_and_get_data(self, request, course_key): + try: + certificate_invalidation_data = parse_request_data(request) + student = _get_student_from_request_data(certificate_invalidation_data) + certificate = _get_certificate_for_user(course_key, student) + # Return the validated student and certificate + return student, certificate, certificate_invalidation_data + except ValueError as error: + # Return a JsonResponse with an error message if validation fails + return JsonResponse({'message': str(error)}, status=400) + def invalidate_certificate(request, generated_certificate, certificate_invalidation_data, student): """ diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index a248b46ae531..281d072f382c 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -88,5 +88,9 @@ name='generate_certificate_exceptions'), path('generate_bulk_certificate_exceptions', api.generate_bulk_certificate_exceptions, name='generate_bulk_certificate_exceptions'), - path('certificate_invalidation_view/', api.certificate_invalidation_view, name='certificate_invalidation_view'), + path( + 'certificate_invalidation_view/', + api.CertificateInvalidationView.as_view(), + name='certificate_invalidation_view' + ), ] From bd9f05124a45a3cb2478193d4bc2b2688f35fd5e Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 23 Sep 2024 14:56:34 +0500 Subject: [PATCH 03/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 906e5df2f5a8..1613dbebdfd9 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3540,7 +3540,7 @@ def build_row_errors(key, _user, row_count): @method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') -# @method_decorator(transaction.non_atomic_requests, name='dispatch') +@method_decorator(transaction.non_atomic_requests, name='dispatch') class CertificateInvalidationView(APIView): """ Invalidate/Re-Validate students to/from certificate. @@ -3554,7 +3554,7 @@ class CertificateInvalidationView(APIView): http_method_names = ['post', 'delete'] @method_decorator(ensure_csrf_cookie) - # @method_decorator(transaction.non_atomic_requests) + @method_decorator(transaction.non_atomic_requests) def post(self, request, course_id): course_key = CourseKey.from_string(course_id) validation_response = self.validate_and_get_data(request, course_key) @@ -3585,7 +3585,7 @@ def post(self, request, course_id): return JsonResponse(certificate_invalidation) @method_decorator(ensure_csrf_cookie) - # @method_decorator(transaction.non_atomic_requests) + @method_decorator(transaction.non_atomic_requests) def delete(self, request, course_id): # Re-Validate student certificate for the course course course_key = CourseKey.from_string(course_id) From 9de5af8a17c6fa4be3f50b730b60949550ebf259 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 23 Sep 2024 14:59:42 +0500 Subject: [PATCH 04/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 1613dbebdfd9..383dc8bef098 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3558,6 +3558,7 @@ class CertificateInvalidationView(APIView): def post(self, request, course_id): course_key = CourseKey.from_string(course_id) validation_response = self.validate_and_get_data(request, course_key) + # If validation fails, return the error response if isinstance(validation_response, JsonResponse): return validation_response @@ -3606,6 +3607,16 @@ def delete(self, request, course_id): return JsonResponse({}, status=204) def validate_and_get_data(self, request, course_key): + """ + Validates the request data, retrieves the student and certificate for the specified course. + This method performs the following steps: + 1. Parses the request data to extract the necessary information. + 2. Retrieves the student object from the parsed request data. + 3. Fetches the certificate for the user and course specified by the course_key. + + If any of the steps fail (e.g., invalid request data or missing certificate), a ValueError is raised, + and the method returns a JsonResponse with a 400 status code. + """ try: certificate_invalidation_data = parse_request_data(request) student = _get_student_from_request_data(certificate_invalidation_data) From 5a8b4f33ff730047993b8061f876453cae718fb7 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Mon, 23 Sep 2024 23:49:42 +0500 Subject: [PATCH 05/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 28 ++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 383dc8bef098..d10ec60258dd 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3371,6 +3371,21 @@ def parse_request_data(request): return data +def parse_request_data_drf(request): + """ + Parse and return request data, raise ValueError in case of invalid JSON data. + + :param request: HttpRequest request object. + :return: dict object containing parsed json data. + """ + try: + data = request.data or '{}' + except ValueError: + raise ValueError(_('The record is not in the correct format. Please add a valid username or email address.')) # lint-amnesty, pylint: disable=raise-missing-from + + return data + + def get_student(username_or_email): """ Retrieve and return User object from db, raise ValueError @@ -3556,9 +3571,13 @@ class CertificateInvalidationView(APIView): @method_decorator(ensure_csrf_cookie) @method_decorator(transaction.non_atomic_requests) def post(self, request, course_id): + """ + Invalidate/Re-Validate students to/from certificate. + """ course_key = CourseKey.from_string(course_id) - validation_response = self.validate_and_get_data(request, course_key) + # Validate request data and return error response in case of invalid data + validation_response = self.validate_and_get_data(request, course_key) # If validation fails, return the error response if isinstance(validation_response, JsonResponse): return validation_response @@ -3588,8 +3607,13 @@ def post(self, request, course_id): @method_decorator(ensure_csrf_cookie) @method_decorator(transaction.non_atomic_requests) def delete(self, request, course_id): + """ + Invalidate/Re-Validate students to/from certificate. + """ # Re-Validate student certificate for the course course course_key = CourseKey.from_string(course_id) + # Validate request data and return error response in case of invalid data + validation_response = self.validate_and_get_data(request, course_key) # If validation fails, return the error response @@ -3618,7 +3642,7 @@ def validate_and_get_data(self, request, course_key): and the method returns a JsonResponse with a 400 status code. """ try: - certificate_invalidation_data = parse_request_data(request) + certificate_invalidation_data = parse_request_data_drf(request) student = _get_student_from_request_data(certificate_invalidation_data) certificate = _get_certificate_for_user(course_key, student) # Return the validated student and certificate From e27b8bc4459e4a702a7a18efe229e1be71cdf1c7 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 12:41:44 +0500 Subject: [PATCH 06/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d10ec60258dd..b4f792716219 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3363,6 +3363,7 @@ def parse_request_data(request): :param request: HttpRequest request object. :return: dict object containing parsed json data. """ + # For DELETE requests: Django REST Framework does not automatically parse the request body. try: data = json.loads(request.body.decode('utf8') or '{}') except ValueError: @@ -3371,17 +3372,23 @@ def parse_request_data(request): return data -def parse_request_data_drf(request): +def parse_request_data_drf(request, method): """ + This method is used in case of DRF apis. + Parse and return request data, raise ValueError in case of invalid JSON data. :param request: HttpRequest request object. :return: dict object containing parsed json data. """ try: - data = request.data or '{}' + if method == 'post': # For POST requests Django REST Framework automatically parses the request body. + data = request.data or '{}' + elif method == 'delete': # For DELETE requests Django REST Framework does not automatically parse the body. + data = json.loads(request.body.decode('utf8') or '{}') except ValueError: - raise ValueError(_('The record is not in the correct format. Please add a valid username or email address.')) # lint-amnesty, pylint: disable=raise-missing-from + raise ValueError( + _('The record is not in the correct format. Please add a valid username or email address.')) # lint-amnesty, pylint: disable=raise-missing-from return data @@ -3577,7 +3584,7 @@ def post(self, request, course_id): course_key = CourseKey.from_string(course_id) # Validate request data and return error response in case of invalid data - validation_response = self.validate_and_get_data(request, course_key) + validation_response = self.validate_and_get_data(course_key, "post") # If validation fails, return the error response if isinstance(validation_response, JsonResponse): return validation_response @@ -3613,8 +3620,7 @@ def delete(self, request, course_id): # Re-Validate student certificate for the course course course_key = CourseKey.from_string(course_id) # Validate request data and return error response in case of invalid data - - validation_response = self.validate_and_get_data(request, course_key) + validation_response = self.validate_and_get_data(course_key, "delete") # If validation fails, return the error response if isinstance(validation_response, JsonResponse): @@ -3630,7 +3636,7 @@ def delete(self, request, course_id): return JsonResponse({}, status=204) - def validate_and_get_data(self, request, course_key): + def validate_and_get_data(self, course_key, method): """ Validates the request data, retrieves the student and certificate for the specified course. This method performs the following steps: @@ -3642,7 +3648,7 @@ def validate_and_get_data(self, request, course_key): and the method returns a JsonResponse with a 400 status code. """ try: - certificate_invalidation_data = parse_request_data_drf(request) + certificate_invalidation_data = parse_request_data_drf(self.request, method) student = _get_student_from_request_data(certificate_invalidation_data) certificate = _get_certificate_for_user(course_key, student) # Return the validated student and certificate From 34b317fe63475075fd68ee19a557e4c47cd7228f Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 15:31:19 +0500 Subject: [PATCH 07/20] feat!: upgrading api to DRF. --- .../instructor/tests/test_certificates.py | 8 +- lms/djangoapps/instructor/views/api.py | 86 ++++++++++--------- lms/djangoapps/instructor/views/serializer.py | 22 +++++ 3 files changed, 70 insertions(+), 46 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index ce3433f312d8..8d6ebacd748a 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -1085,9 +1085,7 @@ def test_missing_username_and_email_error(self): res_json = json.loads(response.content.decode('utf-8')) # 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 "Invalidate Certificate" button.' + assert res_json['errors'] == {'user': ['This field may not be blank.']} def test_invalid_user_name_error(self): """ @@ -1106,9 +1104,9 @@ def test_invalid_user_name_error(self): # Assert 400 status code in response assert response.status_code == 400 res_json = json.loads(response.content.decode('utf-8')) - # Assert Error Message - assert res_json['message'] == f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.' + assert res_json['errors'] == ('test_invalid_user_name does not exist in the LMS. ' + 'Please check your spelling and retry.') def test_no_generated_certificate_error(self): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index b4f792716219..28284eb111ad 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -36,6 +36,8 @@ from edx_when.api import get_date_for_block from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey + +from cms.djangoapps.contentstore.views.transcripts_ajax import error_response from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name from rest_framework.exceptions import MethodNotAllowed from rest_framework import serializers, status # lint-amnesty, pylint: disable=wrong-import-order @@ -107,8 +109,15 @@ from lms.djangoapps.instructor_task.data import InstructorTaskTypes from lms.djangoapps.instructor_task.models import ReportStore from lms.djangoapps.instructor.views.serializer import ( - AccessSerializer, BlockDueDateSerializer, RoleNameSerializer, ShowStudentExtensionSerializer, UserSerializer, - SendEmailSerializer, StudentAttemptsSerializer, ListInstructorTaskInputSerializer + AccessSerializer, + BlockDueDateSerializer, + CertificateSerializer, + ListInstructorTaskInputSerializer, + RoleNameSerializer, + SendEmailSerializer, + ShowStudentExtensionSerializer, + StudentAttemptsSerializer, + UserSerializer ) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted @@ -3573,6 +3582,7 @@ class CertificateInvalidationView(APIView): """ permission_classes = (IsAuthenticated, permissions.InstructorPermission) permission_name = permissions.CERTIFICATE_INVALIDATION_VIEW + serializer_class = CertificateSerializer http_method_names = ['post', 'delete'] @method_decorator(ensure_csrf_cookie) @@ -3583,14 +3593,25 @@ def post(self, request, course_id): """ course_key = CourseKey.from_string(course_id) # Validate request data and return error response in case of invalid data + serializer_data = self.serializer_class(data=request.data) + if not serializer_data.is_valid(): + # return HttpResponseBadRequest(reason=serializer_data.errors) + return JsonResponse({'errors': serializer_data.errors}, status=400) + + student = serializer_data.validated_data.get('user') + notes = serializer_data.validated_data.get('notes') + + if not student: + invalid_user = request.data.get('user') + response_payload = f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.' - validation_response = self.validate_and_get_data(course_key, "post") - # If validation fails, return the error response - if isinstance(validation_response, JsonResponse): - return validation_response - # Otherwise, extract the validated data + return JsonResponse({'errors': response_payload}, status=400) + + try: + certificate = _get_certificate_for_user(course_key, student) + except Exception as ex: + return JsonResponse({'errors': str(ex)}, status=400) - student, certificate, certificate_invalidation_data = validation_response # Invalidate certificate of the given student for the course course try: if certs_api.is_on_allowlist(student, course_key): @@ -3604,9 +3625,10 @@ def post(self, request, course_id): certificate_invalidation = invalidate_certificate( request, certificate, - certificate_invalidation_data, + notes, student ) + except ValueError as error: return JsonResponse({'message': str(error)}, status=400) return JsonResponse(certificate_invalidation) @@ -3619,15 +3641,18 @@ def delete(self, request, course_id): """ # Re-Validate student certificate for the course course course_key = CourseKey.from_string(course_id) - # Validate request data and return error response in case of invalid data - validation_response = self.validate_and_get_data(course_key, "delete") + try: + data = json.loads(self.request.body.decode('utf8') or '{}') + except Exception: # pylint: disable=broad-except + data = {} + + serializer_data = self.serializer_class(data=data) - # If validation fails, return the error response - if isinstance(validation_response, JsonResponse): - return validation_response + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) - # Otherwise, extract the validated data - student, certificate, certificate_invalidation_data = validation_response + student = serializer_data.validated_data.get('user') + certificate = _get_certificate_for_user(course_key, student) try: re_validate_certificate(request, course_key, certificate, student) @@ -3636,35 +3661,14 @@ def delete(self, request, course_id): return JsonResponse({}, status=204) - def validate_and_get_data(self, course_key, method): - """ - Validates the request data, retrieves the student and certificate for the specified course. - This method performs the following steps: - 1. Parses the request data to extract the necessary information. - 2. Retrieves the student object from the parsed request data. - 3. Fetches the certificate for the user and course specified by the course_key. - - If any of the steps fail (e.g., invalid request data or missing certificate), a ValueError is raised, - and the method returns a JsonResponse with a 400 status code. - """ - try: - certificate_invalidation_data = parse_request_data_drf(self.request, method) - student = _get_student_from_request_data(certificate_invalidation_data) - certificate = _get_certificate_for_user(course_key, student) - # Return the validated student and certificate - return student, certificate, certificate_invalidation_data - except ValueError as error: - # Return a JsonResponse with an error message if validation fails - return JsonResponse({'message': str(error)}, status=400) - -def invalidate_certificate(request, generated_certificate, certificate_invalidation_data, student): +def invalidate_certificate(request, generated_certificate, notes, student): """ Invalidate given GeneratedCertificate and add CertificateInvalidation record for future reference or re-validation. :param request: HttpRequest object :param generated_certificate: GeneratedCertificate object, the certificate we want to invalidate - :param certificate_invalidation_data: dict object containing data for CertificateInvalidation. + :param notes: notes values. :param student: User object, this user is tied to the generated_certificate we are going to invalidate :return: dict object containing updated certificate invalidation data. """ @@ -3687,7 +3691,7 @@ def invalidate_certificate(request, generated_certificate, certificate_invalidat certificate_invalidation = certs_api.create_certificate_invalidation_entry( generated_certificate, request.user, - certificate_invalidation_data.get("notes", ""), + notes, ) # Invalidate the certificate @@ -3698,7 +3702,7 @@ def invalidate_certificate(request, generated_certificate, certificate_invalidat 'user': student.username, 'invalidated_by': certificate_invalidation.invalidated_by.username, 'created': certificate_invalidation.created.strftime("%B %d, %Y"), - 'notes': certificate_invalidation.notes, + 'notes': notes, } diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 5d123ad66c81..e685c4e27646 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -222,3 +222,25 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if disable_due_datetime: self.fields['due_datetime'].required = False + + +class CertificateSerializer(serializers.Serializer): + """ + Serializer for resetting a students attempts counter or starts a task to reset all students + attempts counters. + """ + notes = serializers.CharField(max_length=128, write_only=True, required=False) + user = serializers.CharField( + help_text="Email or username of student.", required=True + ) + + def validate_user(self, value): + """ + Validate that the user corresponds to an existing user. + """ + try: + user = get_student_from_identifier(value) + except User.DoesNotExist: + return None + + return user From bc78825981c6c71fa8fcdcb2906547f563660f85 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 16:16:23 +0500 Subject: [PATCH 08/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 368 +++++++++++++++---------- 1 file changed, 217 insertions(+), 151 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 28284eb111ad..6514530e3c2b 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -36,8 +36,6 @@ from edx_when.api import get_date_for_block from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey - -from cms.djangoapps.contentstore.views.transcripts_ajax import error_response from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_name from rest_framework.exceptions import MethodNotAllowed from rest_framework import serializers, status # lint-amnesty, pylint: disable=wrong-import-order @@ -1685,18 +1683,31 @@ def get_proctored_exam_results(request, course_id): return JsonResponse({"status": success_status}) -@transaction.non_atomic_requests -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.CAN_RESEARCH) -def get_anon_ids(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 GetAnonIds(APIView): """ - Respond with 2-column CSV output of user-id, anonymized-user-id + Respond with 2-column CSV output of user-id, anonymized-user-id. + This API processes the incoming request to generate a CSV file containing + two columns: `user-id` and `anonymized-user-id`. The CSV is returned as a + response to the client. """ - report_type = _('Anonymized User IDs') - success_status = SUCCESS_MESSAGE_TEMPLATE.format(report_type=report_type) - task_api.generate_anonymous_ids(request, course_id) - return JsonResponse({"status": success_status}) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.CAN_RESEARCH + + @method_decorator(ensure_csrf_cookie) + @method_decorator(transaction.non_atomic_requests) + def post(self, request, course_id): + """ + Handle POST request to generate a CSV output. + + Returns: + Response: A CSV file with two columns: `user-id` and `anonymized-user-id`. + """ + report_type = _('Anonymized User IDs') + success_status = SUCCESS_MESSAGE_TEMPLATE.format(report_type=report_type) + task_api.generate_anonymous_ids(request, course_id) + return JsonResponse({"status": success_status}) @require_POST @@ -1814,23 +1825,24 @@ def post(self, request, course_id): return Response(serializer.data) -@transaction.non_atomic_requests -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.GIVE_STUDENT_EXTENSION) -@require_post_params( - problem_to_reset="problem urlname to reset" -) -@common_exceptions_400 -def reset_student_attempts(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 ResetStudentAttempts(DeveloperErrorViewMixin, APIView): """ - Resets a students attempts counter or starts a task to reset all students attempts counters. Optionally deletes student state for a problem. Limited to staff access. Some sub-methods limited to instructor access. + """ + http_method_names = ['post'] + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.GIVE_STUDENT_EXTENSION + serializer_class = StudentAttemptsSerializer - Takes some of the following query parameters + @method_decorator(ensure_csrf_cookie) + @transaction.non_atomic_requests + def post(self, request, course_id): + """ + Takes some of the following query parameters - problem_to_reset is a urlname of a problem - unique_student_identifier is an email or username - all_students is a boolean @@ -1840,65 +1852,74 @@ def reset_student_attempts(request, course_id): - delete_module is a boolean requires instructor access mutually exclusive with all_students - """ - course_id = CourseKey.from_string(course_id) - course = get_course_with_access( - request.user, 'staff', course_id, depth=None - ) - all_students = _get_boolean_param(request, 'all_students') - - if all_students and not has_access(request.user, 'instructor', course): - return HttpResponseForbidden("Requires instructor access.") + """ + course_id = CourseKey.from_string(course_id) + serializer_data = self.serializer_class(data=request.data) - problem_to_reset = strip_if_string(request.POST.get('problem_to_reset')) - student_identifier = request.POST.get('unique_student_identifier', None) - student = None - if student_identifier is not None: - student = get_student_from_identifier(student_identifier) - delete_module = _get_boolean_param(request, 'delete_module') + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) - # parameter combinations - if all_students and student: - return HttpResponseBadRequest( - "all_students and unique_student_identifier are mutually exclusive." - ) - if all_students and delete_module: - return HttpResponseBadRequest( - "all_students and delete_module are mutually exclusive." + course = get_course_with_access( + request.user, 'staff', course_id, depth=None ) - try: - module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) - except InvalidKeyError: - return HttpResponseBadRequest() + all_students = serializer_data.validated_data.get('all_students') - response_payload = {} - response_payload['problem_to_reset'] = problem_to_reset + if all_students and not has_access(request.user, 'instructor', course): + return HttpResponseForbidden("Requires instructor access.") - if student: - try: - enrollment.reset_student_attempts( - course_id, - student, - module_state_key, - requesting_user=request.user, - delete_module=delete_module + problem_to_reset = strip_if_string(serializer_data.validated_data.get('problem_to_reset')) + student_identifier = request.POST.get('unique_student_identifier', None) + student = serializer_data.validated_data.get('unique_student_identifier') + delete_module = serializer_data.validated_data.get('delete_module') + + # parameter combinations + if all_students and student: + return HttpResponseBadRequest( + "all_students and unique_student_identifier are mutually exclusive." + ) + if all_students and delete_module: + return HttpResponseBadRequest( + "all_students and delete_module are mutually exclusive." ) - except StudentModule.DoesNotExist: - return HttpResponseBadRequest(_("Module does not exist.")) - except sub_api.SubmissionError: - # Trust the submissions API to log the error - error_msg = _("An error occurred while deleting the score.") - return HttpResponse(error_msg, status=500) - response_payload['student'] = student_identifier - elif all_students: - task_api.submit_reset_problem_attempts_for_all_students(request, module_state_key) - response_payload['task'] = TASK_SUBMISSION_OK - response_payload['student'] = 'All Students' - else: - return HttpResponseBadRequest() - return JsonResponse(response_payload) + try: + module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) + except InvalidKeyError: + return HttpResponseBadRequest() + + response_payload = {} + response_payload['problem_to_reset'] = problem_to_reset + + if student: + try: + enrollment.reset_student_attempts( + course_id, + student, + module_state_key, + requesting_user=request.user, + delete_module=delete_module + ) + except StudentModule.DoesNotExist: + return HttpResponseBadRequest(_("Module does not exist.")) + except sub_api.SubmissionError: + # Trust the submissions API to log the error + error_msg = _("An error occurred while deleting the score.") + return HttpResponse(error_msg, status=500) + response_payload['student'] = student_identifier + + elif all_students: + try: + task_api.submit_reset_problem_attempts_for_all_students(request, module_state_key) + response_payload['task'] = TASK_SUBMISSION_OK + response_payload['student'] = 'All Students' + except Exception: # pylint: disable=broad-except + error_msg = _("An error occurred while attempting to reset for all students.") + return HttpResponse(error_msg, status=500) + else: + return HttpResponseBadRequest() + + return JsonResponse(response_payload) @transaction.non_atomic_requests @@ -1935,8 +1956,10 @@ def reset_student_attempts_for_entrance_exam(request, course_id): student_identifier = request.POST.get('unique_student_identifier', None) student = None + if student_identifier is not None: student = get_student_from_identifier(student_identifier) + all_students = _get_boolean_param(request, 'all_students') delete_module = _get_boolean_param(request, 'delete_module') @@ -2358,9 +2381,8 @@ def get(self, request, course_id): return _list_instructor_tasks(request=request, course_id=course_id) -@require_POST -@ensure_csrf_cookie -def list_instructor_tasks(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class ListInstructorTasks(APIView): """ List instructor tasks. @@ -2370,21 +2392,44 @@ def list_instructor_tasks(request, course_id): - `problem_location_str` and `unique_student_identifier` lists task history for problem AND student (intersection) """ - return _list_instructor_tasks(request=request, course_id=course_id) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.SHOW_TASKS + serializer_class = ListInstructorTaskInputSerializer + + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """ + List instructor tasks. + """ + serializer = self.serializer_class(data=request.data) + serializer.is_valid(raise_exception=True) + + return _list_instructor_tasks( + request=request, course_id=course_id, serialize_data=serializer.validated_data + ) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_course_permission(permissions.SHOW_TASKS) -def _list_instructor_tasks(request, course_id): +def _list_instructor_tasks(request, course_id, serialize_data=None): """ List instructor tasks. Internal function with common code for both DRF and and tradition views. """ + # This method is also used by other APIs with the GET method. + # The query_params attribute is utilized for GET requests, + # where parameters are passed as query strings. + course_id = CourseKey.from_string(course_id) - params = getattr(request, 'query_params', request.POST) - problem_location_str = strip_if_string(params.get('problem_location_str', False)) - student = params.get('unique_student_identifier', None) + if serialize_data is not None: + problem_location_str = strip_if_string(serialize_data.get('problem_location_str', False)) + student = serialize_data.get('unique_student_identifier', None) + else: + params = getattr(request, 'query_params', request.POST) + problem_location_str = strip_if_string(params.get('problem_location_str', False)) + student = params.get('unique_student_identifier', None) + if student is not None: student = get_student_from_identifier(student) @@ -2538,16 +2583,22 @@ def get(self, request, course_id): return _list_report_downloads(request=request, course_id=course_id) -@require_POST -@ensure_csrf_cookie -def list_report_downloads(request, course_id): +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class ListReportDownloads(APIView): + """ List grade CSV files that are available for download for this course. Takes the following query parameters: - (optional) report_name - name of the report """ - return _list_report_downloads(request=request, course_id=course_id) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.CAN_RESEARCH + + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + + return _list_report_downloads(request=request, course_id=course_id) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -2761,81 +2812,96 @@ def extract_user_info(user): return JsonResponse(response_payload) -@transaction.non_atomic_requests -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.EMAIL) -@require_post_params(send_to="sending to whom", subject="subject line", message="message text") -@common_exceptions_400 -def send_email(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 SendEmail(DeveloperErrorViewMixin, APIView): """ Send an email to self, staff, cohorts, or everyone involved in a course. - Query Parameters: - - 'send_to' specifies what group the email should be sent to - Options are defined by the CourseEmail model in - lms/djangoapps/bulk_email/models.py - - 'subject' specifies email's subject - - 'message' specifies email's content """ - course_id = CourseKey.from_string(course_id) - course_overview = CourseOverview.get_from_id(course_id) + http_method_names = ['post'] + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + permission_name = permissions.EMAIL + serializer_class = SendEmailSerializer - if not is_bulk_email_feature_enabled(course_id): - log.warning(f"Email is not enabled for course {course_id}") - return HttpResponseForbidden("Email is not enabled for this course.") + @method_decorator(ensure_csrf_cookie) + @method_decorator(transaction.non_atomic_requests) + def post(self, request, course_id): + """ + Query Parameters: + - 'send_to' specifies what group the email should be sent to + Options are defined by the CourseEmail model in + lms/djangoapps/bulk_email/models.py + - 'subject' specifies email's subject + - 'message' specifies email's content + """ + course_id = CourseKey.from_string(course_id) + course_overview = CourseOverview.get_from_id(course_id) - targets = json.loads(request.POST.get("send_to")) - subject = request.POST.get("subject") - message = request.POST.get("message") - # optional, this is a date and time in the form of an ISO8601 string - schedule = request.POST.get("schedule", "") + if not is_bulk_email_feature_enabled(course_id): + log.warning(f"Email is not enabled for course {course_id}") + return HttpResponseForbidden("Email is not enabled for this course.") - schedule_dt = None - if schedule: - try: - # convert the schedule from a string to a datetime, then check if its a valid future date and time, dateutil - # will throw a ValueError if the schedule is no good. - schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) - if schedule_dt < datetime.datetime.now(pytz.utc): - raise ValueError("the requested schedule is in the past") - except ValueError as value_error: - error_message = ( - f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " - f"{value_error}" - ) - log.error(error_message) - return HttpResponseBadRequest(error_message) + serializer_data = self.serializer_class(data=request.data) + if not serializer_data.is_valid(): + return HttpResponseBadRequest(reason=serializer_data.errors) - # Retrieve the customized email "from address" and email template from site configuration for the course/partner. If - # there is no site configuration enabled for the current site then we use system defaults for both. - from_addr = _get_branded_email_from_address(course_overview) - template_name = _get_branded_email_template(course_overview) + # Skipping serializer validation to avoid potential disruptions. + # The API handles numerous input variations, and changes here could introduce breaking issues. - # Create the CourseEmail object. This is saved immediately so that any transaction that has been pending up to this - # point will also be committed. - try: - email = create_course_email( - course_id, - request.user, - targets, - subject, - message, - template_name=template_name, - from_addr=from_addr, - ) - except ValueError as err: - return HttpResponseBadRequest(repr(err)) + targets = json.loads(request.POST.get("send_to")) - # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) - task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) + subject = serializer_data.validated_data.get("subject") + message = serializer_data.validated_data.get("message") + # optional, this is a date and time in the form of an ISO8601 string + schedule = serializer_data.validated_data.get("schedule", "") - response_payload = { - 'course_id': str(course_id), - 'success': True, - } + schedule_dt = None + if schedule: + try: + # convert the schedule from a string to a datetime, then check if its a + # valid future date and time, dateutil + # will throw a ValueError if the schedule is no good. + schedule_dt = dateutil.parser.parse(schedule).replace(tzinfo=pytz.utc) + if schedule_dt < datetime.datetime.now(pytz.utc): + raise ValueError("the requested schedule is in the past") + except ValueError as value_error: + error_message = ( + f"Error occurred creating a scheduled bulk email task. Schedule provided: '{schedule}'. Error: " + f"{value_error}" + ) + log.error(error_message) + return HttpResponseBadRequest(error_message) - return JsonResponse(response_payload) + # Retrieve the customized email "from address" and email template from site configuration for the c + # ourse/partner. + # If there is no site configuration enabled for the current site then we use system defaults for both. + from_addr = _get_branded_email_from_address(course_overview) + template_name = _get_branded_email_template(course_overview) + + # Create the CourseEmail object. This is saved immediately so that any transaction that has been + # pending up to this point will also be committed. + try: + email = create_course_email( + course_id, + request.user, + targets, + subject, + message, + template_name=template_name, + from_addr=from_addr, + ) + except ValueError as err: + return HttpResponseBadRequest(repr(err)) + + # Submit the task, so that the correct InstructorTask object gets created (for monitoring purposes) + task_api.submit_bulk_course_email(request, course_id, email.id, schedule_dt) + + response_payload = { + 'course_id': str(course_id), + 'success': True, + } + + return JsonResponse(response_payload) @require_POST From a4c678d3c3d5d6a63abff7da829cba484c0af65d Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 16:21:05 +0500 Subject: [PATCH 09/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 24 +----------------------- 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6514530e3c2b..7678a0a47126 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3061,7 +3061,7 @@ def post(self, request, course_id): student (str): The email or username of the student whose access is being modified. reason (str): Optional param. """ - serializer_data = self.serializer_class(data=request.data, context={'make_due_datetime': True}) + serializer_data = self.serializer_class(data=request.data, context={'disable_due_datetime': True}) if not serializer_data.is_valid(): return HttpResponseBadRequest(reason=serializer_data.errors) @@ -3438,7 +3438,6 @@ def parse_request_data(request): :param request: HttpRequest request object. :return: dict object containing parsed json data. """ - # For DELETE requests: Django REST Framework does not automatically parse the request body. try: data = json.loads(request.body.decode('utf8') or '{}') except ValueError: @@ -3447,27 +3446,6 @@ def parse_request_data(request): return data -def parse_request_data_drf(request, method): - """ - This method is used in case of DRF apis. - - Parse and return request data, raise ValueError in case of invalid JSON data. - - :param request: HttpRequest request object. - :return: dict object containing parsed json data. - """ - try: - if method == 'post': # For POST requests Django REST Framework automatically parses the request body. - data = request.data or '{}' - elif method == 'delete': # For DELETE requests Django REST Framework does not automatically parse the body. - data = json.loads(request.body.decode('utf8') or '{}') - except ValueError: - raise ValueError( - _('The record is not in the correct format. Please add a valid username or email address.')) # lint-amnesty, pylint: disable=raise-missing-from - - return data - - def get_student(username_or_email): """ Retrieve and return User object from db, raise ValueError From 67bde49c160112e527ccf608a65c7e21a1b902b2 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 16:24:30 +0500 Subject: [PATCH 10/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/tests/test_certificates.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 8d6ebacd748a..5aa2be766759 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -1123,9 +1123,8 @@ def test_no_generated_certificate_error(self): # Assert 400 status code in response assert response.status_code == 400 res_json = json.loads(response.content.decode('utf-8')) - # Assert Error Message - assert res_json['message'] == f'The student {self.enrolled_user_2.username} does not have certificate for the course {self.course.number}. Kindly verify student username/email and the selected course are correct and try again.' # pylint: disable=line-too-long + assert res_json['errors'] == f'The student {self.enrolled_user_2.username} does not have certificate for the course {self.course.number}. Kindly verify student username/email and the selected course are correct and try again.' # pylint: disable=line-too-long def test_certificate_already_invalid_error(self): """ From ee6045cb42ab34115daabaa90cdf02953e5930ed Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 16:33:38 +0500 Subject: [PATCH 11/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/tests/test_certificates.py | 4 ++-- lms/djangoapps/instructor/views/api.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 5aa2be766759..88d991be9af0 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -1105,7 +1105,7 @@ def test_invalid_user_name_error(self): assert response.status_code == 400 res_json = json.loads(response.content.decode('utf-8')) # Assert Error Message - assert res_json['errors'] == ('test_invalid_user_name does not exist in the LMS. ' + assert res_json['message'] == ('test_invalid_user_name does not exist in the LMS. ' 'Please check your spelling and retry.') def test_no_generated_certificate_error(self): @@ -1124,7 +1124,7 @@ def test_no_generated_certificate_error(self): assert response.status_code == 400 res_json = json.loads(response.content.decode('utf-8')) # Assert Error Message - assert res_json['errors'] == f'The student {self.enrolled_user_2.username} does not have certificate for the course {self.course.number}. Kindly verify student username/email and the selected course are correct and try again.' # pylint: disable=line-too-long + assert res_json['message'] == f'The student {self.enrolled_user_2.username} does not have certificate for the course {self.course.number}. Kindly verify student username/email and the selected course are correct and try again.' # pylint: disable=line-too-long def test_certificate_already_invalid_error(self): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 7678a0a47126..2bc88637443f 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3649,12 +3649,12 @@ def post(self, request, course_id): invalid_user = request.data.get('user') response_payload = f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.' - return JsonResponse({'errors': response_payload}, status=400) + return JsonResponse({'message': response_payload}, status=400) try: certificate = _get_certificate_for_user(course_key, student) except Exception as ex: - return JsonResponse({'errors': str(ex)}, status=400) + return JsonResponse({'message': str(ex)}, status=400) # Invalidate certificate of the given student for the course course try: From 4c3f2245e2898ac74f5389894e6d566e21e891de Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 16:36:28 +0500 Subject: [PATCH 12/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/tests/test_certificates.py | 2 +- lms/djangoapps/instructor/views/api.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 88d991be9af0..4c22bd204e6f 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -1085,7 +1085,7 @@ def test_missing_username_and_email_error(self): res_json = json.loads(response.content.decode('utf-8')) # Assert Error Message - assert res_json['errors'] == {'user': ['This field may not be blank.']} + assert res_json['message'] == {'user': ['This field may not be blank.']} def test_invalid_user_name_error(self): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 2bc88637443f..6e6b87677621 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3640,7 +3640,7 @@ def post(self, request, course_id): serializer_data = self.serializer_class(data=request.data) if not serializer_data.is_valid(): # return HttpResponseBadRequest(reason=serializer_data.errors) - return JsonResponse({'errors': serializer_data.errors}, status=400) + return JsonResponse({'message': serializer_data.errors}, status=400) student = serializer_data.validated_data.get('user') notes = serializer_data.validated_data.get('notes') From e1e32caec1aaf1bb8b245259aaff3ea590f4fab8 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 22:47:41 +0500 Subject: [PATCH 13/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/serializer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index e685c4e27646..7b95a467662d 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -229,10 +229,10 @@ class CertificateSerializer(serializers.Serializer): Serializer for resetting a students attempts counter or starts a task to reset all students attempts counters. """ - notes = serializers.CharField(max_length=128, write_only=True, required=False) user = serializers.CharField( help_text="Email or username of student.", required=True ) + notes = serializers.CharField(required=False, allow_null=True) def validate_user(self, value): """ From bd11e8cf488026b2983420a76a74a2393b435d41 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 23:02:57 +0500 Subject: [PATCH 14/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/tests/test_certificates.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 4c22bd204e6f..16cbac9ee8cd 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -1105,8 +1105,7 @@ def test_invalid_user_name_error(self): assert response.status_code == 400 res_json = json.loads(response.content.decode('utf-8')) # Assert Error Message - assert res_json['message'] == ('test_invalid_user_name does not exist in the LMS. ' - 'Please check your spelling and retry.') + assert res_json['message'] == f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.' def test_no_generated_certificate_error(self): """ From 405cc1baba88460aa20ba38fe7ca74ed822ba827 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 23:09:44 +0500 Subject: [PATCH 15/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/tests/test_certificates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index 16cbac9ee8cd..b24ef618c7ce 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -1105,7 +1105,7 @@ def test_invalid_user_name_error(self): assert response.status_code == 400 res_json = json.loads(response.content.decode('utf-8')) # Assert Error Message - assert res_json['message'] == f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.' + assert res_json['message'] == f'{invalid_user} does not exist in the LMS. Please check your spelling and retry.' def test_no_generated_certificate_error(self): """ From 9a28eacbe649dbe834a9a52b28550a2b28a8db60 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Tue, 24 Sep 2024 23:29:18 +0500 Subject: [PATCH 16/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 6e6b87677621..4c6802d63347 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3653,7 +3653,7 @@ def post(self, request, course_id): try: certificate = _get_certificate_for_user(course_key, student) - except Exception as ex: + except Exception as ex: # pylint: disable=broad-except return JsonResponse({'message': str(ex)}, status=400) # Invalidate certificate of the given student for the course course From dbd8aba23297c22657151b0ab820df73e63f6679 Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Wed, 25 Sep 2024 09:48:38 +0500 Subject: [PATCH 17/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 4c6802d63347..898bfcb8b43f 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3696,7 +3696,12 @@ def delete(self, request, course_id): return HttpResponseBadRequest(reason=serializer_data.errors) student = serializer_data.validated_data.get('user') - certificate = _get_certificate_for_user(course_key, student) + + try: + certificate = _get_certificate_for_user(course_key, student) + except Exception as ex: # pylint: disable=broad-except + return JsonResponse({'message': str(ex)}, status=400) + try: re_validate_certificate(request, course_key, certificate, student) From 9bc3e481ce089203c1e99bd1ffce4ebd5ab66d7b Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Wed, 25 Sep 2024 11:11:40 +0500 Subject: [PATCH 18/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/api.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 898bfcb8b43f..7d9cdc3c7ae4 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3702,7 +3702,6 @@ def delete(self, request, course_id): except Exception as ex: # pylint: disable=broad-except return JsonResponse({'message': str(ex)}, status=400) - try: re_validate_certificate(request, course_key, certificate, student) except ValueError as error: From f041dc5ca8c50f0c50be7d24d1039ecf155262fd Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Fri, 4 Oct 2024 17:52:39 +0500 Subject: [PATCH 19/20] chore: Update api.py --- lms/djangoapps/instructor/views/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index e4e522fedba4..b5f940ba4578 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -115,7 +115,8 @@ SendEmailSerializer, ShowStudentExtensionSerializer, StudentAttemptsSerializer, - UserSerializer + UserSerializer, + UniqueStudentIdentifierSerializer ) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted From e3caa8d0fb32641e65fb6996d009b3a3957f40bc Mon Sep 17 00:00:00 2001 From: awais qureshi Date: Fri, 4 Oct 2024 18:08:48 +0500 Subject: [PATCH 20/20] feat!: upgrading api to DRF. --- lms/djangoapps/instructor/views/serializer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/serializer.py b/lms/djangoapps/instructor/views/serializer.py index 9527ace44da5..2ac794bc2943 100644 --- a/lms/djangoapps/instructor/views/serializer.py +++ b/lms/djangoapps/instructor/views/serializer.py @@ -238,7 +238,7 @@ class CertificateSerializer(serializers.Serializer): user = serializers.CharField( help_text="Email or username of student.", required=True ) - notes = serializers.CharField(required=False, allow_null=True) + notes = serializers.CharField(required=False, allow_null=True, allow_blank=True) def validate_user(self, value): """