ADR for openedx_catalog models (layering openedx_content above catalog) [FC-0138]

.importlinter

@@ -5,6 +5,7 @@
 
 [importlinter]
 root_packages =
+    openedx_catalog
     openedx_learning
     openedx_content
     openedx_tagging
@@ -19,17 +20,23 @@ name = "top-level source folders are layered correctly"
 type = layers
 layers =
     # Learning-domain features (currently CBE; Learning Pathways to follow).
-    # May build on content and tagging. Nothing below may import it: in
-    # particular, openedx_tagging must never know that CBE exists.
+    # May build on content, catalog and tagging. Nothing below may import it:
+    # in particular, openedx_tagging must never know that CBE exists.
     openedx_learning
 
-    # Content: authoring-side models and APIs.
+    # Content: authoring-side models and APIs. May reference catalog models
+    # (e.g. to associate content with a CourseRun).
     openedx_content
 
+    # Catalog: the enrollable things (course runs, catalog courses, pathways).
+    # Not aware of content; must never import openedx_content. See
+    # docs/openedx_catalog/decisions/0001-catalog-models-usage.rst
+    openedx_catalog
+
     # Tagging is very simple & fundamental. Should probably not depend on any other Django apps.
     openedx_tagging
 
-    # Django utilities. Should not dependend on any of the real apps (above).
+    # Django utilities. Should not depend on any of the real apps (above).
     openedx_django_lib
 
     # This just an empty shell package, to expose the __version__ number.

docs/index.rst

@@ -16,6 +16,7 @@
    openedx_core/decisions/index
 
    openedx_content/index
+   openedx_catalog/index
    openedx_tagging/index
    openedx_learning/index
 

docs/openedx_catalog/decisions/0001-catalog-models-usage.rst

