fix: don't send emails on library backup/restore
There is no way to resume either the backup or restore library actions, i.e. if you navigate away from it, you have to do it again. This is a limitation of the current UI because we wanted to get something quick and simple in for Ulmo, but it also reflects the fact that library backup/restore should be much faster than course import/export has historically been. In any case, sending an email for a 5-10 second task is unnecessary and distracting, so this commit suppresses the email. Note: I'm using local imports to get around the fact that the content_libraries public API is used by content_libraries/tasks.py which defines the tasks. I can't import from content_libraries/tasks.py directly, because that would violate import linter rules forbidding other apps from importing things outside of api.py. This isn't ideal, but it keeps the fix small and it keeps the logic in the content_libraries app.
This commit is contained in:
@@ -12,6 +12,7 @@ from user_tasks.signals import user_task_stopped
|
||||
|
||||
from cms.djangoapps.contentstore.toggles import bypass_olx_failure_enabled
|
||||
from cms.djangoapps.contentstore.utils import course_import_olx_validation_is_enabled
|
||||
from openedx.core.djangoapps.content_libraries.api import is_library_backup_task, is_library_restore_task
|
||||
|
||||
from .tasks import send_task_complete_email
|
||||
|
||||
@@ -64,6 +65,28 @@ def user_task_stopped_handler(sender, **kwargs): # pylint: disable=unused-argum
|
||||
if olx_artifact and not bypass_olx_failure_enabled():
|
||||
return olx_artifact.text
|
||||
|
||||
def should_skip_end_of_task_email(task_name) -> bool:
|
||||
"""
|
||||
Studio tasks generally send an email when finished, but not always.
|
||||
|
||||
Some tasks can last many minutes, e.g. course import/export. For these
|
||||
tasks, there is a high chance that the user has navigated away and will
|
||||
want to check back in later. Yet email notification is unnecessary and
|
||||
distracting for things like the Library restore task, which is
|
||||
relatively quick and cannot be resumed (i.e. if you navigate away, you
|
||||
have to upload again).
|
||||
|
||||
The task_name passed in will be lowercase.
|
||||
"""
|
||||
# We currently have to pattern match on the name to differentiate
|
||||
# between tasks. A better long term solution would be to add a separate
|
||||
# task type identifier field to Django User Tasks.
|
||||
return (
|
||||
is_library_content_update(task_name) or
|
||||
is_library_backup_task(task_name) or
|
||||
is_library_restore_task(task_name)
|
||||
)
|
||||
|
||||
status = kwargs['status']
|
||||
|
||||
# Only send email when the entire task is complete, should only send when
|
||||
@@ -72,7 +95,7 @@ def user_task_stopped_handler(sender, **kwargs): # pylint: disable=unused-argum
|
||||
task_name = status.name.lower()
|
||||
|
||||
# Also suppress emails on library content XBlock updates (too much like spam)
|
||||
if is_library_content_update(task_name):
|
||||
if should_skip_end_of_task_email(task_name):
|
||||
LOGGER.info(f"Suppressing end-of-task email on task {task_name}")
|
||||
return
|
||||
|
||||
|
||||
@@ -108,6 +108,8 @@ __all__ = [
|
||||
"get_backup_task_status",
|
||||
"assign_library_role_to_user",
|
||||
"user_has_permission_across_lib_authz_systems",
|
||||
"is_library_backup_task",
|
||||
"is_library_restore_task",
|
||||
]
|
||||
|
||||
|
||||
@@ -852,3 +854,15 @@ def _is_legacy_permission(permission: str) -> bool:
|
||||
or the new openedx-authz system.
|
||||
"""
|
||||
return permission in LEGACY_LIB_PERMISSIONS
|
||||
|
||||
|
||||
def is_library_backup_task(task_name: str) -> bool:
|
||||
"""Case-insensitive match to see if a task is a library backup."""
|
||||
from ..tasks import LibraryBackupTask # avoid circular import error
|
||||
return task_name.startswith(LibraryBackupTask.NAME_PREFIX.lower())
|
||||
|
||||
|
||||
def is_library_restore_task(task_name: str) -> bool:
|
||||
"""Case-insensitive match to see if a task is a library restore."""
|
||||
from ..tasks import LibraryRestoreTask # avoid circular import error
|
||||
return task_name.startswith(LibraryRestoreTask.NAME_PREFIX.lower())
|
||||
|
||||
@@ -512,6 +512,7 @@ class LibraryBackupTask(UserTask): # pylint: disable=abstract-method
|
||||
"""
|
||||
Base class for tasks related with Library backup functionality.
|
||||
"""
|
||||
NAME_PREFIX = "Library Learning Package Backup"
|
||||
|
||||
@classmethod
|
||||
def generate_name(cls, arguments_dict) -> str:
|
||||
@@ -529,7 +530,7 @@ class LibraryBackupTask(UserTask): # pylint: disable=abstract-method
|
||||
str: The generated name
|
||||
"""
|
||||
key = arguments_dict['library_key_str']
|
||||
return f'Backup of {key}'
|
||||
return f'{cls.NAME_PREFIX} of {key}'
|
||||
|
||||
|
||||
@shared_task(base=LibraryBackupTask, bind=True)
|
||||
@@ -591,10 +592,12 @@ class LibraryRestoreTask(UserTask):
|
||||
|
||||
ERROR_LOG_ARTIFACT_NAME = 'Error log'
|
||||
|
||||
NAME_PREFIX = "Library Learning Package Restore"
|
||||
|
||||
@classmethod
|
||||
def generate_name(cls, arguments_dict):
|
||||
storage_path = arguments_dict['storage_path']
|
||||
return f'learning package restore of {storage_path}'
|
||||
return f'{cls.NAME_PREFIX} of {storage_path}'
|
||||
|
||||
def fail_with_error_log(self, logfile) -> None:
|
||||
"""
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
"""
|
||||
Unit tests for content libraries Celery tasks
|
||||
"""
|
||||
from unittest import mock
|
||||
|
||||
from django.test import override_settings
|
||||
from ..models import ContentLibrary
|
||||
@@ -14,6 +15,7 @@ class ContentLibraryBackupTaskTest(ContentLibrariesRestApiTest):
|
||||
"""
|
||||
Tests for Content Library export task.
|
||||
"""
|
||||
SEND_TASK_COMPLETE_FN = 'cms.djangoapps.cms_user_tasks.tasks.send_task_complete_email.delay'
|
||||
|
||||
def setUp(self) -> None:
|
||||
super().setUp()
|
||||
@@ -31,7 +33,9 @@ class ContentLibraryBackupTaskTest(ContentLibrariesRestApiTest):
|
||||
|
||||
@override_settings(CMS_BASE="test.com")
|
||||
def test_backup_task_success(self):
|
||||
result = backup_library.delay(self.user.id, str(self.lib1.library_key))
|
||||
with mock.patch(self.SEND_TASK_COMPLETE_FN) as send_task_complete_email:
|
||||
result = backup_library.delay(self.user.id, str(self.lib1.library_key))
|
||||
send_task_complete_email.assert_not_called()
|
||||
assert result.state == 'SUCCESS'
|
||||
# Ensure an artifact was created with the output file
|
||||
artifact = UserTaskArtifact.objects.filter(status__task_id=result.task_id, name='Output').first()
|
||||
@@ -44,7 +48,9 @@ class ContentLibraryBackupTaskTest(ContentLibrariesRestApiTest):
|
||||
assert b'origin_server = "test.com"' in content
|
||||
|
||||
def test_backup_task_failure(self):
|
||||
result = backup_library.delay(self.user.id, self.wrong_task_id)
|
||||
with mock.patch(self.SEND_TASK_COMPLETE_FN) as send_task_complete_email:
|
||||
result = backup_library.delay(self.user.id, self.wrong_task_id)
|
||||
send_task_complete_email.assert_not_called()
|
||||
assert result.state == 'FAILURE'
|
||||
# Ensure an error artifact was created
|
||||
artifact = UserTaskArtifact.objects.filter(status__task_id=result.task_id, name='Error').first()
|
||||
|
||||
Reference in New Issue
Block a user