feat: enforce competency-hierarchy dominance for criteria

[alezconsultant]

Description

Implements openedx#666: before a CompetencyCriterion is created, reject it with
a 400 if any taxonomy ancestor or descendant of its competency tag already has a
CompetencyCriterion in the same course. This is the containment check
create_competency_criterion() reserved a seam for in openedx#665. Siblings, and a repeat criterion
for the same tag in the same course, stay allowed, per the ticket's own acceptance criteria.

Branched off alezconsultant/665-criterion-endpoint (PR openedx#14 into
jesperhodge/feat--641-competency-criteria-models), since that's where the models and the
create-criterion endpoint this check attaches to actually live.

Changes

• src/openedx_tagging/api.py: adds get_ancestor_tags(tag) and get_descendant_tags(tag),
  both -> QuerySet[Tag], matching the ticket's own approved technical notes exactly (name,
  file, and signature). Mirrors Tag.descendant_count's lineage-prefix mechanism and its
  free-text-taxonomy guard.
• src/openedx_learning/applets/cbe/api.py: adds get_courses_with_relative_criteria(tag_ids)
  (public, also listed in __all__) and _validate_containment(group, course) (private).
  Wires the latter into create_competency_criterion() at its existing reserved insertion
  point, replacing the marker comment with a three-line addition (two assert calls narrowing
  group.parent's type, then the validation call).
• tests/openedx_tagging/test_api.py, tests/openedx_learning/applets/cbe/test_api.py,
  tests/openedx_learning/applets/cbe/test_views.py: unit tests for the two new tagging
  functions and for get_courses_with_relative_criteria/_validate_containment in isolation,
  plus one endpoint test per acceptance-criteria scenario in the ticket (all six), each with its
  own uniquely-prefixed tag family so scenarios can't interfere with each other. The three
  reject-scenario tests assert the response body actually names the conflicting tag and course,
  not just that an error key is present, per the ticket's own acceptance criteria.

Manual Testing

Due to a requirements mismatch and incorrect migrations, this section will be updated once the changes from openedx#800 are merged. Until then, rely on unit tests.

Related to openedx#666

🤖 Created with the help of Claude Code

[tbain]

Hello! A few comments;

1. Question/bug(?) about src/openedx_learning/applets/cbe/api.py::get_courses_with_relative_criteria. We have a test that asserts that we'll tie break to the lower 'id'/'tag_id' value if there's a conflict. Is this because we want the 'first' or 'oldest' value? While database IDs like this are sequentially generated, it makes me concerned to tie business logic to that (usually very valid) assumption, since it's not necessarily guaranteed to be an 'earlier' value, it simply happens to be that way. If we want to satisfy this sort of tie-breaking logic to whichever value came first, is there a 'create_date' or something similar we can use that is concretely tied to determining if it is sequentially earlier? e.g. Sort by a creation date rather than the id?

2. AI Possible Bug: The Codex review is worried about api.py:145-156, api.py:225-227 that we process this in accordance to the course id related to the criterion object, rather than comparing the actual in-database retrieved course object itself most up-to-date data about the course. The Codex analysis states that there's no DB constraint that ties the course to the criterion so it could be stale data (e.g. the course might have changed, been deleted, etc.). Admittedly this seems incorrect to me to think there isn't a DB Foreign Key constraint, so can you check for this, and if the FK constraint is not there, does it make sense to pull in the course so that we can de-conflict with the most accurate course data?

3. Bug — concurrent requests can bypass the invariant: api.py:168-175, api.py:229 Codex points out that this transaction to save the criterion to the DB is not necessarily atomic, so two transactions could run concurrently and theoretically insert criterion that are conflicted at the time of write but not at the time of checking. The AI suggests doing something like the following to address the issue, though I defer to human judgement:

Remediation Suggestion from Codex:

Details

Lock a shared CourseRun row before checking containment, and keep the lock until the criterion is inserted: with transaction.atomic(): course_run = CourseRun.objects.select_for_update().get(pk=course_run.pk)

# create/resolve the group
_validate_containment(group, course_run)
return CompetencyCriterion.objects.create(...)

Place the lock before _validate_containment()—ideally before group creation—so concurrent requests for the same course serialize. Requests for different courses remain concurrent. Ensure every criterion-creation path uses this transaction.

4. Bug: AI points out that api.py:209-215, api.py:225-229. tag_object() runs before containment validation. This seems problematic since tag_object() does commit changes that we wouldn't want to commit if the _validate_containment() logic fails? AI Suggests remediating this step by running it in an atomic transaction and then validate before tab_object() is called;

Remediation Suggestion from Codex:

Details

Run containment validation before tag_object() and wrap the entire operation in transaction.atomic(): with transaction.atomic(): _validate_containment(group, course)

# Only mutate tags after validation succeeds.
tag_object(...)
return CompetencyCriterion.objects.create(...)

This prevents rejected requests from leaving behind an ObjectTag, including when callers invoke create_competency_criterion() directly.

---

Raw Codex output:

Details

Request changes. I found three behavioral issues. - Bug — course scope is derived from group structure, not the criterion’s object: [api.py:145-156](https://github.com/alezconsultant/openedx-core/blob/723a8d52d944ff6924219f8f179110591f737e0a/src/openedx_learning/applets/cbe/api.py#L145-L156), [api.py:225-227](https://github.com/alezconsultant/openedx-core/blob/723a8d52d944ff6924219f8f179110591f737e0a/src/openedx_learning/applets/cbe/api.py#L225-L227). The ticket requires comparing UsageKey.course_key from each criterion’s object_tag.object_id; this implementation trusts group.parent.course. Since no database constraint ties those values together, stale/imported/direct API data can cause false accepts or rejects. - Bug — concurrent requests can bypass the invariant: [api.py:168-175](https://github.com/alezconsultant/openedx-core/blob/723a8d52d944ff6924219f8f179110591f737e0a/src/openedx_learning/applets/cbe/api.py#L168-L175), [api.py:229](https://github.com/alezconsultant/openedx-core/blob/723a8d52d944ff6924219f8f179110591f737e0a/src/openedx_learning/applets/cbe/api.py#L229). Two simultaneous ancestor/descendant creations can both observe no conflict and then insert successfully. transaction.atomic() alone does not serialize this read-then-write check, and no database constraint prevents the result. - Bug — rejected direct API calls leave tag mutations behind: [api.py:209-215](https://github.com/alezconsultant/openedx-core/blob/723a8d52d944ff6924219f8f179110591f737e0a/src/openedx_learning/applets/cbe/api.py#L209-L215), [api.py:225-229](https://github.com/alezconsultant/openedx-core/blob/723a8d52d944ff6924219f8f179110591f737e0a/src/openedx_learning/applets/cbe/api.py#L225-L229). tag_object() runs before containment validation. The endpoint’s outer transaction rolls this back, but the exported create_competency_criterion() function is not itself atomic, so direct callers can receive a validation error while the ObjectTag persists.

[alezconsultant]

@tbain Thanks for feedback.

1. Right now timestamp field exists on Tag, and "lowest id" is the ticket's suggestion for get_courses_with_relative_criteria , chosen for determinism, not chronological accuracy, it doesn't affects the accept/reject decision.
2. CompetencyCriteriaGroup.course has an FK, but add in create_competency_criterion() check that rejects a mismatch between criterion and course
3. fixed with your suggested approach select_for_update() on the target CourseRun before the containment check.
4. fixed validation now runs before tag_object() both wrapped in one transaction.atomic().

[tbain]

I think this looks good to me, and Codex agrees, for the most part. I think it's chasing an erroneous issue with the parent_d checking (the #2 issue noted). I feel like if that is a proper FK relationship up to the parent course, then that means that the concern the AI has (as far as I can determine) is erroneous, since the FK relationship would enforce a valid relationship up to a valid parent course.

Approved!