@@ -0,0 +1,81 @@
+.. _openedx-catalog-adr-0001:
+
+1. Role of Catalog
+==================
+
+Status
+------
+
+Accepted and partially implemented.
+
+Context
+-------
+
+``openedx_catalog`` holds the core models for tracking enrollable things (and eventually, enrollments as well).
+
+Specifically, its main models are:
+
+- :class:`CourseRun` (one course run, e.g. "Math 100 2026Fall").
+- :class:`CatalogCourse` (a set of course runs, e.g. "Math 100")
+- :class:`CatalogPathway` (a pathway that learners can enroll in)
+- :class:`PathwayCategory` (learner-facing label for pathway types, e.g. "Masters Degree")
+- :class:`PathwayEnrollment` (tracks enrollment into pathways)
+
+This ADR clarifies how the catalog models are meant to be used.
+
+``openedx_content`` holds the authored, versioned material itself, grouped into :class:`LearningPackage` instances. Until now the direction of the relationship between the two apps has been left open.
+
+Decisions
+---------
+
+1. Catalog entries may be placeholders with no content
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+A :class:`CatalogCourse` or :class:`CourseRun` may exist with no content behind it: as a marketing or enrollment placeholder, as a planned future run, or because its content still lives in modulestore.
+
+The converse guarantee does hold: if a course exists anywhere in the system, it exists as a :class:`CatalogCourse` and :class:`CourseRun` row.
+
+2. The catalog app is not aware of content
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+``openedx_content`` will have a table(s) for tracking the relationship between a :class:`CourseRun` and its content. But the catalog app itself is not aware of content, and does not maintain any relationship between enrollable things (course runs, pathways) and their content.
+
+``openedx_content`` may import and hold foreign keys to ``openedx_catalog``. But ``openedx_catalog`` must never import ``openedx_content``.
+
+``openedx_learning`` (Pathways, Competency-Based Education, and more) and other parts of the platform sit above both.
+
+The resulting order, enforced by the ``src_layering`` contract in ``.importlinter``, is::
+
+    openedx_learning > openedx_content > openedx_catalog > openedx_tagging
+
+The general principle behind this is that changes in how content is represented should not require changes to the catalog app. For example, if we were to change from associating each course run with a :class:`LearningPackage` to associating each course run with an ``OutlineRoot`` in a :class:`LearningPackage` that contains multiple runs, that should not require changes to the catalog app, which would be the case if we used foreign keys from :class:`CourseRun` to :class:`LearningPackage` within the catalog app.
+
+3. Catalog models are the canonical target for course foreign keys
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+For performance and correctness, any Django model in this repository or in ``openedx-platform`` that needs to reference a course should do so with a foreign key to :class:`CourseRun` (or, rarely, :class:`CatalogCourse`), rather than by storing a course key string or pointing at ``CourseOverview`` (although much existing code does not yet follow this new convention).
+
+On the other hand, public REST APIs and events should continue to identify courses by their full string course key and never expose the integer primary keys.
+
+4. Catalog models stay minimal, unversioned, and extended by related models
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Catalog's models like :class:`CatalogCourse` and :class:`CourseRun` carry only identity and a title. They are not versioned, unlike content. Additional course metadata (schedules, grading policy, enrollment options, pricing) will live in dedicated models with a ``ForeignKey`` or ``OneToOneField`` to :class:`CourseRun`, in this app or in others, following the same progressive-enhancement pattern as :ref:`openedx-content-adr-0002`. Whether a given metadata model is versioned, and how, is decided per model and is out of scope here.
+
+Consequences
+------------
+
+- ``openedx_content`` will need a table(s) that associates course content with catalog's course runs, and which ensures that no more than one content outline can be associated with the same :class:`CourseRun`.
+- Code must never assume that content, a ``CourseOverview``, or any other related model exists just because a catalog row does. Content-dependent behavior must check for the relationship and degrade gracefully.
+- Deleting a learning package can never cascade into catalog entries, enrollments, or anything else that hangs off the catalog.
+- A :class:`LearningPackage` can still be created and populated without yet being associated with a course/library/etc.
+- Import Linter will fail any change that makes ``openedx_catalog`` import ``openedx_content``.
+
+Rejected Alternatives
+---------------------
+
+**Another app joins content and catalog.** In this case, ``catalog`` and ``content`` would be wholly independent, prohibited from referencing each other. Another app, like ``openedx_learning``, ``openedx_courses``, or ``cms.contentstore`` would be layered on top and hold the records that associate each catalog with each course. This is a perfectly viable option, and is currently how content libraries are implemented. However, for now it seems simpler and more useful to put the mapping into the ``content`` app directly.
+
+**Peer layering with cross-references.** In this case, we'd state that in general, :class:`LearningPackage` is context agnostic, and catalog models point to :class:`LearningPackage` rather than vice versa, but *within* ``openedx_content`` a new ``PathwayItem`` model allows references to ``CourseRun``. This is probably workable, but lacks the clean separation that we're looking for. It is also a package cycle: ``openedx_catalog`` imports ``openedx_content`` for :class:`LearningPackage` while ``openedx_content`` imports ``openedx_catalog`` for :class:`CourseRun`, which a ``layers`` contract in Import Linter cannot express at all. What's more, ``PathwayItem`` is only useful for the ``pathways`` app, which is presumably optional, so it's not as generic or reusable as the other models offered by ``openedx_content``.
+
+**Catalog layers above the content.** In this case, ``openedx_catalog`` would hold a foreign key from :class:`CourseRun` to :class:`LearningPackage`, but any refactors to how content is stored (e.g. relationship to ``OutlineRoot`` instead of ``LearningPackage``) would require changing this foreign key, which shouldn't be the case.

docs/openedx_catalog/decisions/index.rst

@@ -0,0 +1,12 @@
+.. _openedx-catalog-decisions-index:
+
+Decisions
+=========
+
+Architecture Decision Records for the ``openedx_catalog`` app.
+
+.. toctree::
+   :maxdepth: 1
+   :glob:
+
+   *

docs/openedx_catalog/index.rst

@@ -0,0 +1,12 @@
+.. _openedx-catalog-index:
+
+openedx_catalog
+===============
+
+Django app for the core catalog models (``CatalogCourse``, ``CourseRun``) that define which courses exist in an
+instance, independently of whether their content exists yet.
+
+.. toctree::
+   :maxdepth: 1
+
+   decisions/index

docs/openedx_learning/decisions/0007-pathway-catalog-content-split.rst

@@ -34,7 +34,8 @@ Decisions
 1. A Pathway is split into two parts:
 
    - **Catalog Pathway** - the learner-browsable, enrollable thing. It includes the display name, the description
-     shown in the catalog, SEO metadata, and a **Category**. It is **not versioned**.
+     shown in the catalog, SEO metadata, and a **Category**. It is **not versioned**. Its definition lives in the
+     ``openedx_catalog`` app.
 
    - **Pathway content** - the definition of the Pathway: its Items and its completion criteria. The content is
      **versioned**, so that we can always tell what the definition was at any given moment. A version of the Pathway

src/openedx_catalog/ARCHITECTURE.md

@@ -15,7 +15,7 @@ flowchart TB
     Organizations["**edx-organizations** (Organization)"]
     Enrollments["**platform: enrollments** (CourseEnrollment, CourseEnrollmentAllowed)"]
     Modes["**platform: course_modes** (CourseMode)"]
