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 ? ( +
    + +