diff --git a/backend/alembic/versions/c5d6e7f8091a_collectible_articles.py b/backend/alembic/versions/c5d6e7f8091a_collectible_articles.py new file mode 100644 index 0000000..a9622f3 --- /dev/null +++ b/backend/alembic/versions/c5d6e7f8091a_collectible_articles.py @@ -0,0 +1,44 @@ +"""A library can hold articles as well as questions. + +Collections were questions only, so the reading side of the site had no way to +put anything aside — the bookmark on an article had nowhere to write. Its own +table rather than a nullable column beside `question_id`, so a row cannot claim +to be both kinds or neither. + +Revision ID: c5d6e7f8091a +Revises: b4c5d6e7f809 +""" +import sqlalchemy as sa +from alembic import op + +revision = "c5d6e7f8091a" +down_revision = "b4c5d6e7f809" +branch_labels = None +depends_on = None + + +def upgrade(): + # `Base.metadata.create_all()` still runs at startup and creates missing + # tables, so on a box that has already booted this code the table is here + # before the migration is. Checked rather than assumed, or the upgrade + # fails on exactly the machines that are up to date. + if "user_collection_articles" in sa.inspect(op.get_bind()).get_table_names(): + return + op.create_table( + "user_collection_articles", + sa.Column("id", sa.Integer(), primary_key=True, index=True), + sa.Column("collection_id", sa.Integer(), + sa.ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False), + sa.Column("article_id", sa.Integer(), + sa.ForeignKey("articles.id", ondelete="CASCADE"), nullable=False), + sa.UniqueConstraint("collection_id", "article_id", name="uq_collection_article"), + ) + op.create_index("ix_user_collection_articles_collection", + "user_collection_articles", ["collection_id"]) + + +def downgrade(): + if "user_collection_articles" not in sa.inspect(op.get_bind()).get_table_names(): + return + op.drop_index("ix_user_collection_articles_collection", table_name="user_collection_articles") + op.drop_table("user_collection_articles") diff --git a/backend/app/models/collection.py b/backend/app/models/collection.py index 9400a3c..54423b5 100644 --- a/backend/app/models/collection.py +++ b/backend/app/models/collection.py @@ -23,3 +23,20 @@ class UserCollectionQuestion(Base): id = Column(Integer, primary_key=True, index=True) collection_id = Column(Integer, ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False) question_id = Column(Integer, ForeignKey("questions.id", ondelete="CASCADE"), nullable=False) + + +class UserCollectionArticle(Base): + """An article put aside into a library. + + Its own table rather than a nullable `article_id` beside `question_id` on + the row above: that shape allows a row with both, or with neither, and + every read then has to say which kind it is looking at. Two tables, one + unique constraint each, and a library is the union of them. + """ + + __tablename__ = "user_collection_articles" + __table_args__ = (UniqueConstraint("collection_id", "article_id", name="uq_collection_article"),) + + id = Column(Integer, primary_key=True, index=True) + collection_id = Column(Integer, ForeignKey("user_collections.id", ondelete="CASCADE"), nullable=False) + article_id = Column(Integer, ForeignKey("articles.id", ondelete="CASCADE"), nullable=False) diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index 41ca78b..0a9ffe1 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -212,13 +212,22 @@ def search_litellm_models( log.warning(f"LiteLLM model search failed: {e}") raise HTTPException(status_code=400, detail=f"Failed to query models API: {e}") + # No proxy configured, so all that is left is whatever the SDK can name by + # itself. It only lists providers whose own API keys are in the + # environment, and this deployment has none — everything goes through the + # proxy above. An empty list is therefore the normal answer here, and + # returning it plainly would read as "this site has no models". try: import litellm models = sorted(litellm.utils.get_valid_models()) - return {"models": models, "source": "litellm-builtin"} except Exception as e: log.warning(f"LiteLLM builtin model list failed: {e}") raise HTTPException(status_code=500, detail="Failed to retrieve LiteLLM built-in model list.") + if not models: + raise HTTPException( + status_code=400, + detail="No model endpoint is configured. Set the API base to your LLM proxy and try again.") + return {"models": models, "source": "litellm-builtin"} @router.get("/models", response_model=list[AIModelConfigResponse]) diff --git a/backend/app/routers/collections.py b/backend/app/routers/collections.py index e62ba02..0979acc 100644 --- a/backend/app/routers/collections.py +++ b/backend/app/routers/collections.py @@ -1,4 +1,10 @@ -"""Personal question libraries (saved questions).""" +"""Personal libraries: questions and articles somebody has put aside. + +A library holds both. It used to hold only questions, so the reading side of +the site had a bookmark with nowhere to write — and the two kinds are the same +act to whoever is doing it, which is why they share a library rather than each +getting their own list. +""" from datetime import datetime from fastapi import APIRouter, Depends, HTTPException @@ -6,9 +12,12 @@ from pydantic import BaseModel, field_validator from sqlalchemy.orm import Session from app.database import get_db -from app.models.collection import UserCollection, UserCollectionQuestion +from app.models.article import Article +from app.models.collection import (UserCollection, UserCollectionArticle, + UserCollectionQuestion) from app.models.question import Question from app.models.user import User +from app.services.article_service import readable_articles from app.services.quiz_builder import bank_question_predicate from app.utils.auth import get_current_user @@ -42,6 +51,8 @@ def _as_json(db, collection) -> dict: "title": collection.title, "question_count": db.query(UserCollectionQuestion).filter( UserCollectionQuestion.collection_id == collection.id).count(), + "article_count": db.query(UserCollectionArticle).filter( + UserCollectionArticle.collection_id == collection.id).count(), "created_at": collection.created_at, "last_used_at": collection.last_used_at, # Every library is one person's. Said plainly rather than assumed, @@ -130,3 +141,60 @@ def remove_collection_question(collection_id: int, question_id: int, db: Session synchronize_session=False) _touch(db, collection) db.commit() + + +@router.get("/{collection_id}/articles") +def collection_articles(collection_id: int, db: Session = Depends(get_db), + user: User = Depends(get_current_user)): + collection = _own(db, user, collection_id) + rows = (db.query(Article) + .join(UserCollectionArticle, UserCollectionArticle.article_id == Article.id) + .filter(UserCollectionArticle.collection_id == collection.id) + .order_by(Article.title) + .all()) + _touch(db, collection) + db.commit() + # An article saved and later unpublished stays in the library and says so, + # rather than vanishing from a list the learner built themselves. + return [{"id": a.id, "title": a.title, "slug": a.slug, "status": a.status} for a in rows] + + +@router.put("/{collection_id}/articles/{article_id}") +def add_collection_article(collection_id: int, article_id: int, db: Session = Depends(get_db), + user: User = Depends(get_current_user)): + collection = _own(db, user, collection_id) + if not readable_articles(db, user).filter(Article.id == article_id).first(): + raise HTTPException(404, "Article not found") + if db.query(UserCollectionArticle.id).filter_by( + collection_id=collection.id, article_id=article_id).first(): + return {"added": False} + db.add(UserCollectionArticle(collection_id=collection.id, article_id=article_id)) + _touch(db, collection) + db.commit() + return {"added": True} + + +@router.delete("/{collection_id}/articles/{article_id}", status_code=204) +def remove_collection_article(collection_id: int, article_id: int, db: Session = Depends(get_db), + user: User = Depends(get_current_user)): + collection = _own(db, user, collection_id) + db.query(UserCollectionArticle).filter_by( + collection_id=collection_id, article_id=article_id).delete(synchronize_session=False) + _touch(db, collection) + db.commit() + + +@router.get("/for-article/{article_id}") +def libraries_holding_article(article_id: int, db: Session = Depends(get_db), + user: User = Depends(get_current_user)): + """Which of this learner's libraries already hold this article. + + The reader needs it to draw the bookmark filled or hollow. Asked as one + question rather than by fetching every library's contents, which would also + stamp each of them as used and reorder the learner's own list. + """ + held = {row.collection_id for row in db.query(UserCollectionArticle.collection_id) + .join(UserCollection, UserCollection.id == UserCollectionArticle.collection_id) + .filter(UserCollection.user_id == user.id, + UserCollectionArticle.article_id == article_id).all()} + return {"collection_ids": sorted(held)} diff --git a/backend/app/services/article_service.py b/backend/app/services/article_service.py index 8936ce2..6b139c6 100644 --- a/backend/app/services/article_service.py +++ b/backend/app/services/article_service.py @@ -171,3 +171,18 @@ def set_status(db: Session, article: Article, status: str, user_id: int | None) article.reviewed_at = now article.reviewed_by = user_id article.status = status + + +def readable_articles(db, user): + """A query over the articles this person may read. + + Published, plus their own drafts, plus everything if they moderate. The + rule was written out inline at every place that needed it, which is how a + draft ends up reachable from one route and not another. + """ + from app.models.article import Article + + query = db.query(Article) + if getattr(user, "is_moderator", False): + return query + return query.filter((Article.status == "published") | (Article.user_id == user.id)) diff --git a/backend/requirements.txt b/backend/requirements.txt index a4ac010..3fa5366 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -9,7 +9,19 @@ python-multipart==0.0.9 pydantic[email]==2.6.1 pydantic-settings==2.1.0 PyMuPDF==1.23.22 -litellm==1.28.13 +# Thumbnails. It arrives transitively today, which is not a reason to let a +# feature depend on somebody else's dependency tree. +Pillow==12.3.0 +# 1.28.13 was withdrawn from PyPI, so this file could not be edited at all +# without the whole pip layer failing to rebuild. 1.53.1 is the nearest version +# still published; the three things we use — completion, acompletion and +# utils.get_valid_models — are unchanged in it. +# +# We use the SDK only as an OpenAI-compatible HTTP client pointed at the proxy +# in LITELLM_API_BASE: every call sets api_base and prefixes the model with +# `openai/`. Embeddings already go over plain httpx. There is a smaller +# dependency in here waiting to be taken. +litellm==1.53.1 chromadb==0.4.24 celery[redis]==5.3.6 redis==5.0.1 diff --git a/frontend/src/components/ArticleEditor.jsx b/frontend/src/components/ArticleEditor.jsx index f75262a..957c54b 100644 --- a/frontend/src/components/ArticleEditor.jsx +++ b/frontend/src/components/ArticleEditor.jsx @@ -3,7 +3,7 @@ import RichEditor from './RichEditor' import './ArticleEditor.css' const VIEWS = [ - { key: 'short', label: 'Short', hint: 'Bullets a learner could revise from the night before an exam.' }, + { key: 'short', label: 'Summary', hint: 'Bullets a learner could revise from the night before an exam.' }, { key: 'long', label: 'Long', hint: 'The full article: definition through management.' }, { key: 'clinical', label: 'Clinical', hint: 'What to do at the bedside, with doses and routes.' }, ] diff --git a/frontend/src/components/ArticleEditor.test.jsx b/frontend/src/components/ArticleEditor.test.jsx index 5ea12e1..69b809c 100644 --- a/frontend/src/components/ArticleEditor.test.jsx +++ b/frontend/src/components/ArticleEditor.test.jsx @@ -41,14 +41,14 @@ describe('editing an article', () => { expect(screen.queryByDisplayValue('In short')).not.toBeInTheDocument() expect(screen.queryByDisplayValue('Management')).not.toBeInTheDocument() - await openView('Short') + await openView('Summary') expect(screen.getByDisplayValue('In short')).toBeInTheDocument() expect(screen.queryByDisplayValue('Definition')).not.toBeInTheDocument() }) it('counts what each view holds, so an empty one is visible before you open it', async () => { mount() - expect(within(screen.getByRole('tab', { name: /^Short/ })).getByText('1')).toBeInTheDocument() + expect(within(screen.getByRole('tab', { name: /^Summary/ })).getByText('1')).toBeInTheDocument() expect(within(screen.getByRole('tab', { name: /^Long/ })).getByText('2')).toBeInTheDocument() }) diff --git a/frontend/src/components/ArticleReader.jsx b/frontend/src/components/ArticleReader.jsx index 0aef7d6..272d225 100644 --- a/frontend/src/components/ArticleReader.jsx +++ b/frontend/src/components/ArticleReader.jsx @@ -31,15 +31,17 @@ export function Markdown({ children, attemptId }) { * links and the pane keeps it to itself; everything else — what is open, the * rail, the mobile drawer — belongs to this reader alone. */ -// One topic, three readings. Short is what you revise from, long is what you -// study from, clinical is what you act from at the bedside. They are views of -// one article rather than three articles, so the numbers cannot drift apart and -// a question linked to the topic still means one thing. +// One topic, three readings. The summary is what you revise from, long is what +// you study from, clinical is what you act from at the bedside. They are views +// of one article rather than three articles, so the numbers cannot drift apart +// and a question linked to the topic still means one thing. // -// Short leads because it is the quickest way to tell whether this is the article -// you wanted; the full text is one click away. +// `short` is the stored variant and stays that way — renaming it would be a +// data migration to change a word on a button. What a reader sees is "Summary", +// because that is what the section itself is called. +const SUMMARY = 'short' const VIEWS = [ - { key: 'short', label: 'Short' }, + { key: SUMMARY, label: 'Summary' }, { key: 'long', label: 'Long' }, { key: 'clinical', label: 'Clinical' }, ] @@ -244,7 +246,7 @@ export default function ArticleReader({
{/* A view with one section does not need a heading over it, or a control to collapse the only thing there is. The tab already - names it — "Short" then a heading reading "In short" says the + names it — "Summary" then a heading reading "In short" says the same word twice and hides the content behind a chevron. */} {soleSection ? (
@@ -292,16 +294,39 @@ export default function ArticleReader({ * it — and the rail is filtered by the same choice, so a reader who cannot * see the switch cannot explain why half the contents just went away. */ + const choose = (key) => { setView(key); setOpenIds({}); setReading('') } + const hasSummary = present.some(option => option.key === SUMMARY) + const full = present.filter(option => option.key !== SUMMARY) + // Where the reader goes when the summary is switched off: the fullest reading + // there is, which is Long unless the article only has the clinical view. + const fallback = full[0]?.key + const viewSwitch = present.length > 1 && ( -
- {present.map(option => ( - - ))} + )} + {full.length > 1 && ( +
+ {full.map(option => ( + + ))} +
+ )}
) diff --git a/frontend/src/components/ArticleSaveButton.jsx b/frontend/src/components/ArticleSaveButton.jsx index c8cad8d..ef368b3 100644 --- a/frontend/src/components/ArticleSaveButton.jsx +++ b/frontend/src/components/ArticleSaveButton.jsx @@ -1,43 +1,39 @@ -import { useCallback, useEffect, useRef, useState } from 'react' +import { useEffect, useRef, useState } from 'react' import api from '../api/client' import './ArticleSaveButton.css' /** - * Keep this topic. + * Keep this article. * - * Libraries hold questions — `user_collection_questions` has a question_id and - * nothing else — so saving a topic saves the questions written against it, - * which is what you would want it for: the reading and the practice on one - * subject, filed together, ready to sit later. Nothing here invents a second - * place to save things; it is the same endpoint the session player writes to - * when you file a question mid-attempt. + * A library holds both articles and questions, because putting one of each + * aside is the same act to whoever is doing it. The article goes in as itself + * rather than standing in for the questions written against it — a topic with + * no questions is still worth keeping, and a reader who saved the reading did + * not ask for a session. * - * The consequence is that the server cannot be asked "is this article saved?", - * only "is this question in that library?", and answering it properly would - * mean fetching every library's contents on every article — which also stamps - * each one as used and reorders the reader's own list. So the answer is kept - * on the device that gave it. It is honest about a save that happened; it will - * not know about one made on another machine. An `article_id` on the join - * table would settle it, and is the right fix when the API can change. + * Which libraries already hold it is asked of the server, as one question. It + * was kept on the device for a while, because the API could not answer; that + * was wrong on the second machine and silently so. */ -const SAVED_KEY = 'pedshub.articleLibraries' - -const readSaved = () => { - try { return JSON.parse(localStorage.getItem(SAVED_KEY) || '{}') } catch { return {} } -} -const writeSaved = (next) => { - try { localStorage.setItem(SAVED_KEY, JSON.stringify(next)) } catch { /* the visit still knows */ } -} - -export default function ArticleSaveButton({ articleId, questionIds = [] }) { +export default function ArticleSaveButton({ articleId }) { const [open, setOpen] = useState(false) const [libraries, setLibraries] = useState([]) const [query, setQuery] = useState('') const [busy, setBusy] = useState(false) - const [saved, setSaved] = useState(() => readSaved()[String(articleId)] || []) + const [saved, setSaved] = useState([]) const wrap = useRef(null) - useEffect(() => { setSaved(readSaved()[String(articleId)] || []) }, [articleId]) + // Asked once per article rather than per library: fetching each library's + // contents to find out would also stamp every one of them as used and + // reorder the reader's own list. + useEffect(() => { + if (!articleId) return undefined + let live = true + api.get(`/collections/for-article/${articleId}`) + .then(res => { if (live) setSaved(res.data?.collection_ids || []) }) + .catch(() => { if (live) setSaved([]) }) + return () => { live = false } + }, [articleId]) useEffect(() => { if (!open) return @@ -54,26 +50,24 @@ export default function ArticleSaveButton({ articleId, questionIds = [] }) { return () => document.removeEventListener('mousedown', away) }, [open]) - const record = useCallback((collectionId) => { - setSaved(prev => { - if (prev.includes(collectionId)) return prev - const next = [...prev, collectionId] - writeSaved({ ...readSaved(), [String(articleId)]: next }) - return next - }) - }, [articleId]) - const saveInto = async (collectionId) => { setBusy(true) try { - // Sequential rather than parallel: a topic with forty questions firing - // forty writes at once is a burst the rate limiter reads as abuse. - for (const questionId of questionIds) { - await api.put(`/collections/${collectionId}/questions/${questionId}`) - } - record(collectionId) + await api.put(`/collections/${collectionId}/articles/${articleId}`) + setSaved(prev => (prev.includes(collectionId) ? prev : [...prev, collectionId])) setQuery('') - } catch { /* nothing is recorded, which is the honest signal */ } + } catch { /* nothing is marked, which is the honest signal */ } + finally { setBusy(false) } + } + + // Putting it back is the same control, so the star is a toggle per library + // rather than a one-way door with the undo somewhere else. + const removeFrom = async (collectionId) => { + setBusy(true) + try { + await api.delete(`/collections/${collectionId}/articles/${articleId}`) + setSaved(prev => prev.filter(id => id !== collectionId)) + } catch { /* it stays marked, which is what the server still believes */ } finally { setBusy(false) } } @@ -100,53 +94,45 @@ export default function ArticleSaveButton({ articleId, questionIds = [] }) { {open && ( -
-

Save this topic

- {questionIds.length === 0 ? ( - /* Said plainly rather than offering a control that would file - nothing: a library holds questions, and this topic has none yet. */ -

No questions are linked to this topic yet, so there is nothing to file. Link one and it can be saved with the reading.

- ) : ( - <> -

Files its {questionIds.length} linked question{questionIds.length === 1 ? '' : 's'} so you can sit them later.

- {/* One box for both. Searching what you have and naming what you - do not are the same act — you type the name of the library - you want and it either exists or it doesn't. */} - setQuery(event.target.value)} - onKeyDown={event => { - if (event.key === 'Enter' && canCreate) { event.preventDefault(); createAndSave(query) } - }} /> - {canCreate && ( - - )} - {found.length > 0 && ( -
    - {found.map(row => { - const inIt = saved.includes(row.id) - return ( -
  • - -
    • - ) - })} -
- )} - {!found.length && !canCreate && ( -

{libraries.length ? 'No library by that name.' : 'Type a name to make your first library.'}

- )} - +
+

Save this article

+ {/* One box for both. Searching what you have and naming what you do + not are the same act — you type the name of the library you want + and it either exists or it doesn't. */} + setQuery(event.target.value)} + onKeyDown={event => { + if (event.key === 'Enter' && canCreate) { event.preventDefault(); createAndSave(query) } + }} /> + {canCreate && ( + + )} + {found.length > 0 && ( +
    + {found.map(row => { + const inIt = saved.includes(row.id) + return ( +
  • + +
    • + ) + })} +
+ )} + {!found.length && !canCreate && ( +

{libraries.length ? 'No library by that name.' : 'Type a name to make your first library.'}

)}
)} diff --git a/frontend/src/components/ArticleSaveButton.test.jsx b/frontend/src/components/ArticleSaveButton.test.jsx index 0bb2348..4897e23 100644 --- a/frontend/src/components/ArticleSaveButton.test.jsx +++ b/frontend/src/components/ArticleSaveButton.test.jsx @@ -4,53 +4,70 @@ import userEvent from '@testing-library/user-event' import ArticleSaveButton from './ArticleSaveButton' import api from '../api/client' -vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn(), put: vi.fn() } })) +vi.mock('../api/client', () => ({ + default: { get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn() }, +})) const libraries = [{ id: 4, title: 'Neurology' }, { id: 9, title: 'Airway' }] -describe('keeping a topic', () => { +// Two different GETs: which libraries exist, and which of them already hold +// this article. +const answer = (held = []) => api.get.mockImplementation(url => Promise.resolve({ + data: url.startsWith('/collections/for-article/') ? { collection_ids: held } : libraries, +})) + +describe('keeping an article', () => { beforeEach(() => { vi.resetAllMocks() - localStorage.clear() - api.get.mockResolvedValue({ data: libraries }) + answer() }) - it('files the topic’s questions into a library and says so afterwards', async () => { - api.put.mockResolvedValue({ data: {} }) - render() + it('files the article itself into a library and says so afterwards', async () => { + api.put.mockResolvedValue({ data: { added: true } }) + render() - await userEvent.click(screen.getByRole('button', { name: 'Save this topic to a library' })) - const panel = await screen.findByRole('dialog', { name: /Save this topic/ }) + await userEvent.click(screen.getByRole('button', { name: 'Save this article to a library' })) + const panel = await screen.findByRole('dialog', { name: /Save this article/ }) await userEvent.click(within(panel).getByRole('button', { name: /Neurology/ })) - await waitFor(() => expect(api.put).toHaveBeenCalledWith('/collections/4/questions/11')) - expect(api.put).toHaveBeenCalledWith('/collections/4/questions/12') - // A reader should be able to tell at a glance that this one is already put - // away, without opening the panel to find out. + // The article, not the questions written against it: a topic with none is + // still worth keeping, and a reader who saved the reading did not ask for + // a session. + await waitFor(() => expect(api.put).toHaveBeenCalledWith('/collections/4/articles/7')) expect(await screen.findByRole('button', { name: 'Saved to 1 library' })).toBeInTheDocument() }) + it('shows it is already kept, from the server rather than from this device', async () => { + answer([9]) + render() + // Nothing was saved in this browser; the server was asked. + expect(await screen.findByRole('button', { name: 'Saved to 1 library' })).toBeInTheDocument() + }) + + it('takes it back out again from the same control', async () => { + answer([4]) + api.delete.mockResolvedValue({}) + render() + + await userEvent.click(await screen.findByRole('button', { name: 'Saved to 1 library' })) + const panel = await screen.findByRole('dialog', { name: /Save this article/ }) + await userEvent.click(within(panel).getByRole('button', { name: /Neurology/ })) + + await waitFor(() => expect(api.delete).toHaveBeenCalledWith('/collections/4/articles/7')) + expect(await screen.findByRole('button', { name: 'Save this article to a library' })).toBeInTheDocument() + }) + it('names a new library and files into it in one press', async () => { api.post.mockResolvedValue({ data: { id: 21, title: 'Seizures' } }) - api.put.mockResolvedValue({ data: {} }) - render() + api.put.mockResolvedValue({ data: { added: true } }) + render() - await userEvent.click(screen.getByRole('button', { name: 'Save this topic to a library' })) - await screen.findByRole('dialog', { name: /Save this topic/ }) + await userEvent.click(screen.getByRole('button', { name: 'Save this article to a library' })) + await screen.findByRole('dialog', { name: /Save this article/ }) await userEvent.type(screen.getByLabelText('Create or find a library'), 'Seizures') await userEvent.click(screen.getByRole('button', { name: /Seizures/ })) await waitFor(() => expect(api.post).toHaveBeenCalledWith('/collections/', { title: 'Seizures' })) - expect(api.put).toHaveBeenCalledWith('/collections/21/questions/11') - }) - - // A library holds questions. Offering the control anyway would file nothing - // and say nothing about why. - it('says plainly when a topic has no questions to file', async () => { - render() - await userEvent.click(screen.getByRole('button', { name: 'Save this topic to a library' })) - - expect(await screen.findByText(/No questions are linked to this topic yet/)).toBeInTheDocument() - expect(screen.queryByLabelText('Create or find a library')).toBeNull() + expect(api.put).toHaveBeenCalledWith('/collections/21/articles/7') }) }) diff --git a/frontend/src/pages/ArticlesPage.css b/frontend/src/pages/ArticlesPage.css index c2af834..a26d4a2 100644 --- a/frontend/src/pages/ArticlesPage.css +++ b/frontend/src/pages/ArticlesPage.css @@ -307,9 +307,28 @@ .asec-body { font-size: 0.92rem; } } -/* One topic, three readings — where AMBOSS puts High-yield, at the right of - the row over the sections. The rail is filtered by the same choice, so the - contents can never list a section the body is no longer showing. */ +/* One topic, three readings — at the right of the row over the sections. The + rail is filtered by the same choice, so the contents can never list a + section the body is no longer showing. */ +.aview-group { display: inline-flex; align-items: center; gap: 10px; } + +/* Summary is a toggle, not a tab: it is the whole topic or the part of it + worth revising, and it names its own state so a reader can tell at a glance + why two thirds of the contents are not there. */ +.aview-only { + display: inline-flex; align-items: center; gap: 8px; + min-height: 36px; padding: 7px 14px; cursor: pointer; + font: inherit; font-size: 0.83rem; font-weight: 600; + color: var(--text-muted); background: var(--card-bg); + border: 1px solid var(--border); border-radius: 8px; +} +.aview-only span { font-size: 0.95rem; line-height: 1; } +.aview-only:hover { color: var(--text); border-color: var(--text-subtle); } +.aview-only.is-on { + color: var(--primary); border-color: var(--primary); + background: color-mix(in srgb, var(--primary) 8%, transparent); +} + .aview-switch { display: inline-flex; gap: 2px; padding: 3px; background: var(--bg); border: 1px solid var(--border); border-radius: 9px; } .aview { min-height: 34px; padding: 6px 13px; border: 0; border-radius: 7px; cursor: pointer; diff --git a/frontend/src/pages/ArticlesPage.jsx b/frontend/src/pages/ArticlesPage.jsx index a309c6b..a7d79a3 100644 --- a/frontend/src/pages/ArticlesPage.jsx +++ b/frontend/src/pages/ArticlesPage.jsx @@ -405,7 +405,7 @@ export function ArticlePage() { breadcrumbs={breadcrumbs} toolbar={ <> - q.question_id)} /> + {editorActions} } diff --git a/frontend/src/pages/ArticlesPage.test.jsx b/frontend/src/pages/ArticlesPage.test.jsx index 45e35e6..526cdbc 100644 --- a/frontend/src/pages/ArticlesPage.test.jsx +++ b/frontend/src/pages/ArticlesPage.test.jsx @@ -108,7 +108,7 @@ describe('topic reading', () => { expect(screen.getByRole('navigation', { name: 'Breadcrumb' })).toHaveTextContent('Neurology') expect(screen.getByText('Introduction markdown')).toBeInTheDocument() // A view of one section is the prose, not a contents page of one entry. - // The tab already names the view — "Short" over a heading reading "In + // The control already names the view — "Summary" over a heading reading "In // short" says the same word twice and hides the only content behind a // chevron. expect(screen.getByText('Section markdown')).toBeInTheDocument() @@ -242,9 +242,10 @@ describe('topic reading', () => { expect(within(card).queryByRole('link')).toBeNull() }) - // Short is our high-yield. The contents are filtered by the same choice as - // the body, so the rail can never offer a heading the article is no longer - // showing — which is the failure worth guarding, not the switch itself. + // Summary is our high-yield, and it is a toggle rather than one tab of + // three. The contents are filtered by the same choice as the body, so the + // rail can never offer a heading the article is no longer showing — which is + // the failure worth guarding, not the switch itself. it('keeps the contents rail in step with the depth being read', async () => { const layered = { ...article, sections: [ { id: 'a'.repeat(32), slug: 'key', title: 'Key points', content: 'Key body', variant: 'short' }, @@ -262,13 +263,18 @@ describe('topic reading', () => { const toc = () => document.querySelector('.article-sections') expect(within(toc()).getByRole('button', { name: 'Key points' })).toBeInTheDocument() expect(within(toc()).queryByRole('button', { name: 'Pathophysiology' })).toBeNull() - expect(screen.getByRole('tab', { name: 'Short' })).toHaveAttribute('aria-selected', 'true') + // It names its own state, so a reader can tell why two thirds of the + // contents are not there. + const summary = () => screen.getByRole('button', { name: /^Summary (on|off)$/ }) + expect(summary()).toHaveAttribute('aria-pressed', 'true') + expect(summary()).toHaveTextContent('Summary on') - await userEvent.click(screen.getByRole('tab', { name: 'Long' })) + await userEvent.click(summary()) expect(within(toc()).getByRole('button', { name: 'Pathophysiology' })).toBeInTheDocument() expect(within(toc()).queryByRole('button', { name: 'Key points' })).toBeNull() expect(screen.queryByText('Key body')).not.toBeInTheDocument() - expect(screen.getByRole('tab', { name: 'Long' })).toHaveAttribute('aria-selected', 'true') + expect(summary()).toHaveAttribute('aria-pressed', 'false') + expect(summary()).toHaveTextContent('Summary off') }) it('nests a sub-section under its parent and keeps references last', async () => {