From d591ba4635158aba777b5130bbed36b750d531fd Mon Sep 17 00:00:00 2001 From: Andrey Morozov Date: Wed, 23 Sep 2026 18:45:50 +0200 Subject: [PATCH 1/4] f-1257 add Editorial interface for questions --- .../rsptx/admin_server_api/routers/editor.py | 148 ++++++++++++++++-- components/rsptx/db/crud/__init__.py | 2 + components/rsptx/db/crud/question.py | 30 +++- .../templates/admin/editor/edit_question.html | 72 +++++++++ .../admin/editor/manage_exercises.html | 10 +- .../staticAssets/js/admin/manage_exercises.js | 36 ++++- .../admin_server_api/test_editor_routes.py | 135 +++++++++++++++- 7 files changed, 404 insertions(+), 29 deletions(-) create mode 100644 components/rsptx/templates/admin/editor/edit_question.html diff --git a/bases/rsptx/admin_server_api/routers/editor.py b/bases/rsptx/admin_server_api/routers/editor.py index 3e7846f44..7ba5a4cf1 100644 --- a/bases/rsptx/admin_server_api/routers/editor.py +++ b/bases/rsptx/admin_server_api/routers/editor.py @@ -18,17 +18,23 @@ from rsptx.configuration import settings from rsptx.db.crud import ( delete_question_by_name, + fetch_assigned_question_ids, fetch_all_course_attributes, fetch_course, fetch_editor_basecourses, fetch_flagged_questions, fetch_question, + fetch_question_by_id, get_book_chapters, update_question, ) from rsptx.endpoint_validators import editor_role_required from rsptx.logging import rslogger -from rsptx.response_helpers.core import get_webpack_static_imports, make_json_response +from rsptx.response_helpers.core import ( + canonical_utcnow, + get_webpack_static_imports, + make_json_response, +) from rsptx.templates import get_shared_templates router = APIRouter( @@ -63,12 +69,12 @@ async def manage_exercises( ): """ Display every question flagged for review in the base courses this user - edits, with controls to delete a question or clear its flag. + edits, with controls to edit or delete a question, or clear its flag. """ course = await fetch_course(user.course_name) base_courses = await fetch_editor_basecourses(user.id) - questions = [] + flagged_questions = [] # Chapter labels are what the questions table stores; map them to the # human-readable chapter titles, per base course. chapter_titles = {} @@ -77,19 +83,24 @@ async def manage_exercises( chapter.chapter_label: chapter.chapter_name for chapter in await get_book_chapters(base_course) } - for q in await fetch_flagged_questions(base_course): - questions.append( - { - "name": q.name, - "base_course": q.base_course, - "chapter": q.chapter, - "chapter_title": chapter_titles[base_course].get(q.chapter, ""), - "subchapter": q.subchapter, - "difficulty": q.difficulty, - "question_type": q.question_type, - "htmlsrc": q.htmlsrc, - } - ) + flagged_questions.extend(await fetch_flagged_questions(base_course)) + + assigned_ids = await fetch_assigned_question_ids(q.id for q in flagged_questions) + questions = [ + { + "id": q.id, + "name": q.name, + "base_course": q.base_course, + "chapter": q.chapter, + "chapter_title": chapter_titles[q.base_course].get(q.chapter, ""), + "subchapter": q.subchapter, + "difficulty": q.difficulty, + "question_type": q.question_type, + "htmlsrc": q.htmlsrc, + "assigned": q.id in assigned_ids, + } + for q in flagged_questions + ] course_attrs = await fetch_all_course_attributes(course.id) templates = get_shared_templates() @@ -115,6 +126,12 @@ class QuestionRequest(BaseModel): base_course: str +class QuestionEditRequest(BaseModel): + question: str + htmlsrc: str + difficulty: float | None = None + + async def _editable_question(user, body: QuestionRequest): """Resolve the question named in a request, but only if the caller edits the base course it belongs to. Returns ``(question, error_response)``. @@ -139,6 +156,85 @@ async def _editable_question(user, body: QuestionRequest): return question, None +async def _editable_question_by_id(user, question_id: int): + """Resolve a question id only when the caller edits its base course.""" + question = await fetch_question_by_id(question_id) + if not question: + return None, make_json_response( + status=status.HTTP_404_NOT_FOUND, + detail={"status": "Error", "message": "Question not found."}, + ) + + base_courses = await fetch_editor_basecourses(user.id) + if question.base_course not in base_courses: + return None, make_json_response( + status=status.HTTP_403_FORBIDDEN, + detail={"status": "Error", "message": "You do not edit that base course."}, + ) + return question, None + + +@router.get("/questions/{question_id}/edit", response_class=HTMLResponse) +@editor_role_required() +async def edit_question_page( + request: Request, + question_id: int, + user=Depends(auth_manager), +): + """Display the focused editor for a question in the review queue.""" + question, err = await _editable_question_by_id(user, question_id) + if err: + return err + + course = await fetch_course(user.course_name) + course_attrs = await fetch_all_course_attributes(course.id) + context = { + "request": request, + "user": user, + "course": course, + "is_instructor": True, + "student_page": False, + "question": question, + "wp_imports": _safe_webpack_imports(course), + "course_attrs": course_attrs, + "latex_preamble": course_attrs.get("latex_macros", ""), + "webwork_js_version": course_attrs.get("webwork_js_version", "2.20"), + "settings": settings, + } + return get_shared_templates().TemplateResponse( + "admin/editor/edit_question.html", context + ) + + +@router.post("/questions/{question_id}/edit", response_class=JSONResponse) +@editor_role_required() +async def edit_question( + request: Request, + question_id: int, + body: QuestionEditRequest, + user=Depends(auth_manager), +): + """Save editorial changes without allowing the question id to change.""" + question, err = await _editable_question_by_id(user, question_id) + if err: + return err + + question.question = body.question + question.htmlsrc = body.htmlsrc + question.difficulty = body.difficulty + question.timestamp = canonical_utcnow() + try: + await update_question(question) + except Exception as e: + rslogger.error(f"Error updating question {question.name}: {e}") + return make_json_response( + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail={"status": "Error", "message": f"Failed to update: {e}"}, + ) + + return make_json_response(detail={"status": "Success"}) + + @router.post("/delete_question", response_class=JSONResponse) @editor_role_required() async def delete_question( @@ -151,8 +247,17 @@ async def delete_question( if err: return err + if question.id in await fetch_assigned_question_ids([question.id]): + return make_json_response( + status=status.HTTP_409_CONFLICT, + detail={ + "status": "Error", + "message": "Assigned questions cannot be deleted.", + }, + ) + try: - await delete_question_by_name(body.name, body.base_course) + deleted = await delete_question_by_name(body.name, body.base_course) except Exception as e: rslogger.error(f"Error deleting question {body.name}: {e}") return make_json_response( @@ -160,6 +265,15 @@ async def delete_question( detail={"status": "Error", "message": f"Failed to delete: {e}"}, ) + if not deleted: + return make_json_response( + status=status.HTTP_409_CONFLICT, + detail={ + "status": "Error", + "message": "The question was assigned or changed before deletion.", + }, + ) + return make_json_response(detail={"status": "Success"}) diff --git a/components/rsptx/db/crud/__init__.py b/components/rsptx/db/crud/__init__.py index 3c0a01610..25f9a8189 100644 --- a/components/rsptx/db/crud/__init__.py +++ b/components/rsptx/db/crud/__init__.py @@ -221,6 +221,7 @@ fetch_question, fetch_question_by_id, fetch_questions_by_name, + fetch_assigned_question_ids, fetch_flagged_questions, fetch_questions_for_chapter_subchapter, fetch_question_count_per_subchapter, @@ -515,6 +516,7 @@ "fetch_question", "fetch_questions_by_name", "fetch_question_by_id", + "fetch_assigned_question_ids", "fetch_flagged_questions", "fetch_questions_for_chapter_subchapter", "fetch_question_count_per_subchapter", diff --git a/components/rsptx/db/crud/question.py b/components/rsptx/db/crud/question.py index 967a2c79c..0f0658677 100644 --- a/components/rsptx/db/crud/question.py +++ b/components/rsptx/db/crud/question.py @@ -1,6 +1,6 @@ import re import inspect -from typing import Dict, Iterable, List, Optional, Tuple +from typing import Dict, Iterable, List, Optional, Set, Tuple from sqlalchemy import select, and_, or_, func, asc, desc, not_, update, delete from sqlalchemy.exc import IntegrityError @@ -126,8 +126,7 @@ async def fetch_flagged_questions(base_course: str) -> List[QuestionValidator]: query = ( select(Question) .where( - (Question.base_course == base_course) - & (Question.review_flag == True) # noqa: E712 + (Question.base_course == base_course) & (Question.review_flag == True) # noqa: E712 ) .order_by(Question.chapter, Question.name) ) @@ -137,17 +136,36 @@ async def fetch_flagged_questions(base_course: str) -> List[QuestionValidator]: return [QuestionValidator.from_orm(x) for x in res.scalars().fetchall()] +async def fetch_assigned_question_ids(question_ids: Iterable[int]) -> Set[int]: + """Return the question ids that are referenced by at least one assignment.""" + ids = set(question_ids) + if not ids: + return set() + + query = select(AssignmentQuestion.question_id).where( + AssignmentQuestion.question_id.in_(ids) + ) + async with async_session() as session: + res = await session.execute(query) + return set(res.scalars().all()) + + 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. + Delete an unassigned question identified by its name (div_id) within a base + course. ``(base_course, name)`` is unique, so at most one row is removed. :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 """ + assigned = ( + select(AssignmentQuestion.id) + .where(AssignmentQuestion.question_id == Question.id) + .exists() + ) stmt = delete(Question).where( - (Question.name == name) & (Question.base_course == base_course) + (Question.name == name) & (Question.base_course == base_course) & ~assigned ) async with async_session.begin() as session: diff --git a/components/rsptx/templates/admin/editor/edit_question.html b/components/rsptx/templates/admin/editor/edit_question.html new file mode 100644 index 000000000..23fa37680 --- /dev/null +++ b/components/rsptx/templates/admin/editor/edit_question.html @@ -0,0 +1,72 @@ +{% extends "admin/_admin_base.html" %} +{% set needs_jquery = true %} + +{% block title %} +Edit Question +{% endblock %} + +{% block page_css %} +{% include 'common/static_assets.html' %} + +{% endblock %} + +{% block content %} +{% include 'common/ebook_config.html' %} + +
+
+

Edit Question

+

+ {{ question.name }} in + {{ question.base_course }} +

+
+ +
+ +
+
+ + +
+ +
+ +

+ Keep this synchronized with the source so the updated question is + visible immediately. The question identifier cannot be changed. +

+ +
+ +
+ + +
+ +
+ + Cancel +
+
+
+{% endblock %} + +{% block page_js %} + +{% endblock %} diff --git a/components/rsptx/templates/admin/editor/manage_exercises.html b/components/rsptx/templates/admin/editor/manage_exercises.html index 9d3cce252..49477e977 100644 --- a/components/rsptx/templates/admin/editor/manage_exercises.html +++ b/components/rsptx/templates/admin/editor/manage_exercises.html @@ -64,9 +64,9 @@

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 edit questions with minor issues, delete questions that are clearly + inappropriate or experimental, or clear the flag when a question is fine + as it stands. Assigned questions cannot be deleted.

{% if questions %}
@@ -85,9 +85,13 @@

Questions for Review

+ + Edit + + {% if q.has_question_json %} + Edit + {% else %} + + + + {% endif %} + +
- -
- -
-

Books you edit

-

{{ base_courses | join(', ') if base_courses else 'None -- ask a Runestone admin to add you as an editor of a book.' }}

-
- -

Questions for Review

-

Please edit questions with minor issues, delete questions that are clearly - inappropriate or experimental, or clear the flag when a question is fine - as it stands. Assigned questions cannot be deleted.

- - {% if questions %} -
- {% for q in questions %} -
-
- Base Course: {{ q.base_course }} - Difficulty: {{ q.difficulty if q.difficulty is not none else 'unrated' }} - Chapter: {{ q.chapter_title or q.chapter }} - Question: {{ q.name }} -
- -
- {{ q.htmlsrc | safe }} -
- -
- - - Edit - - -
-
- {% endfor %} -
- {% else %} -

No questions are currently flagged for review.

- {% endif %} + {% endfor %} + + {% else %} +

No questions are currently flagged for review.

+ {% endif %} -{% endblock %} - -{% block page_js %} +{% endblock %} {% block page_js %} {% endblock %} diff --git a/components/rsptx/templates/staticAssets/js/admin/manage_exercises.js b/components/rsptx/templates/staticAssets/js/admin/manage_exercises.js index c10d923f0..7efc1260c 100644 --- a/components/rsptx/templates/staticAssets/js/admin/manage_exercises.js +++ b/components/rsptx/templates/staticAssets/js/admin/manage_exercises.js @@ -63,17 +63,24 @@ function clearFlag(qname, baseCourse, cardId) { async function saveQuestionEdit(event) { event.preventDefault(); const form = event.currentTarget; - const difficultyValue = form.elements.difficulty.value; - const body = { - question: form.elements.question.value, - htmlsrc: form.elements.htmlsrc.value, - difficulty: difficultyValue === "" ? null : Number(difficultyValue) - }; + let questionJson; + + try { + questionJson = JSON.parse(form.elements.question_json.value); + } catch (error) { + showAlert(`Question JSON is invalid: ${error.message}`, "error"); + return; + } + + if (questionJson === null || Array.isArray(questionJson) || typeof questionJson !== "object") { + showAlert("Question JSON must be an object.", "error"); + return; + } try { const data = await postJSON( `/admin/editor/questions/${form.dataset.questionId}/edit`, - body + { question_json: questionJson } ); if (data.detail && data.detail.status === "Success") { window.location.assign("/admin/editor/manage_exercises"); diff --git a/test/bases/rsptx/admin_server_api/test_editor_routes.py b/test/bases/rsptx/admin_server_api/test_editor_routes.py index f218d2a17..2f9ea2fef 100644 --- a/test/bases/rsptx/admin_server_api/test_editor_routes.py +++ b/test/bases/rsptx/admin_server_api/test_editor_routes.py @@ -20,6 +20,7 @@ fetch_assignment_by_name, fetch_course, fetch_question, + update_question, ) from rsptx.db.models import ( # noqa: E402 AssignmentQuestionValidator, @@ -33,10 +34,20 @@ OTHER_BASE_COURSE = "fopp" -async def _make_question(name, base_course=EDITED_BASE_COURSE, flagged=True): +async def _make_question( + name, base_course=EDITED_BASE_COURSE, flagged=True, with_question_json=True +): """Create (or return) a question, flagged for review by default.""" + question_json = ( + {"type": "shortanswer", "prompt": "Flagged for review?"} + if with_question_json + else None + ) existing = await fetch_question(name, basecourse=base_course) if existing: + existing.question_json = question_json + existing.review_flag = flagged + await update_question(existing) return existing return await create_question( QuestionValidator( @@ -49,6 +60,7 @@ async def _make_question(name, base_course=EDITED_BASE_COURSE, flagged=True): htmlsrc=f"

html for {name}

", timestamp=canonical_utcnow(), question_type="shortanswer", + question_json=question_json, is_private=False, from_source=False, review_flag=flagged, @@ -113,6 +125,17 @@ async def test_manage_exercises_lists_flagged_questions(auth_editor_client): assert "editor_test_unflagged" not in resp.text +async def test_manage_exercises_disables_edit_for_legacy_question(auth_editor_client): + question = await _make_question("editor_test_legacy", with_question_json=False) + + resp = await auth_editor_client.get("/editor/manage_exercises") + + assert resp.status_code == 200 + assert "This legacy question cannot be edited because it does not have question_json." in resp.text + assert 'class="disabled-action-tooltip" tabindex="0"' in resp.text + assert f"/editor/questions/{question.id}/edit" not in resp.text + + async def test_manage_exercises_skips_other_peoples_books(auth_editor_client): """Flagged questions from a base course the editor does not edit are hidden.""" await _make_question("editor_test_other_book", base_course=OTHER_BASE_COURSE) @@ -142,6 +165,7 @@ async def test_edit_question_page(auth_editor_client): assert resp.status_code == 200 assert "editor_test_edit_page" in resp.text + assert "Question JSON" in resp.text assert "Flagged for review?" in resp.text @@ -151,20 +175,37 @@ async def test_edit_question(auth_editor_client): resp = await auth_editor_client.post( f"/editor/questions/{question.id}/edit", json={ - "question": "Updated editorial source", - "htmlsrc": "

Updated editorial HTML

", - "difficulty": 2.5, + "question_json": { + "type": "shortanswer", + "prompt": "Updated editorial prompt", + } }, ) assert resp.status_code == 200 updated = await fetch_question("editor_test_edit_me", basecourse=EDITED_BASE_COURSE) - assert updated.question == "Updated editorial source" - assert updated.htmlsrc == "

Updated editorial HTML

" - assert updated.difficulty == 2.5 + assert updated.question_json == { + "type": "shortanswer", + "prompt": "Updated editorial prompt", + } + assert updated.question == "Flagged for review?" + assert updated.htmlsrc == "

html for editor_test_edit_me

" assert updated.review_flag is True +async def test_edit_question_rejects_legacy_question(auth_editor_client): + question = await _make_question("editor_test_legacy_edit", with_question_json=False) + + page = await auth_editor_client.get(f"/editor/questions/{question.id}/edit") + save = await auth_editor_client.post( + f"/editor/questions/{question.id}/edit", + json={"question_json": {"type": "shortanswer"}}, + ) + + assert page.status_code == 409 + assert save.status_code == 409 + + async def test_edit_question_rejects_unedited_base_course(auth_editor_client): question = await _make_question( "editor_test_other_edit", base_course=OTHER_BASE_COURSE @@ -172,7 +213,7 @@ async def test_edit_question_rejects_unedited_base_course(auth_editor_client): resp = await auth_editor_client.post( f"/editor/questions/{question.id}/edit", - json={"question": "No", "htmlsrc": "

No

", "difficulty": 1}, + json={"question_json": {"type": "shortanswer", "prompt": "No"}}, ) assert resp.status_code == 403 From 1608dcc9cb8288c503518df32fe2ee73f3cf3c6b Mon Sep 17 00:00:00 2001 From: Andrey Morozov Date: Sat, 26 Sep 2026 09:34:23 +0200 Subject: [PATCH 4/4] f-1257 fix test --- test/bases/rsptx/admin_server_api/test_editor_routes.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/bases/rsptx/admin_server_api/test_editor_routes.py b/test/bases/rsptx/admin_server_api/test_editor_routes.py index 2f9ea2fef..79d22cab7 100644 --- a/test/bases/rsptx/admin_server_api/test_editor_routes.py +++ b/test/bases/rsptx/admin_server_api/test_editor_routes.py @@ -131,8 +131,9 @@ async def test_manage_exercises_disables_edit_for_legacy_question(auth_editor_cl resp = await auth_editor_client.get("/editor/manage_exercises") assert resp.status_code == 200 - assert "This legacy question cannot be edited because it does not have question_json." in resp.text - assert 'class="disabled-action-tooltip" tabindex="0"' in resp.text + assert 'title="This legacy question cannot be edited"' in resp.text + assert 'class="disabled-action-tooltip"' in resp.text + assert 'tabindex="0"' in resp.text assert f"/editor/questions/{question.id}/edit" not in resp.text