feat(mail): delete for good, and a way back out of the bin

Reported as: deleting a mail that is already in the trash takes the row off the
list and leaves the mail in the trash. And, separately: why can spam and trashed
mail be snoozed and archived?

Both are the same cause. Trashing is "add the Trash label", so trashing
something already trashed is a no-op on the server — while the list removed the
row anyway, which is why the mail looked deleted twice and was there both times.
Archive and Snooze in the bin are the same question from the other side.

## The action set now depends on where the mail sits

MailPlacement reads the conversation's own labels rather than the current route,
because a thread reached from a search result, a live update or a URL has no
list behind it and a route check would go quietly back to offering the wrong
controls in exactly those cases. Archive, Delete and Snooze are hidden in trash
and spam, on the row, in the thread header and in the overflow menu.

It is a service the Twig function delegates to, not a Twig function alone: the
purge route refuses anything MailPlacement does not call discarded, and an
interface that hides a control while the route behind it stays open is a habit
rather than a rule.

## Delete forever

Hiding the control left the bin with no way to be emptied, which is the wrong
place to stop, so this is the real thing.

Most of it turned out to be written and unconnected: ApplyImapFlagsHandler has
implemented `'delete' => $imapMessage->delete(expunge: true)` all along, under a
comment saying the row was "on its way to having no row at all", and
MailChangeRecorder::emailDestroyed() existed with no callers. What was missing
was the part that removes the local copy — and one thing that could not have
been reused.

**LabelChangePropagator cannot carry a purge, and that is the whole design.**
Every propagation message names local Message rows and lets the handler look
them up. That works while the row survives the operation. A purge deletes it, so
a handler would find nothing, log "no messages found" and return — leaving the
mail on the server to come back at the next sync as a message the user had
explicitly destroyed. Invisible against a fake provider. So
PurgeRemoteMessagesMessage carries the ADDRESS instead: IMAP folder and UID,
Gmail id, Graph id. Onto the export queue with every other outbound push, so a
purge cannot overtake a relabel already queued for the same mail.

Note propagator::delete() is NOT dead code and is untouched — it is
ComposeController::discard(), where TRASH at Gmail is right rather than a
compromise: a draft that was never sent belongs in the bin its own client would
put it in. Its docblock now says so, because I misread it as dead on the way
past.

Gmail gets messages.batchDelete, needing the full mail scope. Without it the API
answers insufficientPermissions, which assertSuccess() already classifies as
unrecoverable — so it lands in the log once rather than retrying forever.
Falling back to TRASH would be the worst available answer: the user is told it
is gone, plMail can no longer see it, and Google still has it.

The files go too. A deletion that leaves the bytes on disk is a lie in the way
that matters most for a mail client. Parts are removed explicitly rather than by
adding cascade to the mapping, which would change every other path that touches
a message.

## Restore

The counterpart the bin never had: mail could be thrown away and not fetched
back, except by finding it and attaching Inbox by hand from the label menu.
Removes both Trash and Spam, not only the one it was found under — a spam mail
somebody then deleted carries both, and one returned to the inbox still marked
SPAM gets filed away again by Gmail without plMail doing anything.

## Also here

- Pasted markup is reduced to content (CODE-REVIEW U-02): style, class, id,
  bgcolor, font and every alignment attribute go; lists, links and bold stay.
  Whitelisted rather than flattened to plain text, which is simpler and throws
  away what people are usually pasting for. The `<style>` block was the worst of
  them — it restyled the whole composer, not just the pasted text.
- Confirmations go through the app's dialog, not the browser's. Note the row's
  Delete forever asks in JavaScript rather than through data-turbo-confirm:
  Turbo only consults that attribute for links and form submissions, so on a
  fetch-posting button it would have looked like a confirmation and never been
  read.

Verified: 4009 unit tests green, PHPStan clean over src/.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Paul LütznerandClaude Opus 5 committed 2026-08-22 13:16:18 +02:00
1 parent 0f80d2c43b
commit 59cbbc9827
34 files changed
+1863 -33

No files matched your search

