feat: add a pre_save hook to prevent Organization.short_name from being changed

Author: bradenmacdonald

src/openedx_catalog/apps.py

@@ -16,4 +16,6 @@ class CatalogAppConfig(AppConfig):
     label = "openedx_catalog"
 
     def ready(self) -> None:
-        pass
+
+        # Connect signal handlers.
+        from . import signals  # pylint: disable=import-outside-toplevel,unused-import

src/openedx_catalog/signals.py

@@ -0,0 +1,47 @@
+"""
+Signal receivers for openedx_catalog
+"""
+# pylint: disable=unused-argument
+
+import logging
+
+from django.core.exceptions import ValidationError
+from django.db.models.signals import pre_save
+from django.dispatch import receiver
+
+from organizations.models import Organization
+
+from .models import CourseRun
+
+logger = logging.getLogger(__name__)
+
+
+@receiver(pre_save, sender=Organization)
+def verify_organization_change(sender, instance, **kwargs):
+    """
+    Check that changes to Organization objects won't create invalid relationships.
+
+    Nothing stops users from changing Organization.short_name entries in the Django admin, but any changes other than
+    capitalization fixes will result in totally invalid data, as CatalogCourse will be related to an Organization that
+    no longer matches the related course's course IDs.
+    """
+    # Check only at User creation time and when not raw.
+    if not instance.pk:
+        return  # It's a brand new Organization; we don't care
+
+    prev_org_code = Organization.objects.get(pk=instance.pk).short_name
+    new_org_code = instance.short_name
+
+    if new_org_code != prev_org_code:
+        # Check if this is going to violate any relationship expectations.
+        # Note: we could make the database enforce this by making the CatalogCourse.org relationship use "short_name" as
+        # its foreign key ID, but that would also make it extremely difficult to ever "fix" an incorrect Organization's
+        # short_name (e.g. change capitalization), because doing so would fail with a foreign key constraint error.
+
+        run_course_ids = CourseRun.objects.filter(catalog_course__org=instance).values_list("course_id", flat=True)
+        for course_id in run_course_ids:
+            if course_id.org.lower() != new_org_code.lower():
+                raise ValidationError(
+                    f'Changing the org short_name to "{new_org_code}" will result in CourseRun "{course_id}" having '
+                    "the incorrect organization code. "
+                )

tests/openedx_catalog/test_signals.py

@@ -0,0 +1,55 @@
+"""
+Tests related to the Catalog signal handlers
+"""
+
+import re
+
+from django.core.exceptions import ValidationError
+from django.test import TestCase
+from organizations.api import ensure_organization  # type: ignore[import]
+from organizations.models import Organization
+import pytest
+
+from openedx_catalog.models import CatalogCourse, CourseRun
+
+
+class TestCatalogSignals(TestCase):
+    """
+    Test openedx_catalog signal handlers
+    """
+
+    def test_org_integrity(self) -> None:
+        """
+        Test that Organization.short_name cannot be changed if it would result in invalid CourseRun relationships.
+
+        Note: this is just Django validation; running an `update()` or raw SQL will easily bypass this check.
+        """
+        org = Organization.objects.get(pk=ensure_organization("Org1")["id"])
+
+        catalog_course = CatalogCourse.objects.create(org=org, course_code="Math100")
+        course_run = CourseRun.objects.create(catalog_course=catalog_course, run="A")
+        assert str(course_run.course_id) == "course-v1:Org1+Math100+A"
+
+        org.short_name = "foo"
+        with pytest.raises(
+            ValidationError,
+            match=re.escape(
+                'Changing the org short_name to "foo" will result in CourseRun "course-v1:Org1+Math100+A" having '
+                "the incorrect organization code."
+            ),
+        ):
+            org.save()
+
+        # BUT, changing just the capitalization is allowed:
+        org.short_name = "orG1"
+        org.save()  # No ValidationError
+
+    def test_org_short_name_change_no_runs(self) -> None:
+        """
+        Test that Organization.short_name CAN be changed if it won't affect any course runs.
+        """
+        org = Organization.objects.get(pk=ensure_organization("Org1")["id"])
+        CatalogCourse.objects.create(org=org, course_code="Math100")
+
+        org.short_name = "foo"
+        org.save()