From bb72aa5f603b0928fbf6de26b2085620aacfb0ee Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 07:37:58 +0200 Subject: [PATCH] feat: AMBOSS exam chrome, review as a study session, and no silent submissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exam player takes the window. The shell was sized against the header with a number that did not include the navbar's own 32px of margin, so the block bar — the one thing on the screen that must always be reachable — sat below the fold and had to be scrolled to. Exam mode now hides the site chrome entirely and is the viewport, which makes the arithmetic honest and matches what a board looks like: item and block in a box at the left, the two arrows in the middle, the tools at the right, the question-status rail down the side, and the clock, Pause and End Block along the bottom. Shortcuts is gone from the bar, and the labs open into the column beside the question in both modes rather than a box over it. Nothing is handed in behind the learner's back. The clock reaching zero stops the block and says so; closing Time's Up submits, and the player stays put showing the answers, which is the review. The server no longer settles an expired attempt at all — listing sessions used to mark any paper whose clock had run out, so opening a page could score a block the learner had walked away from, and the first they knew of it was a result. Reviewing an attempt is now the player with the answers in, not a dropdown and a card. Same rail, same layout, same labs, same way out — and on a phone the same burger opens the same question list, from one shared rule about which routes are a session. Also: the rule-out toggle sits beside its option instead of pinned to the far edge of the card, so an option box is as wide as its own words; the voice picker leaves the player, since a reader's voice is a setting and not a decision to retake every session; figures carry no invented "Figure 1" — a label is what prose refers to, and the backfill knew of no prose, so 346 of them said only that an image was an image; and the landing page shows the two modes happening rather than promising six things in a sentence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/routers/attempts.py | 14 +- backend/app/routers/quizzes.py | 16 +- backend/scripts/backfill_question_figures.py | 6 +- backend/scripts/merge_duplicate_categories.py | 122 +++++ backend/tests/test_quiz_builder.py | 14 +- backend/tests/test_session_lifecycle.py | 70 +-- frontend/src/App.jsx | 3 +- frontend/src/components/FigureManager.jsx | 28 +- frontend/src/components/Navbar.jsx | 3 +- frontend/src/index.css | 2 + frontend/src/pages/LandingPage.css | 143 ++++++ frontend/src/pages/LandingPage.jsx | 96 +++- frontend/src/pages/LandingPage.test.jsx | 5 +- frontend/src/pages/QuizPage.jsx | 88 ++-- frontend/src/pages/QuizPage.test.jsx | 23 +- frontend/src/pages/QuizPlayer.css | 148 +++++- frontend/src/pages/ResultsPage.jsx | 422 +++++++++++------- frontend/src/utils/session.js | 11 + 18 files changed, 935 insertions(+), 279 deletions(-) create mode 100644 backend/scripts/merge_duplicate_categories.py create mode 100644 frontend/src/utils/session.js diff --git a/backend/app/routers/attempts.py b/backend/app/routers/attempts.py index a55ab59..cd576ca 100644 --- a/backend/app/routers/attempts.py +++ b/backend/app/routers/attempts.py @@ -9,7 +9,7 @@ from pydantic import BaseModel from sqlalchemy.orm import Session from sqlalchemy import case, func -from app.services.attempt_expiry import active_key, load_saved, progress_key, settle_if_expired +from app.services.attempt_expiry import active_key, load_saved, progress_key from app.services.knowledge_groups import Grouping, score_rows 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 @@ -352,12 +352,12 @@ def get_progress( r.setex(key, 7 * 24 * 3600, _json.dumps(saved)) return saved - # An unsuspended exam whose clock ran out is submitted with what was - # answered, and the score counts. The client is told so it can show the - # result rather than an empty resume. - if settle_if_expired(db, r, current_user.id, attempt, saved): - return {"expired_submitted": True, "attempt_id": attempt.id, "quiz_id": quiz_id} - + # An exam whose clock ran out is handed back with no time on it, and + # nothing more. The player opens it, shows Time's Up, and submits when + # the learner closes that — which is the only moment anybody has said + # the block is over. Marking it here instead meant a paper could be + # taken in and scored by a request the learner never made, days after + # they last saw it, and the first they knew of it was a result. return saved except Exception: logger.warning("Redis unavailable for progress retrieval", exc_info=True) diff --git a/backend/app/routers/quizzes.py b/backend/app/routers/quizzes.py index 6ba6ea1..73218f2 100644 --- a/backend/app/routers/quizzes.py +++ b/backend/app/routers/quizzes.py @@ -6,7 +6,6 @@ from sqlalchemy import cast, String, or_, and_, func from sqlalchemy.orm import Session from app.services import site_settings -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 @@ -337,20 +336,17 @@ def list_quiz_sessions( r = redis_lib.from_url(settings.REDIS_URL, decode_responses=True) live_attempts = list(active.values()) keys = [f"quiz_progress:{current_user.id}:{a.id}" for a in live_attempts] - settled = [] for attempt, raw in zip(live_attempts, r.mget(keys)): if not raw: continue saved = _json.loads(raw) - # An unsuspended exam whose time is up is a finished exam. - if settle_if_expired(db, r, current_user.id, attempt, saved): - settled.append(attempt) - continue + # Counted, not settled. Listing sessions used to hand in any + # exam whose clock had run out, so simply opening this page + # could mark a paper — the learner's next sight of a block they + # had walked away from was a score. An out-of-time block stays + # open until it is opened, where Time's Up is shown and closing + # it hands the paper in. answered[attempt.id] = len(saved.get("answers", {}) or {}) - # Moved after the loop: `active` must not change while it is read. - for attempt in settled: - active.pop(attempt.quiz_id, None) - finished.setdefault(attempt.quiz_id, []).append(attempt) except Exception: logger.warning("Redis unavailable for session progress", exc_info=True) diff --git a/backend/scripts/backfill_question_figures.py b/backend/scripts/backfill_question_figures.py index 433b924..30fc08e 100644 --- a/backend/scripts/backfill_question_figures.py +++ b/backend/scripts/backfill_question_figures.py @@ -76,8 +76,12 @@ def main(): for question_id, asset, role, category in planned: position = db.query(QuestionMedia).filter_by( question_id=question_id, role=role).count() + # No label. A label is what the prose refers to — "as in Figure 2" + # — and a backfill knows of no prose that refers to anything. The + # numbers this used to invent were printed under every image on the + # site, telling a learner that an image was an image. db.add(QuestionMedia(question_id=question_id, media_id=asset.id, role=role, - label=f"Figure {position + 1}", position=position)) + position=position)) made += 1 # Keep the filename as provenance in the caption; it is the only # record of which page of which PDF this came from. diff --git a/backend/scripts/merge_duplicate_categories.py b/backend/scripts/merge_duplicate_categories.py new file mode 100644 index 0000000..e67357c --- /dev/null +++ b/backend/scripts/merge_duplicate_categories.py @@ -0,0 +1,122 @@ +"""Merge topics that are the same topic twice. + +The tree was built from a model's per-question tags, so the same condition was +created once under each discipline that happened to mention it: Chlamydial +Infection exists under Ophthalmology, Infectious Disease and Neonatology, and a +learner filtering by any one of them sees a third of the questions. + +What is kept is the cross-filing, not the duplicate. Questions move to one +surviving topic, and each one gains an extra category link to the parent of the +copy it came from — so a chlamydial conjunctivitis question is still reachable +from Ophthalmology, without Ophthalmology owning a second copy of the disease. + +Names are compared with punctuation and case removed but **digits kept**: a +normaliser that strips digits makes Trisomy 18 and Trisomy 21 the same row, and +Type 1 and Type 2 Diabetes with them. That is not a merge, it is a loss. + +The survivor is the copy with the most questions; ties go to the lowest id, so +a rerun picks the same one. + +Idempotent, and a dry run by default: + + docker compose exec backend python -m scripts.merge_duplicate_categories + docker compose exec backend python -m scripts.merge_duplicate_categories --apply +""" +import re +import sys +from collections import defaultdict + +from sqlalchemy import text as sa_text + +from app.database import SessionLocal + +#: Punctuation and case go; digits stay. +KEY = re.compile(r"[^a-z0-9]") + + +def normalise(name: str) -> str: + return KEY.sub("", (name or "").lower()) + + +def main(apply: bool) -> int: + db = SessionLocal() + try: + rows = db.execute(sa_text(""" + SELECT c.id, c.name, c.parent_id, c.system_id, + (SELECT count(*) FROM questions q + WHERE q.question_category_id = c.id AND q.deleted_at IS NULL) AS direct, + (SELECT count(*) FROM question_categories k WHERE k.parent_id = c.id) AS children + FROM question_categories c + """)).mappings().all() + + groups: dict[str, list] = defaultdict(list) + for row in rows: + groups[normalise(row["name"])].append(row) + + plans = [] + for key, copies in groups.items(): + if len(copies) < 2: + continue + # A topic with children is a branch of the tree, not a leaf that was + # duplicated; merging those would reparent somebody's subtree. + if any(c["children"] for c in copies): + continue + ordered = sorted(copies, key=lambda c: (-c["direct"], c["id"])) + plans.append((ordered[0], ordered[1:])) + + moved = linked = removed = 0 + print(f"{len(plans)} topics exist more than once\n") + for keep, drop in plans: + names = ", ".join(f"#{d['id']} ({d['direct']})" for d in drop) + print(f" {keep['name']:<30} keep #{keep['id']} ({keep['direct']}) ← {names}") + for loser in drop: + moved += loser["direct"] + removed += 1 + + print(f"\n{moved} questions move, {removed} duplicate topics go") + if not apply: + print("\ndry run. Pass --apply to write.") + return 0 + + for keep, drop in plans: + for loser in drop: + # The copy's parent is the association worth keeping: it is why + # somebody filed the disease there in the first place. + if loser["parent_id"] and loser["parent_id"] != keep["parent_id"]: + linked += db.execute(sa_text(""" + INSERT INTO question_category_links (question_id, category_id) + SELECT q.id, :parent FROM questions q + WHERE q.question_category_id = :loser + ON CONFLICT DO NOTHING + """), {"parent": loser["parent_id"], "loser": loser["id"]}).rowcount + db.execute(sa_text( + "UPDATE questions SET question_category_id = :keep WHERE question_category_id = :loser"), + {"keep": keep["id"], "loser": loser["id"]}) + # An extra link to the survivor may already exist; the unique + # pair would refuse the move, so the duplicate row goes first. + db.execute(sa_text(""" + DELETE FROM question_category_links a + WHERE a.category_id = :loser AND EXISTS ( + SELECT 1 FROM question_category_links b + WHERE b.question_id = a.question_id AND b.category_id = :keep) + """), {"loser": loser["id"], "keep": keep["id"]}) + db.execute(sa_text( + "UPDATE question_category_links SET category_id = :keep WHERE category_id = :loser"), + {"keep": keep["id"], "loser": loser["id"]}) + # The survivor keeps its own system unless it has none. + if not keep["system_id"] and loser["system_id"]: + db.execute(sa_text( + "UPDATE question_categories SET system_id = :s WHERE id = :id"), + {"s": loser["system_id"], "id": keep["id"]}) + db.execute(sa_text("DELETE FROM blueprint_category_links WHERE category_id = :id"), + {"id": loser["id"]}) + db.execute(sa_text("DELETE FROM question_categories WHERE id = :id"), {"id": loser["id"]}) + db.commit() + print(f"\nmerged. {linked} cross-filings kept as extra links.") + return 0 + finally: + db.close() + + +if __name__ == "__main__": + sys.exit(main("--apply" in sys.argv)) diff --git a/backend/tests/test_quiz_builder.py b/backend/tests/test_quiz_builder.py index 60d52c1..df74815 100644 --- a/backend/tests/test_quiz_builder.py +++ b/backend/tests/test_quiz_builder.py @@ -365,11 +365,15 @@ class BuilderTests(unittest.TestCase): with patch.dict(sys.modules, {"redis": redis}): response = self.client.get(f"/attempts/progress?quiz_id={saved}") self.assertEqual(response.status_code, 200, response.text) - # Time ran out: submitted, and the client is told so. It is a completed - # attempt that counts, not an "expired" one hidden from history. - self.assertEqual(response.json(), {"expired_submitted": True, "attempt_id": aid, "quiz_id": saved}) - self.assertIsNotNone(self.db.get(QuizAttempt, aid).completed_at) - self.assertFalse(self.db.get(QuizAttempt, aid).expired) + # Out of time, and handed back unmarked: resuming shows the player its + # own saved answers so it can open on Time's Up. Nothing is graded by a + # request the learner did not make. + self.assertNotIn("expired_submitted", response.json()) + self.assertIsNone(self.db.get(QuizAttempt, aid).completed_at) + # And when the learner does submit, it is still graded over the + # selected questions only — the parity this test is named for. + submitted = self.client.post(f"/attempts/{aid}/submit", json={"answers": []}) + self.assertEqual(submitted.status_code, 200, submitted.text) self.assertEqual(self.db.query(AttemptAnswer).filter_by(attempt_id=aid).count(), 1) self.assertFalse(self.db.query(AttemptAnswer).filter_by(attempt_id=aid).one().is_correct) diff --git a/backend/tests/test_session_lifecycle.py b/backend/tests/test_session_lifecycle.py index 3819907..79adf30 100644 --- a/backend/tests/test_session_lifecycle.py +++ b/backend/tests/test_session_lifecycle.py @@ -1,9 +1,10 @@ """What happens to a session when its clock runs out, when it is deleted, and when a learner resets everything. -Disposable SQLite; Redis is a Mock. The rule under test for expiry: an exam -closed without being suspended keeps running, and when time is up it is -submitted with what was answered and the score counts. +Disposable SQLite; Redis is a Mock. The rule under test for expiry: an exam's +clock runs only while the exam is on screen, and running out ends the block but +hands nothing in. The learner is shown Time's Up when they next open it, and +closing that submits — so no request they did not make ever marks a paper. """ import json import sys @@ -104,14 +105,19 @@ class SessionLifecycleTests(unittest.TestCase): left) self.assertEqual(left, 0.0) - def test_an_exam_closed_at_zero_is_submitted_on_the_next_look(self): - """If the tab goes before the submit lands, the next page settles it.""" + def test_an_exam_closed_at_zero_is_not_marked_by_looking_at_the_list(self): + """Listing sessions reads; it does not hand papers in. + + It used to settle any attempt whose clock had run out, so merely + opening the sessions page could mark a block — and the learner's first + sight of an exam they had walked away from was a score. + """ quiz_id, aid = self.timed_quiz() self.save(self.bank.owner.id, aid, time_left=0) with patch.dict(sys.modules, {"redis": self.redis}): rows = {row["quiz_id"]: row for row in self.client.get("/quizzes/sessions").json()} - self.assertEqual(rows[quiz_id]["state"], "completed") - self.assertIsNotNone(self.db.get(QuizAttempt, aid).completed_at) + self.assertNotEqual(rows[quiz_id]["state"], "completed") + self.assertIsNone(self.db.get(QuizAttempt, aid).completed_at) def test_an_exam_with_time_on_it_is_left_alone(self): quiz_id, aid = self.timed_quiz() @@ -122,35 +128,35 @@ class SessionLifecycleTests(unittest.TestCase): self.assertNotEqual(rows[quiz_id]["state"], "completed") self.assertIsNone(self.db.get(QuizAttempt, aid).completed_at) - def test_an_unsuspended_exam_that_ran_out_is_a_finished_exam_in_the_session_list(self): + def test_an_out_of_time_exam_stays_open_in_the_session_list(self): quiz_id, aid = self.timed_quiz() self.save(self.bank.owner.id, aid) - # A second one, also out of time: settling the first must not disturb - # the pass over the rest (it once mutated the dict being iterated). + # A second one, also out of time: neither is touched, and the answered + # counts are still reported for both. other_quiz, other_aid = self.timed_quiz() self.save(self.bank.owner.id, other_aid) with patch.dict(sys.modules, {"redis": self.redis}): rows = {row["quiz_id"]: row for row in self.client.get("/quizzes/sessions").json()} - self.assertEqual(rows[other_quiz]["state"], "completed") - row = rows[quiz_id] - self.assertEqual(row["state"], "completed") - self.assertEqual(row["last_attempt_id"], aid) - self.assertEqual(row["attempts_count"], 1) - attempt = self.db.get(QuizAttempt, aid) - self.assertIsNotNone(attempt.completed_at) - self.assertFalse(attempt.expired) - # Graded on what was answered; the unanswered question counts against. - self.assertEqual(attempt.total_questions, 2) - self.assertNotIn(f"quiz_progress:{self.bank.owner.id}:{aid}", self.store) + for qid in (quiz_id, other_quiz): + self.assertNotEqual(rows[qid]["state"], "completed") + self.assertIsNone(self.db.get(QuizAttempt, aid).completed_at) + # The saved progress is still there for the player to open. + self.assertIn(f"quiz_progress:{self.bank.owner.id}:{aid}", self.store) - def test_resuming_an_expired_exam_reports_the_submission(self): + def test_resuming_an_out_of_time_exam_hands_back_the_paper_unmarked(self): + """The player is given the block, not a verdict. + + It opens on Time's Up because there is no time on it, and submits when + the learner closes that. Marking it here instead meant the answer sheet + was taken and scored by a request the learner never made. + """ quiz_id, aid = self.timed_quiz() self.save(self.bank.owner.id, aid) with patch.dict(sys.modules, {"redis": self.redis}): - response = self.client.get(f"/attempts/progress?quiz_id={quiz_id}") - self.assertEqual(response.json(), {"expired_submitted": True, "attempt_id": aid, "quiz_id": quiz_id}) - # And the analysis exists for it straight away. - self.assertEqual(self.client.get(f"/attempts/{aid}/analysis").status_code, 200) + saved = self.client.get(f"/attempts/progress?quiz_id={quiz_id}").json() + self.assertNotIn("expired_submitted", saved) + self.assertEqual(saved["answers"], {"1": "yes"}) + self.assertIsNone(self.db.get(QuizAttempt, aid).completed_at) def test_a_suspended_exam_resumes_with_its_clock_held(self): quiz_id, aid = self.timed_quiz() @@ -369,11 +375,17 @@ class HintRecordingTests(SessionLifecycleTests): self.assertTrue(rows) self.assertFalse(any(row.used_hint for row in rows)) - def test_an_exam_that_runs_out_carries_the_tips_from_its_saved_progress(self): + def test_an_exam_that_runs_out_carries_the_tips_it_was_submitted_with(self): + """A tip opened before answering is recorded whoever pressed submit. + + The clock running out used to hand the paper in from the server, which + read the tips out of the saved progress. It is the player that submits + now — from the Time's Up dialog — and it sends the same list. + """ quiz_id, aid = self.timed_quiz() - self.save(self.bank.owner.id, aid, time_left=0, hints=[1]) with patch.dict(sys.modules, {"redis": self.redis}): - self.client.get("/quizzes/sessions") + self.client.post(f"/attempts/{aid}/submit", json={ + "answers": [{"question_id": 1, "user_answer": "yes"}], "hints": [1]}) rows = {row.question_id: row for row in self.db.query(AttemptAnswer).filter(AttemptAnswer.attempt_id == aid)} self.assertTrue(rows[1].used_hint) diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index 122204f..a948361 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -9,6 +9,7 @@ import ChooseObjective from './components/ChooseObjective' import SearchOverlay from './components/SearchOverlay' import ErrorBoundary from './components/ErrorBoundary' import lazyPage from './utils/lazyPage' +import { isSessionPath } from './utils/session' const LoginPage = lazyPage(() => import('./pages/LoginPage')) const RegisterPage = lazyPage(() => import('./pages/RegisterPage')) @@ -63,7 +64,7 @@ function AppLayout() { // Keyed by path so navigating away from a broken page clears the error. const location = useLocation() const [searching, setSearching] = useState(false) - const inSession = location.pathname.startsWith('/study/') + const inSession = isSessionPath(location.pathname) useEffect(() => { const onKey = (event) => { diff --git a/frontend/src/components/FigureManager.jsx b/frontend/src/components/FigureManager.jsx index 73f691f..2341d2b 100644 --- a/frontend/src/components/FigureManager.jsx +++ b/frontend/src/components/FigureManager.jsx @@ -8,7 +8,7 @@ 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".' }, + help: 'Shown with the explanation, after the answer is in. Give one a label only if the explanation refers to it by name.' }, ] const apiError = (err, fallback) => { @@ -24,8 +24,11 @@ const apiError = (err, fallback) => { * 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. + * Adding an image and naming it are two separate things, and only the first is + * required. A label is what prose refers to — "as in Figure 2" — so it is + * worth writing when the explanation points at a particular image, and worth + * leaving empty when it does not. An invented "Figure 1" under every image + * tells a learner only that an image is an image. */ export default function FigureManager({ questionId }) { const [figures, setFigures] = useState([]) @@ -113,15 +116,16 @@ export default function FigureManager({ questionId }) { {editing === figure.id ? (