From 755445c71206545bf5c1b9a1447e09df9a5ec61f Mon Sep 17 00:00:00 2001 From: Andrii Date: Wed, 9 Apr 2025 18:49:38 +0300 Subject: [PATCH 1/7] feat: [AXM-1879] implement contentlibrary subsections --- openedx_learning/api/authoring.py | 1 + openedx_learning/api/authoring_models.py | 1 + .../apps/authoring/publishing/api.py | 6 +- .../apps/authoring/subsections/__init__.py | 0 .../apps/authoring/subsections/api.py | 307 +++++ .../apps/authoring/subsections/apps.py | 25 + .../subsections/migrations/0001_initial.py | 36 + .../subsections/migrations/__init__.py | 0 .../apps/authoring/subsections/models.py | 50 + projects/dev.py | 1 + test_settings.py | 1 + .../apps/authoring/subsections/__init__.py | 0 .../apps/authoring/subsections/test_api.py | 1076 +++++++++++++++++ 13 files changed, 1503 insertions(+), 1 deletion(-) create mode 100644 openedx_learning/apps/authoring/subsections/__init__.py create mode 100644 openedx_learning/apps/authoring/subsections/api.py create mode 100644 openedx_learning/apps/authoring/subsections/apps.py create mode 100644 openedx_learning/apps/authoring/subsections/migrations/0001_initial.py create mode 100644 openedx_learning/apps/authoring/subsections/migrations/__init__.py create mode 100644 openedx_learning/apps/authoring/subsections/models.py create mode 100644 tests/openedx_learning/apps/authoring/subsections/__init__.py create mode 100644 tests/openedx_learning/apps/authoring/subsections/test_api.py diff --git a/openedx_learning/api/authoring.py b/openedx_learning/api/authoring.py index c06104878..900713ecc 100644 --- a/openedx_learning/api/authoring.py +++ b/openedx_learning/api/authoring.py @@ -13,6 +13,7 @@ from ..apps.authoring.components.api import * from ..apps.authoring.contents.api import * from ..apps.authoring.publishing.api import * +from ..apps.authoring.subsections.api import * from ..apps.authoring.units.api import * # This was renamed after the authoring API refactoring pushed this and other diff --git a/openedx_learning/api/authoring_models.py b/openedx_learning/api/authoring_models.py index 2413fc2b9..f7fd08de7 100644 --- a/openedx_learning/api/authoring_models.py +++ b/openedx_learning/api/authoring_models.py @@ -11,4 +11,5 @@ from ..apps.authoring.components.models import * from ..apps.authoring.contents.models import * from ..apps.authoring.publishing.models import * +from ..apps.authoring.subsections.models import * from ..apps.authoring.units.models import * diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 4e1af6414..9285c1048 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -1322,7 +1322,11 @@ def contains_unpublished_changes(container_id: int) -> bool: return True # We only care about children that are un-pinned, since published changes to pinned children don't matter - entity_list = container.versioning.draft.entity_list + entity_list = getattr(container.versioning.draft, "entity_list", None) + # ?FOR REVIEW: Is this correct? + if entity_list is None: + # This container has not been published yet, or has been deleted. + return False # This is a naive and inefficient implementation but should be correct. # TODO: Once we have expanded the containers system to support multiple levels (not just Units and Components but diff --git a/openedx_learning/apps/authoring/subsections/__init__.py b/openedx_learning/apps/authoring/subsections/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/openedx_learning/apps/authoring/subsections/api.py b/openedx_learning/apps/authoring/subsections/api.py new file mode 100644 index 000000000..e335b4b48 --- /dev/null +++ b/openedx_learning/apps/authoring/subsections/api.py @@ -0,0 +1,307 @@ +"""Subsections API. + +This module provides functions to manage subsections. +""" +from dataclasses import dataclass +from datetime import datetime + +from django.db.transaction import atomic + +from openedx_learning.apps.authoring.units.models import Unit, UnitVersion + +from ..publishing import api as publishing_api +from .models import Subsection, SubsectionVersion + +# 🛑 UNSTABLE: All APIs related to containers are unstable until we've figured +# out our approach to dynamic content (randomized, A/B tests, etc.) +__all__ = [ + "create_subsection", + "create_subsection_version", + "create_next_subsection_version", + "create_subsection_and_version", + "get_subsection", + "get_subsection_version", + "get_latest_subsection_version", + "SubsectionListEntry", + "get_units_in_subsection", + "get_units_in_subsection", + "get_units_in_published_subsection_as_of", +] + + +def create_subsection( + learning_package_id: int, + key: str, + created: datetime, + created_by: int | None, + *, + can_stand_alone: bool = True, +) -> Subsection: + """ + [ 🛑 UNSTABLE ] Create a new subsection. + + Args: + learning_package_id: The learning package ID. + key: The key. + created: The creation date. + created_by: The user who created the subsection. + can_stand_alone: Set to False when created as part of containers + """ + return publishing_api.create_container( + learning_package_id, + key, + created, + created_by, + can_stand_alone=can_stand_alone, + container_cls=Subsection, + ) + + +def create_subsection_version( + subsection: Subsection, + version_num: int, + *, + title: str, + publishable_entities_pks: list[int], + entity_version_pks: list[int | None], + created: datetime, + created_by: int | None = None, +) -> SubsectionVersion: + """ + [ 🛑 UNSTABLE ] Create a new subsection version. + + This is a very low-level API, likely only needed for import/export. In + general, you will use `create_subsection_and_version()` and + `create_next_subsection_version()` instead. + + Args: + subsection_pk: The subsection ID. + version_num: The version number. + title: The title. + publishable_entities_pk: The publishable entities. + entity: The entity. + created: The creation date. + created_by: The user who created the subsection. + """ + return publishing_api.create_container_version( + subsection.pk, + version_num, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + container_version_cls=SubsectionVersion, + ) + + +def _pub_entities_for_units( + units: list[Unit | UnitVersion] | None, +) -> tuple[list[int], list[int | None]] | tuple[None, None]: + """ + Helper method: given a list of Unit | UnitVersion, return the + lists of publishable_entities_pks and entity_version_pks needed for the + base container APIs. + + UnitVersion is passed when we want to pin a specific version, otherwise + Unit is used for unpinned. + """ + if units is None: + # When these are None, that means don't change the entities in the list. + return None, None + for u in units: + if not isinstance(u, (Unit, UnitVersion)): + raise TypeError("Subsection units must be either Unit or UnitVersion.") + publishable_entities_pks = [ + (u.publishable_entity_id if isinstance(u, Unit) else u.unit.publishable_entity_id) + for u in units + ] + entity_version_pks = [ + (uv.pk if isinstance(uv, UnitVersion) else None) + for uv in units + ] + return publishable_entities_pks, entity_version_pks + + +def create_next_subsection_version( + subsection: Subsection, + *, + title: str | None = None, + units: list[Unit | UnitVersion] | None = None, + created: datetime, + created_by: int | None = None, + entities_action: publishing_api.ChildrenEntitiesAction = publishing_api.ChildrenEntitiesAction.REPLACE, +) -> SubsectionVersion: + """ + [ 🛑 UNSTABLE ] Create the next subsection version. + + Args: + subsection_pk: The subsection ID. + title: The title. Leave as None to keep the current title. + units: The units, as a list of Units (unpinned) and/or UnitVersions (pinned). Passing None + will leave the existing units unchanged. + created: The creation date. + created_by: The user who created the subsection. + """ + publishable_entities_pks, entity_version_pks = _pub_entities_for_units(units) + subsection_version = publishing_api.create_next_container_version( + subsection.pk, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + container_version_cls=SubsectionVersion, + entities_action=entities_action, + ) + return subsection_version + + +def create_subsection_and_version( + learning_package_id: int, + key: str, + *, + title: str, + units: list[Unit | UnitVersion] | None = None, + created: datetime, + created_by: int | None = None, + can_stand_alone: bool = True, +) -> tuple[Subsection, SubsectionVersion]: + """ + [ 🛑 UNSTABLE ] Create a new subsection and its version. + + Args: + learning_package_id: The learning package ID. + key: The key. + created: The creation date. + created_by: The user who created the subsection. + can_stand_alone: Set to False when created as part of containers + """ + publishable_entities_pks, entity_version_pks = _pub_entities_for_units(units) + with atomic(): + subsection = create_subsection( + learning_package_id, + key, + created, + created_by, + can_stand_alone=can_stand_alone, + ) + subsection_version = create_subsection_version( + subsection, + 1, + title=title, + publishable_entities_pks=publishable_entities_pks or [], + entity_version_pks=entity_version_pks or [], + created=created, + created_by=created_by, + ) + return subsection, subsection_version + + +def get_subsection(subsection_pk: int) -> Subsection: + """ + [ 🛑 UNSTABLE ] Get a subsection. + + Args: + subsection_pk: The subsection ID. + """ + return Subsection.objects.get(pk=subsection_pk) + + +def get_subsection_version(subsection_version_pk: int) -> SubsectionVersion: + """ + [ 🛑 UNSTABLE ] Get a subsection version. + + Args: + subsection_version_pk: The subsection version ID. + """ + return SubsectionVersion.objects.get(pk=subsection_version_pk) + + +def get_latest_subsection_version(subsection_pk: int) -> SubsectionVersion: + """ + [ 🛑 UNSTABLE ] Get the latest subsection version. + + Args: + subsection_pk: The subsection ID. + """ + return Subsection.objects.get(pk=subsection_pk).versioning.latest + + +@dataclass(frozen=True) +class SubsectionListEntry: + """ + [ 🛑 UNSTABLE ] + Data about a single entity in a container, e.g. a unit in a subsection. + """ + unit_version: UnitVersion + pinned: bool = False + + @property + def unit(self): + return self.unit_version.unit + + +def get_units_in_subsection( + subsection: Subsection, + *, + published: bool, +) -> list[SubsectionListEntry]: + """ + [ 🛑 UNSTABLE ] + Get the list of entities and their versions in the draft or published + version of the given Subsection. + + Args: + subsection: The Subsection, e.g. returned by `get_subsection()` + published: `True` if we want the published version of the subsection, or + `False` for the draft version. + """ + assert isinstance(subsection, Subsection) + units = [] + for entry in publishing_api.get_entities_in_container(subsection, published=published): + # Convert from generic PublishableEntityVersion to UnitVersion: + unit_version = entry.entity_version.containerversion.unitversion + assert isinstance(unit_version, UnitVersion) + units.append(SubsectionListEntry(unit_version=unit_version, pinned=entry.pinned)) + return units + + +def get_units_in_published_subsection_as_of( + subsection: Subsection, + publish_log_id: int, +) -> list[SubsectionListEntry] | None: + """ + [ 🛑 UNSTABLE ] + Get the list of entities and their versions in the published version of the + given container as of the given PublishLog version (which is essentially a + version for the entire learning package). + + TODO: This API should be updated to also return the SubsectionVersion so we can + see the subsection title and any other metadata from that point in time. + TODO: accept a publish log UUID, not just int ID? + TODO: move the implementation to be a generic 'containers' implementation + that this subsections function merely wraps. + TODO: optimize, perhaps by having the publishlog store a record of all + ancestors of every modified PublishableEntity in the publish. + """ + assert isinstance(subsection, Subsection) + subsection_pub_entity_version = publishing_api.get_published_version_as_of(subsection.publishable_entity_id, publish_log_id) + if subsection_pub_entity_version is None: + return None # This subsection was not published as of the given PublishLog ID. + container_version = subsection_pub_entity_version.containerversion + + entity_list = [] + rows = container_version.entity_list.entitylistrow_set.order_by("order_num") + for row in rows: + if row.entity_version is not None: + unit_version = row.entity_version.containerversion.unitversion + assert isinstance(unit_version, UnitVersion) + entity_list.append(SubsectionListEntry(unit_version=unit_version, pinned=True)) + else: + # Unpinned unit - figure out what its latest published version was. + # This is not optimized. It could be done in one query per subsection rather than one query per unit. + pub_entity_version = publishing_api.get_published_version_as_of(row.entity_id, publish_log_id) + if pub_entity_version: + entity_list.append(SubsectionListEntry(unit_version=pub_entity_version.containerversion.unitversion, pinned=False)) + return entity_list diff --git a/openedx_learning/apps/authoring/subsections/apps.py b/openedx_learning/apps/authoring/subsections/apps.py new file mode 100644 index 000000000..281246da0 --- /dev/null +++ b/openedx_learning/apps/authoring/subsections/apps.py @@ -0,0 +1,25 @@ +""" +Subsection Django application initialization. +""" + +from django.apps import AppConfig + + +class SubsectionsConfig(AppConfig): + """ + Configuration for the subsections Django application. + """ + + name = "openedx_learning.apps.authoring.subsections" + verbose_name = "Learning Core > Authoring > Subsections" + default_auto_field = "django.db.models.BigAutoField" + label = "oel_subsections" + + def ready(self): + """ + Register Subsection and SubsectionVersion. + """ + from ..publishing.api import register_content_models # pylint: disable=import-outside-toplevel + from .models import Subsection, SubsectionVersion # pylint: disable=import-outside-toplevel + + register_content_models(Subsection, SubsectionVersion) diff --git a/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py b/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py new file mode 100644 index 000000000..62e2e473d --- /dev/null +++ b/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py @@ -0,0 +1,36 @@ +# Generated by Django 4.2.19 on 2025-04-09 12:59 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ('oel_publishing', '0005_alter_entitylistrow_options'), + ] + + operations = [ + migrations.CreateModel( + name='Subsection', + fields=[ + ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='oel_publishing.container')), + ], + options={ + 'abstract': False, + }, + bases=('oel_publishing.container',), + ), + migrations.CreateModel( + name='SubsectionVersion', + fields=[ + ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='oel_publishing.containerversion')), + ], + options={ + 'abstract': False, + }, + bases=('oel_publishing.containerversion',), + ), + ] diff --git a/openedx_learning/apps/authoring/subsections/migrations/__init__.py b/openedx_learning/apps/authoring/subsections/migrations/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/openedx_learning/apps/authoring/subsections/models.py b/openedx_learning/apps/authoring/subsections/models.py new file mode 100644 index 000000000..bf5747f8f --- /dev/null +++ b/openedx_learning/apps/authoring/subsections/models.py @@ -0,0 +1,50 @@ +""" +Models that implement units +""" +from django.db import models + +from ..publishing.models import Container, ContainerVersion + +__all__ = [ + "Subsection", + "SubsectionVersion", +] + + +class Subsection(Container): + """ + A Subsection is type of Container that holds Units. + + Via Container and its PublishableEntityMixin, Subsections are also publishable + entities and can be added to other containers. + """ + container = models.OneToOneField( + Container, + on_delete=models.CASCADE, + parent_link=True, + primary_key=True, + ) + + +class SubsectionVersion(ContainerVersion): + """ + A SubsectionVersion is a specific version of a Subsection. + + Via ContainerVersion and its EntityList, it defines the list of Units + in this version of the Subsection. + """ + container_version = models.OneToOneField( + ContainerVersion, + on_delete=models.CASCADE, + parent_link=True, + primary_key=True, + ) + + @property + def subsection(self): + """ Convenience accessor to the Subsection this version is associated with """ + return self.container_version.container.subsection # pylint: disable=no-member + + # Note: the 'publishable_entity_version' field is inherited and will appear on this model, but does not exist + # in the underlying database table. It only exists in the ContainerVersion table. + # You can verify this by running 'python manage.py sqlmigrate oel_subsections 0001_initial' diff --git a/projects/dev.py b/projects/dev.py index 85d01f4a5..cb86f8013 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -35,6 +35,7 @@ "openedx_learning.apps.authoring.components.apps.ComponentsConfig", "openedx_learning.apps.authoring.contents.apps.ContentsConfig", "openedx_learning.apps.authoring.publishing.apps.PublishingConfig", + "openedx_learning.apps.authoring.subsections.apps.SubsectionsConfig", "openedx_learning.apps.authoring.units.apps.UnitsConfig", # Learning Contrib Apps "openedx_learning.contrib.media_server.apps.MediaServerConfig", diff --git a/test_settings.py b/test_settings.py index 24af8ac89..0d4a9177d 100644 --- a/test_settings.py +++ b/test_settings.py @@ -45,6 +45,7 @@ def root(*args): "openedx_learning.apps.authoring.contents.apps.ContentsConfig", "openedx_learning.apps.authoring.publishing.apps.PublishingConfig", "openedx_tagging.core.tagging.apps.TaggingConfig", + "openedx_learning.apps.authoring.subsections.apps.SubsectionsConfig", "openedx_learning.apps.authoring.units.apps.UnitsConfig", ] diff --git a/tests/openedx_learning/apps/authoring/subsections/__init__.py b/tests/openedx_learning/apps/authoring/subsections/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/openedx_learning/apps/authoring/subsections/test_api.py b/tests/openedx_learning/apps/authoring/subsections/test_api.py new file mode 100644 index 000000000..b43c733e3 --- /dev/null +++ b/tests/openedx_learning/apps/authoring/subsections/test_api.py @@ -0,0 +1,1076 @@ +""" +Basic tests for the units API. +""" +import ddt # type: ignore[import] +import pytest +from django.core.exceptions import ValidationError +from django.db import IntegrityError + +from openedx_learning.api import authoring as authoring_api +from openedx_learning.api import authoring_models + +from ..units.test_api import UnitTestCase + +Entry = authoring_api.SubsectionListEntry + + +@ddt.ddt +class SubSectionTestCase(UnitTestCase): + """ Test cases for Subsections (containers of units) """ + + def setUp(self) -> None: + super().setUp() + self.unit_1, self.unit_1_v1 = self.create_unit( + key="Unit (1)", + title="Unit (1)", + ) + self.unit_2, self.unit_2_v1 = self.create_unit( + key="Unit (2)", + title="Unit (2)", + ) + + def create_unit(self, *, title: str = "Test Unit", key: str = "unit:1") -> tuple[ + authoring_models.Unit, authoring_models.UnitVersion + ]: + """ Helper method to quickly create a unit """ + return authoring_api.create_unit_and_version( + self.learning_package.id, + key=key, + title=title, + created=self.now, + created_by=None, + ) + + def create_subsection_with_units( + self, + units: list[authoring_models.Unit | authoring_models.UnitVersion], + *, + title="Unit", + key="unit:key", + ) -> authoring_models.Subsection: + """ Helper method to quickly create a subsection with some units """ + subsection, _subsection_v1 = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key=key, + title=title, + units=units, + created=self.now, + created_by=None, + ) + return subsection + + def modify_unit( + self, + unit: authoring_models.Unit, + *, + title="Modified Unit", + timestamp=None, + ) -> authoring_models.UnitVersion: + """ + Helper method to modify a unit for the purposes of testing units/drafts/pinning/publishing/etc. + """ + return authoring_api.create_next_unit_version( + unit, + title=title, + created=timestamp or self.now, + created_by=None, + ) + + def publish_unit(self, unit: authoring_models.Unit): + """ + Helper method to publish a single unit. + """ + authoring_api.publish_from_drafts( + self.learning_package.pk, + draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter( + entity=unit.publishable_entity, + ), + ) + + def test_get_subsection(self): + """ + Test get_subsection() + """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + with self.assertNumQueries(1): + result = authoring_api.get_subsection(subsection.pk) + assert result == subsection + # Versioning data should be pre-loaded via select_related() + with self.assertNumQueries(0): + assert result.versioning.has_unpublished_changes + + def test_get_subsection_version(self): + """ + Test get_subsection_version() + """ + subsection = self.create_subsection_with_units([]) + draft = subsection.versioning.draft + with self.assertNumQueries(1): + result = authoring_api.get_subsection_version(draft.pk) + assert result == draft + + def test_get_latest_subsection_version(self): + """ + Test test_get_latest_subsection_version() + """ + subsection = self.create_subsection_with_units([]) + draft = subsection.versioning.draft + with self.assertNumQueries(2): + result = authoring_api.get_latest_subsection_version(subsection.pk) + assert result == draft + + def test_get_containers(self): + """ + Test get_containers() + """ + subsection = self.create_subsection_with_units([]) + with self.assertNumQueries(1): + result = list(authoring_api.get_containers(self.learning_package.id)) + assert result == [self.unit_1.container, self.unit_2.container, subsection.container] + # Versioning data should be pre-loaded via select_related() + with self.assertNumQueries(0): + assert result[0].versioning.has_unpublished_changes + + def test_get_container(self): + """ + Test get_container() + """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + with self.assertNumQueries(1): + result = authoring_api.get_container(subsection.pk) + assert result == subsection.container + # Versioning data should be pre-loaded via select_related() + with self.assertNumQueries(0): + assert result.versioning.has_unpublished_changes + + def test_get_container_by_key(self): + """ + Test get_container_by_key() + """ + subsection = self.create_subsection_with_units([]) + with self.assertNumQueries(1): + result = authoring_api.get_container_by_key( + self.learning_package.id, + key=subsection.publishable_entity.key, + ) + assert result == subsection.container + # Versioning data should be pre-loaded via select_related() + with self.assertNumQueries(0): + assert result.versioning.has_unpublished_changes + + def test_subsection_container_versioning(self): + """ + Test that the .versioning helper of a Sebsection returns a SubsectionVersion, and + same for the generic Container equivalent. + """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + container = subsection.container + container_version = container.versioning.draft + assert isinstance(container_version, authoring_models.ContainerVersion) + subsection_version = subsection.versioning.draft + assert isinstance(subsection_version, authoring_models.SubsectionVersion) + assert subsection_version.container_version == container_version + assert subsection_version.container_version.container == container + assert subsection_version.subsection == subsection + + def test_create_subsection_queries(self): + """ + Test how many database queries are required to create a subsection + """ + # The exact numbers here aren't too important - this is just to alert us if anything significant changes. + with self.assertNumQueries(22): + _empty_subsection = self.create_subsection_with_units([]) + with self.assertNumQueries(26): + # And try with a non-empty subsection: + self.create_subsection_with_units([self.unit_1, self.unit_2_v1], key="u2") + + def test_create_subsection_with_invalid_children(self): + """ + Verify that only units can be added to subsections, and a specific + exception is raised. + """ + # Create two subsections: + subsection, subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key", + title="Subsection", + created=self.now, + created_by=None, + ) + assert subsection.versioning.draft == subsection_version + subsection2, _s2v1 = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key2", + title="Subsection 2", + created=self.now, + created_by=None, + ) + # Try adding a Subsection to a Subsection + with pytest.raises(TypeError, match="Subsection units must be either Unit or UnitVersion."): + authoring_api.create_next_subsection_version( + subsection=subsection, + title="Subsection Containing a Subsection", + units=[subsection2], + created=self.now, + created_by=None, + ) + # Check that a new version was not created: + subsection.refresh_from_db() + assert authoring_api.get_subsection(subsection.pk).versioning.draft == subsection_version + assert subsection.versioning.draft == subsection_version + + def test_adding_external_units(self): + """ + Test that units from another learning package cannot be added to a + subsection. + """ + learning_package2 = authoring_api.create_learning_package(key="other-package", title="Other Package") + subsection, _subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=learning_package2.pk, + key="subsection:key", + title="Subsection", + created=self.now, + created_by=None, + ) + assert self.unit_1.container.publishable_entity.learning_package != learning_package2 + # Try adding a a unit from LP 1 (self.learning_package) to a subsection from LP 2 + with pytest.raises(ValidationError, match="Container entities must be from the same learning package."): + authoring_api.create_next_subsection_version( + subsection=subsection, + title="Subsection Containing an External Unit", + units=[self.unit_1], + created=self.now, + created_by=None, + ) + + # ?FOR REVIEW + @ddt.data(True, False) + @pytest.mark.skip(reason="FIXME: This test is failing because the publishable_entity is not deleted from the database with the unit.") + # FIXME: Also, exception is Container.DoesNotExist, not Unit.DoesNotExist + def test_cannot_add_invalid_ids(self, pin_version): + """ + Test that non-existent units cannot be added to subsections + """ + self.unit_1.delete() + if pin_version: + units = [self.unit_1_v1] + else: + units = [self.unit_1] + with pytest.raises((IntegrityError, authoring_models.Unit.DoesNotExist)): + self.create_subsection_with_units(units) + + def test_create_empty_subsection_and_version(self): + """Test creating a subsection with no units. + + Expected results: + 1. A subsection and subsection version are created. + 2. The subsection version number is 1. + 3. The subsection is a draft with unpublished changes. + 4. There is no published version of the subsection. + """ + subsection, subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key", + title="Subsection", + created=self.now, + created_by=None, + ) + assert subsection, subsection_version + assert subsection_version.version_num == 1 + assert subsection_version in subsection.versioning.versions.all() + assert subsection.versioning.has_unpublished_changes + assert subsection.versioning.draft == subsection_version + assert subsection.versioning.published is None + assert subsection.publishable_entity.can_stand_alone + + def test_create_next_subsection_version_with_two_unpinned_units(self): + """Test creating a subsection version with two unpinned units. + + Expected results: + 1. A new subsection version is created. + 2. The subsection version number is 2. + 3. The subsection version is in the subsection's versions. + 4. The units are in the draft subsection version's unit list and are unpinned. + """ + subsection, _subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key", + title="Subsection", + created=self.now, + created_by=None, + ) + subsection_version_v2 = authoring_api.create_next_subsection_version( + subsection=subsection, + title="Subsection", + units=[self.unit_1, self.unit_2], + created=self.now, + created_by=None, + ) + assert subsection_version_v2.version_num == 2 + assert subsection_version_v2 in subsection.versioning.versions.all() + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1.versioning.draft), + Entry(self.unit_2.versioning.draft), + ] + with pytest.raises(authoring_models.ContainerVersion.DoesNotExist): + # There is no published version of the subsection: + authoring_api.get_units_in_subsection(subsection, published=True) + + def test_create_next_subsection_version_with_unpinned_and_pinned_units(self): + """ + Test creating a subsection version with one unpinned and one pinned 📌 unit. + """ + subsection, _subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key", + title="Subsection", + created=self.now, + created_by=None, + ) + subsection_version_v2 = authoring_api.create_next_subsection_version( + subsection=subsection, + title="Subsection", + units=[self.unit_1, self.unit_2_v1], # Note the "v1" pinning 📌 the second one to version 1 + created=self.now, + created_by=None, + ) + assert subsection_version_v2.version_num == 2 + assert subsection_version_v2 in subsection.versioning.versions.all() + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1_v1), + Entry(self.unit_2_v1, pinned=True), # Pinned 📌 to v1 + ] + with pytest.raises(authoring_models.ContainerVersion.DoesNotExist): + # There is no published version of the subsection: + authoring_api.get_units_in_subsection(subsection, published=True) + + def test_auto_publish_children(self): + """ + Test that publishing a subsection publishes its child units automatically. + """ + # Create a draft subsection with two draft units + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + # Also create another unit that's not in the subsection at all: + other_unit, _ou_v1 = self.create_unit(title="A draft unit not in the subsection", key="unit:3") + + assert authoring_api.contains_unpublished_changes(subsection.pk) + assert self.unit_1.versioning.published is None + assert self.unit_2.versioning.published is None + + # Publish ONLY the subsection. This should however also auto-publish units 1 & 2 since they're children + authoring_api.publish_from_drafts( + self.learning_package.pk, + draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter(entity=subsection.publishable_entity), + ) + # Now all changes to the subsection and to unit 1 are published: + subsection.refresh_from_db() + self.unit_1.refresh_from_db() + assert subsection.versioning.has_unpublished_changes is False # Shallow check + assert self.unit_1.versioning.has_unpublished_changes is False + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # Deep check + assert self.unit_1.versioning.published == self.unit_1_v1 # v1 is now the published version. + + # But our other unit that's outside the subsection is not affected: + other_unit.refresh_from_db() + assert other_unit.versioning.has_unpublished_changes + assert other_unit.versioning.published is None + + def test_no_publish_parent(self): + """ + Test that publishing a unit does NOT publish changes to its parent subsection + """ + # Create a draft subsection with two draft units + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + assert subsection.versioning.has_unpublished_changes + # Publish ONLY one of its child units + self.publish_unit(self.unit_1) + self.unit_1.refresh_from_db() # Clear cache on '.versioning' + assert self.unit_1.versioning.has_unpublished_changes is False + + # The subsection that contains that unit should still be unpublished: + subsection.refresh_from_db() # Clear cache on '.versioning' + assert subsection.versioning.has_unpublished_changes + assert subsection.versioning.published is None + with pytest.raises(authoring_models.ContainerVersion.DoesNotExist): + # There is no published version of the subsection: + authoring_api.get_units_in_subsection(subsection, published=True) + + def test_add_unit_after_publish(self): + """ + Adding a unit to a published subsection will create a new version and + show that the subsection has unpublished changes. + """ + subsection, subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key", + title="Subsection", + created=self.now, + created_by=None, + ) + assert subsection.versioning.draft == subsection_version + assert subsection.versioning.published is None + assert subsection.versioning.has_unpublished_changes + # Publish the empty subsection: + authoring_api.publish_all_drafts(self.learning_package.id) + subsection.refresh_from_db() # Reloading the subsection is necessary + assert subsection.versioning.has_unpublished_changes is False # Shallow check for just the subsection itself, not children + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # Deeper check + + # Add a published unit (unpinned): + assert self.unit_1.versioning.has_unpublished_changes is False + subsection_version_v2 = authoring_api.create_next_subsection_version( + subsection=subsection, + title=subsection_version.title, + units=[self.unit_1], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.APPEND, + ) + # Now the subsection should have unpublished changes: + subsection.refresh_from_db() # Reloading the subsection is necessary + assert subsection.versioning.has_unpublished_changes # Shallow check - adding a child is a change to the subsection + assert authoring_api.contains_unpublished_changes(subsection.pk) # Deeper check + assert subsection.versioning.draft == subsection_version_v2 + assert subsection.versioning.published == subsection_version + + def test_modify_unpinned_unit_after_publish(self): + """ + Modifying an unpinned unit in a published subsection will NOT create a + new version nor show that the subsection has unpublished changes (but it will + "contain" unpublished changes). The modifications will appear in the + published version of the subsection only after the unit is published. + """ + # Create a subsection with one unpinned draft unit: + assert self.unit_1.versioning.has_unpublished_changes + subsection = self.create_subsection_with_units([self.unit_1]) + assert subsection.versioning.has_unpublished_changes + + # Publish the subsection and the unit: + authoring_api.publish_all_drafts(self.learning_package.id) + subsection.refresh_from_db() # Reloading the subsection is necessary if we accessed 'versioning' before publish + self.unit_1.refresh_from_db() + assert subsection.versioning.has_unpublished_changes is False # Shallow check + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # Deeper check + assert self.unit_1.versioning.has_unpublished_changes is False + + # Now modify the unit by changing its title (it remains a draft): + unit_1_v2 = self.modify_unit(self.unit_1, title="Modified Counting Problem with new title") + + # The unit now has unpublished changes; the subsection doesn't directly but does contain + subsection.refresh_from_db() # Reloading the subsection is necessary, or 'subsection.versioning' will be outdated + self.unit_1.refresh_from_db() + assert subsection.versioning.has_unpublished_changes is False # Shallow check should be false - subsection is unchanged + assert authoring_api.contains_unpublished_changes(subsection.pk) # But subsection DOES contain changes + assert self.unit_1.versioning.has_unpublished_changes + + # Since the unit changes haven't been published, they should only appear in the draft subsection + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(unit_1_v2), # new version + ] + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), # old version + ] + + # But if we publish the unit, the changes will appear in the published version of the subsection. + self.publish_unit(self.unit_1) + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(unit_1_v2), # new version + ] + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(unit_1_v2), # new version + ] + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # No longer contains unpublished changes + + def test_modify_pinned_unit(self): + """ + When a pinned 📌 unit in subsection is modified and/or published, it will + have no effect on either the draft nor published version of the subsection, + which will continue to use the pinned version. + """ + # Create a subsection with one unit (pinned 📌 to v1): + subsection = self.create_subsection_with_units([self.unit_1_v1]) + + # Publish the subsection and the unit: + authoring_api.publish_all_drafts(self.learning_package.id) + expected_subsection_contents = [ + Entry(self.unit_1_v1, pinned=True), # pinned 📌 to v1 + ] + assert authoring_api.get_units_in_subsection(subsection, published=True) == expected_subsection_contents + + # Now modify the unit by changing its title (it remains a draft): + self.modify_unit(self.unit_1, title="Modified Counting Problem with new title") + + # The unit now has unpublished changes; the subsection is entirely unaffected + subsection.refresh_from_db() # Reloading the subsection is necessary, or 'subsection.versioning' will be outdated + self.unit_1.refresh_from_db() + assert subsection.versioning.has_unpublished_changes is False # Shallow check + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # Deep check + assert self.unit_1.versioning.has_unpublished_changes is True + + # Neither the draft nor the published version of the subsection is affected + assert authoring_api.get_units_in_subsection(subsection, published=False) == expected_subsection_contents + assert authoring_api.get_units_in_subsection(subsection, published=True) == expected_subsection_contents + # Even if we publish the unit, the subsection stays pinned to the specified version: + self.publish_unit(self.unit_1) + assert authoring_api.get_units_in_subsection(subsection, published=False) == expected_subsection_contents + assert authoring_api.get_units_in_subsection(subsection, published=True) == expected_subsection_contents + + def test_create_two_subsections_with_same_units(self): + """ + Test creating two subsections with different combinations of the same two + units in each subsection. + """ + # Create a subsection with unit 2 unpinned, unit 2 pinned 📌, and unit 1: + subsection1 = self.create_subsection_with_units([self.unit_2, self.unit_2_v1, self.unit_1], key="u1") + # Create a second subsection with unit 1 pinned 📌, unit 2, and unit 1 unpinned: + subsection2 = self.create_subsection_with_units([self.unit_1_v1, self.unit_2, self.unit_1], key="u2") + + # Check that the contents are as expected: + assert [row.unit_version for row in authoring_api.get_units_in_subsection(subsection1, published=False)] == [ + self.unit_2_v1, self.unit_2_v1, self.unit_1_v1, + ] + assert [row.unit_version for row in authoring_api.get_units_in_subsection(subsection2, published=False)] == [ + self.unit_1_v1, self.unit_2_v1, self.unit_1_v1, + ] + + # Modify unit 1 + unit_1_v2 = self.modify_unit(self.unit_1, title="unit 1 v2") + # Publish changes + authoring_api.publish_all_drafts(self.learning_package.id) + # Modify unit 2 - only in the draft + unit_2_v2 = self.modify_unit(self.unit_2, title="unit 2 DRAFT") + + # Check that the draft contents are as expected: + assert authoring_api.get_units_in_subsection(subsection1, published=False) == [ + Entry(unit_2_v2), # v2 in the draft version + Entry(self.unit_2_v1, pinned=True), # pinned 📌 to v1 + Entry(unit_1_v2), # v2 + ] + assert authoring_api.get_units_in_subsection(subsection2, published=False) == [ + Entry(self.unit_1_v1, pinned=True), # pinned 📌 to v1 + Entry(unit_2_v2), # v2 in the draft version + Entry(unit_1_v2), # v2 + ] + + # Check that the published contents are as expected: + assert authoring_api.get_units_in_subsection(subsection1, published=True) == [ + Entry(self.unit_2_v1), # v1 in the published version + Entry(self.unit_2_v1, pinned=True), # pinned 📌 to v1 + Entry(unit_1_v2), # v2 + ] + assert authoring_api.get_units_in_subsection(subsection2, published=True) == [ + Entry(self.unit_1_v1, pinned=True), # pinned 📌 to v1 + Entry(self.unit_2_v1), # v1 in the published version + Entry(unit_1_v2), # v2 + ] + + def test_publishing_shared_unit(self): + """ + A complex test case involving two subsections with a shared unit and + other non-shared units. + + Subsection 1: units C1, C2, C3 + Subsection 2: units C2, C4, C5 + Everything is "unpinned". + """ + # 1️⃣ Create the subsections and publish them: + (u1, u1_v1), (u2, _u2_v1), (u3, u3_v1), (u4, u4_v1), (u5, u5_v1) = [ + self.create_unit(key=f"C{i}", title=f"Unit {i}") for i in range(1, 6) + ] + subsection1 = self.create_subsection_with_units([u1, u2, u3], title="Subsection 1", key="subsection:1") + subsection2 = self.create_subsection_with_units([u2, u4, u5], title="Subsection 2", key="subsection:2") + authoring_api.publish_all_drafts(self.learning_package.id) + assert authoring_api.contains_unpublished_changes(subsection1.pk) is False + assert authoring_api.contains_unpublished_changes(subsection2.pk) is False + + # 2️⃣ Then the author edits U2 inside of Subsection 1 making U2v2. + u2_v2 = self.modify_unit(u2, title="U2 version 2") + # This makes S1 and S2 both show up as Subsections that CONTAIN unpublished changes, because they share the unit. + assert authoring_api.contains_unpublished_changes(subsection1.pk) + assert authoring_api.contains_unpublished_changes(subsection2.pk) + # (But the subsections themselves are unchanged:) + subsection1.refresh_from_db() + subsection2.refresh_from_db() + assert subsection1.versioning.has_unpublished_changes is False + assert subsection2.versioning.has_unpublished_changes is False + + # 3️⃣ In addition to this, the author also modifies another unit in Subsection 2 (U5) + u5_v2 = self.modify_unit(u5, title="U5 version 2") + + # 4️⃣ The author then publishes Subsection 1, and therefore everything in it. + authoring_api.publish_from_drafts( + self.learning_package.pk, + draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter( + # Note: we only publish the subsection; the publishing API should auto-publish its units too. + entity_id=subsection1.publishable_entity.id, + ), + ) + + # Result: Subsection 1 will show the newly published version of U2: + assert authoring_api.get_units_in_subsection(subsection1, published=True) == [ + Entry(u1_v1), + Entry(u2_v2), # new published version of U2 + Entry(u3_v1), + ] + + # Result: someone looking at Subsection 2 should see the newly published unit 2, because publishing it anywhere + # publishes it everywhere. But publishing U2 and Subsection 1 does not affect the other units in Subsection 2. + # (Publish propagates downward, not upward) + assert authoring_api.get_units_in_subsection(subsection2, published=True) == [ + Entry(u2_v2), # new published version of U2 + Entry(u4_v1), # still original version of U4 (it was never modified) + Entry(u5_v1), # still original version of U5 (it hasn't been published) + ] + + # Result: Subsection 2 CONTAINS unpublished changes because of the modified U5. Subsection 1 doesn't contain unpub changes. + assert authoring_api.contains_unpublished_changes(subsection1.pk) is False + assert authoring_api.contains_unpublished_changes(subsection2.pk) + + # 5️⃣ Publish unit U5, which should be the only thing unpublished in the learning package + self.publish_unit(u5) + # Result: Subsection 2 shows the new version of C5 and no longer contains unpublished changes: + assert authoring_api.get_units_in_subsection(subsection2, published=True) == [ + Entry(u2_v2), # new published version of U2 + Entry(u4_v1), # still original version of U4 (it was never modified) + Entry(u5_v2), # new published version of U5 + ] + assert authoring_api.contains_unpublished_changes(subsection2.pk) is False + + def test_query_count_of_contains_unpublished_changes(self): + """ + Checking for unpublished changes in a subsection should require a fixed number + of queries, not get more expensive as the subsection gets larger. + """ + # Add 2 units (unpinned) + unit_count = 2 + units = [] + for i in range(unit_count): + unit, _version = self.create_unit( + key=f"Unit {i}", + title=f"Unit {i}", + ) + units.append(unit) + subsection = self.create_subsection_with_units(units) + authoring_api.publish_all_drafts(self.learning_package.id) + subsection.refresh_from_db() + with self.assertNumQueries(6): + assert authoring_api.contains_unpublished_changes(subsection.pk) is False + + # Modify the most recently created unit: + self.modify_unit(unit, title="Modified Unit") + with self.assertNumQueries(5): + assert authoring_api.contains_unpublished_changes(subsection.pk) is True + + def test_metadata_change_doesnt_create_entity_list(self): + """ + Test that changing a container's metadata like title will create a new + version, but can re-use the same EntityList. API consumers generally + shouldn't depend on this behavior; it's an optimization. + """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2_v1]) + + orig_version_num = subsection.versioning.draft.version_num + orig_entity_list_id = subsection.versioning.draft.entity_list.pk + + authoring_api.create_next_subsection_version(subsection, title="New Title", created=self.now) + + subsection.refresh_from_db() + new_version_num = subsection.versioning.draft.version_num + new_entity_list_id = subsection.versioning.draft.entity_list.pk + + assert new_version_num > orig_version_num + assert new_entity_list_id == orig_entity_list_id + + @ddt.data(True, False) + @pytest.mark.skip(reason="FIXME: we don't yet prevent adding soft-deleted units to subsections") + def test_cannot_add_soft_deleted_unit(self, publish_first): + """ + Test that a soft-deleted unit cannot be added to a subsection. + + Although it's valid for subsections to contain soft-deleted units (by + deleting the unit after adding it), it is likely a mistake if + you're trying to add one to the subsection. + """ + unit, _cv = self.create_unit(title="Deleted unit") + if publish_first: + # Publish the unit: + authoring_api.publish_all_drafts(self.learning_package.id) + # Now delete it. The draft version is now deleted: + authoring_api.soft_delete_draft(unit.pk) + # Now try adding that unit to a subsection: + with pytest.raises(ValidationError, match="unit is deleted"): + self.create_subsection_with_units([unit]) + + def test_removing_unit(self): + """ Test removing a unit from a subsection (but not deleting it) """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now remove unit 2 + authoring_api.create_next_subsection_version( + subsection=subsection, + title="Revised with unit 2 deleted", + units=[self.unit_2], + created=self.now, + entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, + ) + + # Now it should not be listed in the subsection: + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1_v1), + ] + subsection.refresh_from_db() + assert subsection.versioning.has_unpublished_changes # The subsection itself and its unit list have change + assert authoring_api.contains_unpublished_changes(subsection.pk) + # The published version of the subsection is not yet affected: + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), + Entry(self.unit_2_v1), + ] + + # But when we publish the new subsection version with the removal, the published version is affected: + authoring_api.publish_all_drafts(self.learning_package.id) + # FIXME: Refreshing the subsection is necessary here because get_entities_in_published_container() accesses + # container.versioning.published, and .versioning is cached with the old version. But this seems like + # a footgun? We could avoid this if get_entities_in_published_container() took only an ID instead of an object, + # but that would involve additional database lookup(s). + subsection.refresh_from_db() + assert authoring_api.contains_unpublished_changes(subsection.pk) is False + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), + ] + + def test_soft_deleting_unit(self): + """ Test soft deleting a unit that's in a subsection (but not removing it) """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now soft delete unit 2 + authoring_api.soft_delete_draft(self.unit_2.pk) + + # Now it should not be listed in the subsection: + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1_v1), + # unit 2 is soft deleted from the draft. + # TODO: should we return some kind of placeholder here, to indicate that a unit is still listed in the + # subsection's unit list but has been soft deleted, and will be fully deleted when published, or restored if + # reverted? + ] + assert subsection.versioning.has_unpublished_changes is False # The subsection itself and its unit list is not changed + assert authoring_api.contains_unpublished_changes(subsection.pk) # But it CONTAINS an unpublished change (a deletion) + # The published version of the subsection is not yet affected: + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), + Entry(self.unit_2_v1), + ] + + # But when we publish the deletion, the published version is affected: + authoring_api.publish_all_drafts(self.learning_package.id) + assert authoring_api.contains_unpublished_changes(subsection.pk) is False + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), + ] + + def test_soft_deleting_and_removing_unit(self): + """ Test soft deleting a unit that's in a subsection AND removing it """ + subsection = self.create_subsection_with_units([self.unit_1, self.unit_2]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now soft delete unit 2 + authoring_api.soft_delete_draft(self.unit_2.pk) + # And remove it from the subsection: + authoring_api.create_next_subsection_version( + subsection=subsection, + title="Revised with unit 2 deleted", + units=[self.unit_2], + created=self.now, + entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, + ) + + # Now it should not be listed in the subsection: + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1_v1), + ] + assert subsection.versioning.has_unpublished_changes is True + assert authoring_api.contains_unpublished_changes(subsection.pk) + # The published version of the subsection is not yet affected: + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), + Entry(self.unit_2_v1), + ] + + # But when we publish the deletion, the published version is affected: + authoring_api.publish_all_drafts(self.learning_package.id) + assert authoring_api.contains_unpublished_changes(subsection.pk) is False + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1), + ] + + def test_soft_deleting_pinned_unit(self): + """ Test soft deleting a pinned 📌 unit that's in a subsection """ + subsection = self.create_subsection_with_units([self.unit_1_v1, self.unit_2_v1]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now soft delete unit 2 + authoring_api.soft_delete_draft(self.unit_2.pk) + + # Now it should still be listed in the subsection: + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1_v1, pinned=True), + Entry(self.unit_2_v1, pinned=True), + ] + assert subsection.versioning.has_unpublished_changes is False # The subsection itself and its unit list is not changed + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # nor does it contain changes + # The published version of the subsection is also not affected: + assert authoring_api.get_units_in_subsection(subsection, published=True) == [ + Entry(self.unit_1_v1, pinned=True), + Entry(self.unit_2_v1, pinned=True), + ] + + def test_soft_delete_subsection(self): + """ + I can delete a subsection without deleting the units it contains. + + See https://github.com/openedx/frontend-app-authoring/issues/1693 + """ + # Create two subsections, one of which we will soon delete: + subsection_to_delete = self.create_subsection_with_units([self.unit_1, self.unit_2]) + other_subsection = self.create_subsection_with_units([self.unit_1], key="other") + + # Publish everything: + authoring_api.publish_all_drafts(self.learning_package.id) + # Delete the subsection: + authoring_api.soft_delete_draft(subsection_to_delete.publishable_entity_id) + subsection_to_delete.refresh_from_db() + # Now the draft subsection is [soft] deleted, but the units, published subsection, and other subsection is unaffected: + assert subsection_to_delete.versioning.draft is None # Subsection is soft deleted. + assert subsection_to_delete.versioning.published is not None + self.unit_1.refresh_from_db() + assert self.unit_1.versioning.draft is not None + assert authoring_api.get_units_in_subsection(other_subsection, published=False) == [Entry(self.unit_1_v1)] + + # Publish everything: + authoring_api.publish_all_drafts(self.learning_package.id) + # Now the subsection's published version is also deleted, but nothing else is affected. + subsection_to_delete.refresh_from_db() + assert subsection_to_delete.versioning.draft is None # Subsection is soft deleted. + assert subsection_to_delete.versioning.published is None + self.unit_1.refresh_from_db() + assert self.unit_1.versioning.draft is not None + assert self.unit_1.versioning.published is not None + assert authoring_api.get_units_in_subsection(other_subsection, published=False) == [Entry(self.unit_1_v1)] + assert authoring_api.get_units_in_subsection(other_subsection, published=True) == [Entry(self.unit_1_v1)] + + def test_snapshots_of_published_subsection(self): + """ + Test that we can access snapshots of the historic published version of + subsections and their contents. + """ + # At first the subsection has one unit (unpinned): + subsection = self.create_subsection_with_units([self.unit_1]) + self.modify_unit(self.unit_1, title="Unit 1 as of checkpoint 1") + before_publish = authoring_api.get_units_in_published_subsection_as_of(subsection, 0) + assert before_publish is None + + # Publish everything, creating Checkpoint 1 + checkpoint_1 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 1") + + ######################################################################## + + # Now we update the title of the unit. + self.modify_unit(self.unit_1, title="Unit 1 as of checkpoint 2") + # Publish everything, creating Checkpoint 2 + checkpoint_2 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 2") + ######################################################################## + + # Now add a second unit to the subsection: + self.modify_unit(self.unit_1, title="Unit 1 as of checkpoint 3") + self.modify_unit(self.unit_2, title="Unit 2 as of checkpoint 3") + authoring_api.create_next_subsection_version( + subsection=subsection, + title="Subsection title in checkpoint 3", + units=[self.unit_1, self.unit_2], + created=self.now, + ) + # Publish everything, creating Checkpoint 3 + checkpoint_3 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 3") + ######################################################################## + + # Now add a third unit to the subsection, a pinned 📌 version of unit 1. + # This will test pinned versions and also test adding at the beginning rather than the end of the subsection. + authoring_api.create_next_subsection_version( + subsection=subsection, + title="Subsection title in checkpoint 4", + units=[self.unit_1_v1, self.unit_1, self.unit_2], + created=self.now, + ) + # Publish everything, creating Checkpoint 4 + checkpoint_4 = authoring_api.publish_all_drafts(self.learning_package.id, message="checkpoint 4") + ######################################################################## + + # Modify the drafts, but don't publish: + self.modify_unit(self.unit_1, title="Unit 1 draft") + self.modify_unit(self.unit_2, title="Unit 2 draft") + + # Now fetch the snapshots: + as_of_checkpoint_1 = authoring_api.get_units_in_published_subsection_as_of(subsection, checkpoint_1.pk) + assert [cv.unit_version.title for cv in as_of_checkpoint_1] == [ + "Unit 1 as of checkpoint 1", + ] + as_of_checkpoint_2 = authoring_api.get_units_in_published_subsection_as_of(subsection, checkpoint_2.pk) + assert [cv.unit_version.title for cv in as_of_checkpoint_2] == [ + "Unit 1 as of checkpoint 2", + ] + as_of_checkpoint_3 = authoring_api.get_units_in_published_subsection_as_of(subsection, checkpoint_3.pk) + assert [cv.unit_version.title for cv in as_of_checkpoint_3] == [ + "Unit 1 as of checkpoint 3", + "Unit 2 as of checkpoint 3", + ] + as_of_checkpoint_4 = authoring_api.get_units_in_published_subsection_as_of(subsection, checkpoint_4.pk) + assert [cv.unit_version.title for cv in as_of_checkpoint_4] == [ + "Unit (1)", # Pinned. This title is self.unit_1_v1.title (original v1 title) + "Unit 1 as of checkpoint 3", # we didn't modify these units so they're same as in snapshot 3 + "Unit 2 as of checkpoint 3", # we didn't modify these units so they're same as in snapshot 3 + ] + + def test_subsections_containing(self): + """ + Test that we can efficiently get a list of all the [draft] subsections + containing a given unit. + """ + unit_1_v2 = self.modify_unit(self.unit_1, title="modified unit 1") + + # Create a few subsections, some of which contain unit 1 and others which don't: + # Note: it is important that some of these subsections contain other units, to ensure the complex JOINs required + # for this query are working correctly, especially in the case of ignore_pinned=True. + # Subsection 1 ✅ has unit 1, pinned 📌 to V1 + subsection1_1pinned = self.create_subsection_with_units([self.unit_1_v1, self.unit_2], key="u1") + # Subsection 2 ✅ has unit 1, pinned 📌 to V2 + subsection2_1pinned_v2 = self.create_subsection_with_units([unit_1_v2, self.unit_2_v1], key="u2") + # Subsection 3 doesn't contain it + _subsection3_no = self.create_subsection_with_units([self.unit_2], key="u3") + # Subsection 4 ✅ has unit 1, unpinned + subsection4_unpinned = self.create_subsection_with_units([ + self.unit_1, self.unit_2, self.unit_2_v1, + ], key="u4") + # Subsections 5/6 don't contain it + _subsection5_no = self.create_subsection_with_units([self.unit_2_v1, self.unit_2], key="u5") + _subsection6_no = self.create_subsection_with_units([], key="u6") + + # No need to publish anything as the get_containers_with_entity() API only considers drafts (for now). + + with self.assertNumQueries(1): + result = [ + c.subsection for c in + authoring_api.get_containers_with_entity(self.unit_1.pk).select_related("subsection") + ] + assert result == [ + subsection1_1pinned, + subsection2_1pinned_v2, + subsection4_unpinned, + ] + + # Test retrieving only "unpinned", for cases like potential deletion of a unit, where we wouldn't care + # about pinned uses anyways (they would be unaffected by a delete). + + with self.assertNumQueries(1): + result2 = [ + c.subsection for c in + authoring_api.get_containers_with_entity(self.unit_1.pk, ignore_pinned=True).select_related("subsection") + ] + assert result2 == [subsection4_unpinned] + + def test_add_remove_container_children(self): + """ + Test adding and removing children units from subsections. + """ + subsection, subsection_version = authoring_api.create_subsection_and_version( + learning_package_id=self.learning_package.id, + key="subsection:key", + title="Subsection", + units=[self.unit_1], + created=self.now, + created_by=None, + ) + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1.versioning.draft), + ] + unit_3, _ = self.create_unit( + key="Unit (3)", + title="Unit (3)", + ) + # Add unit_2 and unit_3 + subsection_version_v2 = authoring_api.create_next_subsection_version( + subsection=subsection, + title=subsection_version.title, + units=[self.unit_2, unit_3], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.APPEND, + ) + subsection.refresh_from_db() + assert subsection_version_v2.version_num == 2 + assert subsection_version_v2 in subsection.versioning.versions.all() + # Verify that unit_2 and unit_3 is added to end + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_1.versioning.draft), + Entry(self.unit_2.versioning.draft), + Entry(unit_3.versioning.draft), + ] + + # Remove unit_1 + authoring_api.create_next_subsection_version( + subsection=subsection, + title=subsection_version.title, + units=[self.unit_1], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, + ) + subsection.refresh_from_db() + # Verify that unit_1 is removed + assert authoring_api.get_units_in_subsection(subsection, published=False) == [ + Entry(self.unit_2.versioning.draft), + Entry(unit_3.versioning.draft), + ] + + def test_get_container_children_count(self): + """ + Test get_container_children_count() + """ + subsection = self.create_subsection_with_units([self.unit_1]) + assert authoring_api.get_container_children_count(subsection.container, published=False) == 1 + # publish + authoring_api.publish_all_drafts(self.learning_package.id) + subsection_version = subsection.versioning.draft + authoring_api.create_next_subsection_version( + subsection=subsection, + title=subsection_version.title, + units=[self.unit_2], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.APPEND, + ) + subsection.refresh_from_db() + # Should have two units in draft version and 1 in published version + assert authoring_api.get_container_children_count(subsection.container, published=False) == 2 + assert authoring_api.get_container_children_count(subsection.container, published=True) == 1 + # publish + authoring_api.publish_all_drafts(self.learning_package.id) + subsection.refresh_from_db() + assert authoring_api.get_container_children_count(subsection.container, published=True) == 2 + # Soft delete unit_1 + authoring_api.soft_delete_draft(self.unit_1.pk) + subsection.refresh_from_db() + # Should contain only 1 child + assert authoring_api.get_container_children_count(subsection.container, published=False) == 1 + authoring_api.publish_all_drafts(self.learning_package.id) + subsection.refresh_from_db() + assert authoring_api.get_container_children_count(subsection.container, published=True) == 1 + + # Tests TODO: + # Test that I can get a [PublishLog] history of a given subsection and all its children, including children that aren't + # currently in the subsection and excluding children that are only in other subsections. + # Test that I can get a [PublishLog] history of a given subsection and its children, that includes changes made to the + # child units while they were part of the subsection but excludes changes made to those children while they were + # not part of the subsection. 🫣 From 6b96b739818e5590cfa20142655ca5e155233048 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 11 Apr 2025 15:39:18 +0300 Subject: [PATCH 2/7] refactor: extend annotation_safe_list with new model & bump package version --- .annotation_safe_list.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index f5b7eec98..91eba7064 100644 --- a/.annotation_safe_list.yml +++ b/.annotation_safe_list.yml @@ -65,6 +65,10 @@ oel_tagging.TagImportTask: ".. no_pii:": "This model has no PII" oel_tagging.Taxonomy: ".. no_pii:": "This model has no PII" +oel_subsections.Subsection: + ".. no_pii:": "This model has no PII" +oel_subsections.SubsectionVersion: + ".. no_pii:": "This model has no PII" oel_units.Unit: ".. no_pii:": "This model has no PII" oel_units.UnitVersion: From 66f84754aa67020a83af1abbcbe564db9ae92e4f Mon Sep 17 00:00:00 2001 From: Andrii Date: Tue, 15 Apr 2025 22:46:01 +0300 Subject: [PATCH 3/7] refactor: update child entities to new API, fix some name & style issues --- .../apps/authoring/publishing/api.py | 3 +- .../apps/authoring/subsections/api.py | 52 ++++++++++--------- .../apps/authoring/subsections/models.py | 2 +- .../apps/authoring/subsections/test_api.py | 37 +++++++++++-- 4 files changed, 61 insertions(+), 33 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/api.py b/openedx_learning/apps/authoring/publishing/api.py index 9285c1048..63514df06 100644 --- a/openedx_learning/apps/authoring/publishing/api.py +++ b/openedx_learning/apps/authoring/publishing/api.py @@ -1323,9 +1323,8 @@ def contains_unpublished_changes(container_id: int) -> bool: # We only care about children that are un-pinned, since published changes to pinned children don't matter entity_list = getattr(container.versioning.draft, "entity_list", None) - # ?FOR REVIEW: Is this correct? if entity_list is None: - # This container has not been published yet, or has been deleted. + # This container has been soft-deleted, so it has no children. return False # This is a naive and inefficient implementation but should be correct. diff --git a/openedx_learning/apps/authoring/subsections/api.py b/openedx_learning/apps/authoring/subsections/api.py index e335b4b48..a47400d5c 100644 --- a/openedx_learning/apps/authoring/subsections/api.py +++ b/openedx_learning/apps/authoring/subsections/api.py @@ -62,8 +62,7 @@ def create_subsection_version( version_num: int, *, title: str, - publishable_entities_pks: list[int], - entity_version_pks: list[int | None], + entity_rows: list[publishing_api.ContainerEntityRow], created: datetime, created_by: int | None = None, ) -> SubsectionVersion: @@ -78,8 +77,7 @@ def create_subsection_version( subsection_pk: The subsection ID. version_num: The version number. title: The title. - publishable_entities_pk: The publishable entities. - entity: The entity. + entity_rows: child entities/versions created: The creation date. created_by: The user who created the subsection. """ @@ -87,8 +85,7 @@ def create_subsection_version( subsection.pk, version_num, title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, + entity_rows=entity_rows, created=created, created_by=created_by, container_version_cls=SubsectionVersion, @@ -97,30 +94,33 @@ def create_subsection_version( def _pub_entities_for_units( units: list[Unit | UnitVersion] | None, -) -> tuple[list[int], list[int | None]] | tuple[None, None]: +) -> list[publishing_api.ContainerEntityRow] | None: """ Helper method: given a list of Unit | UnitVersion, return the - lists of publishable_entities_pks and entity_version_pks needed for the - base container APIs. + list of ContainerEntityRows needed for the base container APIs. UnitVersion is passed when we want to pin a specific version, otherwise Unit is used for unpinned. """ if units is None: # When these are None, that means don't change the entities in the list. - return None, None + return None for u in units: if not isinstance(u, (Unit, UnitVersion)): raise TypeError("Subsection units must be either Unit or UnitVersion.") - publishable_entities_pks = [ - (u.publishable_entity_id if isinstance(u, Unit) else u.unit.publishable_entity_id) + return [ + ( + publishing_api.ContainerEntityRow( + entity_pk=u.container.publishable_entity_id, + version_pk=None, + ) if isinstance(u, Unit) + else publishing_api.ContainerEntityRow( + entity_pk=u.unit.container.publishable_entity_id, + version_pk=u.container_version.publishable_entity_version_id, + ) + ) for u in units ] - entity_version_pks = [ - (uv.pk if isinstance(uv, UnitVersion) else None) - for uv in units - ] - return publishable_entities_pks, entity_version_pks def create_next_subsection_version( @@ -143,12 +143,11 @@ def create_next_subsection_version( created: The creation date. created_by: The user who created the subsection. """ - publishable_entities_pks, entity_version_pks = _pub_entities_for_units(units) + entity_rows = _pub_entities_for_units(units) subsection_version = publishing_api.create_next_container_version( subsection.pk, title=title, - publishable_entities_pks=publishable_entities_pks, - entity_version_pks=entity_version_pks, + entity_rows=entity_rows, created=created, created_by=created_by, container_version_cls=SubsectionVersion, @@ -177,7 +176,7 @@ def create_subsection_and_version( created_by: The user who created the subsection. can_stand_alone: Set to False when created as part of containers """ - publishable_entities_pks, entity_version_pks = _pub_entities_for_units(units) + entity_rows = _pub_entities_for_units(units) with atomic(): subsection = create_subsection( learning_package_id, @@ -190,8 +189,7 @@ def create_subsection_and_version( subsection, 1, title=title, - publishable_entities_pks=publishable_entities_pks or [], - entity_version_pks=entity_version_pks or [], + entity_rows=entity_rows or [], created=created, created_by=created_by, ) @@ -286,7 +284,9 @@ def get_units_in_published_subsection_as_of( ancestors of every modified PublishableEntity in the publish. """ assert isinstance(subsection, Subsection) - subsection_pub_entity_version = publishing_api.get_published_version_as_of(subsection.publishable_entity_id, publish_log_id) + subsection_pub_entity_version = publishing_api.get_published_version_as_of( + subsection.publishable_entity_id, publish_log_id + ) if subsection_pub_entity_version is None: return None # This subsection was not published as of the given PublishLog ID. container_version = subsection_pub_entity_version.containerversion @@ -303,5 +303,7 @@ def get_units_in_published_subsection_as_of( # This is not optimized. It could be done in one query per subsection rather than one query per unit. pub_entity_version = publishing_api.get_published_version_as_of(row.entity_id, publish_log_id) if pub_entity_version: - entity_list.append(SubsectionListEntry(unit_version=pub_entity_version.containerversion.unitversion, pinned=False)) + entity_list.append( + SubsectionListEntry(unit_version=pub_entity_version.containerversion.unitversion, pinned=False) + ) return entity_list diff --git a/openedx_learning/apps/authoring/subsections/models.py b/openedx_learning/apps/authoring/subsections/models.py index bf5747f8f..8d662ed4e 100644 --- a/openedx_learning/apps/authoring/subsections/models.py +++ b/openedx_learning/apps/authoring/subsections/models.py @@ -1,5 +1,5 @@ """ -Models that implement units +Models that implement subsections """ from django.db import models diff --git a/tests/openedx_learning/apps/authoring/subsections/test_api.py b/tests/openedx_learning/apps/authoring/subsections/test_api.py index b43c733e3..b58c0607c 100644 --- a/tests/openedx_learning/apps/authoring/subsections/test_api.py +++ b/tests/openedx_learning/apps/authoring/subsections/test_api.py @@ -1,6 +1,8 @@ """ -Basic tests for the units API. +Basic tests for the subsections API. """ +from unittest.mock import patch + import ddt # type: ignore[import] import pytest from django.core.exceptions import ValidationError @@ -180,7 +182,7 @@ def test_create_subsection_queries(self): # The exact numbers here aren't too important - this is just to alert us if anything significant changes. with self.assertNumQueries(22): _empty_subsection = self.create_subsection_with_units([]) - with self.assertNumQueries(26): + with self.assertNumQueries(27): # And try with a non-empty subsection: self.create_subsection_with_units([self.unit_1, self.unit_2_v1], key="u2") @@ -243,7 +245,28 @@ def test_adding_external_units(self): created_by=None, ) - # ?FOR REVIEW + @patch('openedx_learning.apps.authoring.subsections.api._pub_entities_for_units') + def test_adding_mismatched_versions(self, mock_entities_for_units): + """ + Test that versioned units must match their entities. + """ + mock_entities_for_units.return_value = [ + authoring_api.ContainerEntityRow( + entity_pk=self.unit_1.pk, + version_pk=self.unit_2_v1.pk, + ), + ] + # Try adding a a unit from LP 1 (self.learning_package) to a subsection from LP 2 + with pytest.raises(ValidationError, match="Container entity versions must belong to the specified entity"): + authoring_api.create_subsection_and_version( + learning_package_id=self.unit_1.container.publishable_entity.learning_package.pk, + key="subsection:key", + title="Subsection", + units=[self.unit_1], + created=self.now, + created_by=None, + ) + @ddt.data(True, False) @pytest.mark.skip(reason="FIXME: This test is failing because the publishable_entity is not deleted from the database with the unit.") # FIXME: Also, exception is Container.DoesNotExist, not Unit.DoesNotExist @@ -360,7 +383,9 @@ def test_auto_publish_children(self): # Publish ONLY the subsection. This should however also auto-publish units 1 & 2 since they're children authoring_api.publish_from_drafts( self.learning_package.pk, - draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter(entity=subsection.publishable_entity), + draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter( + entity=subsection.publishable_entity + ), ) # Now all changes to the subsection and to unit 1 are published: subsection.refresh_from_db() @@ -976,7 +1001,9 @@ def test_subsections_containing(self): with self.assertNumQueries(1): result2 = [ c.subsection for c in - authoring_api.get_containers_with_entity(self.unit_1.pk, ignore_pinned=True).select_related("subsection") + authoring_api.get_containers_with_entity( + self.unit_1.pk, ignore_pinned=True + ).select_related("subsection") ] assert result2 == [subsection4_unpinned] From 9e5d6406b671a37dbf98d59c791455c8ef022c7c Mon Sep 17 00:00:00 2001 From: Andrii Date: Thu, 17 Apr 2025 13:44:27 +0300 Subject: [PATCH 4/7] style: fix lint errors --- .../subsections/migrations/0001_initial.py | 2 +- .../apps/authoring/subsections/test_api.py | 40 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py b/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py index 62e2e473d..710c5532b 100644 --- a/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/subsections/migrations/0001_initial.py @@ -1,7 +1,7 @@ # Generated by Django 4.2.19 on 2025-04-09 12:59 -from django.db import migrations, models import django.db.models.deletion +from django.db import migrations, models class Migration(migrations.Migration): diff --git a/tests/openedx_learning/apps/authoring/subsections/test_api.py b/tests/openedx_learning/apps/authoring/subsections/test_api.py index b58c0607c..6c1e362c5 100644 --- a/tests/openedx_learning/apps/authoring/subsections/test_api.py +++ b/tests/openedx_learning/apps/authoring/subsections/test_api.py @@ -17,7 +17,7 @@ @ddt.ddt -class SubSectionTestCase(UnitTestCase): +class SubSectionTestCase(UnitTestCase): # pylint: disable=test-inherits-tests """ Test cases for Subsections (containers of units) """ def setUp(self) -> None: @@ -246,7 +246,7 @@ def test_adding_external_units(self): ) @patch('openedx_learning.apps.authoring.subsections.api._pub_entities_for_units') - def test_adding_mismatched_versions(self, mock_entities_for_units): + def test_adding_mismatched_versions(self, mock_entities_for_units): # pylint: disable=arguments-renamed """ Test that versioned units must match their entities. """ @@ -268,7 +268,7 @@ def test_adding_mismatched_versions(self, mock_entities_for_units): ) @ddt.data(True, False) - @pytest.mark.skip(reason="FIXME: This test is failing because the publishable_entity is not deleted from the database with the unit.") + @pytest.mark.skip(reason="FIXME: publishable_entity is not deleted from the database with the unit.") # FIXME: Also, exception is Container.DoesNotExist, not Unit.DoesNotExist def test_cannot_add_invalid_ids(self, pin_version): """ @@ -438,7 +438,7 @@ def test_add_unit_after_publish(self): # Publish the empty subsection: authoring_api.publish_all_drafts(self.learning_package.id) subsection.refresh_from_db() # Reloading the subsection is necessary - assert subsection.versioning.has_unpublished_changes is False # Shallow check for just the subsection itself, not children + assert subsection.versioning.has_unpublished_changes is False # Shallow check for subsection only, not children assert authoring_api.contains_unpublished_changes(subsection.pk) is False # Deeper check # Add a published unit (unpinned): @@ -453,7 +453,7 @@ def test_add_unit_after_publish(self): ) # Now the subsection should have unpublished changes: subsection.refresh_from_db() # Reloading the subsection is necessary - assert subsection.versioning.has_unpublished_changes # Shallow check - adding a child is a change to the subsection + assert subsection.versioning.has_unpublished_changes # Shallow check: adding a child changes the subsection assert authoring_api.contains_unpublished_changes(subsection.pk) # Deeper check assert subsection.versioning.draft == subsection_version_v2 assert subsection.versioning.published == subsection_version @@ -482,9 +482,9 @@ def test_modify_unpinned_unit_after_publish(self): unit_1_v2 = self.modify_unit(self.unit_1, title="Modified Counting Problem with new title") # The unit now has unpublished changes; the subsection doesn't directly but does contain - subsection.refresh_from_db() # Reloading the subsection is necessary, or 'subsection.versioning' will be outdated + subsection.refresh_from_db() # Refresh to avoid stale 'versioning' cache self.unit_1.refresh_from_db() - assert subsection.versioning.has_unpublished_changes is False # Shallow check should be false - subsection is unchanged + assert subsection.versioning.has_unpublished_changes is False # Shallow check: subsection unchanged assert authoring_api.contains_unpublished_changes(subsection.pk) # But subsection DOES contain changes assert self.unit_1.versioning.has_unpublished_changes @@ -504,7 +504,7 @@ def test_modify_unpinned_unit_after_publish(self): assert authoring_api.get_units_in_subsection(subsection, published=True) == [ Entry(unit_1_v2), # new version ] - assert authoring_api.contains_unpublished_changes(subsection.pk) is False # No longer contains unpublished changes + assert authoring_api.contains_unpublished_changes(subsection.pk) is False # No more unpublished changes def test_modify_pinned_unit(self): """ @@ -526,7 +526,7 @@ def test_modify_pinned_unit(self): self.modify_unit(self.unit_1, title="Modified Counting Problem with new title") # The unit now has unpublished changes; the subsection is entirely unaffected - subsection.refresh_from_db() # Reloading the subsection is necessary, or 'subsection.versioning' will be outdated + subsection.refresh_from_db() # Refresh to avoid stale 'versioning' cache self.unit_1.refresh_from_db() assert subsection.versioning.has_unpublished_changes is False # Shallow check assert authoring_api.contains_unpublished_changes(subsection.pk) is False # Deep check @@ -610,7 +610,7 @@ def test_publishing_shared_unit(self): # 2️⃣ Then the author edits U2 inside of Subsection 1 making U2v2. u2_v2 = self.modify_unit(u2, title="U2 version 2") - # This makes S1 and S2 both show up as Subsections that CONTAIN unpublished changes, because they share the unit. + # Both S1 and S2 now contain unpublished changes since they share the unit. assert authoring_api.contains_unpublished_changes(subsection1.pk) assert authoring_api.contains_unpublished_changes(subsection2.pk) # (But the subsections themselves are unchanged:) @@ -647,7 +647,7 @@ def test_publishing_shared_unit(self): Entry(u5_v1), # still original version of U5 (it hasn't been published) ] - # Result: Subsection 2 CONTAINS unpublished changes because of the modified U5. Subsection 1 doesn't contain unpub changes. + # Result: Subsection 2 contains unpublished changes due to modified U5; Subsection 1 does not. assert authoring_api.contains_unpublished_changes(subsection1.pk) is False assert authoring_api.contains_unpublished_changes(subsection2.pk) @@ -781,8 +781,8 @@ def test_soft_deleting_unit(self): # subsection's unit list but has been soft deleted, and will be fully deleted when published, or restored if # reverted? ] - assert subsection.versioning.has_unpublished_changes is False # The subsection itself and its unit list is not changed - assert authoring_api.contains_unpublished_changes(subsection.pk) # But it CONTAINS an unpublished change (a deletion) + assert subsection.versioning.has_unpublished_changes is False # Subsection and unit list unchanged + assert authoring_api.contains_unpublished_changes(subsection.pk) # It still contains an unpublished deletion # The published version of the subsection is not yet affected: assert authoring_api.get_units_in_subsection(subsection, published=True) == [ Entry(self.unit_1_v1), @@ -844,7 +844,7 @@ def test_soft_deleting_pinned_unit(self): Entry(self.unit_1_v1, pinned=True), Entry(self.unit_2_v1, pinned=True), ] - assert subsection.versioning.has_unpublished_changes is False # The subsection itself and its unit list is not changed + assert subsection.versioning.has_unpublished_changes is False # Subsection and unit list unchanged assert authoring_api.contains_unpublished_changes(subsection.pk) is False # nor does it contain changes # The published version of the subsection is also not affected: assert authoring_api.get_units_in_subsection(subsection, published=True) == [ @@ -867,7 +867,7 @@ def test_soft_delete_subsection(self): # Delete the subsection: authoring_api.soft_delete_draft(subsection_to_delete.publishable_entity_id) subsection_to_delete.refresh_from_db() - # Now the draft subsection is [soft] deleted, but the units, published subsection, and other subsection is unaffected: + # Now the draft subsection is soft deleted; units, published subsection, and other subsection are unaffected: assert subsection_to_delete.versioning.draft is None # Subsection is soft deleted. assert subsection_to_delete.versioning.published is not None self.unit_1.refresh_from_db() @@ -1096,8 +1096,8 @@ def test_get_container_children_count(self): assert authoring_api.get_container_children_count(subsection.container, published=True) == 1 # Tests TODO: - # Test that I can get a [PublishLog] history of a given subsection and all its children, including children that aren't - # currently in the subsection and excluding children that are only in other subsections. - # Test that I can get a [PublishLog] history of a given subsection and its children, that includes changes made to the - # child units while they were part of the subsection but excludes changes made to those children while they were - # not part of the subsection. 🫣 + # Test that I can get a [PublishLog] history of a given subsection and all its children, including children + # that aren't currently in the subsection and excluding children that are only in other subsections. + # Test that I can get a [PublishLog] history of a given subsection and its children, that includes changes + # made to the child units while they were part of the subsection but excludes changes + # made to those children while they were not part of the subsection. 🫣 From e05267f6b1aec82c08ed4ecec31ab4d8ea6f89ea Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 18 Apr 2025 09:56:04 +0300 Subject: [PATCH 5/7] test: adapt queries check to new changes --- tests/openedx_learning/apps/authoring/subsections/test_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/subsections/test_api.py b/tests/openedx_learning/apps/authoring/subsections/test_api.py index 6c1e362c5..311bfb9bf 100644 --- a/tests/openedx_learning/apps/authoring/subsections/test_api.py +++ b/tests/openedx_learning/apps/authoring/subsections/test_api.py @@ -180,9 +180,9 @@ def test_create_subsection_queries(self): Test how many database queries are required to create a subsection """ # The exact numbers here aren't too important - this is just to alert us if anything significant changes. - with self.assertNumQueries(22): + with self.assertNumQueries(25): _empty_subsection = self.create_subsection_with_units([]) - with self.assertNumQueries(27): + with self.assertNumQueries(30): # And try with a non-empty subsection: self.create_subsection_with_units([self.unit_1, self.unit_2_v1], key="u2") From 498e5cdee05390a4289dc7624b42b306ae4b3e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=86=D0=B2=D0=B0=D0=BD=20=D0=9D=D1=94=D0=B4=D1=94=D0=BB?= =?UTF-8?q?=D1=8C=D0=BD=D1=96=D1=86=D0=B5=D0=B2?= Date: Fri, 18 Apr 2025 19:53:38 +0300 Subject: [PATCH 6/7] style: fix wrong import order --- .../0008_alter_draftchangelogrecord_options_and_more.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx_learning/apps/authoring/publishing/migrations/0008_alter_draftchangelogrecord_options_and_more.py b/openedx_learning/apps/authoring/publishing/migrations/0008_alter_draftchangelogrecord_options_and_more.py index 571c8bdbc..e83bf7202 100644 --- a/openedx_learning/apps/authoring/publishing/migrations/0008_alter_draftchangelogrecord_options_and_more.py +++ b/openedx_learning/apps/authoring/publishing/migrations/0008_alter_draftchangelogrecord_options_and_more.py @@ -1,7 +1,6 @@ # Generated by Django 4.2.20 on 2025-04-17 18:22 - -from django.db import migrations, models import django.db.models.deletion +from django.db import migrations, models class Migration(migrations.Migration): From 7b5cf6eb5d1c33a5db0b1fb12929603d7066d6f4 Mon Sep 17 00:00:00 2001 From: Ivan Niedielnitsev <81557788+NiedielnitsevIvan@users.noreply.github.com> Date: Fri, 18 Apr 2025 19:59:57 +0300 Subject: [PATCH 7/7] chore: add TODO for tests Co-authored-by: Kyle McCormick --- tests/openedx_learning/apps/authoring/subsections/test_api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/openedx_learning/apps/authoring/subsections/test_api.py b/tests/openedx_learning/apps/authoring/subsections/test_api.py index 311bfb9bf..acbaf6713 100644 --- a/tests/openedx_learning/apps/authoring/subsections/test_api.py +++ b/tests/openedx_learning/apps/authoring/subsections/test_api.py @@ -16,6 +16,9 @@ Entry = authoring_api.SubsectionListEntry +# TODO: Turn UnitTestCase into UnitTestMixin and remove the +# test-inherits-tests pylint warning below. +# https://github.com/openedx/openedx-learning/issues/308 @ddt.ddt class SubSectionTestCase(UnitTestCase): # pylint: disable=test-inherits-tests """ Test cases for Subsections (containers of units) """