From 52ef7acdea4f4d3fb94eecd2eb2c9dd1e75bce48 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sat, 12 Sep 2026 14:52:20 +0200 Subject: [PATCH] feat: articles are collectible, and Summary is a toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A library holds both articles and questions now. It held only questions, so the bookmark on an article had nowhere to write and stood in for the questions filed under the topic instead — which is not what a reader who saved the reading asked for, and left a topic with no questions unsaveable. Its own table rather than a nullable column beside `question_id`: that shape allows a row with both or neither, and every read then has to say which kind it is looking at. Which libraries already hold an article is now asked of the server, as one question. It was kept on the device because the API could not answer, which was wrong on the second machine and silently so. Putting one back is the same control rather than an undo somewhere else. "Short" is called Summary, because that is what the section is called, and it is a toggle rather than one tab of three — the whole topic, or the part of it worth revising, which is a different kind of choice from Long versus Clinical. It names its own state, so a reader can tell why two thirds of the contents are not there. The stored variant stays `short`: renaming it would be a data migration to change a word on a button. Also: `litellm==1.28.13` has been withdrawn from PyPI, so requirements.txt could not be edited at all without the pip layer failing to rebuild — which is what blocked pinning Pillow. Repinned to 1.53.1, the nearest still published; the three things we use are unchanged in it, and both suites pass on the new set. Pillow is pinned properly now rather than arriving through PyMuPDF. One consequence, handled: `litellm.utils.get_valid_models()` now returns nothing unless a provider's own API key is in the environment, and ours is a proxy. That branch is only reached when no proxy is configured, and it now says so instead of answering with an empty list that reads as "this site has no models". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- .../c5d6e7f8091a_collectible_articles.py | 44 +++++ backend/app/models/collection.py | 17 ++ backend/app/routers/admin.py | 11 +- backend/app/routers/collections.py | 72 +++++++- backend/app/services/article_service.py | 15 ++ backend/requirements.txt | 14 +- frontend/src/components/ArticleEditor.jsx | 2 +- .../src/components/ArticleEditor.test.jsx | 4 +- frontend/src/components/ArticleReader.jsx | 57 +++++-- frontend/src/components/ArticleSaveButton.jsx | 158 ++++++++---------- .../src/components/ArticleSaveButton.test.jsx | 73 ++++---- frontend/src/pages/ArticlesPage.css | 25 ++- frontend/src/pages/ArticlesPage.jsx | 2 +- frontend/src/pages/ArticlesPage.test.jsx | 20 ++- 14 files changed, 366 insertions(+), 148 deletions(-) create mode 100644 backend/alembic/versions/c5d6e7f8091a_collectible_articles.py 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 () => {