Skip to content

[Placeholder] Competency Criteria Deletions #799

Closed as not planned
Closed as not planned

Description

@jesperhodge

Status: superseded, closed

This issue owned nothing by the time #655 resolved. It existed to hold #613's delete-protection
criteria while #655 decided how to implement them, and to replace the provisional on_delete=PROTECT
values that #641 and #642 were told to declare in the meantime. #655 is now closed with an approved
design that answers the question differently, so none of the four criteria below survive. They are
kept for history rather than as work.

Where each piece went:

Criterion below Where it lands now
on_delete=PROTECT on Student*Status foreign keys #641 and #642 declare it directly. #641 also records the one question still open about the two foreign keys crossing into openedx_tagging.
on_delete=CASCADE on definition-to-definition foreign keys Dropped. #655's appendix puts enforcement at the application layer rather than in database-level cascade behavior.
Transitive delete tests #641 and #642 each now carry a criterion requiring test coverage for whatever on_delete values they land. The product behavior is covered by #674 and #675 through their own endpoints.
CompetencyRuleProfile is archive-only, never hard-deleted #655 resolves this differently: the model gets no DELETE endpoint at all, and only the single system-default row exists in MVP, so there is no archive-versus-delete case for it. Whoever builds the profile endpoints owns not building a DELETE.

Work #655 identified that no issue owns yet. Recorded here so it is not lost, but it belongs in
freshly filed tickets rather than in this one, because it changes openedx_tagging rather than CBE:

  1. An openedx_tagging ticket adding archived to Tag, Taxonomy and ObjectTag, deletion_locked
    to ObjectTag only, and the three lock functions CBE calls. [Arch] Implementation approach for competency data delete/edit guardrails #655 states explicitly that [BE] Add archived field to Competency Criteria & Group models #716 does
    not cover this.
  2. The CBE call site that locks an ObjectTag when a StudentCompetencyCriteriaStatus row is written,
    with the read-before-write guard so a shared ObjectTag row is not write-locked inside every grade
    transaction.
  3. The tagging-side delete-path work: the per-tag branch in Taxonomy.delete_tags(), a
    perform_destroy on TaxonomyView, the archive branch in tag_object(), and the archived
    exclusions across the read paths.
  4. An openedx-platform ticket extending ADR-0013's taxonomy_type computation to the per-object tag
    endpoint, plus the frontend gating of the delete and Add Tag controls.
  5. The ADR follow-up PR @mgwozdz-unicon committed to on [Arch] Implementation approach for competency data delete/edit guardrails #655, covering the nine ADR edits raised there.

Parent: #613.


Historical content

Everything below is the issue as it stood before #655 closed. Kept for the record.

Why this is separate

ADR-0002 Decision 7 and ADR-0003 Decision 4 already decide what should happen when someone tries to
delete or edit competency data after a learner has mastery status against it. #655 is deciding how
that gets implemented: how a "can this be deleted" signal is computed and surfaced on the GET endpoints
without N+1 queries, and where the write-path validation lives so the rule is enforced rather than only
hinted at in the UI.

Those answers change what the model layer has to look like, so these criteria were pulled out of #613's
model tickets rather than guessed at. #641 and #642 build the tables with no delete behavior of their
own.

What #641 and #642 leave behind

