Clear the reading panel when its last message leaves
"Mark as Spam" moved the message but left it on screen. Two causes, both fixed generally rather than for this one action. `move_job` published message.changed without a thread_id, so the WS handler invalidated the message list but never the thread the user was reading — the panel had no reason to refetch. It now names the thread. And a thread whose last visible message has gone should not linger. MailLayout closes the panel when the open thread comes back empty, whoever emptied it — core action or plugin. `move_to_special` now also publishes the move immediately rather than waiting for the IMAP job, since a plugin acting on the user's behalf has no client-side handle to update optimistically the way the core endpoint's caller does. That exposed an ordering bug: the optimistic event reaches the passive-learning hook, which would re-teach the lesson /learn had just taught. /learn now records `learned` before moving, so the hook's dedup stands down — and records it with an upsert, because provider-filed spam has no verdict row of ours to update. One real mistake on the way: the event first reported the destination folder as the source, which would have made every move look like a no-op to anything reading the pair. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SqJYt7jD7oUEkoY68sBZo
This commit is contained in:
@@ -254,10 +254,34 @@ class PluginActions:
|
|||||||
if dst is None or dst.id == msg.mailbox_id:
|
if dst is None or dst.id == msg.mailbox_id:
|
||||||
return False
|
return False
|
||||||
dst_id, src_id = dst.id, msg.mailbox_id
|
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
|
msg.mailbox_id = dst_id # optimistic, as the core endpoint does
|
||||||
await session.commit()
|
await session.commit()
|
||||||
|
|
||||||
await enqueue_job("move_job", str(message_id), str(dst_id), str(src_id))
|
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
|
return True
|
||||||
|
|
||||||
async def trash(self, user_id: uuid.UUID, message_id: uuid.UUID) -> bool:
|
async def trash(self, user_id: uuid.UUID, message_id: uuid.UUID) -> bool:
|
||||||
|
|||||||
@@ -130,6 +130,10 @@ async def move_job(
|
|||||||
# transition, not the destination. "Now in Spam" and "moved out of Spam"
|
# transition, not the destination. "Now in Spam" and "moved out of Spam"
|
||||||
# are different facts, and only the pair distinguishes them.
|
# are different facts, and only the pair distinguishes them.
|
||||||
src_special, dst_special = src.special_use, dst.special_use
|
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)
|
provider = build_provider(account)
|
||||||
try:
|
try:
|
||||||
@@ -152,6 +156,7 @@ async def move_job(
|
|||||||
{
|
{
|
||||||
"user_id": str(user_id),
|
"user_id": str(user_id),
|
||||||
"message_id": str(mid),
|
"message_id": str(mid),
|
||||||
|
"thread_id": str(thread_id) if thread_id else None,
|
||||||
"mailbox_id": str(dst_id),
|
"mailbox_id": str(dst_id),
|
||||||
"previous_mailbox_id": src_mailbox_id,
|
"previous_mailbox_id": src_mailbox_id,
|
||||||
"special_use": dst_special,
|
"special_use": dst_special,
|
||||||
|
|||||||
@@ -186,3 +186,20 @@ async def test_ui_bundles_are_served_and_gated(ctx: _Ctx) -> None:
|
|||||||
assert r.status_code == 200
|
assert r.status_code == 200
|
||||||
assert "javascript" in r.headers["content-type"]
|
assert "javascript" in r.headers["content-type"]
|
||||||
assert (await ctx.client_a.get(f"{BASE}/ui/../hook.py")).status_code == 404
|
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"
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -200,6 +200,15 @@ export function MailLayout() {
|
|||||||
enabled: Boolean(threadId),
|
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({
|
const markRead = useMutation({
|
||||||
mutationFn: (id: string) => api.threadFlags(id, ["\\Seen"], []),
|
mutationFn: (id: string) => api.threadFlags(id, ["\\Seen"], []),
|
||||||
onSuccess: () => {
|
onSuccess: () => {
|
||||||
|
|||||||
@@ -17,6 +17,7 @@ from sqlalchemy import text
|
|||||||
|
|
||||||
from app.plugins.sdk import CurrentUserId, require_csrf, sdk
|
from app.plugins.sdk import CurrentUserId, require_csrf, sdk
|
||||||
from webmail_plugin_antispam import policy, rspamd
|
from webmail_plugin_antispam import policy, rspamd
|
||||||
|
from webmail_plugin_antispam.hook import _remember_learned
|
||||||
from webmail_plugin_antispam.schemas import (
|
from webmail_plugin_antispam.schemas import (
|
||||||
LearnIn,
|
LearnIn,
|
||||||
MessageContextOut,
|
MessageContextOut,
|
||||||
@@ -225,24 +226,21 @@ async def learn(body: LearnIn, user_id: CurrentUserId) -> dict[str, object]:
|
|||||||
if raw is not None:
|
if raw is not None:
|
||||||
taught = await rspamd.learn(raw, user_key=str(user_id), spam=spam)
|
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(
|
moved = await _SDK.actions.move_to_special(
|
||||||
user_id, body.message_id, "junk" if spam else "inbox"
|
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}
|
return {"status": "ok", "taught": taught, "moved": moved}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user