Repository navigation
feat: add read-only REST endpoint for CBE rule profiles - #846
Conversation
|
Thanks for the pull request, @javoconsultant! 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. |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
I had already approved this code here: jesperhodge#9
Please take this PR out of Draft since it is ready.
|
@javoconsultant One change is needed on this PR before it can support the Competency Management page. Reading the rule profiles must be allowed for course authors who are not platform staff, not only for taxonomy admins. This comes from @AShatsila's review on openedx/frontend-app-authoring#3275: that page is used by course authors (#672), and it reads the instance-wide default profile from What to change:
|
|
@mgwozdz-unicon could you review this again? i've added some permission changes here |
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude and I reviewed together. Items 1-4 are from me, and items 5-7 are from Claude. Claude drafted this comment.
@javoconsultant Thanks for the quick turnaround on the permission change. The logic in can_view_competency_rule_profile looks right to me. The remaining requests are about docstrings, tests, and the PR description. Please make all of them before merge:
- In
src/openedx_learning/applets/cbe/rules.py, thecan_view_competency_rule_profiledocstring quotes the Competency Management page's UI text ("score of X% or higher"). A backend library docstring shouldn't depend on frontend copy that can change at any time. Please replace the sentence starting "Reading competency configuration is an authoring concern" with something like: "Course authors, not only platform staff, need to read this." - Please drop the docstrings from the new and changed test functions in
test_rules.py, and fromtest_any_signed_in_caller_may_read_the_collectionintest_views.py. The test names already say what each test checks, and the repo's pylint config exemptstest_functions from the docstring requirement (no-docstring-rgxinpylintrc). The phrase "part company" in the disabled-taxonomy test's docstring was also hard to follow on first read, which is one more reason to remove it. - Please delete
test_anonymous_is_refused_by_the_endpoint_rather_than_by_this_predicate. It asserts that the predicate returnsTruefor an anonymous user, which no requirement asks for, so the test locks in counterintuitive behavior. The real requirement, that an anonymous request is refused, is already covered by the(None, 401)case intest_views.py. - Please delete
test_permission_is_registered_for_the_django_permission_check. Its purpose is to prove the permission is registered withrules, but the("user", 200)case intest_any_signed_in_caller_may_read_the_collectionalready proves that end to end: if the permission were not registered,has_permwould refuse everyone and that request would get a 403. - Please merge
test_staff_may_view_rule_profiles,test_non_staff_may_view_rule_profiles, andtest_staff_and_non_staff_alike_may_view_the_instance_wide_defaultinto one test, for example parametrized over the two user fixtures. The predicate receives the same input in all three: calling it with no profile and calling it with the default profile both pass no taxonomy toview_taxonomy, because the default profile has no taxonomy. - The
can_view_competency_rule_profiledocstring says "An anonymous caller is turned away by authentication at the endpoint, not here." Strictly, the refusal comes from DRF'sDjangoObjectPermissions, whoseauthenticated_users_onlyflag defaults toTrue. The authentication classes only decide whether that refusal is reported as a 401 or a 403. Please reword it to something like: "The endpoint refuses an anonymous caller before this predicate is asked." - Please update the PR description to match the new behavior. Manual testing step 6 still expects
403 Forbiddenfor the non-staff user, and the note below it says the collection is refused outright; that user now gets200 OKwith the default profile, so a tester following the steps would conclude the PR is broken. The "What this does" section and step 2 still say the predicate defers tois_taxonomy_admin, and the "What this does" and "Reviewing this PR" sections still describetest_permission_is_registered_for_the_django_permission_checkand the anonymous predicate test, which items 3 and 4 remove.
ormsbee
left a comment
There was a problem hiding this comment.
Just a few small things. Thank you.
Claude drafted this review, ran the test suite and quality checks on d4198cc, did the trial merge against main, and ran the has_perm probe quoted in the rules.py comment.
| Asked through ``has_perm`` rather than by calling ``can_view_taxonomy``: openedx-platform | ||
| replaces that rule with an org-aware one via ``rules.set_perm``, which a direct call skips. | ||
| """ | ||
| return user.has_perm("oel_tagging.view_taxonomy", profile.competency_taxonomy if profile else None) |
There was a problem hiding this comment.
Two small things:
Anonymous callers. This predicate, and so user.has_perm("openedx_learning.view_competencyruleprofile"), is True for AnonymousUser, because both openedx-core's can_view_taxonomy and the platform override return True the moment taxonomy is falsy. The docstring's "the endpoint refuses an anonymous caller before this predicate is asked" is accurate: DRF's DjangoModelPermissions.has_permission checks authenticated_users_only before it asks any predicate. Claude ran this on d4198cc (SQLite test settings):
can_view_competency_rule_profile(AnonymousUser()) -> True
AnonymousUser().has_perm("openedx_learning.view_competencyruleprofile") -> True
GET /api/cbe/v1/rule_profiles/ as anonymous -> 401
I'd rather the permission be right on its own than depend on which view happens to be in front of it, since #847's view, the LMS eventually, and anything calling has_perm() from Python are going to ask it too. @mgwozdz-unicon deleted the test that pinned the True because it was counterintuitive, and I agree with her, but I'd like the behavior gone rather than just the test. It should be one line at registration:
rules.add_perm(
"openedx_learning.view_competencyruleprofile",
rules.is_authenticated & can_view_competency_rule_profile,
)Course- and organization-scoped profiles. "An unscoped profile passes no taxonomy" in the docstring also covers the course- and organization-scoped ones, since profile.competency_taxonomy is None for them too, so view_taxonomy(None) grants them to everyone (same probe: a non-staff user gets True for both kinds, and so does AnonymousUser). Nothing can create those rows yet, so it's not reachable, but I don't want "everyone" to be what we get by falling through. Please have the predicate handle them explicitly–refuse them, or at least branch on them with a comment naming the ticket that will decide–so that whoever enables course-scoped profiles has to make a decision rather than inherit one. Something like this (untested, just to illustrate):
@rules.predicate
def can_view_competency_rule_profile(user, profile=None):
if profile is None:
# Listing. Any row-level filtering happens in the queryset.
return True
if profile.competency_taxonomy_id is not None:
return user.has_perm("oel_tagging.view_taxonomy", profile.competency_taxonomy)
if profile.course_id is None and profile.organization_id is None:
# The system default: anyone who is signed in may read it.
return True
# Course- and organization-scoped profiles: no authoring flow creates these yet, and
# neither tagging permission describes who may see them. Refuse until <ticket> decides.
return FalseFWIW, I'd also rather the system-default case say True directly than get there via view_taxonomy(None). The platform's hook for "who can see CBE configuration" should be set_perm on openedx_learning.view_competencyruleprofile itself, the same way it overrides oel_tagging.view_taxonomy, and that reads more clearly if the default rule here is written in CBE's own terms. But I won't insist on that part as long as the two points above are handled.
| return user.has_perm("oel_tagging.view_taxonomy", profile.competency_taxonomy if profile else None) | ||
|
|
||
|
|
||
| rules.add_perm("openedx_learning.view_competencyruleprofile", can_view_competency_rule_profile) |
There was a problem hiding this comment.
[Comment, no action needed] For whoever writes the platform-side override of this: rules.add_perm raises KeyError if the name is already registered, so the override has to use set_perm, and the app that does it has to come after openedx_learning in INSTALLED_APPS so that this module is imported first. That's already how content_tagging works relative to openedx_tagging (it's listed right after it), but openedx_learning sits near the bottom of the platform's list, so it's an easier mistake to make here.
There was a problem hiding this comment.
@ormsbee thanks, good to know. One more thing for whoever writes that override: the signed-in check is now part of the registration (rules.is_authenticated & can_view_competency_rule_profile), not the predicate, so the set_perm replacement needs to keep is_authenticated too.
| CBE API v1 URLs. | ||
| """ | ||
|
|
||
| from django.urls.conf import include, path |
There was a problem hiding this comment.
| from django.urls.conf import include, path | |
| from django.urls import include, path |
django.urls.conf is the implementation module; django.urls is the documented import path.
| """Shared fixtures for the CBE criteria test modules.""" | ||
| """Shared fixtures for the CBE test modules: schema, deletion, tree-integration, and REST tests.""" | ||
| import pytest | ||
| from django.contrib.auth.models import User as UserType # pylint: disable=imported-auth-user |
There was a problem hiding this comment.
This is where the rebase conflicts. main's 5984e3e added a user fixture to this file (get_user_model().objects.create(username="learner"), plus a now fixture) for the student status tests, and this PR adds a user fixture of its own. They're both a plain non-staff user, and nothing in test_rules.py or test_views.py depends on the username, so please keep main's and drop this one. While you're there, I'd switch staff_user to get_user_model() as well and drop this import (and the matching one in test_rules.py): UserType in openedx_tagging.rules is the User | AnonymousUser type alias, so aliasing the concrete model to the same name is confusing when reading the two side by side. Claude did the trial merge exactly that way–main's file plus your staff_user, api_client and staff_client fixtures–and the CBE suite passes on the merged tree (160 tests, vs. 111 on this branch alone).
3e9ebf4 to
ad50631
Compare
ormsbee
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on all of these. The predicate now does what I was after, the README section is exactly what I wanted, and the rebase resolution matches the trial merge. CI is green, and Claude re-ran the suite, the quality tools and makemigrations --check on ad50631 for me: all clean.
Please squash this down to a single feat: commit. The headline should match what's actually here–something like feat: add read-only REST endpoint for CBE rule profiles, since "implement CBE REST API" promises more than one endpoint–and the body should carry the context from the PR description: why this is a collection with pagination from day one, why scope_code stays internal, and who may read which profile and why that goes through has_perm.
Once that's pushed I'll merge. semantic-release should cut a version from the feat: commit, and then openedx/openedx-platform#39099 can bump the pin.
Thank you!
Claude drafted this review and ran the test suite, quality checks, makemigrations --check, and a has_perm probe against ad50631.
ad50631 to
545aa7a
Compare
|
@ormsbee i've addressed all your comments |
|
There was no commit message body with expanded context. I added one with Claude providing the initial draft. |
For #773 (#773). A complete implementation of that ticket in one PR. Replaces jesperhodge#9, rebased onto
mainnow that #800 has merged.Depends on:
CompetencyRuleProfilemodel, its seeded system-default row, andopenedx_learning's place in the layering contract. This PR is based directly onmain.rest_api/orurls.pyanywhere undersrc/openedx_learningonmain), so this PR builds them. See "What [BE] Build endpoint for creating a Competency Criteria Group #664 hasn't actually landed" below.idis on the response at all: it's the identifier [BE] Update/Edit Competency Criteria #759's criterion-update endpoint is meant to accept.What this does
Adds one read-only collection endpoint,
GET rule_profiles/, returning every non-archivedCompetencyRuleProfileto any signed-in caller, in the standard paginated DRF envelope.get_competency_rule_profiles()(applets/cbe/api.py) — the applet's public API function:CompetencyRuleProfile.objects.filter(archived=False).order_by("id"). The ordering is load-bearing, not cosmetic — an unordered queryset gives a paginating caller overlapping or skipped pages.CompetencyRuleProfileSerializer— read-only,fields = ["id", "scope_type", "rule_type", "rule_payload", "archived"].scope_typeis aSerializerMethodFieldthat recognizes all four scope kinds (system_default,taxonomy,course,organization) from the outset by which_idcolumn is set, so enabling a narrower scope later needs no serializer change.scope_codeand the raw scope foreign keys are left out, per ADR-0002 Decision 3.rule_payloadis returned verbatim,scalekey included, so a caller can't misread the threshold as a fraction versus a percentage.CompetencyRuleProfileView— aListModelMixin+GenericViewSet, not a bareListAPIView, so a create/detail route can be added later as another mixin without touching the URL module.CompetencyRuleProfilePagination— page size pinned at 100/500 on the view rather than inherited from the consuming project'sDEFAULT_PAGINATION_CLASS, matching howopenedx_taggingpins its own paginators, because this is a published library and the page size is part of its contract.can_view_competency_rule_profile— a newrulespredicate: whoever may view a profile's taxonomy may read that profile. It asksuser.has_perm("oel_tagging.view_taxonomy", ...)with the profile'scompetency_taxonomy, or with no taxonomy when there is no profile or the profile has none. Course authors, not only platform staff, need to read rule profiles, andview_taxonomygrants a request with no taxonomy to everyone, so any signed-in caller may read the seeded default; a profile scoped to a disabled taxonomy is readable only by taxonomy admins. It goes throughhas_permrather than callingcan_view_taxonomydirectly so that openedx-platform's org-aware replacement of that rule (installed withrules.set_perm) applies here too. The list endpoint asks this question once for the collection, with no profile, so it does not filter rows per profile yet; that is left for when narrower scopes are enabled.src/openedx_learning/rules.py— required forrules.apps.AutodiscoverRulesConfig's autodiscovery to ever import the applet'srules.py; without this module the predicate never registers and every caller is silently refused. The("user", 200)case intest_any_signed_in_caller_may_read_the_collectionis what proves this file does its job end to end: if the permission were not registered,has_permwould refuse everyone and that request would get a 403.What #664 hasn't actually landed, so this PR builds it
#773's technical notes assume the CBE REST package,
src/openedx_learning/urls.py, and the mount from Studio already exist. Onmainnone of it does, so this PR addsapplets/cbe/rest_api/(its__init__.py,urls.py, andv1/urls.py),src/openedx_learning/urls.py, and theprojects/urls.pymount, in addition to the endpoint #773 actually asks for.One thing worth a second look before merge. #773 says #664 mounts CBE at
api/cbe/rest_api/, putting this endpoint atapi/cbe/rest_api/v1/rule_profiles/. This PR's mount doesn't reproduce that segment —rest_api/here is only the Python package name — so the URLs resolve toapi/cbe/v1/rule_profiles/instead (pinned bytest_collection_resolves_to_the_documented_path). If the real #664 is meant to still add a literalrest_api/path segment on top of this, this route will need to move under it later; if this PR is effectively standing in for #664's mount, #773's technical notes should be corrected instead. Flagging rather than guessing.Reviewing this PR
test_api.py—get_competency_rule_profiles()alone, no HTTP: returns the seeded default, excludes archived rows, orders byid.test_rules.py— the predicate directly: staff and non-staff alike may read profiles with no taxonomy (both with no profile and with the seeded default), and a profile on a disabled taxonomy is refused to non-staff but allowed to staff, which is what shows the predicate really defers toview_taxonomy. Registration withrulesis covered end to end intest_views.py, not here.test_views.py— works through [BE] Read the rule profiles an instance defines #773's acceptance criteria close to one-for-one: response shape and content, omitted internal fields, all four scope types distinguishable in one response, retired-row exclusion, an empty table reported as an empty collection rather than a 404, the paginated envelope, an uneven page boundary followed to its end, order stability across two calls, and the permission matrix: 401 for an anonymous caller, 200 for both a non-staff and a staff user.Verifying
1. Migrate. The system-default profile arrives from a data migration, so there is nothing to
create by hand afterwards:
Confirm the seeded row before going on — every expected body below is this row:
2. Create the two users the permission matrix needs — a platform staff user, and a plain
signed-in user standing in for a course author:
can_view_competency_rule_profileasksoel_tagging.view_taxonomyabout the profile'staxonomy. The seeded default has none, and
view_taxonomygrants a request with no taxonomy toeveryone, so both users may read it. Use a plain
is_staff=Trueuser rather thancreatesuperuser—ModelBackendgrants a superuser every permission unconditionally, so asuperuser returns 200 even if the predicate were never registered, and step 7 would prove
nothing.
3. Mint a session cookie for each user. The endpoint accepts JWT or session auth;
cbe-learneris not staff and so cannot log in through/admin/, so create both sessionsdirectly:
4. Start the server:
5. Anonymous — expect
401 Unauthorized. No cookie, no token:The
WWW-Authenticateheader is why this is a 401 and not a 403:JwtAuthenticationsuppliesone, so DRF reports an unidentified caller rather than a refused one.
6. Non-staff — expect
200 OKand the seeded system default. Signed in, but not platformstaff, as a course author would be:
The body is the same paginated envelope shown in step 7. This step also shows the predicate is
registered: an unregistered permission is refused to every user who is not a superuser, so this
request would get
403 Forbiddeninstead.7. Staff — expect
200 OKand the seeded system default, in the paginated envelope:Worth checking on that body:
scope_typeis"system_default"(the seeded row has every scopecolumn null);
rule_payloadcarriesscalealongsideopandvalue, so0.8cannot bemisread as 0.8%;
scope_code,organization,course, andcompetency_taxonomyare allabsent; and the results are wrapped in an envelope rather than returned as a bare array.
Refs #773
🤖 Generated with Claude Code