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