fix: clean up course reset post endpoint (#34419)

lms/djangoapps/support/tests/test_views.py

@@ -54,7 +54,7 @@
 from common.djangoapps.third_party_auth.tests.factories import SAMLProviderConfigFactory
 from common.test.utils import disable_signal
 from lms.djangoapps.program_enrollments.tests.factories import ProgramCourseEnrollmentFactory, ProgramEnrollmentFactory
-from lms.djangoapps.support.models import CourseResetAudit, CourseResetCourseOptIn
+from lms.djangoapps.support.models import CourseResetAudit
 from lms.djangoapps.support.serializers import ProgramEnrollmentSerializer
 from lms.djangoapps.support.tests.factories import CourseResetCourseOptInFactory, CourseResetAuditFactory
 from lms.djangoapps.verify_student.models import VerificationDeadline
@@ -2071,10 +2071,10 @@ def test_verified_in_another_course(self):
         self.assertEqual(response_data['current_status']['course_id'], self.course_id)
 
 
-@ddt.ddt
-class TestResetCourseViewGET(SupportViewTestCase):
-    """ Tests for the list endpoint for course reset """
-
+class ResetCourseViewTestBase(SupportViewTestCase):
+    """
+    Shared base class for course reset view tests
+    """
     def _url(self, username):
         """ Helper to generate URL """
         return reverse("support:course_reset", kwargs={'username_or_email': username})
@@ -2086,7 +2086,6 @@ def setUp(self):
         super().setUp()
         SupportStaffRole().add_users(self.user)
         self.now = datetime.now().replace(tzinfo=UTC)
-
         self.course = CourseFactory.create(
             start=self.now - timedelta(days=90),
             end=self.now + timedelta(days=90),
@@ -2097,6 +2096,11 @@ def setUp(self):
         self.enrollment = CourseEnrollmentFactory.create(user=self.learner, course_id=self.course.id)
         self.opt_in = CourseResetCourseOptInFactory.create(course_id=self.course.id)
 
+
+@ddt.ddt
+class TestResetCourseListView(ResetCourseViewTestBase):
+    """ Tests for the list endpoint for course reset """
+
     def assert_course_ids(self, expected_course_ids, learner=None):
         """ Helper that asserts the course ids that will be returned from the listing endpoint """
         learner = learner or self.learner
@@ -2117,15 +2121,14 @@ def test_not_opted_in(self):
         it will not be returned by the endpoint
         """
         non_opted_in_course = CourseFactory.create()
-        enrollment = CourseEnrollmentFactory.create(user=self.learner, course_id=non_opted_in_course.id)
+        CourseEnrollmentFactory.create(user=self.learner, course_id=non_opted_in_course.id)
         self.assert_course_ids([self.course_id])
 
     def test_deactivated_opt_in(self):
         """
         If a learner is enrolled in a course that has opted in, but that opt-in is
         deactivated, it will not be returned from the endpoint
         """
-        response = self.client.get(self._url(self.learner))
         self.assert_course_ids([self.course_id])
 
         self.opt_in.active = False
@@ -2138,18 +2141,16 @@ def test_deactivated_enrollment(self):
         If a learner's enrollment in an opted in course is deactivated,
         the course will not be returned by the endpoint
         """
-        response = self.client.get(self._url(self.learner))
         self.assert_course_ids([self.course_id])
 
         self.enrollment.is_active = False
         self.enrollment.save()
 
         self.assert_course_ids([])
 
-    def assertResponse(self, expected_response, learner=None):
+    def assertResponse(self, expected_response):
         """ Helper to assert the contents of the response from the listing endpoint """
-        learner = learner or self.learner
-        response = self.client.get(self._url(learner))
+        response = self.client.get(self._url(self.learner))
         self.assertEqual(response.status_code, 200)
 
         actual_response = response.json()
@@ -2350,60 +2351,74 @@ def test_multiple_failed_audits(self):
         }])
 
 
