diff --git a/backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py b/backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py new file mode 100644 index 0000000..d54e4cf --- /dev/null +++ b/backend/alembic/versions/b8c9d0e1f2a3_fix_question_quiz_fk.py @@ -0,0 +1,38 @@ +"""Deleting a quiz must not destroy the questions extracted for it. + +`questions.quiz_id` was created ON DELETE CASCADE, while the model has long +declared SET NULL and its comment says the column is informational — "which +quiz this question was originally extracted for" — with real membership held +in `quiz_question_links`. The database was the one being obeyed, so removing a +quiz silently erased every question it had produced, along with their attempts, +exam membership, media and article links, and straight past the trash that was +built to make exactly that impossible. + +The model's intent is the correct one and is what this applies. + +`quizzes.user_id` is the opposite case: the database cascades and the model +said nothing, and here the database is right — deleting an account is +documented as removing what it made. The model is corrected to match rather +than the data. + +Revision ID: b8c9d0e1f2a3 +Revises: a7b8c9d0e1f2 +""" +from alembic import op + +revision = "b8c9d0e1f2a3" +down_revision = "a7b8c9d0e1f2" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.drop_constraint("questions_quiz_id_fkey", "questions", type_="foreignkey") + op.create_foreign_key("questions_quiz_id_fkey", "questions", "quizzes", + ["quiz_id"], ["id"], ondelete="SET NULL") + + +def downgrade() -> None: + op.drop_constraint("questions_quiz_id_fkey", "questions", type_="foreignkey") + op.create_foreign_key("questions_quiz_id_fkey", "questions", "quizzes", + ["quiz_id"], ["id"], ondelete="CASCADE") diff --git a/backend/app/models/quiz.py b/backend/app/models/quiz.py index d027bf8..49a54dc 100644 --- a/backend/app/models/quiz.py +++ b/backend/app/models/quiz.py @@ -18,7 +18,10 @@ class Quiz(Base): id = Column(Integer, primary_key=True, index=True) section_id = Column(Integer, ForeignKey("sections.id", ondelete="SET NULL"), nullable=True) - user_id = Column(Integer, ForeignKey("users.id"), nullable=False) + # Cascades: deleting an account is documented as removing what it made. + # Declared here because the database has always done it, and a model that + # says nothing is a model that will be believed. + user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False) category_id = Column(Integer, ForeignKey("quiz_categories.id", ondelete="SET NULL"), nullable=True) title = Column(String, nullable=False) questions_count = Column(Integer, default=0) diff --git a/backend/app/routers/articles.py b/backend/app/routers/articles.py index cedd2a8..5f0a647 100644 --- a/backend/app/routers/articles.py +++ b/backend/app/routers/articles.py @@ -183,6 +183,34 @@ def _views_for(db, user) -> list[str]: return views_for(db.get(Exam, exam_id) if exam_id else None) +def _article_card_json(article: Article) -> dict: + """An article as it appears in a list: enough to find it, not to read it. + + The listing used to send `content` and `sections` for every article — 214KB + of prose across the library, none of which a list renders. A browser that + waits for the whole corpus before it can draw a column of titles is slow + for no reason. + """ + sections = article.sections or [] + return { + "id": article.id, + "slug": article.slug, + "title": article.title, + "summary": article.summary, + "category_id": article.category_id, + "section_id": article.section_id, + "user_id": article.user_id, + "status": article.status, + "section_count": len(sections), + "variants": article_service.available_variants(article), + "generated_by": article.generated_by, + "reviewed_at": article.reviewed_at, + "submitted_at": article.submitted_at, + "created_at": article.created_at, + "updated_at": article.updated_at, + } + + def _article_json(article: Article) -> dict: return { "id": article.id, @@ -293,7 +321,7 @@ def list_articles( articles.sort(key=lambda article: rank_of.get(article.id, len(rank_of))) if not current_user.is_moderator: articles = [a for a in articles if a.status == "published"] - return [_article_json(a) for a in articles] + return [_article_card_json(a) for a in articles] @router.post("/") diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index 40a70cb..4a88f9d 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -58,15 +58,19 @@ function AppLayout() { // Keyed by path so navigating away from a broken page clears the error. const location = useLocation() return ( - <> + /* A column the height of the window, so the footer is at the bottom of the + screen rather than at the bottom of the content — a page still loading + is a spinner, and the footer used to sit halfway up with the page + background showing beneath it. */ +
-
+
- +
) } diff --git a/frontend/src/components/ArticleLink.css b/frontend/src/components/ArticleLink.css index 32c9c9d..6dfa1d3 100644 --- a/frontend/src/components/ArticleLink.css +++ b/frontend/src/components/ArticleLink.css @@ -25,17 +25,31 @@ font-size: 0.84rem; line-height: 1.5; text-align: left; - /* The card is a hint before it is a target: its body stays transparent to the - pointer so it never sits between the reader and the link it describes, and - only the controls below take clicks. */ - pointer-events: none; + /* The card must hold the pointer. It used to be transparent to it, on the + idea that a hint should never sit between the reader and the link — but + the card is offset below the link and never covered it, while the pointer + travelling down to Split view crossed a body it could not enter, so no + mouseenter fired and the hide timer closed the card on the way. */ + pointer-events: auto; } + +/* Bridges the 8px gap between the link and the card, so crossing it never + leaves the wrapper at all and the grace period is never needed. */ +.al-card::before { + content: ''; + position: absolute; + left: 0; + right: 0; + top: -9px; + height: 9px; +} +.al-card.is-above::before { top: auto; bottom: -9px; } .al-card.is-above { top: auto; bottom: calc(100% + 8px); } .al-card-title { font-weight: 700; font-size: 0.92rem; color: var(--text); } .al-card-excerpt { color: var(--text-muted); } .al-card-meta { font-size: 0.75rem; color: var(--text-subtle); } -.al-card-actions { display: flex; gap: 6px; margin-top: 2px; pointer-events: auto; } +.al-card-actions { display: flex; gap: 6px; margin-top: 2px; } .al-card-action { display: inline-flex; align-items: center; @@ -55,7 +69,8 @@ .al-card-action:hover { border-color: var(--primary); color: var(--primary); } .al-card-action:focus-visible { outline: 2px solid var(--primary); outline-offset: 1px; } -/* No hover on touch, so the card never appears there and the link is just a link. */ +/* Touch has no hover, so the card arrives on the tap that would have followed + the link. It is the only route to the article there, so it must be shown. */ @media (hover: none) { - .al-card { display: none; } + .al-card { width: min(340px, calc(100vw - 32px)); } } diff --git a/frontend/src/components/ArticleLink.jsx b/frontend/src/components/ArticleLink.jsx index b920d55..c8b9cfa 100644 --- a/frontend/src/components/ArticleLink.jsx +++ b/frontend/src/components/ArticleLink.jsx @@ -31,8 +31,9 @@ export function fetchPreview(slug) { * couple of sentences, how much is there, and — where the page can hold one — * the offer to open it beside what you are reading rather than in place of it. * - * Touch has no hover, so on a phone this is just a link — a card that appears on - * tap would sit between the finger and the thing it was about to open. + * Touch has no hover, so on a phone the tap that would have followed the link + * opens the card instead — which is the same journey a pointer makes, minus + * the hover it does not have. */ /** The numeric id behind a slug, reusing whatever the hover card already fetched. */ export const resolveArticleId = (slug) => fetchPreview(slug).then(data => data?.id ?? null) @@ -80,20 +81,37 @@ export default function ArticleLink({ slug, children, className = '' }) { timer.current = setTimeout(() => setOpen(false), HIDE_DELAY) }, []) + // Reaching the card is arriving, not leaving: whatever the pointer did on + // the way, it is here now. + const hideCancel = useCallback(() => clearTimeout(timer.current), []) + const hideNow = useCallback(() => { clearTimeout(timer.current) setOpen(false) }, []) + /** + * Clicking the words opens the card, not the article. + * + * A cross-reference is read mid-sentence, and navigating away to find out + * whether it was worth it is the thing that breaks the thread. So the link + * itself shows the card, and the card's two controls — beside what you are + * reading, or in a tab for later — are how you actually go. Nothing here + * takes you off the page you are on by accident. + * + * Modified and middle clicks are left to the browser, so ctrl-click, ⌘-click + * and "open in new tab" behave as they do on any other link. + */ const followLink = (event) => { - hideNow() - // Inside the second pane a cross-reference stays in that pane: the article - // you started from is on the left and should still be there afterwards. - // Modified and middle clicks are left alone so they still open a tab. - if (!split?.inPane || event.button !== 0) return + if (event.button !== 0) return if (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) return event.preventDefault() - split.open(slug) + clearTimeout(timer.current) + if (open) { + setOpen(false) + return + } + reveal() } return ( @@ -104,7 +122,8 @@ export default function ArticleLink({ slug, children, className = '' }) { {children} {open && preview && ( - + {preview.title} {preview.excerpt && {preview.excerpt}} diff --git a/frontend/src/components/ArticleLink.test.jsx b/frontend/src/components/ArticleLink.test.jsx index 11212e7..f7cfff8 100644 --- a/frontend/src/components/ArticleLink.test.jsx +++ b/frontend/src/components/ArticleLink.test.jsx @@ -151,18 +151,45 @@ describe('reading a cross-reference without leaving the article', () => { expect(within(card).getByRole('link', { name: /new tab/i })).toHaveFocus() }) - it('leaves an ordinary click alone: the link is still a link', async () => { + it('opens the card rather than the article, so reading is not interrupted', async () => { const open = vi.fn() mountBeside({ open, inPane: false }, 'plain-click') await userEvent.click(linkNamed()) - expect(await screen.findByText('Whole article')).toBeInTheDocument() + // The card is the answer to "is this worth following?", and the two + // controls on it are how you go. Nothing navigates by accident. + expect(await screen.findByRole('tooltip')).toBeInTheDocument() + expect(screen.queryByText('Whole article')).not.toBeInTheDocument() expect(open).not.toHaveBeenCalled() }) - it('keeps a link followed inside the pane inside the pane', async () => { + it('clicking the words again puts the card away', async () => { + mountBeside({ open: vi.fn(), inPane: false }, 'toggle-me') + await userEvent.click(linkNamed()) + await screen.findByRole('tooltip') + await userEvent.click(linkNamed()) + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() + }) + + it('leaves a modified click to the browser, as on any other link', async () => { + const open = vi.fn() + mountBeside({ open, inPane: true }, 'deeper-topic') + // ⌘-click, ctrl-click and "open in new tab" are the browser's to handle; + // this component does not intercept them. + await userEvent.keyboard('{Meta>}') + await userEvent.click(linkNamed()) + await userEvent.keyboard('{/Meta}') + // Not intercepted: the split view is not opened and the page is not + // changed, leaving the browser to do what it does with a modified click. + expect(open).not.toHaveBeenCalled() + expect(screen.queryByText('Whole article')).not.toBeInTheDocument() + }) + + it('goes to the pane from the card, even inside a pane', async () => { const open = vi.fn() mountBeside({ open, inPane: true }, 'deeper-topic') await userEvent.click(linkNamed()) + const card = await screen.findByRole('tooltip') + await userEvent.click(within(card).getByRole('button', { name: /split view/i })) expect(open).toHaveBeenCalledWith('deeper-topic') expect(screen.queryByText('Whole article')).not.toBeInTheDocument() }) diff --git a/frontend/src/components/CategoryColumns.jsx b/frontend/src/components/CategoryColumns.jsx index 10b7e59..70de05f 100644 --- a/frontend/src/components/CategoryColumns.jsx +++ b/frontend/src/components/CategoryColumns.jsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import './CategoryColumns.css' /** A folder of further topics. */ @@ -45,35 +45,47 @@ export default function CategoryColumns({ return map }, [categories]) + const articlesIn = useCallback( + (categoryId) => (articlesOf ? articlesOf(categoryId) : []), [articlesOf]) + + /** + * How much is behind each folder, counting the thing this browser opens. + * + * It counted questions while listing articles, so "Hyperinflammatory Sepsis + * 4" meant four questions and opened onto no reading at all. A number beside + * a folder is a promise about what is inside it. + */ const rollup = useMemo(() => { const totals = {} const walk = (cat) => { - let sum = cat.question_count || 0 + let sum = articlesOf ? articlesIn(cat.id).length : (cat.question_count || 0) for (const child of childrenOf[cat.id] || []) sum += walk(child) totals[cat.id] = sum return sum } for (const root of childrenOf[0] || []) walk(root) return totals - }, [childrenOf]) + }, [childrenOf, articlesOf, articlesIn]) const byId = useMemo(() => Object.fromEntries(categories.map(c => [c.id, c])), [categories]) - const articlesIn = (categoryId) => (articlesOf ? articlesOf(categoryId) : []) - // One column per opened level: the roots, then each opened parent's contents. // Anything filed nowhere is listed in the root column rather than being // unreachable because it has no heading to sit under. - const columns = [{ parentId: 0, items: childrenOf[0] || [], articles: articlesIn(null) }] + // Only folders with something behind them, once the subtree is counted — + // an empty branch is a row that promises reading and opens onto nothing. + const stocked = (list) => list.filter(cat => (rollup[cat.id] ?? 0) > 0) + + const columns = [{ parentId: 0, items: stocked(childrenOf[0] || []), articles: articlesIn(null) }] for (const id of path) { - const kids = childrenOf[id] || [] + const kids = stocked(childrenOf[id] || []) const articles = articlesIn(id) if (!kids.length && !articles.length) break columns.push({ parentId: id, items: kids, articles }) } const openAt = (level, cat) => { - const hasContents = (childrenOf[cat.id] || []).length > 0 || articlesIn(cat.id).length > 0 + const hasContents = stocked(childrenOf[cat.id] || []).length > 0 || articlesIn(cat.id).length > 0 setPath(hasContents ? [...path.slice(0, level), cat.id] : path.slice(0, level)) onSelect(cat.id) } @@ -96,7 +108,7 @@ export default function CategoryColumns({ {cat.name} {rollup[cat.id] ?? cat.question_count} - {(kids.length > 0 || articlesIn(cat.id).length > 0) && ( + {(stocked(kids).length > 0 || articlesIn(cat.id).length > 0) && ( )} diff --git a/frontend/src/index.css b/frontend/src/index.css index d848659..37bf012 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -119,6 +119,13 @@ html, body { overflow-x: hidden; max-width: 100%; } /* ── Layout ─────────────────────────────────────────────────── */ .container { max-width: 1200px; margin: 0 auto; padding: 0 28px; } +/* The footer belongs at the bottom of the window, not wherever the content + happens to stop. A loading page is a spinner, and without this the footer + sat mid-screen with the page background below it. */ +.app-shell { display: flex; flex-direction: column; min-height: 100dvh; } +.app-main { flex: 1 0 auto; width: 100%; } +.app-shell > .site-footer { flex: none; } + /* ── Navbar ─────────────────────────────────────────────────── */ /* Two bars. The primary one — identity, search, account — never moves. The section bar leaves while you scroll down and returns on the way up: it is diff --git a/frontend/src/pages/ArticleSplitView.test.jsx b/frontend/src/pages/ArticleSplitView.test.jsx index 9ea4a35..897c452 100644 --- a/frontend/src/pages/ArticleSplitView.test.jsx +++ b/frontend/src/pages/ArticleSplitView.test.jsx @@ -104,7 +104,10 @@ describe('reading a cross-reference beside the article', () => { await openSplit('meningitis') const pane = await screen.findByRole('region', { name: 'Split view: Meningitis' }) + // Clicking the words opens the card; the card's control is what goes. await userEvent.click(within(pane).getByRole('link', { name: 'sepsis' })) + const card = await screen.findByRole('tooltip') + await userEvent.click(within(card).getByRole('button', { name: /split view/i })) await waitFor(() => expect(api.get).toHaveBeenCalledWith('/articles/9')) expect(await screen.findByRole('region', { name: 'Split view: Sepsis' })).toBeInTheDocument() expect(document.querySelectorAll('.article-split-pane')).toHaveLength(1) diff --git a/frontend/src/pages/ArticlesPage.jsx b/frontend/src/pages/ArticlesPage.jsx index aa380a4..6f06098 100644 --- a/frontend/src/pages/ArticlesPage.jsx +++ b/frontend/src/pages/ArticlesPage.jsx @@ -141,7 +141,7 @@ export default function ArticlesPage() {

{article.title}

{article.summary &&

{article.summary}

} - {article.sections?.length || 0} sections + {article.section_count ?? 0} sections ))}
diff --git a/frontend/src/pages/ArticlesPage.test.jsx b/frontend/src/pages/ArticlesPage.test.jsx index f96a962..8a1a14f 100644 --- a/frontend/src/pages/ArticlesPage.test.jsx +++ b/frontend/src/pages/ArticlesPage.test.jsx @@ -61,15 +61,39 @@ describe('topic reading', () => { expect(screen.getByText('Draft')).toBeInTheDocument() expect(screen.queryByRole('button', { name: /Seizures/ })).not.toBeInTheDocument() + // Cardiology has seven questions and no reading. This browser opens + // articles, so it is not offered: a folder here promises something to read. + expect(screen.queryByRole('button', { name: /Cardiology/ })).not.toBeInTheDocument() + // Opening a topic opens its contents in the next column; the trail stays. await userEvent.click(screen.getByRole('button', { name: /Neurology/ })) expect(screen.getByRole('button', { name: /Seizures/ })).toBeInTheDocument() - expect(screen.getByRole('button', { name: /Cardiology/ })).toBeInTheDocument() await userEvent.click(screen.getByRole('button', { name: /Seizures/ })) expect(screen.getByRole('button', { name: /Febrile seizures/ })).toBeInTheDocument() }) + it('counts the reading behind a topic, not its questions', async () => { + const cats = [ + { id: 2, name: 'Neurology', parent_id: null, question_count: 10 }, + { id: 3, name: 'Seizures', parent_id: 2, question_count: 4 }, + ] + // Two articles, both filed three levels in. The old count showed the + // question total, so "Neurology 10" opened onto two pieces of reading. + const filed = { ...article, category_id: 3 } + const second = { id: 12, title: 'Status epilepticus', slug: 'status', status: 'published', category_id: 3, sections: [] } + api.get.mockImplementation(url => { + if (url === '/question-categories/') return Promise.resolve({ data: cats }) + if (url === '/articles/') return Promise.resolve({ data: [filed, second] }) + return Promise.resolve({ data: [] }) + }) + render(} />) + + const neurology = await screen.findByRole('button', { name: /Neurology/ }) + expect(neurology).toHaveTextContent('2') + expect(neurology).not.toHaveTextContent('10') + }) + it('searching answers with the matches, not the shelf they sit on', async () => { render(} />) await screen.findByText('Febrile seizures')