diff --git a/src/openedx_catalog/api_impl/courses.py b/src/openedx_catalog/api_impl/courses.py index 0620b38a0..04c8faddb 100644 --- a/src/openedx_catalog/api_impl/courses.py +++ b/src/openedx_catalog/api_impl/courses.py @@ -3,6 +3,7 @@ """ import logging +from collections.abc import Iterable from typing import overload from opaque_keys.edx.keys import CourseKey @@ -19,6 +20,7 @@ "update_catalog_course", "delete_catalog_course", "get_course_run", + "get_course_run_ids", "sync_course_run_details", "create_course_run_for_modulestore_course_with", "delete_course_run", @@ -116,6 +118,26 @@ def get_course_run(course_key: CourseKey) -> CourseRun: return CourseRun.objects.get(course_key__exact=course_key) +def get_course_run_ids(course_keys: Iterable[CourseKey]) -> dict[CourseKey, int]: + """ + Return a {course_key: CourseRun.id} mapping for the keys that resolve to a CourseRun. + + A key with no matching CourseRun is absent from the result. + + ⚠️ Does not check permissions or visibility rules. + """ + course_keys = list(course_keys) + if not course_keys: + return {} + # course_key's accessor is typed CourseKey | None regardless of null=False (opaque_keys' + # own stub), so filter here instead of asserting past mypy. + return { + key: run_id + for key, run_id in CourseRun.objects.filter(course_key__in=course_keys).values_list("course_key", "id") + if key is not None + } + + def sync_course_run_details( course_key: CourseKey, *, diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 79e4ab252..ac2d51609 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -15,9 +15,9 @@ from django.db.transaction import TransactionManagementError from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx_catalog.api import get_course_run +from openedx_catalog.api import get_course_run, get_course_run_ids from openedx_catalog.models_api import CourseRun 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 @@ -167,52 +167,110 @@ class CompetencyCriteriaTree: UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a deprecation cycle. ``groups`` and ``criteria`` are flat lists: each group carries its ``parent_id`` and each criterion its ``group_id``, and both always name a group in ``groups``. + + ``criteria_count`` is ``len(criteria)``. ``total_criteria_count`` counts the criteria under + every reachable group before any course scoping. """ groups: list[CompetencyCriteriaGroup] criteria: list[CompetencyCriterion] + criteria_count: int + total_criteria_count: int -def get_competency_criteria_tree(tag: Tag) -> CompetencyCriteriaTree: +def get_competency_criteria_tree(tag: Tag, course_keys: list[CourseKey] | None = None) -> CompetencyCriteriaTree: """ Return the non-archived CompetencyCriteriaGroup tree and CompetencyCriterion leaves for ``tag``. - A group is returned only if it and every ancestor up to its root are non-archived, and a - criterion only if it and its group are returned, so the result always forms one tree: no - returned ``parent_id`` or ``group_id`` names a row that was left out. Groups come back - root first, then ordered by ``(parent_id, ordering, id)``; criteria by ``(group_id, id)``. - UNSTABLE: the criteria-authoring family is incomplete, so its group update and archive behavior may change this function's shape without a deprecation cycle. + + A group is reachable only if it and every ancestor up to its root are non-archived. A + criterion is returned only if it is non-archived and its group is returned, so the result + always forms one tree: no returned ``parent_id`` or ``group_id`` names a row that was left out. + + ``course_keys`` scopes the result by course run. A course run's groups are its course-level + group (the group whose ``course`` is that run) and every group nested below it, at any depth. + The groups above any course are the ones not nested in any course-level group. + + - ``None`` (the default) returns every reachable group. + - A list returns the groups above any course plus the groups of the named course runs. + ``[]`` 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. + + 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. + Criteria are ordered by their group's position in ``groups``, then ``id``. + + ``criteria_count`` is ``len(criteria)``. ``total_criteria_count`` is the number of criteria + 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. """ groups = list( CompetencyCriteriaGroup.objects.filter(tag=tag, archived=False) .select_related("course") .order_by(F("parent_id").asc(nulls_first=True), "ordering", "id") ) + if not groups: + return CompetencyCriteriaTree(groups=[], criteria=[], criteria_count=0, total_criteria_count=0) # Walk down from the roots instead of relying on row order. A group's parent is fixed once # saved, but a QuerySet.update() can still give a group a parent with a larger id, and # ordering by parent_id would then visit that child before its parent. - children_by_parent_id: dict[int | None, list[int]] = {} + groups_by_parent_id: dict[int | None, list[CompetencyCriteriaGroup]] = {} for group in groups: - children_by_parent_id.setdefault(group.parent_id, []).append(group.id) - kept_group_ids: set[int] = set() - pending = list(children_by_parent_id.get(None, [])) + groups_by_parent_id.setdefault(group.parent_id, []).append(group) + # Maps each reachable group id to the course run id of its branch, or None above any course. + branch_course_ids: dict[int, int | None] = {} + pending = [(group, group.course_id) for group in groups_by_parent_id.get(None, [])] while pending: - group_id = pending.pop() - kept_group_ids.add(group_id) - pending.extend(children_by_parent_id.get(group_id, [])) - + group, branch_course_id = pending.pop() + branch_course_ids[group.id] = branch_course_id + for child in groups_by_parent_id.get(group.id, []): + pending.append((child, branch_course_id if branch_course_id is not None else child.course_id)) + reachable_groups = [group for group in groups if group.id in branch_course_ids] + + if course_keys is None: + scoped_groups = reachable_groups + else: + run_ids_by_key = get_course_run_ids(course_keys) + rank_by_run_id = { + run_ids_by_key[key]: rank + for rank, key in enumerate((key for key in dict.fromkeys(course_keys) if key in run_ids_by_key), start=1) + } + # Groups above any course rank 0, so a scoped tree never orphans a child from its parent, and + # a branch ranks by its course. A branch whose course was not requested is left out. + # sorted() is stable, so each rank keeps the query order. + ranked_groups = [] + for group in reachable_groups: + group_course_id = branch_course_ids[group.id] + if group_course_id is None: + ranked_groups.append((0, group)) + elif group_course_id in rank_by_run_id: + ranked_groups.append((rank_by_run_id[group_course_id], group)) + scoped_groups = [group for _, group in sorted(ranked_groups, key=lambda ranked: ranked[0])] + + position_by_group_id = {group.id: position for position, group in enumerate(scoped_groups)} criteria = list( - CompetencyCriterion.objects.filter(group_id__in=kept_group_ids, archived=False) + CompetencyCriterion.objects.filter(group_id__in=position_by_group_id.keys(), archived=False) .select_related("object_tag") .order_by("group_id", "id") ) + # sort() is stable, so criteria of one group keep their id order. + criteria.sort(key=lambda criterion: position_by_group_id[criterion.group_id]) + if course_keys is None: + total_criteria_count = len(criteria) + else: + total_criteria_count = CompetencyCriterion.objects.filter( + group_id__in=branch_course_ids.keys(), archived=False, + ).count() return CompetencyCriteriaTree( - groups=[group for group in groups if group.id in kept_group_ids], + groups=scoped_groups, criteria=criteria, + criteria_count=len(criteria), + total_criteria_count=total_criteria_count, ) 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 347020326..57376ee8f 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -5,7 +5,7 @@ from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey from rest_framework import serializers from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator @@ -173,6 +173,46 @@ def validate(self, attrs: dict) -> dict: return attrs +MAX_COURSE_KEYS = 100 + + +class CompetencyCriteriaQueryParamsSerializer(serializers.Serializer): + """Query params for the criteria-tree read endpoint.""" + + course_keys = serializers.CharField(required=False, allow_blank=True) + + 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. + """ + entries = [entry for entry in (entry.strip() for entry in value.split(",")) if entry] + if len(entries) > MAX_COURSE_KEYS: + raise serializers.ValidationError( + _("No more than %(max)s course_keys may be requested at once.") % {"max": MAX_COURSE_KEYS} + ) + parsed = [] + for entry in entries: + try: + parsed.append(CourseKey.from_string(entry)) + except InvalidKeyError as exc: + raise serializers.ValidationError( + _("'%(entry)s' is not a valid course key.") % {"entry": entry} + ) from exc + return list(dict.fromkeys(parsed)) + + def create(self, validated_data): + """Not supported: this serializer only validates query parameters.""" + raise NotImplementedError + + def update(self, instance, validated_data): + """Not supported: this serializer only validates query parameters.""" + raise NotImplementedError + + class CompetencyCriterionReadSerializer(serializers.ModelSerializer): """ Read-only representation of a CompetencyCriterion, for the criteria-tree read endpoint. 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 715009016..d7c3a4d8c 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -37,6 +37,7 @@ from .serializers import ( CompetencyCriteriaGroupSerializer, CompetencyCriteriaGroupUpdateSerializer, + CompetencyCriteriaQueryParamsSerializer, CompetencyCriterionBulkUpdateSerializer, CompetencyCriterionReadSerializer, CompetencyCriterionSerializer, @@ -185,6 +186,17 @@ class CompetencyCriteriaTreeView(generics.GenericAPIView): for client-side reconstruction: each group carries its parent_id, and each criterion its group_id. An empty tree (the tag exists but nothing has been authored under it yet) is a valid 200 with two empty arrays; a 404 is reserved for tag resolution failure only. + + The optional ``course_keys`` query parameter (comma-separated course keys) scopes the tree by + course run. A course run's groups are its course-level group and every group nested below it. + 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. + + ``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 + criteria can still tell the competency is not empty. """ authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) @@ -197,10 +209,17 @@ def get(self, request, tag_id): except Tag.DoesNotExist as exc: raise NotFound(str(exc)) from exc self.check_object_permissions(request, tag) - tree = get_competency_criteria_tree(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) + course_keys = query_params.validated_data.get("course_keys") + tree = get_competency_criteria_tree(tag, course_keys) return Response({ "groups": CompetencyCriteriaGroupSerializer(tree.groups, many=True).data, "criteria": CompetencyCriterionReadSerializer(tree.criteria, many=True).data, + "criteria_count": tree.criteria_count, + "total_criteria_count": tree.total_criteria_count, }) diff --git a/tests/openedx_catalog/test_api.py b/tests/openedx_catalog/test_api.py index 52d48e395..e1abc04bd 100644 --- a/tests/openedx_catalog/test_api.py +++ b/tests/openedx_catalog/test_api.py @@ -178,6 +178,50 @@ def test_get_course_run(python100_summer26: CourseRun) -> None: assert run == python100_summer26 +# get_course_run_ids + + +def test_get_course_run_ids_empty_input_returns_empty_dict_with_no_queries(django_assert_num_queries) -> None: + """An empty course_keys input returns {} and costs zero queries: nothing to look up.""" + with django_assert_num_queries(0): + assert api.get_course_run_ids([]) == {} + + +def test_get_course_run_ids_unknown_key_is_absent_from_the_result() -> None: + """A course key with no matching CourseRun is simply absent from the result, not an error or a None value.""" + unknown_key = CourseKey.from_string("course-v1:NoOrg+NoCourse+NoRun") + + result = api.get_course_run_ids([unknown_key]) + + assert result == {} + + +def test_get_course_run_ids_duplicate_key_yields_one_entry(python100_summer26: CourseRun) -> None: + """The same course key passed twice in the input produces exactly one entry in the result dict.""" + assert python100_summer26.course_key is not None + key = python100_summer26.course_key + + result = api.get_course_run_ids([key, key]) + + assert result == {key: python100_summer26.id} + + +def test_get_course_run_ids_maps_found_keys_and_omits_the_unknown_one( + python100_summer26: CourseRun, python100_winter26: CourseRun, +) -> None: + """With two keys that resolve and one that does not, the result maps exactly the two found keys to their ids.""" + assert python100_summer26.course_key is not None + assert python100_winter26.course_key is not None + unknown_key = CourseKey.from_string("course-v1:NoOrg+NoCourse+NoRun") + + result = api.get_course_run_ids([python100_summer26.course_key, unknown_key, python100_winter26.course_key]) + + assert result == { + python100_summer26.course_key: python100_summer26.id, + python100_winter26.course_key: python100_winter26.id, + } + + # sync_course_run_details diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 7a1f9274f..52f844e07 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1,6 +1,7 @@ """ Tests for the CBE public API surface (openedx_learning.api). """ +import re from collections.abc import Iterator from typing import Any @@ -12,6 +13,7 @@ from django.db.utils import IntegrityError from django.forms.models import model_to_dict from django.test.utils import CaptureQueriesContext +from opaque_keys.edx.keys import CourseKey from organizations.models import Organization from rules.permissions import permissions as rule_permissions @@ -1107,6 +1109,45 @@ def test_get_competency_criteria_tree_excludes_an_archived_group_and_its_archive assert archived_leaf.id not in returned_group_ids assert live_criterion.id in returned_criterion_ids assert archived_criterion.id not in returned_criterion_ids + assert tree.criteria_count == 1 + assert tree.total_criteria_count == 1 + + +def test_get_competency_criteria_tree_excludes_archived_rows_when_scoped_to_their_own_course( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + An archived leaf group and an archived criterion under it stay excluded even when the + request is scoped to exactly the course run they belong to, not just when unscoped. + """ + assert course_run.course_key is not None + live_leaf = _create_leaf_group(tag, course_run) + live_object_tag = ObjectTag.objects.create( + object_id=usage_key(course_run, "live"), taxonomy=tag.taxonomy, tag=tag, + ) + live_criterion = CompetencyCriterion.objects.create( + group=live_leaf, object_tag=live_object_tag, rule_profile=default_rule_profile, + ) + archived_leaf = _create_leaf_group(tag, course_run) + archived_leaf.archived = True + archived_leaf.save() + archived_object_tag = ObjectTag.objects.create( + object_id=usage_key(course_run, "archived"), taxonomy=tag.taxonomy, tag=tag, + ) + archived_criterion = CompetencyCriterion.objects.create( + group=archived_leaf, object_tag=archived_object_tag, rule_profile=default_rule_profile, archived=True, + ) + + tree = get_competency_criteria_tree(tag, [course_run.course_key]) + + returned_group_ids = {group.id for group in tree.groups} + returned_criterion_ids = {criterion.id for criterion in tree.criteria} + assert live_leaf.id in returned_group_ids + assert archived_leaf.id not in returned_group_ids + assert live_criterion.id in returned_criterion_ids + assert archived_criterion.id not in returned_criterion_ids + assert tree.criteria_count == 1 + assert tree.total_criteria_count == 1 def test_get_competency_criteria_tree_combines_groups_and_criteria_from_every_course_unscoped( @@ -1297,14 +1338,16 @@ def test_get_competency_criteria_tree_costs_a_bounded_number_of_queries( tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, django_assert_num_queries, ) -> None: """ - Reading several groups and criteria costs a fixed two queries, not one per row. + Reading several groups and criteria costs two queries unscoped and four scoped, not one per row. Pins the select_related("course")/select_related("object_tag") calls in get_competency_criteria_tree(): dropping either would still pass every other test in this module (the rows returned would be identical), since select_related only changes how many - queries fetch them, not which rows come back. + queries fetch them, not which rows come back. The two extra scoped queries resolve the course + keys to course run ids and count the criteria under every reachable group. """ leaf = _create_leaf_group(tag, course_run) + assert course_run.course_key is not None for i in range(3): object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, f"p{i}"), taxonomy=tag.taxonomy, tag=tag) CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) @@ -1316,6 +1359,395 @@ def test_get_competency_criteria_tree_costs_a_bounded_number_of_queries( for criterion in tree.criteria: _ = criterion.object_tag + with django_assert_num_queries(4): + tree = get_competency_criteria_tree(tag, [course_run.course_key]) + for group in tree.groups: + _ = group.course + for criterion in tree.criteria: + _ = criterion.object_tag + + +def test_get_competency_criteria_tree_scoped_fetches_only_the_scoped_criteria_and_counts_the_rest( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A scoped read fetches criteria rows only for the groups it returns, and gets total_criteria_count + from a COUNT, so criteria of a course that was not requested are never loaded. + """ + other_run = make_course_run(organization, "Python200", "Fall2026") + leaf = _create_leaf_group(tag, course_run) + other_leaf = _create_leaf_group(tag, other_run) + for run, group in ((course_run, leaf), (other_run, other_leaf)): + object_tag = ObjectTag.objects.create(object_id=usage_key(run, "p1"), taxonomy=tag.taxonomy, tag=tag) + CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + assert course_run.course_key is not None + assert other_leaf.parent is not None + + with CaptureQueriesContext(connection) as queries: + tree = get_competency_criteria_tree(tag, [course_run.course_key]) + + criterion_queries = [q["sql"] for q in queries.captured_queries if "competencycriterion" in q["sql"].lower()] + selects = [sql for sql in criterion_queries if "COUNT(" not in sql.upper()] + counts = [sql for sql in criterion_queries if "COUNT(" in sql.upper()] + assert len(selects) == 1 and len(counts) == 1 + in_list = re.search(r"IN \(([^)]*)\)", selects[0]) + assert in_list is not None + selected_group_ids = {int(i) for i in re.findall(r"\d+", in_list.group(1))} + assert other_leaf.id not in selected_group_ids + assert other_leaf.parent.id not in selected_group_ids + assert tree.criteria_count == 1 + assert tree.total_criteria_count == 2 + + +def test_get_competency_criteria_tree_costs_one_query_when_the_competency_has_no_groups( + tag: Tag, course_run: CourseRun, django_assert_num_queries, +) -> None: + """A competency with no groups skips the criteria query and, when scoped, the course run lookup.""" + assert course_run.course_key is not None + with django_assert_num_queries(1): + get_competency_criteria_tree(tag, [course_run.course_key]) + + +# ============================================================================================== +# get_competency_criteria_tree: course_keys scoping +# ============================================================================================== + + +def test_get_competency_criteria_tree_course_keys_none_matches_unfiltered_behavior( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """course_keys=None is unfiltered, identical to omitting it, and criteria_count equals total_criteria_count.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + leaf1 = _create_leaf_group(tag, course_run) + leaf2 = _create_leaf_group(tag, other_course_run) + object_tag1 = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + object_tag2 = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, + ) + CompetencyCriterion.objects.create(group=leaf1, object_tag=object_tag1, rule_profile=default_rule_profile) + CompetencyCriterion.objects.create(group=leaf2, object_tag=object_tag2, rule_profile=default_rule_profile) + + unfiltered = get_competency_criteria_tree(tag) + explicit_none = get_competency_criteria_tree(tag, None) + + assert {g.id for g in explicit_none.groups} == {g.id for g in unfiltered.groups} + assert {c.id for c in explicit_none.criteria} == {c.id for c in unfiltered.criteria} + assert explicit_none.criteria_count == len(explicit_none.criteria) == 2 + assert explicit_none.criteria_count == explicit_none.total_criteria_count + + +def test_get_competency_criteria_tree_course_keys_empty_list_returns_only_the_root( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """course_keys=[] scopes to the instance-wide subtree only: the root, no course-level or leaf groups.""" + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + 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) + + tree = get_competency_criteria_tree(tag, []) + + assert {g.id for g in tree.groups} == {root.id} + assert not tree.criteria + assert tree.criteria_count == 0 + assert tree.total_criteria_count == 1 + + +def test_get_competency_criteria_tree_course_keys_scoped_to_one_course( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Scoping to course A's key returns only A's subtree and criteria, nothing from course B.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + leaf_a = _create_leaf_group(tag, course_run) + leaf_b = _create_leaf_group(tag, other_course_run) + object_tag_a = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + object_tag_b = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, + ) + criterion_a = CompetencyCriterion.objects.create( + group=leaf_a, object_tag=object_tag_a, rule_profile=default_rule_profile, + ) + CompetencyCriterion.objects.create(group=leaf_b, object_tag=object_tag_b, rule_profile=default_rule_profile) + course_level_a = leaf_a.parent + assert course_level_a is not None + root = course_level_a.parent + assert root is not None + assert course_run.course_key is not None + + tree = get_competency_criteria_tree(tag, [course_run.course_key]) + + assert {g.id for g in tree.groups} == {root.id, course_level_a.id, leaf_a.id} + assert [c.id for c in tree.criteria] == [criterion_a.id] + assert tree.criteria_count == 1 + assert tree.total_criteria_count == 2 + + +def test_get_competency_criteria_tree_course_keys_preserve_request_order( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """Requesting [B, A] sorts B's groups/criteria before A's; requesting [A, B] reverses the order.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + leaf_a = _create_leaf_group(tag, course_run) + leaf_b = _create_leaf_group(tag, other_course_run) + object_tag_a = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + object_tag_b = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, + ) + criterion_a = CompetencyCriterion.objects.create( + group=leaf_a, object_tag=object_tag_a, rule_profile=default_rule_profile, + ) + criterion_b = CompetencyCriterion.objects.create( + group=leaf_b, object_tag=object_tag_b, rule_profile=default_rule_profile, + ) + assert course_run.course_key is not None + assert other_course_run.course_key is not None + a_key = course_run.course_key + b_key = other_course_run.course_key + a_group_ids = {leaf_a.id, leaf_a.parent_id} + b_group_ids = {leaf_b.id, leaf_b.parent_id} + + tree_b_first = get_competency_criteria_tree(tag, [b_key, a_key]) + b_positions = [i for i, g in enumerate(tree_b_first.groups) if g.id in b_group_ids] + a_positions = [i for i, g in enumerate(tree_b_first.groups) if g.id in a_group_ids] + assert max(b_positions) < min(a_positions) + assert [c.id for c in tree_b_first.criteria] == [criterion_b.id, criterion_a.id] + + tree_a_first = get_competency_criteria_tree(tag, [a_key, b_key]) + a_positions2 = [i for i, g in enumerate(tree_a_first.groups) if g.id in a_group_ids] + b_positions2 = [i for i, g in enumerate(tree_a_first.groups) if g.id in b_group_ids] + assert max(a_positions2) < min(b_positions2) + assert [c.id for c in tree_a_first.criteria] == [criterion_a.id, criterion_b.id] + + +def test_get_competency_criteria_tree_course_keys_unresolvable_key_is_silently_ignored( + tag: Tag, course_run: CourseRun, +) -> None: + """A well-formed course key with no matching CourseRun is dropped: no error, no contribution to the result.""" + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + unresolvable_key = CourseKey.from_string("course-v1:NoOrg+NoCourse+NoRun") + + tree = get_competency_criteria_tree(tag, [unresolvable_key]) + + assert {g.id for g in tree.groups} == {root.id} + assert not tree.criteria + assert tree.criteria_count == 0 + + +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: + """ + Criteria exist under courses A and B; scoping to a real course C with nothing under it + returns zero criteria but still reports the true instance-wide total. + """ + other_course_run = make_course_run(organization, "Python200", "Fall2026") + empty_course_run = make_course_run(organization, "Python300", "Fall2026") + leaf_a = _create_leaf_group(tag, course_run) + leaf_b = _create_leaf_group(tag, other_course_run) + object_tag_a = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + object_tag_b = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, + ) + CompetencyCriterion.objects.create(group=leaf_a, object_tag=object_tag_a, rule_profile=default_rule_profile) + CompetencyCriterion.objects.create(group=leaf_b, object_tag=object_tag_b, rule_profile=default_rule_profile) + assert empty_course_run.course_key is not None + + tree = get_competency_criteria_tree(tag, [empty_course_run.course_key]) + + assert not tree.criteria + assert tree.criteria_count == 0 + assert tree.total_criteria_count == 2 + + +def test_get_competency_criteria_tree_scoped_prunes_a_leaf_whose_course_level_parent_is_archived( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A non-archived leaf under an archived course-level parent is absent, even when its own course is + requested: the tree never returns a row whose ancestor is archived. Its criterion is also left + out of both counts. + """ + other_course_run = make_course_run(organization, "Python200", "Fall2026") + leaf_archived_parent = _create_leaf_group(tag, course_run) + course_level_group = leaf_archived_parent.parent + assert course_level_group is not None + course_level_group.archived = True + course_level_group.save() + leaf_other = _create_leaf_group(tag, other_course_run) + object_tag_archived_parent = ObjectTag.objects.create( + object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, + ) + object_tag_other = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "p1"), taxonomy=tag.taxonomy, tag=tag, + ) + CompetencyCriterion.objects.create( + group=leaf_archived_parent, object_tag=object_tag_archived_parent, rule_profile=default_rule_profile, + ) + criterion_other = CompetencyCriterion.objects.create( + group=leaf_other, object_tag=object_tag_other, rule_profile=default_rule_profile, + ) + assert course_run.course_key is not None + assert other_course_run.course_key is not None + + tree = get_competency_criteria_tree(tag, [other_course_run.course_key, course_run.course_key]) + + assert leaf_archived_parent.id not in {g.id for g in tree.groups} + assert [c.id for c in tree.criteria] == [criterion_other.id] + assert tree.criteria_count == 1 + assert tree.total_criteria_count == 1 + + +def test_get_competency_criteria_tree_scopes_a_branch_three_levels_below_its_course( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A group nested below the leaf level belongs to its course-level group's course run, so scoping + to that course keeps it and scoping to another course drops it. + """ + other_course_run = make_course_run(organization, "Python200", "Fall2026") + leaf = _create_leaf_group(tag, course_run) + deep_group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=leaf) + deeper_group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=deep_group) + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + criterion = CompetencyCriterion.objects.create( + group=deeper_group, object_tag=object_tag, rule_profile=default_rule_profile, + ) + assert course_run.course_key is not None + assert other_course_run.course_key is not None + + scoped = get_competency_criteria_tree(tag, [course_run.course_key]) + other = get_competency_criteria_tree(tag, [other_course_run.course_key]) + + assert {deep_group.id, deeper_group.id} <= {g.id for g in scoped.groups} + assert [c.id for c in scoped.criteria] == [criterion.id] + assert not {deep_group.id, deeper_group.id} & {g.id for g in other.groups} + assert not other.criteria + assert other.total_criteria_count == 1 + + +def test_get_competency_criteria_tree_handles_duplicate_and_unresolvable_keys_together( + tag: Tag, course_run: CourseRun, organization: Organization, +) -> None: + """A repeated key and an unresolvable key in one request leave roots, then a's branch, then b's, once each.""" + run_b = make_course_run(organization, "Python200", "Fall2026") + leaf_a = _create_leaf_group(tag, course_run) + leaf_b = _create_leaf_group(tag, run_b) + assert course_run.course_key is not None + assert run_b.course_key is not None + unknown = CourseKey.from_string("course-v1:NoOrg+NoCourse+NoRun") + + tree = get_competency_criteria_tree(tag, [course_run.course_key, unknown, run_b.course_key, course_run.course_key]) + + course_level_a = leaf_a.parent + course_level_b = leaf_b.parent + assert course_level_a is not None + assert course_level_b is not None + root = course_level_a.parent + assert root is not None + assert [g.id for g in tree.groups] == [root.id, course_level_a.id, leaf_a.id, course_level_b.id, leaf_b.id] + + +def test_total_criteria_count_excludes_criteria_under_an_archived_ancestor( + tag: Tag, course_run: CourseRun, organization: Organization, default_rule_profile: CompetencyRuleProfile, +) -> None: + """total_criteria_count counts what the unscoped tree returns, leaving out criteria under an archived ancestor.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + live_leaf = _create_leaf_group(tag, course_run) + hidden_leaf = _create_leaf_group(tag, other_course_run) + hidden_course_level = hidden_leaf.parent + assert hidden_course_level is not None + hidden_course_level.archived = True + hidden_course_level.save() + live_object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "live"), taxonomy=tag.taxonomy, tag=tag) + hidden_object_tag = ObjectTag.objects.create( + object_id=usage_key(other_course_run, "hidden"), taxonomy=tag.taxonomy, tag=tag, + ) + CompetencyCriterion.objects.create(group=live_leaf, object_tag=live_object_tag, rule_profile=default_rule_profile) + CompetencyCriterion.objects.create( + group=hidden_leaf, object_tag=hidden_object_tag, rule_profile=default_rule_profile, + ) + assert course_run.course_key is not None + + scoped = get_competency_criteria_tree(tag, [course_run.course_key]) + empty_scope = get_competency_criteria_tree(tag, []) + + assert scoped.total_criteria_count == 1 + assert empty_scope.total_criteria_count == 1 + assert empty_scope.criteria_count == 0 + + +def test_get_competency_criteria_tree_zero_criteria_anywhere(tag: Tag) -> None: + """A competency with no criteria at all reports both counts as zero, with or without a course_keys scope.""" + tree_unfiltered = get_competency_criteria_tree(tag) + tree_scoped = get_competency_criteria_tree(tag, []) + + assert tree_unfiltered.criteria_count == 0 + assert tree_unfiltered.total_criteria_count == 0 + assert tree_scoped.criteria_count == 0 + assert tree_scoped.total_criteria_count == 0 + + +def test_get_competency_criteria_tree_a_different_tag_never_leaks_into_this_ones_counts( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A second, independent competency tag with its own criteria never contributes to this tag's + returned rows or counts. + + This test file's fixtures have no modeled "sub-competency" relationship distinct from an + ordinary Tag; two unrelated tags are the closest analogue available, and isolation here is + already guaranteed structurally by filtering the group query on tag_id. + """ + assert tag.taxonomy is not None + other_tag = Tag.objects.create(taxonomy=tag.taxonomy, value="Other Competency") + leaf = _create_leaf_group(tag, course_run) + other_leaf = _create_leaf_group(other_tag, course_run) + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + other_object_tag = ObjectTag.objects.create( + object_id=usage_key(course_run, "p2"), taxonomy=tag.taxonomy, tag=other_tag, + ) + criterion = CompetencyCriterion.objects.create( + group=leaf, object_tag=object_tag, rule_profile=default_rule_profile, + ) + CompetencyCriterion.objects.create(group=other_leaf, object_tag=other_object_tag, rule_profile=default_rule_profile) + + tree = get_competency_criteria_tree(tag) + + assert [c.id for c in tree.criteria] == [criterion.id] + assert tree.criteria_count == 1 + assert tree.total_criteria_count == 1 + + +def test_get_competency_criteria_tree_scoped_keeps_every_group_above_any_course( + tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A group with no course on its path stays in a scoped result even when it is not a root, so no + returned parent_id names a group that was left out. Scoping to [] keeps only those groups. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, parent=None) + mid = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + course_level = CompetencyCriteriaGroup.objects.create(tag=tag, parent=mid, course=course_run) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=course_level) + 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) + assert course_run.course_key is not None + + scoped = get_competency_criteria_tree(tag, [course_run.course_key]) + empty = get_competency_criteria_tree(tag, []) + + scoped_ids = {g.id for g in scoped.groups} + assert scoped_ids == {root.id, mid.id, course_level.id, leaf.id} + assert all(g.parent_id is None or g.parent_id in scoped_ids for g in scoped.groups) + assert {g.id for g in empty.groups} == {root.id, mid.id} + def test_get_courses_with_relative_criteria_single_match( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 99eb2f5da..be957f857 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -5,6 +5,7 @@ directly rather than through a live call, because no create or archive endpoint exists yet. """ from typing import Any, Generator +from urllib.parse import urlencode import pytest import rules @@ -1347,6 +1348,180 @@ def test_view_checks_the_view_taxonomy_rule_not_view_tag(user_client: APIClient, assert response.status_code == status.HTTP_403_FORBIDDEN +# course_keys scoping + + +def test_course_keys_scopes_and_orders_the_response_over_a_real_request( + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A real GET request scoped to two of three course runs returns only those two courses' + groups and criteria, in the order requested, with nothing from the third course run. This + covers the scoping and ordering behavior end to end (view, permission, serializers), not + just at the get_competency_criteria_tree() function level. + """ + course_b = make_course_run(organization, "Python200", "Fall2026") + course_c = make_course_run(organization, "Python300", "Fall2026") + leaf_a = _create_leaf_group(tag, course_run) + leaf_b = _create_leaf_group(tag, course_b) + leaf_c = _create_leaf_group(tag, course_c) + for leaf, run, name in [(leaf_a, course_run, "a"), (leaf_b, course_b, "b"), (leaf_c, course_c, "c")]: + object_tag = ObjectTag.objects.create(object_id=usage_key(run, name), taxonomy=tag.taxonomy, tag=tag) + CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) + assert course_run.course_key is not None + assert course_b.course_key is not None + b_group_ids = {leaf_b.id, leaf_b.parent_id} + a_group_ids = {leaf_a.id, leaf_a.parent_id} + c_group_ids = {leaf_c.id, leaf_c.parent_id} + + response = user_client.get( + criteria_tree_url(tag.id), {"course_keys": f"{course_b.course_key},{course_run.course_key}"}, + ) + + assert response.status_code == status.HTTP_200_OK + returned_group_ids = {group["id"] for group in response.data["groups"]} + assert not returned_group_ids & c_group_ids + b_positions = [i for i, g in enumerate(response.data["groups"]) if g["id"] in b_group_ids] + a_positions = [i for i, g in enumerate(response.data["groups"]) if g["id"] in a_group_ids] + assert max(b_positions) < min(a_positions) + assert response.data["criteria_count"] == 2 + assert response.data["total_criteria_count"] == 3 + + +def test_course_keys_with_a_trailing_comma_matches_the_clean_list( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A trailing comma in course_keys is ignored: the response is identical to the clean list.""" + 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) + assert course_run.course_key is not None + key = str(course_run.course_key) + + clean = user_client.get(criteria_tree_url(tag.id), {"course_keys": key}) + trailing_comma = user_client.get(criteria_tree_url(tag.id), {"course_keys": f"{key},"}) + + assert clean.status_code == trailing_comma.status_code == status.HTTP_200_OK + assert trailing_comma.data == clean.data + + +def test_course_keys_with_a_doubled_comma_matches_the_clean_list( + user_client: APIClient, tag: Tag, course_run: CourseRun, organization: Organization, +) -> None: + """A doubled comma between two course_keys is ignored: the response is identical to the clean list.""" + other_course_run = make_course_run(organization, "Python200", "Fall2026") + _create_leaf_group(tag, course_run) + _create_leaf_group(tag, other_course_run) + assert course_run.course_key is not None + assert other_course_run.course_key is not None + key_a = str(course_run.course_key) + key_b = str(other_course_run.course_key) + + clean = user_client.get(criteria_tree_url(tag.id), {"course_keys": f"{key_a},{key_b}"}) + doubled_comma = user_client.get(criteria_tree_url(tag.id), {"course_keys": f"{key_a},,{key_b}"}) + + assert clean.status_code == doubled_comma.status_code == status.HTTP_200_OK + assert doubled_comma.data == clean.data + + +def test_course_keys_of_only_commas_behaves_like_an_empty_scope( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course_keys value of only commas behaves like an omitted/empty scope: only the root group.""" + leaf = _create_leaf_group(tag, course_run) + course_level = leaf.parent + assert course_level is not None + root = course_level.parent + assert root is not None + 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) + + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": ",,,"}) + + assert response.status_code == status.HTTP_200_OK + assert {group["id"] for group in response.data["groups"]} == {root.id} + assert response.data["criteria"] == [] + assert response.data["criteria_count"] == 0 + assert response.data["total_criteria_count"] == 1 + + +def test_a_malformed_course_key_is_400(user_client: APIClient, tag: Tag) -> None: + """An entry in course_keys that isn't a valid course key 400s, with the error keyed by course_keys.""" + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": "not-a-course-key"}) + + 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)) + + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": too_many}) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert set(response.data) == {"course_keys"} + assert "100" in str(response.data) + + +def test_exactly_the_limit_of_course_keys_is_200(user_client: APIClient, tag: Tag) -> None: + """Exactly 100 distinct, well-formed course_keys entries is accepted.""" + at_limit = ",".join(f"course-v1:Org+Course{i}+Run" for i in range(100)) + + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": at_limit}) + + assert response.status_code == status.HTTP_200_OK + + +def test_repeating_one_course_key_past_the_limit_is_400(user_client: APIClient, tag: Tag) -> None: + """101 copies of one key 400s: the cap counts entries before duplicates are removed.""" + repeated = ",".join(["course-v1:Org+Course+Run"] * 101) + + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": repeated}) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert set(response.data) == {"course_keys"} + + +def test_a_repeated_course_keys_query_parameter_is_400(user_client: APIClient, tag: Tag) -> None: + """Supplying course_keys twice 400s, keyed by course_keys, instead of silently using the last value.""" + query = urlencode({"course_keys": ["course-v1:Org+A+Run", "course-v1:Org+B+Run"]}, doseq=True) + + response = user_client.get(f"{criteria_tree_url(tag.id)}?{query}") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert set(response.data) == {"course_keys"} + + +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: + """ + A 403 for missing view access discloses neither array nor either count, even with garbage + in course_keys: permission is checked before course_keys is parsed. + """ + 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) + taxonomy = tag.taxonomy + assert taxonomy is not None + taxonomy.enabled = False + taxonomy.save() + api_client.force_authenticate(user=user) + + response = api_client.get(criteria_tree_url(tag.id), {"course_keys": "not-a-course-key"}) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "groups" not in response.data + assert "criteria" not in response.data + assert "criteria_count" not in response.data + assert "total_criteria_count" not in response.data + + 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: