From 637e96440845eddf8efed30d24e3af7e3b83af68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Wed, 14 Oct 2026 11:49:02 +0200 Subject: [PATCH] Serve the inbox tabs over JMAP, and tell phones when a re-sort moves mail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A phone could see the five built-in categories and nothing of how the reader arranged them. urn:plmail:params:jmap:inboxtabs adds: - InboxTab/get and /changes: every tab in the reader's order, built-ins with isEnabled, the reader's own with their name. Per user, answered by every account; a change is recorded in each account's log so push and /changes work as they do for labels. - Thread.inboxTab: the tab the web shows a conversation in — the reader's own, the built-in, or Primary for a switched-off category. - Email/query threadInboxTab: the same list the web shows under a key. - Thread/set { inboxTab }: a move between tabs, pinned like a drop. A tab the reader does not have is refused. Thread.category and threadCategory are unchanged for clients that know only the five. The recompute statements wrote every conversation's category back whether it changed or not and recorded nothing, so a re-sort — any of the sorting choices, and now switching a tab — left phones drawing the old tabs. They update only what moves and return it, and every caller records those as Thread changes. Co-Authored-By: Claude Opus 5.5 --- src/Command/Backfill/CategoryBackfillTask.php | 9 +- .../Settings/InboxCategoryController.php | 10 + .../Messaging/Handler/ClassifyMailHandler.php | 7 +- .../Handler/ResortMailboxHandler.php | 11 +- src/Jmap/Mapper/InboxTabMapper.php | 68 +++++ .../Method/Mail/InboxTabChangesMethod.php | 55 ++++ src/Jmap/Method/Mail/InboxTabGetMethod.php | 91 ++++++ src/Jmap/Method/Mail/ThreadGetMethod.php | 13 +- src/Jmap/Method/Mail/ThreadSetMethod.php | 45 ++- src/Jmap/Protocol/Capability.php | 14 + src/Jmap/Protocol/StateChangeBuilder.php | 3 + src/Jmap/Query/EmailFilterCompiler.php | 61 ++++ src/Jmap/README.md | 35 ++- src/Jmap/Session/SessionBuilder.php | 5 + src/Jmap/State/JmapObjectType.php | 3 + .../Mail/MessageThreadRepository.php | 135 +++++---- src/Service/Mail/InboxCategoryManager.php | 45 ++- src/Service/Mail/InboxTabChanges.php | 59 ++++ tests/Jmap/Method/InboxTabTest.php | 278 ++++++++++++++++++ .../Mail/ClassificationHoldFlowTest.php | 1 + .../Service/Mail/InboxCategoryManagerTest.php | 2 + 21 files changed, 873 insertions(+), 77 deletions(-) create mode 100644 src/Jmap/Mapper/InboxTabMapper.php create mode 100644 src/Jmap/Method/Mail/InboxTabChangesMethod.php create mode 100644 src/Jmap/Method/Mail/InboxTabGetMethod.php create mode 100644 src/Service/Mail/InboxTabChanges.php create mode 100644 tests/Jmap/Method/InboxTabTest.php diff --git a/src/Command/Backfill/CategoryBackfillTask.php b/src/Command/Backfill/CategoryBackfillTask.php index 0679c562..b80ac01c 100644 --- a/src/Command/Backfill/CategoryBackfillTask.php +++ b/src/Command/Backfill/CategoryBackfillTask.php @@ -39,6 +39,7 @@ final readonly class CategoryBackfillTask implements BackfillTaskInterface private ContactRepository $contactRepository, private MessageCategorizer $categorizer, private EntityManagerInterface $em, + private \App\Jmap\State\StateManager $stateManager, ) {} public function getName(): string @@ -119,8 +120,12 @@ final readonly class CategoryBackfillTask implements BackfillTaskInterface // Resolve each thread most-recent-wins from its messages, in one // statement. Runs after clear(), so it is pure DBAL. - $threads = $this->threadRepository->recomputeCategoriesForAccount($account); + $moved = $this->threadRepository->recomputeCategoriesForAccount($account); - $io->success(sprintf('Categorised %d message(s), resolved %d thread(s).', $processed, $threads)); + // So a JMAP client hears about the conversations that changed tab. + $this->stateManager->recordThreadsTouched((int) $account->id, $moved); + $this->em->flush(); + + $io->success(sprintf('Categorised %d message(s), moved %d thread(s).', $processed, count($moved))); } } diff --git a/src/Controller/Settings/InboxCategoryController.php b/src/Controller/Settings/InboxCategoryController.php index 5c51afe2..52213bee 100644 --- a/src/Controller/Settings/InboxCategoryController.php +++ b/src/Controller/Settings/InboxCategoryController.php @@ -52,6 +52,7 @@ final class InboxCategoryController extends AbstractController private readonly MessageThreadRepository $threads, private readonly EntityManagerInterface $em, private readonly MessageBusInterface $bus, + private readonly \App\Service\Mail\InboxTabChanges $tabChanges, ) { } @@ -103,6 +104,7 @@ final class InboxCategoryController extends AbstractController } $user->categorySorting->switchOff($off); + $this->tabChanges->updated($user, [$category->value]); $this->em->flush(); // After the flush, so the worker reads the new setting. @@ -134,12 +136,20 @@ final class InboxCategoryController extends AbstractController /** @var User $user */ $user = $this->getUser(); $known = array_map(static fn ($tab): string => $tab->value, $this->tabs->managedFor($user)); + $was = $user->categorySorting->tabOrder; $user->categorySorting->arrange(array_values(array_filter( array_map(static fn (mixed $key): string => is_string($key) ? $key : '', $keys), static fn (string $key): bool => true === in_array($key, $known, true), ))); + // Every tab, because every sortOrder may have moved. Not for a request + // that changed nothing: the page writes after each move, including a + // move straight back. + if ($was !== $user->categorySorting->tabOrder) { + $this->tabChanges->updated($user, $known); + } + $this->em->flush(); return $this->json(['ok' => true]); diff --git a/src/Infrastructure/Messaging/Handler/ClassifyMailHandler.php b/src/Infrastructure/Messaging/Handler/ClassifyMailHandler.php index 52e6ea45..c9caa211 100644 --- a/src/Infrastructure/Messaging/Handler/ClassifyMailHandler.php +++ b/src/Infrastructure/Messaging/Handler/ClassifyMailHandler.php @@ -80,6 +80,7 @@ final readonly class ClassifyMailHandler private HeldMailReleaser $releaser, private EntityManagerInterface $entityManager, private LoggerInterface $logger, + private \App\Jmap\State\StateManager $stateManager, ) { } @@ -235,7 +236,11 @@ final readonly class ClassifyMailHandler // for mail the timed release has already shown. $this->releaser->release($mail); - $this->threads->recomputeCategoriesForThreads(array_keys($threadIds)); + foreach ($this->threads->recomputeCategoriesForThreads(array_keys($threadIds)) as $accountId => $moved) { + $this->stateManager->recordThreadsTouched($accountId, $moved); + } + + $this->entityManager->flush(); // And the ones the timed release got to first. They were shown under // the rules' answer and have just been moved, which an open inbox has diff --git a/src/Infrastructure/Messaging/Handler/ResortMailboxHandler.php b/src/Infrastructure/Messaging/Handler/ResortMailboxHandler.php index ea970f7f..d3c19cb9 100644 --- a/src/Infrastructure/Messaging/Handler/ResortMailboxHandler.php +++ b/src/Infrastructure/Messaging/Handler/ResortMailboxHandler.php @@ -10,6 +10,7 @@ use App\Repository\Mail\ContactRepository; use App\Repository\Mail\MessageRepository; use App\Repository\Mail\MessageThreadRepository; use App\Repository\User\UserRepository; +use App\Jmap\State\StateManager; use App\Service\Mail\MessageCategorizer; use App\Service\Monitoring\ProcessHeartbeatService; use Doctrine\ORM\EntityManagerInterface; @@ -51,6 +52,7 @@ final readonly class ResortMailboxHandler private EntityManagerInterface $em, private ProcessHeartbeatService $heartbeats, private LoggerInterface $logger, + private StateManager $stateManager, ) { } @@ -101,7 +103,14 @@ final readonly class ResortMailboxHandler $fresh = $this->accounts->find($accountId); if (null !== $fresh) { - $this->threads->recomputeCategoriesForAccount($fresh); + // Told to JMAP, conversation by conversation: a phone draws its + // tabs from Thread.category and its own tab, and a re-sort that + // moved them silently left it drawing the old ones. + $this->stateManager->recordThreadsTouched( + $accountId, + $this->threads->recomputeCategoriesForAccount($fresh), + ); + $this->em->flush(); } } diff --git a/src/Jmap/Mapper/InboxTabMapper.php b/src/Jmap/Mapper/InboxTabMapper.php new file mode 100644 index 00000000..428c8668 --- /dev/null +++ b/src/Jmap/Mapper/InboxTabMapper.php @@ -0,0 +1,68 @@ + $tab->value, + 'name' => $tab->isCustom() ? $tab->name : null, + 'builtIn' => $tab->builtIn?->value, + 'isEnabled' => null === $tab->builtIn || false === $user->categorySorting->isSwitchedOff($tab->builtIn), + 'sortOrder' => $sortOrder, + ]; + } + + public function tabOf(MessageThread $thread, User $user): ?string + { + if (null !== $thread->userCategory) { + return $thread->userCategory->tabKey(); + } + + if (null === $thread->category) { + return null; + } + + return true === $user->categorySorting->isSwitchedOff($thread->category) + ? MessageCategory::Primary->value + : $thread->category->value; + } +} diff --git a/src/Jmap/Method/Mail/InboxTabChangesMethod.php b/src/Jmap/Method/Mail/InboxTabChangesMethod.php new file mode 100644 index 00000000..1979e405 --- /dev/null +++ b/src/Jmap/Method/Mail/InboxTabChangesMethod.php @@ -0,0 +1,55 @@ +accountResolver->resolve($context->user, $arguments['accountId'] ?? null); + + $sinceState = $arguments['sinceState'] ?? null; + + if (false === is_string($sinceState)) { + throw new MethodException('invalidArguments', '"sinceState" is required.'); + } + + $maxChanges = $arguments['maxChanges'] ?? null; + + if (null !== $maxChanges && false === is_int($maxChanges)) { + throw new MethodException('invalidArguments', '"maxChanges" must be a number.'); + } + + return $this->stateManager + ->changesSince($account->id, JmapObjectType::InboxTab, $sinceState, $maxChanges) + ->toResult((string) $account->id); + } +} diff --git a/src/Jmap/Method/Mail/InboxTabGetMethod.php b/src/Jmap/Method/Mail/InboxTabGetMethod.php new file mode 100644 index 00000000..05343e66 --- /dev/null +++ b/src/Jmap/Method/Mail/InboxTabGetMethod.php @@ -0,0 +1,91 @@ +accountResolver->resolve($context->user, $arguments['accountId'] ?? null); + + $properties = $arguments['properties'] ?? null; + + if (null !== $properties && false === is_array($properties)) { + throw new MethodException('invalidArguments', '"properties" must be an array or null.'); + } + + $ids = $arguments['ids'] ?? null; + + if (null !== $ids && false === is_array($ids)) { + throw new MethodException('invalidArguments', '"ids" must be an array or null.'); + } + + $all = []; + + foreach ($this->tabs->managedFor($context->user) as $position => $tab) { + $all[$tab->value] = $this->mapper->toJmap($tab, $position, $context->user); + } + + $list = []; + $notFound = []; + + foreach (null === $ids ? array_keys($all) : array_map('strval', $ids) as $id) { + if (false === isset($all[$id])) { + $notFound[] = $id; + + continue; + } + + $object = $all[$id]; + + if (null !== $properties) { + // "id" always comes back, whatever was asked for (RFC 8620 §5.1). + $object = array_intersect_key($object, array_flip([...array_map('strval', $properties), 'id'])); + } + + $list[] = $object; + } + + return [ + 'accountId' => (string) $account->id, + 'state' => $this->stateManager->stateFor($account->id, JmapObjectType::InboxTab), + 'list' => $list, + 'notFound' => $notFound, + ]; + } +} diff --git a/src/Jmap/Method/Mail/ThreadGetMethod.php b/src/Jmap/Method/Mail/ThreadGetMethod.php index fbadd4fd..ba71f3d7 100644 --- a/src/Jmap/Method/Mail/ThreadGetMethod.php +++ b/src/Jmap/Method/Mail/ThreadGetMethod.php @@ -17,8 +17,8 @@ use App\Repository\Mail\MessageThreadRepository; * receivedAt order, which is exactly how MessageThread::$messages is mapped * (#[ORM\OrderBy] on receivedAt then id), so no re-sorting is needed here. * - * Three plMail extensions ride along: `snoozedUntil`, `category` and `isNew`. - * Standard clients ignore all three. + * Four plMail extensions ride along: `snoozedUntil`, `category`, `inboxTab` + * and `isNew`. Standard clients ignore all four. */ final class ThreadGetMethod implements JmapMethod { @@ -28,6 +28,7 @@ final class ThreadGetMethod implements JmapMethod private readonly AccountResolver $accountResolver, private readonly MessageThreadRepository $threadRepository, private readonly StateManager $stateManager, + private readonly \App\Jmap\Mapper\InboxTabMapper $tabs, ) { } @@ -117,6 +118,14 @@ final class ThreadGetMethod implements JmapMethod // own inbox query does not, so folding would put mail on the // phone's Primary tab that the browser's does not have. 'category' => $thread->category?->value, + // plMail extension (urn:plmail:params:jmap:inboxtabs): the tab + // the web shows this conversation in — a tab of the reader's + // own, the built-in category, or Primary for a category they + // switched off. What a client that knows InboxTab draws from; + // `category` above stays the built-in category whatever the + // reader did, for clients that know only the five. See + // InboxTabMapper::tabOf(). + 'inboxTab' => $this->tabs->tabOf($thread, $context->user), // plMail extension, and the one that had no wire surface at // all until a client needed it. Newness is // MessageThread::isNewAt(): never put in front of the user AND diff --git a/src/Jmap/Method/Mail/ThreadSetMethod.php b/src/Jmap/Method/Mail/ThreadSetMethod.php index 98823cb8..994bcf7f 100644 --- a/src/Jmap/Method/Mail/ThreadSetMethod.php +++ b/src/Jmap/Method/Mail/ThreadSetMethod.php @@ -54,6 +54,12 @@ use Doctrine\ORM\EntityManagerInterface; * what it asked for instead. On a thread because a conversation is the unit of * a move, as it is of a snooze. * + * The fourth is `inboxTab`: put the conversation in a tab — a built-in one + * or one the reader made — the way dragging it onto the tab does on the web, + * through the same ThreadStatusUpdater::setCategory(). The value is a tab key + * (see InboxTabMapper); one the reader does not have, a switched-off built-in + * included, is refused rather than read as Primary. + * * Deliberately narrow. `create` and `destroy` are refused outright: threads * come into being by mail arriving and go away when their last message does, * and a client that could conjure one would be describing something the rest @@ -74,6 +80,9 @@ final class ThreadSetMethod implements JmapMethod private readonly ThreadSnoozeService $snoozeService, private readonly MoveToService $moveTo, private readonly LabelBindingRepository $bindingRepository, + private readonly \App\Service\Mail\InboxTabs $inboxTabs, + private readonly \App\Service\Mail\ThreadStatusUpdater $status, + private readonly \App\Repository\Mail\InboxCategoryRepository $inboxCategories, ) { } @@ -218,7 +227,7 @@ final class ThreadSetMethod implements JmapMethod } foreach (array_keys($patch) as $property) { - if (false === in_array($property, ['snoozedUntil', 'isNew', 'moveTo'], true)) { + if (false === in_array($property, ['snoozedUntil', 'isNew', 'moveTo', 'inboxTab'], true)) { throw new MethodException( 'invalidProperties', sprintf('"%s" is not a settable Thread property.', (string) $property), @@ -245,6 +254,10 @@ final class ThreadSetMethod implements JmapMethod $this->move($thread, $patch['moveTo'], $user, $accountId); } + if (true === array_key_exists('inboxTab', $patch)) { + $this->fileInTab($thread, $patch['inboxTab'], $user); + } + if (false === array_key_exists('snoozedUntil', $patch)) { // An empty patch is a no-op rather than an error: it changes // nothing, and refusing it would make a client's "apply whatever @@ -267,6 +280,36 @@ final class ThreadSetMethod implements JmapMethod return $shown; } + /** + * `inboxTab: ""` — the web's drop onto a tab. Pins the conversation + * there, as the drop does, and records the Thread change itself. + * + * @throws MethodException when the key names no tab this reader has + */ + private function fileInTab(MessageThread $thread, mixed $key, User $user): void + { + $tab = is_string($key) ? $this->inboxTabs->resolve($user, $key) : null; + + if (null === $tab) { + throw new MethodException( + 'invalidProperties', + sprintf('"%s" is not one of your inbox tabs. InboxTab/get lists them.', is_string($key) ? $key : get_debug_type($key)), + ); + } + + $target = true === $tab->isCustom() + ? $this->inboxCategories->find((int) $tab->categoryId) + : $tab->builtIn; + + // resolve() checked ownership; a category deleted between the two + // reads is the only way to land here. + if (null === $target) { + throw new MethodException('invalidProperties', 'That tab no longer exists.'); + } + + $this->status->setCategory($thread, $target); + } + /** * `moveTo: { mailboxId, fromMailboxId }` — the web's "Move to". * diff --git a/src/Jmap/Protocol/Capability.php b/src/Jmap/Protocol/Capability.php index 60fa5190..647013ab 100644 --- a/src/Jmap/Protocol/Capability.php +++ b/src/Jmap/Protocol/Capability.php @@ -92,6 +92,19 @@ final class Capability */ public const string TEMPLATES = 'urn:plmail:params:jmap:templates'; + /** + * Vendor extension: the inbox tabs, as the reader arranged them — which + * built-in categories are switched on, the tabs they made themselves, and + * the order of all of them. InboxTab/get and InboxTab/changes, plus + * Thread.inboxTab and Email/query's threadInboxTab. + * + * On every account rather than at the top level like TEMPLATES, though the + * tabs are the user's: Thread.inboxTab is an account-scoped property, and + * the change log a phone is pushed from is per account. Every account of a + * user answers with the same tabs, the way a label is a Mailbox in each. + */ + public const string INBOX_TABS = 'urn:plmail:params:jmap:inboxtabs'; + /** * Capabilities a client is currently allowed to declare in "using". * Grow this list as new object types come online. @@ -112,5 +125,6 @@ final class Capability self::SYNC, self::CONTACTS, self::TEMPLATES, + self::INBOX_TABS, ]; } diff --git a/src/Jmap/Protocol/StateChangeBuilder.php b/src/Jmap/Protocol/StateChangeBuilder.php index 75f55ecc..3ea6d0e0 100644 --- a/src/Jmap/Protocol/StateChangeBuilder.php +++ b/src/Jmap/Protocol/StateChangeBuilder.php @@ -30,6 +30,9 @@ final class StateChangeBuilder JmapObjectType::Email, JmapObjectType::Thread, JmapObjectType::EmailSubmission, + // Rare — a tab made, renamed, switched or moved in Settings — and + // exactly what a phone cannot find out any other way. + JmapObjectType::InboxTab, ]; public function __construct( diff --git a/src/Jmap/Query/EmailFilterCompiler.php b/src/Jmap/Query/EmailFilterCompiler.php index 5232acc1..685c4252 100644 --- a/src/Jmap/Query/EmailFilterCompiler.php +++ b/src/Jmap/Query/EmailFilterCompiler.php @@ -155,6 +155,8 @@ final class EmailFilterCompiler 'listId' => $this->header('list-id', $value, $parameters), // plMail extension; see threadCategory() and src/Jmap/README.md. 'threadCategory' => $this->threadCategory($value, $parameters), + // plMail extension; see threadInboxTab(). + 'threadInboxTab' => $this->threadInboxTab($value, $parameters), default => throw new MethodException('unsupportedFilter', sprintf('Filter condition "%s" is not supported.', $property)), }; } @@ -278,6 +280,65 @@ final class EmailFilterCompiler ); } + /** + * The tab the web shows the conversation in — `urn:plmail:params:jmap:inboxtabs`. + * + * threadCategory() above, for a client that knows the reader's tabs: the + * same conversations the web lists under the same key + * (MessageThreadRepository::narrowToCategory()), and so the same rules. + * + * - One of the reader's own tabs ("c12") holds what is filed there. No + * ownership check is needed in PHP: a category is filed into only by + * its owner's filters and drops, so another person's id matches none of + * this reader's conversations. + * - A built-in tab holds its category, minus what was filed into one of + * the reader's own tabs. + * - Primary also holds the categories the reader switched off, and a + * switched-off tab holds nothing — read from the reader's settings in + * the same statement, so this compiler stays free of who is asking. + * + * @param array $parameters + */ + private function threadInboxTab(mixed $value, array &$parameters): string + { + if (false === is_string($value)) { + throw new MethodException('invalidArguments', '"threadInboxTab" must be a string.'); + } + + if (1 === preg_match('/^c([1-9][0-9]*)$/', $value, $match)) { + $name = $this->bind((int) $match[1], $parameters); + + return sprintf( + 'EXISTS (SELECT 1 FROM message_thread mt WHERE mt.id = m.thread_id AND mt.user_category_id = :%s)', + $name, + ); + } + + $category = MessageCategory::tryFrom($value); + + if (null === $category) { + throw new MethodException('invalidArguments', sprintf( + '"%s" is not a tab key. Use an id from InboxTab/get: one of %s, or "c" and the id of one of your own.', + $value, + implode(', ', array_column(MessageCategory::cases(), 'value')), + )); + } + + $name = $this->bind($category->value, $parameters); + + // jsonb_exists() rather than the `?` operator, which PDO would read as + // a positional placeholder. + $condition = MessageCategory::Primary === $category + ? sprintf('(mt.category = :%1$s OR jsonb_exists(u.category_disabled::jsonb, mt.category))', $name) + : sprintf('mt.category = :%1$s AND NOT jsonb_exists(u.category_disabled::jsonb, :%1$s)', $name); + + return sprintf( + 'EXISTS (SELECT 1 FROM message_thread mt JOIN account ta ON ta.id = mt.account_id JOIN "user" u ON u.id = ta.usr_id' + . ' WHERE mt.id = m.thread_id AND mt.user_category_id IS NULL AND %s)', + $condition, + ); + } + /** * Matches any attachment's filename. message_part rows exist for inline * parts too, but those carry a null filename and so never match. diff --git a/src/Jmap/README.md b/src/Jmap/README.md index 74660984..b6d4ac5a 100644 --- a/src/Jmap/README.md +++ b/src/Jmap/README.md @@ -235,20 +235,27 @@ Get these wrong and things fail quietly rather than loudly. category` is what fills those in, and re-running it after a change to `MessageCategorizer` needs no resync — it reads only persisted data. - **The tabs a reader makes themselves are not in this vocabulary, on purpose.** - The five are a closed, documented set that clients draw their tabs from, so a - sixth value would be a breaking change for every client that has never heard of - it — and `null` instead would make the conversation vanish from all of their - tabs. A conversation filed under one of the reader's own tabs - (`message_thread.user_category_id`) is therefore served here as the built-in - category the rules decided, which is what `message_thread.category` keeps, and - `threadCategory` still refuses anything outside the five. The consequence is - stated rather than hidden: for a reader who uses own tabs, the web and a JMAP - client differ about which tab such a conversation is in, and the JMAP client - shows it where it always would have. One consequence of dragging: a - conversation the reader drags onto a tab of their own is pinned, and a pin - stops the built-in category beneath it following new messages, so a JMAP - client keeps seeing it in the tab it was in when it was dragged. + **The tabs as the reader arranged them are a second, separate surface: + `urn:plmail:params:jmap:inboxtabs`.** The five stay a closed vocabulary on + `Thread.category` and `threadCategory`, because a sixth value would break + every client that has never heard of it. A client that knows the extension + reads `InboxTab/get` instead — every tab in the reader's order, the built-in + ones with `isEnabled`, the reader's own with their `name` — and draws from + `Thread.inboxTab`, filters with `threadInboxTab`, and moves a conversation + with `Thread/set { inboxTab }`. Those three follow the web exactly: a + conversation filed in a tab of the reader's own is in that tab and in no + built-in one, a category the reader switched off is listed under Primary, + and a switched-off tab is no tab (`threadInboxTab` matches nothing, + `Thread/set` refuses it). `InboxTabMapper` is where that rule is written for + JMAP and `MessageThreadRepository::narrowToCategory()` for the web. + + The tabs are the user's, and JMAP state is per account, so a change is + recorded in every account's change log (`InboxTabChanges`) — the way a label + is a Mailbox in each account — and every account answers `InboxTab/get` + with the same list. A re-sort that moves conversations between tabs records + each one it moved as a Thread change, so `Thread/changes` tells a phone; it + used to rewrite the column and tell nobody. + - **A JMAP CalendarEvent id is a `calendar_event` id — the series.** The query that finds events is a `tsrange &&` overlap over `calendar_event_occurrence`, so the ids the database hands back are *occurrence* ids and diff --git a/src/Jmap/Session/SessionBuilder.php b/src/Jmap/Session/SessionBuilder.php index 8aa997b3..aa6558de 100644 --- a/src/Jmap/Session/SessionBuilder.php +++ b/src/Jmap/Session/SessionBuilder.php @@ -93,6 +93,9 @@ final class SessionBuilder // to draw once per account, and a client composing from the // second account can ask the account it is composing from. Capability::CONTACTS => new \stdClass(), + // The user's tabs, answered by every account — see + // Capability::INBOX_TABS. + Capability::INBOX_TABS => new \stdClass(), ]; if ($accountId === $calendarAccountId) { @@ -112,6 +115,7 @@ final class SessionBuilder if (null !== $primaryId) { $primaryAccounts[Capability::MAIL] = $primaryId; $primaryAccounts[Capability::CONTACTS] = $primaryId; + $primaryAccounts[Capability::INBOX_TABS] = $primaryId; } if (null !== $calendarAccountId) { @@ -145,6 +149,7 @@ final class SessionBuilder Capability::SYNC => new \stdClass(), Capability::CONTACTS => $this->contactsCapabilities(), Capability::TEMPLATES => $this->templatesCapabilities(), + Capability::INBOX_TABS => new \stdClass(), ], 'accounts' => $accountsValue, 'primaryAccounts' => $primaryAccountsValue, diff --git a/src/Jmap/State/JmapObjectType.php b/src/Jmap/State/JmapObjectType.php index a9d210f1..91efed5c 100644 --- a/src/Jmap/State/JmapObjectType.php +++ b/src/Jmap/State/JmapObjectType.php @@ -15,4 +15,7 @@ enum JmapObjectType: string case Thread = 'Thread'; case EmailSubmission = 'EmailSubmission'; case Identity = 'Identity'; + // The reader's inbox tabs — per user, recorded once for each of their + // accounts, the way a label is (MailChangeRecorder::labelChanged()). + case InboxTab = 'InboxTab'; } diff --git a/src/Repository/Mail/MessageThreadRepository.php b/src/Repository/Mail/MessageThreadRepository.php index e213e82f..f5ad3944 100644 --- a/src/Repository/Mail/MessageThreadRepository.php +++ b/src/Repository/Mail/MessageThreadRepository.php @@ -2459,45 +2459,31 @@ class MessageThreadRepository extends ServiceEntityRepository } /** - * Resolve every thread's category from its own messages, most-recent-wins, - * in one statement. + * The same resolution as recomputeCategoriesForAccount(), for a handful of + * threads rather than a mailbox. * - * Raw SQL because DISTINCT ON is a Postgres extension DQL has no form of, - * and because this is an UPDATE ... FROM over a derived table: the ORM's - * write path is per-entity, so the alternative is loading every thread and - * its messages to compute in PHP what the database can answer in one pass. - * - * Threads whose category was chosen by hand are excluded, which is the same - * rule MessageThreader::adoptCategory() applies one row at a time. It has - * to be stated in both places: the backfill does not go through the - * threader, so a run of `app:backfill category` would otherwise wipe every - * category anybody had ever dragged a conversation into — the whole install - * at once, which is worse than the arrival-time case the flag was written - * for. - */ - /** - * The same resolution, for a handful of threads rather than a mailbox. - * - * recomputeCategoriesForAccount() below is right for a backfill and wrong - * for a batch: it runs `DISTINCT ON (thread_id)` across every message the - * account has, which is the correct shape once and a scan of a hundred - * thousand rows if it runs after each batch of arriving mail. This is the - * same statement with the threads named, so the plan is bounded by the - * batch instead of by the mailbox. + * That one is right for a re-sort and wrong for a batch: it resolves every + * conversation the account has, which is the correct shape once and a + * scan of a hundred thousand rows if it runs after each batch of arriving + * mail. This is the same statement with the threads named, so the plan is + * bounded by the batch instead of by the mailbox. * * `category_pinned_at IS NULL` is carried over verbatim, and it is the part * that must not be lost in the narrowing: a conversation somebody dragged * onto a tab keeps it, whatever any model or rule now says. * * @param list $threadIds + * + * @return array> account id => the conversations that + * changed tab, for the JMAP change log */ - public function recomputeCategoriesForThreads(array $threadIds): int + public function recomputeCategoriesForThreads(array $threadIds): array { if ([] === $threadIds) { - return 0; + return []; } - return (int) $this->getEntityManager()->getConnection()->executeStatement( + $rows = $this->getEntityManager()->getConnection()->fetchAllAssociative( <<<'SQL' UPDATE message_thread t SET category = sub.category, @@ -2522,10 +2508,77 @@ class MessageThreadRepository extends ServiceEntityRepository ) sub WHERE t.id = sub.thread_id AND t.category_pinned_at IS NULL + -- Only what moves: a phone is told about every row returned. + AND (t.category IS DISTINCT FROM sub.category + OR t.user_category_id IS DISTINCT FROM sub.rule_category_id) + RETURNING t.id, t.account_id SQL, ['threadIds' => $threadIds], ['threadIds' => ArrayParameterType::INTEGER], ); + + $changed = []; + + foreach ($rows as $row) { + $changed[(int) $row['account_id']][] = (int) $row['id']; + } + + return $changed; + } + + /** + * Resolve every thread's category from its own messages, most-recent-wins, + * in one statement, for one account. + * + * Raw SQL because DISTINCT ON is a Postgres extension DQL has no form of, + * and because this is an UPDATE ... FROM over a derived table: the ORM's + * write path is per-entity, so the alternative is loading every thread and + * its messages to compute in PHP what the database can answer in one pass. + * + * Threads whose category was chosen by hand are excluded, which is the same + * rule MessageThreader::adoptCategory() applies one row at a time. It has + * to be stated in both places: the backfill does not go through the + * threader, so a run of `app:backfill category` would otherwise wipe every + * category anybody had ever dragged a conversation into — the whole install + * at once, which is worse than the arrival-time case the flag was written + * for. + * + * RETURNS THE CONVERSATIONS THAT MOVED, and only writes those. A re-sort + * used to rewrite every row with its own value and tell nobody, so a JMAP + * client went on drawing a conversation in the tab it had left until + * something else happened to it. The caller records these. + * + * @return list + */ + public function recomputeCategoriesForAccount(Account $account): array + { + $sql = <<<'SQL' + UPDATE message_thread t + SET category = sub.category, + user_category_id = sub.rule_category_id + FROM ( + SELECT DISTINCT ON (m.thread_id) m.thread_id, m.category, m.rule_category_id + FROM message m + WHERE m.category IS NOT NULL + AND (m.category_held_at IS NULL OR m.category_released_at IS NOT NULL) + -- This account's conversations only. Without it the DISTINCT ON + -- sorted every message on the installation, every user's, once + -- per account re-sorted — and switching a tab on or off re-sorts. + AND m.thread_id IN (SELECT id FROM message_thread WHERE account_id = :accountId) + ORDER BY m.thread_id, m.received_at DESC NULLS LAST + ) sub + WHERE t.id = sub.thread_id + AND t.account_id = :accountId + AND t.category_pinned_at IS NULL + AND (t.category IS DISTINCT FROM sub.category + OR t.user_category_id IS DISTINCT FROM sub.rule_category_id) + RETURNING t.id + SQL; + + return array_map( + intval(...), + $this->getEntityManager()->getConnection()->fetchFirstColumn($sql, ['accountId' => $account->id]), + ); } /** @@ -2554,34 +2607,6 @@ class MessageThreadRepository extends ServiceEntityRepository ); } - public function recomputeCategoriesForAccount(Account $account): int - { - $sql = <<<'SQL' - UPDATE message_thread t - SET category = sub.category, - user_category_id = sub.rule_category_id - FROM ( - SELECT DISTINCT ON (m.thread_id) m.thread_id, m.category, m.rule_category_id - FROM message m - WHERE m.category IS NOT NULL - AND (m.category_held_at IS NULL OR m.category_released_at IS NOT NULL) - -- This account's conversations only. Without it the DISTINCT ON - -- sorted every message on the installation, every user's, once - -- per account re-sorted — and switching a tab on or off re-sorts. - AND m.thread_id IN (SELECT id FROM message_thread WHERE account_id = :accountId) - ORDER BY m.thread_id, m.received_at DESC NULLS LAST - ) sub - WHERE t.id = sub.thread_id - AND t.account_id = :accountId - AND t.category_pinned_at IS NULL - SQL; - - return (int) $this->getEntityManager()->getConnection()->executeStatement( - $sql, - ['accountId' => $account->id], - ); - } - /** * How many of the user's conversations are snoozed right now. * diff --git a/src/Service/Mail/InboxCategoryManager.php b/src/Service/Mail/InboxCategoryManager.php index 25b42e09..13dd1144 100644 --- a/src/Service/Mail/InboxCategoryManager.php +++ b/src/Service/Mail/InboxCategoryManager.php @@ -8,6 +8,8 @@ use App\Domain\Enum\AppLocale; use App\Domain\Enum\Mail\MessageCategory; use App\Domain\Exception\InvalidInboxCategoryException; use App\Entity\Mail\InboxCategory; +use App\Entity\Mail\Message; +use App\Entity\Mail\MessageThread; use App\Entity\User\User; use App\Repository\Mail\InboxCategoryRepository; use App\Repository\Mail\MessageThreadRepository; @@ -46,6 +48,8 @@ final readonly class InboxCategoryManager private MessageThreadRepository $threads, private EntityManagerInterface $em, private TranslatorInterface $translator, + private \App\Jmap\State\StateManager $stateManager, + private InboxTabChanges $tabChanges, ) { } @@ -61,6 +65,10 @@ final readonly class InboxCategoryManager $this->em->persist($category); $this->em->flush(); + // The id exists only now, and so does the key a phone knows it by. + $this->tabChanges->created($owner, [$category->tabKey()]); + $this->em->flush(); + return $category; } @@ -71,6 +79,7 @@ final readonly class InboxCategoryManager { $category->name = $this->validName($category->usr, $name, $category); + $this->tabChanges->updated($category->usr, [$category->tabKey()]); $this->em->flush(); } @@ -83,6 +92,17 @@ final readonly class InboxCategoryManager $this->em->wrapInTransaction(function () use ($category): void { $connection = $this->em->getConnection(); + // Recorded while the id is still there to be named. + $this->tabChanges->destroyed($category->usr, [$category->tabKey()]); + + // Its key leaves the stored order too, so the order never names + // a tab that is gone — InboxTabs would skip it, but a restore or + // a later category could otherwise read it. + $category->usr->categorySorting->arrange(array_values(array_diff( + $category->usr->categorySorting->tabOrder, + [$category->tabKey()], + ))); + // Taken BEFORE the delete, because the delete is what empties the // column this is read from. $held = array_map( @@ -98,6 +118,23 @@ final readonly class InboxCategoryManager ['id' => $category->id], ); + // The foreign keys un-file the rows; what is already loaded in this + // request still points at the category, and the next flush would + // try to write the reference back. Mirrored here. + $loaded = $this->em->getUnitOfWork()->getIdentityMap(); + + foreach ($loaded[MessageThread::class] ?? [] as $thread) { + if ($thread instanceof MessageThread && $thread->userCategory === $category) { + $thread->userCategory = null; + } + } + + foreach ($loaded[Message::class] ?? [] as $message) { + if ($message instanceof Message && $message->ruleCategory === $category) { + $message->ruleCategory = null; + } + } + $this->em->remove($category); $this->em->flush(); @@ -108,7 +145,13 @@ final readonly class InboxCategoryManager // the one place this removal was meant to put right. The foreign // keys have already emptied the verdicts, so this resolves each of // them from its messages like any other conversation. - $this->threads->recomputeCategoriesForThreads($held); + // And told: a JMAP client shows the reader's own tabs too, and + // these conversations have just left one. + foreach ($this->threads->recomputeCategoriesForThreads($held) as $accountId => $moved) { + $this->stateManager->recordThreadsTouched($accountId, $moved); + } + + $this->em->flush(); }); } diff --git a/src/Service/Mail/InboxTabChanges.php b/src/Service/Mail/InboxTabChanges.php new file mode 100644 index 00000000..c18e6107 --- /dev/null +++ b/src/Service/Mail/InboxTabChanges.php @@ -0,0 +1,59 @@ + $keys */ + public function created(User $user, array $keys): void + { + $this->each($user, $keys, fn (int $account, string $key) => $this->stateManager->recordCreated($account, JmapObjectType::InboxTab, $key)); + } + + /** @param list $keys */ + public function updated(User $user, array $keys): void + { + $this->each($user, $keys, fn (int $account, string $key) => $this->stateManager->recordUpdated($account, JmapObjectType::InboxTab, $key)); + } + + /** @param list $keys */ + public function destroyed(User $user, array $keys): void + { + $this->each($user, $keys, fn (int $account, string $key) => $this->stateManager->recordDestroyed($account, JmapObjectType::InboxTab, $key)); + } + + /** + * @param list $keys + * @param callable(int, string): void $record + */ + private function each(User $user, array $keys, callable $record): void + { + foreach ($user->accounts as $account) { + foreach (array_unique($keys) as $key) { + $record((int) $account->id, $key); + } + } + } +} diff --git a/tests/Jmap/Method/InboxTabTest.php b/tests/Jmap/Method/InboxTabTest.php new file mode 100644 index 00000000..b2dcde2f --- /dev/null +++ b/tests/Jmap/Method/InboxTabTest.php @@ -0,0 +1,278 @@ +shipping = new InboxCategory(); + $this->shipping->usr = $this->user; + $this->shipping->name = 'Shipping'; + $this->em->persist($this->shipping); + $this->em->flush(); + } + + public function testTheSessionAdvertisesTheCapabilityOnEveryAccount(): void + { + $second = $this->secondAccount(); + $session = self::getContainer()->get(SessionBuilder::class)->build($this->user); + + self::assertArrayHasKey('urn:plmail:params:jmap:inboxtabs', $session['capabilities']); + self::assertArrayHasKey('urn:plmail:params:jmap:inboxtabs', $session['accounts'][$this->accountId()]['accountCapabilities']); + self::assertArrayHasKey('urn:plmail:params:jmap:inboxtabs', $session['accounts'][(string) $second->id]['accountCapabilities']); + } + + /** + * Primary first, then the reader's order; a switched-off built-in listed + * and marked, so a settings screen can switch it back on; built-ins + * unnamed, for the client to name in its own language. + */ + public function testTheTabsComeInTheReadersOrderWithTheirSwitches(): void + { + $this->user->categorySorting->arrange([$this->shipping->tabKey(), 'updates']); + $this->user->categorySorting->switchOff([MessageCategory::Forums]); + $this->em->flush(); + + $result = $this->get(); + + self::assertSame( + ['primary', $this->shipping->tabKey(), 'updates', 'social', 'promotions', 'forums'], + array_column($result['list'], 'id'), + ); + self::assertSame([0, 1, 2, 3, 4, 5], array_column($result['list'], 'sortOrder')); + + $byId = array_column($result['list'], null, 'id'); + + self::assertSame(['id' => $this->shipping->tabKey(), 'name' => 'Shipping', 'builtIn' => null, 'isEnabled' => true, 'sortOrder' => 1], $byId[$this->shipping->tabKey()]); + self::assertSame(['id' => 'forums', 'name' => null, 'builtIn' => 'forums', 'isEnabled' => false, 'sortOrder' => 5], $byId['forums']); + } + + public function testEveryAccountAnswersWithTheSameTabs(): void + { + $second = $this->secondAccount(); + + self::assertSame( + array_column($this->get()['list'], 'id'), + array_column($this->get((string) $second->id)['list'], 'id'), + ); + } + + public function testAskingForIdsAndPropertiesNarrowsTheAnswer(): void + { + $result = $this->get(ids: ['social', 'c999999'], properties: ['name']); + + self::assertSame([['id' => 'social', 'name' => null]], $result['list']); + self::assertSame(['c999999'], $result['notFound']); + } + + /** + * A change to the tabs is recorded in every account's log, so a phone + * watching any of them is told and InboxTab/changes says what moved. + */ + public function testAChangeIsReportedThroughEveryAccountsChanges(): void + { + $second = $this->secondAccount(); + $before = $this->get()['state']; + $beforeSecond = $this->get((string) $second->id)['state']; + + self::getContainer()->get(InboxTabChanges::class)->updated($this->user, ['forums']); + $this->em->flush(); + + foreach ([[$this->accountId(), $before], [(string) $second->id, $beforeSecond]] as [$accountId, $since]) { + $changes = self::getContainer()->get(InboxTabChangesMethod::class)->handle( + ['accountId' => $accountId, 'sinceState' => $since], + $this->context(), + ); + + self::assertSame(['forums'], $changes['updated']); + self::assertNotSame($since, $changes['newState']); + } + } + + /** + * Thread.inboxTab is where the web shows the conversation: the reader's + * own tab when filed in one, Primary for a category switched off, the + * category otherwise. Thread.category stays the built-in, for clients that + * know only the five. + */ + public function testThreadInboxTabIsWhereTheWebShowsTheConversation(): void + { + $filed = $this->inThread(MessageCategory::Promotions, $this->shipping); + $off = $this->inThread(MessageCategory::Forums); + $updates = $this->inThread(MessageCategory::Updates); + + $this->user->categorySorting->switchOff([MessageCategory::Forums]); + $this->em->flush(); + + $threads = array_column( + self::getContainer()->get(ThreadGetMethod::class)->handle( + ['accountId' => $this->accountId(), 'ids' => [(string) $filed->thread->id, (string) $off->thread->id, (string) $updates->thread->id]], + $this->context(), + )['list'], + null, + 'id', + ); + + self::assertSame($this->shipping->tabKey(), $threads[(string) $filed->thread->id]['inboxTab']); + self::assertSame('promotions', $threads[(string) $filed->thread->id]['category']); + self::assertSame('primary', $threads[(string) $off->thread->id]['inboxTab']); + self::assertSame('forums', $threads[(string) $off->thread->id]['category']); + self::assertSame('updates', $threads[(string) $updates->thread->id]['inboxTab']); + } + + /** + * The filter lists what the web lists under the same key. + */ + public function testThreadInboxTabFiltersLikeTheWebTabs(): void + { + $filed = $this->inThread(MessageCategory::Promotions, $this->shipping); + $off = $this->inThread(MessageCategory::Forums); + $primary = $this->inThread(MessageCategory::Primary); + $promo = $this->inThread(MessageCategory::Promotions); + + $this->user->categorySorting->switchOff([MessageCategory::Forums]); + $this->em->flush(); + + self::assertSame([(string) $filed->id], $this->query($this->shipping->tabKey())); + self::assertEqualsCanonicalizing([(string) $off->id, (string) $primary->id], $this->query('primary')); + self::assertSame([(string) $promo->id], $this->query('promotions'), 'a conversation filed in the reader\'s own tab is not also in its built-in one'); + self::assertSame([], $this->query('forums'), 'a switched-off tab holds nothing'); + } + + public function testAKeyThatIsNoTabIsRefused(): void + { + $this->expectException(MethodException::class); + + $this->query('quatsch'); + } + + /** + * Thread/set puts a conversation in a tab as a drop on the web does, own + * tabs included, and refuses a tab the reader does not have — a + * switched-off one, or somebody else's. + */ + public function testThreadSetMovesAConversationIntoATab(): void + { + $message = $this->inThread(MessageCategory::Promotions); + + $result = $this->set((string) $message->thread->id, $this->shipping->tabKey()); + + self::assertArrayHasKey((string) $message->thread->id, (array) $result['updated']); + + $this->em->refresh($message->thread); + self::assertSame($this->shipping->id, $message->thread->userCategory?->id); + self::assertNotNull($message->thread->categoryPinnedAt, 'a move is pinned, as a drop is'); + + $result = $this->set((string) $message->thread->id, 'updates'); + self::assertArrayHasKey((string) $message->thread->id, (array) $result['updated']); + + $this->em->refresh($message->thread); + self::assertNull($message->thread->userCategory); + self::assertSame(MessageCategory::Updates, $message->thread->category); + } + + public function testThreadSetRefusesATabTheReaderDoesNotHave(): void + { + $message = $this->inThread(MessageCategory::Promotions); + + $this->user->categorySorting->switchOff([MessageCategory::Forums]); + + $stranger = new \App\Entity\User\User(); + $stranger->email = 'stranger-' . uniqid('', true) . '@example.test'; + $stranger->nameFirst = 'S'; + $stranger->nameLast = 'T'; + $stranger->roles = ['ROLE_USER']; + $stranger->password = 'x'; + $this->em->persist($stranger); + + $theirs = new InboxCategory(); + $theirs->usr = $stranger; + $theirs->name = 'Theirs'; + $this->em->persist($theirs); + $this->em->flush(); + + foreach (['forums', $theirs->tabKey(), 'quatsch'] as $key) { + $result = $this->set((string) $message->thread->id, $key); + + self::assertArrayHasKey((string) $message->thread->id, (array) $result['notUpdated'], $key . ' was accepted'); + } + } + + // ── fixture ────────────────────────────────────────────────────────────── + + /** + * @param list|null $ids + * @param list|null $properties + * + * @return array + */ + private function get(?string $accountId = null, ?array $ids = null, ?array $properties = null): array + { + return self::getContainer()->get(InboxTabGetMethod::class)->handle( + ['accountId' => $accountId ?? $this->accountId(), 'ids' => $ids, 'properties' => $properties], + $this->context(), + ); + } + + /** @return list email ids */ + private function query(string $tab): array + { + return self::getContainer()->get(EmailQueryMethod::class)->handle( + ['accountId' => $this->accountId(), 'filter' => ['threadInboxTab' => $tab]], + $this->context(), + )['ids']; + } + + /** @return array */ + private function set(string $threadId, string $tab): array + { + return self::getContainer()->get(ThreadSetMethod::class)->handle( + ['accountId' => $this->accountId(), 'update' => [$threadId => ['inboxTab' => $tab]]], + $this->context(), + ); + } + + private function inThread(MessageCategory $category, ?InboxCategory $own = null): Message + { + $message = $this->receivedMessage(); + $message->category = $category; + $message->ruleCategory = $own; + + $message->thread->category = $category; + $message->thread->userCategory = $own; + $this->em->flush(); + + return $message; + } +} diff --git a/tests/Service/Mail/ClassificationHoldFlowTest.php b/tests/Service/Mail/ClassificationHoldFlowTest.php index 2a084646..24c0d917 100644 --- a/tests/Service/Mail/ClassificationHoldFlowTest.php +++ b/tests/Service/Mail/ClassificationHoldFlowTest.php @@ -392,6 +392,7 @@ final class ClassificationHoldFlowTest extends KernelTestCase $container->get(HeldMailReleaser::class), $this->em, new NullLogger(), + $container->get(\App\Jmap\State\StateManager::class), ); } diff --git a/tests/Service/Mail/InboxCategoryManagerTest.php b/tests/Service/Mail/InboxCategoryManagerTest.php index 7f07ed59..18d47120 100644 --- a/tests/Service/Mail/InboxCategoryManagerTest.php +++ b/tests/Service/Mail/InboxCategoryManagerTest.php @@ -49,6 +49,8 @@ final class InboxCategoryManagerTest extends KernelTestCase $container->get(MessageThreadRepository::class), $this->em, $container->get(\Symfony\Contracts\Translation\TranslatorInterface::class), + $container->get(\App\Jmap\State\StateManager::class), + $container->get(\App\Service\Mail\InboxTabChanges::class), ); $this->connection->beginTransaction();