diff options
| -rw-r--r-- | docs/USERGUIDE.md | 10 | ||||
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/api/notifications.py | 61 | ||||
| -rw-r--r-- | packages/meshbay-hub/src/meshbay_hub/static/app.js | 8 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_notification_dismissal.py | 48 | ||||
| -rw-r--r-- | packages/meshbay-hub/tests/test_notifications.py | 13 |
5 files changed, 109 insertions, 31 deletions
diff --git a/docs/USERGUIDE.md b/docs/USERGUIDE.md index 7bd695b..de5ce3f 100644 --- a/docs/USERGUIDE.md +++ b/docs/USERGUIDE.md @@ -327,8 +327,9 @@ what it is about and dismisses it. ``` GET /v1/notifications → {"notifications": [{id, kind, group_id, title, link, read, created_at}], "unread": 3} -POST /v1/notifications/{id}/read → mark one read -POST /v1/notifications/read-all → mark every one read +DELETE /v1/notifications/{id} → dismiss one — the row is deleted +POST /v1/notifications/{id}/read → the same thing, under the name older clients use +POST /v1/notifications/read-all → dismiss every one DELETE /v1/notifications → delete them all POST /v1/groups/{group_id}/mute {"muted": true} ``` @@ -1099,8 +1100,9 @@ There is no hub endpoint that touches group key material. | Method | Path | Auth | Description | |---|---|---|---| | GET | `/v1/notifications` | Access token | Your notifications, newest first, plus an `unread` count. Chat is one entry per group. | -| POST | `/v1/notifications/{id}/read` | Access token | Mark one read. | -| POST | `/v1/notifications/read-all` | Access token | Mark all read. | +| DELETE | `/v1/notifications/{id}` | Access token | Dismiss one. The row is deleted — a notification is a signal, not a record, and the group, the message and the invitation it pointed at are all still there. | +| POST | `/v1/notifications/{id}/read` | Access token | The same thing. Kept because the interface ships inside the desktop package, so a hub is always answering some client older than itself. | +| POST | `/v1/notifications/read-all` | Access token | Dismiss every one. Same as `DELETE /v1/notifications`. | | DELETE | `/v1/notifications` | Access token | Delete all of yours. | | POST | `/v1/groups/{group_id}/mute` | Access token (member) | Body: `muted`. A muted group creates no notifications at all. | diff --git a/packages/meshbay-hub/src/meshbay_hub/api/notifications.py b/packages/meshbay-hub/src/meshbay_hub/api/notifications.py index ca56620..9d5c125 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/notifications.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/notifications.py @@ -1,9 +1,28 @@ -"""Notification endpoints — /v1/notifications/*""" +""" +Notification endpoints — /v1/notifications/* + +**Dismissing one deletes it.** These are signals, not a record: the group is +still there, the message is still in the chat, the invitation is still an +invitation, so nothing is lost by dropping the row — which is what +`purge_notifications` below has always said, now applied to one at a time. + +That resolves a disagreement between two halves that were each defensible +alone. The interface treats a click as "this is gone" and removes the entry; +the hub marked it read and kept it; and the next launch listed read entries +too, so everything dismissed came back. Filtering the list to unread fixed +what the user saw and left the rows accumulating for nothing, invisible for +ever — which is the state this replaces. + +`Notification.read` is therefore **vestigial**: nothing stored can be read, +because reading it deletes it. It stays because dropping a column is a +migration for no gain, and `unread_only` stays because it is what an older +interface asks for and it still answers correctly — every row is unread. +""" from fastapi import APIRouter, Depends, HTTPException from datetime import datetime, timezone -from sqlalchemy import delete, func, select, update +from sqlalchemy import delete, func, select from sqlalchemy.ext.asyncio import AsyncSession from meshbay_hub.api.deps import get_current_user @@ -52,16 +71,27 @@ async def list_notifications( } +@router.delete("/{notification_id}") @router.post("/{notification_id}/read") -async def mark_read( +async def dismiss( notification_id: int, current_user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), ): + """ + Dismiss one. The row goes. + + Two paths to the same handler. `DELETE /{id}` says what happens and is what + the interface calls; `POST /{id}/read` is what every already-installed + client calls, and it has to keep working — the SPA ships inside the desktop + package, so a hub is always talking to some interface older than itself. + Giving the old path the new behaviour means those clients stop accumulating + rows too, rather than only the ones that have been updated. + """ notif = await db.get(Notification, notification_id) if not notif or notif.user_id != current_user.id: raise HTTPException(status_code=404, detail="Notification not found") - notif.read = True + await db.delete(notif) await db.commit() return {"status": "ok"} @@ -76,7 +106,8 @@ async def purge_notifications( These are signals, not a record: the group is still there, the message is still in the chat, the invitation is still an invitation. Nothing is lost by - clearing the list, so it clears rather than marking a hundred rows read. + clearing the list, so it clears rather than marking a hundred rows read — + the reasoning the whole module now follows. """ result = await db.execute( delete(Notification).where(Notification.user_id == current_user.id)) @@ -85,17 +116,23 @@ async def purge_notifications( @router.post("/read-all") -async def mark_all_read( +async def dismiss_all( current_user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), ): - await db.execute( - update(Notification) - .where(Notification.user_id == current_user.id, Notification.read == False) # noqa: E712 - .values(read=True) - ) + """ + Dismiss every one — the same thing as `DELETE ""`, under the name an older + client knows it by. + + Marking them read instead would put back exactly what this change removes: + rows the list can never show again. Nothing in this repository calls it, + but an endpoint that is reachable is an endpoint that can be called, and it + should not be the one route that still hoards. + """ + result = await db.execute( + delete(Notification).where(Notification.user_id == current_user.id)) await db.commit() - return {"status": "ok"} + return {"status": "ok", "removed": result.rowcount} async def create_notification( diff --git a/packages/meshbay-hub/src/meshbay_hub/static/app.js b/packages/meshbay-hub/src/meshbay_hub/static/app.js index 7cc7c83..5311403 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/app.js @@ -650,7 +650,9 @@ function App() { // the page navigates, which reads as "the click did nothing". setNotifications(prev => prev.filter(n => n.id !== id)); setUnreadCount(c => Math.max(0, c - 1)); - hubFetch(`/v1/notifications/${id}/read`, { method: 'POST', token: user.token }) + // DELETE, not `/read`: dismissing one drops the row. The old path still + // works and still deletes, for interfaces older than the hub. + hubFetch(`/v1/notifications/${id}`, { method: 'DELETE', token: user.token }) .catch(() => fetchNotifications()); }, [user, fetchNotifications]); @@ -667,8 +669,8 @@ function App() { if (!user) return; setNotifications(prev => { const gone = prev.filter(n => n.group_id === groupId && n.kind === 'group_invite'); - gone.forEach(n => hubFetch(`/v1/notifications/${n.id}/read`, - { method: 'POST', token: user.token }).catch(() => {})); + gone.forEach(n => hubFetch(`/v1/notifications/${n.id}`, + { method: 'DELETE', token: user.token }).catch(() => {})); if (gone.length) setUnreadCount(c => Math.max(0, c - gone.length)); return prev.filter(n => !gone.includes(n)); }); diff --git a/packages/meshbay-hub/tests/test_notification_dismissal.py b/packages/meshbay-hub/tests/test_notification_dismissal.py index 7324656..07cf715 100644 --- a/packages/meshbay-hub/tests/test_notification_dismissal.py +++ b/packages/meshbay-hub/tests/test_notification_dismissal.py @@ -15,8 +15,15 @@ notification came straight back. The feed still carried the fossil of the older intent — `class="notif-item ${n.read ? '' : 'notif-unread'}"`, styling for a read entry rendered greyed out, from before clicking meant dismissing. -So the contract is between the two halves, and that is what this pins: the -hub's filter behaves, and the SPA asks for it. +Filtering the list to unread was the first fix. It corrected what the user saw +and left the rows accumulating for nothing, invisible for ever, which is not a +resolution so much as a place to hide the disagreement. **Dismissing one now +deletes it** — the reasoning `purge_notifications` always carried, applied one +at a time: these are signals, not a record. + +So what this pins is the contract between the two halves: the hub drops the +row by either path, old client or new, and the SPA still sends the filter for +the case where it is newer than the hub it is talking to. """ import re @@ -91,19 +98,42 @@ async def test_a_read_notification_is_gone_from_what_the_spa_asks_for(client): @pytest.mark.asyncio -async def test_without_the_filter_it_does_come_back(client): - """The other half of the same fact, so the fix cannot be read as a - coincidence: unfiltered, the endpoint still returns it, and that is the - request the SPA used to make.""" +async def test_it_is_gone_from_the_unfiltered_list_too(client): + """The filter is no longer what makes it disappear — the row is. + + This assertion is the inverse of what it was. Filtering to unread fixed + what the user saw and left the rows behind, invisible for ever; dismissing + now deletes, so even a client that asks for everything sees nothing. The + filter stays because an interface older than the hub still sends it. + """ token = await _login_and_notify(client) auth = {"Authorization": f"Bearer {token}"} nid = (await client.get("/v1/notifications", headers=auth)).json()["notifications"][0]["id"] - await client.post(f"/v1/notifications/{nid}/read", headers=auth) + await client.delete(f"/v1/notifications/{nid}", headers=auth) unfiltered = (await client.get("/v1/notifications?limit=20", headers=auth)).json() - assert len(unfiltered["notifications"]) == 1 - assert unfiltered["notifications"][0]["read"] is True + assert unfiltered["notifications"] == [] + + +@pytest.mark.asyncio +async def test_the_old_read_path_dismisses_too(client): + """Version skew is the normal case here, not the exception. + + The SPA ships inside the desktop package, so a hub is always talking to + some interface older than itself — CLAUDE.md records that as a standing + consequence of shipping the UI in a package. An old client calling + `/read` must stop accumulating rows as well, or the fix only reaches + whoever updated. + """ + token = await _login_and_notify(client) + auth = {"Authorization": f"Bearer {token}"} + + nid = (await client.get("/v1/notifications", headers=auth)).json()["notifications"][0]["id"] + r = await client.post(f"/v1/notifications/{nid}/read", headers=auth) + assert r.status_code == 200 + + assert (await client.get("/v1/notifications", headers=auth)).json()["notifications"] == [] # `_one_notification_for_alice` is the setup; this alias keeps the tests reading diff --git a/packages/meshbay-hub/tests/test_notifications.py b/packages/meshbay-hub/tests/test_notifications.py index 8da589a..35ed288 100644 --- a/packages/meshbay-hub/tests/test_notifications.py +++ b/packages/meshbay-hub/tests/test_notifications.py @@ -92,7 +92,7 @@ async def test_notification_on_suspend(client): @pytest.mark.asyncio -async def test_mark_notification_read(client): +async def test_dismissing_one_deletes_it(client): _, admin_token = await _setup_admin(client) uid = await _register(client, "alice", email="a@x.com") alice_token = await _login(client, "alice") @@ -105,6 +105,8 @@ async def test_mark_notification_read(client): headers={"Authorization": f"Bearer {alice_token}"}) nid = r.json()["notifications"][0]["id"] + # `/read` is the old path and still the one older clients call. It + # dismisses, like DELETE — see api/notifications.py. r = await client.post(f"/v1/notifications/{nid}/read", headers={"Authorization": f"Bearer {alice_token}"}) assert r.status_code == 200 @@ -112,11 +114,13 @@ async def test_mark_notification_read(client): r = await client.get("/v1/notifications", headers={"Authorization": f"Bearer {alice_token}"}) assert r.json()["unread_count"] == 0 - assert r.json()["notifications"][0]["read"] is True + assert r.json()["notifications"] == [], ( + "a dismissed notification is deleted, not kept as a row nothing can " + "ever show again") @pytest.mark.asyncio -async def test_mark_all_read(client): +async def test_dismissing_all_deletes_them(client): _, admin_token = await _setup_admin(client) uid = await _register(client, "alice", email="a@x.com") alice_token = await _login(client, "alice") @@ -135,10 +139,13 @@ async def test_mark_all_read(client): r = await client.post("/v1/notifications/read-all", headers={"Authorization": f"Bearer {alice_token}"}) assert r.status_code == 200 + assert r.json()["removed"] == 2 r = await client.get("/v1/notifications", headers={"Authorization": f"Bearer {alice_token}"}) assert r.json()["unread_count"] == 0 + assert r.json()["notifications"] == [], ( + "read-all must not be the one route that still hoards rows") @pytest.mark.asyncio |