diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index a6c59211e..dae8ffeec 100644 --- a/olx_importer/management/commands/load_components.py +++ b/olx_importer/management/commands/load_components.py @@ -173,7 +173,7 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry): content_api.create_component_version_media( component_version, text_content.pk, - key="block.xml", + path="block.xml", ) # Cycle through static assets references and add those as well... diff --git a/src/openedx_content/applets/backup_restore/zipper.py b/src/openedx_content/applets/backup_restore/zipper.py index 4a5413577..77bb26b7a 100644 --- a/src/openedx_content/applets/backup_restore/zipper.py +++ b/src/openedx_content/applets/backup_restore/zipper.py @@ -381,9 +381,8 @@ def create_zip(self, path: str) -> None: for component_version_media in prefetched_media: media: Media = component_version_media.media - # Important: The component_version_media.key contains implicitly - # the file name and the file extension - file_path = version_folder / component_version_media.key + # component_version_media.path is the file name and extension + file_path = version_folder / component_version_media.path if media.has_file and media.path: # If has_file, we pull it from the file system diff --git a/src/openedx_content/applets/components/admin.py b/src/openedx_content/applets/components/admin.py index 018f21bf1..2118f1cf3 100644 --- a/src/openedx_content/applets/components/admin.py +++ b/src/openedx_content/applets/components/admin.py @@ -50,9 +50,9 @@ class ComponentAdmin(ReadOnlyModelAdmin): inlines = [ComponentVersionInline] -class ContentInline(admin.TabularInline): +class MediaInline(admin.TabularInline): """ - Django admin configuration for Content + Django admin configuration for Media """ model = ComponentVersion.media.through @@ -69,13 +69,13 @@ def get_queryset(self, request): ) fields = [ - "key", + "path", "format_size", "rendered_data", ] readonly_fields = [ "media", - "key", + "path", "format_size", "rendered_data", ] @@ -113,7 +113,7 @@ class ComponentVersionAdmin(ReadOnlyModelAdmin): "created", ] list_display = ["component", "version_num", "uuid", "created"] - inlines = [ContentInline] + inlines = [MediaInline] def get_queryset(self, request): queryset = super().get_queryset(request) diff --git a/src/openedx_content/applets/components/api.py b/src/openedx_content/applets/components/api.py index 814fbf1ef..4c9f136a1 100644 --- a/src/openedx_content/applets/components/api.py +++ b/src/openedx_content/applets/components/api.py @@ -250,7 +250,7 @@ def create_next_component_version( ComponentVersionMedia.objects.create( media_id=media_pk, component_version=component_version, - key=key, + path=key, ) if ignore_previous_media: @@ -261,11 +261,11 @@ def create_next_component_version( last_version_media_mapping = ComponentVersionMedia.objects \ .filter(component_version=last_version) for cvrc in last_version_media_mapping: - if cvrc.key not in media_to_replace: + if cvrc.path not in media_to_replace: ComponentVersionMedia.objects.create( media_id=cvrc.media_id, component_version=component_version, - key=cvrc.key, + path=cvrc.path, ) return component_version @@ -435,7 +435,7 @@ def look_up_component_version_media( learning_package_ref: str, entity_ref: str, version_num: int, - key: Path, + path: Path, ) -> ComponentVersionMedia: """ Look up ComponentVersionMedia by human readable identifiers. @@ -450,7 +450,7 @@ def look_up_component_version_media( Q(component_version__component__learning_package__package_ref=learning_package_ref) & Q(component_version__component__publishable_entity__entity_ref=entity_ref) & Q(component_version__publishable_entity_version__version_num=version_num) - & Q(key=key) + & Q(path=path) ) return ComponentVersionMedia.objects \ .select_related( @@ -466,29 +466,29 @@ def create_component_version_media( component_version_id: int, media_id: int, /, - key: str, + path: str, ) -> ComponentVersionMedia: """ Add a Media to the given ComponentVersion - We don't allow keys that would be absolute paths, e.g. ones that start with + We don't allow paths that would be absolute, e.g. ones that start with '/'. Storing these causes headaches with building relative paths and because of mismatches with things that expect a leading slash and those that don't. So for safety and consistency, we strip off leading slashes and emit a warning when we do. """ - if key.startswith('/'): + if path.startswith('/'): logger.warning( "Absolute paths are not supported: " f"removed leading '/' from ComponentVersion {component_version_id} " - f"media key: {repr(key)} (media_id: {media_id})" + f"media path: {repr(path)} (media_id: {media_id})" ) - key = key.lstrip('/') + path = path.lstrip('/') cvrc, _created = ComponentVersionMedia.objects.get_or_create( component_version_id=component_version_id, media_id=media_id, - key=key, + path=path, ) return cvrc @@ -603,7 +603,7 @@ def _error_header(error: AssetError) -> dict[str, str]: # Check: Does the ComponentVersion have the requested asset (Media)? try: - cv_media = component_version.componentversionmedia_set.get(key=asset_path) + cv_media = component_version.componentversionmedia_set.get(path=asset_path) except ComponentVersionMedia.DoesNotExist: logger.error(f"ComponentVersion {component_version_uuid} has no asset {asset_path}") info_headers.update( diff --git a/src/openedx_content/applets/components/models.py b/src/openedx_content/applets/components/models.py index 2e16e7727..1852aae6f 100644 --- a/src/openedx_content/applets/components/models.py +++ b/src/openedx_content/applets/components/models.py @@ -258,10 +258,9 @@ class ComponentVersionMedia(models.Model): For instance, a Video ComponentVersion might be associated with multiple transcripts in different languages. - When Media is associated with an ComponentVersion, it has some local - key that is unique within the the context of that ComponentVersion. This - allows the ComponentVersion to do things like store an image file and - reference it by a "path" key. + When Media is associated with a ComponentVersion, it has a ``path`` + that is unique within the context of that ComponentVersion. This is + used as a local file-path-like identifier, e.g. "static/image.png". Media is immutable and sharable across multiple ComponentVersions. """ @@ -269,20 +268,15 @@ class ComponentVersionMedia(models.Model): component_version = models.ForeignKey(ComponentVersion, on_delete=models.CASCADE) media = models.ForeignKey(Media, on_delete=models.RESTRICT) - # "key" is a reserved word for MySQL, so we're temporarily using the column - # name of "_key" to avoid breaking downstream tooling. A possible - # alternative name for this would be "path", since it's most often used as - # an internal file path. However, we might also want to put special - # identifiers that don't map as cleanly to file paths at some point. - key = ref_field(db_column="_key") + # path is a local file-path-like identifier for the media within a + # ComponentVersion. + path = ref_field() class Meta: constraints = [ - # Uniqueness is only by ComponentVersion and key. If for some reason - # a ComponentVersion wants to associate the same piece of Media - # with two different identifiers, that is permitted. + # Uniqueness is only by ComponentVersion and path. models.UniqueConstraint( - fields=["component_version", "key"], + fields=["component_version", "path"], name="oel_cvcontent_uniq_cv_key", ), ] diff --git a/src/openedx_content/management/commands/add_assets_to_component.py b/src/openedx_content/management/commands/add_assets_to_component.py index 03bbaae50..21fc2cbe0 100644 --- a/src/openedx_content/management/commands/add_assets_to_component.py +++ b/src/openedx_content/management/commands/add_assets_to_component.py @@ -83,4 +83,4 @@ def handle(self, *args, **options): f"{next_version.component.entity_ref} ({next_version.uuid}):" ) for cvm in next_version.componentversionmedia_set.all(): - self.stdout.write(f"- {cvm.key} ({cvm.uuid})") + self.stdout.write(f"- {cvm.path} ({cvm.uuid})") diff --git a/src/openedx_content/migrations/0012_rename_componentversionmedia_key_to_path.py b/src/openedx_content/migrations/0012_rename_componentversionmedia_key_to_path.py new file mode 100644 index 000000000..ee44550df --- /dev/null +++ b/src/openedx_content/migrations/0012_rename_componentversionmedia_key_to_path.py @@ -0,0 +1,46 @@ +""" +Rename ComponentVersionMedia.key -> ComponentVersionMedia.path. + +The field previously had db_column='_key'; this migration also renames the +underlying DB column to match the new field name. +""" +from django.db import migrations, models + +import openedx_django_lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_content', '0011_rename_entity_key_and_package_key_to_refs'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='componentversionmedia', + name='oel_cvcontent_uniq_cv_key', + ), + migrations.RenameField( + model_name='componentversionmedia', + old_name='key', + new_name='path', + ), + # RenameField only changes the Django field name; the DB column is still + # '_key' (set via db_column). AlterField drops db_column, so Django sees + # old column='_key' vs new column='path' and renames it. + migrations.AlterField( + model_name='componentversionmedia', + name='path', + field=openedx_django_lib.fields.MultiCollationCharField( + db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, + max_length=500, + ), + ), + migrations.AddConstraint( + model_name='componentversionmedia', + constraint=models.UniqueConstraint( + fields=['component_version', 'path'], + name='oel_cvcontent_uniq_cv_key', + ), + ), + ] diff --git a/src/openedx_core/__init__.py b/src/openedx_core/__init__.py index 356ebefd6..415ef3d66 100644 --- a/src/openedx_core/__init__.py +++ b/src/openedx_core/__init__.py @@ -6,4 +6,4 @@ """ # The version for the entire repository -__version__ = "0.42.0" +__version__ = "0.43.0" diff --git a/tests/openedx_content/applets/backup_restore/test_backup.py b/tests/openedx_content/applets/backup_restore/test_backup.py index 74c4b320e..5dddce65a 100644 --- a/tests/openedx_content/applets/backup_restore/test_backup.py +++ b/tests/openedx_content/applets/backup_restore/test_backup.py @@ -109,7 +109,7 @@ def setUpTestData(cls): api.create_component_version_media( new_problem_version.pk, new_txt_media.pk, - key="hello.txt", + path="hello.txt", ) # Create a Draft component, one in each learning package @@ -138,7 +138,7 @@ def setUpTestData(cls): api.create_component_version_media( new_html_version.pk, cls.html_asset_media.id, - key="static/other/subdirectory/hello.html", + path="static/other/subdirectory/hello.html", ) components = api.get_publishable_entities(cls.learning_package) diff --git a/tests/openedx_content/applets/components/test_api.py b/tests/openedx_content/applets/components/test_api.py index b55a97730..b6960a226 100644 --- a/tests/openedx_content/applets/components/test_api.py +++ b/tests/openedx_content/applets/components/test_api.py @@ -422,7 +422,7 @@ def test_add(self): components_api.create_component_version_media( new_version.pk, new_media.pk, - key="my/path/to/hello.txt", + path="my/path/to/hello.txt", ) # re-fetch from the database to check to see if we wrote it correctly new_version = components_api.get_component(self.problem.id) \ @@ -430,7 +430,7 @@ def test_add(self): .get(publishable_entity_version__version_num=1) assert ( new_media == - new_version.media.get(componentversionmedia__key="my/path/to/hello.txt") + new_version.media.get(componentversionmedia__path="my/path/to/hello.txt") ) # Write the same content again, but to an absolute path (should auto- @@ -438,14 +438,14 @@ def test_add(self): components_api.create_component_version_media( new_version.pk, new_media.pk, - key="//nested/path/hello.txt", + path="//nested/path/hello.txt", ) new_version = components_api.get_component(self.problem.id) \ .versions \ .get(publishable_entity_version__version_num=1) assert ( new_media == - new_version.media.get(componentversionmedia__key="nested/path/hello.txt") + new_version.media.get(componentversionmedia__path="nested/path/hello.txt") ) def test_bytes_content(self): @@ -461,8 +461,8 @@ def test_bytes_content(self): created=self.now, ) - content_txt = version_1.media.get(componentversionmedia__key="raw.txt") - content_raw_txt = version_1.media.get(componentversionmedia__key="no_ext") + content_txt = version_1.media.get(componentversionmedia__path="raw.txt") + content_raw_txt = version_1.media.get(componentversionmedia__path="no_ext") assert content_txt.size == len(bytes_media) assert str(content_txt.media_type) == 'text/plain' @@ -509,12 +509,12 @@ def test_multiple_versions(self): assert ( hello_media == version_1.media - .get(componentversionmedia__key="hello.txt") + .get(componentversionmedia__path="hello.txt") ) assert ( goodbye_media == version_1.media - .get(componentversionmedia__key="goodbye.txt") + .get(componentversionmedia__path="goodbye.txt") ) # This should keep the old value for goodbye.txt, add blank.txt, and set @@ -533,17 +533,17 @@ def test_multiple_versions(self): assert ( blank_media == version_2.media - .get(componentversionmedia__key="hello.txt") + .get(componentversionmedia__path="hello.txt") ) assert ( goodbye_media == version_2.media - .get(componentversionmedia__key="goodbye.txt") + .get(componentversionmedia__path="goodbye.txt") ) assert ( blank_media == version_2.media - .get(componentversionmedia__key="blank.txt") + .get(componentversionmedia__path="blank.txt") ) # Now we're going to set "hello.txt" back to hello_content, but remove @@ -564,7 +564,7 @@ def test_multiple_versions(self): assert ( hello_media == version_3.media - .get(componentversionmedia__key="hello.txt") + .get(componentversionmedia__path="hello.txt") ) def test_create_next_version_forcing_num_version(self): @@ -626,17 +626,17 @@ def test_create_multiple_next_versions_and_diff_content(self): assert ( python_source_asset == version_2_draft.media.get( - componentversionmedia__key="static/profile.webp") + componentversionmedia__path="static/profile.webp") ) assert ( python_source_asset == version_2_draft.media.get( - componentversionmedia__key="static/new_file.webp") + componentversionmedia__path="static/new_file.webp") ) with self.assertRaises(ObjectDoesNotExist): # This file was in the published version, but not in the draft version # since we ignored previous content. - version_2_draft.media.get(componentversionmedia__key="static/background.webp") + version_2_draft.media.get(componentversionmedia__path="static/background.webp") class SetCollectionsTestCase(ComponentTestCase): diff --git a/tests/openedx_content/applets/components/test_assets.py b/tests/openedx_content/applets/components/test_assets.py index 2c10984cc..d580e86b2 100644 --- a/tests/openedx_content/applets/components/test_assets.py +++ b/tests/openedx_content/applets/components/test_assets.py @@ -75,7 +75,7 @@ def setUpTestData(cls) -> None: components_api.create_component_version_media( cls.component_version.pk, cls.problem_media.id, - key="block.xml", + path="block.xml", ) # Python source file, stored as a file. This is hypothetical, as we @@ -89,7 +89,7 @@ def setUpTestData(cls) -> None: components_api.create_component_version_media( cls.component_version.pk, cls.python_source_asset.id, - key="src/grader.py", + path="src/grader.py", ) # An HTML file that is student downloadable @@ -102,7 +102,7 @@ def setUpTestData(cls) -> None: components_api.create_component_version_media( cls.component_version.pk, cls.html_asset_media.id, - key="static/hello.html", + path="static/hello.html", ) def test_no_component_version(self):