+1
View File
@@ -2,6 +2,7 @@ import './stimulus_bootstrap.js';
import './password_manager_ignore.js';
import './nav_origin.js';
import './motion.js';
import './confirm.js';
/*
* The stylesheets are NOT imported here, and their absence is deliberate.
*
+48
View File
@@ -0,0 +1,48 @@
/**
* Point Turbo's confirmation at the application's own dialog.
*
* `data-turbo-confirm` calls `window.confirm()` unless Turbo is told otherwise,
* and the browser dialog is the one piece of interface plMail cannot style,
* theme or translate — it also announces the origin at the top, which makes
* "delete this label?" read like a phishing warning. Twenty-two places used it.
*
* One registration rather than twenty-two rewrites, and that is the point: the
* next `data-turbo-confirm` anybody writes is covered without them having to
* know this file exists.
*
* Falls back to `window.confirm` when the dialog is not on the page. That is
* not defensive noise — the sharing and booking pages have their own layout
* which does not include it, and a confirmation that silently returned false
* there would turn a destructive button into one that does nothing, which is
* the worse failure of the two.
*/
import * as Turbo from "@hotwired/turbo";
/**
* Ask the question, and answer true or false.
*
* Exported as well as registered with Turbo, because not every destructive
* action in the app is a link or a form submission — `data-turbo-confirm` is
* only consulted for those two, and a Stimulus controller that posts with
* fetch() would carry the attribute and never be asked anything. Delete
* forever, on a row, is exactly that shape. One function so those callers do
* not each grow their own dialog.
*
* @param {string} message
* @returns {Promise<boolean>}
*/
export function askConfirm(message) {
const dialog = document.getElementById("confirm-dialog");
if (dialog?.plConfirm) {
return dialog.plConfirm(message);
}
// Not defensive noise: the sharing and booking pages use their own layout,
// which does not include the dialog. A confirmation that silently answered
// false there would turn a destructive button into one that does nothing,
// which is the worse of the two failures.
return Promise.resolve(window.confirm(message));
}
Turbo.setConfirmMethod(askConfirm);
@@ -14,6 +14,16 @@ import { Controller } from '@hotwired/stimulus'
* server's answer only hands it the two URLs it needs (see
* compose--send-hold), and nothing has to hunt the DOM for a button to press.
*/
/**
* Markup that never belongs in a mail body, removed with its contents.
*
* <style> is the one that matters: pasting from a page brings its stylesheet
* along, and it would restyle the whole composer rather than only the pasted
* text. <meta> and <link> are how Word announces itself. The rest cannot do
* anything useful in mail and can do plenty elsewhere.
*/
const DROPPED_ON_PASTE = 'style, script, link, meta, iframe, object, embed, form, input, button, textarea, select';
export default class extends Controller {
static targets = ['toField', 'ccField', 'bccField', 'subject', 'body', 'saveStatus', 'toCollection', 'collapsible', 'minimizeIcon', 'expandIcon', 'ccBtn', 'bccBtn', 'title', 'accountSelect', 'fromBtn', 'fromLabel', 'fromChevron', 'fromDropdown', 'fromRow', 'fields', 'fieldsChevron', 'fileInput', 'imageInput', 'attachments', 'scroller', 'formatBar', 'formatToggle', 'sendBtn', 'sendLabel', 'sendProgress', 'sendDots', 'sendCancelHint', 'errors', 'plainBody', 'plainToggle', 'plainCheck', 'plainWarning', 'plainWarningConfirm', 'sendWarning', 'sendWarningBody', 'sendWarningConfirm', 'richOnly'];
@@ -3086,6 +3096,8 @@ export default class extends Controller {
.filter((file) => file.type.startsWith('image/'));
if (0 === files.length) {
this._pasteMarkup(event);
return;
}
@@ -3096,6 +3108,63 @@ export default class extends Controller {
this._placeImages(files);
}
/**
* HTML from the clipboard, reduced to the markup a mail should carry.
*
* The browser's own paste keeps everything: three sentences out of Word or
* a web page bring that document's fonts, its background colours, its
* nested tables and its class names into the body. That is the whole "why
* does my mail look like this" effect, and the recipient sees it in full,
* because this goes on the wire.
*
* Whitelisted rather than flattened to plain text, and that is the choice
* worth defending. Normalising to text/plain is what several clients do and
* it is simpler — it also throws away the thing people are usually pasting
* FOR. A list stays a list, a link stays a link, bold stays bold. What goes
* is everything presentational: style, class, id, width, bgcolor, font,
* every alignment attribute.
*
* Shift-paste is left alone. The browser's own "paste as plain text" is
* already the escape hatch, and reimplementing it would take it away from
* the people who know it.
*
* This is a convenience, not a security boundary. The server sanitises
* every composed body on save (MailBodySanitizer::sanitizeComposedBody),
* which is what actually stops a paste carrying script onto the wire — and
* it has to, because nothing here can be relied on to have run.
*/
_pasteMarkup(event) {
const html = event.clipboardData?.getData('text/html');
if (!html) {
return;
}
event.preventDefault();
const doc = new DOMParser().parseFromString(html, 'text/html');
doc.body.querySelectorAll(DROPPED_ON_PASTE).forEach((node) => node.remove());
doc.body.querySelectorAll('*').forEach((node) => {
for (const name of [...node.attributes].map((attribute) => attribute.name)) {
// href, src and alt survive on the elements that mean them. The
// server has the final say on the schemes.
const keep = ('href' === name && 'A' === node.tagName)
|| (('src' === name || 'alt' === name) && 'IMG' === node.tagName);
if (false === keep) {
node.removeAttribute(name);
}
}
});
// insertHTML rather than writing to the node: it keeps this a single
// undoable step, so ctrl+z after a paste undoes the paste rather than
// the last thing typed before it.
document.execCommand('insertHTML', false, doc.body.innerHTML);
}
/** Same again for a file dragged onto the body. */
_handleBodyDrop(event) {
const files = Array.from(event.dataTransfer?.files ?? [])
@@ -1,6 +1,7 @@
import { Controller } from "@hotwired/stimulus";
import { jsonCsrfHeaders } from "../../csrf.js";
import { announceWrite } from "../../mail_writes.js";
import { askConfirm } from "../../confirm.js";
/**
* Handles per-row status actions in the message list.
@@ -53,6 +54,33 @@ export default class extends Controller {
await this.#post(trashUrl);
}
/**
* Delete for good — offered only on mail already in the trash or in spam.
*
* Asks here rather than through `data-turbo-confirm`, which Turbo only
* consults for links and form submissions. This posts with fetch(), so the
* attribute would have sat on the button looking like a confirmation and
* never been read — the row would simply have vanished for good on the
* first click.
*/
async purge(event) {
const { purgeUrl, purgeConfirm } = event.params;
event.stopPropagation();
if (false === await askConfirm(purgeConfirm)) {
return;
}
await this.#post(purgeUrl);
}
/** Back to the inbox from the trash or from spam. */
async restore(event) {
const { restoreUrl } = event.params;
event.stopPropagation();
await this.#post(restoreUrl);
}
/**
* The wake time arrives as a param, set by the snooze menu — see
* mail--snooze-menu, which computes it in the browser because the server
@@ -0,0 +1,86 @@
import { Controller } from "@hotwired/stimulus";
/**
* The application's confirmation dialog, and the thing Turbo asks instead of
* asking the browser.
*
* See templates/_partials/_confirm_dialog.html.twig for why the browser dialog
* had to go and why this is a <dialog>.
*
* The controller's whole job is to turn a modal dialog into a promise: ask()
* shows it, and resolves true or false when it closes. Everything else —
* focus, Escape, inertness, restoring focus to whatever was focused before — is
* the platform's.
*/
export default class extends Controller {
static targets = ["message", "confirm", "cancel"];
connect() {
// Registered on the element so assets/confirm.js can find whichever
// instance is on the page, without either knowing about the other.
this.element.plConfirm = (message) => this.ask(message);
}
disconnect() {
delete this.element.plConfirm;
// A dialog left open across a Turbo body swap would be showing over the
// next page with nothing left to answer it.
this._settle(false);
}
/**
* @param {string} message
* @returns {Promise<boolean>}
*/
ask(message) {
// A second question while one is already up: answer the first with "no"
// rather than leaving its promise dangling forever. Two confirmations
// at once is not a state the app produces today, and a promise that
// never settles is the kind of thing that only shows up as a button
// that has silently stopped working.
this._settle(false);
this.messageTarget.textContent = message ?? "";
return new Promise((resolve) => {
this._resolve = resolve;
this.element.addEventListener(
"close",
() => {
const confirmed = "confirm" === this.element.returnValue;
this._resolve = null;
resolve(confirmed);
},
{ once: true },
);
// Cleared first: returnValue persists between openings, so a dialog
// dismissed with Escape would otherwise report whatever button was
// pressed the previous time.
this.element.returnValue = "";
this.element.showModal();
// Cancel focused rather than Continue. Every caller of this is
// destructive — that is what a confirmation is for — so the safe
// answer is the one a stray Enter or Space lands on.
this.cancelTarget.focus();
});
}
_settle(answer) {
if (!this._resolve) {
return;
}
const resolve = this._resolve;
this._resolve = null;
if (this.element.open) {
this.element.close();
}
resolve(answer);
}
}
+60
View File
@@ -3694,6 +3694,66 @@ emoji-picker {
font-family: 'Noto Color Emoji', ui-sans-serif, system-ui, sans-serif;
}
/* ── The confirmation dialog ───────────────────────────────────────────────
The box and the dim behind templates/_partials/_confirm_dialog.html.twig.
Here rather than in the class list on the element, because none of it is
something a utility can say.
The geometry first. A <dialog> is centred by the UA stylesheet with
`margin: auto` against `inset: 0`, and Tailwind's preflight zeroes the margin
of every element in the document — including this one. What is left is a
dialog pinned to the top-left corner, sized to fit its content, which is
exactly what it looked like: a browser artefact sitting on the page rather
than a panel in the app. `m-auto` in the template would have put it back, but
it would also have read as decoration rather than as the repair of a reset,
and the next person to tidy the class list would have removed it.
Then the dim. `::backdrop` is painted by the browser and belongs to no parent
in the document, so the modal's `bg-black/50 backdrop-blur-sm` cannot simply
be repeated on it: Tailwind's blur utilities are assembled from custom
properties, and a browser that does not inherit those into ::backdrop (before
Chrome 122 none did) would paint the dim without the blur. Both values are
therefore written out, and they are the modal's own two numbers so the two
surfaces dim the page by the same amount.
NOT --scrim-alpha and --pane-blur. Those are the user's wallpaper knobs — the
scrim is 0 by default, so a dialog dimming the page with it would frequently
not dim the page at all. What a modal does to the page behind it is the app's
decision, not a preference.
───────────────────────────────────────────────────────────────────────── */
#confirm-dialog {
position: fixed;
inset: 0;
margin: auto;
/* Never edge to edge: 2rem of gutter is what keeps it a panel at 390px
instead of a bar across the phone. */
width: min(28rem, calc(100vw - 2rem));
max-height: calc(100dvh - 2rem);
/* The form owns the padding, so the surface can round without the corners
cutting into it. Scrolls rather than overflows, for the two or three
questions long enough to need it on a short window. */
padding: 0;
overflow-y: auto;
}
#confirm-dialog::backdrop {
background-color: rgb(0 0 0 / 0.5);
backdrop-filter: blur(4px);
-webkit-backdrop-filter: blur(4px);
/* The dim comes up with the panel rather than snapping on. Deliberately
without a fallback: on a browser that does not inherit the motion tokens
into ::backdrop the duration is invalid and resolves to 0s, so the dim
simply appears — which is the same thing reduced motion asks for, and the
right way round to fail. */
animation-name: plmail-fade;
animation-duration: var(--motion-base);
animation-timing-function: var(--motion-ease);
}
/* ═══ Appearance: typography, the mail list, and per-surface density ════════
Everything below is driven by custom properties AppearanceRenderer writes
inline on <html>. That is not a decoration of the existing scheme, it IS the
+5
View File
@@ -96,6 +96,11 @@ framework:
App\Infrastructure\Messaging\Message\SyncAccountMessage: ingest
App\Infrastructure\Messaging\Message\SendMessageMessage: export
App\Infrastructure\Messaging\Message\ApplyImapFlagsMessage: export
# Onto export with every other outbound push, and that placement is
# load-bearing rather than tidy: a purge must not overtake a
# relabel already queued for the same mail, and one queue is what
# guarantees the order.
App\Infrastructure\Messaging\Message\PurgeRemoteMessagesMessage: export
# Unrouted until now, which meant every Gmail label change — archive,
# trash, star, mark read, attach a label — made a live Google API
# call inside the HTTP request the user was waiting on. Its IMAP and
@@ -12,6 +12,8 @@ use App\Repository\Mail\MessageRepository;
use App\Repository\Mail\MessageThreadRepository;
use App\Security\Voter\OwnershipVoter;
use App\Service\Mail\ThreadSnoozeService;
use App\Service\Mail\MailPlacement;
use App\Service\Mail\MessagePurger;
use App\Service\Mail\ThreadStatusUpdater;
use DateTimeImmutable;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
@@ -42,6 +44,8 @@ class ThreadStatusController extends AbstractController
private readonly LabelRepository $labelRepository,
private readonly ThreadStatusUpdater $status,
private readonly ThreadSnoozeService $snoozeService,
private readonly MessagePurger $purger,
private readonly MailPlacement $placement,
) {}
#[Route('/star', name: 'star', methods: ['POST'])]
@@ -81,6 +85,64 @@ class ThreadStatusController extends AbstractController
]);
}
/**
* Out of the trash or spam, back into the inbox.
*
* The bin could be reached and not left. An over-eager delete, or a real
* mail the spam filter got wrong, could only be rescued by finding it and
* attaching Inbox by hand from the label menu — which is not something
* somebody looking at a wrongly binned mail would think to try.
*/
#[Route('/restore', name: 'restore', methods: ['POST'])]
public function restore(Request $request, string $type, int $id): Response
{
$messages = $this->resolveMessages($request, $type, $id);
$this->status->restore($messages);
return $this->renderTurboStream('thread/status/_delete.stream.html.twig', [
$type => 'message' === $type ? $messages[0] : $messages[0]->thread,
]);
}
/**
* Delete for good. The only route in plMail that destroys mail.
*
* Separate from /trash rather than a flag on it, because they are different
* promises: trash moves a conversation and can be undone by moving it back,
* this ends it. A caller that reaches the wrong one of the two by fumbling
* a boolean is a caller that deletes somebody's mail by accident.
*
* Only reachable for mail already in the trash or in spam. Not because the
* operation could not work elsewhere, but because "delete forever" on an
* inbox conversation is a click away from "archive" and there is no undo to
* catch the mistake. The bin is the confirmation step the interface already
* has; this route enforces what the templates already show.
*/
#[Route('/purge', name: 'purge', methods: ['POST'])]
public function purge(Request $request, string $type, int $id): Response
{
$messages = $this->resolveMessages($request, $type, $id);
foreach ($messages as $message) {
if (false === $this->placement->isDiscarded($message)) {
throw $this->createAccessDeniedException(
'Only mail already in the trash or in spam can be deleted for good.',
);
}
}
// Captured before the purge: the row is about to stop existing, and the
// stream that removes it from the list is addressed by its id.
$target = 'message' === $type ? $messages[0]->id : $messages[0]->thread?->id;
$this->purger->purge($messages);
return $this->renderTurboStream('thread/status/_purge.stream.html.twig', [
'targetId' => $target,
]);
}
/**
* Attach or detach a custom label.
* Expects JSON body: { "labelId": 42, "attach": true }
@@ -95,7 +95,11 @@ use Webklex\PHPIMAP\Message as ImapMessage;
#[AsMessageHandler]
final class ApplyImapFlagsHandler
{
private const array MOVE_ACTIONS = ['archive', 'trash', 'move'];
// 'restore' is a move like the other two, and it is spelled out here
// rather than folded into 'move' because the destination is resolved from
// the account's special-use folders below rather than named by the caller
// — the same reason 'archive' and 'trash' are their own actions.
private const array MOVE_ACTIONS = ['archive', 'trash', 'restore', 'move'];
public function __construct(
private readonly MessageRepository $messageRepository,
@@ -182,7 +186,7 @@ final class ApplyImapFlagsHandler
$action = $envelope->action;
$destinationPath = $envelope->destinationPath;
if ('archive' === $action || 'trash' === $action) {
if (true === in_array($action, ['archive', 'trash', 'restore'], true)) {
$destinationPath = $this->resolveDestinationPath($client, $account, $action);
}
@@ -512,11 +516,11 @@ final class ApplyImapFlagsHandler
*/
private function resolveDestinationPath(Client $client, Account $account, string $action): ?string
{
$specialUse = MailboxSpecialUse::TRASH;
if ('archive' === $action) {
$specialUse = MailboxSpecialUse::ARCHIVE;
}
$specialUse = match ($action) {
'archive' => MailboxSpecialUse::ARCHIVE,
'restore' => MailboxSpecialUse::INBOX,
default => MailboxSpecialUse::TRASH,
};
$mailbox = $this->mailboxRepository->findOneBy([
'account' => $account,
@@ -0,0 +1,166 @@
<?php
declare(strict_types=1);
namespace App\Infrastructure\Messaging\Handler;
use App\Domain\Helper\ImapConnectionFactory;
use App\Entity\Mail\Account;
use App\Infrastructure\Messaging\Message\PurgeRemoteMessagesMessage;
use App\Repository\Mail\AccountRepository;
use App\Service\Mail\GmailApiClient;
use App\Service\Mail\GraphApiClient;
use Psr\Log\LoggerInterface;
use Symfony\Component\Messenger\Attribute\AsMessageHandler;
use Throwable;
/**
* Destroy at the provider what plMail has already destroyed locally.
*
* The one propagation handler that cannot look anything up. By the time it
* runs, the rows are gone — that is the whole point of the operation — so
* every address it needs travels in the envelope. See
* PurgeRemoteMessagesMessage for why that shape was necessary rather than
* merely tidy.
*
* ## Failure is per message, and never fatal to the batch
*
* A message that has already been deleted by another client answers 404, and a
* folder somebody renamed is simply not there. Neither is an error worth
* retrying or worth taking the other twenty messages down with, because the
* desired end state — that message not existing — is already true. What is
* worth surfacing is the case that will not fix itself: Gmail refusing for want
* of the full mail scope. GmailApiClient::batchDelete() classifies that as
* unrecoverable, so it lands in the log once instead of retrying until the
* transport gives up.
*
* ## Ordering
*
* Queued from MessagePurger BEFORE the local rows are removed, onto the same
* export queue every other propagation uses — so any relabel already queued for
* these messages is ahead of this and cannot arrive after the delete.
*/
#[AsMessageHandler]
final readonly class PurgeRemoteMessagesHandler
{
public function __construct(
private AccountRepository $accountRepository,
private ImapConnectionFactory $imapConnectionFactory,
private GmailApiClient $gmail,
private GraphApiClient $graph,
private LoggerInterface $logger,
) {
}
public function __invoke(PurgeRemoteMessagesMessage $message): void
{
if (true === $message->isEmpty()) {
return;
}
$account = $this->accountRepository->find($message->accountId);
if (null === $account) {
// The account went with the mail — a disconnect, or a reset. There
// is nothing left to authenticate as, and nothing to be done about
// it, so this is not an error.
$this->logger->info('PurgeRemoteMessages: account is gone, nothing to purge', [
'accountId' => $message->accountId,
]);
return;
}
if ([] !== $message->imapUids) {
$this->purgeImap($account, $message->imapUids);
}
if ([] !== $message->gmailIds) {
$this->gmail->batchDelete($account, $message->gmailIds);
}
if ([] !== $message->graphIds) {
$this->purgeGraph($account, $message->graphIds);
}
}
/**
* \Deleted plus an EXPUNGE, folder by folder.
*
* Expunged per folder rather than per message: EXPUNGE renumbers, and doing
* it once at the end of each folder is both fewer round trips and the
* ordering the protocol is happiest with.
*
* @param array<string, list<int>> $byFolder
*/
private function purgeImap(Account $account, array $byFolder): void
{
$client = $this->imapConnectionFactory->connect($account);
foreach ($byFolder as $path => $uids) {
$folder = $client->getFolder($path);
if (null === $folder) {
$this->logger->warning('PurgeRemoteMessages: folder not found on the server', [
'accountId' => $account->id,
'folder' => $path,
]);
continue;
}
foreach ($uids as $uid) {
try {
// setFetchBody(false): the body is about to stop existing,
// and fetching it to mark it deleted would download every
// attachment of every message being purged.
$folder->messages()
->whereUid($uid)
->setFetchBody(false)
->get()
->first()
?->delete(expunge: false);
} catch (Throwable $e) {
// Already gone is the common case, and it is a success:
// the end state this job wants is that the message does
// not exist.
$this->logger->info('PurgeRemoteMessages: could not delete by UID', [
'accountId' => $account->id,
'folder' => $path,
'uid' => $uid,
'error' => $e->getMessage(),
]);
}
}
// Once per folder, after the whole batch. EXPUNGE renumbers, so
// expunging per message would be both more round trips and a
// sequence the server is entitled to reorder underneath us.
try {
$client->expunge();
} catch (Throwable $e) {
$this->logger->warning('PurgeRemoteMessages: expunge failed', [
'accountId' => $account->id,
'folder' => $path,
'error' => $e->getMessage(),
]);
}
}
}
/** @param list<string> $graphIds */
private function purgeGraph(Account $account, array $graphIds): void
{
foreach ($graphIds as $graphId) {
try {
$this->graph->deleteMessage($account, $graphId);
} catch (Throwable $e) {
$this->logger->info('PurgeRemoteMessages: Graph delete failed', [
'accountId' => $account->id,
'graphId' => $graphId,
'error' => $e->getMessage(),
]);
}
}
}
}
@@ -0,0 +1,55 @@
<?php
declare(strict_types=1);
namespace App\Infrastructure\Messaging\Message;
/**
* Destroy these messages at the provider, after plMail has already forgotten
* them.
*
* ## Why this carries coordinates instead of ids
*
* Every other propagation message names local Message rows and lets the
* handler look them up — ApplyImapFlagsHandler opens with
* `findBy(['id' => …])`, ApplyGmailLabelsHandler collects gmailIds the same
* way. That works because those operations leave the row in place.
*
* A purge does not. The row is gone before the worker runs, so a handler that
* looked it up would find nothing, log "no messages found" and return —
* leaving the mail on the server forever, to reappear at the next sync as a
* message the user had explicitly destroyed. That is the failure this shape
* exists to prevent, and it is invisible in testing against a fake provider.
*
* So the address of each message travels with the job: the IMAP folder and UID,
* the Gmail id, the Graph id. Once dispatched, this envelope is self-contained
* and does not care what the database says.
*
* ## Why one message for three providers
*
* An account is one provider — the three lists are never all populated — but
* the purge that produced them is one user action over a selection that may
* span accounts. Keeping it as one message type keeps the ordering property
* that matters: whatever plMail queued for these messages earlier is already
* on the queue ahead of this, so a relabel cannot arrive after the delete.
*/
final class PurgeRemoteMessagesMessage
{
/**
* @param array<string, list<int>> $imapUids folder path => UIDs in it
* @param list<string> $gmailIds
* @param list<string> $graphIds
*/
public function __construct(
public int $accountId,
public array $imapUids = [],
public array $gmailIds = [],
public array $graphIds = [],
) {
}
public function isEmpty(): bool
{
return [] === $this->imapUids && [] === $this->gmailIds && [] === $this->graphIds;
}
}
+39 -2
View File
@@ -112,11 +112,48 @@ final readonly class LabelChangePropagator
/**
* @param iterable<Message> $messages
*/
/**
* Out of the bin (or out of spam) and back into the inbox.
*
* The counterpart trash() never had. Every provider expresses it as the
* inverse of what put the mail there: Gmail attaches INBOX and drops both
* TRASH and SPAM, IMAP moves the message back into the account's inbox
* folder, Graph moves it into the inbox.
*
* Both Gmail labels are removed rather than only the one it was found
* under, because a spam mail somebody then deleted carries both — and one
* that came back to the inbox still marked SPAM would be filed away again
* by Gmail without plMail doing anything.
*
* @param iterable<Message> $messages
*/
public function restore(iterable $messages): void
{
$this->dispatchFlags($messages, 'restore');
$this->dispatchGmail($messages, ['add' => ['INBOX'], 'remove' => ['TRASH', 'SPAM']]);
$this->dispatchGraph($messages, fn (Account $account): ?int => $this->graphDestinationLabelId($account, LabelRole::Inbox));
}
/**
* Discarding a draft — get rid of this copy wherever it exists.
*
* Named delete() and reached only from ComposeController::discard(). It is
* NOT plMail's permanent delete: that is MessagePurger, which cannot use
* this and says why. The difference is that a discard leaves the local row
* to be removed by its own caller, so a handler here can still look the
* message up — a purge removes it first, and a handler that looked it up
* would find nothing and leave the mail on the server forever.
*
* Gmail gets TRASH rather than a real delete, and for a discarded draft
* that is right rather than a compromise: the draft was never sent, Gmail's
* bin is where its own client puts a discarded one, and it is recoverable
* for the user who did not mean it.
*
* @param iterable<Message> $messages
*/
public function delete(iterable $messages): void
{
$this->dispatchFlags($messages, 'delete');
// Gmail: permanent delete requires the full mail scope; TRASH is the
// Gmail-native equivalent of what the UI exposes.
$this->dispatchGmail($messages, ['add' => ['TRASH'], 'remove' => []]);
$this->dispatchGraph($messages, fn (Account $account): ?int => $this->graphDestinationLabelId($account, LabelRole::Trash));
}
+35
View File
@@ -649,6 +649,41 @@ final class GmailApiClient
$this->assertSuccess($response, 'messages.batchModify');
}
/**
* Destroy these messages at Google, for good.
*
* `messages.batchDelete` is not `messages.trash`: there is no undo, the mail
* does not appear in Bin, and Gmail's own interface warns before offering
* it. plMail only reaches this from an explicit "delete forever", never
* from the ordinary Delete — see MessagePurger.
*
* It needs the full https://mail.google.com/ scope. An account connected
* before that scope was requested, or one where the user declined it, gets
* a 403 with reason `insufficientPermissions`, which assertSuccess()
* classifies as unrecoverable — so it fails loudly into the log rather than
* retrying forever, and the caller decides what to tell the user. Quietly
* falling back to trash() would be worse than either: the message would
* still be at Google while plMail had already destroyed its own copy and
* told the user it was gone.
*
* @param list<string> $gmailMessageIds
*/
public function batchDelete(Account $account, array $gmailMessageIds): void
{
if (count($gmailMessageIds) === 0) {
return;
}
$token = $this->tokenManager->getValidAccessToken($account);
$response = $this->httpClient->request('POST', self::BASE . '/messages/batchDelete', [
'auth_bearer' => $token,
'json' => ['ids' => $gmailMessageIds],
]);
$this->assertSuccess($response, 'messages.batchDelete');
}
// ── Failure handling ──────────────────────────────────────────────────────
/**
+18
View File
@@ -409,6 +409,24 @@ final class GraphApiClient
])->getContent();
}
/**
* Destroy a message, for good.
*
* Graph's DELETE is a permanent delete — it is not the same as moving to
* the Deleted Items folder, which is what moveMessage() into the
* Trash-role folder does. plMail only reaches this from an explicit
* "delete forever"; see MessagePurger.
*
* A 404 is left to the caller rather than swallowed here: for a purge it
* means somebody else already deleted it, which is a success, but this
* client has no way to know that is the context.
*/
public function deleteMessage(Account $account, string $messageId): void
{
$this->request($account, 'DELETE', self::ME . '/messages/' . rawurlencode($messageId))
->getContent();
}
/**
* Move a message to another folder. Returns the moved resource — whose id
* is unchanged when immutable ids are in play, and different when they
+83
View File
@@ -0,0 +1,83 @@
<?php
declare(strict_types=1);
namespace App\Service\Mail;
use App\Domain\Enum\Mail\LabelRole;
use App\Entity\Mail\Message;
use App\Entity\Mail\MessageThread;
/**
* Where a conversation currently sits, and therefore which actions mean
* anything on it.
*
* Two reports, one cause. Deleting a mail already in the trash took the row off
* the list while the mail stayed exactly where it was — trashing is "add the
* Trash label", and adding a label a message already carries is a no-op that
* the list nevertheless treated as a change. And spam and trash both offered
* Archive and Snooze, which is the same question from the other side: what
* would archiving something out of the bin do?
*
* Read from the conversation's own labels rather than from which list the user
* clicked in. A thread reached from a search result, from a live update or by
* URL has no list behind it, and a check against the current route would go
* quietly back to offering the wrong controls in exactly those cases. What the
* mail carries is true everywhere.
*
* A service rather than only a Twig function, because the templates are not the
* only thing that has to agree with it: /status/{type}/{id}/purge refuses
* anything this does not call discarded, and an interface that hides a control
* while the route behind it stays open is not a rule, it is a habit.
*/
final class MailPlacement
{
public const string TRASH = 'trash';
public const string SPAM = 'spam';
public const string NORMAL = 'normal';
/**
* Trash wins over Spam when something carries both, because deleting is the
* more final of the two and the controls should describe the more final
* one.
*
* @return self::TRASH|self::SPAM|self::NORMAL
*/
public function of(Message|MessageThread|null $entity): string
{
if (null === $entity) {
return self::NORMAL;
}
$roles = [];
foreach ($entity->labels as $label) {
if (null !== $label->role) {
$roles[] = $label->role;
}
}
if (in_array(LabelRole::Trash, $roles, true)) {
return self::TRASH;
}
if (in_array(LabelRole::Spam, $roles, true)) {
return self::SPAM;
}
return self::NORMAL;
}
/**
* Already thrown away once — the only state from which mail may be deleted
* for good.
*
* Not because a purge could not work elsewhere, but because "delete
* forever" one click from "archive" has no undo to catch the mistake. The
* bin IS the confirmation step, and it is one the user has already taken.
*/
public function isDiscarded(Message|MessageThread|null $entity): bool
{
return self::NORMAL !== $this->of($entity);
}
}
+265
View File
@@ -0,0 +1,265 @@
<?php
declare(strict_types=1);
namespace App\Service\Mail;
use App\Domain\Helper\AttachmentStorageHelper;
use App\Domain\Helper\RawMessageStorage;
use App\Entity\Mail\Account;
use App\Entity\Mail\Message;
use App\Entity\Mail\MessageThread;
use App\Infrastructure\Messaging\Message\PurgeRemoteMessagesMessage;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Component\Messenger\MessageBusInterface;
use Psr\Log\LoggerInterface;
/**
* Deleting mail for good — the only operation in plMail that destroys
* something rather than moving it.
*
* Everything else here is a label change wearing a verb. Archive removes Inbox,
* trash adds Trash, and every one of them is undoable by adding the label back.
* This one is not, which is why it exists as its own service with its own name
* rather than as another method on ThreadStatusUpdater: nothing should reach
* this by accident, and a reader of a controller should be able to see which of
* the two kinds of delete they are looking at.
*
* ## Why this had to be written rather than wired up
*
* Most of it was already here and none of it was connected.
* LabelChangePropagator::delete() existed and had no callers;
* ApplyImapFlagsHandler already implemented `'delete' => $imapMessage->delete(
* expunge: true)` and carried a comment saying the row was "on its way to
* having no row at all"; MailChangeRecorder::emailDestroyed() was written and
* unused. What was missing was the part that actually removes the local copy,
* and without it the UI could only offer a Delete that moved things to the bin
* — including for mail that was already in the bin, which is how a report of
* "deleting a mail in the trash makes it vanish from the list and stay in the
* trash" arrives.
*
* ## Order, which is the only subtle thing here
*
* The provider goes first, while the row still knows where the message lives:
* the IMAP UID, the Gmail id, the Graph id. Delete the row first and there is
* nothing left to name the message with, so it stays on the server forever and
* comes back on the next sync — the bug this ordering exists to prevent, and
* the reason the propagator call is not at the end where it would read more
* naturally.
*
* The dispatch is asynchronous (it goes onto the export queue), so a provider
* that is unreachable does not stop the local delete. That is deliberate: the
* alternative is refusing to delete anything while an account is offline, and
* the queue retries.
*
* ## What "for good" includes
*
* The row, its parts, the attachment files, and the raw source. Leaving the
* files behind would make the deletion a lie in the way that matters most —
* the bytes of the mail would still be on the disk, which for a mail client is
* the thing a person deleting a message is usually trying to prevent.
*/
final readonly class MessagePurger
{
public function __construct(
private EntityManagerInterface $entityManager,
private MessageBusInterface $bus,
private MailChangeRecorder $changes,
private AttachmentStorageHelper $attachments,
private RawMessageStorage $raw,
private LoggerInterface $logger,
) {
}
/**
* Destroy these messages, locally and at the provider.
*
* @param list<Message> $messages
*
* @return int how many rows were removed
*/
public function purge(array $messages): int
{
if ([] === $messages) {
return 0;
}
// Before anything is removed, and carrying the addresses rather than
// the ids. LabelChangePropagator would have been the obvious route —
// its delete() has existed unused since before this was written — but
// every propagation message names local rows and lets the handler look
// them up, and a purge is the one operation where those rows will not
// be there when the worker runs. See PurgeRemoteMessagesMessage.
$this->dispatchRemotePurge($messages);
$threads = [];
$purged = 0;
foreach ($messages as $message) {
// Through the thread when the message has no account of its own,
// which is the same two-association ownership path
// MessageRepository::findOneForAccountByMessageId spells out.
$account = $message->account ?? $message->thread?->account;
if (null !== $message->thread) {
$threads[spl_object_id($message->thread)] = $message->thread;
}
// Recorded while the id is still readable, and before the flush
// that invalidates it. A JMAP client that never learns a message
// was destroyed keeps showing it.
if ($account instanceof Account && null !== $message->id) {
$this->changes->emailDestroyed(
(int) $account->id,
(string) $message->id,
$message->thread,
);
}
$this->removeFiles($message);
// The parts explicitly: Message::$messageParts is a plain
// OneToMany with neither cascade nor orphanRemoval, so removing the
// message alone leaves rows pointing at one that no longer exists.
// Doctrine notices before the database does — "a new entity was
// found through the relationship" — which is a confusing way to be
// told about a foreign key.
//
// Deliberately not fixed by adding cascade to the mapping. That
// would change what happens on every other path that touches a
// message, including the syncers that detach and re-attach parts,
// and this is the only place that wants a part destroyed.
foreach ($message->messageParts as $part) {
$this->entityManager->remove($part);
}
$this->entityManager->remove($message);
++$purged;
}
$this->entityManager->flush();
$this->reconcileThreads($threads);
$this->entityManager->flush();
return $purged;
}
/**
* Queue the provider-side delete, one job per account.
*
* Grouped by account because an envelope authenticates as one account, and
* split by the coordinate each provider understands: IMAP wants a folder
* and a UID, Gmail its own message id, Graph its own. A message with none
* of them — a draft that never reached a server, or one whose account has
* gone — simply contributes nothing, and the job is not sent at all if the
* whole batch is like that.
*
* @param list<Message> $messages
*/
private function dispatchRemotePurge(array $messages): void
{
/** @var array<int, array{imap: array<string, list<int>>, gmail: list<string>, graph: list<string>}> $byAccount */
$byAccount = [];
foreach ($messages as $message) {
$account = $message->account ?? $message->thread?->account;
if (null === $account || null === $account->id) {
continue;
}
$accountId = (int) $account->id;
$byAccount[$accountId] ??= ['imap' => [], 'gmail' => [], 'graph' => []];
$folder = $message->mailbox?->fullPath;
if (null !== $message->imapUid && null !== $folder) {
$byAccount[$accountId]['imap'][$folder][] = $message->imapUid;
}
if (null !== $message->gmailId && '' !== $message->gmailId) {
$byAccount[$accountId]['gmail'][] = $message->gmailId;
}
if (null !== $message->graphId && '' !== $message->graphId) {
$byAccount[$accountId]['graph'][] = $message->graphId;
}
}
foreach ($byAccount as $accountId => $coordinates) {
$job = new PurgeRemoteMessagesMessage(
$accountId,
$coordinates['imap'],
$coordinates['gmail'],
$coordinates['graph'],
);
if (false === $job->isEmpty()) {
$this->bus->dispatch($job);
}
}
}
/**
* The bytes on disk, which the database rows do not take with them.
*
* Failures are logged and swallowed rather than aborting the delete. A file
* that has already gone, or one on a volume that is momentarily unwritable,
* must not leave the user with a message they asked to destroy still in
* their mailbox — the row going is the part they can see, and a leftover
* file is a cleanup problem rather than a correctness one.
*/
private function removeFiles(Message $message): void
{
foreach ($message->messageParts as $part) {
try {
$this->attachments->delete($part->storagePath);
} catch (\Throwable $e) {
$this->logger->warning('MessagePurger: could not delete an attachment', [
'part' => $part->id,
'error' => $e->getMessage(),
]);
}
}
try {
$this->raw->delete($message->rawPath);
} catch (\Throwable $e) {
$this->logger->warning('MessagePurger: could not delete a raw source', [
'message' => $message->id,
'error' => $e->getMessage(),
]);
}
}
/**
* A conversation that has lost messages, and one that has lost all of them.
*
* messageCount is recounted from the association rather than decremented,
* which is the rule DraftPersister and ComposeController::discard() already
* state: the stored counter is what the thread header renders, and anything
* that adjusts it by arithmetic drifts.
*
* An empty thread is removed outright. Leaving it would put a conversation
* with no messages in the list — a row with a subject, no participants and
* a count of zero, which nothing else in the app knows how to render.
*
* @param array<int, MessageThread> $threads
*/
private function reconcileThreads(array $threads): void
{
foreach ($threads as $thread) {
$remaining = $thread->messages->count();
if (0 === $remaining) {
$this->entityManager->remove($thread);
continue;
}
$thread->messageCount = $remaining;
}
}
}
+48
View File
@@ -186,6 +186,54 @@ final readonly class ThreadStatusUpdater
$this->finish($messages);
}
/**
* Out of the trash or spam and back into the inbox.
*
* The counterpart the bin never had. Trash and Spam could be reached and
* not left: an over-eager delete, or a real mail the filter got wrong,
* could only be rescued by finding it and attaching Inbox by hand through
* the label menu — which is not something a person looking at a wrongly
* binned mail would think to do.
*
* Both roles are removed rather than only the one it was found under. A
* message that carries Trash AND Spam — a spam mail somebody then deleted —
* would otherwise come back into the inbox still marked as spam, and
* disappear again on the next rule run.
*
* @param list<Message> $messages
*/
public function restore(array $messages): void
{
$account = $this->accountOf($messages[0]);
$inboxLabel = $this->labelResolver->systemLabel(LabelRole::Inbox, $account);
$trashLabel = $this->labels->findOneByRoleForUser(LabelRole::Trash, $account->usr);
$spamLabel = $this->labels->findOneByRoleForUser(LabelRole::Spam, $account->usr);
// Before the mailbox is re-pointed, so the IMAP job still knows which
// folder to move the message out of — the same ordering archive() and
// trash() rely on, and for the same reason.
$this->propagator->restore($messages);
$inboxMailbox = $inboxLabel->bindingFor($account)?->mailbox;
foreach ($messages as $message) {
$message->addLabel($inboxLabel);
foreach ([$trashLabel, $spamLabel] as $label) {
if (null !== $label) {
$message->removeLabel($label);
}
}
if (null !== $message->imapUid && null !== $inboxMailbox) {
$message->relocateTo($inboxMailbox);
}
}
$this->finish($messages);
}
/**
* Attach or detach a custom label across the set.
*
+32
View File
@@ -0,0 +1,32 @@
<?php
declare(strict_types=1);
namespace App\Twig;
use App\Service\Mail\MailPlacement;
use Twig\Extension\AbstractExtension;
use Twig\TwigFunction;
/**
* `mail_placement(thread)` for the templates that decide which controls to
* draw.
*
* A pass-through to the service on purpose. The purge route refuses anything
* MailPlacement does not call discarded, so the templates and the controller
* have to be answering the same question — and the way to guarantee that is for
* there to be one answer rather than two implementations that agree today.
*/
final class MailPlacementExtension extends AbstractExtension
{
public function __construct(private readonly MailPlacement $placement)
{
}
public function getFunctions(): array
{
return [
new TwigFunction('mail_placement', $this->placement->of(...)),
];
}
}
+8 -1
View File
@@ -245,7 +245,14 @@
with. Reading `app.flashes` here also CLEARS the bag, which is what stops
the message reappearing on the next navigation.
#}
{% include '_partials/_session_expired.html.twig' %}
{# Only for somebody who HAS a session to lose. Besides being meaningless
on the login and install screens, the bar carries a link to /login — and
the install flow asserts that a freshly restored instance with no users
offers no way to sign in, because there is nobody to sign in as. #}
{% if app.user %}
{% include '_partials/_session_expired.html.twig' %}
{% endif %}
{% include '_partials/_confirm_dialog.html.twig' %}
<div id="toast-region" class="fixed bottom-4 left-4 z-[60] flex flex-col gap-2 items-end">
{% for type, messages in app.flashes(['success', 'error', 'info']) %}
@@ -0,0 +1,93 @@
{#
The dialog behind every `data-turbo-confirm` in the application.
Turbo's confirmation calls `window.confirm()` unless something says otherwise,
and the browser dialog is the one piece of UI the app cannot style, cannot
translate consistently, cannot theme, and which announces the origin at the top
as though the page were doing something suspicious. Twenty-two places used it —
deleting a label, running a filter over existing mail, resetting the database —
and each looked like a security prompt rather than part of plMail.
One dialog for all of them, and it is `Turbo.setConfirmMethod` that connects the
two (assets/confirm.js), so this is not twenty-two changes: any
`data-turbo-confirm` written after today gets it for free, which is the only
version of this fix that stays fixed.
`<dialog>` rather than a div. showModal() puts the element in the browser's top
layer, which is above every stacking context on the page — so this cannot be
trapped behind a pane the way the app's other popovers can be (the panes use
backdrop-filter, which makes them a containing block for fixed positioning too).
It also brings focus trapping, restoring focus to the trigger on close, the
Escape key, and inertness of the rest of the page, all of which would otherwise
be hand-written.
It looks like the app's modal (_modal.html.twig) because it is the same kind of
object: one continuous surface, spacing rather than rules to separate the title
from the question, actions bottom-right, and the two footer buttons copied from
_modal_form.html.twig rather than re-invented a fifth time.
What it CANNOT borrow from the modal is the box itself. The modal draws its own
backdrop element and centres its panel with flexbox inside it; a <dialog> has
neither — the browser paints ::backdrop, and the centring is the UA's
`margin: auto`, which Tailwind's preflight zeroes along with every other margin
in the document. That is why the geometry and the dim live in app.css under
`#confirm-dialog` and not in the class list here: see the block there.
#}
<dialog id="confirm-dialog"
data-controller="ui--confirm"
aria-labelledby="confirm-dialog-title"
{# Same entrance as the modal panel, for the same reason: this is the
largest thing that ever appears over the page, and a whole screen
changing state in one frame is the jump cut motion exists to answer.
Safe to declare on markup that is in the document from first paint,
because a closed <dialog> is display:none and a display:none element
has no running animation — the entrance starts at showModal() and
again on every open after that. Reduced motion is handled where it
is handled for everything else, by motion.css zeroing the tokens. #}
data-enter="pop"
class="pane-flat shadow-float text-ink">
<form method="dialog" class="flex flex-col gap-4 p-6">
<h2 id="confirm-dialog-title" class="text-base font-semibold text-ink">
{{ 'confirm_dialog.heading'|trans }}
</h2>
{# `whitespace-pre-line` because the question is plain text written into
a `data-turbo-confirm` attribute, and several of them are two
sentences with a line break between them. #}
<p class="text-sm leading-relaxed text-ink-soft whitespace-pre-line"
data-ui--confirm-target="message"></p>
<div class="flex items-center justify-end gap-2 pt-1">
{# `value` is what <form method="dialog"> hands back as returnValue,
which is how the controller learns which button was pressed
without a listener on either. #}
<button type="submit" value="cancel"
data-ui--confirm-target="cancel"
class="inline-flex items-center justify-center h-8 px-3.5 rounded-lg text-sm font-medium
text-ink-soft
bg-field
border border-field
hover:bg-hover
focus:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-1
transition-colors duration-100">
{{ 'confirm_dialog.cancel'|trans }}
</button>
{# Accent rather than red, even though every caller is destructive.
The red belongs to the control that OPENED this — the delete
button on the row — and repeating it here would leave the two
weightiest colours on screen sitting beside a quiet Cancel, which
is how a confirmation stops being read at all. The safety is that
Cancel holds the focus, not that Continue is frightening. #}
<button type="submit" value="confirm"
data-ui--confirm-target="confirm"
class="inline-flex items-center justify-center h-8 px-3.5 rounded-lg text-sm font-medium
text-accent-ink
bg-accent hover:bg-accent-strong active:bg-accent-strong
focus:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-1
transition-colors duration-100">
{{ 'confirm_dialog.confirm'|trans }}
</button>
</div>
</form>
</dialog>
+41 -2
View File
@@ -34,6 +34,10 @@ same colour on every row forever, and an account-scoped list already says which
account it is at the top. `unified` defaults to true because most lists are —
the account view passes false.
#}
{# Where this conversation sits, for the hover actions below. Read from the
mail's own labels rather than from the route, so a row reached from a search
result or a live update answers the same as one in the folder listing. #}
{% set placement = mail_placement(item) %}
{% set unified = unified|default(true) %}
{% set rowAccount = item.account ?? null %}
{% set showAccountDot = unified and rowAccount is not null and accounts|length > 1 %}
@@ -513,7 +517,7 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
</span>
<div class="hidden @xl:flex absolute inset-0 items-center justify-end invisible group-hover:visible pointer-events-auto gap-1">
{% if false == draft %}
{% if false == draft and placement == 'normal' %}
<button type="button" data-action="click->mail--message-row#archive"
data-mail--message-row-archive-url-param="{{ path('app_status_archive', {'type': type, 'id': rowId}) }}"
aria-label="{{ 'thread_row.actions.archive'|trans }}"
@@ -523,6 +527,11 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
</button>
{% endif %}
{# Hidden in spam and the trash. Trashing something already
there adds a label it already carries — nothing happens on
the server — while the row left the list anyway, so the mail
looked deleted and was not. #}
{% if placement == 'normal' %}
<button type="button" data-action="click->mail--message-row#trash"
data-mail--message-row-trash-url-param="{{ path('app_status_trash', {'type': type, 'id': rowId}) }}"
aria-label="{{ 'thread_row.actions.delete'|trans }}"
@@ -530,8 +539,38 @@ Threads only. A bare Message row is a message, and only a conversation arrives.
class="z-10 p-1.5 cursor-pointer rounded-lg text-gray-400 hover:text-red-600 hover:bg-gray-200 dark:hover:bg-gray-700 transition-colors">
<i class="fa-solid fa-trash w-4 h-4" aria-hidden="true"></i>
</button>
{% endif %}
{% if false == draft %}
{# In the bin and in spam, the two actions that DO mean
something there. Restore is the counterpart the bin never
had: until it existed, mail could be thrown away and not
fetched back except by attaching Inbox by hand from the
label menu. #}
{% if placement != 'normal' %}
<button type="button" data-action="click->mail--message-row#restore"
data-mail--message-row-restore-url-param="{{ path('app_status_restore', {'type': type, 'id': rowId}) }}"
aria-label="{{ 'discarded.actions.restore'|trans }}"
title="{{ 'discarded.actions.restore'|trans }}"
class="z-10 p-1.5 cursor-pointer rounded-lg text-gray-400 hover:text-gray-700 dark:hover:text-gray-200 hover:bg-gray-200 dark:hover:bg-gray-700 transition-colors">
<i class="fa-solid fa-inbox w-4 h-4" aria-hidden="true"></i>
</button>
{# The question travels as a param, not as
data-turbo-confirm: Turbo only consults that attribute
for links and form submissions, and this posts with
fetch(). The controller asks the application's dialog
itself — see mail--message-row#purge. #}
<button type="button" data-action="click->mail--message-row#purge"
data-mail--message-row-purge-url-param="{{ path('app_status_purge', {'type': type, 'id': rowId}) }}"
data-mail--message-row-purge-confirm-param="{{ 'discarded.actions.purge_confirm'|trans }}"
aria-label="{{ 'discarded.actions.purge'|trans }}"
title="{{ 'discarded.actions.purge'|trans }}"
class="z-10 p-1.5 cursor-pointer rounded-lg text-gray-400 hover:text-danger hover:bg-gray-200 dark:hover:bg-gray-700 transition-colors">
<i class="fa-solid fa-fire w-4 h-4" aria-hidden="true"></i>
</button>
{% endif %}
{% if false == draft and placement == 'normal' %}
{# The button opens a menu rather than snoozing directly:
there is no defensible default wake time, and the one
this had (none at all) cleared the snooze instead of
+1 -1
View File
@@ -96,7 +96,7 @@ not a way into a mailbox.
<form method="post"
action="{{ path('app_admin_user_default_delete', { id: user.id }) }}"
data-turbo-frame="_top"
onsubmit="return confirm('{{ 'admin.users.delete_confirm'|trans({'%name%': user.name})|e('js') }}')">
data-turbo-confirm="{{ 'admin.users.delete_confirm'|trans({'%name%': user.name}) }}">
<input type="hidden" name="_token" value="{{ csrf_token('admin-user-delete-' ~ user.id) }}">
<button type="submit"
class="shrink-0 h-8 w-8 flex items-center justify-center rounded-lg text-ink-faint
+17 -12
View File
@@ -97,19 +97,24 @@ Variables:
{{ 'message.actions.mark_unread'|trans }}
</button>
<button type="button"
data-action="click->mail--message-actions#archive"
class="w-full flex items-center gap-2 px-4 py-2 text-left text-ink-soft hover:bg-hover transition-colors cursor-pointer">
<i class="fa-solid fa-box-archive w-4 text-ink-faint" aria-hidden="true"></i>
{{ 'message.actions.archive'|trans }}
</button>
{# Not in spam or the trash: archiving out of the bin means nothing, and
deleting something already deleted is a no-op the list used to
report as a change. See App\Twig\MailPlacementExtension. #}
{% if mail_placement(message) == 'normal' %}
<button type="button"
data-action="click->mail--message-actions#archive"
class="w-full flex items-center gap-2 px-4 py-2 text-left text-ink-soft hover:bg-hover transition-colors cursor-pointer">
<i class="fa-solid fa-box-archive w-4 text-ink-faint" aria-hidden="true"></i>
{{ 'message.actions.archive'|trans }}
</button>
<button type="button"
data-action="click->mail--message-actions#trash"
class="w-full flex items-center gap-2 px-4 py-2 text-left text-danger hover:bg-hover transition-colors cursor-pointer">
<i class="fa-solid fa-trash w-4" aria-hidden="true"></i>
{{ 'message.actions.delete'|trans }}
</button>
<button type="button"
data-action="click->mail--message-actions#trash"
class="w-full flex items-center gap-2 px-4 py-2 text-left text-danger hover:bg-hover transition-colors cursor-pointer">
<i class="fa-solid fa-trash w-4" aria-hidden="true"></i>
{{ 'message.actions.delete'|trans }}
</button>
{% endif %}
<div class="border-t border-line my-1"></div>
+13 -2
View File
@@ -33,8 +33,19 @@ self-contained: no {% extends %}, no block dependencies.
data-mail--message-actions-entity-id-value="{{ thread.id }}"
data-mail--message-actions-entity-type-value="thread"
>
{{ tbtn('click->mail--message-actions#archive', 'fa-solid fa-box-archive', 'toolbar.actions.archive'|trans) }}
{{ tbtn('click->mail--message-actions#trash', 'fa-solid fa-trash', 'toolbar.actions.delete'|trans) }}
{# Archive and Delete are hidden where they would do nothing.
Trashing something already in the trash adds a label it already
carries — a no-op that nonetheless took the row off the list, so the
mail appeared to be deleted twice and was still there both times. And
archiving out of the bin is not a thing anyone means. See
App\Twig\MailPlacementExtension. #}
{% set placement = mail_placement(thread) %}
{% if placement == 'normal' %}
{{ tbtn('click->mail--message-actions#archive', 'fa-solid fa-box-archive', 'toolbar.actions.archive'|trans) }}
{{ tbtn('click->mail--message-actions#trash', 'fa-solid fa-trash', 'toolbar.actions.delete'|trans) }}
{% endif %}
{{ tbtn('click->mail--message-actions#markUnread', 'fa-regular fa-envelope', 'toolbar.actions.mark_unread'|trans) }}
</div>
+1 -1
View File
@@ -65,7 +65,7 @@
<form method="post" action="{{ path('app_api_token_revoke', { id: token.id }) }}"
data-turbo-stream class="shrink-0"
onsubmit="return confirm('{{ 'settings.app_passwords.revoke_confirm'|trans({ '%name%': token.name })|e('js') }}')">
data-turbo-confirm="{{ 'settings.app_passwords.revoke_confirm'|trans({ '%name%': token.name }) }}">
<input type="hidden" name="_token" value="{{ csrf_token('app-password-revoke' ~ token.id) }}">
<button type="submit"
class="text-[11px] text-ink-faint hover:text-red-400 transition-colors cursor-pointer">
+1 -1
View File
@@ -101,7 +101,7 @@ a state change was delivered and can never say which mail it was about.
<form method="post"
action="{{ path('app_push_device_remove', { id: device.id }) }}"
data-turbo-stream class="ml-auto shrink-0"
onsubmit="return confirm('{{ 'settings.notifications.devices.remove_confirm'|trans({ '%device%': device.deviceClientId })|e('js') }}')">
data-turbo-confirm="{{ 'settings.notifications.devices.remove_confirm'|trans({ '%device%': device.deviceClientId }) }}">
<input type="hidden" name="_token" value="{{ csrf_token('push-device-remove' ~ device.id) }}">
<button type="submit"
class="text-[11px] text-ink-faint hover:text-red-400 transition-colors cursor-pointer">
+3 -3
View File
@@ -143,7 +143,7 @@
<form method="post" action="{{ path('app_two_factor_revoke_device', { id: device.id }) }}"
data-turbo="false" class="shrink-0"
onsubmit="return confirm('{{ 'settings.security.devices.revoke_confirm'|trans({ '%name%': device.label })|e('js') }}')">
data-turbo-confirm="{{ 'settings.security.devices.revoke_confirm'|trans({ '%name%': device.label }) }}">
<input type="hidden" name="_token" value="{{ csrf_token('two-factor-revoke-device' ~ device.id) }}">
<button type="submit"
class="text-[11px] text-ink-faint hover:text-red-400 transition-colors cursor-pointer">
@@ -157,7 +157,7 @@
<div class="px-4 py-3 border-t border-line">
<form method="post" action="{{ path('app_two_factor_revoke_all_devices') }}"
data-turbo="false"
onsubmit="return confirm('{{ 'settings.security.devices.revoke_all_confirm'|trans|e('js') }}')">
data-turbo-confirm="{{ 'settings.security.devices.revoke_all_confirm'|trans }}">
<input type="hidden" name="_token" value="{{ csrf_token('two-factor-revoke-all-devices') }}">
<button type="submit"
class="text-[11px] font-medium text-red-400 hover:underline cursor-pointer">
@@ -177,7 +177,7 @@
<form method="post" action="{{ path('app_two_factor_disable') }}"
data-turbo="false" class="flex items-end gap-2"
onsubmit="return confirm('{{ 'settings.security.disable.confirm'|trans|e('js') }}')">
data-turbo-confirm="{{ 'settings.security.disable.confirm'|trans }}">
<input type="hidden" name="_token" value="{{ csrf_token('two-factor-disable') }}">
<div class="flex-1 min-w-0 max-w-[200px]">
<label for="disable-code" class="block text-[11px] text-ink-faint mb-1">
@@ -86,7 +86,7 @@ into a message they can act on.
<form method="post" action="{{ path('app_settings_integrations_delete', { id: integration.id }) }}"
data-turbo-stream
onsubmit="return confirm('{{ 'settings.integrations.disconnect_confirm'|trans({ '%name%': integration.name })|e('js') }}')">
data-turbo-confirm="{{ 'settings.integrations.disconnect_confirm'|trans({ '%name%': integration.name }) }}">
<input type="hidden" name="_token" value="{{ csrf_token('settings-integration-' ~ integration.id) }}">
<button type="submit" class="text-[11px] text-ink-faint hover:text-red-400 transition-colors cursor-pointer">
{{ 'settings.integrations.disconnect'|trans }}
@@ -0,0 +1,6 @@
{# The row is gone for good, so it goes from the list the same way a trashed one
does — the difference is that nothing will bring this one back. Addressed by
an id captured before the purge, because by now the row it named does not
exist. #}
<turbo-stream action="remove" target="thread_{{ targetId }}">
</turbo-stream>
+315
View File
@@ -0,0 +1,315 @@
<?php
declare(strict_types=1);
namespace App\Tests\Service\Mail;
use App\Domain\Enum\Mail\LabelRole;
use App\Domain\Enum\Mail\ThreadingMethod;
use App\Entity\Label\Label;
use App\Entity\Mail\Account;
use App\Entity\Mail\Mailbox;
use App\Entity\Mail\Message;
use App\Entity\Mail\MessagePart;
use App\Entity\Mail\MessageThread;
use App\Entity\User\User;
use App\Infrastructure\Messaging\Message\PurgeRemoteMessagesMessage;
use App\Service\Label\LabelResolver;
use App\Service\Mail\MessagePurger;
use Doctrine\DBAL\Connection;
use Doctrine\ORM\EntityManagerInterface;
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
use Symfony\Component\Messenger\Transport\InMemoryTransport;
/**
* The only operation in plMail that destroys something.
*
* Everything else is a label change wearing a verb: archive removes Inbox,
* trash adds Trash, and each is undone by putting the label back. This one
* cannot be undone, which is why the assertions below are about what is left
* behind rather than about what the user sees.
*
* The one that would be easy to get wrong, and is the reason this class exists
* in the shape it does: the job that tells the provider has to be dispatched
* with the message's ADDRESS on that provider, not with its local row id. Every
* other propagation names rows and lets the handler look them up — which is
* fine while the row survives the operation. Here it does not, so a handler
* that looked it up would find nothing, return quietly, and leave the mail on
* the server to reappear at the next sync as a message the user had explicitly
* destroyed.
*/
final class MessagePurgerTest extends KernelTestCase
{
private EntityManagerInterface $em;
private Connection $connection;
private MessagePurger $purger;
private LabelResolver $labelResolver;
private Account $account;
private Mailbox $inboxMailbox;
protected function setUp(): void
{
self::bootKernel();
$container = self::getContainer();
$this->em = $container->get(EntityManagerInterface::class);
$this->connection = $container->get(Connection::class);
$this->purger = $container->get(MessagePurger::class);
$this->labelResolver = $container->get(LabelResolver::class);
$this->connection->beginTransaction();
$this->account = $this->seedAccount();
$this->inboxMailbox = $this->seedMailbox();
}
protected function tearDown(): void
{
if (true === $this->connection->isTransactionActive()) {
$this->connection->rollBack();
}
parent::tearDown();
}
public function testThePurgedMessageIsGoneFromTheDatabase(): void
{
$thread = $this->thread(2);
$victim = $thread->messages->first();
$id = $victim->id;
$this->purger->purge([$victim]);
self::assertNull($this->em->getRepository(Message::class)->find($id));
}
/**
* The address travels with the job, and this is the assertion the whole
* design turns on.
*
* Dispatching the local row id — as every other propagation does — would
* leave the worker with nothing to look up, because the row is deleted in
* the same call. The mail would stay on the server and come back.
*/
public function testTheProviderJobCarriesTheAddressRatherThanTheRowId(): void
{
$thread = $this->thread(1);
$message = $thread->messages->first();
$this->purger->purge([$message]);
$jobs = $this->dispatched();
self::assertCount(1, $jobs, 'exactly one purge job, for the one account');
self::assertSame(
['INBOX' => [2000]],
$jobs[0]->imapUids,
'the folder and UID must be in the envelope, not looked up later',
);
}
/**
* A message that never reached a server has no address on one, so nothing
* is announced. Without this, discarding a local-only row would queue a job
* that connects to IMAP to delete nothing.
*/
public function testAMessageThatNeverReachedAServerAnnouncesNothing(): void
{
$thread = $this->thread(1);
$message = $thread->messages->first();
$message->imapUid = null;
$message->mailbox = null;
$this->em->flush();
$this->purger->purge([$message]);
self::assertCount(0, $this->dispatched());
}
/**
* The bytes, not just the rows.
*
* Leaving the file behind makes the deletion a lie in the way that matters
* most for a mail client: the message the user destroyed would still be
* readable on the disk.
*/
public function testTheAttachmentFileGoesWithTheMessage(): void
{
$thread = $this->thread(1);
$message = $thread->messages->first();
// The real shape of a stored path. AttachmentStorageHelper::delete()
// refuses anything not under the configured attachments directory —
// which is the guard that stops a crafted storagePath deleting
// arbitrary files — so a fixture with a made-up prefix would be
// silently ignored and this test would pass while deleting nothing.
$relative = 'var/attachments/purge-test/' . uniqid('', true) . '.txt';
$absolute = self::getContainer()->getParameter('kernel.project_dir') . '/' . $relative;
@mkdir(dirname($absolute), 0o775, true);
file_put_contents($absolute, 'attachment bytes');
$part = new MessagePart();
$part->message = $message;
$part->filename = 'note.txt';
$part->contentType = 'text/plain';
$part->disposition = 'attachment';
$part->isInline = false;
$part->storagePath = $relative;
$part->size = 16;
$message->addMessagePart($part);
$this->em->persist($part);
$this->em->flush();
self::assertFileExists($absolute);
$this->purger->purge([$message]);
self::assertFileDoesNotExist($absolute, 'the attachment outlived the message');
}
/**
* A conversation that lost every message goes with them.
*
* Left behind it is a row in the list with a subject, no participants and a
* count of zero — a shape nothing else in the app knows how to render.
*/
public function testAThreadEmptiedByThePurgeIsRemovedToo(): void
{
$thread = $this->thread(1);
$threadId = $thread->id;
$this->purger->purge([$thread->messages->first()]);
self::assertNull($this->em->getRepository(MessageThread::class)->find($threadId));
}
/**
* And one that still has messages keeps its count honest.
*
* Recounted from the association rather than decremented — the rule
* DraftPersister already states, because a stored counter adjusted by
* arithmetic drifts and it is what the thread header renders.
*/
public function testAThreadThatKeepsMessagesKeepsAnHonestCount(): void
{
$thread = $this->thread(3);
$this->purger->purge([$thread->messages->first()]);
self::assertSame(2, $thread->messageCount);
self::assertCount(2, $thread->messages);
}
public function testPurgingNothingDoesNothing(): void
{
self::assertSame(0, $this->purger->purge([]));
self::assertCount(0, $this->dispatched());
}
// ── fixture ──────────────────────────────────────────────────────────────
/** @return list<PurgeRemoteMessagesMessage> */
private function dispatched(): array
{
/** @var InMemoryTransport $transport */
$transport = self::getContainer()->get('messenger.transport.export');
$found = [];
foreach ($transport->getSent() as $envelope) {
$message = $envelope->getMessage();
if ($message instanceof PurgeRemoteMessagesMessage) {
$found[] = $message;
}
}
return $found;
}
private function thread(int $messages): MessageThread
{
$inbox = $this->labelResolver->systemLabel(LabelRole::Inbox, $this->account);
$thread = new MessageThread();
$thread->account = $this->account;
$thread->subject = 'Purge fixture';
$thread->normalizedSubject = 'purge fixture';
$thread->lastMessageAt = new \DateTimeImmutable('-1 hour');
$thread->threadingMethod = ThreadingMethod::References;
$thread->unreadCount = 0;
$thread->messageCount = $messages;
$this->em->persist($thread);
for ($i = 0; $i < $messages; ++$i) {
$message = new Message();
$message->account = $this->account;
$message->subject = sprintf('Purge fixture %d', $i);
$message->fromAddress = 'sender@example.test';
$message->receivedAt = new \DateTimeImmutable('-1 hour');
$message->hasAttachments = false;
$message->messageId = sprintf('<purge-%s-%d@example.test>', uniqid('', true), $i);
// A mailbox and a UID, because a message with neither has no
// address at a provider and the dispatch assertions would then be
// testing the fixture rather than the service.
$message->mailbox = $this->inboxMailbox;
$message->imapUid = 2000 + $i;
$message->addLabel($inbox);
$thread->addMessage($message);
$this->em->persist($message);
}
$this->em->flush();
return $thread;
}
private function seedAccount(): Account
{
$user = new User();
$user->email = 'purger-' . uniqid('', true) . '@example.test';
$user->nameFirst = 'Message';
$user->nameLast = 'Purger';
$user->roles = ['ROLE_USER'];
$user->password = 'x';
$this->em->persist($user);
$email = 'purger-fixture-' . uniqid('', true) . '@example.test';
$account = new Account();
$account->usr = $user;
$account->email = $email;
$account->username = $email;
$account->imapHost = 'localhost';
$account->imapPort = 993;
$account->imapEncryption = 'ssl';
$account->smtpHost = 'localhost';
$account->smtpPort = 587;
$account->smtpEncryption = 'starttls';
$account->password = 'x';
$account->authType = 'password';
$account->isActive = true;
$this->em->persist($account);
$this->em->flush();
return $account;
}
private function seedMailbox(string $name = 'INBOX'): Mailbox
{
$mailbox = new Mailbox();
$mailbox->account = $this->account;
$mailbox->name = $name;
$mailbox->fullPath = $name;
$mailbox->isSyncEnabled = true;
$mailbox->isIdleEnabled = false;
$this->em->persist($mailbox);
$this->em->flush();
return $mailbox;
}
}
+102
View File
@@ -0,0 +1,102 @@
import { test, expect } from "./support/test";
import { seed } from "./support/config";
/**
* Pasted markup arrives as content, not as somebody else's stylesheet.
*
* CODE-REVIEW.md U-02. The composer handled pasted IMAGES and let everything
* else through untouched, so three sentences out of Word or a web page brought
* that document's fonts, background colours, nested tables and class names into
* the body — and, since this is what goes on the wire, the recipient saw all of
* it. The `<style>` block was the worst of them: it restyled the whole composer
* rather than only the text that had just been pasted.
*
* The assertions are two-sided on purpose. Flattening to plain text would pass
* every "no styling survived" check and would also throw away the thing people
* are usually pasting for, so the structure has to be shown surviving in the
* same test that shows the presentation being dropped.
*/
test.beforeEach(() => {
seed("seed-mail");
});
const EDITOR = '[data-compose--compose-toolbar-target="editor"]';
/** Paste real clipboard HTML, the way a browser delivers it. */
async function pasteHtml(page: import("@playwright/test").Page, html: string): Promise<void> {
await page.locator(EDITOR).first().click();
await page.evaluate((markup) => {
const editor = document.querySelector(
'[data-compose--compose-toolbar-target="editor"]',
) as HTMLElement;
const data = new DataTransfer();
data.setData("text/html", markup);
data.setData("text/plain", "fallback");
editor.dispatchEvent(new ClipboardEvent("paste", {
clipboardData: data,
bubbles: true,
cancelable: true,
}));
}, html);
}
test.describe("pasting into the composer", () => {
test("drops the presentation and keeps the content", async ({ page }) => {
await page.goto("/mail/inbox");
await page.getByRole("link", { name: "Compose" }).first().click();
await expect(page.locator(".compose-window").first()).toBeVisible();
await pasteHtml(
page,
'<style>body { background: #ff0000 }</style>'
+ '<div class="WordSection1" style="font-family:Calibri;background:#ffff00" bgcolor="#ffff00" width="600">'
+ '<p style="font-size:48px"><b>Angebot</b> für <a href="https://example.test/x">das Projekt</a></p>'
+ '<ul><li>Erster Punkt</li><li>Zweiter Punkt</li></ul>'
+ '</div>',
);
const body = await page.locator(EDITOR).first().innerHTML();
// The presentation is gone — including the stylesheet, which would
// otherwise have restyled the composer itself.
expect(body).not.toContain("<style");
expect(body).not.toContain("background");
expect(body).not.toContain("font-family");
expect(body).not.toContain("WordSection1");
expect(body).not.toContain("bgcolor");
expect(body).not.toContain("48px");
// The content is not.
expect(body).toContain("Angebot");
expect(body).toContain("<b>");
expect(body).toContain("<ul>");
expect(body).toContain("Zweiter Punkt");
expect(body).toContain("https://example.test/x");
});
/**
* The editor is still an editor afterwards.
*
* A paste handler that replaces the body instead of inserting into it is an
* easy mistake and a destructive one — it would take out the signature and
* the quoted original along with the caret position.
*/
test("inserts into the body rather than replacing it", async ({ page }) => {
await page.goto("/mail/inbox");
await page.getByRole("link", { name: "Compose" }).first().click();
await expect(page.locator(".compose-window").first()).toBeVisible();
const editor = page.locator(EDITOR).first();
await editor.click();
await page.keyboard.type("Vorher");
await pasteHtml(page, "<p>Eingefügt</p>");
const text = await editor.innerText();
expect(text).toContain("Vorher");
expect(text).toContain("Eingefügt");
});
});
+130
View File
@@ -0,0 +1,130 @@
import { test, expect } from "./support/test";
import { seed } from "./support/config";
/**
* Destructive actions ask in the application, not in the browser.
*
* `data-turbo-confirm` calls `window.confirm()` unless Turbo is told otherwise,
* and that dialog is the one piece of interface plMail cannot style, theme or
* translate — it also puts the origin at the top, so "delete this label?" reads
* like a phishing warning rather than part of the app. Twenty-eight places used
* it, counting the forms that called confirm() from an `onsubmit` attribute.
*
* The fix is a single `Turbo.setConfirmMethod`, so what is worth asserting is
* the mechanism rather than any one screen: whatever carries a confirmation
* goes through the app's dialog, and nothing anywhere can reach the browser's.
* A native dialog makes `page.on("dialog")` fire, so that is watched throughout.
*/
test.beforeEach(() => {
seed("seed-mail");
});
/** Fails loudly instead of hanging: a native confirm() blocks the page. */
function refuseNativeDialogs(page: import("@playwright/test").Page): () => number {
let seen = 0;
page.on("dialog", (dialog) => {
seen++;
void dialog.dismiss();
});
return () => seen;
}
test.describe("confirmations", () => {
test("a confirmed action asks in the app, and Cancel means no", async ({ page }) => {
const nativeDialogs = refuseNativeDialogs(page);
await page.goto("/settings?section=labels");
const form = page.locator("form[data-turbo-confirm]").first();
await expect(form, "no confirmed action on this page to exercise").toBeVisible();
const question = await form.getAttribute("data-turbo-confirm") ?? "";
expect(question.length, "the confirmation has no question in it").toBeGreaterThan(0);
await form.locator('button[type="submit"], button:not([type])').first().click();
const dialog = page.locator("#confirm-dialog");
await expect(dialog).toBeVisible();
// The app's dialog, carrying the app's question — not a generic one.
await expect(dialog).toContainText(question.slice(0, 40));
await dialog.getByRole("button", { name: "Cancel" }).click();
await expect(dialog).toBeHidden();
// Cancelling did not submit: the form is still there to be asked again.
await expect(form).toBeVisible();
expect(nativeDialogs(), "a browser confirm() was reached").toBe(0);
});
/**
* Escape means no.
*
* <dialog> closes on Escape by itself, which is a reason to use one — but it
* closes with an empty returnValue, and reading an empty value as anything
* other than "cancel" would turn the safest gesture a user has into the
* destructive one.
*/
test("Escape cancels rather than confirming", async ({ page }) => {
const nativeDialogs = refuseNativeDialogs(page);
await page.goto("/settings?section=labels");
const form = page.locator("form[data-turbo-confirm]").first();
await form.locator('button[type="submit"], button:not([type])').first().click();
const dialog = page.locator("#confirm-dialog");
await expect(dialog).toBeVisible();
await page.keyboard.press("Escape");
await expect(dialog).toBeHidden();
await expect(form, "Escape submitted the form").toBeVisible();
expect(nativeDialogs()).toBe(0);
});
/**
* Cancel is focused, not Continue.
*
* Every caller of this is destructive — that is what a confirmation is for
* — so the safe answer is the one a stray Enter or Space lands on.
*/
test("the safe answer has the focus", async ({ page }) => {
await page.goto("/settings?section=labels");
const form = page.locator("form[data-turbo-confirm]").first();
await form.locator('button[type="submit"], button:not([type])').first().click();
await expect(page.locator("#confirm-dialog")).toBeVisible();
await expect(page.locator("#confirm-dialog").getByRole("button", { name: "Cancel" }))
.toBeFocused();
});
/**
* Nothing in the application still reaches window.confirm().
*
* The fix is one registration, so the risk is not that it stops working —
* it is that somebody writes `onsubmit="return confirm(…)"` again, which
* bypasses Turbo entirely and cannot be caught by any amount of correct
* wiring elsewhere. Twenty-eight of those existed; this is what stops the
* twenty-ninth.
*/
test("no page still calls the browser dialog from an attribute", async ({ page }) => {
for (const url of ["/settings?section=labels", "/settings?section=security", "/settings?section=filters", "/admin"]) {
const response = await page.goto(url);
if ((response?.status() ?? 500) >= 400) {
continue;
}
const inlineConfirms = await page.evaluate(
() => document.querySelectorAll('[onsubmit*="confirm"], [onclick*="confirm"]').length,
);
expect(inlineConfirms, `${url} calls confirm() from an attribute`).toBe(0);
}
});
});
+9
View File
@@ -1314,6 +1314,15 @@ form:
cancel: Abbrechen
submit: Speichern
confirm_dialog:
heading: Bist du sicher?
cancel: Abbrechen
confirm: Weiter
discarded:
actions:
purge: Endgültig löschen
purge_confirm: 'Diese Unterhaltung endgültig löschen? Sie lässt sich nicht zurückholen und wird auch vom Mailserver entfernt.'
restore: In den Posteingang
header:
labels:
add: Label erstellen
+11
View File
@@ -1305,6 +1305,17 @@ form:
cancel: Cancel
submit: Save
confirm_dialog:
# The in-app replacement for window.confirm(). Turbo's data-turbo-confirm
# calls the browser dialog by default; setConfirmMethod points it here.
heading: Are you sure?
cancel: Cancel
confirm: Continue
discarded:
actions:
purge: Delete forever
purge_confirm: 'Delete this conversation for good? It cannot be brought back, and it is removed from the mail server too.'
restore: Move to inbox
header:
labels:
add: Create label