From 62c8805e3edeece520fb2ccec579ba32976e21db Mon Sep 17 00:00:00 2001 From: Awais Jibran Date: Mon, 19 Apr 2021 23:49:42 +0500 Subject: [PATCH] Refactor + Tests: Course Import Feature (#27369) * Code Refactoring This PR bumps code coverage by adding unit tests & clean up some code for improving code quality and maintainability. --- cms/djangoapps/contentstore/exceptions.py | 45 ++++ cms/djangoapps/contentstore/tasks.py | 41 ++- cms/djangoapps/contentstore/views/assets.py | 2 +- .../contentstore/views/certificates.py | 2 +- .../contentstore/views/exception.py | 17 -- .../views/tests/test_import_export.py | 247 ++++++++++++++---- .../xmodule/modulestore/xml_importer.py | 44 +--- 7 files changed, 284 insertions(+), 114 deletions(-) create mode 100644 cms/djangoapps/contentstore/exceptions.py delete mode 100644 cms/djangoapps/contentstore/views/exception.py diff --git a/cms/djangoapps/contentstore/exceptions.py b/cms/djangoapps/contentstore/exceptions.py new file mode 100644 index 0000000000..380a2a6373 --- /dev/null +++ b/cms/djangoapps/contentstore/exceptions.py @@ -0,0 +1,45 @@ +""" +A common module for managing exceptions. Helps to avoid circular references +""" +from .errors import ERROR_WHILE_READING, FAILED_TO_IMPORT_MODULE + + +class CourseImportException(Exception): + """Base exception class for course import workflows.""" + + def __init__(self): + super().__init__(self.description) # pylint: disable=no-member + + +class ErrorReadingFileException(CourseImportException): + """ + Raised when error occurs while trying to read a file. + """ + + def __init__(self, filename, **kwargs): + self.description = ERROR_WHILE_READING.format(filename) + super().__init__(**kwargs) + + +class ModuleFailedToImport(CourseImportException): + """ + Raised when a module is failed to import. + """ + + def __init__(self, display_name, location, **kwargs): + self.description = FAILED_TO_IMPORT_MODULE.format(display_name, location) + super().__init__(**kwargs) + + +class AssetNotFoundException(Exception): + """ + Raised when asset not found + """ + pass # lint-amnesty, pylint: disable=unnecessary-pass + + +class AssetSizeTooLargeException(Exception): + """ + Raised when the size of an uploaded asset exceeds the maximum size limit. + """ + pass # lint-amnesty, pylint: disable=unnecessary-pass diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 34c507fe48..d3e28034ba 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -39,6 +39,7 @@ from pytz import UTC from user_tasks.models import UserTaskArtifact, UserTaskStatus from user_tasks.tasks import UserTask +import cms.djangoapps.contentstore.errors as UserErrors from cms.djangoapps.contentstore.courseware_index import ( CoursewareSearchIndexer, LibrarySearchIndexer, @@ -61,10 +62,11 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import DuplicateCourseError, InvalidProctoringProvider, ItemNotFoundError from xmodule.modulestore.xml_exporter import export_course_to_xml, export_library_to_xml from xmodule.modulestore.xml_importer import import_course_from_xml, import_library_from_xml -import cms.djangoapps.contentstore.errors as UserErrors + +from .exceptions import CourseImportException from .outlines import update_outline_from_modulestore -from .utils import course_import_olx_validation_is_enabled from .toggles import bypass_olx_failure_enabled +from .utils import course_import_olx_validation_is_enabled User = get_user_model() @@ -606,7 +608,6 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan static_content_store=contentstore(), target_id=courselike_key, verbose=True, - status=self.status ) new_location = courselike_items[0].location @@ -614,15 +615,10 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan LOGGER.info(f'{log_prefix}: Course import successful') set_custom_attribute('course_import_completed', True) + except (CourseImportException, InvalidProctoringProvider, DuplicateCourseError) as known_exe: + handle_course_import_exception(courselike_key, known_exe, self.status) except Exception as exception: # pylint: disable=broad-except - msg = str(exception) - status_msg = UserErrors.UNKNOWN_ERROR_IN_IMPORT - if isinstance(exception, InvalidProctoringProvider): - status_msg = msg - LOGGER.exception(f'{log_prefix}: Unknown error while importing course {str(exception)}') - if self.status.state != UserTaskStatus.FAILED: - self.status.fail(status_msg) - monitor_import_failure(courselike_key, current_step, exception=exception) + handle_course_import_exception(courselike_key, exception, self.status, known=False) finally: if course_dir.isdir(): shutil.rmtree(course_dir) @@ -726,3 +722,26 @@ def log_errors_to_artifact(errorstore, status): 'warnings': get_error_by_type(ErrorLevel.WARNING.name), }) UserTaskArtifact.objects.create(status=status, name='OLX_VALIDATION_ERROR', text=message) + + +def handle_course_import_exception(courselike_key, exception, status, known=True): + """ + Handle course import exception and fail task status. + + Arguments: + courselike_key: A locator identifies a course resource. + exception: Exception object + status: UserTaskStatus object. + known: boolean indicating if this is a known failure or unknown. + """ + exception_message = str(exception) + log_prefix = f"Course import {courselike_key}:" + LOGGER.exception(f"{log_prefix} Error while importing course: {exception_message}") + task_fail_message = UserErrors.UNKNOWN_ERROR_IN_IMPORT + monitor_import_failure(courselike_key, status.state, exception=exception) + + if known: + task_fail_message = exception_message + + if status.state != UserTaskStatus.FAILED: + status.fail(task_fail_message) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 62e784ce29..85c3c79ab9 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -30,8 +30,8 @@ from xmodule.exceptions import NotFoundError from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError +from ..exceptions import AssetNotFoundException, AssetSizeTooLargeException from ..utils import reverse_course_url -from .exception import AssetNotFoundException, AssetSizeTooLargeException __all__ = ['assets_handler'] diff --git a/cms/djangoapps/contentstore/views/certificates.py b/cms/djangoapps/contentstore/views/certificates.py index e27d9aad27..a12d4755de 100644 --- a/cms/djangoapps/contentstore/views/certificates.py +++ b/cms/djangoapps/contentstore/views/certificates.py @@ -46,9 +46,9 @@ from common.djangoapps.util.json_request import JsonResponse from xmodule.modulestore import EdxJSONEncoder from xmodule.modulestore.django import modulestore +from ..exceptions import AssetNotFoundException from ..utils import get_lms_link_for_certificate_web_view, get_proctored_exam_settings_url, reverse_course_url from .assets import delete_asset -from .exception import AssetNotFoundException CERTIFICATE_SCHEMA_VERSION = 1 CERTIFICATE_MINIMUM_ID = 100 diff --git a/cms/djangoapps/contentstore/views/exception.py b/cms/djangoapps/contentstore/views/exception.py deleted file mode 100644 index 98b26286b9..0000000000 --- a/cms/djangoapps/contentstore/views/exception.py +++ /dev/null @@ -1,17 +0,0 @@ -""" -A common module for managing exceptions. Helps to avoid circular references -""" - - -class AssetNotFoundException(Exception): - """ - Raised when asset not found - """ - pass # lint-amnesty, pylint: disable=unnecessary-pass - - -class AssetSizeTooLargeException(Exception): - """ - Raised when the size of an uploaded asset exceeds the maximum size limit. - """ - pass # lint-amnesty, pylint: disable=unnecessary-pass diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 2f237986c9..f14a2f5f29 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -1,7 +1,6 @@ """ Unit tests for course import and export """ - import copy import json import logging @@ -17,6 +16,8 @@ from uuid import uuid4 import ddt import lxml from django.conf import settings +from django.contrib.auth import get_user_model +from django.core.exceptions import SuspiciousOperation from django.core.files.storage import FileSystemStorage from django.test.utils import override_settings from milestones.tests.utils import MilestonesTestCaseMixin @@ -26,6 +27,9 @@ from storages.backends.s3boto import S3BotoStorage from storages.backends.s3boto3 import S3Boto3Storage from user_tasks.models import UserTaskStatus +from cms.djangoapps.contentstore import errors as import_error +from cms.djangoapps.contentstore.exceptions import ErrorReadingFileException, ModuleFailedToImport +from cms.djangoapps.contentstore.storage import course_import_export_storage from cms.djangoapps.contentstore.tests.test_libraries import LibraryTestCase from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url @@ -37,14 +41,17 @@ from openedx.core.lib.extract_tar import safetar_extractall from xmodule.contentstore.django import contentstore from xmodule.modulestore import LIBRARY_ROOT, ModuleStoreEnum from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import DuplicateCourseError, InvalidProctoringProvider from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory from xmodule.modulestore.tests.utils import SPLIT_MODULESTORE_SETUP, TEST_DATA_DIR, MongoContentstoreBuilder from xmodule.modulestore.xml_exporter import export_course_to_xml, export_library_to_xml -from xmodule.modulestore.xml_importer import import_course_from_xml, import_library_from_xml +from xmodule.modulestore.xml_importer import CourseImportManager, import_course_from_xml, import_library_from_xml +TASK_LOGGER = 'cms.djangoapps.contentstore.tasks.LOGGER' TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT +User = get_user_model() log = logging.getLogger(__name__) @@ -178,42 +185,68 @@ class ImportTestCase(CourseTestCase): btar.add(bad_dir) self.unsafe_common_dir = path(tempfile.mkdtemp(dir=self.content_dir)) + self.log_prefix = f"Course import {self.course.id}:" - def test_no_coursexml(self): + @classmethod + def setUpClass(cls): + """ + Creates data shared by all tests. + """ + super().setUpClass() + cls.UnpackingError = -1 + cls.VerifyingError = -2 + cls.UpdatingError = -3 + + def assertImportStatusResponse(self, response, status=None, expected_message=None): + """ + Fail if the import response does not match with the provided status and message. + """ + self.assertEqual(response["ImportStatus"], status) + if expected_message: + self.assertEqual(response['Message'], expected_message) + + def get_import_status(self, course_id, tarfile_path): + """Helper method to get course import status.""" + resp = self.client.get( + reverse_course_url( + 'import_status_handler', + course_id, + kwargs={'filename': os.path.split(tarfile_path)[1]} + ) + ) + return json.loads(resp.content) + + def import_tarfile_in_course(self, tarfile_path): + """Helper method to import provided tarfile in the course.""" + with open(tarfile_path, 'rb') as gtar: + args = {"name": tarfile_path, "course-data": [gtar]} + return self.client.post(self.url, args) + + @patch(TASK_LOGGER) + def test_no_coursexml(self, mocked_log): """ Check that the response for a tar.gz import without a course.xml is correct. """ - with open(self.bad_tar, 'rb') as btar: # lint-amnesty, pylint: disable=bad-option-value, open-builtin - resp = self.client.post( - self.url, - { - "name": self.bad_tar, - "course-data": [btar] - }) - self.assertEqual(resp.status_code, 200) + error_msg = import_error.FILE_MISSING.format('course.xml') + expected_error_mesg = f'{self.log_prefix} {error_msg}' + response = self.import_tarfile_in_course(self.bad_tar) + + self.assertEqual(response.status_code, 200) + mocked_log.error.assert_called_once_with(expected_error_mesg) + # Check that `import_status` returns the appropriate stage (i.e., the # stage at which import failed). - resp_status = self.client.get( - reverse_course_url( - 'import_status_handler', - self.course.id, - kwargs={'filename': os.path.split(self.bad_tar)[1]} - ) - ) - - self.assertEqual(json.loads(resp_status.content.decode('utf-8'))["ImportStatus"], -2) + resp_status = self.get_import_status(self.course.id, self.bad_tar) + self.assertImportStatusResponse(resp_status, self.VerifyingError, error_msg) def test_with_coursexml(self): """ Check that the response for a tar.gz import with a course.xml is correct. """ - with open(self.good_tar, 'rb') as gtar: # lint-amnesty, pylint: disable=bad-option-value, open-builtin - args = {"name": self.good_tar, "course-data": [gtar]} - resp = self.client.post(self.url, args) - - self.assertEqual(resp.status_code, 200) + response = self.import_tarfile_in_course(self.good_tar) + self.assertEqual(response.status_code, 200) def test_import_in_existing_course(self): """ @@ -228,10 +261,8 @@ class ImportTestCase(CourseTestCase): display_name_before_import = course.display_name # Check that global staff user can import course - with open(self.good_tar, 'rb') as gtar: # lint-amnesty, pylint: disable=bad-option-value, open-builtin - args = {"name": self.good_tar, "course-data": [gtar]} - resp = self.client.post(self.url, args) - self.assertEqual(resp.status_code, 200) + response = self.import_tarfile_in_course(self.good_tar) + self.assertEqual(response.status_code, 200) course = self.store.get_course(self.course.id) self.assertIsNotNone(course) @@ -246,9 +277,7 @@ class ImportTestCase(CourseTestCase): # Now course staff user can also successfully import course self.client.login(username=nonstaff_user.username, password='foo') - with open(self.good_tar, 'rb') as gtar: # lint-amnesty, pylint: disable=bad-option-value, open-builtin - args = {"name": self.good_tar, "course-data": [gtar]} - resp = self.client.post(self.url, args) + resp = self.import_tarfile_in_course(self.good_tar) self.assertEqual(resp.status_code, 200) # Now check that non_staff user has his same role @@ -340,19 +369,11 @@ class ImportTestCase(CourseTestCase): def try_tar(tarpath): """ Attempt to tar an unacceptable file """ - with open(tarpath, 'rb') as tar: # lint-amnesty, pylint: disable=bad-option-value, open-builtin - args = {"name": tarpath, "course-data": [tar]} - resp = self.client.post(self.url, args) + resp = self.import_tarfile_in_course(tarpath) self.assertEqual(resp.status_code, 200) - resp = self.client.get( - reverse_course_url( - 'import_status_handler', - self.course.id, - kwargs={'filename': os.path.split(tarpath)[1]} - ) - ) - status = json.loads(resp.content.decode('utf-8'))["ImportStatus"] - self.assertEqual(status, -1) + + resp = self.get_import_status(self.course.id, tarpath) + self.assertEqual(resp["ImportStatus"], -1) try_tar(self._fifo_tar()) try_tar(self._symlink_tar()) @@ -367,14 +388,8 @@ class ImportTestCase(CourseTestCase): # Check that `import_status` returns the appropriate stage (i.e., # either 3, indicating all previous steps are completed, or 0, # indicating no upload in progress) - resp_status = self.client.get( - reverse_course_url( - 'import_status_handler', - self.course.id, - kwargs={'filename': os.path.split(self.good_tar)[1]} - ) - ) - import_status = json.loads(resp_status.content.decode('utf-8'))["ImportStatus"] + resp_status = self.get_import_status(self.course.id, self.good_tar) + import_status = resp_status["ImportStatus"] self.assertIn(import_status, (0, 3)) def test_library_import(self): @@ -525,6 +540,134 @@ class ImportTestCase(CourseTestCase): finally: shutil.rmtree(extract_dir) + @patch(TASK_LOGGER) + def test_import_failed_with_no_user_permission(self, mocked_log): + """ + Tests course import failure when user have no permission + """ + expected_error_mesg = f'{self.log_prefix} User permission denied: {self.user.username}' + with patch('cms.djangoapps.contentstore.tasks.has_course_author_access', Mock(return_value=False)): + response = self.import_tarfile_in_course(self.good_tar) + self.assertEqual(response.status_code, 200) + mocked_log.error.assert_called_once_with(expected_error_mesg) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.UnpackingError, import_error.COURSE_PERMISSION_DENIED) + + @patch(TASK_LOGGER) + def test_import_failed_with_unknown_user(self, mocked_log): + """ + Tests that course import failure with an unknown user id. + """ + expected_error_mesg = f'{self.log_prefix} Unknown User: {self.user.id}' + + with patch('django.contrib.auth.models.User.objects.get', side_effect=User.DoesNotExist): + response = self.import_tarfile_in_course(self.good_tar) + self.assertEqual(response.status_code, 200) + mocked_log.error.assert_called_once_with(expected_error_mesg) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.UnpackingError, import_error.USER_PERMISSION_DENIED) + + @patch(TASK_LOGGER) + def test_import_failed_with_unsafe_tarfile(self, mocked_log): + """ + Tests course import failure with unsafe tar file. + """ + expected_error_mesg = f'{self.log_prefix} Unsafe tar file' + with patch('cms.djangoapps.contentstore.tasks.safetar_extractall', side_effect=SuspiciousOperation): + response = self.import_tarfile_in_course(self.good_tar) + + self.assertEqual(response.status_code, 200) + mocked_log.error.assert_called_once_with(expected_error_mesg) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.UnpackingError, import_error.UNSAFE_TAR_FILE) + + @patch(TASK_LOGGER) + def test_import_failed_with_unknown_unpacking_error(self, mocked_log): + """ + Tests that course import failure for unknown error while unpacking + """ + expected_error_mesg = f'{self.log_prefix} Unknown error while unpacking' + with patch.object(course_import_export_storage, 'open', side_effect=Exception): + response = self.import_tarfile_in_course(self.good_tar) + + self.assertEqual(response.status_code, 200) + mocked_log.exception.assert_called_once_with(expected_error_mesg, exc_info=True) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.UnpackingError, import_error.UNKNOWN_ERROR_IN_UNPACKING) + + @patch(TASK_LOGGER) + @patch('olxcleaner.validate') + @patch('cms.djangoapps.contentstore.tasks.report_error_summary') + @patch('cms.djangoapps.contentstore.tasks.report_errors') + def test_import_failed_with_olx_validations(self, mocked_report, mocked_summary, mocked_validate, mocked_log): + """ + Tests that course import failure for unknown error while unpacking + """ + errors = [Mock(description='DuplicateURLNameError', level_val=3)] + mocked_summary.return_value = [f'ERROR {error.description} found in content' for error in errors] + mocked_report.return_value = [f'Errors: {len(errors)}'] + mocked_validate.return_value = [ + Mock(), Mock(errors=errors, return_error=Mock(return_value=True)), Mock() + ] + expected_error_mesg = f'{self.log_prefix} CourseOlx validation failed.' + with patch.dict(settings.FEATURES, ENABLE_COURSE_OLX_VALIDATION=True): + response = self.import_tarfile_in_course(self.good_tar) + + self.assertEqual(response.status_code, 200) + mocked_log.error.assert_called_once_with(expected_error_mesg) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.VerifyingError, import_error.OLX_VALIDATION_FAILED) + + @patch(TASK_LOGGER) + @patch.object(CourseImportManager, 'import_children') + @ddt.data( + ( + InvalidProctoringProvider('foo', ['bar']), + "The selected proctoring provider, foo, is not a valid provider. Please select from one of ['bar'].", + ), + (DuplicateCourseError("foo", "foobar"), 'Cannot create course foo, which duplicates foobar'), + (ErrorReadingFileException("assets.xml"), "Error while reading assets.xml. Check file for XML errors."), + (ModuleFailedToImport("Unit 1", "foo/bar"), "Failed to import module: Unit 1 at location: foo/bar"), + ) + @ddt.unpack + def test_import_failure_is_descriptive_for_known_failures(self, exc, expected_mesg, mocked_import, mocked_log): + """ + Test that when course import fails with a known failure, user get a descriptive error message. + """ + mocked_import.side_effect = exc + expected_exception_messages = f"{self.log_prefix} Error while importing course: {str(exc)}" + response = self.import_tarfile_in_course(self.good_tar) + self.assertEqual(response.status_code, 200) + mocked_log.exception.assert_called_once_with(expected_exception_messages) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.UpdatingError, expected_mesg) + + @patch(TASK_LOGGER) + @patch.object(CourseImportManager, 'import_children') + @ddt.data( + Exception("foo exbar"), + KeyError("foo kebar"), + ValueError("foo vebar"), + ) + def test_import_failure_for_unknown_failures(self, exception, mocked_import, mocked_log): + """ + Test that import status and logged exception when course import fails with an unknown failure. + """ + mocked_import.side_effect = exception + expected_exc_mesg = f"{self.log_prefix} Error while importing course: {str(exception)}" + response = self.import_tarfile_in_course(self.good_tar) + self.assertEqual(response.status_code, 200) + mocked_log.exception.assert_called_once_with(expected_exc_mesg) + + status_response = self.get_import_status(self.course.id, self.good_tar) + self.assertImportStatusResponse(status_response, self.UpdatingError, import_error.UNKNOWN_ERROR_IN_IMPORT) + @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) @ddt.ddt diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index c1684e5f00..33a2ff2039 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -21,7 +21,6 @@ Modulestore virtual | XML physical (draft, published) (a, b) | (a, b) | (x, b) | (x, x) | (x, y) | (a, x) """ - import json import logging import mimetypes @@ -38,8 +37,7 @@ from xblock.core import XBlockMixin from xblock.fields import Reference, ReferenceList, ReferenceValueDict, Scope from xblock.runtime import DictKeyValueStore, KvsFieldData -import cms.djangoapps.contentstore.errors as UserErrors - +from cms.djangoapps.contentstore.exceptions import ErrorReadingFileException, ModuleFailedToImport from common.djangoapps.util.monitoring import monitor_import_failure from xmodule.assetstore import AssetMetadata from xmodule.contentstore.content import StaticContent @@ -69,6 +67,7 @@ class LocationMixin(XBlockMixin): with old-style :class:`XModule` API. This is a simplified version of :class:`XModuleMixin`. """ + @property def location(self): """ Get the UsageKey of this block. """ @@ -235,7 +234,6 @@ class ImportManager: create_if_not_present=False, raise_on_failure=False, static_content_subdir=DEFAULT_STATIC_CONTENT_SUBDIR, python_lib_filename='python_lib.zip', - status=None ): self.store = store self.user_id = user_id @@ -260,7 +258,6 @@ class ImportManager: xblock_select=store.xblock_select, target_course_id=target_id, ) - self.status = status self.logger, self.errors = make_error_tracker() def preflight(self): @@ -363,12 +360,10 @@ class ImportManager: logging.info(f'Course import {course_id}: No {assets_filename} file present.') return except Exception as exc: # pylint: disable=W0703 - monitor_import_failure(course_id, 'Updating', exception=exc) - logging.exception(f'Course import {course_id}: Error while parsing {assets_filename}.') if self.raise_on_failure: # lint-amnesty, pylint: disable=no-else-raise - if self.status: - self.status.fail(UserErrors.ERROR_WHILE_READING).format(assets_filename) - raise + monitor_import_failure(course_id, 'Updating', exception=exc) + logging.exception(f'Course import {course_id}: Error while parsing {assets_filename}.') + raise ErrorReadingFileException(assets_filename) # pylint: disable=raise-missing-from else: return @@ -485,13 +480,7 @@ class ImportManager: log.exception( f'Course import {dest_id}: failed to import module location {child.location}' ) - if self.status: - self.status.fail( - UserErrors.FAILED_TO_IMPORT_MODULE.format( - child.display_name, child.location - ) - ) - raise + raise ModuleFailedToImport(child.display_name, child.location) # pylint: disable=raise-missing-from depth_first(child) @@ -512,15 +501,11 @@ class ImportManager: runtime=courselike.runtime, ) except Exception: - msg = f'Course import {dest_id}: failed to import module location {leftover}' - log.error(msg) - if self.status: - self.status.fail( - UserErrors.FAILED_TO_IMPORT_MODULE.format( - leftover.display_name, leftover.location - ) - ) - raise + log.exception( + f'Course import {dest_id}: failed to import module location {leftover}' + ) + # pylint: disable=raise-missing-from + raise ModuleFailedToImport(leftover.display_name, leftover.location) def run_imports(self): """ @@ -606,10 +591,6 @@ class CourseImportManager(ImportManager): "Skipping import of course with id, %s, " "since it collides with an existing one", dest_id ) - if self.status: - self.status.fail( - UserErrors.COURSE_ALREADY_EXIST.format(dest_id) - ) raise return dest_id, runtime @@ -719,8 +700,6 @@ class LibraryImportManager(ImportManager): "Skipping import of Library with id %s, " "since it collides with an existing one", dest_id ) - if self.status: - self.status.fail(UserErrors.LIBRARY_ALREADY_EXIST) raise return dest_id, runtime @@ -785,6 +764,7 @@ def _update_and_import_module( """ Move the module to a new course. """ + def _convert_ref_fields_to_new_namespace(reference): """ Convert a reference to the new namespace, but only