Repository navigation
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.
| 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.
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
|
There was a problem hiding this comment.
Thanks for the quick turnaround on all of this. Claude re-ran the CBE suite and makemigrations --check for me (223 tests, both clean; CI is green on MySQL) and sent the archived-group requests I quote in the criteria.py thread. Everything from the last round is addressed.
I have a couple of minor requests, but please feel free to squash the commits after they're taken care of, and rewrite the commit message to take into account the various adjustments that have been made.
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.
I think that's fine, though I defer to @mgwozdz-unicon on this. Please update #665's acceptance criteria (or get them updated) so that #672 builds against what's actually there.
Thank you.
Claude drafted this review and ran the test suite, makemigrations --check, and the probe requests quoted in the criteria.py thread on dd5f40e for me.
| tag/course pair can hold more than one leaf, one per criterion. The root and course-level | ||
| group names are truncated to fit the name column; they are never shown in the UI. | ||
|
|
||
| Must run inside a transaction holding the tag row lock. |
There was a problem hiding this comment.
Please add a sentence here that both get_or_create()s match archived rows deliberately and that #717 decides what happens to them.
| try: | ||
| tag = resolve_competency_tag(self.kwargs["tag_id"]) | ||
| except Tag.DoesNotExist as exc: | ||
| raise NotFound() from exc |
There was a problem hiding this comment.
Nit: NotFound(str(exc)) here and on line 103, so a client can tell "no such tag" from "that tag isn't a competency" from "no such group". All three come back as {"detail": "Not found."} today, and resolve_competency_tag() already composes the useful message.
| # Backstops create_leaf_group()'s get_or_create() for course-level groups. It has no | ||
| # `condition` because MySQL has no partial indexes: a unique index allows repeated | ||
| # NULLs, and `course` is NULL for roots and leaves. | ||
| models.UniqueConstraint( |
There was a problem hiding this comment.
Okay, please create a follow-up issue for the scope_code. I don't want to drag out this PR any longer, but I also don't want us to rely on locking the Tag to ensure data integrity in the long term. Thank you.
dd5f40e to
6034b36
Compare
Add POST /api/cbe/v1/competencies/<int:tag_id>/criteria/, associating a gradeable subsection with a competency. When group_id is supplied, the CompetencyCriterion is attached to that existing leaf CompetencyCriteriaGroup. When it is omitted, the root and course-level groups for this competency and course are reused or created, and a new leaf is always created under them. Add to applets/cbe/api.py, all marked UNSTABLE: resolve_competency_tag(), resolve_supplied_leaf_group(), create_competency_criterion(), and associate_competency_criterion() as the single entry point the view calls. Group creation is the private _create_leaf_group(), so groups are only ever created behind the duplicate-association check. The API raises Django ValidationError and model DoesNotExist exceptions; the view translates them into 400 and 404 responses, and a 404 carries the exception's message, so a missing tag, a tag outside a competency taxonomy, and a missing group can be told apart. Group creation and the duplicate-association check run inside one transaction that first locks the competency's Tag row with select_for_update(), so concurrent requests for the same competency are serialized. MySQL has no partial unique indexes, so the one-root-per-tag rule cannot be backed by a constraint and relies on that lock. Add migration 0015: a UniqueConstraint on CompetencyCriteriaGroup (tag, course, parent) that backstops course-level group creation. The root and course-level lookups match archived rows deliberately, so an archived container is never replaced by a second row in its slot. When a request supplies no rule fields, the criterion is assigned the live rule profile scoped to the competency's taxonomy if one exists, and the seeded system-default profile otherwise. It is never left with all three rule fields null, because CompetencyCriterion's profile_xor_override_check constraint forbids that. A request that supplies a profile together with an override, only one of the two overrides, or an archived or unknown profile is rejected with a 400 before anything is written. Related to openedx#665 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6034b36 to
c91b6fa
Compare
|
#665 is updated. |
Add PATCH /api/cbe/v1/criteria-groups/<group_id>/criteria/bulk-update/ and bulk_update_competency_criteria(), which apply one rule source (a rule profile, or a pair of override values) to a named batch of CompetencyCriterion rows in a leaf group, atomically, with one history row per changed criterion (#759). Deviations from #759: - The Python keyword is rule_profile_id rather than competency_rule_profile_id, matching the names #847 settled on for associate_competency_criterion() and the serializer. - The applicable profile is resolved once per batch from the group's taxonomy, through a helper shared with create_competency_criterion(), rather than per criterion via #679's helper: #679 is out of scope, and every criterion in a leaf shares the group's tag. - A batch under an archived leaf, course-level, or root group is refused with 409, not just one naming an archived criterion, because archiving a group does not archive its criteria. That check runs before the criterion lookup, so it takes precedence over a 404. Follow-ups: #898 for dropping the foreign key existence queries that CompetencyCriterion.save() issues through full_clean(); #723 for the learner-status warning that will sit in front of this endpoint. Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: javoconsultant <jmeliev_consultant@flex.unicon.net>
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