From ab8afd1ba109011ad9701b75125a4f520ade5b45 Mon Sep 17 00:00:00 2001 From: Oscar Levin Date: Wed, 16 Sep 2026 22:58:20 -0600 Subject: [PATCH] Retire flagged exercises instead of deleting them The editorial page let an editor delete a flagged question outright. assignment_questions, question_tags and question_grades all reference questions.id with ON DELETE CASCADE, so that one click removed the exercise from every assignment in every course still using it, along with its grading configuration, while leaving the useinfo rows orphaned. A course could lose an assignment question mid-term with no warning to its instructor. Retiring stamps questions.retired_on instead. The row stays, so courses that already assign the exercise carry on untouched; what changes is that it drops out of the three instructor-facing search paths, so nobody can build a new assignment around it. The student-facing lookups (fetch_question, fetch_matching_questions, fetch_qualified_questions, the page-progress query) are deliberately left alone -- that is how a live course reaches the exercise. retired_on is a timestamp rather than a flag so the deletion can happen later, once it is safe: rsmanage questions purge deletes exercises that have been retired past a grace period and that nothing has referenced since a cutoff -- no assignment in a course whose term started in the window, no useinfo activity, no rows in any registered answer table, and not still compiled into the book source. 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. Co-Authored-By: Claude Opus 5 --- .../rsptx/admin_server_api/routers/editor.py | 22 +- bases/rsptx/rsmanage/core.py | 159 ++++++++++++ components/rsptx/db/crud/__init__.py | 18 ++ components/rsptx/db/crud/question.py | 231 +++++++++++++++++- components/rsptx/db/models.py | 6 + .../admin/editor/manage_exercises.html | 12 +- .../staticAssets/js/admin/manage_exercises.js | 16 +- .../c3f7e2a8b491_add_retired_to_questions.py | 49 ++++ .../admin_server_api/test_editor_routes.py | 70 ++++-- 9 files changed, 543 insertions(+), 40 deletions(-) create mode 100644 migrations/versions/c3f7e2a8b491_add_retired_to_questions.py diff --git a/bases/rsptx/admin_server_api/routers/editor.py b/bases/rsptx/admin_server_api/routers/editor.py index 3e7846f44..81b1686dc 100644 --- a/bases/rsptx/admin_server_api/routers/editor.py +++ b/bases/rsptx/admin_server_api/routers/editor.py @@ -17,13 +17,13 @@ from rsptx.auth.session import auth_manager from rsptx.configuration import settings from rsptx.db.crud import ( - delete_question_by_name, fetch_all_course_attributes, fetch_course, fetch_editor_basecourses, fetch_flagged_questions, fetch_question, get_book_chapters, + retire_question_by_name, update_question, ) from rsptx.endpoint_validators import editor_role_required @@ -139,25 +139,33 @@ async def _editable_question(user, body: QuestionRequest): return question, None -@router.post("/delete_question", response_class=JSONResponse) +@router.post("/retire_question", response_class=JSONResponse) @editor_role_required() -async def delete_question( +async def retire_question( request: Request, body: QuestionRequest, user=Depends(auth_manager), ): - """Permanently remove a flagged question from the questions table.""" + """Take a flagged question out of circulation. + + The row is kept on purpose. Courses that already assign the exercise carry + on unchanged -- deleting it would cascade through ``assignment_questions`` + and pull the exercise out from under a course that may be mid-term. What + changes is that the exercise stops appearing in exercise search, so nobody + can build a *new* assignment around it. ``rsmanage questions purge`` + deletes retired exercises later, once it can prove nothing depends on them. + """ question, err = await _editable_question(user, body) if err: return err try: - await delete_question_by_name(body.name, body.base_course) + await retire_question_by_name(body.name, body.base_course, user.username) except Exception as e: - rslogger.error(f"Error deleting question {body.name}: {e}") + rslogger.error(f"Error retiring question {body.name}: {e}") return make_json_response( status=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail={"status": "Error", "message": f"Failed to delete: {e}"}, + detail={"status": "Error", "message": f"Failed to retire: {e}"}, ) return make_json_response(detail={"status": "Success"}) diff --git a/bases/rsptx/rsmanage/core.py b/bases/rsptx/rsmanage/core.py index 6b73476ca..aca85b233 100644 --- a/bases/rsptx/rsmanage/core.py +++ b/bases/rsptx/rsmanage/core.py @@ -46,7 +46,12 @@ create_library_book, create_user_course_entry, delete_course_completely, + delete_questions_by_id, delete_user, + fetch_retired_questions, + find_questions_in_use, + retire_question_by_name, + unretire_question_by_name, fetch_all_course_attributes, fetch_assignments, fetch_course, @@ -1920,5 +1925,159 @@ async def telemetry(config, send): await term_models() +# command group for managing the questions (exercise) table + + +@cli.group() +@click.pass_context +def questions(ctx): + """subcommands for managing retired exercises""" + pass + + +@questions.command("listretired") +@click.pass_context +@click.option("--base-course", default=None, help="Limit to one base course") +async def questions_listretired(ctx, base_course): + """ + List exercises editors have retired, newest retirement last. + """ + retired = await fetch_retired_questions(canonical_utcnow(), base_course) + if not retired: + click.echo("No exercises are retired.") + return + + for q in retired: + click.echo( + f"{q.retired_on:%Y-%m-%d} {q.base_course:30} {q.name:40} " + f"retired by {q.retired_by or 'unknown'}" + ) + click.echo(f"\n{len(retired)} retired exercise(s).") + + +@questions.command("retire") +@click.pass_context +@click.argument("name") +@click.argument("base_course") +async def questions_retire(ctx, name, base_course): + """ + Retire exercise NAME in BASE_COURSE: take it out of exercise search while + leaving every course that already assigns it untouched. + """ + count = await retire_question_by_name(name, base_course, "rsmanage") + if count: + click.echo(f"Retired {name}. Courses already using it are unaffected.") + else: + click.echo(f"No searchable exercise named {name} in {base_course}.") + sys.exit(-1) + + +@questions.command("unretire") +@click.pass_context +@click.argument("name") +@click.argument("base_course") +async def questions_unretire(ctx, name, base_course): + """ + Put retired exercise NAME in BASE_COURSE back into circulation. + """ + count = await unretire_question_by_name(name, base_course) + if count: + click.echo(f"{name} is searchable again.") + else: + click.echo(f"No exercise named {name} in {base_course}.") + sys.exit(-1) + + +def _purge_blockers_to_report(question, reasons): + """Format one kept exercise for the report.""" + return f" keep {question.base_course}/{question.name}: " + ", ".join(reasons) + + +@questions.command("purge") +@click.pass_context +@click.option( + "--retired-for-days", + default=365, + show_default=True, + help="Only consider exercises retired at least this long ago", +) +@click.option( + "--unused-for-years", + default=3.0, + show_default=True, + help="An exercise counts as unused if nothing has touched it in this many years", +) +@click.option("--base-course", default=None, help="Limit to one base course") +@click.option( + "--apply", + "apply_changes", + is_flag=True, + help="Actually delete. Without this the command only reports what it would do.", +) +async def questions_purge( + ctx, retired_for_days, unused_for_years, base_course, apply_changes +): + """ + Permanently delete retired exercises that nothing depends on any more. + + An exercise is deleted only when it cleared both windows: it was retired at + least --retired-for-days ago, and for the last --unused-for-years nothing + has referenced it -- no assignment in a course whose term started in that + window, no student activity, no graded answers -- and it is no longer part + of the book source. + + There is deliberately no way to override those checks. Deletion cascades + into assignment_questions, question_tags and question_grades, so an + exercise that is still tied to anything is simply kept, and the report says + why. If you want one gone regardless, remove whatever still references it + first. The default is a dry run; pass --apply to go through with it. + """ + now = canonical_utcnow() + retired_before = now - datetime.timedelta(days=retired_for_days) + active_since = now - datetime.timedelta(days=round(unused_for_years * 365.25)) + + candidates = await fetch_retired_questions(retired_before, base_course) + if not candidates: + click.echo( + f"No exercises have been retired since before {retired_before:%Y-%m-%d}." + ) + return + + click.echo( + f"{len(candidates)} exercise(s) retired before {retired_before:%Y-%m-%d}; " + f"checking for use since {active_since:%Y-%m-%d}." + ) + + in_use = await find_questions_in_use([q.id for q in candidates], active_since) + + purge = [] + keep = [] + for q in candidates: + reasons = in_use.get(q.id, []) + if reasons: + keep.append((q, reasons)) + else: + purge.append(q) + + for q, reasons in keep: + click.echo(_purge_blockers_to_report(q, reasons)) + for q in purge: + click.echo( + f" PURGE {q.base_course}/{q.name} (retired {q.retired_on:%Y-%m-%d})" + ) + + click.echo(f"\n{len(purge)} to delete, {len(keep)} kept.") + + if not purge: + return + + if not apply_changes: + click.echo("Dry run -- nothing deleted. Re-run with --apply to delete.") + return + + deleted = await delete_questions_by_id([q.id for q in purge]) + click.echo(f"Deleted {deleted} exercise(s).") + + if __name__ == "__main__": cli(_anyio_backend="asyncio") diff --git a/components/rsptx/db/crud/__init__.py b/components/rsptx/db/crud/__init__.py index 9aeb8810c..6ad035522 100644 --- a/components/rsptx/db/crud/__init__.py +++ b/components/rsptx/db/crud/__init__.py @@ -209,6 +209,15 @@ from .question import ( count_matching_questions, delete_question_by_name, + delete_questions_by_id, + find_questions_in_use, + IN_USE_ACTIVITY, + IN_USE_ANSWERS, + IN_USE_ASSIGNED, + IN_USE_FROM_SOURCE, + retire_question_by_name, + unretire_question_by_name, + fetch_retired_questions, create_question_grade_entry, create_question, create_user_experiment_entry, @@ -500,6 +509,15 @@ __all__ += [ "count_matching_questions", "delete_question_by_name", + "delete_questions_by_id", + "find_questions_in_use", + "IN_USE_ACTIVITY", + "IN_USE_ANSWERS", + "IN_USE_ASSIGNED", + "IN_USE_FROM_SOURCE", + "retire_question_by_name", + "unretire_question_by_name", + "fetch_retired_questions", "create_question", "create_question_grade_entry", "create_user_experiment_entry", diff --git a/components/rsptx/db/crud/question.py b/components/rsptx/db/crud/question.py index 7d3c23933..daf5b50a3 100644 --- a/components/rsptx/db/crud/question.py +++ b/components/rsptx/db/crud/question.py @@ -1,5 +1,6 @@ import re -from typing import List, Optional, Tuple, Dict +from datetime import datetime +from typing import List, Optional, Tuple, Dict, Set from sqlalchemy import select, and_, or_, func, asc, desc, not_, update, delete from sqlalchemy.exc import IntegrityError @@ -10,6 +11,7 @@ Chapter, ChapterValidator, Competency, + Courses, Question, QuestionGrade, QuestionGradeValidator, @@ -20,6 +22,7 @@ Useinfo, UserExperiment, UserExperimentValidator, + runestone_component_dict, ) from ..async_session import async_session from rsptx.validation import schemas @@ -101,11 +104,82 @@ async def fetch_flagged_questions(base_course: str) -> List[QuestionValidator]: return [QuestionValidator.from_orm(x) for x in res.scalars().fetchall()] +# 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) + + +async def retire_question_by_name( + name: str, base_course: str, retired_by: Optional[str] = None +) -> int: + """ + Retire a question: take it out of the searchable exercise pool without + removing the row. ``(base_course, name)`` is unique, so at most one row is + touched. + + Retiring is idempotent -- a question already retired keeps its original + ``retired_on``, so the purge grace period is measured from the first + editorial decision rather than being reset by a second click. + + The review flag is cleared in the same statement. Retiring settles the + review, and doing it here rather than through ``update_question`` keeps it + atomic: ``update_question`` rewrites every column from whatever the caller + read earlier, which for a row this function just changed would put + ``retired_on`` straight back to NULL. + + :param name: str, the name (div_id) of the question + :param base_course: str, the base course the question belongs to + :param retired_by: Optional[str], username of the editor making the call + :return: int, the number of rows retired (0 if it was already retired) + """ + 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) + ) + + async with async_session.begin() as session: + res = await session.execute(stmt) + return res.rowcount + + +async def unretire_question_by_name(name: str, base_course: str) -> int: + """ + Put a retired question back into circulation. + + :param name: str, the name (div_id) of the question + :param base_course: str, the base course the question belongs to + :return: int, the number of rows restored + """ + stmt = ( + update(Question) + .where((Question.name == name) & (Question.base_course == base_course)) + .values(retired_on=None, retired_by=None) + ) + + async with async_session.begin() as session: + res = await session.execute(stmt) + return res.rowcount + + async def delete_question_by_name(name: str, base_course: str) -> int: """ Delete a question identified by its name (div_id) within a base course. ``(base_course, name)`` is unique, so at most one row is removed. + .. warning:: + ``assignment_questions``, ``question_tags`` and ``question_grades`` all + reference ``questions.id`` with ``ON DELETE CASCADE``, so this removes + the exercise from every assignment that uses it, in every course, along + with its grading configuration. Interactive callers should use + :func:`retire_question_by_name` instead; this is for the purge script, + which first proves nothing depends on the row. + :param name: str, the name (div_id) of the question :param base_course: str, the base course the question belongs to :return: int, the number of rows deleted @@ -119,6 +193,156 @@ async def delete_question_by_name(name: str, base_course: str) -> int: return res.rowcount +async def fetch_retired_questions( + retired_before: datetime, base_course: Optional[str] = None +) -> List[QuestionValidator]: + """ + Fetch every question retired on or before ``retired_before``. + + The purge script uses this to build its candidate list: a question has to + have been out of circulation for the full grace period before it is even + considered for deletion. + + :param retired_before: datetime, the newest ``retired_on`` to include + :param base_course: Optional[str], restrict to a single base course + :return: List[QuestionValidator], oldest retirement first + """ + query = ( + select(Question) + .where( + Question.retired_on.isnot(None) & (Question.retired_on <= retired_before) + ) + .order_by(Question.retired_on, Question.base_course, Question.name) + ) + if base_course: + query = query.where(Question.base_course == base_course) + + async with async_session() as session: + res = await session.execute(query) + return [QuestionValidator.from_orm(q) for q in res.scalars().fetchall()] + + +# The reasons a retired question is still worth keeping. The purge script prints +# these verbatim, so they read as explanations rather than as codes. +IN_USE_FROM_SOURCE = "still in the book source" +IN_USE_ASSIGNED = "assigned in an active course" +IN_USE_ACTIVITY = "recent student activity" +IN_USE_ANSWERS = "recent graded answers" + + +async def find_questions_in_use( + question_ids: List[int], active_since: datetime +) -> Dict[int, List[str]]: + """ + Work out which of ``question_ids`` are still in use, and why. + + Four independent signals, matching the four ways an exercise can still + matter to somebody: + + * ``from_source`` -- the exercise is still compiled into the book, so a + rebuild would simply recreate the row. + * an ``assignment_questions`` row belonging to a course whose term started + on or after ``active_since``. + * a ``useinfo`` row for the question's ``div_id`` since ``active_since``. + This catches reading-page exercises that were never formally assigned. + * a row in any registered answer table since ``active_since``. Narrower + than ``useinfo``, but it points at real graded work. + + Every check is batched over the whole candidate list rather than run per + question -- ``useinfo`` in particular is far too large to probe one row at + a time. + + :param question_ids: List[int], the candidate question ids + :param active_since: datetime, the start of the "still in use" window + :return: Dict[int, List[str]], candidate id -> reasons it is in use. + A question with no reasons is absent from the dict. + """ + if not question_ids: + return {} + + in_use: Dict[int, List[str]] = {} + + def mark(ids, reason: str) -> None: + for qid in ids: + in_use.setdefault(qid, []).append(reason) + + async with async_session() as session: + # div_id is the join key for useinfo and the answer tables, which store + # the question's name rather than its id. + name_rows = await session.execute( + select(Question.id, Question.name, Question.from_source).where( + Question.id.in_(question_ids) + ) + ) + ids_by_name: Dict[str, Set[int]] = {} + from_source_ids = [] + for qid, name, from_source in name_rows: + ids_by_name.setdefault(name, set()).add(qid) + if from_source: + from_source_ids.append(qid) + mark(from_source_ids, IN_USE_FROM_SOURCE) + + names = list(ids_by_name) + + assigned = await session.execute( + select(AssignmentQuestion.question_id) + .join(Assignment, Assignment.id == AssignmentQuestion.assignment_id) + .join(Courses, Courses.id == Assignment.course) + .where( + AssignmentQuestion.question_id.in_(question_ids) + & (Courses.term_start_date >= active_since.date()) + ) + .distinct() + ) + mark(assigned.scalars().fetchall(), IN_USE_ASSIGNED) + + active_names = await session.execute( + select(Useinfo.div_id) + .where(Useinfo.div_id.in_(names) & (Useinfo.timestamp >= active_since)) + .distinct() + ) + for name in active_names.scalars().fetchall(): + mark(ids_by_name.get(name, ()), IN_USE_ACTIVITY) + + # runestone_component_dict is the registry every @register_answer_table + # model joins; iterating it means a new question type's answers are + # honoured here without anyone remembering to update this list. + answered_names: Set[str] = set() + for component in runestone_component_dict.values(): + model = component.model + rows = await session.execute( + select(model.div_id) + .where(model.div_id.in_(names) & (model.timestamp >= active_since)) + .distinct() + ) + answered_names.update(rows.scalars().fetchall()) + for name in answered_names: + mark(ids_by_name.get(name, ()), IN_USE_ANSWERS) + + return in_use + + +async def delete_questions_by_id(question_ids: List[int]) -> int: + """ + Delete questions by primary key. Used by the purge script once + :func:`find_questions_in_use` has cleared them. + + .. warning:: + See :func:`delete_question_by_name` -- this cascades into + ``assignment_questions``, ``question_tags`` and ``question_grades``. + + :param question_ids: List[int], the questions to remove + :return: int, the number of rows deleted + """ + if not question_ids: + return 0 + + stmt = delete(Question).where(Question.id.in_(question_ids)) + async with async_session.begin() as session: + res = await session.execute(stmt) + return res.rowcount + + async def count_matching_questions(name: str) -> int: """ Count the number of Question entries that match the given name. @@ -224,7 +448,7 @@ async def fetch_questions_by_search_criteria( raise ValueError("No search criteria provided") # todo: add support for tags - query = select(Question).where(and_(*where_criteria)) + query = select(Question).where(and_(NOT_RETIRED, *where_criteria)) rslogger.debug(f"{query=}") async with async_session() as session: res = await session.execute(query) @@ -243,7 +467,7 @@ async def search_exercises( :return: Dictionary with search results and pagination metadata """ # Base query - query = select(Question).where(Question.question_type != "page") + query = select(Question).where((Question.question_type != "page") & NOT_RETIRED) # If base_course is provided, filter by base_course if criteria.base_course: @@ -784,6 +1008,7 @@ async def fetch_questions_for_chapter_subchapter( Question.owner == None, # noqa: E711 ) ), # noqa: E712 + NOT_RETIRED, skipr_clause, froms_clause, page_clause, diff --git a/components/rsptx/db/models.py b/components/rsptx/db/models.py index cf54be033..985c2dd80 100644 --- a/components/rsptx/db/models.py +++ b/components/rsptx/db/models.py @@ -615,6 +615,12 @@ class Question(Base, IdMixin): String(512) ) # username of the owner of the question (Author could be any name) tags = Column(String(512)) # comma separated list of tags + # When an editor pulls this exercise out of circulation. NULL means it is + # live. A retired exercise disappears from every exercise-search path but + # keeps working for the courses that already assign it -- deleting the row + # would cascade into assignment_questions and break those courses mid-term. + retired_on = Column(DateTime) + retired_by = Column(String(512)) # username of the editor who retired it QuestionValidator: TypeAlias = sqlalchemy_to_pydantic(Question) # type: ignore diff --git a/components/rsptx/templates/admin/editor/manage_exercises.html b/components/rsptx/templates/admin/editor/manage_exercises.html index 9d3cce252..0774b499a 100644 --- a/components/rsptx/templates/admin/editor/manage_exercises.html +++ b/components/rsptx/templates/admin/editor/manage_exercises.html @@ -64,9 +64,11 @@

Books you edit

Questions for Review

-

Please delete the questions that are clearly inappropriate or just - experimental. If a question is fine as it stands, clear its flag to take - it off this list.

+

Please retire the questions that are clearly inappropriate or just + experimental. Retiring keeps the question working for courses that + already assign it, but takes it out of exercise search so it cannot be + added to anything new. If a question is fine as it stands, clear its + flag to take it off this list.

{% if questions %}
@@ -85,8 +87,8 @@

Questions for Review