From ceeff166f19f670dbf1b2d2d10616c33e130e157 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Fri, 15 Aug 2025 11:49:57 -0600 Subject: [PATCH 1/7] feat: add component versioning directory structure In the lp_dump file, only the draft and publishable versions are serialized, rather than all versions. Static files remain out of scope for this change. --- .../apps/authoring/backup_restore/zipper.py | 77 ++++++++++++------- .../authoring/backup_restore/test_backup.py | 12 ++- 2 files changed, 58 insertions(+), 31 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index c71409fd8..3c2bb69b3 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -4,10 +4,16 @@ """ import zipfile from pathlib import Path +from typing import Optional from openedx_learning.apps.authoring.backup_restore.toml import toml_learning_package, toml_publishable_entity from openedx_learning.apps.authoring.publishing import api as publishing_api -from openedx_learning.apps.authoring.publishing.models.learning_package import LearningPackage +from openedx_learning.apps.authoring.publishing.models import ( + Draft, + LearningPackage, + PublishableEntityVersion, + Published, +) TOML_PACKAGE_NAME = "package.toml" @@ -19,15 +25,19 @@ class LearningPackageZipper: def __init__(self, learning_package: LearningPackage): self.learning_package = learning_package + self.folders_already_created: set[Path] = set() def create_folder(self, folder_path: Path, zip_file: zipfile.ZipFile) -> None: """ Create a folder for the zip file structure. + Skips creating the folder if it already exists based on the folder path. Args: folder_path (Path): The path of the folder to create. """ - zip_info = zipfile.ZipInfo(str(folder_path) + "/") - zip_file.writestr(zip_info, "") # Add explicit empty directory entry + if folder_path not in self.folders_already_created: + zip_info = zipfile.ZipInfo(str(folder_path) + "/") + zip_file.writestr(zip_info, "") # Add explicit empty directory entry + self.folders_already_created.add(folder_path) def create_zip(self, path: str) -> None: """ @@ -38,7 +48,6 @@ def create_zip(self, path: str) -> None: Exception: If the learning package cannot be found or if the zip creation fails. """ package_toml_content: str = toml_learning_package(self.learning_package) - folders_already_created = set() with zipfile.ZipFile(path, "w", compression=zipfile.ZIP_DEFLATED) as zipf: # Add the package.toml string @@ -75,21 +84,15 @@ def create_zip(self, path: str) -> None: component_namespace_folder = entities_folder / entity.component.component_type.namespace # Example of component namespace is: "xblock.v1" - if component_namespace_folder not in folders_already_created: - self.create_folder(component_namespace_folder, zipf) - folders_already_created.add(component_namespace_folder) + self.create_folder(component_namespace_folder, zipf) component_type_folder = component_namespace_folder / entity.component.component_type.name # Example of component type is: "html" - if component_type_folder not in folders_already_created: - self.create_folder(component_type_folder, zipf) - folders_already_created.add(component_type_folder) + self.create_folder(component_type_folder, zipf) component_id_folder = component_type_folder / entity.component.local_key # entity.key # Example of component id is: "i-dont-like-the-sidebar-aa1645ade4a7" - if component_id_folder not in folders_already_created: - self.create_folder(component_id_folder, zipf) - folders_already_created.add(component_id_folder) + self.create_folder(component_id_folder, zipf) # Add the entity TOML file inside the component type folder as well component_entity_toml_path = component_type_folder / f"{entity.component.local_key}.toml" @@ -97,19 +100,35 @@ def create_zip(self, path: str) -> None: # Add component version folder into the component id folder component_version_folder = component_id_folder / "component_versions" - if component_version_folder not in folders_already_created: - self.create_folder(component_version_folder, zipf) - folders_already_created.add(component_version_folder) - - for entity_version in entity.component.versions.all(): - component_number_version_folder = component_version_folder / f"v{entity_version.version_num}" - # Create a folder for each version of the component. Example: "v1", "v2", etc. - if component_number_version_folder not in folders_already_created: - self.create_folder(component_number_version_folder, zipf) - folders_already_created.add(component_number_version_folder) - - # Add the static folder inside the component version folder - static_folder = component_number_version_folder / "static" - if static_folder not in folders_already_created: - self.create_folder(static_folder, zipf) - folders_already_created.add(static_folder) + self.create_folder(component_version_folder, zipf) + + # ------ COMPONENT VERSIONING ------------- + # Focusing on draft and published versions + + # Get the draft and published versions + current_draft: Optional[Draft] = getattr(entity, "draft", None) + current_published: Optional[Published] = getattr(entity, "published", None) + + draft_version: Optional[PublishableEntityVersion] = getattr(current_draft, "version", None) + published_version: Optional[PublishableEntityVersion] = getattr(current_published, "version", None) + + # Creating draft version folder + if draft_version: + # Create a folder for the draft version + draft_version_number = f"v{draft_version.version_num}" + draft_version_folder = component_version_folder / draft_version_number + self.create_folder(draft_version_folder, zipf) + + # Add static folder for the draft version + static_folder = draft_version_folder / "static" + self.create_folder(static_folder, zipf) + + if published_version and published_version != draft_version: + # Create a folder for the published version + published_version_number = f"v{published_version.version_num}" + published_version_folder = component_version_folder / published_version_number + self.create_folder(published_version_folder, zipf) + + # Add static folder for the published version + static_folder = published_version_folder / "static" + self.create_folder(static_folder, zipf) diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index 5e361847d..e31d5acf4 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -68,6 +68,14 @@ def setUpTestData(cls): published_at=cls.now, ) + api.create_component_version( + cls.published_component.pk, + version_num=cls.published_component.versioning.draft.version_num + 1, + title="My published problem draft v2", + created=cls.now, + created_by=cls.user.id, + ) + # Create a Draft component, one in each learning package cls.draft_component, _ = api.create_component_and_version( cls.learning_package.id, @@ -114,8 +122,8 @@ def check_zip_file_structure(self, zip_path: Path): "entities/xblock.v1/html/", "entities/xblock.v1/html/my_draft_example/", "entities/xblock.v1/html/my_draft_example/component_versions/", - "entities/xblock.v1/html/my_draft_example/component_versions/v1/", - "entities/xblock.v1/html/my_draft_example/component_versions/v1/static/", + "entities/xblock.v1/html/my_draft_example/component_versions/v2/", + "entities/xblock.v1/html/my_draft_example/component_versions/v2/static/", "entities/xblock.v1/problem/", "entities/xblock.v1/problem/my_published_example/", "entities/xblock.v1/problem/my_published_example/component_versions/", From 404259c4e951cc87391cab07b72d7991b967a62b Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Fri, 15 Aug 2025 13:07:07 -0600 Subject: [PATCH 2/7] feat: exclude component TOML files from the top-level directory --- .../apps/authoring/backup_restore/zipper.py | 8 ++- .../authoring/backup_restore/test_backup.py | 55 +++++++++---------- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index 3c2bb69b3..e1022eb37 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -67,9 +67,11 @@ def create_zip(self, path: str) -> None: # Create a TOML representation of the entity entity_toml_content: str = toml_publishable_entity(entity) - entity_toml_filename = f"{entity.key}.toml" - entity_toml_path = entities_folder / entity_toml_filename - zipf.writestr(str(entity_toml_path), entity_toml_content) + + if hasattr(entity, 'container'): + entity_toml_filename = f"{entity.key}.toml" + entity_toml_path = entities_folder / entity_toml_filename + zipf.writestr(str(entity_toml_path), entity_toml_content) if hasattr(entity, 'component'): # Create the component folder structure for the entity. The structure is as follows: diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index e31d5acf4..e6e930a3c 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -133,7 +133,10 @@ def check_zip_file_structure(self, zip_path: Path): # Add expected entity files for entity in self.all_components: - expected_files.append(f"entities/{entity.key}.toml") + if hasattr(entity, 'component'): + component_type_name = entity.component.component_type.name + file_path = f"entities/xblock.v1/{component_type_name}/{entity.component.local_key}.toml" + expected_files.append(file_path) # Check that all expected files are present for expected_file in expected_files: @@ -180,33 +183,29 @@ def test_lp_dump_command(self): # Check the content of the entity TOML files for entity in self.all_components: - current_draft_version = getattr(entity, "draft", None) - current_published_version = getattr(entity, "published", None) - expected_content = [ - '[entity]', - f'uuid = "{entity.uuid}"', - 'can_stand_alone = true', - '[entity.draft]', - f'version_num = {current_draft_version.version.version_num}', - '[entity.published]', - ] - if current_published_version: - expected_content.append(f'version_num = {current_published_version.version.version_num}') - else: - expected_content.append('# unpublished: no published_version_num') - - for entity_version in entity.versions.all(): - expected_content.append(f'title = "{entity_version.title}"') - expected_content.append(f'uuid = "{entity_version.uuid}"') - expected_content.append(f'version_num = {entity_version.version_num}') - expected_content.append('[version.container]') - expected_content.append('[version.container.unit]') - - self.check_toml_file( - zip_path, - Path(f"entities/{entity.key}.toml"), - expected_content - ) + if hasattr(entity, 'component'): + current_draft_version = getattr(entity, "draft", None) + current_published_version = getattr(entity, "published", None) + expected_content = [ + '[entity]', + f'uuid = "{entity.uuid}"', + 'can_stand_alone = true', + '[entity.draft]', + f'version_num = {current_draft_version.version.version_num}', + '[entity.published]', + ] + if current_published_version: + expected_content.append(f'version_num = {current_published_version.version.version_num}') + else: + expected_content.append('# unpublished: no published_version_num') + + component_type_name = entity.component.component_type.name + file_path = f"entities/xblock.v1/{component_type_name}/{entity.component.local_key}.toml" + self.check_toml_file( + zip_path, + Path(file_path), + expected_content + ) # Check the output message message = f'{lp_key} written to {file_name}' From d54a255bc55f909f07082623514f7113715adb6a Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 20 Aug 2025 13:23:29 -0600 Subject: [PATCH 3/7] feat: optimize get_publishable_entities query to prevent N+1 issue --- .../apps/authoring/backup_restore/zipper.py | 2 +- .../apps/authoring/publishing/api.py | 18 ++- .../authoring/backup_restore/test_backup.py | 2 +- .../apps/authoring/publishing/test_api.py | 130 +++++++++++++----- 4 files changed, 113 insertions(+), 39 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index e1022eb37..ea08aa097 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -62,7 +62,7 @@ def create_zip(self, path: str) -> None: self.create_folder(collections_folder, zipf) # Add each entity's TOML file - for entity in publishing_api.get_entities(self.learning_package.pk): + for entity in publishing_api.get_publishable_entities(self.learning_package.pk): # entity: PublishableEntity = entity # Type hint for clarity # Create a TOML representation of the entity diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 159b8aec0..b6e837b7e 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -58,9 +58,9 @@ "create_publishable_entity_version", "get_publishable_entity", "get_publishable_entity_by_key", + "get_publishable_entities", "get_last_publish", "get_all_drafts", - "get_entities", "get_entities_with_unpublished_changes", "get_entities_with_unpublished_deletes", "publish_all_drafts", @@ -262,11 +262,23 @@ def get_all_drafts(learning_package_id: int, /) -> QuerySet[Draft]: ) -def get_entities(learning_package_id: int, /) -> QuerySet[PublishableEntity]: +def get_publishable_entities(learning_package_id: int, /) -> QuerySet[PublishableEntity]: """ Get all entities in a learning package. """ - return PublishableEntity.objects.filter(learning_package_id=learning_package_id) + return ( + PublishableEntity.objects + .filter(learning_package_id=learning_package_id) + .select_related( + "draft", + "published", + "draft__version", + "published__version", + "component", + "container", + "component__component_type", + ) + ) def get_entities_with_unpublished_changes( diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index e6e930a3c..b0e304513 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -94,7 +94,7 @@ def setUpTestData(cls): created_by=cls.user.id, ) - components = api.get_entities(cls.learning_package) + components = api.get_publishable_entities(cls.learning_package) cls.all_components = components def check_toml_file(self, zip_path: Path, zip_member_name: Path, content_to_check: list): diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 94c328f22..8e505eecb 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -7,8 +7,10 @@ from uuid import UUID import pytest +from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError +from openedx_learning.apps.authoring.components import api as components_api from openedx_learning.apps.authoring.publishing import api as publishing_api from openedx_learning.apps.authoring.publishing.models import ( Container, @@ -22,6 +24,8 @@ ) from openedx_learning.lib.test_utils import TestCase +User = get_user_model() + class LearningPackageTestCase(TestCase): """ @@ -1083,57 +1087,115 @@ class EntitiesQueryTestCase(TestCase): """ now: datetime learning_package_1: LearningPackage - learning_package_2: LearningPackage @classmethod def setUpTestData(cls) -> None: + """ + Initialize our content data + """ + cls.now = datetime(2025, 8, 4, 12, 00, 00, tzinfo=timezone.utc) cls.learning_package_1 = publishing_api.create_learning_package( "my_package_key_1", "Entities Testing LearningPackage 🔥 1", created=cls.now, ) - cls.learning_package_2 = publishing_api.create_learning_package( - "my_package_key_2", - "Entities Testing LearningPackage 🔥 2", + + # Create a user for the test + user = User.objects.create( + username="user", + email="user@example.com", + ) + + # Create component namespace + xblock_v1_namespace = "xblock.v1" + + # Create component types + html_type = components_api.get_or_create_component_type(xblock_v1_namespace, "html") + problem_type = components_api.get_or_create_component_type(xblock_v1_namespace, "problem") + + # Make and publish one Component + published_component, _ = components_api.create_component_and_version( + cls.learning_package_1.id, + problem_type, + local_key="my_published_example", + title="My published problem", + created=cls.now, + created_by=user.id, + ) + + # Create a Content entry for the published Component + publishing_api.publish_all_drafts( + cls.learning_package_1.id, + message="Publish from CollectionTestCase.setUpTestData", + published_at=cls.now, + ) + + # Create a Draft component for the published_component + components_api.create_component_version( + published_component.pk, + version_num=published_component.versioning.draft.version_num + 1, + title="My published problem draft v2", + created=cls.now, + created_by=user.id, + ) + + # Create a Draft component + draft_component, _ = components_api.create_component_and_version( + cls.learning_package_1.id, + html_type, + local_key="my_draft_example", + title="My draft html", + created=cls.now, + created_by=user.id, + ) + + components_api.create_component_version( + draft_component.pk, + version_num=draft_component.versioning.draft.version_num + 1, + title="My draft html v2", created=cls.now, + created_by=user.id, ) - def test_get_entities(self) -> None: + def test_get_publishable_entities(self) -> None: """ Test that get_entities returns all entities for a learning package. """ - with publishing_api.bulk_draft_changes_for(self.learning_package_1.id): - entity = publishing_api.create_publishable_entity( - self.learning_package_1.id, - "my_entity", - created=self.now, - created_by=None, - ) - publishing_api.create_publishable_entity_version( - entity.id, - version_num=1, - title="An Entity 🌴", - created=self.now, - created_by=None, - ) - entity2 = publishing_api.create_publishable_entity( - self.learning_package_1.id, - "my_entity2", - created=self.now, - created_by=None, - ) - publishing_api.create_publishable_entity_version( - entity2.id, - version_num=1, - title="An Entity 🌴 2", - created=self.now, - created_by=None, - ) - entities = publishing_api.get_entities(self.learning_package_1.id) + entities = publishing_api.get_publishable_entities(self.learning_package_1.id) assert entities.count() == 2 for entity in entities: assert isinstance(entity, PublishableEntity) assert entity.learning_package_id == self.learning_package_1.id assert entity.created == self.now - assert entity.created_by is None + + def test_get_publishable_entities_n_plus_problem(self) -> None: + """ + Check get_publishable_entities if N+1 query problem exists when accessing related entities. + """ + entities = publishing_api.get_publishable_entities(self.learning_package_1.id) + + # assert that only 1 query is made even when accessing related entities + with self.assertNumQueries(1): + # Related entities to review: + # - draft.version + # - published.version + # - container + # - component + # - component.component_type + + for e in entities: + draft = getattr(e, 'draft', None) + published = getattr(e, 'published', None) + + _ = getattr(draft, 'version', None) + _ = getattr(published, 'version', None) + + assert hasattr(e, 'container') or hasattr(e, 'component') + + if hasattr(e, 'container'): + _ = e.container + + if hasattr(e, 'component'): + _ = e.component + _ = e.component.component_type From a166d99d8c97684b5981652dacbd834babc09d88 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 20 Aug 2025 16:24:59 -0600 Subject: [PATCH 4/7] test: make lp_dump test more explicit by avoiding internal logic --- .../apps/authoring/backup_restore/zipper.py | 26 +++---- .../authoring/backup_restore/test_backup.py | 76 ++++++++----------- 2 files changed, 40 insertions(+), 62 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index ea08aa097..db46608c1 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -114,23 +114,17 @@ def create_zip(self, path: str) -> None: draft_version: Optional[PublishableEntityVersion] = getattr(current_draft, "version", None) published_version: Optional[PublishableEntityVersion] = getattr(current_published, "version", None) - # Creating draft version folder - if draft_version: - # Create a folder for the draft version - draft_version_number = f"v{draft_version.version_num}" - draft_version_folder = component_version_folder / draft_version_number - self.create_folder(draft_version_folder, zipf) - - # Add static folder for the draft version - static_folder = draft_version_folder / "static" - self.create_folder(static_folder, zipf) + versions_to_write = [draft_version] if draft_version else [] if published_version and published_version != draft_version: - # Create a folder for the published version - published_version_number = f"v{published_version.version_num}" - published_version_folder = component_version_folder / published_version_number - self.create_folder(published_version_folder, zipf) + versions_to_write.append(published_version) + + for version in versions_to_write: + # Create a folder for the version + version_number = f"v{version.version_num}" + version_folder = component_version_folder / version_number + self.create_folder(version_folder, zipf) - # Add static folder for the published version - static_folder = published_version_folder / "static" + # Add static folder for the version + static_folder = version_folder / "static" self.create_folder(static_folder, zipf) diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index b0e304513..508f6646b 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -111,46 +111,31 @@ def check_zip_file_structure(self, zip_path: Path): """ Check that the zip file has the expected structure. """ - with zipfile.ZipFile(zip_path, "r") as zip_file: # Check that the zip file contains the expected files - expected_files = [ + expected_paths = [ "package.toml", "entities/", "collections/", "entities/xblock.v1/", "entities/xblock.v1/html/", + "entities/xblock.v1/html/my_draft_example.toml", "entities/xblock.v1/html/my_draft_example/", "entities/xblock.v1/html/my_draft_example/component_versions/", "entities/xblock.v1/html/my_draft_example/component_versions/v2/", "entities/xblock.v1/html/my_draft_example/component_versions/v2/static/", "entities/xblock.v1/problem/", "entities/xblock.v1/problem/my_published_example/", + "entities/xblock.v1/problem/my_published_example.toml", "entities/xblock.v1/problem/my_published_example/component_versions/", "entities/xblock.v1/problem/my_published_example/component_versions/v1/", "entities/xblock.v1/problem/my_published_example/component_versions/v1/static/", ] - # Add expected entity files - for entity in self.all_components: - if hasattr(entity, 'component'): - component_type_name = entity.component.component_type.name - file_path = f"entities/xblock.v1/{component_type_name}/{entity.component.local_key}.toml" - expected_files.append(file_path) - - # Check that all expected files are present - for expected_file in expected_files: - self.assertIn(expected_file, zip_file.namelist()) - - def check_content_in_zip(self, zip_path: Path, expected_file_path: Path, zip_member_name: str): - """ - Compare a file inside the zip with an expected file. - """ - with zipfile.ZipFile(zip_path, "r") as zip_file: - with zip_file.open(zip_member_name) as content_file: - actual_content = content_file.read().decode("utf-8") - expected_content = expected_file_path.read_text() - self.assertEqual(actual_content, expected_content) + # Check that all expected paths are present + zip_name_list = zip_file.namelist() + for expected_path in expected_paths: + self.assertIn(expected_path, zip_name_list) def test_lp_dump_command(self): lp_key = self.learning_package.key @@ -182,30 +167,29 @@ def test_lp_dump_command(self): ) # Check the content of the entity TOML files - for entity in self.all_components: - if hasattr(entity, 'component'): - current_draft_version = getattr(entity, "draft", None) - current_published_version = getattr(entity, "published", None) - expected_content = [ - '[entity]', - f'uuid = "{entity.uuid}"', - 'can_stand_alone = true', - '[entity.draft]', - f'version_num = {current_draft_version.version.version_num}', - '[entity.published]', - ] - if current_published_version: - expected_content.append(f'version_num = {current_published_version.version.version_num}') - else: - expected_content.append('# unpublished: no published_version_num') - - component_type_name = entity.component.component_type.name - file_path = f"entities/xblock.v1/{component_type_name}/{entity.component.local_key}.toml" - self.check_toml_file( - zip_path, - Path(file_path), - expected_content - ) + expected_files = { + "entities/xblock.v1/problem/my_published_example.toml": [ + '[entity]', + f'uuid = "{self.published_component.uuid}"', + 'can_stand_alone = true', + '[entity.draft]', + 'version_num = 2', + '[entity.published]', + 'version_num = 1', + ], + "entities/xblock.v1/html/my_draft_example.toml": [ + '[entity]', + f'uuid = "{self.draft_component.uuid}"', + 'can_stand_alone = true', + '[entity.draft]', + 'version_num = 2', + '[entity.published]', + '# unpublished: no published_version_num', + ], + } + + for file_path, expected_content in expected_files.items(): + self.check_toml_file(zip_path, Path(file_path), expected_content) # Check the output message message = f'{lp_key} written to {file_name}' From e5164adea0027f860fa5306c57da1c485945559e Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 20 Aug 2025 21:43:20 -0600 Subject: [PATCH 5/7] feat: support preloading in get_published_version and get_draft_version --- .../apps/authoring/backup_restore/toml.py | 9 +++++---- .../apps/authoring/backup_restore/zipper.py | 14 +++----------- .../apps/authoring/publishing/api.py | 16 +++++++++++++--- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/toml.py b/openedx_learning/apps/authoring/backup_restore/toml.py index 0551201ba..72a8aed97 100644 --- a/openedx_learning/apps/authoring/backup_restore/toml.py +++ b/openedx_learning/apps/authoring/backup_restore/toml.py @@ -6,6 +6,7 @@ import tomlkit +from openedx_learning.apps.authoring.publishing import api as publishing_api from openedx_learning.apps.authoring.publishing.models import PublishableEntity, PublishableEntityVersion from openedx_learning.apps.authoring.publishing.models.learning_package import LearningPackage @@ -27,8 +28,8 @@ def toml_learning_package(learning_package: LearningPackage) -> str: def toml_publishable_entity(entity: PublishableEntity) -> str: """Create a TOML representation of a publishable entity.""" - current_draft_version = getattr(entity, "draft", None) - current_published_version = getattr(entity, "published", None) + current_draft_version = publishing_api.get_draft_version(entity) + current_published_version = publishing_api.get_published_version(entity) doc = tomlkit.document() entity_table = tomlkit.table() @@ -37,12 +38,12 @@ def toml_publishable_entity(entity: PublishableEntity) -> str: if current_draft_version: draft_table = tomlkit.table() - draft_table.add("version_num", current_draft_version.version.version_num) + draft_table.add("version_num", current_draft_version.version_num) entity_table.add("draft", draft_table) published_table = tomlkit.table() if current_published_version: - published_table.add("version_num", current_published_version.version.version_num) + published_table.add("version_num", current_published_version.version_num) else: published_table.add(tomlkit.comment("unpublished: no published_version_num")) entity_table.add("published", published_table) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index db46608c1..c64139de3 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -8,12 +8,7 @@ from openedx_learning.apps.authoring.backup_restore.toml import toml_learning_package, toml_publishable_entity from openedx_learning.apps.authoring.publishing import api as publishing_api -from openedx_learning.apps.authoring.publishing.models import ( - Draft, - LearningPackage, - PublishableEntityVersion, - Published, -) +from openedx_learning.apps.authoring.publishing.models import LearningPackage, PublishableEntityVersion TOML_PACKAGE_NAME = "package.toml" @@ -108,11 +103,8 @@ def create_zip(self, path: str) -> None: # Focusing on draft and published versions # Get the draft and published versions - current_draft: Optional[Draft] = getattr(entity, "draft", None) - current_published: Optional[Published] = getattr(entity, "published", None) - - draft_version: Optional[PublishableEntityVersion] = getattr(current_draft, "version", None) - published_version: Optional[PublishableEntityVersion] = getattr(current_published, "version", None) + draft_version: Optional[PublishableEntityVersion] = publishing_api.get_draft_version(entity) + published_version: Optional[PublishableEntityVersion] = publishing_api.get_published_version(entity) versions_to_write = [draft_version] if draft_version else [] diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index b6e837b7e..e7dcaf704 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -10,7 +10,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from enum import Enum -from typing import ContextManager, TypeVar +from typing import ContextManager, Optional, TypeVar from django.core.exceptions import ObjectDoesNotExist, ValidationError from django.db.models import F, Q, QuerySet @@ -437,12 +437,17 @@ def publish_from_drafts( return publish_log -def get_draft_version(publishable_entity_id: int, /) -> PublishableEntityVersion | None: +def get_draft_version(publishable_entity_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: """ Return current draft PublishableEntityVersion for this PublishableEntity. This function will return None if there is no current draft. """ + if isinstance(publishable_entity_id, PublishableEntity): + # The following code retrieves the draft version for a given PublishableEntity. + # Useful for preloading the draft version in certain contexts. + draft: Optional[Draft] = getattr(publishable_entity_id, "draft", None) + return getattr(draft, "version", None) try: draft = Draft.objects.select_related("version").get( entity_id=publishable_entity_id @@ -457,12 +462,17 @@ def get_draft_version(publishable_entity_id: int, /) -> PublishableEntityVersion return draft.version -def get_published_version(publishable_entity_id: int, /) -> PublishableEntityVersion | None: +def get_published_version(publishable_entity_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: """ Return current published PublishableEntityVersion for this PublishableEntity. This function will return None if there is no current published version. """ + # The following code retrieves the published version for a given PublishableEntity. + # Useful for preloading the published version in certain contexts. + if isinstance(publishable_entity_id, PublishableEntity): + published: Optional[Published] = getattr(publishable_entity_id, "published", None) + return getattr(published, "version", None) try: published = Published.objects.select_related("version").get( entity_id=publishable_entity_id From 42e8fed6b7aefba521fadd4f1576dac76daf1882 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 21 Aug 2025 11:55:00 -0600 Subject: [PATCH 6/7] refactor: remove high-level references in get_publishable_entities and misc improvements --- .../apps/authoring/backup_restore/zipper.py | 13 ++- .../apps/authoring/publishing/api.py | 37 ++++--- .../apps/authoring/publishing/test_api.py | 96 ++++++------------- 3 files changed, 56 insertions(+), 90 deletions(-) diff --git a/openedx_learning/apps/authoring/backup_restore/zipper.py b/openedx_learning/apps/authoring/backup_restore/zipper.py index c64139de3..5386e9aff 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -6,9 +6,15 @@ from pathlib import Path from typing import Optional +from django.db.models import QuerySet + from openedx_learning.apps.authoring.backup_restore.toml import toml_learning_package, toml_publishable_entity from openedx_learning.apps.authoring.publishing import api as publishing_api -from openedx_learning.apps.authoring.publishing.models import LearningPackage, PublishableEntityVersion +from openedx_learning.apps.authoring.publishing.models import ( + LearningPackage, + PublishableEntity, + PublishableEntityVersion, +) TOML_PACKAGE_NAME = "package.toml" @@ -43,6 +49,7 @@ def create_zip(self, path: str) -> None: Exception: If the learning package cannot be found or if the zip creation fails. """ package_toml_content: str = toml_learning_package(self.learning_package) + lp_id = self.learning_package.pk with zipfile.ZipFile(path, "w", compression=zipfile.ZIP_DEFLATED) as zipf: # Add the package.toml string @@ -57,7 +64,9 @@ def create_zip(self, path: str) -> None: self.create_folder(collections_folder, zipf) # Add each entity's TOML file - for entity in publishing_api.get_publishable_entities(self.learning_package.pk): + publishable_entities: QuerySet[PublishableEntity] = publishing_api.get_publishable_entities(lp_id) + publishable_entities = publishable_entities.select_related("container", "component__component_type") + for entity in publishable_entities: # entity: PublishableEntity = entity # Type hint for clarity # Create a TOML representation of the entity diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index e7dcaf704..68183c64f 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -270,13 +270,8 @@ def get_publishable_entities(learning_package_id: int, /) -> QuerySet[Publishabl PublishableEntity.objects .filter(learning_package_id=learning_package_id) .select_related( - "draft", - "published", "draft__version", "published__version", - "component", - "container", - "component__component_type", ) ) @@ -437,20 +432,22 @@ def publish_from_drafts( return publish_log -def get_draft_version(publishable_entity_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: +def get_draft_version(publishable_entity_or_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: """ Return current draft PublishableEntityVersion for this PublishableEntity. This function will return None if there is no current draft. """ - if isinstance(publishable_entity_id, PublishableEntity): - # The following code retrieves the draft version for a given PublishableEntity. - # Useful for preloading the draft version in certain contexts. - draft: Optional[Draft] = getattr(publishable_entity_id, "draft", None) - return getattr(draft, "version", None) + if isinstance(publishable_entity_or_id, PublishableEntity): + # Fetches the draft version for a given PublishableEntity. + # Gracefully handles cases where no draft is present. + draft: Optional[Draft] = getattr(publishable_entity_or_id, "draft", None) + if draft is None: + return None + return draft.version try: draft = Draft.objects.select_related("version").get( - entity_id=publishable_entity_id + entity_id=publishable_entity_or_id ) except Draft.DoesNotExist: # No draft was ever created. @@ -462,20 +459,22 @@ def get_draft_version(publishable_entity_id: PublishableEntity | int, /) -> Publ return draft.version -def get_published_version(publishable_entity_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: +def get_published_version(publishable_entity_or_id: PublishableEntity | int, /) -> PublishableEntityVersion | None: """ Return current published PublishableEntityVersion for this PublishableEntity. This function will return None if there is no current published version. """ - # The following code retrieves the published version for a given PublishableEntity. - # Useful for preloading the published version in certain contexts. - if isinstance(publishable_entity_id, PublishableEntity): - published: Optional[Published] = getattr(publishable_entity_id, "published", None) - return getattr(published, "version", None) + if isinstance(publishable_entity_or_id, PublishableEntity): + # Fetches the published version for a given PublishableEntity. + # Gracefully handles cases where no published version is present. + published: Optional[Published] = getattr(publishable_entity_or_id, "published", None) + if published is None: + return None + return published.version try: published = Published.objects.select_related("version").get( - entity_id=publishable_entity_id + entity_id=publishable_entity_or_id ) except Published.DoesNotExist: return None diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 8e505eecb..a7e664280 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -10,7 +10,6 @@ from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError -from openedx_learning.apps.authoring.components import api as components_api from openedx_learning.apps.authoring.publishing import api as publishing_api from openedx_learning.apps.authoring.publishing.models import ( Container, @@ -1101,62 +1100,33 @@ def setUpTestData(cls) -> None: created=cls.now, ) - # Create a user for the test - user = User.objects.create( - username="user", - email="user@example.com", - ) - - # Create component namespace - xblock_v1_namespace = "xblock.v1" - - # Create component types - html_type = components_api.get_or_create_component_type(xblock_v1_namespace, "html") - problem_type = components_api.get_or_create_component_type(xblock_v1_namespace, "problem") - - # Make and publish one Component - published_component, _ = components_api.create_component_and_version( - cls.learning_package_1.id, - problem_type, - local_key="my_published_example", - title="My published problem", - created=cls.now, - created_by=user.id, - ) - - # Create a Content entry for the published Component - publishing_api.publish_all_drafts( - cls.learning_package_1.id, - message="Publish from CollectionTestCase.setUpTestData", - published_at=cls.now, - ) - - # Create a Draft component for the published_component - components_api.create_component_version( - published_component.pk, - version_num=published_component.versioning.draft.version_num + 1, - title="My published problem draft v2", - created=cls.now, - created_by=user.id, - ) - - # Create a Draft component - draft_component, _ = components_api.create_component_and_version( - cls.learning_package_1.id, - html_type, - local_key="my_draft_example", - title="My draft html", - created=cls.now, - created_by=user.id, - ) - - components_api.create_component_version( - draft_component.pk, - version_num=draft_component.versioning.draft.version_num + 1, - title="My draft html v2", - created=cls.now, - created_by=user.id, - ) + with publishing_api.bulk_draft_changes_for(cls.learning_package_1.id): + entity = publishing_api.create_publishable_entity( + cls.learning_package_1.id, + "my_entity", + created=cls.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity.id, + version_num=1, + title="An Entity 🌴", + created=cls.now, + created_by=None, + ) + entity2 = publishing_api.create_publishable_entity( + cls.learning_package_1.id, + "my_entity2", + created=cls.now, + created_by=None, + ) + publishing_api.create_publishable_entity_version( + entity2.id, + version_num=1, + title="An Entity 🌴 2", + created=cls.now, + created_by=None, + ) def test_get_publishable_entities(self) -> None: """ @@ -1180,9 +1150,6 @@ def test_get_publishable_entities_n_plus_problem(self) -> None: # Related entities to review: # - draft.version # - published.version - # - container - # - component - # - component.component_type for e in entities: draft = getattr(e, 'draft', None) @@ -1190,12 +1157,3 @@ def test_get_publishable_entities_n_plus_problem(self) -> None: _ = getattr(draft, 'version', None) _ = getattr(published, 'version', None) - - assert hasattr(e, 'container') or hasattr(e, 'component') - - if hasattr(e, 'container'): - _ = e.container - - if hasattr(e, 'component'): - _ = e.component - _ = e.component.component_type From 768ad8afc81310a0d38326aee441de8d1ee92e7d Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Thu, 21 Aug 2025 13:35:30 -0600 Subject: [PATCH 7/7] refactor: incorporate minor suggestions --- .../apps/authoring/backup_restore/test_backup.py | 10 ++++------ .../apps/authoring/publishing/test_api.py | 8 +++++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index 508f6646b..a40bc3c99 100644 --- a/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py +++ b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py @@ -68,12 +68,11 @@ def setUpTestData(cls): published_at=cls.now, ) - api.create_component_version( + api.create_next_component_version( cls.published_component.pk, - version_num=cls.published_component.versioning.draft.version_num + 1, title="My published problem draft v2", + content_to_replace={}, created=cls.now, - created_by=cls.user.id, ) # Create a Draft component, one in each learning package @@ -86,12 +85,11 @@ def setUpTestData(cls): created_by=cls.user.id, ) - api.create_component_version( + api.create_next_component_version( cls.draft_component.pk, - version_num=cls.draft_component.versioning.draft.version_num + 1, title="My draft html v2", + content_to_replace={}, created=cls.now, - created_by=cls.user.id, ) components = api.get_publishable_entities(cls.learning_package) diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index a7e664280..0ab4d33c8 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -1127,6 +1127,7 @@ def setUpTestData(cls) -> None: created=cls.now, created_by=None, ) + publishing_api.publish_all_drafts(cls.learning_package_1.id) def test_get_publishable_entities(self) -> None: """ @@ -1152,8 +1153,9 @@ def test_get_publishable_entities_n_plus_problem(self) -> None: # - published.version for e in entities: + # Instead of just checking the version number, we verify the related query count. + # If an N+1 issue exists, accessing versions or other related fields would trigger more than one query. draft = getattr(e, 'draft', None) published = getattr(e, 'published', None) - - _ = getattr(draft, 'version', None) - _ = getattr(published, 'version', None) + assert draft and draft.version.version_num == 1 + assert published and published.version.version_num == 1