From 60fdb3c2ff69a04791bbdd7bc132da841ec204b9 Mon Sep 17 00:00:00 2001 From: Zacharis278 Date: Wed, 31 Jul 2024 10:26:27 -0400 Subject: [PATCH 1/2] feat: delete allowance api --- edx_exams/apps/api/v1/tests/test_views.py | 26 +++++++++++++++++++++-- edx_exams/apps/api/v1/urls.py | 5 +++++ edx_exams/apps/api/v1/views.py | 16 ++++++++++++++ edx_exams/apps/core/constants.py | 2 ++ 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/edx_exams/apps/api/v1/tests/test_views.py b/edx_exams/apps/api/v1/tests/test_views.py index e683fa28..e3f0b2ce 100644 --- a/edx_exams/apps/api/v1/tests/test_views.py +++ b/edx_exams/apps/api/v1/tests/test_views.py @@ -1789,16 +1789,18 @@ def setUp(self): course_id=self.course_id, ) - def request_api(self, method, user, course_id, data=None): + def request_api(self, method, user, course_id, data=None, allowance_id=None): """ Helper function to make API request """ - assert method in ['get', 'post'] + assert method in ['get', 'post', 'delete'] headers = self.build_jwt_headers(user) url = reverse( 'api:v1:course-allowances', kwargs={'course_id': course_id} ) + if allowance_id: + url = f'{url}/{allowance_id}' if data: return getattr(self.client, method)(url, json.dumps(data), **headers, content_type='application/json') @@ -1878,6 +1880,26 @@ def test_get_empty_response(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.data, []) + def test_delete(self): + """ + Test that the endpoint deletes an allowance + """ + allowance = StudentAllowanceFactory.create( + exam=self.exam, + user=self.user, + ) + + response = self.request_api('delete', self.user, self.exam.course_id, allowance_id=allowance.id) + self.assertEqual(response.status_code, 204) + self.assertFalse(StudentAllowance.objects.filter(id=allowance.id).exists()) + + def test_delete_not_found(self): + """ + Test that 404 is returned if allowance does not exist + """ + response = self.request_api('delete', self.user, self.exam.course_id, allowance_id=19) + self.assertEqual(response.status_code, 404) + def test_post_allowances(self): """ Test that the endpoint creates allowances for the given request data diff --git a/edx_exams/apps/api/v1/urls.py b/edx_exams/apps/api/v1/urls.py index e5720f18..73db0061 100644 --- a/edx_exams/apps/api/v1/urls.py +++ b/edx_exams/apps/api/v1/urls.py @@ -19,6 +19,11 @@ app_name = 'v1' urlpatterns = [ + re_path( + fr'exams/course_id/{COURSE_ID_PATTERN}/allowances/(?P\d+)', + AllowanceView.as_view(), + name='course-allowance' + ), re_path( fr'exams/course_id/{COURSE_ID_PATTERN}/allowances', AllowanceView.as_view(), diff --git a/edx_exams/apps/api/v1/views.py b/edx_exams/apps/api/v1/views.py index 8f40e044..00645bb5 100644 --- a/edx_exams/apps/api/v1/views.py +++ b/edx_exams/apps/api/v1/views.py @@ -828,6 +828,22 @@ def get(self, request, course_id): allowances = StudentAllowance.get_allowances_for_course(course_id) return Response(AllowanceSerializer(allowances, many=True).data) + def delete(self, request, course_id, allowance_id): # pylint: disable=unused-argument + """ + HTTP DELETE handler. Deletes all allowances for a course. + + TODO: both this and the POST endpoint should be limiting operations by course_id + """ + try: + StudentAllowance.objects.get(id=allowance_id).delete() + except StudentAllowance.DoesNotExist: + return Response( + status=status.HTTP_404_NOT_FOUND, + data={'detail': f'Allowance with id={allowance_id} does not exist.'} + ) + + return Response(status=status.HTTP_204_NO_CONTENT) + def post(self, request, course_id): # pylint: disable=unused-argument """ HTTP POST handler. Creates allowances based on the given list. diff --git a/edx_exams/apps/core/constants.py b/edx_exams/apps/core/constants.py index 8159d000..894f12be 100644 --- a/edx_exams/apps/core/constants.py +++ b/edx_exams/apps/core/constants.py @@ -18,3 +18,5 @@ class Status: USAGE_KEY_PATTERN = r'(?P(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))' EXAM_ID_PATTERN = r'(?P\d+)' + +ATTEMPT_ID_PATTERN = r'(?P\d+)' From f9d02162320504232eba1ec49be8ec42ec3c92ca Mon Sep 17 00:00:00 2001 From: Zacharis278 Date: Wed, 31 Jul 2024 10:46:51 -0400 Subject: [PATCH 2/2] feat: fix auth issue --- edx_exams/apps/api/v1/tests/test_views.py | 27 +++++++++++++++++ edx_exams/apps/api/v1/views.py | 37 +++++++++++++---------- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/edx_exams/apps/api/v1/tests/test_views.py b/edx_exams/apps/api/v1/tests/test_views.py index e3f0b2ce..6be0f561 100644 --- a/edx_exams/apps/api/v1/tests/test_views.py +++ b/edx_exams/apps/api/v1/tests/test_views.py @@ -1900,6 +1900,18 @@ def test_delete_not_found(self): response = self.request_api('delete', self.user, self.exam.course_id, allowance_id=19) self.assertEqual(response.status_code, 404) + def test_delete_not_allowed(self): + """ + Test that 404 is returned if the allowance is not for the authorized course + """ + other_course_id = 'course-v1:edx+another+course' + allowance = StudentAllowanceFactory.create( + exam=ExamFactory.create(course_id=other_course_id), + user=self.user, + ) + response = self.request_api('delete', self.user, self.exam.course_id, allowance_id=allowance.id) + self.assertEqual(response.status_code, 404) + def test_post_allowances(self): """ Test that the endpoint creates allowances for the given request data @@ -1958,3 +1970,18 @@ def test_post_invalid_missing_user(self): ] response = self.request_api('post', self.user, self.exam.course_id, data=request_data) self.assertEqual(response.status_code, 400) + + def test_post_unauthorized_course(self): + """ + Test that an error is returned when atttempting to create an allowance for + an exam in a different course. + """ + other_course_id = 'course-v1:edx+another+course' + other_course_exam = ExamFactory.create(course_id=other_course_id) + + request_data = [ + {'exam_id': self.exam.id, 'username': self.user.username, 'extra_time_mins': 45}, + {'exam_id': other_course_exam.id, 'username': self.user.username, 'extra_time_mins': 45}, + ] + response = self.request_api('post', self.user, self.exam.course_id, data=request_data) + self.assertEqual(response.status_code, 400) diff --git a/edx_exams/apps/api/v1/views.py b/edx_exams/apps/api/v1/views.py index 00645bb5..20451c05 100644 --- a/edx_exams/apps/api/v1/views.py +++ b/edx_exams/apps/api/v1/views.py @@ -828,14 +828,12 @@ def get(self, request, course_id): allowances = StudentAllowance.get_allowances_for_course(course_id) return Response(AllowanceSerializer(allowances, many=True).data) - def delete(self, request, course_id, allowance_id): # pylint: disable=unused-argument + def delete(self, request, course_id, allowance_id): """ HTTP DELETE handler. Deletes all allowances for a course. - - TODO: both this and the POST endpoint should be limiting operations by course_id """ try: - StudentAllowance.objects.get(id=allowance_id).delete() + StudentAllowance.objects.get(id=allowance_id, exam__course_id=course_id).delete() except StudentAllowance.DoesNotExist: return Response( status=status.HTTP_404_NOT_FOUND, @@ -844,7 +842,7 @@ def delete(self, request, course_id, allowance_id): # pylint: disable=unused-ar return Response(status=status.HTTP_204_NO_CONTENT) - def post(self, request, course_id): # pylint: disable=unused-argument + def post(self, request, course_id): """ HTTP POST handler. Creates allowances based on the given list. """ @@ -856,18 +854,25 @@ def post(self, request, course_id): # pylint: disable=unused-argument # We expect the number of allowances in each request to be small. Should they increase, # we should not query within the loop, and instead refactor this to optimize # the DB calls. - allowance_objects = [ - StudentAllowance( - user=( - User.objects.get(username=allowance['username']) - if allowance.get('username') - else User.objects.get(email=allowance['email']) - ), - exam=Exam.objects.get(id=allowance['exam_id']), - extra_time_mins=allowance['extra_time_mins'] + try: + allowance_objects = [ + StudentAllowance( + user=( + User.objects.get(username=allowance['username']) + if allowance.get('username') + else User.objects.get(email=allowance['email']) + ), + exam=Exam.objects.get(id=allowance['exam_id'], course_id=course_id), + extra_time_mins=allowance['extra_time_mins'] + ) + for allowance in allowances + ] + except Exam.DoesNotExist: + return Response( + status=status.HTTP_400_BAD_REQUEST, + data={'detail': 'Exam does not exist'} ) - for allowance in allowances - ] + StudentAllowance.objects.bulk_create( allowance_objects, update_conflicts=True,