feat: provide openedx_learning djangoapp and CompetencyTaxonomy model by jesperhodge · Pull Request #712 · openedx/openedx-core

jesperhodge · 2026-08-07T14:39:20Z

AI-first work.

This was coded with human-in-the-loop. I created the initial instructions; Claude Code created a plan, I (Jesper) reviewed the plan and edited as needed; Claude Code implemented, and I reviewed the implementation and tested.

What this does

Open edX already has "taxonomies", which are just collections of tags. This PR adds a way
to mark a taxonomy as holding competencies rather than ordinary tags, so that
competency-based education (CBE) features can later be built on top of it.

Adds a new Django app, openedx_learning, and its first sub-package, cbe. This is a
new home for learning-side features. It follows the same layout as the existing
openedx_content app: one Django app, split into subfolders for readability.
Adds one model, CompetencyTaxonomy. Marking a taxonomy as a competency taxonomy means
creating one of these. It does not copy the taxonomy or replace it; it points at the
existing taxonomy and adds a single extra setting to it.
Adds a small public API so other code can ask "is this taxonomy a competency taxonomy?"
without needing to know how that link is stored.
Wires it all up: app registration, the database migration, a Django admin page, and a
rule that keeps the dependency pointing one way (the CBE code may use the tagging code,
never the reverse).

There is no REST API and no user interface here. This is the groundwork that the follow-up
tickets #641 and #642 are waiting on.

Depends on work in edx-platform

openedx-core is a library, so this app only becomes active once edx-platform lists it in
its settings. That platform-side work is tracked in openedx/openedx-platform#38958.

How to test

This runs entirely inside this repo against a local SQLite file, so you do not need
devstack, tutor, or MySQL. manage.py defaults to the projects.dev settings, which are
already configured to use SQLite at ./dev.db, so there is nothing to configure.

1. Set up a virtualenv (skip if you already have one for this repo):

python3 -m venv .venv
.venv/bin/pip install -r requirements/dev.txt
.venv/bin/pip install -e .

.venv/ is already in .gitignore.

2. Create the database and a login.

.venv/bin/python manage.py migrate
.venv/bin/python manage.py createsuperuser

The first command creates dev.db and applies all migrations, including the new
openedx_learning.0001_initial.

3. Start the server.

.venv/bin/python manage.py runserver

4. Add a competency taxonomy through the admin.

Open http://127.0.0.1:8000/admin/ and log in. You should see a section called
Open edX Core > Learning containing Competency Taxonomies. Click Add, fill in
Name (for example Nursing) and Export id (for example nursing-v1), and save.

You should land back on a list showing the columns Name, Export id, Enabled, and Taxonomy
overrides org.

5. Confirm it wrote to both tables.

sqlite3 dev.db "SELECT t.id, t.name, c.taxonomy_overrides_org
  FROM oel_tagging_taxonomy t
  JOIN openedx_learning_competencytaxonomy c ON c.taxonomy_ptr_id = t.id;"

You should get your row back, something like 1|Nursing|0.

That join returning a result is the thing worth seeing: one save through the admin created
a row in the existing tagging table and a linked row in the new CBE table. That is the
link this PR is really about.

Cleaning up: rm dev.db and re-run step 2 to start over.

openedx-webhooks · 2026-08-07T14:39:28Z

Thanks for the pull request, @jesperhodge!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

kdmccormick

two stylistic comments, but otherwise this looks good to me.

@ormsbee let me know if you'd like to do a pass before I approve and merge.

kdmccormick · 2026-08-07T17:54:04Z

+# The accessor Django generates for the multi-table-inheritance link from Taxonomy to
+# CompetencyTaxonomy. Deliberately private: callers use the functions below rather than
+# spelling this out, so a model rename is a one-line change here and nowhere else.
+_COMPETENCY_TAXONOMY_RELATION = "competencytaxonomy"


i think that this is unnecessarily indirect, and as an abstraction it's leaky. you cannot stop callers from accessing taxonomy.competencytaxonomy directly, so renaming the CompetencyTaxonomy model would be a breaking change--we'd best treat it that way rather than pretending that all references to .competencytaxonomy will go through the two functions below.

I would just use the "competencytaxonomy" string literal in the function below, as is idiomatic in django code. mypy is sometimes actually smart enough to do type checking on expressions like taxonomies.select_related("competencytaxonomy"), but when it's been abstracted out to taxonomies.select_related(_COMPETENCY_TAXONOMY_RELATION), it will never be able to do any static analysis.

kdmccormick · 2026-08-07T17:58:00Z

