Skip to content

[Arch] Implementation approach for competency data delete/edit guardrails #655

Closed

Description

@mgwozdz-unicon

Context

docs/openedx_learning/decisions/0002-competency-criteria-model.rst (Decision 7, "Delete protection boundaries") and 0003-competency-criteria-versioning.rst (Decision 4, "Authoring guardrails must warn on potentially impactful edits") already decide what should happen when a Platform Administrator or Course Author tries to change competency taxonomy data once a related learner mastery status exists:

  • Hard delete is blocked once any related StudentCompetencyStatus, StudentCompetencyCriteriaGroupStatus, or StudentCompetencyCriteriaStatus row exists, for Taxonomy/CompetencyTaxonomy, Tag, ObjectTag, CompetencyCriteriaGroup, and CompetencyCriteria.
  • CompetencyRuleProfile is never hard-deletable, with or without learner status.
  • Edits are allowed even after learner status exists, but must warn the user and require explicit confirmation.

What's not yet decided is how to implement this: how to compute and surface a "can this be deleted" signal on the relevant GET endpoints without introducing N+1 queries, and where the corresponding write-path validation should live so the rule is actually enforced, not just hinted at in the UI.

Ask

Agree on the implementation approach below before opening execution tickets against it. See the proposal in the comment.

Scope

In scope:

  • CompetencyCriteria, CompetencyCriteriaGroup, CompetencyRuleProfile, Tag, Taxonomy/CompetencyTaxonomy, ObjectTag

Out of scope for this MVP (raised as an open question in the proposal, not silently assumed):

  • Gradeable subsection edit/delete behavior when a subsection has contributed to a learner's competency status. No existing Open edX precedent blocks or warns on this today; the proposal recommends staying at parity with current behavior for now.
  • Archiving/restore UI beyond what ADR-0002/0003 already require for delete-protected records.

