From 39dd3c002b296a205e867f147a90a4eea6aee8c3 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Tue, 30 Jul 2024 13:18:49 +0500 Subject: [PATCH] feat: converting existing api to drf base api. (#35039) Adding generic permission class. Added standard authentication classes. --- lms/djangoapps/instructor/permissions.py | 12 +++- lms/djangoapps/instructor/tests/test_api.py | 51 ++++++++++++++- lms/djangoapps/instructor/views/api.py | 71 ++++++++++++++------- lms/djangoapps/instructor/views/api_urls.py | 3 +- 4 files changed, 111 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index e1a1cbf466f6..24e0079fcce3 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -1,11 +1,13 @@ """ Permissions for the instructor dashboard and associated actions """ - from bridgekeeper import perms from bridgekeeper.rules import is_staff +from opaque_keys.edx.keys import CourseKey +from rest_framework.permissions import BasePermission from lms.djangoapps.courseware.rules import HasAccessRule, HasRolesRule +from openedx.core.lib.courses import get_course_by_id ALLOW_STUDENT_TO_BYPASS_ENTRANCE_EXAM = 'instructor.allow_student_to_bypass_entrance_exam' ASSIGN_TO_COHORTS = 'instructor.assign_to_cohorts' @@ -72,3 +74,11 @@ ) | HasAccessRule('staff') | HasAccessRule('instructor') perms[VIEW_ENROLLMENTS] = HasAccessRule('staff') perms[VIEW_FORUM_MEMBERS] = HasAccessRule('staff') + + +class InstructorPermission(BasePermission): + """Generic permissions""" + def has_permission(self, request, view): + course = get_course_by_id(CourseKey.from_string(view.kwargs.get('course_id'))) + permission = getattr(view, 'permission_name', None) + return request.user.has_perm(permission, course) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index f5d8b0408950..2548cc4f411d 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -2945,7 +2945,37 @@ def test_get_student_progress_url(self): response = self.client.post(url, data) assert response.status_code == 200 res_json = json.loads(response.content.decode('utf-8')) - assert 'progress_url' in res_json + expected_data = { + 'course_id': str(self.course.id), + 'progress_url': f'/courses/{self.course.id}/progress/{self.students[0].id}/' + } + + for key, value in expected_data.items(): + self.assertIn(key, res_json) + self.assertEqual(res_json[key], value) + + def test_get_student_progress_url_response_headers(self): + """ + Test that the progress_url endpoint returns the correct headers. + """ + url = reverse('get_student_progress_url', kwargs={'course_id': str(self.course.id)}) + data = {'unique_student_identifier': self.students[0].email} + response = self.client.post(url, data) + assert response.status_code == 200 + + expected_headers = { + 'Allow': 'POST, OPTIONS', # drf view brings this key. + 'Cache-Control': 'no-cache, no-store, must-revalidate', + 'Content-Language': 'en', + 'Content-Length': str(len(response.content.decode('utf-8'))), + 'Content-Type': 'application/json', + 'Vary': 'Cookie, Accept-Language, origin', + 'X-Frame-Options': 'DENY' + } + + for key, value in expected_headers.items(): + self.assertIn(key, response.headers) + self.assertEqual(response.headers[key], value) def test_get_student_progress_url_from_uname(self): """ Test that progress_url is in the successful response. """ @@ -2955,6 +2985,14 @@ def test_get_student_progress_url_from_uname(self): assert response.status_code == 200 res_json = json.loads(response.content.decode('utf-8')) assert 'progress_url' in res_json + expected_data = { + 'course_id': str(self.course.id), + 'progress_url': f'/courses/{self.course.id}/progress/{self.students[0].id}/' + } + + for key, value in expected_data.items(): + self.assertIn(key, res_json) + self.assertEqual(res_json[key], value) def test_get_student_progress_url_noparams(self): """ Test that the endpoint 404's without the required query params. """ @@ -2968,6 +3006,17 @@ def test_get_student_progress_url_nostudent(self): response = self.client.post(url) assert response.status_code == 400 + def test_get_student_progress_url_without_permissions(self): + """ Test that progress_url returns 403 without credentials. """ + + # removed both roles from courses for instructor + CourseDataResearcherRole(self.course.id).remove_users(self.instructor) + CourseInstructorRole(self.course.id).remove_users(self.instructor) + url = reverse('get_student_progress_url', kwargs={'course_id': str(self.course.id)}) + data = {'unique_student_identifier': self.students[0].email} + response = self.client.post(url, data) + assert response.status_code == 403 + class TestInstructorAPIRegradeTask(SharedModuleStoreTestCase, LoginEnrollmentTestCase): """ diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 02a91e7d84de..e7a82496456e 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -122,6 +122,7 @@ from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from openedx.core.lib.courses import get_course_by_id +from openedx.core.lib.api.serializers import CourseKeyField from openedx.features.course_experience.url_helpers import get_learning_mfe_home_url from .tools import ( dump_block_extensions, @@ -1718,15 +1719,35 @@ def get_student_enrollment_status(request, course_id): return JsonResponse(response_payload) -@require_POST -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_course_permission(permissions.ENROLLMENT_REPORT) -@require_post_params( - unique_student_identifier="email or username of student for whom to get progress url" -) -@common_exceptions_400 -def get_student_progress_url(request, course_id): +class StudentProgressUrlSerializer(serializers.Serializer): + """Serializer for course renders""" + unique_student_identifier = serializers.CharField(write_only=True) + course_id = CourseKeyField(required=False) + progress_url = serializers.SerializerMethodField() + + def get_progress_url(self, obj): # pylint: disable=unused-argument + """ + Return the progress URL for the student. + Args: + obj (dict): The dictionary containing data for the serializer. + Returns: + str: The URL for the progress of the student in the course. + """ + user = get_student_from_identifier(obj.get('unique_student_identifier')) + course_id = obj.get('course_id') # Adjust based on your data structure + + if course_home_mfe_progress_tab_is_active(course_id): + progress_url = get_learning_mfe_home_url(course_id, url_fragment='progress') + if user is not None: + progress_url += '/{}/'.format(user.id) + else: + progress_url = reverse('student_progress', kwargs={'course_id': str(course_id), 'student_id': user.id}) + + return progress_url + + +@method_decorator(cache_control(no_cache=True, no_store=True, must_revalidate=True), name='dispatch') +class StudentProgressUrl(APIView): """ Get the progress url of a student. Limited to staff access. @@ -1736,21 +1757,25 @@ def get_student_progress_url(request, course_id): 'progress_url': '/../...' } """ - course_id = CourseKey.from_string(course_id) - user = get_student_from_identifier(request.POST.get('unique_student_identifier')) - - if course_home_mfe_progress_tab_is_active(course_id): - progress_url = get_learning_mfe_home_url(course_id, url_fragment='progress') - if user is not None: - progress_url += '/{}/'.format(user.id) - else: - progress_url = reverse('student_progress', kwargs={'course_id': str(course_id), 'student_id': user.id}) + authentication_classes = ( + JwtAuthentication, + BearerAuthenticationAllowInactiveUser, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated, permissions.InstructorPermission) + serializer_class = StudentProgressUrlSerializer + permission_name = permissions.ENROLLMENT_REPORT - response_payload = { - 'course_id': str(course_id), - 'progress_url': progress_url, - } - return JsonResponse(response_payload) + @method_decorator(ensure_csrf_cookie) + def post(self, request, course_id): + """Post method for validating incoming data and generating progress URL""" + data = { + 'course_id': course_id, + 'unique_student_identifier': request.data.get('unique_student_identifier') + } + serializer = self.serializer_class(data=data) + serializer.is_valid(raise_exception=True) + return Response(serializer.data) @transaction.non_atomic_requests diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index 0b4a88d1b7c6..6b072ef0c12e 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -1,3 +1,4 @@ + """ Instructor API endpoint urls. """ @@ -32,7 +33,7 @@ path('get_students_who_may_enroll', api.get_students_who_may_enroll, name='get_students_who_may_enroll'), path('get_anon_ids', api.get_anon_ids, name='get_anon_ids'), path('get_student_enrollment_status', api.get_student_enrollment_status, name="get_student_enrollment_status"), - path('get_student_progress_url', api.get_student_progress_url, name='get_student_progress_url'), + path('get_student_progress_url', api.StudentProgressUrl.as_view(), name='get_student_progress_url'), path('reset_student_attempts', api.reset_student_attempts, name='reset_student_attempts'), path('rescore_problem', api.rescore_problem, name='rescore_problem'), path('override_problem_score', api.override_problem_score, name='override_problem_score'),