diff --git a/common/djangoapps/util/file.py b/common/djangoapps/util/file.py index aac1864329f8..14abb895725c 100644 --- a/common/djangoapps/util/file.py +++ b/common/djangoapps/util/file.py @@ -6,6 +6,7 @@ import os from datetime import datetime +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 _ @@ -89,16 +90,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(str(separator).join([course_id.org, course_id.course, course_id.run])) + filename = str(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 = 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 1bb033798ce3..cc16cd36129d 100644 --- a/common/djangoapps/util/tests/test_file.py +++ b/common/djangoapps/util/tests/test_file.py @@ -14,10 +14,13 @@ from django.core.files.uploadedfile import SimpleUploadedFile from django.http import HttpRequest from django.test import TestCase +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 ccx_keys.locator import CCXLocator + import common.djangoapps.util.file from common.djangoapps.util.file import ( FileValidationException, @@ -33,14 +36,63 @@ 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): + """ + 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): + """ + 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 class FilenameGeneratorTestCase(TestCase): diff --git a/lms/envs/common.py b/lms/envs/common.py index a4b51690c892..0e749e8194d2 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -636,6 +636,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"]