From 58231c4262ad54a6dd3ec90424e1dc7552ce23d8 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Thu, 15 Apr 2021 12:25:04 +0500 Subject: [PATCH] Moved Import error messages to single place (#27331) --- cms/djangoapps/contentstore/errors.py | 19 ++++++++++++++ cms/djangoapps/contentstore/tasks.py | 25 +++++++++---------- .../xmodule/modulestore/xml_importer.py | 13 +++++----- 3 files changed, 38 insertions(+), 19 deletions(-) create mode 100644 cms/djangoapps/contentstore/errors.py diff --git a/cms/djangoapps/contentstore/errors.py b/cms/djangoapps/contentstore/errors.py new file mode 100644 index 0000000000..4193dddf52 --- /dev/null +++ b/cms/djangoapps/contentstore/errors.py @@ -0,0 +1,19 @@ +""" +User facing error messages during course import and export +""" +from django.utils.translation import ugettext as _ + +COURSE_ALREADY_EXIST = _('Aborting import because a course with this id: {} already exists.') +COURSE_PERMISSION_DENIED = _('Permission denied. You do not have write access to this course.') +ERROR_WHILE_READING = _('Error while reading {}. Check file for XML errors.') +FAILED_TO_IMPORT_MODULE = _('Failed to import module: {} at location: {}') +FILE_MISSING = _('Could not find the {0} file in the package.') +FILE_NOT_FOUND = _('Uploaded Tar file not found. Try again.') +INVALID_FILE_TYPE = _('We only support uploading a .tar.gz file.') +LIBRARY_ALREADY_EXIST = _('Aborting import since a library with this id already exists.') +PERMISSION_DENIED = _('Permission denied') +UNKNOWN_ERROR_IN_IMPORT = _('Unknown error while importing course.') +UNKNOWN_ERROR_IN_UNPACKING = _('An Unknown error occurred during the unpacking step.') +UNKNOWN_USER_ID = _('Unknown User ID: {0}') +UNSAFE_TAR_FILE = _('Unsafe tar file. Aborting import.') +USER_PERMISSION_DENIED = _('User permission denied.') diff --git a/cms/djangoapps/contentstore/tasks.py b/cms/djangoapps/contentstore/tasks.py index 2fb4d8db2b..b6c9fb0d68 100644 --- a/cms/djangoapps/contentstore/tasks.py +++ b/cms/djangoapps/contentstore/tasks.py @@ -22,7 +22,6 @@ from django.core.exceptions import SuspiciousOperation from django.core.files import File from django.test import RequestFactory from django.utils.text import get_valid_filename -from django.utils.translation import ugettext as _ from edx_django_utils.monitoring import ( set_code_owner_attribute, set_code_owner_attribute_from_module, @@ -62,7 +61,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError, InvalidProctoringProvider 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 .outlines import update_outline_from_modulestore from .utils import course_import_olx_validation_is_enabled @@ -276,11 +275,11 @@ def export_olx(self, user_id, course_key_string, language): user = User.objects.get(pk=user_id) except User.DoesNotExist: with translation_language(language): - self.status.fail(_('Unknown User ID: {0}').format(user_id)) + self.status.fail(UserErrors.UNKNOWN_USER_ID.format(user_id)) return if not has_course_author_access(user, courselike_key): with translation_language(language): - self.status.fail(_('Permission denied')) + self.status.fail(UserErrors.PERMISSION_DENIED) return if isinstance(courselike_key, LibraryLocator): @@ -421,7 +420,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan return User.objects.get(pk=user_id) except User.DoesNotExist as exc: with translation_language(language): - self.status.fail(_('User permission denied.')) + self.status.fail(UserErrors.USER_PERMISSION_DENIED) LOGGER.error(f'{log_prefix}: Unknown User: {user_id}') monitor_import_failure(courselike_key, current_step, exception=exc) return @@ -432,7 +431,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan if not has_access: message = f'User permission denied: {user.username}' with translation_language(language): - self.status.fail(_('Permission denied. You do not have write access to this course.')) + self.status.fail(UserErrors.COURSE_PERMISSION_DENIED) LOGGER.error(f'{log_prefix}: {message}') monitor_import_failure(courselike_key, current_step, message=message) return has_access @@ -444,7 +443,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan if not file_is_valid: message = f'Unsupported file {archive_name}' with translation_language(language): - self.status.fail(_('We only support uploading a .tar.gz file.')) + self.status.fail(UserErrors.INVALID_FILE_TYPE) LOGGER.error(f'{log_prefix}: {message}') monitor_import_failure(courselike_key, current_step, message=message) return file_is_valid @@ -456,7 +455,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan if not archive_path_exists: message = f'Uploaded file {archive_path} not found' with translation_language(language): - self.status.fail(_('Uploaded Tar file not found. Try again.')) + self.status.fail(UserErrors.FILE_NOT_FOUND) LOGGER.error(f'{log_prefix}: {message}') monitor_import_failure(courselike_key, current_step, message=message) return archive_path_exists @@ -486,9 +485,9 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan dirpath = get_dir_for_filename(course_dir, root_name) if not dirpath: - message = f'Could not find the {root_name} file in the package.' + message = UserErrors.FILE_MISSING.format(root_name) with translation_language(language): - self.status.fail(_('Could not find the {0} file in the package.').format(root_name)) + self.status.fail(message) LOGGER.error(f'{log_prefix}: {message}') monitor_import_failure(courselike_key, current_step, message=message) return @@ -561,7 +560,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan shutil.rmtree(course_dir) LOGGER.info(f'{log_prefix}: Temp data cleared') - self.status.fail(_('An Unknown error occurred during the unpacking step.')) + self.status.fail(UserErrors.UNKNOWN_ERROR_IN_UNPACKING) LOGGER.exception(f'{log_prefix}: Unknown error while unpacking', exc_info=True) monitor_import_failure(courselike_key, current_step, exception=exception) return @@ -573,7 +572,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan safetar_extractall(tar_file, (course_dir + '/')) except SuspiciousOperation as exc: with translation_language(language): - self.status.fail(_('Unsafe tar file. Aborting import.')) + self.status.fail(UserErrors.UNSAFE_TAR_FILE) LOGGER.error(f'{log_prefix}: Unsafe tar file') monitor_import_failure(courselike_key, current_step, exception=exc) return @@ -616,7 +615,7 @@ def import_olx(self, user_id, course_key_string, archive_path, archive_name, lan set_custom_attribute('course_import_completed', True) except Exception as exception: # pylint: disable=broad-except msg = str(exception) - status_msg = _('Unknown error while importing course.') + 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)}') diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 0e967acf05..c1684e5f00 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -30,7 +30,6 @@ import re from abc import abstractmethod import xblock -from django.utils.translation import ugettext as _ from lxml import etree from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryLocator @@ -39,6 +38,8 @@ 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 common.djangoapps.util.monitoring import monitor_import_failure from xmodule.assetstore import AssetMetadata from xmodule.contentstore.content import StaticContent @@ -366,7 +367,7 @@ class ImportManager: 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(_('Error while reading {}. Check file for XML errors.').format(assets_filename)) + self.status.fail(UserErrors.ERROR_WHILE_READING).format(assets_filename) raise else: return @@ -486,7 +487,7 @@ class ImportManager: ) if self.status: self.status.fail( - _('Failed to import module: {} at location: {}').format( + UserErrors.FAILED_TO_IMPORT_MODULE.format( child.display_name, child.location ) ) @@ -515,7 +516,7 @@ class ImportManager: log.error(msg) if self.status: self.status.fail( - _('Failed to import module: {} at location: {}').format( + UserErrors.FAILED_TO_IMPORT_MODULE.format( leftover.display_name, leftover.location ) ) @@ -607,7 +608,7 @@ class CourseImportManager(ImportManager): ) if self.status: self.status.fail( - _('Aborting import because a course with this id: {} already exists.').format(dest_id) + UserErrors.COURSE_ALREADY_EXIST.format(dest_id) ) raise @@ -719,7 +720,7 @@ class LibraryImportManager(ImportManager): "since it collides with an existing one", dest_id ) if self.status: - self.status.fail(_('Aborting import since a library with this id already exists.')) + self.status.fail(UserErrors.LIBRARY_ALREADY_EXIST) raise return dest_id, runtime