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.
This commit is contained in:
Awais Jibran
2021-04-19 23:49:42 +05:00
committed by GitHub
parent 9e8684cc72
commit 62c8805e3e
7 changed files with 284 additions and 114 deletions

View File

@@ -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

View File

@@ -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)

View File

@@ -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']

View File

@@ -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

View File

@@ -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

View File

@@ -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

View File

@@ -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