From 2509afba2193d4c7e30252fa142e6a739b6b9e9d Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Tue, 20 Oct 2026 19:04:36 +0400 Subject: [PATCH 1/9] feat: add blocked-courses endpoint for a competency Adds GET api/cbe/v1/competencies//blocked-courses/, which lists the courses where a taxonomy ancestor or descendant of the competency already has a criterion, so authoring can gray them out before the create-criterion endpoint rejects them. An optional comma-separated course_keys parameter narrows the list, and the result is paginated. get_blocked_courses() reuses get_courses_with_relative_criteria(), so this read and the write-side check always agree. CompetencyReadPermission is copied verbatim from the criteria-groups endpoint's PR. Related to openedx/openedx-core#796 Co-Authored-By: Claude Opus 5.5 --- src/openedx_learning/applets/cbe/api.py | 36 ++ .../applets/cbe/rest_api/paginators.py | 9 + .../applets/cbe/rest_api/v1/serializers.py | 36 ++ .../applets/cbe/rest_api/v1/urls.py | 5 + .../applets/cbe/rest_api/v1/views.py | 37 +- .../openedx_learning/applets/cbe/test_api.py | 171 ++++++++- .../applets/cbe/test_views.py | 343 ++++++++++++++++++ 7 files changed, 635 insertions(+), 2 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 29205bb0d..2ef231863 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -47,6 +47,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", @@ -364,6 +365,41 @@ def get_courses_with_relative_criteria( return result +@dataclass +class BlockedCourse: + """A course where a taxonomy relative of a competency already has a live CompetencyCriterion.""" + + course_key: CourseKey + conflicting_tag_id: int + conflicting_tag_value: str + + +def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None) -> list[BlockedCourse]: + """ + Return the courses where ``tag`` can't get a new criterion, because an ancestor or descendant has one there. + + 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, and sorted by course key. + """ + relative_ids = ( + set(get_ancestor_tags(tag).values_list("id", flat=True)) + | set(get_descendant_tags(tag).values_list("id", flat=True)) + ) + if not relative_ids: + return [] + conflicts = get_courses_with_relative_criteria(relative_ids) + requested = set(course_keys) if course_keys is not None else None + 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 + if requested is None or course_run.course_key in requested: + rows.append(BlockedCourse(course_run.course_key, conflicting_tag.id, 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. diff --git a/src/openedx_learning/applets/cbe/rest_api/paginators.py b/src/openedx_learning/applets/cbe/rest_api/paginators.py index ea23f67c9..4912bd1e2 100644 --- a/src/openedx_learning/applets/cbe/rest_api/paginators.py +++ b/src/openedx_learning/applets/cbe/rest_api/paginators.py @@ -18,3 +18,12 @@ class CompetencyRuleProfilePagination(DefaultPagination): page_size = 100 max_page_size = 500 + + +class CompetencyBlockedCoursesPagination(DefaultPagination): + """ + Page size for a competency's blocked-courses collection, pinned for the same reason as the rule profiles'. + """ + + 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..fa078df54 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -235,3 +235,39 @@ class Meta: "id", "group_id", "object_tag_id", "object_id", "rule_profile_id", "rule_type_override", "rule_payload_override", ] + + +class BlockedCourseSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + 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) + + +class CompetencyBlockedCoursesQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Query params for the blocked-courses endpoint: an optional comma-separated ``course_keys`` filter. + """ + + course_keys = serializers.CharField(required=False) + + def validate_course_keys(self, value: str) -> list[CourseKey]: + """Parse the comma-separated course keys, rejecting a filter that names none or an unparseable one.""" + pieces = [piece.strip() for piece in value.split(",") if piece.strip()] + if not pieces: + raise serializers.ValidationError(_("course_keys must name at least one course key.")) + course_keys = [] + for piece in pieces: + try: + course_keys.append(CourseKey.from_string(piece)) + except InvalidKeyError as exc: + raise serializers.ValidationError( + _("'{course_key}' is not a valid course key.").format(course_key=piece) + ) from exc + return course_keys 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..2fcc55f21 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,18 @@ 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, + CompetencyBlockedCoursesQueryParamsSerializer, CompetencyCriteriaGroupSerializer, CompetencyCriteriaGroupUpdateSerializer, CompetencyCriteriaQueryParamsSerializer, @@ -273,3 +276,35 @@ 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 a competency can't get a new criterion, paginated. + + ``GET api/cbe/v1/competencies//blocked-courses/`` returns every course where a + taxonomy ancestor or descendant of the competency already has a live criterion, sorted by + course key; ``course_keys`` (comma-separated) narrows it. The result is a hint for authoring: + the create-criterion endpoint still enforces the rule. 404 if ``tag_id`` isn't a competency + tag, 403 if the caller can't view its taxonomy, 400 for a malformed ``course_keys``. + + UNSTABLE: the criteria-authoring family is incomplete, so this 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) + params = CompetencyBlockedCoursesQueryParamsSerializer(data=request.query_params.dict()) + params.is_valid(raise_exception=True) + rows = get_blocked_courses(tag, course_keys=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..0fd27686b 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -25,6 +25,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, @@ -34,7 +35,7 @@ update_competency_criteria_group, ) from openedx_learning.applets.cbe import api as cbe_api -from openedx_learning.applets.cbe.api import _create_leaf_group +from openedx_learning.applets.cbe.api import BlockedCourse, _create_leaf_group from openedx_learning.models import ( CompetencyCourseRun, CompetencyCriteriaGroup, @@ -105,6 +106,13 @@ def make_course_run(organization: Organization, course_code: str, run_code: str) return CourseRun.objects.create(catalog_course=catalog_course, run_code=run_code) +def 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 key + + 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 @@ -2302,3 +2310,164 @@ 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 +# ============================================================================================== + + +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 = make_tag(taxonomy, "Cognitive Skills") + critical_thinking = make_tag(taxonomy, "Critical Thinking", parent=cognitive_skills) + inference = make_tag(taxonomy, "Inference", parent=critical_thinking) + analysis = make_tag(taxonomy, "Analysis", parent=critical_thinking) + return cognitive_skills, critical_thinking, inference, analysis + + +def test_get_blocked_courses_reports_an_ancestor_claim( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course where an ancestor has a criterion is blocked, naming that ancestor.""" + _, 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_run), critical_thinking.id, "Critical Thinking"), + ] + + +def test_get_blocked_courses_reports_a_descendant_claim( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course where a descendant has a criterion is blocked, naming that descendant.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(inference, course_run, default_rule_profile, "p1") + + assert get_blocked_courses(critical_thinking) == [ + BlockedCourse(course_key(course_run), inference.id, "Inference"), + ] + + +def test_get_blocked_courses_reports_a_grandparent_claim( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """The block reaches past the immediate parent to a grandparent.""" + cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(cognitive_skills, course_run, default_rule_profile, "p1") + + assert get_blocked_courses(inference) == [ + BlockedCourse(course_key(course_run), cognitive_skills.id, "Cognitive Skills"), + ] + + +def test_get_blocked_courses_reports_a_grandchild_claim( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """The block reaches past the immediate child to a grandchild.""" + cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(inference, course_run, default_rule_profile, "p1") + + assert get_blocked_courses(cognitive_skills) == [ + BlockedCourse(course_key(course_run), inference.id, "Inference"), + ] + + +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_course_with_no_relative_claim( + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Only the course holding a relative's criterion is reported.""" + _, 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") + + assert [row.course_key for row in get_blocked_courses(inference)] == [course_run.course_key] + + +def test_get_blocked_courses_is_empty_when_no_relative_has_a_claim( + competency_taxonomy: CompetencyTaxonomy, +) -> None: + """A tag whose ancestors and descendants have no criteria anywhere blocks no course.""" + _, critical_thinking, _, _ = make_skill_family(competency_taxonomy) + + assert get_blocked_courses(critical_thinking) == [] + + +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_reports_one_row_per_course_naming_the_lowest_id_relative( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course claimed by two relatives is reported once, naming the lower-id relative.""" + cognitive_skills, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + assert cognitive_skills.id < critical_thinking.id + make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + make_leaf_criterion(cognitive_skills, course_run, default_rule_profile, "p2") + + assert get_blocked_courses(inference) == [ + BlockedCourse(course_key(course_run), cognitive_skills.id, "Cognitive Skills"), + ] + + +def test_get_blocked_courses_narrows_to_the_requested_course_keys( + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + organization: Organization, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """Only the requested courses come back, even when others are 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") + + rows = get_blocked_courses(inference, course_keys=[course_key(course_run), course_key(course_y)]) + + assert {row.course_key for row in rows} == {course_run.course_key, course_y.course_key} + + +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 == sorted(keys) + assert len(keys) == 3 + + +def test_get_blocked_courses_ignores_an_archived_claim( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """An archived criterion on a relative blocks nothing.""" + _, 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) + + assert get_blocked_courses(inference) == [] diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index be957f857..dc9c0a277 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -1993,3 +1993,346 @@ 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 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 collect_pages(client: APIClient, url: str) -> list[dict]: + """Follow `next` links from `url` and return every row across all pages.""" + rows: list[dict] = [] + next_url: str | None = url + while next_url: + response = client.get(next_url) + assert response.status_code == status.HTTP_200_OK + rows.extend(response.data["results"]) + next_url = response.data["next"] + return rows + + +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 = make_tag(taxonomy, "Cognitive Skills") + critical_thinking = make_tag(taxonomy, "Critical Thinking", parent=cognitive_skills) + inference = make_tag(taxonomy, "Inference", parent=critical_thinking) + analysis = make_tag(taxonomy, "Analysis", parent=critical_thinking) + return cognitive_skills, critical_thinking, inference, analysis + + +def test_blocked_courses_reports_an_ancestor_claim( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course where an ancestor has a criterion is reported, with the conflicting tag's id and value.""" + _, 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 response.data["results"] == [{ + "course_key": str(course_run.course_key), + "conflicting_tag_id": critical_thinking.id, + "conflicting_tag_value": "Critical Thinking", + }] + + +def test_blocked_courses_reports_a_descendant_claim( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course where a descendant has a criterion is reported, naming that descendant.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(inference, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(critical_thinking.id)) + + assert response.status_code == status.HTTP_200_OK + assert [row["course_key"] for row in response.data["results"]] == [str(course_run.course_key)] + assert response.data["results"][0]["conflicting_tag_value"] == "Inference" + + +def test_blocked_courses_reports_a_grandparent_claim( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """The block reaches a grandparent, and the grandparent is the tag reported.""" + cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(cognitive_skills, 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)] + assert response.data["results"][0]["conflicting_tag_value"] == "Cognitive Skills" + + +def test_blocked_courses_reports_a_grandchild_claim( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """The block reaches a grandchild, and the grandchild is the tag reported.""" + cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) + make_leaf_criterion(inference, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(cognitive_skills.id)) + + assert response.status_code == status.HTTP_200_OK + assert [row["course_key"] for row in response.data["results"]] == [str(course_run.course_key)] + assert response.data["results"][0]["conflicting_tag_value"] == "Inference" + + +def test_blocked_courses_ignores_a_sibling_claim( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course claimed only by a sibling tag is not reported.""" + _, _, inference, analysis = make_skill_family(competency_taxonomy) + make_leaf_criterion(analysis, course_run, default_rule_profile, "p1") + + response = user_client.get(blocked_courses_url(inference.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data["results"] == [] + + +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) + course_y = 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 + returned_keys = [row["course_key"] for row in response.data["results"]] + assert str(course_y.course_key) not in returned_keys + assert returned_keys == [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.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + expected_keys = set() + for course_code in ("A100", "B200", "C300"): + course = make_course_run(organization, course_code, "Run1") + make_leaf_criterion(critical_thinking, course, default_rule_profile, "p1") + expected_keys.add(str(course.course_key)) + url = blocked_courses_url(inference.id, page_size="2") + + first_page = user_client.get(url) + rows = collect_pages(user_client, url) + + assert first_page.data["count"] == 3 + assert len(first_page.data["results"]) == 2 + assert first_page.data["next"] is not None + keys = [row["course_key"] for row in rows] + assert len(keys) == len(set(keys)) == 3 + assert set(keys) == expected_keys + + +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, and nothing outside them.""" + _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) + requested_keys = [] + for course_code in ("A100", "B200", "C300", "D400"): + course = make_course_run(organization, course_code, "Run1") + make_leaf_criterion(critical_thinking, course, default_rule_profile, "p1") + requested_keys.append(str(course.course_key)) + outside_key = requested_keys.pop(1) + url = blocked_courses_url(inference.id, course_keys=",".join(requested_keys), page_size="2") + + first_page = user_client.get(url) + rows = collect_pages(user_client, url) + + assert first_page.data["count"] == 3 + assert len(first_page.data["results"]) == 2 + keys = [row["course_key"] for row in rows] + assert len(keys) == len(set(keys)) == 3 + assert set(keys) == set(requested_keys) + assert outside_key not in keys + + +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.""" + _, 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)) + + assert response.status_code == status.HTTP_403_FORBIDDEN + 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_empty_course_keys_400s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_keys: str, +) -> None: + """A course_keys filter that names no course is rejected rather than ignored.""" + _, _, inference, _ = make_skill_family(competency_taxonomy) + + response = user_client.get(blocked_courses_url(inference.id, course_keys=course_keys)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "course_keys" in response.data + + +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"] == [] From d0b7469d9f4c2bdad7d842b9f98cd2a43dec36bc Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Thu, 5 Nov 2026 23:04:33 +0400 Subject: [PATCH 2/9] fix: address review feedback on the blocked-courses endpoint The view reuses CompetencyCriteriaQueryParamsSerializer, so course_keys follows one contract on both read endpoints: supplied once, at most 100 keys, blank means an empty page. Each parsed key is normalized to its plain course key, so a branch- or version-qualified key matches on both endpoints. get_courses_with_relative_criteria takes an optional course_keys argument and the filter runs in the database. BlockedCourse is frozen, keyword-only, exported on openedx_learning.api and marked UNSTABLE. Docstrings state the rule the endpoint reports on and every status code it returns. Tests: anonymous 401, repeated parameter, key cap, blank filter, branch qualified key, version-only key, own-criterion case, and ordered pagination. The direction scenarios are parametrized and the shared helpers live in a utils module. Related to openedx/openedx-core#796 Co-Authored-By: Claude Fable 5.1 --- src/openedx_learning/applets/cbe/api.py | 35 ++- .../applets/cbe/rest_api/paginators.py | 6 +- .../applets/cbe/rest_api/v1/serializers.py | 43 ++- .../applets/cbe/rest_api/v1/views.py | 31 ++- .../openedx_learning/applets/cbe/test_api.py | 174 ++++-------- .../applets/cbe/test_views.py | 261 ++++++++++-------- tests/openedx_learning/applets/cbe/utils.py | 32 +++ 7 files changed, 301 insertions(+), 281 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/utils.py diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 2ef231863..c2e51d5bb 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -39,6 +39,7 @@ from .statuses import raise_status __all__ = [ + "BlockedCourse", "CompetencyCriteriaGroupArchivedError", "CompetencyCriteriaTree", "CompetencyCriterionArchivedError", @@ -338,13 +339,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, @@ -357,6 +359,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" @@ -365,9 +369,14 @@ def get_courses_with_relative_criteria( return result -@dataclass +@dataclass(frozen=True, kw_only=True, slots=True) class BlockedCourse: - """A course where a taxonomy relative of a competency already has a live CompetencyCriterion.""" + """ + A course where a taxonomy relative 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 @@ -376,12 +385,14 @@ class BlockedCourse: def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None) -> list[BlockedCourse]: """ - Return the courses where ``tag`` can't get a new criterion, because an ancestor or descendant has one there. + 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, and sorted by course key. + Narrowed to ``course_keys`` when given, and 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. """ relative_ids = ( set(get_ancestor_tags(tag).values_list("id", flat=True)) @@ -389,14 +400,16 @@ def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None ) if not relative_ids: return [] - conflicts = get_courses_with_relative_criteria(relative_ids) - requested = set(course_keys) if course_keys is not None else None + 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 - if requested is None or course_run.course_key in requested: - rows.append(BlockedCourse(course_run.course_key, conflicting_tag.id, conflicting_tag.value)) + 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)) diff --git a/src/openedx_learning/applets/cbe/rest_api/paginators.py b/src/openedx_learning/applets/cbe/rest_api/paginators.py index 4912bd1e2..8d84e2125 100644 --- a/src/openedx_learning/applets/cbe/rest_api/paginators.py +++ b/src/openedx_learning/applets/cbe/rest_api/paginators.py @@ -22,7 +22,11 @@ class CompetencyRuleProfilePagination(DefaultPagination): class CompetencyBlockedCoursesPagination(DefaultPagination): """ - Page size for a competency's blocked-courses collection, pinned for the same reason as the rule profiles'. + 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 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 fa078df54..47b58f9d7 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, LibraryLocator from rest_framework import serializers from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator @@ -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,15 @@ 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) + # CourseKey's type lacks for_branch(); mypy needs the narrowing to the two base locators that define it. + if isinstance(key, (CourseLocator, LibraryLocator)): + key = key.for_branch(None).version_agnostic() 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): @@ -237,7 +243,7 @@ class Meta: ] -class BlockedCourseSerializer(serializers.Serializer): # pylint: disable=abstract-method +class BlockedCourseSerializer(serializers.Serializer): """ Read-only representation of a BlockedCourse, for the blocked-courses endpoint. @@ -249,25 +255,10 @@ class BlockedCourseSerializer(serializers.Serializer): # pylint: disable=abstra 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 -class CompetencyBlockedCoursesQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method - """ - Query params for the blocked-courses endpoint: an optional comma-separated ``course_keys`` filter. - """ - - course_keys = serializers.CharField(required=False) - - def validate_course_keys(self, value: str) -> list[CourseKey]: - """Parse the comma-separated course keys, rejecting a filter that names none or an unparseable one.""" - pieces = [piece.strip() for piece in value.split(",") if piece.strip()] - if not pieces: - raise serializers.ValidationError(_("course_keys must name at least one course key.")) - course_keys = [] - for piece in pieces: - try: - course_keys.append(CourseKey.from_string(piece)) - except InvalidKeyError as exc: - raise serializers.ValidationError( - _("'{course_key}' is not a valid course key.").format(course_key=piece) - ) from exc - return course_keys + 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/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index 2fcc55f21..d96a86baf 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -37,7 +37,6 @@ from .permissions import CompetencyReadPermission, CompetencyRuleProfilePermissions from .serializers import ( BlockedCourseSerializer, - CompetencyBlockedCoursesQueryParamsSerializer, CompetencyCriteriaGroupSerializer, CompetencyCriteriaGroupUpdateSerializer, CompetencyCriteriaQueryParamsSerializer, @@ -280,16 +279,22 @@ def patch(self, request, tag_id: int, group_id: int): class CompetencyBlockedCoursesView(generics.GenericAPIView): """ - GET-only. List the courses where a competency can't get a new criterion, paginated. + GET-only. List the courses where an ancestor or descendant of a competency already has a live + criterion, paginated. - ``GET api/cbe/v1/competencies//blocked-courses/`` returns every course where a - taxonomy ancestor or descendant of the competency already has a live criterion, sorted by - course key; ``course_keys`` (comma-separated) narrows it. The result is a hint for authoring: - the create-criterion endpoint still enforces the rule. 404 if ``tag_id`` isn't a competency - tag, 403 if the caller can't view its taxonomy, 400 for a malformed ``course_keys``. + 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. - UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a - deprecation cycle. + ``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. The result is an authoring hint: the create-criterion endpoint still enforces + the rule. 401 for an unauthenticated caller, 403 if the caller can't view the competency's + taxonomy, 404 if ``tag_id`` isn't a competency tag or ``page`` is out of range, 400 if + ``course_keys`` is malformed, given more than once, or names more than ``MAX_COURSE_KEYS`` keys. + + UNSTABLE: the criteria-authoring family is incomplete, so this response shape may change + without a deprecation cycle. """ authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) @@ -303,8 +308,10 @@ def get(self, request, tag_id): except Tag.DoesNotExist as exc: raise NotFound(str(exc)) from exc self.check_object_permissions(request, tag) - params = CompetencyBlockedCoursesQueryParamsSerializer(data=request.query_params.dict()) - params.is_valid(raise_exception=True) - rows = get_blocked_courses(tag, course_keys=params.validated_data.get("course_keys")) + 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 0fd27686b..6ca13cbac 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -19,6 +19,7 @@ from openedx_catalog.models import CatalogCourse, CourseRun from openedx_learning.api import ( + BlockedCourse, CompetencyCriteriaGroupArchivedError, CompetencyCriterionArchivedError, associate_competency_criterion, @@ -35,7 +36,7 @@ update_competency_criteria_group, ) from openedx_learning.applets.cbe import api as cbe_api -from openedx_learning.applets.cbe.api import BlockedCourse, _create_leaf_group +from openedx_learning.applets.cbe.api import _create_leaf_group from openedx_learning.models import ( CompetencyCourseRun, CompetencyCriteriaGroup, @@ -50,6 +51,8 @@ ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy +from .utils import make_leaf_criterion, make_skill_family, usage_key + pytestmark = pytest.mark.django_db GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} @@ -113,29 +116,11 @@ def course_key(course_run: CourseRun) -> CourseKey: return key -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_tag(taxonomy: CompetencyTaxonomy, value: str, parent: Tag | None = None) -> Tag: """Create a Tag on `taxonomy`, optionally under `parent`, for taxonomy-hierarchy tests.""" return Tag.objects.create(taxonomy=taxonomy, value=value, parent=parent) -def make_leaf_criterion( - tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, block_id: str, -) -> CompetencyCriterion: - """ - Create a leaf group and one live CompetencyCriterion for `tag` in `course_run`. - """ - leaf = _create_leaf_group(tag, course_run) - object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, block_id), taxonomy=tag.taxonomy, tag=tag) - return CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) - - def test_is_competency_taxonomy() -> None: """ is_competency_taxonomy() is True for a competency taxonomy, False for a plain one. @@ -1931,6 +1916,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: @@ -2317,62 +2322,30 @@ def test_update_never_reads_or_writes_learner_status( # ============================================================================================== -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 = make_tag(taxonomy, "Cognitive Skills") - critical_thinking = make_tag(taxonomy, "Critical Thinking", parent=cognitive_skills) - inference = make_tag(taxonomy, "Inference", parent=critical_thinking) - analysis = make_tag(taxonomy, "Analysis", parent=critical_thinking) - return cognitive_skills, critical_thinking, inference, analysis - - -def test_get_blocked_courses_reports_an_ancestor_claim( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, -) -> None: - """A course where an ancestor has a criterion is blocked, naming that ancestor.""" - _, 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_run), critical_thinking.id, "Critical Thinking"), - ] - - -def test_get_blocked_courses_reports_a_descendant_claim( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, -) -> None: - """A course where a descendant has a criterion is blocked, naming that descendant.""" - _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(inference, course_run, default_rule_profile, "p1") - - assert get_blocked_courses(critical_thinking) == [ - BlockedCourse(course_key(course_run), inference.id, "Inference"), - ] - - -def test_get_blocked_courses_reports_a_grandparent_claim( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, -) -> None: - """The block reaches past the immediate parent to a grandparent.""" - cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(cognitive_skills, course_run, default_rule_profile, "p1") - - assert get_blocked_courses(inference) == [ - BlockedCourse(course_key(course_run), cognitive_skills.id, "Cognitive Skills"), - ] - - -def test_get_blocked_courses_reports_a_grandchild_claim( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +@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: - """The block reaches past the immediate child to a grandchild.""" - cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(inference, course_run, default_rule_profile, "p1") + """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(cognitive_skills) == [ - BlockedCourse(course_key(course_run), inference.id, "Inference"), + assert get_blocked_courses(queried) == [ + BlockedCourse( + course_key=course_key(course_run), conflicting_tag_id=claimant.id, conflicting_tag_value=claimant.value, + ), ] @@ -2386,27 +2359,14 @@ def test_get_blocked_courses_ignores_a_sibling_claim( assert not get_blocked_courses(inference) -def test_get_blocked_courses_ignores_a_course_with_no_relative_claim( - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - organization: Organization, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """Only the course holding a relative's criterion is reported.""" - _, 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") - - assert [row.course_key for row in get_blocked_courses(inference)] == [course_run.course_key] - - -def test_get_blocked_courses_is_empty_when_no_relative_has_a_claim( - competency_taxonomy: CompetencyTaxonomy, +def test_get_blocked_courses_ignores_a_claim_on_the_tag_itself( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A tag whose ancestors and descendants have no criteria anywhere blocks no course.""" - _, critical_thinking, _, _ = make_skill_family(competency_taxonomy) + """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(critical_thinking) == [] + assert get_blocked_courses(inference) == [] def test_get_blocked_courses_is_empty_for_a_tag_with_no_relatives(tag: Tag) -> None: @@ -2414,20 +2374,6 @@ def test_get_blocked_courses_is_empty_for_a_tag_with_no_relatives(tag: Tag) -> N assert get_blocked_courses(tag) == [] -def test_get_blocked_courses_reports_one_row_per_course_naming_the_lowest_id_relative( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, -) -> None: - """A course claimed by two relatives is reported once, naming the lower-id relative.""" - cognitive_skills, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) - assert cognitive_skills.id < critical_thinking.id - make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") - make_leaf_criterion(cognitive_skills, course_run, default_rule_profile, "p2") - - assert get_blocked_courses(inference) == [ - BlockedCourse(course_key(course_run), cognitive_skills.id, "Cognitive Skills"), - ] - - def test_get_blocked_courses_narrows_to_the_requested_course_keys( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, @@ -2444,7 +2390,7 @@ def test_get_blocked_courses_narrows_to_the_requested_course_keys( rows = get_blocked_courses(inference, course_keys=[course_key(course_run), course_key(course_y)]) - assert {row.course_key for row in rows} == {course_run.course_key, course_y.course_key} + assert {row.course_key for row in rows} == {course_key(course_run), course_key(course_y)} def test_get_blocked_courses_sorts_rows_by_course_key( @@ -2458,16 +2404,4 @@ def test_get_blocked_courses_sorts_rows_by_course_key( keys = [str(row.course_key) for row in get_blocked_courses(inference)] - assert keys == sorted(keys) - assert len(keys) == 3 - - -def test_get_blocked_courses_ignores_an_archived_claim( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, -) -> None: - """An archived criterion on a relative blocks nothing.""" - _, 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) - - assert 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 dc9c0a277..32a133d13 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -28,6 +28,8 @@ ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy +from .utils import make_leaf_criterion, make_skill_family, usage_key + pytestmark = pytest.mark.django_db # A marker embedded in an object_id to signal "this caller has no object-level tagging @@ -73,13 +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) @@ -1457,6 +1452,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 +1501,23 @@ def test_a_repeated_course_keys_query_parameter_is_400(user_client: APIClient, t assert set(response.data) == {"course_keys"} +def test_a_branch_qualified_course_key_matches_the_plain_course( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A course key with a branch 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}+branch@draft"}) + + 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: @@ -2006,15 +2027,6 @@ def blocked_courses_url(tag_id: int, **params: str) -> str: return f"{url}?{urlencode(params)}" if params else url -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 collect_pages(client: APIClient, url: str) -> list[dict]: """Follow `next` links from `url` and return every row across all pages.""" rows: list[dict] = [] @@ -2027,88 +2039,37 @@ def collect_pages(client: APIClient, url: str) -> list[dict]: return rows -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 = make_tag(taxonomy, "Cognitive Skills") - critical_thinking = make_tag(taxonomy, "Critical Thinking", parent=cognitive_skills) - inference = make_tag(taxonomy, "Inference", parent=critical_thinking) - analysis = make_tag(taxonomy, "Analysis", parent=critical_thinking) - return cognitive_skills, critical_thinking, inference, analysis - - -def test_blocked_courses_reports_an_ancestor_claim( +@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_blocked_courses_reports_a_relative_claim( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, + claimant_and_queried: tuple[int, int], ) -> None: - """A course where an ancestor has a criterion is reported, with the conflicting tag's id and value.""" - _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") + """A course where an ancestor or descendant at any depth has a criterion is reported, 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") - response = user_client.get(blocked_courses_url(inference.id)) + response = user_client.get(blocked_courses_url(queried.id)) assert response.status_code == status.HTTP_200_OK assert response.data["results"] == [{ "course_key": str(course_run.course_key), - "conflicting_tag_id": critical_thinking.id, - "conflicting_tag_value": "Critical Thinking", + "conflicting_tag_id": claimant.id, + "conflicting_tag_value": claimant.value, }] -def test_blocked_courses_reports_a_descendant_claim( - user_client: APIClient, - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """A course where a descendant has a criterion is reported, naming that descendant.""" - _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(inference, course_run, default_rule_profile, "p1") - - response = user_client.get(blocked_courses_url(critical_thinking.id)) - - assert response.status_code == status.HTTP_200_OK - assert [row["course_key"] for row in response.data["results"]] == [str(course_run.course_key)] - assert response.data["results"][0]["conflicting_tag_value"] == "Inference" - - -def test_blocked_courses_reports_a_grandparent_claim( - user_client: APIClient, - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """The block reaches a grandparent, and the grandparent is the tag reported.""" - cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(cognitive_skills, 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)] - assert response.data["results"][0]["conflicting_tag_value"] == "Cognitive Skills" - - -def test_blocked_courses_reports_a_grandchild_claim( - user_client: APIClient, - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """The block reaches a grandchild, and the grandchild is the tag reported.""" - cognitive_skills, _, inference, _ = make_skill_family(competency_taxonomy) - make_leaf_criterion(inference, course_run, default_rule_profile, "p1") - - response = user_client.get(blocked_courses_url(cognitive_skills.id)) - - assert response.status_code == status.HTTP_200_OK - assert [row["course_key"] for row in response.data["results"]] == [str(course_run.course_key)] - assert response.data["results"][0]["conflicting_tag_value"] == "Inference" - - def test_blocked_courses_ignores_a_sibling_claim( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, @@ -2134,15 +2095,13 @@ def test_blocked_courses_ignores_an_unaffected_course( ) -> 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) - course_y = make_course_run(organization, "Python200", "Fall2026") + 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 - returned_keys = [row["course_key"] for row in response.data["results"]] - assert str(course_y.course_key) not in returned_keys - assert returned_keys == [str(course_run.course_key)] + assert [row["course_key"] for row in response.data["results"]] == [str(course_run.course_key)] def test_blocked_courses_narrows_to_requested_course_keys( @@ -2189,13 +2148,11 @@ def test_blocked_courses_paginates_without_repeats_or_gaps( organization: Organization, default_rule_profile: CompetencyRuleProfile, ) -> None: - """Three blocked courses at two per page arrive across two pages, each exactly once.""" + """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) - expected_keys = set() - for course_code in ("A100", "B200", "C300"): + 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") - expected_keys.add(str(course.course_key)) url = blocked_courses_url(inference.id, page_size="2") first_page = user_client.get(url) @@ -2204,9 +2161,9 @@ def test_blocked_courses_paginates_without_repeats_or_gaps( assert first_page.data["count"] == 3 assert len(first_page.data["results"]) == 2 assert first_page.data["next"] is not None - keys = [row["course_key"] for row in rows] - assert len(keys) == len(set(keys)) == 3 - assert set(keys) == expected_keys + assert [row["course_key"] for row in rows] == [ + "course-v1:Org1+A100+Run1", "course-v1:Org1+B200+Run1", "course-v1:Org1+C300+Run1", + ] def test_blocked_courses_filters_before_paginating( @@ -2215,25 +2172,22 @@ def test_blocked_courses_filters_before_paginating( organization: Organization, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A narrowed result that spans two pages holds exactly the requested courses, and nothing outside them.""" + """A narrowed result that spans two pages holds exactly the requested courses, in course key order.""" _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) - requested_keys = [] - for course_code in ("A100", "B200", "C300", "D400"): + 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_keys.append(str(course.course_key)) - outside_key = requested_keys.pop(1) - url = blocked_courses_url(inference.id, course_keys=",".join(requested_keys), page_size="2") + requested = "course-v1:Org1+D400+Run1,course-v1:Org1+A100+Run1,course-v1:Org1+C300+Run1" + url = blocked_courses_url(inference.id, course_keys=requested, page_size="2") first_page = user_client.get(url) rows = collect_pages(user_client, url) assert first_page.data["count"] == 3 assert len(first_page.data["results"]) == 2 - keys = [row["course_key"] for row in rows] - assert len(keys) == len(set(keys)) == 3 - assert set(keys) == set(requested_keys) - assert outside_key not in keys + assert [row["course_key"] for row in rows] == [ + "course-v1:Org1+A100+Run1", "course-v1:Org1+C300+Run1", "course-v1:Org1+D400+Run1", + ] def test_blocked_courses_unknown_tag_404s(user_client: APIClient) -> None: @@ -2273,6 +2227,21 @@ def test_blocked_courses_without_view_access_403s( 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: @@ -2285,17 +2254,87 @@ def test_blocked_courses_malformed_course_key_400s( assert "not-a-key" in str(response.data["course_keys"]) -@pytest.mark.parametrize("course_keys", ["", ",,"]) -def test_blocked_courses_empty_course_keys_400s( - user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_keys: str, +def test_blocked_courses_version_only_course_key_400s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, ) -> None: - """A course_keys filter that names no course is rejected rather than ignored.""" + """A version-only course key parses but can't drop its version, so it is rejected like a malformed one.""" _, _, inference, _ = make_skill_family(competency_taxonomy) + version_only = "course-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa" + + response = user_client.get(blocked_courses_url(inference.id, course_keys=version_only)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert version_only 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_more_than_the_limit_of_course_keys_400s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, +) -> None: + """More than 100 course_keys entries is rejected, keyed by course_keys.""" + _, _, inference, _ = make_skill_family(competency_taxonomy) + too_many = ",".join(f"course-v1:Org+Course{i}+Run" for i in range(101)) + + response = user_client.get(blocked_courses_url(inference.id, course_keys=too_many)) + assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "course_keys" in response.data + assert set(response.data) == {"course_keys"} + + +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 course key with a branch matches its plain course, which is reported under 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"]] == [str(course_run.course_key)] def test_blocked_courses_strips_whitespace_around_course_keys( diff --git a/tests/openedx_learning/applets/cbe/utils.py b/tests/openedx_learning/applets/cbe/utils.py new file mode 100644 index 000000000..12f732428 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/utils.py @@ -0,0 +1,32 @@ +"""Shared test helpers for the CBE test modules.""" +from openedx_catalog.models import 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_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 From 44f318ddff3f9012e9566e2e3ad26d72cc29d79b Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 6 Nov 2026 04:36:23 +0400 Subject: [PATCH 3/9] fix: address second-round review of the blocked-courses endpoint Both read endpoints and both API functions match a course key without its branch or version, and a version-only key fails up front. get_blocked_courses and _validate_containment share one relative-ids helper. course_keys on get_courses_with_relative_criteria is keyword-only. View docstrings state the normalization and each status code. Tests cover all three qualified key forms, permission before parsing, the lowest-id tie-break, the default page size, an out-of-range page, and that the read and the create path agree on a conflict. Duplicate view-layer scenarios are dropped. Related to openedx/openedx-core#796 Co-Authored-By: Claude Fable 5.1 --- src/openedx_learning/applets/cbe/api.py | 37 ++-- .../applets/cbe/rest_api/v1/serializers.py | 11 +- .../applets/cbe/rest_api/v1/views.py | 15 +- .../openedx_learning/applets/cbe/test_api.py | 69 ++++--- .../applets/cbe/test_views.py | 183 ++++++++---------- tests/openedx_learning/applets/cbe/utils.py | 15 +- 6 files changed, 178 insertions(+), 152 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index c2e51d5bb..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 @@ -203,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. @@ -212,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") @@ -339,7 +344,7 @@ 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, course_keys: Iterable[CourseKey] | 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. @@ -372,7 +377,7 @@ def get_courses_with_relative_criteria( @dataclass(frozen=True, kw_only=True, slots=True) class BlockedCourse: """ - A course where a taxonomy relative of a competency already has a live CompetencyCriterion. + 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. @@ -383,6 +388,18 @@ class BlockedCourse: 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. @@ -390,14 +407,15 @@ def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None 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, and sorted by course key. When several relatives claim + 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. """ - relative_ids = ( - set(get_ancestor_tags(tag).values_list("id", flat=True)) - | set(get_descendant_tags(tag).values_list("id", flat=True)) - ) + 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) @@ -421,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/v1/serializers.py b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py index 47b58f9d7..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,7 +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, LibraryLocator +from opaque_keys.edx.locator import CourseLocator from rest_framework import serializers from ...models import CompetencyCriteriaGroup, CompetencyCriterion, CompetencyRuleProfile, LogicOperator @@ -178,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) @@ -200,9 +200,10 @@ def validate_course_keys(self, value: str) -> list[CourseKey]: for entry in entries: try: key = CourseKey.from_string(entry) - # CourseKey's type lacks for_branch(); mypy needs the narrowing to the two base locators that define it. - if isinstance(key, (CourseLocator, LibraryLocator)): - key = key.for_branch(None).version_agnostic() + # 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} 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 d96a86baf..b29835a56 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -194,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 @@ -288,10 +290,13 @@ class CompetencyBlockedCoursesView(generics.GenericAPIView): ``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. The result is an authoring hint: the create-criterion endpoint still enforces - the rule. 401 for an unauthenticated caller, 403 if the caller can't view the competency's - taxonomy, 404 if ``tag_id`` isn't a competency tag or ``page`` is out of range, 400 if - ``course_keys`` is malformed, given more than once, or names more than ``MAX_COURSE_KEYS`` keys. + 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. diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 6ca13cbac..157372655 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -17,7 +17,7 @@ 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, @@ -51,7 +51,7 @@ ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy -from .utils import make_leaf_criterion, make_skill_family, usage_key +from .utils import make_course_run, make_leaf_criterion, make_skill_family, make_tag, usage_key pytestmark = pytest.mark.django_db @@ -103,12 +103,6 @@ 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 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 @@ -116,11 +110,6 @@ def course_key(course_run: CourseRun) -> CourseKey: return key -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 test_is_competency_taxonomy() -> None: """ is_competency_taxonomy() is True for a competency taxonomy, False for a plain one. @@ -1533,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: @@ -1755,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]) @@ -2374,23 +2384,24 @@ def test_get_blocked_courses_is_empty_for_a_tag_with_no_relatives(tag: Tag) -> N assert get_blocked_courses(tag) == [] -def test_get_blocked_courses_narrows_to_the_requested_course_keys( - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - organization: Organization, - default_rule_profile: CompetencyRuleProfile, +def test_get_blocked_courses_and_create_competency_criterion_agree_on_a_conflict( + competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: - """Only the requested courses come back, even when others are 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") + """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") - make_leaf_criterion(cognitive_skills, course_y, default_rule_profile, "p1") - make_leaf_criterion(critical_thinking, course_z, default_rule_profile, "p1") - - rows = get_blocked_courses(inference, course_keys=[course_key(course_run), course_key(course_y)]) - assert {row.course_key for row in rows} == {course_key(course_run), course_key(course_y)} + 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( diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 32a133d13..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,7 +28,7 @@ ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy -from .utils import make_leaf_criterion, make_skill_family, usage_key +from .utils import make_course_run, make_leaf_criterion, make_skill_family, make_tag, usage_key pytestmark = pytest.mark.django_db @@ -75,17 +75,6 @@ def criterion_create_url(tag_id: int) -> str: return reverse("cbe:criterion-create", kwargs={"tag_id": tag_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"} @@ -1501,17 +1490,26 @@ 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, + user_client: APIClient, + tag: Tag, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + suffix: str, ) -> None: - """A course key with a branch scopes the tree exactly as the plain course key does.""" + """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}+branch@draft"}) + 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 @@ -2027,65 +2025,43 @@ def blocked_courses_url(tag_id: int, **params: str) -> str: return f"{url}?{urlencode(params)}" if params else url -def collect_pages(client: APIClient, url: str) -> list[dict]: - """Follow `next` links from `url` and return every row across all pages.""" - rows: list[dict] = [] +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 - rows.extend(response.data["results"]) + if count is None: + count = response.data["count"] + pages.append(response.data["results"]) next_url = response.data["next"] - return rows + assert count is not None + return count, pages -@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_blocked_courses_reports_a_relative_claim( +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, - claimant_and_queried: tuple[int, int], ) -> None: - """A course where an ancestor or descendant at any depth has a criterion is reported, 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") + """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(queried.id)) + 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": str(course_run.course_key), - "conflicting_tag_id": claimant.id, - "conflicting_tag_value": claimant.value, + "course_key": "course-v1:Org1+Python100+Fall2026", + "conflicting_tag_id": critical_thinking.id, + "conflicting_tag_value": "Critical Thinking", }] -def test_blocked_courses_ignores_a_sibling_claim( - user_client: APIClient, - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - default_rule_profile: CompetencyRuleProfile, -) -> None: - """A course claimed only by a sibling tag is not reported.""" - _, _, inference, analysis = make_skill_family(competency_taxonomy) - make_leaf_criterion(analysis, course_run, default_rule_profile, "p1") - - response = user_client.get(blocked_courses_url(inference.id)) - - assert response.status_code == status.HTTP_200_OK - assert response.data["results"] == [] - - def test_blocked_courses_ignores_an_unaffected_course( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, @@ -2153,15 +2129,11 @@ def test_blocked_courses_paginates_without_repeats_or_gaps( 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") - url = blocked_courses_url(inference.id, page_size="2") + count, pages = collect_pages(user_client, blocked_courses_url(inference.id, page_size="2")) - first_page = user_client.get(url) - rows = collect_pages(user_client, url) - - assert first_page.data["count"] == 3 - assert len(first_page.data["results"]) == 2 - assert first_page.data["next"] is not None - assert [row["course_key"] for row in rows] == [ + 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", ] @@ -2178,18 +2150,50 @@ def test_blocked_courses_filters_before_paginating( 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" - url = blocked_courses_url(inference.id, course_keys=requested, page_size="2") - - first_page = user_client.get(url) - rows = collect_pages(user_client, url) + count, pages = collect_pages(user_client, blocked_courses_url(inference.id, course_keys=requested, page_size="2")) - assert first_page.data["count"] == 3 - assert len(first_page.data["results"]) == 2 - assert [row["course_key"] for row in rows] == [ + 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)) @@ -2214,14 +2218,17 @@ def test_blocked_courses_without_view_access_403s( course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A non-staff caller may not read blocked courses for a competency on a disabled taxonomy.""" + """ + 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)) + 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 @@ -2254,19 +2261,6 @@ def test_blocked_courses_malformed_course_key_400s( assert "not-a-key" in str(response.data["course_keys"]) -def test_blocked_courses_version_only_course_key_400s( - user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, -) -> None: - """A version-only course key parses but can't drop its version, so it is rejected like a malformed one.""" - _, _, inference, _ = make_skill_family(competency_taxonomy) - version_only = "course-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa" - - response = user_client.get(blocked_courses_url(inference.id, course_keys=version_only)) - - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert version_only 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, @@ -2286,19 +2280,6 @@ def test_blocked_courses_blank_course_keys_is_an_empty_page( assert response.data["results"] == [] -def test_blocked_courses_more_than_the_limit_of_course_keys_400s( - user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, -) -> None: - """More than 100 course_keys entries is rejected, keyed by course_keys.""" - _, _, inference, _ = make_skill_family(competency_taxonomy) - too_many = ",".join(f"course-v1:Org+Course{i}+Run" for i in range(101)) - - response = user_client.get(blocked_courses_url(inference.id, course_keys=too_many)) - - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert set(response.data) == {"course_keys"} - - def test_blocked_courses_repeated_course_keys_parameter_400s( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, @@ -2325,7 +2306,7 @@ def test_blocked_courses_matches_a_branch_qualified_course_key( course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A course key with a branch matches its plain course, which is reported under the plain key.""" + """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") @@ -2334,7 +2315,7 @@ def test_blocked_courses_matches_a_branch_qualified_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)] + assert [row["course_key"] for row in response.data["results"]] == ["course-v1:Org1+Python100+Fall2026"] def test_blocked_courses_strips_whitespace_around_course_keys( diff --git a/tests/openedx_learning/applets/cbe/utils.py b/tests/openedx_learning/applets/cbe/utils.py index 12f732428..4e291ddfe 100644 --- a/tests/openedx_learning/applets/cbe/utils.py +++ b/tests/openedx_learning/applets/cbe/utils.py @@ -1,5 +1,7 @@ """Shared test helpers for the CBE test modules.""" -from openedx_catalog.models import CourseRun +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 @@ -21,6 +23,17 @@ def make_leaf_criterion( 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". From 0f763e1b716ef00dd74045aa4263e4c8e245a090 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 6 Nov 2026 04:36:23 +0400 Subject: [PATCH 4/9] docs: record the competency dominance rule in ADR 0002 Decision 4 states that in one course, live criteria along any ancestor-descendant line of the taxonomy belong to one competency, that creation enforces it with no database constraint, and that get_blocked_courses is the advisory read. Related to openedx/openedx-core#796 Co-Authored-By: Claude Fable 5.1 --- .../decisions/0002-competency-criteria-model.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 5869b2885..f94080739 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -154,6 +154,7 @@ Decision - Belongs to one ``CompetencyCriteriaGroup``. - Points to one ``oel_tagging_objecttag`` association. - Uses one ``CompetencyRuleProfile`` by default, with optional per-criterion overrides. + - In one course, the live criteria (non-archived, under non-archived groups) along any ancestor-descendant line of the taxonomy belong to one competency; siblings are unrestricted. The check runs only at creation, inside the transaction that locks the competency's ``Tag`` and the course's ``CompetencyCourseRun`` rows; no database constraint enforces it, and existing criteria are not revalidated when the hierarchy changes. ``get_blocked_courses`` lists the courses closed to a competency as an advisory hint, through the same ``get_courses_with_relative_criteria`` lookup creation uses, so both name the same lowest-id ancestor or descendant; it does not report Decision 2's archived course-level groups. The rule exists so evaluation never has to reconcile two independently authored, overlapping rules for related competencies in one course. This new database table will have the following columns: @@ -490,3 +491,9 @@ 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: + +* Amended Decision 4 for #666 and #796: in one course, live criteria along any ancestor-descendant + line of the taxonomy belong to one competency, checked at creation only, with + ``get_blocked_courses`` as the advisory read. From f09283bd4fb15615099ff04174ff3f20bd826353 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 6 Nov 2026 20:04:16 +0400 Subject: [PATCH 5/9] docs: state the competency dominance rule at decision altitude ADR 0002 Decision 4 now says that no ancestor or descendant of a competency tag may hold live criteria in the same course as that tag, and why. The locking, tie-break, and endpoint mechanics live in docstrings and the ticket. Related to openedx/openedx-core#796 Co-Authored-By: Claude Fable 5.1 --- .../decisions/0002-competency-criteria-model.rst | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index f94080739..5446629c5 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -154,7 +154,14 @@ Decision - Belongs to one ``CompetencyCriteriaGroup``. - Points to one ``oel_tagging_objecttag`` association. - Uses one ``CompetencyRuleProfile`` by default, with optional per-criterion overrides. - - In one course, the live criteria (non-archived, under non-archived groups) along any ancestor-descendant line of the taxonomy belong to one competency; siblings are unrestricted. The check runs only at creation, inside the transaction that locks the competency's ``Tag`` and the course's ``CompetencyCourseRun`` rows; no database constraint enforces it, and existing criteria are not revalidated when the hierarchy changes. ``get_blocked_courses`` lists the courses closed to a competency as an advisory hint, through the same ``get_courses_with_relative_criteria`` lookup creation uses, so both name the same lowest-id ancestor or descendant; it does not report Decision 2's archived course-level groups. The rule exists so evaluation never has to reconcile two independently authored, overlapping rules for related competencies in one course. + + 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: @@ -494,6 +501,5 @@ Changelog 2026-11-06: -* Amended Decision 4 for #666 and #796: in one course, live criteria along any ancestor-descendant - line of the taxonomy belong to one competency, checked at creation only, with - ``get_blocked_courses`` as the advisory read. +* 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. From 4de62a2338ad6c46e7dc977608904491238bb48f Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 6 Nov 2026 22:36:07 +0400 Subject: [PATCH 6/9] fix: reject library keys in course_keys and tighten the blocked-courses tests A library key parses as a CourseKey but can never name a course run, so both read endpoints now return 400 for it. A version-only library key previously skipped normalization and raised from the database layer, which the view turned into a 500. The read-write agreement test now covers all four directions of the dominance rule and checks that the write names the same relative the read does. New tests pin the version-only key rejection in both API functions and that the view checks permission before counting the course_keys parameter. Related to openedx/openedx-core#796 Co-Authored-By: Claude Fable 5.1 --- src/openedx_learning/applets/cbe/api.py | 2 +- .../applets/cbe/rest_api/v1/serializers.py | 9 ++-- .../openedx_learning/applets/cbe/test_api.py | 43 +++++++++++++----- .../applets/cbe/test_views.py | 45 ++++++++++++++++--- 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 1e62bd6bb..3d439fdad 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -408,7 +408,7 @@ def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None 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 + version-only key raises ``InvalidKeyError``. Results are 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. """ 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 d9e5c764a..7639083a4 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -189,7 +189,8 @@ def validate_course_keys(self, value: str) -> list[CourseKey]: 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. + can't dodge the cap, or if any entry isn't a course key. A library key parses as a + CourseKey but can never name a course run, so it is rejected too. """ entries = [entry for entry in (entry.strip() for entry in value.split(",")) if entry] if len(entries) > MAX_COURSE_KEYS: @@ -200,10 +201,10 @@ def validate_course_keys(self, value: str) -> list[CourseKey]: for entry in entries: try: key = CourseKey.from_string(entry) + if not isinstance(key, CourseLocator): + raise InvalidKeyError(type(key), 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) + key = key.for_branch(None) except InvalidKeyError as exc: raise serializers.ValidationError( _("'%(entry)s' is not a valid course key.") % {"entry": entry} diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 157372655..bf5cea984 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -13,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 import InvalidKeyError from opaque_keys.edx.keys import CourseKey from organizations.models import Organization from rules.permissions import permissions as rule_permissions @@ -2384,24 +2385,46 @@ def test_get_blocked_courses_is_empty_for_a_tag_with_no_relatives(tag: Tag) -> N assert get_blocked_courses(tag) == [] +@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_and_create_competency_criterion_agree_on_a_conflict( - competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + claimant_and_queried: tuple[int, int], ) -> 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") + """A course the read reports as blocked for a tag is one the write rejects, and both name the same 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(inference) == [ + assert get_blocked_courses(queried) == [ BlockedCourse( - course_key=course_key(course_run), - conflicting_tag_id=critical_thinking.id, - conflicting_tag_value=critical_thinking.value, + course_key=course_key(course_run), conflicting_tag_id=claimant.id, conflicting_tag_value=claimant.value, ), ] - group = _create_leaf_group(inference, course_run) + group = _create_leaf_group(queried, 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 + assert claimant.value in str(exc_info.value.message_dict["tag_id"]) + + +@pytest.mark.parametrize( + "read", [get_blocked_courses, get_competency_criteria_tree], ids=["blocked courses", "criteria tree"], +) +def test_a_version_only_course_key_raises_before_the_database_is_asked(tag: Tag, read) -> None: + """A version-only key has no course to match, so both reads reject it up front rather than from the lookup.""" + version_only = CourseKey.from_string("course-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa") + + with pytest.raises(InvalidKeyError): + read(tag, [version_only]) def test_get_blocked_courses_sorts_rows_by_course_key( diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 72550ba5d..898019861 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -1450,6 +1450,15 @@ def test_a_version_only_course_key_is_400(user_client: APIClient, tag: Tag) -> N assert set(response.data) == {"course_keys"} +@pytest.mark.parametrize("library_key", ["library-v1:Org1+Lib1", "library-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa"]) +def test_a_library_key_is_400(user_client: APIClient, tag: Tag, library_key: str) -> None: + """A library key parses as a CourseKey but can never name a course run, so it is rejected like a malformed one.""" + response = user_client.get(criteria_tree_url(tag.id), {"course_keys": library_key}) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert library_key in str(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)) @@ -2220,18 +2229,21 @@ def test_blocked_courses_without_view_access_403s( ) -> 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. + with garbage in course_keys: permission is checked before course_keys is counted or 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) + malformed = urlencode({"course_keys": "not-a-key"}) + repeated = urlencode({"course_keys": ["a", "b"]}, doseq=True) - response = api_client.get(blocked_courses_url(inference.id, course_keys="not-a-key")) + for query in (malformed, repeated): + response = api_client.get(f"{blocked_courses_url(inference.id)}?{query}") - assert response.status_code == status.HTTP_403_FORBIDDEN - assert "results" not in response.data + 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( @@ -2261,6 +2273,24 @@ def test_blocked_courses_malformed_course_key_400s( assert "not-a-key" in str(response.data["course_keys"]) +@pytest.mark.parametrize("library_key", ["library-v1:Org1+Lib1", "library-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa"]) +def test_blocked_courses_library_key_400s( + user_client: APIClient, + competency_taxonomy: CompetencyTaxonomy, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + library_key: str, +) -> None: + """A library key parses as a CourseKey but can never name a course run, so it is rejected like a malformed one.""" + _, 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=library_key)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert library_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, @@ -2306,7 +2336,12 @@ def test_blocked_courses_matches_a_branch_qualified_course_key( course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: - """A branch-qualified key matches its plain course, and the row reports the plain key.""" + """ + A branch-qualified key matches its plain course, and the row reports the plain key. + + This pins the database match, which ignores a key's branch, so it holds with or without the + serializer's normalization. + """ _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") From 1695998dbc0674fc0fff89cff020aaf32d66cffb Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 6 Nov 2026 22:45:44 +0400 Subject: [PATCH 7/9] test: tidy the blocked-courses tests Pin the 500 maximum page size, read the page count from the first response in collect_pages, build the branch-qualified input from the fixture's course key, and say where the pagination envelope comes from. Related to openedx/openedx-core#796 Co-Authored-By: Claude Fable 5.1 --- .../openedx_learning/applets/cbe/test_api.py | 2 +- .../applets/cbe/test_views.py | 27 ++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index bf5cea984..c8df7b696 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1533,7 +1533,7 @@ def test_get_competency_criteria_tree_course_keys_branch_qualified_key_matches_t 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") + branched_key = CourseKey.from_string(f"{course_key(course_run)}+branch@draft") tree = get_competency_criteria_tree(tag, [branched_key]) diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 898019861..31b800af3 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -14,10 +14,12 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from organizations.models import Organization from rest_framework import status -from rest_framework.test import APIClient +from rest_framework.request import Request +from rest_framework.test import APIClient, APIRequestFactory from openedx_catalog.models import CourseRun from openedx_learning.applets.cbe.api import _create_leaf_group +from openedx_learning.applets.cbe.rest_api.paginators import CompetencyBlockedCoursesPagination from openedx_learning.models import ( CompetencyCriteriaGroup, CompetencyCriterion, @@ -2036,17 +2038,14 @@ def blocked_courses_url(tag_id: int, **params: str) -> str: 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) + response = client.get(url) + assert response.status_code == status.HTTP_200_OK + count = response.data["count"] + pages = [response.data["results"]] + while response.data["next"]: + response = client.get(response.data["next"]) 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 @@ -2063,6 +2062,7 @@ def test_blocked_courses_reports_a_relative_claim_with_the_row_shape( response = user_client.get(blocked_courses_url(inference.id)) assert response.status_code == status.HTTP_200_OK + # The seven keys are the standard envelope of edx-platform ADR 0032, which DefaultPagination implements. assert set(response.data) == {"next", "previous", "count", "num_pages", "current_page", "start", "results"} assert response.data["results"] == [{ "course_key": "course-v1:Org1+Python100+Fall2026", @@ -2188,6 +2188,13 @@ def test_blocked_courses_default_page_size_is_100( assert response.data["next"] is not None +def test_blocked_courses_page_size_is_capped_at_500() -> None: + """A page_size above the cap is clamped to 500 rather than honored or rejected.""" + request = Request(APIRequestFactory().get("/", {"page_size": "1000"})) + + assert CompetencyBlockedCoursesPagination().get_page_size(request) == 500 + + def test_blocked_courses_page_out_of_range_404s( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, From 8608b0009569dd8a9e5f402bd05fa220902bc2a9 Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 13 Nov 2026 16:11:01 +0400 Subject: [PATCH 8/9] fix: address third-round review of the blocked-courses endpoint Rename the private helper to _get_ancestor_and_descendant_tag_ids and its locals to ancestor_and_descendant_ids. get_competency_criteria_tree and get_blocked_courses take course keys as given. The query-params serializer is the one place that drops a key's branch and version, and its comment explains both. Shorten the serializer and view docstrings, and correct the first docstring line of get_courses_with_relative_criteria. Tests: merge the two pagination tests into one parameterized test, parameterize the 403 test, drop three tests that repeat other tests or cannot fail, and add two cases where several ancestors or descendants have a criterion in one course, asserting the read and the create error name the lowest-id tag. Related to https://github.com/openedx/openedx-core/issues/796 Co-Authored-By: Claude Fable 5.2 --- src/openedx_learning/applets/cbe/api.py | 33 ++-- .../applets/cbe/rest_api/v1/serializers.py | 16 +- .../applets/cbe/rest_api/v1/views.py | 18 +-- .../openedx_learning/applets/cbe/test_api.py | 91 +++-------- .../applets/cbe/test_views.py | 143 +++++++----------- 5 files changed, 101 insertions(+), 200 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 3d439fdad..4f8e1da2d 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -16,7 +16,6 @@ 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 @@ -204,7 +203,6 @@ 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. @@ -214,9 +212,6 @@ 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") @@ -347,7 +342,7 @@ def get_courses_with_relative_criteria( 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. + Return every course where one of ``tag_ids`` has a live CompetencyCriterion. Maps each conflicting CourseRun to the lowest-id tag (of ``tag_ids``) with a criterion there, narrowed by ``course`` and by ``course_keys``, each when given. Archived criteria and criteria @@ -388,7 +383,7 @@ class BlockedCourse: conflicting_tag_value: str -def _get_relative_tag_ids(tag: Tag) -> set[int]: +def _get_ancestor_and_descendant_tag_ids(tag: Tag) -> set[int]: """ Return the ids of every taxonomy ancestor and descendant of ``tag``. @@ -407,21 +402,17 @@ def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None 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``. Results are 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. + Narrowed to ``course_keys`` when given. Results are sorted by course key. When several ancestors + or descendants have a criterion in one course, the one with the lowest id 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: + ancestor_and_descendant_ids = _get_ancestor_and_descendant_tag_ids(tag) + if not ancestor_and_descendant_ids: return [] - conflicts = get_courses_with_relative_criteria(relative_ids, course_keys=course_keys) + conflicts = get_courses_with_relative_criteria(ancestor_and_descendant_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. + # Typed optional, but save() always sets it. 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, @@ -439,10 +430,10 @@ def _validate_containment(group: CompetencyCriteriaGroup, course: CourseRun) -> CompetencyCriterion in ``course``. """ tag = group.tag - relative_ids = _get_relative_tag_ids(tag) - if not relative_ids: + ancestor_and_descendant_ids = _get_ancestor_and_descendant_tag_ids(tag) + if not ancestor_and_descendant_ids: return - conflicting_tag = get_courses_with_relative_criteria(relative_ids, course=course).get(course) + conflicting_tag = get_courses_with_relative_criteria(ancestor_and_descendant_ids, course=course).get(course) if conflicting_tag is not None: raise ValidationError({ "tag_id": _( 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 7639083a4..7fbe3298e 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -184,13 +184,12 @@ class CompetencyCriteriaQueryParamsSerializer(serializers.Serializer): def validate_course_keys(self, value: str) -> list[CourseKey]: """ - Parse a comma-separated course_keys value into a deduplicated list of CourseKey. + Parse a comma-separated course_keys value into a deduplicated list of plain course keys. - 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 course key. A library key parses as a - CourseKey but can never name a course run, so it is rejected too. + Branch and version are stripped before dedup, and stray separators (trailing, doubled, or + all-commas) are ignored. Raises a validation error if more than MAX_COURSE_KEYS entries + remain after splitting (counted before dedup, so repeats can't dodge the cap) or if an + entry isn't a course key, library keys included. """ entries = [entry for entry in (entry.strip() for entry in value.split(",")) if entry] if len(entries) > MAX_COURSE_KEYS: @@ -203,7 +202,10 @@ def validate_course_keys(self, value: str) -> list[CourseKey]: key = CourseKey.from_string(entry) if not isinstance(key, CourseLocator): raise InvalidKeyError(type(key), entry) - # Match the stored plain course key: drop branch and version; this also rejects a version-only key. + # A course key may carry a branch (draft or published copy) and a version (one saved + # snapshot). The database field stores only the plain key, so we drop both here to + # match what it holds. A version-only key has no org, so it can't be turned into a + # plain key and is rejected. key = key.for_branch(None) except InvalidKeyError as exc: raise serializers.ValidationError( 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 b29835a56..abe3aa15a 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -195,8 +195,7 @@ class CompetencyCriteriaTreeView(generics.GenericAPIView): 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. 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. + and version are ignored, and a version-only or library 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 @@ -281,22 +280,17 @@ def patch(self, request, tag_id: int, group_id: int): class CompetencyBlockedCoursesView(generics.GenericAPIView): """ - GET-only. List the courses where an ancestor or descendant of a competency already has a live - criterion, paginated. + GET-only. List the courses where a competency can't get a new 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. + course key. ``course_keys`` (comma-separated, supplied once, at most 100) narrows it; a blank + value returns an empty page. A key's branch and version are ignored, and each row carries the + plain key. The result is an authoring hint: the create-criterion endpoint still enforces the + rule. UNSTABLE: the criteria-authoring family is incomplete, so this response shape may change without a deprecation cycle. diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index c8df7b696..396116d6e 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -13,7 +13,6 @@ from django.db.utils import IntegrityError from django.forms.models import model_to_dict from django.test.utils import CaptureQueriesContext -from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey from organizations.models import Organization from rules.permissions import permissions as rule_permissions @@ -1523,24 +1522,6 @@ 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(f"{course_key(course_run)}+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: @@ -2333,33 +2314,6 @@ def test_update_never_reads_or_writes_learner_status( # ============================================================================================== -@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: @@ -2386,45 +2340,44 @@ def test_get_blocked_courses_is_empty_for_a_tag_with_no_relatives(tag: Tag) -> N @pytest.mark.parametrize( - "claimant_and_queried", + "claimants_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"), + 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"), + pytest.param(((1, 0), 2), id="two ancestor claims"), + pytest.param(((2, 1), 0), id="two descendant claims"), ], ) def test_get_blocked_courses_and_create_competency_criterion_agree_on_a_conflict( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, - claimant_and_queried: tuple[int, int], + claimants_and_queried: tuple[tuple[int, ...], int], ) -> None: - """A course the read reports as blocked for a tag is one the write rejects, and both name the same relative.""" + """ + A course the read reports as blocked for a tag is one the write rejects, and both name the same + tag: the lowest-id one when several ancestors or descendants have a criterion in the course. + """ 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") + claimant_indexes, queried_index = claimants_and_queried + claimants = [family[index] for index in claimant_indexes] + queried = family[queried_index] + for position, claimant in enumerate(claimants): + make_leaf_criterion(claimant, course_run, default_rule_profile, f"p{position}") + named = min(claimants, key=lambda claimant: claimant.id) assert get_blocked_courses(queried) == [ BlockedCourse( - course_key=course_key(course_run), conflicting_tag_id=claimant.id, conflicting_tag_value=claimant.value, + course_key=course_key(course_run), conflicting_tag_id=named.id, conflicting_tag_value=named.value, ), ] group = _create_leaf_group(queried, course_run) with pytest.raises(ValidationError) as exc_info: - create_competency_criterion(group, usage_key(course_run, "p2"), course_run=course_run) - assert claimant.value in str(exc_info.value.message_dict["tag_id"]) - - -@pytest.mark.parametrize( - "read", [get_blocked_courses, get_competency_criteria_tree], ids=["blocked courses", "criteria tree"], -) -def test_a_version_only_course_key_raises_before_the_database_is_asked(tag: Tag, read) -> None: - """A version-only key has no course to match, so both reads reject it up front rather than from the lookup.""" - version_only = CourseKey.from_string("course-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa") - - with pytest.raises(InvalidKeyError): - read(tag, [version_only]) + create_competency_criterion(group, usage_key(course_run, "new"), course_run=course_run) + message = str(exc_info.value.message_dict["tag_id"]) + assert [claimant.value for claimant in claimants if f"'{claimant.value}'" in message] == [named.value] def test_get_blocked_courses_sorts_rows_by_course_key( diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 31b800af3..10f6f5bfc 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -2080,13 +2080,16 @@ def test_blocked_courses_ignores_an_unaffected_course( ) -> 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") + blocked = course_run + unaffected = make_course_run(organization, "Python200", "Fall2026") + make_leaf_criterion(critical_thinking, blocked, 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)] + keys = [row["course_key"] for row in response.data["results"]] + assert keys == [str(blocked.course_key)] + assert str(unaffected.course_key) not in keys def test_blocked_courses_narrows_to_requested_course_keys( @@ -2127,45 +2130,48 @@ def test_blocked_courses_is_an_empty_200_when_nothing_is_claimed( assert response.data["results"] == [] +@pytest.mark.parametrize( + "scenario", + [ + pytest.param( + ( + ("C300", "A100", "B200"), + {}, + ["course-v1:Org1+A100+Run1", "course-v1:Org1+B200+Run1", "course-v1:Org1+C300+Run1"], + ), + id="unfiltered", + ), + pytest.param( + ( + ("C300", "A100", "D400", "B200"), + {"course_keys": "course-v1:Org1+D400+Run1,course-v1:Org1+A100+Run1,course-v1:Org1+C300+Run1"}, + ["course-v1:Org1+A100+Run1", "course-v1:Org1+C300+Run1", "course-v1:Org1+D400+Run1"], + ), + id="narrowed", + ), + ], +) def test_blocked_courses_paginates_without_repeats_or_gaps( user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, organization: Organization, default_rule_profile: CompetencyRuleProfile, + scenario: tuple[tuple[str, ...], dict[str, str], list[str]], ) -> 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.""" + """ + Three courses at two per page arrive across two pages, each exactly once, in course key order, + whether they are every blocked course or the ones ``course_keys`` narrows the result to. + """ + course_codes, params, expected = scenario _, critical_thinking, inference, _ = make_skill_family(competency_taxonomy) - for course_code in ("C300", "A100", "D400", "B200"): + for course_code in course_codes: 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")) + count, pages = collect_pages(user_client, blocked_courses_url(inference.id, page_size="2", **params)) 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", - ] + assert [row["course_key"] for page in pages for row in page] == expected def test_blocked_courses_default_page_size_is_100( @@ -2227,12 +2233,19 @@ def test_blocked_courses_tag_outside_a_competency_taxonomy_404s(user_client: API assert response.status_code == status.HTTP_404_NOT_FOUND +@pytest.mark.parametrize( + "query", + [ + pytest.param(urlencode({"course_keys": "not-a-key"}), id="malformed course_keys"), + pytest.param(urlencode([("course_keys", "a"), ("course_keys", "b")]), id="repeated course_keys"), + ], +) def test_blocked_courses_without_view_access_403s( - api_client: APIClient, - user, + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, + query: str, ) -> None: """ A non-staff caller may not read blocked courses for a competency on a disabled taxonomy, even @@ -2242,15 +2255,11 @@ def test_blocked_courses_without_view_access_403s( make_leaf_criterion(critical_thinking, course_run, default_rule_profile, "p1") competency_taxonomy.enabled = False competency_taxonomy.save() - api_client.force_authenticate(user=user) - malformed = urlencode({"course_keys": "not-a-key"}) - repeated = urlencode({"course_keys": ["a", "b"]}, doseq=True) - for query in (malformed, repeated): - response = api_client.get(f"{blocked_courses_url(inference.id)}?{query}") + response = user_client.get(f"{blocked_courses_url(inference.id)}?{query}") - assert response.status_code == status.HTTP_403_FORBIDDEN - assert "results" not in response.data + 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( @@ -2280,24 +2289,6 @@ def test_blocked_courses_malformed_course_key_400s( assert "not-a-key" in str(response.data["course_keys"]) -@pytest.mark.parametrize("library_key", ["library-v1:Org1+Lib1", "library-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa"]) -def test_blocked_courses_library_key_400s( - user_client: APIClient, - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - default_rule_profile: CompetencyRuleProfile, - library_key: str, -) -> None: - """A library key parses as a CourseKey but can never name a course run, so it is rejected like a malformed one.""" - _, 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=library_key)) - - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert library_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, @@ -2318,18 +2309,11 @@ def test_blocked_courses_blank_course_keys_is_an_empty_page( def test_blocked_courses_repeated_course_keys_parameter_400s( - user_client: APIClient, - competency_taxonomy: CompetencyTaxonomy, - course_run: CourseRun, - organization: Organization, - default_rule_profile: CompetencyRuleProfile, + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, ) -> 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) + """Supplying course_keys twice is rejected instead of silently using one value.""" + _, _, inference, _ = make_skill_family(competency_taxonomy) + query = urlencode([("course_keys", "course-v1:Org+A+1"), ("course_keys", "course-v1:Org+B+1")]) response = user_client.get(f"{blocked_courses_url(inference.id)}?{query}") @@ -2337,29 +2321,6 @@ def test_blocked_courses_repeated_course_keys_parameter_400s( 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. - - This pins the database match, which ignores a key's branch, so it holds with or without the - serializer's normalization. - """ - _, 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, From 25d598c24c8410ad1250d21382dc737773028b4a Mon Sep 17 00:00:00 2001 From: alezconsultant Date: Fri, 13 Nov 2026 16:13:14 +0400 Subject: [PATCH 9/9] refactor: rename the live-criteria course lookup and mark it UNSTABLE get_courses_with_relative_criteria is now get_courses_with_live_criteria. The function returns the courses where the tag ids it is given have live criteria, and it computes no ancestors or descendants itself. Its docstring gains the UNSTABLE paragraph that every other function in the criteria-authoring family carries. Nothing in openedx-platform calls the function. Related to https://github.com/openedx/openedx-core/issues/796 Co-Authored-By: Claude Fable 5.2 --- src/openedx_learning/applets/cbe/api.py | 11 ++-- .../openedx_learning/applets/cbe/test_api.py | 50 +++++++++---------- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 4f8e1da2d..18a553c53 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -51,7 +51,7 @@ "get_blocked_courses", "get_competency_criteria_tree", "get_competency_rule_profiles", - "get_courses_with_relative_criteria", + "get_courses_with_live_criteria", "is_competency_taxonomy", "record_graded_object_statuses", "resolve_competency_tag", @@ -338,12 +338,15 @@ def resolve_supplied_leaf_group(group_id: int, tag: Tag, course_run: CourseRun) return group -def get_courses_with_relative_criteria( +def get_courses_with_live_criteria( tag_ids: Iterable[int], course: CourseRun | None = None, *, course_keys: Iterable[CourseKey] | None = None, ) -> dict[CourseRun, Tag]: """ Return every course where one of ``tag_ids`` has a live CompetencyCriterion. + UNSTABLE: the criteria-authoring family is incomplete, so its group update and archive + behavior may change this function's shape without a deprecation cycle. + Maps each conflicting CourseRun to the lowest-id tag (of ``tag_ids``) with a criterion there, narrowed by ``course`` and by ``course_keys``, each when given. Archived criteria and criteria under an archived group are ignored. @@ -409,7 +412,7 @@ def get_blocked_courses(tag: Tag, course_keys: Iterable[CourseKey] | None = None ancestor_and_descendant_ids = _get_ancestor_and_descendant_tag_ids(tag) if not ancestor_and_descendant_ids: return [] - conflicts = get_courses_with_relative_criteria(ancestor_and_descendant_ids, course_keys=course_keys) + conflicts = get_courses_with_live_criteria(ancestor_and_descendant_ids, course_keys=course_keys) rows = [] for course_run, conflicting_tag in conflicts.items(): # Typed optional, but save() always sets it. @@ -433,7 +436,7 @@ def _validate_containment(group: CompetencyCriteriaGroup, course: CourseRun) -> ancestor_and_descendant_ids = _get_ancestor_and_descendant_tag_ids(tag) if not ancestor_and_descendant_ids: return - conflicting_tag = get_courses_with_relative_criteria(ancestor_and_descendant_ids, course=course).get(course) + conflicting_tag = get_courses_with_live_criteria(ancestor_and_descendant_ids, course=course).get(course) if conflicting_tag is not None: raise ValidationError({ "tag_id": _( diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index 396116d6e..c7a2f7cea 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -29,7 +29,7 @@ get_blocked_courses, get_competency_criteria_tree, get_competency_rule_profiles, - get_courses_with_relative_criteria, + get_courses_with_live_criteria, is_competency_taxonomy, resolve_supplied_leaf_group, select_competency_taxonomies, @@ -1731,17 +1731,17 @@ def test_get_competency_criteria_tree_scoped_keeps_every_group_above_any_course( assert {g.id for g in empty.groups} == {root.id, mid.id} -def test_get_courses_with_relative_criteria_single_match( +def test_get_courses_with_live_criteria_single_match( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """A single tag with a live criterion in a course is returned, mapped to that tag.""" relative = make_tag(competency_taxonomy, "Relative Skill") make_leaf_criterion(relative, course_run, default_rule_profile, "p1") - assert get_courses_with_relative_criteria([relative.id]) == {course_run: relative} + assert get_courses_with_live_criteria([relative.id]) == {course_run: relative} -def test_get_courses_with_relative_criteria_ties_break_on_the_lowest_tag_id( +def test_get_courses_with_live_criteria_ties_break_on_the_lowest_tag_id( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """ @@ -1754,27 +1754,27 @@ def test_get_courses_with_relative_criteria_ties_break_on_the_lowest_tag_id( 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]) + result = get_courses_with_live_criteria([second.id, first.id]) assert result == {course_run: first} -def test_get_courses_with_relative_criteria_empty_input_yields_an_empty_result() -> None: +def test_get_courses_with_live_criteria_empty_input_yields_an_empty_result() -> None: """An empty tag_ids collection yields an empty result, not an error.""" - assert not get_courses_with_relative_criteria([]) + assert not get_courses_with_live_criteria([]) -def test_get_courses_with_relative_criteria_excludes_a_group_with_no_live_criterion( +def test_get_courses_with_live_criteria_excludes_a_group_with_no_live_criterion( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, ) -> None: """A CompetencyCriteriaGroup for a relative tag with no attached criterion is not a conflict.""" relative = make_tag(competency_taxonomy, "Empty Relative") _create_leaf_group(relative, course_run) - assert not get_courses_with_relative_criteria([relative.id]) + assert not get_courses_with_live_criteria([relative.id]) -def test_get_courses_with_relative_criteria_ignores_an_archived_criterion( +def test_get_courses_with_live_criteria_ignores_an_archived_criterion( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """An archived criterion is gone for authoring, so it blocks nothing.""" @@ -1782,10 +1782,10 @@ def test_get_courses_with_relative_criteria_ignores_an_archived_criterion( criterion = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") CompetencyCriterion.objects.filter(pk=criterion.pk).update(archived=True) - assert not get_courses_with_relative_criteria([relative.id]) + assert not get_courses_with_live_criteria([relative.id]) -def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archived_leaf_group( +def test_get_courses_with_live_criteria_ignores_a_criterion_under_an_archived_leaf_group( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """A live criterion under an archived leaf group blocks nothing.""" @@ -1793,10 +1793,10 @@ def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archive criterion = make_leaf_criterion(relative, course_run, default_rule_profile, "p1") CompetencyCriteriaGroup.objects.filter(pk=criterion.group_id).update(archived=True) - assert not get_courses_with_relative_criteria([relative.id]) + assert not get_courses_with_live_criteria([relative.id]) -def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archived_course_level_group( +def test_get_courses_with_live_criteria_ignores_a_criterion_under_an_archived_course_level_group( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """A live criterion under an archived course-level group blocks nothing.""" @@ -1806,10 +1806,10 @@ def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archive assert course_level_id is not None CompetencyCriteriaGroup.objects.filter(pk=course_level_id).update(archived=True) - assert not get_courses_with_relative_criteria([relative.id]) + assert not get_courses_with_live_criteria([relative.id]) -def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archived_root_group( +def test_get_courses_with_live_criteria_ignores_a_criterion_under_an_archived_root_group( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """A live criterion under an archived root group blocks nothing.""" @@ -1821,10 +1821,10 @@ def test_get_courses_with_relative_criteria_ignores_a_criterion_under_an_archive assert root_id is not None CompetencyCriteriaGroup.objects.filter(pk=root_id).update(archived=True) - assert not get_courses_with_relative_criteria([relative.id]) + assert not get_courses_with_live_criteria([relative.id]) -def test_get_courses_with_relative_criteria_counts_a_course_with_one_live_and_one_archived_criterion( +def test_get_courses_with_live_criteria_counts_a_course_with_one_live_and_one_archived_criterion( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """Archiving one of a tag's two criteria in a course leaves the course blocked by the other.""" @@ -1833,7 +1833,7 @@ def test_get_courses_with_relative_criteria_counts_a_course_with_one_live_and_on make_leaf_criterion(relative, course_run, default_rule_profile, "p2") CompetencyCriterion.objects.filter(pk=archived.pk).update(archived=True) - assert get_courses_with_relative_criteria([relative.id]) == {course_run: relative} + assert get_courses_with_live_criteria([relative.id]) == {course_run: relative} def test_validate_containment_rejects_an_ancestor_conflict( @@ -1893,7 +1893,7 @@ def test_validate_containment_allows_a_tag_with_no_relatives(tag: Tag, course_ru cbe_api._validate_containment(group, course_run) # pylint: disable=protected-access # Must not raise. -def test_get_courses_with_relative_criteria_narrows_to_the_given_course( +def test_get_courses_with_live_criteria_narrows_to_the_given_course( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, organization: Organization, @@ -1905,10 +1905,10 @@ def test_get_courses_with_relative_criteria_narrows_to_the_given_course( make_leaf_criterion(relative, course_run, default_rule_profile, "p1") make_leaf_criterion(relative, other_course_run, default_rule_profile, "p1") - assert get_courses_with_relative_criteria([relative.id], course=course_run) == {course_run: relative} + assert get_courses_with_live_criteria([relative.id], course=course_run) == {course_run: relative} -def test_get_courses_with_relative_criteria_narrows_to_the_given_course_keys( +def test_get_courses_with_live_criteria_narrows_to_the_given_course_keys( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, organization: Organization, @@ -1921,14 +1921,14 @@ def test_get_courses_with_relative_criteria_narrows_to_the_given_course_keys( for course in (course_run, course_y, course_z): make_leaf_criterion(relative, course, default_rule_profile, "p1") - result = get_courses_with_relative_criteria( + result = get_courses_with_live_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( +def test_get_courses_with_live_criteria_ignores_a_criterion_outside_a_leaf( competency_taxonomy: CompetencyTaxonomy, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, ) -> None: """A criterion on a group nested below a leaf, e.g. made through the admin, is skipped rather than crashing.""" @@ -1940,7 +1940,7 @@ def test_get_courses_with_relative_criteria_ignores_a_criterion_outside_a_leaf( ) CompetencyCriterion.objects.create(group=nested, object_tag=object_tag, rule_profile=default_rule_profile) - assert not get_courses_with_relative_criteria([relative.id]) + assert not get_courses_with_live_criteria([relative.id]) def test_create_competency_criterion_uses_the_callers_course_run(