diff --git a/common/djangoapps/util/file.py b/common/djangoapps/util/file.py index 5ee3f958df1f..8f98075b5deb 100644 --- a/common/djangoapps/util/file.py +++ b/common/djangoapps/util/file.py @@ -7,6 +7,7 @@ from datetime import datetime import six +from django.conf import settings from django.core.exceptions import PermissionDenied from django.core.files.storage import DefaultStorage, get_valid_filename from django.utils.translation import ugettext as _ @@ -90,16 +91,30 @@ def store_uploaded_file( def course_filename_prefix_generator(course_id, separator='_'): """ - Generates a course-identifying unicode string for use in a file - name. + Generates a course-identifying unicode string for use in a file name. Args: course_id (object): A course identification object. + separator (str): The character or chain of characters used for separating course details in + the filename. Returns: - str: A unicode string which can safely be inserted into a - filename. + str: A unicode string which can safely be inserted into a filename. """ - return get_valid_filename(six.text_type(separator).join([course_id.org, course_id.course, course_id.run])) + filename = six.text_type(separator).join([ + course_id.org, + course_id.course, + course_id.run + ]) + + enable_course_filename_ccx_suffix = settings.FEATURES.get( + 'ENABLE_COURSE_FILENAME_CCX_SUFFIX', + False + ) + + if enable_course_filename_ccx_suffix and getattr(course_id, 'ccx', None): + filename = six.text_type(separator).join([filename, 'ccx', course_id.ccx]) + + return get_valid_filename(filename) def course_and_time_based_filename_generator(course_id, base_name): diff --git a/common/djangoapps/util/tests/test_file.py b/common/djangoapps/util/tests/test_file.py index 0a3ef652604d..d2803849c177 100644 --- a/common/djangoapps/util/tests/test_file.py +++ b/common/djangoapps/util/tests/test_file.py @@ -15,11 +15,14 @@ from django.http import HttpRequest from django.test import TestCase from mock import Mock, patch +from django.test.utils import override_settings from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import CourseLocator from pytz import UTC from six import text_type +from ccx_keys.locator import CCXLocator + import common.djangoapps.util.file from common.djangoapps.util.file import ( FileValidationException, @@ -35,13 +38,62 @@ class FilenamePrefixGeneratorTestCase(TestCase): """ Tests for course_filename_prefix_generator """ - @ddt.data(CourseLocator(org='foo', course='bar', run='baz'), CourseKey.from_string('foo/bar/baz')) + @ddt.data( + CourseLocator(org='foo', course='bar', run='baz'), + CourseKey.from_string('foo/bar/baz'), + CCXLocator.from_course_locator(CourseLocator(org='foo', course='bar', run='baz'), '1'), + ) def test_locators(self, course_key): - self.assertEqual(course_filename_prefix_generator(course_key), u'foo_bar_baz') + """ + Test filename prefix genaration from multiple course key formats. + + Test that the filename prefix is generated from a CCX course locator or a course key. If the + filename is generated for a CCX course but the related 'ENABLE_COURSE_FILENAME_CCX_SUFFIX' + feature is not turned on, the generated filename shouldn't contain the CCX course ID. + """ + assert course_filename_prefix_generator(course_key) == 'foo_bar_baz' + + @ddt.data( + [CourseLocator(org='foo', course='bar', run='baz'), 'foo_bar_baz'], + [CourseKey.from_string('foo/bar/baz'), 'foo_bar_baz'], + [CCXLocator.from_course_locator(CourseLocator(org='foo', course='bar', run='baz'), '1'), 'foo_bar_baz_ccx_1'], + ) + @ddt.unpack + @override_settings(FEATURES={'ENABLE_COURSE_FILENAME_CCX_SUFFIX': True}) + def test_include_ccx_id(self, course_key, expected_filename): + """ + Test filename prefix genaration from multiple course key formats. + + Test that the filename prefix is generated from a CCX course locator or a course key. If the + filename is generated for a CCX course but the related 'ENABLE_COURSE_FILENAME_CCX_SUFFIX' + feature is not turned on, the generated filename shouldn't contain the CCX course ID. + """ + assert course_filename_prefix_generator(course_key) == expected_filename @ddt.data(CourseLocator(org='foo', course='bar', run='baz'), CourseKey.from_string('foo/bar/baz')) def test_custom_separator(self, course_key): - self.assertEqual(course_filename_prefix_generator(course_key, separator='-'), u'foo-bar-baz') + """ + Test filename prefix is generated with a custom separator. + + The filename should be build up from the course locator separated by a custom separator. + """ + assert course_filename_prefix_generator(course_key, separator='-') == 'foo-bar-baz' + + @ddt.data( + [CourseLocator(org='foo', course='bar', run='baz'), 'foo-bar-baz'], + [CourseKey.from_string('foo/bar/baz'), 'foo-bar-baz'], + [CCXLocator.from_course_locator(CourseLocator(org='foo', course='bar', run='baz'), '1'), 'foo-bar-baz-ccx-1'], + ) + @ddt.unpack + @override_settings(FEATURES={'ENABLE_COURSE_FILENAME_CCX_SUFFIX': True}) + def test_custom_separator_including_ccx_id(self, course_key, expected_filename): + """ + Test filename prefix is generated with a custom separator. + + The filename should be build up from the course locator separated by a custom separator + including the CCX ID if the related 'ENABLE_COURSE_FILENAME_CCX_SUFFIX' is turned on. + """ + assert course_filename_prefix_generator(course_key, separator='-') == expected_filename @ddt.ddt diff --git a/lms/envs/common.py b/lms/envs/common.py index f325a5e42914..9fb2adbfc5e7 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -493,6 +493,17 @@ # .. toggle_tickets: https://github.com/edx/edx-platform/pull/7845 'ENABLE_COURSE_DISCOVERY': False, + # .. toggle_name: FEATURES['ENABLE_COURSE_FILENAME_CCX_SUFFIX'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: If set to True, CCX ID will be included in the generated filename for CCX courses. + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2021-03-16 + # .. toggle_target_removal_date: None + # .. toggle_tickets: None + # .. toggle_warnings: Turning this feature ON will affect all generated filenames which are related to CCX courses. + 'ENABLE_COURSE_FILENAME_CCX_SUFFIX': False, + # Setting for overriding default filtering facets for Course discovery # COURSE_DISCOVERY_FILTERS = ["org", "language", "modes"]