From 2d828c3c03327c9dacd4eeecd37777e846ee80a3 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 01:08:42 +0200 Subject: [PATCH] feat: a repetition does not raise your score; no more deleting a session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sitting the same questions again is practice, not a new measurement. You have already seen the answers, so getting them right the second time says nothing about whether you knew them — and it cannot be allowed to raise a figure that means "how much of this do you know". A repeated session is titled "(repetition)", analysed in full on its own page, and left out of every aggregate: the overall accuracy, the per-quiz history, the averages, and the readiness that drives recommendations. Deleting a single session is gone — control, endpoint, tests and all. A session is a record of work done, and removing one edits the history every figure on the analysis is computed from, which turns a measurement into a number somebody chose. Starting again is still offered whole, under Settings, Your data, which takes everything rather than the parts that flatter. Two layout bugs behind that. The category tree kept its appearance in QuestionBankPage.css, so it looked right on the bank and took whatever the host page did to a label everywhere else — in the question editor that centred the name, leaving it adrift with the count at the far right; it owns its own stylesheet now. And the editor's grid collapsed to `1fr` below 900px, whose automatic minimum lets one unshrinkable child push the column past the window: the page had padding down its left and none down its right because the right was off the screen. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- .../versions/d0e1f2a3b4c5_repetition.py | 28 ++++++++++ backend/app/models/quiz.py | 5 ++ backend/app/routers/attempts.py | 54 ++++++------------- backend/app/routers/study_tools.py | 8 +++ backend/app/services/quiz_builder.py | 6 ++- backend/tests/test_session_lifecycle.py | 35 ++---------- frontend/src/components/CategoryTree.css | 39 ++++++++++++++ frontend/src/components/CategoryTree.jsx | 1 + frontend/src/components/RepeatSession.jsx | 5 +- frontend/src/pages/AnalysisSessionPage.jsx | 44 ++------------- .../src/pages/AnalysisSessionPage.test.jsx | 17 ++---- frontend/src/pages/QuestionBankPage.css | 10 +++- frontend/src/pages/QuestionEditPage.css | 11 +++- 13 files changed, 136 insertions(+), 127 deletions(-) create mode 100644 backend/alembic/versions/d0e1f2a3b4c5_repetition.py create mode 100644 frontend/src/components/CategoryTree.css diff --git a/backend/alembic/versions/d0e1f2a3b4c5_repetition.py b/backend/alembic/versions/d0e1f2a3b4c5_repetition.py new file mode 100644 index 0000000..0082625 --- /dev/null +++ b/backend/alembic/versions/d0e1f2a3b4c5_repetition.py @@ -0,0 +1,28 @@ +"""Mark a repeated session as a repetition. + +Sitting the same questions again is practice, not a new measurement: the +answers have already been seen, so getting them right the second time says +nothing about whether they were known. Such a session is still analysed on its +own page — that is the point of repeating it — but it is left out of the +figures that claim to say how much of the bank you know. + +Revision ID: d0e1f2a3b4c5 +Revises: c9d0e1f2a3b4 +""" +import sqlalchemy as sa +from alembic import op + +revision = "d0e1f2a3b4c5" +down_revision = "c9d0e1f2a3b4" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column("quizzes", sa.Column("is_repetition", sa.Integer(), server_default="0")) + op.create_index("ix_quizzes_is_repetition", "quizzes", ["is_repetition"]) + + +def downgrade() -> None: + op.drop_index("ix_quizzes_is_repetition", table_name="quizzes") + op.drop_column("quizzes", "is_repetition") diff --git a/backend/app/models/quiz.py b/backend/app/models/quiz.py index 49a54dc..9be04ed 100644 --- a/backend/app/models/quiz.py +++ b/backend/app/models/quiz.py @@ -32,6 +32,11 @@ class Quiz(Base): deleted_at = Column(DateTime, nullable=True) # soft delete — null = active is_published = Column(Integer, default=1) # 1 = visible to users, 0 = hidden (admin only) is_shared = Column(Integer, default=0) # 0=private, 1=shared (visible to other users) + # A repetition is practice, not a new measurement. You have already seen + # these questions and their answers, so getting them right the second time + # says nothing about whether you knew them — it cannot raise a percentage + # that is meant to mean "how much of this do you know". + is_repetition = Column(Integer, default=0, index=True) course_id = Column(Integer, ForeignKey("courses.id", ondelete="CASCADE"), nullable=True) # set = course-only quiz, hidden from main page max_attempts = Column(Integer, nullable=True) # null = unlimited questions_per_attempt = Column(Integer, nullable=True) # null = all; set = random subset from pool diff --git a/backend/app/routers/attempts.py b/backend/app/routers/attempts.py index a78791c..95c2adc 100644 --- a/backend/app/routers/attempts.py +++ b/backend/app/routers/attempts.py @@ -429,44 +429,10 @@ def reset_all_practice_data( return {"removed": removed} -@router.delete("/{attempt_id}", status_code=204) -def delete_attempt( - attempt_id: int, - db: Session = Depends(get_db), - current_user: User = Depends(get_current_user), -): - """Delete own attempt and its answers. Cannot delete course quiz attempts.""" - attempt = db.query(QuizAttempt).filter( - QuizAttempt.id == attempt_id, - QuizAttempt.user_id == current_user.id, - ).first() - if not attempt: - raise HTTPException(status_code=404, detail="Attempt not found") - - # Block deletion of course quiz attempts - quiz = db.query(Quiz).filter(Quiz.id == attempt.quiz_id).first() - if quiz and quiz.course_id: - raise HTTPException(status_code=403, detail="Cannot delete course quiz attempts") - - quiz_id = attempt.quiz_id - db.delete(attempt) - db.flush() - # A block is done only while a completed attempt says so. Deleting the one - # that completed it puts the block back to unfinished. - still_done = db.query(QuizAttempt.id).filter( - QuizAttempt.quiz_id == quiz_id, QuizAttempt.user_id == current_user.id, - QuizAttempt.completed_at.isnot(None)).first() - if not still_done: - unmark_block_complete(db, current_user.id, quiz_id) - db.commit() - # Any saved in-progress state and device lock go with it. - try: - import redis as redis_lib - from app.config import settings - r = redis_lib.from_url(settings.REDIS_URL, decode_responses=True) - r.delete(progress_key(current_user.id, attempt_id), active_key(current_user.id, attempt_id)) - except Exception: - logger.warning("Redis unavailable when deleting attempt %s", attempt_id, exc_info=True) +# Deleting a single attempt is gone. A session is a record of work done, and +# removing one edits the history every figure on the analysis is computed from +# — which turns a measurement into something somebody chose. Starting again is +# offered whole instead: POST /attempts/reset-all, under Settings, Your data. @router.get("/quiz/{quiz_id}/in-progress") @@ -541,6 +507,10 @@ def get_quiz_history( QuizAttempt.user_id == current_user.id, QuizAttempt.completed_at.isnot(None), Quiz.course_id.is_(None), + # A repetition is practice, not a new measurement: the answers have + # already been seen, so getting them right again says nothing about + # whether they were known. It has its own analysis; it is not in this. + (Quiz.is_repetition == 0) | (Quiz.is_repetition.is_(None)), (QuizAttempt.expired == 0) | (QuizAttempt.expired.is_(None)), ) .order_by(QuizAttempt.completed_at) @@ -585,6 +555,10 @@ def get_dashboard_stats( QuizAttempt.user_id == current_user.id, QuizAttempt.completed_at.isnot(None), Quiz.course_id.is_(None), + # A repetition is practice, not a new measurement: the answers have + # already been seen, so getting them right again says nothing about + # whether they were known. It has its own analysis; it is not in this. + (Quiz.is_repetition == 0) | (Quiz.is_repetition.is_(None)), (QuizAttempt.expired == 0) | (QuizAttempt.expired.is_(None)), ) .distinct().count() @@ -597,6 +571,10 @@ def get_dashboard_stats( QuizAttempt.user_id == current_user.id, QuizAttempt.completed_at.isnot(None), Quiz.course_id.is_(None), + # A repetition is practice, not a new measurement: the answers have + # already been seen, so getting them right again says nothing about + # whether they were known. It has its own analysis; it is not in this. + (Quiz.is_repetition == 0) | (Quiz.is_repetition.is_(None)), (QuizAttempt.expired == 0) | (QuizAttempt.expired.is_(None)), ) .all() diff --git a/backend/app/routers/study_tools.py b/backend/app/routers/study_tools.py index 980242b..830be31 100644 --- a/backend/app/routers/study_tools.py +++ b/backend/app/routers/study_tools.py @@ -184,6 +184,10 @@ def performance_by_category(db: Session = Depends(get_db), user: User = Depends( Question, Question.id == AttemptAnswer.question_id).filter( QuizAttempt.user_id == user.id, QuizAttempt.completed_at.isnot(None), or_(QuizAttempt.expired == 0, QuizAttempt.expired.is_(None)), Quiz.course_id.is_(None), + # A repetition is practice, not a new measurement: the answers have + # already been seen, so getting them right again says nothing about + # whether they were known. It has its own analysis; it is not in this. + or_(Quiz.is_repetition == 0, Quiz.is_repetition.is_(None)), # A skipped question is not a wrong answer. Counting it as one made a # 360-question sitting that was never worked through read as 0% accuracy # across every category it touched. @@ -277,6 +281,10 @@ def study_recommendations( QuizAttempt.completed_at.isnot(None), or_(QuizAttempt.expired == 0, QuizAttempt.expired.is_(None)), Quiz.course_id.is_(None), + # A repetition is practice, not a new measurement: the answers have + # already been seen, so getting them right again says nothing about + # whether they were known. It has its own analysis; it is not in this. + or_(Quiz.is_repetition == 0, Quiz.is_repetition.is_(None)), *([exam_filter] if exam_filter is not None else []), ).all() diff --git a/backend/app/services/quiz_builder.py b/backend/app/services/quiz_builder.py index 03466d1..c92d7fc 100644 --- a/backend/app/services/quiz_builder.py +++ b/backend/app/services/quiz_builder.py @@ -163,6 +163,9 @@ class TestOptions(BaseModel): mode: Literal["timed", "learning"] = "timed" time_limit_minutes: int | None = Field(default=None, gt=0) is_shared: bool = False + #: Sitting the same questions again. Analysed on its own page, left out of + #: the figures that claim to say how much of the bank you know. + is_repetition: bool = False @field_validator("title") @classmethod @@ -227,7 +230,8 @@ def create_saved_test(db, user, data, question_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=exam_minutes(data, len(ids)), - questions_count=len(ids), is_published=0, is_shared=int(data.is_shared)) + questions_count=len(ids), is_published=0, is_shared=int(data.is_shared), + is_repetition=int(getattr(data, "is_repetition", False))) db.add(quiz) db.flush() add_questions_to_quiz(db, quiz.id, ids) diff --git a/backend/tests/test_session_lifecycle.py b/backend/tests/test_session_lifecycle.py index 38619f8..a2123a1 100644 --- a/backend/tests/test_session_lifecycle.py +++ b/backend/tests/test_session_lifecycle.py @@ -45,6 +45,9 @@ def fake_redis(store): class SessionLifecycleTests(unittest.TestCase): + # Deleting a single attempt is gone: a session is a record of work done, + # and removing one edits the history every figure on the analysis is + # computed from. Starting again is offered whole, via reset-all. def setUp(self): self.bank = fixtures.BuilderTests() self.bank.setUp() @@ -160,38 +163,6 @@ class SessionLifecycleTests(unittest.TestCase): # ── delete ──────────────────────────────────────────────────────────────── - def test_deleting_the_attempt_that_completed_a_block_makes_the_block_unfinished(self): - self.db.add(StudyPlan(id=1, slug="p", name="Plan", kind="set", is_published=1)) - self.db.flush() - self.db.add(StudyPlanBlock(id=10, plan_id=1, position=1, title="Block 1", question_ids=[1, 2])) - self.db.commit() - quiz_id = self.client.post("/study-plans/blocks/10/start", params={"mode": "timed"}).json()["id"] - self.db.add(QuizAttempt(id=800, quiz_id=quiz_id, user_id=self.bank.owner.id, mode="exam", - total_questions=2, score=2, started_at=datetime(2026, 1, 1), - completed_at=datetime(2026, 1, 1, 1))) - self.db.add(AttemptAnswer(attempt_id=800, question_id=1, user_answer="A", is_correct=True)) - progress = self.db.query(StudyPlanBlockProgress).filter_by(quiz_id=quiz_id).one() - progress.completed_at = datetime(2026, 1, 1, 1) - self.db.commit() - self.store[f"quiz_progress:{self.bank.owner.id}:800"] = "{}" - - with patch.dict(sys.modules, {"redis": self.redis}): - self.assertEqual(self.client.delete("/attempts/800").status_code, 204) - self.db.expire_all() - self.assertIsNone(self.db.get(QuizAttempt, 800)) - self.assertEqual(self.db.query(AttemptAnswer).filter_by(attempt_id=800).count(), 0) - self.assertIsNone(self.db.get(StudyPlanBlockProgress, progress.id).completed_at) - self.assertEqual(self.db.get(StudyPlanBlockProgress, progress.id).quiz_id, quiz_id) - self.assertNotIn(f"quiz_progress:{self.bank.owner.id}:800", self.store) - - def test_someone_elses_attempt_cannot_be_deleted(self): - quiz_id, aid = self.timed_quiz() - self.bank.user = self.bank.peer - with patch.dict(sys.modules, {"redis": self.redis}): - self.assertEqual(self.client.delete(f"/attempts/{aid}").status_code, 404) - - # ── reset ───────────────────────────────────────────────────────────────── - def test_reset_removes_what_was_practised_and_keeps_what_was_made(self): quiz_id, aid = self.timed_quiz() uid = self.bank.owner.id diff --git a/frontend/src/components/CategoryTree.css b/frontend/src/components/CategoryTree.css new file mode 100644 index 0000000..54cd083 --- /dev/null +++ b/frontend/src/components/CategoryTree.css @@ -0,0 +1,39 @@ +/* The tree carries its own appearance. + * + * These rules lived in QuestionBankPage.css, so the tree looked right on the + * bank and took whatever the host page did to a label everywhere else — in the + * question editor that centred the name, leaving it floating in the middle of + * the row with the count adrift at the far right. A component that is used in + * four places should look the same in all four. + */ +.category-tree { list-style: none; margin: 0; padding: 0; max-height: 46vh; overflow-y: auto; } +.category-tree ul { list-style: none; margin: 0 0 0 16px; padding: 0; } +.category-tree li { margin: 1px 0; } + +.category-tree label { + display: flex; align-items: baseline; gap: 8px; + font-size: 0.84rem; text-align: left; cursor: pointer; +} +.category-tree label > input[type="checkbox"] { flex: none; } +.category-tree-name { flex: 1; min-width: 0; text-align: left; overflow-wrap: anywhere; } +.category-tree-count { flex: none; color: var(--text-muted); font-size: 0.74rem; } +.category-tree-excluded { opacity: 0.5; } + +.category-tree-branch { display: flex; align-items: center; cursor: pointer; list-style: none; } +.category-tree-branch::-webkit-details-marker { display: none; } +.category-tree-branch > label { flex: 1; min-width: 0; } +.category-tree-chevron { + display: inline-block; width: 7px; height: 7px; flex-shrink: 0; + border-right: 2px solid var(--text-muted); border-bottom: 2px solid var(--text-muted); + transform: rotate(-45deg); margin-right: 6px; transition: transform 0.15s ease; +} +details[open] > .category-tree-branch .category-tree-chevron { transform: rotate(45deg); } + +.category-tree-search { + width: 100%; margin-bottom: 8px; padding: 8px 10px; + /* 16px on touch: iOS zooms in on anything smaller and never zooms back. */ + font-size: 16px; font-family: inherit; + border: 1px solid var(--border); border-radius: 8px; + background: var(--input-bg); color: var(--text); +} +@media (min-width: 700px) { .category-tree-search { font-size: 0.84rem; } } diff --git a/frontend/src/components/CategoryTree.jsx b/frontend/src/components/CategoryTree.jsx index 9385270..b701e54 100644 --- a/frontend/src/components/CategoryTree.jsx +++ b/frontend/src/components/CategoryTree.jsx @@ -1,4 +1,5 @@ import { useState } from 'react' +import './CategoryTree.css' export default function CategoryTree({ categories, selectedIds, onToggle, excludedId = null, searchable = false }) { const [query, setQuery] = useState('') diff --git a/frontend/src/components/RepeatSession.jsx b/frontend/src/components/RepeatSession.jsx index 3916dad..3d104b1 100644 --- a/frontend/src/components/RepeatSession.jsx +++ b/frontend/src/components/RepeatSession.jsx @@ -61,9 +61,12 @@ export default function RepeatSession({ title, rows, onClose }) { // Shuffled, so repeating twice is not the same order twice. const ids = [...pool].sort(() => Math.random() - 0.5).slice(0, count) const res = await api.post('/questions/from-bank', { - title: `${title} — again`, + title: `${title} (repetition)`, question_ids: ids, mode: 'study', + // Practice, not a new measurement: it is analysed on its own page but + // left out of the figures that say how much of the bank you know. + is_repetition: true, }) navigate(`/study/${res.data.id}?start=1`) } catch (err) { diff --git a/frontend/src/pages/AnalysisSessionPage.jsx b/frontend/src/pages/AnalysisSessionPage.jsx index fbd755d..6e43ae7 100644 --- a/frontend/src/pages/AnalysisSessionPage.jsx +++ b/frontend/src/pages/AnalysisSessionPage.jsx @@ -86,27 +86,9 @@ export default function AnalysisSessionPage() { const [sort, setSort] = useState('position') // Ten at a time: a session of forty is a table nobody reads to the end of. const [page, setPage] = useState(0) - const [confirmDelete, setConfirmDelete] = useState(false) const [repeating, setRepeating] = useState(false) - const [deleting, setDeleting] = useState(false) const navigate = useNavigate() - const deleteSession = async () => { - setDeleting(true) - try { - // The attempt the page is showing, not the one named in the URL: reached - // by quiz (a session whose only attempt is still in progress) there is no - // attemptId in the path, and this deleted "/attempts/undefined". - await api.delete(`/attempts/${data?.attempt_id ?? attemptId}`) - navigate('/sessions', { replace: true }) - } catch { - setDeleting(false) - setError('Could not delete this session') - } - } - - // Addressed by attempt, or — for a session nobody has sat — by quiz, which - // answers with the same shape at zero. The page does not branch on which. const load = useCallback(() => { setLoading(true) const url = attemptId ? `/attempts/${attemptId}/analysis` : `/attempts/quiz/${quizId}/analysis` @@ -163,12 +145,6 @@ export default function AnalysisSessionPage() { )} - {confirmDelete && ( -

