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();