From 7033c4856563903949b68d9dbe76e62b1592b1b5 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Wed, 28 Oct 2026 14:51:41 +0400 Subject: [PATCH] feat: enforce competency-hierarchy dominance for criteria Reject a CompetencyCriterion at creation time if any taxonomy ancestor or descendant of its competency tag already has a live CompetencyCriterion in the same course. Siblings, and a repeat criterion for the same tag in the same course, stay allowed. Archived criteria, and criteria under an archived leaf, course-level, or root group, do not count, since ADR 0002 hides archived rows from authoring and new associations. Add to openedx_tagging/api.py: get_ancestor_tags(tag) and get_descendant_tags(tag), both returning a QuerySet[Tag], mirroring Tag.descendant_count's own lineage-prefix mechanism and its free-text-taxonomy guard. Add to applets/cbe/api.py: get_courses_with_relative_criteria(tag_ids, course=None), a public function mapping each course already blocked by one of the given tags to the lowest-id conflicting tag, narrowed to one course when course is given. It only counts criteria on a leaf group, whose parent carries the course, so a criterion misplaced on another group is skipped rather than failing every create for the tag. Add _validate_containment(group, course), called from create_competency_criterion() before the object is tagged. create_competency_criterion() takes the caller's course_run instead of re-parsing object_id and fetching the course run a second time, and its rule fields are keyword-only. It runs inside the transaction and locks that associate_competency_criterion() holds. Add CompetencyCourseRun, a CBE-owned row whose primary key is a OneToOneField to the catalog's CourseRun (migration 0016). associate_competency_criterion() creates it with get_or_create() on a course's first criterion and locks it with select_for_update() right after the Tag lock, so two concurrent creates in the same course cannot both pass the containment check. Locking this row instead of CourseRun keeps the lock from blocking other apps' inserts that reference the course run, and keeps openedx_learning from querying catalog tables directly. Related to openedx/openedx-core#666 Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Opus 5.5 Co-Authored-By: Claude Fable 5.1 --- src/openedx_learning/applets/cbe/api.py | 75 ++++- .../applets/cbe/models/__init__.py | 2 + .../applets/cbe/models/course_run.py | 32 +++ .../0016_add_competency_course_run.py | 21 ++ src/openedx_tagging/api.py | 24 ++ .../openedx_learning/applets/cbe/test_api.py | 264 +++++++++++++++++- .../applets/cbe/test_views.py | 136 +++++++++ tests/openedx_tagging/test_api.py | 50 ++++ 8 files changed, 599 insertions(+), 5 deletions(-) create mode 100644 src/openedx_learning/applets/cbe/models/course_run.py create mode 100644 src/openedx_learning/migrations/0016_add_competency_course_run.py diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 87552fee7..abe29ebf8 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -7,7 +7,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from decimal import Decimal -from typing import Any, Sequence +from typing import Any, Iterable, Sequence from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import connection, transaction @@ -19,12 +19,13 @@ from openedx_catalog.api import get_course_run from openedx_catalog.models_api import CourseRun -from openedx_tagging.api import create_taxonomy, get_object_tags, tag_object +from openedx_tagging.api import create_taxonomy, get_ancestor_tags, get_descendant_tags, get_object_tags, tag_object from openedx_tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.rules import ObjectTagPermissionItem, UserType from .data import GradedObjectScore from .models import ( + CompetencyCourseRun, CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, @@ -45,6 +46,7 @@ "create_competency_taxonomy", "get_competency_criteria_tree", "get_competency_rule_profiles", + "get_courses_with_relative_criteria", "is_competency_taxonomy", "record_graded_object_statuses", "resolve_competency_tag", @@ -260,6 +262,58 @@ def resolve_supplied_leaf_group(group_id: int, tag: Tag, course_run: CourseRun) return group +def get_courses_with_relative_criteria( + tag_ids: Iterable[int], course: CourseRun | None = None, +) -> dict[CourseRun, Tag]: + """ + Return every course already blocked for one of ``tag_ids`` by an existing CompetencyCriterion. + + Maps each conflicting CourseRun to the lowest-id tag (of ``tag_ids``) with a criterion there, + narrowed to ``course`` when given. Archived criteria and criteria under an archived group are ignored. + """ + groups: QuerySet[CompetencyCriteriaGroup, CompetencyCriteriaGroup] = CompetencyCriteriaGroup.objects.filter( + tag_id__in=tag_ids, + archived=False, + parent__archived=False, + parent__parent__archived=False, + # Only a leaf's parent carries a course, so this skips criteria misplaced on other groups. + parent__course__isnull=False, + criteria__archived=False, + ) + if course is not None: + groups = groups.filter(parent__course=course) + result: dict[CourseRun, Tag] = {} + for group in groups.select_related("tag", "parent__course").distinct().order_by("tag_id"): + assert group.parent is not None, f"CompetencyCriteriaGroup {group.id} has no parent" + assert group.parent.course is not None, f"CompetencyCriteriaGroup {group.id}'s parent has no course" + result.setdefault(group.parent.course, group.tag) + return result + + +def _validate_containment(group: CompetencyCriteriaGroup, course: CourseRun) -> None: + """ + Raise ValidationError (keyed "tag_id") if ``group.tag`` conflicts with an existing criterion. + + A conflict is a taxonomy ancestor or descendant of ``group.tag`` that already has a + CompetencyCriterion in ``course``. + """ + tag = group.tag + relative_ids = ( + set(get_ancestor_tags(tag).values_list("id", flat=True)) + | set(get_descendant_tags(tag).values_list("id", flat=True)) + ) + if not relative_ids: + return + conflicting_tag = get_courses_with_relative_criteria(relative_ids, course=course).get(course) + if conflicting_tag is not None: + raise ValidationError({ + "tag_id": _( + "Tag '{tag_value}' already has a criterion in course {course_key}, and is a taxonomy " + "ancestor or descendant of this criterion's tag." + ).format(tag_value=conflicting_tag.value, course_key=course.course_key), + }) + + def _get_applicable_rule_profile(taxonomy_id: int) -> CompetencyRuleProfile | None: """ Return the live profile ADR-0002 Decision 4 assigns a criterion in ``taxonomy_id``, or None if there is none. @@ -281,6 +335,8 @@ def _get_applicable_rule_profile(taxonomy_id: int) -> CompetencyRuleProfile | No def create_competency_criterion( group: CompetencyCriteriaGroup, object_id: str, + course_run: CourseRun, + *, rule_profile_id: int | None = None, rule_type_override: str | None = None, rule_payload_override: dict | None = None, @@ -300,10 +356,16 @@ def create_competency_criterion( else the live system default, rather than persisting null: ADR-0002 Decision 4 always resolves to a concrete profile, and the model's own xor-override constraint forbids leaving all three fields null. + + ``course_run`` is ``group``'s course. Must run inside a transaction holding the tag and + course run locks that associate_competency_criterion() takes, so the dominance check below + can't race another create. """ tag = group.tag # tag.taxonomy_id is nullable at the model level; already guaranteed set by the caller. assert tag.taxonomy_id is not None + _validate_containment(group, course_run) + existing_values = [existing_tag.value for existing_tag in get_object_tags(object_id, taxonomy_id=tag.taxonomy_id)] if tag.value not in existing_values: existing_values.append(tag.value) @@ -319,8 +381,6 @@ def create_competency_criterion( raise CompetencyRuleProfile.DoesNotExist("No live rule profile applies to this criterion.") rule_profile_id = profile.id - # Parent-competency dominance/containment validation belongs here, before creation. - return CompetencyCriterion.objects.create( group=group, object_tag=object_tag, @@ -433,8 +493,14 @@ def associate_competency_criterion( _validate_rule_fields(rule_profile_id, rule_type_override, rule_payload_override) with transaction.atomic(): + # Lock order is always the tag, then the course run, so two creates can't deadlock. # Serializes the duplicate check and root creation; MySQL has no partial unique index for one root per tag. Tag.objects.select_for_update().get(pk=tag.pk) + # Serializes the per-course dominance check on CBE's own row, not the catalog's CourseRun. + # Created first, since a locking read of a row that doesn't exist yet locks nothing; get_or_create() + # resolves two first creates racing on the primary key. + CompetencyCourseRun.objects.get_or_create(course_run=course_run) + CompetencyCourseRun.objects.select_for_update().get(pk=course_run.pk) _reject_duplicate_criterion(tag, object_id, group_id) if group_id is not None: group = resolve_supplied_leaf_group(group_id, tag, course_run) @@ -443,6 +509,7 @@ def associate_competency_criterion( return create_competency_criterion( group=group, object_id=object_id, + course_run=course_run, rule_profile_id=rule_profile_id, rule_type_override=rule_type_override, rule_payload_override=rule_payload_override, diff --git a/src/openedx_learning/applets/cbe/models/__init__.py b/src/openedx_learning/applets/cbe/models/__init__.py index 8b525773c..58d76ddf6 100644 --- a/src/openedx_learning/applets/cbe/models/__init__.py +++ b/src/openedx_learning/applets/cbe/models/__init__.py @@ -4,6 +4,7 @@ from ..rule_payloads import RuleType from .competency_taxonomy import CompetencyTaxonomy +from .course_run import CompetencyCourseRun from .criteria import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator from .learner_status import ( CompetencyMasteryStatus, @@ -14,6 +15,7 @@ ) __all__ = [ + "CompetencyCourseRun", "CompetencyCriteriaGroup", "CompetencyCriterion", "CompetencyMasteryStatus", diff --git a/src/openedx_learning/applets/cbe/models/course_run.py b/src/openedx_learning/applets/cbe/models/course_run.py new file mode 100644 index 000000000..0f85f533f --- /dev/null +++ b/src/openedx_learning/applets/cbe/models/course_run.py @@ -0,0 +1,32 @@ +""" +CompetencyCourseRun: CBE's own per-course row for a catalog CourseRun. +""" +from __future__ import annotations + +from django.db import models +from django.utils.translation import gettext_lazy as _ + +from openedx_catalog.models_api import CourseRun + +__all__ = [ + "CompetencyCourseRun", +] + + +class CompetencyCourseRun(models.Model): + """ + A course run that has competency criteria, created with its first criterion. + + Criterion creation locks this row, rather than the catalog's CourseRun, to serialize the + per-course dominance check without blocking other apps' writes that reference the course run. + + .. no_pii: + """ + + course_run = models.OneToOneField( + CourseRun, + primary_key=True, + on_delete=models.CASCADE, + related_name="competency_course_run", + help_text=_("The catalog course run this row extends."), + ) diff --git a/src/openedx_learning/migrations/0016_add_competency_course_run.py b/src/openedx_learning/migrations/0016_add_competency_course_run.py new file mode 100644 index 000000000..86630d0cf --- /dev/null +++ b/src/openedx_learning/migrations/0016_add_competency_course_run.py @@ -0,0 +1,21 @@ +# Generated by Django 5.2.17 on 2026-10-20 12:42 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_catalog', '0001_initial'), + ('openedx_learning', '0015_criteria_group_unique_constraints'), + ] + + operations = [ + migrations.CreateModel( + name='CompetencyCourseRun', + fields=[ + ('course_run', models.OneToOneField(help_text='The catalog course run this row extends.', on_delete=django.db.models.deletion.CASCADE, primary_key=True, related_name='competency_course_run', serialize=False, to='openedx_catalog.courserun')), + ], + ), + ] diff --git a/src/openedx_tagging/api.py b/src/openedx_tagging/api.py index 005600d63..8d2f90353 100644 --- a/src/openedx_tagging/api.py +++ b/src/openedx_tagging/api.py @@ -169,6 +169,30 @@ def get_children_tags( return taxonomy.get_filtered_tags(parent_tag_value=parent_tag_value, depth=1) +def get_ancestor_tags(tag: Tag) -> QuerySet[Tag]: + """ + Return every ancestor of ``tag`` (its parent, grandparent, ... up to the root). + + Returns an empty queryset if ``tag``'s taxonomy is unset or allows free text, since + free-text tags have no taxonomy-defined hierarchy. + """ + if tag.taxonomy is None or tag.taxonomy.allow_free_text: + return Tag.objects.none() + return Tag.objects.filter(taxonomy=tag.taxonomy, value__in=tag.get_lineage()[:-1]) + + +def get_descendant_tags(tag: Tag) -> QuerySet[Tag]: + """ + Return every descendant of ``tag`` at any depth, excluding ``tag`` itself. + + Returns an empty queryset if ``tag``'s taxonomy is unset or allows free text, since + free-text tags have no taxonomy-defined hierarchy. + """ + if tag.taxonomy is None or tag.taxonomy.allow_free_text: + return Tag.objects.none() + return Tag.objects.filter(taxonomy=tag.taxonomy, depth__gt=tag.depth, lineage__startswith=tag.lineage) + + def resync_object_tags(object_tags: QuerySet | None = None) -> int: """ Reconciles ObjectTag entries with any changes made to their associated taxonomies and tags. diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index d1ef776b9..f4dbcbd68 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -23,6 +23,7 @@ create_competency_taxonomy, get_competency_criteria_tree, get_competency_rule_profiles, + get_courses_with_relative_criteria, is_competency_taxonomy, resolve_supplied_leaf_group, select_competency_taxonomies, @@ -30,6 +31,7 @@ from openedx_learning.applets.cbe import api as cbe_api from openedx_learning.applets.cbe.api import _create_leaf_group from openedx_learning.models import ( + CompetencyCourseRun, CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, @@ -105,6 +107,22 @@ def usage_key(course_run: CourseRun, block_id: str) -> str: return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@{block_id}" +def make_tag(taxonomy: CompetencyTaxonomy, value: str, parent: Tag | None = None) -> Tag: + """Create a Tag on `taxonomy`, optionally under `parent`, for taxonomy-hierarchy tests.""" + return Tag.objects.create(taxonomy=taxonomy, value=value, parent=parent) + + +def make_leaf_criterion( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, block_id: str, +) -> CompetencyCriterion: + """ + Create a leaf group and one live CompetencyCriterion for `tag` in `course_run`. + """ + leaf = _create_leaf_group(tag, course_run) + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, block_id), taxonomy=tag.taxonomy, tag=tag) + return CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) + + def test_is_competency_taxonomy() -> None: """ is_competency_taxonomy() is True for a competency taxonomy, False for a plain one. @@ -698,7 +716,7 @@ def test_bulk_update_values_return_a_criterion_to_the_taxonomy_profile_not_the_d rule_type=RuleType.GRADE, rule_payload=NEW_PAYLOAD, competency_taxonomy=competency_taxonomy, ) assert leaf.parent is not None and leaf.parent.course is not None - criterion = create_competency_criterion(leaf, usage_key(leaf.parent.course, "p1")) + criterion = create_competency_criterion(leaf, usage_key(leaf.parent.course, "p1"), course_run=leaf.parent.course) assert rule_state(criterion) == (taxonomy_profile.id, None, None) bulk_update_competency_criteria([criterion.id], leaf.id, user=user, **SEEDED_RULE) @@ -1295,3 +1313,247 @@ def test_get_competency_criteria_tree_costs_a_bounded_number_of_queries( _ = group.course # accessing the select_related'd relation must not add a query for criterion in tree.criteria: _ = criterion.object_tag + + +def test_get_courses_with_relative_criteria_single_match( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A single tag with a live criterion in a course is returned, mapped to that tag.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + + assert get_courses_with_relative_criteria([relative.id]) == {course_run: relative} + + +def test_get_courses_with_relative_criteria_ties_break_on_the_lowest_tag_id( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """When two of the given tags both have a live criterion in the same course, the lower id tag wins.""" + first = make_tag(competency_taxonomy, "First Relative") + second = make_tag(competency_taxonomy, "Second Relative") + assert first.id < second.id + make_leaf_criterion(first, course_run, default_rule_profile, "p1") + make_leaf_criterion(second, course_run, default_rule_profile, "p2") + + result = get_courses_with_relative_criteria([second.id, first.id]) + + assert result == {course_run: first} + + +def test_get_courses_with_relative_criteria_empty_input_yields_an_empty_result() -> None: + """An empty tag_ids collection yields an empty result, not an error.""" + assert not get_courses_with_relative_criteria([]) + + +def test_get_courses_with_relative_criteria_excludes_a_group_with_no_live_criterion( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """A CompetencyCriteriaGroup for a relative tag with no attached criterion is not a conflict.""" + relative = make_tag(competency_taxonomy, "Empty Relative") + _create_leaf_group(relative, course_run) + + assert not get_courses_with_relative_criteria([relative.id]) + + +def test_get_courses_with_relative_criteria_ignores_an_archived_criterion( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """An archived criterion is gone for authoring, so it blocks nothing.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + criterion = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + CompetencyCriterion.objects.filter(pk=criterion.pk).update(archived=True) + + assert not get_courses_with_relative_criteria([relative.id]) + + +def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archived_leaf_group( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live criterion under an archived leaf group blocks nothing.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + criterion = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + CompetencyCriteriaGroup.objects.filter(pk=criterion.group_id).update(archived=True) + + assert not get_courses_with_relative_criteria([relative.id]) + + +def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archived_course_level_group( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live criterion under an archived course-level group blocks nothing.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + criterion = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + course_level_id = criterion.group.parent_id + assert course_level_id is not None + CompetencyCriteriaGroup.objects.filter(pk=course_level_id).update(archived=True) + + assert not get_courses_with_relative_criteria([relative.id]) + + +def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archived_root_group( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live criterion under an archived root group blocks nothing.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + criterion = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + course_level = criterion.group.parent + assert course_level is not None + root_id = course_level.parent_id + assert root_id is not None + CompetencyCriteriaGroup.objects.filter(pk=root_id).update(archived=True) + + assert not get_courses_with_relative_criteria([relative.id]) + + +def test_get_courses_with_relative_criteria_counts_a_course_with_one_live_and_one_archived_criterion( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Archiving one of a tag's two criteria in a course leaves the course blocked by the other.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + archived = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + make_leaf_criterion(relative, course_run, default_rule_profile, "p2") + CompetencyCriterion.objects.filter(pk=archived.pk).update(archived=True) + + assert get_courses_with_relative_criteria([relative.id]) == {course_run: relative} + + +def test_validate_containment_rejects_an_ancestor_conflict( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live criterion on an ancestor of group.tag, in the same course, is rejected.""" + parent = make_tag(competency_taxonomy, "Ancestor Skill") + child = make_tag(competency_taxonomy, "Descendant Skill", parent=parent) + make_leaf_criterion(parent, course_run, default_rule_profile, "p1") + group = _create_leaf_group(child, course_run) + + with pytest.raises(ValidationError, match="tag_id"): + cbe_api._validate_containment(group, course_run) # pylint: disable=protected-access + + +def test_validate_containment_rejects_a_descendant_conflict( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live criterion on a descendant of group.tag, in the same course, is rejected.""" + parent = make_tag(competency_taxonomy, "Ancestor Skill 2") + child = make_tag(competency_taxonomy, "Descendant Skill 2", parent=parent) + make_leaf_criterion(child, course_run, default_rule_profile, "p1") + group = _create_leaf_group(parent, course_run) + + with pytest.raises(ValidationError, match="tag_id"): + cbe_api._validate_containment(group, course_run) # pylint: disable=protected-access + + +def test_validate_containment_allows_sibling_tags( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Two tags sharing a parent, neither one the other's ancestor, do not conflict.""" + parent = make_tag(competency_taxonomy, "Shared Parent") + sibling_a = make_tag(competency_taxonomy, "Sibling A", parent=parent) + sibling_b = make_tag(competency_taxonomy, "Sibling B", parent=parent) + make_leaf_criterion(sibling_a, course_run, default_rule_profile, "p1") + group = _create_leaf_group(sibling_b, course_run) + + cbe_api._validate_containment(group, course_run) # pylint: disable=protected-access # Must not raise. + + +def test_validate_containment_allows_the_same_tag_in_a_different_course( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A live criterion for the same tag, scoped to a different course, does not block this course.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + make_leaf_criterion(tag, other_course_run, default_rule_profile, "p1") + group = _create_leaf_group(tag, course_run) + + cbe_api._validate_containment(group, course_run) # pylint: disable=protected-access # Must not raise. + + +def test_validate_containment_allows_a_tag_with_no_relatives(tag: Tag, course_run: CourseRun) -> None: + """A tag with neither ancestors nor descendants has nothing to conflict with.""" + group = _create_leaf_group(tag, course_run) + + cbe_api._validate_containment(group, course_run) # pylint: disable=protected-access # Must not raise. + + +def test_get_courses_with_relative_criteria_narrows_to_the_given_course( + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """With ``course`` given, only that course's conflict comes back, though another course has one too.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + other_course_run = make_course_run(organization, "Python200", "Fall2026") + make_leaf_criterion(relative, course_run, default_rule_profile, "p1") + make_leaf_criterion(relative, other_course_run, default_rule_profile, "p1") + + assert get_courses_with_relative_criteria([relative.id], course=course_run) == {course_run: relative} + + +def test_get_courses_with_relative_criteria_ignores_a_criterion_outside_a_leaf( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A criterion on a group nested below a leaf, e.g. made through the admin, is skipped rather than crashing.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + leaf = _create_leaf_group(relative, course_run) + nested = CompetencyCriteriaGroup.objects.create(tag=relative, parent=leaf) + object_tag = ObjectTag.objects.create( + object_id=usage_key(course_run, "p1"), taxonomy=relative.taxonomy, tag=relative, + ) + CompetencyCriterion.objects.create(group=nested, object_tag=object_tag, rule_profile=default_rule_profile) + + assert not get_courses_with_relative_criteria([relative.id]) + + +def test_create_competency_criterion_uses_the_callers_course_run( + tag: Tag, course_run: CourseRun, monkeypatch: pytest.MonkeyPatch, +) -> None: + """The caller already resolved the course run, so creating the criterion doesn't look it up again.""" + def _fail(*args, **kwargs): + raise AssertionError("get_course_run() called again") + monkeypatch.setattr(cbe_api, "get_course_run", _fail) + group = _create_leaf_group(tag, course_run) + + criterion = cbe_api.create_competency_criterion(group, usage_key(course_run, "p1"), course_run=course_run) + + assert criterion.group_id == group.id + + +def test_associate_competency_criterion_creates_the_course_runs_cbe_row( + tag: Tag, course_run: CourseRun, +) -> None: + """A course's first criterion creates its CompetencyCourseRun row.""" + associate_competency_criterion(tag.id, usage_key(course_run, "p1")) + + assert CompetencyCourseRun.objects.filter(course_run=course_run).exists() + + +def test_associate_competency_criterion_reuses_the_course_runs_cbe_row( + tag: Tag, course_run: CourseRun, +) -> None: + """Later criteria in the same course reuse the one CompetencyCourseRun row.""" + associate_competency_criterion(tag.id, usage_key(course_run, "p1")) + associate_competency_criterion(tag.id, usage_key(course_run, "p2")) + + assert CompetencyCourseRun.objects.filter(course_run=course_run).count() == 1 + + +def test_associate_competency_criterion_does_not_lock_the_catalog_course_run( + tag: Tag, course_run: CourseRun, monkeypatch: pytest.MonkeyPatch, +) -> None: + """The per-course lock is on CBE's own row, never on the catalog's CourseRun.""" + def _fail(*args, **kwargs): + raise AssertionError("CourseRun row locked") + monkeypatch.setattr(CourseRun.objects, "select_for_update", _fail) + + associate_competency_criterion(tag.id, usage_key(course_run, "p1")) + + +def test_competency_course_run_is_deleted_with_its_course_run( + tag: Tag, course_run: CourseRun, +) -> None: + """Deleting a course run deletes its CompetencyCourseRun row.""" + associate_competency_criterion(tag.id, usage_key(course_run, "p1")) + + course_run.delete() + + assert not CompetencyCourseRun.objects.exists() diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 0320b1fea..983d252ae 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -85,6 +85,11 @@ def make_course_run(organization: Organization, course_code: str, run_code: str) return CourseRun.objects.create(catalog_course=catalog_course, run_code=run_code) +def make_tag(taxonomy: CompetencyTaxonomy, value: str, parent: Tag | None = None) -> Tag: + """Create a Tag on `taxonomy`, optionally under `parent`, for taxonomy-hierarchy tests.""" + return Tag.objects.create(taxonomy=taxonomy, value=value, parent=parent) + + # What migration 0005 seeds the system default with. Asserted verbatim rather than imported, so # that a change to the seed surfaces here as a failing contract instead of passing silently. SEEDED_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} @@ -1344,3 +1349,134 @@ def test_view_checks_the_view_taxonomy_rule_not_view_tag(user_client: APIClient, rules.set_perm(view_taxonomy, original_view_taxonomy) assert response.status_code == status.HTTP_403_FORBIDDEN + + +def test_descendant_is_rejected_when_its_ancestor_already_has_a_criterion_in_the_same_course( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """A tag is rejected when its taxonomy ancestor already has a criterion in the same course.""" + parent = make_tag(competency_taxonomy, "S1 Parent") + child = make_tag(competency_taxonomy, "S1 Child", parent=parent) + first = user_client.post( + criterion_create_url(parent.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + assert first.status_code == status.HTTP_201_CREATED + + response = user_client.post( + criterion_create_url(child.id), {"object_id": usage_key(course_run, "p2")}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert parent.value in str(response.data["tag_id"]) + assert str(course_run.course_key) in str(response.data["tag_id"]) + + +def test_ancestor_is_rejected_when_a_descendant_already_has_a_criterion_in_the_same_course( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """A tag is rejected when one of its taxonomy descendants already has a criterion in the same course.""" + parent = make_tag(competency_taxonomy, "S2 Parent") + child = make_tag(competency_taxonomy, "S2 Child", parent=parent) + first = user_client.post( + criterion_create_url(child.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + assert first.status_code == status.HTTP_201_CREATED + + response = user_client.post( + criterion_create_url(parent.id), {"object_id": usage_key(course_run, "p2")}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert child.value in str(response.data["tag_id"]) + assert str(course_run.course_key) in str(response.data["tag_id"]) + + +def test_multi_level_ancestor_conflict_names_the_actual_conflicting_tag( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """ + A conflict several levels up the taxonomy is rejected, and the response names the real + conflicting tag (the grandparent), not just any ancestor. + """ + grandparent = make_tag(competency_taxonomy, "S3 Grandparent") + parent = make_tag(competency_taxonomy, "S3 Parent", parent=grandparent) + child = make_tag(competency_taxonomy, "S3 Child", parent=parent) + first = user_client.post( + criterion_create_url(grandparent.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + assert first.status_code == status.HTTP_201_CREATED + + response = user_client.post( + criterion_create_url(child.id), {"object_id": usage_key(course_run, "p2")}, format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert grandparent.value in str(response.data["tag_id"]) + assert str(course_run.course_key) in str(response.data["tag_id"]) + + +def test_sibling_tags_are_allowed( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """Sibling tags, sharing a parent but neither one the other's ancestor, are both allowed.""" + parent = make_tag(competency_taxonomy, "S4 Parent") + sibling_a = make_tag(competency_taxonomy, "S4 Sibling A", parent=parent) + sibling_b = make_tag(competency_taxonomy, "S4 Sibling B", parent=parent) + first = user_client.post( + criterion_create_url(sibling_a.id), {"object_id": usage_key(course_run, "p1")}, format="json", + ) + assert first.status_code == status.HTTP_201_CREATED + + response = user_client.post( + criterion_create_url(sibling_b.id), {"object_id": usage_key(course_run, "p2")}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + + +def test_an_archived_ancestor_criterion_does_not_block_its_descendant( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """Once the ancestor's criterion is archived, the descendant may be associated in the same course.""" + parent = make_tag(competency_taxonomy, "S7 Parent") + child = make_tag(competency_taxonomy, "S7 Child", parent=parent) + parent_object_id = usage_key(course_run, "p1") + first = user_client.post(criterion_create_url(parent.id), {"object_id": parent_object_id}, format="json") + assert first.status_code == status.HTTP_201_CREATED + CompetencyCriterion.objects.filter(object_tag__object_id=parent_object_id).update(archived=True) + + response = user_client.post( + criterion_create_url(child.id), {"object_id": usage_key(course_run, "p2")}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + + +def test_hierarchically_related_tags_scoped_to_different_courses_are_allowed( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, organization: Organization, +) -> None: + """An ancestor/descendant pair is allowed when each criterion is scoped to a different course.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + parent = make_tag(competency_taxonomy, "S5 Parent") + child = make_tag(competency_taxonomy, "S5 Child", parent=parent) + first = user_client.post(criterion_create_url(parent.id), {"object_id": usage_key(course_run, "p1")}, format="json") + assert first.status_code == status.HTTP_201_CREATED + + response = user_client.post( + criterion_create_url(child.id), {"object_id": usage_key(other_course_run, "p1")}, format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + + +def test_the_same_tag_with_two_different_criteria_in_the_same_course_is_allowed( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, +) -> None: + """A repeat criterion for the same tag in the same course stays allowed.""" + tag = make_tag(competency_taxonomy, "S6 Tag") + first = user_client.post(criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p1")}, format="json") + assert first.status_code == status.HTTP_201_CREATED + + response = user_client.post(criterion_create_url(tag.id), {"object_id": usage_key(course_run, "p2")}, format="json") + + assert response.status_code == status.HTTP_201_CREATED diff --git a/tests/openedx_tagging/test_api.py b/tests/openedx_tagging/test_api.py index 7c9931925..e047c6a07 100644 --- a/tests/openedx_tagging/test_api.py +++ b/tests/openedx_tagging/test_api.py @@ -209,6 +209,56 @@ def test_get_children_tags_no_children(self) -> None: """ assert not list(tagging_api.get_children_tags(self.read_only_taxonomy, self.read_only_taxonomy_tag.value)) + def test_get_ancestor_tags(self) -> None: + """ + get_ancestor_tags() returns every ancestor of a multi-level tag, up to the root. + """ + eukaryota = get_tag("Eukaryota") + result = {tag.value for tag in tagging_api.get_ancestor_tags(self.mammalia)} + assert result == {eukaryota.value, self.animalia.value, self.chordata.value} + + def test_get_ancestor_tags_root_has_no_ancestors(self) -> None: + """ + A root tag (no parent) has no ancestors. + """ + assert not list(tagging_api.get_ancestor_tags(self.archaea)) + + def test_get_ancestor_tags_free_text_taxonomy(self) -> None: + """ + A tag on a free-text-enabled taxonomy has no taxonomy-defined hierarchy to walk. + """ + tag = Tag.objects.create(taxonomy=self.free_text_taxonomy, value="some_tag") + assert not list(tagging_api.get_ancestor_tags(tag)) + + def test_get_descendant_tags(self) -> None: + """ + get_descendant_tags() returns every descendant at any depth, including grandchildren. + """ + result = {tag.value for tag in tagging_api.get_descendant_tags(self.animalia)} + assert result == { + "Arthropoda", + self.chordata.value, + self.mammalia.value, # A grandchild of Animalia (via Chordata), not just a direct child. + "Cnidaria", + "Ctenophora", + "Gastrotrich", + "Placozoa", + "Porifera", + } + + def test_get_descendant_tags_leaf_has_no_descendants(self) -> None: + """ + A leaf tag (no children) has no descendants. + """ + assert not list(tagging_api.get_descendant_tags(self.mammalia)) + + def test_get_descendant_tags_free_text_taxonomy(self) -> None: + """ + A tag on a free-text-enabled taxonomy has no taxonomy-defined hierarchy to walk. + """ + tag = Tag.objects.create(taxonomy=self.free_text_taxonomy, value="some_tag") + assert not list(tagging_api.get_descendant_tags(tag)) + def test_resync_object_tags(self) -> None: self.taxonomy.allow_multiple = True self.taxonomy.save()