Repository navigation
feat: add endpoint to delete or archive a CompetencyCriterion (#674) - #879
ufedaseyeuconsultant wants to merge 12 commits into
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ufedaseyeuconsultant! 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. |
4e083c4 to
12bdeda
Compare
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
See inline comments.
| assert ObjectTag.objects.filter(id=object_tag.id).exists() | ||
|
|
||
|
|
||
| def test_status_on_the_criterion_missed_by_the_first_check_archives_it( |
There was a problem hiding this comment.
what real situation does this test stand for?
| assert criterion.archived is True | ||
| assert ObjectTag.objects.filter(id=object_tag.id).exists() | ||
|
|
||
|
|
There was a problem hiding this comment.
Thanks for covering the fallback path with tests. I'm having trouble working out which real concurrency situations these tests stand for. Could you add a short explanation, either in the PR description or in the docstrings of delete_competency_criterion and these tests? It would help to cover:
Which races can actually happen here. For example: a grade write adds a status row on the criterion, or on an ancestor group, between the status check and the delete. For each race, which concurrent operation causes it?
Which of those races the criterion row lock at api.py:520 already prevents. If the lock blocks a concurrent status insert on the criterion, can test_status_on_the_criterion_missed_by_the_first_check_archives_it still happen in practice, or is that path only defensive?
How each test maps to one of those races. The tests force a stale check with side_effect=[False, True], so it's hard to tell from the tests alone which real interleaving each one stands for.
A sentence or two per race would be plenty. Thanks!
There was a problem hiding this comment.
The docstring of delete_or_archive_competency_criterion now lists them. There are two: a concurrent create for the same competency, which now waits on the Tag row lock, and a learner-status insert on the criterion or on one of its ancestor groups, which waits on that row's lock. Both are prevented by the locks, so the side-effect tests that simulated a stale check are removed.
| user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, | ||
| ) -> None: | ||
| """ | ||
| The leaf group has a status row, but the cascade's first check misses it, so the group delete is |
There was a problem hiding this comment.
Can you explain what "the cascade's first check" is?
There was a problem hiding this comment.
It was the ancestor-group status check inside _cascade_from_group on the first of two attempts. The retry is gone, so there is only one check, made while the ancestor groups are locked. The test is removed.
| An IntegrityError from the object_tag cleanup, with no status anywhere, is retried once and then | ||
| propagates. The criterion is not archived and its group is unchanged. |
There was a problem hiding this comment.
Can you explain why? what is the reasoning for the retry and propagate?
There was a problem hiding this comment.
The retry existed to recover from an ancestor gaining a status between the cascade's check and its delete. Locking the ancestor groups closes that window, so the retry is removed, and any integrity error now propagates on the first attempt.
| ) | ||
| if not request.user.has_perm("oel_tagging.can_tag_object", perm_obj): | ||
| raise PermissionDenied() | ||
| result = delete_competency_criterion(criterion.id) |
There was a problem hiding this comment.
Maybe name the method delete_or_archive_competency_criterion?
There was a problem hiding this comment.
renamed
| # status on the criterion itself justifies archiving it. | ||
| with transaction.atomic(): | ||
| criterion = CompetencyCriterion.objects.select_for_update().get(pk=criterion_id) | ||
| if _criterion_has_status(criterion_id): |
There was a problem hiding this comment.
Could we find a better term than status? it's hard to tell what that refers to. Maybe if _criterion_has_learner_progress?
There was a problem hiding this comment.
renamed
| group.criteria.filter(archived=False).exists() or group.child_groups.filter(archived=False).exists() | ||
| ) | ||
| parent = group.parent | ||
| if has_live_child: |
|
Here's a pretty in-depth AI review. The points it makes sound right to me. Review of PR #879: delete or archive a CompetencyCriterionReview DetailsPrerequisites, deferred items, what passed, and the action listPrerequisites (skip if you know the codebase)The models. A What the change adds. Names the tickets coined. "Cascade" or "the walk" is the ancestor loop just described. An "archive event" versus a "delete event" is the mode the walk runs in: after an archive it only archives; after a delete it deletes a group that is fully empty and archives one whose only remaining children are archived. "Delete protection" is ADR-0002 Decision 7: a row referenced by learner status is archived instead of deleted. The ADRs cited (in Database terms (unsourced, general knowledge). The tickets around this one. #674 is this ticket. #665 built the create endpoint and the How the endpoint reaches Studio. edx-platform's How the review tested the tests. For each acceptance criterion the review broke the code path on purpose (for example, forced the status check to return False, or made the cascade return immediately), ran the two new test files, and checked that at least one test failed for that reason. A break that left all 68 tests green means the behavior is untested. Things the review looked at and is leaving to other tickets
What passedAt the head commit, the two touched test files pass (68 tests), Action list (in priority order)
Must fix before mergeThe branch no longer merges cleanly with DetailsThe branch conflicts with Once this change archives a group, nothing in the system can bring it back, and other features get stuck on it. DetailsNothing can ever un-archive a group, and the create path and bulk update both get stuck on archived groups. This PR is the first code that archives groups. Once the walk archives a course-level group (or the root group that every course shares for that tag), three existing pieces of code turn that into a dead end. The create path's The permission check happens only in the web endpoint and not in the delete function, which departs from the ticket and from how newer code on DetailsThe permission check lives only in the view, and the function takes no user. Ticket #674 asked for The delete does not take the same lock the create takes, so on some databases a create happening at the same moment can be silently deleted. DetailsThe delete path skips the Tag-row lock the create path relies on, so a concurrent create can be silently deleted. Untagging works out which tags to keep from a query that hides disabled-taxonomy tags but deletes through one that does not, so with the competency taxonomy disabled it can delete other competencies' tags on the same subsection, or fail with a protected-row error; only callers that bypass the web endpoint reach this. DetailsUntagging reads through a filtered query but writes through an unfiltered one. The delete branch builds the list of tag values to keep from Sending the same delete request twice at nearly the same time can produce a server error instead of a not-found response. DetailsA concurrent repeat DELETE returns 500 instead of 404. The view resolves the criterion with Two tests meant to check where the ancestor cleanup stops do not fail when that rule breaks. DetailsThe cascade's stop rule is tested only as "the row still exists". The two tests for "the walk stops at an ancestor with a surviving sibling" ( One behavior the ticket describes has no test that fails when it breaks. DetailsNo test covers a hard delete whose group keeps only archived siblings. The ticket says that when a hard delete leaves a group with nothing but archived children, the group is archived and the walk continues upward as an archive. The code does this correctly (the review probed it), but when the review removed that branch, all 68 tests still passed. Add a service test with one archived criterion (with a status) and one live criterion in the same group, delete the live one, and assert the group and its parent end up archived. The permission-denied test would still pass if the check ran too late, and one half of the permission rule has no test. DetailsThe 403 test proves only the status code, and half of the permission is untested. The documented reason for the endpoint's URL shape is wrong, and the ticket is never told the URL differs from what it specified. DetailsThe view docstring gives a false reason for the URL shape and the PR hides the change from the ticket. Ticket #674 specified The pull request description is out of date and uses a wrong class name. DetailsThe PR description is stale. It says #613 "has not landed on main yet" and that the branch cherry-picks its migrations plus a renumbering commit. The PR has three commits, no migrations, and #613 landed on One docstring claims a rule that only the create endpoint enforces, not the data model. Details
Decisions to make or write downThe implementation checks something an architecture decision says is not checked for this purpose, and which rule wins is unrecorded. DetailsChecking group status directly departs from ADR-0002 Decision 7. The ADR calls After an archive the competency tag stays visible on the subsection, which differs from what an architecture decision describes. DetailsLeaving the A hard delete that lands at the wrong moment can make a learner's grade save fail. DetailsA hard delete can now fail a learner's grade write. Answering not-found before checking permission lets an unauthorized caller learn which criterion ids exist. Details404 before 403 lets an unauthorized caller enumerate criterion ids under a tag. The view looks the row up before checking permission, so a caller without rights learns which One test covers a situation that no supported input can produce. DetailsOne test pins a path no supported input reaches. Improvements worth doingThe cleanup loop rewrites rows that did not change and saves more columns than it needs to. DetailsThe walk re-saves levels that do not change, and uses full saves. A repeat DELETE on an archived criterion re-saves the criterion and every ancestor, writing one history row per level with nothing changed in it. The delete leans on a retry where locking the rows up front would be more reliable and would avoid a possible deadlock. DetailsLock the rows the checks read instead of retrying. The retry cannot loop or archive twice, and both archive call sites are inside a transaction. But locking the Tag row, then the ancestor groups, then the criterion keeps the status checks from going stale, which is the only reason the fallback block exists, and it matches #874's groups-before-criteria lock order. The current order (criterion first, then group writes) can deadlock against a concurrent bulk update, which would surface as an uncaught A result type returned by a public function is not exported. DetailsExport One scenario is covered only by a mocked test and not by a real one. DetailsAdd the depth-2 variant of the "group with its own status" case. The real test puts the status on the leaf group; the ancestor case is covered only by a mocked test. One extra parametrize row with the status on the root is enough; the review probed it and it works. Two tests simulate a race by scripting call counts, so they would break for the wrong reason. DetailsSimulate the race instead of scripting helper call counts. Two tests patch a private helper with Small thingsA commit subject line is longer than the repository guidance asks for (73 characters against "under 72"), though CI's commitlint passed. DetailsCommit The same retry explanation is written out three times in the code. DetailsThe retry rule is explained three times: in the A helper's name and the main docstring's emphasis do not describe the code as well as they could. Details
A lock comment suggests more protection than the lock gives. DetailsThe lock comment in Several tests have redundant assertions, overlapping cases, or loosely worded names and docstrings. DetailsTests: two The URL name differs slightly from the one the ticket gives. DetailsThe URL name is |
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
Great work!
I mostly found some nits. AI found a bunch of small problems that should be fixed, then this should be ready.
github-project-automation
Bot
moved this to Needs additional details
in Competency Criteria and Student Progress
jesperhodge
moved this from Needs additional details
to Unicon Code Review
in Competency Criteria and Student Progress
47b564a to
747c68a
Compare
|
@jesperhodge Thanks, this was a thorough review, and the main points were right. I rebased onto
Two things I kept as they are, with the reason:
Lock behavior cannot be exercised by the test suite, because the tests run on SQLite, where |
|
Hi @ufedaseyeuconsultant , According to Claude, these locks are unnecessary and can be removed:
And you'd need to clean up the code around that to account for removing these a bit. The one lock we might be leaving is the tag lock with the select_for_update() double checking with him that it is fine.
|
|
@ufedaseyeuconsultant Claude found one other issue: Quoting Claude
|
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
Hi Seva, thanks for the good fixes.
I found two more things - see the two comments I left in the PR next to this.
Most importantly we should make sure we don't use select_for_update() where it's not strictly necessary.
30a70f7 to
ceb5d96
Compare
Reply to the
|
ceb5d96 to
3e0580d
Compare
…x#674) DELETE competencies/<tag_id>/criteria/<criterion_id>/ hard-deletes a CompetencyCriterion when no StudentCompetencyCriterionStatus references it, or archives it in place otherwise. On the hard-delete path, the now-unreferenced ObjectTag is untagged via the same read-merge-write tag_object() creation already uses, but only once no other CompetencyCriterion still shares it. The archive path never touches the ObjectTag. An ancestor CompetencyCriteriaGroup left with no live children is cascaded in turn (hard-deleted or archived, per the same learner-status rule, stopping at the first ancestor that still has live content), so a removal never leaves an empty group behind per ADR-0002 Decision 2. The view checks oel_tagging.can_tag_object the same way the create view does, and delete_competency_criterion() leaves that check to its caller, like associate_competency_criterion(). The create endpoint's response now includes the criterion's archived flag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
delete_competency_criterion() checked for a StudentCompetencyCriterionStatus row before opening its transaction. The status foreign keys are on_delete=PROTECT, so a status row committed between that check and criterion.delete() made the delete raise ProtectedError (or IntegrityError from the database constraint), and the endpoint returned a 500 instead of archiving. The criterion row is now locked with select_for_update() and the status check, the hard delete, the ObjectTag cleanup, and the ancestor cascade all run in one transaction. On MySQL and PostgreSQL, inserting a status row that references the locked criterion waits for this transaction, so the check stays valid until the delete. The archive path runs in the same transaction too. If the hard delete is still refused, for example because a group-level status appears for an ancestor that _cascade_from_group() deletes, the ProtectedError or IntegrityError is caught after the transaction has rolled back, and the criterion is archived in a new transaction. The result then reports archived=True. The tests cover the fallback by making the delete raise ProtectedError (a real status row created just before the real delete) and IntegrityError. SQLite cannot reproduce row locks, so the lock itself is not unit-tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…penedx#674) The fallback after a refused hard delete archived the criterion on any ProtectedError or IntegrityError. The attempt also covers the ancestor cascade and the ObjectTag cleanup, so an ancestor that gained a status mid-cascade, or an unrelated integrity failure, archived a criterion that has no status and reported archived=True. The fallback now re-checks the criterion's own status and archives only if one exists. Otherwise the delete is retried once, so the cascade sees any ancestor status committed meanwhile and archives only that ancestor; a failure on the retry propagates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ion (openedx#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…levels (openedx#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts (openedx#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…der the tag lock (openedx#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…penedx#674) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop the ancestor-group, criterion and ObjectTag row locks from delete_or_archive_competency_criterion. The criterion is still re-read inside the transaction after the competency's Tag lock, the same lock a create for that competency takes first, so a concurrent create still waits for the delete. The criterion's ObjectTag is deleted only when no other criterion references it, as before. Also revert the pk ordering this PR added to the group lock in bulk_update_competency_criteria; it only kept a common lock order with the ancestor locks removed here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
) _cascade_from_group stopped at any archived group. After a learner whose status archived a criterion is deleted, a repeat delete hard-deletes the criterion and left its archived groups behind with nothing in them, breaking ADR-0002 Decision 2. On a delete event, an archived group left with no criteria, no child groups and no group-level status is now deleted and the walk continues upward. Otherwise the walk still stops at an archived group without saving it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3e0580d to
b1e27af
Compare
jesperhodge
left a comment
jesperhodge
left a comment
There was a problem hiding this comment.
because the concurrency considerations are so complex here, I gave it another full review, and AI discovered some important additional concurrency issues. I workshopped it with AI for quite a while to get a full picture - I'm pretty sure the points about the possible deadlock are accurate, so we should make sure to fix these.
When you have implemented the fixes, please run a full AI /backend-code-review on your end, and prompt it to focus especially on concurrency and db issues, then fix follow-ups it brings up until you and AI are satisfied. Attach the PR review and a fix report please.
Because this is complex, please run the review on Fable with effort "xhigh".
Here's the AI review:
"
Must fix before merge
Deleting a criterion while someone else edits it can deadlock.
Details
In delete_or_archive_competency_criterion and _cascade_from_group the delete takes the Tag lock, then writes the criterion row (an UPDATE to archive or a DELETE), then writes the leaf group and its ancestors. In bulk_update_competency_criteria the order is reversed: it locks the root, course-level, and leaf groups in one select_for_update() statement, then locks the criteria. Bulk update takes no Tag lock, so the Tag lock does not serialize the two. The review confirmed both orders by reading the code at commit b1e27af; the cycle itself is inferred from row-lock semantics, since SQLite cannot show it.
When an author edits a criterion's rule (bulk update) while the same criterion is being removed (the #710 click), from two tabs or by two authors, the delete holds the criterion and waits for the leaf group, bulk update holds the leaf group and waits for the criterion, and the database kills one request with an OperationalError that nothing catches, a 500. Round 1 named this exact cycle, round 2's ancestor-group locks closed it, and round 3 removed those locks. InnoDB usually picks the transaction that changed fewer rows as the victim, so bulk update, which has written nothing while it waits, loses its edit. InnoDB rolls back the victim's whole transaction, so nothing inside the request can retry. The cycle needs the leaf to be left with no other live criterion, in both the archive and the hard-delete branch. With a surviving live sibling the walk only reads. In the default tree shape every create without a group id makes a new leaf for its criterion, so every delete empties its leaf and the cycle is the common case rather than an edge case.
The fix with the fewest locks is one rule for every authoring writer of a competency's tree: take that competency's Tag row lock first, read only after it, take no other row lock, and write several criteria in ascending primary-key order. It must land with or before this PR.
- Bulk update moves onto the Tag lock. It reads the group to learn its tag id, takes
Tag.objects.select_for_update().get(pk=group.tag_id), drops its two row locks, keeps the archived and membership checks after the lock so they read current data, and saves criteria in ascending primary-key order. ADR-0004 describes the one-lock shape as deadlock-free. The ordering is not optional. Once bulk update no longer locks all its criteria in one statement, each UPDATE takes an exclusive lock on its own, while the grading write inserts status rows in ascending criterion order and takes a shared lock on each criterion. Two different orders would form a new cycle once grading integration lands. - Landing commit 7ca4eaa on its own is not enough. It removes the cycle with zero locks, but it leaves bulk update's archived check with no lock behind it: an edit racing an archive writes onto an archived criterion, and an edit racing a hard delete fails with a 500 when the delete commits between the read and the save. Its commit message's premise, that no code path archives groups or criteria, stops being true when this PR lands. Keep its primary-key loop and replace its lock removal with the Tag lock.
- PATCH takes the same lock.
update_competency_criteria_groupchecksarchivedand later saveslogic_operatorwith no lock between, so it is check-then-act against this archive. If a delete archives the group in between, PATCH writes onto an archived group and simple_history records a row witharchived=False. If the delete hard-deletes the group, the save updates zero rows and Django raisesDatabaseError, a 500. There is no cycle, but the fix is two lines and it keeps the rule without an exception the next writer could copy: read the group to learn its tag id, take the Tag lock, re-read the group, then run the existing checks. Both outcomes then become a clean archived error or a 404.
No test exercises the locks with a second connection, although CI runs the suite on MySQL 8.
Details
Plain pytest runs on in-memory SQLite, which has no row locks, so none of the 175 tests can show a lock doing anything. The django52 tox target that CI runs takes DJANGO_SETTINGS_MODULE=mysql_test_settings from tox's base [testenv] section and runs against a MySQL 8 service container, so tests that need real locks can run there and skip locally. Every test below uses two connections: one thread runs the delete and pauses inside its transaction, and a second connection acts while the first is paused. Put them in a new file, tests/openedx_learning/applets/cbe/test_criterion_concurrency.py.
Harness rules. Each one prevents a specific hang or false pass.
- Mark each test
@pytest.mark.django_db(transaction=True). The default test transaction would hide the fixtures from the second connection and would never release the lock mid-test. - Skip when
connection.features.has_select_for_updateis false, so the tests skip on SQLite and run in CI. - Run the second actor in a
threading.Thread. Django gives each thread its own connection. Catch any exception in the thread and re-raise it in the test, and callconnection.close()in the thread'sfinally, because the table flush after the test waits on any transaction the thread left open. - Pause deterministically with two
threading.Eventobjects, never withsleep(). To pause before the walk, patch_cascade_from_groupin the api module with a wrapper that signals, waits, then calls the original, and make it pause only on its first call, because the walk calls itself at each level. To pause after the walk, patchCriterionDeletionResultin the api module the same way, since the function constructs it last, inside the transaction. - Set
innodb_lock_wait_timeoutto a few seconds on each thread's connection withSET SESSION, so a test that blocks where it should not fails in seconds instead of fifty. Release every paused thread in afinally. - To check that a row is locked, run
Tag.objects.select_for_update(nowait=True).get(pk=...)insidetransaction.atomic()on the probing connection. It raisesDatabaseErrorat once while another connection holds the row, with no timing involved. - To check that a thread is blocked,
join()it with a short timeout and assert it is still alive.
The tests to add. Tests 4 and 5 fail at head and pass once the deadlock fix above is in, so write them first.
| Test | Steps | Must assert |
|---|---|---|
| 1. The Tag lock is held for the whole request | Pause the delete before the walk. Probe the Tag row with nowait=True from the test. Resume, join, probe again. |
The first probe raises DatabaseError. The second succeeds. |
| 2. A create for the same competency waits, and its criterion survives | Pause the delete of a leaf's only criterion before the walk. In a thread, start a create for the same competency on a second unit in the same course. Resume the delete, join both. | The create thread is still alive while the delete is paused. Afterward the new criterion exists under a new leaf, course-level group and root, and the deleted criterion and its original leaf, course-level group and root rows are gone. |
| 3. Two deletes of sibling criteria leave no empty leaf | Put two criteria in one leaf. Pause the delete of the first before the walk. In a thread, start the delete of the second. Resume, join both. | The second thread is alive while the first is paused. Afterward both criteria and the leaf, course-level group and root are gone. |
| 4. Delete and bulk update do not deadlock | Leaf with one criterion. Pause its delete before the walk. In a thread, start a bulk update of that criterion. Resume, join both. Parametrize over hard delete and archive, adding a learner status row for the archive case. | Neither thread raises OperationalError. The delete returns normally. Bulk update raises CompetencyCriterion.DoesNotExist or CompetencyCriteriaGroup.DoesNotExist. At head one thread raises OperationalError, MySQL error 1213, instead. |
| 5. PATCH does not write onto an archived or deleted group | Leaf with one criterion. Pause its delete after the walk. In a thread, start a PATCH of the leaf's logic operator. Resume, join both. Parametrize as in test 4. | PATCH raises CompetencyCriteriaGroup.DoesNotExist after a hard delete, or CompetencyCriteriaGroupArchivedError after an archive, and no group history row records a logic-operator change. At head PATCH raises DatabaseError after a hard delete and writes onto the archived group after an archive. |
| 6. A grade write and a hard delete fail one side, never silently | Ordering A: in a thread, inside transaction.atomic(), call record_graded_object_statuses for the criterion's object and pause before the transaction.atomic() block ends. Run the delete in a second thread, confirm it is blocked, then resume the grade thread. Ordering B: pause the delete before the walk. In a thread, inside transaction.atomic(), call record_graded_object_statuses for the same object, confirm it is blocked, then resume the delete. |
A: the delete thread raises IntegrityError, the criterion and the status row both still exist, and a second delete call archives the criterion. B: the grade thread raises IntegrityError, the criterion is gone, and no status row exists. |
Run them locally against the container that the settings module documents:
docker run --rm -e MYSQL_DATABASE=test_oel_db -e MYSQL_USER=test_oel_user \
-e MYSQL_PASSWORD=test_oel_pass -e MYSQL_RANDOM_ROOT_PASSWORD=true -p 3306:3306 mysql:8
DJANGO_SETTINGS_MODULE=mysql_test_settings pytest --no-cov tests/openedx_learning/applets/cbe/test_criterion_concurrency.py
tox -e django52 -- <path> runs the same thing. The per-writer lock-point tests in the action list, which patch the lock with a side effect, stay separate: they run on SQLite too, and they cover a different property, that each writer reads after the lock.
The pull request description was not rewritten, although the author's reply of 2026-10-26 said it was, and it is still inaccurate.
Details
The author's reply in the review thread says the description was rewritten. It still says #613 has not landed on main and that the branch cherry-picks it with a renumbering commit (the branch has twelve commits of #674 work and no migrations). It still says untagging goes through the tag_object() read-merge-write call (the code deletes the ObjectTag row directly through the ORM). It still names the status model StudentCompetencyCriteriaStatus (the class is StudentCompetencyCriterionStatus), gives no way to create a status row for manual testing, and says the 403 is checked "for the criterion's course" when the check is against the criterion's ObjectTag.object_id. It has no deviations section: the URL is scoped by tag rather than group, the URL name is criterion-delete, archived now appears in the bulk-update response, and archived groups stay reachable by the create path until #717 lands. Rewrite it.
Two comments the review asked for were never posted.
Details
Round 3 asked for a comment pinging ormsbee to double-check the remaining Tag select_for_update(); the only mention of ormsbee in the thread is that request. The author's reply says the ADR questions were "posted separately below"; no such comment exists. The author reported both as done. Post both. The content for the ormsbee check-in is in the finding on what the Tag lock guards, under "Improvements worth doing", and the content for the ADR comment is the finding on the four ADR deviations in the same section.
Two code comments on the create path say the Tag lock exists only for the duplicate check, and the new delete makes that false.
Details
The _create_leaf_group docstring says "The caller's tag lock is for _reject_duplicate_criterion()", and the comment at the lock in associate_competency_criterion says "the tag lock only serializes _reject_duplicate_criterion()". The delete now relies on that same lock to keep a concurrent create's get_or_create and leaf INSERT from landing between the walk's emptiness check and the collector SELECT inside group.delete(). The diff does not touch either comment. Add one sentence to each saying the delete depends on the lock too.
One test describes a mechanism the code no longer uses.
Details
test_hard_delete_leaves_a_tag_of_another_competency_on_the_same_object in test_criterion_removal.py monkeypatches get_object_tags in the api module, which the delete path no longer calls; the review removed the patch and the test still passed. Its docstring says the tag list is stubbed and "the delete must not depend on reading it", which is no longer true of anything. The remaining assertion (another competency's ObjectTag on the same subsection survives) does protect something: it fails if the delete filters by object_id instead of by primary key. Drop the monkeypatch, the stale_list, and the monkeypatch parameter, and reword the docstring to what it asserts.
Improvements worth doing
The delete's documentation does not explain everything its lock protects.
Details
The delete's docstring says only that a concurrent create waits. The lock prevents three things, all following from Django's Python-side cascade:
- A create for the same competency that commits a new leaf and criterion between the walk's emptiness check and the collector SELECT inside
group.delete()would have those rows collected and deleted silently, after the creator already received a 201. - Two deletes of sibling criteria in one leaf would each still see the other's criterion and leave an empty leaf behind.
- A create that reuses the same
ObjectTagand commits between the "still referenced" check and theObjectTagdelete would have its criterion cascaded away.
No lock-free alternative keeps all three. Catching IntegrityError cannot help because the silent-loss case raises nothing. A raw conditional DELETE would skip the collector but also skip the simple_history deletion row that ADR-0003 requires. Changing the foreign keys to RESTRICT is a model change outside #674, and it would fix only the first race: two sibling deletes never try to delete the leaf, and the criterion's ObjectTag foreign key stays CASCADE. It would also break course-run deletion, which reaches leaves only through parent. Locking the root group row instead of the Tag does not work either. The root may not exist yet when a create runs, and the walk can delete it mid-request, so every writer would need a get-or-create-then-lock loop. One nuance on the first race: the loss is silent only when the create commits inside the window between the emptiness check and the collector's SELECT. A create whose INSERT ran before that window and committed after it makes the delete's DELETE wait on the create's shared lock and then fail with an integrity error, a 500 rather than a loss.
The same docstring, next to the lock, should state that the code assumes READ COMMITTED. The view's get_object_or_404 and the function's first read run before the Tag lock, inside the request transaction. Under READ COMMITTED the re-read after the lock sees a concurrent delete that won, and the walk's exists() checks see concurrent commits. A deployment can set isolation_level to REPEATABLE READ in the database options. Then the re-read sees the pre-lock snapshot, a racing duplicate DELETE returns 200 and writes a second deletion history row, and two sibling deletes can leave an empty group. ADR-0004 rejected a design because its "correctness would depend on the isolation level, silently", and this is the same dependency.
The lock is held until the request commits. Create and delete take it today, and bulk update and PATCH join them with the fix above. Inserts that reference the Tag (object tags from ordinary Studio tagging in any course, groups, the roll-up's competency status, child tags from a taxonomy import) and updates or deletes of the Tag row itself wait meanwhile, for the length of one authoring request. None of those waiters holds anything the delete needs, so none can form a cycle, except an admin or import deleting the Tag, a roll-up wrapped in a transaction, and any single transaction that untags this competency from one unit and tags another unit with it. Write the three races, the isolation assumption, and the waiters in the delete docstring in a few sentences, note at the create lock that delete depends on it, and use the same text as the ormsbee check-in.
Saving a learner's grade at the same moment as a hard delete fails one side with an error, and the docstring should name both outcomes.
Details
The delete checks for a status row, then deletes the criterion. A grade write that inserts a status row at the same moment is not waited for, and which side fails depends on the order:
- Status INSERT first: the author's DELETE waits for the grade transaction and then fails the foreign-key check, a 500. A second click archives the criterion.
- DELETE first: the grade's INSERT waits for the delete to commit and then fails inside the learner's grade transaction.
On PostgreSQL the check runs at commit instead, so the grade side usually fails there, but this deployment runs MySQL. The author's position that this is "an error rather than silent data loss" is true, but the grade side is the worse side and the docstring names neither outcome. Catching IntegrityError around the delete in a savepoint and archiving instead is not a sound fix: it never helps the grade side, and it brings back the retry branch round 1 found untestable.
Decision: name both outcomes in the delete docstring, and require in the grading-integration ticket that record_graded_object_statuses and the roll-up run inside tasks that retry on DatabaseError, as edx-platform's grade tasks already do. On retry the deleted criterion is simply gone, and the roll-up's group-status insert, which fails with an integrity error when the delete has removed the group, is skipped the same way. Do not add a 409 mapping now: no edx-platform caller of the grading write exists yet, the author-side failure on MySQL is a clean full rollback that a second click resolves, and the catch would have to be either broad, hiding real integrity bugs, or specific to MySQL's foreign-key error code. Revisit it when grading integration lands.
Four places where this change departs from the written design decisions need the ADRs amended.
Details
#674 already made the first three of these calls, and the fourth follows from the lock rule in the first Must-fix item, so nothing is left to decide. What is missing is the record: one line each in the PR description's deviations section, and an amendment with a changelog entry in the ADR concerned.
_group_has_learner_progresschecks the group-status table directly, while ADR-0002 Decision 7 says that table is "not independently checked for this purpose". #674's AC12 requires the check, so amend Decision 7.- An archived criterion's
ObjectTagstays live, so the competency tag stays visible on the subsection, while ADR-0003 Decision 3 says ordinary reads exclude archived associations. #674 accepts it for the MVP, so amend Decision 3 to allow the exception. - ADR-0002 Decision 2 says "backend validation must reject" empty groups, and #674 chose to cascade them away instead. Amend Decision 2 to name the cascade as the enforcement mechanism.
- ADR-0004 Decision 6 says "no other path takes a lock". Read literally it contradicts the Tag lock that create and delete take, and that bulk update and PATCH will take after the fix. Narrow it to learner-status writers and record that authoring writers serialize per competency on the Tag row.
A parameter of an internal function has no effect on what the code does.
Details
The event parameter of _cascade_from_group changes nothing. Forcing event = "delete" on every call, or swapping either archive branch's next_event to "delete", left all 175 tests green; forcing "archive" failed six. After an archive the archived child still exists, so the existence checks already decide every place event is tested. Drop the parameter and let the existence checks decide, which also simplifies #675's reuse, or add a test that distinguishes the two if a reachable state exists (the review found none).
Deletion history records can lack the user who acted.
Details
The archive path sets _history_user on the instance. The delete path calls .delete() on the criterion and on each group without setting it, so simple_history falls back to the request middleware, and a shell, task, or plugin caller writes deletion rows with no user. That is what this module's own comments say must not happen. Set _history_user = user before each .delete().
One step of the delete has no test that fails when the step is removed.
Details
Deleting the re-read line under the Tag lock leaves all 175 tests green. A test that patches Tag.objects.select_for_update to delete the criterion row and then return the real queryset fails without the re-read and passes with it. Add it to test_criterion_removal.py.
Archiving re-validates the rule payload, and the view's 400 test should use that real case.
Details
CompetencyCriterion.save() always calls full_clean(), so the archive write re-validates the override payload although the archive changes only the flag. The review confirmed it with real input: a criterion whose stored payload has become invalid and that has a learner status returns 400 on DELETE with the error under __all__, and can be neither archived nor deleted. This is reachable only if payload validation is tightened after rows exist. Decision: accept it for now, since it needs that later tightening to become reachable, and cover it with a real test. test_delete_maps_a_validation_error_from_the_api_to_400 raises {"object_id": "x"}, a key the endpoint cannot produce, so replace the mocked test with one that archives a criterion carrying an invalid stored payload. If the case ever bites, skipping payload validation when update_fields excludes the rule fields is a model change for a follow-up ticket.
Two tests largely overlap.
Details
test_untag_with_the_competency_taxonomy_disabled_removes_only_the_orphaned_object_tag still passes with the taxonomy-disable and the permission swap removed, and the disabled-taxonomy 403 is already covered in test_views.py. Merge it with the other "other ObjectTag survives" test: keep one test, named for what it asserts, without the word "untag".
A code comment states a fact as always true when it holds only through one code path.
Details
The comment "A group's tag is always a competency tag, so its taxonomy is always set" is true only through the create path: Tag.taxonomy is nullable, the group's tag foreign key has no taxonomy constraint, and a comment in update_competency_criteria_group in the same file says the opposite. Say "the create path only accepts competency tags", or call resolve_competency_tag as that function does. The assert itself has six precedents in the module.
Small things
The re-read inside the transaction skips a select_related, costing two extra queries.
Details
The re-read inside the transaction has no select_related("group", "object_tag"), so the two attribute reads after it cost two more queries, and the same row is read three times (view, first read, re-read).
The main docstring of the delete function contains several inaccuracies and formatting errors.
Details
- It says "the whole operation runs in one transaction", but the first read and the permission check precede
atomic(). - It says the permission is "as
bulk_update_competency_criteriarequires", but bulk update checks a course key and this checks a usage key. - It states
PermissionDeniedtwice. - It omits the "only archived children remain" archive branch.
- It uses single backticks, which RST renders as interpreted text (also in
_cascade_from_group).
Several comments and docstrings are duplicated or needlessly reworded.
Details
The "set on the instance so attribution doesn't depend on simple_history's request middleware" comment now appears three times in the file. The cast(Any, ...) comment restates main's one-line version. The view docstring is reflowed mid-sentence.
A competency tag deleted at the same moment as a delete request produces an unhandled error.
Details
Tag.DoesNotExist from the lock read (an admin deleting the competency tag at the same moment) is unmapped, a 500. Create has the same exposure.
Some view tests duplicate each other and checks the API tests already make.
Details
In test_views.py, test_unknown_criterion_id_404s and test_repeat_delete_after_hard_delete_404s take the same lookup miss and could be one parametrized test. The view tests also repeat row-state assertions the API tests already own.
The ticket does not yet note where this change departs from it.
Details
The deviations from #674 (tag-scoped URL, URL name) are still not noted on the ticket.
The accuracy gate returned PASS WITH FIXES; the fixes are applied above. The cleanup added 12 definitions or glosses that nobody has confirmed against the code or the ADRs.
Definitions the cleanup added, unconfirmed
- "Review rounds": rounds 1 to 3 are the earlier review passes on this pull request.
- The group's
tagforeign key points at the competency'sTagrow inopenedx_tagging, and the "Tag lock" is aselect_for_update()on that row. - The labels "the walk" (for
_cascade_from_group), "create", "bulk update", and "PATCH" (for the three named API functions), and "deletion collector" (for Django's Python-side cascade emulation). - Isolation level, glossed as the rule for which concurrent changes a statement sees; REPEATABLE READ, glossed as showing a transaction the snapshot from its first read.
- Deadlock, glossed as a cycle in which two transactions each wait for a lock the other holds, which the database breaks by aborting one with an error.
- Check-then-act, savepoint, and deferred foreign key glosses.
- ADR-0002, ADR-0003, and ADR-0004 named as "competency criteria model", "competency criteria versioning", and "competency mastery concurrency", located in
docs/openedx_learning/decisions/(the file names exist on the branch; the ADR bodies were not reread for this). - "AC" expanded as acceptance criteria, and the statement that #674 lists numbered acceptance criteria (the numbering was added by the review's ticket extract).
Description
Adds a
DELETEendpoint that removes aCompetencyCriterion(a gradeable-subsectionassociation within a Competency Criteria tree): hard-deletes it when no learner mastery status
references it, or archives it in place (
archived=True) when one does, so learner progressalready recorded against that criterion stays intact and auditable.
On the hard-delete path, the criterion's
ObjectTagis untagged via the same read-merge-writetag_object()callcreate_competency_criterion()already uses to tag it, but only once noother
CompetencyCriterionstill shares thatObjectTag(the same tagged assignment can be usedin two different groups). The archive path never touches the
ObjectTag.Removing or archiving a criterion can leave its parent
CompetencyCriteriaGroup, and potentiallyseveral ancestor groups above it, with no remaining active content. This endpoint walks the
ancestor chain and hard-deletes or archives each level in turn (mirroring the same
learner-status rule), so no empty group is ever left persisted, per
ADR-0002 Decision 2. The
delete-vs-archive choice, for both the criterion and any cascaded ancestor group, is a
server-side detail driven entirely by learner-status presence; the
DELETEverb covers bothoutcomes; see the ADR-0002 Decision 7 delete-protection rule this implements.
This change impacts the Course Author / Platform Administrator roles (correcting or
retiring a mastery rule). It is a backend API addition only; no Studio UI consumes it yet (the
in-use warning/confirmation flow this enables is a separate, Studio-side ticket).
Supporting information
StudentCompetencyCriteriaStatus/StudentCompetencyCriteriaGroupStatusmodels this endpoint's archive-vs-delete branch queries) and [BE] Create CompetencyCriteria #665 (creates the
CompetencyCriterionrows this endpoint removes, and therest_api/v1/scaffolding this PRextends) and [BE] Add archived field to Competency Criteria & Group models #716 (the
archivedfield onCompetencyCriteriaGroup/CompetencyCriterion)Testing instructions
pip install -e .(or your usual dev setup), then from the repo root:criteria:
POST /api/cbe/v1/competencies/<tag_id>/criteria/to create a criterion (see [BE] Create CompetencyCriteria #665).DELETE /api/cbe/v1/competencies/<tag_id>/criteria/<criterion_id>/on it:200 {"id": ..., "archived": false}, row gone.StudentCompetencyCriteriaStatusrow recorded against it →200 {"id": ..., "archived": true},row still present with
archived=true.DELETEon an archived row → same200response again.DELETEon an already hard-deleted row →404.tag_id, or an unknowncriterion_id→404.oel_tagging.can_tag_objectfor the criterion's course →403.Other information
Does this change depend on other changes elsewhere? Yes, see "Depends on" above. [BE] Implement CBE core data models (CompetencyTaxonomy, criteria, learner status) #613
(
StudentCompetencyCriteriaStatus/StudentCompetencyCriteriaGroupStatus) has not landed onmainyet; this branch cherry-picks that model work from an in-progress stacked PR(
jesperhodge/openedx-core, local branch642b-02-group-status, tracked upstream as issue[BE] Implement CBE core data models (CompetencyTaxonomy, criteria, learner status) #613/Mastery status lookup + learner progress models #642) so this ticket's own logic could be built and tested against it. One commit on this
branch (
chore: renumber borrowed mastery-status migrations onto this branch's base) existssolely to resolve a migration-number collision between that cherry-picked work and this
branch's own base; both the cherry-picked commits and that renumbering commit should be dropped
and this branch rebased once [BE] Implement CBE core data models (CompetencyTaxonomy, criteria, learner status) #613 lands on
mainthrough its own review.No new model fields or migrations are introduced by this ticket's own commit (the feature
commit itself touches no migrations); it only adds application logic (
api.py,views.py,urls.py) and a read-onlyarchivedfield on the existingCompetencyCriterionSerializer.Not in scope here (left to [BE] Build endpoint for removing a Competency Criteria Group #675, the group-level counterpart): archiving or deleting a
CompetencyCriteriaGroupdirectly, or the Studio in-use warning/confirmation flow ADR-0003Decision 4 requires before calling this endpoint.
Rebased version of Feat: add endpoint to delete or archive a CompetencyCriterion (openedx#674) jesperhodge/openedx-core#27
🤖 Generated with help of Claude Code