From 6abe3cca11b3c43282a2249e94073b944801c403 Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 11 Sep 2026 23:42:27 +0200 Subject: [PATCH] fix: review or resume, and an exam clock set by the questions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The card offered Review answers and Resume session at once on a session still in progress, which is the muddle: there is nothing to review yet and nothing to resume once it is done. It is one or the other now, and what decides it is whether anything is left to answer — not whether it was an exam or a study session, which have the same two states as each other. A study session keeps going until every question is answered and becomes the review at that point, without waiting to be handed in. Repeat is offered either way. The questions worth sitting again are worth sitting again now. "Skipped" meant gone past, and was shown for questions in a session still running that had not been reached. Those read "not yet answered". And a timed block is now ninety seconds a question, set from the count rather than asked for. Choosing a limit is a decision nobody has the information to make — the pace belongs to the exam being rehearsed, not to a preference — and a block sat at the wrong pace teaches the wrong pace. Forty questions is an hour. An explicit limit is still honoured. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/routers/attempts.py | 13 ++- backend/app/services/quiz_builder.py | 25 +++++- backend/tests/test_quiz_builder.py | 37 ++++++++ backend/tests/test_session_lifecycle.py | 6 +- frontend/src/pages/AnalysisSessionPage.css | 2 + frontend/src/pages/AnalysisSessionPage.jsx | 84 ++++++++++--------- .../src/pages/AnalysisSessionPage.test.jsx | 38 +++++++-- 7 files changed, 157 insertions(+), 48 deletions(-) diff --git a/backend/app/routers/attempts.py b/backend/app/routers/attempts.py index 51d980e..f6abe6e 100644 --- a/backend/app/routers/attempts.py +++ b/backend/app/routers/attempts.py @@ -879,8 +879,17 @@ def attempt_analysis( "position": index, "question_id": row.question_id, "excerpt": (getattr(question, "question_text", "") or "")[:120], - "status": ("correct" if row.is_correct else "skipped" if not row.user_answer else "incorrect") - if graded else ("answered" if row.user_answer else "skipped"), + # "Skipped" means you went past it. In a session still running you + # have not been past it yet, so it says so — and in an exam that is + # running, an answered one says only that. + "status": ( + ("correct" if row.is_correct else "skipped" if not row.user_answer else "incorrect") + if attempt.completed_at + else ("correct" if graded and row.is_correct + else "incorrect" if graded and row.user_answer + else "answered" if row.user_answer + else "unanswered") + ), "difficulty": getattr(question, "difficulty", None), "category": categories.get(getattr(question, "question_category_id", None)), "seconds_spent": row.seconds_spent, diff --git a/backend/app/services/quiz_builder.py b/backend/app/services/quiz_builder.py index 6d5fa9d..03466d1 100644 --- a/backend/app/services/quiz_builder.py +++ b/backend/app/services/quiz_builder.py @@ -193,6 +193,29 @@ class GenerateTestRequest(TestOptions): explicit_ids: list[int] = Field(default_factory=list) +#: Seconds a timed block allows per question. The pace a real paper is sat at, +#: so a block of forty runs an hour — and so a learner rehearsing on this bank +#: is rehearsing the clock as well as the questions. +SECONDS_PER_QUESTION = 90 + + +def exam_minutes(data, count: int) -> int | None: + """How long a timed block gets, rounded up to the minute. + + Set from the number of questions rather than asked for. Choosing a limit + is a decision nobody has the information to make — the pace is a property + of the exam being rehearsed, not a preference — and a block sat at the + wrong pace teaches the wrong pace. An explicit limit is still honoured for + the cases that genuinely differ. + """ + if getattr(data, "mode", None) != "timed": + return None + asked = getattr(data, "time_limit_minutes", None) + if asked: + return asked + return max(1, -(-(count * SECONDS_PER_QUESTION) // 60)) + + def create_saved_test(db, user, data, question_ids): ids = list(dict.fromkeys(question_ids)) if not 1 <= len(ids) <= 200: @@ -203,7 +226,7 @@ def create_saved_test(db, user, data, question_ids): if query.count() != len(ids): raise HTTPException(400, "Some questions are missing, private, or unavailable for this test") quiz = Quiz(user_id=user.id, title=data.title, mode=data.mode, - time_limit_minutes=data.time_limit_minutes if data.mode == "timed" else None, + time_limit_minutes=exam_minutes(data, len(ids)), questions_count=len(ids), is_published=0, is_shared=int(data.is_shared)) db.add(quiz) db.flush() diff --git a/backend/tests/test_quiz_builder.py b/backend/tests/test_quiz_builder.py index 790fe7d..ad903f2 100644 --- a/backend/tests/test_quiz_builder.py +++ b/backend/tests/test_quiz_builder.py @@ -416,3 +416,40 @@ class AdaptiveSelectionTests(unittest.TestCase): # Everything has been seen, so the whole selection is recycled. picked = adaptive_select(self.db, self.user, 1, [], "all", None) self.assertEqual(picked, [1]) + + +class ExamClockTests(unittest.TestCase): + """A timed block's length is a property of the exam, not a preference.""" + + def test_ninety_seconds_a_question_rounded_up_to_the_minute(self): + from app.services.quiz_builder import exam_minutes + + class Ask: + mode = "timed" + time_limit_minutes = None + + # Forty questions is an hour, which is the pace a real paper is sat at. + self.assertEqual(exam_minutes(Ask(), 40), 60) + self.assertEqual(exam_minutes(Ask(), 20), 30) + # Five is seven and a half minutes, and a block never gets less than a + # minute however short it is. + self.assertEqual(exam_minutes(Ask(), 5), 8) + self.assertEqual(exam_minutes(Ask(), 0), 1) + + def test_a_study_block_has_no_clock(self): + from app.services.quiz_builder import exam_minutes + + class Ask: + mode = "study" + time_limit_minutes = 30 + + self.assertIsNone(exam_minutes(Ask(), 40)) + + def test_an_explicit_limit_is_still_honoured(self): + from app.services.quiz_builder import exam_minutes + + class Ask: + mode = "timed" + time_limit_minutes = 15 + + self.assertEqual(exam_minutes(Ask(), 40), 15) diff --git a/backend/tests/test_session_lifecycle.py b/backend/tests/test_session_lifecycle.py index e8fa36c..38619f8 100644 --- a/backend/tests/test_session_lifecycle.py +++ b/backend/tests/test_session_lifecycle.py @@ -310,7 +310,7 @@ class LiveAnalysisTests(unittest.TestCase): self.assertEqual(body["score"], 1) statuses = {q["status"] for q in body["questions"]} self.assertIn("correct", statuses) - self.assertIn("skipped", statuses) + self.assertIn("unanswered", statuses) # Nothing was written: submitting is what records answers. self.assertEqual(self.db.query(AttemptAnswer).filter_by(attempt_id=aid).count(), 0) @@ -335,7 +335,9 @@ class LiveAnalysisTests(unittest.TestCase): # How far through, which is not a leak, and nothing about rightness. self.assertEqual(body["answered"], 1) statuses = {q["status"] for q in body["questions"]} - self.assertEqual(statuses, {"answered", "skipped"}) + # "Skipped" would mean gone past; in a session still running it has + # not been reached. + self.assertEqual(statuses, {"answered", "unanswered"}) self.assertNotIn("correct", statuses) self.assertNotIn("incorrect", statuses) # And nothing to go back to yet: a recommendation is a verdict. diff --git a/frontend/src/pages/AnalysisSessionPage.css b/frontend/src/pages/AnalysisSessionPage.css index ffb6a31..4f0c5cd 100644 --- a/frontend/src/pages/AnalysisSessionPage.css +++ b/frontend/src/pages/AnalysisSessionPage.css @@ -82,6 +82,8 @@ td.an-col-q { display: flex; flex-wrap: wrap; align-items: baseline; gap: 6px; } .an-status.is-correct { background: var(--correct-bg); color: var(--correct-fg); } .an-status.is-incorrect { background: var(--wrong-bg); color: var(--wrong-fg); } .an-status.is-skipped { background: var(--bg); color: var(--text-muted); } +/* Not reached yet, which is not the same as passed by. */ +.an-status.is-unanswered { background: var(--bg); color: var(--text-subtle); } @media (max-width: 900px) { .an-page { grid-template-columns: 1fr; } diff --git a/frontend/src/pages/AnalysisSessionPage.jsx b/frontend/src/pages/AnalysisSessionPage.jsx index 7de87f7..fbd755d 100644 --- a/frontend/src/pages/AnalysisSessionPage.jsx +++ b/frontend/src/pages/AnalysisSessionPage.jsx @@ -136,6 +136,8 @@ export default function AnalysisSessionPage() { // donut by except answered and not. Showing a nought here would read as a // score, and a score before the exam is over is the exam defeated. const graded = data.graded !== false + // Nothing left to answer is finished, whether or not it was handed in. + const finished = !!data.completed_at || (data.total > 0 && data.answered >= data.total) const correct = graded ? data.score : 0 const incorrect = graded ? data.answered - data.score : 0 const answeredOnly = graded ? 0 : data.answered @@ -222,48 +224,50 @@ export default function AnalysisSessionPage() { {/* What to do about it, beneath the thing it is about. These were a row of buttons beside the page heading, which put the - decision as far as possible from the result it follows from. */} + decision as far as possible from the result it follows from. + + Review or resume, never both. Whether a session is finished + decides which — not whether it was an exam or a study session, + which have the same two states as each other. Offering both at + once was the muddle: a session you are part-way through has + nothing to review yet, and a finished one has nothing to + resume. */}
- {data.not_started ? ( - // One label either way. The session exists the moment it is - // made, so picking it up is resuming it whether or not a - // question has been answered yet. + {/* Repeating does not wait for the end: the questions worth + sitting again are worth sitting again now. */} + {data.questions?.length > 0 && ( + + )} + {/* Review or resume, decided by whether there is anything left + to do. A study session just keeps going until every question + has been answered, and at that point there is nothing to + resume — so it becomes the review without waiting to be + handed in. A submitted session is finished either way. */} + {finished ? ( + Review session + ) : ( Resume session - ) : ( - <> - {/* Repeating rarely means all of it: the questions worth - sitting again are the ones you got wrong and the ones you - never reached. The dialog asks which, and how many. */} - {data.questions?.length > 0 && ( - - )} - Review answers - {!data.completed_at && ( - - Resume session - - )} - {/* Deleting a session throws away answers the analysis is - built from, so it is not a button sitting next to Resume. - It is still offered: sessions are made freely here, and a - mis-made one is clutter worth removing. */} - {confirmDelete ? ( - - - - - ) : ( - - )} - )} + + {/* Deleting a session throws away answers the analysis is built + from, so it is not a button sitting next to the one you came + here to press. It is still offered: sessions are made freely + here, and a mis-made one is clutter worth removing. */} + {!data.not_started && (confirmDelete ? ( + + + + + ) : ( + + ))}
@@ -328,7 +332,11 @@ export default function AnalysisSessionPage() { {row.category && {row.category}} - {row.status} + + + {row.status === 'unanswered' ? 'not yet answered' : row.status} + + {row.difficulty ? {row.difficulty} diff --git a/frontend/src/pages/AnalysisSessionPage.test.jsx b/frontend/src/pages/AnalysisSessionPage.test.jsx index 754e207..8252d30 100644 --- a/frontend/src/pages/AnalysisSessionPage.test.jsx +++ b/frontend/src/pages/AnalysisSessionPage.test.jsx @@ -74,7 +74,7 @@ describe('a session nobody has sat', () => { mountQuiz() expect(await screen.findByRole('link', { name: 'Resume session' })) .toHaveAttribute('href', '/study/3?start=1') - expect(screen.queryByRole('link', { name: 'Review answers' })).not.toBeInTheDocument() + expect(screen.queryByRole('link', { name: /Review/ })).not.toBeInTheDocument() expect(screen.queryByRole('button', { name: 'Delete session' })).not.toBeInTheDocument() }) @@ -92,7 +92,7 @@ describe('a session that has been sat', () => { mountAttempt() const figures = (await screen.findByText("2/2")).closest('.an-figures') expect(within(figures).getByText('50%')).toBeInTheDocument() - expect(screen.getByRole('link', { name: 'Review answers' })).toHaveAttribute('href', '/results/91') + expect(screen.getByRole('link', { name: 'Review session' })).toHaveAttribute('href', '/results/91') // Repeating opens a dialog: which outcomes, and how many. Sitting all of // it again is rarely what anyone wants. expect(screen.getByRole('button', { name: 'Repeat session' })).toBeInTheDocument() @@ -127,10 +127,38 @@ describe('a session whose only attempt is still in progress', () => { await waitFor(() => expect(api.delete).toHaveBeenCalledWith('/attempts/77')) }) - it('reviews that attempt too', async () => { + it('offers to resume it, not to review it', async () => { mountQuiz() - expect(await screen.findByRole('link', { name: 'Review answers' })) - .toHaveAttribute('href', '/results/77') + // Review or resume, never both: a session you are part-way through has + // nothing to review yet, and whether it was an exam or a study session + // does not come into it. + expect(await screen.findByRole('link', { name: 'Resume session' })) + .toHaveAttribute('href', '/study/3?start=1') + expect(screen.queryByRole('link', { name: /Review/ })).toBeNull() + }) +}) + +describe('a study session with every question answered', () => { + it('becomes the review without waiting to be handed in', async () => { + api.get.mockImplementation(url => { + if (url === '/attempts/8/analysis') return Promise.resolve({ data: { + attempt_id: 8, quiz_id: 3, title: 'All done', mode: 'study', + completed_at: null, graded: true, + total: 2, answered: 2, score: 1, percent: 50, + seconds_total: null, seconds_per_question: 30, + questions: [], recommendations: [], plan: null, not_started: false, + } }) + if (url === '/attempts/sessions') return Promise.resolve({ data: [] }) + return Promise.resolve({ data: [] }) + }) + render( + } /> + ) + + // A study session keeps going until everything is answered. At that point + // there is nothing left to resume, so it is the review. + expect(await screen.findByRole('link', { name: 'Review session' })).toBeInTheDocument() + expect(screen.queryByRole('link', { name: 'Resume session' })).toBeNull() }) })