Activity

  1. moved this from Todo to Needs additional details in Competency Criteria and Student Progresson Jul 8, 2026
  2. mgwozdz-unicon commented on Jul 8, 2026

    @mgwozdz-unicon
    ContributorAuthor

    Implementation proposal: delete/edit guardrails for competency taxonomy data

    What's already decided (not up for debate here, except the open question below)

    Model Delete Edit warning (once learner status exists / profile is "in use")
    CompetencyCriteria Blocked once learner status exists Required
    CompetencyCriteriaGroup Blocked once learner status exists Required
    Tag / Taxonomy / CompetencyTaxonomy Blocked once learner status exists Not required per ADR-0003 today — see open question below
    ObjectTag Hard delete allowed before learner status exists; archive-only (soft delete) after Required
    CompetencyRuleProfile No DELETE endpoint at all. Archive (archived field, set via a normal field-level update) is the only removal path, regardless of learner status Required when the profile is "in use"

    Source: ADR-0002 Decision 7, ADR-0003 Decision 4.

    CompetencyRuleProfile is not a stricter version of the "blocked once status exists" rule the other models follow, it's a different rule: there's no delete action for it at all, ever. This proposal doesn't add a DELETE endpoint for it, and it shouldn't have one.

    Open question: should Tag/Taxonomy edits also require a warning?

    ADR-0003 Decision 2 exempts Tag, Taxonomy, and CompetencyTaxonomy from the edit warning, on the grounds that these are "non-evaluative display/metadata" for competency purposes: editing a tag's name or a taxonomy's description doesn't change how mastery gets computed, so there's nothing to warn about.

    Worth reconsidering for consistency: every other model in scope warns on edit once learner status exists, and a Platform Administrator has no way to know which of these five models is the "safe to edit silently" one without reading the ADR. The tradeoff is added friction on what's likely the highest-frequency, lowest-risk edit in the set (fixing a typo in a tag's display name).

    Raising this here rather than deciding it unilaterally: if the answer is "yes, warn here too," that's an amendment to ADR-0003 Decision 2, not just an implementation detail, and this issue is already the place we're getting community agreement on this feature.

    Proposed approach, part 1: surface deletability on GET endpoints

    Add a deletable field to the relevant list/detail endpoints (CompetencyCriteria, CompetencyCriteriaGroup, Tag, Taxonomy/CompetencyTaxonomy, ObjectTag) so the frontend can disable the delete action without needing a separate round trip per record.

    Compute it as a queryset annotation using Django's Exists()/OuterRef(), which asks the database to answer "does at least one matching row exist" as a correlated subquery run once per row, entirely inside the single SQL statement that already fetches the list. That avoids checking each record with a separate query.

    • CompetencyCriteria.deletable = NOT Exists(StudentCompetencyCriteriaStatus.objects.filter(competency_criteria=OuterRef('pk')))
    • CompetencyCriteriaGroup.deletable = NOT Exists(StudentCompetencyCriteriaGroupStatus.objects.filter(competency_criteria_group=OuterRef('pk')))
    • Tag.deletable = NOT Exists(StudentCompetencyStatus.objects.filter(tag=OuterRef('pk')))
    • ObjectTag.deletable = NOT Exists(StudentCompetencyCriteriaStatus.objects.filter(competency_criteria__object_tag=OuterRef('pk'))) (no direct FK from any status table to ObjectTag; joins through CompetencyCriterion)
    • Taxonomy/CompetencyTaxonomy.deletable = NOT Exists(StudentCompetencyStatus.objects.filter(tag__taxonomy=OuterRef('pk'))) (joins through Tag, since no status table points at Taxonomy directly; widest fan-out of the group, worth confirming indexes on StudentCompetencyStatus.tag_id and Tag.taxonomy_id before shipping)
    • CompetencyRuleProfile.deletable: not applicable, there's no delete action to gate (see table above).

    This repo doesn't yet use Exists() specifically, but it does use the same underlying technique (OuterRef + Subquery) for the same purpose elsewhere — e.g. child_count in openedx_tagging/models/base.py and last_contributed in openedx_content/applets/publishing/api.py are both computed as correlated subquery annotations to avoid per-row queries. Exists() is the more direct tool for a plain yes/no check than Subquery+Count.

    Whether to show a disabled delete action or an enabled one with a warning dialog (mirroring the edit-warning pattern) is a UX call, not an API one; either way the API needs to expose the same deletable signal. Flagging it here so it isn't decided by default: a disabled control with no explanation risks leaving the admin unsure why, so this probably needs a tooltip or similar at minimum. Leaving the actual choice to UX design.

    Proposed approach, part 2: enforce it on write endpoints

    The GET-endpoint flag above is a UX convenience, not enforcement — it disables a button, but nothing stops a direct API call from deleting a protected record. The actual rule needs to live on the write path too:

    • DELETE: each DELETE endpoint for the five deletable-with-conditions models re-runs the same existence check server-side (perform_destroy or the model's delete path) and returns 409 Conflict, naming the blocking status record(s), if one is found. CompetencyRuleProfile simply has no DELETE endpoint to guard.
    • Edit: no backend block needed, ADR-0003 permits the edit and only requires the frontend warning + confirmation step (scope fields on CompetencyRuleProfile/CompetencyCriterion stay immutable after creation per ADR-0002, but that's ordinary field validation, not guardrail logic).

    Open question: gradeable subsections

    Student competency statuses are ultimately derived from grades, which raises the question of whether editing or deleting a gradeable subsection after a learner has been graded on it needs a similar guardrail.

    There's no existing precedent to inherit here. openedx-platform's only grading-immutability rule (grades/docs/decisions/0005-grade-freeze.rst) freezes grade values a fixed time after course end, unrelated to content edits, and Studio's delete handler removes any xblock, including graded subsections, with no grade check at all. There's also no FK from any StudentCompetency*Status table to a subsection or usage key, so deleting one wouldn't corrupt an already-computed status row, it would only affect the ObjectTag linking that subsection to a taxonomy tag, and orphaned ObjectTags already degrade safely (see below).

    Proposed answer: out of scope for this MVP. Leave subsection edit/delete at parity with today's behavior, no new restriction, no new warning. Flagging as an explicit open question rather than a silent assumption: if the community wants better protection here, it's a new precedent for openedx-platform, not something this feature should introduce as a side effect.

    Out of scope

    • Any DB-level FK constraint (ON DELETE RESTRICT) in place of application-layer validation. Keeping the check at the serializer/view layer matches how this ADR set already handles similar validation (see ADR-0002's "backend validation must reject" language for empty criteria groups) and keeps the error message controllable.
    • Archiving workflows beyond the archive-only path ADR-0002/0003 already specify for ObjectTag and CompetencyRuleProfile.

    Appendix: how orphaned tags already degrade safely

    Referenced above, for context: ObjectTag (the record linking a piece of content to a tag) doesn't only hold a foreign key to Tag/Taxonomy, it also stores its own copy of the tag's display text (_export_id, _value) at the time the content was tagged. The foreign keys are set to NULL automatically if the source Tag/Taxonomy is deleted (on_delete=SET_NULL, not a cascade delete), but the cached copy stays put, so the content's tag display keeps working using the cached text instead of breaking or going blank. See ADR-0006 and openedx_tagging/models/base.py.

    Appendix: PUT vs PATCH for the new endpoints

    This repo has no single stated convention. TaxonomyView uses DRF's default ModelViewSet, which supports both. ObjectTagView explicitly disables PATCH and only allows PUT, because its update semantics are a full replace. Given ADR-0002/0003 describe edits to CompetencyRuleProfile and CompetencyCriteria as changing specific fields only (e.g. "editing a profile may change rule_type/rule_payload only"), PATCH is the better fit for the new CompetencyCriteria/CompetencyCriteriaGroup/CompetencyRuleProfile endpoints, following the same "restrict when the semantics call for it" reasoning ObjectTagView already sets as precedent, not a repo-wide rule.

  3. moved this from Needs additional details to Ready for Community Review in Competency Criteria and Student Progresson Jul 10, 2026
  4. jesperhodge commented on Jul 13, 2026

    @jesperhodge
    Contributor

    Some thoughts about adding this deletable property.

    • If it's a UX nice-to-have to disable the button, but there's no real critical reason not to just return an error when trying to delete except from aesthetics, I wouldn't think this is needed for the MVP. The backend guardrail already prevents deletion, so as we are talking about an "MVP"-like iteration, I would say this is an effort that we can consider out of scope for the competencies MVP. Very much "nice-to-have"-y.
    • Reading the criteria will happen a lot, while deleting something will happen only occasionally. I'm not sure adding the additional database JOINS and such to every single read is optimally performance-wise, so we should add caching here with generous time to live.
    • If instead you make the "deletable" a database field and set it when a related object is written that prevents deletion would be better than calculating at every read, but need some overhead in terms of development to make sure this works correctly.

    If we really want to go with the "deletable" property - and I don't see a strong reason not to move this out scope - I would recommend using caching.

  5. jesperhodge commented on Jul 13, 2026

    @jesperhodge
    Contributor

    About deleting / editing graded subsections that have already been graded by learners: we investigated this a while ago. As far as I'm up to date:

    • editing or deleting a subsection with existing grades can indeed skew learner data and impose problems.
    • we generally hold course authors responsible when they edit subsections with learner data on live courses. I think there is a warning already in place. The philosophy so far is to not treat this as our responsibility.
    • For course authors editing / deleting already graded subsections there are ways for them to fix it, I believe. With our system of append-only writing fulfilled assignments, editing / deleting graded subsections should not impact the mastery, but if it would for some reason, we should also provide some conceivable way to fix it.

    I second considering this out of scope while we make sure that deleting / editing subsections don't corrupt skill mastery for a learner; if we do find a reason to worry about this, I recommend just providing some way for course authors to correct it if it happens.

  6. mgwozdz-unicon commented on Jul 15, 2026

    @mgwozdz-unicon
    ContributorAuthor

    @jesperhodge, addressed both:

    1. Decided to keep deletable in scope: frontend validation should match backend validation, so the delete action can be disabled/warned on directly rather than only surfacing the block after a failed request. Went with a persisted field rather than caching, given the read-cost concern you raised. It turned into a real design question (how do you keep a persisted signal correct without a general invalidation scheme), so it got its own ADR rather than staying an implementation detail here: openedx-core#661 adds ADR-0005. Short version: the flip is one-directional (deletable only ever goes True to False, never back, since learner status is append-only), set synchronously at learner-status write time, so there's no cache-invalidation problem to solve in the first place.
    2. Agreed, and thanks for confirming the existing precedent there. Treating gradeable-subsection edit/delete as out of scope for this MVP: no new restriction, no new warning, revisit only if it turns out to actually corrupt mastery in practice.
  7. moved this from Ready for Community Review to Needs additional details in Competency Criteria and Student Progresson Aug 7, 2026
  8. mgwozdz-unicon commented on Aug 12, 2026

    @mgwozdz-unicon
    ContributorAuthor

    Posting a status update on the "Proposed approach" sections above, since both halves have moved since mid-July.

    Part 2 (enforcement) is done for CompetencyCriteria and CompetencyCriteriaGroup. #674 and #675 spec the DELETE behavior directly: check for a referencing StudentCompetencyCriteriaStatus/StudentCompetencyCriteriaGroupStatus row, hard-delete if none exists, archive (archived=True) instead of deleting if one does. That answers the enforcement half of this proposal for those two models.

    Part 1 (the GET-side deletable signal) is also settled without needing the standalone ADR (#661), for CompetencyCriteria, CompetencyCriteriaGroup, and ObjectTag. #681 already returns the criteria-group/criteria list for a competency, and ObjectTagView's own GET endpoint only ever returns tags for one object at a time. Both lists are small and already scoped narrowly enough, one competency or one object, that adding a plain per-row Exists() annotation to each is implementation detail on those endpoints rather than a design question.

    Tag and Taxonomy/CompetencyTaxonomy are still open, and that's now the entire remaining scope of #661. The open question there is real: those two are read through the existing, much higher-traffic TaxonomyTagsView list rather than a narrowly-scoped one, so the read-cost concern Jesper raised on this issue back in July still applies. I've posted a proposal on #661 to resolve it using the same technique openedx_tagging already uses for per-tag usage_count (a bulk query plus in-Python computation, instead of a per-row database annotation), rather than the persisted database field originally proposed here. Following up there rather than duplicating the technical discussion in two places.

    If anyone here has thoughts on the remaining Tag/Taxonomy question, #661 is where that's being worked out.

  9. mgwozdz-unicon commented on Aug 20, 2026

    @mgwozdz-unicon
    ContributorAuthor

    Implementation proposal: delete/edit guardrails for competency taxonomy data

    Updated from the original proposal above to reflect where things stand after #674, #675, #681, and the discussion on #661. Section by section below; anything not mentioned is unchanged from the original.

    What's already decided (not up for debate here, except the open question below)

    Unchanged from the original proposal:

    Model Delete Edit warning (once learner status exists / profile is "in use")
    CompetencyCriteria Blocked once learner status exists Required
    CompetencyCriteriaGroup Blocked once learner status exists Required
    Tag / Taxonomy / CompetencyTaxonomy Blocked once learner status exists Not required per ADR-0003 today — see open question below
    ObjectTag Hard delete allowed before learner status exists; archive-only (soft delete) after Required
    CompetencyRuleProfile No DELETE endpoint at all. Archive (archived field, set via a normal field-level update) is the only removal path, regardless of learner status Required when the profile is "in use"

    Source: ADR-0002 Decision 7, ADR-0003 Decision 4.

    Open question: should Tag/Taxonomy edits also require a warning?

    Unchanged from the original proposal. Not discussed on #661; still open.

    Resolved: gradeable subsections

    No longer an open question. jesperhodge confirmed on this issue (comment, 2026-07-13) that editing/deleting a gradeable subsection after learner grading is a known, separately-handled risk in openedx-platform today, that course authors are already held responsible for it, and that there's a way for authors to correct it if it happens. Agreed: out of scope for this MVP, no new restriction, no new warning, revisit only if it turns out to actually corrupt mastery in practice.

    Proposed approach, part 1: surface deletability on GET endpoints

    This has split into two tracks with different answers, where the original proposal treated all five models the same way.

    CompetencyCriteria, CompetencyCriteriaGroup, and ObjectTag: settled, no design question remains. Each of these is read through an endpoint scoped to a single competency's criteria tree or a single object's tags (#681 for the first two; ObjectTagView.get_queryset, which only ever returns tags for one object at a time, for the third). At that scope, a plain per-row Exists() annotation, exactly as the original proposal described, is implementation detail on each endpoint rather than a design decision. No standalone design doc needed for these three.

    Tag and Taxonomy/CompetencyTaxonomy: still the open question, being worked out on #661. These are read through TaxonomyTagsView, a much higher-traffic endpoint used throughout Studio's tagging UI, not just this feature. jesperhodge's original cost concern (comment, 2026-07-13) — that adding a per-row JOIN to every read of a high-traffic list is worth avoiding — still applies here, which is why this pair needs a different answer than the other three:

    • Rejected: a persisted deletable field. This was the original response to Jesper's concern (issue comment, 2026-07-15) and became ADR-0005 / PR #661: a boolean set synchronously at learner-status write time, flipping one-directionally from True to False. kdmccormick and jesperhodge both came out against maintaining a bespoke deletable row (PR docs: add ADR for persisted competency deletability signal #661 comments, 2026-08-13 and 2026-08-18), and mgwozdz has proposed closing PR docs: add ADR for persisted competency deletability signal #661 as a result once this is settled.
    • Rejected: a dry-run delete-and-rollback using ON DELETE RESTRICT. bradenmacdonald proposed this on PR docs: add ADR for persisted competency deletability signal #661 (2026-07-20): attempt the real delete inside a transaction, roll it back, and let the FK constraint's RestrictedError report what's blocking it. This depends on Tag/Taxonomy's foreign keys carrying a real DB constraint, which was in question at the time because Competency Status Storage ADR #657 (still open) proposed moving learner-status tables onto a separate DB alias with logical-only FKs. That dependency has since resolved: jesperhodge confirmed (PR docs: add ADR for persisted competency deletability signal #661, 2026-07-22/23) that only the ...History table moves to a separate alias, and the active status tables keep normal foreign keys, so the constraint would in fact be there to trial against. Even so, mgwozdz's response (PR docs: add ADR for persisted competency deletability signal #661, 2026-08-16) is that this doesn't serve the same purpose as the GET-side signal: a dry-run only tells you the answer at the moment you attempt to delete, so it can produce a better error message but it cannot let Studio disable the delete control before the user opens that dialog in the first place, which is what this deletable field is for.
    • Current direction: the same bulk-query-plus-Python-attachment technique the endpoint already uses for usage_count. TaxonomyTagsView already computes something analogous via add_usage_counts() (openedx_tagging/api.py:529), whose own docstring calls a per-row annotation "a very expensive annotation" and instead runs one query for the whole page of tags, then attaches each tag's count in Python before the response goes out. Applying the same technique to deletable (one bulk existence-check query per page, attached to each row in Python) avoids the per-row JOIN Jesper flagged, without needing a persisted field or any write-side bookkeeping. deletable is a plain existence check with no counting or dedup, so it should cost less than usage_count already does on this same endpoint today. As of 2026-08-16, kdmccormick's summary confirmed no objection to this direction and jesperhodge confirmed no objections (2026-08-18).

    Whether to show a disabled delete action or an enabled one with a warning dialog is still nominally a UX call, but mgwozdz stated a preference on PR #661 (2026-08-16) for disabling with an explanation over letting someone click through to a failed attempt, given that ADR-0002 blocks the delete outright rather than allowing an override. Not yet a final decision.

    Proposed approach, part 2: enforce it on write endpoints

    CompetencyCriteria and CompetencyCriteriaGroup: settled and shipped differently than originally proposed. #674 and #675 spec the actual DELETE behavior: check for a referencing StudentCompetencyCriteriaStatus/StudentCompetencyCriteriaGroupStatus row; if none exists, hard-delete; if one exists, set archived=True instead of deleting. This replaces the original proposal's "return 409 Conflict, naming the blocking record" for these two models — the record is archived instead of the request being rejected.

    Tag/Taxonomy: recommend enforcing the delete block at the database level rather than with an application-layer check, reversing the original proposal for this pair. A foreign key can be configured to automatically block a delete when other rows still reference it (on_delete=RESTRICT), raising an error the view can catch and return, instead of the view checking first. The original proposal ruled this out because #657 was going to move the learner-status tables to a separate database without a real constraint to enforce; #657 has since resolved to keep those tables on normal foreign keys (only the ...History table moves), so the constraint is available again. We recommend adopting the RestrictedError idea from PR #661 (2026-07-20) for the real DELETE call itself, not just as an on-demand check. This differs from CompetencyCriteria/CompetencyCriteriaGroup, which run their own manual existence check instead because those will be archived instead of deleted in this case per ADR 0002 (#674/#675).

    Edit: unchanged from the original proposal, no backend block needed.

    Out of scope

    Unchanged for CompetencyRuleProfile and the archiving-workflow line. The blanket "no DB-level FK constraint" line no longer applies cleanly to Tag/Taxonomy per the open point above; it still holds for CompetencyCriteria/CompetencyCriteriaGroup, whose enforcement mechanism (#674/#675) is application-layer archive-on-conflict, not a DB constraint.


    Appendix: how orphaned tags already degrade safely

    Unchanged from the original proposal.

    Appendix: PUT vs PATCH for the new endpoints

    Unchanged from the original proposal.

  10. mgwozdz-unicon commented on Aug 20, 2026

    @mgwozdz-unicon
    ContributorAuthor

    @jmakowski1123 I'd like to nail down the precise product requirements for our Competency Guardrails Epic before settling on a specific technical approach here. Could you please review the following questions and options and let me know what you think is best? cc @adolfo @ormsbee @thelmick-unicon

    Competency data guardrails: questions for Product

    Once a learner has been credited with mastering a competency, we want to make sure an admin can't accidentally destroy the tag, rule, or grouping behind that record. For each record type below there are two questions: what happens to the record itself, and what the admin sees when they try to delete it, from the specific page where that removal happens.

    "Scope" marks whether an option was already part of our original ticket list. "Development Effort" is a rough relative sizing (Lowest/Low/Medium/Medium-High/Highest).

    Already decided (Context only)

    • Once a learner has been credited using a competency rule, rule grouping, tag, taxonomy, or tagged association, an admin silently and permanently destroying that record isn't acceptable according to CBE ADR-0002.
    • That same ADR says retiring any of these five record types (Tag, Taxonomy, Object Tag association, Competency Criterion, Competency Criteria Group) may be archive-only rather than a hard block — it doesn't single any of them out as block-only. Right now, only Competency Criterion ([BE] Delete Competency Criterion #674) and Competency Criteria Group ([BE] Build endpoint for removing a Competency Criteria Group #675) have tickets drafted to actually build that. The questions below ask, case by case, whether the rest should match or stay simpler.

    Taxonomy Editing page

    Tag

    Parity Note: deleting a tag today already shows a confirmation dialog.

    Q1. Once a learner has been credited using a tag, what should happen if someone tries to delete it?

    Option Scope Development Effort
    Block it permanently — no way to ever delete it Simpler, but doesn't match current draft of #674 & #675 described below Lowest
    Archive it instead — hide it going forward, keep it on file, matching Criterion/Group The ADR explicitly allows this ("may be archive-only"), though doesn't mandate it — no ticket has scoped it yet Medium-High (needs a new "archived" concept for tags, which doesn't exist today)

    Q2. Depending on your Q1 answer, what should the admin see?

    If Q1 is... Admin sees... Scope Development Effort
    Block Nothing changes up front; error only if they try and it fails No Lowest
    Block Delete option greyed out / disabled in advance Yes Medium
    Block Delete option removed entirely No Medium
    Block Confirmation dialog; block only shows up after they confirm No Low
    Archive No visible difference — disappears the same as a real removal No Lowest
    Archive Confirmation dialog before archiving No Low
    Archive Distinct "archive" icon and tooltip, clear in advance Yes Medium-High

    Taxonomy

    Parity Note: deleting a taxonomy today already shows a confirmation dialog too, same as deleting a tag.

    Q3. Once a learner has been credited using any tag within a taxonomy, what should happen if someone tries to delete the whole taxonomy?

    Option Scope Development Effort
    Block it permanently — no way to ever delete it Simpler, but doesn't match current draft of #674 & #675 described below Lowest
    Archive it instead — hide it going forward, keep it on file, matching Criterion/Group The ADR explicitly allows this ("may be archive-only"), though doesn't mandate it — no ticket has scoped it yet Medium-High (same missing-field gap as tags)

    Q4. Depending on your Q3 answer, what should the admin see?

    If Q3 is... Admin sees... Scope Development Effort
    Block Nothing changes up front; error only if they try and it fails No Lowest
    Block Delete option greyed out / disabled in advance Yes Medium
    Block Delete option deleted entirely No Medium
    Block Confirmation dialog; block only shows up after they confirm No Low
    Archive No visible difference — disappears the same as a real removal No Lowest
    Archive Confirmation dialog before archiving No Low
    Archive Distinct "archive" icon and tooltip, clear in advance Yes Medium-High

    Competency Management page

    Competency Criterion (a single mastery rule)

    Draft ticket: #674 (not yet built)

    Q5. Once a learner has been credited using this rule, what should happen if someone tries to delete it?

    Option Scope Development Effort
    Archive it instead — hide it going forward, keep it on file Current draft plan (#674) Medium-High (drafted, not built; includes cascading logic for the rule grouping above it)
    Block it permanently — no way to ever delete it Would mean revising #674 Lower (a simple check-and-reject, without the archive/cascade logic #674 currently specs)

    Q6. Depending on your Q5 answer, what should the admin see?

    If Q5 is... Admin sees... Scope Development Effort
    Block Nothing changes up front; error only if they try and it fails No Lowest
    Block Delete option greyed out / disabled in advance Yes Medium
    Block Delete option removed entirely No Medium
    Block Confirmation dialog; block only shows up after they confirm No Low
    Archive No visible difference — disappears the same as a real removal No Lowest
    Archive Confirmation dialog before archiving No Low
    Archive Distinct "archive" icon and tooltip, clear in advance Yes Medium-High

    Competency Criteria Group (a grouping of rules)

    Draft ticket: #675 (not yet built)

    Q7. Once a learner has been credited using this grouping, or anything nested inside it, what should happen if someone tries to delete it?

    Option Scope Development Effort
    Archive it instead — hide it going forward, keep it on file Current draft plan (#675) Medium-High (drafted, not built; archives the whole nested grouping together)
    Block it permanently — no way to ever delete it Would mean revising #675 Lower

    Q8. Depending on your Q7 answer, what should the admin see?

    If Q7 is... Admin sees... Scope Development Effort
    Block Nothing changes up front; error only if they try and it fails No Lowest
    Block Delete option greyed out / disabled in advance Yes Medium
    Block Delete option deleted entirely No Medium
    Block Confirmation dialog; block only shows up after they confirm No Low
    Archive No visible difference — disappears the same as a real removal No Lowest
    Archive Confirmation dialog before archiving No Low
    Archive Distinct "archive" icon and tooltip, clear in advance Yes Medium-High

    Object Tag associations

    (For CBE, this is the link that connects a subsection to a competency tag, but this is also used by plain taxonomy tags to link any content to a plain tag.)

    Bigger question first: an association created via the Competency Management page could, in principle, also show up on the Course Outline page or the Libraries page. But those two pages show tags generically today, with no competency context — an admin there has no way to tell a competency tag apart from an ordinary one.

    Q9. What should happen on the Course Outline and Libraries pages?

    Option Scope Development Effort
    Don't display competency tags on these pages at all Not yet scoped Medium — confirmed no way to tell these tags apart exists today; needs one new field
    Display them, but don't allow editing or deleting from these pages Not yet scoped Medium-High — needs that same new field, but the on/off switch for "can this tag be edited/deleted" already exists in the UI, which helps
    Display them, and allow editing/deleting, following whatever process is chosen below for the Competency Management page Not yet scoped Highest — confirmed, and likely higher than shown: this depends on the Competency Management page's own guardrail behavior (Q5-Q8 above), which isn't built yet either

    (If you pick the third option, Q10/Q11 below apply on Course Outline and Libraries too, using the same answer. If you pick either of the first two, Q10/Q11 below apply only to the Competency Management page.)

    Q10. Once a learner has been credited using a tagged association, what should happen if someone tries to delete it?

    Option Scope Development Effort
    Block it permanently — no way to ever delete it Simpler, but doesn't match current draft of #674 & #675 Lowest
    Archive it instead — hide it going forward, keep it on file, matching Criterion/Group The ADR explicitly allows this ("may be archive-only"), though doesn't mandate it — no ticket has scoped it yet Medium-High (same missing-field gap as tags)

    Q11. Depending on your Q10 answer, what should the admin see?

    If Q10 is... Admin sees... Scope Development Effort
    Block Nothing changes up front; error only if they try and it fails No Lowest
    Block Delete option greyed out / disabled in advance Yes Medium
    Block Delete option removed entirely No Medium
    Block Confirmation dialog; block only shows up after they confirm No Low
    Archive No visible difference — disappears the same as a real removal No Lowest
    Archive Confirmation dialog before archiving No Low
    Archive Distinct "archive" icon and tooltip, clear in advance Yes Medium-High

    Graded assignments already used in mastery calculations

    This one is a proposal of mine that still needs your sign-off, not something engineering has already agreed to. My proposal: leave today's behavior as-is, no new warning when editing or deleting a graded assignment a learner has already been credited against. This matches how the platform already treats graded assignments today (parity), and avoids new engineering work here.

    Q12. Do you approve leaving this as-is for the MVP, or do you want a new warning added?

    Option Scope Development Effort
    Approve as-is — no new warning, matches today's platform behavior This is the proposal on the table Lowest, no new work
    Add a new warning here too Not yet scoped Not yet estimated
  11. jmakowski1123 commented on Aug 20, 2026

    @jmakowski1123

    If we went with the "never delete it options" here, is there a way to hide it from the end user in the UI? The assumption here is that an admin doesn't want to use (or have their teams use) that tag going forward. So it were hidden from the UI, but saved on the backend for learners already in progress or who have completed it, is that the best outcome?

  12. 28 remaining items

  13. jesperhodge commented on Sep 2, 2026

    @jesperhodge
    Contributor

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

    @mgwozdz-unicon @ormsbee: while bringing #613's subtasks in line with its latest revision, I hit a case
    where the on_delete values #641 sets and the approved design in this issue disagree. Flagging it
    before either gets built. I have left #641 and #642 declaring PROTECT as a fail-closed value and
    recorded the question as open in both, rather than resolving it myself.

    Explaining it from the ground up, since the interaction is easy to miss.

    The mechanic

    Every Django foreign key has to declare what happens when the row it points at is deleted. Two values
    matter here:

    • CASCADE means "delete me too."
    • PROTECT means "refuse the whole deletion and raise ProtectedError."

    The three players

    • A Tag is a competency, say "Writing Poetry." It lives in openedx_tagging, a generic library used
      across Open edX that knows nothing about CBE.
    • A CompetencyCriteriaGroup is a CBE row saying "to demonstrate Writing Poetry, pass assignment 1 OR
      assignment 2." It holds a foreign key pointing at that Tag.
    • A learner status row records that a specific learner actually earned mastery against the competency.

    The reference direction is what matters: the CBE row points at the Tag, and the Tag knows nothing
    about the CBE row.

    What this issue decided

    When an admin deletes a competency tag, the outcome depends on whether any learner has earned mastery
    against it. The approved table says: archive it if someone has, and hard-delete it, unchanged from
    today's behavior, if nobody has.

    The load-bearing part is the enforcement mechanism. @ormsbee rejected the original proposal, in which
    openedx_tagging would ask CBE "is this in use?", in favor of a push model: CBE stamps a
    deletion_locked flag onto the ObjectTag row when a learner earns something, and at delete time the
    tagging code reads only its own flag. As the design puts it, openedx_tagging never calls into CBE.

    The hole

    Walk the second branch. An author has built a criteria tree for "Writing Poetry," but no learner has
    been graded yet. That is a completely ordinary state, not an edge case: #665 creates the root,
    course-level and leaf CompetencyCriteriaGroup rows at authoring time, long before any grade exists.

    Because nobody has earned anything, deletion_locked is false, so the tagging code proceeds to a plain
    hard delete. But a CompetencyCriteriaGroup row is still pointing at that tag. With PROTECT on that
    foreign key, Django refuses and raises ProtectedError. The admin gets a failure where this issue
    promised a successful delete, and nothing cleans up the CBE row first, because the tagging code is
    deliberately forbidden from knowing the CBE row exists.

    Three things I checked rather than assumed:

    • Both delete paths are plain ORM deletes that go through Django's collector: taxonomy.delete() in
      src/openedx_tagging/rest_api/v1/views.py, and Taxonomy.delete_tags() in
      src/openedx_tagging/models/base.py.
    • Tag.taxonomy is already on_delete=CASCADE, so deleting a taxonomy collects every tag beneath it
      and each one hits the same wall. That makes taxonomy deletion the likeliest instance rather than an
      outlier, since a CompetencyTaxonomy is precisely the taxonomy whose tags carry criteria.
    • Django's PROTECT raises even when the referencing row is in the same delete batch. Only RESTRICT
      exempts that case.

    Two foreign keys cross this boundary: CompetencyCriteriaGroup.tag to oel_tagging_tag, and
    CompetencyCriterion.object_tag to oel_tagging_objecttag. StudentCompetencyStatus.tag is not
    affected, because once learner status exists this issue archives the tag rather than deleting it, and
    archiving is a field update that never reaches on_delete.

    The options as I see them

    Option What it costs
    Keep PROTECT and have the tagging-side delete paths resolve referencing CBE rows before hard-deleting Real work that this issue's design did not scope, and it needs a way to do it without openedx_tagging importing CBE.
    Make the two crossing foreign keys CASCADE Deleting a tag with no learner status then works with no tagging-side work at all, but it silently destroys the author's criteria tree with no archive step. It also needs CompetencyCriterion.group and CompetencyCriteriaGroup.parent to be CASCADE, otherwise the collector walks down and trips their PROTECT instead, so it is closer to a full revert to the earlier database-level design than a two-line change.
    Set the lock at authoring time rather than at learner-status write time Closes the gap by making deletion_locked true whenever criteria exist, but it changes what the flag means and would block deletes this issue says should succeed.

    I do not think this reopens the design. The push-based lock is right for the case it was chosen for,
    which is protecting earned mastery. What is missing is the unlocked branch, where CBE rows exist and no
    mastery has been earned yet.

    Two smaller things from the same pass

  14. ormsbee commented on Sep 2, 2026

    @ormsbee
    Contributor

    @jesperhodge: When you paste in a Claude-generated analysis with options, please note which option you support and why.

  15. mgwozdz-unicon commented on Sep 2, 2026

    @mgwozdz-unicon
    ContributorAuthor

    @jesperhodge @ormsbee
    Thanks for digging into this. After walking through the tradeoff with Claude, I think the two crossing foreign keys, CompetencyCriteriaGroup.tag and CompetencyCriterion.object_tag, should become CASCADE rather than staying PROTECT.

    The case that matters here is a tag with an authored-but-ungraded criteria tree: no learner has earned anything yet, so the delete should succeed, but it currently fails because the criteria rows still point at the tag. Keeping PROTECT and having the tagging-side delete paths clean those rows up first (your option 1) means openedx_tagging has to call into CBE-specific models to do that cleanup, which breaks the "never calls into CBE" boundary this design was built around. I think adhering to the dependency direction boundary of the design takes priority over addressing what feels like a bunch of edge cases to me, so these should become CASCADE. Here's that tradeoff laid out:

    PROTECT (current) CASCADE
    Deleting a tag with no criteria attached Succeeds normally Succeeds normally
    Deleting a tag with a live criteria tree, no learner status yet Fails, this is the bug this thread is about Succeeds, deletes the whole criteria tree along with it
    Deleting a tag through a bug, script, admin action, or future plugin that skips the "should this archive?" check Fails loudly (ProtectedError), nothing is lost Succeeds silently, wipes out the author's competency definition with no warning
    CBE dependency-direction problem (openedx_tagging calling into CBE) Still needs solving Goes away

    One case isn't in the table because it doesn't actually depend on this choice: a tag with a live criteria tree where a learner has also earned status against it. StudentCompetencyStatus.tag, a separate foreign key from the learner's status row to the tag, already stays PROTECT regardless of what we pick here, and Django aborts the whole deletion if it finds even one PROTECT anywhere in what it collects, so that case is protected either way.

    One thing worth flagging before this lands: CompetencyCriterion.group and CompetencyCriteriaGroup.parent are also PROTECT, so the collector would still hit those on its way down and abort partway through a tag deletion. Those would need to become CASCADE too for a tag delete to actually cascade cleanly through the whole tree, and that's a bigger change than just the tag FKs: it's the same backstop #674/#675 relies on to keep a group deletion from bypassing their student-status check, so it removes that protection for direct group deletion too, not only for deletion that starts from the tag. I'm applying the same edge-case reasoning to that as I am to the tag FKs, but flagging it since it's a second place this decision now reaches, not a detail of the first one.

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

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions