Alezconsultant/665 criterion endpoint on 846

[alezconsultant]

Description

Stacked on openedx#846 ; review only the last commit

Adds the create-criterion endpoint from openedx#665: POST /api/cbe/v1/competencies/<int:tag_id>/criteria/, letting a course author associate a
gradeable subsection with a competency, either by attaching it into an existing leaf
CompetencyCriteriaGroup or by deriving/creating whatever part of the root/course-level/leaf
group hierarchy doesn't yet exist for this competency and course.

Changes

• src/openedx_learning/migrations/0004_criteria_group_unique_constraints.py (new): two
  partial UniqueConstraints on CompetencyCriteriaGroup — one root per tag
  (condition=Q(parent__isnull=True)), one course-level group per (tag, course, parent)
  (condition=Q(course__isnull=False)). Matching constraints added to
  CompetencyCriteriaGroup.Meta in models/criteria.py.
• src/openedx_learning/applets/cbe/api.py: adds resolve_competency_tag(),
  resolve_or_create_leaf_group(), resolve_supplied_leaf_group(),
  create_competency_criterion(), and associate_competency_criterion() (the single public
  entry point this ticket's view calls).
• src/openedx_learning/applets/cbe/rest_api/v1/serializers.py: adds
  CompetencyCriterionSerializer alongside the existing CompetencyRuleProfileSerializer.
  object_id/group_id/logic_operator are plain write-only fields (none of them are
  CompetencyCriterion model fields); competency_rule_profile_id,
  competency_criteria_group_id, and oel_tagging_objecttag_id are the ticket's contracted
  JSON names, mapped via explicit source= to the model's actual attributes
  (rule_profile_id, group_id, object_tag_id).
• src/openedx_learning/applets/cbe/rest_api/v1/views.py: adds
  CompetencyCriterionCreateView(generics.CreateAPIView) alongside the existing
  CompetencyRuleProfileView.
• src/openedx_learning/applets/cbe/rest_api/v1/urls.py: adds
  competencies/<int:tag_id>/criteria/, a plain path() (not a second router
  registration — the URL carries a path parameter mid-route, so it isn't a flat
  router-friendly resource like rule_profiles).
• tests/openedx_learning/applets/cbe/test_api.py, test_views.py,
  test_criteria_models.py: unit and DRF integration tests (see Tests below); one
  pre-existing test in test_criteria_models.py updated to use two tags instead of one,
  since it previously created two root groups for the same tag, which the new constraint
  now correctly rejects.

Tests

One test per scenario in openedx#665's Acceptance Criteria (all 20): the full-hierarchy derivation
and reuse scenarios, logic_operator provided/defaulted, the group_id/logic_operator
mutual-exclusion rejection, every supplied-group validation rejection (non-leaf, wrong
competency, wrong course), all three duplicate-association rejections, the all-null-rule-
fields deviation above, the same-taxonomy tag-preservation scenario, every not-found/malformed
scenario, and the permission rejection. Plus unit tests for resolve_or_create_leaf_group()
and resolve_supplied_leaf_group() directly, including two tests that pre-commit a
competing row out-of-band to prove the constraint-backed get_or_create() fallback (standing
in for a real concurrent request), two tests proving the constraints themselves exist via a
direct IntegrityError, and one test proving the atomic block rolls back newly-created groups
when the final CompetencyCriterion.objects.create() call fails.

Depends on

openedx-core#9 (openedx#773), which this branch is based on and which already
adds the CBE REST scaffolding (openedx_learning/urls.py, the rest_api/ package,
serializers.py/views.py/urls.py) this PR extends rather than creates. That PR is itself
based on openedx#641/openedx#613's CompetencyCriteriaGroup/CompetencyCriterion/CompetencyRuleProfile
models.

Companion PR

A one-line openedx-platform change (cms/urls.py: path('api/cbe/', include('openedx_learning.urls')), mirroring the existing content_tagging mount) is a
separate PR against that repo, not included here.

Verification

• pytest tests/openedx_learning/applets/cbe/ --no-cov: 147 passed.
• pylint, mypy, pycodestyle, isort --check-only, pydocstyle: all clean on every
  changed file.
• lint-imports: 2 contracts kept, 0 broken.
• The underlying make pii_check check: 2 pre-existing, unrelated failures only
  (openedx_content.Draft/PublishableEntityVersion, confirmed via git stash to predate
  this branch). Both models this PR touches already carry .. no_pii:; the migration adds no
  new fields.
• Also verified against a live devstack running the real openedx-platform permission chain
  (a real CourseStaffRole holder granted access, an unpermitted user refused), isolated from
  an unrelated, pre-existing import failure elsewhere in that repo's content_tagging app —
  see the companion PR for details.

Related to openedx#665

Created with the help of Claude

[tbain]

I didn't see anything in my human review, but Codex points out a few questions/bugs/concerns

1. api.py:196is defaulting to a system-wide default profile rather than one that has tighter scope which is what the AC asks for. However, I recall from another review that the other scopes have not been implemented? If we need to adjust this to match the ticket AND the other profiles do exist, please fix, and if not can you just confirm that that hasn't been implemented yet, or other technical reason why this isn't done?

2. serializers.py:81 returns object_tag_id, but the current ticket’s response contract requires oel_tagging_objecttag_id. Clients following the ticket contract will not find the required field. I seem to recall we had this on another ticket and this is fine. I'm tempted to ignore this one, I think your implementation is correct, but can you confirm?

3. Codex is concerned, and it makes sense, that the duplicate check at api.py:253 happens outside the atomic lock on line api.py:273. This seems like it might be a valid concern (albeit very niche); we can do a duplicate check, have it find no duplicates, and then between the time of the check and the actual atomic transaction another process could insert what would have been a duplicate and cause a conflict. Do we need to move the duplicate check into the atomic transaction lock?

---

Raw Codex output:

Details

Fresh review completed from the live PR and issue state; all earlier findings were discarded.

Findings:

1. [P1] Taxonomy-scoped rule profiles are ignored
   [api.py:196](https://github.com/javoconsultant/openedx-core/pull/1/files) always selects the system-default profile when rule fields are omitted. The current issue requires using the taxonomy-scoped profile when one exists, falling back to the system default only otherwise.

2. [P1] Response uses the wrong object-tag field name
   [serializers.py:81](https://github.com/javoconsultant/openedx-core/pull/1/files) returns object_tag_id, but the current ticket’s response contract requires oel_tagging_objecttag_id. Clients following the ticket contract will not find the required field.

3. [P2] Duplicate criteria can be created concurrently
   [api.py:253](https://github.com/javoconsultant/openedx-core/pull/1/files) performs duplicate detection outside the transaction, and [api.py:273](https://github.com/javoconsultant/openedx-core/pull/1/files) does not re-check it under lock. Concurrent requests can both pass validation and create duplicate criteria for the same object/tag/group combination. No uniqueness constraint protects this association.

Validation:

• Live issue fetched at 2026-09-30T15:51:10Z.
• Live PR head: a1f37191d116ce1bbab6c3aaf1a0b757262a12a6.
• git diff --check: passed.
• Python compilation: passed.
• Targeted pytest collection was blocked by the environment missing openedx_events; no test results are claimed.

[alezconsultant]

@tbain

1. For now ADR and ticket body
   MVP note: only the system-default profile exists in this phase; taxonomy-, course-, and organization-scoped profiles are all out of scope for this MVP, so every profile row in this phase has competency_taxonomy_id, course_id, and organization_id null. If/when taxonomy-, course-, or organization-scoped competency rule profiles are built, no schema change is needed here to support them -- the columns, the uniqueness constraint, and the re-assignment behavior (Decision 4) already do.

rule_profile_id, rule_type_override, and rule_payload_override may all be omitted together in the same request: that is a valid, complete request, not an incomplete one. The created CompetencyCriterion row is persisted with all three fields null, and that all-null state is itself the signal that the system default competency rule profile applies. This ticket does not resolve or store a concrete default profile at creation time; it only accepts and persists the all-null state as meaningful.
contradict AC
jesperhodge#14 (review) ( item 6 from this PR)
Scenario: A taxonomy-scoped rule profile is assigned ahead of the system default
2. jesperhodge#14 (review) approved by 3 item in this PR
3. Thanks! Fixed

[tbain]

Approved - I found no new issues and neither did Codex. Thanks for addressing that issue!