From ff8f794a5cf63e9bd8ebc79e5852c97dec66fd4f Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 25 Oct 2024 07:19:03 -0500 Subject: [PATCH 1/9] feat: handle file content logic when creating next component version --- .../apps/authoring/components/api.py | 17 ++++++++++++++++- .../commands/add_assets_to_component.py | 16 +++------------- .../apps/authoring/components/test_api.py | 7 ++++--- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index 22750d9dd..f4e5fdfeb 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -12,6 +12,7 @@ """ from __future__ import annotations +import mimetypes from datetime import datetime, timezone from enum import StrEnum, auto from logging import getLogger @@ -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, @@ -191,6 +192,20 @@ def create_next_component_version( # 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 isinstance(content_pk, bytes): + file_path, file_content = key, content_pk + 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.publishable_entity.learning_package.id, + media_type.id, + data=file_content, + created=created, + ) + content_pk = content.pk ComponentVersionContent.objects.create( content_id=content_pk, component_version=component_version, diff --git a/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py b/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py index 3b37eda1c..6e4f1aefe 100644 --- a/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py +++ b/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py @@ -4,14 +4,12 @@ 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 @@ -70,7 +68,7 @@ def handle(self, *args, **options): created = datetime.now(tz=timezone.utc) keys_to_remove = set() - local_keys_to_content = {} + local_keys_to_raw_content = {} for file_mapping in file_mappings: local_key, file_path = file_mapping.split(":", 1) @@ -80,22 +78,14 @@ def handle(self, *args, **options): 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_raw_content[local_key] = pathlib.Path(file_path).read_bytes() next_version = create_next_component_version( component.pk, content_to_replace={local_key: None for local_key in keys_to_remove}, created=created, ) - for local_key, content_id in sorted(local_keys_to_content.items()): + for local_key, content_id in sorted(local_keys_to_raw_content.items()): create_component_version_content( next_version.pk, content_id, diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py index 9cb761daf..7a9537b13 100644 --- a/tests/openedx_learning/apps/authoring/components/test_api.py +++ b/tests/openedx_learning/apps/authoring/components/test_api.py @@ -442,13 +442,14 @@ def test_multiple_versions(self): content_to_replace={ "hello.txt": hello_content.pk, "goodbye.txt": goodbye_content.pk, + "raw.txt": b'raw content', }, created=self.now, ) assert version_1.version_num == 1 assert version_1.title == "Problem Version 1" version_1_contents = list(version_1.contents.all()) - assert len(version_1_contents) == 2 + assert len(version_1_contents) == 3 assert ( hello_content == version_1.contents @@ -472,7 +473,7 @@ def test_multiple_versions(self): created=self.now, ) assert version_2.version_num == 2 - assert version_2.contents.count() == 3 + assert version_2.contents.count() == 4 assert ( blank_content == version_2.contents @@ -503,7 +504,7 @@ def test_multiple_versions(self): created=self.now, ) assert version_3.version_num == 3 - assert version_3.contents.count() == 1 + assert version_3.contents.count() == 2 assert ( hello_content == version_3.contents From 7226354f98b170341f669ec6384a00db08ad6c02 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 25 Oct 2024 11:49:55 -0500 Subject: [PATCH 2/9] test: add tests for bytes content --- .../apps/authoring/components/test_api.py | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py index 7a9537b13..78ee79f8a 100644 --- a/tests/openedx_learning/apps/authoring/components/test_api.py +++ b/tests/openedx_learning/apps/authoring/components/test_api.py @@ -415,6 +415,31 @@ 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, @@ -442,14 +467,13 @@ def test_multiple_versions(self): content_to_replace={ "hello.txt": hello_content.pk, "goodbye.txt": goodbye_content.pk, - "raw.txt": b'raw content', }, created=self.now, ) assert version_1.version_num == 1 assert version_1.title == "Problem Version 1" version_1_contents = list(version_1.contents.all()) - assert len(version_1_contents) == 3 + assert len(version_1_contents) == 2 assert ( hello_content == version_1.contents @@ -473,7 +497,7 @@ def test_multiple_versions(self): created=self.now, ) assert version_2.version_num == 2 - assert version_2.contents.count() == 4 + assert version_2.contents.count() == 3 assert ( blank_content == version_2.contents @@ -504,7 +528,7 @@ def test_multiple_versions(self): created=self.now, ) assert version_3.version_num == 3 - assert version_3.contents.count() == 2 + assert version_3.contents.count() == 1 assert ( hello_content == version_3.contents From cfe49841a75de0494dae8932f938707fb057891c Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 25 Oct 2024 12:20:57 -0500 Subject: [PATCH 3/9] refactor: add/delete assets in one shot in command --- .../commands/add_assets_to_component.py | 20 +++---------------- .../apps/authoring/components/test_api.py | 1 - 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py b/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py index 6e4f1aefe..5e6518a99 100644 --- a/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py +++ b/openedx_learning/apps/authoring/components/management/commands/add_assets_to_component.py @@ -9,7 +9,6 @@ from django.core.management.base import BaseCommand -from ....components.api import create_component_version_content from ....publishing.api import get_learning_package_by_key from ...api import create_next_component_version, get_component_by_key @@ -67,31 +66,18 @@ def handle(self, *args, **options): ) created = datetime.now(tz=timezone.utc) - keys_to_remove = set() - local_keys_to_raw_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 - - local_keys_to_raw_content[local_key] = pathlib.Path(file_path).read_bytes() + 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_raw_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 " diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py index 78ee79f8a..2a683b05b 100644 --- a/tests/openedx_learning/apps/authoring/components/test_api.py +++ b/tests/openedx_learning/apps/authoring/components/test_api.py @@ -439,7 +439,6 @@ def test_bytes_content(self): 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, From da5ecfcaeaaa95366a8780d578d33ba4eb8f88b5 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 25 Oct 2024 12:53:04 -0500 Subject: [PATCH 4/9] chore: handle PR comments --- .../apps/authoring/components/api.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index f4e5fdfeb..9561bbb6d 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -144,8 +144,8 @@ def create_next_component_version( API, since ``content_to_replace`` needs Content IDs for the 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. 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 @@ -187,25 +187,27 @@ def create_next_component_version( component_id=component_pk, ) # First copy the new stuff over... - 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 isinstance(content_pk, bytes): - file_path, file_content = key, content_pk + 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.publishable_entity.learning_package.id, + 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, From d41cc649d362027fa63d38e5c032a42d864174db Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 25 Oct 2024 12:53:25 -0500 Subject: [PATCH 5/9] test: ignore migrations from coverage report --- .coveragerc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.coveragerc b/.coveragerc index 32fc62c81..3ae6aba7f 100644 --- a/.coveragerc +++ b/.coveragerc @@ -6,7 +6,7 @@ source = openedx_tagging omit = test_settings - *migrations* + **/migrations/* *admin.py *static* *templates* From c6bb9e8d53959d22b00ea58e751fa8332417a17a Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Fri, 25 Oct 2024 12:58:59 -0500 Subject: [PATCH 6/9] test: ignore tests from coverage report --- .coveragerc | 1 + 1 file changed, 1 insertion(+) diff --git a/.coveragerc b/.coveragerc index 3ae6aba7f..fb2ddbfe5 100644 --- a/.coveragerc +++ b/.coveragerc @@ -10,3 +10,4 @@ omit = *admin.py *static* *templates* + **/tests/** From 07b4d07e20b2f105cda4030f6c8bbf67e42a9f48 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 29 Oct 2024 09:00:22 -0500 Subject: [PATCH 7/9] docs: update create next component version docstring --- openedx_learning/apps/authoring/components/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index 9561bbb6d..f58dca6d1 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -141,7 +141,7 @@ 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`` or content bytes values. Using From 74ac2d96ee991391556830649e4931955ba2efce Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 29 Oct 2024 09:02:03 -0500 Subject: [PATCH 8/9] docs: update create next component version docstring --- openedx_learning/apps/authoring/components/api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index f58dca6d1..ace74fbb0 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/openedx_learning/apps/authoring/components/api.py @@ -147,6 +147,9 @@ def create_next_component_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 of ``{"a.txt": None, "c.txt": None}`` will remove ``a.txt`` from the next From d92e8e34b773e69e8df03b98bf8345d4ad5aca31 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Tue, 29 Oct 2024 09:03:02 -0500 Subject: [PATCH 9/9] chore: bump version to v0.16.3 --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index e4d85ebf3..1521ec581 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.16.2" +__version__ = "0.16.3"