[alezconsultant]

Description

Implements #796: a read-only endpoint that lists the courses where a
taxonomy ancestor or descendant of a competency already has a live CompetencyCriterion. Along
any ancestor-descendant line of the taxonomy, a course may hold live criteria for one competency
only, so these are the courses where the competency can't take a new criterion. The authoring UI
(#797) will use the list to gray those courses out before the
create-criterion endpoint rejects them.

GET /api/cbe/v1/competencies/<int:tag_id>/blocked-courses/[?course_keys=<key>,<key>]

Each row carries the blocked course's key and the conflicting tag's id and value:

{"count": 1, "next": null, "previous": null, "results": [
  {"course_key": "course-v1:Org+Course+Run", "conflicting_tag_id": 12, "conflicting_tag_value": "Critical Thinking"}
]}

The read side and the write side share one lookup. get_blocked_courses and the create-criterion
path both build the competency's set of ancestors and descendants with _get_ancestor_and_descendant_tag_ids and both call
get_courses_with_live_criteria, so the two always agree on which courses are blocked and
which tag is named. That includes the handling of archived rows: an archived criterion, or one
under an archived group, blocks nothing in either place.

Changes

• src/openedx_learning/applets/cbe/api.py: adds get_blocked_courses(tag, course_keys=None)
  and the frozen BlockedCourse dataclass, both UNSTABLE and exported on openedx_learning.api.
  get_courses_with_live_criteria gains an optional keyword-only course_keys argument so
  the course filter runs in the database instead of in Python.
• rest_api/v1/views.py, urls.py, rest_api/paginators.py: adds CompetencyBlockedCoursesView
  and CompetencyBlockedCoursesPagination (page size 100, max 500, pinned like
  CompetencyRuleProfilePagination). Authentication and permission match the criteria-tree view.
• rest_api/v1/serializers.py: adds BlockedCourseSerializer. The view reuses
  CompetencyCriteriaQueryParamsSerializer for course_keys, so both endpoints parse that
  parameter by one contract: supplied once, at most 100 keys, and each key is normalized to its
  plain course key, and a key that is not a course key (a library key, for instance) is a 400.
  Three responses of the existing criteria-tree endpoint change as a result, all fixes under its
  UNSTABLE marker: a branch-qualified key now scopes to its plain course where it previously
  scoped nothing, a version-only key now returns 400 where it previously raised from the database
  layer, and a library key now returns 400 where it was previously dropped silently, or raised
  from the database layer when it carried only a version.
• docs/openedx_learning/decisions/0002-competency-criteria-model.rst: Decision 4 now states that
  no ancestor or descendant of a competency tag may hold live criteria in the same course as that
  tag, and why.
• Tests: the direction and sibling scenarios of the acceptance criteria are tested at the API
  layer in tests/openedx_learning/applets/cbe/test_api.py, together with the function's own
  branches and one test that the read and the create path report the same conflict. The view
  tests keep one row-shape case plus the parsing, pagination, and permission scenarios. The
  shared tag-family, leaf-criterion, course-run and usage-key helpers live in
  tests/openedx_learning/applets/cbe/utils.py.

Testing instructions

Automated: pytest tests/openedx_learning/applets/cbe --no-cov.

Manual, against a Tutor devstack with this branch's openedx-core mounted. The CBE API is
already mounted under api/ by cms/urls.py, so no URL change is needed. Run
tutor dev exec cms ./manage.py cms migrate and restart CMS. Log in as a staff user and get a JWT
Authorization header, then use three real course keys as Course X, Y and Z. A course key
contains +, which a URL decodes as a space, so percent-encode it as %2B in every
course_keys value below, or let curl -G --data-urlencode do it.

1. Create a competency taxonomy (POST /api/content_tagging/v1/taxonomies/ with
   "taxonomy_type": "competency") with the tags "Cognitive Skills" > "Critical Thinking" >
   "Inference", plus "Analysis" under "Critical Thinking" as a sibling of "Inference". Create each
   tag with POST /api/content_tagging/v1/taxonomies/<taxonomy_id>/tags/, passing the name as
   tag and the parent's name as parent_tag_value, top down.
2. Create criteria with POST /api/cbe/v1/competencies/<tag_id>/criteria/: "Critical Thinking" in
   Course X, "Cognitive Skills" in Course Y, "Analysis" in Course Z. Each criterion's object_id
   is a gradeable subsection in that course, of the form
   block-v1:<Org>+<Course>+<Run>+type@sequential+block@<id>.
3. GET /api/cbe/v1/competencies/<inference_id>/blocked-courses/. Expect 200 with Course X naming
   "Critical Thinking" and Course Y naming "Cognitive Skills". Course Z doesn't appear, because
   "Analysis" is a sibling of "Inference".
4. GET the same endpoint for "Critical Thinking". Expect Course Y naming "Cognitive Skills" and
   Course Z naming "Analysis". Course X doesn't appear, because a tag's own criterion doesn't
   block it.
5. Repeat step 3 with ?course_keys=<Course X>. Expect only Course X, with count 1. Repeat with
   ?course_keys=<Course X>+branch@draft. Expect the same result.
6. Repeat step 3 with ?page_size=1. Expect count 2, one row, and a next link that returns
   the other course.
7. Repeat step 3 with ?course_keys=not-a-key and with ?course_keys=<Course X>&course_keys=<Course Y>.
   Expect 400 for both, as for ?course_keys=course-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa and
   ?course_keys=library-v1:Org+Lib. Repeat with ?course_keys=. Expect 200 with count 0.
8. GET the endpoint for tag_id=999999. Expect 404.
9. Disable the taxonomy and GET the endpoint as a non-staff user. Expect 403. Log out and GET
   it again. Expect 401.

Other information

• Public API: one new UNSTABLE function and one new UNSTABLE dataclass on openedx_learning.api,
  and one new optional keyword-only argument on get_courses_with_live_criteria. The
  criteria-tree endpoint, which is UNSTABLE, changes how it treats a branch- or version-qualified
  course key, as described under Changes.
• get_courses_with_relative_criteria is renamed to get_courses_with_live_criteria and now carries
  the UNSTABLE paragraph. It shipped in v1.17.0 without the paragraph, which every other function in
  the criteria-authoring family has, and nothing in openedx-platform calls it.
• AI usage: implemented with Claude Code (Claude Opus 5.5 and Claude Fable 5.1), then reviewed and
  tested by the author.

Related to #796

🤖 Generated with the help of Claude Code

Summary by CodeRabbit

• New Features
  • Added a paginated view of courses where criteria on a competency’s ancestors or descendants prevent adding criteria to that competency. Results can be filtered by course key.
• Updates
  • Course-key filters now ignore branch and version details; version-only and library keys are rejected.
  • Criteria on a competency and its ancestors or descendants cannot be active in the same course, avoiding ambiguity in the competency hierarchy.
• Documentation
  • Documented the restriction on criteria across related competency levels.

[openedx-webhooks]

Thanks for the pull request, @alezconsultant!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[tbain]

Very nice, I appreciate how concise this implementation is; I feel like it could have gotten kind of bloated considering the problem statement.

I had one human nit about a comment, though T3 (Claude/Fable+GPT/Sol Astra) found a couple of issues that all seem relevant to address;

---

Review Details

Prerequisites, items left to other tickets, what passed, and the action list

Prerequisites (skip if you know the codebase)

The models. A Tag belongs to a Taxonomy; a taxonomy whose rows are competencies is a CompetencyTaxonomy. A CompetencyCriteriaGroup is a node in a per-competency tree: a root group for the tag, a course-level group under it that carries a course foreign key to a CourseRun, and leaf groups under that whose course is null and which belong to the course through their parent. A CompetencyCriterion hangs off a leaf group and points at an ObjectTag, which tags a subsection in a course. "Live" means the row and every group above it have archived=False (source: src/openedx_learning/applets/cbe/models/criteria.py and ADR 0002 in docs/openedx_learning/decisions/).

The dominance rule. Ticket #666 (merged as PR #875) forbids a course from holding live criteria for two tags where one is a taxonomy ancestor of the other. The function get_courses_with_relative_criteria(tag_ids, course=None) in applets/cbe/api.py returns, for a set of tag ids, every CourseRun that already has a live criterion on one of them, mapped to the lowest-id such tag. The write path calls it with one course to reject a new criterion. This PR calls it without a course to list every blocked course. Ticket #796 calls this endpoint the "read-only counterpart" of #666.

The sibling endpoint. CompetencyCriteriaTreeView (ticket #681, PR #878, then PR #911) is GET competencies/<tag_id>/criteria-tree/. It already accepts a comma-separated course_keys query parameter, parsed by CompetencyCriteriaQueryParamsSerializer in rest_api/v1/serializers.py, and it rejects a repeated parameter in its view. This PR adds a second serializer for the same parameter name.

The permission. CompetencyReadPermission in rest_api/v1/permissions.py checks oel_tagging.view_taxonomy on the tag's taxonomy. In the platform, that rule grants access to any registered user for an enabled all-org taxonomy, and to org staff and course creators for their org's taxonomies.

How the route reaches Studio. openedx-platform/cms/urls.py includes openedx_learning.urls under api/, which includes the cbe routes under cbe/. The new route is therefore api/cbe/v1/competencies/<tag_id>/blocked-courses/, with no platform change.

The UNSTABLE marker. A sentence in a docstring beginning "UNSTABLE:" declares that the API may change without the deprecation process (source: applets/cbe/api.py, for example on CompetencyCriteriaTree).

The second-model adversarial reviewer. A separate review pass, run by a different model, whose task was to look for faults in the PR (unsourced; inferred from the name).

How the review tested the tests. For each acceptance scenario, the reviewer edited the code in a throwaway worktree to break the behavior, for example by dropping the descendant half of the relative set, removing the permission check, or removing pagination. The reviewer then ran the new tests and recorded which ones failed and why. A test that stays green while the behavior is broken does not count as coverage.

Things the review looked at and is leaving to other tickets

• Criteria under deeper group trees are not seen. The helper's filter requires a leaf's immediate parent to carry a course, so a leaf under an intermediate group is skipped, and ADR 0002 anticipates deeper trees. The write path rejects such a leaf today, so the product cannot create the data, and read and write agree. Whichever ticket introduces deeper authoring will have to widen the helper's filter, which does not undo anything in this change.
• ADR 0002 says archiving a course-level group blocks the course for that competency, and the create path does not enforce that. This behavior is already on main and is outside #796, whose user story is about the rule from #666. Only the docstring wording in the last "Small things" item is on this change.
• Cross-org disclosure is shared with the criteria-tree endpoint, as the second decision below describes. Any fix applies to the whole family of endpoints.

What passed

The branch merges cleanly onto current main, with the one overlapping api.py hunk merging automatically. makemigrations --check reports no changes, the import linter keeps all three contracts, and the 328 tests in the two cbe test files pass both at the PR head and on the merged tree. The PR's stated command runs 579 tests and all pass. Pylint, pycodestyle, isort, and mypy are clean on the changed files, and CI is green. All 11 acceptance scenarios have an endpoint test that failed when the reviewer broke the behavior. The second-model adversarial reviewer found the permission ordering, the 404 mapping, the archived-row handling, and the page ordering solid.

Action list

1. Add the repeated-parameter guard to CompetencyBlockedCoursesView.get and a test for it.
2. Reuse CompetencyCriteriaQueryParamsSerializer, or add the 100-key cap and decide the blank-value behavior deliberately.
3. Export BlockedCourse in __all__, make it frozen=True, kw_only=True, slots=True, add the UNSTABLE sentence, and construct it with keywords.
4. Add an anonymous 401 test.
5. Rewrite the PR title, description, and commit body to match the diff and the platform's existing URL include.
6. Decide on the three questions: the ADR amendment, the cross-org disclosure, and branch-qualified keys.
7. Work through the improvements and small things as you see fit.

Must fix before merge

When a client sends the course filter several times in one request, the endpoint silently uses only the last one.

Details

In CompetencyBlockedCoursesView.get, the query string is handed to the serializer as request.query_params.dict(). Django's QueryDict.dict() keeps only the last value of a key that appears more than once. The review sent ?course_keys=X&course_keys=Y with both courses blocked and got a 200 containing only Y. Swapping the order returned only X. The sibling CompetencyCriteriaTreeView guards against this: it checks request.query_params.getlist("course_keys") and returns 400 "Supply course_keys once, as a comma-separated list." A frontend that builds its query string with one parameter per course, which is a common pattern, would be told a blocked course is free, which is the one thing this endpoint exists to prevent.

Fix: copy the sibling's guard into the new view and add a test for a repeated parameter.

The new endpoint reads the course filter by different rules than the existing endpoint that reads the same filter, and only the existing one limits its size.

Details

The new CompetencyBlockedCoursesQueryParamsSerializer and the existing CompetencyCriteriaQueryParamsSerializer differ in four ways.

• Cap. The new one has no cap on the number of keys. The sibling refuses more than MAX_COURSE_KEYS (100). The review sent 2,000 keys and 100,000 characters to the new endpoint and got 200 both times.
• Blank values. The new one returns 400 for a blank value or a value of only commas. The sibling declares allow_blank=True, ignores stray separators, and treats a blank filter as "scope to nothing".
• Duplicates. The new one does not deduplicate, which is harmless because the filter is a set.
• Lint directive. The new one suppresses pylint's abstract-method warning with a directive. The sibling defines create and update methods that raise NotImplementedError.

Two endpoints under one prefix now give one parameter name two contracts, and only one of them is bounded.

Fix: the simplest option is to reuse CompetencyCriteriaQueryParamsSerializer. A blank course_keys then yields an empty list and an empty page, which is coherent. If the 400 on a blank filter is a deliberate choice, keep the new serializer, add the cap, and say in the view docstring why blank is an error here.

The row type returned by a public function is not exported from the public API, and it lacks its neighbor's frozen dataclass settings and UNSTABLE marker.

Details

get_blocked_courses is added to __all__ in applets/cbe/api.py, and openedx_learning/api.py re-exports that module with a star import, so platform code reaches openedx_learning.api.get_blocked_courses. BlockedCourse is not in __all__, so openedx_learning.api.BlockedCourse does not exist. A caller who wants to annotate list[BlockedCourse] must import from the applets path that the package's own urls.py docstring says is meant to stay internal. The PR's own tests do exactly that. The dataclass is also a plain mutable @dataclass with no UNSTABLE sentence, while its neighbor CompetencyCriteriaTree in the same module is declared @dataclass(frozen=True, kw_only=True, slots=True) and carries the UNSTABLE marker.

Fix: add BlockedCourse to __all__, match the neighbor's decorator arguments and marker, and construct it with keyword arguments where get_blocked_courses builds rows.

No test checks that an anonymous caller is refused.

Details

The review removed IsAuthenticated from the view's permission classes and all 30 new tests still passed. Only a throwaway probe showed that an anonymous request gets 401 today. Every sibling view in test_views.py has such a test, for example test_anonymous_request_is_401_and_discloses_nothing for the criteria tree, and the maintainer asked for one when reviewing PR #878.

Fix: add one test that an unauthenticated GET returns 401 and that the body has no results key.

The pull request title, description, and commit message describe a different diff and setup than the one submitted.

Details

• The Changes list says rest_api/v1/permissions.py adds CompetencyReadPermission. The diff touches seven files and that is not one of them, because PR #878 merged on 2026-10-27 and the class is already on the base commit.
• The paragraph about the class being "copied verbatim from #878" and "whichever PR merges second" is stale for the same reason, and the commit message repeats it.
• "Built on top of #875 (pr)" has a stray "(pr)".
• The manual-testing preamble tells the tester to add path('api/cbe/', include('openedx_learning.urls')) to cms/urls.py. openedx-platform already includes openedx_learning.urls under api/, so following the instruction would mount the API a second time at api/cbe/cbe/.
• The PR title is the branch name. Merged PRs in this repository use a conventional-commit sentence, and the commit subject feat: add blocked-courses endpoint for a competency is the right title.

The maintainer asked on #875, #878, and #911 to bring the description up to date before merge.

Decisions to make or write down

The rule that blocks these courses, and the fact that this endpoint shares the write side's lookup, are recorded in no architecture decision record.

Details

No file under docs/ mentions the rule. It lives in ticket #666 and in code. Ticket #666 justified the rule by saying there is no defined precedence between a parent's own criterion and a rollup from a child's. A commit now on main (136bcd0) amends ADR 0002 Decision 6 to define exactly that precedence, "the better of two inputs". This PR makes the rule visible to authors, which makes it costlier to change later.

The reviewer suggests a Proposed amendment to ADR 0002, outside this PR, stating five things: the rule, that the course comes from the leaf's parent group, that archived rows do not block, that the check runs at creation time only, and that the read-side hint shares the write-side lookup so the two cannot disagree. This is a question for the maintainers, not a blocker.

Any registered user can list, across the whole instance, the courses where a competency's relatives are used, which the sibling endpoint already allows.

Details

The permission is the taxonomy view rule, which in the platform passes for any registered user on an enabled all-org taxonomy, and the response is not filtered by any course or org permission. The sibling criteria-tree endpoint already discloses each criterion's course key and subsection id under the same rule, so this PR adds no new class of exposure. If that exposure is unwanted, the fix belongs to the whole family of endpoints, not this one.

A course key that carries a branch or version never matches here, although it matches on the sibling endpoint, and it is unconfirmed whether any client sends such keys.

Details

get_blocked_courses tests membership with Python equality on CourseKey objects, and course-v1:O+C+R+branch@draft is not equal to course-v1:O+C+R. The sibling criteria-tree endpoint resolves course_keys through a database lookup, where the opaque-keys field strips branch and version, so it matches. Sending ?course_keys=course-v1:O+C+R+branch@draft returns an empty page here and the course on the sibling. Studio URLs carry plain course ids, so whether a client would ever send a qualified key is unconfirmed. This is a question rather than a defect. The second-model adversarial reviewer found it.

Fix, if you want them to match: normalize each key with for_branch(None).version_agnostic() in the serializer, or resolve through get_course_run_ids as the sibling does.

Improvements worth doing

The docstring of the main lookup function leaves out three behaviors that callers can observe.

Details

Three behaviors are visible to callers and documented only on the callee or in tests. When several relatives claim one course, the lowest-id relative is named. The requested tag's own criteria never block a course. Requested course keys that match no blocked course are dropped.

Fix: say in the get_blocked_courses docstring what callers can observe but cannot read from the signature. One sentence covers all three.

The endpoint loads every conflicting row on the instance even when the request names only a few courses.

Details

get_blocked_courses calls get_courses_with_relative_criteria without a course and filters in Python. A request for one course with page_size=1 therefore still loads every conflicting group row on the instance, joined to its tag, parent, and course run. The query count is constant and the ticket's own description accepts this as response-shaping only, but the maintainer raised the same objection on PR #911 for the sibling.

Fix: consider extending the helper to accept a set of courses, so both endpoints can scope the query in the database.

The view-level pagination tests do not check the order of results.

Details

The two view pagination tests compare sets of keys. With the sort removed, only the API-level sort test fails. Ordering is what makes "none repeated, none skipped" true across separate page requests.

Fix: compare the concatenated keys from all pages with a literal ordered list.

One sort test computes its expected value the same way the code does.

Details

test_get_blocked_courses_sorts_rows_by_course_key asserts keys == sorted(keys), which recomputes what the code does.

Fix: build the expected list from the three known course codes.

Two API-level tests repeat behavior that another function's tests already cover.

Details

test_get_blocked_courses_ignores_an_archived_claim and test_get_blocked_courses_reports_one_row_per_course_naming_the_lowest_id_relative assert behaviors that get_courses_with_relative_criteria already has tests for in the same file.

Fix: drop the two tests and keep the view-level archived test as a wiring check.

Nine scenarios are tested at both the API layer and the view layer, although either layer fails on the same breaks.

Details

The API suite repeats ancestor, descendant, grandparent, grandchild, sibling, unaffected course, narrowing, empty result, and archived. PR #875 split by concern: API tests for the function's own branches, view tests for the acceptance scenarios.

Fix: at the API layer, keep the union of ancestors and descendants, narrowing, sort, row shape, and the early return.

Four direction tests in each test file differ only in their data.

Details

Ancestor, descendant, grandparent, and grandchild differ only in which tag holds the criterion and which tag is requested.

Fix: parametrize the four tests in each file. The repository already uses pytest.mark.parametrize.

It is undecided whether an early return that only saves one query should stay, since removing it fails no test.

Details

get_courses_with_relative_criteria already returns an empty mapping for an empty iterable, and deleting the if not relative_ids: return [] branch leaves every test green. The branch saves one query. If it stays, note that the "tag with no relatives" test is really testing the callee.

Small things

• One assertion in the new lookup function has no failure message, although the assertions beside it carry one.

  Details

  The assert course_run.course_key is not None in get_blocked_courses has no message. The two asserts a few lines above it in get_courses_with_relative_criteria carry the row id, and the maintainer asked for that on PR #875.

• A pagination class docstring points at a reason without stating it.

  Details

  The CompetencyBlockedCoursesPagination docstring, "pinned for the same reason as the rule profiles'", points at a reason instead of stating it and ends in a dangling possessive. State the reason in one sentence, as the sibling class does.

• The view docstring omits the 401 for anonymous callers and does not mention that an out-of-range page also returns 404.

  Details

  The view docstring lists 400, 403, and 404. An out-of-range page also gives 404 (Django REST framework's InvalidPage), and an anonymous caller gets 401.

• No test pins the default page size.

  Details

  The review set the default page size of 100 to 10 and every test passed. The maximum is covered indirectly. A test of a constant is borderline, so this is a note.

• One assertion in a test cannot fail independently of the assertion after it.

  Details

  In test_blocked_courses_ignores_an_unaffected_course, the not in assertion cannot fail independently of the equality assertion on the next line.

• Test helpers are duplicated across the two test files, and one helper is used inconsistently.

  Details

  make_skill_family is identical in both test files. make_leaf_criterion is copied into test_views.py while test_api.py already has it. The new course_key() helper is used in three places while other new lines read course_run.course_key directly.

• Two lint-suppression directives were added, and the project's rules allow these only with explicit approval.

  Details

  The change adds two # pylint: disable=abstract-method directives. The project's instructions say a suppression directive needs explicit approval. The module has precedent, and the nearest sibling avoids it by defining create and update. Reusing the sibling serializer removes one.

• A stability note on the view words its phrase differently from the sibling view.

  Details

  The UNSTABLE sentence on the view says "this shape"; the sibling view says "this response shape".

• Two docstrings claim more than the endpoint reports about which courses cannot get a new criterion.

  Details

  The docstrings on get_blocked_courses and the view say the courses "can't get a new criterion". That overstates: the endpoint reports only the ancestor-or-descendant conflict, and ADR 0002 uses "blocks that course" for a different case, an archived course-level group. Say "where an ancestor or descendant already has a live criterion".

[tbain]

I feel like this description is a little vague; "where a competency can't get a new criterion," could be clarified as to why it can't get a new Criterion? I think the next paragraph kind of touches on it, but I feel like this would be the place to concisely explain the 'why'

[alezconsultant]

Thanks for the review. I tried to address each item, pr title and description also has been updated. I will update commit message on squash step. Brief summary:

1. Sending course_keys more than once now returns 400, with the same guard and message the criteria-tree view uses.
2. The view now uses CompetencyCriteriaQueryParamsSerializer instead of its own, so course_keys works the same way on both endpoints: one parameter, up to 100 keys, and a blank value means an empty page rather than an error.
3. BlockedCourse is now frozen, keyword-only and slotted like CompetencyCriteriaTree, has the UNSTABLE sentence, is exported from openedx_learning.api, and is built with keyword arguments.
4. There's a test that an anonymous request gets 401 and no results.
5. The shared serializer now strips branch and version from every key, so course-v1:O+C+R+branch@draft matches course-v1:O+C+R on both endpoints.
6. get_courses_with_relative_criteria takes an optional course_keys argument and get_blocked_courses passes it through, so a request for one course no longer loads every conflicting row. The course argument the create path uses is unchanged.
7. The get_blocked_courses docstring now says what a caller can observe but not read from the signature: when several relatives claim one course the lowest-id one is named, the tag's own criteria never block a course, and requested keys that match nothing are dropped.
8. The view docstring explains why a course is blocked, and lists every status code including 401 and the out-of-range page 404. BlockedCourseSerializer defines create and update. The course_key assert has a message, the UNSTABLE sentence says "response shape", and both docstrings say "where an ancestor or descendant already has a live criterion".
9. The pagination tests compare all pages against an ordered list, the sort test spells out its expected order, the four direction tests are one parametrized test per file, and the API suite keeps only the function's own branches now that the view tests cover the scenarios. The redundant not in assertion is gone. The shared helpers moved to a utils.py next to the tests. The early return stays, as the ticket asked for it, and the default page size test is left.
   As for your questions:

I agree with cross-org disclosure issue, since tag app already implement this flow, and we need to update a bunch of CBE endpoints to add this logic, i lean to follow up ticket. I can create it if you don't mind.

As for ADR 0002 amendment, i think it's better to do it in this pr as edit, rather than create follow up ticket, but i'm not against putting it in follow up ticket either.

[tbain]

This looks good to me from a human reviewer perspective. T3 had a few hopefully minor changes/findings that I think are worth addressing, though;

---

T3 Review:

Review Details

Prerequisites, first-round status, new issues, deferred items, what passed, and the action list

Prerequisites (skip if you know the codebase)

The models. A Tag belongs to a Taxonomy; a taxonomy of competencies is a CompetencyTaxonomy. A CompetencyCriteriaGroup is a node in a per-competency tree: a root group for the tag, a course-level group under it with a course foreign key to a CourseRun, and leaf groups under that. A CompetencyCriterion hangs off a leaf group. "Live" means the criterion and every group above it have archived=False (source: src/openedx_learning/applets/cbe/models/criteria.py, ADR 0002).

The dominance rule. Ticket #666 (merged as PR #875) forbids a course from holding live criteria for two tags where one is a taxonomy ancestor of the other. get_courses_with_relative_criteria(tag_ids, course=None, course_keys=None) in applets/cbe/api.py returns, for a set of tag ids, every course that already has a live criterion on one of them, mapped to the lowest-id such tag. The write path calls it with one course to reject a new criterion; this PR's get_blocked_courses calls it with the optional course_keys filter the fix-up added, to list blocked courses.

The sibling endpoint. CompetencyCriteriaTreeView is GET competencies/<tag_id>/criteria-tree/. It accepts a comma-separated course_keys parameter through CompetencyCriteriaQueryParamsSerializer in rest_api/v1/serializers.py. The fix-up made the new view reuse that serializer and changed the serializer itself, so the change reaches both endpoints.

Course keys, branches and versions. A course key such as course-v1:Org+Course+Run can carry a branch (+branch@draft) or a version (+version@<hex>). The database field that stores CourseRun.course_key strips both whenever it prepares a value for a query (source: opaque_keys/edx/django/models.py, get_prep_value). Two Python CourseKey objects that differ only by branch are not equal, so a Python dictionary lookup does not strip. The fix-up added for_branch(None).version_agnostic() in the serializer so every key is plain before it reaches either endpoint's code.

The permission. CompetencyReadPermission checks oel_tagging.view_taxonomy on the tag's taxonomy. In the platform that passes for any registered user on an enabled all-org taxonomy.

UNSTABLE. A docstring sentence beginning "UNSTABLE:" declares that the API may change without the deprecation process (source: applets/cbe/api.py). get_courses_with_relative_criteria is the one criteria-family export without that sentence, so it is a stable contract.

ADR 0002. docs/openedx_learning/decisions/0002-competency-criteria-model.rst is the record of the criteria model. It is amended in place with dated Changelog entries. Commit 136bcd0 on main, which is not on this branch, rewrote its Decisions 2 and 6.

How the review tested the tests. For each behavior, the reviewer broke the code in a throwaway worktree (dropped a filter line, removed a permission class, reverted the normalization), ran the tests, and recorded which failed. A test that stays green while its behavior is broken is not coverage. The reviewer also ran the full cbe test package on the branch merged into current main. A "probe" is a throwaway script or test the reviewer ran to see how the code behaves. The "second-model adversarial reviewer" is a second AI model the reviewer ran to attack the PR independently. The "squash" is the merge step that combines the PR's commits into one, whose message becomes the release note.

Were the first-round findings addressed?

Yes, with three exceptions that are decisions rather than defects. The table below is the status of every first-round item.

First-round item	Status	What the head shows
Repeated course_keys silently used the last value	Fixed	The view rejects a repeated parameter with the sibling's message; the new test fails when the guard is removed.
Separate query-parameter serializer with no cap and a 400 on blank	Fixed	The view reuses the sibling serializer; cap, blank, and ,, are tested and fail when broken.
BlockedCourse not exported, mutable, no UNSTABLE marker	Fixed	Frozen, keyword-only, slotted, marked UNSTABLE, exported through openedx_learning.api, built with keywords.
No anonymous 401 test	Fixed	The new test fails when IsAuthenticated is removed.
Title, description and commit message described a different diff	Mostly fixed	Title and description now match the eight changed files and the real URL mount. The first commit's message is still stale; you said it will be fixed on squash.
Where the dominance rule is recorded (ADR)	Open	You prefer an ADR edit in this PR; nothing is pushed. See "Decisions to make or write down".
Cross-org disclosure	Deferred	You proposed a follow-up ticket; it is not filed yet. See "Decisions to make or write down".
Branch-qualified keys never matched here	Fixed	Normalization in the shared serializer. One correction to the first review: the sibling endpoint did not match such a key either, because its Python dictionary lookup after the database query dropped it. Your description's "previously scoped nothing" is right.
Docstring, database-side narrowing, ordered pagination asserts, literal sort expectation, parametrized direction tests	Fixed	As claimed.
Duplicate tests across layers	Partly	The API suite dropped four scenarios; the direction, sibling and narrowing cases are still at both layers.
Dead early return	Declined	Kept "as the ticket asked for it". The ticket says nothing about it; see "Improvements worth doing".
Assert message, paginator docstring, view docstring codes, redundant assertion, pylint disables, "response shape", "can't get a new criterion"	Fixed	As claimed.
Default page size untested	Declined	Your comment says the test "is left"; no such test exists, so the review reads that as "left out".
Duplicated helpers	Partly	Three helpers moved to utils.py; make_course_run is still in both files.
Tyler's inline comment: explain why a course is blocked	Fixed	The view docstring now states the rule.

Did the fix-up introduce new issues?

No new bug. The fix-up introduced one stale docstring, one understated claim about a behavior change, two untested branches, and a weakened test, all listed below. The full cbe package passes at the head (530 tests) and on the branch merged into current main (613 tests).

Things the review looked at and is leaving to other tickets

• Cross-org disclosure of course keys to any registered user: owned by the follow-up ticket you offered to file; it can tighten both endpoints without undoing anything here because both are UNSTABLE.
• Deeper group trees, and the archived course-level-group block from ADR 0002 Decision 2: unchanged since round one, pre-existing, owned elsewhere.
• Database-side versus Python-side narrowing: done; no test can distinguish the two and none is needed.

What passed

The branch merges cleanly onto current main, with the one overlapping api.py hunk merging automatically. On the merged tree, makemigrations --check reports no changes, the import linter keeps all three contracts, and the cbe test package passes with 613 tests. At the head, the PR's stated command runs 530 tests and all pass; pylint, pycodestyle, isort, mypy, and lint-imports are clean; CI is green. All 11 acceptance scenarios and all six first-round fixes have a test that failed when the reviewer broke the behavior. A probe of every malformed input the review could think of (version-only, library and CCX keys, NUL bytes, 5,000-character keys, 101 keys, repeated and blank parameters, bad page and page_size values, huge tag ids) returned 400, 404, or a documented 200, with 401 for anonymous callers and 403 for callers without permission; no 500 is reachable. The second-model adversarial reviewer found the permission ordering, the branch and version handling on both endpoints, the positional compatibility of the new argument, and the filtered pagination solid.

Action list

1. Fix the CompetencyCriteriaQueryParamsSerializer class docstring.
2. Replace "purely additive" in the description, add the normalization sentence to both view docstrings, and list both criteria-tree corrections in the squash commit body.
3. Parametrize the sibling branch test over branch, branch plus version, and version.
4. Add course_keys=not-a-key to the blocked-courses 403 test.
5. Decide on the ADR 0002 amendment (separate docs PR on current main recommended) and on in-process key matching for get_competency_criteria_tree.
6. File the cross-org disclosure ticket and link it, and link the frontend ticket once numbered.
7. Make course_keys keyword-only, swap the creation order in the tie-break test, and work through the other improvements and small things as you see fit.

Must fix before merge

Nothing else blocks the merge; once these four items are done the branch is ready.

A code comment that describes what a shared piece of code is for is now out of date.

Details

CompetencyCriteriaQueryParamsSerializer in rest_api/v1/serializers.py still says "Query params for the criteria-tree read endpoint." The fix-up made it serve the blocked-courses endpoint too. The project's instructions treat a stale comment as a bug. Change it to "Query params shared by the criteria-tree and blocked-courses read endpoints."

The PR description says the change only adds things, but it also changes how an existing endpoint responds.

Details

The description calls the change "purely additive", but the serializer change alters two responses of the existing criteria-tree endpoint, and neither view docstring mentions the normalization. The review reverted the normalization and two criteria-tree tests failed: a branch-qualified key went back to scoping nothing, and a version-only key went back to reaching the database layer instead of returning 400. The description mentions only the first. The criteria-tree view is marked UNSTABLE, so no deprecation is needed and both are fixes, but a reviewer reading "purely additive" would not look for a changed response on an endpoint the title does not name, and the squash commit body becomes the release note. Drop "purely additive" or qualify it, add one sentence to both view docstrings saying keys are matched without branch or version and rows carry the plain key, and list both corrections in the squash commit body.

One half of a new input clean-up step has no test that would notice if it broke.

Details

In validate_course_keys, the review replaced key.for_branch(None).version_agnostic() with key.for_branch(None) alone and all 530 tests passed. No test sends a key that carries both a branch and a version. Parametrize the sibling's test_a_branch_qualified_course_key_matches_the_plain_course over +branch@draft, +branch@draft+version@<hex>, and +version@<hex>.

No test checks that the new endpoint verifies the caller's permission before it examines the input.

Details

In CompetencyBlockedCoursesView.get, the review moved check_object_permissions below is_valid and every test stayed green. The sibling has test_no_view_access_with_course_keys_still_403s_without_leaking_data; the new 403 test sends no course_keys. The order is correct today, as a probe confirmed, and a reordering would let a forbidden caller learn whether a key parses. Send course_keys=not-a-key in test_blocked_courses_without_view_access_403s and assert 403.

Decisions to make or write down

The rule that stops a course from holding competing criteria is recorded only in a ticket and in code, not in the project's documentation, and where to record it is undecided.

Details

No document under docs/ states the ancestor-descendant rule; it lives in ticket #666 and in code. You offered to amend ADR 0002 in this PR. The repository does amend ADR 0002 in place with dated Changelog entries, including inside feature commits, so the offer has precedent. Two things argue for a separate docs PR on current main, linked from this one: commit 136bcd0 on main rewrote Decisions 2 and 6 of that file after this branch's base, so an edit here would conflict on rebase; and the rule's only written rationale, ticket #666's "no defined precedence between a parent's own criterion and a rollup", is now contradicted by Decision 6 on main, which defines that precedence as "the better of two inputs", so the rationale needs a maintainer's word. If the maintainer wants it in this PR, rebase first and keep it a separate commit. The amendment should state: the rule (along any ancestor-descendant line, live criteria in one course belong to one competency; siblings may coexist); that the course comes from the leaf's course-level parent group; that "live" means the whole chain is non-archived, so an archived claim blocks nothing; that the rule is checked at creation only, under the CompetencyCourseRun lock, with no database constraint; that the read endpoint shares the lookup, is advisory, and names the lowest-id relative; a rationale consistent with Decision 6; and that this "blocked" is different from Decision 2's archived-course-group block, which the endpoint does not report.

A follow-up ticket about disclosing course keys to users in other organizations was agreed but has not been filed.

Details

You asked whether you may create the ticket. Yes. Paste its URL into the description, and note in it that both endpoints are UNSTABLE, so tightening by org is cheap until the frontend ships against the current behavior. The second-model adversarial reviewer reproduced the disclosure with a probe-supplied object-level permission rule, which is a simulation rather than a Studio role test; it changes nothing about the first-round question.

Two functions that platform code calls directly treat the same unusual course identifier differently, and one kind of identifier makes both fail with an unsuitable error.

Details

get_blocked_courses(tag, [branched_key]) finds the course, because the database field strips the branch. get_competency_criteria_tree(tag, [branched_key]) still drops it, because after the database query it tests key in run_ids_by_key with Python equality. Only HTTP callers get the serializer's normalization, and the platform calls these functions in-process. A version-only key passed to either function raises InvalidKeyError from inside the database layer, where the repository's AGENTS.md asks for DoesNotExist, ValidationError, or an application exception. Decide between normalizing inside get_competency_criteria_tree as well, which is two lines, and stating in both API docstrings that keys are matched exactly and that a version-only key raises InvalidKeyError.

Improvements worth doing

A function that is already a stable public contract gained a new input that callers can now pass by position.

Details

get_courses_with_relative_criteria is the one criteria-family export with no UNSTABLE sentence and has shipped since v1.17.0. The fix-up added course_keys as a third positional parameter, so get_courses_with_relative_criteria(ids, None, keys) is now a frozen contract. No caller passes positionally past tag_ids. A bare * before course_keys keeps the change additive and makes the description's "keyword argument" true.

The fix-up left one tie-breaking rule less well tested than before.

Details

The second-model adversarial reviewer found this and the review confirmed it by reading both tests. The deleted test_get_blocked_courses_reports_one_row_per_course_naming_the_lowest_id_relative created the higher-id tag's criterion first. The surviving callee test, test_get_courses_with_relative_criteria_ties_break_on_the_lowest_tag_id, creates the lower-id tag's criterion first, so replacing order_by("tag_id") with order_by("id") passes every test. The new docstring promises the lowest-id relative. Swap the two make_leaf_criterion calls in the callee test.

One new test still passes when the normalization it appears to guard is removed.

Details

test_blocked_courses_matches_a_branch_qualified_course_key passes with the normalization reverted, because the database filter strips the key on this endpoint. Delete it, or reword its docstring to say it guards the database behavior, and attribute the branch-key fix to the criteria-tree endpoint in the description.

A code comment explains a typing workaround rather than the behavior, and mentions a case that cannot occur.

Details

The comment in validate_course_keys says why the isinstance check satisfies mypy and nothing about why branch and version are dropped. It also admits LibraryLocator, while CourseRun.save() only ever builds a CourseLocator. The for_branch(None) call is also what turns a version-only key into a 400, which is worth a word. Two short lines do it: "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()." Check CourseLocator only.

The same calculation is written out in two places that are supposed to agree.

Details

get_blocked_courses and _validate_containment each build ancestors plus descendants with the same early return; only the code below is shared. Ticket #666 requires one shared implementation so the read and the write never disagree. Extract a private _get_relative_tag_ids(tag) used by both, and consider one test that creates a criterion through associate_competency_criterion and asserts that both the read and the write see the conflict.

The description speaks of a consumer that does not exist yet, and the agreed follow-up tickets are not linked.

Details

The description says "The authoring UI uses the list"; no frontend code does yet, and ticket #796 says the frontend ticket is not yet numbered. Say "will use", and paste the URLs of the disclosure ticket, the ADR follow-up, and the frontend ticket once it exists.

Some tests still repeat each other across the two layers.

Details

The API direction test, sibling test, and narrowing test fail on the same breaks as their view twins; the narrowing line is covered by six tests. Keep the four directions at the API layer and one view case that asserts the JSON shape; keep the sibling case at one layer; delete the API narrowing test. The three 400 tests on the blocked view each repeat a serializer rule already tested through the sibling; one representative proves the view uses the serializer.

An early return in the code is kept with the ticket as its reason, but the ticket extract does not mention it, and removing the return changes no result.

Details

Removing if not relative_ids: return [] from get_blocked_courses leaves every test green, because the callee returns an empty mapping for an empty set. Your comment says the ticket asked for it; the ticket extract contains nothing of the kind. Keep it as "saves one query" without citing the ticket, or delete the branch and its test.

Small things

• One default setting is not pinned by any test.

  Details

  The default page size of 100 is pinned by no test; 10, 200, or deleting the line all pass. You declined this in round one; it is listed again only because your comment reads as if a test exists.

• One documented error response has no test.

  Details

  "404 if page is out of range" in the view docstring has no test; one ?page=999 request would pin it.

• A test helper is still defined in two files.

  Details

  make_course_run is still defined in both test files after the helper move to utils.py.

• The view's documentation has one overloaded sentence and names an internal constant.

  Details

  The view docstring's status sentence packs four codes and three 400 causes into one sentence; split it. It also names the Python constant MAX_COURSE_KEYS in REST documentation; say "more than 100 keys".

• One docstring uses a looser word than the rule the code applies.

  Details

  The BlockedCourse docstring says "a taxonomy relative", which reads as including siblings; say "a taxonomy ancestor or descendant", as the function docstring does.

• The PR description has three small gaps or inaccuracies.

  Details

  "The edge cases listed below" has no list below it; manual step 2 does not say each criterion needs a gradeable-subsection usage key of the form block-v1:...type@sequential+block@...; "blank means an empty result" is only true for blocked-courses, since the criteria-tree view documents that an empty value returns the groups above any course.

• A class may belong in a different file, though its current home is defensible.

  Details

  BlockedCourse could live in cbe/data.py next to GradedObjectScore, which AGENTS.md names as the home for value objects crossing the API boundary. CompetencyCriteriaTree in api.py is the other precedent, so either is defensible.

• A pagination test helper makes one request more than it needs.

  Details

  The collect_pages helper fetches page 1 a second time in both pagination tests; return the first response from it or take count from it.

• One test builds its expected value with the same expression as the code it checks.

  Details

  The view direction test builds its expected key as str(course_run.course_key), the same expression the serializer uses; the pagination tests spell the literal.

• Two commit messages have small problems.

  Details

  The first commit's message is stale until the squash, and the fix-up commit's "Tests:" paragraph has one overlong line.

[alezconsultant]

@tbain Fixed and rebased, please review.

1. Serializer docstring. Now reads "Query params shared by the criteria-tree and blocked-courses read endpoints."

2. "Purely additive", view docstrings, squash body. The description now lists both criteria-tree corrections (a branch-qualified key scopes to its plain course, a version-only key returns 400), both view docstrings say the same, and the squash body will too.

3. Branch, branch plus version, and version. The endpoint test is parametrized over all three suffixes. The .version_agnostic() call was dead code and is gone: opaque_keys documents for_branch as "also version agnostic", and it sets version_guid=None, which is why swapping it in alone changed nothing.

4. Permission before parsing. The 403 test sends course_keys=not-a-key and asserts no results, so moving check_object_permissions below is_valid fails it.

5. ADR 0002. Amended in a separate commit after the rebase: Decision 4 gains one bullet with the dominance rule, its creation-only enforcement under the Tag and CompetencyCourseRun locks, and get_blocked_courses as the advisory read through the same lookup. Both API functions now drop branch and version up front, so the tree and blocked-courses reads match the same course run, and a version-only key raises InvalidKeyError in both; an API-level test fails when the normalization is removed.

6. Improvements. course_keys is keyword-only, the tie-break test creates the higher-id criterion first so order_by("id") fails it, and _get_relative_tag_ids is shared by the read and the write path with a test that both name the same course. Duplicate view tests are trimmed to one row-shape case and one representative 400; added tests for the default page size of 100 and a 404 on page=999; make_course_run and make_tag moved to utils.py.

Kept as it was: the early return in get_blocked_courses. #796's implementation specifics say "return an empty list immediately if that union is empty, without calling get_courses_with_relative_criteria at all."

The frontend ticket #797 is linked in the description. As for the disclosure ticket: if @mgwozdz-unicon approves, I will create it and link it in the description.

[mgwozdz-unicon]

@alezconsultant @tbain Two things:

ADR 0002 amendment. Amending the ADR in this PR is fine, but the bullet as written is at implementation altitude, and ADRs in this repo record decisions, not mechanics. Function names, which rows are locked, the lowest-id tie-break, and what the endpoint does or doesn't report all belong in docstrings and the ticket, not here. Dave has pushed back on this level of detail in ADR changes more than once, so I want the text at decision altitude before he's asked to review it. Please replace the added Decision 4 bullet with a short paragraph after the "Relationship to other concepts" list, along these lines:

   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.

And the Changelog entry trimmed to match:

2026-11-06:

* Decision 4 states that no ancestor or descendant of a competency tag may hold live criteria
  in the same course as that tag, and why.

Cross-org disclosure follow-up. I don't think a ticket is needed, and I'm fine with the current behavior. #681 and #796 both chose the taxonomy view permission on purpose: a competency taxonomy enabled for all orgs is a shared framework, course keys are already public through the catalog, and the criteria-tree response carries subsection ids, not learner data. I defer to @ormsbee on whether letting any logged-in user see which courses in other orgs use a competency deviates from platform norms in a way that concerns you. If it does, we'll file the ticket. Otherwise please drop the follow-up line from the description.

[alezconsultant]

@mgwozdz-unicon @tbain fixed

[tbain]

No new findings from me, and though the AI review found some nitpicks, neither I nor the AI think they're a reason to hold up this PR.

There is one issue that needs to be addressed, just updating the description, but that's not a blocker to the code, just a one-liner update:

"""
The PR description says every acceptance scenario has an endpoint-level test, which stopped being true when the view-layer tests for three scenarios were removed and only API-layer tests remain.

Details

The Changes list says "one endpoint test per acceptance-criteria scenario". The third-round commit removed the view-layer descendant, grandparent, grandchild and sibling tests, so those scenarios are tested at the API layer only. That layering is reasonable, since the view only parses, calls and serializes, but a reader of the description would look for tests that are not there.

Fix: say the direction and sibling scenarios are tested at the API layer and the view has one row-shape case.

"""

---

AI Review output (T3: Fable/GPT)

Third-round review of PR #915: the blocked-courses endpoint for competency criteria

Triage after discussion with the reviewer (Tyler), 2026-11-06: nothing found in this round affects the feature's correctness, its tests of the acceptance criteria, its security, or its public contract, so the pull request is not being sent back for another round of changes. One description sentence should be fixed before merge. The reachable 500 and the untested normalization line are recorded under "Improvements worth doing" as non-blocking follow-ups, with the reasoning in each. The ADR wording question is left with the maintainer, mgwozdz-unicon. Everything else is optional.

Review Details

Prerequisites, second-round status, new issues, deferred items, what passed, and the action list

Prerequisites (skip if you know the codebase)

The models. A Tag belongs to a Taxonomy; a taxonomy of competencies is a CompetencyTaxonomy. A CompetencyCriteriaGroup is a node in a per-competency tree: a root group, a course-level group with a course foreign key to a CourseRun, and leaf groups. A CompetencyCriterion hangs off a leaf group. A criterion is "live" when it and every group above it have archived=False (source: applets/cbe/models/criteria.py, ADR 0002).

The dominance rule. Ticket #666 forbids a course from holding live criteria for two tags where one is a taxonomy ancestor of the other. The read side (get_blocked_courses) and the write side (_validate_containment) share _get_relative_tag_ids(tag) for the ancestors and descendants and get_courses_with_relative_criteria(tag_ids, course=None, *, course_keys=None) for the lookup.

Course keys, branches and versions. A course key such as course-v1:Org+Course+Run can carry a branch (+branch@draft) or a version (+version@<hex>). The database field for CourseRun.course_key strips both when it prepares a lookup value, by calling for_branch(None) on the key (source: opaque_keys/edx/django/models.py). CourseKey.from_string also accepts library keys (library-v1:Org+Lib), which parse as LibraryLocator, a CourseKey that is not a CourseLocator. A key with only a version and no org, such as course-v1:version@<hex> or library-v1:version@<hex>, makes for_branch raise InvalidKeyError, the exception the opaque-keys library raises for an invalid or malformed key. The serializer and both API functions call for_branch(None) up front on every CourseLocator, so a version-only course key becomes a 400 at the view and an InvalidKeyError from the API functions.

The sibling endpoint. CompetencyCriteriaTreeView is GET competencies/<tag_id>/criteria-tree/. It shares CompetencyCriteriaQueryParamsSerializer with the new view, so the normalization reaches both.

ADR 0002. docs/openedx_learning/decisions/0002-competency-criteria-model.rst. Decision 4 defines CompetencyCriterion. Decision 6, rewritten on main by commit 136bcd0, defines how a learner's status rolls up: a parent competency's status is "the better of two inputs", its own criteria result and its sub-competency result. The maintainer (mgwozdz-unicon) asked for the dominance rule as a short paragraph in Decision 4, supplied the wording, and the author pasted it verbatim.

UNSTABLE. An API function marked UNSTABLE in its docstring may change without the deprecation process that stable public APIs require (source: CLAUDE.md, Architecture section).

Were the second-round findings addressed?

Yes. Every item is fixed, declined with a reason, or decided by the maintainer.

Second-round item	Status	What the head shows
Stale serializer class docstring	Fixed	Names both endpoints.
"Purely additive"; view docstrings silent on normalization	Fixed	The description lists both criteria-tree corrections, and both view docstrings state the normalization. The squash body is still a promise.
.version_agnostic() half untested	Fixed by removal	for_branch(None) already clears the version. The criteria-tree test covers branch, branch plus version, and version.
Permission before validation untested	Fixed	The 403 test sends a malformed key; moving the permission check below validation fails it.
Where the dominance rule is recorded	Addressed, with a new problem	ADR 0002 Decision 4 has the paragraph, where the maintainer asked for it, and it renders correctly. Its rationale contradicts Decision 6 (see "Must fix before merge").
Cross-org disclosure follow-up	Closed by the maintainer	No ticket, and the description's follow-up line is gone; ormsbee may reopen it.
The two API functions disagreed; both leaked InvalidKeyError	Partly	Both now normalize, and the criteria-tree function has a failing test. InvalidKeyError is documented rather than translated (a decision below), and the get_blocked_courses normalization has no test (a must-fix below).
Keyword-only course_keys	Fixed	A bare * precedes it.
Tie-break test weakened	Fixed	The higher-id tag's criterion is created first, so order_by("id") fails the test.
Blocked-view branch test cannot fail	Not addressed	Still passes with the serializer line removed.
Normalization comment; LibraryLocator	Fixed as suggested, which caused the new bug	See "Must fix before merge".
Relative set computed twice	Fixed	_get_relative_tag_ids is shared.
Follow-ups and "UI uses" wording	Fixed	#797 is linked; wording is "will use".
Duplicate tests across layers	Fixed	The view keeps one row-shape case and one malformed-key 400.
Early return with ticket attribution	Declined with reason	The author quotes the ticket's implementation notes, which this review does not read. The code is correct, so the review accepts it.
Default page size, page out of range, duplicated make_course_run, docstring sentence, "taxonomy relative", description items, collect_pages double fetch, recomputed expected value	Fixed	As claimed.
BlockedCourse in data.py	Not taken	Acceptable; CompetencyCriteriaTree in api.py is precedent.
First commit message	Deferred to squash	Unchanged.

Did the changes introduce new issues?

Nothing that affects the feature. Narrowing the serializer's guard from (CourseLocator, LibraryLocator) to CourseLocator, as the second review suggested, lets a version-only library key skip normalization and reach the database layer, which raises an unhandled error; that input is hand-crafted only and the criteria-tree endpoint behaved the same way on main before this pull request. The normalization added to get_blocked_courses has no test, but the only behavior it changes is for an input no caller sends. One description sentence went stale when duplicate view tests were removed. The ADR paragraph the maintainer supplied contradicts Decision 6 of the same ADR, which is the maintainer's to resolve.

Things the review looked at and is leaving to other tickets

• Cross-org disclosure of course keys: the maintainer declined a ticket and deferred to ormsbee.
• The early return in get_blocked_courses: the code is correct.
• Deeper group trees, and Decision 2's archived-course-group block: pre-existing and unchanged.
• BlockedCourse staying in api.py: the neighbor sets the precedent.

What passed

The base is current main, so no merge was needed. makemigrations --check reports no changes, the import linter keeps all three contracts, the full cbe package passes with 612 tests, pylint, pycodestyle, isort, mypy and pydocstyle are clean, the Sphinx build and doc8 report nothing, and the new ADR paragraph renders inside Decision 4 after the relationship list. The base commit's view tests pass unchanged against the new source. For each behavior the reviewer broke the code in a throwaway worktree and ran the tests; all 11 acceptance scenarios have a test that failed when the behavior was broken, and the tie-break and permission-order tests now fail under the breaks the second review described. The default page size and the out-of-range page are tested. A probe file sent every malformed input the review could think of to both endpoints, and each returned 400, 404, or a documented 200, with 401 for anonymous callers and 403 for callers without permission, except the version-only library key below. A second AI model (GPT-6.1-Sol) attacked the change independently, and the reviewer reproduced its findings before keeping them. It found the branch and version handling, the custom-course (CCX) key path, the reordered tie-break test, the remaining coverage of the deleted view cases, and the pagination envelope solid.

Action list

1. Before merge: reword the description's test-coverage sentence to say the direction and sibling scenarios are tested at the API layer.
2. Maintainer (mgwozdz-unicon): decide whether to reconcile the new Decision 4 paragraph with Decision 6, and whether the Changelog entry names its issues.
3. Optional, in the squash or a follow-up: reject non-course locators in validate_course_keys with a test per endpoint; add the version-only-key test for both API functions or drop the untested line and its docstring sentence.
4. Optional: decide whether to translate InvalidKeyError to ValidationError in the API functions.
5. Optional: the remaining improvements and small things, of which the + percent-encoding note in the manual steps is the one most likely to save a tester time.
6. Confirm the squash body at merge.

Must fix before merge

The PR description says every acceptance scenario has an endpoint-level test, which stopped being true when the view-layer tests for three scenarios were removed and only API-layer tests remain.

Details

The Changes list says "one endpoint test per acceptance-criteria scenario". The third-round commit removed the view-layer descendant, grandparent, grandchild and sibling tests, so those scenarios are tested at the API layer only. That layering is reasonable, since the view only parses, calls and serializes, but a reader of the description would look for tests that are not there.

Fix: say the direction and sibling scenarios are tested at the API layer and the view has one row-shape case.

Left with the maintainer

The new ADR paragraph says the order of precedence between two inputs is undefined, while another decision in the same ADR already defines it.

Details

Left with the maintainer. The author pasted the wording mgwozdz-unicon supplied, so the author has nothing to change here. The reviewer defers to mgwozdz-unicon and ormsbee on whether and how to reconcile the two decisions.

The paragraph in Decision 4 says that with criteria at two levels of one hierarchy in a course, "it would be undefined whether that given or the authored criteria decide the parent's status, and which takes precedence." Decision 6 says "A parent's status is the better of two inputs: its own criteria result ... and its sub-competency result." The ADR would assert both. You pasted the maintainer's wording verbatim, so the maintainer resolves this. Either keep Decision 6 and replace the "undefined" sentence with the consequence under it (if one course held authored criteria at two levels, the more lenient input would always decide the parent's status and the other would have no effect), or reopen Decision 6. The phrase "that given" also reads as a leftover; "that assumption" is probably meant.

Decisions to make or write down

It is undecided whether the two API functions should convert a third-party key error into a Django validation error or keep documenting the third-party error.

Details

Both get_blocked_courses and get_competency_criteria_tree document that a version-only key raises InvalidKeyError. The repository's AGENTS.md says API code reports failures with Model.DoesNotExist, Django's ValidationError, or an app exception, and create_competency_criterion in the same module already translates InvalidKeyError to ValidationError for a bad object_id. The stable get_courses_with_relative_criteria gets the same exception from the database layer for a version-only key in its new course_keys argument, and its docstring says nothing about branch or version.

• Option 1: a private helper that normalizes and raises ValidationError({"course_keys": ...}), used by both functions. It also collapses the three copies of the normalization line into one.
• Option 2: keep the documented exception.

Both functions are marked UNSTABLE, so this does not block the merge.

Improvements worth doing

A library key that carries only a version and no org, sent in the course filter, returns a 500 on both endpoints instead of a 400, because the key guard no longer covers library keys.

Details

Why this is not blocking. The only input that triggers it is a legacy library key carrying a version and no org, such as library-v1:version@<24 hex characters>. Normal library keys, version-only course keys, and every other malformed input in the probe return 400 or a documented 200. The authoring frontend (#797) builds course_keys from Studio course ids and never handles library keys, so only a hand-written request reaches this path, and the effect is one logged 500 with nothing exposed or written. Anonymous callers and callers without taxonomy permission are refused before the key is parsed. The criteria-tree endpoint behaved the same way on main before this pull request. The narrowed guard was the second review's own suggestion. The fix is one line plus a test, so it can ride along with any later push or follow-up.

Sending ?course_keys=library-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa to the blocked-courses endpoint for a competency with a blocked course, or to the criteria-tree endpoint for a tag with groups, returns 500. The review reproduced both as a staff user. CourseKey.from_string parses the value as a LibraryLocator, which is not a CourseLocator, so the isinstance guard in validate_course_keys skips it. The database field then calls for_branch(None) on it and raises InvalidKeyError, which nothing catches. The second-round head returned 400 because its guard included LibraryLocator. The serializer comment, "this also rejects a version-only key", is therefore untrue for library keys. The second AI model found this.

Fix: treat any parsed key that is not a CourseLocator as invalid, with the existing "is not a valid course key" message, since a library key can never match a course run. Restoring the two-class guard also works. Add one test per endpoint with that key.

The course-key normalization in the blocked-courses function, and the error both API functions document for a version-only key, have no test, so the normalization could be deleted and every test would still pass.

Details

Why this is not blocking. No feature behavior is untested. The view normalizes keys in the serializer before calling the function, so no endpoint test can reach this line, and a branch-qualified key cannot tell the two states apart because the database strips the branch anyway. The only input that observes the line is a version-only key passed in-process, and the only difference it makes is for a tag with no relatives, where the early return yields an empty list instead of the documented InvalidKeyError. What is untested is the docstring sentence, not the lookup. Either add the one small test or delete the line and its docstring sentence; neither needs a review round.

Removing the normalization line from get_blocked_courses leaves all 612 tests green, and test_api.py has no test that passes a version-only key to either API function, though both now document the InvalidKeyError it raises. A branch-qualified key cannot guard the line, because the database strips the branch anyway; only the version-only case observes it. Your comment says an API-level test fails when the normalization is removed; that is true for get_competency_criteria_tree only.

Fix: one API test parametrized over both functions that passes course-v1:version@<hex> and asserts pytest.raises(InvalidKeyError).

The test that checks reading and writing agree on a conflict covers only one of four directions and does not check that the right tag is named.

Details

test_get_blocked_courses_and_create_competency_criterion_agree_on_a_conflict uses an ancestor claim only. When _validate_containment was made to compute its own relative set without descendants, the test still passed, and only pre-existing containment tests caught it. When the write's error message was changed to name a wrong tag, the test still passed, because it checks only that the tag_id key is present.

Fix: parametrize it over the four directions the API direction test already uses, and assert the conflicting tag's value appears in the error.

One endpoint test looks like it protects the course-identifier cleanup but passes even when the cleanup is removed.

Details

test_blocked_courses_matches_a_branch_qualified_course_key passes with the serializer normalization removed, because the database strips the branch on this endpoint. Delete it, or reword its docstring to say it guards the database behavior.

No test fails if the permission check runs after the repeated-parameter check instead of before it.

Details

Moving check_object_permissions below the getlist check fails nothing. Add a second case to the 403 test that sends ?course_keys=a&course_keys=b as a user without permission.

The manual testing steps give course keys that fail when pasted into a URL as written.

Details

Manual steps 5 and 7 paste a raw + into a query string. An unencoded + decodes as a space, so a tester using curl or a browser gets 400 on every course-key step. Say to percent-encode + as %2B or use curl -G --data-urlencode.

Small things

• The get_blocked_courses docstring has a sentence fragment.

  Details

  "Sorted by course key." sits between two unrelated rules; write "Results are sorted by course key."

• The ADR Changelog entry omits a prefix that most earlier entries carry.

  Details

  It lacks the "For issue #N" prefix that four of the six earlier entries carry. The maintainer wrote it; ask whether to add #666 and #796.

• Three test inputs hard-code a course key that the fixture already guarantees.

  Details

  They use course-v1:Org1+Python100+Fall2026. Inputs read better built from course_key(course_run), with literals kept for expected values.

• The row-shape test may pin more of the pagination envelope than anyone promised.

  Details

  It asserts all seven envelope keys of DefaultPagination. If that envelope is a contract, cite the library's ADR; otherwise assert only next, previous, count, results.

• collect_pages uses a placeholder value and an assert where it could read the first response.

  Details

  It uses a count = None sentinel and an assert; initialize it from the first response.

• No test pins the maximum page size.

  Details

  max_page_size of 500 is untested here, and the rule-profile paginator has the same gap.

• The sort order is asserted three times.

  Details

  The assertions are in the API and view tests. Cheap and optional.

• Manual step 1 does not say how to create the child tags with parents.

  Details

  The step names the taxonomy endpoint and the tag names, but not how the child tags are created under their parents.

• Several commit messages are stale, pending the squash.

  Details

  The first commit is stale. The first docs commit describes ADR text the second removed. The fix commit's "all three qualified key forms" is true for the criteria-tree only.

---

The accuracy gate returned PASS WITH FIXES; the fixes are applied above, and the cleanup added three definitions that nobody has confirmed.

Definitions added by the cleanup, unconfirmed

• InvalidKeyError is the exception the opaque-keys library raises for an invalid or malformed key (general knowledge; in this review it is raised by for_branch on a key that already parsed).
• An API marked UNSTABLE in its docstring may change without the deprecation process that stable public APIs require (inferred from the Architecture section of the workspace CLAUDE.md, which states the converse).
• CCX stands for custom course, a course variant keyed by a CCXLocator (general knowledge).
• The issue URLs for #666, #796 and #797 assume all three live in openedx/openedx-core.

For the reviewer who ran this review: the structured output

The structured output below is the review's phase C record as produced before the triage above; its verdict and severities follow the review rubric, under which any unhandled error reachable from client input is a Bug and any untested new line is a Gap. The triage at the top of this file is the operative assessment.

Phase C structured output (C6) for round 3

PR #915 round 3: structured review output (C6)

1. Verdict

Ready to merge: with fixes. Every round-2 Gap is closed and the two round-2 Questions were decided by the maintainer, but the round-3 commit introduced one reachable 500 (a version-only library key in course_keys, on both endpoints), the newly documented InvalidKeyError contract of get_blocked_courses has no test, the PR body's "one endpoint test per acceptance-criteria scenario" is no longer true, and the ADR paragraph the maintainer supplied says precedence is undefined where Decision 6 of the same ADR defines it.

2. Inputs

• Repository openedx-core; PR #915 "feat: add blocked-courses endpoint for a competency".
• Head f09283bd4fb15615099ff04174ff3f20bd826353 (commits 2509afb, d0b7469 rebased from rounds 1 and 2; new 44f318d fix: address second-round review, 0f763e1 docs: record the competency dominance rule in ADR 0002, f09283b docs: state the competency dominance rule at decision altitude). Base and merge-base 7f52bb6330213d620fb4c7cb5b61eec789fe2669, which is current openedx/main.
• Ticket #796 updated_at 2026-10-06T04:14:33Z, unchanged; round-1 extracts reused. Frontend consumer ticket #797 now linked in the body.
• Author's round-3 comment (2026-11-06 11:12), maintainer mgwozdz-unicon's reply (14:43: ADR wording at decision altitude; no cross-org ticket, deferring to ormsbee), author's "fixed" (16:12). CI green on the head.
• Companion repositories: unchanged from round 2 (platform route already mounted; no in-process callers; no frontend consumer yet).

3. Round-2 findings: resolution

Round-2 finding	Status at f09283b	Evidence
G1 stale serializer docstring	Fixed	serializers.py:181 names both endpoints
G2 "purely additive"; view docstrings silent on normalization	Fixed	body lists both criteria-tree corrections; views.py:196-199, :292-295; squash body pending
G3 .version_agnostic() untested	Fixed by removal	for_branch is replace(branch=branch, version_guid=None), docstring "also version agnostic" (opaque_keys/edx/locator.py:353-359); tree test parametrized over three suffixes
G4 permission before validation untested	Fixed	test_blocked_courses_without_view_access_403s sends not-a-key; moving the check below is_valid fails it (400 == 403)
Q1 ADR record	Addressed, with a new conflict	ADR 0002 :158-164 paragraph and :502-505 Changelog, verbatim the maintainer's wording, in the requested place, renders inside Decision 4; see G3 below
Q2 cross-org follow-up	Closed by maintainer	no ticket; body line removed; ormsbee may reopen
Q3 in-process disagreement; raw InvalidKeyError	Partly	both functions normalize (api.py:217-219, :415-417) and the tree has a failing API test; InvalidKeyError is documented rather than translated (Question 1 below); get_blocked_courses normalization has no test (G1 below)
S-a keyword-only	Fixed	api.py:347 *, course_keys
S-b tie-break test	Fixed	test_api.py:1762-1777 creates the higher-id tag's criterion first; order_by("id") fails it and only it
S-c blocked-view branch test cannot fail	Not addressed	test_views.py:2303 unchanged but for its docstring; still passes with the serializer line removed
S-d comment and LibraryLocator	Fixed as suggested, and the LibraryLocator removal caused Bug 1	serializers.py:203-206
S-e relative set twice	Fixed	_get_relative_tag_ids at api.py:391, called at :418 and :442
S-f follow-ups and "UI uses"	Fixed	#797 linked; "will use"
S-g duplicate tests	Fixed	view direction, sibling, version-only, cap tests removed; one row-shape and one malformed 400 remain
S-h early return	Declined with reason	author quotes #796's implementation specifics; UNVERIFIABLE from the extract, which drops Technical Details; the code is correct, so accepted
N-a default page size	Fixed	test_blocked_courses_default_page_size_is_100; 10, 99, 200 each fail it
N-b page out of range	Fixed	test_blocked_courses_page_out_of_range_404s
N-c make_course_run duplicated	Fixed	moved to utils.py with make_tag
N-d docstring sentence; MAX_COURSE_KEYS	Fixed	split into three sentences; "more than 100 keys"
N-e "a taxonomy relative"	Fixed	api.py:380
N-f PR body items	Fixed	no dangling "listed below"; step 2 names the usage-key form; "blank means empty" claim for the tree removed
N-g BlockedCourse in data.py	Not taken; acceptable	CompetencyCriteriaTree in api.py is precedent
N-h collect_pages double fetch	Fixed	returns (count, pages) from one walk
N-i expected value recomputed	Fixed	literal key in the row-shape test
N-j first commit message	Deferred to squash	2509afb unchanged

4. Acceptance criteria

Criterion	Implemented at	Tested by (fails under break)	Verdict
1 ancestor	api.py:391-400,403-430	test_api.py::test_get_blocked_courses_reports_a_relative_claim[ancestor claim] and test_views.py::test_blocked_courses_reports_a_relative_claim_with_the_row_shape (12 fail when ancestors are dropped)	Covered
2 descendant	same	...[descendant claim] (API layer only; the view test was removed in round 3)	Covered
3 grandparent, grandchild	same	...[grandparent claim], ...[grandchild claim]	Covered
4 sibling	same	test_get_blocked_courses_ignores_a_sibling_claim (API layer only)	Covered
5 unaffected	api.py:356-368	test_blocked_courses_ignores_an_unaffected_course	Covered
6 narrowing	api.py:366-367	test_blocked_courses_narrows_to_requested_course_keys, callee test	Covered
7 empty 200	view over []	test_blocked_courses_is_an_empty_200_when_nothing_is_claimed	Covered
8 pagination	views.py:321-322, paginators.py:23-33	test_blocked_courses_paginates_without_repeats_or_gaps (order and page lengths literal)	Covered
9 narrowed across pages	same	test_blocked_courses_filters_before_paginating	Covered
10 unknown tag	views.py:311-314	two 404 tests	Covered
11 permission	views.py:305,315	403 test (400 == 403 when the check is removed or moved), 401 test	Covered

Behavior no criterion asked for, carried from round 2 and now documented: the shared serializer and both API functions normalize branch and version; get_competency_criteria_tree changed accordingly. New in round 3: a version-only library key reaches the ORM unnormalized (Bug 1).

5. Premise audit

Ten numbered claims plus the author's nine comment items checked. FALSE or partly false:

• Author: "an API-level test fails when the normalization is removed" (item 5): TRUE for get_competency_criteria_tree (test_get_competency_criteria_tree_course_keys_branch_qualified_key_matches_the_plain_course fails {1} == {1, 2, 3}), FALSE for get_blocked_courses (removing api.py:415-417 leaves 612 tests green; a direct call with a version-only key then returns [] instead of raising).
• PR body: "one endpoint test per acceptance-criteria scenario": FALSE since round 3 removed the view-layer direction and sibling tests; criteria 2, 3, 4 are tested at the API layer only.
• Serializer comment serializers.py:203: "Match the stored plain course key: drop branch and version; this also rejects a version-only key": TRUE for CourseLocator, FALSE for a version-only LibraryLocator, which the guard skips (Bug 1).
• Commit 44f318d: "Tests cover all three qualified key forms": TRUE for the tree endpoint only.
• Commit 0f763e1 describes ADR text (locking, tie-break, advisory read) that f09283b removed; 2509afb still carries round-1 text. Both go away on squash.
• ADR 0002 :161-163: "it would be undefined whether that given or the authored criteria decide the parent's status, and which takes precedence": contradicted by Decision 6 at :279, "A parent's status is the better of two inputs" (G3 below). Maintainer's wording.

TRUE: for_branch sets version_guid=None and is documented "also version agnostic"; the tie-break test order; the 403 test mechanism; the ADR placement and docutils rendering (no block quote, inside Decision 4's list item; Sphinx build clean; doc8 clean); "a version-only key now returns 400 where it previously raised from the database layer" (at base, get_course_run_ids raised InvalidKeyError); "both UNSTABLE"; "keyword-only"; the seven envelope keys match DefaultPagination.get_paginated_response (edx_rest_framework_extensions/paginators.py:21-33); the early-return quote is UNVERIFIABLE by design and accepted.

6. Invariants

No model, constraint, index, migration, or lock changed. Read and write now share both the relative set and the conflict query, so they agree by construction. The ADR states the rule as absolute while the code enforces it at creation only; pre-existing and the maintainer's choice.

7. Findings

Bug

• B1. A version-only library key in course_keys returns 500 on both endpoints. serializers.py:205. Reproduced at the head with ?course_keys=library-v1:version@aaaaaaaaaaaaaaaaaaaaaaaa as staff: blocked-courses 500 (competency with a relative's criterion), criteria-tree 500 (tag with groups). CourseKey.from_string returns a LibraryLocator, which is not a CourseLocator, so the guard skips normalization; the ORM field's get_prep_value then calls for_branch(None) and raises InvalidKeyError outside any handler. Round 2's head caught this as a 400 through the (CourseLocator, LibraryLocator) guard; round 3 removed LibraryLocator on round 2's suggestion S-d, which was wrong to omit the version-only case. Raised by the B6 agent. Fix: in validate_course_keys, treat anything that is not a CourseLocator as invalid ("is not a valid course key"), since a library key can never match a CourseRun; or restore the two-class guard. Add one test per endpoint with that key.

Gap

• G1. The get_blocked_courses normalization and the InvalidKeyError contract both public functions now document have no test. api.py:415-417, :219. Removing the get_blocked_courses line leaves all 612 tests green; test_api.py contains no InvalidKeyError or version@ for either function. Fix: one API test parametrized over both functions, pytest.raises(InvalidKeyError) on course-v1:version@<hex>; it fails when either line is removed.
• G2. The PR body's test-coverage claim is false. "Tests: one endpoint test per acceptance-criteria scenario" (body, Changes). Criteria 2, 3 and 4 have API tests only since round 3. Fix: say the direction and sibling scenarios are tested at the API layer and the view has one row-shape case.
• G3. ADR 0002's new Decision 4 rationale contradicts Decision 6. 0002-competency-criteria-model.rst:161-163 versus :279. The paragraph says precedence between the sub-competency given and authored parent criteria "would be undefined"; Decision 6 (from 136bcd0, in the base) defines a parent's status as "the better of two inputs". The author pasted the maintainer's wording verbatim, so the fix is the maintainer's: either (a) keep Decision 6 and replace the "undefined" sentence with the consequence under it (with both levels authored in one course, the more lenient input always decides the parent's status and the other has no effect), or (b) reopen Decision 6. Raised by the B5 agent. Also "that given" reads as a leftover; "that assumption" is probably meant.

Question

• Q1. Should the API functions translate InvalidKeyError instead of documenting it? api.py:207,217-219,410-411,415-417; AGENTS.md:145-148 says API failures are Model.DoesNotExist, Django ValidationError, or an app exception, and the same module translates InvalidKeyError to ValidationError({"object_id": ...}) at api.py:621-622. The stable get_courses_with_relative_criteria gets the same exception from the ORM for a version-only key and its docstring says nothing about branch or version. Options: a private _plain_course_keys() used by both functions that raises ValidationError({"course_keys": ...}) (three copies of the normalization line become one), or keep the documented library exception. Both functions are UNSTABLE, so not blocking.

Suggestion

• S1. The agreement test guards one direction only and does not check the named tag. test_api.py:2387-2404. With _validate_containment computing its own set without descendants, the test still passes (only pre-existing containment tests catch it); with the write's error renamed to "WRONG TAG" it still passes (B6). Fix: parametrize over the four claimant_and_queried directions and assert critical_thinking.value in str(exc_info.value.message_dict["tag_id"]).
• S2. test_blocked_courses_matches_a_branch_qualified_course_key still cannot fail on the serializer line (round-2 S-c). test_views.py:2303. Delete it or reword its docstring to say it guards the database behavior.
• S3. Permission before the repeated-parameter check is unguarded. Moving check_object_permissions below the getlist check fails nothing. Fix: a second case in the 403 test sending ?course_keys=a&course_keys=b.
• S4. Manual steps 5 and 7 paste raw + into a query string. An unencoded + decodes as a space, so a tester using curl or a browser gets 400 on every course-key step. Fix: say to percent-encode + as %2B or use curl -G --data-urlencode.

Nit

• N1. get_blocked_courses docstring (api.py:410): "Sorted by course key." is a fragment wedged between two unrelated rules; write "Results are sorted by course key."
• N2. ADR Changelog entry (:502-505) lacks the "For issue #N" prefix four of the six earlier entries carry; maintainer's wording, ask.
• N3. Three test inputs hard-code course-v1:Org1+Python100+Fall2026 (test_api.py:1535, test_views.py:2058, :2316); the fixture guarantees it, but build inputs from course_key(course_run) and keep literals for expected values only.
• N4. The row-shape test's envelope assertion (test_views.py:2056) pins the seven DefaultPagination keys; if the envelope is a contract (ADR 0032 in the library), cite it, else assert only next, previous, count, results.
• N5. collect_pages uses a count = None sentinel plus assert count is not None; initialize from the first response.
• N6. max_page_size = 500 is pinned by no test (precedent: the rule-profile paginator is not either).
• N7. Sort order is asserted three times (test_api.py:2407, test_views.py:2121, :2141); cheap, optional.
• N8. Manual step 1 does not say how to create child tags with parents.
• N9. Commit messages pending squash: 2509afb stale; 0f763e1 describes ADR text f09283b removed; 44f318d "all three qualified key forms" is tree-only.

8. Tests checked

B4 ran every criterion and every round-3 fix against a break in a detached worktree; all 11 criteria fail on the asserted behavior. Breaks that stayed green: get_blocked_courses normalization removed (G1); _validate_containment own set without descendants (S1); permission moved below the getlist check (S3); * removed from the signature (nothing observes it; accepted); serializer normalization removed does not fail the blocked-view branch test (S2); max_page_size 50 (N6); order_by removed entirely (SQLite returns insertion order; not a test defect). Error-path probe: every malformed input returned 400, 404, or 200 with the documented shape, 401 anonymous, 403 without permission, except the version-only library key (B1, found by B6 and reproduced by the orchestrator).

9. Checks run and skipped

• Base is current main (merge-base --is-ancestor true); no merge needed. makemigrations --check --dry-run: no changes. lint-imports: 3 kept. Full cbe package: 612 passed. pylint, pycodestyle, isort, mypy (347 files), pydocstyle (tox form): clean. Sphinx make -C docs html: no warnings; doc8 clean; the ADR paragraph renders inside Decision 4.
• Base test_views.py run against head source: 144 passed (12 for -k course_key).
• Skipped: make selfcheck; manual curl steps; migrate --check exits 1 on the unmigrated in-memory test database, not a model problem.
• B6 (GPT-6.1-Sol) ran.

10. Declined to judge

• Cross-org disclosure: maintainer declined a ticket and deferred to ormsbee; body line removed.
• Early return in get_blocked_courses: author cites the ticket's Technical Details, which the review does not read; code is correct.
• Deeper group trees; Decision 2's archived-course-group block: pre-existing, unchanged.
• BlockedCourse location: api.py precedent accepted.

11. Dropped

• Keyword-only course_keys untested: proposes no change.
• "100 keys" literal duplicates the constant: round 2 asked for it; no change.
• Three copies of the normalization line: folded into Q1.
• test_blocked_courses_version_only_course_key_400s deleted: the shared serializer's tree test guards the line, and swapping the serializer fails the malformed test; acceptable.

12. Open questions

1. For mgwozdz-unicon: which resolution of G3 (reword the "undefined" sentence against Decision 6, or reopen Decision 6), and whether the Changelog entry should name #666 and #796.
2. For the author: translate InvalidKeyError to ValidationError in the API functions (Q1), or keep it documented?
3. Confirm at merge that the squash body describes the final ADR paragraph and both criteria-tree corrections.

[alezconsultant]

@tbain Description updated as asked. Also I fixed the rest of the AI review's items in 4de62a2 and 1695998 ( need to be squashed):

A library key in course_keys now returns 400 on both endpoints. A version-only library key used to bypass the normalization and raise from the database layer, which surfaced as a 500. One test per endpoint.
The read-write agreement test runs over all four directions and asserts the write names the same relative the read does.
New tests pin the version-only key rejection in both API functions, the 500 maximum page size, and that the view checks permission before counting the course_keys parameter.
The manual steps say to percent-encode + and how to create the child tags.
Smaller tidy-ups in the tests: the page collection helper, the branch-qualified input, and a comment naming ADR 0032 as the source of the pagination envelope.

Left alone: translating InvalidKeyError to ValidationError in the API functions, since that is a contract choice on UNSTABLE functions.

[tbain]

Thanks for making those changes, those alleviate any lingering wants from the AI code review. :)

[mgwozdz-unicon]

@alezconsultant Claude and I reviewed and these are our findings:

Found by me

1. api.py: "relative" is an unclear name. Please rename _get_relative_tag_ids to _get_ancestor_and_descendant_tag_ids, and the relative_ids locals to ancestor_and_descendant_ids. Please also rename the public get_courses_with_relative_criteria to get_courses_with_live_criteria: it computes no ancestors or descendants, it returns the courses where the tag ids it is given have live criteria.

2. api.py: remove the branch and version stripping from get_competency_criteria_tree and get_blocked_courses. The serializer already strips every key that arrives over REST, so these two lines only affect direct Python callers, and the one in get_competency_criteria_tree changes an existing function that #796 does not cover. With the lines removed, all REST tests still pass. Please also remove the docstring sentences about a version-only key in both functions, the CourseLocator import, test_get_competency_criteria_tree_course_keys_branch_qualified_key_matches_the_plain_course, test_a_version_only_course_key_raises_before_the_database_is_asked, and the matching line in the PR description.

3. serializers.py and views.py: shorten the docstrings and explain branch and version once.

   • validate_course_keys docstring: "Parse a comma-separated course_keys value into a deduplicated list of plain course keys. 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."
   • The inline comment above key.for_branch(None) in the same method is the one place to say what a branch and a version are: "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."
   • CompetencyCriteriaTreeView docstring, the three added lines: "The parameter may be supplied once. A key's branch and version are ignored, and a version-only or library key is a 400."
   • CompetencyBlockedCoursesView docstring: remove the status-code paragraph, which restates the code directly below it, and replace the paragraph before it with "GET api/cbe/v1/competencies/<tag_id>/blocked-courses/ returns those courses sorted by 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."

4. Tests: four tests are hard to read.

   • test_blocked_courses_paginates_without_repeats_or_gaps and test_blocked_courses_filters_before_paginating share one body. Please merge them into one parameterized test with the ids unfiltered and narrowed.
   • test_blocked_courses_without_view_access_403s: replace the for loop with pytest.mark.parametrize, so a failure names the input.
   • test_blocked_courses_ignores_an_unaffected_course: name the two courses blocked and unaffected, and add assert str(unaffected.course_key) not in keys, so the absent course is visible.
   • test_blocked_courses_repeated_course_keys_parameter_400s: build the query with urlencode([("course_keys", "course-v1:Org+A+1"), ("course_keys", "course-v1:Org+B+1")]) so the repeated parameter is visible, and drop the courses and criteria, since the 400 is returned before any lookup.

Found by Claude

5. api.py: the renamed get_courses_with_live_criteria needs an UNSTABLE paragraph and a corrected first docstring line. It is the only function in the criteria-authoring family without the UNSTABLE paragraph. It was added two weeks ago and nothing in openedx-platform calls it, so the missing marker looks like an omission. Please add the paragraph, and note the rename and the marker under "Other information" in the description so the maintainers see them. Its first docstring line says the courses are blocked "for" the given tags, when they are the tags doing the blocking. Please change it to "Return every course where one of tag_ids has a live CompetencyCriterion."

6. api.py and views.py: two small wording fixes. In get_blocked_courses, save() derives course_key only when it is missing, so the comment above the assert should read "Typed optional, but save() always sets it." In the CompetencyBlockedCoursesView docstring, make the first sentence fit on one line, like the other views in the file.

7. Tests: one test can't fail, three repeat other tests, and one case is missing.

   • test_blocked_courses_matches_a_branch_qualified_course_key still passes with all of this PR's branch handling removed, because the database field strips the branch by itself. Please delete it. test_a_branch_qualified_course_key_matches_the_plain_course is the test that guards the behavior.
   • test_get_blocked_courses_reports_a_relative_claim makes the same assertion on the same setup as the first half of test_get_blocked_courses_and_create_competency_criterion_agree_on_a_conflict. Keep one.
   • test_blocked_courses_library_key_400s and test_blocked_courses_malformed_course_key_400s repeat the criteria-tree tests of the one shared serializer. One 400 test on the new view is enough.
   • No get_blocked_courses test has two ancestors or descendants with a criterion in the same course. Please add one such case to the agreement test, asserting that the read and the create error both name the tag with the lowest id.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The CBE API now exposes live-criteria lookup and blocked-course results for taxonomy ancestors or descendants. An authenticated, paginated endpoint supports normalized course-key filters. Tests and the decision record cover the conflict rule, filtering, ordering, and pagination.

Changes: Blocked-course discovery

Layer / File(s)	Summary
Conflict rule and API query docs/openedx_learning/decisions/0002-competency-criteria-model.rst, src/openedx_learning/applets/cbe/api.py, tests/openedx_learning/applets/cbe/test_api.py, tests/openedx_learning/applets/cbe/utils.py	The API renames the live-criteria lookup, adds course-key filtering, and returns sorted blocked-course results with the lowest-ID conflicting tag per course. The decision record documents the ancestor/descendant restriction. Tests cover lookup and conflict behavior.
Filter validation and paginated endpoint src/openedx_learning/applets/cbe/rest_api/..., tests/openedx_learning/applets/cbe/test_views.py	The endpoint validates course-key filters, checks permissions, and returns serialized, paginated blocked-course results. Tests cover filtering, pagination, invalid requests, and access failures.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant CompetencyBlockedCoursesView
  participant CompetencyCriteriaQueryParamsSerializer
  participant get_blocked_courses
  participant CompetencyBlockedCoursesPagination
  Client->>CompetencyBlockedCoursesView: GET blocked-courses request
  CompetencyBlockedCoursesView->>CompetencyCriteriaQueryParamsSerializer: validate course_keys
  CompetencyCriteriaQueryParamsSerializer-->>CompetencyBlockedCoursesView: normalized course keys
  CompetencyBlockedCoursesView->>get_blocked_courses: query tag and course keys
  get_blocked_courses-->>CompetencyBlockedCoursesView: blocked-course rows
  CompetencyBlockedCoursesView->>CompetencyBlockedCoursesPagination: paginate rows
  CompetencyBlockedCoursesPagination-->>CompetencyBlockedCoursesView: page of results
  CompetencyBlockedCoursesView-->>Client: serialized paginated response

Loading

Suggested reviewers: javoconsultant, ufedaseyeuconsultant

Merge Risk: Minimal · up to 25d59

This adds a read-only blocked-courses endpoint and tightens course-key validation on the criteria-tree endpoint. No concrete merge-blocking risk was identified in the supplied evidence. Normal CI and the test commands in the PR description are still the checks to run before merging.

Security architecture risk: Low · up to 25d59

The endpoint preserves existing sign-in and taxonomy permissions, and its advisory results cannot bypass creation checks. The main residual risk is resource consumption: a small response page can still require computing conflicts across a large competency hierarchy and many courses.

Retained concerns

• Low · security · inferred: The new endpoint computes all matching ancestor/descendant conflicts across course runs before applying response pagination. An authenticated taxonomy reader can therefore trigger hierarchy-wide database and worker work even when requesting a small page. Compared with the existing single-tag tree read and course-scoped write check, this adds a broader per-request aggregation path. Whether it can exhaust shared resources depends on deployment scale and request controls, which were not supplied.

Security review details

Security Blast Radius

• observed — An unfiltered authorized request can return blocked-course identities and conflicting-tag metadata across all course runs containing live criteria on the selected tag's ancestors or descendants. The tag set is taxonomy-contained, but neither response visibility nor lookup work is restricted to a course the caller edits. The shared database and request worker perform the aggregate lookup.

Security Findings and Attack Paths

• inferred — The availability concern is an authenticated taxonomy reader repeatedly requesting a broad ancestor's blocked-course collection without course_keys. Every request computes the full matching collection before pagination. The existing tree endpoint also materializes data, but only for one requested tag; the new path adds hierarchy-wide aggregation. Harmful resource exhaustion has not been demonstrated, and effective deployment throttling and dataset size remain unknown.

Trust Boundaries and Controls

• observed — The new view uses the same JWT/session authentication and authenticated-user plus taxonomy-object permission checks as the existing tree view. Permission is checked before lookup, and both ancestry queries constrain conflicting tags to the authorized taxonomy. These controls counter a cross-taxonomy disclosure claim; no weaker read gate was identified in the inspected change.

Resilience and Maintainability Implications

• inferred — A blocked-course read may become stale during concurrent authoring, but it does not grant write authority. Creation reruns containment under the existing per-course lock, repeated associations are checked for duplicates, and downstream failure rolls back transaction-local changes. The helper extraction therefore does not introduce a new read-preflight bypass or cleanup obligation.

Hardening Proposals

• proposed — Consider database-side reduction and pagination, or another explicit work budget for unfiltered hierarchy queries, so response-page limits also provide meaningful resource containment. Validate the chosen budget against large hierarchies and course populations together with the consuming application's request limits.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title "feat: add blocked-courses endpoint for a competency" matches the main change. The changeset adds the GET blocked-courses REST view, route, serializer, paginator, and the exported get_blocke…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "feat: add blocked-courses endpoint for a competency" matches the main change. The changeset adds the GET blocked-courses REST view, route, serializer, paginator, and the exported get_blocked_courses function. The title is short, specific, and names the primary feature. It does not cover the rename to get_courses_with_live_criteria or the ADR update, which is acceptable.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 73930a79-dced-4fc6-b90f-b319384e9279

Commits

Reviewing files that changed from the base of the PR and between 7f52bb6 and 25d598c.

Files selected for processing: 9

• docs/openedx_learning/decisions/0002-competency-criteria-model.rst
• src/openedx_learning/applets/cbe/api.py
• src/openedx_learning/applets/cbe/rest_api/paginators.py
• src/openedx_learning/applets/cbe/rest_api/v1/serializers.py
• src/openedx_learning/applets/cbe/rest_api/v1/urls.py
• src/openedx_learning/applets/cbe/rest_api/v1/views.py
• tests/openedx_learning/applets/cbe/test_api.py
• tests/openedx_learning/applets/cbe/test_views.py
• tests/openedx_learning/applets/cbe/utils.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[mgwozdz-unicon]

Thank you!