Repository navigation
feat: evaluate and record a learner's leaf competency status for a grade - #862
alezconsultant wants to merge 1 commit into
Conversation
Add record_graded_object_statuses(*, user_id, scores) to the CBE applet's public API: given a learner and one or more graded-object fractions, resolves the competency criteria attached to those objects, evaluates each against its assigned rule profile or per-criterion override, and writes the learner's leaf-level status, raising a stored status but never lowering one. Opens no transaction of its own, so the caller's grade write and the leaf status commit or roll back together. Add GradedObjectScore, a frozen dataclass carrying an object_id and a fraction between 0 and 1, in a new data.py. Add evaluate_rule to the existing rule_payloads.py rather than a new module, deriving its operator set from the same map validate_rule_payload already uses, so the two can never disagree about which operators exist. A criterion whose rule cannot be applied (an unrecognized rule type or an invalid payload) is skipped and logged at warning level; the grade write is not affected. Related to openedx#699 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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. |
tbain
left a comment
There was a problem hiding this comment.
I reviewed this as another PR here and approved it:
jesperhodge#24
ormsbee
left a comment
There was a problem hiding this comment.
@tbain already went through this on the fork PR, so I mostly looked at the write path and at how it behaves on MySQL when grade tasks overlap. Claude ran the CBE suite, the quality tools, and makemigrations --check on this branch for me (all clean, and CI is green on MySQL 8).
The code does what #699 asks, the tests cover the acceptance criteria well, and the conditional-update write looks right to me. A few small requests before merge:
- It looks like openedx-platform#39179 is going to want to import some models from
openedx_learning. Please expose those models in themodels_api.pyfor this app. Otherwise, openedx-platform#39179 will run into import-linter issues. - Please skip archived criteria, now that #857 (
afce3e7) is onmain(inline onapi.py). #699 asked for the skip and for a note on the issue if the field didn't exist yet; the criterion-level field exists now, and theObjectTag-level one is #856. - Please put the learner and the object in the skipped-criterion warning (inline).
Nits are inline. Anything marked Nit: is optional.
Logistics: #846 has merged, so please rebase onto main. It conflicts on the from .models import line in api.py, and that's the whole conflict: Claude did the trial rebase for me, and with that one line resolved the CBE suite passes (201 tests), makemigrations --check is clean, and the quality checks pass.
Thank you.
Claude drafted this review and ran the test suite, the quality checks, makemigrations --check, and the query probes quoted inline for me on 44ac0e3, and then did the trial rebase onto main once #846 had merged.
| criteria = ( | ||
| CompetencyCriterion.objects.filter( | ||
| object_tag__object_id__in=list(fraction_by_object_id), | ||
| object_tag__tag__taxonomy__competencytaxonomy__isnull=False, |
There was a problem hiding this comment.
Please add archived=False to this filter once you rebase onto main (#857 landed as afce3e7 and added the column to CompetencyCriterion), with a test. #699 asked for criteria whose association is archived to be skipped, and for a note on the issue if the field didn't exist yet; the note never got written, and now the criterion-level half of the field does exist. An archived criterion is one an author removed from the tree after some learner already had status for it (ADR-0002 Decision 4, item 7), and I don't think a later grade should create a new status row for it, or raise an existing one, any more than it would for a hard-deleted criterion (@mgwozdz-unicon: does that sound right to you?). #643's rollup is going to have to ignore those rows too, so it's better if they're never written. If the intent is instead that archived criteria keep accumulating status so that un-archiving is lossless, I'd want that written down in ADR-0003, because it's the opposite of what "hidden from new associations" reads as.
@ufedaseyeuconsultant, @mgwozdz-unicon: if #674/#675 archive the leaves whenever they archive a group, the leaf flag is enough here. If a group can be archived with live leaves beneath it, this query needs to look at the group too. I'm not sure what the intended behavior is. Either way, it might make sense to make a custom manager on the model to encapsulate some of that logic in exactly one place as part of the work for those tickets.
Nit, while you're in there: object_tag__taxonomy__competencytaxonomy__isnull=False gives the same answer and skips the join through oel_tagging_tag. Claude printed both queries, and that one INNER JOIN is the only difference, on the no-criteria path that every grade write pays.
| log.warning( | ||
| "Skipping CompetencyCriterion %s: its rule cannot be applied: %s", criterion.pk, exc.messages, | ||
| ) |
There was a problem hiding this comment.
Please include the learner and the object in this message, to make it easier to diagnose a support issue from logging. Something like:
| log.warning( | |
| "Skipping CompetencyCriterion %s: its rule cannot be applied: %s", criterion.pk, exc.messages, | |
| ) | |
| log.warning( | |
| "Skipping CompetencyCriterion %s for user %s on object %s: its rule cannot be applied: %s", | |
| criterion.pk, user_id, criterion.object_tag.object_id, exc.messages, | |
| ) |
| """ | ||
| Record a learner's leaf-level competency status for each graded object in ``scores``. | ||
|
|
||
| Call this only from inside the same transaction that also writes the grade: this function |
There was a problem hiding this comment.
Nit (optional): Since the whole contract is "inside the caller's transaction", would it be worth raising if connection.in_atomic_block is false? In autocommit mode the atomic() inside get_or_create() would commit the leaf on its own, which is the one thing ADR-0004 Decision 1 says can't happen, and nothing would notice. pytest-django already wraps each test in atomic(), so the existing tests wouldn't need to change.
| ).update(status_id=status, modified=now) | ||
|
|
||
|
|
||
| def record_graded_object_statuses(*, user_id: int, scores: Sequence[GradedObjectScore]) -> int: |
| @@ -7,7 +7,9 @@ | |||
| """ | |||
There was a problem hiding this comment.
Nit: The module docstring above still describes this as the shapes and the validator. Please add a sentence about evaluate_rule(), since that's now what api.py imports from here.
Description
Implements #699: adds
record_graded_object_statuses(*, user_id, scores)to the CBE applet's public API. Given a learner and one or more graded-object fractions, it
resolves the competency criteria attached to those objects, evaluates each against its assigned
rule profile or per-criterion override, and writes the learner's leaf-level mastery status --
raising a stored status but never lowering one, and opening no transaction of its own, so the
caller's grade write and the leaf status commit or roll back together (ADR-0004 Decision 1).
Changes
src/openedx_learning/applets/cbe/data.py(new):GradedObjectScore, a frozen, keyword-onlydataclass carrying
object_id: strandfraction: Decimal. Validates its own contract atconstruction: a non-
Decimalfraction raisesTypeError, an out-of-range, NaN, or infiniteone raises
ValueError.src/openedx_learning/applets/cbe/rule_payloads.py: addsevaluate_rule(rule_type, payload, fraction) -> bool, exported in this module's existing__all__alongsidevalidate_rule_payload. Derives the existing_GRADE_OPERATORSfrozenset from the sameoperator-to-function map
evaluate_ruleuses for comparison, rather than independently, sothe validator and the evaluator can't drift apart on which operators exist.
src/openedx_learning/applets/cbe/api.py: adds the publicrecord_graded_object_statuses,plus two private helpers --
_effective_rule(reads whichever of a criterion's assignedprofile or per-criterion override is set) and
_raise_leaf_status(the monotoneget_or_create+ conditionalUPDATE ... WHERE status_id < computedwrite, matchingADR-0004 Decision 4's raise-only rule). Both
record_graded_object_statusesandGradedObjectScoreare added to this module's__all__, reaching external callers throughthe existing
openedx_learning.apiwildcard aggregator with no further changes needed there.tests/openedx_learning/applets/cbe/test_record_graded_object_statuses.py(new): one test peracceptance-criteria scenario in the ticket, including both
@unit-test-onlyones, plus aregression test for comparing a
Decimalfraction against a rule_payload value Django'sJSONFieldreturns as a plainfloat, and coverage for a duplicateobject_id, a genuinestorage error propagating uncaught, and a rolled-back caller transaction leaving no status
behind.
tests/openedx_learning/applets/cbe/test_rule_payloads.py: adds tests forevaluate_ruleanda regression test guarding the operator-set derivation above.
No new model or migration: this reads only what #641/#642 already define. No
.importlinterchanges: every new import stays inside
openedx_learningor points down toopenedx_tagging.Manual Testing
Manual testing via django shell/containers.
docker exec -it -e PYTHONPATH=/mnt/openedx-core/src tutor_dev-cms-1 ./manage.py cms shell:Automated:
Related to #699
🤖 Generated with Claude Code