Repository navigation
Alezconsultant/665 criterion endpoint on 846 - #847
alezconsultant wants to merge 2 commits 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. |
2c888e6 to
2ca89b7
Compare
The help_text on CompetencyCriteriaGroup.archived and CompetencyCriterion.archived named "the criteria-tree read endpoint" (openedx#681, not yet built) as if it were the only consumer, when openedx#674 and openedx#675 are what actually set the flag and openedx#847's create_leaf_group() will need to read it too. Reworded both to match CompetencyRuleProfile.archived's existing convention (ADR-0002 Decision 2): describe what archiving means for the row itself, not what one downstream endpoint does with it. Regenerates the migration (Django bakes help_text into it) with a descriptive name instead of the auto-generated "_and_more" one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
I reviewed and approved here: jesperhodge#14
45c0162 to
dd837ca
Compare
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Apologies for the delay on this. It took me a while to try to wrap my head around this, and I'm honestly still not sure I totally grok everything that's going on.
Claude ran the CBE suite, the quality checks, and makemigrations --check on this head for me (all clean, and CI is green on MySQL), and then sent a handful of requests the tests don't send, which is where most of what follows comes from.
The shape is right and the tests are thorough for the inputs they cover. What I'd like fixed before merge is mostly about input the tests don't send, plus one design question that this PR makes concrete:
- Please validate the rule fields before the row reaches the database: two field combinations 500 on the check constraint, an archived profile is accepted for a new association, and a taxonomy with
allow_multiple=False500s (inline onserializers.pyandapi.py). - The generated root and course-level group names can be longer than the column, which is a
DataErroron MySQL in strict mode (inline onapi.py). - #665's taxonomy-scoped profile scenario isn't implemented; the criterion always gets the system default (inline on
api.py). Please implement it or write down that it's deferred; I'd rather have the lookup. - Please keep
Http404out ofapi.py(inline). - [Question] How archived container groups are meant to interact with
get_or_create()and the new unique constraint (inline oncriteria.py). #857 has merged since I started this review, soarchivedis on both models now, and once this is rebased, this is the code that ignores it. I don't need code for this here, but I want the decision written down before this merges, because the fix if we get it wrong is anotherscope_code-style column and a data migration on a table that will have grown by then.
There are also some smaller, non-blocking issues in the inline comments.
Logistics: #846 and #857 have both merged, so please rebase just this commit onto main. Claude did that for me and it applies with no conflicts; the one thing that breaks is the migration number, since #857 took 0014. Please rename it to 0015_criteria_group_unique_constraints and point its dependency at 0014_add_archived_to_criteria_group_and_criterion (a renumber, not a merge migration). With just that change, makemigrations --check is clean and the CBE suite passes on the rebased tree (212 tests).
Thank you.
Claude drafted this review and ran the test suite, quality checks, makemigrations --check, and the probe requests quoted inline on 2ca89b7 for me, and then did the trial rebase onto main once #846 and #857 had merged.
| logic_operator = serializers.ChoiceField( | ||
| choices=LogicOperator.choices, write_only=True, required=False, allow_null=True, | ||
| ) | ||
| rule_profile_id = serializers.IntegerField(required=False, allow_null=True) |
There was a problem hiding this comment.
Please validate the rule fields before the row reaches the database. Claude sent these requests at 2ca89b7, with the same fixtures and permission stub as test_views.py:
rule_payload_override alone -> IntegrityError on oel_cbe_criterion_profile_xor_override_check (a 500)
rule_profile_id + both overrides -> IntegrityError, same constraint (a 500)
rule_profile_id of an archived profile -> 201
rule_type_override alone -> 400, from clean()
unknown rule_profile_id -> 400, from full_clean(), keyed "rule_profile"
The xor rule is already in the model's constraint and docstring, so the API should say the same thing as a 400 keyed by the offending field before the database gets to say it as a 500. And an archived profile shouldn't be accepted for a new association: ADR-0002 Decision 3 says archived profiles are hidden from authoring and new associations, and get_competency_rule_profiles() already hides them from the list the UI will pick from, so accepting one here only happens when a client guesses an id. Consider putting the checks in associate_competency_criterion() so that a Python caller gets the same answer as the view.
| existing_values = [existing_tag.value for existing_tag in get_object_tags(object_id, taxonomy_id=tag.taxonomy_id)] | ||
| if tag.value not in existing_values: | ||
| existing_values.append(tag.value) | ||
| tag_object(object_id, tag.taxonomy, existing_values) |
There was a problem hiding this comment.
tag_object() raises ValueError when the taxonomy has allow_multiple=False and this is the second competency on the object, and again when the object is already at OBJECT_MAX_TAGS (_get_current_tags() and _check_new_tag_count()). Claude hit the first one and it came back as a 500. Please catch it here and raise a ValidationError on object_id, so the taxonomy's message reaches the caller.
| root, _ = CompetencyCriteriaGroup.objects.get_or_create( | ||
| tag=tag, | ||
| parent=None, | ||
| defaults={"name": f"{tag.value} (root)"}, |
There was a problem hiding this comment.
Tag.value is max_length=500 and CourseRun.title is 255, but CompetencyCriteriaGroup.name is 255, so both of these names can overflow. Claude created a 300-character tag and a 255-character course title, and create_leaf_group() stored a 307-character root name and a 558-character course-level name (SQLite doesn't enforce the length). On MySQL with STRICT_TRANS_TABLES, which Tutor sets and Django expects, that's a DataError and a 500; without strict mode, MySQL truncates silently. Since this is a real outlier and #665 says these two groups are never shown in the UI, I'd just truncate name to fit and make a note of it in the docstring.
Related question: is the leaf meant to have a name? #665 says "named from the course's display name" for the leaf, and you've put that on the course-level group instead, which seems more sensible to me. But whoever is building #672 should know which it is.
| object_tag = ObjectTag.objects.get(object_id=object_id, taxonomy_id=tag.taxonomy_id, tag_id=tag.id) | ||
|
|
||
| if rule_profile_id is None and rule_type_override is None and rule_payload_override is None: | ||
| rule_profile_id = CompetencyRuleProfile.objects.get( |
There was a problem hiding this comment.
This always assigns the system default, but #665's acceptance criteria include "A taxonomy-scoped rule profile is assigned ahead of the system default", and ADR-0002 Decision 4's table says the taxonomy profile wins when no course or organization profile exists. Claude created a live taxonomy-scoped profile for the tag's taxonomy and POSTed with no rule fields: the criterion came back with the system default. The ADR's MVP note says every criterion gets the default because no authoring screen supplies a context, but this endpoint does know the taxonomy (it's tag.taxonomy), and taxonomy-scoped profiles can be created today through the admin, which is what test_api.py's own fixtures do. Since the ADR also says the FK is never re-resolved at read time, a criterion created with the wrong profile stays wrong until something edits it.
Please either look up the live taxonomy-scoped profile first and fall back to the default, or state in the commit message that profile resolution beyond the default is deferred and get #665 updated to match. I'd rather have the lookup; it's a few lines. (I wouldn't go as far as course-scoped profiles here: the ADR says a criterion is assigned using the scope the creating screen operates in, and this one is taxonomy-scoped.)
| Raises Http404 if no such Tag exists, or if it isn't on a CompetencyTaxonomy. Shared by | ||
| the view and :func:`associate_competency_criterion` so both agree on what counts as valid. | ||
| """ | ||
| tag = get_object_or_404(Tag, pk=tag_id) |
There was a problem hiding this comment.
Please keep HTTP out of api.py. The only other api.py in this repo that imports from django.http is the components one, where the function's whole job is to build the response the reverse proxy serves, 404s included; nothing else here raises Http404 from a lookup. And this module is going to be called from places that aren't views (the containment check in #666, event handlers, management commands). Raise Tag.DoesNotExist and CompetencyCriteriaGroup.DoesNotExist here (or a small CBE exception, if you'd rather callers not depend on model classes), and have the view turn those into Http404. #665 suggested get_object_or_404, but that was about the view not having an exception handler, and the view is where the translation belongs. Same note for resolve_supplied_leaf_group().
| """ | ||
| import pytest | ||
| import rules | ||
| from django.contrib.auth.models import User as UserType # pylint: disable=imported-auth-user |
There was a problem hiding this comment.
UserType in openedx_tagging.rules is the User | AnonymousUser alias, so aliasing the concrete model to the same name is confusing when the two files are read side by side. #846 dropped this alias from conftest.py and test_rules.py before it merged, so after the rebase this would be the only one left. get_user_model() is fine, or just drop the annotation.
|
|
||
| expected = [default_rule_profile, taxonomy_scoped, organization_scoped] | ||
| assert list(get_competency_rule_profiles()) == expected | ||
| assert list(get_competency_rule_profiles()) == expected |
There was a problem hiding this comment.
| assert list(get_competency_rule_profiles()) == expected |
Is there a reason this was duplicated?
| assert CompetencyCriteriaGroup.objects.filter(tag=tag, course=course_run).count() == 1 | ||
|
|
||
|
|
||
| def test_create_leaf_group_locks_the_tag_row_before_creating_groups(tag: Tag, course_run: CourseRun) -> None: |
There was a problem hiding this comment.
Nit: This test and test_associate_competency_criterion_locks_the_tag_row_before_the_duplicate_check pin the implementation (that select_for_update() was called, and in what order) rather than any behavior, and they'll have to be rewritten by whoever changes the locking, including if you take my suggestion on _reject_duplicate_criterion(). I'd drop them; the pre-committed-row tests above are the useful ones.
| # A different tag from `childless`'s: the one-root-per-tag constraint allows only one root group | ||
| # per tag, and this scenario is about child count, not about tag reuse. | ||
| other_tag = Tag.objects.create(taxonomy=tag.taxonomy, value=f"{tag.value} (other)") | ||
| parent = CompetencyCriteriaGroup.objects.create(tag=other_tag, logic_operator=logic_operator) | ||
| CompetencyCriteriaGroup.objects.create(tag=other_tag, parent=parent) | ||
| CompetencyCriteriaGroup.objects.create(tag=other_tag, parent=parent) |
There was a problem hiding this comment.
This change isn't needed, and the comment is wrong: there's no one-root-per-tag constraint in this PR (the commit message explains why MySQL can't have one), and NULLs are distinct in a unique index, so two roots for the same tag still save. Claude ran the pre-commit version of this file against this branch and it passes. Please revert the hunk, and fix the sentence in the PR description that says the constraint rejects this.
| # A different tag from `childless`'s: the one-root-per-tag constraint allows only one root group | |
| # per tag, and this scenario is about child count, not about tag reuse. | |
| other_tag = Tag.objects.create(taxonomy=tag.taxonomy, value=f"{tag.value} (other)") | |
| parent = CompetencyCriteriaGroup.objects.create(tag=other_tag, logic_operator=logic_operator) | |
| CompetencyCriteriaGroup.objects.create(tag=other_tag, parent=parent) | |
| CompetencyCriteriaGroup.objects.create(tag=other_tag, parent=parent) | |
| parent = CompetencyCriteriaGroup.objects.create(tag=tag, logic_operator=logic_operator) | |
| CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) | |
| CompetencyCriteriaGroup.objects.create(tag=tag, parent=parent) |
| Must run inside a transaction holding the tag row lock. | ||
| """ | ||
| assert tag.taxonomy_id is not None # narrowing for mypy; callers resolve competency tags only | ||
| # Locking reads, so a duplicate committed by a concurrent request is seen under MySQL REPEATABLE READ. |
There was a problem hiding this comment.
We should never be running MySQL with REPEATABLE_READ as the isolation level. Django has defaulted to READ COMMITTED with MySQL since Django 2.0. There isn't any good reason to override that behavior, given that the framework assumes READ COMMITTED.
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. Group creation is made race-safe by locking the competency's Tag row with select_for_update() inside the transaction, so two concurrent requests cannot each create a root or course-level group for the same tag. MySQL has no partial unique indexes, so the root group cannot be backed by a constraint. Add migration 0014: a UniqueConstraint on CompetencyCriteriaGroup (tag, course, parent) that backstops course-level group creation. 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. It runs under the same Tag row lock as group creation, with locking reads, so a duplicate committed by a concurrent request is seen under MySQL REPEATABLE READ. 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 concurrent-request race case for group creation, the duplicate-association case, 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>
dd837ca to
dd5f40e
Compare
|
@ormsbee Thanks for the review. Everything is addressed in the new commit, branch was rebased. PR description updated. Several of these decisions were made before the archived logic was designed.
Here are the answers and proposals to implementation: 1. Archived container groups
Container groups can be archived. #674, #675 and #717 tickets suggest that we should archive every level. #674's "must run all the way to the root group, not just the immediate parent, stopping only once a level still has active content of its own". I think option with 2. Leaf name
A course can have multiple leaf, so a name based on the course would be duplicated for leafs, and a name based on subsections would mean handling leaves with several subsections and edge cases after rename. I think it's better to update the #665 AC to match the current implementation. 3. Unique constraint on
|
4 participants
Description
Adds the create-criterion endpoint,
POST /api/cbe/v1/competencies/<int:tag_id>/criteria/.A course author uses it to associate a gradeable subsection with a competency. When the
request supplies a
group_id, the criterion is attached to that existing leafCompetencyCriteriaGroup. When it doesn't, the endpoint reuses or creates the root andcourse-level groups for this competency and course, and always creates a new leaf under them.
Changes
src/openedx_learning/migrations/0015_criteria_group_unique_constraints.py(new): oneUniqueConstraintonCompetencyCriteriaGroupover(tag, course, parent), so there is atmost one course-level group per competency, course and parent. It has no
condition,because MySQL has no partial indexes. A unique index allows repeated NULLs, and
courseisNULL for roots and leaves, so roots and leaves are not constrained by it. The matching
constraint is in
CompetencyCriteriaGroup.Metainmodels/criteria.py.src/openedx_learning/applets/cbe/api.py: addsresolve_competency_tag(),resolve_supplied_leaf_group(),create_competency_criterion(), andassociate_competency_criterion(), which is the single entry point the view calls. All ofthem are marked UNSTABLE. They raise Django
ValidationErrorand modelDoesNotExistexceptions, never HTTP exceptions. Group creation lives in the private
_create_leaf_group(), so no caller can create groups without going through theduplicate-association check.
src/openedx_learning/applets/cbe/rest_api/v1/serializers.py: addsCompetencyCriterionSerializer.object_idandlogic_operatorare write-only, becauseneither is a
CompetencyCriterionfield. The other JSON names match the model attributes(
group_id,rule_profile_id,object_tag_id).validate_object_id()rejects anunparseable usage key with a 400.
src/openedx_learning/applets/cbe/rest_api/v1/views.py: addsCompetencyCriterionCreateView. It checksoel_tagging.can_tag_objectfor the tag's taxonomyand the
object_id, then callsassociate_competency_criterion(), translatingValidationErrorinto a 400 andDoesNotExistinto a 404.src/openedx_learning/applets/cbe/rest_api/v1/urls.py: addscompetencies/<int:tag_id>/criteria/as a plainpath(), since the URL carries a pathparameter mid-route.
tests/openedx_learning/applets/cbe/test_api.pyandtest_views.py: unit and DRF tests (seeTests).
tests/openedx_tagging/test_api.py:test_get_taxonomylooks up2**31 - 1instead of200as its missing taxonomy id. MySQL does not reset auto-increment counters between tests, so
with this PR's extra tests a taxonomy with id 200 can exist by the time
test_get_taxonomyruns, and CI on MySQL failed on it.
Behavior
that first locks the competency's
Tagrow withselect_for_update(). That lock serializesconcurrent requests for the same competency, and it is the only thing that keeps one root
per competency, since MySQL can't express that as a constraint. The
(tag, course, parent)constraint backstops course-level groups.
rule_profile_id,rule_type_overrideand
rule_payload_override, the criterion gets the live rule profile scoped to thecompetency's taxonomy if one exists, and the seeded system-default profile otherwise.
Course- and organization-scoped profiles are not resolved here, because this endpoint
operates at taxonomy scope.
overrides, or an unknown or archived
rule_profile_idis a 400 keyed by the offending field,returned before anything is written.
object_idis rejected when ittargets the same group, or when no
group_idis supplied. A different explicit group isallowed, because ADR-0002 lets one
ObjectTagbe referenced by criteria in more than onegroup. The rules are enforced in application code under the tag lock, not by a constraint
on
CompetencyCriterion.tag_object()replaces an object's tag list for a taxonomy, so the object'sexisting tags are read first and the competency tag is unioned in. When the taxonomy refuses
the tag (
allow_multiple=False, or the object is at its tag limit), the taxonomy's messageis returned as a 400 on
object_id.title, truncated to the column length. Neither is shown in the UI. Leaf groups are unnamed.
get_or_create()lookups for the root and course-level groups matcharchived rows too, so they never insert a second row into an occupied slot. Rejecting an
archived course-level group or leaf, and reactivating an archived root, are handled by [BE] Enforce archived in Competency Criteria creation #717.
Nothing sets
archivedon groups or criteria yet.Tests
There is one test per scenario in #665's acceptance criteria. Beyond those, the tests cover:
allow_multiple=Falseand the object tag limit_create_leaf_group()andresolve_supplied_leaf_group(), including two thatcommit a root or course-level group out of band, standing in for a concurrent request, and
check that
get_or_create()reuses itthe criterion fails
Verification
pytest tests/openedx_learning/applets/cbe/ tests/openedx_tagging/ --no-cov: 794 passed (223 in the CBE suite).pylint,mypy,pycodestyle,isort --check-only,pydocstyle: clean on every changedfile.
lint-imports: all contracts kept.makemigrations --check: no changes.Related to #665
🤖 Generated with Claude Code