From beb9839bfde95afc0497ba737f776df865bb9d77 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 6 Feb 2026 15:59:21 -0800 Subject: [PATCH 01/12] 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 5cdb8bdd7..e082d8d64 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 b6b5f2fa4..f12b2291e 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.129.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.1 diff --git a/requirements/doc.txt b/requirements/doc.txt index 9cbea1c8f..84e3353ad 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.129.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 7e85ed1da..4c869ce72 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.129.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.1 # via pylint pluggy==1.6.0 diff --git a/requirements/test.txt b/requirements/test.txt index e37404a32..5ee95a5ef 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.129.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 56d5d86e8dec8bad2d45f2dd801b208137048640 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 13 Feb 2026 11:50:34 -0800 Subject: [PATCH 02/12] 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 f987ecfdcd401c93ac20e8ddf58d42a053657fb6 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 19 Feb 2026 10:45:20 -0800 Subject: [PATCH 05/12] 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 59c7301c97bb0595c4c5f03e0c1d98673fc9589e Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 14:05:00 -0800 Subject: [PATCH 06/12] 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 bb70048e95ca64cb415afa307c7463676a949e75 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 16:55:14 -0800 Subject: [PATCH 07/12] 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 9d9c2d0209b7d72de2b1dd11a8aee3e7d377db93 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 17:17:01 -0800 Subject: [PATCH 08/12] 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 775909b722665d20be1369755467139977aa5b5d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 18:01:51 -0800 Subject: [PATCH 09/12] 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 1c61f304634e0f940c85fb3440887b5a1c4fe76f Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 18:14:04 -0800 Subject: [PATCH 10/12] 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 bf0fbd65f8e4fa805f4147ed7ffc4ed641770b4d Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 21 Feb 2026 18:17:09 -0800 Subject: [PATCH 11/12] 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 7ca2a15b2a4ea3f086027ef7a9c7055be7eae363 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sun, 22 Feb 2026 14:01:50 -0800 Subject: [PATCH 12/12] 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