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() }) })