Retire flagged exercises instead of deleting them

[oscarlevin]

The problem

The editorial page gives an editor two choices for a flagged question: clear the flag, or delete it. delete_question runs a real DELETE FROM questions, and three tables reference questions.id with ON DELETE CASCADE:

• assignment_questions (models.py:697)
• question_tags (models.py:633)
• question_grades (models.py:931)

So one click removes the exercise from every assignment in every course still using it, along with its points and grading configuration. useinfo.div_id has no foreign key, so the student answer log survives as orphaned rows pointing at a question that no longer exists.

A course can lose an assignment question mid-term, and neither the instructor nor the students get any warning. An editor triaging a flagged exercise has no way to see who depends on it.

What this changes

Retiring instead of deleting. questions.retired_on is stamped (with retired_by), the row stays, and the exercise drops out of the three instructor-facing search paths:

• search_exercises — the assignment builder's exercise search
• fetch_questions_by_search_criteria — /search_questions
• fetch_questions_for_chapter_subchapter — the book-browsing chooser (fetch_chooser_data)

Courses that already assign the exercise are untouched. What changes is that nobody can build a new assignment around it.

Deliberately not filtered, because these are how a live course reaches the exercise: fetch_question, fetch_question_by_id, fetch_matching_questions (selectquestion), fetch_qualified_questions (practice), and the page-progress query in crud/book.py. Retiring is invisible to students.

The editorial page's Delete button becomes Retire; there is no hard-delete path left in the UI.

Why a timestamp rather than a boolean

So the deletion can still happen later, once it is provably safe. rsmanage questions purge deletes exercises that cleared two windows: retired longer than --retired-for-days (default 365), and untouched for --unused-for-years (default 3) by any of four signals:

• an assignment_questions row in a course whose term_start_date falls in the window
• a useinfo row for the div_id — catches reading-page exercises that were never formally assigned
• a row in any registered answer table — iterates runestone_component_dict, so a new @register_answer_table type is covered automatically
• from_source = True — still compiled into the book, so a rebuild would just recreate it

Every check is batched over the whole candidate set rather than run per question; useinfo is far too large to probe a row at a time.

There is no override flag. An exercise still tied to anything is kept and the report says why. It is a dry run unless given --apply.

rsmanage questions listretired
rsmanage questions retire <name> <base_course>
rsmanage questions unretire <name> <base_course>
rsmanage questions purge [--retired-for-days N] [--unused-for-years N] [--base-course X] [--apply]

Migration

c3f7e2a8b491 adds retired_on (nullable DateTime) and retired_by (String(512)), plus a partial index on the retired rows only — every search path tests retired_on IS NULL, which matches nearly every row, so a plain b-tree index would never be chosen; the partial index is small and is what the purge scans.

Single head after this, on top of f3b8d5c2a710.

Testing

test_editor_routes.py is updated for the new endpoint and adds test_retired_question_drops_out_of_search, which asserts the exercise is returned by search_exercises before retiring and absent after.

test/bases/rsptx/admin_server_api/test_editor_routes.py   10 passed
test/components/rsptx/db                                 123 passed
test/bases/rsptx/assignment_server_api                   127 passed

Also exercised by hand in a full docker compose stack: retire from the editorial page, confirm the exercise disappears from both exercise search and the book chooser while the existing assignment using it keeps working.

Note for deployers

Polylith bundles a copy of components/ into each project wheel, so this change needs admin (endpoint + template), assignment (all three search filters run there), and rsmanage (the purge command) rebuilt. It also touches components/rsptx/templates/staticAssets/js/admin/manage_exercises.js, which the proxy serves from a copy baked into its own image — so caddy/nginx needs rebuilding too, or the Retire button throws retireQuestion is not defined against a cached script. book_server does not need rebuilding; none of the filtered functions are called from it.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved purge safety races, unsafe negative retention values, and paths that can clear or bypass retirement remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Replaces hard deletion of flagged exercises with reversible retirement and adds controlled purge tooling.