+        help_text=_(
+            "When both an organization-scoped and a taxonomy-scoped rule profile "
+            "could apply to a criterion, this decides which one is assigned: false "
+            "assigns the organization's, true assigns this taxonomy's."


could you add a note stating that this isn't yet used anywhere? bonus points if you add a comment linking to a task or epic issue that would implement it.

We do not have a task or epic that would implement this since it's out of scope. Epic 6 is what most closely relates to it though, but we don't currently have Epic level Github Issues, so that's a handful of issues. The one most closely related is probably #631 though.

If it's out of scope for Willow, then can we omit the field for now @mgwozdz-unicon ?

I think it would be fine to omit the field for now.

ormsbee · 2026-08-07T19:10:03Z

@kdmccormick: No need to block on me. Thank you.

mgwozdz-unicon · 2026-08-10T15:40:31Z

A couple of things from going through this:

READMEs. Every other top-level Django app in this repo (openedx_content, openedx_tagging, openedx_catalog, openedx_django_lib) has a README.rst in its own directory; openedx_learning doesn't yet. Given that, I'd add at least one for the app itself, and preferably a second one for the cbe applet too (that tier's less consistent across the repo, 3 of 8 applets have one, but still seems worth it for a brand-new applet).

test_select_competency_taxonomies_avoids_n_plus_1 comment. The test's comment ties its explanation of the pk__in filter to one specific named seed, the "Language" taxonomy from oel_tagging's data migration. #654 removes or converts that exact row, so the comment would end up referencing something that no longer exists. Wording it more generally now, e.g. guarding against "any taxonomies seeded outside this test" rather than naming "Language" specifically, keeps the comment accurate regardless of what happens to that seed data later, without anyone needing to circle back and touch it.

A couple smaller things Claude surfaced while I was reviewing this:

Question (src/openedx_learning/applets/cbe/api.py, is_competency_taxonomy): behavior is untested for an unsaved instance (pk is None). Accessing the reverse one-to-one descriptor on an unsaved parent raises a plain ValueError in Django, which hasattr() won't catch, so that would propagate uncaught instead of returning False. Probably a non-issue if no caller ever passes an unsaved instance, just flagging it as an assumption that's currently implicit rather than stated or guarded.

Nit (src/openedx_learning/admin.py:5): wildcard-imports from applets/cbe/admin.py, which doesn't declare an __all__, unlike this PR's models.py/api.py aggregators, which wildcard-import from modules that do declare one. Low risk, just inconsistent with the pattern the rest of the PR sets up.

jesperhodge · 2026-08-12T16:14:01Z

@mgwozdz-unicon thanks for the review! I made changes to address all your comments.

One exception:
"Question (src/openedx_learning/applets/cbe/api.py, is_competency_taxonomy): behavior is untested for an unsaved instance (pk is None). Accessing the reverse one-to-one descriptor on an unsaved parent raises a plain ValueError in Django, which hasattr() won't catch, so that would propagate uncaught instead of returning False. Probably a non-issue if no caller ever passes an unsaved instance, just flagging it as an assumption that's currently implicit rather than stated or guarded."

Claude figured out that this is not correct - hasattr() evaluates to False for this, and Claude added a test that shows that this works, so I think that's resolved as well.

I wonder why Claude surfaced this erroneous point. Could you share what model and effort level you ran it with, and what your prompt was? Did Claude use the /code-review skill for this?

mgwozdz-unicon

Thanks for addressing all of these. I've reviewed it again and this looks good to me.

On your question: Sonnet 5, high reasoning effort. I didn't use the /code-review skill here, my prompt included a specific list of questions plus an open "anything else worth flagging?", and that didn't happen to trigger it to use the skill (though I've found on my other projects that it often does), but I didn't force it to use the skill afterward either. This particular point came out of that open-ended last question rather than a targeted look at that one method. Going forward I'll make a point of invoking /code-review explicitly for these.

It's called out separately from my own points since it came straight from Claude as a flag to double-check rather than a verified finding. This time it was wrong on the specific exception type, but right that it was worth a test rather than an assumption.

bradenmacdonald

From a quick look, LGTM! I didn't do a thorough review.

jesperhodge · 2026-08-25T14:15:07Z

@kdmccormick bump - I addressed your comments, could you check this again?

jesperhodge · 2026-08-26T19:13:41Z

Hi @ormsbee it seems Kyle is out of office. Could you take this over? I fixed everything mentioned in Kyle's comments.

ormsbee · 2026-08-26T20:00:50Z

@jesperhodge: @kdmccormick is back today, and plans to review this PR.

kdmccormick

two small nits but if you agree with those, then good to go 👍🏻

kdmccormick · 2026-08-26T20:33:33Z

+
+# The default MTI reverse accessor. django-stubs cannot see dynamically added
+# accessors, so these tests reach it by name; that name is the ADR-0013 contract.
+RELATION = "competencytaxonomy"


again, this is strangely indirect. just access .competencytaxonomy like app code would. if we messed up, then there'll be an AttributeError.

kdmccormick · 2026-08-26T20:36:46Z

@@ -0,0 +1,11 @@
+Learning App


Please bump the version to 1.3.0 https://github.com/openedx/openedx-core/blob/main/src/openedx_core/__init__.py

Going forward: do a patch bump for every bugfix, a minor bump for every backwards-compatible feature, and a major bump for every breaking change. (We'll soon have tooling set up to do this automatically based on conventional commits -- we're testing that out on other repos first).

ormsbee · 2026-08-27T14:04:48Z

@jesperhodge: I see that you've made the requested changes. Please resolve the conflicts, and I'll merge. If you could also squash your commits and give a revised commit message, that would be appreciated (otherwise, I just have to take a stab at it).

Thank you.

This PR adds a way to mark a taxonomy as holding competencies rather than ordinary tags, so that competency-based education (CBE) features can later be built on top of it. * Adds a new Django app, openedx_learning, and its first sub-package, cbe. This is a new home for learning-side features. It follows the same layout as the existing openedx_content app: one Django app, split into subfolders for readability. * Adds one model, CompetencyTaxonomy. Marking a taxonomy as a competency taxonomy means creating one of these. It does not copy the taxonomy or replace it; it points at the existing taxonomy and adds a single extra setting to it. * Adds a small public API so other code can ask 'is this taxonomy a competency taxonomy?' without needing to know how that link is stored. * Wires it all up: app registration, the database migration, a Django admin page, and a rule that keeps the dependency pointing one way (the CBE code may use the tagging code, never the reverse).

jesperhodge · 2026-08-27T17:23:47Z

@ormsbee thanks a lot; it's squashed. If you wouldn't mind could you just ping me and @anton Lezhneu in slack when it's merged? A single "ping" word is enough

Implements the authoring and definition half of the CBE data model from ADR-0002: CompetencyCriteriaGroup (internal AND/OR nodes), CompetencyRuleProfile (reusable scoped evaluation defaults) and CompetencyCriterion (leaf nodes). Also adds the taxonomy_overrides_org column that PR openedx#712 left off CompetencyTaxonomy. CompetencyRuleProfile.scope_code is a generated, never-null column with a plain unique constraint. SQL never treats two NULLs as equal, so a unique constraint over the three nullable scope columns would accept two rows with the same scope, and the conditional UniqueConstraint that would normally fix that compiles to a partial index MySQL does not support. Both structural invariants are database check constraints rather than clean() checks, since DRF serializers, QuerySet.update() and bulk_create() never call full_clean(). Payload shape validation stays in clean(), per the issue. Every new foreign key is on_delete=PROTECT with a TODO(openedx#799) comment. That is a fail-closed placeholder, not a per-key decision; openedx#799 sets the real values once openedx#655 lands. openedx_catalog joins .importlinter's root_packages and the src_layering contract, since CompetencyCriteriaGroup.course is the first foreign key from openedx_learning into that app. django-simple-history moves into base.in: it was only ever a transitive dependency of edx-organizations, and setup.py builds install_requires from base.in. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

PR openedx#712 shipped CompetencyTaxonomy without this column. It settles a tiebreak that cannot arise yet: when a criterion could inherit its rule from an organization-scoped CompetencyRuleProfile or from a taxonomy-scoped one, this flag decides which wins (ADR-0002 Decision 4). Organization-scoped profiles do not exist, so no code path reads it. Adding the column now avoids a later migration against a table that by then has learner data hanging off it. Turns cbe/models.py into a models/ package, since the three criteria models that follow form one connected structure and want a module of their own. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@jesperhodge

Decision 3 described scope_code as a never-null, database-generated column. The implementation needs a plain column that goes null while a profile is archived, because nulling it is what frees that scope for a replacement: SQL never treats two NULLs as equal, so any number of archived rows may share a scope while exactly one live row holds it. The alternative, a conditional unique index over the three nullable scope columns, is what Rejected Alternative 6 already ruled out, because MySQL does not support partial indexes and Django silently skips creating one there. Says explicitly that the three scope columns are never cleared, so archiving loses no information and an archived profile stays restorable. Nulling scope_code releases its claim on the unique slot, not the record of the scope. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> docs: record every on_delete on the competency delete path Decision 7 described the archive-versus-hard-delete rule and left every foreign key's on_delete to be inferred from it. State them instead, so the models can be written from the ADR rather than the ADR from the models. The criteria tree cascades, which is what carries a delete down to the learner status tables where the guarantee is enforced. CompetencyCriterion.rule_profile is RESTRICT rather than PROTECT: both refuse a direct profile delete, but only RESTRICT lets a scope owner's deletion carry the profile away, because it ignores referencing rows that the same operation is already deleting. That removes the taxonomy-delete failure PROTECT would have caused. A narrower course-scoped case survives, since a criterion's profile assignment is independent of its tree's course scope, and it is recorded rather than fixed. The learner status tables protect the node they track, cascade from the learner so this library cannot veto User.delete() platform-wide, and protect the seeded status lookup. Foreign keys into tables this decision does not own are listed as inherited, so the whole delete path is legible in one place. Also note that CompetencyMasteryStatuses has no delete constraint of its own: the referencing PROTECTs cover a status in use, and nothing covers an unused one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> fix: Apply batched suggestions from code review Co-authored-by: Jesper Hodge <19345795+jesperhodge@users.noreply.github.com> docs: apply suggestions from github PR build: declare django-simple-history and layer openedx_catalog django-simple-history is pinned in the compiled requirements only as a transitive dependency of edx-organizations, and simple_history is absent from INSTALLED_APPS, so HistoricalRecords() would not work. Declare it directly and register the app in both settings modules. HistoricalRecords() itself works without the app installed, but its admin integration and management commands do not, and the package ships no system check to say so. openedx_catalog appears in neither .importlinter's root_packages nor its layering contract, so the first openedx_learning to openedx_catalog import would pass unexamined. The criteria models scope to a CourseRun, so that import is about to exist. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> fix: rank openedx_catalog as openedx_content's sibling, not below it Issue openedx#641's AC calls for openedx_content | openedx_catalog so the layers contract doesn't also decide the catalog-to-content direction, which src/openedx_catalog/ARCHITECTURE.md still records as TBD. fix: Apply suggestion from @jesperhodge Apply suggestion from @jesperhodge feat: add CompetencyTaxonomy.taxonomy_overrides_org PR openedx#712 shipped CompetencyTaxonomy without this column. It settles a tiebreak that cannot arise yet: when a criterion could inherit its rule from an organization-scoped CompetencyRuleProfile or from a taxonomy-scoped one, this flag decides which wins (ADR-0002 Decision 4). Organization-scoped profiles do not exist, so no code path reads it. Adding the column now avoids a later migration against a table that by then has learner data hanging off it. Turns cbe/models.py into a models/ package, since the three criteria models that follow form one connected structure and want a module of their own. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> docs: Apply batched suggestions from code review Co-authored-by: Jesper Hodge <19345795+jesperhodge@users.noreply.github.com> feat: add CompetencyCriteriaGroup, the criteria tree's AND/OR node A single CompetencyAchievementCriteria is one root CompetencyCriteriaGroup plus all of its descendant groups, per ADR-0002 Decision 2. Two constraints a reader may expect are deliberately absent: nothing ties logic_operator to child count, and there is no UniqueConstraint on (parent, ordering). A child group cannot be saved until its parent's primary key exists, so a parent's clean() always sees zero children, and leaf criteria carry no ordering column at all, so sibling order among them would stay undefined while looking solved. Both belong to the authoring API. All three foreign keys cascade, per ADR-0002 Decision 7. A criteria tree means nothing without the competency it evaluates, the course run that scopes it, or the group above it, so on_delete here expresses containment rather than protection. Those same cascades are how a delete reaches the PROTECT on the learner status tables, which is where deletion is actually refused. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> test: move CompetencyCriteriaGroup's own delete tests down to this PR These five tests only need CompetencyCriteriaGroup, so they belong with the on_delete values this PR already declares, not in the cross-model part 8 suite they were drafted alongside. fix: Apply suggestion from @jesperhodge test: cover multi-level cascade, taxonomy delete, and no delete() override Issue openedx#641's AC asks for group deletion to cascade at any depth, for a taxonomy delete to reach this model's criteria groups transitively, and for no delete() override to land in this ticket. Only depth-1 cascade and the tag-level cascade were pinned; add the three gaps directly. fix: delete the root, not the middle node, in the depth-cascade test Deleting the middle node only re-proved the one-hop cascade the depth-1 test already covers. Deleting the root and checking the grandchild is what actually exercises the collector recursing through more than one level from a single delete. test: drop the no-delete()-override test Not valuable enough to keep. feat: add the CBE rule payload contract ADR-0002 Decision 3 stores an evaluation rule as a rule_type plus a JSON rule_payload whose shape that type defines, rather than as fixed op/value/scale columns, so a future rule type can add its own fields without a migration. The cost of JSON is that nothing enforces the shape, so this adds the validator the two criteria models will call from clean(). Grade is the only supported type. Its value is a fraction from 0.0 to 1.0, not a number out of 100, which is the mistake an author is most likely to make, so the out-of-range message names the convention rather than only rejecting the value. RuleType declares exactly the types that have a payload spec, so a type can never be offered as a choice without being saveable. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> refactor: type the CBE rule payload shape with a TypedDict GradePayload declares the stored shape of a Grade rule_payload, so that a site holding a validated payload can be annotated with it and mypy checks payload literals against the declared field names and types. The runtime validation is unchanged: a TypedDict value is a plain dict, so the JSONField round-trip and the seeded default row are unaffected. This also removes two duplications. The expected key set now comes from the TypedDict rather than a hand-written frozenset repeating the keys _validate_grade_payload reads, and the comparison operators are declared once as a Literal instead of again in a separate set. validate_rule_payload takes `object` rather than `Any`. Its argument is untrusted JSON out of a JSONField and cannot be narrowed at the signature, but `object` makes mypy enforce the isinstance guard that `Any` allowed a caller to skip unchecked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> refactor: improve rule payload validations fix: Apply suggestion from @jesperhodge feat: add CompetencyRuleProfile and seed the system default A reusable evaluation rule scoped to at most one of an organization, a course or a taxonomy, plus the one row scoped to none of them, which is the rule every criterion falls back to. A deployment that adds no profiles of its own gets an 80% threshold. See ADR-0002 Decision 3. Uniqueness per scope cannot be a plain constraint over the three nullable scope columns, because SQL never treats two NULLs as equal, and it cannot be a conditional constraint either, because MySQL has no partial unique indexes and Django silently skips creating one there. So the scope collapses into a derived scope_code column with one unconditional unique constraint. scope_code goes null while a profile is archived, which frees that scope for a replacement; the three scope columns are never cleared, so nothing is lost. Scope is immutable after creation, so criteria already resolved to a profile are never silently re-scoped. Deleting a scope owner takes the profile scoped to it, so course and competency_taxonomy cascade, per ADR-0002 Decision 7. organization does not: an Organization is not a competency definition record, and edx-organizations retires one by clearing its active flag rather than deleting the row, so PROTECT there refuses a delete that should not be happening. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> test: move CompetencyRuleProfile's own delete tests down to this PR These six tests only need CompetencyRuleProfile, so they belong with the on_delete values this PR already declares, not in the cross-model part 8 suite they were drafted alongside. fix: state the rule_payload shape in help_text instead of naming a function rule_payload's help_text told the reader to "see validate_rule_payload for the shape it must match". help_text renders in the Django admin and the DRF browsable API, so that is user-facing text pointing at an internal Python function the reader cannot open, which is what rule_payloads.py's own docstring forbids for the error messages beside it. It now states the shape. 0004 is amended in place because help_text is part of a field's deconstructed kwargs; makemigrations --check confirms no new migration is needed. Also cover the seeded system-default row against the payload contract. 0005_seed_default_rule_profile writes its payload as a literal and cannot check it: a historical migration must not import rule_payloads, and apps.get_model() returns a model without the custom clean(). Nothing else reconciled that literal with the validator, so tightening _validate_grade_payload would have left every deployment's default row invalid with no failing test. The new test is the one place the two meet. The neighbouring docstring credited the seeding to migration 0003, which is competencycriteriagroup. It is 0005. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> feat: add CompetencyCriterion, the criteria tree's leaf A leaf points at one ObjectTag, meaning one specific piece of tagged content, and takes its pass rule either from a shared CompetencyRuleProfile or from its own inline override pair. A check constraint enforces ADR-0002 Decision 4's invariant: never both, never neither. The stored rule_profile is not resolved at read time. Decision 4 assigns it at four named write events and stores the result, so a criterion that already resolved to a less specific profile is not silently re-governed when a more specific one appears later. Computing that assignment is authoring-API work and is not here. group and object_tag cascade, per ADR-0002 Decision 7: a leaf means nothing without the group above it or the content association it evaluates. rule_profile is RESTRICT rather than PROTECT. Both refuse a direct profile delete while any criterion is assigned to it, which is what makes a profile archive-only at the ORM layer. They differ once the profile is deleted as part of a larger operation: PROTECT raises for any referencing row it finds in the database, so deleting a CompetencyTaxonomy would fail naming a criterion the same operation was already about to remove, while RESTRICT ignores rows that are themselves being deleted and lets that cascade through. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> test: move CompetencyCriterion's delete tests and the tree integration test down to this PR This model completes the criteria tree, so its own on_delete values, the transitive tag/taxonomy cascades that only exist once it does, the two RESTRICT-vs-PROTECT payoff/residual scenarios, and the tree-wide integration test all belong here, not in the cross-model part 8 suite they were drafted alongside.

@jesperhodge

Decision 3 described scope_code as a never-null, database-generated column. The implementation needs a plain column that goes null while a profile is archived, because nulling it is what frees that scope for a replacement: SQL never treats two NULLs as equal, so any number of archived rows may share a scope while exactly one live row holds it. The alternative, a conditional unique index over the three nullable scope columns, is what Rejected Alternative 6 already ruled out, because MySQL does not support partial indexes and Django silently skips creating one there. Says explicitly that the three scope columns are never cleared, so archiving loses no information and an archived profile stays restorable. Nulling scope_code releases its claim on the unique slot, not the record of the scope. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> docs: record every on_delete on the competency delete path Decision 7 described the archive-versus-hard-delete rule and left every foreign key's on_delete to be inferred from it. State them instead, so the models can be written from the ADR rather than the ADR from the models. The criteria tree cascades, which is what carries a delete down to the learner status tables where the guarantee is enforced. CompetencyCriterion.rule_profile is RESTRICT rather than PROTECT: both refuse a direct profile delete, but only RESTRICT lets a scope owner's deletion carry the profile away, because it ignores referencing rows that the same operation is already deleting. That removes the taxonomy-delete failure PROTECT would have caused. A narrower course-scoped case survives, since a criterion's profile assignment is independent of its tree's course scope, and it is recorded rather than fixed. The learner status tables protect the node they track, cascade from the learner so this library cannot veto User.delete() platform-wide, and protect the seeded status lookup. Foreign keys into tables this decision does not own are listed as inherited, so the whole delete path is legible in one place. Also note that CompetencyMasteryStatuses has no delete constraint of its own: the referencing PROTECTs cover a status in use, and nothing covers an unused one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> fix: Apply batched suggestions from code review Co-authored-by: Jesper Hodge <19345795+jesperhodge@users.noreply.github.com> docs: apply suggestions from github PR build: declare django-simple-history and layer openedx_catalog django-simple-history is pinned in the compiled requirements only as a transitive dependency of edx-organizations, and simple_history is absent from INSTALLED_APPS, so HistoricalRecords() would not work. Declare it directly and register the app in both settings modules. HistoricalRecords() itself works without the app installed, but its admin integration and management commands do not, and the package ships no system check to say so. openedx_catalog appears in neither .importlinter's root_packages nor its layering contract, so the first openedx_learning to openedx_catalog import would pass unexamined. The criteria models scope to a CourseRun, so that import is about to exist. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> fix: rank openedx_catalog as openedx_content's sibling, not below it Issue openedx#641's AC calls for openedx_content | openedx_catalog so the layers contract doesn't also decide the catalog-to-content direction, which src/openedx_catalog/ARCHITECTURE.md still records as TBD. fix: Apply suggestion from @jesperhodge Apply suggestion from @jesperhodge feat: add CompetencyTaxonomy.taxonomy_overrides_org PR openedx#712 shipped CompetencyTaxonomy without this column. It settles a tiebreak that cannot arise yet: when a criterion could inherit its rule from an organization-scoped CompetencyRuleProfile or from a taxonomy-scoped one, this flag decides which wins (ADR-0002 Decision 4). Organization-scoped profiles do not exist, so no code path reads it. Adding the column now avoids a later migration against a table that by then has learner data hanging off it. Turns cbe/models.py into a models/ package, since the three criteria models that follow form one connected structure and want a module of their own. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> docs: Apply batched suggestions from code review Co-authored-by: Jesper Hodge <19345795+jesperhodge@users.noreply.github.com> feat: add CompetencyCriteriaGroup, the criteria tree's AND/OR node A single CompetencyAchievementCriteria is one root CompetencyCriteriaGroup plus all of its descendant groups, per ADR-0002 Decision 2. Two constraints a reader may expect are deliberately absent: nothing ties logic_operator to child count, and there is no UniqueConstraint on (parent, ordering). A child group cannot be saved until its parent's primary key exists, so a parent's clean() always sees zero children, and leaf criteria carry no ordering column at all, so sibling order among them would stay undefined while looking solved. Both belong to the authoring API. All three foreign keys cascade, per ADR-0002 Decision 7. A criteria tree means nothing without the competency it evaluates, the course run that scopes it, or the group above it, so on_delete here expresses containment rather than protection. Those same cascades are how a delete reaches the PROTECT on the learner status tables, which is where deletion is actually refused. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> test: move CompetencyCriteriaGroup's own delete tests down to this PR These five tests only need CompetencyCriteriaGroup, so they belong with the on_delete values this PR already declares, not in the cross-model part 8 suite they were drafted alongside. fix: Apply suggestion from @jesperhodge test: cover multi-level cascade, taxonomy delete, and no delete() override Issue openedx#641's AC asks for group deletion to cascade at any depth, for a taxonomy delete to reach this model's criteria groups transitively, and for no delete() override to land in this ticket. Only depth-1 cascade and the tag-level cascade were pinned; add the three gaps directly. fix: delete the root, not the middle node, in the depth-cascade test Deleting the middle node only re-proved the one-hop cascade the depth-1 test already covers. Deleting the root and checking the grandchild is what actually exercises the collector recursing through more than one level from a single delete. test: drop the no-delete()-override test Not valuable enough to keep. feat: add the CBE rule payload contract ADR-0002 Decision 3 stores an evaluation rule as a rule_type plus a JSON rule_payload whose shape that type defines, rather than as fixed op/value/scale columns, so a future rule type can add its own fields without a migration. The cost of JSON is that nothing enforces the shape, so this adds the validator the two criteria models will call from clean(). Grade is the only supported type. Its value is a fraction from 0.0 to 1.0, not a number out of 100, which is the mistake an author is most likely to make, so the out-of-range message names the convention rather than only rejecting the value. RuleType declares exactly the types that have a payload spec, so a type can never be offered as a choice without being saveable. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> refactor: type the CBE rule payload shape with a TypedDict GradePayload declares the stored shape of a Grade rule_payload, so that a site holding a validated payload can be annotated with it and mypy checks payload literals against the declared field names and types. The runtime validation is unchanged: a TypedDict value is a plain dict, so the JSONField round-trip and the seeded default row are unaffected. This also removes two duplications. The expected key set now comes from the TypedDict rather than a hand-written frozenset repeating the keys _validate_grade_payload reads, and the comparison operators are declared once as a Literal instead of again in a separate set. validate_rule_payload takes `object` rather than `Any`. Its argument is untrusted JSON out of a JSONField and cannot be narrowed at the signature, but `object` makes mypy enforce the isinstance guard that `Any` allowed a caller to skip unchecked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> refactor: improve rule payload validations fix: Apply suggestion from @jesperhodge feat: add CompetencyRuleProfile and seed the system default A reusable evaluation rule scoped to at most one of an organization, a course or a taxonomy, plus the one row scoped to none of them, which is the rule every criterion falls back to. A deployment that adds no profiles of its own gets an 80% threshold. See ADR-0002 Decision 3. Uniqueness per scope cannot be a plain constraint over the three nullable scope columns, because SQL never treats two NULLs as equal, and it cannot be a conditional constraint either, because MySQL has no partial unique indexes and Django silently skips creating one there. So the scope collapses into a derived scope_code column with one unconditional unique constraint. scope_code goes null while a profile is archived, which frees that scope for a replacement; the three scope columns are never cleared, so nothing is lost. Scope is immutable after creation, so criteria already resolved to a profile are never silently re-scoped. Deleting a scope owner takes the profile scoped to it, so course and competency_taxonomy cascade, per ADR-0002 Decision 7. organization does not: an Organization is not a competency definition record, and edx-organizations retires one by clearing its active flag rather than deleting the row, so PROTECT there refuses a delete that should not be happening. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> test: move CompetencyRuleProfile's own delete tests down to this PR These six tests only need CompetencyRuleProfile, so they belong with the on_delete values this PR already declares, not in the cross-model part 8 suite they were drafted alongside. fix: state the rule_payload shape in help_text instead of naming a function rule_payload's help_text told the reader to "see validate_rule_payload for the shape it must match". help_text renders in the Django admin and the DRF browsable API, so that is user-facing text pointing at an internal Python function the reader cannot open, which is what rule_payloads.py's own docstring forbids for the error messages beside it. It now states the shape. 0004 is amended in place because help_text is part of a field's deconstructed kwargs; makemigrations --check confirms no new migration is needed. Also cover the seeded system-default row against the payload contract. 0005_seed_default_rule_profile writes its payload as a literal and cannot check it: a historical migration must not import rule_payloads, and apps.get_model() returns a model without the custom clean(). Nothing else reconciled that literal with the validator, so tightening _validate_grade_payload would have left every deployment's default row invalid with no failing test. The new test is the one place the two meet. The neighbouring docstring credited the seeding to migration 0003, which is competencycriteriagroup. It is 0005. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> feat: add CompetencyCriterion, the criteria tree's leaf A leaf points at one ObjectTag, meaning one specific piece of tagged content, and takes its pass rule either from a shared CompetencyRuleProfile or from its own inline override pair. A check constraint enforces ADR-0002 Decision 4's invariant: never both, never neither. The stored rule_profile is not resolved at read time. Decision 4 assigns it at four named write events and stores the result, so a criterion that already resolved to a less specific profile is not silently re-governed when a more specific one appears later. Computing that assignment is authoring-API work and is not here. group and object_tag cascade, per ADR-0002 Decision 7: a leaf means nothing without the group above it or the content association it evaluates. rule_profile is RESTRICT rather than PROTECT. Both refuse a direct profile delete while any criterion is assigned to it, which is what makes a profile archive-only at the ORM layer. They differ once the profile is deleted as part of a larger operation: PROTECT raises for any referencing row it finds in the database, so deleting a CompetencyTaxonomy would fail naming a criterion the same operation was already about to remove, while RESTRICT ignores rows that are themselves being deleted and lets that cascade through. Refs openedx#641 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> test: move CompetencyCriterion's delete tests and the tree integration test down to this PR This model completes the criteria tree, so its own on_delete values, the transitive tag/taxonomy cascades that only exist once it does, the two RESTRICT-vs-PROTECT payoff/residual scenarios, and the tree-wide integration test all belong here, not in the cross-model part 8 suite they were drafted alongside.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 7, 2026

openedx-webhooks added this to Contributions Aug 7, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Aug 7, 2026

jesperhodge marked this pull request as ready for review August 7, 2026 15:54

jesperhodge requested review from kdmccormick, mgwozdz-unicon and ormsbee and removed request for kdmccormick and ormsbee August 7, 2026 15:55

kdmccormick requested changes Aug 7, 2026

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Aug 7, 2026

jesperhodge requested a review from kdmccormick August 12, 2026 16:14

mgwozdz-unicon approved these changes Aug 12, 2026

View reviewed changes

mgwozdz-unicon requested review from bradenmacdonald and ormsbee August 21, 2026 22:42

bradenmacdonald reviewed Aug 24, 2026

View reviewed changes

kdmccormick approved these changes Aug 26, 2026

View reviewed changes

jesperhodge force-pushed the jesperhodge/feat--640-cbe-app-foundation branch from 78c605f to 2ff6cb0 Compare August 27, 2026 17:08

jesperhodge force-pushed the jesperhodge/feat--640-cbe-app-foundation branch from 2ff6cb0 to d4b259d Compare August 27, 2026 17:23

ormsbee merged commit c0733f1 into openedx:main Aug 27, 2026
7 checks passed

github-project-automation Bot moved this from In Eng Review to Done in Contributions Aug 27, 2026

This was referenced Aug 31, 2026

Competency criteria models (authoring/definition layer) #641

Open

Mastery status lookup + learner progress models #642

Open

mgwozdz-unicon mentioned this pull request Sep 1, 2026

[BE] Implement CBE core data models (CompetencyTaxonomy, criteria, learner status) #613

Open

38 tasks

jesperhodge mentioned this pull request Sep 1, 2026

feat: add competency criteria models for CBE authoring layer #800

Draft

jesperhodge mentioned this pull request Sep 9, 2026

feat: add CompetencyTaxonomy.taxonomy_overrides_org jesperhodge/openedx-core#3

Open

Conversation

jesperhodge commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI-first work.

Related links

What this does

Depends on work in edx-platform

How to test

Uh oh!

openedx-webhooks commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Aug 7, 2026

Uh oh!

mgwozdz-unicon commented Aug 10, 2026

Uh oh!

jesperhodge commented Aug 12, 2026

Uh oh!

mgwozdz-unicon left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

jesperhodge commented Aug 25, 2026

Uh oh!

jesperhodge commented Aug 26, 2026

Uh oh!

ormsbee commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Aug 27, 2026

Uh oh!

jesperhodge commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jesperhodge commented Aug 7, 2026 •

edited

Loading

openedx-webhooks commented Aug 7, 2026 •

edited

Loading

ormsbee commented Aug 26, 2026 •

edited

Loading