From d8b88851673c2791bc22c34d9168569d6a919fae Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 6 Feb 2026 15:59:21 -0800 Subject: [PATCH 01/29] feat: depend on edx-organizations --- requirements/base.in | 2 ++ requirements/base.txt | 20 ++++++++++++++++++-- requirements/dev.txt | 23 ++++++++++++++++++++++- requirements/doc.txt | 23 ++++++++++++++++++++++- requirements/quality.txt | 23 ++++++++++++++++++++++- requirements/test.txt | 23 ++++++++++++++++++++++- 6 files changed, 108 insertions(+), 6 deletions(-) diff --git a/requirements/base.in b/requirements/base.in index 15bcd9748..508635e10 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -13,3 +13,5 @@ edx-drf-extensions # Extensions to the Django REST Framework used by Open rules<4.0 # Django extension for rules-based authorization checks tomlkit # Parses and writes TOML configuration files + +edx-organizations # Implemented the "Organization" model that CatalogCourse/CourseRun are keyed to diff --git a/requirements/base.txt b/requirements/base.txt index 59dfbb6f0..7349942a2 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -42,13 +42,20 @@ django==5.2.11 # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/base.in # django-crum + # django-model-utils + # django-simple-history # django-waffle # djangorestframework # drf-jwt # edx-django-utils # edx-drf-extensions + # edx-organizations django-crum==0.7.9 # via edx-django-utils +django-model-utils==5.0.0 + # via edx-organizations +django-simple-history==3.11.0 + # via edx-organizations django-waffle==5.0.0 # via # edx-django-utils @@ -58,6 +65,7 @@ djangorestframework==3.16.1 # -r requirements/base.in # drf-jwt # edx-drf-extensions + # edx-organizations dnspython==2.8.0 # via pymongo drf-jwt==1.19.2 @@ -65,15 +73,23 @@ drf-jwt==1.19.2 edx-django-utils==8.0.1 # via edx-drf-extensions edx-drf-extensions==10.6.0 - # via -r requirements/base.in + # via + # -r requirements/base.in + # edx-organizations edx-opaque-keys==3.0.0 - # via edx-drf-extensions + # via + # edx-drf-extensions + # edx-organizations +edx-organizations==7.3.0 + # via -r requirements/base.in idna==3.11 # via requests kombu==5.6.2 # via celery packaging==26.0 # via kombu +pillow==12.1.0 + # via edx-organizations prompt-toolkit==3.0.52 # via click-repl psutil==7.2.2 diff --git a/requirements/dev.txt b/requirements/dev.txt index 209bee996..6338b9cd2 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -133,6 +133,8 @@ django==5.2.11 # -r requirements/quality.txt # django-crum # django-debug-toolbar + # django-model-utils + # django-simple-history # django-stubs # django-stubs-ext # django-waffle @@ -141,6 +143,7 @@ django==5.2.11 # edx-django-utils # edx-drf-extensions # edx-i18n-tools + # edx-organizations django-crum==0.7.9 # via # -r requirements/quality.txt @@ -149,6 +152,14 @@ django-debug-toolbar==6.2.0 # via # -r requirements/dev.in # -r requirements/quality.txt +django-model-utils==5.0.0 + # via + # -r requirements/quality.txt + # edx-organizations +django-simple-history==3.11.0 + # via + # -r requirements/quality.txt + # edx-organizations django-stubs==5.2.9 # via # -r requirements/quality.txt @@ -167,6 +178,7 @@ djangorestframework==3.16.1 # -r requirements/quality.txt # drf-jwt # edx-drf-extensions + # edx-organizations djangorestframework-stubs==3.16.8 # via -r requirements/quality.txt dnspython==2.8.0 @@ -186,7 +198,9 @@ edx-django-utils==8.0.1 # -r requirements/quality.txt # edx-drf-extensions edx-drf-extensions==10.6.0 - # via -r requirements/quality.txt + # via + # -r requirements/quality.txt + # edx-organizations edx-i18n-tools==1.9.0 # via -r requirements/dev.in edx-lint==5.6.0 @@ -195,6 +209,9 @@ edx-opaque-keys==3.0.0 # via # -r requirements/quality.txt # edx-drf-extensions + # edx-organizations +edx-organizations==7.3.0 + # via -r requirements/quality.txt fastapi==0.131.0 # via # -r requirements/quality.txt @@ -330,6 +347,10 @@ pathspec==1.0.4 # via # -r requirements/quality.txt # mypy +pillow==12.1.0 + # via + # -r requirements/quality.txt + # edx-organizations pip-tools==7.5.3 # via -r requirements/pip-tools.txt platformdirs==4.9.2 diff --git a/requirements/doc.txt b/requirements/doc.txt index 2e8b7ff8c..7b689d45b 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -96,6 +96,8 @@ django==5.2.11 # -r requirements/test.txt # django-crum # django-debug-toolbar + # django-model-utils + # django-simple-history # django-stubs # django-stubs-ext # django-waffle @@ -103,6 +105,7 @@ django==5.2.11 # drf-jwt # edx-django-utils # edx-drf-extensions + # edx-organizations # sphinxcontrib-django django-crum==0.7.9 # via @@ -110,6 +113,14 @@ django-crum==0.7.9 # edx-django-utils django-debug-toolbar==6.2.0 # via -r requirements/test.txt +django-model-utils==5.0.0 + # via + # -r requirements/test.txt + # edx-organizations +django-simple-history==3.11.0 + # via + # -r requirements/test.txt + # edx-organizations django-stubs==5.2.9 # via # -r requirements/test.txt @@ -128,6 +139,7 @@ djangorestframework==3.16.1 # -r requirements/test.txt # drf-jwt # edx-drf-extensions + # edx-organizations djangorestframework-stubs==3.16.8 # via -r requirements/test.txt dnspython==2.8.0 @@ -152,11 +164,16 @@ edx-django-utils==8.0.1 # -r requirements/test.txt # edx-drf-extensions edx-drf-extensions==10.6.0 - # via -r requirements/test.txt + # via + # -r requirements/test.txt + # edx-organizations edx-opaque-keys==3.0.0 # via # -r requirements/test.txt # edx-drf-extensions + # edx-organizations +edx-organizations==7.3.0 + # via -r requirements/test.txt fastapi==0.131.0 # via # -r requirements/test.txt @@ -232,6 +249,10 @@ pathspec==1.0.4 # via # -r requirements/test.txt # mypy +pillow==12.1.0 + # via + # -r requirements/test.txt + # edx-organizations pluggy==1.6.0 # via # -r requirements/test.txt diff --git a/requirements/quality.txt b/requirements/quality.txt index d012bfed4..5fd7d0126 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -101,6 +101,8 @@ django==5.2.11 # -r requirements/test.txt # django-crum # django-debug-toolbar + # django-model-utils + # django-simple-history # django-stubs # django-stubs-ext # django-waffle @@ -108,12 +110,21 @@ django==5.2.11 # drf-jwt # edx-django-utils # edx-drf-extensions + # edx-organizations django-crum==0.7.9 # via # -r requirements/test.txt # edx-django-utils django-debug-toolbar==6.2.0 # via -r requirements/test.txt +django-model-utils==5.0.0 + # via + # -r requirements/test.txt + # edx-organizations +django-simple-history==3.11.0 + # via + # -r requirements/test.txt + # edx-organizations django-stubs==5.2.9 # via # -r requirements/test.txt @@ -132,6 +143,7 @@ djangorestframework==3.16.1 # -r requirements/test.txt # drf-jwt # edx-drf-extensions + # edx-organizations djangorestframework-stubs==3.16.8 # via -r requirements/test.txt dnspython==2.8.0 @@ -149,13 +161,18 @@ edx-django-utils==8.0.1 # -r requirements/test.txt # edx-drf-extensions edx-drf-extensions==10.6.0 - # via -r requirements/test.txt + # via + # -r requirements/test.txt + # edx-organizations edx-lint==5.6.0 # via -r requirements/quality.in edx-opaque-keys==3.0.0 # via # -r requirements/test.txt # edx-drf-extensions + # edx-organizations +edx-organizations==7.3.0 + # via -r requirements/test.txt fastapi==0.131.0 # via # -r requirements/test.txt @@ -253,6 +270,10 @@ pathspec==1.0.4 # via # -r requirements/test.txt # mypy +pillow==12.1.0 + # via + # -r requirements/test.txt + # edx-organizations platformdirs==4.9.2 # via pylint pluggy==1.6.0 diff --git a/requirements/test.txt b/requirements/test.txt index 49854ba83..9e6755778 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -79,6 +79,8 @@ ddt==1.7.2 # -r requirements/base.txt # django-crum # django-debug-toolbar + # django-model-utils + # django-simple-history # django-stubs # django-stubs-ext # django-waffle @@ -86,12 +88,21 @@ ddt==1.7.2 # drf-jwt # edx-django-utils # edx-drf-extensions + # edx-organizations django-crum==0.7.9 # via # -r requirements/base.txt # edx-django-utils django-debug-toolbar==6.2.0 # via -r requirements/test.in +django-model-utils==5.0.0 + # via + # -r requirements/base.txt + # edx-organizations +django-simple-history==3.11.0 + # via + # -r requirements/base.txt + # edx-organizations django-stubs==5.2.9 # via # -r requirements/test.in @@ -108,6 +119,7 @@ djangorestframework==3.16.1 # -r requirements/base.txt # drf-jwt # edx-drf-extensions + # edx-organizations djangorestframework-stubs==3.16.8 # via -r requirements/test.in dnspython==2.8.0 @@ -123,11 +135,16 @@ edx-django-utils==8.0.1 # -r requirements/base.txt # edx-drf-extensions edx-drf-extensions==10.6.0 - # via -r requirements/base.txt + # via + # -r requirements/base.txt + # edx-organizations edx-opaque-keys==3.0.0 # via # -r requirements/base.txt # edx-drf-extensions + # edx-organizations +edx-organizations==7.3.0 + # via -r requirements/base.txt fastapi==0.131.0 # via import-linter freezegun==1.5.5 @@ -174,6 +191,10 @@ packaging==26.0 # pytest pathspec==1.0.4 # via mypy +pillow==12.1.0 + # via + # -r requirements/base.txt + # edx-organizations pluggy==1.6.0 # via # pytest From 1c225aa54cbdbd0e7e9abfa89f4f0094c74d2a7a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 13 Feb 2026 11:50:34 -0800 Subject: [PATCH 02/29] feat: new catalog app to model course runs and catalog courses --- projects/dev.py | 3 + src/openedx_catalog/ARCHITECTURE.md | 35 +++ src/openedx_catalog/README.rst | 22 ++ src/openedx_catalog/__init__.py | 0 src/openedx_catalog/admin.py | 84 ++++++ src/openedx_catalog/apps.py | 19 ++ .../migrations/0001_initial.py | 207 ++++++++++++++ src/openedx_catalog/migrations/__init__.py | 0 src/openedx_catalog/models/__init__.py | 6 + src/openedx_catalog/models/catalog_course.py | 173 ++++++++++++ src/openedx_catalog/models/course_run.py | 230 +++++++++++++++ src/openedx_catalog/py.typed | 2 + test_settings.py | 3 + tests/openedx_catalog/__init__.py | 0 tests/openedx_catalog/test_models.py | 267 ++++++++++++++++++ 15 files changed, 1051 insertions(+) create mode 100644 src/openedx_catalog/ARCHITECTURE.md create mode 100644 src/openedx_catalog/README.rst create mode 100644 src/openedx_catalog/__init__.py create mode 100644 src/openedx_catalog/admin.py create mode 100644 src/openedx_catalog/apps.py create mode 100644 src/openedx_catalog/migrations/0001_initial.py create mode 100644 src/openedx_catalog/migrations/__init__.py create mode 100644 src/openedx_catalog/models/__init__.py create mode 100644 src/openedx_catalog/models/catalog_course.py create mode 100644 src/openedx_catalog/models/course_run.py create mode 100644 src/openedx_catalog/py.typed create mode 100644 tests/openedx_catalog/__init__.py create mode 100644 tests/openedx_catalog/test_models.py diff --git a/projects/dev.py b/projects/dev.py index 58c39eb53..c4961316d 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -34,6 +34,9 @@ "django.contrib.admin", "django.contrib.admindocs", + # Open edX Organizations (dependency for openedx_catalog) + "organizations", + # Our Apps "openedx_tagging", "openedx_content", diff --git a/src/openedx_catalog/ARCHITECTURE.md b/src/openedx_catalog/ARCHITECTURE.md new file mode 100644 index 000000000..6feda9546 --- /dev/null +++ b/src/openedx_catalog/ARCHITECTURE.md @@ -0,0 +1,35 @@ +# Catalog App Architecture Diagram + +Here's a visual overview of how this app relates to other apps. + +(_Note: to see the diagram below, view this on GitHub or view in VS Code with [a Markdown-Mermaid extension](https://marketplace.visualstudio.com/items?itemName=bierner.markdown-mermaid) enabled._) + +```mermaid +--- +config: + theme: 'forest' +--- +flowchart TB + Catalog["**openedx_catalog** (CourseRun, CatalogCourse plus core metadata models, e.g. CourseSchedule. Other metadata models live in other apps but are 1:1 with CourseRun.)"] + Content["**openedx_content**
The content of the course. (publishing, containers, components, media)"] + Organizations["**edx-organizations** (Organization)"] + Enrollments["**platform: enrollments** (CourseEnrollment, CourseEnrollmentAllowed)"] + Modes["**platform: course_modes** (CourseMode)"] + Catalog <-. "Direction of this relationship TBD." .-> Content + Catalog -- References --> Organizations + Enrollments -- References --> Modes + Enrollments -- References --> Catalog + + style Enrollments fill:#ccc + style Modes fill:#ccc + style Organizations fill:#ccc + + Pathways["**openedx_pathways** (Pathway, PathwaySchedule, PathwayEnrollment, PathwayCertificate, etc.)"] + Pathways -- References --> Catalog + + style Pathways fill:#c0ffee,stroke-dasharray: 5 5 + + FutureCatalog["Future discovery service - learner-oriented, pluggable, browse/search courses and programs"] -- References --> Catalog + FutureCatalog <-- Plugin API --> Pathways + style FutureCatalog fill:#ffc0ee,stroke-dasharray: 5 5 +``` \ No newline at end of file diff --git a/src/openedx_catalog/README.rst b/src/openedx_catalog/README.rst new file mode 100644 index 000000000..f7ca28a0c --- /dev/null +++ b/src/openedx_catalog/README.rst @@ -0,0 +1,22 @@ +Learning Core: Catalog App +========================== + +Overview +-------- + +The ``openedx_catalog`` Django apps provides core models to represent all courses in the Open edX platform. Higher-level apps can build on these models to implement features like enrollment, grading, scheduling, exams, and much more. + +Motivation +---------- + +The existing ``CourseOverview`` model in ``openedx-platform`` is derived from various places, but mostly from the metadata fields of the root ``Course`` object stored in modulestore (MongoDB) for each course. As we slowly transition toward storing course content fully in Learning Core (in ``openedx_content``), we want to move to storing all course data and metadata in these sort of MySQL models. We're creating this new ``CourseRun`` model in ``openedx_catalog`` to support these goals: + +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 content or learning core content, as we transition content storage. + +Architecture +------------ + +See `the architecture diagram <./ARCHITECTURE.md>`__. (Because we use RST for all Python READMEs, it cannot be embedded here directly.) diff --git a/src/openedx_catalog/__init__.py b/src/openedx_catalog/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/src/openedx_catalog/admin.py b/src/openedx_catalog/admin.py new file mode 100644 index 000000000..b05e25d3b --- /dev/null +++ b/src/openedx_catalog/admin.py @@ -0,0 +1,84 @@ +""" +Django Admin pages for openedx_catalog. +""" + +import datetime + +from django.contrib import admin +from django.db.models import Count +from django.urls import reverse +from django.utils.html import format_html +from django.utils.translation import gettext_lazy as _ + +from .models import CatalogCourse, CourseRun + + +class CatalogCourseAdmin(admin.ModelAdmin): + """ + The CatalogCourse model admin. + """ + + list_filter = ["org__short_name", "language"] + list_display = ["display_name", "org_display", "course_code", "runs_summary", "created_date", "language"] + + def get_readonly_fields(self, request, obj: CatalogCourse | None = None): + if obj: # editing an existing object + return self.readonly_fields + ("org", "course_code") + return self.readonly_fields + + def get_queryset(self, request): + """Add the 'run_count' to the list_display queryset""" + qs = super().get_queryset(request) + qs = qs.annotate(run_count=Count("runs")) + return qs + + @admin.display(description="Organization", ordering="org__short_name") + def org_display(self, obj: CatalogCourse) -> str: + """Display the organization, only showing the short_name if different from full name""" + if obj.org.name == obj.org.short_name: + return obj.org.short_name + return str(obj.org) + + @admin.display(description=_("Created"), ordering="created") + def created_date(self, obj: CatalogCourse) -> datetime.date: + """Display the created date without the timestamp""" + return obj.created.date() + + @admin.display(description=_("Runs")) + def runs_summary(self, obj: CatalogCourse) -> str: + """Summarize the runs""" + if obj.run_count == 0: + return "-" + url = reverse("admin:openedx_catalog_courserun_changelist") + f"?catalog_course={obj.pk}" + first_few_runs = obj.runs.order_by("-run")[:3] + runs_summary = ", ".join(run.run for run in first_few_runs) + if obj.run_count > 4: + runs_summary += f", ... ({obj.runs_count})" + return format_html('{}', url, runs_summary) + + +admin.site.register(CatalogCourse, CatalogCourseAdmin) + + +class CourseRunAdmin(admin.ModelAdmin): + """ + The CourseRun model admin. + """ + + list_display = ["display_name", "created_date", "catalog_course", "run", "org_code"] + readonly_fields = ("course_id",) + # There may be thousands of catalog courses, so don't use raw_id_fields = ["catalog_course"] @@ -76,9 +76,20 @@ def get_readonly_fields(self, request, obj: CourseRun | None = None): return self.readonly_fields @admin.display(description=_("Created"), ordering="created") - def created_date(self, obj: CatalogCourse) -> datetime.date: + def created_date(self, obj: CourseRun) -> datetime.date: """Display the created date without the timestamp""" return obj.created.date() + def warnings(self, obj: CourseRun) -> str: + """Display warnings of any detected issues""" + if obj.course_code != obj.catalog_course.course_code: + return "🚨 Critical: mismatched course code" + if obj.org_code != obj.catalog_course.org.short_name: + if obj.org_code.lower() == obj.catalog_course.org.short_name.lower(): + return "⚠️ Warning: Incorrect org code capitalization" + return "🚨 Critical: mismatched org code" + # It would be nice to indicate if there's associated course content or not, but openedx-core isn't aware of + # modulestore so we have no way to check that here. + admin.site.register(CourseRun, CourseRunAdmin) diff --git a/src/openedx_catalog/models/course_run.py b/src/openedx_catalog/models/course_run.py index 85eabd86a..31e235d46 100644 --- a/src/openedx_catalog/models/course_run.py +++ b/src/openedx_catalog/models/course_run.py @@ -147,6 +147,7 @@ def org_code(self) -> str: return self.course_id.org @property + @admin.display(ordering="catalog_course__course_code") def course_code(self) -> str: """Get the course code of this course, e.g. "Math100" """ # Note: 'self.catalog_course.course_code' may require a JOIN/query, but self.course_id.course does not. From cd1345b9791a73f001fd69532f9abb98f02c8814 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 19 Feb 2026 10:45:20 -0800 Subject: [PATCH 05/29] chore: fix mypy warnings --- projects/dev.py | 1 + src/openedx_catalog/admin.py | 25 +++++++++++++------- src/openedx_catalog/models/catalog_course.py | 6 ++--- src/openedx_catalog/signals.py | 2 +- tests/openedx_catalog/test_models.py | 6 ++++- tests/openedx_catalog/test_signals.py | 2 +- 6 files changed, 28 insertions(+), 14 deletions(-) diff --git a/projects/dev.py b/projects/dev.py index c4961316d..277a78598 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -38,6 +38,7 @@ "organizations", # Our Apps + "openedx_catalog", "openedx_tagging", "openedx_content", *openedx_content_backcompat_apps_to_install(), diff --git a/src/openedx_catalog/admin.py b/src/openedx_catalog/admin.py index 06b14c759..4679e79d9 100644 --- a/src/openedx_catalog/admin.py +++ b/src/openedx_catalog/admin.py @@ -2,16 +2,24 @@ Django Admin pages for openedx_catalog. """ +from __future__ import annotations + import datetime +from typing import TYPE_CHECKING from django.contrib import admin -from django.db.models import Count +from django.db.models import Count, QuerySet from django.urls import reverse from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ from .models import CatalogCourse, CourseRun +if TYPE_CHECKING: + + class CatalogCourseWithRunCount(CatalogCourse): + run_count: int + class CatalogCourseAdmin(admin.ModelAdmin): """ @@ -21,12 +29,12 @@ class CatalogCourseAdmin(admin.ModelAdmin): list_filter = ["org__short_name", "language"] list_display = ["display_name", "org_display", "course_code", "runs_summary", "created_date", "language"] - def get_readonly_fields(self, request, obj: CatalogCourse | None = None): + def get_readonly_fields(self, request, obj: CatalogCourse | None = None) -> tuple[str, ...]: if obj: # editing an existing object - return self.readonly_fields + ("org", "course_code") - return self.readonly_fields + return ("org", "course_code") + return tuple() - def get_queryset(self, request): + def get_queryset(self, request) -> QuerySet[CatalogCourseWithRunCount]: """Add the 'run_count' to the list_display queryset""" qs = super().get_queryset(request) qs = qs.annotate(run_count=Count("runs")) @@ -45,7 +53,7 @@ def created_date(self, obj: CatalogCourse) -> datetime.date: return obj.created.date() @admin.display(description=_("Runs")) - def runs_summary(self, obj: CatalogCourse) -> str: + def runs_summary(self, obj: CatalogCourseWithRunCount) -> str: """Summarize the runs""" if obj.run_count == 0: return "-" @@ -53,7 +61,7 @@ def runs_summary(self, obj: CatalogCourse) -> str: first_few_runs = obj.runs.order_by("-run")[:3] runs_summary = ", ".join(run.run for run in first_few_runs) if obj.run_count > 4: - runs_summary += f", ... ({obj.runs_count})" + runs_summary += f", ... ({obj.run_count})" return format_html('{}', url, runs_summary) @@ -80,7 +88,7 @@ def created_date(self, obj: CourseRun) -> datetime.date: """Display the created date without the timestamp""" return obj.created.date() - def warnings(self, obj: CourseRun) -> str: + def warnings(self, obj: CourseRun) -> str | None: """Display warnings of any detected issues""" if obj.course_code != obj.catalog_course.course_code: return "🚨 Critical: mismatched course code" @@ -90,6 +98,7 @@ def warnings(self, obj: CourseRun) -> str: return "🚨 Critical: mismatched org code" # It would be nice to indicate if there's associated course content or not, but openedx-core isn't aware of # modulestore so we have no way to check that here. + return None admin.site.register(CourseRun, CourseRunAdmin) diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index 3e96f4a52..cff7b8860 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -10,7 +10,7 @@ from django.db.models.functions import Length from django.db.models.lookups import Regex from django.utils.translation import gettext_lazy as _ -from organizations.models import Organization +from organizations.models import Organization # type: ignore[import] from openedx_django_lib.fields import case_insensitive_char_field, case_sensitive_char_field from openedx_django_lib.validators import validate_utc_datetime @@ -66,7 +66,7 @@ class CatalogCourse(models.Model): # Initially, I had to_field="short_name" here, which has the nice property that we can look up an org's # short_name without doing a JOIN. But that also prevents changing the org's short_name, which could be # necessary to fix capitalization problems. (We wouldn't want to allow other changes to an org's short_name - # though; only fixing capitalization.) + # though; only fixing capitalization - see openedx_catalog.signals.verify_organization_change.) ) course_code = case_insensitive_char_field( max_length=255, @@ -142,7 +142,7 @@ def save(self, *args, **kwargs): self.display_name = self.course_code super().save(*args, **kwargs) - def __str__(self): + def __str__(self) -> str: return f"{self.display_name} ({self.org_code} {self.course_code})" class Meta: diff --git a/src/openedx_catalog/signals.py b/src/openedx_catalog/signals.py index e01fe64c2..0031ac12b 100644 --- a/src/openedx_catalog/signals.py +++ b/src/openedx_catalog/signals.py @@ -8,7 +8,7 @@ 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 organizations.models import Organization # type: ignore[import] from .models import CourseRun diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index a622c2cb0..bed393631 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -1,6 +1,9 @@ """ Tests related to the Catalog models (CatalogCourse, CourseRun) """ +# mypy: disable-error-code="misc" +# (Ignore 'Unexpected attribute "org_code" for model "CatalogCourse"' until +# https://github.com/typeddjango/django-stubs/issues/1034 is fixed.) import ddt # type: ignore[import] import pytest @@ -10,7 +13,7 @@ from django.test import TestCase, override_settings from opaque_keys.edx.locator import CourseLocator from organizations.api import ensure_organization # type: ignore[import] -from organizations.models import Organization +from organizations.models import Organization # type: ignore[import] from openedx_catalog.models import CatalogCourse, CourseRun @@ -150,6 +153,7 @@ class TestCourseRunModel(TestCase): """ Low-level tests of the CourseRun model. """ + catalog_course: CatalogCourse @classmethod def setUpClass(cls): diff --git a/tests/openedx_catalog/test_signals.py b/tests/openedx_catalog/test_signals.py index 7a4bf36e0..b4c5940d5 100644 --- a/tests/openedx_catalog/test_signals.py +++ b/tests/openedx_catalog/test_signals.py @@ -8,7 +8,7 @@ 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 +from organizations.models import Organization # type: ignore[import] from openedx_catalog.models import CatalogCourse, CourseRun From 4497b69eddc048716e9c9dcf419758729622ce33 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 14:05:00 -0800 Subject: [PATCH 06/29] fix: better normalization of language codes for catalog courses. --- .../migrations/0001_initial.py | 6 +- src/openedx_catalog/models/catalog_course.py | 67 +++++++++++++++++-- tests/openedx_catalog/test_models.py | 27 +++++++- 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/src/openedx_catalog/migrations/0001_initial.py b/src/openedx_catalog/migrations/0001_initial.py index 6f5ff5da2..23bffae6f 100644 --- a/src/openedx_catalog/migrations/0001_initial.py +++ b/src/openedx_catalog/migrations/0001_initial.py @@ -60,7 +60,7 @@ class Migration(migrations.Migration): openedx_django_lib.fields.MultiCollationCharField( db_collations={"mysql": "utf8mb4_bin", "sqlite": "BINARY"}, default=openedx_catalog.models.catalog_course.get_default_language_code, - help_text='The code representing the language of this catalog course\'s content. The first two digits must be the lowercase ISO 639-1 language code. e.g. "en", "es", "en-us", "pt-br". ', + help_text='The code representing the language of this catalog course\'s content. The first two digits must be the lowercase ISO 639-1 language code, optionally followed by a country/locale code. e.g. "en", "es", "fr-ca", "pt-br", "zh-cn", "zh-hk". ', max_length=64, ), ), @@ -159,9 +159,9 @@ class Migration(migrations.Migration): migrations.AddConstraint( model_name="catalogcourse", constraint=models.CheckConstraint( - condition=django.db.models.lookups.Regex(models.F("language"), "^[a-z][a-z]((\\-|@)[a-z]+)?$"), + condition=django.db.models.lookups.Regex(models.F("language"), "^[a-z][a-z](\\-[a-z0-9]+)*$"), name="oex_catalog_catalogcourse_language_regex", - violation_error_message='The language code must be lowercase, e.g. "en". If a country/locale code is provided, it must be separated by a hyphen or @ sign, e.g. "en-us", "zh-hk", or "ca@valencia". ', + violation_error_message='The language code must be lowercase, e.g. "en". If a country/locale code is provided, it must be separated by a hyphen, e.g. "en-us", "zh-hk". ', ), ), migrations.AddConstraint( diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index cff7b8860..151dd1fba 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -19,7 +19,13 @@ def get_default_language_code() -> str: - return settings.LANGUAGE_CODE + """ + Default language code used for CatalogCourse.language + + Note: this function is used in migration 0001 so update that migration if + moving it or changing its signature. + """ + return settings.LANGUAGE_CODE # e.g. "en-us", "fr-ca" # Make 'length' available for CHECK constraints. OK if this is called multiple times. @@ -93,6 +99,11 @@ class CatalogCourse(models.Model): 'Individual course runs may override this, e.g. "Into to Calc (Fall 2026 with Dr. Newton)".' ), ) + # Note: language codes used on the Open edX platform are inconsistent. + # See https://github.com/openedx/openedx-platform/issues/38036 + # For this model going forward, we normalized them to match settings.LANGUAGES (en, fr-ca, zh-cn, zh-hk) but for + # backwards compatibility, you can get/set the language_short field which uses the mostly two-letter values from + # the platform's ALL_LANGUAGES setting (en, fr, es, zh_HANS, zh_HANT). language = case_sensitive_char_field( # Case sensitive but constraints force it to be lowercase. max_length=64, blank=False, @@ -100,8 +111,9 @@ class CatalogCourse(models.Model): default=get_default_language_code, help_text=_( "The code representing the language of this catalog course's content. " - "The first two digits must be the lowercase ISO 639-1 language code. " - 'e.g. "en", "es", "en-us", "pt-br". ' + "The first two digits must be the lowercase ISO 639-1 language code, " + "optionally followed by a country/locale code. " + 'e.g. "en", "es", "fr-ca", "pt-br", "zh-cn", "zh-hk". ' ), ) @@ -109,6 +121,37 @@ class CatalogCourse(models.Model): # we don't need "description", "visibility", "prereqs", or anything else. If you want to use such fields and # expose them to users, you'll need to supplement this with additional data/models as mentioned in the docstring. + @property + def language_short(self) -> str: + """ + Get the language code used by this catalog course, without locale. + This is always a two-digit code, except for Mandarin and Cantonese. + (This should be a value from settings.ALL_LANGUAGES, and should match + the CourseOverview.language field.) + """ + if self.language == "zh-cn": # Chinese (Mainland China) + return "zh_HANS" # Mandarin / Simplified + elif self.language == "zh-hk": # Chinese (Hong Kong) + return "zh_HANT" # Cantonese / Traditional + return self.language[:2] # Strip locale + + @language_short.setter + def language_short(self, legacy_code: str) -> str: + """ + Set the language code used by this catalog course, without locale. + This is always a two-digit code, except for Mandarin and Cantonese. + (This should be a value from settings.ALL_LANGUAGES, and should match + the CourseOverview.language field.) + """ + if hasattr(settings, "ALL_LANGUAGES"): + assert legacy_code in [code for (code, _name) in settings.ALL_LANGUAGES] + if legacy_code == "zh_HANS": # Mandarin / Simplified + self.language = "zh-cn" # Chinese (Mainland China) + elif legacy_code == "zh_HANT": # Cantonese / Traditional + self.language = "zh-hk" # Chinese (Hong Kong) + else: + self.language = legacy_code + @property @admin.display(ordering="org__short_name") def org_code(self) -> str: @@ -135,11 +178,21 @@ def org_code(self, org_code: str) -> None: # not possible on MySQL, using the default collation used by the Organizations app. # So we do not worry about that possibility here. - def save(self, *args, **kwargs): - """Save the model, with defaults and validation.""" + def clean(self): + """Validate/normalize fields when edited via Django admin""" # Set a default value for display_name: if not self.display_name: self.display_name = self.course_code + # Normalize language codes to match settings.LANGUAGES. + # It's safe to assume language is lowercase here, because if it's not the DB will reject its CHECK constraint. + if self.language == "zh-hans": + self.language = "zh-cn" + if self.language == "zh-hant": + self.language = "zh-hk" + + def save(self, *args, **kwargs): + """Save the model, with some defaults and validation.""" + self.clean() super().save(*args, **kwargs) def __str__(self) -> str: @@ -163,11 +216,11 @@ class Meta: ), # Language code must be lowercase, and locale codes separated by "-" (django convention) not "_" models.CheckConstraint( - condition=Regex(models.F("language"), r"^[a-z][a-z]((\-|@)[a-z]+)?$"), + condition=Regex(models.F("language"), r"^[a-z][a-z](\-[a-z0-9]+)*$"), name="oex_catalog_catalogcourse_language_regex", violation_error_message=_( 'The language code must be lowercase, e.g. "en". If a country/locale code is provided, ' - 'it must be separated by a hyphen or @ sign, e.g. "en-us", "zh-hk", or "ca@valencia". ' + 'it must be separated by a hyphen, e.g. "en-us", "zh-hk". ' ), ), ] diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index bed393631..9f0ce6a05 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -116,16 +116,21 @@ def test_language_code_default2(self) -> None: ("en", True), ("en-us", True), ("fr", True), + ("fr-fr", True), + ("fr-ca", True), ("pt-br", True), - ("ca@valencia", True), # This is one of the valid values in openedx-platform's default LANGUAGES setting + ("es-419", True), # Spanish (Latin America) + ("ca-es-valencia", True), # Catalan (Valencia) # ❌ Invalid language codes: ("", False), ("x", False), ("EN", False), # must be lowercase ("en-US", False), # must be lowercase ("en_us", False), # hyphen, not underscore, for consistency + ("en--us", False), # typo ("English", False), ("english", False), + ("ca@valencia", False), # Don't support old gettext-style locales; should be "ca-es-valencia" ) @ddt.unpack def test_language_code_validation(self, language_code: str, valid: bool) -> None: @@ -147,12 +152,32 @@ def create(): org_code="Org1", course_code="Python100", language=language_code, display_name="x" ).full_clean() + @ddt.data( + # input field, expected .language value, expected .language_short value + ({"language": "fr"}, "fr", "fr"), + ({"language": "fr-ca"}, "fr-ca", "fr"), + ({"language": "zh-cn"}, "zh-cn", "zh_HANS"), + ({"language": "zh-hk"}, "zh-hk", "zh_HANT"), + ({"language_short": "zh_HANS"}, "zh-cn", "zh_HANS"), + ({"language_short": "zh_HANT"}, "zh-hk", "zh_HANT"), + ({"language_short": "fr"}, "fr", "fr"), + ({"language": "zh-hans"}, "zh-cn", "zh_HANS"), # Input is invalid but gets corrected by clean() + ({"language": "zh-hant"}, "zh-hk", "zh_HANT"), # Input is invalid but gets corrected by clean() + ) + @ddt.unpack + def test_language_code_compatibility(self, kwargs: dict, expected_full_lang: str, expected_short: str) -> None: + """Test that the language_short field is fully backwards compatible with CourseOverview.language""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Locale100", **kwargs) + assert cc.language == expected_full_lang + assert cc.language_short == expected_short + @ddt.ddt class TestCourseRunModel(TestCase): """ Low-level tests of the CourseRun model. """ + catalog_course: CatalogCourse @classmethod From b13c4657465e85a57a677b29abe5027bfaafce64 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 16:55:14 -0800 Subject: [PATCH 07/29] feat: add an initial API for catalog courses / course runs --- src/openedx_catalog/api.py | 30 +++++ src/openedx_catalog/api_impl.py | 177 ++++++++++++++++++++++++++++++ src/openedx_catalog/models_api.py | 9 ++ 3 files changed, 216 insertions(+) create mode 100644 src/openedx_catalog/api.py create mode 100644 src/openedx_catalog/api_impl.py create mode 100644 src/openedx_catalog/models_api.py diff --git a/src/openedx_catalog/api.py b/src/openedx_catalog/api.py new file mode 100644 index 000000000..d9afd84d9 --- /dev/null +++ b/src/openedx_catalog/api.py @@ -0,0 +1,30 @@ +""" +Open edX Core Catalog API + +Note: this is currently a very minimal API. At this point, the openedx_catalog +app mainly exists to provide core models that represent "catalog courses" and +"course runs" for use by foreign keys across the system. + +If a course "exists" in the system, you can trust that it will exist as a +CatalogCourse and CourseRun row in this openedx_catalog app, and use those as +needed when creating foreign keys in various apps. This should be much more +efficient than storing the full course ID as a string or creating a foreign key +to the (large) CourseOverview table. + +Note that the opposite does not hold. Admins can now create CourseRuns and/or +CatalogCourses that don't yet have any content attached. So you may find entries +in this openedx_catalog app that don't correspond to courses in modulestore. + +In addition, we currently do not account for which courses should be visible to +which users. So this API does not yet provide any "list courses" methods. In the +future, the catalog API will be extended to implement course listing along with +pluggable logic for managing multiple catalogs of courses that can account for +instance-specific logic (e.g. enterprise, subscriptions, white labelling) when +determining which courses are visible to which users. +""" + +# Import only the public API methods denoted with __all__ +# pylint: disable=wildcard-import +from .api_impl import * + +# You'll also want the models from .models_api diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py new file mode 100644 index 000000000..fb3f7d7e0 --- /dev/null +++ b/src/openedx_catalog/api_impl.py @@ -0,0 +1,177 @@ +""" +Implementation of the `openedx_catalog` API. +""" + +import logging + +from django.conf import settings +from opaque_keys.edx.keys import CourseKey +from organizations.api import ensure_organization +from organizations.api import exceptions as org_exceptions + +from .models import CatalogCourse, CourseRun + +log = logging.getLogger(__name__) + +# These are the public API methods that anyone can use +__all__ = [ + "get_catalog_course", + "get_course_run", + "sync_course_run_details", + "create_course_run_for_modulestore_course_with", + "update_catalog_course", +] + + +def get_catalog_course(org_code: str, course_code: str) -> CatalogCourse: + """ + Get a catalog course (set of runs). + + Does not check permissions nor load related models. + + The CatalogCourse may not have any runs associated with it. + """ + return CatalogCourse.objects.get(org__short_name=org_code, course_code=course_code) + + +def get_course_run(course_id: CourseKey) -> CourseRun: + """ + Get a single course run. Does not check permissions nor load related models. + + The CourseRun may or may not have content associated with it. + """ + return CourseRun.objects.get(course_id__exact=course_id) + + +def sync_course_run_details( + course_id: CourseKey, + *, + display_name: str | None, # Specify a string to change the display name. +) -> None: + """ + Update a `CourseRun` with details from a more authoritative model (e.g. + `CourseOverview`). Currently the only field that can be updated is + `display_name`. + + The name of this function reflects the fact that the `CourseRun` model is + not currently a source of truth. So it's not a "rename the course" API, but + rather a "some other part of the system already renamed the course" API, + during a transition period until `CourseRun` is the main source of truth. + + Once `CourseRun` is the main source of truth, this will be replaced with a + `update_course_run` API that will become the main way to rename a course. + + ⚠️ Does not check permissions. + """ + run = CourseRun.objects.get(course_id=course_id) + if display_name: + run.display_name = display_name + run.save(update_fields=["display_name"]) + + +def create_course_run_for_modulestore_course_with( + course_id: CourseKey, + *, + display_name: str, + # The short language code (one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" + language_short: str | None = None, +) -> CourseRun: + """ + Create a `CourseRun` (and, if necessary, its corresponding `CatalogCourse`). + This API is meant to be used for data synchonrization purposes (keeping the + new catalog models in sync with modulestore), and is not a generic "create a + course run" API. + + If the `CourseRun` already exists, this will log a warning. + + The `created` timestamp of the `CourseRun` will be set to now, so this is + not meant for historical data (use a data migration). + + ⚠️ Does not check permissions. + """ + # Note: this code shares a lot with the code in + # openedx-platform/openedx/core/djangoapps/content/course_overviews/migrations/0030_backfill_... + # but migrations should generally represent a point-in-time transformation, not call an API method that may continue + # to be developed. So even though it's not DRY, the code is repeated here. + + org_code = course_id.org + course_code = course_id.course + try: + cc = CatalogCourse.objects.get(org__short_name=org_code, course_code=course_code) + except CatalogCourse.DoesNotExist: + cc = None + + if not cc: + # Create the catalog course. + + # First, ensure that the Organization exists. + try: + org_data = ensure_organization(org_code) + except org_exceptions.InvalidOrganizationException as exc: + # Note: IFF the org exists among the modulestore courses but not in the Organizations database table, + # and if auto-create is disabled (it's enabled by default), this will raise InvalidOrganizationException. It + # would be up to the operator to decide how they want to resolve that. + raise ValueError( + f'The organization short code "{org_code}" exists in modulestore ({str(course_id)}) but ' + "not the Organizations table, and auto-creating organizations is disabled. You can resolve this by " + "creating the Organization manually (e.g. from the Django admin) or turning on auto-creation. " + "You can set active=False to prevent this Organization from being used other than for historical data. " + ) from exc + if org_data["short_name"] != org_code: + # On most installations, the 'short_name' database column is case insensitive (unfortunately) + log.warning( + 'The course with ID "%s" does not match its Organization.short_name "%s"', + str(course_id), + org_data["short_name"], + ) + + # Actually create the CatalogCourse. We use get_or_create just to be extra robust against race conditions, since + # we don't care if another worker/thread/etc has beaten us to creating this. + cc, _cc_created = CatalogCourse.objects.get_or_create( + org_id=org_data["id"], + course_code=course_code, + defaults={ + "display_name": display_name, + "language_short": language_short, + }, + ) + + new_run, created = CourseRun.objects.get_or_create( + catalog_course=cc, + run=course_id.run, + course_id=course_id, + defaults={"display_name": display_name}, + ) + + if not created: + log.warning('Expected to create CourseRun for "%s" but it already existed.', str(course_id)) + + return new_run + + +def update_catalog_course( + catalog_course: CatalogCourse | int, + *, + display_name: str | None = None, # Specify a string to change the display name. + # The short language code (one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" + language_short: str | None = None, +) -> None: + """ + Update a `CatalogCourse`. + + ⚠️ Does not check permissions. + """ + if isinstance(catalog_course, CatalogCourse): + cc = catalog_course + else: + cc = CatalogCourse.objects.get(pk=catalog_course) + + update_fields = [] + if display_name: + cc.display_name = display_name + update_fields.append("display_name") + if language_short: + cc.language_short = language_short + update_fields.append("language") + if update_fields: + cc.save(update_fields=update_fields) diff --git a/src/openedx_catalog/models_api.py b/src/openedx_catalog/models_api.py new file mode 100644 index 000000000..50c0f79cd --- /dev/null +++ b/src/openedx_catalog/models_api.py @@ -0,0 +1,9 @@ +""" +Core models available for use in other apps. These are mostly meant to be used +as foreign key targets. Each model should be considered read-only and only +mutated using API methods available in `openedx_catalog.api`. + +See the `openedx_catalog.api` docstring for much more details. +""" + +from .models import CatalogCourse, CourseRun From 22f1676240a649171cc4d1bd4cf1178617b16cd1 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 17:17:01 -0800 Subject: [PATCH 08/29] feat: add a 'url_slug' property to CatalogCourse --- src/openedx_catalog/admin.py | 10 +++++++++- src/openedx_catalog/models/catalog_course.py | 12 ++++++++++++ tests/openedx_catalog/test_models.py | 6 ++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/openedx_catalog/admin.py b/src/openedx_catalog/admin.py index 4679e79d9..ff825e13e 100644 --- a/src/openedx_catalog/admin.py +++ b/src/openedx_catalog/admin.py @@ -27,7 +27,15 @@ class CatalogCourseAdmin(admin.ModelAdmin): """ list_filter = ["org__short_name", "language"] - list_display = ["display_name", "org_display", "course_code", "runs_summary", "created_date", "language"] + list_display = [ + "display_name", + "org_display", + "course_code", + "runs_summary", + "url_slug", + "created_date", + "language", + ] def get_readonly_fields(self, request, obj: CatalogCourse | None = None) -> tuple[str, ...]: if obj: # editing an existing object diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index 151dd1fba..37945cb0e 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -178,6 +178,18 @@ def org_code(self, org_code: str) -> None: # not possible on MySQL, using the default collation used by the Organizations app. # So we do not worry about that possibility here. + @property + def url_slug(self): # Do we need this? Would an opaque key be better? + """ + An ID that can be used to identify this catalog course in URLs or APIs. + In the future, this may be an editable SlugField, so don't assume that + it never changes. + """ + # '+' is a bad separator because it can mean " " in URLs. + # '-', '.', and '_' cannot be used since they're allowed in the org code + # So for now we use ':', and in the future we may make the whole slug customizable. + return f"{self.org_code}:{self.course_code}" + def clean(self): """Validate/normalize fields when edited via Django admin""" # Set a default value for display_name: diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index 9f0ce6a05..b22ed654a 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -74,6 +74,12 @@ def test_course_code_required(self) -> None: # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: CatalogCourse.objects.filter(pk=cc.pk).update(course_code="") + # url_slug field tests: + def test_url_slug(self) -> None: + """Test that url_slug is generated automatically""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + assert cc.url_slug == "Org1:Python100" + # display_name field tests: def test_display_name_default(self) -> None: From 47c916b7dfa260db1b524fe38074b212222cfa5c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 18:01:51 -0800 Subject: [PATCH 09/29] feat: expand API, add tests --- src/openedx_catalog/api_impl.py | 38 +++++++++++++-- tests/openedx_catalog/test_api.py | 79 +++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 tests/openedx_catalog/test_api.py diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py index fb3f7d7e0..a25ac734f 100644 --- a/src/openedx_catalog/api_impl.py +++ b/src/openedx_catalog/api_impl.py @@ -3,6 +3,7 @@ """ import logging +from typing import overload from django.conf import settings from opaque_keys.edx.keys import CourseKey @@ -23,22 +24,49 @@ ] -def get_catalog_course(org_code: str, course_code: str) -> CatalogCourse: +@overload +def get_catalog_course(*, org_code: str, course_code: str) -> CatalogCourse: ... +@overload +def get_catalog_course(*, url_slug: str) -> CatalogCourse: ... +@overload +def get_catalog_course(*, pk: int) -> CatalogCourse: ... + + +def get_catalog_course( + pk: int | None = None, + url_slug: str = "", + org_code: str = "", + course_code: str = "", +) -> CatalogCourse: """ Get a catalog course (set of runs). - Does not check permissions nor load related models. + ⚠️ Does not check permissions or visibility rules. The CatalogCourse may not have any runs associated with it. """ - return CatalogCourse.objects.get(org__short_name=org_code, course_code=course_code) + if pk: + assert not org_code + assert not url_slug + return CatalogCourse.objects.get(pk=pk) + if url_slug: + assert not org_code + assert not course_code + org_code, course_code = url_slug.split(":", 1) + # We might as well select_related org because we're joining to check the org__short_name field anyways. + return CatalogCourse.objects.select_related("org").get(org__short_name=org_code, course_code=course_code) def get_course_run(course_id: CourseKey) -> CourseRun: """ - Get a single course run. Does not check permissions nor load related models. + Get a single course run. + + ⚠️ Does not check permissions or visibility rules. The CourseRun may or may not have content associated with it. + + Tip: to get all runs associated with a CatalogCourse, use + `get_catalog_course(...).runs` """ return CourseRun.objects.get(course_id__exact=course_id) @@ -73,7 +101,7 @@ def create_course_run_for_modulestore_course_with( course_id: CourseKey, *, display_name: str, - # The short language code (one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" + # The short language code (in openedx-platform, this is one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" language_short: str | None = None, ) -> CourseRun: """ diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py new file mode 100644 index 000000000..1108ce3df --- /dev/null +++ b/tests/openedx_catalog/test_api.py @@ -0,0 +1,79 @@ +""" +Tests related to the openedx_catalog python API +""" + +import pytest +from django.core.exceptions import ValidationError +from django.db import transaction +from django.db.utils import IntegrityError +from django.test import TestCase, override_settings +from opaque_keys.edx.locator import CourseLocator +from organizations.api import ensure_organization # type: ignore[import] +from organizations.models import Organization # type: ignore[import] + +from openedx_catalog import api +from openedx_catalog.models_api import CatalogCourse, CourseRun + + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(name="python100") +def _python100(): + """Create a "Python100" course for use in these tests""" + ensure_organization("Org1") + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + assert cc.url_slug == "Org1:Python100" + return cc + + +@pytest.fixture(name="csharp200") +def _csharp200(): + """Create a "CSharp200" course for use in these tests""" + ensure_organization("Org1") + cc = CatalogCourse.objects.create(org_code="Org1", course_code="CSharp200") + assert cc.url_slug == "Org1:CSharp200" + return cc + + +# get_catalog_course + + +def test_get_catalog_course(python100: CatalogCourse, csharp200: CatalogCourse) -> None: + """ + Test using get_catalog_course to get a course in various ways: + """ + # Retrieve by ID: + assert api.get_catalog_course(pk=python100.pk) == python100 + assert api.get_catalog_course(pk=csharp200.pk) == csharp200 + with pytest.raises(CatalogCourse.DoesNotExist): + api.get_catalog_course(pk=8234758243) + + # Retrieve by URL slug: + assert api.get_catalog_course(url_slug="Org1:Python100") == python100 + assert api.get_catalog_course(url_slug="Org1:CSharp200") == csharp200 + with pytest.raises(CatalogCourse.DoesNotExist): + api.get_catalog_course(url_slug="foo:bar") + + # Retrieve by (org_code, course_code) + assert api.get_catalog_course(org_code="Org1", course_code="Python100") == python100 + assert api.get_catalog_course(org_code="Org1", course_code="CSharp200") == csharp200 + with pytest.raises(CatalogCourse.DoesNotExist): + api.get_catalog_course(org_code="Org2", course_code="CSharp200") + + +def test_get_catalog_course_url_slug_case(python100: CatalogCourse) -> None: + """ + Test that get_catalog_course(url_slug=...) is case-insensitive + """ + # FIXME: The Organization model's short_code is case sensitive on SQLite but case insensitive on MySQL :/ + # So for now, we only make assertions about the 'course_code' field case, which we can control. + assert api.get_catalog_course(url_slug="Org1:Python100") == python100 # Correct case + assert api.get_catalog_course(url_slug="Org1:python100") == python100 # Wrong course code case + assert api.get_catalog_course(url_slug="Org1:PYTHON100").url_slug == "Org1:Python100" # Gets normalized + + +# get_course_run +# sync_course_run_details +# create_course_run_for_modulestore_course_with +# update_catalog_course From d2b5fbd1ece1835e3cc7d7fd8af4f02bc91618f8 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 18:14:04 -0800 Subject: [PATCH 10/29] refactor: convert tests to pytest format --- tests/openedx_catalog/test_models.py | 494 ++++++++++++++------------ tests/openedx_catalog/test_signals.py | 73 ++-- 2 files changed, 292 insertions(+), 275 deletions(-) diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index b22ed654a..891207021 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -5,119 +5,142 @@ # (Ignore 'Unexpected attribute "org_code" for model "CatalogCourse"' until # https://github.com/typeddjango/django-stubs/issues/1034 is fixed.) -import ddt # type: ignore[import] import pytest from django.core.exceptions import ValidationError from django.db import transaction from django.db.utils import IntegrityError -from django.test import TestCase, override_settings +from django.test import override_settings from opaque_keys.edx.locator import CourseLocator from organizations.api import ensure_organization # type: ignore[import] from organizations.models import Organization # type: ignore[import] from openedx_catalog.models import CatalogCourse, CourseRun +pytestmark = pytest.mark.django_db -@ddt.ddt -class TestCatalogCourseModel(TestCase): + +@pytest.fixture(name="org1") +def _org1() -> None: + """Create an "Org1" organization for use in these tests""" + ensure_organization("Org1") + + +@pytest.fixture(name="org2") +def _org2() -> None: + """Create an "Org2" organization for use in these tests""" + ensure_organization("Org2") + + +@pytest.fixture(name="python100") +def _python100(org1) -> CatalogCourse: + """Create a CatalogCourse for use in these tests""" + return CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + + +# Low-level tests of the CatalogCourse model. + + +def test_invalid_org() -> None: + """Organization must exist in the DB before CatalogCourse can be created""" + + def create(): + CatalogCourse.objects.create(org_code="NewOrg", course_code="Python100") + + with pytest.raises(Organization.DoesNotExist): + create() + ensure_organization("NewOrg") + create() + + +# course_code field tests: + + +def test_course_code_unique(org1, org2) -> None: + """ + Test that course code is unique per org. + """ + course_code = "Python100" + CatalogCourse.objects.create(org_code="Org1", course_code=course_code) + with pytest.raises(IntegrityError): + with transaction.atomic(): + CatalogCourse.objects.create(org_code="Org1", course_code=course_code) + # But different org is fine: + CatalogCourse.objects.create(org_code="Org2", course_code=course_code) + + +def test_course_code_case_sensitive(org1) -> None: """ - Low-level tests of the CatalogCourse model. + Test that we cannot have two different catalog courses whose course code differs only by case. """ + org_code = "Org1" + CatalogCourse.objects.create(org_code=org_code, course_code="Python100") + with pytest.raises(IntegrityError): + with transaction.atomic(): + CatalogCourse.objects.create(org_code=org_code, course_code="pYTHon100") - @classmethod - def setUpClass(cls): - ensure_organization("Org1") - return super().setUpClass() - # org field tests: +def test_course_code_required(org1) -> None: + """Test that course_code cannot be blank""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", display_name="Python 100") + with pytest.raises(IntegrityError): + # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: + CatalogCourse.objects.filter(pk=cc.pk).update(course_code="") - def test_invalid_org(self) -> None: - """Organization must exist in the DB before CatalogCourse can be created""" - def create(): - CatalogCourse.objects.create(org_code="NewOrg", course_code="Python100") +# url_slug field tests: +def test_url_slug(org1) -> None: + """Test that url_slug is generated automatically""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + assert cc.url_slug == "Org1:Python100" - with pytest.raises(Organization.DoesNotExist): - create() - ensure_organization("NewOrg") - create() - # course_code field tests: +# display_name field tests: - def test_course_code_unique(self) -> None: - """ - Test that course code is unique per org. - """ - course_code = "Python100" - ensure_organization("Org2") - CatalogCourse.objects.create(org_code="Org1", course_code=course_code) - with pytest.raises(IntegrityError): - with transaction.atomic(): - CatalogCourse.objects.create(org_code="Org1", course_code=course_code) - # But different org is fine: - CatalogCourse.objects.create(org_code="Org2", course_code=course_code) - - def test_course_code_case_sensitive(self) -> None: - """ - Test that we cannot have two different catalog courses whose course code differs only by case. - """ - org_code = "Org1" - CatalogCourse.objects.create(org_code=org_code, course_code="Python100") - with pytest.raises(IntegrityError): - with transaction.atomic(): - CatalogCourse.objects.create(org_code=org_code, course_code="pYTHon100") - - def test_course_code_required(self) -> None: - """Test that course_code cannot be blank""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", display_name="Python 100") - with pytest.raises(IntegrityError): - # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: - CatalogCourse.objects.filter(pk=cc.pk).update(course_code="") - - # url_slug field tests: - def test_url_slug(self) -> None: - """Test that url_slug is generated automatically""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") - assert cc.url_slug == "Org1:Python100" - - # display_name field tests: - - def test_display_name_default(self) -> None: - """Test that display_name has a default""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") - assert cc.display_name == "Python100" - - def test_display_name_required(self) -> None: - """Test that display_name cannot be blank""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", display_name="Python 100") - with pytest.raises(IntegrityError): - # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: - CatalogCourse.objects.filter(pk=cc.pk).update(display_name="") - - def test_display_name_unicode(self) -> None: - """Test that display_name can handle any valid unicode value""" - # If it works with emojis, it should work with any human language characters. - display_name = "Happy 😊" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="HAPPY", display_name=display_name) - cc.refresh_from_db() - assert cc.display_name == display_name - - # language code field tests: - - def test_language_code_default(self) -> None: - """Test that 'language' has a default""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") - # Our test settings don't specify a LANGUAGE_CODE, so this should be the Django default of "en-us" - # https://docs.djangoproject.com/en/6.0/ref/settings/#language-code - assert cc.language == "en-us" - - @override_settings(LANGUAGE_CODE="fr") - def test_language_code_default2(self) -> None: - """Test that 'language' gets its default from settings""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") - assert cc.language == "fr" - - @ddt.data( + +def test_display_name_default(org1) -> None: + """Test that display_name has a default""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + assert cc.display_name == "Python100" + + +def test_display_name_required(org1) -> None: + """Test that display_name cannot be blank""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", display_name="Python 100") + with pytest.raises(IntegrityError): + # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: + CatalogCourse.objects.filter(pk=cc.pk).update(display_name="") + + +def test_display_name_unicode(org1) -> None: + """Test that display_name can handle any valid unicode value""" + # If it works with emojis, it should work with any human language characters. + display_name = "Happy 😊" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="HAPPY", display_name=display_name) + cc.refresh_from_db() + assert cc.display_name == display_name + + +# language code field tests: + + +def test_language_code_default(org1) -> None: + """Test that 'language' has a default""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + # Our test settings don't specify a LANGUAGE_CODE, so this should be the Django default of "en-us" + # https://docs.djangoproject.com/en/6.0/ref/settings/#language-code + assert cc.language == "en-us" + + +@override_settings(LANGUAGE_CODE="fr") +def test_language_code_default2(org1) -> None: + """Test that 'language' gets its default from settings""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + assert cc.language == "fr" + + +@pytest.mark.parametrize( + "language_code,valid", + [ # ✅ Valid language codes: ("en", True), ("en-us", True), @@ -137,28 +160,31 @@ def test_language_code_default2(self) -> None: ("English", False), ("english", False), ("ca@valencia", False), # Don't support old gettext-style locales; should be "ca-es-valencia" - ) - @ddt.unpack - def test_language_code_validation(self, language_code: str, valid: bool) -> None: - """Test that language codes must follow the prescribed format""" - - def create(): - CatalogCourse.objects.create(org_code="Org1", course_code="Python100", language=language_code) - - if valid: - create() - else: - with pytest.raises(IntegrityError, match="oex_catalog_catalogcourse_language_regex"): - with transaction.atomic(): - create() - # And from the Django admin, we'd get a nicer error message: - expected_msg = "The language code must be lowercase" if language_code else "This field cannot be blank." - with pytest.raises(ValidationError, match=expected_msg): - CatalogCourse( - org_code="Org1", course_code="Python100", language=language_code, display_name="x" - ).full_clean() - - @ddt.data( + ], +) +def test_language_code_validation(language_code: str, valid: bool, org1) -> None: + """Test that language codes must follow the prescribed format""" + + def create(): + CatalogCourse.objects.create(org_code="Org1", course_code="Python100", language=language_code) + + if valid: + create() + else: + with pytest.raises(IntegrityError, match="oex_catalog_catalogcourse_language_regex"): + with transaction.atomic(): + create() + # And from the Django admin, we'd get a nicer error message: + expected_msg = "The language code must be lowercase" if language_code else "This field cannot be blank." + with pytest.raises(ValidationError, match=expected_msg): + CatalogCourse( + org_code="Org1", course_code="Python100", language=language_code, display_name="x" + ).full_clean() + + +@pytest.mark.parametrize( + "kwargs,expected_full_lang,expected_short", + [ # input field, expected .language value, expected .language_short value ({"language": "fr"}, "fr", "fr"), ({"language": "fr-ca"}, "fr-ca", "fr"), @@ -169,134 +195,128 @@ def create(): ({"language_short": "fr"}, "fr", "fr"), ({"language": "zh-hans"}, "zh-cn", "zh_HANS"), # Input is invalid but gets corrected by clean() ({"language": "zh-hant"}, "zh-hk", "zh_HANT"), # Input is invalid but gets corrected by clean() - ) - @ddt.unpack - def test_language_code_compatibility(self, kwargs: dict, expected_full_lang: str, expected_short: str) -> None: - """Test that the language_short field is fully backwards compatible with CourseOverview.language""" - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Locale100", **kwargs) - assert cc.language == expected_full_lang - assert cc.language_short == expected_short + ], +) +def test_language_code_compatibility(kwargs: dict, expected_full_lang: str, expected_short: str, org1) -> None: + """Test that the language_short field is fully backwards compatible with CourseOverview.language""" + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Locale100", **kwargs) + assert cc.language == expected_full_lang + assert cc.language_short == expected_short + +################################################################################ +# Low-level tests of the CourseRun model. -@ddt.ddt -class TestCourseRunModel(TestCase): +# course ID tests: + + +def test_course_id_autogenerated(python100: CatalogCourse) -> None: """ - Low-level tests of the CourseRun model. + Test that course_id is auto-generated (optionally, for convenience) + + Among other things, this allows users to create course runs from the Django admin, without having to know the + details of how to format the course IDs. """ + org_code = python100.org_code + course_code = python100.course_code + run = "Fall2026" + cr = CourseRun.objects.create(catalog_course=python100, run=run) + cr.refresh_from_db() + assert isinstance(cr.course_id, CourseLocator) + assert str(cr.course_id) == f"course-v1:{org_code}+{course_code}+{run}" - catalog_course: CatalogCourse - - @classmethod - def setUpClass(cls): - ensure_organization("Org1") - cls.catalog_course = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") - return super().setUpClass() - - # course ID tests: - - def test_course_id_autogenerated(self) -> None: - """ - Test that course_id is auto-generated (optionally, for convenience) - - Among other things, this allows users to create course runs from the Django admin, without having to know the - details of how to format the course IDs. - """ - org_code = self.catalog_course.org_code - course_code = self.catalog_course.course_code - run = "Fall2026" - cr = CourseRun.objects.create(catalog_course=self.catalog_course, run=run) - cr.refresh_from_db() - assert isinstance(cr.course_id, CourseLocator) - assert str(cr.course_id) == f"course-v1:{org_code}+{course_code}+{run}" - - def test_course_id_run_match(self) -> None: - """Test that course_id must match the org and course from the related CatalogCourse""" - # Note: "run" is tested separately, in "test_run_exact" below. - org_code = self.catalog_course.org_code - course_code = self.catalog_course.course_code - run = "Fall2026" - - def create_with(**kwargs): - id_args = {"org": org_code, "course": course_code, "run": run, **kwargs} - obj = CourseRun.objects.create( - catalog_course=self.catalog_course, run=run, course_id=CourseLocator(**id_args) - ) - obj.delete() - - # course code must match exactly: - with pytest.raises(ValidationError): - assert course_code.upper() != course_code - create_with(course=course_code.upper()) - - # The org cannot be completely different - with pytest.raises(ValidationError): - create_with(org="other_org") - - # But we DO allow the org to have different case: - assert org_code.upper() != org_code - create_with(org=org_code.upper()) - - # And if everything case matches exactly, it works (normal situation): - create_with() - - # run field tests: - - def test_run_unique(self) -> None: - """ - Test that run is unique per catalog course. - """ - run = "Fall2026" - catalog_course2 = CatalogCourse.objects.create(org_code="Org1", course_code="Systems300") - CourseRun.objects.create(catalog_course=self.catalog_course, run=run) - # Creating different catalog course with the same run name is fine: - CourseRun.objects.create(catalog_course=catalog_course2, run=run) - # But creating another run with the same catalog course and run name is not: - with pytest.raises(IntegrityError): - CourseRun.objects.create(catalog_course=self.catalog_course, run=run) - - def test_run_case_insensitive(self) -> None: - """ - Test that we cannot have two different course runs whose run code - differs only by case. - - We would like to support this, but we cannot, because we have a lot of legacy parts of the system that store - course IDs without case sensitivity. - """ - CourseRun.objects.create(catalog_course=self.catalog_course, run="fall2026") - with pytest.raises(IntegrityError): - CourseRun.objects.create(catalog_course=self.catalog_course, run="FALL2026") - - def test_run_required(self) -> None: - """Test that run cannot be blank""" - course = CourseRun.objects.create(catalog_course=self.catalog_course, run="fall2026") - with pytest.raises(IntegrityError): - # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: - CourseRun.objects.filter(pk=course.pk).update(run="") - - def test_run_exact(self) -> None: - """Test that `run` must exactly match the run of the course ID (including case)""" - catalog_course = CatalogCourse.objects.create(org_code="Org1", course_code="Test302") - course_id = CourseLocator(org="Org1", course="Test302", run="MixedCase") - - with pytest.raises(IntegrityError, match="oex_catalog_courserun_courseid_run_match_exactly"): - with transaction.atomic(): - CourseRun.objects.create(catalog_course=catalog_course, course_id=course_id, run="mixedcase") - run = CourseRun.objects.create(catalog_course=catalog_course, course_id=course_id, run="MixedCase") +def test_course_id_run_match(python100: CatalogCourse) -> None: + """Test that course_id must match the org and course from the related CatalogCourse""" + # Note: "run" is tested separately, in "test_run_exact" below. + org_code = python100.org_code + course_code = python100.course_code + run = "Fall2026" - # Do not allow modifying the run so it's completely different from the run in the course ID - with pytest.raises(IntegrityError): - with transaction.atomic(): - CourseRun.objects.filter(pk=run.pk).update(run="foobar") + def create_with(**kwargs): + id_args = {"org": org_code, "course": course_code, "run": run, **kwargs} + obj = CourseRun.objects.create(catalog_course=python100, run=run, course_id=CourseLocator(**id_args)) + obj.delete() - # Do not allow modifying the run so it doesn't match the course ID: - with pytest.raises(IntegrityError): - with transaction.atomic(): - CourseRun.objects.filter(pk=run.pk).update(run="mixedcase") + # course code must match exactly: + with pytest.raises(ValidationError): + assert course_code.upper() != course_code + create_with(course=course_code.upper()) - # Do not allow modifying the course ID so it doesn't match the run: - with pytest.raises(IntegrityError): - with transaction.atomic(): - CourseRun.objects.filter(pk=run.pk).update( - course_id=CourseLocator(org="Org1", course="Test302", run="mixedcase"), - ) + # The org cannot be completely different + with pytest.raises(ValidationError): + create_with(org="other_org") + + # But we DO allow the org to have different case: + assert org_code.upper() != org_code + create_with(org=org_code.upper()) + + # And if everything case matches exactly, it works (normal situation): + create_with() + + +# run field tests: + + +def test_run_unique(python100: CatalogCourse) -> None: + """ + Test that run is unique per catalog course. + """ + run = "Fall2026" + catalog_course2 = CatalogCourse.objects.create(org_code="Org1", course_code="Systems300") + CourseRun.objects.create(catalog_course=python100, run=run) + # Creating different catalog course with the same run name is fine: + CourseRun.objects.create(catalog_course=catalog_course2, run=run) + # But creating another run with the same catalog course and run name is not: + with pytest.raises(IntegrityError): + CourseRun.objects.create(catalog_course=python100, run=run) + + +def test_run_case_insensitive(python100: CatalogCourse) -> None: + """ + Test that we cannot have two different course runs whose run code + differs only by case. + + We would like to support this, but we cannot, because we have a lot of legacy parts of the system that store + course IDs without case sensitivity. + """ + CourseRun.objects.create(catalog_course=python100, run="fall2026") + with pytest.raises(IntegrityError): + CourseRun.objects.create(catalog_course=python100, run="FALL2026") + + +def test_run_required(python100: CatalogCourse) -> None: + """Test that run cannot be blank""" + course = CourseRun.objects.create(catalog_course=python100, run="fall2026") + with pytest.raises(IntegrityError): + # Using .update() will bypass all checks and defaults in save()/clean(), to see if the DB enforces this: + CourseRun.objects.filter(pk=course.pk).update(run="") + + +def test_run_exact(org1) -> None: + """Test that `run` must exactly match the run of the course ID (including case)""" + catalog_course = CatalogCourse.objects.create(org_code="Org1", course_code="Test302") + course_id = CourseLocator(org="Org1", course="Test302", run="MixedCase") + + with pytest.raises(IntegrityError, match="oex_catalog_courserun_courseid_run_match_exactly"): + with transaction.atomic(): + CourseRun.objects.create(catalog_course=catalog_course, course_id=course_id, run="mixedcase") + + run = CourseRun.objects.create(catalog_course=catalog_course, course_id=course_id, run="MixedCase") + + # Do not allow modifying the run so it's completely different from the run in the course ID + with pytest.raises(IntegrityError): + with transaction.atomic(): + CourseRun.objects.filter(pk=run.pk).update(run="foobar") + + # Do not allow modifying the run so it doesn't match the course ID: + with pytest.raises(IntegrityError): + with transaction.atomic(): + CourseRun.objects.filter(pk=run.pk).update(run="mixedcase") + + # Do not allow modifying the course ID so it doesn't match the run: + with pytest.raises(IntegrityError): + with transaction.atomic(): + CourseRun.objects.filter(pk=run.pk).update( + course_id=CourseLocator(org="Org1", course="Test302", run="mixedcase"), + ) diff --git a/tests/openedx_catalog/test_signals.py b/tests/openedx_catalog/test_signals.py index b4c5940d5..1ac312176 100644 --- a/tests/openedx_catalog/test_signals.py +++ b/tests/openedx_catalog/test_signals.py @@ -6,50 +6,47 @@ import pytest 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 # type: ignore[import] from openedx_catalog.models import CatalogCourse, CourseRun +pytestmark = pytest.mark.django_db -class TestCatalogSignals(TestCase): - """ - Test openedx_catalog signal handlers + +def test_org_integrity() -> None: """ + Test that Organization.short_name cannot be changed if it would result in invalid CourseRun relationships. - 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" + 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() -> 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() From f6c594585438b575ab35e36bb55bc238b103d82f Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 18:17:09 -0800 Subject: [PATCH 11/29] chore: mypy/pylint fixes --- src/openedx_catalog/api_impl.py | 3 +-- src/openedx_catalog/models/catalog_course.py | 4 ++-- src/openedx_catalog/models_api.py | 1 + tests/openedx_catalog/test_api.py | 1 - 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py index a25ac734f..5a3e6a6ae 100644 --- a/src/openedx_catalog/api_impl.py +++ b/src/openedx_catalog/api_impl.py @@ -5,9 +5,8 @@ import logging from typing import overload -from django.conf import settings from opaque_keys.edx.keys import CourseKey -from organizations.api import ensure_organization +from organizations.api import ensure_organization # type: ignore[import] from organizations.api import exceptions as org_exceptions from .models import CatalogCourse, CourseRun diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index 37945cb0e..f51569b52 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -136,7 +136,7 @@ def language_short(self) -> str: return self.language[:2] # Strip locale @language_short.setter - def language_short(self, legacy_code: str) -> str: + def language_short(self, legacy_code: str) -> None: """ Set the language code used by this catalog course, without locale. This is always a two-digit code, except for Mandarin and Cantonese. @@ -144,7 +144,7 @@ def language_short(self, legacy_code: str) -> str: the CourseOverview.language field.) """ if hasattr(settings, "ALL_LANGUAGES"): - assert legacy_code in [code for (code, _name) in settings.ALL_LANGUAGES] + assert legacy_code in [code for (code, _name) in settings.ALL_LANGUAGES] # type: ignore if legacy_code == "zh_HANS": # Mandarin / Simplified self.language = "zh-cn" # Chinese (Mainland China) elif legacy_code == "zh_HANT": # Cantonese / Traditional diff --git a/src/openedx_catalog/models_api.py b/src/openedx_catalog/models_api.py index 50c0f79cd..2836a2aa1 100644 --- a/src/openedx_catalog/models_api.py +++ b/src/openedx_catalog/models_api.py @@ -6,4 +6,5 @@ See the `openedx_catalog.api` docstring for much more details. """ +# pylint: disable=unused-import from .models import CatalogCourse, CourseRun diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 1108ce3df..99d5241ab 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -14,7 +14,6 @@ from openedx_catalog import api from openedx_catalog.models_api import CatalogCourse, CourseRun - pytestmark = pytest.mark.django_db From 9f59c9298389aa6a2c9f21dccda59d4397601669 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sun, 22 Feb 2026 14:01:50 -0800 Subject: [PATCH 12/29] fix: make course code (on CatalogCourse) case sensitive --- src/openedx_catalog/migrations/0001_initial.py | 10 ++++++++-- src/openedx_catalog/models/catalog_course.py | 14 ++++++++------ tests/openedx_catalog/test_api.py | 4 ++-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/openedx_catalog/migrations/0001_initial.py b/src/openedx_catalog/migrations/0001_initial.py index 23bffae6f..afcdbfb9a 100644 --- a/src/openedx_catalog/migrations/0001_initial.py +++ b/src/openedx_catalog/migrations/0001_initial.py @@ -36,7 +36,7 @@ class Migration(migrations.Migration): ( "course_code", openedx_django_lib.fields.MultiCollationCharField( - db_collations={"mysql": "utf8mb4_unicode_ci", "sqlite": "NOCASE"}, + db_collations={"mysql": "utf8mb4_bin", "sqlite": "BINARY"}, help_text='The course ID, e.g. "Math100".', max_length=255, ), @@ -136,10 +136,16 @@ class Migration(migrations.Migration): "ordering": ("-created",), }, ), + migrations.AddIndex( + model_name="catalogcourse", + index=models.Index(fields=["org", "course_code"], name="openedx_cat_org_id_bd314b_idx"), + ), migrations.AddConstraint( model_name="catalogcourse", constraint=models.UniqueConstraint( - fields=("org", "course_code"), name="oex_catalog_catalog_course_org_code_pair_uniq" + models.F("org"), + django.db.models.functions.text.Lower("course_code"), + name="oex_catalog_catalogcourse_org_code_uniq_ci", ), ), migrations.AddConstraint( diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index f51569b52..05ba625b3 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -7,7 +7,7 @@ from django.conf import settings from django.contrib import admin from django.db import models -from django.db.models.functions import Length +from django.db.models.functions import Length, Lower from django.db.models.lookups import Regex from django.utils.translation import gettext_lazy as _ from organizations.models import Organization # type: ignore[import] @@ -74,7 +74,7 @@ class CatalogCourse(models.Model): # necessary to fix capitalization problems. (We wouldn't want to allow other changes to an org's short_name # though; only fixing capitalization - see openedx_catalog.signals.verify_organization_change.) ) - course_code = case_insensitive_char_field( + course_code = case_sensitive_char_field( max_length=255, blank=False, null=False, @@ -214,11 +214,13 @@ class Meta: verbose_name = _("Catalog Course") verbose_name_plural = _("Catalog Courses") ordering = ("-created",) + indexes = [ + # We need fast lookups by (org, course_code) pairs. We generally want this lookup to be case sensitive. + models.Index(fields=["org", "course_code"]), + ] constraints = [ - models.UniqueConstraint( - fields=["org", "course_code"], - name="oex_catalog_catalog_course_org_code_pair_uniq", - ), + # The course_course must be case-insensitively unique per org: + models.UniqueConstraint("org", Lower("course_code"), name="oex_catalog_catalogcourse_org_code_uniq_ci"), # Enforce at the DB level that these required fields are not blank: models.CheckConstraint( condition=models.Q(course_code__length__gt=0), name="oex_catalog_catalogcourse_course_code_not_blank" diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 99d5241ab..60f882eb7 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -68,8 +68,8 @@ def test_get_catalog_course_url_slug_case(python100: CatalogCourse) -> None: # FIXME: The Organization model's short_code is case sensitive on SQLite but case insensitive on MySQL :/ # So for now, we only make assertions about the 'course_code' field case, which we can control. assert api.get_catalog_course(url_slug="Org1:Python100") == python100 # Correct case - assert api.get_catalog_course(url_slug="Org1:python100") == python100 # Wrong course code case - assert api.get_catalog_course(url_slug="Org1:PYTHON100").url_slug == "Org1:Python100" # Gets normalized + with pytest.raises(CatalogCourse.DoesNotExist): + api.get_catalog_course(url_slug="Org1:python100") # Wrong course code case # get_course_run From 735feda18b7d2ac16d1cd19ef87e3f63ec077248 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 23 Feb 2026 22:40:45 -0800 Subject: [PATCH 13/29] refactor: re-order API methods --- src/openedx_catalog/api_impl.py | 58 ++++++++++++++++----------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py index 5a3e6a6ae..7614beda2 100644 --- a/src/openedx_catalog/api_impl.py +++ b/src/openedx_catalog/api_impl.py @@ -16,10 +16,10 @@ # These are the public API methods that anyone can use __all__ = [ "get_catalog_course", + "update_catalog_course", "get_course_run", "sync_course_run_details", "create_course_run_for_modulestore_course_with", - "update_catalog_course", ] @@ -56,6 +56,34 @@ def get_catalog_course( return CatalogCourse.objects.select_related("org").get(org__short_name=org_code, course_code=course_code) +def update_catalog_course( + catalog_course: CatalogCourse | int, + *, + display_name: str | None = None, # Specify a string to change the display name. + # The short language code (one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" + language_short: str | None = None, +) -> None: + """ + Update a `CatalogCourse`. + + ⚠️ Does not check permissions. + """ + if isinstance(catalog_course, CatalogCourse): + cc = catalog_course + else: + cc = CatalogCourse.objects.get(pk=catalog_course) + + update_fields = [] + if display_name: + cc.display_name = display_name + update_fields.append("display_name") + if language_short: + cc.language_short = language_short + update_fields.append("language") + if update_fields: + cc.save(update_fields=update_fields) + + def get_course_run(course_id: CourseKey) -> CourseRun: """ Get a single course run. @@ -174,31 +202,3 @@ def create_course_run_for_modulestore_course_with( log.warning('Expected to create CourseRun for "%s" but it already existed.', str(course_id)) return new_run - - -def update_catalog_course( - catalog_course: CatalogCourse | int, - *, - display_name: str | None = None, # Specify a string to change the display name. - # The short language code (one of settings.ALL_LANGUAGES), e.g. "en", "es", "zh_HANS" - language_short: str | None = None, -) -> None: - """ - Update a `CatalogCourse`. - - ⚠️ Does not check permissions. - """ - if isinstance(catalog_course, CatalogCourse): - cc = catalog_course - else: - cc = CatalogCourse.objects.get(pk=catalog_course) - - update_fields = [] - if display_name: - cc.display_name = display_name - update_fields.append("display_name") - if language_short: - cc.language_short = language_short - update_fields.append("language") - if update_fields: - cc.save(update_fields=update_fields) From 54d1576b8a88c17888c0603dae31a04a9d19a73e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 23 Feb 2026 22:40:54 -0800 Subject: [PATCH 14/29] test: expand API tests --- tests/openedx_catalog/test_api.py | 240 ++++++++++++++++++++++++++++-- 1 file changed, 229 insertions(+), 11 deletions(-) diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 60f882eb7..832dd1854 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -2,39 +2,61 @@ Tests related to the openedx_catalog python API """ +from datetime import datetime, timezone + import pytest -from django.core.exceptions import ValidationError -from django.db import transaction -from django.db.utils import IntegrityError -from django.test import TestCase, override_settings -from opaque_keys.edx.locator import CourseLocator +from django.db import connection +from django.test import override_settings +from freezegun import freeze_time +from opaque_keys.edx.keys import CourseKey from organizations.api import ensure_organization # type: ignore[import] -from organizations.models import Organization # type: ignore[import] from openedx_catalog import api from openedx_catalog.models_api import CatalogCourse, CourseRun pytestmark = pytest.mark.django_db +# Fixtures used by these tests: + @pytest.fixture(name="python100") def _python100(): - """Create a "Python100" course for use in these tests""" + """Create a "Python100" catalog course for use in these tests""" ensure_organization("Org1") - cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100") + # Note: in the future, this could use an API to create the CatalogCourse, + # but we haven't created the full CRUD API yet. + cc = CatalogCourse.objects.create(org_code="Org1", course_code="Python100", display_name="Python 100") assert cc.url_slug == "Org1:Python100" return cc @pytest.fixture(name="csharp200") def _csharp200(): - """Create a "CSharp200" course for use in these tests""" + """Create a "CSharp200" catalog course for use in these tests""" ensure_organization("Org1") - cc = CatalogCourse.objects.create(org_code="Org1", course_code="CSharp200") + cc = CatalogCourse.objects.create(org_code="Org1", course_code="CSharp200", display_name="C# 200") assert cc.url_slug == "Org1:CSharp200" return cc +@pytest.fixture(name="python100_summer26") +def _python100_summer26(python100: CatalogCourse): + """Create a "Python100" "Summer 2026" course run for use in these tests""" + # Note: in the future, this could use an API to create the CourseRun, + # but we haven't created the full CRUD API yet. + return CourseRun.objects.create( + catalog_course=python100, + run="2026summer", + display_name="Python 100 (Summer ☀️ 2026)", # A random emoji just to test Unicode support in display_name + ) + + +@pytest.fixture(name="python100_winter26") +def _python100_winter26(python100: CatalogCourse): + """Create a "Python100" "Winter 2026" course run for use in these tests""" + return CourseRun.objects.create(catalog_course=python100, run="2026winter", display_name="Python 100 (Winter '26)") + + # get_catalog_course @@ -72,7 +94,203 @@ def test_get_catalog_course_url_slug_case(python100: CatalogCourse) -> None: api.get_catalog_course(url_slug="Org1:python100") # Wrong course code case +def get_get_all_runs(python100_summer26: CourseRun, python100_winter26: CourseRun) -> None: + """ + Test getting all runs of a course using `get_catalog_course(...).runs` as + recommended by the `get_course_run()` docstring. + """ + cc = api.get_catalog_course(org_code="Org1", course_code="Python100") + assert cc.runs.order_by("-run") == [ + python100_summer26, + python100_winter26, + ] + + +# update_catalog_course + + +def test_update_display_name(python100: CatalogCourse, csharp200: CatalogCourse) -> None: + """Test that we can use the API to update the name of a catalog course""" + csharp200_old_name = csharp200.display_name + # Update display_name using a CatalogCourse object: + api.update_catalog_course(python100, display_name="New name for Python 100") + assert api.get_catalog_course(pk=python100.pk).display_name == "New name for Python 100" + assert api.get_catalog_course(pk=csharp200.pk).display_name == csharp200_old_name # Unchanged + # Update display name using PK only: + api.update_catalog_course(csharp200.pk, display_name="New name for C# 200") + assert api.get_catalog_course(pk=python100.pk).display_name == "New name for Python 100" # Unchanged + assert api.get_catalog_course(pk=csharp200.pk).display_name == "New name for C# 200" + + +def test_update_language(python100: CatalogCourse) -> None: + """Test that we can use the API to update the language of a catalog course""" + assert python100.language_short == "en" + api.update_catalog_course(python100, language_short="fr") + python100.refresh_from_db() + assert python100.language_short == "fr" + + +def test_update_ignore_other_fields(python100: CatalogCourse) -> None: + """Test that when we pass on object to update_catalog_course, only expected fields are updated""" + python100.course_code = "CHANGED" + api.update_catalog_course(python100, language_short="fr", display_name="New Name") + python100.refresh_from_db() + assert python100.course_code == "Python100" # course_code field was not modified by update_catalog_course() + assert python100.language_short == "fr" + assert python100.display_name == "New Name" + + # get_course_run + + +def test_get_course_run_nonexistent() -> None: + """ + Getting a non-existent course run raises CourseRun.DoesNotExist + """ + with pytest.raises(CourseRun.DoesNotExist): + api.get_course_run(CourseKey.from_string("course-v1:org_code+course_run+run")) + + +def test_get_course_run(python100_summer26: CourseRun) -> None: + """Basic retrieval of a CourseRun using the API""" + run = api.get_course_run(python100_summer26.course_id) + assert run == python100_summer26 + + # sync_course_run_details + + +def test_sync_course_run_details(python100_summer26) -> None: + """Test `sync_course_run_details()`""" + course_id = python100_summer26.course_id + assert python100_summer26.display_name == "Python 100 (Summer ☀️ 2026)" + api.sync_course_run_details(course_id, display_name="✅ New name") + python100_summer26.refresh_from_db() + assert python100_summer26.display_name == "✅ New name" + + # create_course_run_for_modulestore_course_with -# update_catalog_course + + +@override_settings(LANGUAGE_CODE="fr-ca") +def test_create_course_run_for_modulestore_course_with(): + """ + Test that create_course_run_for_modulestore_course_with() can be used to + keep CourseRun+CatalogCourse in sync with modulestore when a brand new + course is created. + + This test: neither org nor catalog course nor run previously exist. + """ + org_code, course_code, run_code = "NewOrg", "Test", "2026" + course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{run_code}") + + created_time = datetime(2026, 6, 8, tzinfo=timezone.utc) + with freeze_time(created_time): + run = api.create_course_run_for_modulestore_course_with( + course_id, + display_name="Introduction aux tests", + # language_short is not specified - should use the default language (French) + ) + assert run.catalog_course.org_code == org_code + assert run.catalog_course.course_code == "Test" + assert run.catalog_course.language == "fr-ca" + assert run.catalog_course.language_short == "fr" + assert run.catalog_course.display_name == "Introduction aux tests" + assert run.catalog_course.created == created_time + assert run.display_name == "Introduction aux tests" + assert run.run == run_code + assert run.created == created_time + assert run.course_id == course_id + + +def test_create_course_run_for_modulestore_course_with_existing_org(): + """ + Test create_course_run_for_modulestore_course_with() when org already exists + but catalog course does not. + """ + org_code, course_code, run_code = "NewOrg", "Test", "2026" + course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{run_code}") + + ensure_organization(org_code) + run = api.create_course_run_for_modulestore_course_with( + course_id, display_name="Introducción a las pruebas", language_short="es" + ) + assert run.catalog_course.org_code == org_code + assert run.catalog_course.course_code == "Test" + assert run.catalog_course.language == "es" + assert run.catalog_course.language_short == "es" + assert run.catalog_course.display_name == "Introducción a las pruebas" + assert run.run == run_code + assert run.display_name == "Introducción a las pruebas" + assert run.course_id == course_id + + +# FIXME: this test passes on MySQL but not SQLite. We need to update the Organizations code to behave consistently. +@pytest.mark.skipif(connection.vendor == "sqlite", reason="Only passes on MySQL") +def test_create_course_run_for_modulestore_course_with_existing_org_different_capitalization(): + """ + Test create_course_run_for_modulestore_course_with() when the org already + exists but with different capitalization + """ + org_code, course_code, run_code = "NewOrg", "Test", "2026" + course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{run_code}") + + existing_org_id = ensure_organization("nEWoRG")["id"] + run = api.create_course_run_for_modulestore_course_with( + course_id, display_name="Introducción a las pruebas", language_short="es" + ) + assert run.catalog_course.org_id == existing_org_id + assert run.catalog_course.org_code == "nEWoRG" # Uses canonical capitalization + assert run.catalog_course.course_code == "Test" + assert run.catalog_course.language == "es" + assert run.catalog_course.language_short == "es" + assert run.catalog_course.display_name == "Introducción a las pruebas" + assert run.run == run_code + assert run.display_name == "Introducción a las pruebas" + assert run.course_id == course_id # But course ID uses original capitalization + + +def test_create_course_run_for_modulestore_course_with_existing_cc(): + """ + Test create_course_run_for_modulestore_course_with() when the org and + catalog catalog course already exist, but no other runs do + """ + org_code, course_code, run_code = "NewOrg", "Test", "2026" + course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{run_code}") + + ensure_organization(org_code) + # Note we don't have an API for creating catalog courses yet, other than this + # `create_course_run_for_modulestore_course_with` method auto-creating them, so just use the model: + CatalogCourse.objects.create(org_code=org_code, course_code=course_code, display_name="Catalog Display Name") + run = api.create_course_run_for_modulestore_course_with(course_id, display_name="Run Display Name") + assert run.catalog_course.org_code == org_code + assert run.catalog_course.course_code == "Test" + assert run.catalog_course.language_short == "en" # Default language + assert run.catalog_course.display_name == "Catalog Display Name" # Should not have changed just by creating a run + assert run.run == run_code + assert run.display_name == "Run Display Name" + assert run.course_id == course_id + + +def test_create_course_run_for_modulestore_course_with_existing_run(): + """ + Test create_course_run_for_modulestore_course_with() when another run of the + same catalog course already exists. + """ + org_code, course_code, run_code = "NewOrg", "Test", "2026" + course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{run_code}") + old_run_code = "2025" + old_course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{old_run_code}") + + old_run = api.create_course_run_for_modulestore_course_with(old_course_id, display_name="Previous Run (2025)") + new_run = api.create_course_run_for_modulestore_course_with(course_id, display_name="New Run (2026)") + old_run.refresh_from_db() # Let's make sure it hasn't changed + assert old_run.display_name == "Previous Run (2025)" + assert old_run.catalog_course == new_run.catalog_course + assert old_run.run == "2025" + # When there was only one run, the catalog course would be given the name of that run: + assert new_run.catalog_course.display_name == "Previous Run (2025)" + assert new_run.display_name + assert new_run.run == "2026" + assert new_run.display_name == "New Run (2026)" + assert new_run.course_id == course_id From c5d43aee3302ccbcc74db37c3a3d71a3c2ff223c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Mon, 23 Feb 2026 23:25:02 -0800 Subject: [PATCH 15/29] fix: bug found by the new tests --- src/openedx_catalog/api_impl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py index 7614beda2..79c3a5960 100644 --- a/src/openedx_catalog/api_impl.py +++ b/src/openedx_catalog/api_impl.py @@ -187,7 +187,7 @@ def create_course_run_for_modulestore_course_with( course_code=course_code, defaults={ "display_name": display_name, - "language_short": language_short, + **({"language_short": language_short} if language_short else {}), }, ) From 162089b4638077fb284d299a8b1784c5b7d651eb Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 24 Feb 2026 15:14:21 -0800 Subject: [PATCH 16/29] refactor: update to use opaque-keys 3.1 --- requirements/base.txt | 2 +- requirements/dev.txt | 2 +- requirements/doc.txt | 2 +- requirements/quality.txt | 2 +- requirements/test.txt | 2 +- src/openedx_catalog/migrations/0001_initial.py | 3 ++- src/openedx_catalog/models/course_run.py | 10 +++++----- 7 files changed, 12 insertions(+), 11 deletions(-) diff --git a/requirements/base.txt b/requirements/base.txt index 7349942a2..3166cede1 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -76,7 +76,7 @@ edx-drf-extensions==10.6.0 # via # -r requirements/base.in # edx-organizations -edx-opaque-keys==3.0.0 +edx-opaque-keys==3.1.0 # via # edx-drf-extensions # edx-organizations diff --git a/requirements/dev.txt b/requirements/dev.txt index 6338b9cd2..e8ada5966 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -205,7 +205,7 @@ edx-i18n-tools==1.9.0 # via -r requirements/dev.in edx-lint==5.6.0 # via -r requirements/quality.txt -edx-opaque-keys==3.0.0 +edx-opaque-keys==3.1.0 # via # -r requirements/quality.txt # edx-drf-extensions diff --git a/requirements/doc.txt b/requirements/doc.txt index 7b689d45b..5912fa49a 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -167,7 +167,7 @@ edx-drf-extensions==10.6.0 # via # -r requirements/test.txt # edx-organizations -edx-opaque-keys==3.0.0 +edx-opaque-keys==3.1.0 # via # -r requirements/test.txt # edx-drf-extensions diff --git a/requirements/quality.txt b/requirements/quality.txt index 5fd7d0126..c702d9239 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -166,7 +166,7 @@ edx-drf-extensions==10.6.0 # edx-organizations edx-lint==5.6.0 # via -r requirements/quality.in -edx-opaque-keys==3.0.0 +edx-opaque-keys==3.1.0 # via # -r requirements/test.txt # edx-drf-extensions diff --git a/requirements/test.txt b/requirements/test.txt index 9e6755778..2c91b09d5 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -138,7 +138,7 @@ edx-drf-extensions==10.6.0 # via # -r requirements/base.txt # edx-organizations -edx-opaque-keys==3.0.0 +edx-opaque-keys==3.1.0 # via # -r requirements/base.txt # edx-drf-extensions diff --git a/src/openedx_catalog/migrations/0001_initial.py b/src/openedx_catalog/migrations/0001_initial.py index afcdbfb9a..a5d8e3a26 100644 --- a/src/openedx_catalog/migrations/0001_initial.py +++ b/src/openedx_catalog/migrations/0001_initial.py @@ -91,10 +91,11 @@ class Migration(migrations.Migration): ( "course_id", opaque_keys.edx.django.models.CourseKeyField( + case_sensitive=True, + db_index=True, editable=False, help_text="The main identifier for this course. Includes the org, course code, and run.", max_length=255, - unique=True, verbose_name="Course ID", ), ), diff --git a/src/openedx_catalog/models/course_run.py b/src/openedx_catalog/models/course_run.py index 31e235d46..adceee7af 100644 --- a/src/openedx_catalog/models/course_run.py +++ b/src/openedx_catalog/models/course_run.py @@ -86,13 +86,13 @@ class CourseRun(models.Model): editable=False, ) course_id = CourseKeyField( - max_length=255, # CourseKeyField should specify this, not us. But Django complains if it's missing. - unique=True, + case_sensitive=True, + db_index=True, verbose_name=_("Course ID"), help_text=_("The main identifier for this course. Includes the org, course code, and run."), editable=False, - # Note: CourseKeyField is case-insensitive on MySQL and case sensitive on SQLite, so we apply an additional - # constraint below in the Meta section to achieve consistent case-insensitivity on both databases. + # This column must be unique, but we don't specify 'unique=True' here because we have an even stronger "case + # insensitively unique" constraint applied to this field below in Meta.constraints. ) # The catalog course stores the 'org' and 'course_code' fields, which must match the ones in the course ID. # Note: there is no need to load this relationship to get 'org' or 'course_code'; get them from `course_id` instead. @@ -213,7 +213,7 @@ class Meta: constraints = [ # catalog_course (org+course_code) and run must be unique together: models.UniqueConstraint("catalog_course", "run", name="oex_catalog_courserun_catalog_course_run_uniq"), - # Make course_id case insensitively unique on SQLite for consistency with MySQL. + # course_id is case-sensitively unique but we also want it to be case-insensitively unique: models.UniqueConstraint(Lower("course_id"), name="oex_catalog_courserun_course_id_ci"), # Enforce at the DB level that these required fields are not blank: models.CheckConstraint(condition=models.Q(run__length__gt=0), name="oex_catalog_courserun_run_not_blank"), From 377310378f2f1ca1f20d4eb95aa372ff400842e1 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 24 Feb 2026 16:43:05 -0800 Subject: [PATCH 17/29] test: expand tests, fix a bug with logging --- src/openedx_catalog/models/course_run.py | 4 +- tests/openedx_catalog/test_api.py | 50 +++++++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/openedx_catalog/models/course_run.py b/src/openedx_catalog/models/course_run.py index adceee7af..c14c187fd 100644 --- a/src/openedx_catalog/models/course_run.py +++ b/src/openedx_catalog/models/course_run.py @@ -184,7 +184,9 @@ def clean(self): correct_short_name = self.catalog_course.org.short_name if correct_short_name.lower() == self.course_id.org.lower(): log.warning( - 'Course run "{self.course_id}" does not match case of its org short_name "{correct_short_name}"' + 'Course run "%s" does not match case of its org short_name "%s"', + self.course_id, + correct_short_name, ) else: raise ValidationError("The CatalogCourse 'org' field should match the org in the course_id key.") diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 832dd1854..459ce773b 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -3,6 +3,7 @@ """ from datetime import datetime, timezone +import logging import pytest from django.db import connection @@ -227,7 +228,9 @@ def test_create_course_run_for_modulestore_course_with_existing_org(): # FIXME: this test passes on MySQL but not SQLite. We need to update the Organizations code to behave consistently. @pytest.mark.skipif(connection.vendor == "sqlite", reason="Only passes on MySQL") -def test_create_course_run_for_modulestore_course_with_existing_org_different_capitalization(): +def test_create_course_run_for_modulestore_course_with_existing_org_different_capitalization( + caplog: pytest.LogCaptureFixture, +): """ Test create_course_run_for_modulestore_course_with() when the org already exists but with different capitalization @@ -239,6 +242,21 @@ def test_create_course_run_for_modulestore_course_with_existing_org_different_ca run = api.create_course_run_for_modulestore_course_with( course_id, display_name="Introducción a las pruebas", language_short="es" ) + + # Verify that a warning was logged. We actually get two warnings - one from the API and one from model.clean(): + assert caplog.record_tuples == [ + ( + "openedx_catalog.api_impl", + logging.WARN, + 'The course with ID "course-v1:NewOrg+Test+2026" does not match its Organization.short_name "nEWoRG"', + ), + ( + "openedx_catalog.models.course_run", + logging.WARN, + 'Course run "course-v1:NewOrg+Test+2026" does not match case of its org short_name "nEWoRG"', + ), + ] + assert run.catalog_course.org_id == existing_org_id assert run.catalog_course.org_code == "nEWoRG" # Uses canonical capitalization assert run.catalog_course.course_code == "Test" @@ -290,7 +308,35 @@ def test_create_course_run_for_modulestore_course_with_existing_run(): assert old_run.run == "2025" # When there was only one run, the catalog course would be given the name of that run: assert new_run.catalog_course.display_name == "Previous Run (2025)" - assert new_run.display_name assert new_run.run == "2026" assert new_run.display_name == "New Run (2026)" assert new_run.course_id == course_id + + +def test_create_course_run_for_modulestore_course_run_that_exists(caplog: pytest.LogCaptureFixture) -> None: + """ + Test create_course_run_for_modulestore_course_with() when that exact + CourseRun already exists, e.g. due to a race condition. + """ + org_code, course_code, run_code = "NewOrg", "Test", "2026" + course_id = CourseKey.from_string(f"course-v1:{org_code}+{course_code}+{run_code}") + + existing_run = api.create_course_run_for_modulestore_course_with(course_id, display_name="Original Name") + # Call the API again to create the exact same run that we just created: + new_run = api.create_course_run_for_modulestore_course_with(course_id, display_name="New Name (ignore)") + + # Verify that a warning was logged: + assert caplog.record_tuples == [ + ( + "openedx_catalog.api_impl", + logging.WARN, + 'Expected to create CourseRun for "course-v1:NewOrg+Test+2026" but it already existed.', + ), + ] + + existing_run.refresh_from_db() # Let's make sure it hasn't changed + assert existing_run == new_run + assert existing_run.display_name == "Original Name" + assert new_run.display_name == "Original Name" + assert new_run.catalog_course.display_name == "Original Name" + assert new_run.run == run_code From 442eec16e1dfe44b0a3750eac339fe7c63ef0da2 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 25 Feb 2026 10:24:04 -0800 Subject: [PATCH 18/29] docs: some parts of platform call the course_code "number", so mention that --- src/openedx_catalog/migrations/0001_initial.py | 2 +- src/openedx_catalog/models/catalog_course.py | 2 +- src/openedx_catalog/models/course_run.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openedx_catalog/migrations/0001_initial.py b/src/openedx_catalog/migrations/0001_initial.py index a5d8e3a26..9e8752c7b 100644 --- a/src/openedx_catalog/migrations/0001_initial.py +++ b/src/openedx_catalog/migrations/0001_initial.py @@ -37,7 +37,7 @@ class Migration(migrations.Migration): "course_code", openedx_django_lib.fields.MultiCollationCharField( db_collations={"mysql": "utf8mb4_bin", "sqlite": "BINARY"}, - help_text='The course ID, e.g. "Math100".', + help_text='The course code/number, e.g. "Math100".', max_length=255, ), ), diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index 05ba625b3..db9464cbb 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -78,7 +78,7 @@ class CatalogCourse(models.Model): max_length=255, blank=False, null=False, - help_text=_('The course ID, e.g. "Math100".'), + help_text=_('The course code/number, e.g. "Math100".'), ) # When this catalog course was first created. We don't track "modified" as this model should be basically immutable, # and the only field that may ever change is "display_name"; we also don't want people to think that the "modified" diff --git a/src/openedx_catalog/models/course_run.py b/src/openedx_catalog/models/course_run.py index c14c187fd..bb0c8327b 100644 --- a/src/openedx_catalog/models/course_run.py +++ b/src/openedx_catalog/models/course_run.py @@ -149,7 +149,7 @@ def org_code(self) -> str: @property @admin.display(ordering="catalog_course__course_code") def course_code(self) -> str: - """Get the course code of this course, e.g. "Math100" """ + """Get the course code/number of this course, e.g. "Math100" """ # Note: 'self.catalog_course.course_code' may require a JOIN/query, but self.course_id.course does not. return self.course_id.course From edc943dfe8ce33a472dc6d3ac96124f519685204 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 25 Feb 2026 10:53:15 -0800 Subject: [PATCH 19/29] chore: fix import order --- tests/openedx_catalog/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 459ce773b..ff775081c 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -2,8 +2,8 @@ Tests related to the openedx_catalog python API """ -from datetime import datetime, timezone import logging +from datetime import datetime, timezone import pytest from django.db import connection From d804a4ec7f33e8bebf83b1ff33a94a6671766c5c Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 25 Feb 2026 10:54:17 -0800 Subject: [PATCH 20/29] docs: clarify CatalogCourse.language is "primary language" --- src/openedx_catalog/migrations/0001_initial.py | 2 +- src/openedx_catalog/models/catalog_course.py | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/openedx_catalog/migrations/0001_initial.py b/src/openedx_catalog/migrations/0001_initial.py index 9e8752c7b..493b93818 100644 --- a/src/openedx_catalog/migrations/0001_initial.py +++ b/src/openedx_catalog/migrations/0001_initial.py @@ -60,7 +60,7 @@ class Migration(migrations.Migration): openedx_django_lib.fields.MultiCollationCharField( db_collations={"mysql": "utf8mb4_bin", "sqlite": "BINARY"}, default=openedx_catalog.models.catalog_course.get_default_language_code, - help_text='The code representing the language of this catalog course\'s content. The first two digits must be the lowercase ISO 639-1 language code, optionally followed by a country/locale code. e.g. "en", "es", "fr-ca", "pt-br", "zh-cn", "zh-hk". ', + help_text='The code representing the primary language of this catalog course\'s content - the language in which the content is authored and which learners can learn without translation. (Translated versions of the course or parts of the course may be available in other languages if supported by the platform.) The first two digits must be the lowercase ISO 639-1 language code, optionally followed by a country/locale code. e.g. "en", "es", "fr-ca", "pt-br", "zh-cn", "zh-hk". ', max_length=64, ), ), diff --git a/src/openedx_catalog/models/catalog_course.py b/src/openedx_catalog/models/catalog_course.py index db9464cbb..dbbbd125f 100644 --- a/src/openedx_catalog/models/catalog_course.py +++ b/src/openedx_catalog/models/catalog_course.py @@ -110,10 +110,11 @@ class CatalogCourse(models.Model): null=False, default=get_default_language_code, help_text=_( - "The code representing the language of this catalog course's content. " - "The first two digits must be the lowercase ISO 639-1 language code, " - "optionally followed by a country/locale code. " - 'e.g. "en", "es", "fr-ca", "pt-br", "zh-cn", "zh-hk". ' + "The code representing the primary language of this catalog course's content - the language in which the " + "content is authored and which learners can learn without translation. (Translated versions of the course " + "or parts of the course may be available in other languages if supported by the platform.) " + "The first two digits must be the lowercase ISO 639-1 language code, optionally followed by a " + 'country/locale code. e.g. "en", "es", "fr-ca", "pt-br", "zh-cn", "zh-hk". ' ), ) From b83e3f7a24977c2c79f35f45d0b3e3a0c55ec4e1 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Wed, 25 Feb 2026 17:44:45 -0800 Subject: [PATCH 21/29] feat: API for deleting a course run and/or catalog course --- src/openedx_catalog/api_impl.py | 31 ++++++++++++++++++++++++++++ tests/openedx_catalog/test_api.py | 34 +++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/openedx_catalog/api_impl.py b/src/openedx_catalog/api_impl.py index 79c3a5960..faa3d143e 100644 --- a/src/openedx_catalog/api_impl.py +++ b/src/openedx_catalog/api_impl.py @@ -17,9 +17,11 @@ __all__ = [ "get_catalog_course", "update_catalog_course", + "delete_catalog_course", "get_course_run", "sync_course_run_details", "create_course_run_for_modulestore_course_with", + "delete_course_run", ] @@ -84,6 +86,20 @@ def update_catalog_course( cc.save(update_fields=update_fields) +def delete_catalog_course(catalog_course: CatalogCourse | int) -> None: + """ + Delete a `CatalogCourse`. This will fail with a `ProtectedError` if any runs exist. + + ⚠️ Does not check permissions. + ⚠️ Does not emit any course lifecycle events. + """ + if isinstance(catalog_course, CatalogCourse): + cc = catalog_course + else: + cc = CatalogCourse.objects.get(pk=catalog_course) + cc.delete() + + def get_course_run(course_id: CourseKey) -> CourseRun: """ Get a single course run. @@ -117,6 +133,7 @@ def sync_course_run_details( `update_course_run` API that will become the main way to rename a course. ⚠️ Does not check permissions. + ⚠️ Does not emit any course lifecycle events. """ run = CourseRun.objects.get(course_id=course_id) if display_name: @@ -143,6 +160,7 @@ def create_course_run_for_modulestore_course_with( not meant for historical data (use a data migration). ⚠️ Does not check permissions. + ⚠️ Does not emit any course lifecycle events. """ # Note: this code shares a lot with the code in # openedx-platform/openedx/core/djangoapps/content/course_overviews/migrations/0030_backfill_... @@ -202,3 +220,16 @@ def create_course_run_for_modulestore_course_with( log.warning('Expected to create CourseRun for "%s" but it already existed.', str(course_id)) return new_run + + +def delete_course_run(course_id: CourseKey) -> None: + """ + Delete a `CourseRun`. + + This may fail with a `ProtectedError` or other `IntegrityError` subclass if + there are still active references to the course run. + + ⚠️ Does not check permissions. + ⚠️ Does not emit any course lifecycle events. + """ + CourseRun.objects.get(course_id=course_id).delete() diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index ff775081c..d0ae573d7 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -7,6 +7,7 @@ import pytest from django.db import connection +from django.db.models import ProtectedError from django.test import override_settings from freezegun import freeze_time from opaque_keys.edx.keys import CourseKey @@ -141,6 +142,22 @@ def test_update_ignore_other_fields(python100: CatalogCourse) -> None: assert python100.display_name == "New Name" +# delete_catalog_course + + +def test_delete_catalog_course(python100: CatalogCourse, python100_summer26: CourseRun) -> None: + """Test that we can delete a CatalogCourse but only if no runs exist""" + # At first, deletion will fail because of the Summer2026 run: + with pytest.raises(ProtectedError): + api.delete_catalog_course(python100) + python100.refresh_from_db() # Make sure it's not deleted. + # Now delete the run, unblocking deletion of the catalog course: + python100_summer26.delete() # FIXME: use an API method for this. + api.delete_catalog_course(python100) + with pytest.raises(CatalogCourse.DoesNotExist): + python100.refresh_from_db() # Make sure it's gone + + # get_course_run @@ -340,3 +357,20 @@ def test_create_course_run_for_modulestore_course_run_that_exists(caplog: pytest assert new_run.display_name == "Original Name" assert new_run.catalog_course.display_name == "Original Name" assert new_run.run == run_code + + +# delete_course_run + + +def test_delete_course_run( + python100: CatalogCourse, + python100_summer26: CourseRun, + python100_winter26: CourseRun, +) -> None: + """Test that we can delete a CourseRun, passing in the object""" + api.delete_course_run(python100_summer26.course_id) + with pytest.raises(CourseRun.DoesNotExist): + python100_summer26.refresh_from_db() # Make sure it's gone + # The catalog course and other run is unaffected: + python100.refresh_from_db() + python100_winter26.refresh_from_db() From d8deaa20eae9eb00553adecf536bbd9ce4a9ec4d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 26 Feb 2026 14:38:25 -0800 Subject: [PATCH 22/29] chore: fix lint issue --- tests/openedx_catalog/test_models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index 891207021..cd3d9dc93 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -1,6 +1,7 @@ """ Tests related to the Catalog models (CatalogCourse, CourseRun) """ +# pylint: disable=unused-argument # mypy: disable-error-code="misc" # (Ignore 'Unexpected attribute "org_code" for model "CatalogCourse"' until # https://github.com/typeddjango/django-stubs/issues/1034 is fixed.) From 43f7b7cf4464fd019905d94e07a8c702acb2239b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 26 Feb 2026 16:39:00 -0800 Subject: [PATCH 23/29] fix: CourseRun database constraint wasn't working with CCX keys --- .../migrations/0001_initial.py | 16 +++++--------- src/openedx_catalog/models/course_run.py | 22 +++++++++++++------ tests/openedx_catalog/test_models.py | 5 +++++ 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/openedx_catalog/migrations/0001_initial.py b/src/openedx_catalog/migrations/0001_initial.py index 493b93818..cf5b9795b 100644 --- a/src/openedx_catalog/migrations/0001_initial.py +++ b/src/openedx_catalog/migrations/0001_initial.py @@ -1,7 +1,6 @@ # Generated by Django 5.2.11 on 2026-02-10 03:08 import django.db.models.deletion -import django.db.models.expressions import django.db.models.functions.text import django.db.models.lookups import opaque_keys.edx.django.models @@ -174,7 +173,10 @@ class Migration(migrations.Migration): migrations.AddConstraint( model_name="courserun", constraint=models.UniqueConstraint( - models.F("catalog_course"), models.F("run"), name="oex_catalog_courserun_catalog_course_run_uniq" + models.F("catalog_course"), + models.F("run"), + condition=models.Q(("course_id__startswith", "ccx"), _negated=True), + name="oex_catalog_courserun_catalog_course_run_uniq", ), ), migrations.AddConstraint( @@ -198,14 +200,8 @@ class Migration(migrations.Migration): migrations.AddConstraint( model_name="courserun", constraint=models.CheckConstraint( - condition=django.db.models.lookups.Exact( - django.db.models.functions.text.Right( - "course_id", - django.db.models.expressions.CombinedExpression( - django.db.models.functions.text.Length("run"), "+", models.Value(1) - ), - ), - django.db.models.functions.text.Concat(models.Value("+"), "run"), + condition=django.db.models.lookups.GreaterThan( + django.db.models.functions.text.StrIndex("course_id", models.F("run")), 0 ), name="oex_catalog_courserun_courseid_run_match_exactly", violation_error_message="The CourseRun 'run' field should match the run in the course_id key.", diff --git a/src/openedx_catalog/models/course_run.py b/src/openedx_catalog/models/course_run.py index bb0c8327b..22cb32233 100644 --- a/src/openedx_catalog/models/course_run.py +++ b/src/openedx_catalog/models/course_run.py @@ -7,8 +7,9 @@ from django.contrib import admin from django.core.exceptions import ValidationError from django.db import models -from django.db.models.functions import Concat, Length, Lower, Right -from django.db.models.lookups import Exact +from django.db.models import F +from django.db.models.functions import Length, Lower, StrIndex +from django.db.models.lookups import GreaterThan from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.django.models import CourseKeyField @@ -214,7 +215,13 @@ class Meta: ordering = ("-created",) constraints = [ # catalog_course (org+course_code) and run must be unique together: - models.UniqueConstraint("catalog_course", "run", name="oex_catalog_courserun_catalog_course_run_uniq"), + models.UniqueConstraint( + "catalog_course", + "run", + name="oex_catalog_courserun_catalog_course_run_uniq", + # With one unfortunate exception: CCX courses all have the same org, code, and run exactly: + condition=~models.Q(course_id__startswith="ccx"), + ), # course_id is case-sensitively unique but we also want it to be case-insensitively unique: models.UniqueConstraint(Lower("course_id"), name="oex_catalog_courserun_course_id_ci"), # Enforce at the DB level that these required fields are not blank: @@ -222,11 +229,12 @@ class Meta: models.CheckConstraint( condition=models.Q(display_name__length__gt=0), name="oex_catalog_courserun_display_name_not_blank" ), - # Enforce that the course ID must end with "+run" where "run" is an exact match for the "run" field. - # This check may be removed or changed in the future if our course ID format ever changes + # Enforce at the DB level that the "run" field value appears in the course ID: models.CheckConstraint( - # Note: EndsWith() on SQLite is always case-insensitive, so we code the constraint like this: - condition=Exact(Right("course_id", Length("run") + 1), Concat(models.Value("+"), "run")), + condition=GreaterThan(StrIndex("course_id", F("run")), 0), + # The following check condition (ends with "+run") is even stronger, but doesn't work with CCX keys + # like "ccx-v1:org+code+run+ccx@1" which we also need to support. + # condition=Exact(Right("course_id", Length("run") + 1), Concat(models.Value("+"), "run")), name="oex_catalog_courserun_courseid_run_match_exactly", violation_error_message=_("The CourseRun 'run' field should match the run in the course_id key."), ), diff --git a/tests/openedx_catalog/test_models.py b/tests/openedx_catalog/test_models.py index cd3d9dc93..e217b1f94 100644 --- a/tests/openedx_catalog/test_models.py +++ b/tests/openedx_catalog/test_models.py @@ -321,3 +321,8 @@ def test_run_exact(org1) -> None: CourseRun.objects.filter(pk=run.pk).update( course_id=CourseLocator(org="Org1", course="Test302", run="mixedcase"), ) + + +# TODO: it would be good to test here that CourseRun objects work correctly with CCX keys like +# "ccx-v1:org+code+run+ccx@1", but I don't want to introduce a dependency on edx-ccx-keys for now. Once we consolidate +# all the key types into a single repo, we can add a test here. From 88aef751beba8c969eead17459b1b9023bb9ba19 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 27 Feb 2026 13:57:08 -0800 Subject: [PATCH 24/29] refactor: course_id -> course_key, run -> run_code --- src/openedx_catalog/admin.py | 8 +- src/openedx_catalog/api.py | 2 +- src/openedx_catalog/api_impl.py | 28 +++---- .../migrations/0001_initial.py | 22 +++--- src/openedx_catalog/models/catalog_course.py | 4 +- src/openedx_catalog/models/course_run.py | 72 +++++++++-------- src/openedx_catalog/signals.py | 10 +-- tests/openedx_catalog/test_api.py | 68 ++++++++-------- tests/openedx_catalog/test_models.py | 79 ++++++++++--------- tests/openedx_catalog/test_signals.py | 4 +- 10 files changed, 154 insertions(+), 143 deletions(-) diff --git a/src/openedx_catalog/admin.py b/src/openedx_catalog/admin.py index ff825e13e..d9fd99502 100644 --- a/src/openedx_catalog/admin.py +++ b/src/openedx_catalog/admin.py @@ -66,7 +66,7 @@ def runs_summary(self, obj: CatalogCourseWithRunCount) -> str: if obj.run_count == 0: return "-" url = reverse("admin:openedx_catalog_courserun_changelist") + f"?catalog_course={obj.pk}" - first_few_runs = obj.runs.order_by("-run")[:3] + first_few_runs = obj.runs.order_by("-run_code")[:3] runs_summary = ", ".join(run.run for run in first_few_runs) if obj.run_count > 4: runs_summary += f", ... ({obj.run_count})" @@ -81,14 +81,14 @@ class CourseRunAdmin(admin.ModelAdmin): The CourseRun model admin. """ - list_display = ["display_name", "created_date", "catalog_course", "org_code", "course_code", "run", "warnings"] - readonly_fields = ("course_id",) + list_display = ["display_name", "created_date", "catalog_course", "org_code", "course_code", "run_code", "warnings"] + readonly_fields = ("course_key",) # There may be thousands of catalog courses, so don't use