From d8dabb45b9216babd951db4677bc1bff4476426f Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 11 Apr 2025 15:39:18 +0300 Subject: [PATCH 1/8] refactor: extend annotation_safe_list with new model & bump package version --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 8c1697cd..bda4b183 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.23.1" +__version__ = "0.25.0" From 29c3c6b4876ef8d908b0e6d1c2eccdef2a78bc98 Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 11 Apr 2025 16:06:39 +0300 Subject: [PATCH 2/8] feat: [AXM-1886] implement content library sections --- .annotation_safe_list.yml | 4 + openedx_learning/api/authoring.py | 1 + openedx_learning/api/authoring_models.py | 1 + .../apps/authoring/sections/__init__.py | 0 .../apps/authoring/sections/api.py | 307 +++++ .../apps/authoring/sections/apps.py | 25 + .../sections/migrations/0001_initial.py | 36 + .../authoring/sections/migrations/__init__.py | 0 .../apps/authoring/sections/models.py | 50 + projects/dev.py | 1 + test_settings.py | 1 + .../apps/authoring/sections/__init__.py | 0 .../apps/authoring/sections/test_api.py | 1082 +++++++++++++++++ 13 files changed, 1508 insertions(+) create mode 100644 openedx_learning/apps/authoring/sections/__init__.py create mode 100644 openedx_learning/apps/authoring/sections/api.py create mode 100644 openedx_learning/apps/authoring/sections/apps.py create mode 100644 openedx_learning/apps/authoring/sections/migrations/0001_initial.py create mode 100644 openedx_learning/apps/authoring/sections/migrations/__init__.py create mode 100644 openedx_learning/apps/authoring/sections/models.py create mode 100644 tests/openedx_learning/apps/authoring/sections/__init__.py create mode 100644 tests/openedx_learning/apps/authoring/sections/test_api.py diff --git a/.annotation_safe_list.yml b/.annotation_safe_list.yml index 91eba706..54d8341f 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_sections.Section: + ".. no_pii:": "This model has no PII" +oel_sections.SectionVersion: + ".. no_pii:": "This model has no PII" oel_subsections.Subsection: ".. no_pii:": "This model has no PII" oel_subsections.SubsectionVersion: diff --git a/openedx_learning/api/authoring.py b/openedx_learning/api/authoring.py index 900713ec..5db14cef 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.sections.api import * from ..apps.authoring.subsections.api import * from ..apps.authoring.units.api import * diff --git a/openedx_learning/api/authoring_models.py b/openedx_learning/api/authoring_models.py index f7fd08de..617d85dc 100644 --- a/openedx_learning/api/authoring_models.py +++ b/openedx_learning/api/authoring_models.py @@ -11,5 +11,6 @@ from ..apps.authoring.components.models import * from ..apps.authoring.contents.models import * from ..apps.authoring.publishing.models import * +from ..apps.authoring.sections.models import * from ..apps.authoring.subsections.models import * from ..apps.authoring.units.models import * diff --git a/openedx_learning/apps/authoring/sections/__init__.py b/openedx_learning/apps/authoring/sections/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/openedx_learning/apps/authoring/sections/api.py b/openedx_learning/apps/authoring/sections/api.py new file mode 100644 index 00000000..bbde80a4 --- /dev/null +++ b/openedx_learning/apps/authoring/sections/api.py @@ -0,0 +1,307 @@ +"""Sections API. + +This module provides functions to manage sections. +""" +from dataclasses import dataclass +from datetime import datetime + +from django.db.transaction import atomic + +from openedx_learning.apps.authoring.subsections.models import Subsection, SubsectionVersion + +from ..publishing import api as publishing_api +from .models import Section, SectionVersion + +# 🛑 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_section", + "create_section_version", + "create_next_section_version", + "create_section_and_version", + "get_section", + "get_section_version", + "get_latest_section_version", + "SectionListEntry", + "get_subsections_in_section", + "get_subsections_in_section", + "get_subsections_in_published_section_as_of", +] + + +def create_section( + learning_package_id: int, + key: str, + created: datetime, + created_by: int | None, + *, + can_stand_alone: bool = True, +) -> Section: + """ + [ 🛑 UNSTABLE ] Create a new section. + + Args: + learning_package_id: The learning package ID. + key: The key. + created: The creation date. + created_by: The user who created the section. + 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=Section, + ) + + +def create_section_version( + section: Section, + version_num: int, + *, + title: str, + publishable_entities_pks: list[int], + entity_version_pks: list[int | None], + created: datetime, + created_by: int | None = None, +) -> SectionVersion: + """ + [ 🛑 UNSTABLE ] Create a new section version. + + This is a very low-level API, likely only needed for import/export. In + general, you will use `create_section_and_version()` and + `create_next_section_version()` instead. + + Args: + section_pk: The section 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 section. + """ + return publishing_api.create_container_version( + section.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=SectionVersion, + ) + + +def _pub_entities_for_subsections( + subsections: list[Subsection | SubsectionVersion] | None, +) -> tuple[list[int], list[int | None]] | tuple[None, None]: + """ + Helper method: given a list of Subsection | SubsectionVersion, return the + lists of publishable_entities_pks and entity_version_pks needed for the + base container APIs. + + SubsectionVersion is passed when we want to pin a specific version, otherwise + Subsection is used for unpinned. + """ + if subsections is None: + # When these are None, that means don't change the entities in the list. + return None, None + for u in subsections: + if not isinstance(u, (Subsection, SubsectionVersion)): + raise TypeError("Section subsections must be either Subsection or SubsectionVersion.") + publishable_entities_pks = [ + (u.publishable_entity_id if isinstance(u, Subsection) else u.subsection.publishable_entity_id) + for u in subsections + ] + entity_version_pks = [ + (uv.pk if isinstance(uv, SubsectionVersion) else None) + for uv in subsections + ] + return publishable_entities_pks, entity_version_pks + + +def create_next_section_version( + section: Section, + *, + title: str | None = None, + subsections: list[Subsection | SubsectionVersion] | None = None, + created: datetime, + created_by: int | None = None, + entities_action: publishing_api.ChildrenEntitiesAction = publishing_api.ChildrenEntitiesAction.REPLACE, +) -> SectionVersion: + """ + [ 🛑 UNSTABLE ] Create the next section version. + + Args: + section_pk: The section ID. + title: The title. Leave as None to keep the current title. + subsections: The subsections, as a list of Subsections (unpinned) and/or SubsectionVersions (pinned). Passing None + will leave the existing subsections unchanged. + created: The creation date. + created_by: The user who created the section. + """ + publishable_entities_pks, entity_version_pks = _pub_entities_for_subsections(subsections) + section_version = publishing_api.create_next_container_version( + section.pk, + title=title, + publishable_entities_pks=publishable_entities_pks, + entity_version_pks=entity_version_pks, + created=created, + created_by=created_by, + container_version_cls=SectionVersion, + entities_action=entities_action, + ) + return section_version + + +def create_section_and_version( + learning_package_id: int, + key: str, + *, + title: str, + subsections: list[Subsection | SubsectionVersion] | None = None, + created: datetime, + created_by: int | None = None, + can_stand_alone: bool = True, +) -> tuple[Section, SectionVersion]: + """ + [ 🛑 UNSTABLE ] Create a new section 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 section. + can_stand_alone: Set to False when created as part of containers + """ + publishable_entities_pks, entity_version_pks = _pub_entities_for_subsections(subsections) + with atomic(): + section = create_section( + learning_package_id, + key, + created, + created_by, + can_stand_alone=can_stand_alone, + ) + section_version = create_section_version( + section, + 1, + title=title, + publishable_entities_pks=publishable_entities_pks or [], + entity_version_pks=entity_version_pks or [], + created=created, + created_by=created_by, + ) + return section, section_version + + +def get_section(section_pk: int) -> Section: + """ + [ 🛑 UNSTABLE ] Get a section. + + Args: + section_pk: The section ID. + """ + return Section.objects.get(pk=section_pk) + + +def get_section_version(section_version_pk: int) -> SectionVersion: + """ + [ 🛑 UNSTABLE ] Get a section version. + + Args: + section_version_pk: The section version ID. + """ + return SectionVersion.objects.get(pk=section_version_pk) + + +def get_latest_section_version(section_pk: int) -> SectionVersion: + """ + [ 🛑 UNSTABLE ] Get the latest section version. + + Args: + section_pk: The section ID. + """ + return Section.objects.get(pk=section_pk).versioning.latest + + +@dataclass(frozen=True) +class SectionListEntry: + """ + [ 🛑 UNSTABLE ] + Data about a single entity in a container, e.g. a subsection in a section. + """ + subsection_version: SubsectionVersion + pinned: bool = False + + @property + def subsection(self): + return self.subsection_version.subsection + + +def get_subsections_in_section( + section: Section, + *, + published: bool, +) -> list[SectionListEntry]: + """ + [ 🛑 UNSTABLE ] + Get the list of entities and their versions in the draft or published + version of the given Section. + + Args: + section: The Section, e.g. returned by `get_section()` + published: `True` if we want the published version of the section, or + `False` for the draft version. + """ + assert isinstance(section, Section) + subsections = [] + for entry in publishing_api.get_entities_in_container(section, published=published): + # Convert from generic PublishableEntityVersion to SubsectionVersion: + subsection_version = entry.entity_version.containerversion.subsectionversion + assert isinstance(subsection_version, SubsectionVersion) + subsections.append(SectionListEntry(subsection_version=subsection_version, pinned=entry.pinned)) + return subsections + + +def get_subsections_in_published_section_as_of( + section: Section, + publish_log_id: int, +) -> list[SectionListEntry] | 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 SectionVersion so we can + see the section 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 sections 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(section, Section) + section_pub_entity_version = publishing_api.get_published_version_as_of(section.publishable_entity_id, publish_log_id) + if section_pub_entity_version is None: + return None # This section was not published as of the given PublishLog ID. + container_version = section_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: + subsection_version = row.entity_version.containerversion.subsectionversion + assert isinstance(subsection_version, SubsectionVersion) + entity_list.append(SectionListEntry(subsection_version=subsection_version, pinned=True)) + else: + # Unpinned subsection - figure out what its latest published version was. + # This is not optimized. It could be done in one query per section rather than one query per subsection. + pub_entity_version = publishing_api.get_published_version_as_of(row.entity_id, publish_log_id) + if pub_entity_version: + entity_list.append(SectionListEntry(subsection_version=pub_entity_version.containerversion.subsectionversion, pinned=False)) + return entity_list diff --git a/openedx_learning/apps/authoring/sections/apps.py b/openedx_learning/apps/authoring/sections/apps.py new file mode 100644 index 00000000..ce7b8a2a --- /dev/null +++ b/openedx_learning/apps/authoring/sections/apps.py @@ -0,0 +1,25 @@ +""" +Subsection Django application initialization. +""" + +from django.apps import AppConfig + + +class SectionsConfig(AppConfig): + """ + Configuration for the subsections Django application. + """ + + name = "openedx_learning.apps.authoring.sections" + verbose_name = "Learning Core > Authoring > Sections" + default_auto_field = "django.db.models.BigAutoField" + label = "oel_sections" + + def ready(self): + """ + Register Subsection and SubsectionVersion. + """ + from ..publishing.api import register_content_models # pylint: disable=import-outside-toplevel + from .models import Section, SectionVersion # pylint: disable=import-outside-toplevel + + register_content_models(Section, SectionVersion) diff --git a/openedx_learning/apps/authoring/sections/migrations/0001_initial.py b/openedx_learning/apps/authoring/sections/migrations/0001_initial.py new file mode 100644 index 00000000..6a9f7ef3 --- /dev/null +++ b/openedx_learning/apps/authoring/sections/migrations/0001_initial.py @@ -0,0 +1,36 @@ +# Generated by Django 4.2.20 on 2025-04-11 12:53 + +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='Section', + 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='SectionVersion', + 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/sections/migrations/__init__.py b/openedx_learning/apps/authoring/sections/migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/openedx_learning/apps/authoring/sections/models.py b/openedx_learning/apps/authoring/sections/models.py new file mode 100644 index 00000000..8fa0793d --- /dev/null +++ b/openedx_learning/apps/authoring/sections/models.py @@ -0,0 +1,50 @@ +""" +Models that implement units +""" +from django.db import models + +from ..publishing.models import Container, ContainerVersion + +__all__ = [ + "Section", + "SectionVersion", +] + + +class Section(Container): + """ + A Section is type of Container that holds Units. + + Via Container and its PublishableEntityMixin, Sections 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 SectionVersion(ContainerVersion): + """ + A SectionVersion is a specific version of a Section. + + Via ContainerVersion and its EntityList, it defines the list of Units + in this version of the Section. + """ + container_version = models.OneToOneField( + ContainerVersion, + on_delete=models.CASCADE, + parent_link=True, + primary_key=True, + ) + + @property + def section(self): + """ Convenience accessor to the Section this version is associated with """ + return self.container_version.container.section # 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_sections 0001_initial' diff --git a/projects/dev.py b/projects/dev.py index cb86f801..41bd7ec5 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.sections.apps.SectionsConfig", "openedx_learning.apps.authoring.subsections.apps.SubsectionsConfig", "openedx_learning.apps.authoring.units.apps.UnitsConfig", # Learning Contrib Apps diff --git a/test_settings.py b/test_settings.py index 0d4a9177..e1e4d79a 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.sections.apps.SectionsConfig", "openedx_learning.apps.authoring.subsections.apps.SubsectionsConfig", "openedx_learning.apps.authoring.units.apps.UnitsConfig", ] diff --git a/tests/openedx_learning/apps/authoring/sections/__init__.py b/tests/openedx_learning/apps/authoring/sections/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/openedx_learning/apps/authoring/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py new file mode 100644 index 00000000..806e2452 --- /dev/null +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -0,0 +1,1082 @@ +""" +Basic tests for the subsections 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 ..subsections.test_api import SubSectionTestCase + +Entry = authoring_api.SectionListEntry + + +@ddt.ddt +class SubSectionTestCase(SubSectionTestCase): + """ Test cases for Sections (containers of subsections) """ + + def setUp(self) -> None: + super().setUp() + self.subsection_1, self.subsection_1_v1 = self.create_subsection( + key="Subsection (1)", + title="Subsection (1)", + ) + self.subsection_2, self.subsection_2_v1 = self.create_subsection( + key="Subsection (2)", + title="Subsection (2)", + ) + + def create_subsection(self, *, title: str = "Test Subsection", key: str = "subsection:1") -> tuple[ + authoring_models.Subsection, authoring_models.SubsectionVersion + ]: + """ Helper method to quickly create a subsection """ + return authoring_api.create_subsection_and_version( + self.learning_package.id, + key=key, + title=title, + created=self.now, + created_by=None, + ) + + def create_section_with_subsections( + self, + subsections: list[authoring_models.Subsection | authoring_models.SubsectionVersion], + *, + title="Subsection", + key="subsection:key", + ) -> authoring_models.Section: + """ Helper method to quickly create a section with some subsections """ + section, _section_v1 = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key=key, + title=title, + subsections=subsections, + created=self.now, + created_by=None, + ) + return section + + def modify_subsection( + self, + subsection: authoring_models.Subsection, + *, + title="Modified Subsection", + timestamp=None, + ) -> authoring_models.SubsectionVersion: + """ + Helper method to modify a subsection for the purposes of testing subsections/drafts/pinning/publishing/etc. + """ + return authoring_api.create_next_subsection_version( + subsection, + title=title, + created=timestamp or self.now, + created_by=None, + ) + + def publish_subsection(self, subsection: authoring_models.Subsection): + """ + Helper method to publish a single subsection. + """ + 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, + ), + ) + + def test_get_section(self): + """ + Test get_section() + """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + with self.assertNumQueries(1): + result = authoring_api.get_section(section.pk) + assert result == section + # Versioning data should be pre-loaded via select_related() + with self.assertNumQueries(0): + assert result.versioning.has_unpublished_changes + + def test_get_section_version(self): + """ + Test get_section_version() + """ + section = self.create_section_with_subsections([]) + draft = section.versioning.draft + with self.assertNumQueries(1): + result = authoring_api.get_section_version(draft.pk) + assert result == draft + + def test_get_latest_section_version(self): + """ + Test test_get_latest_section_version() + """ + section = self.create_section_with_subsections([]) + draft = section.versioning.draft + with self.assertNumQueries(2): + result = authoring_api.get_latest_section_version(section.pk) + assert result == draft + + def test_get_containers(self): + """ + Test get_containers() + """ + section = self.create_section_with_subsections([]) + with self.assertNumQueries(1): + result = list(authoring_api.get_containers(self.learning_package.id)) + assert result == [ + self.unit_1.container, + self.unit_2.container, + self.subsection_1.container, + self.subsection_2.container, + section.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() + """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + with self.assertNumQueries(1): + result = authoring_api.get_container(section.pk) + assert result == section.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() + """ + section = self.create_section_with_subsections([]) + with self.assertNumQueries(1): + result = authoring_api.get_container_by_key( + self.learning_package.id, + key=section.publishable_entity.key, + ) + assert result == section.container + # Versioning data should be pre-loaded via select_related() + with self.assertNumQueries(0): + assert result.versioning.has_unpublished_changes + + def test_section_container_versioning(self): + """ + Test that the .versioning helper of a Sebsection returns a SectionVersion, and + same for the generic Container equivalent. + """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + container = section.container + container_version = container.versioning.draft + assert isinstance(container_version, authoring_models.ContainerVersion) + section_version = section.versioning.draft + assert isinstance(section_version, authoring_models.SectionVersion) + assert section_version.container_version == container_version + assert section_version.container_version.container == container + assert section_version.section == section + + def test_create_section_queries(self): + """ + Test how many database queries are required to create a section + """ + # The exact numbers here aren't too important - this is just to alert us if anything significant changes. + with self.assertNumQueries(22): + _empty_section = self.create_section_with_subsections([]) + with self.assertNumQueries(26): + # And try with a non-empty section: + self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1], key="u2") + + def test_create_section_with_invalid_children(self): + """ + Verify that only subsections can be added to sections, and a specific + exception is raised. + """ + # Create two sections: + section, section_version = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key", + title="Section", + created=self.now, + created_by=None, + ) + assert section.versioning.draft == section_version + section2, _s2v1 = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key2", + title="Section 2", + created=self.now, + created_by=None, + ) + # Try adding a Section to a Section + with pytest.raises(TypeError, match="Section subsections must be either Subsection or SubsectionVersion."): + authoring_api.create_next_section_version( + section=section, + title="Section Containing a Section", + subsections=[section2], + created=self.now, + created_by=None, + ) + # Check that a new version was not created: + section.refresh_from_db() + assert authoring_api.get_section(section.pk).versioning.draft == section_version + assert section.versioning.draft == section_version + + def test_adding_external_subsections(self): + """ + Test that subsections from another learning package cannot be added to a + section. + """ + learning_package2 = authoring_api.create_learning_package(key="other-package", title="Other Package") + section, _section_version = authoring_api.create_section_and_version( + learning_package_id=learning_package2.pk, + key="section:key", + title="Section", + created=self.now, + created_by=None, + ) + assert self.subsection_1.container.publishable_entity.learning_package != learning_package2 + # Try adding a a subsection from LP 1 (self.learning_package) to a section from LP 2 + with pytest.raises(ValidationError, match="Container entities must be from the same learning package."): + authoring_api.create_next_section_version( + section=section, + title="Section Containing an External Subsection", + subsections=[self.subsection_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 subsection.") + # FIXME: Also, exception is Container.DoesNotExist, not Subsection.DoesNotExist + def test_cannot_add_invalid_ids(self, pin_version): + """ + Test that non-existent subsections cannot be added to sections + """ + self.subsection_1.delete() + if pin_version: + subsections = [self.subsection_1_v1] + else: + subsections = [self.subsection_1] + with pytest.raises((IntegrityError, authoring_models.Subsection.DoesNotExist)): + self.create_section_with_subsections(subsections) + + def test_create_empty_section_and_version(self): + """Test creating a section with no subsections. + + Expected results: + 1. A section and section version are created. + 2. The section version number is 1. + 3. The section is a draft with unpublished changes. + 4. There is no published version of the section. + """ + section, section_version = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key", + title="Section", + created=self.now, + created_by=None, + ) + assert section, section_version + assert section_version.version_num == 1 + assert section_version in section.versioning.versions.all() + assert section.versioning.has_unpublished_changes + assert section.versioning.draft == section_version + assert section.versioning.published is None + assert section.publishable_entity.can_stand_alone + + def test_create_next_section_version_with_two_unpinned_subsections(self): + """Test creating a section version with two unpinned subsections. + + Expected results: + 1. A new section version is created. + 2. The section version number is 2. + 3. The section version is in the section's versions. + 4. The subsections are in the draft section version's subsection list and are unpinned. + """ + section, _section_version = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key", + title="Section", + created=self.now, + created_by=None, + ) + section_version_v2 = authoring_api.create_next_section_version( + section=section, + title="Section", + subsections=[self.subsection_1, self.subsection_2], + created=self.now, + created_by=None, + ) + assert section_version_v2.version_num == 2 + assert section_version_v2 in section.versioning.versions.all() + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1.versioning.draft), + Entry(self.subsection_2.versioning.draft), + ] + with pytest.raises(authoring_models.ContainerVersion.DoesNotExist): + # There is no published version of the section: + authoring_api.get_subsections_in_section(section, published=True) + + def test_create_next_section_version_with_unpinned_and_pinned_subsections(self): + """ + Test creating a section version with one unpinned and one pinned 📌 subsection. + """ + section, _section_version = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key", + title="Section", + created=self.now, + created_by=None, + ) + section_version_v2 = authoring_api.create_next_section_version( + section=section, + title="Section", + subsections=[self.subsection_1, self.subsection_2_v1], # Note the "v1" pinning 📌 the second one to version 1 + created=self.now, + created_by=None, + ) + assert section_version_v2.version_num == 2 + assert section_version_v2 in section.versioning.versions.all() + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1_v1), + Entry(self.subsection_2_v1, pinned=True), # Pinned 📌 to v1 + ] + with pytest.raises(authoring_models.ContainerVersion.DoesNotExist): + # There is no published version of the section: + authoring_api.get_subsections_in_section(section, published=True) + + def test_auto_publish_children(self): + """ + Test that publishing a section publishes its child subsections automatically. + """ + # Create a draft section with two draft subsections + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + # Also create another subsection that's not in the section at all: + other_subsection, _ou_v1 = self.create_subsection(title="A draft subsection not in the section", key="subsection:3") + + assert authoring_api.contains_unpublished_changes(section.pk) + assert self.subsection_1.versioning.published is None + assert self.subsection_2.versioning.published is None + + # Publish ONLY the section. This should however also auto-publish subsections 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=section.publishable_entity), + ) + # Now all changes to the section and to subsection 1 are published: + section.refresh_from_db() + self.subsection_1.refresh_from_db() + assert section.versioning.has_unpublished_changes is False # Shallow check + assert self.subsection_1.versioning.has_unpublished_changes is False + assert authoring_api.contains_unpublished_changes(section.pk) is False # Deep check + assert self.subsection_1.versioning.published == self.subsection_1_v1 # v1 is now the published version. + + # But our other subsection that's outside the section is not affected: + other_subsection.refresh_from_db() + assert other_subsection.versioning.has_unpublished_changes + assert other_subsection.versioning.published is None + + def test_no_publish_parent(self): + """ + Test that publishing a subsection does NOT publish changes to its parent section + """ + # Create a draft section with two draft subsections + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + assert section.versioning.has_unpublished_changes + # Publish ONLY one of its child subsections + self.publish_subsection(self.subsection_1) + self.subsection_1.refresh_from_db() # Clear cache on '.versioning' + assert self.subsection_1.versioning.has_unpublished_changes is False + + # The section that contains that subsection should still be unpublished: + section.refresh_from_db() # Clear cache on '.versioning' + assert section.versioning.has_unpublished_changes + assert section.versioning.published is None + with pytest.raises(authoring_models.ContainerVersion.DoesNotExist): + # There is no published version of the section: + authoring_api.get_subsections_in_section(section, published=True) + + def test_add_subsection_after_publish(self): + """ + Adding a subsection to a published section will create a new version and + show that the section has unpublished changes. + """ + section, section_version = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key", + title="Section", + created=self.now, + created_by=None, + ) + assert section.versioning.draft == section_version + assert section.versioning.published is None + assert section.versioning.has_unpublished_changes + # Publish the empty section: + authoring_api.publish_all_drafts(self.learning_package.id) + section.refresh_from_db() # Reloading the section is necessary + assert section.versioning.has_unpublished_changes is False # Shallow check for just the section itself, not children + assert authoring_api.contains_unpublished_changes(section.pk) is False # Deeper check + + # Add a published subsection (unpinned): + assert self.subsection_1.versioning.has_unpublished_changes is False + section_version_v2 = authoring_api.create_next_section_version( + section=section, + title=section_version.title, + subsections=[self.subsection_1], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.APPEND, + ) + # Now the section should have unpublished changes: + section.refresh_from_db() # Reloading the section is necessary + assert section.versioning.has_unpublished_changes # Shallow check - adding a child is a change to the section + assert authoring_api.contains_unpublished_changes(section.pk) # Deeper check + assert section.versioning.draft == section_version_v2 + assert section.versioning.published == section_version + + def test_modify_unpinned_subsection_after_publish(self): + """ + Modifying an unpinned subsection in a published section will NOT create a + new version nor show that the section has unpublished changes (but it will + "contain" unpublished changes). The modifications will appear in the + published version of the section only after the subsection is published. + """ + # Create a section with one unpinned draft subsection: + assert self.subsection_1.versioning.has_unpublished_changes + section = self.create_section_with_subsections([self.subsection_1]) + assert section.versioning.has_unpublished_changes + + # Publish the section and the subsection: + authoring_api.publish_all_drafts(self.learning_package.id) + section.refresh_from_db() # Reloading the section is necessary if we accessed 'versioning' before publish + self.subsection_1.refresh_from_db() + assert section.versioning.has_unpublished_changes is False # Shallow check + assert authoring_api.contains_unpublished_changes(section.pk) is False # Deeper check + assert self.subsection_1.versioning.has_unpublished_changes is False + + # Now modify the subsection by changing its title (it remains a draft): + subsection_1_v2 = self.modify_subsection(self.subsection_1, title="Modified Counting Problem with new title") + + # The subsection now has unpublished changes; the section doesn't directly but does contain + section.refresh_from_db() # Reloading the section is necessary, or 'section.versioning' will be outdated + self.subsection_1.refresh_from_db() + assert section.versioning.has_unpublished_changes is False # Shallow check should be false - section is unchanged + assert authoring_api.contains_unpublished_changes(section.pk) # But section DOES contain changes + assert self.subsection_1.versioning.has_unpublished_changes + + # Since the subsection changes haven't been published, they should only appear in the draft section + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(subsection_1_v2), # new version + ] + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), # old version + ] + + # But if we publish the subsection, the changes will appear in the published version of the section. + self.publish_subsection(self.subsection_1) + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(subsection_1_v2), # new version + ] + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(subsection_1_v2), # new version + ] + assert authoring_api.contains_unpublished_changes(section.pk) is False # No longer contains unpublished changes + + def test_modify_pinned_subsection(self): + """ + When a pinned 📌 subsection in section is modified and/or published, it will + have no effect on either the draft nor published version of the section, + which will continue to use the pinned version. + """ + # Create a section with one subsection (pinned 📌 to v1): + section = self.create_section_with_subsections([self.subsection_1_v1]) + + # Publish the section and the subsection: + authoring_api.publish_all_drafts(self.learning_package.id) + expected_section_contents = [ + Entry(self.subsection_1_v1, pinned=True), # pinned 📌 to v1 + ] + assert authoring_api.get_subsections_in_section(section, published=True) == expected_section_contents + + # Now modify the subsection by changing its title (it remains a draft): + self.modify_subsection(self.subsection_1, title="Modified Counting Problem with new title") + + # The subsection now has unpublished changes; the section is entirely unaffected + section.refresh_from_db() # Reloading the section is necessary, or 'section.versioning' will be outdated + self.subsection_1.refresh_from_db() + assert section.versioning.has_unpublished_changes is False # Shallow check + assert authoring_api.contains_unpublished_changes(section.pk) is False # Deep check + assert self.subsection_1.versioning.has_unpublished_changes is True + + # Neither the draft nor the published version of the section is affected + assert authoring_api.get_subsections_in_section(section, published=False) == expected_section_contents + assert authoring_api.get_subsections_in_section(section, published=True) == expected_section_contents + # Even if we publish the subsection, the section stays pinned to the specified version: + self.publish_subsection(self.subsection_1) + assert authoring_api.get_subsections_in_section(section, published=False) == expected_section_contents + assert authoring_api.get_subsections_in_section(section, published=True) == expected_section_contents + + def test_create_two_sections_with_same_subsections(self): + """ + Test creating two sections with different combinations of the same two + subsections in each section. + """ + # Create a section with subsection 2 unpinned, subsection 2 pinned 📌, and subsection 1: + section1 = self.create_section_with_subsections([self.subsection_2, self.subsection_2_v1, self.subsection_1], key="u1") + # Create a second section with subsection 1 pinned 📌, subsection 2, and subsection 1 unpinned: + section2 = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2, self.subsection_1], key="u2") + + # Check that the contents are as expected: + assert [row.subsection_version for row in authoring_api.get_subsections_in_section(section1, published=False)] == [ + self.subsection_2_v1, self.subsection_2_v1, self.subsection_1_v1, + ] + assert [row.subsection_version for row in authoring_api.get_subsections_in_section(section2, published=False)] == [ + self.subsection_1_v1, self.subsection_2_v1, self.subsection_1_v1, + ] + + # Modify subsection 1 + subsection_1_v2 = self.modify_subsection(self.subsection_1, title="subsection 1 v2") + # Publish changes + authoring_api.publish_all_drafts(self.learning_package.id) + # Modify subsection 2 - only in the draft + subsection_2_v2 = self.modify_subsection(self.subsection_2, title="subsection 2 DRAFT") + + # Check that the draft contents are as expected: + assert authoring_api.get_subsections_in_section(section1, published=False) == [ + Entry(subsection_2_v2), # v2 in the draft version + Entry(self.subsection_2_v1, pinned=True), # pinned 📌 to v1 + Entry(subsection_1_v2), # v2 + ] + assert authoring_api.get_subsections_in_section(section2, published=False) == [ + Entry(self.subsection_1_v1, pinned=True), # pinned 📌 to v1 + Entry(subsection_2_v2), # v2 in the draft version + Entry(subsection_1_v2), # v2 + ] + + # Check that the published contents are as expected: + assert authoring_api.get_subsections_in_section(section1, published=True) == [ + Entry(self.subsection_2_v1), # v1 in the published version + Entry(self.subsection_2_v1, pinned=True), # pinned 📌 to v1 + Entry(subsection_1_v2), # v2 + ] + assert authoring_api.get_subsections_in_section(section2, published=True) == [ + Entry(self.subsection_1_v1, pinned=True), # pinned 📌 to v1 + Entry(self.subsection_2_v1), # v1 in the published version + Entry(subsection_1_v2), # v2 + ] + + def test_publishing_shared_subsection(self): + """ + A complex test case involving two sections with a shared subsection and + other non-shared subsections. + + Section 1: subsections C1, C2, C3 + Section 2: subsections C2, C4, C5 + Everything is "unpinned". + """ + # 1️⃣ Create the sections and publish them: + (u1, u1_v1), (u2, _u2_v1), (u3, u3_v1), (u4, u4_v1), (u5, u5_v1) = [ + self.create_subsection(key=f"C{i}", title=f"Subsection {i}") for i in range(1, 6) + ] + section1 = self.create_section_with_subsections([u1, u2, u3], title="Section 1", key="section:1") + section2 = self.create_section_with_subsections([u2, u4, u5], title="Section 2", key="section:2") + authoring_api.publish_all_drafts(self.learning_package.id) + assert authoring_api.contains_unpublished_changes(section1.pk) is False + assert authoring_api.contains_unpublished_changes(section2.pk) is False + + # 2️⃣ Then the author edits U2 inside of Section 1 making U2v2. + u2_v2 = self.modify_subsection(u2, title="U2 version 2") + # This makes S1 and S2 both show up as Sections that CONTAIN unpublished changes, because they share the subsection. + assert authoring_api.contains_unpublished_changes(section1.pk) + assert authoring_api.contains_unpublished_changes(section2.pk) + # (But the sections themselves are unchanged:) + section1.refresh_from_db() + section2.refresh_from_db() + assert section1.versioning.has_unpublished_changes is False + assert section2.versioning.has_unpublished_changes is False + + # 3️⃣ In addition to this, the author also modifies another subsection in Section 2 (U5) + u5_v2 = self.modify_subsection(u5, title="U5 version 2") + + # 4️⃣ The author then publishes Section 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 section; the publishing API should auto-publish its subsections too. + entity_id=section1.publishable_entity.id, + ), + ) + + # Result: Section 1 will show the newly published version of U2: + assert authoring_api.get_subsections_in_section(section1, published=True) == [ + Entry(u1_v1), + Entry(u2_v2), # new published version of U2 + Entry(u3_v1), + ] + + # Result: someone looking at Section 2 should see the newly published subsection 2, because publishing it anywhere + # publishes it everywhere. But publishing U2 and Section 1 does not affect the other subsections in Section 2. + # (Publish propagates downward, not upward) + assert authoring_api.get_subsections_in_section(section2, 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: Section 2 CONTAINS unpublished changes because of the modified U5. Section 1 doesn't contain unpub changes. + assert authoring_api.contains_unpublished_changes(section1.pk) is False + assert authoring_api.contains_unpublished_changes(section2.pk) + + # 5️⃣ Publish subsection U5, which should be the only thing unpublished in the learning package + self.publish_subsection(u5) + # Result: Section 2 shows the new version of C5 and no longer contains unpublished changes: + assert authoring_api.get_subsections_in_section(section2, 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(section2.pk) is False + + def test_query_count_of_contains_unpublished_changes(self): + """ + Checking for unpublished changes in a section should require a fixed number + of queries, not get more expensive as the section gets larger. + """ + # Add 2 subsections (unpinned) + subsection_count = 2 + subsections = [] + for i in range(subsection_count): + subsection, _version = self.create_subsection( + key=f"Subsection {i}", + title=f"Subsection {i}", + ) + subsections.append(subsection) + section = self.create_section_with_subsections(subsections) + authoring_api.publish_all_drafts(self.learning_package.id) + section.refresh_from_db() + with self.assertNumQueries(6): + assert authoring_api.contains_unpublished_changes(section.pk) is False + + # Modify the most recently created subsection: + self.modify_subsection(subsection, title="Modified Subsection") + with self.assertNumQueries(5): + assert authoring_api.contains_unpublished_changes(section.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. + """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1]) + + orig_version_num = section.versioning.draft.version_num + orig_entity_list_id = section.versioning.draft.entity_list.pk + + authoring_api.create_next_section_version(section, title="New Title", created=self.now) + + section.refresh_from_db() + new_version_num = section.versioning.draft.version_num + new_entity_list_id = section.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 subsections to sections") + def test_cannot_add_soft_deleted_subsection(self, publish_first): + """ + Test that a soft-deleted subsection cannot be added to a section. + + Although it's valid for sections to contain soft-deleted subsections (by + deleting the subsection after adding it), it is likely a mistake if + you're trying to add one to the section. + """ + subsection, _cv = self.create_subsection(title="Deleted subsection") + if publish_first: + # Publish the subsection: + authoring_api.publish_all_drafts(self.learning_package.id) + # Now delete it. The draft version is now deleted: + authoring_api.soft_delete_draft(subsection.pk) + # Now try adding that subsection to a section: + with pytest.raises(ValidationError, match="subsection is deleted"): + self.create_section_with_subsections([subsection]) + + def test_removing_subsection(self): + """ Test removing a subsection from a section (but not deleting it) """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now remove subsection 2 + authoring_api.create_next_section_version( + section=section, + title="Revised with subsection 2 deleted", + subsections=[self.subsection_2], + created=self.now, + entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, + ) + + # Now it should not be listed in the section: + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1_v1), + ] + section.refresh_from_db() + assert section.versioning.has_unpublished_changes # The section itself and its subsection list have change + assert authoring_api.contains_unpublished_changes(section.pk) + # The published version of the section is not yet affected: + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), + Entry(self.subsection_2_v1), + ] + + # But when we publish the new section version with the removal, the published version is affected: + authoring_api.publish_all_drafts(self.learning_package.id) + # FIXME: Refreshing the section 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). + section.refresh_from_db() + assert authoring_api.contains_unpublished_changes(section.pk) is False + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), + ] + + def test_soft_deleting_subsection(self): + """ Test soft deleting a subsection that's in a section (but not removing it) """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now soft delete subsection 2 + authoring_api.soft_delete_draft(self.subsection_2.pk) + + # Now it should not be listed in the section: + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1_v1), + # subsection 2 is soft deleted from the draft. + # TODO: should we return some kind of placeholder here, to indicate that a subsection is still listed in the + # section's subsection list but has been soft deleted, and will be fully deleted when published, or restored if + # reverted? + ] + assert section.versioning.has_unpublished_changes is False # The section itself and its subsection list is not changed + assert authoring_api.contains_unpublished_changes(section.pk) # But it CONTAINS an unpublished change (a deletion) + # The published version of the section is not yet affected: + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), + Entry(self.subsection_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(section.pk) is False + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), + ] + + def test_soft_deleting_and_removing_subsection(self): + """ Test soft deleting a subsection that's in a section AND removing it """ + section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now soft delete subsection 2 + authoring_api.soft_delete_draft(self.subsection_2.pk) + # And remove it from the section: + authoring_api.create_next_section_version( + section=section, + title="Revised with subsection 2 deleted", + subsections=[self.subsection_2], + created=self.now, + entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, + ) + + # Now it should not be listed in the section: + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1_v1), + ] + assert section.versioning.has_unpublished_changes is True + assert authoring_api.contains_unpublished_changes(section.pk) + # The published version of the section is not yet affected: + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), + Entry(self.subsection_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(section.pk) is False + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1), + ] + + def test_soft_deleting_pinned_subsection(self): + """ Test soft deleting a pinned 📌 subsection that's in a section """ + section = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2_v1]) + authoring_api.publish_all_drafts(self.learning_package.id) + + # Now soft delete subsection 2 + authoring_api.soft_delete_draft(self.subsection_2.pk) + + # Now it should still be listed in the section: + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1_v1, pinned=True), + Entry(self.subsection_2_v1, pinned=True), + ] + assert section.versioning.has_unpublished_changes is False # The section itself and its subsection list is not changed + assert authoring_api.contains_unpublished_changes(section.pk) is False # nor does it contain changes + # The published version of the section is also not affected: + assert authoring_api.get_subsections_in_section(section, published=True) == [ + Entry(self.subsection_1_v1, pinned=True), + Entry(self.subsection_2_v1, pinned=True), + ] + + def test_soft_delete_section(self): + """ + I can delete a section without deleting the subsections it contains. + + See https://github.com/openedx/frontend-app-authoring/issues/1693 + """ + # Create two sections, one of which we will soon delete: + section_to_delete = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) + other_section = self.create_section_with_subsections([self.subsection_1], key="other") + + # Publish everything: + authoring_api.publish_all_drafts(self.learning_package.id) + # Delete the section: + authoring_api.soft_delete_draft(section_to_delete.publishable_entity_id) + section_to_delete.refresh_from_db() + # Now the draft section is [soft] deleted, but the subsections, published section, and other section is unaffected: + assert section_to_delete.versioning.draft is None # Section is soft deleted. + assert section_to_delete.versioning.published is not None + self.subsection_1.refresh_from_db() + assert self.subsection_1.versioning.draft is not None + assert authoring_api.get_subsections_in_section(other_section, published=False) == [Entry(self.subsection_1_v1)] + + # Publish everything: + authoring_api.publish_all_drafts(self.learning_package.id) + # Now the section's published version is also deleted, but nothing else is affected. + section_to_delete.refresh_from_db() + assert section_to_delete.versioning.draft is None # Section is soft deleted. + assert section_to_delete.versioning.published is None + self.subsection_1.refresh_from_db() + assert self.subsection_1.versioning.draft is not None + assert self.subsection_1.versioning.published is not None + assert authoring_api.get_subsections_in_section(other_section, published=False) == [Entry(self.subsection_1_v1)] + assert authoring_api.get_subsections_in_section(other_section, published=True) == [Entry(self.subsection_1_v1)] + + def test_snapshots_of_published_section(self): + """ + Test that we can access snapshots of the historic published version of + sections and their contents. + """ + # At first the section has one subsection (unpinned): + section = self.create_section_with_subsections([self.subsection_1]) + self.modify_subsection(self.subsection_1, title="Subsection 1 as of checkpoint 1") + before_publish = authoring_api.get_subsections_in_published_section_as_of(section, 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 subsection. + self.modify_subsection(self.subsection_1, title="Subsection 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 subsection to the section: + self.modify_subsection(self.subsection_1, title="Subsection 1 as of checkpoint 3") + self.modify_subsection(self.subsection_2, title="Subsection 2 as of checkpoint 3") + authoring_api.create_next_section_version( + section=section, + title="Section title in checkpoint 3", + subsections=[self.subsection_1, self.subsection_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 subsection to the section, a pinned 📌 version of subsection 1. + # This will test pinned versions and also test adding at the beginning rather than the end of the section. + authoring_api.create_next_section_version( + section=section, + title="Section title in checkpoint 4", + subsections=[self.subsection_1_v1, self.subsection_1, self.subsection_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_subsection(self.subsection_1, title="Subsection 1 draft") + self.modify_subsection(self.subsection_2, title="Subsection 2 draft") + + # Now fetch the snapshots: + as_of_checkpoint_1 = authoring_api.get_subsections_in_published_section_as_of(section, checkpoint_1.pk) + assert [cv.subsection_version.title for cv in as_of_checkpoint_1] == [ + "Subsection 1 as of checkpoint 1", + ] + as_of_checkpoint_2 = authoring_api.get_subsections_in_published_section_as_of(section, checkpoint_2.pk) + assert [cv.subsection_version.title for cv in as_of_checkpoint_2] == [ + "Subsection 1 as of checkpoint 2", + ] + as_of_checkpoint_3 = authoring_api.get_subsections_in_published_section_as_of(section, checkpoint_3.pk) + assert [cv.subsection_version.title for cv in as_of_checkpoint_3] == [ + "Subsection 1 as of checkpoint 3", + "Subsection 2 as of checkpoint 3", + ] + as_of_checkpoint_4 = authoring_api.get_subsections_in_published_section_as_of(section, checkpoint_4.pk) + assert [cv.subsection_version.title for cv in as_of_checkpoint_4] == [ + "Subsection (1)", # Pinned. This title is self.subsection_1_v1.title (original v1 title) + "Subsection 1 as of checkpoint 3", # we didn't modify these subsections so they're same as in snapshot 3 + "Subsection 2 as of checkpoint 3", # we didn't modify these subsections so they're same as in snapshot 3 + ] + + def test_sections_containing(self): + """ + Test that we can efficiently get a list of all the [draft] sections + containing a given subsection. + """ + subsection_1_v2 = self.modify_subsection(self.subsection_1, title="modified subsection 1") + + # Create a few sections, some of which contain subsection 1 and others which don't: + # Note: it is important that some of these sections contain other subsections, to ensure the complex JOINs required + # for this query are working correctly, especially in the case of ignore_pinned=True. + # Section 1 ✅ has subsection 1, pinned 📌 to V1 + section1_1pinned = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2], key="u1") + # Section 2 ✅ has subsection 1, pinned 📌 to V2 + section2_1pinned_v2 = self.create_section_with_subsections([subsection_1_v2, self.subsection_2_v1], key="u2") + # Section 3 doesn't contain it + _section3_no = self.create_section_with_subsections([self.subsection_2], key="u3") + # Section 4 ✅ has subsection 1, unpinned + section4_unpinned = self.create_section_with_subsections([ + self.subsection_1, self.subsection_2, self.subsection_2_v1, + ], key="u4") + # Sections 5/6 don't contain it + _section5_no = self.create_section_with_subsections([self.subsection_2_v1, self.subsection_2], key="u5") + _section6_no = self.create_section_with_subsections([], 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.section for c in + authoring_api.get_containers_with_entity(self.subsection_1.pk).select_related("section") + ] + assert result == [ + section1_1pinned, + section2_1pinned_v2, + section4_unpinned, + ] + + # Test retrieving only "unpinned", for cases like potential deletion of a subsection, where we wouldn't care + # about pinned uses anyways (they would be unaffected by a delete). + + with self.assertNumQueries(1): + result2 = [ + c.section for c in + authoring_api.get_containers_with_entity(self.subsection_1.pk, ignore_pinned=True).select_related("section") + ] + assert result2 == [section4_unpinned] + + def test_add_remove_container_children(self): + """ + Test adding and removing children subsections from sections. + """ + section, section_version = authoring_api.create_section_and_version( + learning_package_id=self.learning_package.id, + key="section:key", + title="Section", + subsections=[self.subsection_1], + created=self.now, + created_by=None, + ) + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1.versioning.draft), + ] + subsection_3, _ = self.create_subsection( + key="Subsection (3)", + title="Subsection (3)", + ) + # Add subsection_2 and subsection_3 + section_version_v2 = authoring_api.create_next_section_version( + section=section, + title=section_version.title, + subsections=[self.subsection_2, subsection_3], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.APPEND, + ) + section.refresh_from_db() + assert section_version_v2.version_num == 2 + assert section_version_v2 in section.versioning.versions.all() + # Verify that subsection_2 and subsection_3 is added to end + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_1.versioning.draft), + Entry(self.subsection_2.versioning.draft), + Entry(subsection_3.versioning.draft), + ] + + # Remove subsection_1 + authoring_api.create_next_section_version( + section=section, + title=section_version.title, + subsections=[self.subsection_1], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.REMOVE, + ) + section.refresh_from_db() + # Verify that subsection_1 is removed + assert authoring_api.get_subsections_in_section(section, published=False) == [ + Entry(self.subsection_2.versioning.draft), + Entry(subsection_3.versioning.draft), + ] + + def test_get_container_children_count(self): + """ + Test get_container_children_count() + """ + section = self.create_section_with_subsections([self.subsection_1]) + assert authoring_api.get_container_children_count(section.container, published=False) == 1 + # publish + authoring_api.publish_all_drafts(self.learning_package.id) + section_version = section.versioning.draft + authoring_api.create_next_section_version( + section=section, + title=section_version.title, + subsections=[self.subsection_2], + created=self.now, + created_by=None, + entities_action=authoring_api.ChildrenEntitiesAction.APPEND, + ) + section.refresh_from_db() + # Should have two subsections in draft version and 1 in published version + assert authoring_api.get_container_children_count(section.container, published=False) == 2 + assert authoring_api.get_container_children_count(section.container, published=True) == 1 + # publish + authoring_api.publish_all_drafts(self.learning_package.id) + section.refresh_from_db() + assert authoring_api.get_container_children_count(section.container, published=True) == 2 + # Soft delete subsection_1 + authoring_api.soft_delete_draft(self.subsection_1.pk) + section.refresh_from_db() + # Should contain only 1 child + assert authoring_api.get_container_children_count(section.container, published=False) == 1 + authoring_api.publish_all_drafts(self.learning_package.id) + section.refresh_from_db() + assert authoring_api.get_container_children_count(section.container, published=True) == 1 + + # Tests TODO: + # Test that I can get a [PublishLog] history of a given section and all its children, including children that aren't + # currently in the section and excluding children that are only in other sections. + # Test that I can get a [PublishLog] history of a given section and its children, that includes changes made to the + # child subsections while they were part of the section but excludes changes made to those children while they were + # not part of the section. 🫣 From 55131b3b8a9ff3d92a541a179258f32ec459eb53 Mon Sep 17 00:00:00 2001 From: Andrii Date: Mon, 14 Apr 2025 15:19:41 +0300 Subject: [PATCH 3/8] refactor: [AXM-1886] remove unused tests & fix variable naming --- .../apps/authoring/sections/test_api.py | 80 +++++-------------- 1 file changed, 22 insertions(+), 58 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py index 806e2452..c758c009 100644 --- a/tests/openedx_learning/apps/authoring/sections/test_api.py +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -249,22 +249,6 @@ def test_adding_external_subsections(self): 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 subsection.") - # FIXME: Also, exception is Container.DoesNotExist, not Subsection.DoesNotExist - def test_cannot_add_invalid_ids(self, pin_version): - """ - Test that non-existent subsections cannot be added to sections - """ - self.subsection_1.delete() - if pin_version: - subsections = [self.subsection_1_v1] - else: - subsections = [self.subsection_1] - with pytest.raises((IntegrityError, authoring_models.Subsection.DoesNotExist)): - self.create_section_with_subsections(subsections) - def test_create_empty_section_and_version(self): """Test creating a section with no subsections. @@ -357,7 +341,7 @@ def test_auto_publish_children(self): # Create a draft section with two draft subsections section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) # Also create another subsection that's not in the section at all: - other_subsection, _ou_v1 = self.create_subsection(title="A draft subsection not in the section", key="subsection:3") + other_subsection, _os_v1 = self.create_subsection(title="A draft subsection not in the section", key="subsection:3") assert authoring_api.contains_unpublished_changes(section.pk) assert self.subsection_1.versioning.published is None @@ -580,17 +564,17 @@ def test_publishing_shared_subsection(self): Everything is "unpinned". """ # 1️⃣ Create the sections and publish them: - (u1, u1_v1), (u2, _u2_v1), (u3, u3_v1), (u4, u4_v1), (u5, u5_v1) = [ + (s1, s1_v1), (s2, _s2_v1), (s3, s3_v1), (s4, s4_v1), (s5, s5_v1) = [ self.create_subsection(key=f"C{i}", title=f"Subsection {i}") for i in range(1, 6) ] - section1 = self.create_section_with_subsections([u1, u2, u3], title="Section 1", key="section:1") - section2 = self.create_section_with_subsections([u2, u4, u5], title="Section 2", key="section:2") + section1 = self.create_section_with_subsections([s1, s2, s3], title="Section 1", key="section:1") + section2 = self.create_section_with_subsections([s2, s4, s5], title="Section 2", key="section:2") authoring_api.publish_all_drafts(self.learning_package.id) assert authoring_api.contains_unpublished_changes(section1.pk) is False assert authoring_api.contains_unpublished_changes(section2.pk) is False - # 2️⃣ Then the author edits U2 inside of Section 1 making U2v2. - u2_v2 = self.modify_subsection(u2, title="U2 version 2") + # 2️⃣ Then the author edits S2 inside of Section 1 making S2v2. + s2_v2 = self.modify_subsection(s2, title="U2 version 2") # This makes S1 and S2 both show up as Sections that CONTAIN unpublished changes, because they share the subsection. assert authoring_api.contains_unpublished_changes(section1.pk) assert authoring_api.contains_unpublished_changes(section2.pk) @@ -601,7 +585,7 @@ def test_publishing_shared_subsection(self): assert section2.versioning.has_unpublished_changes is False # 3️⃣ In addition to this, the author also modifies another subsection in Section 2 (U5) - u5_v2 = self.modify_subsection(u5, title="U5 version 2") + s5_v2 = self.modify_subsection(s5, title="S5 version 2") # 4️⃣ The author then publishes Section 1, and therefore everything in it. authoring_api.publish_from_drafts( @@ -614,18 +598,18 @@ def test_publishing_shared_subsection(self): # Result: Section 1 will show the newly published version of U2: assert authoring_api.get_subsections_in_section(section1, published=True) == [ - Entry(u1_v1), - Entry(u2_v2), # new published version of U2 - Entry(u3_v1), + Entry(s1_v1), + Entry(s2_v2), # new published version of U2 + Entry(s3_v1), ] # Result: someone looking at Section 2 should see the newly published subsection 2, because publishing it anywhere # publishes it everywhere. But publishing U2 and Section 1 does not affect the other subsections in Section 2. # (Publish propagates downward, not upward) assert authoring_api.get_subsections_in_section(section2, 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) + Entry(s2_v2), # new published version of U2 + Entry(s4_v1), # still original version of U4 (it was never modified) + Entry(s5_v1), # still original version of U5 (it hasn't been published) ] # Result: Section 2 CONTAINS unpublished changes because of the modified U5. Section 1 doesn't contain unpub changes. @@ -636,9 +620,9 @@ def test_publishing_shared_subsection(self): self.publish_subsection(u5) # Result: Section 2 shows the new version of C5 and no longer contains unpublished changes: assert authoring_api.get_subsections_in_section(section2, 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 + Entry(s2_v2), # new published version of U2 + Entry(s4_v1), # still original version of U4 (it was never modified) + Entry(s5_v2), # new published version of U5 ] assert authoring_api.contains_unpublished_changes(section2.pk) is False @@ -687,26 +671,6 @@ def test_metadata_change_doesnt_create_entity_list(self): 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 subsections to sections") - def test_cannot_add_soft_deleted_subsection(self, publish_first): - """ - Test that a soft-deleted subsection cannot be added to a section. - - Although it's valid for sections to contain soft-deleted subsections (by - deleting the subsection after adding it), it is likely a mistake if - you're trying to add one to the section. - """ - subsection, _cv = self.create_subsection(title="Deleted subsection") - if publish_first: - # Publish the subsection: - authoring_api.publish_all_drafts(self.learning_package.id) - # Now delete it. The draft version is now deleted: - authoring_api.soft_delete_draft(subsection.pk) - # Now try adding that subsection to a section: - with pytest.raises(ValidationError, match="subsection is deleted"): - self.create_section_with_subsections([subsection]) - def test_removing_subsection(self): """ Test removing a subsection from a section (but not deleting it) """ section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) @@ -950,18 +914,18 @@ def test_sections_containing(self): # Note: it is important that some of these sections contain other subsections, to ensure the complex JOINs required # for this query are working correctly, especially in the case of ignore_pinned=True. # Section 1 ✅ has subsection 1, pinned 📌 to V1 - section1_1pinned = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2], key="u1") + section1_1pinned = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2], key="s1") # Section 2 ✅ has subsection 1, pinned 📌 to V2 - section2_1pinned_v2 = self.create_section_with_subsections([subsection_1_v2, self.subsection_2_v1], key="u2") + section2_1pinned_v2 = self.create_section_with_subsections([subsection_1_v2, self.subsection_2_v1], key="s2") # Section 3 doesn't contain it - _section3_no = self.create_section_with_subsections([self.subsection_2], key="u3") + _section3_no = self.create_section_with_subsections([self.subsection_2], key="s3") # Section 4 ✅ has subsection 1, unpinned section4_unpinned = self.create_section_with_subsections([ self.subsection_1, self.subsection_2, self.subsection_2_v1, - ], key="u4") + ], key="s4") # Sections 5/6 don't contain it - _section5_no = self.create_section_with_subsections([self.subsection_2_v1, self.subsection_2], key="u5") - _section6_no = self.create_section_with_subsections([], key="u6") + _section5_no = self.create_section_with_subsections([self.subsection_2_v1, self.subsection_2], key="s5") + _section6_no = self.create_section_with_subsections([], key="s6") # No need to publish anything as the get_containers_with_entity() API only considers drafts (for now). From 0a45fd1911e5a8a311186f3a01cc0f20a720de77 Mon Sep 17 00:00:00 2001 From: Andrii Date: Mon, 14 Apr 2025 15:29:27 +0300 Subject: [PATCH 4/8] style: [AXM-1886] fix lint errors --- .../apps/authoring/sections/test_api.py | 68 +++++++++++-------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py index c758c009..79c9c625 100644 --- a/tests/openedx_learning/apps/authoring/sections/test_api.py +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -4,7 +4,6 @@ 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 @@ -15,7 +14,7 @@ @ddt.ddt -class SubSectionTestCase(SubSectionTestCase): +class SectionTestCase(SubSectionTestCase): """ Test cases for Sections (containers of subsections) """ def setUp(self) -> None: @@ -320,7 +319,10 @@ def test_create_next_section_version_with_unpinned_and_pinned_subsections(self): section_version_v2 = authoring_api.create_next_section_version( section=section, title="Section", - subsections=[self.subsection_1, self.subsection_2_v1], # Note the "v1" pinning 📌 the second one to version 1 + subsections=[ + self.subsection_1, + self.subsection_2_v1 + ], # Note the "v1" pinning 📌 the second one to version 1 created=self.now, created_by=None, ) @@ -341,7 +343,9 @@ def test_auto_publish_children(self): # Create a draft section with two draft subsections section = self.create_section_with_subsections([self.subsection_1, self.subsection_2]) # Also create another subsection that's not in the section at all: - other_subsection, _os_v1 = self.create_subsection(title="A draft subsection not in the section", key="subsection:3") + other_subsection, _os_v1 = self.create_subsection( + title="A draft subsection not in the section", key="subsection:3" + ) assert authoring_api.contains_unpublished_changes(section.pk) assert self.subsection_1.versioning.published is None @@ -403,7 +407,7 @@ def test_add_subsection_after_publish(self): # Publish the empty section: authoring_api.publish_all_drafts(self.learning_package.id) section.refresh_from_db() # Reloading the section is necessary - assert section.versioning.has_unpublished_changes is False # Shallow check for just the section itself, not children + assert section.versioning.has_unpublished_changes is False # Shallow check for the section itself, not children assert authoring_api.contains_unpublished_changes(section.pk) is False # Deeper check # Add a published subsection (unpinned): @@ -449,7 +453,7 @@ def test_modify_unpinned_subsection_after_publish(self): # The subsection now has unpublished changes; the section doesn't directly but does contain section.refresh_from_db() # Reloading the section is necessary, or 'section.versioning' will be outdated self.subsection_1.refresh_from_db() - assert section.versioning.has_unpublished_changes is False # Shallow check should be false - section is unchanged + assert section.versioning.has_unpublished_changes is False # Shallow check should be false - section unchanged assert authoring_api.contains_unpublished_changes(section.pk) # But section DOES contain changes assert self.subsection_1.versioning.has_unpublished_changes @@ -511,17 +515,21 @@ def test_create_two_sections_with_same_subsections(self): subsections in each section. """ # Create a section with subsection 2 unpinned, subsection 2 pinned 📌, and subsection 1: - section1 = self.create_section_with_subsections([self.subsection_2, self.subsection_2_v1, self.subsection_1], key="u1") + section1 = self.create_section_with_subsections( + [self.subsection_2, self.subsection_2_v1, self.subsection_1], key="u1" + ) # Create a second section with subsection 1 pinned 📌, subsection 2, and subsection 1 unpinned: - section2 = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2, self.subsection_1], key="u2") + section2 = self.create_section_with_subsections( + [self.subsection_1_v1, self.subsection_2, self.subsection_1], key="u2" + ) # Check that the contents are as expected: - assert [row.subsection_version for row in authoring_api.get_subsections_in_section(section1, published=False)] == [ - self.subsection_2_v1, self.subsection_2_v1, self.subsection_1_v1, - ] - assert [row.subsection_version for row in authoring_api.get_subsections_in_section(section2, published=False)] == [ - self.subsection_1_v1, self.subsection_2_v1, self.subsection_1_v1, - ] + assert [ + row.subsection_version for row in authoring_api.get_subsections_in_section(section1, published=False) + ] == [self.subsection_2_v1, self.subsection_2_v1, self.subsection_1_v1,] + assert [ + row.subsection_version for row in authoring_api.get_subsections_in_section(section2, published=False) + ] == [self.subsection_1_v1, self.subsection_2_v1, self.subsection_1_v1,] # Modify subsection 1 subsection_1_v2 = self.modify_subsection(self.subsection_1, title="subsection 1 v2") @@ -575,7 +583,7 @@ def test_publishing_shared_subsection(self): # 2️⃣ Then the author edits S2 inside of Section 1 making S2v2. s2_v2 = self.modify_subsection(s2, title="U2 version 2") - # This makes S1 and S2 both show up as Sections that CONTAIN unpublished changes, because they share the subsection. + # This makes S1, S2 both show up as Sections that CONTAIN unpublished changes, because they share the subsection assert authoring_api.contains_unpublished_changes(section1.pk) assert authoring_api.contains_unpublished_changes(section2.pk) # (But the sections themselves are unchanged:) @@ -603,8 +611,9 @@ def test_publishing_shared_subsection(self): Entry(s3_v1), ] - # Result: someone looking at Section 2 should see the newly published subsection 2, because publishing it anywhere - # publishes it everywhere. But publishing U2 and Section 1 does not affect the other subsections in Section 2. + # Result: someone looking at Section 2 should see the newly published subsection 2, + # because publishing it anywhere publishes it everywhere. + # But publishing U2 and Section 1 does not affect the other subsections in Section 2. # (Publish propagates downward, not upward) assert authoring_api.get_subsections_in_section(section2, published=True) == [ Entry(s2_v2), # new published version of U2 @@ -612,12 +621,13 @@ def test_publishing_shared_subsection(self): Entry(s5_v1), # still original version of U5 (it hasn't been published) ] - # Result: Section 2 CONTAINS unpublished changes because of the modified U5. Section 1 doesn't contain unpub changes. + # Result: Section 2 CONTAINS unpublished changes because of the modified U5. + # Section 1 doesn't contain unpub changes. assert authoring_api.contains_unpublished_changes(section1.pk) is False assert authoring_api.contains_unpublished_changes(section2.pk) # 5️⃣ Publish subsection U5, which should be the only thing unpublished in the learning package - self.publish_subsection(u5) + self.publish_subsection(s5) # Result: Section 2 shows the new version of C5 and no longer contains unpublished changes: assert authoring_api.get_subsections_in_section(section2, published=True) == [ Entry(s2_v2), # new published version of U2 @@ -723,11 +733,11 @@ def test_soft_deleting_subsection(self): Entry(self.subsection_1_v1), # subsection 2 is soft deleted from the draft. # TODO: should we return some kind of placeholder here, to indicate that a subsection is still listed in the - # section's subsection list but has been soft deleted, and will be fully deleted when published, or restored if - # reverted? + # section's subsection list but has been soft deleted, and will be fully deleted when published, + # or restored if reverted? ] - assert section.versioning.has_unpublished_changes is False # The section itself and its subsection list is not changed - assert authoring_api.contains_unpublished_changes(section.pk) # But it CONTAINS an unpublished change (a deletion) + assert section.versioning.has_unpublished_changes is False # The section and its subsection list is not changed + assert authoring_api.contains_unpublished_changes(section.pk) # But it CONTAINS unpublished change (deletion) # The published version of the section is not yet affected: assert authoring_api.get_subsections_in_section(section, published=True) == [ Entry(self.subsection_1_v1), @@ -789,7 +799,7 @@ def test_soft_deleting_pinned_subsection(self): Entry(self.subsection_1_v1, pinned=True), Entry(self.subsection_2_v1, pinned=True), ] - assert section.versioning.has_unpublished_changes is False # The section itself and its subsection list is not changed + assert section.versioning.has_unpublished_changes is False # The section and its subsection list is not changed assert authoring_api.contains_unpublished_changes(section.pk) is False # nor does it contain changes # The published version of the section is also not affected: assert authoring_api.get_subsections_in_section(section, published=True) == [ @@ -812,7 +822,7 @@ def test_soft_delete_section(self): # Delete the section: authoring_api.soft_delete_draft(section_to_delete.publishable_entity_id) section_to_delete.refresh_from_db() - # Now the draft section is [soft] deleted, but the subsections, published section, and other section is unaffected: + # Now draft section is [soft] deleted, but the subsections, published section, and other section is unaffected: assert section_to_delete.versioning.draft is None # Section is soft deleted. assert section_to_delete.versioning.published is not None self.subsection_1.refresh_from_db() @@ -911,7 +921,7 @@ def test_sections_containing(self): subsection_1_v2 = self.modify_subsection(self.subsection_1, title="modified subsection 1") # Create a few sections, some of which contain subsection 1 and others which don't: - # Note: it is important that some of these sections contain other subsections, to ensure the complex JOINs required + # Note: it is important that some of these sections contain other subsections, to ensure complex JOINs required # for this query are working correctly, especially in the case of ignore_pinned=True. # Section 1 ✅ has subsection 1, pinned 📌 to V1 section1_1pinned = self.create_section_with_subsections([self.subsection_1_v1, self.subsection_2], key="s1") @@ -946,7 +956,9 @@ def test_sections_containing(self): with self.assertNumQueries(1): result2 = [ c.section for c in - authoring_api.get_containers_with_entity(self.subsection_1.pk, ignore_pinned=True).select_related("section") + authoring_api.get_containers_with_entity( + self.subsection_1.pk, ignore_pinned=True + ).select_related("section") ] assert result2 == [section4_unpinned] @@ -1042,5 +1054,5 @@ def test_get_container_children_count(self): # Test that I can get a [PublishLog] history of a given section and all its children, including children that aren't # currently in the section and excluding children that are only in other sections. # Test that I can get a [PublishLog] history of a given section and its children, that includes changes made to the - # child subsections while they were part of the section but excludes changes made to those children while they were + # child subsections while they were part of section but excludes changes made to those children while they were # not part of the section. 🫣 From 390f72aa83e28ed540b77af02d47c3fda6338fec Mon Sep 17 00:00:00 2001 From: Andrii Date: Fri, 18 Apr 2025 13:23:18 +0300 Subject: [PATCH 5/8] refactor: update child entities to new API + tests, fix some name & style issues --- openedx_learning/__init__.py | 2 +- .../apps/authoring/sections/api.py | 55 ++++++++++--------- .../sections/migrations/0001_initial.py | 2 +- .../apps/authoring/sections/models.py | 2 +- .../apps/authoring/sections/test_api.py | 4 +- 5 files changed, 34 insertions(+), 31 deletions(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index bda4b183..42a0252d 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.25.0" +__version__ = "0.24.0" diff --git a/openedx_learning/apps/authoring/sections/api.py b/openedx_learning/apps/authoring/sections/api.py index bbde80a4..24207653 100644 --- a/openedx_learning/apps/authoring/sections/api.py +++ b/openedx_learning/apps/authoring/sections/api.py @@ -62,8 +62,7 @@ def create_section_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, ) -> SectionVersion: @@ -78,8 +77,7 @@ def create_section_version( section_pk: The section 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 section. """ @@ -87,8 +85,7 @@ def create_section_version( section.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=SectionVersion, @@ -97,7 +94,7 @@ def create_section_version( def _pub_entities_for_subsections( subsections: list[Subsection | SubsectionVersion] | None, -) -> tuple[list[int], list[int | None]] | tuple[None, None]: +) -> list[publishing_api.ContainerEntityRow] | None: """ Helper method: given a list of Subsection | SubsectionVersion, return the lists of publishable_entities_pks and entity_version_pks needed for the @@ -108,19 +105,23 @@ def _pub_entities_for_subsections( """ if subsections is None: # When these are None, that means don't change the entities in the list. - return None, None + return None for u in subsections: if not isinstance(u, (Subsection, SubsectionVersion)): raise TypeError("Section subsections must be either Subsection or SubsectionVersion.") - publishable_entities_pks = [ - (u.publishable_entity_id if isinstance(u, Subsection) else u.subsection.publishable_entity_id) - for u in subsections - ] - entity_version_pks = [ - (uv.pk if isinstance(uv, SubsectionVersion) else None) - for uv in subsections + return [ + ( + publishing_api.ContainerEntityRow( + entity_pk=s.container.publishable_entity_id, + version_pk=None, + ) if isinstance(s, Subsection) + else publishing_api.ContainerEntityRow( + entity_pk=s.subsection.container.publishable_entity_id, + version_pk=s.container_version.publishable_entity_version_id, + ) + ) + for s in subsections ] - return publishable_entities_pks, entity_version_pks def create_next_section_version( @@ -138,17 +139,16 @@ def create_next_section_version( Args: section_pk: The section ID. title: The title. Leave as None to keep the current title. - subsections: The subsections, as a list of Subsections (unpinned) and/or SubsectionVersions (pinned). Passing None - will leave the existing subsections unchanged. + subsections: The subsections, as a list of Subsections (unpinned) and/or SubsectionVersions (pinned). + Passing None will leave the existing subsections unchanged. created: The creation date. created_by: The user who created the section. """ - publishable_entities_pks, entity_version_pks = _pub_entities_for_subsections(subsections) + entity_rows = _pub_entities_for_subsections(subsections) section_version = publishing_api.create_next_container_version( section.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=SectionVersion, @@ -177,7 +177,7 @@ def create_section_and_version( created_by: The user who created the section. can_stand_alone: Set to False when created as part of containers """ - publishable_entities_pks, entity_version_pks = _pub_entities_for_subsections(subsections) + entity_rows = _pub_entities_for_subsections(subsections) with atomic(): section = create_section( learning_package_id, @@ -190,8 +190,7 @@ def create_section_and_version( section, 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 +285,9 @@ def get_subsections_in_published_section_as_of( ancestors of every modified PublishableEntity in the publish. """ assert isinstance(section, Section) - section_pub_entity_version = publishing_api.get_published_version_as_of(section.publishable_entity_id, publish_log_id) + section_pub_entity_version = publishing_api.get_published_version_as_of( + section.publishable_entity_id, publish_log_id + ) if section_pub_entity_version is None: return None # This section was not published as of the given PublishLog ID. container_version = section_pub_entity_version.containerversion @@ -303,5 +304,7 @@ def get_subsections_in_published_section_as_of( # This is not optimized. It could be done in one query per section rather than one query per subsection. pub_entity_version = publishing_api.get_published_version_as_of(row.entity_id, publish_log_id) if pub_entity_version: - entity_list.append(SectionListEntry(subsection_version=pub_entity_version.containerversion.subsectionversion, pinned=False)) + entity_list.append(SectionListEntry( + subsection_version=pub_entity_version.containerversion.subsectionversion, pinned=False + )) return entity_list diff --git a/openedx_learning/apps/authoring/sections/migrations/0001_initial.py b/openedx_learning/apps/authoring/sections/migrations/0001_initial.py index 6a9f7ef3..5083cda7 100644 --- a/openedx_learning/apps/authoring/sections/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/sections/migrations/0001_initial.py @@ -1,7 +1,7 @@ # Generated by Django 4.2.20 on 2025-04-11 12:53 -from django.db import migrations, models import django.db.models.deletion +from django.db import migrations, models class Migration(migrations.Migration): diff --git a/openedx_learning/apps/authoring/sections/models.py b/openedx_learning/apps/authoring/sections/models.py index 8fa0793d..34e001a5 100644 --- a/openedx_learning/apps/authoring/sections/models.py +++ b/openedx_learning/apps/authoring/sections/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/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py index 79c9c625..e354ac59 100644 --- a/tests/openedx_learning/apps/authoring/sections/test_api.py +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -14,7 +14,7 @@ @ddt.ddt -class SectionTestCase(SubSectionTestCase): +class SectionTestCase(SubSectionTestCase): # pylint: disable=test-inherits-tests """ Test cases for Sections (containers of subsections) """ def setUp(self) -> None: @@ -185,7 +185,7 @@ def test_create_section_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_section = self.create_section_with_subsections([]) - with self.assertNumQueries(26): + with self.assertNumQueries(27): # And try with a non-empty section: self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1], key="u2") From 0a39f37367bf62dbf3dd22abac764a32a29edd6f Mon Sep 17 00:00:00 2001 From: Andrii Date: Tue, 22 Apr 2025 11:25:03 +0300 Subject: [PATCH 6/8] refactor: bump version & add TODO for tests + fix queries test --- openedx_learning/__init__.py | 2 +- tests/openedx_learning/apps/authoring/sections/test_api.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 42a0252d..bda4b183 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.24.0" +__version__ = "0.25.0" diff --git a/tests/openedx_learning/apps/authoring/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py index e354ac59..cfb039d1 100644 --- a/tests/openedx_learning/apps/authoring/sections/test_api.py +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -13,6 +13,9 @@ Entry = authoring_api.SectionListEntry +# TODO: Turn SubSectionTestCase into SubSectionTestMixin and remove the +# test-inherits-tests pylint warning below. +# https://github.com/openedx/openedx-learning/issues/308 @ddt.ddt class SectionTestCase(SubSectionTestCase): # pylint: disable=test-inherits-tests """ Test cases for Sections (containers of subsections) """ @@ -183,9 +186,9 @@ def test_create_section_queries(self): Test how many database queries are required to create a section """ # 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_section = self.create_section_with_subsections([]) - with self.assertNumQueries(27): + with self.assertNumQueries(30): # And try with a non-empty section: self.create_section_with_subsections([self.subsection_1, self.subsection_2_v1], key="u2") From cf4a02b245e12d6021ca2e8210b5559bdbd776e7 Mon Sep 17 00:00:00 2001 From: Andrii Date: Tue, 22 Apr 2025 11:39:00 +0300 Subject: [PATCH 7/8] refactor: use assertCountEqual in test to prevent failure because of different order --- tests/openedx_learning/apps/authoring/sections/test_api.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openedx_learning/apps/authoring/sections/test_api.py b/tests/openedx_learning/apps/authoring/sections/test_api.py index cfb039d1..4faa14c8 100644 --- a/tests/openedx_learning/apps/authoring/sections/test_api.py +++ b/tests/openedx_learning/apps/authoring/sections/test_api.py @@ -128,13 +128,13 @@ def test_get_containers(self): section = self.create_section_with_subsections([]) with self.assertNumQueries(1): result = list(authoring_api.get_containers(self.learning_package.id)) - assert result == [ + self.assertCountEqual(result, [ self.unit_1.container, self.unit_2.container, self.subsection_1.container, self.subsection_2.container, section.container, - ] + ]) # Versioning data should be pre-loaded via select_related() with self.assertNumQueries(0): assert result[0].versioning.has_unpublished_changes From 53c656569233396924bbfbc3be433ae277c433a8 Mon Sep 17 00:00:00 2001 From: Andrii Date: Tue, 22 Apr 2025 21:31:08 +0300 Subject: [PATCH 8/8] refactor: correct package naming in docstrings --- openedx_learning/apps/authoring/sections/apps.py | 2 +- openedx_learning/apps/authoring/sections/models.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx_learning/apps/authoring/sections/apps.py b/openedx_learning/apps/authoring/sections/apps.py index ce7b8a2a..64bc5f87 100644 --- a/openedx_learning/apps/authoring/sections/apps.py +++ b/openedx_learning/apps/authoring/sections/apps.py @@ -17,7 +17,7 @@ class SectionsConfig(AppConfig): def ready(self): """ - Register Subsection and SubsectionVersion. + Register Section and SectionVersion. """ from ..publishing.api import register_content_models # pylint: disable=import-outside-toplevel from .models import Section, SectionVersion # pylint: disable=import-outside-toplevel diff --git a/openedx_learning/apps/authoring/sections/models.py b/openedx_learning/apps/authoring/sections/models.py index 34e001a5..afcb0ae0 100644 --- a/openedx_learning/apps/authoring/sections/models.py +++ b/openedx_learning/apps/authoring/sections/models.py @@ -1,5 +1,5 @@ """ -Models that implement subsections +Models that implement sections """ from django.db import models