Repository navigation
Alezconsultant/796 blocked courses - #915
alezconsultant wants to merge 1 commit into
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @alezconsultant! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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: 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
2e373c5 to
f36c0a2
Compare
Adds GET api/cbe/v1/competencies/<tag_id>/blocked-courses/, which lists the courses where a taxonomy ancestor or descendant of the competency already has a criterion, so authoring can gray them out before the create-criterion endpoint rejects them. An optional comma-separated course_keys parameter narrows the list, and the result is paginated. get_blocked_courses() reuses get_courses_with_relative_criteria(), so this read and the write-side check always agree. CompetencyReadPermission is copied verbatim from the criteria-groups endpoint's PR. Related to openedx#796 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f36c0a2 to
6f88a96
Compare
tbain
left a comment
tbain
left a comment
There was a problem hiding this comment.
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
mainand 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
- Add the repeated-parameter guard to
CompetencyBlockedCoursesView.getand a test for it. - Reuse
CompetencyCriteriaQueryParamsSerializer, or add the 100-key cap and decide the blank-value behavior deliberately. - Export
BlockedCoursein__all__, make itfrozen=True, kw_only=True, slots=True, add the UNSTABLE sentence, and construct it with keywords. - Add an anonymous 401 test.
- Rewrite the PR title, description, and commit body to match the diff and the platform's existing URL include.
- Decide on the three questions: the ADR amendment, the cross-org disclosure, and branch-qualified keys.
- 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
createandupdatemethods that raiseNotImplementedError.
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.pyaddsCompetencyReadPermission. 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'))tocms/urls.py.openedx-platformalready includesopenedx_learning.urlsunderapi/, so following the instruction would mount the API a second time atapi/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 competencyis 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 Noneinget_blocked_courseshas no message. The two asserts a few lines above it inget_courses_with_relative_criteriacarry 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
CompetencyBlockedCoursesPaginationdocstring, "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
pagealso gives 404 (Django REST framework'sInvalidPage), 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, thenot inassertion 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_familyis identical in both test files.make_leaf_criterionis copied intotest_views.pywhiletest_api.pyalready has it. The newcourse_key()helper is used in three places while other new lines readcourse_run.course_keydirectly. -
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-methoddirectives. The project's instructions say a suppression directive needs explicit approval. The module has precedent, and the nearest sibling avoids it by definingcreateandupdate. 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_coursesand 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".
|
|
||
| class CompetencyBlockedCoursesView(generics.GenericAPIView): | ||
| """ | ||
| GET-only. List the courses where a competency can't get a new criterion, paginated. |
There was a problem hiding this comment.
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'
Description
Implements #796: a read-only endpoint that lists the courses where a competency
can't get a new
CompetencyCriterion, because a taxonomy ancestor or descendant of it already hasone there.
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"} ]}This PR is built on top of #875 (pr), which provides
get_courses_with_relative_criteriaand thetagging hierarchy helpers.
CompetencyReadPermissionis copied verbatim from #878 (#681), so this PR doesn't depend on#878. The two copies are identical, so whichever PR merges second reuses the class from the
first, and the merge conflict there resolves to the same lines.
Changes
src/openedx_learning/applets/cbe/api.py: addsget_blocked_courses(tag, course_keys=None)(UNSTABLE) and the
BlockedCoursedataclass. It calls [BE] Add Competency hierarchy dominance validation to Competency Criteria Creation #666'sget_courses_with_relative_criteriawithout a course, so the read and the write-side rejection share one query and always agree on
which courses are blocked and which tag is named. That includes feat: enforce competency-hierarchy dominance for criteria #875's handling of archived rows:
an archived criterion, or one under an archived group, blocks nothing in either place.
rest_api/v1/views.py,serializers.py,urls.py,rest_api/paginators.py: addsCompetencyBlockedCoursesView,BlockedCourseSerializer,CompetencyBlockedCoursesQueryParamsSerializer, andCompetencyBlockedCoursesPagination(page size 100, max 500, pinned like
CompetencyRuleProfilePagination). Authentication andpermission match feat: add GET competency criteria-groups endpoint #878's criteria-groups view.
rest_api/v1/permissions.py: addsCompetencyReadPermission, identical to feat: add GET competency criteria-groups endpoint #878's.grandchild direction, plus API-level tests and the edge cases listed below.
Testing instructions
Automated:
pytest tests/openedx_learning/applets/cbe tests/openedx_tagging/test_api.py --no-cov.Manual, against a Tutor devstack with this branch's
openedx-coremounted. Mount the CBE API inyour local
cms/urls.pywithpath('api/cbe/', include('openedx_learning.urls')), runtutor dev exec cms ./manage.py cms migrate, and restart CMS. Log in as a staff user and get a JWTAuthorizationheader, then use three real course keys as Course X, Y and Z.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".
POST /api/cbe/v1/competencies/<tag_id>/criteria/: "Critical Thinking" inCourse X, "Cognitive Skills" in Course Y, "Analysis" in Course Z.
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".
GETthe same endpoint for "Critical Thinking". Expect Course Y naming "Cognitive Skills" andCourse Z naming "Analysis". Course X doesn't appear, because a tag's own criterion doesn't
block it.
?course_keys=<Course X>. Expect only Course X, withcount1.?page_size=1. Expectcount2, one row, and anextlink that returnsthe other course.
?course_keys=not-a-keyand with?course_keys=. Expect 400 for both.GETthe endpoint fortag_id=999999. Expect 404.GETthe endpoint as a non-staff user. Expect 403.Other information
openedx_learning.api. The change is purely additive.BlockedCourseSerializerand the query-params serializer carry# pylint: disable=abstract-method, the same as the plainSerializersubclasses inopenedx_taggingandopenedx_content.Related to #796
🤖 Generated with the help of Claude Code