Skip to content
Merged
3 changes: 2 additions & 1 deletion .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ source =
openedx_tagging
omit =
test_settings
*migrations*
**/migrations/*
*admin.py
*static*
*templates*
**/tests/**
2 changes: 1 addition & 1 deletion openedx_learning/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Open edX Learning ("Learning Core").
"""

__version__ = "0.16.2"
__version__ = "0.16.3"
32 changes: 26 additions & 6 deletions openedx_learning/apps/authoring/components/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"""
from __future__ import annotations

import mimetypes
from datetime import datetime, timezone
from enum import StrEnum, auto
from logging import getLogger
Expand Down Expand Up @@ -129,7 +130,7 @@ def create_component_version(
def create_next_component_version(
component_pk: int,
/,
content_to_replace: dict[str, int | None],
content_to_replace: dict[str, int | None | bytes],
created: datetime,
title: str | None = None,
created_by: int | None = None,
Expand All @@ -140,11 +141,14 @@ def create_next_component_version(
A very common pattern for making a new ComponentVersion is going to be "make
it just like the last version, except changing these one or two things".
Before calling this, you should create any new contents via the contents
API, since ``content_to_replace`` needs Content IDs for the values.
API or send the content bytes as part of ``content_to_replace`` values.

The ``content_to_replace`` dict is a mapping of strings representing the
local path/key for a file, to ``Content.id`` values. Using a `None` for
a value in this dict means to delete that key in the next version.
local path/key for a file, to ``Content.id`` or content bytes values. Using
`None` for a value in this dict means to delete that key in the next version.

Make sure to wrap the function call on a atomic statement:
``with transaction.atomic():``

It is okay to mark entries for deletion that don't exist. For instance, if a
version has ``a.txt`` and ``b.txt``, sending a ``content_to_replace`` value
Expand Down Expand Up @@ -186,11 +190,27 @@ def create_next_component_version(
component_id=component_pk,
)
# First copy the new stuff over...
Copy link
Contributor

Choose a reason for hiding this comment

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

[non-blocking question] Since we're potentially doing multiple insert operations here, should this method wrap its contents in with atomic():? We have a few other api methods that do.

Would also be ok with updating the function docs to recommend wrapping the method call with atomic if passing content bytes in, since that's what the ticket did.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, this was an oversight on my part in the review. Whenever possible, my intent was for these API functions to be atomic, so that we don't get inconsistent half-writes to the database. (Inconsistent half-writes to the file store are harder to guard against, but I feel better about that since they're idempotent inserts storing file data by hash–so "do it again" results in the same thing and hopefully the worst thing we do is write some unreferenced data.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ormsbee should we do a follow-up PR to wrap this function in a transaction?

Copy link
Contributor Author

@Ian2012 Ian2012 Oct 31, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

😄 Ah, that makes sense. Thank you!

for key, content_pk in content_to_replace.items():
for key, content_pk_or_bytes in content_to_replace.items():
# If the content_pk is None, it means we want to remove the
# content represented by our key from the next version. Otherwise,
# we add our key->content_pk mapping to the next version.
if content_pk is not None:
if content_pk_or_bytes is not None:
if isinstance(content_pk_or_bytes, bytes):
file_path, file_content = key, content_pk_or_bytes
media_type_str, _encoding = mimetypes.guess_type(file_path)
# We use "application/octet-stream" as a generic fallback media type, per
# RFC 2046: https://datatracker.ietf.org/doc/html/rfc2046
media_type_str = media_type_str or "application/octet-stream"
media_type = contents_api.get_or_create_media_type(media_type_str)
content = contents_api.get_or_create_file_content(
component.learning_package.id,
media_type.id,
data=file_content,
created=created,
)
content_pk = content.pk
else:
content_pk = content_pk_or_bytes
ComponentVersionContent.objects.create(
content_id=content_pk,
component_version=component_version,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,11 @@
This is mostly meant to be a debugging tool to let us to easily load some test
asset data into the system.
"""
import mimetypes
import pathlib
from datetime import datetime, timezone

from django.core.management.base import BaseCommand

from ....components.api import create_component_version_content
from ....contents.api import get_or_create_file_content, get_or_create_media_type
from ....publishing.api import get_learning_package_by_key
from ...api import create_next_component_version, get_component_by_key

Expand Down Expand Up @@ -69,39 +66,18 @@ def handle(self, *args, **options):
)

created = datetime.now(tz=timezone.utc)
keys_to_remove = set()
local_keys_to_content = {}
local_keys_to_content_bytes = {}

for file_mapping in file_mappings:
local_key, file_path = file_mapping.split(":", 1)

# No file_path means to delete this entry from the next version.
if not file_path:
keys_to_remove.add(local_key)
continue

media_type_str, _encoding = mimetypes.guess_type(file_path)
media_type = get_or_create_media_type(media_type_str)
content = get_or_create_file_content(
learning_package.id,
media_type.id,
data=pathlib.Path(file_path).read_bytes(),
created=created,
)
local_keys_to_content[local_key] = content.id
local_keys_to_content_bytes[local_key] = pathlib.Path(file_path).read_bytes() if file_path else None

next_version = create_next_component_version(
component.pk,
content_to_replace={local_key: None for local_key in keys_to_remove},
content_to_replace=local_keys_to_content_bytes,
created=created,
)
for local_key, content_id in sorted(local_keys_to_content.items()):
create_component_version_content(
next_version.pk,
content_id,
key=local_key,
learner_downloadable=True,
)

self.stdout.write(
f"Created v{next_version.version_num} of "
Expand Down
24 changes: 24 additions & 0 deletions tests/openedx_learning/apps/authoring/components/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,30 @@ def test_add(self):
new_version.contents.get(componentversioncontent__key="nested/path/hello.txt")
)

def test_bytes_content(self):
bytes_content = b'raw content'

version_1 = components_api.create_next_component_version(
self.problem.pk,
title="Problem Version 1",
content_to_replace={
"raw.txt": bytes_content,
"no_ext": bytes_content,
},
created=self.now,
)

content_txt = version_1.contents.get(componentversioncontent__key="raw.txt")
content_raw_txt = version_1.contents.get(componentversioncontent__key="no_ext")

assert content_txt.size == len(bytes_content)
assert str(content_txt.media_type) == 'text/plain'
assert content_txt.read_file().read() == bytes_content

assert content_raw_txt.size == len(bytes_content)
assert str(content_raw_txt.media_type) == 'application/octet-stream'
assert content_raw_txt.read_file().read() == bytes_content

def test_multiple_versions(self):
hello_content = contents_api.get_or_create_text_content(
self.learning_package.id,
Expand Down