-    Catalog <-. "Direction of this relationship TBD." .-> Content
+    Content -- "References (see ADR 0001)" --> Catalog
     Catalog -- References --> Organizations
     Enrollments -- References --> Modes
     Enrollments -- References --> Catalog
@@ -24,8 +24,9 @@ flowchart TB
     style Modes fill:#ccc
     style Organizations fill:#ccc
 
-    Pathways["<a href='https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5148147732/Brief+Modular+Content+Delivery+-+Platform+Strategy'>**openedx_pathways**</a> (Pathway, PathwaySchedule, PathwayEnrollment, PathwayCertificate, etc.)"]
+    Pathways["<a href='https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5148147732/Brief+Modular+Content+Delivery+-+Platform+Strategy'>**openedx_learning: pathways**</a> (Pathway definition, PathwayItem, PathwayCertificate, etc. The unversioned CatalogPathway lives in openedx_catalog.)"]
     Pathways -- References --> Catalog
+    Pathways -- References --> Content
 
     style Pathways fill:#c0ffee,stroke-dasharray: 5 5
 

src/openedx_catalog/README.rst

@@ -14,7 +14,7 @@ The existing ``CourseOverview`` model in ``openedx-platform`` is derived from va
 1. Provide a core model to represent each course, for foreign key purposes.
 2. To allow provisioning placeholder courses before any content even exists.
 3. To be much simpler and more performant than ``CourseOverview`` was (far fewer fields generally, fewer legacy fields, integer primary key).
-4. Perhaps to provide a transition mechanism, a pointer than can point either to modulestore or openedx_content, as we transition content storage.
+4. To be independent of how and where content is stored: the catalog is not aware of content, and the mapping from a course run to its content (in modulestore or in ``openedx_content``) is maintained outside this app. See `ADR 0001 <../../docs/openedx_catalog/decisions/0001-catalog-models-usage.rst>`__.
 
 Architecture
 ------------

src/openedx_catalog/models/course_run.py

@@ -75,9 +75,10 @@ class CourseRun(models.Model):
       this catalog app or other apps. They should either be versioned using
       `PublishableEntity` or use the `HistoricalRecords()` history from
       `django-simple-history` to preserve a record of all changes.
-    - In the future, there will be a relationship to Learning Package. Several
-      course runs from the same catalog course may be stored in the same
-      learning package.
+    - This app is deliberately not aware of content. The mapping from a course
+      run to its content (a Learning Package and/or an OutlineRoot container in
+      `openedx_content`) is maintained on the `openedx_content` side, never as a
+      field on this model. See docs/openedx_catalog/decisions/0001.
     """
 
     CourseRunID = NewType("CourseRunID", int)
@@ -168,14 +169,12 @@ def course_code(self) -> str:
 
     # Do we want mix in SoftDeletableModel from django-model-utils to make courses soft deletable?
 
-    # In the future, either this model or CatalogCourse will have:
-    # learning_package = models.ForeignKey(LearningPackage)
-
-    # In the future, this model will likely have a relationship to the
-    # OutlineRoot which would be an `openedx_content` `Container` instance that
-    # holds the conten tree (Sections, Subsections, Units, etc.). For now, if
-    # the content exists, it will be in modulestore instead (you can get the
-    # `SplitModulestoreCourseIndex` using TODO: define API method).
+    # 🛑 Do not add a relationship to LearningPackage, OutlineRoot, or any other
+    #    `openedx_content` model here. The catalog app is not aware of content;
+    #    `openedx_content` holds the table that maps a CourseRun to its content
+    #    (see docs/openedx_catalog/decisions/0001). For now, if the content
+    #    exists, it will be in modulestore instead (you can get the
+    #    `SplitModulestoreCourseIndex` using TODO: define API method).
 
     def clean(self):
         """Defaults and validation of model fields"""

src/openedx_learning/README.rst

@@ -7,5 +7,5 @@ and how they get there. Its sibling ``openedx_content`` holds the material itsel
 Like ``openedx_content``, it is one Django app split into applets. Its first applet is
 ``cbe``, for Competency-Based Education; Learning Pathways are expected to follow.
 
-In the layering that ``.importlinter`` enforces, this app sits above ``openedx_content``
-and ``openedx_tagging``. It may build on either of them; neither may import it.
+In the layering that ``.importlinter`` enforces, this app sits above ``openedx_catalog``,
+``openedx_content`` and ``openedx_tagging``. It may build on any of them; none may import it.