Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion olx_importer/management/commands/load_components.py
Original file line number Diff line number Diff line change
Expand Up @@ -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...
Expand Down
5 changes: 2 additions & 3 deletions src/openedx_content/applets/backup_restore/zipper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions src/openedx_content/applets/components/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ class ComponentAdmin(ReadOnlyModelAdmin):
inlines = [ComponentVersionInline]


class ContentInline(admin.TabularInline):
class MediaInline(admin.TabularInline):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I feel silly for missing that one.

"""
Django admin configuration for Content
Django admin configuration for Media
Comment on lines -53 to +55
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, just something I noticed

"""
model = ComponentVersion.media.through

Expand All @@ -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",
]
Expand Down Expand Up @@ -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)
Expand Down
24 changes: 12 additions & 12 deletions src/openedx_content/applets/components/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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(
Expand All @@ -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

Expand Down Expand Up @@ -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(
Expand Down
22 changes: 8 additions & 14 deletions src/openedx_content/applets/components/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,31 +258,25 @@ 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.
"""

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",
),
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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})")
Original file line number Diff line number Diff line change
@@ -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',
),
),
]
2 changes: 1 addition & 1 deletion src/openedx_core/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
"""

# The version for the entire repository
__version__ = "0.42.0"
__version__ = "0.43.0"
4 changes: 2 additions & 2 deletions tests/openedx_content/applets/backup_restore/test_backup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
30 changes: 15 additions & 15 deletions tests/openedx_content/applets/components/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -422,30 +422,30 @@ 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) \
.versions \
.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-
# strip) the leading '/'s.
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):
Expand All @@ -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'
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
6 changes: 3 additions & 3 deletions tests/openedx_content/applets/components/test_assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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):
Expand Down