Django requires an on_delete value on every ForeignKey, so #641 and #642 cannot leave the question
open. Both declare every new foreign key as on_delete=PROTECT with a TODO(#799) comment. That is a
fail-closed placeholder, not a considered per-foreign-key choice: nothing can be hard-deleted until this
issue sets the real values. Neither ticket ships an authoring API, so nothing performs deletes meanwhile.

So the first job here, whatever #655 decides, is to replace every on_delete=PROTECT that carries a
TODO(#799) comment, and to leave none behind.

Acceptance criteria (from #613)

Copied unchanged from #613's "Deletions" section. Expect these to change once #655 is resolved.

  • Delete protection is implemented via on_delete=PROTECT on FKs from Student*Status models to definition models; hard deletion is blocked at the ORM layer without requiring instance-level delete() overrides, since QuerySet.delete() bypasses those.
  • Every foreign key from one definition table to another uses on_delete=CASCADE: criterion to group, group to parent group, group to tag, and criterion to objecttag. The one exception is the criterion's foreign key to CompetencyRuleProfile, which uses PROTECT, because a rule profile is never hard-deleted at all. These values are what make ADR-0002 Decision 7's delete protection work. That protection is conditional, not a blanket ban: deleting a tag has to succeed when no learner has a status under it, and fail when one does. The PROTECT that produces the failure lives on the foreign keys from the three Student*Status tables, and two of those point at rows one and two levels below the tag. Django reaches them only by walking down the tree from the tag, and it only walks down a foreign key marked CASCADE. Change any link in that chain to SET_NULL and deleting the tag succeeds while learner statuses still exist.
  • The transitive cases are tested, not only the direct ones: deleting an oel_tagging_tag with a learner status row anywhere beneath it raises ProtectedError, and deleting one with no status rows beneath it succeeds and cascades.
  • CompetencyRuleProfile is never hard-deleted, even before any learner status exists. Retirement is archive-only through its archived column, per ADR-0002 Decision 7's exception, because a profile is shared across many criteria rather than tied to one.

Out of scope

The archived column itself. #641 creates it as one of CompetencyRuleProfile's columns, per ADR-0002
Decision 3. What this issue adds is the rule that retirement goes through it.

The edit guardrails from ADR-0003 Decision 4, meaning the Studio warning and confirmation when someone
edits criteria that learners already have status against. #655 covers edits as well as deletes, but that
warning is API and UI work, not model work.

Depends on

#655 for the approach. #641 and #642 for the tables and the foreign keys to change.

Activity

  1. jesperhodge commented on Sep 1, 2026

    @jesperhodge
    ContributorAuthor

    Hi, Claude here.

    While implementing #642's learner status models I hit a gap in this issue's scope that is
    worth recording before anyone starts work here.

    This issue's acceptance criteria scope delete protection to "FKs from Student*Status
    models to definition models". Each of the three Student*Status models carries two further
    foreign keys that are not to definition models, and so are covered by no criterion here:

    1. user_id, pointing at settings.AUTH_USER_MODEL.
    2. status_id, pointing at the CompetencyMasteryStatuses lookup table.

    Both of those are declared on_delete=PROTECT with a TODO(#799) comment, because #641 and
    #642 declare every new foreign key that way as a fail-closed placeholder. This issue's
    stated first job is to replace every PROTECT carrying a TODO(#799) comment and to leave
    none behind, so both of those foreign keys land in scope by that sentence while no
    acceptance criterion mentions them.

    That matters most for the user foreign key. Taken literally, PROTECT there means deleting a
    user row fails once that user has any competency status. That may well be the answer we want,
    since Open edX retires learners rather than deleting rows, but it is currently an accident of
    the placeholder rather than a decision. The existing precedent in openedx-core is different
    again: every user foreign key in openedx_content is SET_NULL with a nullable column, but
    those are created_by and changed_by attribution fields, where a null is meaningful. On a
    learner status row the user is identifying rather than attributive, and SET_NULL would
    break the (user_id, node_id) uniqueness that the whole in-place-update design rests on. So
    the realistic candidates are CASCADE and PROTECT, and the choice needs stating.

    Suggested addition to this issue, whenever #655 unblocks it: one criterion covering the
    user_id and status_id foreign keys on all three Student*Status models, either setting a
    value or recording explicitly that they keep PROTECT and why.

  2. jesperhodge commented on Sep 2, 2026

    @jesperhodge
    ContributorAuthor

    Hi, Claude here, coming from a conversation with @jesperhodge.

    Closing this as superseded. #655 is now resolved with an approved design that answers the
    delete-and-edit guardrail question at the application layer, so none of the four criteria this issue
    held survive. The body above records where each one went, and lists the work #655 identified that no
    ticket owns yet.

    Two things worth stating plainly, since this issue's stated first job was to replace every provisional
    on_delete value:

    One question is still open rather than closed, and it is recorded in #641 and raised on #655 instead of
    here: with PROTECT on CompetencyCriteriaGroup.tag and CompetencyCriterion.object_tag, deleting a
    Tag or Taxonomy that an author has built criteria against, but against which no learner holds
    mastery, raises ProtectedError where #655's approved table says it should hard-delete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions