Alezconsultant/665 criterion endpoint on 846 - #847
alezconsultant wants to merge 7 commits into
Conversation
…RuleProfileSerializer
Add POST /api/cbe/v1/competencies/<int:tag_id>/criteria/, associating a gradeable subsection with a competency: attaches a CompetencyCriterion to an existing leaf CompetencyCriteriaGroup when group_id is supplied, or derives/creates whatever part of the root/course-level/leaf hierarchy doesn't yet exist for this competency and course when it isn't. Add to applets/cbe/api.py: resolve_competency_tag(), resolve_or_create_leaf_group(), resolve_supplied_leaf_group(), create_competency_criterion(), and associate_competency_criterion() as the single public entry point. Add migration 0008: two partial UniqueConstraints on CompetencyCriteriaGroup (one root per tag; one course-level group per tag+course), making root/course-level resolution race-safe via plain get_or_create() rather than select_for_update or catch-and-retry, neither of which appears elsewhere in this codebase. Add CompetencyCriterionSerializer and CompetencyCriterionCreateView to the existing CBE REST module (serializers.py/views.py/urls.py). Reuses oel_tagging.can_tag_object for authorization rather than a new DjangoObjectPermissions class: every such class already in this codebase only ever contributes the authentication gate, since its underlying rules predicate returns True whenever called with no object (the case DRF's has_permission() always hits), so copying that shape here would mean registering a new, otherwise-unused permission for no behavioral gain. Deviations from the ticket's literal text, developer-approved: - competency_rule_profile_id resolves to the seeded system-default CompetencyRuleProfile's id when all three rule fields are omitted, rather than persisting null: CompetencyCriterion's own profile_xor_override_check constraint never allows all three fields null, and ADR 0002 Decision 4 resolves a criterion's profile to a concrete row at creation time, never leaves it unresolved. - The duplicate-association check (reject a second criterion for the same tag/object pair) stays an application-level check with no backing DB constraint. ADR 0002's own worked example requires the same ObjectTag to be referenced by more than one CompetencyCriterion across different groups under the same competency, so a schema-level UniqueConstraint would foreclose that documented capability for every current and future caller, not just this endpoint. tag_object() replaces the full tag list for a taxonomy/object_id pair rather than appending, so create_competency_criterion() reads the object's existing tags first and unions in the new one, rather than risking silently dropping a sibling competency's tag applied to the same object under the same taxonomy. Tests cover every acceptance-criteria scenario in openedx#665, plus the constraint-backed concurrent-request race case for group creation and the atomic-rollback case for a downstream criterion-creation failure. Related to openedx#665 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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: 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. |
2 participants
Description
Stacked on #846 ; review only the last commit
Adds the create-criterion endpoint from #665:
POST /api/cbe/v1/competencies/<int:tag_id>/criteria/, letting a course author associate agradeable subsection with a competency, either by attaching it into an existing leaf
CompetencyCriteriaGroupor by deriving/creating whatever part of the root/course-level/leafgroup hierarchy doesn't yet exist for this competency and course.
Changes
src/openedx_learning/migrations/0004_criteria_group_unique_constraints.py(new): twopartial
UniqueConstraints onCompetencyCriteriaGroup— one root pertag(
condition=Q(parent__isnull=True)), one course-level group per(tag, course, parent)(
condition=Q(course__isnull=False)). Matching constraints added toCompetencyCriteriaGroup.Metainmodels/criteria.py.src/openedx_learning/applets/cbe/api.py: addsresolve_competency_tag(),resolve_or_create_leaf_group(),resolve_supplied_leaf_group(),create_competency_criterion(), andassociate_competency_criterion()(the single publicentry point this ticket's view calls).
src/openedx_learning/applets/cbe/rest_api/v1/serializers.py: addsCompetencyCriterionSerializeralongside the existingCompetencyRuleProfileSerializer.object_id/group_id/logic_operatorare plain write-only fields (none of them areCompetencyCriterionmodel fields);competency_rule_profile_id,competency_criteria_group_id, andoel_tagging_objecttag_idare the ticket's contractedJSON names, mapped via explicit
source=to the model's actual attributes(
rule_profile_id,group_id,object_tag_id).src/openedx_learning/applets/cbe/rest_api/v1/views.py: addsCompetencyCriterionCreateView(generics.CreateAPIView)alongside the existingCompetencyRuleProfileView.src/openedx_learning/applets/cbe/rest_api/v1/urls.py: addscompetencies/<int:tag_id>/criteria/, a plainpath()(not a second routerregistration — the URL carries a path parameter mid-route, so it isn't a flat
router-friendly resource like
rule_profiles).tests/openedx_learning/applets/cbe/test_api.py,test_views.py,test_criteria_models.py: unit and DRF integration tests (see Tests below); onepre-existing test in
test_criteria_models.pyupdated to use two tags instead of one,since it previously created two root groups for the same tag, which the new constraint
now correctly rejects.
Tests
One test per scenario in #665's Acceptance Criteria (all 20): the full-hierarchy derivation
and reuse scenarios,
logic_operatorprovided/defaulted, thegroup_id/logic_operatormutual-exclusion rejection, every supplied-group validation rejection (non-leaf, wrong
competency, wrong course), all three duplicate-association rejections, the all-null-rule-
fields deviation above, the same-taxonomy tag-preservation scenario, every not-found/malformed
scenario, and the permission rejection. Plus unit tests for
resolve_or_create_leaf_group()and
resolve_supplied_leaf_group()directly, including two tests that pre-commit acompeting row out-of-band to prove the constraint-backed
get_or_create()fallback (standingin for a real concurrent request), two tests proving the constraints themselves exist via a
direct
IntegrityError, and one test proving the atomic block rolls back newly-created groupswhen the final
CompetencyCriterion.objects.create()call fails.Depends on
openedx-core#9 (#773), which this branch is based on and which already
adds the CBE REST scaffolding (
openedx_learning/urls.py, therest_api/package,serializers.py/views.py/urls.py) this PR extends rather than creates. That PR is itselfbased on #641/#613's
CompetencyCriteriaGroup/CompetencyCriterion/CompetencyRuleProfilemodels.
Companion PR
A one-line
openedx-platformchange (cms/urls.py:path('api/cbe/', include('openedx_learning.urls')), mirroring the existingcontent_taggingmount) is aseparate PR against that repo, not included here.
Verification
pytest tests/openedx_learning/applets/cbe/ --no-cov: 147 passed.pylint,mypy,pycodestyle,isort --check-only,pydocstyle: all clean on everychanged file.
lint-imports: 2 contracts kept, 0 broken.make pii_checkcheck: 2 pre-existing, unrelated failures only(
openedx_content.Draft/PublishableEntityVersion, confirmed viagit stashto predatethis branch). Both models this PR touches already carry
.. no_pii:; the migration adds nonew fields.
openedx-platformpermission chain(a real
CourseStaffRoleholder granted access, an unpermitted user refused), isolated froman unrelated, pre-existing import failure elsewhere in that repo's
content_taggingapp —see the companion PR for details.
Related to #665
Created with the help of Claude