- This removes the attempt and its answers. Your overall statistics are - recalculated without it, and it cannot be undone. -

- )} {/* The figures below are all zero and every row reads "skipped", which is the true picture. Saying why keeps that from looking like a @@ -253,21 +229,11 @@ export default function AnalysisSessionPage() { )} - {/* 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 ? ( - - - - - ) : ( - - ))} + {/* No delete. A session is a record of work done, and removing + one edits the history the analysis is computed from — which + makes every figure on this page a number somebody chose + rather than a number they earned. Starting again is offered + whole, under Settings, Your data. */} diff --git a/frontend/src/pages/AnalysisSessionPage.test.jsx b/frontend/src/pages/AnalysisSessionPage.test.jsx index 8742fed..c079aa0 100644 --- a/frontend/src/pages/AnalysisSessionPage.test.jsx +++ b/frontend/src/pages/AnalysisSessionPage.test.jsx @@ -75,7 +75,7 @@ describe('a session nobody has sat', () => { expect(await screen.findByRole('link', { name: 'Resume session' })) .toHaveAttribute('href', '/study/3?start=1') expect(screen.queryByRole('link', { name: /Review/ })).not.toBeInTheDocument() - expect(screen.queryByRole('button', { name: 'Delete session' })).not.toBeInTheDocument() + expect(screen.queryByRole('button', { name: /Delete/ })).toBeNull() }) it('asks the quiz endpoint, since there is no attempt to ask about', async () => { @@ -96,7 +96,10 @@ describe('a session that has been sat', () => { // 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() - expect(screen.getByRole('button', { name: 'Delete session' })).toBeInTheDocument() + // No delete. A session is a record of work done; removing one edits the + // history every figure here is computed from. Starting again is offered + // whole, under Settings, Your data. + expect(screen.queryByRole('button', { name: /Delete/ })).toBeNull() // Nothing outstanding, so no "still unanswered" note. expect(screen.queryByText(/still unanswered/)).not.toBeInTheDocument() }) @@ -117,16 +120,6 @@ describe('a session whose only attempt is still in progress', () => { mock({ ...SAT, attempt_id: 77, completed_at: null, answered: 0, score: 0, percent: 0 }) }) - it('deletes the attempt the page is showing, not the one named in the URL', async () => { - mountQuiz() - await screen.findByRole('button', { name: 'Delete session' }) - await userEvent.click(screen.getByRole('button', { name: 'Delete session' })) - api.delete.mockResolvedValue({}) - await userEvent.click(screen.getByRole('button', { name: 'Delete for good' })) - // It used to send /attempts/undefined, and nothing happened. - await waitFor(() => expect(api.delete).toHaveBeenCalledWith('/attempts/77')) - }) - it('offers to resume it, not to review it', async () => { mountQuiz() // Review or resume, never both: a session you are part-way through has diff --git a/frontend/src/pages/QuestionBankPage.css b/frontend/src/pages/QuestionBankPage.css index 91a627c..201fe78 100644 --- a/frontend/src/pages/QuestionBankPage.css +++ b/frontend/src/pages/QuestionBankPage.css @@ -25,8 +25,14 @@ .category-tree { list-style: none; margin: 0; padding: 0; max-height: 46vh; overflow-y: auto; } .category-tree ul { list-style: none; margin: 0 0 0 16px; padding: 0; } .category-tree li { margin: 1px 0; } -.category-tree label { display: flex; align-items: baseline; gap: 6px; font-size: .84rem; cursor: pointer; } -.category-tree-count { color: var(--text-muted); font-size: .74rem; } +.category-tree label { display: flex; align-items: baseline; gap: 8px; font-size: .84rem; cursor: pointer; text-align: left; } +.category-tree label > input[type="checkbox"] { flex: none; } +/* The name owns the row and reads from the left, wherever the tree is used. + It had no styles of its own, so it took whatever the host page did to a + label — which in the question editor centred it, leaving the name floating + in the middle with the count adrift at the far right. */ +.category-tree-name { flex: 1; min-width: 0; text-align: left; overflow-wrap: anywhere; } +.category-tree-count { flex: none; color: var(--text-muted); font-size: .74rem; } .category-tree-excluded { opacity: .5; } .category-tree-branch { display: flex; align-items: center; cursor: pointer; list-style: none; } .category-tree-branch::-webkit-details-marker { display: none; } diff --git a/frontend/src/pages/QuestionEditPage.css b/frontend/src/pages/QuestionEditPage.css index 485c29f..f9387da 100644 --- a/frontend/src/pages/QuestionEditPage.css +++ b/frontend/src/pages/QuestionEditPage.css @@ -119,8 +119,15 @@ @media (max-width: 900px) { - .qe-grid { grid-template-columns: 1fr; } - .qe-aside { position: static; } + /* minmax(0, 1fr), not 1fr: a plain `1fr` track takes its minimum from its + content, so one child that will not shrink — the formatting bar, a long + word, a wide table — pushes the column past the window. The page then had + padding down its left and none down its right, because the right was off + the screen rather than absent. */ + .qe-grid { grid-template-columns: minmax(0, 1fr); } + .qe-aside { position: static; min-width: 0; } + /* The unnamed column holding the cards. */ + .qe-grid > * { min-width: 0; } } @media (max-width: 640px) { .qe-top-actions { width: 100%; }