Changes:

• Adds retirement metadata and migration support.
• Filters retired exercises from instructor-facing searches.
• Updates editor API, UI, and retirement behavior.
• Adds rsmanage questions lifecycle and purge commands.
• Extends retirement/search test coverage.

File summaries

File	Summary
test/bases/rsptx/admin_server_api/test_editor_routes.py	Tests retirement and search exclusion; purge safety scenarios remain untested.
migrations/versions/c3f7e2a8b491_add_retired_to_questions.py	Adds retirement columns and a partial index.
components/rsptx/templates/staticAssets/js/admin/manage_exercises.js	Replaces the delete interaction with retirement.
components/rsptx/templates/admin/editor/manage_exercises.html	Updates editorial controls and wording.
components/rsptx/db/models.py	Adds retirement fields; ordinary edits must preserve them.
components/rsptx/db/crud/question.py	Implements retirement, filtering, and purge checks; unresolved purge race and mutation/update lifecycle issues remain.
components/rsptx/db/crud/__init__.py	Exports the new question CRUD functions.
bases/rsptx/rsmanage/core.py	Adds retirement and purge commands; retention validation, transactional safety, and coverage require changes.
bases/rsptx/admin_server_api/routers/editor.py	Replaces the editor delete endpoint with retirement.

Review details

Suppressed comments (5)

bases/rsptx/rsmanage/core.py:2037

• Both retention windows accept negative values. --retired-for-days -1 includes recently retired (even future-dated) rows, while a negative --unused-for-years moves active_since into the future and suppresses current-use checks, effectively providing an unsafe override despite the command's contract. Reject negative values before calculating these cutoffs.

    retired_before = now - datetime.timedelta(days=retired_for_days)
    active_since = now - datetime.timedelta(days=round(unused_for_years * 365.25))

bases/rsptx/rsmanage/core.py:2051

• This adds a destructive --apply path and four independent retention blockers, but the only rsmanage test remains a smoke import and the new tests cover only the editor endpoint/search. Please add tests for the dry-run/apply boundary and each blocker before relying on this purge to protect active course data.

    in_use = await find_questions_in_use([q.id for q in candidates], active_since)

components/rsptx/db/crud/question.py:143

• The NOT_RETIRED predicate makes a second retire request a no-op for the whole update, so it does not clear review_flag on an already-retired question. An instructor can flag a retired question through the existing instructor route; it then reappears on the editorial page, and clicking Retire leaves it flagged. Make the idempotent update clear review_flag while preserving the original retirement timestamp and editor.

    stmt = (
        update(Question)
        .where(
            (Question.name == name)
            & (Question.base_course == base_course)
            & NOT_RETIRED
        )
        .values(retired_on=canonical_utcnow(), retired_by=retired_by, review_flag=False)

components/rsptx/db/crud/question.py:110

• Filtering the three search queries does not enforce the retirement invariant on assignment mutations. new_assignment_question still accepts any caller-supplied question_id, and copy_question_endpoint accepts any original_question_id; both paths can be reached with an existing assignment's retired ID and respectively attach it or create a new assignable copy. Validate retired_on IS NULL in those mutation paths so a stale client/API call cannot create a new assignment around a retired exercise.

# Every instructor-facing exercise search hangs this on its where clause.
# Retired exercises stay readable by name/id -- the courses that already assign
# them keep working -- they simply stop being discoverable in new assignments.
NOT_RETIRED = Question.retired_on.is_(None)

components/rsptx/db/models.py:623

• QuestionValidator now includes these nullable columns, but update_question() writes every field. The instructor update route constructs a fresh validator without retired_on/retired_by, so editing a retired exercise writes both as NULL, makes it searchable again, and loses the audit actor. Preserve these lifecycle fields when updating ordinary question content, or make that update partial.

    retired_on = Column(DateTime)
    retired_by = Column(String(512))  # username of the editor who retired it

• Files reviewed: 9/9 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.