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