-class TestResetCourseViewPost(SupportViewTestCase):
+class TestResetCourseCreateView(ResetCourseViewTestBase):
     """
-    Tests for creating course request
+    Tests for POST endpoint for performing course reset
     """
 
-    def setUp(self):
-        super().setUp()
-        SupportStaffRole().add_users(self.user)
+    def request(self, username=None, course_id=None, comment=None):
+        """ Helper to perform request """
+        username = username or self.learner.username
+        return self.client.post(
+            self._url(username),
+            data={
+                "course_id": course_id if course_id else self.course_id,
+                "comment": comment if comment else ""
+            }
+        )
 
-        self.course_id = 'course-v1:a+b+c'
+    def assert_error_response(self, response, expected_status_code, expected_error_message):
+        """ Helper to assert status code and error message """
+        self.assertEqual(response.status_code, expected_status_code)
+        self.assertEqual(response.data['error'], expected_error_message)
 
-        self.other_user = User.objects.create(username='otheruser', password='test')
+    def test_wrong_username(self):
+        """ A request with a username which does not exits returns 404 """
+        response = self.request(username='does_not_exist')
+        self.assert_error_response(response, 404, "User does not exist")
 
-        self.course = CourseFactory.create(
-            org='a',
-            course='b',
-            run='c',
-            enable_proctored_exams=True,
-            proctoring_provider=settings.PROCTORING_BACKENDS['DEFAULT'],
-        )
-        self.enrollment = CourseEnrollmentFactory(
-            is_active=True,
-            mode='verified',
-            course_id=self.course.id,
-            user=self.user
-        )
-        self.opt_in = CourseResetCourseOptInFactory.create(course_id=self.course.id)
+    def test_invalid_course_id(self):
+        """ A request for an invalid course id returns 400 """
+        response = self.request(course_id='thisisnotacourseid')
+        self.assert_error_response(response, 400, "invalid course id")
 
-        self.other_course = CourseFactory.create(
-            org='x',
-            course='y',
-            run='z',
-        )
+    def test_missing_course_id(self):
+        """ A request without a course id returns 400 """
+        response = self.client.post(self._url(self.learner.username))
+        self.assert_error_response(response, 400, "Must specify course id")
 
-    def _url(self, username):
-        return reverse("support:course_reset", kwargs={'username_or_email': username})
+    def test_course_not_opt_in(self):
+        """ A request for a course which isn't opted into the feature returns 404 """
+        self.opt_in.active = False
+        self.opt_in.save()
 
-    def test_wrong_username(self):
-        """
-        Test that a request with a username which does not exits returns 404
-        """
-        response = self.client.post(self._url(username='does_not_exist'), data={'course_id': 'course-v1:aa+bb+c'})
-        self.assertEqual(response.status_code, 404)
+        response = self.request()
+        self.assert_error_response(response, 404, "Course is not eligible")
+
+    def test_unenrolled(self):
+        """ A request for a learner who isn't enrolled in the given course returns a 404 """
+        self.enrollment.is_active = False
+        self.enrollment.save()
+
+        response = self.request()
+        self.assert_error_response(response, 404, "Learner is not enrolled in course")
+
+    @patch('lms.djangoapps.support.views.course_reset.can_enrollment_be_reset')
+    def test_cannot_reset(self, mock_can_reset):
+        """ A request for a course which isn't able to be reset returns a 404 """
+        mock_status = str(uuid4())
+        mock_can_reset.return_value = (False, mock_status)
+
+        response = self.request()
+        self.assert_error_response(response, 400, f"Cannot reset course: {mock_status}")
 
     @patch('lms.djangoapps.support.views.course_reset.reset_student_course')
     def test_learner_course_reset(self, mock_reset_student_course):
+        """ Happy path test """
         comment = str(uuid4())
-        response = self.client.post(
-            self._url(username=self.user.username),
-            data={
-                'course_id': self.course_id,
-                'comment': comment,
-            }
-        )
+
+        # A request for a given learner and course with a comment should return a 201
+        response = self.request(comment=comment)
         self.assertEqual(response.status_code, 201)
         self.assertEqual(response.data, {
             'course_id': self.course_id,
@@ -2412,49 +2427,40 @@ def test_learner_course_reset(self, mock_reset_student_course):
             'comment': comment,
             'display_name': self.course.display_name
         })
+        # The reset task should be queued
+        mock_reset_student_course.delay.assert_called_once_with(self.course_id, self.learner.id, self.user.id)
+        # And an audit should be created as ENQUEUED
         self.assertEqual(
-            mock_reset_student_course.delay.call_count, 1
+            self.enrollment.courseresetaudit_set.first().status,
+            CourseResetAudit.CourseResetStatus.ENQUEUED
         )
 
-    def test_course_not_opt_in(self):
-        response = self.client.post(self._url(username=self.user.username), data={'course_id': 'course-v1:aa+bb+c'})
-        self.assertEqual(response.status_code, 404)
-
     @patch('lms.djangoapps.support.views.course_reset.reset_student_course')
     def test_course_reset_failed(self, mock_reset_student_course):
-        course = CourseFactory.create(
-            org='xx',
-            course='yy',
-            run='zz',
-        )
-        enrollment = CourseEnrollmentFactory(
-            is_active=True,
-            mode='verified',
-            course_id=course.id,
-            user=self.user
-        )
-
-        opt_in_course = CourseResetCourseOptIn.objects.create(
-            course_id=course.id,
-            active=True
-        )
-
+        """ An audit that has failed previously should be able to be run successfully """
         CourseResetAudit.objects.create(
-            course=opt_in_course,
-            course_enrollment=enrollment,
-            reset_by=self.other_user,
+            course=self.opt_in,
+            course_enrollment=self.enrollment,
+            reset_by=self.user,
             status=CourseResetAudit.CourseResetStatus.FAILED
         )
-        response = self.client.post(self._url(username=self.user.username), data={'course_id': course.id})
-        self.assertEqual(
-            mock_reset_student_course.delay.call_count, 1
-        )
-        self.assertEqual(response.status_code, 200)
+        response = self.request()
+        self.assertEqual(response.status_code, 201)
+        self.assertEqual(response.data, {
+            'course_id': self.course_id,
+            'status': response.data['status'],
+            'can_reset': False,
+            'comment': '',
+            'display_name': self.course.display_name
+        })
+        mock_reset_student_course.delay.assert_called_once_with(self.course_id, self.learner.id, self.user.id)
 
