diff --git a/openedx_learning/apps/authoring/backup_restore/toml.py b/openedx_learning/apps/authoring/backup_restore/toml.py index 0551201b..72a8aed9 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 c71409fd..5386e9af 100644 --- a/openedx_learning/apps/authoring/backup_restore/zipper.py +++ b/openedx_learning/apps/authoring/backup_restore/zipper.py @@ -4,10 +4,17 @@ """ import zipfile 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.learning_package import LearningPackage +from openedx_learning.apps.authoring.publishing.models import ( + LearningPackage, + PublishableEntity, + PublishableEntityVersion, +) TOML_PACKAGE_NAME = "package.toml" @@ -19,15 +26,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 +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) - folders_already_created = set() + lp_id = self.learning_package.pk with zipfile.ZipFile(path, "w", compression=zipfile.ZIP_DEFLATED) as zipf: # Add the package.toml string @@ -53,14 +64,18 @@ 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): + 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 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: @@ -75,21 +90,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 +106,26 @@ 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 + 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 [] + + if published_version and published_version != draft_version: + 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 version + static_folder = version_folder / "static" + self.create_folder(static_folder, zipf) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 159b8aec..68183c64 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 @@ -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,18 @@ 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__version", + "published__version", + ) + ) def get_entities_with_unpublished_changes( @@ -425,15 +432,22 @@ def publish_from_drafts( return publish_log -def get_draft_version(publishable_entity_id: 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_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. @@ -445,15 +459,22 @@ 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_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. """ + 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/backup_restore/test_backup.py b/tests/openedx_learning/apps/authoring/backup_restore/test_backup.py index 5e361847..a40bc3c9 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,13 @@ def setUpTestData(cls): published_at=cls.now, ) + api.create_next_component_version( + cls.published_component.pk, + title="My published problem draft v2", + content_to_replace={}, + created=cls.now, + ) + # Create a Draft component, one in each learning package cls.draft_component, _ = api.create_component_and_version( cls.learning_package.id, @@ -78,15 +85,14 @@ 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_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): @@ -103,43 +109,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/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.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: - expected_files.append(f"entities/{entity.key}.toml") - - # 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 @@ -171,34 +165,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 = [ + expected_files = { + "entities/xblock.v1/problem/my_published_example.toml": [ '[entity]', - f'uuid = "{entity.uuid}"', + f'uuid = "{self.published_component.uuid}"', 'can_stand_alone = true', '[entity.draft]', - f'version_num = {current_draft_version.version.version_num}', + 'version_num = 2', '[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 - ) + '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}' diff --git a/tests/openedx_learning/apps/authoring/publishing/test_api.py b/tests/openedx_learning/apps/authoring/publishing/test_api.py index 94c328f2..0ab4d33c 100644 --- a/tests/openedx_learning/apps/authoring/publishing/test_api.py +++ b/tests/openedx_learning/apps/authoring/publishing/test_api.py @@ -7,6 +7,7 @@ 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.publishing import api as publishing_api @@ -22,6 +23,8 @@ ) from openedx_learning.lib.test_utils import TestCase +User = get_user_model() + class LearningPackageTestCase(TestCase): """ @@ -1083,57 +1086,76 @@ 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", - created=cls.now, - ) - def test_get_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): + with publishing_api.bulk_draft_changes_for(cls.learning_package_1.id): entity = publishing_api.create_publishable_entity( - self.learning_package_1.id, + cls.learning_package_1.id, "my_entity", - created=self.now, + created=cls.now, created_by=None, ) publishing_api.create_publishable_entity_version( entity.id, version_num=1, title="An Entity 🌴", - created=self.now, + created=cls.now, created_by=None, ) entity2 = publishing_api.create_publishable_entity( - self.learning_package_1.id, + cls.learning_package_1.id, "my_entity2", - created=self.now, + created=cls.now, created_by=None, ) publishing_api.create_publishable_entity_version( entity2.id, version_num=1, title="An Entity 🌴 2", - created=self.now, + created=cls.now, created_by=None, ) - entities = publishing_api.get_entities(self.learning_package_1.id) + publishing_api.publish_all_drafts(cls.learning_package_1.id) + + def test_get_publishable_entities(self) -> None: + """ + Test that get_entities returns all entities for a learning package. + """ + 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 + + 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) + assert draft and draft.version.version_num == 1 + assert published and published.version.version_num == 1