From a16e45a7f109e3d2f6b1ffaa9e59e3535e566ed7 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 05:28:16 +0200 Subject: [PATCH] fix: user deletion, a real end-block dialog, and the axes people pick by MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a user failed with a not-null violation from quiz_attempts. Every foreign key to users is already CASCADE or SET NULL in Postgres, but the ORM relationships had no passive_deletes, so SQLAlchemy insisted on emptying each one itself by writing NULL into columns that refuse it. passive_deletes leaves it to the database, which knows. The two tables that genuinely cannot forget a user — question_categories and quiz_categories are NOT NULL and NO ACTION — hand their rows to the administrator doing the deleting: the taxonomy is the site's, not the author's. /tags counted through question_tag_links, which is now empty, so every organ system read zero and an active exam hid them entirely. It counts through the topics that carry them instead: 2,919 of 2,924 questions, all sixteen systems with real numbers. The session builder's Disciplines and Symptoms pickers were over the retired vocabularies and are gone — Topics is the same axis said once and said better, 673 against 203. The end-block dialog offered one button. A confirmation with one button is not a confirmation: it now leads with the way back into the block, says how many are unanswered as a sentence rather than a grid to count by eye, and the unanswered are numbers you can press to go there. Registration asks for the password twice, on both forms — a password you cannot see is one you can mistype into an account you then cannot open. The public pages had no footer, so signing in meant losing the way to About, Contact and the clinical disclaimer. They sit in a plain layout that keeps it. Draft batches can be filed from the workbench: the topic they file into is a picker at the top, and nothing crosses over until it is set. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/models/user.py | 18 +++- backend/app/routers/admin.py | 12 ++- backend/app/routers/tags.py | 112 ++++++++++++++------- backend/tests/test_exam_scoped_tags.py | 76 +++++++------- backend/tests/test_tag_hierarchy.py | 112 +++++++++------------ frontend/src/App.jsx | 38 +++++-- frontend/src/components/DraftBatch.css | 16 +++ frontend/src/components/DraftBatch.jsx | 47 ++++++++- frontend/src/pages/CustomQuizPage.jsx | 43 ++------ frontend/src/pages/CustomQuizPage.test.jsx | 18 ++-- frontend/src/pages/LandingPage.jsx | 23 ++++- frontend/src/pages/LoginTurnstile.test.jsx | 4 + frontend/src/pages/QuizPage.jsx | 57 ++++++++--- frontend/src/pages/QuizPage.test.jsx | 18 ++-- frontend/src/pages/QuizPlayer.css | 16 +++ frontend/src/pages/RegisterInvite.test.jsx | 3 + frontend/src/pages/RegisterPage.jsx | 18 +++- frontend/src/pages/ToolsPage.test.jsx | 22 +++- 18 files changed, 432 insertions(+), 221 deletions(-) diff --git a/backend/app/models/user.py b/backend/app/models/user.py index 2b7099e..1cbd220 100644 --- a/backend/app/models/user.py +++ b/backend/app/models/user.py @@ -22,11 +22,19 @@ class User(Base): reminders_disabled = Column(Boolean, default=False, nullable=False, server_default=sa_false()) created_at = Column(DateTime, default=datetime.utcnow) - documents = relationship("PDFDocument", back_populates="user") - quizzes = relationship("Quiz", back_populates="user") - attempts = relationship("QuizAttempt", back_populates="user") - favorites = relationship("Favorite", back_populates="user", cascade="all, delete-orphan") - note = relationship("UserNote", back_populates="user", cascade="all, delete-orphan", uselist=False) + # passive_deletes leaves the child rows to the database, which already + # knows what to do with every one of them — the foreign keys are CASCADE or + # SET NULL. Without it SQLAlchemy insists on emptying each relationship + # itself first, by writing NULL into columns that are NOT NULL, and + # deleting a user failed with a constraint violation from a table nobody + # was looking at. + documents = relationship("PDFDocument", back_populates="user", passive_deletes=True) + quizzes = relationship("Quiz", back_populates="user", passive_deletes=True) + attempts = relationship("QuizAttempt", back_populates="user", passive_deletes=True) + favorites = relationship("Favorite", back_populates="user", + cascade="all, delete-orphan", passive_deletes=True) + note = relationship("UserNote", back_populates="user", cascade="all, delete-orphan", + uselist=False, passive_deletes=True) @property def is_admin(self): diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index a5d1918..41ca78b 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -82,9 +82,15 @@ def delete_user( raise HTTPException(status_code=404, detail="User not found") if user.id == admin.id: raise HTTPException(status_code=400, detail="Cannot delete yourself") - # Nullify non-cascading FKs before delete - db.execute(text("UPDATE question_categories SET user_id = NULL WHERE user_id = :uid"), {"uid": user_id}) - db.execute(text("UPDATE quiz_categories SET user_id = NULL WHERE user_id = :uid"), {"uid": user_id}) + # Two tables name a user and refuse to forget one: question_categories and + # quiz_categories are NOT NULL and NO ACTION. The taxonomy is the site's, + # not the author's, so it is handed to the administrator doing the deleting + # rather than deleted with them. Everything else the database already knows + # what to do with — every other foreign key is CASCADE or SET NULL. + db.execute(text("UPDATE question_categories SET user_id = :new WHERE user_id = :uid"), + {"new": admin.id, "uid": user_id}) + db.execute(text("UPDATE quiz_categories SET user_id = :new WHERE user_id = :uid"), + {"new": admin.id, "uid": user_id}) db.delete(user) db.commit() diff --git a/backend/app/routers/tags.py b/backend/app/routers/tags.py index cd41a30..6321f2f 100644 --- a/backend/app/routers/tags.py +++ b/backend/app/routers/tags.py @@ -1,16 +1,25 @@ -"""AI-powered question tagging/classification — subjects, diseases, keywords. -Tables are created via raw SQL in setup_pgvector() to avoid race conditions with multiple workers. +"""Organ systems: the vocabulary, and how many questions reach each one. + +Once this served four vocabularies a model wrote per question — subjects, +diseases, keywords, systems. The first three became the category tree and have +been retired; what is left is the sixteen systems, which a topic points at. + +The table is created via raw SQL in setup_pgvector() to avoid race conditions +with multiple workers, so there is no ORM class for it. """ import logging - +from collections import defaultdict from typing import Literal from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel, Field -from sqlalchemy import text +from sqlalchemy import select, text from sqlalchemy.orm import Session from app.database import get_db +from app.models.exam import QuestionExamLink +from app.models.question import Question +from app.models.question_category import QuestionCategory, QuestionCategoryLink from app.models.user import User from app.utils.auth import get_current_user, require_moderator @@ -20,39 +29,67 @@ log = logging.getLogger(__name__) @router.get("") def get_tags(db: Session = Depends(get_db), current_user: User = Depends(get_current_user)): - """Return all tags grouped by type, with question counts.""" - # Counts are computed within the learner's active exam, and tags scoped to a - # different exam are left out — an unscoped list would offer disciplines that - # cannot match anything they are studying. - exam_id = getattr(current_user, "active_exam_id", None) - rows = db.execute(text(""" - SELECT t.id, t.name, t.type, t.parent_id, t.sort_order, - COUNT(DISTINCT tl.question_id) AS count - FROM question_tags t - LEFT JOIN question_tag_links tl ON tl.tag_id = t.id - LEFT JOIN question_exam_links el - ON el.question_id = tl.question_id AND (:exam_id IS NULL OR el.exam_id = :exam_id) - WHERE (t.exam_id IS NULL OR :exam_id IS NULL OR t.exam_id = :exam_id) - AND (:exam_id IS NULL OR tl.question_id IS NULL OR el.question_id IS NOT NULL) - GROUP BY t.id, t.name, t.type, t.parent_id, t.sort_order - ORDER BY t.sort_order, count DESC, t.name - """), {"exam_id": exam_id}).fetchall() + """The organ systems, with how many questions reach each one. - result = {"subjects": [], "diseases": [], "keywords": [], "systems": []} - mapping = {"subject": "subjects", "disease": "diseases", - "keyword": "keywords", "system": "systems"} - # A parent with no direct questions still has to appear, or its children - # become unreachable in a tree picker. - has_children = {r.parent_id for r in rows if r.parent_id} - for r in rows: - key = mapping.get(r.type) - if not key: + A question reaches a system through the topic it is filed under, and + through every topic above that one — the same rule the analysis groups by, + so the number here is the number the filter will return. + + It used to come from a symptom keyword the question happened to mention. + Those keywords are retired, and with them the subject and disease + vocabularies that had already become the topic tree. The shape of the + response is kept so nothing that reads it has to be rewritten to find an + empty list where it expected one. + """ + exam_id = getattr(current_user, "active_exam_id", None) + systems = db.execute(text( + "SELECT id, name, sort_order FROM question_tags " + "WHERE type = 'system' AND parent_id IS NULL ORDER BY sort_order, name")).fetchall() + + categories = db.query(QuestionCategory).all() + parents = {cat.id: cat.parent_id for cat in categories} + own = {cat.id: cat.system_id for cat in categories} + + def system_of(cid): + """The topic's own system, or the nearest one above it.""" + walk, guard = cid, 0 + while walk is not None and guard < 20: + if own.get(walk): + return own[walk] + walk = parents.get(walk) + guard += 1 + return None + + effective = {cid: system_of(cid) for cid in parents} + + # Scoped to what the learner is studying, so the number matches what the + # filter will actually return. + live = db.query(Question.id).filter(Question.deleted_at.is_(None)) + if exam_id: + live = live.filter(Question.id.in_( + select(QuestionExamLink.question_id).where(QuestionExamLink.exam_id == exam_id))) + in_scope = {qid for (qid,) in live.all()} + + counts: dict[int, set[int]] = defaultdict(set) + filings = [ + *db.query(Question.id, Question.question_category_id).filter( + Question.question_category_id.isnot(None)).all(), + # A question counts towards every topic it is filed under, primary or + # not, exactly as the analysis counts it. + *db.query(QuestionCategoryLink.question_id, QuestionCategoryLink.category_id).all(), + ] + for qid, cid in filings: + if qid not in in_scope: continue - if exam_id and r.count == 0 and r.id not in has_children: - continue - result[key].append({"id": r.id, "name": r.name, "count": r.count, - "parent_id": r.parent_id}) - return result + system = effective.get(cid) + if system: + counts[system].add(qid) + + return { + "subjects": [], "diseases": [], "keywords": [], + "systems": [{"id": row.id, "name": row.name, "count": len(counts.get(row.id, ())), + "parent_id": None} for row in systems], + } class TagWrite(BaseModel): @@ -164,6 +201,11 @@ def delete_tag(tag_id: int, move_to: int | None = None, db: Session = Depends(ge db.execute(text("UPDATE question_tags SET parent_id = :p WHERE parent_id = :i"), {"p": tag.parent_id, "i": tag_id}) db.execute(text("DELETE FROM question_tag_links WHERE tag_id = :i"), {"i": tag_id}) + # Topics point at this row. Postgres would set them null on its own, but + # the column carries no ForeignKey in the model — question_tags has no ORM + # class — so saying it here is what makes it true everywhere. + db.query(QuestionCategory).filter(QuestionCategory.system_id == tag_id).update( + {"system_id": None}, synchronize_session=False) db.execute(text("DELETE FROM question_tags WHERE id = :i"), {"i": tag_id}) db.commit() diff --git a/backend/tests/test_exam_scoped_tags.py b/backend/tests/test_exam_scoped_tags.py index 2e63d2a..364d8ab 100644 --- a/backend/tests/test_exam_scoped_tags.py +++ b/backend/tests/test_exam_scoped_tags.py @@ -1,4 +1,9 @@ -"""Tags may be shared across exams or scoped to one, and facets reflect the choice. +"""System counts follow the exam the learner is studying for. + +The subject and keyword vocabularies these once scoped are retired; a question +reaches a system through the topic it is filed under. What still has to hold is +that the number beside a system is the number the filter will return — which +means counting only what is in scope. Run: DATABASE_URL=sqlite:///:memory: PYTHONPATH=backend python -m unittest discover -s backend/tests """ @@ -17,6 +22,7 @@ from app.database import Base, get_db from app.models.course import Course # noqa — Quiz.course_id FK needs the table in metadata. from app.models.exam import Exam, QuestionExamLink from app.models.question import Question +from app.models.question_category import QuestionCategory from app.models.user import User from app.routers import tags from app.utils.auth import get_current_user @@ -53,18 +59,21 @@ class ExamScopedTagTests(unittest.TestCase): question_type="mcq", options=["a", "b"], correct_answer="a")) self.db.flush() self.db.add(QuestionExamLink(question_id=qid, exam_id=exam_id)) - # Cardiology is shared; each Pediatrics is scoped to its own exam. self.db.execute(text(""" - INSERT INTO question_tags (id, name, type, exam_id) VALUES - (10, 'Cardiology', 'subject', NULL), - (11, 'Pediatrics', 'subject', 1), - (12, 'Pediatrics', 'subject', 2), - (13, 'Obstetrics', 'subject', NULL) - """)) - self.db.execute(text(""" - INSERT INTO question_tag_links (question_id, tag_id) VALUES - (1, 10), (1, 11), (2, 11), (3, 12), (3, 10) + INSERT INTO question_tags (id, name, type, parent_id) VALUES + (10, 'Cardiovascular System', 'system', NULL), + (11, 'Respiratory System', 'system', NULL), + (12, 'Immune System', 'system', NULL) """)) + # Questions 1 and 2 are cardiovascular, question 3 respiratory. Only + # question 3 belongs to the second exam. + self.db.add_all([ + QuestionCategory(id=1, name="Cardiology", user_id=1, system_id=10), + QuestionCategory(id=2, name="Pulmonology", user_id=1, system_id=11), + ]) + self.db.flush() + for qid, cid in ((1, 1), (2, 1), (3, 2)): + self.db.get(Question, qid).question_category_id = cid self.db.commit() app = FastAPI() @@ -78,38 +87,33 @@ class ExamScopedTagTests(unittest.TestCase): self.db.close() self.engine.dispose() - def subjects(self): - return {t["name"]: t["count"] for t in self.client.get("/tags").json()["subjects"]} + def systems(self): + return {t["name"]: t["count"] for t in self.client.get("/tags").json()["systems"]} - def test_the_same_name_can_exist_once_per_exam(self): - rows = self.db.execute(text( - "SELECT exam_id FROM question_tags WHERE name='Pediatrics' ORDER BY exam_id")).fetchall() - self.assertEqual([r[0] for r in rows], [1, 2]) + def test_without_an_exam_everything_is_counted(self): + self.assertEqual(self.systems(), + {"Cardiovascular System": 2, "Respiratory System": 1, + "Immune System": 0}) - def test_without_an_exam_every_tag_is_offered(self): - rows = self.client.get("/tags").json()["subjects"] - # Both Pediatrics rows appear, since neither exam is in scope. - self.assertEqual(sorted((t["id"], t["count"]) for t in rows), - [(10, 2), (11, 2), (12, 1), (13, 0)]) - - def test_choosing_an_exam_scopes_counts_and_hides_the_other_exams_tags(self): + def test_choosing_an_exam_narrows_the_counts_to_it(self): self.user.active_exam_id = 1 self.db.commit() - subjects = self.subjects() - # Cardiology is shared but only question 1 is in this exam. - self.assertEqual(subjects["Cardiology"], 1) - self.assertEqual(subjects["Pediatrics"], 2) - # Obstetrics matches nothing here, and Step 2's Pediatrics is out of scope. - self.assertNotIn("Obstetrics", subjects) - self.assertEqual(len([t for t in self.client.get("/tags").json()["subjects"] - if t["name"] == "Pediatrics"]), 1) + # Questions 1 and 2 are in this exam; question 3 is not. + self.assertEqual(self.systems()["Cardiovascular System"], 2) + self.assertEqual(self.systems()["Respiratory System"], 0) - def test_switching_exam_switches_which_pediatrics_is_offered(self): + def test_switching_exam_switches_the_counts(self): self.user.active_exam_id = 2 self.db.commit() - subjects = self.subjects() - self.assertEqual(subjects["Pediatrics"], 1) # question 3 only - self.assertEqual(subjects["Cardiology"], 1) + self.assertEqual(self.systems()["Cardiovascular System"], 0) + self.assertEqual(self.systems()["Respiratory System"], 1) + + def test_a_system_with_nothing_in_scope_is_still_offered(self): + self.user.active_exam_id = 2 + self.db.commit() + # Shown at nought rather than hidden: a filter that silently loses an + # option teaches nobody why their bank has no cardiology in it. + self.assertIn("Immune System", self.systems()) if __name__ == "__main__": diff --git a/backend/tests/test_tag_hierarchy.py b/backend/tests/test_tag_hierarchy.py index 30cf6a9..0be3641 100644 --- a/backend/tests/test_tag_hierarchy.py +++ b/backend/tests/test_tag_hierarchy.py @@ -17,6 +17,7 @@ from app.database import Base, get_db from app.models.course import Course # noqa — Quiz.course_id FK needs the table in metadata. from app.models.media import MediaAsset # noqa — health report walks every embeddable table. from app.models.question import Question +from app.models.question_category import QuestionCategory from app.models.user import User from app.routers import tags from app.utils.auth import get_current_user @@ -49,11 +50,17 @@ class TagHierarchyTests(unittest.TestCase): self.db.execute(text(""" INSERT INTO question_tags (id, name, type, parent_id, sort_order) VALUES (10, 'Nervous System & Special Senses', 'system', NULL, 0), - (11, 'Seizure', 'keyword', 10, 100), - (12, 'Headache', 'keyword', 10, 100), - (13, 'Cardiology', 'subject', NULL, 100) + (11, 'Respiratory System', 'system', NULL, 1) """)) - self.db.execute(text("INSERT INTO question_tag_links (question_id, tag_id) VALUES (1, 11), (2, 11)")) + # A question reaches a system through the topic it is filed under. + self.db.add_all([ + QuestionCategory(id=1, name="Neurology", user_id=1, system_id=10), + QuestionCategory(id=2, name="Seizure", parent_id=1, user_id=1), + QuestionCategory(id=3, name="Pulmonology", user_id=1, system_id=11), + ]) + self.db.flush() + for qid, cid in ((1, 2), (2, 2), (3, 3)): + self.db.get(Question, qid).question_category_id = cid self.db.commit() self.user = self.mod @@ -68,80 +75,53 @@ class TagHierarchyTests(unittest.TestCase): self.db.close() self.engine.dispose() - def test_the_tree_is_returned_with_parents(self): + def test_systems_are_counted_through_the_topics_that_carry_them(self): body = self.client.get("/tags").json() - systems = {t["name"]: t for t in body["systems"]} - self.assertIn("Nervous System & Special Senses", systems) - keywords = {t["name"]: t["parent_id"] for t in body["keywords"]} - # Symptoms hang off the system they present in. - self.assertEqual(keywords["Seizure"], 10) - self.assertEqual(keywords["Headache"], 10) + systems = {t["name"]: t["count"] for t in body["systems"]} + # Two questions under Seizure, whose own topic has no system, so they + # reach the one on Neurology above it. + self.assertEqual(systems["Nervous System & Special Senses"], 2) + self.assertEqual(systems["Respiratory System"], 1) - def test_a_moderator_can_add_a_top_level_entry_and_a_child(self): - top = self.client.post("/tags/", json={"name": "Respiratory System", "type": "system"}) - self.assertEqual(top.status_code, 201, top.text) - parent_id = top.json()["id"] + def test_the_retired_vocabularies_answer_empty_rather_than_missing(self): + body = self.client.get("/tags").json() + # Subjects, diseases and keywords became the topic tree. The shape of + # the response is kept so nothing reading it has to be rewritten. + for gone in ("subjects", "diseases", "keywords"): + self.assertEqual(body[gone], []) - child = self.client.post("/tags/", json={"name": "Wheeze", "type": "keyword", - "parent_id": parent_id}) - self.assertEqual(child.status_code, 201, child.text) + def test_a_moderator_can_add_a_system(self): + made = self.client.post("/tags/", json={"name": "Endocrine System", "type": "system"}) + self.assertEqual(made.status_code, 201, made.text) # It shows up immediately, which is what every picker reads. - body = self.client.get("/tags").json() - self.assertIn("Wheeze", [t["name"] for t in body["keywords"]]) - - def test_a_child_must_match_its_parents_kind(self): - response = self.client.post("/tags/", json={"name": "Nonsense", "type": "keyword", - "parent_id": 13}) - self.assertEqual(response.status_code, 400) + self.assertIn("Endocrine System", + [t["name"] for t in self.client.get("/tags").json()["systems"]]) def test_duplicate_names_in_the_same_place_are_refused(self): self.assertEqual(self.client.post("/tags/", json={ - "name": "Seizure", "type": "keyword", "parent_id": 10}).status_code, 409) - # The same name under a different parent is fine. - self.assertEqual(self.client.post("/tags/", json={ - "name": "Seizure", "type": "keyword"}).status_code, 201) + "name": "Respiratory System", "type": "system"}).status_code, 409) + + def test_renaming_a_system_keeps_the_topics_pointing_at_it(self): + self.assertEqual(self.client.patch( + "/tags/10", json={"name": "Neurological System"}).status_code, 200) + systems = {t["name"]: t["count"] for t in self.client.get("/tags").json()["systems"]} + # The topics point at the row, not the word, so the count survives. + self.assertEqual(systems["Neurological System"], 2) + + def test_deleting_a_system_leaves_its_topics_pointing_at_nothing(self): + from app.models.question_category import QuestionCategory + self.client.delete("/tags/10") + self.db.expire_all() + # The foreign key is SET NULL in Postgres. Null means nobody has said + # which system this topic belongs to — it is not a claim that it has + # none, and the analysis leaves such a topic out rather than guessing. + self.assertIsNone(self.db.get(QuestionCategory, 1).system_id) def test_a_tag_cannot_become_its_own_descendants_child(self): - self.assertEqual(self.client.patch("/tags/10", json={"parent_id": 11}).status_code, 400) self.assertEqual(self.client.patch("/tags/10", json={"parent_id": 10}).status_code, 400) - def test_renaming_and_reparenting(self): - self.assertEqual(self.client.patch("/tags/12", json={"name": "Head pain"}).status_code, 200) - self.assertEqual(self.db.execute(text( - "SELECT name FROM question_tags WHERE id = 12")).scalar(), "Head pain") - self.client.patch("/tags/12", json={"parent_id": None}) - self.assertIsNone(self.db.execute(text( - "SELECT parent_id FROM question_tags WHERE id = 12")).scalar()) - - def test_questions_attached_to_a_tag_appear_in_its_count(self): - response = self.client.post("/tags/12/questions", json={"question_ids": [1, 2, 3]}) - self.assertEqual(response.json()["added"], 3) - counts = {t["name"]: t["count"] for t in self.client.get("/tags").json()["keywords"]} - self.assertEqual(counts["Headache"], 3) - # Adding the same ones again is a no-op rather than a duplicate. - self.assertEqual(self.client.post("/tags/12/questions", - json={"question_ids": [1, 2]}).json()["added"], 0) - - def test_deleting_reparents_children_rather_than_orphaning_them(self): - self.client.delete("/tags/10") - # Seizure and Headache were under it; they rise to top level, not nowhere. - rows = self.db.execute(text( - "SELECT parent_id FROM question_tags WHERE id IN (11, 12)")).fetchall() - self.assertEqual([r[0] for r in rows], [None, None]) - - def test_deleting_can_move_its_questions_onto_another_tag(self): - self.client.delete("/tags/11", params={"move_to": 12}) - counts = {t["name"]: t["count"] for t in self.client.get("/tags").json()["keywords"]} - self.assertEqual(counts["Headache"], 2) - def test_editing_the_taxonomy_is_moderator_only(self): self.user = self.learner - self.assertEqual(self.client.post("/tags/", json={"name": "X", "type": "keyword"}).status_code, 403) + self.assertEqual(self.client.post("/tags/", json={"name": "X", "type": "system"}).status_code, 403) self.assertEqual(self.client.patch("/tags/11", json={"name": "X"}).status_code, 403) self.assertEqual(self.client.delete("/tags/11").status_code, 403) - self.assertEqual(self.client.post("/tags/11/questions", - json={"question_ids": [1]}).status_code, 403) - - -if __name__ == "__main__": - unittest.main() diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index 20e776a..f2ec8c7 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -77,6 +77,24 @@ function AppLayout() { ) } +/** + * The frame for a page anybody can reach without an account. + * + * No navigation — a Sign In link on the sign-in page is noise — but the footer + * stays, because About, Contact and the clinical disclaimer are not things to + * hide from someone who has not signed in yet. + */ +function PublicLayout() { + return ( +
+
+ +
+ +
+ ) +} + /** * Somewhere this account cannot go. * @@ -138,15 +156,19 @@ function AppRoutes() { return ( }> - {/* Always public */} + {/* Always public. The landing page carries its own frame; the rest sit + in a plain one that still has the footer — signing in is not a + reason to lose the way to About, Contact or the disclaimer. */} } /> - : } /> - : } /> - } /> - } /> - } /> - } /> - } /> + }> + : } /> + : } /> + } /> + } /> + } /> + } /> + } /> + {/* Authenticated app — wrapped in AppLayout */} }> diff --git a/frontend/src/components/DraftBatch.css b/frontend/src/components/DraftBatch.css index 6fefdd4..49dda50 100644 --- a/frontend/src/components/DraftBatch.css +++ b/frontend/src/components/DraftBatch.css @@ -57,3 +57,19 @@ } @media (min-width: 700px) { .db-edit textarea, .db-edit input { font-size: 0.86rem; } } .db-edit-actions { display: flex; gap: 8px; } + +/* Where the batch files into. Above the list, because it is true of every row + in it and nothing can leave until it is answered. */ +.db-filing { + display: flex; align-items: center; gap: 12px; flex-wrap: wrap; + padding: 11px 14px; margin-bottom: 10px; + background: var(--bg); border: 1px solid var(--border); border-radius: 10px; +} +.db-filing label { display: flex; align-items: center; gap: 9px; flex: 1 1 320px; min-width: 0; } +.db-filing span { font-size: 0.82rem; font-weight: 600; color: var(--text-muted); white-space: nowrap; } +.db-filing select { + flex: 1; min-width: 0; padding: 7px 10px; font: inherit; font-size: 0.84rem; + border: 1px solid var(--border); border-radius: 8px; + background: var(--input-bg); color: var(--text); +} +.db-filing-note { flex: 1 1 100%; font-size: 0.78rem; line-height: 1.5; color: var(--wrong-fg); } diff --git a/frontend/src/components/DraftBatch.jsx b/frontend/src/components/DraftBatch.jsx index 5e1cb88..98f7806 100644 --- a/frontend/src/components/DraftBatch.jsx +++ b/frontend/src/components/DraftBatch.jsx @@ -22,6 +22,10 @@ export default function DraftBatch({ batchId, onChanged }) { const [picked, setPicked] = useState(() => new Set()) const [editing, setEditing] = useState(null) const [draftEdit, setDraftEdit] = useState(null) + // Where the whole batch files into. Said once here rather than on every + // draft, and required before anything crosses over: a question filed nowhere + // reaches no discipline, no organ system and no row on any analysis tab. + const [categories, setCategories] = useState([]) const load = useCallback(() => { api.get(`/drafts/batches/${batchId}`) @@ -31,6 +35,25 @@ export default function DraftBatch({ batchId, onChanged }) { useEffect(() => { load() }, [load]) + useEffect(() => { + api.get('/question-categories/') + .then(res => setCategories(Array.isArray(res.data) ? res.data : [])) + .catch(() => setCategories([])) + }, []) + + /** Names shown as a path, so "Croup" is not ambiguous between two parents. */ + const paths = useMemo(() => { + const byId = Object.fromEntries(categories.map(c => [c.id, c])) + const trail = (cat) => { + const parts = [] + let walk = cat, guard = 0 + while (walk && guard < 20) { parts.unshift(walk.name); walk = byId[walk.parent_id]; guard += 1 } + return parts.join(' › ') + } + return categories.map(c => ({ id: c.id, label: trail(c) })) + .sort((a, b) => a.label.localeCompare(b.label)) + }, [categories]) + const pending = useMemo( () => (batch?.drafts || []).filter(d => d.status === 'pending'), [batch]) const ready = useMemo(() => pending.filter(d => d.problems.length === 0), [pending]) @@ -92,6 +115,26 @@ export default function DraftBatch({ batchId, onChanged }) {
{error &&

{error}

} +
+ + {!batch.category_id && ( + + Every question from this batch is filed here when it crosses over. + Nothing can cross over until it is set. + + )} +
+
@@ -275,8 +266,6 @@ export default function CustomQuizPage() {
setOpenFacet('organ-systems')} /> setOpenFacet('systems')} /> - setOpenFacet('disciplines')} /> - setOpenFacet('symptoms')} /> setOpenFacet('articles')} /> setOpenFacet('saved')} /> @@ -416,21 +405,11 @@ export default function CustomQuizPage() { )} - setOpenFacet(null)} - onReset={() => setTagIds(ids => ids.filter(id => !subjectTags.some(t => t.id === id)))} - selected={subjectTags.filter(t => tagIds.includes(t.id)).map(t => ({ id: t.id, label: t.name }))} - onRemove={id => toggleTag(id, false)} - helper="By default, all disciplines are included unless filters are selected."> - {query => checkList(subjectTags, t => tagIds.includes(t.id), (t, on) => toggleTag(t.id, on), query)} - - - setOpenFacet(null)} - onReset={() => setTagIds(ids => ids.filter(id => !keywordTags.some(t => t.id === id)))} - selected={keywordTags.filter(t => tagIds.includes(t.id)).map(t => ({ id: t.id, label: t.name }))} - onRemove={id => toggleTag(id, false)} - helper="By default, all keywords are included unless filters are selected."> - {query => checkList(keywordTags, t => tagIds.includes(t.id), (t, on) => toggleTag(t.id, on), query)} - + {/* Disciplines and Symptoms & keywords were two more pickers here, over + the 203 subject tags and 4,281 keyword tags a model had written per + question. Both vocabularies were converted into Topics — 673 of them, + in a tree, which is the same axis said once and said better — and + have been retired. Topics is where a discipline is chosen. */} setOpenFacet(null)} onReset={() => setArticleIds([])} diff --git a/frontend/src/pages/CustomQuizPage.test.jsx b/frontend/src/pages/CustomQuizPage.test.jsx index f07f8ae..9bfad56 100644 --- a/frontend/src/pages/CustomQuizPage.test.jsx +++ b/frontend/src/pages/CustomQuizPage.test.jsx @@ -12,7 +12,7 @@ const categories = [ { id: 1, name: 'Pediatrics', question_count: 30, breadcrumbs: [{ id: 1, name: 'Pediatrics' }] }, { id: 2, name: 'Neonatal', question_count: 10, breadcrumbs: [{ id: 1, name: 'Pediatrics' }, { id: 2, name: 'Neonatal' }] }, ] -const TAGS = { subjects: [{ id: 7, name: 'Cardiology' }], keywords: [{ id: 8, name: 'Board Review 2019' }] } +const TAGS = { subjects: [], keywords: [], systems: [{ id: 7, name: 'Cardiovascular System', count: 255 }] } function setupCount(count = 30) { api.get.mockImplementation(url => { if (url === '/question-categories/') return Promise.resolve({ data: categories }) @@ -152,9 +152,9 @@ describe('CustomQuizPage', () => { await screen.findByText('30 questions available') await userEvent.type(screen.getByLabelText('Filter search'), 'cardio') - const hit = await screen.findByRole('checkbox', { name: /Cardiology/ }) + const hit = await screen.findByRole('checkbox', { name: /Cardiovascular System/ }) await userEvent.click(hit) - expect(screen.getByRole('button', { name: /^Disciplines/ })).toHaveTextContent('Cardiology') + expect(screen.getByRole('button', { name: /^Systems/ })).toHaveTextContent('Cardiovascular System') await userEvent.clear(screen.getByLabelText('Filter search')) await userEvent.type(screen.getByLabelText('Filter search'), 'zzzz') @@ -221,16 +221,16 @@ it('shows what you picked inside the picker, and lets you take one off', async ( return Promise.resolve({ data: { count: 30 } }) }) renderBuilder() - await userEvent.click(await screen.findByRole('button', { name: /Disciplines/ })) + await userEvent.click(await screen.findByRole('button', { name: /Systems/ })) - const panel = screen.getByRole('dialog', { name: 'Disciplines' }) - await userEvent.click(within(panel).getByRole('checkbox', { name: /Cardiology/ })) + const panel = screen.getByRole('dialog', { name: 'Systems' }) + await userEvent.click(within(panel).getByRole('checkbox', { name: /Cardiovascular System/ })) // The chip is the way back out. Hunting the checkbox down again in a list of // several hundred is not, and Reset throws away every other choice. - const chip = within(panel).getByRole('button', { name: 'Remove Cardiology' }) + const chip = within(panel).getByRole('button', { name: 'Remove Cardiovascular System' }) await userEvent.click(chip) - expect(within(panel).queryByRole('button', { name: 'Remove Cardiology' })).not.toBeInTheDocument() - expect(within(panel).getByRole('checkbox', { name: /Cardiology/ })).not.toBeChecked() + expect(within(panel).queryByRole('button', { name: 'Remove Cardiovascular System' })).not.toBeInTheDocument() + expect(within(panel).getByRole('checkbox', { name: /Cardiovascular System/ })).not.toBeChecked() }) diff --git a/frontend/src/pages/LandingPage.jsx b/frontend/src/pages/LandingPage.jsx index 723df08..190bbf5 100644 --- a/frontend/src/pages/LandingPage.jsx +++ b/frontend/src/pages/LandingPage.jsx @@ -183,6 +183,8 @@ function AuthModal({ mode, onClose, onSwitch }) { const [resendSent, setResendSent] = useState(false) const [resending, setResending] = useState(false) const [registered, setRegistered] = useState(false) + // Typed twice. The same field exists on /register; this is the other door. + const [confirm, setConfirm] = useState('') // Whether this site is invite only. The register page asks the same // question; this form is the other door and used not to ask at all, so // turning the gate on left everyone here failing with "an invite code is @@ -199,7 +201,7 @@ function AuthModal({ mode, onClose, onSwitch }) { }, []) const reset = () => { setError(''); setUnverified(false); setResendSent(false); setRegistered(false) } - const switchMode = (m) => { reset(); setName(''); setEmail(''); setPassword(''); onSwitch(m) } + const switchMode = (m) => { reset(); setName(''); setEmail(''); setPassword(''); setConfirm(''); onSwitch(m) } const handleLogin = async (e) => { e.preventDefault() @@ -218,6 +220,7 @@ function AuthModal({ mode, onClose, onSwitch }) { const handleRegister = async (e) => { e.preventDefault() setError('') + if (password !== confirm) { setError('The two passwords do not match.'); return } setLoading(true) try { const res = await api.post('/auth/register', { @@ -339,8 +342,19 @@ function AuthModal({ mode, onClose, onSwitch }) { setEmail(e.target.value)} required />
- - setPassword(e.target.value)} required minLength={8} /> + + setPassword(e.target.value)} /> +
+
+ + setConfirm(e.target.value)} /> + {confirm && password !== confirm && ( + + These do not match yet. + + )}
{inviteRequired && (
@@ -359,7 +373,8 @@ function AuthModal({ mode, onClose, onSwitch }) { )} diff --git a/frontend/src/pages/LoginTurnstile.test.jsx b/frontend/src/pages/LoginTurnstile.test.jsx index 9cc8f42..f8e8e37 100644 --- a/frontend/src/pages/LoginTurnstile.test.jsx +++ b/frontend/src/pages/LoginTurnstile.test.jsx @@ -56,6 +56,10 @@ it('keeps registration protected on the landing page', async () => { const signup = screen.getByRole('button', { name: 'Sign Up', exact: true }) expect(signup).toBeDisabled() act(() => window.turnstile.render.mock.calls[1][1].callback('synthetic-valid-token')) + // Still not enough: the password has to be typed twice and agree. + expect(signup).toBeDisabled() + await userEvent.type(screen.getByLabelText('Password'), 'longenough1') + await userEvent.type(screen.getByLabelText('Confirm password'), 'longenough1') expect(signup).toBeEnabled() }) diff --git a/frontend/src/pages/QuizPage.jsx b/frontend/src/pages/QuizPage.jsx index b4cefc8..85c70a1 100644 --- a/frontend/src/pages/QuizPage.jsx +++ b/frontend/src/pages/QuizPage.jsx @@ -1323,19 +1323,50 @@ const timerStarted = timeLeft !== null screen only made it ambiguous which one you were writing in. The global note still lives on the dashboard. */} {tool && setTool(null)} />} - {showReview && setShowReview(false)}> - {/* They are not counted as wrong: the score is out of what you - answered. But they are questions you did not do, and that is worth - saying before you hand it in. */} -

{answeredCount} of {totalCount} questions answered.{' '} - {answeredCount < totalCount - ? `${totalCount - answeredCount} will be left unanswered — they are not marked wrong, but they are not marked.` - : 'Everything is answered.'}

-
{questions.map((question, index) => )}
- - } + {showReview && (() => { + const missing = questions + .map((question, index) => ({ question, index })) + .filter(({ question }) => !answers[question.id]) + return ( + setShowReview(false)}> + {/* A confirmation with one button is not a confirmation. The way + back into the block is the first thing offered, and the count + is a sentence rather than a grid to be counted by eye. */} +

+ {missing.length === 0 + ? `All ${totalCount} questions are answered.` + : `${missing.length} of ${totalCount} question${totalCount === 1 ? '' : 's'} ` + + `${missing.length === 1 ? 'is' : 'are'} unanswered.`} +

+ {missing.length > 0 && ( + <> +

+ They are not marked wrong — the score is out of what you + answered — but they are not marked at all. +

+
+ {missing.map(({ question, index }) => ( + + ))} +
+ + )} +
+ + +
+ + ) + })()} {submitError &&
{submitError}
} {progressError &&
{progressError}
} {timeUp && ( diff --git a/frontend/src/pages/QuizPage.test.jsx b/frontend/src/pages/QuizPage.test.jsx index e443dfe..0ed9d34 100644 --- a/frontend/src/pages/QuizPage.test.jsx +++ b/frontend/src/pages/QuizPage.test.jsx @@ -461,13 +461,17 @@ describe('quiz player', () => { expect(screen.queryByText(/Full explanation/)).not.toBeInTheDocument() expect(api.get.mock.calls.some(([url]) => url.startsWith('/study-tools/attempts/'))).toBe(false) await userEvent.click(screen.getAllByRole('button', { name: 'Review & Complete' })[0]) - const review = screen.getByRole('dialog', { name: 'Review & Complete' }) - expect(within(review).getByText(/1 of 2 questions answered/)).toBeInTheDocument() - await userEvent.click(within(review).getByRole('button', { name: '2 · Unanswered' })) + const review = screen.getByRole('dialog', { name: 'End block?' }) + // The sentence names what is missing, not what is done — that is the + // decision being confirmed. + expect(within(review).getByText(/1 of 2 questions is unanswered/)).toBeInTheDocument() + // And there is a way back into the block, not only a way out of it. + expect(within(review).getByRole('button', { name: 'Go back and answer them' })).toBeInTheDocument() + await userEvent.click(within(review).getByRole('button', { name: '2' })) await findStem('Full second clinical question.') expect(api.post.mock.calls.some(([url]) => url === '/attempts/50/submit')).toBe(false) await userEvent.click(screen.getAllByRole('button', { name: 'Review & Complete' })[0]) - await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Complete test' })) + await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'End block' })) expect(await screen.findByText('Submitted results')).toBeInTheDocument() expect(api.post).toHaveBeenCalledWith('/attempts/50/submit', expect.objectContaining({ answers: [{ question_id: 1, user_answer: 'First answer' }], @@ -482,7 +486,7 @@ describe('quiz player', () => { expect(screen.getByRole('note')).toHaveTextContent('extrathoracic') fireEvent.keyDown(window, { key: '1' }) await userEvent.click(screen.getAllByRole('button', { name: 'Review & Complete' })[0]) - await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Complete test' })) + await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'End block' })) expect(await screen.findByText('Submitted results')).toBeInTheDocument() expect(api.post).toHaveBeenCalledWith('/attempts/50/submit', expect.objectContaining({ hints: [1], @@ -499,7 +503,7 @@ describe('quiz player', () => { fireEvent.keyDown(window, { key: '1' }) await userEvent.click(await screen.findByRole('button', { name: 'stridor' })) await userEvent.click(screen.getAllByRole('button', { name: 'Review & Complete' })[0]) - await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Complete test' })) + await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'End block' })) expect(await screen.findByText('Submitted results')).toBeInTheDocument() expect(api.post).toHaveBeenCalledWith('/attempts/50/submit', expect.objectContaining({ hints: [], @@ -576,7 +580,7 @@ describe('quiz player', () => { return originalPost(url, ...args) }) await userEvent.click(screen.getAllByRole('button', { name: 'Review & Complete' })[0]) - await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Complete test' })) + await userEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'End block' })) expect(await screen.findByRole('alert')).toHaveTextContent('Try again safely') expect(api.delete).not.toHaveBeenCalled() await userEvent.click(screen.getByRole('button', { name: 'Retry submission' })) diff --git a/frontend/src/pages/QuizPlayer.css b/frontend/src/pages/QuizPlayer.css index daaf119..92222ad 100644 --- a/frontend/src/pages/QuizPlayer.css +++ b/frontend/src/pages/QuizPlayer.css @@ -440,3 +440,19 @@ body:has(.quiz-player.is-boxed) .site-footer { display: none; } border-radius: 8px; } .quiz-drawer-menu a:hover { background: var(--bg); } + +/* ── Ending a block ─────────────────────────────────────────────────── + A confirmation with one button is not a confirmation. The way back into + the block is offered first, and the unanswered are numbers you can press + rather than a list to be read. */ +.quiz-end-lead { margin: 0 0 8px; font-size: 1rem; font-weight: 600; } +.quiz-end-note { margin: 0 0 14px; font-size: 0.86rem; line-height: 1.6; color: var(--text-muted); } +.quiz-end-missing { display: flex; flex-wrap: wrap; gap: 8px; margin-bottom: 18px; } +.quiz-end-missing button { + min-width: 42px; min-height: 42px; padding: 0 12px; + font: inherit; font-weight: 700; cursor: pointer; + background: var(--wrong-bg); color: var(--wrong-fg); + border: 1px solid var(--wrong-bd); border-radius: 9px; +} +.quiz-end-missing button:hover { border-color: var(--wrong-fg); } +.quiz-end-actions { display: flex; gap: 10px; flex-wrap: wrap; justify-content: flex-end; } diff --git a/frontend/src/pages/RegisterInvite.test.jsx b/frontend/src/pages/RegisterInvite.test.jsx index bb0614a..cf8f731 100644 --- a/frontend/src/pages/RegisterInvite.test.jsx +++ b/frontend/src/pages/RegisterInvite.test.jsx @@ -17,6 +17,8 @@ const fill = async () => { await userEvent.type(screen.getByLabelText('Name'), 'Ada') await userEvent.type(screen.getByLabelText('Email'), 'ada@example.test') await userEvent.type(screen.getByLabelText('Password'), 'longenough1') + // Typed twice, as a real person would. + await userEvent.type(screen.getByLabelText('Confirm password'), 'longenough1') } describe('registering when the site is invite only', () => { @@ -60,6 +62,7 @@ describe('registering when the site is invite only', () => { render() // No code asked for, rather than a form nobody can complete. await waitFor(() => expect(screen.queryByLabelText('Invite code')).not.toBeInTheDocument()) + await fill() expect(screen.getByRole('button', { name: /Sign Up/ })).toBeEnabled() }) }) diff --git a/frontend/src/pages/RegisterPage.jsx b/frontend/src/pages/RegisterPage.jsx index 6c2f837..d8fedca 100644 --- a/frontend/src/pages/RegisterPage.jsx +++ b/frontend/src/pages/RegisterPage.jsx @@ -33,6 +33,9 @@ export default function RegisterPage() { const [name, setName] = useState('') const [email, setEmail] = useState('') const [password, setPassword] = useState('') + // Typed twice, because a password you cannot see is a password you can + // mistype into an account you then cannot get into. + const [confirm, setConfirm] = useState('') const [error, setError] = useState('') const [loading, setLoading] = useState(false) const [done, setDone] = useState(false) @@ -54,6 +57,7 @@ export default function RegisterPage() { const handleSubmit = async (e) => { e.preventDefault() setError('') + if (password !== confirm) { setError('The two passwords do not match.'); return } setLoading(true) try { const res = await api.post('/auth/register', { @@ -119,6 +123,17 @@ export default function RegisterPage() { setPassword(e.target.value)} required minLength={8} />
+
+ + setConfirm(e.target.value)} /> + {confirm && password !== confirm && ( + + These do not match yet. + + )} +
{inviteRequired && (
@@ -136,7 +151,8 @@ export default function RegisterPage() { )} diff --git a/frontend/src/pages/ToolsPage.test.jsx b/frontend/src/pages/ToolsPage.test.jsx index 359ea30..5682f9f 100644 --- a/frontend/src/pages/ToolsPage.test.jsx +++ b/frontend/src/pages/ToolsPage.test.jsx @@ -20,7 +20,8 @@ const BATCHES = [ ] const BATCH = { - id: 1, title: 'Nelson ch. 12', status: 'open', counts: { pending: 2, accepted: 1 }, + id: 1, title: 'Nelson ch. 12', status: 'open', category_id: 4, + counts: { pending: 2, accepted: 1 }, drafts: [ { id: 11, position: 0, question_text: 'A neonate with cyanosis…', options: ['TGA', 'ASD'], correct_answer: 'TGA', explanation: 'Because.', status: 'pending', problems: [], question_id: null }, @@ -105,3 +106,22 @@ describe('the question workbench', () => { expect.objectContaining({ correct_answer: 'two' }))) }) }) + +it('will not let a batch cross over until it is filed somewhere', async () => { + api.get.mockImplementation(url => { + if (url === '/drafts/batches') return Promise.resolve({ data: BATCHES }) + if (url === '/drafts/batches/1') return Promise.resolve({ data: { ...BATCH, category_id: null } }) + if (url === '/question-categories/') return Promise.resolve({ data: [{ id: 4, name: 'Neonatology', parent_id: null }] }) + return Promise.resolve({ data: [] }) + }) + render() + await userEvent.click(await screen.findByRole('button', { name: /Nelson ch\. 12/ })) + await userEvent.click(await screen.findByRole('checkbox', { name: 'Select draft 1' })) + // A question filed nowhere reaches no discipline, no organ system and no row + // on any analysis tab, so it cannot leave until somebody says where it goes. + expect(screen.getByRole('button', { name: /Move 1 into the bank/ })).toBeDisabled() + + await userEvent.selectOptions(screen.getByRole('combobox'), '4') + await waitFor(() => expect(api.patch).toHaveBeenCalledWith( + '/drafts/batches/1', { category_id: 4 })) +})