From 4b95b350699aab7af7801ce38ee0620878ed6c23 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 28 Feb 2025 17:33:55 -0800 Subject: [PATCH] refactor: Unit is not directly a PublishableEntity --- .../apps/authoring/containers/models_mixin.py | 20 +++--- openedx_learning/apps/authoring/units/api.py | 15 ++-- openedx_learning/apps/authoring/units/apps.py | 9 --- .../units/migrations/0001_initial.py | 14 ++-- .../apps/authoring/units/models.py | 16 +++-- .../apps/authoring/units/test_api.py | 68 +++++++++---------- 6 files changed, 62 insertions(+), 80 deletions(-) diff --git a/openedx_learning/apps/authoring/containers/models_mixin.py b/openedx_learning/apps/authoring/containers/models_mixin.py index 2d76cd252..9f2eb6427 100644 --- a/openedx_learning/apps/authoring/containers/models_mixin.py +++ b/openedx_learning/apps/authoring/containers/models_mixin.py @@ -8,10 +8,6 @@ from django.db import models from openedx_learning.apps.authoring.containers.models import Container, ContainerVersion -from openedx_learning.apps.authoring.publishing.model_mixins import ( - PublishableEntityMixin, - PublishableEntityVersionMixin, -) from openedx_learning.lib.managers import WithRelationsManager __all__ = [ @@ -20,17 +16,20 @@ ] -class ContainerMixin(PublishableEntityMixin): +class ContainerMixin(models.Model): """ Convenience mixin to link your models against Container. Please see docstring for Container for more details. - - If you use this class, you *MUST* also use ContainerVersionMixin """ # select these related entities by default for all queries - objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager("container") # type: ignore[assignment] + objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( # type: ignore[assignment] + "container", + "container__publishable_entity", + "container__publishable_entity__published", + "container__publishable_entity__draft", + ) container = models.OneToOneField( Container, @@ -49,18 +48,17 @@ class Meta: abstract = True -class ContainerVersionMixin(PublishableEntityVersionMixin): +class ContainerVersionMixin(models.Model): """ Convenience mixin to link your models against ContainerVersion. Please see docstring for ContainerVersion for more details. - - If you use this class, you *MUST* also use ContainerMixin """ # select these related entities by default for all queries objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( # type: ignore[assignment] "container_version", + "container_version__publishable_entity_version", ) container_version = models.OneToOneField( diff --git a/openedx_learning/apps/authoring/units/api.py b/openedx_learning/apps/authoring/units/api.py index 9da2703ad..ef827858f 100644 --- a/openedx_learning/apps/authoring/units/api.py +++ b/openedx_learning/apps/authoring/units/api.py @@ -46,10 +46,7 @@ def create_unit( container = container_api.create_container( learning_package_id, key, created, created_by ) - unit = Unit.objects.create( - container=container, - publishable_entity=container.publishable_entity, - ) + unit = Unit.objects.create(container=container) return unit @@ -86,9 +83,7 @@ def create_unit_version( created_by=created_by, ) unit_version = UnitVersion.objects.create( - unit=unit, container_version=container_version, - publishable_entity_version=container_version.publishable_entity_version, ) return unit_version @@ -138,9 +133,7 @@ def create_next_unit_version( created_by=created_by, ) unit_version = UnitVersion.objects.create( - unit=unit, container_version=container_version, - publishable_entity_version=container_version.publishable_entity_version, ) return unit_version @@ -279,13 +272,13 @@ def get_components_in_published_unit_as_of( ancestors of every modified PublishableEntity in the publish. """ assert isinstance(unit, Unit) - unit_pub_entity_version = get_published_version_as_of(unit.publishable_entity_id, publish_log_id) + unit_pub_entity_version = get_published_version_as_of(unit.container.publishable_entity_id, publish_log_id) if unit_pub_entity_version is None: return None # This unit was not published as of the given PublishLog ID. - unit_version = unit_pub_entity_version.unitversion # type: ignore[attr-defined] + container_version = unit_pub_entity_version.containerversion entity_list = [] - rows = unit_version.container_version.entity_list.entitylistrow_set.order_by("order_num") + rows = container_version.entity_list.entitylistrow_set.order_by("order_num") for row in rows: if row.entity_version is not None: component_version = row.entity_version.componentversion diff --git a/openedx_learning/apps/authoring/units/apps.py b/openedx_learning/apps/authoring/units/apps.py index f0beecf3d..a63f0d3df 100644 --- a/openedx_learning/apps/authoring/units/apps.py +++ b/openedx_learning/apps/authoring/units/apps.py @@ -14,12 +14,3 @@ class UnitsConfig(AppConfig): verbose_name = "Learning Core > Authoring > Units" default_auto_field = "django.db.models.BigAutoField" label = "oel_units" - - def ready(self): - """ - Register Unit and UnitVersion. - """ - from ..publishing.api import register_content_models # pylint: disable=import-outside-toplevel - from .models import Unit, UnitVersion # pylint: disable=import-outside-toplevel - - register_content_models(Unit, UnitVersion) diff --git a/openedx_learning/apps/authoring/units/migrations/0001_initial.py b/openedx_learning/apps/authoring/units/migrations/0001_initial.py index 8a72507e4..8cf24d5a8 100644 --- a/openedx_learning/apps/authoring/units/migrations/0001_initial.py +++ b/openedx_learning/apps/authoring/units/migrations/0001_initial.py @@ -10,26 +10,24 @@ class Migration(migrations.Migration): dependencies = [ ('oel_containers', '0001_initial'), - ('oel_publishing', '0002_alter_learningpackage_key_and_more'), ] operations = [ migrations.CreateModel( - name='Unit', + name='UnitVersion', fields=[ - ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), - ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.container')), + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.containerversion')), ], options={ 'abstract': False, }, ), migrations.CreateModel( - name='UnitVersion', + name='Unit', fields=[ - ('publishable_entity_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentityversion')), - ('container_version', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.containerversion')), - ('unit', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='versions', to='oel_units.unit')), + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('container', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='oel_containers.container')), ], options={ 'abstract': False, diff --git a/openedx_learning/apps/authoring/units/models.py b/openedx_learning/apps/authoring/units/models.py index 3e5044916..93dd81f52 100644 --- a/openedx_learning/apps/authoring/units/models.py +++ b/openedx_learning/apps/authoring/units/models.py @@ -1,7 +1,6 @@ """ Models that implement units """ -from django.db import models from ..containers.models_mixin import ContainerMixin, ContainerVersionMixin @@ -14,18 +13,21 @@ class Unit(ContainerMixin): """ A Unit is Container, which is a PublishableEntity. + + The only purpose of this table at the moment is to distinguish units from + other container types at the database level, and to provide a target for + foreign keys that need to apply only to units (if any?). """ class UnitVersion(ContainerVersionMixin): """ - A UnitVersion is a ContainerVersion, which is a PublishableEntityVersion. + A UnitVersion has a ContainerVersion, which is a PublishableEntityVersion. """ # Not sure what other metadata goes here, but we want to try to separate things # like scheduling information and such into different models. - unit = models.ForeignKey( - Unit, - on_delete=models.CASCADE, - related_name="versions", - ) + + @property + def unit(self): + return self.container_version.container.unit # pylint: disable=no-member diff --git a/tests/openedx_learning/apps/authoring/units/test_api.py b/tests/openedx_learning/apps/authoring/units/test_api.py index e7d470d02..898909c86 100644 --- a/tests/openedx_learning/apps/authoring/units/test_api.py +++ b/tests/openedx_learning/apps/authoring/units/test_api.py @@ -115,7 +115,7 @@ def test_create_unit_with_invalid_children(self): created_by=None, ) # Check that a new version was not created: - assert unit.versioning.draft == unit_version + assert unit.container.versioning.draft == unit_version.container_version def test_adding_external_components(self): """ @@ -172,10 +172,10 @@ def test_create_empty_unit_and_version(self): ) assert unit, unit_version assert unit_version.version_num == 1 - assert unit_version in unit.versioning.versions.all() - assert unit.versioning.has_unpublished_changes - assert unit.versioning.draft == unit_version - assert unit.versioning.published is None + assert unit_version.container_version in unit.container.versioning.versions.all() + assert unit.container.versioning.has_unpublished_changes + assert unit.container.versioning.draft == unit_version.container_version + assert unit.container.versioning.published is None def test_create_next_unit_version_with_two_unpinned_components(self): """Test creating a unit version with two unpinned components. @@ -201,7 +201,7 @@ def test_create_next_unit_version_with_two_unpinned_components(self): created_by=None, ) assert unit_version_v2.version_num == 2 - assert unit_version_v2 in unit.versioning.versions.all() + assert unit_version_v2.container_version in unit.container.versioning.versions.all() assert authoring_api.get_components_in_draft_unit(unit) == [ Entry(self.component_1.versioning.draft), Entry(self.component_2.versioning.draft), @@ -227,7 +227,7 @@ def test_create_next_unit_version_with_unpinned_and_pinned_components(self): created_by=None, ) assert unit_version_v2.version_num == 2 - assert unit_version_v2 in unit.versioning.versions.all() + assert unit_version_v2.container_version in unit.container.versioning.versions.all() assert authoring_api.get_components_in_draft_unit(unit) == [ Entry(self.component_1_v1), Entry(self.component_2_v1, pinned=True), # Pinned 📌 to v1 @@ -256,7 +256,7 @@ def test_auto_publish_children(self): # Now all changes to the unit and to component 1 are published: unit.refresh_from_db() self.component_1.refresh_from_db() - assert unit.versioning.has_unpublished_changes is False # Shallow check + assert unit.container.versioning.has_unpublished_changes is False # Shallow check assert self.component_1.versioning.has_unpublished_changes is False assert authoring_api.contains_unpublished_changes(unit) is False # Deep check assert self.component_1.versioning.published == self.component_1_v1 # v1 is now the published version. @@ -272,7 +272,7 @@ def test_no_publish_parent(self): """ # Create a draft unit with two draft components unit = self.create_unit_with_components([self.component_1, self.component_2]) - assert unit.versioning.has_unpublished_changes + assert unit.container.versioning.has_unpublished_changes # Publish ONLY one of its child components self.publish_component(self.component_1) self.component_1.refresh_from_db() # Clear cache on '.versioning' @@ -280,8 +280,8 @@ def test_no_publish_parent(self): # The unit that contains that component should still be unpublished: unit.refresh_from_db() # Clear cache on '.versioning' - assert unit.versioning.has_unpublished_changes - assert unit.versioning.published is None + assert unit.container.versioning.has_unpublished_changes + assert unit.container.versioning.published is None assert authoring_api.get_components_in_published_unit(unit) is None def test_add_component_after_publish(self): @@ -296,13 +296,13 @@ def test_add_component_after_publish(self): created=self.now, created_by=None, ) - assert unit.versioning.draft == unit_version - assert unit.versioning.published is None - assert unit.versioning.has_unpublished_changes + assert unit.container.versioning.draft == unit_version.container_version + assert unit.container.versioning.published is None + assert unit.container.versioning.has_unpublished_changes # Publish the empty unit: authoring_api.publish_all_drafts(self.learning_package.id) unit.refresh_from_db() # Reloading the unit is necessary - assert unit.versioning.has_unpublished_changes is False # Shallow check for just the unit itself, not children + assert unit.container.versioning.has_unpublished_changes is False # Shallow check for just the unit itself, not children assert authoring_api.contains_unpublished_changes(unit) is False # Deeper check # Add a published component (unpinned): @@ -316,10 +316,10 @@ def test_add_component_after_publish(self): ) # Now the unit should have unpublished changes: unit.refresh_from_db() # Reloading the unit is necessary - assert unit.versioning.has_unpublished_changes # Shallow check - adding a child is a change to the unit + assert unit.container.versioning.has_unpublished_changes # Shallow check - adding a child is a change to the unit assert authoring_api.contains_unpublished_changes(unit) # Deeper check - assert unit.versioning.draft == unit_version_v2 - assert unit.versioning.published == unit_version + assert unit.container.versioning.draft == unit_version_v2.container_version + assert unit.container.versioning.published == unit_version.container_version def test_modify_unpinned_component_after_publish(self): """ @@ -331,13 +331,13 @@ def test_modify_unpinned_component_after_publish(self): # Create a unit with one unpinned draft component: assert self.component_1.versioning.has_unpublished_changes unit = self.create_unit_with_components([self.component_1]) - assert unit.versioning.has_unpublished_changes + assert unit.container.versioning.has_unpublished_changes # Publish the unit and the component: authoring_api.publish_all_drafts(self.learning_package.id) unit.refresh_from_db() # Reloading the unit is necessary if we accessed 'versioning' before publish self.component_1.refresh_from_db() - assert unit.versioning.has_unpublished_changes is False # Shallow check + assert unit.container.versioning.has_unpublished_changes is False # Shallow check assert authoring_api.contains_unpublished_changes(unit) is False # Deeper check assert self.component_1.versioning.has_unpublished_changes is False @@ -347,7 +347,7 @@ def test_modify_unpinned_component_after_publish(self): # The component now has unpublished changes; the unit doesn't directly but does contain unit.refresh_from_db() # Reloading the unit is necessary, or 'unit.versioning' will be outdated self.component_1.refresh_from_db() - assert unit.versioning.has_unpublished_changes is False # Shallow check should be false - unit is unchanged + assert unit.container.versioning.has_unpublished_changes is False # Shallow check should be false - unit is unchanged assert authoring_api.contains_unpublished_changes(unit) # But unit DOES contain changes assert self.component_1.versioning.has_unpublished_changes @@ -391,7 +391,7 @@ def test_modify_pinned_component(self): # The component now has unpublished changes; the unit is entirely unaffected unit.refresh_from_db() # Reloading the unit is necessary, or 'unit.versioning' will be outdated self.component_1.refresh_from_db() - assert unit.versioning.has_unpublished_changes is False # Shallow check + assert unit.container.versioning.has_unpublished_changes is False # Shallow check assert authoring_api.contains_unpublished_changes(unit) is False # Deep check assert self.component_1.versioning.has_unpublished_changes is True @@ -479,8 +479,8 @@ def test_publishing_shared_component(self): # (But the units themselves are unchanged:) unit1.refresh_from_db() unit2.refresh_from_db() - assert unit1.versioning.has_unpublished_changes is False - assert unit2.versioning.has_unpublished_changes is False + assert unit1.container.versioning.has_unpublished_changes is False + assert unit2.container.versioning.has_unpublished_changes is False # 3️⃣ In addition to this, the author also modifies another component in Unit 2 (C5) c5_v2 = self.modify_component(c5, title="C5 version 2") @@ -491,7 +491,7 @@ def test_publishing_shared_component(self): self.learning_package.pk, draft_qset=authoring_api.get_all_drafts(self.learning_package.pk).filter( entity_id__in=[ - unit1.publishable_entity.id, + unit1.container.publishable_entity_id, c1.publishable_entity.id, c2.publishable_entity.id, c3.publishable_entity.id, @@ -612,7 +612,7 @@ def test_removing_component(self): Entry(self.component_1_v1), ] unit.refresh_from_db() - assert unit.versioning.has_unpublished_changes # The unit itself and its component list have change + assert unit.container.versioning.has_unpublished_changes # The unit itself and its component list have change assert authoring_api.contains_unpublished_changes(unit) # The published version of the unit is not yet affected: assert authoring_api.get_components_in_published_unit(unit) == [ @@ -648,7 +648,7 @@ def test_soft_deleting_component(self): # unit's component list but has been soft deleted, and will be fully deleted when published, or restored if # reverted? ] - assert unit.versioning.has_unpublished_changes is False # The unit itself and its component list is not changed + assert unit.container.versioning.has_unpublished_changes is False # The unit itself and its component list is not changed assert authoring_api.contains_unpublished_changes(unit) # But it CONTAINS an unpublished change (a deletion) # The published version of the unit is not yet affected: assert authoring_api.get_components_in_published_unit(unit) == [ @@ -682,7 +682,7 @@ def test_soft_deleting_and_removing_component(self): assert authoring_api.get_components_in_draft_unit(unit) == [ Entry(self.component_1_v1), ] - assert unit.versioning.has_unpublished_changes is True + assert unit.container.versioning.has_unpublished_changes is True assert authoring_api.contains_unpublished_changes(unit) # The published version of the unit is not yet affected: assert authoring_api.get_components_in_published_unit(unit) == [ @@ -710,7 +710,7 @@ def test_soft_deleting_pinned_component(self): Entry(self.component_1_v1, pinned=True), Entry(self.component_2_v1, pinned=True), ] - assert unit.versioning.has_unpublished_changes is False # The unit itself and its component list is not changed + assert unit.container.versioning.has_unpublished_changes is False # The unit itself and its component list is not changed assert authoring_api.contains_unpublished_changes(unit) is False # nor does it contain changes # The published version of the unit is also not affected: assert authoring_api.get_components_in_published_unit(unit) == [ @@ -731,11 +731,11 @@ def test_soft_delete_unit(self): # Publish everything: authoring_api.publish_all_drafts(self.learning_package.id) # Delete the unit: - authoring_api.soft_delete_draft(unit_to_delete.publishable_entity_id) + authoring_api.soft_delete_draft(unit_to_delete.container.publishable_entity_id) unit_to_delete.refresh_from_db() # Now the draft unit is [soft] deleted, but the components, published unit, and other unit is unaffected: - assert unit_to_delete.versioning.draft is None # Unit is soft deleted. - assert unit_to_delete.versioning.published is not None + assert unit_to_delete.container.versioning.draft is None # Unit is soft deleted. + assert unit_to_delete.container.versioning.published is not None self.component_1.refresh_from_db() assert self.component_1.versioning.draft is not None assert authoring_api.get_components_in_draft_unit(other_unit) == [Entry(self.component_1_v1)] @@ -744,8 +744,8 @@ def test_soft_delete_unit(self): authoring_api.publish_all_drafts(self.learning_package.id) # Now the unit's published version is also deleted, but nothing else is affected. unit_to_delete.refresh_from_db() - assert unit_to_delete.versioning.draft is None # Unit is soft deleted. - assert unit_to_delete.versioning.published is None + assert unit_to_delete.container.versioning.draft is None # Unit is soft deleted. + assert unit_to_delete.container.versioning.published is None self.component_1.refresh_from_db() assert self.component_1.versioning.draft is not None assert self.component_1.versioning.published is not None