diff --git a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst index 5869b2885..9eee316d3 100644 --- a/docs/openedx_learning/decisions/0002-competency-criteria-model.rst +++ b/docs/openedx_learning/decisions/0002-competency-criteria-model.rst @@ -108,7 +108,7 @@ Decision Boundaries and intended behavior: - - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing, but backend validation must reject them. + - Empty groups: Persisted criteria definitions should not contain empty groups. Authoring flows may temporarily create empty groups while editing. The criterion delete path enforces this rule for the groups it empties: such a group is deleted, or archived when learner progress references it or when only archived children remain, in the same transaction, so that path persists no empty group. Writers outside this path are not covered by it. - Mixed tree depths: Backend supports deeply nested groups. Current frontend authoring constraint is a maximum depth of 3 layers total, using zero-indexed depth (``0=root``, ``1=course-scope group``, ``2=leaf criteria/group``). - Retrieval scope: A read always includes the root group, plus any selected subset of course-scoped groups, each returned as a complete subtree, never truncated. - Practical size and growth: Total rows in ``CompetencyCriteriaGroup`` are expected to grow over time as course runs are added; this ADR sets no global DB row cap. No max total node-count cap is required per root group. Each course-scoped subtree is expected to stay small (realistically <=500 nodes). @@ -311,7 +311,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 not independently checked for this purpose, with one exception: the criterion delete cascade checks ``StudentCompetencyCriteriaGroupStatus`` directly before hard-deleting a group, because a group row about to be deleted cannot wait for the roll-up to catch up. :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. @@ -490,3 +490,10 @@ Changelog supersedes the ordered, short-circuited evaluation the earlier text described. Decision 6's update flow now continues past the root group to the 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: + +* Amended Decisions 2 and 7 for issue #674: the criterion delete cascade, not a backend validation + rejection, keeps the groups it empties out of the database, and it checks + ``StudentCompetencyCriteriaGroupStatus`` directly before hard-deleting a group, as #674's + acceptance criteria require. diff --git a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst index 0c839b74a..5b46163d0 100644 --- a/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst +++ b/docs/openedx_learning/decisions/0003-competency-criteria-versioning.rst @@ -35,7 +35,7 @@ For the initial implementation, versioning and traceability of competency achiev - Before any related learner status exists, edits and deletes are allowed. - After any related learner status exists, disassociation/deletion is archive-only (soft delete), not hard delete. - - Ordinary reads (tag lists, object-tag-for-an-object lookups used by authoring UIs, and similar) exclude archived rows, so an archived association behaves as gone from those paths. + - Ordinary reads (tag lists, object-tag-for-an-object lookups used by authoring UIs, and similar) exclude archived rows, so an archived association behaves as gone from those paths. For MVP, one exception applies: the ``ObjectTag`` of a criterion archived by the delete endpoint stays live, so the competency tag remains visible on the tagged object while the criterion itself is closed to authoring. - The read path that resolves a learner status row back to the ``ObjectTag`` it references is exempt from that exclusion: archived rows remain queryable on that path, so learner status records can continue to be traced back to their source association. 4. Authoring guardrails must warn on potentially impactful edits: @@ -128,3 +128,11 @@ Rejected Alternatives 11. Drop the database-level foreign key constraint on the learner column. - Cons: - This repo's convention is a real foreign key to ``settings.AUTH_USER_MODEL``. Reports of contention on the user row exist elsewhere in the ecosystem, but are not understood well enough here to design around. + +Changelog +--------- + +2026-11-04: + +* Amended Decision 3 for issue #674: the ``ObjectTag`` of a criterion archived by the delete endpoint stays live for MVP, + so the competency tag remains visible on the object. diff --git a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst index a018cd45b..1d20a08a6 100644 --- a/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst +++ b/docs/openedx_learning/decisions/0004-competency-mastery-concurrency.rst @@ -68,7 +68,9 @@ 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. Criterion + authoring writers (create, delete, bulk update, and group update) serialize per competency on + the competency's ``Tag`` row, each taking that lock first and no other explicit locking read on groups or criteria. @@ -162,3 +164,11 @@ 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: + +* Amended Decision 6 for issue #674: the rule that no other path takes a lock covers learner-status + writers only. Criterion authoring writers serialize per competency on the ``Tag`` row. diff --git a/src/openedx_learning/applets/cbe/api.py b/src/openedx_learning/applets/cbe/api.py index 29205bb0d..3bdd080a7 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 @@ -42,11 +43,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_competency_criteria_tree", "get_competency_rule_profiles", "get_courses_with_relative_criteria", @@ -291,7 +294,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 @@ -431,10 +435,13 @@ 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. The caller must also hold the course run lock that + associate_competency_criterion() takes, so the dominance check below can't race another create. """ + # 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 @@ -568,10 +575,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() @@ -630,6 +638,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, @@ -664,8 +677,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"]) @@ -696,7 +708,10 @@ 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. + + Takes the competency's Tag row lock before reading anything that can change, like every writer of + its criteria, so it waits for a concurrent create or delete of the same competency. """ with transaction.atomic(): group = CompetencyCriteriaGroup.objects.select_related("tag__taxonomy", "parent__course").get( @@ -705,7 +720,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): @@ -717,13 +732,16 @@ 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 writer of a competency's criteria takes its Tag row lock first, and none takes another row lock + # before it, so the writers can't deadlock. 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( @@ -732,9 +750,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( @@ -756,7 +776,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, @@ -799,6 +822,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. @@ -865,12 +892,15 @@ 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 course. Raises CompetencyCriteriaGroupArchivedError if the group is archived, even when nothing would change. + + Takes the competency's Tag row lock and re-reads the group under it, like every writer of the + competency's criteria, so a concurrent delete that archived or removed the group is seen. """ with transaction.atomic(): group = CompetencyCriteriaGroup.objects.select_related("course", "parent__course").get(pk=group_id) @@ -898,6 +928,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( @@ -912,8 +948,149 @@ 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_from_group(group: CompetencyCriteriaGroup, user: UserType) -> None: + """ + Walk ``group`` and its ancestors, collapsing/archiving any left with no live children. + + Checking both relations (``criteria`` and ``child_groups``) at every level is harmless: the one that + does not apply is an empty queryset, and it avoids a separate leaf/non-leaf code path. + + 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_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_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_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 locks only the competency's Tag row, the lock every writer of its criteria takes + first, so a concurrent create waits until this call commits. The lock prevents three races that + follow from Django's Python-side cascade. 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. 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 fails 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. If the status INSERT comes first, this delete waits for the grade + transaction and then fails the foreign-key check, and a second delete archives the criterion. If the + DELETE comes first, 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(): + 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_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 57376ee8f..a42110a23 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/serializers.py @@ -82,14 +82,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 e96d754a1..19ee778ca 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/urls.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/urls.py @@ -32,4 +32,9 @@ views.CompetencyCriteriaGroupDetailView.as_view(), name="criteria-group-detail", ), + path( + "competencies//criteria//", + views.CompetencyCriterionDeleteView.as_view(), + name="criterion-delete", + ), ] 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 d7c3a4d8c..c83d8bbd2 100644 --- a/src/openedx_learning/applets/cbe/rest_api/v1/views.py +++ b/src/openedx_learning/applets/cbe/rest_api/v1/views.py @@ -26,6 +26,7 @@ CompetencyCriterionArchivedError, associate_competency_criterion, bulk_update_competency_criteria, + delete_or_archive_competency_criterion, get_competency_criteria_tree, get_competency_rule_profiles, resolve_competency_tag, @@ -165,7 +166,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 @@ -266,10 +267,46 @@ 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 except CompetencyCriteriaGroupArchivedError as exc: raise CompetencyCriteriaConflict(str(exc)) from exc 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. + """ + + serializer_class = CompetencyCriterionSerializer + authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) + permission_classes = [IsAuthenticated] + + def delete(self, request, *args, **kwargs): + """ + Resolve the criterion scoped to both path segments, then delegate to the public API, which checks permission. + + The URL is scoped by tag_id, not group_id, because the route follows the + ``competencies//criteria/`` family #665 created, and the Studio flow addresses a criterion by + tag and criterion id. + The lookup deliberately does not filter `archived=False`, so a repeat + DELETE against an already-archived row still resolves (200), and a mismatched tag_id or a + nonexistent criterion_id both 404 the same way, via this one get_object_or_404 call. + """ + 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 + return Response({"id": result.id, "archived": result.archived}, status=status.HTTP_200_OK) 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 52f844e07..ad695bb35 100644 --- a/tests/openedx_learning/applets/cbe/test_api.py +++ b/tests/openedx_learning/applets/cbe/test_api.py @@ -1041,6 +1041,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 # ============================================================================================== @@ -1952,6 +2010,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: @@ -2237,6 +2309,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..d2437ddde --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_concurrency.py @@ -0,0 +1,465 @@ +""" +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_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_from_group", pause.wrap(_cascade_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 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. Nothing is lost: 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 with ``IntegrityError``. + + The grade's status insert waits for the delete to commit and then fails the foreign-key check. 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..bdfd93a33 --- /dev/null +++ b/tests/openedx_learning/applets/cbe/test_criterion_removal.py @@ -0,0 +1,541 @@ +"""Tests for delete_or_archive_competency_criterion() and its ancestor-cascade helper, _cascade_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() + + +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, +) -> 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. + """ + group = CompetencyCriteriaGroup.objects.create(tag=tag) + criterion = _make_criterion(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() + + +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, + ) + + delete_or_archive_competency_criterion(criterion.id, user) + + 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 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) + 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 be957f857..8b77f5268 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 @@ -24,7 +25,9 @@ CompetencyRuleProfile, CompetencyTaxonomy, LogicOperator, + MasteryStatus, RuleType, + StudentCompetencyCriterionStatus, ) from openedx_tagging.models import ObjectTag, Tag, Taxonomy @@ -73,6 +76,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}) + + def usage_key(course_run: CourseRun, block_id: str) -> str: """Build a gradeable-subsection-shaped usage key string under `course_run`.""" key = course_run.course_key @@ -341,6 +349,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( @@ -822,7 +831,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: @@ -1993,3 +2004,207 @@ def test_group_detail_offers_only_patch( response = getattr(user_client, method)(group_detail_url(tag.id, leaf.id), {}, format="json") assert response.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + + +# ============================================================================================== +# 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 + + +def test_delete_no_permission_is_403( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, tag: Tag, group: CompetencyCriteriaGroup, + default_rule_profile: CompetencyRuleProfile, +) -> None: + """A caller refused by the object-level tagging rule gets a 403, and the criterion remains.""" + object_tag = ObjectTag.objects.create( + object_id=f"block-v1:unused+{UNAUTHORIZED_MARKER}+unused+type@sequential+block@p1", + taxonomy=competency_taxonomy, + tag=tag, + ) + criterion = CompetencyCriterion.objects.create( + group=group, object_tag=object_tag, rule_profile=default_rule_profile, + ) + + 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_without_view_access_to_the_taxonomy_is_403( + user_client: APIClient, competency_taxonomy: CompetencyTaxonomy, criterion: CompetencyCriterion, +) -> None: + """A disabled taxonomy refuses an ordinary caller even when the object id itself is permitted.""" + competency_taxonomy.enabled = False + competency_taxonomy.save() + + response = user_client.delete(criterion_delete_url(criterion.group.tag_id, criterion.id)) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert CompetencyCriterion.objects.filter(id=criterion.id).exists() + + +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