diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 13c72ff3391a..85f394fc359e 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -174,6 +174,11 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i source_course_key = CourseKey.from_string(source_course_key_string) destination_course_key = CourseKey.from_string(destination_course_key_string) + # Tracks whether the rerun has already been marked succeeded, i.e. the destination course is a + # complete, usable course. Once this is True, the catch-all handler below must never delete the + # course out from under the user again -- any failure past this point is in auxiliary, + # non-essential post-processing (see the individually-guarded steps further down). + rerun_succeeded = False try: # deserialize the payload fields = deserialize_fields(fields) if fields else None @@ -191,6 +196,7 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i # update state: Succeeded CourseRerunState.objects.succeeded(course_key=destination_course_key) + rerun_succeeded = True COURSE_RERUN_COMPLETED.send_event( time=datetime.now(timezone.utc), @@ -198,20 +204,47 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i course_key=destination_course_key ) ) + + # The steps below are auxiliary/best-effort: the rerun has already succeeded and the course + # is live and usable at this point, so a failure here must not flip the state back to failed + # or delete the course. Each is wrapped and logged independently so a failure in one doesn't + # prevent the others from running. + # call edxval to attach videos to the rerun - copy_course_videos(source_course_key, destination_course_key) + try: + copy_course_videos(source_course_key, destination_course_key) + except Exception: # pylint: disable=broad-except + LOGGER.exception( + 'Course Rerun: failed to copy videos from %s to %s. The rerun course itself is unaffected.', + source_course_key, destination_course_key, + ) # Copy RestrictedCourse - restricted_course = RestrictedCourse.objects.filter(course_key=source_course_key).first() + try: + restricted_course = RestrictedCourse.objects.filter(course_key=source_course_key).first() - if restricted_course: - country_access_rules = CountryAccessRule.objects.filter(restricted_course=restricted_course) - new_restricted_course = clone_instance(restricted_course, {'course_key': destination_course_key}) - for country_access_rule in country_access_rules: - clone_instance(country_access_rule, {'restricted_course': new_restricted_course}) + if restricted_course: + country_access_rules = CountryAccessRule.objects.filter(restricted_course=restricted_course) + new_restricted_course = clone_instance(restricted_course, {'course_key': destination_course_key}) + for country_access_rule in country_access_rules: + clone_instance(country_access_rule, {'restricted_course': new_restricted_course}) + except Exception: # pylint: disable=broad-except + LOGGER.exception( + 'Course Rerun: failed to clone RestrictedCourse/CountryAccessRule from %s to %s. ' + 'The rerun course itself is unaffected.', + source_course_key, destination_course_key, + ) + + try: + org_data = ensure_organization(destination_course_key.org) + add_organization_course(org_data, destination_course_key) + except Exception: # pylint: disable=broad-except + LOGGER.exception( + 'Course Rerun: failed to link destination course %s to organization %s. The course exists ' + 'but may not be linked to its organization -- needs investigation.', + destination_course_key, destination_course_key.org, + ) - org_data = ensure_organization(destination_course_key.org) - add_organization_course(org_data, destination_course_key) return "succeeded" except DuplicateCourseError: @@ -226,12 +259,21 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i CourseRerunState.objects.failed(course_key=destination_course_key) LOGGER.exception('Course Rerun Error') - try: - # cleanup any remnants of the course - modulestore().delete_course(destination_course_key, user_id) - except ItemNotFoundError: - # it's possible there was an error even before the course block was created - pass + if not rerun_succeeded: + try: + # cleanup any remnants of the course + modulestore().delete_course(destination_course_key, user_id) + except ItemNotFoundError: + # it's possible there was an error even before the course block was created + pass + else: + # The course was already fully cloned and marked succeeded before this exception was + # raised -- it's a valid, usable course. Do not delete it; just log for investigation. + LOGGER.exception( + 'Course Rerun: destination course %s already succeeded before this error was raised; ' + 'preserving the course instead of deleting it.', + destination_course_key, + ) return "exception: " + str(exc) diff --git a/cms/djangoapps/contentstore/tests/test_clone_course.py b/cms/djangoapps/contentstore/tests/test_clone_course.py index 6d45ae5469e1..df43b0155ec4 100644 --- a/cms/djangoapps/contentstore/tests/test_clone_course.py +++ b/cms/djangoapps/contentstore/tests/test_clone_course.py @@ -141,3 +141,91 @@ def test_rerun_course(self): course_key=split_course4_id, state=CourseRerunUIStateManager.State.FAILED ) + + def test_rerun_course_auxiliary_step_failure_does_not_delete_course(self): + """ + Regression test for EDLYPRODUCT-8393: if an auxiliary, post-succeeded step (e.g. linking the + destination course to its organization) raises, the rerun must still be reported as + "succeeded" and the already-cloned course must NOT be deleted. + """ + org = 'edX' + course_number = 'CS101' + course_run = '2015_Q1' + display_name = 'rerun' + + split_course = CourseFactory.create( + org=org, + number=course_number, + run=course_run, + display_name=display_name, + default_store=ModuleStoreEnum.Type.split + ) + + rerun_course_id = CourseLocator(org=org, course=course_number, run="rerun_aux_failure") + fields = {'display_name': 'rerun'} + CourseRerunState.objects.initiated(split_course.id, rerun_course_id, self.user, fields['display_name']) + + with patch( + 'cms.djangoapps.contentstore.tasks.add_organization_course', + Mock(side_effect=Exception('org link boom!')), + ): + result = rerun_course.delay( + str(split_course.id), str(rerun_course_id), self.user.id, + json.dumps(fields, cls=EdxJSONEncoder) + ) + + # The failure in the auxiliary org-linking step must not surface as a task failure... + self.assertEqual(result.get(), "succeeded") + # ...the rerun state must remain SUCCEEDED... + rerun_state = CourseRerunState.objects.find_first(course_key=rerun_course_id) + self.assertEqual(rerun_state.state, CourseRerunUIStateManager.State.SUCCEEDED) + # ...and the course itself must still exist and be usable. + self.assertIsNotNone(self.store.get_course(rerun_course_id), "Course was deleted after an auxiliary failure") + + def test_rerun_course_post_succeeded_failure_preserves_course(self): + """ + Regression test for EDLYPRODUCT-8393: even a failure that isn't in one of the individually + wrapped auxiliary steps -- as long as it happens after the rerun has already been marked + succeeded -- must not delete the already-cloned, already-usable course. The rerun is still + reported/marked as failed (something genuinely went wrong and should be surfaced), but the + course itself is preserved rather than destroyed. + """ + org = 'edX' + course_number = 'CS101' + course_run = '2015_Q1' + display_name = 'rerun' + + split_course = CourseFactory.create( + org=org, + number=course_number, + run=course_run, + display_name=display_name, + default_store=ModuleStoreEnum.Type.split + ) + + rerun_course_id = CourseLocator(org=org, course=course_number, run="rerun_post_succeeded_failure") + fields = {'display_name': 'rerun'} + CourseRerunState.objects.initiated(split_course.id, rerun_course_id, self.user, fields['display_name']) + + with patch( + 'cms.djangoapps.contentstore.tasks.COURSE_RERUN_COMPLETED.send_event', + Mock(side_effect=Exception('event bus boom!')), + ): + result = rerun_course.delay( + str(split_course.id), str(rerun_course_id), self.user.id, + json.dumps(fields, cls=EdxJSONEncoder) + ) + + # The task itself surfaces the failure... + self.assertIn("exception: ", result.get()) + # ...and the rerun state is (correctly) marked failed, since something genuinely broke... + CourseRerunState.objects.find_first( + course_key=rerun_course_id, + state=CourseRerunUIStateManager.State.FAILED + ) + # ...but the course was already fully cloned and marked succeeded before the failure, so it + # must NOT be deleted. + self.assertIsNotNone( + self.store.get_course(rerun_course_id), + "Course was deleted even though it had already succeeded before the failure" + ) diff --git a/cms/djangoapps/contentstore/tests/test_tasks.py b/cms/djangoapps/contentstore/tests/test_tasks.py index 5a76b7d67fe7..1ae3ab81fc5f 100644 --- a/cms/djangoapps/contentstore/tests/test_tasks.py +++ b/cms/djangoapps/contentstore/tests/test_tasks.py @@ -22,6 +22,7 @@ from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from common.djangoapps.course_action_state.managers import CourseRerunUIStateManager from common.djangoapps.course_action_state.models import CourseRerunState from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.course_apps.toggles import EXAMS_IDA @@ -209,6 +210,34 @@ def test_success_different_org(self): self.assertEqual(OrganizationCourse.objects.count(), 2) OrganizationCourse.objects.get(course_id=new_course_id, organization__short_name='neworg') + def test_auxiliary_step_failure_does_not_delete_course(self): + """ + Regression test for EDLYPRODUCT-8393. + + If an auxiliary/best-effort post-succeeded step (video copy, RestrictedCourse clone, or + organization linking) raises, the task must still report "succeeded", the CourseRerunState + must remain SUCCEEDED, and the already-cloned course must remain retrievable from the + modulestore -- it must NOT be deleted. + """ + old_course_key = self.course.id + new_course_key = CourseLocator(org=old_course_key.org, course=old_course_key.course, run='rerun') + + CourseRerunState.objects.initiated(old_course_key, new_course_key, self.user, 'Test Re-run') + + with mock.patch( + 'edxval.api.copy_course_videos', + side_effect=Exception('edxval boom!'), + ): + result = rerun_course(str(old_course_key), str(new_course_key), self.user.id) + + self.assertEqual(result, "succeeded") + rerun_state = CourseRerunState.objects.find_first(course_key=new_course_key) + self.assertEqual(rerun_state.state, CourseRerunUIStateManager.State.SUCCEEDED) + self.assertIsNotNone( + modulestore().get_course(new_course_key), + "Course was deleted after an auxiliary (video copy) failure", + ) + @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class RegisterExamsTaskTestCase(CourseTestCase): # pylint: disable=missing-class-docstring