diff --git a/backend/app/plugins/actions.py b/backend/app/plugins/actions.py index eaf4063..16b966e 100644 --- a/backend/app/plugins/actions.py +++ b/backend/app/plugins/actions.py @@ -254,10 +254,34 @@ class PluginActions: if dst is None or dst.id == msg.mailbox_id: return False dst_id, src_id = dst.id, msg.mailbox_id + # The SOURCE folder's special-use, not the destination parameter: + # consumers of message.changed read the pair as a transition, and + # reporting them equal would erase the move. + src_special = await session.scalar( + select(Mailbox.special_use).where(Mailbox.id == src_id) + ) + thread_id, dst_special = msg.thread_id, dst.special_use msg.mailbox_id = dst_id # optimistic, as the core endpoint does await session.commit() await enqueue_job("move_job", str(message_id), str(dst_id), str(src_id)) + # Announce the optimistic state now rather than waiting for the IMAP move + # to finish. The core's own move endpoint gets this for free — its caller + # updates the UI from the response — but a plugin acting on the user's + # behalf has no client-side handle, so without this the message lingers on + # screen until the job lands. move_job publishes again when it does. + await get_event_bus().publish( + "message.changed", + { + "user_id": str(user_id), + "message_id": str(message_id), + "thread_id": str(thread_id) if thread_id else None, + "mailbox_id": str(dst_id), + "previous_mailbox_id": str(src_id), + "special_use": dst_special, + "previous_special_use": src_special, + }, + ) return True async def trash(self, user_id: uuid.UUID, message_id: uuid.UUID) -> bool: diff --git a/backend/app/sync/actions.py b/backend/app/sync/actions.py index 39c0062..38ba0ad 100644 --- a/backend/app/sync/actions.py +++ b/backend/app/sync/actions.py @@ -130,6 +130,10 @@ async def move_job( # transition, not the destination. "Now in Spam" and "moved out of Spam" # are different facts, and only the pair distinguishes them. src_special, dst_special = src.special_use, dst.special_use + # The open reading panel is keyed by thread: without this the UI + # invalidates the message list but never refetches the thread the user is + # looking at, so a moved message stays on screen. + thread_id = message.thread_id provider = build_provider(account) try: @@ -152,6 +156,7 @@ async def move_job( { "user_id": str(user_id), "message_id": str(mid), + "thread_id": str(thread_id) if thread_id else None, "mailbox_id": str(dst_id), "previous_mailbox_id": src_mailbox_id, "special_use": dst_special, diff --git a/backend/tests/plugins/test_antispam_api.py b/backend/tests/plugins/test_antispam_api.py index 118c62d..b0e0a18 100644 --- a/backend/tests/plugins/test_antispam_api.py +++ b/backend/tests/plugins/test_antispam_api.py @@ -186,3 +186,20 @@ async def test_ui_bundles_are_served_and_gated(ctx: _Ctx) -> None: assert r.status_code == 200 assert "javascript" in r.headers["content-type"] assert (await ctx.client_a.get(f"{BASE}/ui/../hook.py")).status_code == 404 + + +async def test_learn_records_even_without_a_prior_verdict(ctx: _Ctx) -> None: + """Mail the provider filed as spam has no verdict row of ours to update. + + The write must therefore upsert — and it must happen before the move, or the + passive-learning hook re-teaches the lesson the move announces. + """ + message_id = uuid.uuid4() + r = await ctx.client_a.post( + f"{BASE}/learn", json={"message_id": str(message_id), "verdict": "spam"} + ) + assert r.status_code == 200 + assert r.json()["moved"] is False # not this user's message: nothing to move + + body = (await ctx.client_a.get(f"{BASE}/context/{message_id}")).json() + assert body["verdict"]["learned"] == "spam" diff --git a/backend/tests/plugins/test_plugin_move_event.py b/backend/tests/plugins/test_plugin_move_event.py new file mode 100644 index 0000000..6de3077 --- /dev/null +++ b/backend/tests/plugins/test_plugin_move_event.py @@ -0,0 +1,118 @@ +"""`move_to_special` must describe the move it just made. + +The reading panel is keyed by thread and the passive-learning hook reads the +folder pair, so an event that names neither leaves a moved message on screen and +teaches nothing. +""" + +from __future__ import annotations + +import datetime as dt +import uuid +from collections.abc import AsyncIterator +from typing import Any + +import pytest_asyncio +from sqlalchemy import delete +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from sqlalchemy.pool import NullPool + +from app.config import get_settings +from app.identity.passwords import hash_password +from app.models import Account, Mailbox, Message, Thread, User +from app.plugins.actions import PluginActions + + +class _Ctx: + user_id: uuid.UUID + message_id: uuid.UUID + thread_id: uuid.UUID + + +@pytest_asyncio.fixture +async def ctx() -> AsyncIterator[_Ctx]: + engine = create_async_engine(get_settings().database_url, poolclass=NullPool) + sm = async_sessionmaker(engine, expire_on_commit=False) + c = _Ctx() + async with sm() as s: + user = User(handle=f"mv{uuid.uuid4().hex[:8]}@test.local", password_hash=hash_password("p")) + s.add(user) + await s.flush() + account = Account( + user_id=user.id, + email="a@b.test", + imap_config={}, + smtp_config={}, + secret_ct=b"x", + discovery_src="manual", + ) + s.add(account) + await s.flush() + inbox = Mailbox(account_id=account.id, user_id=user.id, path="INBOX", special_use="inbox") + junk = Mailbox(account_id=account.id, user_id=user.id, path="SPAM", special_use="junk") + thread = Thread( + user_id=user.id, + subject_norm="t", + last_message_at=dt.datetime.now(dt.UTC), + ) + s.add_all([inbox, junk, thread]) + await s.flush() + msg = Message( + user_id=user.id, + account_id=account.id, + mailbox_id=inbox.id, + thread_id=thread.id, + uid=1, + message_id_hdr=f"<{uuid.uuid4()}@t>", + subject="t", + from_addr=[{"email": "s@x.test"}], + to_addr=[], + flags=[], + internal_date=dt.datetime.now(dt.UTC), + blob_sha256="0" * 64, + snippet="t", + ) + s.add(msg) + await s.commit() + c.user_id, c.message_id, c.thread_id = user.id, msg.id, thread.id + try: + yield c + finally: + async with sm() as s: + await s.execute(delete(User).where(User.id == c.user_id)) + await s.commit() + await engine.dispose() + + +async def test_move_event_names_the_thread_and_both_folders( + ctx: _Ctx, monkeypatch: pytest_asyncio.plugin.pytest.MonkeyPatch +) -> None: + published: list[tuple[str, dict[str, Any]]] = [] + enqueued: list[tuple[Any, ...]] = [] + + class _Bus: + async def publish(self, event: str, payload: dict[str, Any]) -> None: + published.append((event, payload)) + + async def _enqueue(*args: Any, **kw: Any) -> None: + enqueued.append(args) + + monkeypatch.setattr("app.plugins.actions.get_event_bus", lambda: _Bus()) + monkeypatch.setattr("app.queue.enqueue_job", _enqueue) + + moved = await PluginActions().move_to_special(ctx.user_id, ctx.message_id, "junk") + assert moved is True + + assert enqueued, "the IMAP move must still be queued" + event, payload = published[0] + assert event == "message.changed" + # The panel is keyed by thread: without this it never refetches. + assert payload["thread_id"] == str(ctx.thread_id) + # The pair is the transition passive learning reads. + assert payload["previous_special_use"] == "inbox" + assert payload["special_use"] == "junk" + + +async def test_no_event_when_there_is_nowhere_to_move(ctx: _Ctx) -> None: + """An account with no such folder: no move, and nothing announced.""" + assert await PluginActions().move_to_special(ctx.user_id, ctx.message_id, "archive") is False diff --git a/frontend/src/routes/MailLayout.tsx b/frontend/src/routes/MailLayout.tsx index 9d0a04d..793ebb4 100644 --- a/frontend/src/routes/MailLayout.tsx +++ b/frontend/src/routes/MailLayout.tsx @@ -200,6 +200,15 @@ export function MailLayout() { enabled: Boolean(threadId), }); + // A thread whose last visible message left — marked as spam, trashed, moved — + // should not linger in the reading panel. General on purpose: it closes the + // panel whoever emptied the thread, core action or plugin. + useEffect(() => { + if (threadId && detailQuery.data && detailQuery.data.messages.length === 0) { + navigate("/mail"); + } + }, [threadId, detailQuery.data, navigate]); + const markRead = useMutation({ mutationFn: (id: string) => api.threadFlags(id, ["\\Seen"], []), onSuccess: () => { diff --git a/plugins/webmail_plugin_antispam/webmail_plugin_antispam/router.py b/plugins/webmail_plugin_antispam/webmail_plugin_antispam/router.py index 0bb4af2..f2038d7 100644 --- a/plugins/webmail_plugin_antispam/webmail_plugin_antispam/router.py +++ b/plugins/webmail_plugin_antispam/webmail_plugin_antispam/router.py @@ -17,6 +17,7 @@ from sqlalchemy import text from app.plugins.sdk import CurrentUserId, require_csrf, sdk from webmail_plugin_antispam import policy, rspamd +from webmail_plugin_antispam.hook import _remember_learned from webmail_plugin_antispam.schemas import ( LearnIn, MessageContextOut, @@ -225,24 +226,21 @@ async def learn(body: LearnIn, user_id: CurrentUserId) -> dict[str, object]: if raw is not None: taught = await rspamd.learn(raw, user_key=str(user_id), spam=spam) + # Record BEFORE moving. The move publishes message.changed, which the passive + # learning hook reads as "the user filed this as spam" — and it would teach + # the very lesson we just taught. Writing `learned` first makes that hook's + # dedup see it and stand down. An upsert, because a message the provider + # filed as spam has no verdict row of ours to update. + await _remember_learned( + user_id, + body.message_id, + verdict=body.verdict, + action="moved" if spam else "none", + ) + moved = await _SDK.actions.move_to_special( user_id, body.message_id, "junk" if spam else "inbox" ) - - async with _SDK.session(user_id) as s: - await s.execute( - text( - "UPDATE plg_antispam_verdicts SET learned = :l, action = :a " - "WHERE message_id = :m AND user_id = :u" - ), - { - "l": body.verdict, - "a": "moved" if spam else "none", - "m": str(body.message_id), - "u": str(user_id), - }, - ) - await s.commit() return {"status": "ok", "taught": taught, "moved": moved}