Repository navigation
feat: enforce one root criteria group per competency in the database - #900
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. |
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude Fable and I reviewed this together. The sections below say which of us found what.
Found by both of us
1. The reactivation test cannot fail. test_criteria_group.py:221 creates the root already archived, so nothing ever blocks a second root and count() == 1 holds with or without scope_code. Claude ran it with _compute_scope_code() returning None always and it still passed. Please create the root live, archive it with save(update_fields=["archived"]), reactivate it with save(), then assert the code is still set and that create(tag=tag) raises IntegrityError.
2. "Archiving is permanent" says more than was decided. The new Decision 7 bullet (0002-competency-criteria-model.rst:313) sits in a list whose archive rule also covers oel_tagging_tag, oel_tagging_taxonomy, CompetencyTaxonomy and oel_tagging_objecttag, so as written it declares ObjectTag archiving permanent too, and #779 un-archives an ObjectTag in place when its tag is re-applied. #717 only decided that archiving below the root is permanent for MVP. Please scope this bullet and the Decision 2 item 8 sentences (line 90) to groups and criteria and to the MVP. On line 90, also say why the root is the exception, something like "because archiving it for good would remove the ability to ever associate criteria with that competency again", and end with "More unarchival capabilities may be added after MVP."
3. Drop the ,course: segment. The course segment of "tag:X,course:" is always empty. The ticket suggested it to mirror the profile's format and leave room for a course-level code if archived course groups ever become replaceable, but nothing uses that room and the ADR now calls archiving below the root permanent for MVP. "tag:X" says what the column means, and now is the time: once a deployment tracking master has run 0016, changing the format is a data migration. (Why not a unique on tag_id directly: every group in a tree carries the same tag, so that would allow one group per competency in total, and "unique where parent is null" needs a partial index MySQL lacks.)
4. Overlapping tests. Several new tests assert the same behavior through slightly different setups:
- test_criteria_group.py:168 (
test_archiving_a_course_level_group_keeps_it_blocking_a_second_one): the "archived course-level group still blocks" assertion is already thearchivedcase of the parametrized test at :141, and "save() leaves a course-level group's code null" is already asserted at :109. I would delete it. - test_criteria_group.py:206 (
test_bulk_archiving_a_root_keeps_it_blocking_a_second_root): the course-level half (bulk-archive the course-level group, then expect a second one to be rejected) is the same :141 case again. Keep the root half, drop the course-level half. - test_criteria_group_deletion.py:126 (
test_deleting_a_course_run_deletes_its_course_level_groups_and_their_leaves): the existing test at :53 already covers a course run cascading to its group, and :20 already covers a child whoseparentis nulled on MySQL before the delete. The only new assertion is that the root keeps its code, which is trivial. Please add two leaves under the group in the test at :53 instead, which is what the ticket's "with leaf groups under them" scenario asks for, and drop :126. The test at :107 is different: "a new root can be created afterwards" is a real new assertion, so keep that one, but its docstring can drop the MySQL paragraph, which the docstring at :60-63 already has. - test_criteria_group.py:239 (
test_course_level_groups_and_leaves_are_not_limited): "not limited" is not something a count of six can prove, and the name says nothing about what is asserted. What the test actually guards is that the new constraints do not touch non-root groups, and :109 already builds a root, a course-level group and a leaf for the same purpose. Please fold the second course and the extra leaves into :109 and delete :239.
Found by Mary
5. ADR line 86. "Only the subtree's topmost group, the course-level group, carries it" is correct but takes a moment to parse, because the topmost group of the subtree is the second level of the tree. "Only the course-level group carries it" is enough.
6. ADR lines 113 and 114. "The root group and any groups under it outside a course-scoped subtree" describes a shape nothing creates: every group under the root is a course-level group or sits under one. "A read always includes the root group, plus any selected subset of course-scoped groups" says it without the edge case, and the size bullet on line 114 can say "For the root group" rather than "For the root scope".
7. A group's parent should be immutable, and three tests move it. test_criteria_group.py:182, :194 and :230 all reparent a group (two move a root under another tree's root, one promotes a child to root). That should not be possible at all, but nothing says so: the ADR marks only course_id as immutable after creation, and neither the model nor the API blocks a parent change (nothing enforces the course_id rule either). I had not written this down before. Please add "Immutable after creation" to parent_id in Decision 2 the way course_id has it. For enforcement, either add a save() check here that rejects a changed parent_id or course_id on an existing row, with one test, or file a follow-up ticket for it and link it from the PR description, whichever you prefer. For the tests: :230 should stay, because it is the only test that fails when the check constraint is removed, but please rename it to say what it proves (a root cannot be given a parent behind save()'s back). :182 and :194 test the update_fields branch of save() through a reparent that the design forbids. If you keep that branch as a safety net, keep one of the two for it and have :194 assert the code was null before the promotion. Otherwise drop both.
Found by Claude
8. Nothing ties the migration's backfill format to the model's. test_migrations.py:36 compares the backfilled value to a hand-written literal, while the migration builds the code with Concat/Cast and the model with an f-string. When Claude changed the model's format, seven model tests failed and no migration test did. After apply_tested_migration(MIGRATE_TO), asserting with the real CompetencyCriteriaGroup that create(tag_id=tag_a.pk) raises IntegrityError would catch drift in either direction. This matters more with the format change in item 3.
9. The duplicate-roots migration test's cleanup runs only on success. test_migrations.py:113, archived_root.delete(), is skipped if any assertion before it fails. The migrator teardown then migrates to the latest state and raises DuplicateRootCriteriaGroupsError a second time as an ERROR (Claude inverted the column assertion to check). A try/finally around the assertions fixes it.
10. The reverse-migration test asserts less than its docstring says. test_migrations.py:74-85 says the reverse "drops scope_code and keeps existing groups", but the only assertion is that the row still exists. The column-introspection block from the test below can be reused to assert the column is gone.
11. Suggestions.
- criteria.py:41-53: the class docstring says nothing about
scope_code. Writes that skipsave()leave a root with a null code (bulk_create(),bulk_create_with_history(), which #694 plans to use,QuerySet.update(parent=None),loaddata, raw SQL), andQuerySet.update(tag=...)leaves a stale one. Claude confirmed both: twobulk_createroots for one tag both insert, and afterupdate(tag=other)the old tag is blocked while the new tag can get a second root. Once two roots exist,get_or_create(tag=tag, parent=None)at api.py:150 raisesMultipleObjectsReturnedfor every request on that competency. No supported path does any of this today and #872 accepted thebulk_creategap, but one docstring sentence naming the writes that bypass the rule would put the contract where the #694 implementer will see it. I would not add abulk_createoverride. - criteria.py:148-149: "MySQL's delete collector nulls a root's cascading
parentkeys" names the wrong component and the wrong rows. It is Django's collector, and it nulls each descendant'sparent(and a deleted course run's groups'course), not anything on the root. The docstring at test_criteria_group_deletion.py:60-63 has it right. Something like: "On MySQL, which cannot defer constraint checks, Django's delete collector sets each descendant's nullableparentto NULL before deleting it, so 'every root has a scope_code' would reject those rows mid-delete." - api.py:145:
_create_leaf_group()'s docstring still says "Must run inside a transaction holding the tag row lock." With both uniqueness rules in the database,get_or_create()recovers from a concurrent insert on its own, so only_reject_duplicate_criterion()still needs the lock, which is what your new comment at 348-349 says. #872 said to leave the function alone, so your call, but the sentence reads as stale now. - api.py:348-349: the "since a request without group_id always creates a fresh leaf" reason covers only one path. With
group_idsupplied,_reject_duplicate_criterion()is also a check-then-act that only the lock serializes, becauseCompetencyCriterionhas no unique constraint on(object_tag, group). Saying the lock serializes the duplicate check on both paths would stop someone removing it for the explicit-group case.
12. Nit. PR description: the first Description sentence is missing its verb ("the select_for_update() lock that associate_competency_criterion() takes"), and openedx-platform master pins openedx-core 1.15.1 now, not 1.12.0.
mgwozdz-unicon
left a comment
mgwozdz-unicon
left a comment
There was a problem hiding this comment.
Claude Fable and I went through 8ab8e59 against the first round. Every item is addressed as asked, and we re-verified each one against the code. Thanks for the thorough pass.
One thing to fix before I approve, then a few small things.
1. The immutability check should cover tag too. _check_tree_position_immutable() compares parent_id and course_id, but scope_code is built from tag_id. Claude confirmed by probe that root.tag = other_tag; root.save() succeeds and rewrites the code to the new tag, after which a second root for the old tag is created without error while the old root's descendants still carry the old tag. With save(update_fields=["tag"]) the new tag is stored and the old code stays, since the branch that forced scope_code into update_fields is gone. So the save() docstring's "parent never changes, so the code is fixed at creation" is not true as written. #760 already says a changed oel_tagging_tag_id is rejected, so the intent is settled. Please add tag_id to the same values_list and comparison (no extra query), a "tag" case to test_a_saved_group_cannot_change_its_parent_or_course, widen the message, and add "Immutable after creation." to item 3 of ADR-0002 Decision 2.
2. The course-run deletion test builds the shape the ADR forbids. test_deleting_a_course_run_also_deletes_its_course_scoped_criteria_groups creates CompetencyCriteriaGroup(tag=tag, course=course_run) with no parent, so its "course-level group" is a root carrying a course, and the fold I asked for hung two leaves under it. That line predates this PR, so this is partly on my earlier request. Could you create the root first, put the course-level group and the two leaves under it, and keep one assertion that the root survives the course run's deletion? That restores the one assertion the deleted test had that nothing else covers.
3. _create_leaf_group()'s "needs no lock" sentence. get_or_create() does retry the get after an IntegrityError, but on MySQL under REPEATABLE READ a transaction that already took a consistent snapshot may not see the other transaction's row, so the retry misses and the IntegrityError re-raises. Today the Tag row lock in associate_competency_criterion() serializes callers so this cannot happen. Consider "the caller's tag lock serializes these lookups and the unique constraints are the backstop", or keep it and accept the edge case for a future lock-free caller. We did not run this on MySQL.
4. ADR small things. The 2026-10-22 changelog entry could mention parent immutability and the MVP scoping of permanent archiving. Line 114 now reads "For the root group, expected size is small (realistically <=500 nodes)", and after the rewording a root group is one row, so the estimate no longer bounds anything; drop it or say what it bounds.
5. Migration test, your call. The backfill test now inserts through the live model at migration state 0016, which is what I asked for. Claude points out that any later migration adding a column to this table would break that INSERT for an unrelated reason. A no-write alternative keeps the tie: compare the backfilled value with CompetencyCriteriaGroup(tag_id=tag_a.pk)._compute_scope_code(). A comment on the current version is also fine.
6. PR description. "After this PR, every rule about the stored criteria tree is enforced by the database" overstates it: parent and course immutability is an application check in save(), and nothing enforces "a root has course_id null". I am not asking for enforcement of the latter. A check constraint would fail root deletion on MySQL for the same reason your scope_code check is one-way (the collector nulls parent on the course-level groups while their course is still set), and no API path creates that shape. Please just soften the sentence. Also: the first sentence is still missing its verb, openedx-platform master pins 1.15.1 (not 1.12.0), Changes still describes the removed update_fields branch and the "tag:<tag_id>,course:" format, Tests still lists the two removed update_fields tests, the count is 278 now, and _check_tree_position_immutable() is not mentioned.
|
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
@mgwozdz-unicon asked on #872 whether I had concerns about keeping scope_code on an archived root. I don't. Each learner has one StudentCompetencyStatus row per competency, derived from one root, and I'd rather the database hold exactly one root row than have every rollup and read filter for the live one. Thank you for writing that down in the ADR.
I have an open question on always creating a scope_code. What's left is the rebase, which isn't quite mechanical: #875 merged this morning and took migration number 0016 and rewrote the lock comment this PR also rewrites, and two of #681's tests reparent a group through save(), which this PR now rejects. Details inline.
Thank you.
Claude drafted the first pass of this review and did a trial merge of this branch onto main (9247ee4) in a scratch worktree: the conflicts were the migration number and the api.py comment, and after renumbering the migration and reworking the two #681 tests as described inline, pytest tests/openedx_learning passed 403 tests on SQLite and makemigrations --check reported no changes.
| dependencies = [ | ||
| ('oel_tagging', '0022_tag_external_id_not_null'), | ||
| ('openedx_catalog', '0002_pathways'), | ||
| ('openedx_learning', '0015_criteria_group_unique_constraints'), |
There was a problem hiding this comment.
This needs to become 0017_criteria_group_scope_code, with this dependency pointed at 0016_add_competency_course_run, since #875 merged this morning and took 0016. Please renumber rather than makemigrations --merge: the two migrations don't touch the same table, so a merge migration would only add noise. MIGRATE_FROM/MIGRATE_TO, the import_module() path, and the "at state 0016" comment in tests/openedx_learning/test_migrations.py need the same change.
| # The database enforces one root per tag; this lock only serializes _reject_duplicate_criterion(). That check | ||
| # is check-then-act on both paths: without group_id a request always creates a fresh leaf, and with group_id | ||
| # CompetencyCriterion has no unique constraint on (object_tag, group), so no index catches a concurrent copy. |
There was a problem hiding this comment.
This conflicts with the comment #875 put here. When you resolve it, please keep that PR's first sentence ("Lock order is always the tag, then the course run, so two creates can't deadlock.") above yours, since it's the rule that keeps the CompetencyCourseRun lock on the next lines from deadlocking against a concurrent create, and your rewrite still holds underneath it. Something like:
# Lock order is always the tag, then the course run, so two creates can't deadlock.
# The database enforces one root per tag; the tag lock only serializes _reject_duplicate_criterion(). That
# check is check-then-act on both paths: without group_id a request always creates a fresh leaf, and with
# group_id CompetencyCriterion has no unique constraint on (object_tag, group), so no index catches a
# concurrent copy.| def _check_tree_position_immutable(self) -> None: | ||
| """Raise ValidationError if tag, parent or course no longer matches what is persisted for this row.""" | ||
| if self.pk is None: | ||
| return | ||
| # Queried rather than compared against a value cached at load time, so a deferred load or | ||
| # a refresh_from_db() cannot bypass the check. | ||
| persisted = ( | ||
| CompetencyCriteriaGroup.objects.filter(pk=self.pk).values_list("tag_id", "parent_id", "course_id").first() | ||
| ) | ||
| if persisted is not None and (self.tag_id, self.parent_id, self.course_id) != persisted: | ||
| raise ValidationError( | ||
| _("A CompetencyCriteriaGroup's tag, parent and course cannot be changed after creation.") | ||
| ) |
There was a problem hiding this comment.
Two of #681's tests reparent a group through save() and fail against this check once the branch is on main: test_get_competency_criteria_tree_keeps_a_group_moved_under_a_later_created_parent in test_api.py and test_tree_response_never_references_a_group_it_left_out in test_views.py. Please rework them as part of the rebase. The first one guards the tree walk against a parent_id that points at a larger id, which is now only reachable behind save(), so building that shape with .filter(pk=group_b.pk).update(parent=group_c) keeps the test honest about what it defends against (the check constraint only forbids giving a root a parent). The second one just needs its kept leaf under a second course run's course-level group instead of being moved under the root. Claude made both changes on a trial merge and the suite passed, so neither needs more than that.
Likewise, the comment above the walk in get_competency_criteria_tree() ("ordering by parent_id does not guarantee a parent comes before its child once groups can be moved") now contradicts ADR-0002 and this method. The walk is still worth keeping, since nothing stops a QuerySet.update() from producing that shape, but please reword it so the next reader doesn't take it as a plan to make groups movable.
FWIW, I'm fine with parent being immutable: #760 already rejects a parent change at the API, relaxing this later is a one-line change, and tightening it after rows have moved around would not be.
| The caller's tag lock serializes the root and course-level lookups; the unique constraints on | ||
| one root per tag and one course-level group per tag and course are the backstop. |
There was a problem hiding this comment.
The version of this sentence from 8ab8e59 was the right one, and I'd like it back: the root and course-level lookups don't need the lock. We run MySQL under READ COMMITTED (Django's default for it), so the get() that get_or_create() retries after an IntegrityError sees the other transaction's committed row; the miss described in the last round only happens under REPEATABLE READ, which we don't use. As written, this reads as a dependency on the tag lock, and the point of this PR is that group integrity no longer has one.
| The caller's tag lock serializes the root and course-level lookups; the unique constraints on | |
| one root per tag and one course-level group per tag and course are the backstop. | |
| The root and course-level lookups need no lock of their own: the unique constraints hold one | |
| root per tag and one course-level group per tag and course, and ``get_or_create()`` recovers | |
| from a concurrent insert. The caller's tag lock is for ``_reject_duplicate_criterion()``. |
| condition=Q(scope_code__isnull=True) | Q(parent__isnull=True), | ||
| name="oel_cbe_criteria_group_scope_code_root_only_check", | ||
| violation_error_message=_("Only a root CompetencyCriteriaGroup may have a scope_code."), | ||
| ), |
There was a problem hiding this comment.
Question: Is this more trouble than it's worth? I think I get the reasoning for this, in that the scope_code is only strictly necessary when one of these values is null, and that we're essentially making a sort of composite constraint where the UniqueConstraint on scope_code is only there to cover what the other UniqueConstraint on (tag, course, parent) can't. Still, it feels fragile, and I'd feel more comfortable if scope_code always existed (i.e. was not nullable).
I realize this uses more space, and that it may complicate the migration a bit (create nullable field, backfill, make field no longer nullable). But it would reduce the code complexity and guard against things like bulk insertion later bypassing our constraints and causing data corruption.
I can be argued out of this one. Please let me know if there's something I'm not seeing here.
80c7956 to
b4eb963
Compare
|
@ormsbee In my opinion it also doesn't simplify much. The check constraint can no longer be a null test and would have to become a prefix test on the string, and the migration gains a full backfill plus an AlterField. On bypasses: non-null does make a It also keeps the same reading as |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
All four of yesterday's items are fixed, and this now merges cleanly onto main with #760 in it.
On making scope_code non-nullable:
The check that would close both, "a root must have a code", doesn't work here. When a root is deleted, Django first sets every child's parent to
NULLand only then deletes them, so during that step the children look like roots without a code and the check would fail.
Okay, that's the piece I was missing. The point of always having a code was that a root could never be without one, and the cascade step rules that check out on MySQL under either design. What's left is the bulk_create() case, and paying for that with a second row key on every leaf isn't worth it. Keep it nullable. (FWIW, Claude also pointed out that non-null wouldn't even make that case fail cleanly: a CharField with no default gets '' when null=False, so the first omitted code would insert and only the second would trip the unique constraint.)
One more thing before squashing: the PR description still calls the migration 0016, and its Changes section says the _create_leaf_group() docstring claims the tag lock serializes its lookups, which is now the opposite of what it says. Please update it, since that's what the squash commit will carry. Then please squash and I'll merge. Thank you.
Claude drafted the first pass of this review and did a trial merge of this branch onto main (d201c97) in a scratch worktree: it merged cleanly, pytest tests/openedx_learning passed 472 tests on SQLite, and makemigrations --check reported no changes.
| # Recomputed in save(); set for roots only, archived or not, so an archived root keeps blocking a second one. | ||
| scope_code = models.CharField( | ||
| max_length=255, | ||
| null=True, |
There was a problem hiding this comment.
Nit (optional): Please consider adding line here on why this stays null below the root (a leaf has no natural key to put here, and the one-way check below means non-null wouldn't buy a stronger constraint anyway) would save the next person from asking my question again.
Add CompetencyCriteriaGroup.scope_code, set in save() to "tag:<tag_id>" for a root and null for every other group, with a unique constraint on it. Until now one root per competency relied on the Tag row lock that associate_competency_criterion() takes: MySQL has no partial unique indexes, and the (tag, course, parent) constraint treats a root's null course and parent as distinct, so two roots for one competency would both save. The lock stays, but it now only serializes the duplicate-criterion check. A group's tag, parent and course are immutable after creation; save() rejects a change to any of them with a ValidationError, so a root's code never has to change. An archived root keeps its code and is reactivated rather than replaced: each learner has one StudentCompetencyStatus row per competency, derived from one root, so a live root beside archived ones would force every rollup and read to pick one. A check constraint allows a code only on a root. It runs one way, "a coded row is a root", because MySQL cannot defer constraint checks and Django's delete collector nulls parent and course before cascading. Below the root the column stays null: a leaf has no natural key to put there, and the one-way check means non-null would not make the rule stronger. Migration 0017 raises DuplicateRootCriteriaGroupsError on existing duplicate roots before any schema change, since MySQL commits each schema change immediately, then adds the column, backfills roots, and adds both constraints. ADR-0002 is amended: Decision 2 gains scope_code and the immutable columns, Decision 3 contrasts the two scope_code columns, Decision 7 states the MVP archive rule, and Rejected Alternative 7 records the replace-the-root design. Related to openedx#872 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
b4eb963 to
ec32430
Compare
|
@ormsbee All done. |
Regenerated via `make upgrade-package package=openedx-core`. Picks up openedx/openedx-core#900 (one root criteria group per competency, enforced in the database by migration openedx_learning 0017, openedx/openedx-core#872), first released in 1.19.0, and openedx/openedx-core#911 (`course_keys` scoping on the criteria-groups read endpoint, openedx/openedx-core#682), released in 1.20.0, so the Competency-Based Education (CBE) work can go to QA. Related to openedx/openedx-core#872 and openedx/openedx-core#682 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Description
Makes "one root
CompetencyCriteriaGroupper competency" a database rule. Until now it reliedon the
select_for_update()lock thatassociate_competency_criterion()takes. MySQL has nopartial unique indexes, and the
(tag, course, parent)constraint from #847 treats a root's nullcourseandparentas distinct, so two roots for one competency would both save. This is thefollow-up for the #847 review
(#847 (comment)).
CompetencyCriteriaGroupgets a derivedscope_codecolumn, following the patternCompetencyRuleProfileuses (ADR-0002 Decision 3). It is set on root groups only, and anunconditional unique constraint on it rejects a second root. A group's
tag,parentandcourseare immutable after creation, so the code is fixed when the row is created. After this PR, one root
per competency and one course-level group per competency and course are both database rules;
immutability stays an application check in
save().Changes
src/openedx_learning/applets/cbe/models/criteria.py: addsscope_code(
CharField(max_length=255, null=True, editable=False)),_compute_scope_code(),_check_tree_position_immutable()andsave().save()rejects a changedtag,parentorcourseon an existing row with aValidationError, then sets the code to"tag:<tag_id>"for a root and null for every other group. Adds a unique constraint on
scope_code, and acheck constraint that only a root may have a code.
historyexcludes the column. The classdocstring names the writes that bypass
save()and so bypass the rule.src/openedx_learning/migrations/0017_criteria_group_scope_code.py(new): checks forduplicate roots, adds the column, backfills existing roots, and adds both constraints (see
Migration).
src/openedx_learning/applets/cbe/api.py: comments only. The Tag row lock comment says whatthe lock still protects (see Behavior), and
_create_leaf_group()'s docstring says its twoget_or_create()lookups need no lock of their own: the unique constraints hold one root pertag and one course-level group per tag and course, and
get_or_create()recovers from aconcurrent insert.
docs/openedx_learning/decisions/0002-competency-criteria-model.rst: ADR-0002 amendment (seeADR).
tests/openedx_learning/applets/cbe/test_criteria_group.py,test_criteria_group_deletion.pyandtests/openedx_learning/test_migrations.py: see Tests.Behavior
scope_codedoes not depend onarchived, so anarchived root keeps blocking a second root, and [BE] Enforce archived in Competency Criteria creation #717 reactivates it in place instead of
creating a new one. This differs from
CompetencyRuleProfile.scope_code, which is nulledwhile archived so that a replacement profile can be created. The reason is how the two tables
are read. A criterion points straight at its profile, so an archived profile beside a live
one costs nothing. A learner has exactly one
StudentCompetencyStatusrow per competency,derived from the competency's root, so a live root beside archived ones would force every
rollup and every read to decide which root counts first. Keeping exactly one root row per
competency, archived or not, keeps that question from existing.
course_idimmutable aftercreation; this PR adds
oel_tagging_tag_idandparent_idand enforces all three insave(),the way
CompetencyRuleProfileenforces its scope columns. A group is created in its place inits competency's tree and stays there, so a root's code never has to change after creation.
Writes that skip
save()are not checked, the same as for the profile.(tag, course, parent)constraint alreadyallows only one per competency and course, live or archived, which matches [BE] Enforce archived in Competency Criteria creation #717 rejecting an
archived course-level group instead of replacing it.
root has a code". On MySQL, which cannot defer constraint checks, Django's delete collector
sets the nullable
parentandcourseof rows it is about to cascade-delete to null first,so the stricter rule would reject deleting a root or a course run. I confirmed this by
temporarily applying the stricter rule: both deletion tests failed on MySQL with error 3819.
_reject_duplicate_criterion(), which is acheck-then-act on both paths: a request without
group_idalways creates a fresh leaf, andwith
group_idtheCompetencyCriteriontable has no unique constraint on(object_tag, group). The same content may legitimately sit in two explicitly named groups,so no unique index can express "this content is already associated with this competency".
Removing the lock would need either a product change (one criterion per content and
competency) or extra constraints that still leave a rare race, so it is out of scope here.
save()or a bulk.update(archived=True)leaves the code in place, so [BE] Delete Competency Criterion #674, [BE] Build endpoint for removing a Competency Criteria Group #675 and [BE] Enforce archived in Competency Criteria creation #717 work as written.Migration
0017runs a read-only check first. If any competency has more than one root, live orarchived, it raises
DuplicateRootCriteriaGroupsError, naming each tag and its root group ids,before changing the schema. The check comes first because MySQL commits each schema change
immediately: a failure after the column was added would leave it behind with the migration
unrecorded, and a plain rerun would then fail. The migration then adds the column, backfills
it for existing roots with one
UPDATE, and adds both constraints. It reverses cleanly.CompetencyCriteriaGrouphas shipped since v1.5.0, and openedx-platformmasterpins v1.15.1with
openedx_learninginstalled, so deployments trackingmastermay already have rows. Nonamed release (Ulmo, Verawood) installs the app yet.
ADR
ADR-0002 is amended, a sentence or two per point and with no implementation detail:
scope_code, marksoel_tagging_tag_idandparent_idimmutable aftercreation alongside
course_id, and states that for MVP archiving a group or criterion is permanent, with thearchived root as the one reversible exception, because archiving it for good would remove
the ability to ever associate criteria with that competency again. Archiving a course-level
group blocks that course for that competency, and archiving a leaf closes only that leaf.
course_idand retrieval-scope descriptions are corrected to match the code:only the course-level group carries
course_id, and a read always includes the root groupplus the selected course-scoped subtrees.
scope_codecolumns, Decision 7 states the MVP archive rule forgroups and criteria and points
oel_tagging_objecttagat ADR-0003, and Rejected Alternative7 records the replace-the-root design. Decision 5's index list gains the
scope_codeuniqueconstraint and the
(tag, course, parent)one from Alezconsultant/665 criterion endpoint on 846 #847. The size estimate now bounds eachcourse-scoped subtree, and the changelog entry lists the immutability and MVP archiving changes.
Tests
There is one test per scenario in #872's acceptance criteria. Also, the tests cover:
.update()tag,parentorcoursethroughsave()being rejected, and therow staying where it was
.update(parent=...)on a root being rejected by the check constraintunder a root that survives the deletion (these exercise MySQL's delete collector)
when an assertion fails), its backfill over live and archived roots, course-level groups and
leaves, that the live model's
save()produces the same code as the backfill (a second rootcreated through the live model after migrating is rejected; a comment notes this INSERT will
need replacing once a later migration adds a column to the table), and its reverse (the
column is gone)
test_group_logic_operator_accepts_and_or_and_null_regardless_of_child_countused to create tworoots for one tag; its second tree now uses a second tag, and its assertions are unchanged.
Manual Testing
Manual testing via the Django shell in the devstack, starting with migration 0017 unapplied
(
./manage.py cms migrate openedx_learning 0016_add_competency_course_run), thendocker exec -it -e PYTHONPATH=/mnt/openedx-core/src tutor_dev-cms-1 ./manage.py cms shell:Verification
pytest tests/openedx_learning --no-cov: 403 passed on SQLite. The three changed testmodules (30 tests) also pass on MySQL 8.
pylint,pycodestyle,isort --check-only,mypy: clean on every changed file, apartfrom the unused-argument and class-docstring warnings every data migration here carries.
lint-imports: all contracts kept.makemigrations --check: no changes.doc8and the Sphinx HTML build (-W): no errors or warnings.Related to #872
🤖 Generated with Claude Code