From eb2b33353dcb8d1001d3dde2e12125b56a92a54b Mon Sep 17 00:00:00 2001 From: ufedaseyeuconsultant Date: Fri, 13 Nov 2026 20:30:29 +0300 Subject: [PATCH] feat: add endpoint to delete or archive a CompetencyCriterion (#674) DELETE /api/cbe/v1/competencies//criteria// hard-deletes a criterion that no learner mastery status references, and archives it in place otherwise, so recorded learner progress stays intact and auditable. The response is 200 {"id", "archived"} in both cases, and a repeat DELETE on an archived row returns the same body. On a hard delete, the criterion's own ObjectTag row is removed, and only while no other criterion references it; no other tag on the object is touched. The archive path never touches the ObjectTag. Groups left with no children by the delete are cascaded: each emptied ancestor is hard-deleted, or archived when a learner group-status row references it or when only archived children remain, in the same transaction, so no empty group persists (ADR-0002 Decision 2). Every writer of a competency's criteria follows one concurrency rule: take the competency's Tag row lock first, read mutable state only after it, take no other explicit lock, and save criteria in ascending primary-key order. Bulk update drops its group and criteria select_for_update() calls in favor of that lock, closing a deadlock against the delete (MySQL error 1213), and the group PATCH re-reads the group under the lock, so losing a race to the delete yields a 404 or an archived-conflict error instead of a DatabaseError. A grade write that races a hard delete fails one side with an IntegrityError, never silently; the delete view maps it to a retryable 409. A MySQL-only two-connection test file exercises the lock across the request, a waiting create, sibling deletes, the delete against bulk update and the group PATCH, and the grade race in both orderings. SQLite lock-point tests pin that each writer reads after taking the lock. ADR-0002 Decision 7 and ADR-0004 Decision 6 are amended with changelog entries. The follow-up for moving the lock off the shared Tag row is #935. Co-Authored-By: Claude Opus 5.5 Co-Authored-By: Claude Sonnet 5 Co-Authored-By: Claude Fable 5 Co-authored-by: Mary Gwozdz --- .../0002-competency-criteria-model.rst | 6 +- .../0004-competency-mastery-concurrency.rst | 9 +- src/openedx_learning/applets/cbe/api.py | 229 ++++++- src/openedx_learning/applets/cbe/data.py | 13 + .../applets/cbe/rest_api/v1/serializers.py | 5 +- .../applets/cbe/rest_api/v1/urls.py | 5 + .../applets/cbe/rest_api/v1/views.py | 41 +- .../openedx_learning/applets/cbe/conftest.py | 33 ++ .../openedx_learning/applets/cbe/test_api.py | 85 +++ .../applets/cbe/test_criterion_concurrency.py | 468 +++++++++++++++ .../applets/cbe/test_criterion_removal.py | 559 ++++++++++++++++++ .../applets/cbe/test_views.py | 242 +++++++- 12 files changed, 1665 insertions(+), 30 deletions(-) create mode 100644 tests/openedx_learning/applets/cbe/test_criterion_concurrency.py create mode 100644 tests/openedx_learning/applets/cbe/test_criterion_removal.py diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 5446629c5..e253f3b9a 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -319,7 +319,7 @@ Decision - If no related row exists in ``StudentCompetencyCriteriaStatus`` (the leaf-level learner status table, Decision 6) for a competency definition, delete behaves as it does today: a hard delete that cascades through competency metadata tables. - Once a related row exists in ``StudentCompetencyCriteriaStatus``, deletion of the associated competency definition row still succeeds, but as an archive (soft delete) instead of a hard delete: the row is hidden from authoring and new associations but remains queryable, so existing learner status rows stay resolvable. This archive-vs-hard-delete rule applies to ``oel_tagging_tag``, ``oel_tagging_taxonomy``, ``CompetencyTaxonomy``, ``oel_tagging_objecttag``, ``CompetencyCriteriaGroup``, and ``CompetencyCriteria``; see :ref:`openedx-learning-adr-0003` Decision 3 for ``oel_tagging_objecttag``'s own archive rule and traceability exception. - - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are not independently checked for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. + - ``StudentCompetencyCriteriaStatus`` is what determines whether a record is protected. ``StudentCompetencyCriteriaGroupStatus`` and ``StudentCompetencyStatus`` are roll-up tables derived from it (Decision 6) and are never relied on alone for this purpose: :ref:`openedx-learning-adr-0004` writes the leaf table synchronously with the grade but rolls the two roll-up tables up later via an asynchronous task, which can lag behind the leaf or, per that ADR's Decision 5, need manual recovery. Checking only the roll-up tables could therefore miss real learner progress that has not rolled up yet. - Direct deletion of a ``CompetencyRuleProfile`` is never a hard delete; retirement is always archive-only, via a normal update to its ``archived`` column (Decision 3). However, if a taxonomy or course that is associated with a taxonomy- or course-scoped profile is deleted, then this profile will be deleted along with it. - For MVP, archiving a ``CompetencyCriteriaGroup`` or a ``CompetencyCriteria`` row is permanent, with one exception: an archived root ``CompetencyCriteriaGroup`` is reactivated when a new criterion is added for its competency (Decision 2). ``oel_tagging_objecttag`` follows :ref:`openedx-learning-adr-0003` Decision 3 instead. @@ -499,6 +499,10 @@ Changelog competency's own status row and up through parent competencies, and states what a competency with no criteria tree of its own receives. +2026-11-04: + +* Clarified Decision 7: the roll-up tables are never relied on alone to protect a record. + 2026-11-06: * Decision 4 states that no ancestor or descendant of a competency tag may hold live criteria diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index a018cd45b..380236b86 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -68,7 +68,7 @@ Decision rows above it are recalculated and overwritten rather than merged. A later grade change can raise what an edit lowered, but never lower what an edit raised. The staff correction takes a row lock on the ``StudentCompetencyCriteriaGroupStatus`` row for that learner and the competency's root - criteria group, the group with no parent; no other path takes a lock. + criteria group, the group with no parent; no other learner-status writer takes a lock. @@ -162,3 +162,10 @@ Rejected Alternatives before the write completes. - It also only checks for a race at the moment each parent is read. If the worker crashes mid-cascade before reaching the next read, nothing notices the rollup was left unfinished. Decision 5's manual recovery mechanism exists to catch that case. + +Changelog +--------- + +2026-11-04: + +* Clarified Decision 6: the rule that no other path takes a lock covers learner-status writers only. diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 18a553c53..ef9f69562 100644 --- a/src/openedx_learning/applets/cbe/api.py +++ b/src/openedx_learning/applets/cbe/api.py @@ -7,7 +7,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from decimal import Decimal -from typing import Any, Iterable, Sequence +from typing import Any, Iterable, Sequence, cast from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError from django.db import connection, transaction @@ -23,7 +23,7 @@ from openedx_tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.rules import ObjectTagPermissionItem, UserType -from .data import GradedObjectScore +from .data import CriterionDeletionResult, GradedObjectScore from .models import ( CompetencyCourseRun, CompetencyCriteriaGroup, @@ -32,6 +32,7 @@ CompetencyTaxonomy, LogicOperator, MasteryStatus, + StudentCompetencyCriteriaGroupStatus, StudentCompetencyCriterionStatus, ) from .rollup import roll_up_competency_statuses @@ -43,11 +44,13 @@ "CompetencyCriteriaGroupArchivedError", "CompetencyCriteriaTree", "CompetencyCriterionArchivedError", + "CriterionDeletionResult", "GradedObjectScore", "associate_competency_criterion", "bulk_update_competency_criteria", "create_competency_criterion", "create_competency_taxonomy", + "delete_or_archive_competency_criterion", "get_blocked_courses", "get_competency_criteria_tree", "get_competency_rule_profiles", @@ -293,7 +296,8 @@ def _create_leaf_group( 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()``. + from a concurrent insert. The caller's tag lock is for ``_reject_duplicate_criterion()``. The delete path + relies on the same lock to keep this function's inserts out of its emptiness-check window. """ name_max_length = CompetencyCriteriaGroup._meta.get_field("name").max_length # Both get_or_create() lookups match archived rows deliberately, so an archived container is @@ -489,10 +493,16 @@ def create_competency_criterion( resolves to a concrete profile, and the model's own xor-override constraint forbids leaving all three fields null. - ``course_run`` is ``group``'s course. Must run inside a transaction holding the tag and - course run locks that associate_competency_criterion() takes, so the dominance check below - can't race another create. + ``course_run`` is ``group``'s course. Must run inside a transaction. This function takes the + competency's Tag row lock itself, so a concurrent delete of a sibling criterion can't cascade + this call's new rows away. It takes the lock here, not only in its caller, so a future direct + caller of this public function can't bypass it: the delete path's correctness depends on every + creator holding it. The caller must also hold the course run lock that + associate_competency_criterion() takes, so the dominance check below can't race another create, + and must take the Tag lock before the course run lock. """ + # Re-acquiring the lock associate_competency_criterion() already holds is harmless within one transaction. + Tag.objects.select_for_update().get(pk=group.tag_id) tag = group.tag # tag.taxonomy_id is nullable at the model level; already guaranteed set by the caller. assert tag.taxonomy_id is not None @@ -626,10 +636,11 @@ def associate_competency_criterion( with transaction.atomic(): # 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 + # The database enforces one root per tag; the tag lock 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. + # concurrent copy. delete_or_archive_competency_criterion() depends on this lock too: it keeps this + # function's inserts out of the delete's emptiness-check window. Tag.objects.select_for_update().get(pk=tag.pk) # Serializes the per-course dominance check on CBE's own row, not the catalog's CourseRun. # Created first, since a locking read of a row that doesn't exist yet locks nothing; get_or_create() @@ -688,6 +699,11 @@ def _validate_rule_source( raise ValidationError(errors) from exc +def _set_history_user(instance: Any, user: UserType) -> None: + """Record ``user`` as the author of ``instance``'s next history row; API callers have no request middleware.""" + instance._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + + def _apply_rule_source( criterion: CompetencyCriterion, rule_profile_id: int | None, @@ -722,8 +738,7 @@ def _apply_rule_source( return criterion.rule_profile_id, criterion.rule_type_override, criterion.rule_payload_override = target - # Set on the instance so attribution doesn't depend on simple_history's request middleware. - criterion._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(criterion, user) criterion._change_reason = change_reason # type: ignore[attr-defined] # pylint: disable=protected-access # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. criterion.save(update_fields=["rule_profile", "rule_type_override", "rule_payload_override"]) @@ -754,7 +769,7 @@ def bulk_update_competency_criteria( if any named id isn't a criterion of that group, and ValidationError, keyed by the REST field names, for a non-leaf group, an empty or repeated id list, or an invalid rule source. Raises CompetencyCriterionArchivedError if the group, either of its ancestors, or any named criterion - is archived. + is archived. Raises Tag.DoesNotExist if the group's competency tag was deleted concurrently. """ with transaction.atomic(): group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy", "parent__course").get( @@ -763,7 +778,7 @@ def bulk_update_competency_criteria( # Criteria hang only off leaf groups, and a leaf's course is on its course-level parent. if group.parent is None or group.course_id is not None or group.parent.course is None: raise ValidationError({"group_id": _("group_id must reference a leaf CompetencyCriteriaGroup.")}) - # A group's tag is always a competency tag, so its taxonomy is always set. + # The create path only accepts competency tags, and a group's tag never changes, so its taxonomy is set. assert group.tag.taxonomy is not None perm_obj = ObjectTagPermissionItem(taxonomy=group.tag.taxonomy, object_id=str(group.parent.course.course_key)) if not user.has_perm("oel_tagging.can_tag_object", perm_obj): @@ -775,13 +790,18 @@ def bulk_update_competency_criteria( raise ValidationError({"criterion_ids": _("criterion_ids must not name the same criterion twice.")}) _validate_rule_source(rule_profile_id, rule_type_override, rule_payload_override) + # Every CBE authoring writer of a competency's criteria takes its Tag row lock first and no other row lock + # before it, so those writers can't deadlock with each other. Django's cascade deletes of a Tag, a Taxonomy + # or a CourseRun take the Tag row last, and can deadlock with a create or a hard delete: one side fails + # with a MySQL 1213 error and no data is corrupted. Everything below reads committed state, since a + # delete, an archive or a create for this competency can't commit while this transaction holds the lock. + # The reads above are safe without it because a group's tag, parent and course never change. + Tag.objects.select_for_update().get(pk=group.tag_id) + # Archiving a group doesn't archive its criteria, yet ADR 0002 Decision 2 closes them to authoring. # An archived ancestor counts too: it stands in for a delete that would have cascaded to this leaf. - # Locked for the same reason as the criteria below. lineage = [group.id, group.parent.id, group.parent.parent_id] - archived_by_id = dict( - CompetencyCriteriaGroup.objects.select_for_update().filter(pk__in=lineage).values_list("pk", "archived") - ) + archived_by_id = dict(CompetencyCriteriaGroup.objects.filter(pk__in=lineage).values_list("pk", "archived")) archived_group_id = next((pk for pk in lineage if archived_by_id.get(pk)), None) if archived_group_id is not None: raise CompetencyCriterionArchivedError( @@ -790,9 +810,11 @@ def bulk_update_competency_criteria( ) ) - # Locked so a concurrent archive can't slip in after the archived check, and so a concurrent - # identical request waits and then sees nothing to change instead of writing a second history row. - criteria_by_id = CompetencyCriterion.objects.select_for_update().filter(group=group).in_bulk(criterion_ids) + # No row lock here: the Tag lock already stops a concurrent archive from slipping in after the archived + # check, and makes a concurrent identical request wait and then see nothing to change instead of + # writing a second history row. Locking the lineage or the criteria as well would only add a second + # lock order for writers to deadlock on. + criteria_by_id = CompetencyCriterion.objects.filter(group=group).in_bulk(criterion_ids) missing_ids = [criterion_id for criterion_id in criterion_ids if criterion_id not in criteria_by_id] if missing_ids: raise CompetencyCriterion.DoesNotExist( @@ -814,7 +836,10 @@ def bulk_update_competency_criteria( # every criterion in a leaf shares the group's tag, so they all resolve to the same profile. applicable = _get_applicable_rule_profile(group.tag.taxonomy.id) if rule_profile_id is None else None criteria = [criteria_by_id[criterion_id] for criterion_id in criterion_ids] - for criterion in criteria: + # Saves run in ascending primary-key order because the grading write inserts status rows in ascending + # criterion-pk order, and each insert takes an implicit shared foreign-key lock on its criterion row; + # a different order here could deadlock with it. + for criterion in sorted(criteria, key=lambda item: item.pk): _apply_rule_source( criterion, rule_profile_id, @@ -857,6 +882,10 @@ def record_graded_object_statuses(*, user_id: int, scores: Sequence[GradedObject raised, where ``0`` means nothing changed. This function performs no authorization check of its own; as with ``openedx_tagging.api``, the caller is responsible for enforcing permissions. + Call this at most once per transaction. Its implicit criterion-row locks are taken in ascending + primary-key order within one call, and two calls in one transaction would form two ascending runs + that can deadlock with a concurrent bulk update. + :raises TransactionManagementError: if called outside an atomic block. """ # In autocommit mode get_or_create() would commit a leaf row on its own, apart from the grade. @@ -923,7 +952,7 @@ def update_competency_criteria_group( only for a real change. Learner status is never read or written. Raises CompetencyCriteriaGroup.DoesNotExist if no group has ``group_id``, or Tag.DoesNotExist - if its tag isn't on a CompetencyTaxonomy. + if its tag isn't on a CompetencyTaxonomy or was deleted concurrently. Raises ValidationError, keyed by the REST field names, for a root group, a group with no course anywhere above it to check permission against, or a ``logic_operator`` other than AND or OR, and PermissionDenied if ``user`` fails ``oel_tagging.can_tag_object`` for the group's taxonomy and @@ -956,6 +985,12 @@ def update_competency_criteria_group( if not user.has_perm("oel_tagging.can_tag_object", perm_obj): # type: ignore[arg-type] raise PermissionDenied + # The checks above read only a group's tag, parent and course, which never change, so they run before + # the lock and a caller who fails them never holds it. + Tag.objects.select_for_update().get(pk=tag.pk) + # Re-read under the Tag lock: a concurrent delete or archive may have won. + group = CompetencyCriteriaGroup.objects.select_related("course").get(pk=group_id) + # Null is a stored state (a single-child group), but never one a caller may ask for. if logic_operator not in LogicOperator.values: raise ValidationError( @@ -970,8 +1005,156 @@ def update_competency_criteria_group( return group group.logic_operator = logic_operator - # Set on the instance so attribution doesn't depend on simple_history's request middleware. - group._history_user = user # type: ignore[attr-defined] # pylint: disable=protected-access + _set_history_user(group, user) # Never QuerySet.update(): it skips post_save, so simple_history would record nothing. group.save(update_fields=["logic_operator"]) return group + + +def _archive_group(group: CompetencyCriteriaGroup, user: UserType) -> None: + """Archive ``group``, writing only the flag and recording ``user`` in its history.""" + group.archived = True + _set_history_user(group, user) + group.save(update_fields=["archived"]) + + +def _cascade_remove_from_group(group: CompetencyCriteriaGroup, user: UserType) -> None: + """ + Walk ``group`` and its ancestors, deleting or archiving any left with no live children. + + An archived group is never saved again: everything above it was handled when it was archived, + so a second save would only add a redundant history row. If it has no criteria, no child groups + and no group-level status, it is deleted and the walk continues upward, so no empty group stays + persisted. Otherwise the walk stops there. + """ + parent = group.parent + if group.archived: + if group.criteria.exists() or group.child_groups.exists(): + return + if _group_has_learner_progress(group.id): + return + _set_history_user(group, user) + group.delete() + if parent is not None: + _cascade_remove_from_group(parent, user) + return + has_live_child = ( + group.criteria.filter(archived=False).exists() or group.child_groups.filter(archived=False).exists() + ) + if has_live_child: + return + if not group.criteria.exists() and not group.child_groups.exists(): + if _group_has_learner_progress(group.id): + _archive_group(group, user) + else: + _set_history_user(group, user) + group.delete() + else: + # Only archived children remain. + _archive_group(group, user) + if parent is not None: + _cascade_remove_from_group(parent, user) + + +def _archive_criterion(criterion: CompetencyCriterion, user: UserType) -> None: + """ + Archive ``criterion`` in place and cascade the archive to any ancestor left with no live children. + + Does nothing if ``criterion`` is already archived, so a repeat call writes no history. + """ + if criterion.archived: + return + criterion.archived = True + _set_history_user(criterion, user) + criterion.save(update_fields=["archived"]) + _cascade_remove_from_group(criterion.group, user) + + +def _criterion_has_learner_progress(criterion_id: int) -> bool: + """Return whether a learner status row references ``criterion_id``.""" + return StudentCompetencyCriterionStatus.objects.filter(criterion_id=criterion_id).exists() + + +def _group_has_learner_progress(group_id: int) -> bool: + """Return whether a learner group-level status row references ``group_id``.""" + return StudentCompetencyCriteriaGroupStatus.objects.filter(group_id=group_id).exists() + + +def delete_or_archive_competency_criterion(criterion_id: int, user: UserType) -> CriterionDeletionResult: + """ + Hard-delete ``criterion_id``, or archive it if a learner status row already references it. + + UNSTABLE: the criteria-authoring family is incomplete, so its validation and group update + and archive behavior may change this function's shape without a deprecation cycle. + + ``user`` must hold ``oel_tagging.can_tag_object`` for the taxonomy of the criterion's group and for + the usage key its ``ObjectTag`` tags. The check runs before anything is locked or changed. + + An ancestor :class:`CompetencyCriteriaGroup` left with no live children by this call is cascaded in + turn: hard-deleted if it has no children of any kind and no group-level status of its own, archived + otherwise, including when only archived children remain. See ADR-0002 Decision 2. + + On a hard delete, the criterion's own ObjectTag is deleted too, and only when no other + criterion references it. No other ObjectTag on the object is touched. + + Raises ``CompetencyCriterion.DoesNotExist`` if no such criterion exists (including one deleted + concurrently), ``Tag.DoesNotExist`` if its competency tag was deleted concurrently, Django's + ``PermissionDenied`` if ``user`` fails the permission check, and Django's ``ValidationError`` if + the archive path's ``full_clean()`` rejects an invalid stored rule payload. + + The first read and the permission check run before the lock. The rest runs in one transaction that + takes only one explicit lock, on the competency's Tag row, the lock every CBE authoring writer of its + criteria takes first, so a concurrent create waits until this call commits. The lock is held until the + request commits, so other authoring writers for this competency, and inserts that reference its Tag + row, wait meanwhile. + + The re-read under the lock and the ancestor walk's ``exists()`` checks rely on READ COMMITTED, the + isolation level this repo always runs (see ``AGENTS.md``): they must see concurrent commits. + + A learner's grade saved at the same moment as a hard delete never loses data silently, but it can + fail one side with an error. The delete checks for a status row and then deletes the criterion, and a + grade write that inserts a status row in between is not waited for. There are three variants when + the grade is the one that gets in first: + + - A status row committed before the status check: the delete archives the criterion, the normal path. + - A status row committed after the check and before ``criterion.delete()`` collects its rows: + ``ProtectedError``. + - A status row inserted but not yet committed when the delete runs: the delete waits for the grade + transaction, then fails the foreign-key check with ``IntegrityError``. + + ``ProtectedError`` is a subclass of ``IntegrityError``. Either one rolls this transaction back and + leaves the criterion in place, and the REST view maps both to a retryable 409, since a second delete + archives the criterion. If the DELETE comes first instead, the grade's INSERT waits for it to commit + and then fails inside the learner's grade transaction. + """ + criterion = CompetencyCriterion.objects.select_related("group__tag__taxonomy", "object_tag").get(pk=criterion_id) + # The create path only accepts competency tags, and a group's tag never changes, so its taxonomy is set. + assert criterion.group.tag.taxonomy is not None + perm_obj = ObjectTagPermissionItem(taxonomy=criterion.group.tag.taxonomy, object_id=criterion.object_tag.object_id) + # has_perm()'s stub expects a Model, but a rules predicate takes any object. + if not user.has_perm("oel_tagging.can_tag_object", cast(Any, perm_obj)): + raise PermissionDenied() + tag_id = criterion.group.tag_id + with transaction.atomic(): + # A lock rather than a constraint, because Django cascades deletes in Python and the database can't + # catch three races that follow. A create that commits a new leaf and criterion between the ancestor + # walk's emptiness check and the row-collecting SELECT inside ``group.delete()`` would have those rows + # deleted silently, after the create call already returned success. 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 ObjectTag and commits between the "still referenced" check and the ObjectTag delete + # would have its criterion cascaded away. + Tag.objects.select_for_update().get(pk=tag_id) + # Re-read under the Tag lock: a concurrent delete may have won; DoesNotExist propagates to the caller. + criterion = CompetencyCriterion.objects.select_related("group", "object_tag").get(pk=criterion_id) + if _criterion_has_learner_progress(criterion_id): + _archive_criterion(criterion, user) + return CriterionDeletionResult(id=criterion_id, archived=True) + group = criterion.group + object_tag = criterion.object_tag + _set_history_user(criterion, user) + criterion.delete() + # Only this criterion's own ObjectTag is a candidate, and only while no other criterion uses it. + if not CompetencyCriterion.objects.filter(object_tag_id=object_tag.id).exists(): + ObjectTag.objects.filter(pk=object_tag.id).delete() + _cascade_remove_from_group(group, user) + return CriterionDeletionResult(id=criterion_id, archived=False) diff --git a/src/openedx_learning/applets/cbe/data.py b/src/openedx_learning/applets/cbe/data.py index a985c6eba..cc01fc7c0 100644 --- a/src/openedx_learning/applets/cbe/data.py +++ b/src/openedx_learning/applets/cbe/data.py @@ -36,3 +36,16 @@ def __post_init__(self) -> None: # NaN and Infinity, actually raise the documented ValueError. if not self.fraction.is_finite() or not 0 <= self.fraction <= 1: raise ValueError(f"fraction must be finite and between 0 and 1 inclusive, not {self.fraction!r}") + + +@dataclass(frozen=True, kw_only=True, slots=True) +class CriterionDeletionResult: + """ + The outcome of removing a CompetencyCriterion: whether its row was hard-deleted or archived. + + UNSTABLE: the criteria-authoring family is incomplete, so this shape may change without a + deprecation cycle. + """ + + id: int + archived: bool diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py index 7fbe3298e..ffc5b7ac4 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -83,14 +83,15 @@ class CompetencyCriterionSerializer(serializers.ModelSerializer): ) rule_profile_id = serializers.IntegerField(required=False, allow_null=True) object_tag_id = serializers.IntegerField(read_only=True) + archived = serializers.BooleanField(read_only=True) class Meta: model = CompetencyCriterion fields = [ "id", "object_id", "group_id", "logic_operator", - "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", + "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", "archived", ] - read_only_fields = ["id", "object_tag_id"] + read_only_fields = ["id", "object_tag_id", "archived"] def validate_object_id(self, value: str) -> str: """Reject an object_id that isn't a parseable usage key.""" diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py index 0570fdd5a..f4acf99cf 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -32,6 +32,11 @@ views.CompetencyCriteriaGroupDetailView.as_view(), name="criteria-group-detail", ), + path( + "competencies//criteria//", + views.CompetencyCriterionDeleteView.as_view(), + name="criterion-delete", + ), path( "competencies//blocked-courses/", views.CompetencyBlockedCoursesView.as_view(), diff --git a/src/openedx_learning/applets/cbe/rest_api/v1/views.py b/src/openedx_learning/applets/cbe/rest_api/v1/views.py index abe3aa15a..6430077b3 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -4,6 +4,7 @@ from __future__ import annotations from django.core.exceptions import ValidationError as DjangoValidationError +from django.db import IntegrityError from django.db.models import QuerySet from django.shortcuts import get_object_or_404 from django.utils.translation import gettext_lazy as _ @@ -26,6 +27,7 @@ CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, + delete_or_archive_competency_criterion, get_blocked_courses, get_competency_criteria_tree, get_competency_rule_profiles, @@ -167,7 +169,7 @@ def patch(self, request, group_id: int): rule_payload_override=data.get("rule_payload_override"), user=request.user, ) - except (CompetencyCriteriaGroup.DoesNotExist, CompetencyCriterion.DoesNotExist) as exc: + except (CompetencyCriteriaGroup.DoesNotExist, CompetencyCriterion.DoesNotExist, Tag.DoesNotExist) as exc: raise NotFound(str(exc)) from exc except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc @@ -269,7 +271,7 @@ def patch(self, request, tag_id: int, group_id: int): group = update_competency_criteria_group( group.id, logic_operator=serializer.validated_data["logic_operator"], user=request.user, ) - except CompetencyCriteriaGroup.DoesNotExist as exc: + except (CompetencyCriteriaGroup.DoesNotExist, Tag.DoesNotExist) as exc: raise NotFound(str(exc)) from exc except DjangoValidationError as exc: raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc @@ -278,6 +280,41 @@ def patch(self, request, tag_id: int, group_id: int): return Response(CompetencyCriteriaGroupSerializer(group).data, status=status.HTTP_200_OK) +class CompetencyCriterionDeleteView(generics.GenericAPIView): + """ + DELETE-only. Remove a CompetencyCriterion: hard-delete, or archive if learner status exists. + + Does not call self.check_object_permissions(): the public API function checks + oel_tagging.can_tag_object itself and raises Django's PermissionDenied, which becomes a 403. + """ + + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated] + + def delete(self, request, *args, **kwargs): + """ + Resolve the criterion by the `criterion_id` and `tag_id` in the URL, then delegate to the public API. + + Archived rows still resolve, so a repeat DELETE on one returns 200. + """ + criterion = get_object_or_404( + CompetencyCriterion, pk=self.kwargs["criterion_id"], group__tag_id=self.kwargs["tag_id"], + ) + try: + result = delete_or_archive_competency_criterion(criterion.id, request.user) + except (CompetencyCriterion.DoesNotExist, Tag.DoesNotExist) as exc: + # The criterion, or its competency tag, was removed between the lookup above and the API call. + raise NotFound() from exc + except DjangoValidationError as exc: + raise DRFValidationError(exc.message_dict if hasattr(exc, "message_dict") else exc.messages) from exc + except IntegrityError as exc: + # ProtectedError is a subclass. A learner's grade write won the race, and a retry archives the criterion. + raise CompetencyCriteriaConflict( + _("A learner's progress was recorded against this criterion during the delete. Retry the DELETE.") + ) from exc + return Response({"id": result.id, "archived": result.archived}, status=status.HTTP_200_OK) + + class CompetencyBlockedCoursesView(generics.GenericAPIView): """ GET-only. List the courses where a competency can't get a new criterion, paginated. diff --git a/tests/openedx_learning/applets/cbe/conftest.py b/tests/openedx_learning/applets/cbe/conftest.py index 90f222927..956ed31c9 100644 --- a/tests/openedx_learning/applets/cbe/conftest.py +++ b/tests/openedx_learning/applets/cbe/conftest.py @@ -1,4 +1,5 @@ """Shared fixtures for the CBE test modules: schema, deletion, tree-integration, status, and REST tests.""" +from collections.abc import Callable from datetime import datetime, timezone import pytest @@ -126,3 +127,35 @@ def _staff_client(staff_user) -> APIClient: client = APIClient() client.force_authenticate(user=staff_user) return client + + +@pytest.fixture(name="run_when_tag_lock_is_taken") +def _run_when_tag_lock_is_taken(monkeypatch: pytest.MonkeyPatch) -> Callable[[Callable[[], None]], None]: + """ + Return a function that makes the API run a callable as it takes the Tag row lock. + + The callable runs once, before the API reads the Tag. It stands in for a transaction that commits + while the API waits for the lock, so a read the API makes after taking the lock sees the change and + a read it made earlier doesn't. + """ + + def _install(side_effect: Callable[[], None]) -> None: + original_select_for_update = Tag.objects.select_for_update + fired: list[bool] = [] + + def _select_for_update(*args, **kwargs): + queryset = original_select_for_update(*args, **kwargs) + original_get = queryset.get + + def _get(*get_args, **get_kwargs): + if not fired: + fired.append(True) + side_effect() + return original_get(*get_args, **get_kwargs) + + queryset.get = _get + return queryset + + monkeypatch.setattr(Tag.objects, "select_for_update", _select_for_update) + + return _install diff --git a/tests/openedx_learning/applets/cbe/test_api.py b/tests/openedx_learning/applets/cbe/test_api.py index c7a2f7cea..78671a64f 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1023,6 +1023,64 @@ def _fail_on_second_save(self, *args, **kwargs) -> None: assert snapshot(batch) == before +def test_bulk_update_saves_in_primary_key_order_and_returns_request_order( + monkeypatch: pytest.MonkeyPatch, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, + user, +) -> None: + """Each save takes a row lock, so the saves run in ascending pk order however the ids were requested.""" + c1, c2 = batch + c3 = make_criterion(leaf, "p3", rule_profile=default_rule_profile) + original_save = CompetencyCriterion.save + saves: list[int] = [] + + def _record_save(self, *args, **kwargs) -> None: + saves.append(self.pk) + original_save(self, *args, **kwargs) + + monkeypatch.setattr(CompetencyCriterion, "save", _record_save) + + result = bulk_update_competency_criteria([c3.id, c1.id, c2.id], leaf.id, user=user, **NEW_RULE) + + assert saves == [c1.id, c2.id, c3.id] + assert [criterion.id for criterion in result] == [c3.id, c1.id, c2.id] + + +def test_bulk_update_checks_archived_state_under_the_tag_lock( + run_when_tag_lock_is_taken, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, +) -> None: + """A group archived while the update waits for the Tag lock refuses the batch, so the check reads after the lock.""" + run_when_tag_lock_is_taken(lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True)) + before = snapshot(batch) + + with pytest.raises(CompetencyCriterionArchivedError, match=f"group {leaf.id}"): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert snapshot(batch) == before + + +def test_bulk_update_reads_the_criteria_under_the_tag_lock( + run_when_tag_lock_is_taken, + batch: tuple[CompetencyCriterion, CompetencyCriterion], + leaf: CompetencyCriteriaGroup, + user, +) -> None: + """A criterion archived while the update waits for the Tag lock refuses the batch, so the criteria read after it.""" + archived = batch[1] + run_when_tag_lock_is_taken(lambda: CompetencyCriterion.objects.filter(pk=archived.pk).update(archived=True)) + before = snapshot(batch) + + with pytest.raises(CompetencyCriterionArchivedError, match=rf"criteria cannot be edited: {archived.id}\."): + bulk_update_competency_criteria([c.id for c in batch], leaf.id, user=user, **NEW_RULE) + + assert snapshot(batch) == before + + # ============================================================================================== # get_competency_criteria_tree # ============================================================================================== @@ -1957,6 +2015,20 @@ def _fail(*args, **kwargs): assert criterion.group_id == group.id +def test_create_competency_criterion_takes_the_tag_lock_itself( + run_when_tag_lock_is_taken, tag: Tag, course_run: CourseRun, +) -> None: + """ + A caller outside associate_competency_criterion() is still serialized with a delete of the competency's + criteria: the lock is taken before anything else, so a tag deleted while the call waits for it is seen. + """ + group = _create_leaf_group(tag, course_run) + run_when_tag_lock_is_taken(lambda: Tag.objects.filter(pk=tag.pk).delete()) + + with pytest.raises(Tag.DoesNotExist): + cbe_api.create_competency_criterion(group, usage_key(course_run, "p1"), course_run=course_run) + + def test_associate_competency_criterion_creates_the_course_runs_cbe_row( tag: Tag, course_run: CourseRun, ) -> None: @@ -2242,6 +2314,19 @@ def test_update_raises_does_not_exist_for_an_unknown_group(user) -> None: update_competency_criteria_group(999999, logic_operator=LogicOperator.AND, user=user) +def test_group_update_re_reads_the_group_under_the_tag_lock( + run_when_tag_lock_is_taken, leaf: CompetencyCriteriaGroup, user, +) -> None: + """A group archived while the update waits for the Tag lock is refused, not written onto.""" + run_when_tag_lock_is_taken(lambda: CompetencyCriteriaGroup.objects.filter(pk=leaf.pk).update(archived=True)) + + with pytest.raises(CompetencyCriteriaGroupArchivedError): + update_competency_criteria_group(leaf.id, logic_operator=LogicOperator.AND, user=user) + + leaf.refresh_from_db() + assert leaf.logic_operator == LogicOperator.OR + + @pytest.mark.parametrize("level", ["course-level", "leaf"]) def test_update_checks_permission_against_the_groups_course( # pylint: disable=too-many-positional-arguments level: str, diff --git a/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py new file mode 100644 index 000000000..62b0224bd --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py @@ -0,0 +1,468 @@ +""" +Two-connection tests for the row locks that criterion authoring writers take. + +Plain SQLite has no row locks, so these run only where ``select_for_update()`` is real (MySQL in CI). +Each test runs the delete in one thread and pauses it inside its transaction, then lets a second +connection act while the first still holds its locks. +""" +import threading +from collections.abc import Callable, Iterator +from datetime import datetime, timezone +from decimal import Decimal +from typing import Any + +import pytest +import rules +from django.apps import apps +from django.db import DatabaseError, IntegrityError, connection, transaction +from rules.permissions import permissions as rule_permissions + +from openedx_catalog.models import CourseRun +from openedx_learning.api import ( + CompetencyCriteriaGroupArchivedError, + CompetencyCriterionArchivedError, + GradedObjectScore, + associate_competency_criterion, + bulk_update_competency_criteria, + delete_or_archive_competency_criterion, + record_graded_object_statuses, + update_competency_criteria_group, +) +from openedx_learning.applets.cbe import api as cbe_api +from openedx_learning.applets.cbe.api import _cascade_remove_from_group, _create_leaf_group +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + LogicOperator, + MasteryStatus, + RuleType, + StudentCompetencyCriterionStatus, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = [ + pytest.mark.django_db(transaction=True), + pytest.mark.skipif(not connection.features.has_select_for_update, reason="needs real row locks"), + pytest.mark.usefixtures("migration_seed_rows"), +] + +CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" +NEW_RULE: dict[str, Any] = { + "rule_type_override": RuleType.GRADE, + "rule_payload_override": {"op": "gte", "value": 0.9, "scale": "percent"}, +} +# How long a test waits for a thread to reach a pause point or finish; far above any healthy run. +WAIT_SECONDS = 30 +# A worker blocked on a lock fails after this many seconds instead of MySQL's default fifty. +LOCK_WAIT_SECONDS = 4 + + +@pytest.fixture(autouse=True) +def _allow_object_level_tagging() -> Iterator[None]: + """Let any user tag any object; restore the real rule afterwards.""" + original = rule_permissions[CHANGE_OBJECTTAG_OBJECTID] + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: True) + yield + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, original) + + +class Worker: + """ + A function running in its own thread, and so on its own database connection. + + The worker's exception is kept and re-raised by ``result()``, so a failure in the thread fails + the test instead of vanishing. + """ + + def __init__(self, func: Callable[..., Any], *args: Any, **kwargs: Any) -> None: + self._func, self._args, self._kwargs = func, args, kwargs + self._value: Any = None + self._error: BaseException | None = None + self.thread = threading.Thread(target=self._run, daemon=True) + self.thread.start() + + def _run(self) -> None: + """Thread body: cap lock waits, run the function, keep its outcome, and close the connection.""" + try: + with connection.cursor() as cursor: + cursor.execute(f"SET SESSION innodb_lock_wait_timeout = {LOCK_WAIT_SECONDS}") + self._value = self._func(*self._args, **self._kwargs) + except BaseException as error: + self._error = error + finally: + # The table flush after the test waits on any transaction this connection left open. + connection.close() + + def is_blocked(self) -> bool: + """Return whether the worker is still running after a short wait, i.e. it is waiting on a lock.""" + self.thread.join(timeout=1.0) + return self.thread.is_alive() + + def result(self) -> Any: + """Wait for the worker, then return its value or re-raise its exception.""" + self.thread.join(timeout=WAIT_SECONDS) + assert not self.thread.is_alive(), "worker is still running" + if self._error is not None: + raise self._error + return self._value + + +@pytest.fixture(name="start") +def _start() -> Iterator[Callable[..., Worker]]: + """Return a function that starts a Worker, and wait for every Worker it started once the test ends.""" + workers: list[Worker] = [] + + def _start_worker(func: Callable[..., Any], *args: Any, **kwargs: Any) -> Worker: + workers.append(Worker(func, *args, **kwargs)) + return workers[-1] + + yield _start_worker + for worker in workers: + worker.thread.join(timeout=WAIT_SECONDS) + + +class PausePoint: + """Two events that stop the first call of a wrapped function until the test lets it go.""" + + def __init__(self) -> None: + self.reached = threading.Event() + self.release = threading.Event() + self._tripped = False + + def wrap(self, original: Callable[..., Any]) -> Callable[..., Any]: + """Return ``original`` wrapped to signal ``reached`` and wait for ``release`` on its first call only.""" + + def _wrapper(*args: Any, **kwargs: Any) -> Any: + if not self._tripped: + self._tripped = True + self.reached.set() + if not self.release.wait(WAIT_SECONDS): + raise RuntimeError("pause point was never released") + return original(*args, **kwargs) + + return _wrapper + + def wait_until_reached(self) -> None: + """Wait for the paused thread to arrive.""" + assert self.reached.wait(WAIT_SECONDS), "the delete never reached its pause point" + + +def pause_before_the_walk(monkeypatch: pytest.MonkeyPatch) -> PausePoint: + """ + Pause the delete after it writes the criterion and before it walks up the groups. + + The walk calls itself at every level, so only its first call pauses. + """ + pause = PausePoint() + monkeypatch.setattr(cbe_api, "_cascade_remove_from_group", pause.wrap(_cascade_remove_from_group)) + return pause + + +def pause_after_the_walk(monkeypatch: pytest.MonkeyPatch) -> PausePoint: + """Pause the delete after the walk, when it builds its result, which is the last step inside its transaction.""" + pause = PausePoint() + monkeypatch.setattr(cbe_api, "CriterionDeletionResult", pause.wrap(cbe_api.CriterionDeletionResult)) + return pause + + +def tag_row_is_locked(tag: Tag) -> bool: + """ + Return whether another connection holds the lock on ``tag``'s row. + + ``nowait`` makes MySQL fail at once instead of waiting, so the answer involves no timing. + """ + try: + with transaction.atomic(): + Tag.objects.select_for_update(nowait=True).get(pk=tag.pk) + except DatabaseError: + return True + return False + + +@pytest.fixture(name="sibling_object_id") +def _sibling_object_id(course_run: CourseRun) -> str: + """The usage key of a second sequential in the same course as the criterion in ``leaf_and_criterion``.""" + key = course_run.course_key + assert key is not None + return f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@s2" + + +@pytest.fixture(name="learner_progress", params=[False, True], ids=["hard delete", "archive"]) +def _learner_progress(request: pytest.FixtureRequest) -> bool: + """Whether the criterion has a learner status row, which makes a delete archive it instead of removing it.""" + return request.param + + +@pytest.fixture(name="leaf_and_criterion") +def _leaf_and_criterion( + learner_progress: bool, + tag: Tag, + course_run: CourseRun, + default_rule_profile: CompetencyRuleProfile, + user, +) -> tuple[CompetencyCriteriaGroup, CompetencyCriterion]: + """A leaf group holding one criterion, with a learner status row on it if ``learner_progress``.""" + leaf = _create_leaf_group(tag, course_run) + key = course_run.course_key + assert key is not None + object_tag = ObjectTag.objects.create( + object_id=f"block-v1:{key.org}+{key.course}+{key.run}+type@sequential+block@s1", + taxonomy=tag.taxonomy, + tag=tag, + ) + criterion = CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=default_rule_profile) + if learner_progress: + now = datetime.now(timezone.utc) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + return leaf, criterion + + +def test_delete_and_bulk_update_do_not_deadlock( + learner_progress: bool, + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + user, +) -> None: + """ + A bulk update of a criterion that is being deleted waits for the delete, then fails cleanly. + + Neither side may be killed by the database as a deadlock victim. After a hard delete the update finds + nothing to edit; after an archive it finds the criterion archived. + """ + leaf, criterion = leaf_and_criterion + pause = pause_before_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + bulk_update = start(bulk_update_competency_criteria, [criterion.id], leaf.id, user=user, **NEW_RULE) + assert bulk_update.is_blocked() + finally: + pause.release.set() + + # An OperationalError (MySQL error 1213, deadlock) raised by either worker is re-raised here. + result = delete.result() + expected: tuple[type[Exception], ...] + if learner_progress: + expected = (CompetencyCriterionArchivedError,) + else: + expected = (CompetencyCriterion.DoesNotExist, CompetencyCriteriaGroup.DoesNotExist) + with pytest.raises(expected): + bulk_update.result() + assert result.archived is learner_progress + + +def test_patch_does_not_write_onto_an_archived_or_deleted_group( + learner_progress: bool, + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + user, +) -> None: + """ + A PATCH of a leaf's logic operator that races the delete of its only criterion changes nothing. + + The delete archives or deletes the leaf. The PATCH waits for it, then reports the group as archived + or missing instead of writing onto it. + """ + leaf, criterion = leaf_and_criterion + assert leaf.logic_operator == LogicOperator.OR + pause = pause_after_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + patch = start(update_competency_criteria_group, leaf.id, logic_operator=LogicOperator.AND, user=user) + assert patch.is_blocked() + finally: + pause.release.set() + + assert delete.result().archived is learner_progress + expected = CompetencyCriteriaGroupArchivedError if learner_progress else CompetencyCriteriaGroup.DoesNotExist + with pytest.raises(expected): + patch.result() + historical_group = apps.get_model("openedx_learning", "HistoricalCompetencyCriteriaGroup") + assert not historical_group.objects.filter(id=leaf.id, logic_operator=LogicOperator.AND).exists() + + +def test_tag_lock_is_held_for_the_whole_delete_request( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + tag: Tag, + user, +) -> None: + """The competency's Tag row stays locked from the delete's first write until its transaction commits.""" + _, criterion = leaf_and_criterion + pause = pause_before_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + assert tag_row_is_locked(tag) + finally: + pause.release.set() + + delete.result() + assert not tag_row_is_locked(tag) + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_create_for_the_same_competency_waits_and_its_criterion_survives( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + sibling_object_id: str, + start: Callable[..., Worker], + user, +) -> None: + """ + A create for the competency whose only criterion is being deleted waits, then builds a fresh tree. + + The delete removes the leaf, course-level group and root. The create, which would otherwise reuse + those containers while the delete is removing them, runs afterwards and makes its own. + """ + leaf, criterion = leaf_and_criterion + course_level_group_id = leaf.parent_id + assert course_level_group_id is not None + root_id = CompetencyCriteriaGroup.objects.get(pk=course_level_group_id).parent_id + pause = pause_before_the_walk(monkeypatch) + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + create = start(associate_competency_criterion, criterion.object_tag.tag_id, sibling_object_id) + assert create.is_blocked() + finally: + pause.release.set() + + delete.result() + new_criterion = create.result() + new_leaf = new_criterion.group + assert new_leaf.id != leaf.id + new_course_level_group = new_leaf.parent + assert new_course_level_group is not None + assert new_course_level_group.id != course_level_group_id + new_root = new_course_level_group.parent + assert new_root is not None + assert new_root.id != root_id + assert new_root.parent_id is None + assert CompetencyCriterion.objects.filter(pk=new_criterion.pk).exists() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, course_level_group_id, root_id]).exists() + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_two_deletes_of_sibling_criteria_leave_no_empty_leaf( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + sibling_object_id: str, + start: Callable[..., Worker], + user, +) -> None: + """ + Two criteria in one leaf, deleted at the same time, leave neither the leaf nor anything above it. + + Without the Tag lock each delete would still see the other's criterion in the leaf and stop the walk + there, leaving an empty leaf behind. + """ + leaf, first = leaf_and_criterion + course_level_group_id = leaf.parent_id + assert course_level_group_id is not None + root_id = CompetencyCriteriaGroup.objects.get(pk=course_level_group_id).parent_id + object_tag = ObjectTag.objects.create( + object_id=sibling_object_id, taxonomy_id=first.object_tag.taxonomy_id, tag_id=first.object_tag.tag_id, + ) + second = CompetencyCriterion.objects.create(group=leaf, object_tag=object_tag, rule_profile=first.rule_profile) + pause = pause_before_the_walk(monkeypatch) + try: + first_delete = start(delete_or_archive_competency_criterion, first.id, user) + pause.wait_until_reached() + second_delete = start(delete_or_archive_competency_criterion, second.id, user) + assert second_delete.is_blocked() + finally: + pause.release.set() + + assert first_delete.result().archived is False + assert second_delete.result().archived is False + assert not CompetencyCriterion.objects.filter(pk__in=[first.id, second.id]).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, course_level_group_id, root_id]).exists() + + +def _grade_then_wait( + user_id: int, object_id: str, reached: threading.Event, release: threading.Event, +) -> None: + """Record a passing grade for ``object_id`` and hold the grade's transaction open until ``release`` is set.""" + with transaction.atomic(): + record_graded_object_statuses( + user_id=user_id, scores=[GradedObjectScore(object_id=object_id, fraction=Decimal("0.9"))], + ) + reached.set() + if not release.wait(WAIT_SECONDS): + raise RuntimeError("the grade transaction was never released") + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_grade_write_and_hard_delete_fail_one_side_never_silently_grade_first( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + start: Callable[..., Worker], + user, +) -> None: + """ + A hard delete that races a grade write already holding the criterion fails, usually with ``IntegrityError``. + + The grade's status row holds a lock on the criterion, so the delete waits for the grade to commit and + then fails the foreign-key check. If the grade transaction also writes a ``StudentCompetencyStatus`` + row, which takes a shared lock on the Tag, the delete can instead lose with MySQL deadlock error 1213. + Nothing is lost either way: both rows survive, and a second delete archives the criterion. + """ + _, criterion = leaf_and_criterion + grade_reached, grade_release = threading.Event(), threading.Event() + try: + grade = start(_grade_then_wait, user.id, criterion.object_tag.object_id, grade_reached, grade_release) + assert grade_reached.wait(WAIT_SECONDS), "the grade write never recorded its status" + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + assert delete.is_blocked() + finally: + grade_release.set() + + grade.result() + with pytest.raises(IntegrityError): + delete.result() + assert CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert StudentCompetencyCriterionStatus.objects.filter(criterion=criterion, user=user).exists() + assert delete_or_archive_competency_criterion(criterion.id, user).archived is True + + +@pytest.mark.parametrize("learner_progress", [False], ids=["hard delete"]) +def test_grade_write_and_hard_delete_fail_one_side_never_silently_delete_first( + leaf_and_criterion: tuple[CompetencyCriteriaGroup, CompetencyCriterion], + monkeypatch: pytest.MonkeyPatch, + start: Callable[..., Worker], + user, +) -> None: + """ + A grade write that races a hard delete already holding the criterion fails, usually with ``IntegrityError``. + + The grade's status insert waits for the delete to commit and then fails the foreign-key check. If the + grade transaction also writes a ``StudentCompetencyStatus`` row, which takes a shared lock on the Tag, + the grade can instead lose with MySQL deadlock error 1213. The criterion is gone and no status row was + written. + """ + _, criterion = leaf_and_criterion + pause = pause_before_the_walk(monkeypatch) + grade_reached, grade_release = threading.Event(), threading.Event() + # Already released: the grade's transaction ends as soon as its insert returns, so only the delete holds locks. + grade_release.set() + try: + delete = start(delete_or_archive_competency_criterion, criterion.id, user) + pause.wait_until_reached() + grade = start(_grade_then_wait, user.id, criterion.object_tag.object_id, grade_reached, grade_release) + assert grade.is_blocked() + finally: + pause.release.set() + + assert delete.result().archived is False + with pytest.raises(IntegrityError): + grade.result() + assert not CompetencyCriterion.objects.filter(pk=criterion.pk).exists() + assert not StudentCompetencyCriterionStatus.objects.filter(user=user).exists() diff --git a/tests/openedx_learning/applets/cbe/test_criterion_removal.py b/tests/openedx_learning/applets/cbe/test_criterion_removal.py new file mode 100644 index 000000000..586dc77e1 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -0,0 +1,559 @@ +"""Tests for delete_or_archive_competency_criterion() and its ancestor-cascade helper, _cascade_remove_from_group().""" +from collections.abc import Iterator +from datetime import datetime + +import pytest +import rules +from django.apps import apps +from django.contrib.auth import get_user_model +from django.core.exceptions import PermissionDenied +from rules.permissions import permissions as rule_permissions + +from openedx_learning.api import delete_or_archive_competency_criterion +from openedx_learning.models import ( + CompetencyCriteriaGroup, + CompetencyCriterion, + CompetencyRuleProfile, + CompetencyTaxonomy, + MasteryStatus, + StudentCompetencyCriteriaGroupStatus, + StudentCompetencyCriterionStatus, +) +from openedx_tagging.models import ObjectTag, Tag + +pytestmark = pytest.mark.django_db + +CHANGE_OBJECTTAG_OBJECTID = "oel_tagging.change_objecttag_objectid" + + +@pytest.fixture(autouse=True) +def _allow_object_level_tagging() -> Iterator[None]: + """ + Let any user tag any object; restore the real rule afterwards. + + openedx_tagging denies change_objecttag_objectid to everyone and leaves the real Studio-role + check to openedx-platform, so these tests substitute a permissive one. + """ + original = rule_permissions[CHANGE_OBJECTTAG_OBJECTID] + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: True) + yield + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, original) + + +def _make_object_tag(competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_id: str) -> ObjectTag: + """Create an ObjectTag for `object_id`, distinct from the conftest `object_tag` fixture's own row.""" + return ObjectTag.objects.create(object_id=object_id, taxonomy=competency_taxonomy, tag=tag) + + +def _make_criterion( + group: CompetencyCriteriaGroup, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile +) -> CompetencyCriterion: + """Create a CompetencyCriterion under `group`, on the system-default rule profile.""" + return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + + +# ================================================================================================= +# Hard-delete vs. archive, on the criterion itself +# ================================================================================================= + + +def test_hard_delete_with_no_status_removes_the_row_and_its_object_tag( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Deleting a criterion with no learner status hard-deletes it and drops its now-unreferenced + object_tag, returning archived=False. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.id == criterion.id + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + + +@pytest.mark.parametrize("shared_object_tag", [False, True], ids=["own object_tag", "shared object_tag"]) +def test_archive_path_leaves_the_row_and_its_object_tag_in_place( + *, + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, + shared_object_tag: bool, +) -> None: + """ + Deleting a criterion that a learner status row already references archives it instead of + hard-deleting it: the row and its object_tag both survive, and archived=True comes back. Another + criterion on the same object_tag is left in place too. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + other = None + if shared_object_tag: + other_group = CompetencyCriteriaGroup.objects.create(tag=tag, parent=group) + other = _make_criterion(other_group, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + assert ObjectTag.objects.filter(id=object_tag.id).exists() + if other is not None: + other.refresh_from_db() + assert other.archived is False + + +def _latest_history_user(model_name: str, pk: int): + """Return the user recorded on the newest historical row of the ``model_name`` row ``pk``.""" + historical = apps.get_model("openedx_learning", model_name) + return historical.objects.filter(id=pk).order_by("-history_date", "-history_id").first().history_user + + +def _history_count(model_name: str, pk: int) -> int: + """Return how many historical rows exist for the ``model_name`` row ``pk``.""" + return apps.get_model("openedx_learning", model_name).objects.filter(id=pk).count() + + +def test_repeat_delete_on_an_archived_criterion_writes_no_history( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A second delete of an already-archived criterion returns archived=True and leaves the + criterion and its groups untouched, so no new historical row is written for any of them. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + delete_or_archive_competency_criterion(criterion.id, user) + counts = ( + _history_count("HistoricalCompetencyCriterion", criterion.id), + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.id == criterion.id + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + assert counts == ( + _history_count("HistoricalCompetencyCriterion", criterion.id), + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + +@pytest.mark.parametrize("has_status_row", [True, False], ids=["archive", "hard delete"]) +def test_removal_records_the_acting_user_in_history( + has_status_row: bool, + *, + user, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, + now: datetime, +) -> None: + """ + Archiving a criterion, or hard-deleting one with no status row, records the acting user on the + newest historical row of it and of each group the cascade archived or deleted. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + if has_status_row: + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert _latest_history_user("HistoricalCompetencyCriterion", criterion.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", leaf.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user + + +def test_shared_object_tag_is_untouched_while_another_criterion_still_references_it( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Two criteria, in two different groups, referencing the same object_tag (a deliberate second + association per ADR-0002): hard-deleting one leaves the other, and the shared object_tag, intact. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + group_a = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + group_b = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion_a = _make_criterion(group_a, object_tag, default_rule_profile) + criterion_b = _make_criterion(group_b, object_tag, default_rule_profile) + + delete_or_archive_competency_criterion(criterion_a.id, user) + + assert not CompetencyCriterion.objects.filter(id=criterion_a.id).exists() + assert CompetencyCriterion.objects.filter(id=criterion_b.id).exists() + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_hard_delete_removes_only_the_criterions_own_object_tag( + user, + competency_taxonomy: CompetencyTaxonomy, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A hard delete removes the criterion's own ObjectTag, now referenced by nothing. An ObjectTag for + another competency on the same object stays with its criterion, so the delete picks the ObjectTag + by its primary key and not by the object it is on. + """ + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Reading Poetry") + other_object_tag = _make_object_tag(competency_taxonomy, other_tag, object_tag.object_id) + criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) + other_criterion = _make_criterion( + CompetencyCriteriaGroup.objects.create(tag=other_tag), other_object_tag, default_rule_profile + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + assert not ObjectTag.objects.filter(id=object_tag.id).exists() + assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() + assert ObjectTag.objects.filter(id=other_object_tag.id).exists() + + +# ================================================================================================= +# The ancestor cascade, on hard-delete +# ================================================================================================= + + +def test_single_child_leaf_group_cascade_deletes_while_a_sibling_keeps_the_root_alive( + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting a criterion that was its leaf group's only child also deletes that now-empty + group, but the cascade stops there: a sibling group under the same root, with its own live + criterion, keeps the root from being touched. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + sibling = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + sibling_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-sibling") + _make_criterion(sibling, sibling_object_tag, default_rule_profile) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=sibling.id).exists() + root.refresh_from_db() + assert root.archived is False + + +def test_multilevel_cascade_fully_collapses_the_root_to_leaf_chain( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting the only criterion in a root -> mid -> leaf chain, with nothing else anywhere in + the tree, collapses every group in the chain, one level at a time, all the way to the root. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + mid = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=mid) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=mid.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + + +def test_cascade_stops_at_an_ancestor_with_a_surviving_sibling_group( + user, competency_taxonomy: CompetencyTaxonomy, tag: Tag, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A deeper chain (root -> branch -> leaf) collapses branch and leaf when the leaf's only + criterion is hard-deleted, but stops at root: a second branch under root, with its own live + criterion, keeps root from being touched. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + branch = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=branch) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + other_branch = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + other_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-other-branch") + other_criterion = _make_criterion(other_branch, other_object_tag, default_rule_profile) + + delete_or_archive_competency_criterion(criterion.id, user) + + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(id=branch.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=root.id).exists() + assert CompetencyCriteriaGroup.objects.filter(id=other_branch.id).exists() + assert CompetencyCriterion.objects.filter(id=other_criterion.id).exists() + root.refresh_from_db() + assert root.archived is False + + +@pytest.mark.parametrize("progress_on", ["leaf group", "root group"]) +def test_group_with_its_own_status_is_archived_not_deleted_at_zero_remaining_children( + progress_on: str, user, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A group carrying its own StudentCompetencyCriteriaGroupStatus is archived, not hard-deleted, + once its last child is gone, whether that group is the leaf or the root above it (depth 2). + The criterion itself, with no status of its own, is still hard-deleted, and an archive + cascades upward as "archive", so every ancestor of an archived group is archived too. + """ + assert object_tag.tag is not None + tag = object_tag.tag + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=leaf if progress_on == "leaf group" else root, + status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + root.refresh_from_db() + assert root.archived is True + if progress_on == "leaf group": + leaf.refresh_from_db() + assert leaf.archived is True + else: + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + + +def test_hard_delete_beside_an_archived_criterion_archives_the_group_and_its_parent( + user, competency_taxonomy: CompetencyTaxonomy, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + A leaf holding one archived criterion (it has learner progress) and one live criterion: hard-deleting + the live one removes it, and the leaf, left with only archived children, is archived with its parent. + """ + assert object_tag.tag is not None + tag = object_tag.tag + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + live = _make_criterion(leaf, object_tag, default_rule_profile) + archived_object_tag = _make_object_tag(competency_taxonomy, tag, "obj-archived") + archived = _make_criterion(leaf, archived_object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=archived, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + delete_or_archive_competency_criterion(archived.id, user) + + result = delete_or_archive_competency_criterion(live.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(id=live.id).exists() + leaf.refresh_from_db() + root.refresh_from_db() + assert leaf.archived is True + assert root.archived is True + + +# ================================================================================================= +# The ancestor cascade, on archive +# ================================================================================================= + + +def test_archiving_a_groups_only_criterion_archives_the_group_and_cascades_upward( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Archiving a criterion that was its leaf group's only live child archives that group too, and + the cascade continues upward through an ancestor with no other active content. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is True + criterion.refresh_from_db() + assert criterion.archived is True + leaf.refresh_from_db() + root.refresh_from_db() + assert leaf.archived is True + assert root.archived is True + + +# ================================================================================================= +# Permission +# ================================================================================================= + + +def test_a_user_without_permission_is_refused_and_nothing_changes( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A user who may not tag the criterion's object is refused, and the criterion, group and ObjectTag survive.""" + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(group, object_tag, default_rule_profile) + rules.set_perm(CHANGE_OBJECTTAG_OBJECTID, lambda _user, _object_id: False) + + with pytest.raises(PermissionDenied): + delete_or_archive_competency_criterion(criterion.id, user) + + criterion.refresh_from_db() + assert criterion.archived is False + assert CompetencyCriteriaGroup.objects.filter(id=group.id).exists() + assert ObjectTag.objects.filter(id=object_tag.id).exists() + + +def test_delete_re_reads_the_criterion_under_the_tag_lock( + run_when_tag_lock_is_taken, + user, + tag: Tag, + object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A criterion removed while the delete waits for the Tag lock raises DoesNotExist, so the delete + reads the criterion again after the lock rather than trusting its first read. + """ + criterion = _make_criterion(CompetencyCriteriaGroup.objects.create(tag=tag), object_tag, default_rule_profile) + # Stands in for a concurrent delete that commits while this one waits for the lock. + run_when_tag_lock_is_taken(lambda: CompetencyCriterion.objects.filter(pk=criterion.pk).delete()) + + with pytest.raises(CompetencyCriterion.DoesNotExist): + delete_or_archive_competency_criterion(criterion.id, user) + + +# ================================================================================================= +# Not found +# ================================================================================================= + + +def test_does_not_exist_for_an_unknown_criterion_id(user) -> None: + """The API layer lets DoesNotExist propagate for an unknown criterion_id so each caller can map it itself.""" + with pytest.raises(CompetencyCriterion.DoesNotExist): + delete_or_archive_competency_criterion(999999999, user) + + +# ================================================================================================= +# The walk stops at archived groups +# ================================================================================================= + + +def test_cascade_stops_at_a_group_that_is_already_archived( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Archiving a criterion whose group is already archived leaves that group and its parent alone, so + neither gets a new historical row: everything above an archived group was handled when it was archived. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, archived=True) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + counts = ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is True + assert CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id], archived=True).count() == 2 + assert counts == ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + +def test_delete_after_the_learner_is_gone_deletes_the_emptied_archived_groups( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """ + Once the learner whose status archived a criterion is deleted, a repeat delete hard-deletes the + criterion, and every archived group it leaves with nothing in it is deleted too: no empty group stays. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + learner = get_user_model().objects.create(username="departing-learner") + StudentCompetencyCriterionStatus.objects.create( + user=learner, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + assert delete_or_archive_competency_criterion(criterion.id, user).archived is True + learner.delete() + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert not CompetencyCriterion.objects.filter(pk=criterion.id).exists() + assert not CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id]).exists() + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", leaf.id) == user + assert _latest_history_user("HistoricalCompetencyCriteriaGroup", root.id) == user + + +def test_delete_leaves_an_archived_group_that_still_holds_an_archived_criterion( + user, tag: Tag, competency_taxonomy: CompetencyTaxonomy, object_tag: ObjectTag, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + A hard delete under an archived group that still holds another criterion leaves that group and its + parent as they are, archived and with no new historical row. + """ + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, archived=True) + kept = _make_criterion(leaf, object_tag, default_rule_profile) + kept.archived = True + kept.save() + other_object_tag = _make_object_tag(competency_taxonomy, tag, "block-v1:Org1+Python100+Fall2026+problem+p2") + deleted = _make_criterion(leaf, other_object_tag, default_rule_profile) + counts = ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + result = delete_or_archive_competency_criterion(deleted.id, user) + + assert result.archived is False + assert CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id], archived=True).count() == 2 + assert counts == ( + _history_count("HistoricalCompetencyCriteriaGroup", leaf.id), + _history_count("HistoricalCompetencyCriteriaGroup", root.id), + ) + + +def test_delete_leaves_an_emptied_archived_group_that_has_its_own_status( + user, tag: Tag, object_tag: ObjectTag, default_rule_profile: CompetencyRuleProfile, now: datetime, +) -> None: + """An archived group left empty by a hard delete stays when a learner group status still references it.""" + root = CompetencyCriteriaGroup.objects.create(tag=tag, archived=True) + leaf = CompetencyCriteriaGroup.objects.create(tag=tag, parent=root, archived=True) + criterion = _make_criterion(leaf, object_tag, default_rule_profile) + # Only a criterion archived by an earlier delete sits under an archived group. + criterion.archived = True + criterion.save() + StudentCompetencyCriteriaGroupStatus.objects.create( + user=user, group=leaf, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + result = delete_or_archive_competency_criterion(criterion.id, user) + + assert result.archived is False + assert CompetencyCriteriaGroup.objects.filter(pk__in=[leaf.id, root.id], archived=True).count() == 2 diff --git a/tests/openedx_learning/applets/cbe/test_views.py b/tests/openedx_learning/applets/cbe/test_views.py index 10f6f5bfc..c71529e64 100644 --- a/tests/openedx_learning/applets/cbe/test_views.py +++ b/tests/openedx_learning/applets/cbe/test_views.py @@ -4,6 +4,7 @@ Fixtures live in this directory's conftest.py. Several scenarios here create rule profile rows directly rather than through a live call, because no create or archive endpoint exists yet. """ +from datetime import datetime from typing import Any, Generator from urllib.parse import urlencode @@ -18,6 +19,7 @@ from rest_framework.test import APIClient, APIRequestFactory from openedx_catalog.models import CourseRun +from openedx_learning.applets.cbe import api as cbe_api from openedx_learning.applets.cbe.api import _create_leaf_group from openedx_learning.applets.cbe.rest_api.paginators import CompetencyBlockedCoursesPagination from openedx_learning.models import ( @@ -26,7 +28,9 @@ CompetencyRuleProfile, CompetencyTaxonomy, LogicOperator, + MasteryStatus, RuleType, + StudentCompetencyCriterionStatus, ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy @@ -77,6 +81,11 @@ def criterion_create_url(tag_id: int) -> str: return reverse("cbe:criterion-create", kwargs={"tag_id": tag_id}) +def criterion_delete_url(tag_id: int, criterion_id: int) -> str: + """Return the delete-criterion endpoint's path for `tag_id`/`criterion_id`, resolved through the URL name.""" + return reverse("cbe:criterion-delete", kwargs={"tag_id": tag_id, "criterion_id": criterion_id}) + + # What migration 0005 seeds the system default with. Asserted verbatim rather than imported, so # that a change to the seed surfaces here as a failing contract instead of passing silently. SEEDED_GRADE_PAYLOAD = {"op": "gte", "value": 0.8, "scale": "percent"} @@ -327,6 +336,7 @@ def test_no_group_no_existing_groups_creates_the_full_hierarchy_and_201s( assert response.data["object_tag_id"] == criterion.object_tag_id assert response.data["group_id"] == criterion.group_id assert response.data["rule_profile_id"] == default_rule_profile.id + assert response.data["archived"] is False def test_logic_operator_provided_is_stored_on_the_new_leaf( @@ -808,7 +818,9 @@ def test_tag_refused_at_the_object_tag_limit_is_400_and_creates_nothing( NEW_PAYLOAD = {"op": "gte", "value": 0.75, "scale": "percent"} NEW_RULE: dict[str, Any] = {"rule_type_override": "Grade", "rule_payload_override": NEW_PAYLOAD} -CRITERION_FIELDS = {"id", "group_id", "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id"} +CRITERION_FIELDS = { + "id", "group_id", "rule_profile_id", "rule_type_override", "rule_payload_override", "object_tag_id", "archived", +} def bulk_update_url(group_id: int) -> str: @@ -2359,3 +2371,231 @@ def test_blocked_courses_ignores_an_archived_claim( assert response.status_code == status.HTTP_200_OK assert response.data["results"] == [] + + +# ============================================================================================== +# CompetencyCriterionDeleteView (#674) +# ============================================================================================== + + +@pytest.fixture(name="criterion") +def _criterion( + group: CompetencyCriteriaGroup, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile +) -> CompetencyCriterion: + """A CompetencyCriterion directly under the root `group`, for the delete-view tests.""" + # The conftest object_tag's object_id isn't a parseable usage key, which _allow_object_level_tagging denies. + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + return CompetencyCriterion.objects.create(group=group, object_tag=object_tag, rule_profile=default_rule_profile) + + +def test_hard_delete_200s_with_archived_false(user_client: APIClient, criterion: CompetencyCriterion) -> None: + """With no learner status on the criterion, the DELETE removes the row and the body says archived=false.""" + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"id": criterion.id, "archived": False} + assert not CompetencyCriterion.objects.filter(id=criterion.id).exists() + + +def test_archive_200s_with_archived_true( + user_client: APIClient, user, criterion: CompetencyCriterion, now: datetime, +) -> None: + """With a learner status on the criterion, the DELETE keeps the row, flags it archived, and says archived=true.""" + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"id": criterion.id, "archived": True} + criterion.refresh_from_db() + assert criterion.archived is True + + +def test_repeat_delete_on_an_archived_criterion_is_idempotent_200( + user_client: APIClient, user, criterion: CompetencyCriterion, now: datetime, +) -> None: + """A second DELETE against an already-archived criterion still 200s with the same body.""" + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"id": criterion.id, "archived": True} + + +@pytest.mark.parametrize("hard_delete_first", [False, True], ids=["never-existed", "already-hard-deleted"]) +def test_delete_of_a_criterion_that_is_not_there_404s( + hard_delete_first: bool, user_client: APIClient, criterion: CompetencyCriterion, +) -> None: + """ + A criterion_id that never existed, or whose row an earlier hard delete removed, 404s from the view's + own lookup. Repeating a hard delete is not idempotent like repeating an archive. + """ + criterion_id = 999999 + if hard_delete_first: + criterion_id = criterion.id + assert user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion_id)).status_code == 200 + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion_id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_cascade_deleted_group_404s_on_subsequent_create( + user_client: APIClient, tag: Tag, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """ + Hard-deleting a group's only criterion cascade-deletes that now-empty leaf group, and a later + create-criterion POST naming that same (now-gone) group_id 404s rather than resolving. + """ + leaf = _create_leaf_group(tag, course_run) + object_tag = ObjectTag.objects.create(object_id=usage_key(course_run, "p1"), taxonomy=tag.taxonomy, tag=tag) + criterion = CompetencyCriterion.objects.create( + group=leaf, object_tag=object_tag, rule_profile=default_rule_profile, + ) + + delete_response = user_client.delete(criterion_delete_url(tag.id, criterion.id)) + assert delete_response.status_code == status.HTTP_200_OK + assert not CompetencyCriteriaGroup.objects.filter(id=leaf.id).exists() + + create_response = user_client.post( + criterion_create_url(tag.id), + {"object_id": usage_key(course_run, "p2"), "group_id": leaf.id}, + format="json", + ) + + assert create_response.status_code == status.HTTP_404_NOT_FOUND + + +def test_tag_id_criterion_id_mismatch_404s( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, criterion: CompetencyCriterion, +) -> None: + """A criterion that exists, but under a different competency tag than the URL names, 404s.""" + other_tag = Tag.objects.create(taxonomy=competency_taxonomy, value="Other Competency") + + response = user_client.delete(criterion_delete_url(other_tag.id, criterion.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +@pytest.mark.parametrize("lacks", ["object tagging access", "taxonomy view access"]) +def test_delete_without_permission_is_403( + *, + lacks: str, user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, tag: Tag, + group: CompetencyCriteriaGroup, course_run: CourseRun, default_rule_profile: CompetencyRuleProfile, +) -> None: + """A caller refused by the object-level tagging rule, or without view access to the taxonomy, gets a 403.""" + block_id = UNAUTHORIZED_MARKER if lacks == "object tagging access" else "p1" + object_tag = ObjectTag.objects.create( + object_id=usage_key(course_run, block_id), taxonomy=competency_taxonomy, tag=tag, + ) + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile, + ) + if lacks == "taxonomy view access": + competency_taxonomy.enabled = False + competency_taxonomy.save() + + response = user_client.delete(criterion_delete_url(tag.id, criterion.id)) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert CompetencyCriterion.objects.filter(id=criterion.id).exists() + + +def test_delete_from_an_unidentified_caller_is_401(api_client: APIClient, criterion: CompetencyCriterion) -> None: + """A caller the system cannot identify is refused before anything is read, and the criterion remains.""" + response = api_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert CompetencyCriterion.objects.filter(id=criterion.id).exists() + + +def test_a_grade_that_lands_during_the_delete_is_a_retryable_409( + monkeypatch: pytest.MonkeyPatch, user_client: APIClient, user, criterion: CompetencyCriterion, now: datetime, +) -> None: + """ + A learner's status row committed after the delete's progress check makes the delete raise ProtectedError. + + The view answers a 409 that tells the caller to retry, and the criterion is neither deleted nor archived. + """ + + def progress_arrives_after_the_check(criterion_id: int) -> bool: + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion_id=criterion_id, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + return False + + monkeypatch.setattr(cbe_api, "_criterion_has_learner_progress", progress_arrives_after_the_check) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_409_CONFLICT + assert "Retry the DELETE" in str(response.data["detail"]) + criterion.refresh_from_db() + assert criterion.archived is False + + +def test_delete_of_a_criterion_gone_by_the_time_of_the_call_is_404( + run_when_tag_lock_is_taken, user_client: APIClient, criterion: CompetencyCriterion, +) -> None: + """A criterion removed between the view's lookup and the API's locked re-read is a 404, not a 500.""" + run_when_tag_lock_is_taken(lambda: CompetencyCriterion.objects.filter(pk=criterion.pk).delete()) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_404_NOT_FOUND + + +def test_archiving_a_criterion_whose_stored_payload_went_invalid_is_400_and_changes_nothing( + user_client: APIClient, user, batch: tuple[CompetencyCriterion, CompetencyCriterion], now: datetime, +) -> None: + """ + Archiving saves the criterion, which re-validates its rule payload. A payload that stopped being + valid after it was stored, here written past validation with update(), makes the DELETE a 400 + keyed by the model-level error, and the criterion is neither archived nor deleted. + """ + criterion = batch[1] + CompetencyCriterion.objects.filter(pk=criterion.pk).update(rule_payload_override={"op": "gt", "value": 75}) + StudentCompetencyCriterionStatus.objects.create( + user=user, criterion=criterion, status_id=MasteryStatus.DEMONSTRATED, created=now, modified=now, + ) + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert list(response.json()) == ["__all__"] + criterion.refresh_from_db() + assert criterion.archived is False + + +@pytest.mark.parametrize("endpoint", ["delete a criterion", "bulk update criteria", "update a group"]) +def test_a_competency_tag_deleted_while_the_request_waits_for_its_lock_is_404( + endpoint: str, + run_when_tag_lock_is_taken, + user_client: APIClient, + leaf: CompetencyCriteriaGroup, + batch: tuple[CompetencyCriterion, CompetencyCriterion], +) -> None: + """ + A competency tag removed by an admin while the request waits for the Tag lock is a 404, not a 500. + The group's foreign key to the tag cascades, so the row can't go before the request starts without + taking the criterion with it; the delete runs as the lock is taken, as a commit during the wait would. + """ + run_when_tag_lock_is_taken(lambda: Tag.objects.filter(pk=leaf.tag_id).delete()) + + if endpoint == "delete a criterion": + response = user_client.delete(criterion_delete_url(leaf.tag_id, batch[0].id)) + elif endpoint == "bulk update criteria": + body = {"criterion_ids": [criterion.id for criterion in batch], **NEW_RULE} + response = user_client.patch(bulk_update_url(leaf.id), body, format="json") + else: + response = user_client.patch( + group_detail_url(leaf.tag_id, leaf.id), {"logic_operator": LogicOperator.AND}, format="json", + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND