refactor: validation should only ever raise ValidationError

Author: bradenmacdonald

src/openedx_content/applets/containers/api.py

@@ -263,7 +263,7 @@ def _create_container_version(
     for entity_row in entity_list.rows:
         try:
             container_subclass.validate_entity(entity_row.entity)
-        except Exception as exc:
+        except ValidationError as exc:
             # This exception is carefully worded. The validation may have failed because the entity is of the wrong
             # type, but it _could_ be a of the correct type but otherwise invalid/corrupt, e.g. partially deleted.
             raise ValidationError(

src/openedx_content/applets/containers/models.py

@@ -177,7 +177,11 @@ class Container(PublishableEntityMixin):
 
     @classmethod
     def validate_entity(cls, entity: PublishableEntity) -> None:
-        """Check if the given entity is allowed as a child of this Container type"""
+        """
+        Check if the given entity is allowed as a child of this Container type
+
+        Subclasses should raise ValidationError if "entity" is invalid.
+        """
 
     @final
     @classmethod

src/openedx_content/applets/sections/models.py

@@ -42,8 +42,9 @@ class Section(Container):
     def validate_entity(cls, entity: PublishableEntity) -> None:
         """Check if the given entity is allowed as a child of a Section"""
         # Sections only allow Subsections as children, so the entity must be 1:1 with Container:
-        container = entity.container  # Could raise PublishableEntity.container.RelatedObjectDoesNotExist
-        if get_container_subclass_of(container) is not Subsection:
+        if not hasattr(entity, "container"):
+            raise ValidationError("Only Units can be added as children of a Subsection (found non-Container child)")
+        if get_container_subclass_of(entity.container) is not Subsection:
             raise ValidationError("Only Subsection can be added as children of a Section")
 
 

src/openedx_content/applets/subsections/models.py

@@ -42,8 +42,9 @@ class Subsection(Container):
     def validate_entity(cls, entity: PublishableEntity) -> None:
         """Check if the given entity is allowed as a child of a Subsection"""
         # Subsections only allow Units as children, so the entity must be 1:1 with Container:
-        container = entity.container  # Could raise PublishableEntity.container.RelatedObjectDoesNotExist
-        if get_container_subclass_of(container) is not Unit:
+        if not hasattr(entity, "container"):
+            raise ValidationError("Only Units can be added as children of a Subsection (found non-Container child)")
+        if get_container_subclass_of(entity.container) is not Unit:
             raise ValidationError("Only Units can be added as children of a Subsection")
 
 

src/openedx_content/applets/units/models.py

@@ -4,6 +4,7 @@
 
 from typing import override
 
+from django.core.exceptions import ValidationError
 from django.db import models
 
 from ..containers.models import Container, ContainerVersion
@@ -39,8 +40,8 @@ class Unit(Container):
     def validate_entity(cls, entity: PublishableEntity) -> None:
         """Check if the given entity is allowed as a child of a Unit"""
         # Units only allow Components as children, so the entity must be 1:1 with Component:
-        # This could raise PublishableEntity.component.RelatedObjectDoesNotExist
-        getattr(entity, "component")  # pylint: disable=literal-used-as-attribute
+        if not hasattr(entity, "component"):
+            raise ValidationError("Only Components can be added as children of a Unit")
 
 
 class UnitVersion(ContainerVersion):

tests/openedx_content/applets/subsections/test_api.py

@@ -149,17 +149,20 @@ def test_create_subsection_with_invalid_children(self):
         # Create a subsection:
         subsection = self.create_subsection_with_units([])
         subsection_version = subsection.versioning.draft
+
         # Try adding a Component to a Subsection
         with pytest.raises(
             ValidationError,
             match='The entity "xblock.v1:problem:Query Counting" cannot be added to a "subsection" container.',
-        ):
+        ) as err:
             content_api.create_next_subsection_version(
                 subsection,
                 units=[self.component_1],
                 created=self.now,
                 created_by=None,
             )
+        assert "(found non-Container child)" in str(err.value.__cause__)
+
         # Check that a new version was not created:
         subsection.refresh_from_db()
         assert content_api.get_subsection(subsection.pk).versioning.draft == subsection_version

tests/openedx_content/applets/units/test_api.py

@@ -151,14 +151,33 @@ def test_create_unit_with_invalid_children(self):
         unit = self.create_unit_with_components([])
         unit_version = unit.versioning.draft
         unit2 = self.create_unit_with_components([], key="unit:key2", title="Unit 2")
+
         # Try adding a Unit to a Unit
-        with pytest.raises(ValidationError, match='The entity "unit:key2" cannot be added to a "unit" container.'):
+        with pytest.raises(
+            ValidationError, match='The entity "unit:key2" cannot be added to a "unit" container.'
+        ) as err:
             content_api.create_next_unit_version(
                 unit,
                 components=[unit2],
                 created=self.now,
                 created_by=None,
             )
+        assert "Only Components can be added as children of a Unit" in str(err.value.__cause__)
+
+        # Try adding a generic entity to a Unit
+        pe = content_api.create_publishable_entity(self.learning_package.id, "pe", created=self.now, created_by=None)
+        pev = content_api.create_publishable_entity_version(
+            pe.pk, version_num=1, title="t", created=self.now, created_by=None
+        )
+        with pytest.raises(ValidationError, match='The entity "pe" cannot be added to a "unit" container.') as err:
+            content_api.create_next_unit_version(
+                unit,
+                components=[pev],
+                created=self.now,
+                created_by=None,
+            )
+        assert "Only Components can be added as children of a Unit" in str(err.value.__cause__)
+
         # Check that a new version was not created:
         unit.refresh_from_db()
         assert content_api.get_unit(unit.pk).versioning.draft == unit_version