diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 5869b2885..5446629c5 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -155,6 +155,14 @@ Decision - Points to one ``oel_tagging_objecttag`` association. - Uses one ``CompetencyRuleProfile`` by default, with optional per-criterion overrides. + No ancestor or descendant of a given competency tag may hold live criteria in the same course + as the given competency tag. CBE takes as given that a learner who demonstrates every + sub-competency has demonstrated the parent competency. If a course could hold authored + criteria at more than one level of the same competency hierarchy, it would be undefined + whether that given or the authored criteria decide the parent's status, and which takes + precedence. There is also no use case for it: nobody authors criteria for a competency and + its ancestors in the same course, because doing so only adds complexity and confusion. + This new database table will have the following columns: 1. ``id``: unique primary key @@ -490,3 +498,8 @@ Changelog supersedes the ordered, short-circuited evaluation the earlier text described. Decision 6's update flow now continues past the root group to the competency's own status row and up through parent competencies, and states what a competency with no criteria tree of its own receives. + +2026-11-06: + +* Decision 4 states that no ancestor or descendant of a competency tag may hold live criteria + in the same course as that tag, and why. diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 29205bb0d..1e62bd6bb 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -16,6 +16,7 @@ from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import CourseLocator from openedx_catalog.api import get_course_run, get_course_run_ids from openedx_catalog.models_api import CourseRun @@ -39,6 +40,7 @@ from .statuses import raise_status __all__ = [ + "BlockedCourse", "CompetencyCriteriaGroupArchivedError", "CompetencyCriteriaTree", "CompetencyCriterionArchivedError", @@ -47,6 +49,7 @@ "bulk_update_competency_criteria", "create_competency_criterion", "create_competency_taxonomy", + "get_blocked_courses", "get_competency_criteria_tree", "get_competency_rule_profiles", "get_courses_with_relative_criteria", @@ -201,6 +204,7 @@ def get_competency_criteria_tree(tag: Tag, course_keys: list[CourseKey] | None = ``[]`` returns only the groups above any course. - A key that matches no course run is dropped, and a repeated key counts once, at its first position. A list in which no key resolves returns only the groups above any course. + - A key's branch and version are ignored, and a version-only key raises ``InvalidKeyError``. Groups come back with the groups above any course first, then the named courses in the order requested, and within each of those by ``(parent_id, ordering, id)`` with root-level rows first. @@ -210,6 +214,9 @@ def get_competency_criteria_tree(tag: Tag, course_keys: list[CourseKey] | None = under every reachable group before ``course_keys`` is applied, so a caller scoped down to zero criteria can still tell the competency is not empty. """ + if course_keys is not None: + # The dict lookup below compares keys exactly, while the database already ignores branch and version. + course_keys = [key.for_branch(None) if isinstance(key, CourseLocator) else key for key in course_keys] groups = list( CompetencyCriteriaGroup.objects.filter(tag=tag, archived=False) .select_related("course") @@ -337,13 +344,14 @@ def resolve_supplied_leaf_group(group_id: int, tag: Tag, course_run: CourseRun) def get_courses_with_relative_criteria( - tag_ids: Iterable[int], course: CourseRun | None = None, + tag_ids: Iterable[int], course: CourseRun | None = None, *, course_keys: Iterable[CourseKey] | 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. + narrowed by ``course`` and by ``course_keys``, each when given. Archived criteria and criteria + under an archived group are ignored. """ groups: QuerySet[CompetencyCriteriaGroup, CompetencyCriteriaGroup] = CompetencyCriteriaGroup.objects.filter( tag_id__in=tag_ids, @@ -356,6 +364,8 @@ def get_courses_with_relative_criteria( ) if course is not None: groups = groups.filter(parent__course=course) + if course_keys is not None: + groups = groups.filter(parent__course__course_key__in=list(course_keys)) 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" @@ -364,6 +374,63 @@ def get_courses_with_relative_criteria( return result +@dataclass(frozen=True, kw_only=True, slots=True) +class BlockedCourse: + """ + A course where a taxonomy ancestor or descendant of a competency already has a live CompetencyCriterion. + + UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a + deprecation cycle. + """ + + course_key: CourseKey + conflicting_tag_id: int + conflicting_tag_value: str + + +def _get_relative_tag_ids(tag: Tag) -> set[int]: + """ + Return the ids of every taxonomy ancestor and descendant of ``tag``. + + The blocked-courses read and the write-time containment check both use this, so they cannot disagree. + """ + return ( + set(get_ancestor_tags(tag).values_list("id", flat=True)) + | set(get_descendant_tags(tag).values_list("id", flat=True)) + ) + + +def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None) -> list[BlockedCourse]: + """ + Return the courses where a taxonomy ancestor or descendant of ``tag`` already has a live criterion. + + UNSTABLE: the criteria-authoring family is incomplete, so its group update and archive + behavior may change this function's shape without a deprecation cycle. + + Narrowed to ``course_keys`` when given. A key's branch and version are ignored, and a + version-only key raises ``InvalidKeyError``. Sorted by course key. When several relatives claim + one course, the lowest-id relative is named. Criteria on ``tag`` itself never block a course. + Requested keys that match no blocked course are dropped. + """ + if course_keys is not None: + # The database filter would drop branch and version anyway; normalizing here fails a version-only key up front. + course_keys = [key.for_branch(None) if isinstance(key, CourseLocator) else key for key in course_keys] + relative_ids = _get_relative_tag_ids(tag) + if not relative_ids: + return [] + conflicts = get_courses_with_relative_criteria(relative_ids, course_keys=course_keys) + rows = [] + for course_run, conflicting_tag in conflicts.items(): + # CourseRun.save() always derives course_key, so a saved row has one. + assert course_run.course_key is not None, f"CourseRun {course_run.id} has no course_key" + rows.append(BlockedCourse( + course_key=course_run.course_key, + conflicting_tag_id=conflicting_tag.id, + conflicting_tag_value=conflicting_tag.value, + )) + return sorted(rows, key=lambda row: str(row.course_key)) + + def _validate_containment(group: CompetencyCriteriaGroup, course: CourseRun) -> None: """ Raise ValidationError (keyed "tag_id") if ``group.tag`` conflicts with an existing criterion. @@ -372,10 +439,7 @@ def _validate_containment(group: CompetencyCriteriaGroup, course: CourseRun) -> 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)) - ) + relative_ids = _get_relative_tag_ids(tag) if not relative_ids: return conflicting_tag = get_courses_with_relative_criteria(relative_ids, course=course).get(course) diff --git a/src/openedx_learning/applets/cbe/rest_api/paginators.py b/src/openedx_learning/applets/cbe/rest_api/paginators.py index ea23f67c9..8d84e2125 100644 --- a/src/openedx_learning/applets/cbe/rest_api/paginators.py +++ b/src/openedx_learning/applets/cbe/rest_api/paginators.py @@ -18,3 +18,16 @@ class CompetencyRuleProfilePagination(DefaultPagination): page_size = 100 max_page_size = 500 + + +class CompetencyBlockedCoursesPagination(DefaultPagination): + """ + Page size for a competency's blocked-courses collection. + + Pinned here rather than inherited from the consuming project's DEFAULT_PAGINATION_CLASS: this + is a published library, so the page size is part of its REST contract instead of varying by + deployment. + """ + + page_size = 100 + max_page_size = 500 diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py index 57376ee8f..d9e5c764a 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -6,6 +6,7 @@ from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import CourseLocator from rest_framework import serializers from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator @@ -177,7 +178,7 @@ def validate(self, attrs: dict) -> dict: class CompetencyCriteriaQueryParamsSerializer(serializers.Serializer): - """Query params for the criteria-tree read endpoint.""" + """Query params shared by the criteria-tree and blocked-courses read endpoints.""" course_keys = serializers.CharField(required=False, allow_blank=True) @@ -185,9 +186,10 @@ def validate_course_keys(self, value: str) -> list[CourseKey]: """ Parse a comma-separated course_keys value into a deduplicated list of CourseKey. - Stray separators (trailing, doubled, or all-commas) are ignored, not rejected. Raises - a validation error if more than MAX_COURSE_KEYS entries remain after splitting, checked - before dedup so repeating a key can't dodge the cap, or if any entry isn't a valid key. + Each key is stripped of any branch or version before dedup. Stray separators (trailing, + doubled, or all-commas) are ignored, not rejected. Raises a validation error if more than + MAX_COURSE_KEYS entries remain after splitting, checked before dedup so repeating a key + can't dodge the cap, or if any entry isn't a valid key. """ entries = [entry for entry in (entry.strip() for entry in value.split(",")) if entry] if len(entries) > MAX_COURSE_KEYS: @@ -197,11 +199,16 @@ def validate_course_keys(self, value: str) -> list[CourseKey]: parsed = [] for entry in entries: try: - parsed.append(CourseKey.from_string(entry)) + key = CourseKey.from_string(entry) + # Match the stored plain course key: drop branch and version; this also rejects a version-only key. + # The isinstance narrows CourseKey, which mypy does not know has for_branch(). + if isinstance(key, CourseLocator): + key = key.for_branch(None) except InvalidKeyError as exc: raise serializers.ValidationError( _("'%(entry)s' is not a valid course key.") % {"entry": entry} ) from exc + parsed.append(key) return list(dict.fromkeys(parsed)) def create(self, validated_data): @@ -235,3 +242,24 @@ class Meta: "id", "group_id", "object_tag_id", "object_id", "rule_profile_id", "rule_type_override", "rule_payload_override", ] + + +class BlockedCourseSerializer(serializers.Serializer): + """ + Read-only representation of a BlockedCourse, for the blocked-courses endpoint. + + UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a + deprecation cycle. + """ + + course_key = serializers.CharField(read_only=True) + conflicting_tag_id = serializers.IntegerField(read_only=True) + conflicting_tag_value = serializers.CharField(read_only=True) + + def create(self, validated_data): + """Not supported: this serializer is read-only.""" + raise NotImplementedError + + def update(self, instance, validated_data): + """Not supported: this serializer is read-only.""" + raise NotImplementedError diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py index e96d754a1..0570fdd5a 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -32,4 +32,9 @@ views.CompetencyCriteriaGroupDetailView.as_view(), name="criteria-group-detail", ), + path( + "competencies//blocked-courses/", + views.CompetencyBlockedCoursesView.as_view(), + name="blocked-courses", + ), ] diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index d7c3a4d8c..b29835a56 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -26,15 +26,17 @@ CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, + get_blocked_courses, get_competency_criteria_tree, get_competency_rule_profiles, resolve_competency_tag, update_competency_criteria_group, ) from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile -from ..paginators import CompetencyRuleProfilePagination +from ..paginators import CompetencyBlockedCoursesPagination, CompetencyRuleProfilePagination from .permissions import CompetencyReadPermission, CompetencyRuleProfilePermissions from .serializers import ( + BlockedCourseSerializer, CompetencyCriteriaGroupSerializer, CompetencyCriteriaGroupUpdateSerializer, CompetencyCriteriaQueryParamsSerializer, @@ -192,7 +194,9 @@ class CompetencyCriteriaTreeView(generics.GenericAPIView): Omitting the parameter returns every reachable group. Supplying it returns the groups above any course plus the groups of the named runs, ordered with the groups above any course first and then by the order requested; keys that match no run are ignored, and an empty value - returns only the groups above any course. The parameter may be supplied once. + returns only the groups above any course. The parameter may be supplied once. A key's branch + and version are ignored, so a branch- or version-qualified key scopes to its plain course, and + a version-only key is a 400. ``criteria_count`` is the number of criteria returned. ``total_criteria_count`` is the number under every reachable group regardless of ``course_keys``, so a client scoped down to zero @@ -273,3 +277,46 @@ def patch(self, request, tag_id: int, group_id: int): except CompetencyCriteriaGroupArchivedError as exc: raise CompetencyCriteriaConflict(str(exc)) from exc return Response(CompetencyCriteriaGroupSerializer(group).data, status=status.HTTP_200_OK) + + +class CompetencyBlockedCoursesView(generics.GenericAPIView): + """ + GET-only. List the courses where an ancestor or descendant of a competency already has a live + criterion, paginated. + + Along any ancestor-descendant line of the taxonomy, a course may hold live criteria for one + competency only. So once an ancestor or descendant of this competency has a live criterion in a + course, this competency can't get one in it. + + ``GET api/cbe/v1/competencies//blocked-courses/`` returns those courses sorted by + course key. ``course_keys`` (comma-separated, supplied once) narrows it; a blank value returns + an empty page. A key's branch and version are ignored, so a branch- or version-qualified key + matches its plain course and each row carries the plain key; a version-only key is a 400. The + result is an authoring hint: the create-criterion endpoint still enforces the rule. + + An unauthenticated caller gets 401. A caller who can't view the competency's taxonomy gets 403. + A ``tag_id`` that isn't a competency tag, or a ``page`` out of range, is a 404. A ``course_keys`` + that is malformed, given more than once, or names more than 100 keys is a 400. + + UNSTABLE: the criteria-authoring family is incomplete, so this response shape may change + without a deprecation cycle. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated, CompetencyReadPermission] + pagination_class = CompetencyBlockedCoursesPagination + + def get(self, request, tag_id): + """Return one page of the competency's blocked courses.""" + try: + tag = resolve_competency_tag(tag_id) + except Tag.DoesNotExist as exc: + raise NotFound(str(exc)) from exc + self.check_object_permissions(request, tag) + if len(request.query_params.getlist("course_keys")) > 1: + raise DRFValidationError({"course_keys": _("Supply course_keys once, as a comma-separated list.")}) + query_params = CompetencyCriteriaQueryParamsSerializer(data=request.query_params.dict()) + query_params.is_valid(raise_exception=True) + rows = get_blocked_courses(tag, course_keys=query_params.validated_data.get("course_keys")) + page = self.paginate_queryset(rows) + return self.get_paginated_response(BlockedCourseSerializer(page, many=True).data) diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 52f844e07..157372655 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -17,14 +17,16 @@ from organizations.models import Organization from rules.permissions import permissions as rule_permissions -from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_catalog.models import CourseRun from openedx_learning.api import ( + BlockedCourse, CompetencyCriteriaGroupArchivedError, CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, create_competency_criterion, create_competency_taxonomy, + get_blocked_courses, get_competency_criteria_tree, get_competency_rule_profiles, get_courses_with_relative_criteria, @@ -49,6 +51,8 @@ ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy +from .utils import make_course_run, make_leaf_criterion, make_skill_family, make_tag, usage_key + pytestmark = pytest.mark.django_db GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} @@ -99,33 +103,11 @@ def test_create_competency_taxonomy_defaults() -> None: assert result.export_id -def make_course_run(organization: Organization, course_code: str, run_code: str) -> CourseRun: - """Create a CourseRun distinct from the `course_run` fixture, for the same-tag-different-course tests.""" - catalog_course = CatalogCourse.objects.create(org=organization, course_code=course_code) - return CourseRun.objects.create(catalog_course=catalog_course, run_code=run_code) - - -def usage_key(course_run: CourseRun, block_id: str) -> str: - """Build a gradeable-subsection-shaped usage key string under `course_run`.""" +def course_key(course_run: CourseRun) -> CourseKey: + """Return `course_run`'s course key, narrowed for mypy; a saved CourseRun always has one.""" key = course_run.course_key assert key is not None - 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) + return key def test_is_competency_taxonomy() -> None: @@ -1540,6 +1522,24 @@ def test_get_competency_criteria_tree_course_keys_unresolvable_key_is_silently_i assert tree.criteria_count == 0 +def test_get_competency_criteria_tree_course_keys_branch_qualified_key_matches_the_plain_course( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A branch-qualified key, passed straight to the API, scopes to its plain course's groups.""" + criterion = make_leaf_criterion(tag, course_run, default_rule_profile, "p1") + leaf = criterion.group + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + branched_key = CourseKey.from_string("course-v1:Org1+Python100+Fall2026+branch@draft") + + tree = get_competency_criteria_tree(tag, [branched_key]) + + assert {g.id for g in tree.groups} == {root.id, course_level.id, leaf.id} + assert [c.id for c in tree.criteria] == [criterion.id] + + def test_get_competency_criteria_tree_course_keys_scoped_to_a_course_with_nothing( tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, ) -> None: @@ -1762,12 +1762,15 @@ def test_get_courses_with_relative_criteria_single_match( 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.""" + """ + When two of the given tags both have a live criterion in the same course, the lower id tag wins, + even when its criterion was created later. + """ 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") + make_leaf_criterion(first, course_run, default_rule_profile, "p1") result = get_courses_with_relative_criteria([second.id, first.id]) @@ -1923,6 +1926,26 @@ def test_get_courses_with_relative_criteria_narrows_to_the_given_course( assert get_courses_with_relative_criteria([relative.id], course=course_run) == {course_run: relative} +def test_get_courses_with_relative_criteria_narrows_to_the_given_course_keys( + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """With ``course_keys`` given, only those courses' conflicts come back, though another course has one too.""" + relative = make_tag(competency_taxonomy, "Relative Skill") + course_y = make_course_run(organization, "Python200", "Fall2026") + course_z = make_course_run(organization, "Python300", "Fall2026") + for course in (course_run, course_y, course_z): + make_leaf_criterion(relative, course, default_rule_profile, "p1") + + result = get_courses_with_relative_criteria( + [relative.id], course_keys=[course_key(course_run), course_key(course_y)], + ) + + assert result == {course_run: relative, course_y: 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: @@ -2302,3 +2325,94 @@ def test_update_never_reads_or_writes_learner_status( pass assert not [q for q in queries.captured_queries if "studentcompetency" in q["sql"].lower()] + + +# ============================================================================================== +# get_blocked_courses +# ============================================================================================== + + +@pytest.mark.parametrize( + "claimant_and_queried", + [ + pytest.param((1, 2), id="ancestor claim"), + pytest.param((2, 1), id="descendant claim"), + pytest.param((0, 2), id="grandparent claim"), + pytest.param((2, 0), id="grandchild claim"), + ], +) +def test_get_blocked_courses_reports_a_relative_claim( + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + claimant_and_queried: tuple[int, int], +) -> None: + """A course where an ancestor or descendant at any depth has a criterion is blocked, naming that relative.""" + family = make_skill_family(competency_taxonomy) + claimant, queried = (family[index] for index in claimant_and_queried) + make_leaf_criterion(claimant, course_run, default_rule_profile, "p1") + + assert get_blocked_courses(queried) == [ + BlockedCourse( + course_key=course_key(course_run), conflicting_tag_id=claimant.id, conflicting_tag_value=claimant.value, + ), + ] + + +def test_get_blocked_courses_ignores_a_sibling_claim( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A sibling is neither ancestor nor descendant, so its criterion blocks nothing.""" + _, _, inference, analysis = make_skill_family(competency_taxonomy) + make_leaf_criterion(analysis, course_run, default_rule_profile, "p1") + + assert not get_blocked_courses(inference) + + +def test_get_blocked_courses_ignores_a_claim_on_the_tag_itself( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A criterion on the queried tag itself is not a conflict, so it blocks nothing.""" + _, _, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(inference, course_run, default_rule_profile, "p1") + + assert get_blocked_courses(inference) == [] + + +def test_get_blocked_courses_is_empty_for_a_tag_with_no_relatives(tag: Tag) -> None: + """A root tag with no children has no relatives to conflict with.""" + assert get_blocked_courses(tag) == [] + + +def test_get_blocked_courses_and_create_competency_criterion_agree_on_a_conflict( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course the read reports as blocked for a tag is one the write rejects a criterion for.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + assert get_blocked_courses(inference) == [ + BlockedCourse( + course_key=course_key(course_run), + conflicting_tag_id=critical_thinking.id, + conflicting_tag_value=critical_thinking.value, + ), + ] + group = _create_leaf_group(inference, course_run) + with pytest.raises(ValidationError) as exc_info: + create_competency_criterion(group, usage_key(course_run, "p2"), course_run=course_run) + assert "tag_id" in exc_info.value.message_dict + + +def test_get_blocked_courses_sorts_rows_by_course_key( + competency_taxonomy: CompetencyTaxonomy, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Rows come back in course key order, not in the order the courses were created.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + for course_code in ("C300", "A100", "B200"): + course = make_course_run(organization, course_code, "Run1") + make_leaf_criterion(critical_thinking, course, default_rule_profile, "p1") + + keys = [str(row.course_key) for row in get_blocked_courses(inference)] + + assert keys == [f"course-v1:Org1+{course_code}+Run1" for course_code in ("A100", "B200", "C300")] diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index be957f857..72550ba5d 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -16,7 +16,7 @@ from rest_framework import status from rest_framework.test import APIClient -from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_catalog.models import CourseRun from openedx_learning.applets.cbe.api import _create_leaf_group from openedx_learning.models import ( CompetencyCriteriaGroup, @@ -28,6 +28,8 @@ ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy +from .utils import make_course_run, make_leaf_criterion, make_skill_family, make_tag, usage_key + pytestmark = pytest.mark.django_db # A marker embedded in an object_id to signal "this caller has no object-level tagging @@ -73,24 +75,6 @@ def criterion_create_url(tag_id: int) -> str: return reverse("cbe:criterion-create", kwargs={"tag_id": tag_id}) -def usage_key(course_run: CourseRun, block_id: str) -> str: - """Build a gradeable-subsection-shaped usage key string under `course_run`.""" - key = course_run.course_key - assert key is not None - return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@{block_id}" - - -def make_course_run(organization: Organization, course_code: str, run_code: str) -> CourseRun: - """Create a CourseRun distinct from the `course_run` fixture, for the different-course tests.""" - catalog_course = CatalogCourse.objects.create(org=organization, course_code=course_code) - 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"} @@ -1457,6 +1441,15 @@ def test_a_malformed_course_key_is_400(user_client: APIClient, tag: Tag) -> None assert set(response.data) == {"course_keys"} +def test_a_version_only_course_key_is_400(user_client: APIClient, tag: Tag) -> None: + """A version-only course key parses but can't drop its version, so it is rejected like a malformed one.""" + version_only = "course-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa" + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": version_only}) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert set(response.data) == {"course_keys"} + + def test_more_than_the_limit_of_course_keys_is_400(user_client: APIClient, tag: Tag) -> None: """More than 100 course_keys entries 400s, keyed by course_keys, and the error names the 100 limit.""" too_many = ",".join(f"course-v1:Org+Course{i}+Run" for i in range(101)) @@ -1497,6 +1490,32 @@ def test_a_repeated_course_keys_query_parameter_is_400(user_client: APIClient, t assert set(response.data) == {"course_keys"} +@pytest.mark.parametrize("suffix", [ + "+branch@draft", + "+branch@draft+version@aaaaaaaaaaaaaaaaaaaaaaaa", + "+version@aaaaaaaaaaaaaaaaaaaaaaaa", +]) +def test_a_branch_qualified_course_key_matches_the_plain_course( + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + suffix: str, +) -> None: + """A course key with a branch or version scopes the tree exactly as the plain course key does.""" + leaf = _create_leaf_group(tag, course_run) + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) + key = str(course_run.course_key) + + plain = user_client.get(criteria_tree_url(tag.id), {"course_keys": key}) + branched = user_client.get(criteria_tree_url(tag.id), {"course_keys": f"{key}{suffix}"}) + + assert plain.status_code == branched.status_code == status.HTTP_200_OK + assert plain.data["criteria_count"] == 1 + assert branched.data == plain.data + + def test_no_view_access_with_course_keys_still_403s_without_leaking_data( api_client: APIClient, user, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: @@ -1993,3 +2012,347 @@ def test_group_detail_offers_only_patch( response = getattr(user_client, method)(group_detail_url(tag.id, leaf.id), {}, format="json") assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + + +# ============================================================================================== +# CompetencyBlockedCoursesView +# ============================================================================================== + + +def blocked_courses_url(tag_id: int, **params: str) -> str: + """Return the blocked-courses endpoint's path for `tag_id`, with `params` as its query string.""" + url = reverse("cbe:blocked-courses", kwargs={"tag_id": tag_id}) + return f"{url}?{urlencode(params)}" if params else url + + +def collect_pages(client: APIClient, url: str) -> tuple[int, list[list[dict]]]: + """Follow `next` links from `url` and return the first page's count and each page's rows.""" + pages: list[list[dict]] = [] + count = None + next_url: str | None = url + while next_url: + response = client.get(next_url) + assert response.status_code == status.HTTP_200_OK + if count is None: + count = response.data["count"] + pages.append(response.data["results"]) + next_url = response.data["next"] + assert count is not None + return count, pages + + +def test_blocked_courses_reports_a_relative_claim_with_the_row_shape( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course an ancestor has a criterion in is reported in a paginated envelope, naming that ancestor.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_200_OK + assert set(response.data) == {"next", "previous", "count", "num_pages", "current_page", "start", "results"} + assert response.data["results"] == [{ + "course_key": "course-v1:Org1+Python100+Fall2026", + "conflicting_tag_id": critical_thinking.id, + "conflicting_tag_value": "Critical Thinking", + }] + + +def test_blocked_courses_ignores_an_unaffected_course( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course with no relative's criterion is not reported, even when another course is blocked.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_course_run(organization, "Python200", "Fall2026") + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_200_OK + assert [row["course_key"] for row in response.data["results"]] == [str(course_run.course_key)] + + +def test_blocked_courses_narrows_to_requested_course_keys( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Only the requested courses come back, though a third course is blocked too.""" + cognitive_skills, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + course_y = make_course_run(organization, "Python200", "Fall2026") + course_z = make_course_run(organization, "Python300", "Fall2026") + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + make_leaf_criterion(cognitive_skills, course_y, default_rule_profile, "p1") + make_leaf_criterion(critical_thinking, course_z, default_rule_profile, "p1") + + response = user_client.get( + blocked_courses_url(inference.id, course_keys=f"{course_run.course_key},{course_y.course_key}"), + ) + + assert response.status_code == status.HTTP_200_OK + assert {row["course_key"] for row in response.data["results"]} == { + str(course_run.course_key), str(course_y.course_key), + } + + +def test_blocked_courses_is_an_empty_200_when_nothing_is_claimed( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """No relative has a criterion anywhere: an empty collection, not a failure.""" + _, _, inference, _ = make_skill_family(competency_taxonomy) + + response = user_client.get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == 0 + assert response.data["results"] == [] + + +def test_blocked_courses_paginates_without_repeats_or_gaps( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Three blocked courses at two per page arrive across two pages, each exactly once, in course key order.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + for course_code in ("C300", "A100", "B200"): + course = make_course_run(organization, course_code, "Run1") + make_leaf_criterion(critical_thinking, course, default_rule_profile, "p1") + count, pages = collect_pages(user_client, blocked_courses_url(inference.id, page_size="2")) + + assert count == 3 + assert [len(page) for page in pages] == [2, 1] + assert [row["course_key"] for page in pages for row in page] == [ + "course-v1:Org1+A100+Run1", "course-v1:Org1+B200+Run1", "course-v1:Org1+C300+Run1", + ] + + +def test_blocked_courses_filters_before_paginating( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A narrowed result that spans two pages holds exactly the requested courses, in course key order.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + for course_code in ("C300", "A100", "D400", "B200"): + course = make_course_run(organization, course_code, "Run1") + make_leaf_criterion(critical_thinking, course, default_rule_profile, "p1") + requested = "course-v1:Org1+D400+Run1,course-v1:Org1+A100+Run1,course-v1:Org1+C300+Run1" + count, pages = collect_pages(user_client, blocked_courses_url(inference.id, course_keys=requested, page_size="2")) + + assert count == 3 + assert [len(page) for page in pages] == [2, 1] + assert [row["course_key"] for page in pages for row in page] == [ + "course-v1:Org1+A100+Run1", "course-v1:Org1+C300+Run1", "course-v1:Org1+D400+Run1", + ] + + +def test_blocked_courses_default_page_size_is_100( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Without page_size, a page holds 100 rows and the 101st is on the next page.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + for i in range(101): + course = make_course_run(organization, f"C{i}", "Run1") + make_leaf_criterion(critical_thinking, course, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == 101 + assert len(response.data["results"]) == 100 + assert response.data["next"] is not None + + +def test_blocked_courses_page_out_of_range_404s( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A page past the last one is reported as not found.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(inference.id, page="999")) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_blocked_courses_unknown_tag_404s(user_client: APIClient) -> None: + """A tag_id that doesn't resolve is reported as not found.""" + response = user_client.get(blocked_courses_url(999999)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_blocked_courses_tag_outside_a_competency_taxonomy_404s(user_client: APIClient) -> None: + """A real Tag that isn't on a CompetencyTaxonomy is reported as not found.""" + plain_taxonomy = Taxonomy.objects.create(name="Plain Tags", export_id="plain-v1") + plain_tag = Tag.objects.create(taxonomy=plain_taxonomy, value="Not A Competency") + + response = user_client.get(blocked_courses_url(plain_tag.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_blocked_courses_without_view_access_403s( + api_client: APIClient, + user, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A non-staff caller may not read blocked courses for a competency on a disabled taxonomy, even + with garbage in course_keys: permission is checked before course_keys is parsed. + """ + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + competency_taxonomy.enabled = False + competency_taxonomy.save() + api_client.force_authenticate(user=user) + + response = api_client.get(blocked_courses_url(inference.id, course_keys="not-a-key")) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "results" not in response.data + + +def test_blocked_courses_anonymous_request_is_401_and_discloses_nothing( + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """An unauthenticated caller gets 401 and no rows, even for a competency with a blocked course.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + response = APIClient().get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert "results" not in response.data + + +def test_blocked_courses_malformed_course_key_400s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """An unparseable course key is rejected, and the error names it.""" + _, _, inference, _ = make_skill_family(competency_taxonomy) + + response = user_client.get(blocked_courses_url(inference.id, course_keys="not-a-key")) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "not-a-key" in str(response.data["course_keys"]) + + +@pytest.mark.parametrize("course_keys", ["", ",,"]) +def test_blocked_courses_blank_course_keys_is_an_empty_page( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + course_keys: str, +) -> None: + """A course_keys filter that names no course narrows to nothing, even when a course is blocked.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(inference.id, course_keys=course_keys)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == 0 + assert response.data["results"] == [] + + +def test_blocked_courses_repeated_course_keys_parameter_400s( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Supplying course_keys twice is rejected instead of silently using one value, even when both are blocked.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + course_y = make_course_run(organization, "Python200", "Fall2026") + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + make_leaf_criterion(critical_thinking, course_y, default_rule_profile, "p1") + query = urlencode({"course_keys": [str(course_run.course_key), str(course_y.course_key)]}, doseq=True) + + response = user_client.get(f"{blocked_courses_url(inference.id)}?{query}") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "results" not in response.data + + +def test_blocked_courses_matches_a_branch_qualified_course_key( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A branch-qualified key matches its plain course, and the row reports the plain key.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + + response = user_client.get( + blocked_courses_url(inference.id, course_keys=f"{course_run.course_key}+branch@draft"), + ) + + assert response.status_code == status.HTTP_200_OK + assert [row["course_key"] for row in response.data["results"]] == ["course-v1:Org1+Python100+Fall2026"] + + +def test_blocked_courses_strips_whitespace_around_course_keys( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Spaces after the commas don't make a key unparseable.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + course_y = make_course_run(organization, "Python200", "Fall2026") + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + make_leaf_criterion(critical_thinking, course_y, default_rule_profile, "p1") + + response = user_client.get( + blocked_courses_url(inference.id, course_keys=f"{course_run.course_key}, {course_y.course_key}"), + ) + + assert response.status_code == status.HTTP_200_OK + assert {row["course_key"] for row in response.data["results"]} == { + str(course_run.course_key), str(course_y.course_key), + } + + +def test_blocked_courses_ignores_an_archived_claim( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A relative's archived criterion doesn't block the course.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + criterion = make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + CompetencyCriterion.objects.filter(pk=criterion.pk).update(archived=True) + + response = user_client.get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["results"] == [] diff --git a/tests/openedx_learning/applets/cbe/utils.py b/tests/openedx_learning/applets/cbe/utils.py new file mode 100644 index 000000000..4e291ddfe --- /dev/null +++ b/tests/openedx_learning/applets/cbe/utils.py @@ -0,0 +1,45 @@ +"""Shared test helpers for the CBE test modules.""" +from organizations.models import Organization + +from openedx_catalog.models import CatalogCourse, CourseRun +from openedx_learning.applets.cbe.api import _create_leaf_group +from openedx_learning.models import CompetencyCriterion, CompetencyRuleProfile, CompetencyTaxonomy +from openedx_tagging.models import ObjectTag, Tag + + +def usage_key(course_run: CourseRun, block_id: str) -> str: + """Build a gradeable-subsection-shaped usage key string under `course_run`.""" + key = course_run.course_key + assert key is not None + return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@{block_id}" + + +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 make_course_run(organization: Organization, course_code: str, run_code: str) -> CourseRun: + """Create a CourseRun distinct from the `course_run` fixture, for the different-course tests.""" + catalog_course = CatalogCourse.objects.create(org=organization, course_code=course_code) + 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) + + +def make_skill_family(taxonomy: CompetencyTaxonomy) -> tuple[Tag, Tag, Tag, Tag]: + """ + Create "Cognitive Skills" > "Critical Thinking" > "Inference", plus "Analysis" as a sibling of "Inference". + """ + cognitive_skills = Tag.objects.create(taxonomy=taxonomy, value="Cognitive Skills") + critical_thinking = Tag.objects.create(taxonomy=taxonomy, value="Critical Thinking", parent=cognitive_skills) + inference = Tag.objects.create(taxonomy=taxonomy, value="Inference", parent=critical_thinking) + analysis = Tag.objects.create(taxonomy=taxonomy, value="Analysis", parent=critical_thinking) + return cognitive_skills, critical_thinking, inference, analysis