-    def test_course_reset_dupe(self):
-        CourseResetAuditFactory.create(
+    def test_course_reset_already_reset(self):
+        """ A course that has an audit that hasn't failed should not be allowed to be run again """
+        additional_audit = CourseResetAuditFactory.create(
             course=self.opt_in,
             course_enrollment=self.enrollment,
+            status=CourseResetAudit.CourseResetStatus.ENQUEUED
         )
-        response2 = self.client.post(self._url(username=self.user.username), data={'course_id': self.course_id})
-        self.assertEqual(response2.status_code, 204)
+        response = self.request()
+        self.assert_error_response(response, 400, f"Cannot reset course: {additional_audit.status_message()}")

lms/djangoapps/support/views/course_reset.py

@@ -3,10 +3,11 @@
 from rest_framework.response import Response
 from django.contrib.auth import get_user_model
 from django.utils.decorators import method_decorator
+from opaque_keys import InvalidKeyError
+from opaque_keys.edx.keys import CourseKey
 from rest_framework.views import APIView
 from rest_framework.permissions import IsAuthenticated
 
-
 from common.djangoapps.student.models import CourseEnrollment, get_user_by_username_or_email
 from common.djangoapps.student.helpers import user_has_passing_grade_in_course
 from lms.djangoapps.support.decorators import require_support_permission
@@ -118,67 +119,46 @@ def post(self, request, username_or_email):
             'course_id': <course id>
             'display_name': <course display name>
             'status': <status of the enrollment wrt/reset, to be displayed to user>
+            'comment': <optional comment made by support staff performing reset>
             'can_reset': (boolean) <can the course be reset for this learner>
         }
         """
-        comment = request.data.get('comment', '')
-        course_id = request.data['course_id']
         try:
+            course_id = request.data['course_id']
+            course_key = CourseKey.from_string(course_id)
             user = get_user_by_username_or_email(username_or_email)
+            opt_in_course = CourseResetCourseOptIn.objects.get(course_id=course_key, active=True)
+        except KeyError:
+            return Response({'error': 'Must specify course id'}, status=400)
+        except InvalidKeyError:
+            return Response({'error': 'invalid course id'}, status=400)
         except User.DoesNotExist:
             return Response({'error': 'User does not exist'}, status=404)
-        try:
-            opt_in_course = CourseResetCourseOptIn.objects.get(course_id=course_id)
         except CourseResetCourseOptIn.DoesNotExist:
             return Response({'error': 'Course is not eligible'}, status=404)
-        enrollment = CourseEnrollment.objects.get(
-            course=course_id,
-            user=user,
-            is_active=True
+
+        if not CourseEnrollment.is_enrolled(user, course_key):
+            return Response({'error': 'Learner is not enrolled in course'}, status=404)
+        course_enrollment = CourseEnrollment.get_enrollment(user, course_key)
+
+        can_reset, status_message = can_enrollment_be_reset(course_enrollment)
+        if not can_reset:
+            return Response({'error': f'Cannot reset course: {status_message}'}, status=400)
+
+        course_reset_audit = CourseResetAudit.objects.create(
+            course=opt_in_course,
+            course_enrollment=course_enrollment,
+            reset_by=request.user,
+            comment=request.data.get('comment', ''),
         )
-        user_passed = user_has_passing_grade_in_course(enrollment=enrollment)
-        course_overview = enrollment.course_overview
-        course_reset_audit = CourseResetAudit.objects.filter(course_enrollment=enrollment).first()
-
-        if course_reset_audit and (
-            course_reset_audit.status == CourseResetAudit.CourseResetStatus.FAILED
-            and not user_passed
-        ):
-            course_reset_audit.status = CourseResetAudit.CourseResetStatus.ENQUEUED
-            course_reset_audit.comment = comment
-            course_reset_audit.save()
-            reset_student_course.delay(course_id, user.email, request.user.email)
-
-            resp = {
-                'course_id': course_id,
-                'status': course_reset_audit.status_message(),
-                'can_reset': False,
-                'display_name': course_overview.display_name
-            }
-            return Response(resp, status=200)
-
-        elif course_reset_audit and course_reset_audit.status in (
-            CourseResetAudit.CourseResetStatus.IN_PROGRESS,
-            CourseResetAudit.CourseResetStatus.ENQUEUED
-        ):
-            return Response(None, status=204)
-
-        if enrollment and opt_in_course and not user_passed:
-            course_reset_audit = CourseResetAudit.objects.create(
-                course=opt_in_course,
-                course_enrollment=enrollment,
-                reset_by=request.user,
-                comment=comment,
-            )
-            resp = {
-                'course_id': course_id,
-                'status': course_reset_audit.status_message(),
-                'can_reset': False,
-                'comment': comment,
-                'display_name': course_overview.display_name
-            }
 
-            reset_student_course.delay(course_id, user.email, request.user.email)
-            return Response(resp, status=201)
-        else:
-            return Response(None, status=400)
+        resp = {
+            'course_id': course_id,
+            'status': course_reset_audit.status_message(),
+            'can_reset': False,
+            'comment': course_reset_audit.comment,
+            'display_name': course_enrollment.course_overview.display_name
+        }
+
+        reset_student_course.delay(course_id, user.id, request.user.id)
+        return Response(resp, status=201)