From 765a477d4b398d6c339fd8ef9457801890ba897f Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 11 Sep 2026 12:59:43 +0200 Subject: [PATCH] =?UTF-8?q?feat:=20figures=20a=20question=20can=20actually?= =?UTF-8?q?=20have=20=E2=80=94=20managed,=20labelled,=20previewed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit question_media replaced the two filename columns months ago: any number of figures per question, each with a role, a label the prose can refer to, a caption and an order. Only the editor's own endpoint ever read them. The editor showed the two legacy text fields, and the player and the answer review rendered the legacy paths — so the model existed and nothing used it. - FigureManager in the question editor: add from the image bank, name, caption, reorder, remove, per role. A figure with no caption is called out, because a caption is how anyone finds it again. The image id is shown, since that is what the link survives a rename by. - FigureStrip on the player and the review. Explanation figures are labelled thumbnails that open full size and page between them — a stack of full-width radiographs between two paragraphs pushes the explanation off the screen, and "as in Figure 2" needs Figure 2 to be named where it sits. A stem figure stays full size: it is the question. - question_figures.py is the single place rows become what a page renders, so the three views cannot disagree. - Explanation figures are withheld until answers are revealed, the same rule the explanation itself follows. The legacy paths still render where a question was never backfilled, so nothing that worked before stops working. Backend 242/242, frontend 290/290. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/routers/attempts.py | 11 +- backend/app/routers/questions.py | 25 +-- backend/app/routers/quizzes.py | 6 + backend/app/schemas/attempt.py | 1 + backend/app/schemas/quiz.py | 3 + backend/app/services/question_figures.py | 56 ++++++ frontend/src/components/FigureManager.css | 57 ++++++ frontend/src/components/FigureManager.jsx | 185 +++++++++++++++++++ frontend/src/components/FigureStrip.css | 61 ++++++ frontend/src/components/FigureStrip.jsx | 74 ++++++++ frontend/src/components/FigureStrip.test.jsx | 58 ++++++ frontend/src/pages/QuestionEditPage.jsx | 37 +--- frontend/src/pages/QuizPage.jsx | 16 +- frontend/src/pages/ResultsPage.jsx | 11 +- 14 files changed, 540 insertions(+), 61 deletions(-) create mode 100644 backend/app/services/question_figures.py create mode 100644 frontend/src/components/FigureManager.css create mode 100644 frontend/src/components/FigureManager.jsx create mode 100644 frontend/src/components/FigureStrip.css create mode 100644 frontend/src/components/FigureStrip.jsx create mode 100644 frontend/src/components/FigureStrip.test.jsx diff --git a/backend/app/routers/attempts.py b/backend/app/routers/attempts.py index 0684ab3..0387620 100644 --- a/backend/app/routers/attempts.py +++ b/backend/app/routers/attempts.py @@ -10,6 +10,7 @@ from sqlalchemy.orm import Session from sqlalchemy import case, func from app.services.attempt_expiry import active_key, progress_key, settle_if_expired +from app.services.question_figures import figures_for_questions from app.services.study_plan_context import mark_block_complete, plan_context_for_quizzes, unmark_block_complete from app.database import get_db from app.models.quiz import Quiz @@ -147,6 +148,7 @@ def submit_attempt( review_allowed = not is_course_quiz or (quiz.allow_review == 1) answer_details = [] if review_allowed: + figures = figures_for_questions(db, [q.id for q, _, _ in grades]) for q, user_answer, is_correct in grades: answer_details.append(AnswerDetail( question_id=q.id, @@ -161,6 +163,7 @@ def submit_attempt( image_path=q.image_path, page_reference=q.page_reference, category_breadcrumbs=category_breadcrumbs(categories, q.question_category_id), + figures=figures.get(q.id, []), )) attempt.score = score @@ -667,9 +670,10 @@ def get_attempt( submitted_map = {ans.question_id: ans for ans in attempt.answers} answer_details = [] if review_allowed: - for q in get_quiz_questions(db, attempt.quiz_id): - if attempt.selected_question_ids is not None and q.id not in attempt.selected_question_ids: - continue + shown = [q for q in get_quiz_questions(db, attempt.quiz_id) + if attempt.selected_question_ids is None or q.id in attempt.selected_question_ids] + figures = figures_for_questions(db, [q.id for q in shown]) + for q in shown: ans = submitted_map.get(q.id) answer_details.append(AnswerDetail( question_id=q.id, @@ -684,6 +688,7 @@ def get_attempt( image_path=q.image_path, page_reference=q.page_reference, category_breadcrumbs=category_breadcrumbs(categories, q.question_category_id), + figures=figures.get(q.id, []), )) percentage = (attempt.score / attempt.total_questions * 100) if attempt.total_questions > 0 else 0 diff --git a/backend/app/routers/questions.py b/backend/app/routers/questions.py index fe2cca5..40eec85 100644 --- a/backend/app/routers/questions.py +++ b/backend/app/routers/questions.py @@ -24,6 +24,7 @@ from app.models.quiz import Quiz from app.models.user import User from app.models.favorite import Favorite from app.services.search_service import hybrid_ids, hybrid_question_ids +from app.services.question_figures import figures_for as _figures_for from app.services.quiz_builder import (bank_query, category_descendants, filtered_bank_query, CreateFromBankRequest, GenerateTestRequest, create_saved_test, exam_scope_predicate, generate_test) from app.utils.auth import get_current_user, require_moderator @@ -1238,30 +1239,6 @@ class FigureUpdate(BaseModel): position: int | None = None -def _figure_json(link, asset) -> dict: - return { - "id": link.id, "media_id": link.media_id, "role": link.role, - # The label the prose refers to. Falls back to a number so a figure is - # never nameless, which is what makes "see the figure" ambiguous. - "label": link.label or f"Figure {link.position + 1}", - "caption": link.caption or getattr(asset, "caption", None), - "title": getattr(asset, "title", None), - "path": getattr(asset, "path", None), - "position": link.position, - } - - -def _figures_for(db: Session, question_id: int) -> list[dict]: - from app.models.media import MediaAsset - from app.models.question_media import QuestionMedia - - rows = db.query(QuestionMedia, MediaAsset).join( - MediaAsset, MediaAsset.id == QuestionMedia.media_id).filter( - QuestionMedia.question_id == question_id).order_by( - QuestionMedia.role, QuestionMedia.position, QuestionMedia.id).all() - return [_figure_json(link, asset) for link, asset in rows] - - @router.get("/detail/{question_id}/figures") def list_figures(question_id: int, db: Session = Depends(get_db), current_user: User = Depends(get_current_user)): diff --git a/backend/app/routers/quizzes.py b/backend/app/routers/quizzes.py index df68939..5b67d53 100644 --- a/backend/app/routers/quizzes.py +++ b/backend/app/routers/quizzes.py @@ -6,6 +6,7 @@ from sqlalchemy import cast, String, or_, and_, func from sqlalchemy.orm import Session from app.services.attempt_expiry import settle_if_expired +from app.services.question_figures import figures_for_questions from app.services.study_plan_context import plan_context_for_quizzes from app.utils.upload_access import validate_image_attachments from app.utils.quiz_questions import validate_option_explanations @@ -437,8 +438,13 @@ def get_quiz( result.questions = [q for q in result.questions if q.id in selected] result.questions_count = len(result.questions) categories = db.query(QuestionCategory).all() + figures = figures_for_questions(db, [q.id for q in result.questions]) for question in result.questions: question.category_breadcrumbs = category_breadcrumbs(categories, question.question_category_id) + # Stem figures always; explanation figures only where answers are + # already revealed, so a figure cannot give away what the stem hides. + question.figures = [figure for figure in figures.get(question.id, []) + if reveal or figure["role"] == "stem"] return result diff --git a/backend/app/schemas/attempt.py b/backend/app/schemas/attempt.py index 6e5a09f..b5da978 100644 --- a/backend/app/schemas/attempt.py +++ b/backend/app/schemas/attempt.py @@ -28,6 +28,7 @@ class AnswerDetail(BaseModel): image_path: str | None = None page_reference: int | None = None category_breadcrumbs: list[dict] = Field(default_factory=list) + figures: list[dict] = Field(default_factory=list) class Config: from_attributes = True diff --git a/backend/app/schemas/quiz.py b/backend/app/schemas/quiz.py index e1287a0..c1378a7 100644 --- a/backend/app/schemas/quiz.py +++ b/backend/app/schemas/quiz.py @@ -33,6 +33,9 @@ class QuestionResponse(BaseModel): question_category_id: int | None = None difficulty: str | None = None category_breadcrumbs: list[dict] = Field(default_factory=list) + # Filled by the endpoint, not read off the model: a figure is a row in + # question_media, not a column on the question. + figures: list[dict] = Field(default_factory=list) class Config: from_attributes = True diff --git a/backend/app/services/question_figures.py b/backend/app/services/question_figures.py new file mode 100644 index 0000000..9173b79 --- /dev/null +++ b/backend/app/services/question_figures.py @@ -0,0 +1,56 @@ +"""The figures attached to a question. + +A question used to carry one stem image and one explanation image as two +filename columns. `question_media` replaced that with rows — any number of +figures, each with a role, a label the prose can refer to, a caption and an +order — but only the editor's own endpoint ever read them, so the quiz player +and the answer review still showed the two legacy paths. + +This is the one place that turns those rows into what a page renders, so the +editor, the player and the review cannot disagree about what is on a question. +""" +from collections import defaultdict + +from sqlalchemy.orm import Session + +from app.models.media import MediaAsset +from app.models.question_media import QuestionMedia + + +def figure_json(link: QuestionMedia, asset: MediaAsset) -> dict: + return { + "id": link.id, + "media_id": link.media_id, + "role": link.role, + # The label the prose refers to. Falls back to a number so a figure is + # never nameless, which is what makes "see the figure" ambiguous. + "label": link.label or f"Figure {link.position + 1}", + "caption": link.caption or getattr(asset, "caption", None), + "title": getattr(asset, "title", None), + "path": getattr(asset, "path", None), + "position": link.position, + } + + +def figures_for_questions(db: Session, question_ids) -> dict[int, list[dict]]: + """Figures for many questions at once, keyed by question id. + + One query for a whole quiz rather than one per question — a 240-question + exam would otherwise fan out into 240 round trips to build one page. + """ + ids = [qid for qid in dict.fromkeys(question_ids) if qid] + if not ids: + return {} + rows = (db.query(QuestionMedia, MediaAsset) + .join(MediaAsset, MediaAsset.id == QuestionMedia.media_id) + .filter(QuestionMedia.question_id.in_(ids)) + .order_by(QuestionMedia.role, QuestionMedia.position, QuestionMedia.id) + .all()) + out: dict[int, list[dict]] = defaultdict(list) + for link, asset in rows: + out[link.question_id].append(figure_json(link, asset)) + return out + + +def figures_for(db: Session, question_id: int) -> list[dict]: + return figures_for_questions(db, [question_id]).get(question_id, []) diff --git a/frontend/src/components/FigureManager.css b/frontend/src/components/FigureManager.css new file mode 100644 index 0000000..6e542b7 --- /dev/null +++ b/frontend/src/components/FigureManager.css @@ -0,0 +1,57 @@ +/* The figures on a question, in the editor. */ + +.fm-error { + margin: 0 0 12px; padding: 9px 12px; font-size: 0.84rem; + color: var(--wrong-fg); background: var(--wrong-bg); + border: 1px solid var(--wrong-bd); border-radius: 8px; +} + +.fm-role + .fm-role { margin-top: 20px; padding-top: 16px; border-top: 1px solid var(--border); } +.fm-role h3 { + margin: 0 0 4px; font-size: 0.7rem; font-weight: 700; letter-spacing: 0.07em; + text-transform: uppercase; color: var(--text-subtle); +} +.fm-help { margin: 0 0 12px; font-size: 0.8rem; line-height: 1.55; color: var(--text-muted); } +.fm-empty { margin: 0 0 12px; font-size: 0.83rem; color: var(--text-muted); } + +.fm-list { list-style: none; margin: 0 0 12px; padding: 0; display: flex; flex-direction: column; gap: 10px; } +.fm-item { + display: flex; gap: 12px; align-items: flex-start; padding: 10px; + background: var(--bg); border: 1px solid var(--border); border-radius: 10px; +} +.fm-item img { + flex-shrink: 0; width: 96px; height: 72px; object-fit: cover; + border-radius: 7px; border: 1px solid var(--border); background: var(--card-bg); +} + +.fm-meta { flex: 1; min-width: 0; display: flex; flex-direction: column; gap: 3px; } +.fm-label { font-size: 0.88rem; font-weight: 650; } +.fm-caption { font-size: 0.8rem; line-height: 1.5; color: var(--text-muted); overflow-wrap: anywhere; } +/* A figure with no caption is one nobody will find when searching the bank. */ +.fm-caption.is-missing { color: var(--wrong-fg); font-style: italic; } +.fm-id { font-size: 0.72rem; color: var(--text-subtle); font-family: ui-monospace, SFMono-Regular, Menlo, monospace; } + +.fm-actions { display: flex; gap: 5px; flex-wrap: wrap; flex-shrink: 0; } +.fm-remove { color: var(--wrong-fg); border-color: var(--wrong-bd); } + +.fm-edit { display: flex; flex-direction: column; gap: 8px; width: 100%; } +.fm-edit label { display: flex; flex-direction: column; gap: 4px; } +.fm-edit span { + font-size: 0.66rem; font-weight: 700; letter-spacing: 0.07em; + text-transform: uppercase; color: var(--text-subtle); +} +.fm-edit input, .fm-edit textarea { + width: 100%; padding: 8px 10px; + /* 16px on touch so iOS does not zoom the page in and refuse to zoom back. */ + font-size: 16px; font-family: inherit; + border: 1px solid var(--border); border-radius: 7px; + background: var(--input-bg); color: var(--text); resize: vertical; +} +@media (min-width: 700px) { .fm-edit input, .fm-edit textarea { font-size: 0.88rem; } } + +@media (max-width: 620px) { + .fm-item { flex-wrap: wrap; } + .fm-item img { width: 64px; height: 48px; } + .fm-actions { width: 100%; } + .fm-actions .btn { flex: 1; } +} diff --git a/frontend/src/components/FigureManager.jsx b/frontend/src/components/FigureManager.jsx new file mode 100644 index 0000000..73f691f --- /dev/null +++ b/frontend/src/components/FigureManager.jsx @@ -0,0 +1,185 @@ +import { useCallback, useEffect, useState } from 'react' +import api from '../api/client' +import ImagePicker from './ImagePicker' +import { uploadUrl } from '../utils/uploads' +import './FigureManager.css' + +const ROLES = [ + { key: 'stem', title: 'Question figures', + help: 'Shown beside the stem, before the answer. Nothing here may give the answer away.' }, + { key: 'explanation', title: 'Explanation figures', + help: 'Shown with the explanation, after the answer is in. Referred to by label — "as in Figure 2".' }, +] + +const apiError = (err, fallback) => { + const detail = err?.response?.data?.detail + return typeof detail === 'string' ? detail : fallback +} + +/** + * The figures on one question: add, name, caption, order, remove. + * + * A question used to carry one stem image and one explanation image, as two + * text fields holding a filename. `question_media` replaced that with rows — + * any number of figures, each with a label the prose can refer to — and until + * now there was no way to see or change them. + * + * The label is the point. "See the figure" is ambiguous the moment there are + * two, so every figure has one, falling back to its position. + */ +export default function FigureManager({ questionId }) { + const [figures, setFigures] = useState([]) + const [loading, setLoading] = useState(true) + const [busy, setBusy] = useState(false) + const [error, setError] = useState('') + const [picking, setPicking] = useState(null) // the role being added to + const [editing, setEditing] = useState(null) // figure id being renamed + const [draft, setDraft] = useState({ label: '', caption: '' }) + + const load = useCallback(() => { + if (!questionId) { setLoading(false); return } + api.get(`/questions/detail/${questionId}/figures`) + .then(res => setFigures(res.data || [])) + .catch(() => setError('Could not load the figures')) + .finally(() => setLoading(false)) + }, [questionId]) + + useEffect(() => { load() }, [load]) + + const run = async (fn, failure) => { + setBusy(true); setError('') + try { + const res = await fn() + // Every write answers with the full list, so nothing has to be guessed. + if (res?.data?.figures) setFigures(res.data.figures) + else load() + } catch (err) { setError(apiError(err, failure)) } + finally { setBusy(false) } + } + + const add = (role, mediaId) => run( + () => api.post(`/questions/detail/${questionId}/figures`, { media_id: mediaId, role }), + 'Could not add that figure') + + const save = (figure) => run( + () => api.patch(`/questions/figures/${figure.id}`, + { label: draft.label.trim() || null, caption: draft.caption.trim() || null }) + .then(res => { setEditing(null); return res }), + 'Could not save that') + + const remove = (figure) => run( + () => api.delete(`/questions/figures/${figure.id}`), 'Could not remove that figure') + + // Swapping positions rather than renumbering the lot: the server stores a + // position per figure, and two writes are enough to move one place. + const move = (role, index, delta) => { + const inRole = figures.filter(f => f.role === role) + const target = index + delta + if (target < 0 || target >= inRole.length) return + const a = inRole[index] + const b = inRole[target] + return run(async () => { + await api.patch(`/questions/figures/${a.id}`, { position: b.position }) + return api.patch(`/questions/figures/${b.id}`, { position: a.position }) + }, 'Could not reorder the figures') + } + + if (!questionId) { + return

Save the question first, then figures can be added to it.

+ } + if (loading) return
+ + return ( +
+ {error &&

{error}

} + + {ROLES.map(role => { + const inRole = figures.filter(f => f.role === role.key) + return ( +
+

{role.title}

+

{role.help}

+ + {inRole.length === 0 ? ( +

None yet.

+ ) : ( +
    + {inRole.map((figure, index) => ( +
  • + {figure.caption { e.target.style.visibility = 'hidden' }} /> + +
    + {editing === figure.id ? ( +
    + +