From 3c0769c1e6d9a5492411f69a7d02fa278325a8d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Wed, 14 Oct 2026 11:34:45 +0200 Subject: [PATCH] Order every inbox tab, and let a built-in one be switched off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One order across the built-in tabs and the reader's own, and a set of built-ins switched off, both beside the other sorting choices on the user (CategorySorting). The order replaces the categories' own position column, which never shipped: the strip is one list, so its order is too. Primary is always first and cannot be switched off — it is where the rest falls back to. A switched-off category takes no mail. Whatever would have gone there — by the header rules, Google's label or the assistant — is in Primary, and the message panel says it would have been that category. The answer is mapped rather than the next rule consulted: "Forums is off" does not make a mailing list Promotions for its List-Unsubscribe. Until the re-sort a switch queues has run, and for a conversation somebody pinned there, Primary lists and counts the switched-off category's conversations too, so nothing is in no tab at all. InboxTabs is where order and on/off are applied, so the strip, the drop targets and every key that resolves agree. A restore rewrites the order's own-tab keys to the categories' new ids. Co-Authored-By: Claude Opus 5.5 --- migrations/Version20261014170000.php | 14 ++- src/Entity/Embeddable/CategorySorting.php | 103 +++++++++++++++++- src/Entity/Mail/InboxCategory.php | 15 ++- .../Mail/InboxCategoryRepository.php | 24 +--- .../Mail/MessageThreadRepository.php | 66 ++++++++--- .../Backup/ConfigBackupUserRestorer.php | 32 +++++- src/Service/Backup/ConfigBackupUsers.php | 5 +- src/Service/Mail/InboxCategoryManager.php | 1 - src/Service/Mail/InboxTabs.php | 75 ++++++++++++- src/Service/Mail/MessageCategorizer.php | 38 +++++++ src/Twig/MessageCategoryExtension.php | 6 +- templates/mail/_message_details.html.twig | 4 + tests/Entity/Mail/InboxCategoryTest.php | 25 ++--- tests/Repository/Mail/InboxTabQueriesTest.php | 31 ++++++ .../ConfigBackupInboxCategoriesTest.php | 50 +++++++-- .../Service/Mail/InboxCategoryManagerTest.php | 7 +- tests/Service/Mail/InboxTabsTest.php | 53 ++++++++- tests/Service/Mail/MessageCategorizerTest.php | 55 ++++++++++ translations/messages.de.yaml | 1 + translations/messages.en.yaml | 1 + translations/messages.en_PI.yaml | 1 + translations/messages.fr.yaml | 1 + 22 files changed, 522 insertions(+), 86 deletions(-) diff --git a/migrations/Version20261014170000.php b/migrations/Version20261014170000.php index e61b07e3..3fa1475d 100644 --- a/migrations/Version20261014170000.php +++ b/migrations/Version20261014170000.php @@ -30,6 +30,11 @@ use Doctrine\Migrations\AbstractMigration; * The foreign keys are ON DELETE SET NULL, which is the point: removing a * category un-files its mail in the database, leaving no pointer at a row that * is gone. The owner's foreign key is CASCADE, so a category goes with its user. + * + * And two settings on the user, beside the other sorting choices + * (CategorySorting): which built-in tabs are switched off, and the order of all + * of them. An empty list is the shipped behaviour for both — every tab on, the + * built-ins in their usual order — so no row is rewritten for them either. */ final class Version20261014170000 extends AbstractMigration { @@ -45,13 +50,12 @@ final class Version20261014170000 extends AbstractMigration id SERIAL NOT NULL, usr_id INT NOT NULL, name VARCHAR(40) NOT NULL, - position INT DEFAULT 0 NOT NULL, created_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, updated_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, PRIMARY KEY(id) ) SQL); - $this->addSql('CREATE INDEX idx_inbox_category_usr_position ON inbox_category (usr_id, position)'); + $this->addSql('CREATE INDEX idx_inbox_category_usr ON inbox_category (usr_id)'); $this->addSql('ALTER TABLE inbox_category ADD CONSTRAINT FK_inbox_category_usr FOREIGN KEY (usr_id) REFERENCES "user" (id) ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE'); $this->addSql('ALTER TABLE message ADD rule_category_id INT DEFAULT NULL'); @@ -61,10 +65,16 @@ final class Version20261014170000 extends AbstractMigration $this->addSql('ALTER TABLE message_thread ADD user_category_id INT DEFAULT NULL'); $this->addSql('ALTER TABLE message_thread ADD CONSTRAINT FK_message_thread_user_category FOREIGN KEY (user_category_id) REFERENCES inbox_category (id) ON DELETE SET NULL NOT DEFERRABLE INITIALLY IMMEDIATE'); $this->addSql('CREATE INDEX idx_message_thread_user_category ON message_thread (user_category_id) WHERE user_category_id IS NOT NULL'); + + $this->addSql('ALTER TABLE "user" ADD category_disabled JSON DEFAULT \'[]\' NOT NULL'); + $this->addSql('ALTER TABLE "user" ADD category_tab_order JSON DEFAULT \'[]\' NOT NULL'); } public function down(Schema $schema): void { + $this->addSql('ALTER TABLE "user" DROP category_tab_order'); + $this->addSql('ALTER TABLE "user" DROP category_disabled'); + $this->addSql('DROP INDEX idx_message_thread_user_category'); $this->addSql('ALTER TABLE message_thread DROP CONSTRAINT FK_message_thread_user_category'); $this->addSql('ALTER TABLE message_thread DROP user_category_id'); diff --git a/src/Entity/Embeddable/CategorySorting.php b/src/Entity/Embeddable/CategorySorting.php index d728e227..2b24210a 100644 --- a/src/Entity/Embeddable/CategorySorting.php +++ b/src/Entity/Embeddable/CategorySorting.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Entity\Embeddable; use App\Domain\Enum\Mail\CategorySource; +use App\Domain\Enum\Mail\MessageCategory; use Doctrine\ORM\Mapping as ORM; /** @@ -83,6 +84,89 @@ class CategorySorting #[ORM\Column(name: 'tabs', type: 'boolean', options: ['default' => true])] public bool $tabs = true; + /** + * The built-in categories this person has switched off, by value. + * + * A FOURTH DECISION, AND UNLIKE $tabs IT MOVES MAIL. Gmail's Forums is a + * tab most people never see anything in, and Promotions is one some would + * rather not have at all. A switched-off category is not merely hidden: + * nothing sorts into it — not the header rules, not Google's labels, not + * the assistant — and what would have gone there is in Primary instead + * (MessageCategorizer). Switching it back on re-sorts, and the mail is + * back, because every answer is recomputed from stored data. + * + * Primary cannot be switched off: it is where everything else falls back + * to, and the tab the inbox opens on. A stored "primary" is ignored rather + * than trusted, whoever wrote it. + * + * @var list + */ + #[ORM\Column(name: 'disabled', type: 'json', options: ['default' => '[]'])] + public array $disabled = []; + + /** + * The order of the tabs after Primary, as tab keys — a built-in's value or + * a category of the person's own (InboxTab). + * + * One list across both kinds, because the person arranges one strip. + * Primary is not in it: it always comes first. A key that names nothing + * any more is skipped, and a tab the list does not name goes after the + * ones it does — built-ins in their usual order, then the person's own + * oldest first — so an empty list is exactly the strip as it shipped. + * + * @var list + */ + #[ORM\Column(name: 'tab_order', type: 'json', options: ['default' => '[]'])] + public array $tabOrder = []; + + /** + * The built-in categories switched off, as cases. Never Primary, and + * nothing this build does not know. + * + * @return list + */ + public function switchedOff(): array + { + $off = []; + + foreach ($this->disabled as $value) { + $category = MessageCategory::tryFrom((string) $value); + + if (null !== $category && MessageCategory::Primary !== $category && false === in_array($category, $off, true)) { + $off[] = $category; + } + } + + return $off; + } + + public function isSwitchedOff(MessageCategory $category): bool + { + return true === in_array($category, $this->switchedOff(), true); + } + + /** + * @param list $categories + */ + public function switchOff(array $categories): void + { + $this->disabled = array_values(array_unique(array_map( + static fn (MessageCategory $category): string => $category->value, + array_filter($categories, static fn (MessageCategory $category): bool => MessageCategory::Primary !== $category), + ))); + } + + /** + * @param list $keys + */ + public function arrange(array $keys): void + { + $this->tabOrder = array_values(array_unique(array_filter( + array_map(static fn (mixed $key): string => (string) $key, $keys), + static fn (string $key): bool => '' !== $key && MessageCategory::Primary->value !== $key, + ))); + } + public function sourceEnum(): CategorySource { return CategorySource::from_($this->source); @@ -133,10 +217,14 @@ class CategorySorting public function toArray(): array { return [ - 'version' => 1, + 'version' => 2, 'source' => $this->source, 'overrideProvider' => $this->overrideProvider, 'tabs' => $this->tabs, + 'disabled' => $this->disabled, + // Custom keys carry the category ids of the installation that wrote + // them; ConfigBackupUserRestorer rewrites them to the new rows. + 'tabOrder' => $this->tabOrder, ]; } @@ -166,6 +254,19 @@ class CategorySorting $this->tabs = self::boolean($data['tabs']); } + // Through the same filters the settings page writes through, so a + // hand-edited file cannot switch Primary off or store a non-string. + if (true === isset($data['disabled']) && true === is_array($data['disabled'])) { + $this->switchOff(array_values(array_filter(array_map( + static fn (mixed $value): ?MessageCategory => is_string($value) ? MessageCategory::tryFrom($value) : null, + $data['disabled'], + )))); + } + + if (true === isset($data['tabOrder']) && true === is_array($data['tabOrder'])) { + $this->arrange(array_values(array_filter($data['tabOrder'], 'is_string'))); + } + return $this; } diff --git a/src/Entity/Mail/InboxCategory.php b/src/Entity/Mail/InboxCategory.php index 2063c36e..5236efd8 100644 --- a/src/Entity/Mail/InboxCategory.php +++ b/src/Entity/Mail/InboxCategory.php @@ -33,9 +33,10 @@ use Doctrine\ORM\Mapping as ORM; */ #[ORM\Entity(repositoryClass: InboxCategoryRepository::class)] #[ORM\Table(name: 'inbox_category')] -// The only lookup is "this user's categories, in order" — on every inbox render -// and every filter edit — so owner leads and position follows. -#[ORM\Index(name: 'idx_inbox_category_usr_position', columns: ['usr_id', 'position'])] +// The only lookup is "this user's categories" — on every inbox render and every +// filter edit. Their order is not here: it is one list across the built-in tabs +// and these, so it lives with the other sorting choices (CategorySorting). +#[ORM\Index(name: 'idx_inbox_category_usr', columns: ['usr_id'])] #[ORM\HasLifecycleCallbacks] class InboxCategory { @@ -55,7 +56,9 @@ class InboxCategory #[ORM\Column(length: self::NAME_MAX_LENGTH)] public string $name; - /** Left to right among the user's own tabs; ties break on id. */ - #[ORM\Column(options: ['default' => 0])] - public int $position = 0; + /** The key this category's tab goes by — see InboxTab. */ + public function tabKey(): string + { + return \App\Domain\DTO\Mail\InboxTab::CUSTOM_PREFIX . (int) $this->id; + } } diff --git a/src/Repository/Mail/InboxCategoryRepository.php b/src/Repository/Mail/InboxCategoryRepository.php index 773ac682..52749d01 100644 --- a/src/Repository/Mail/InboxCategoryRepository.php +++ b/src/Repository/Mail/InboxCategoryRepository.php @@ -20,32 +20,14 @@ class InboxCategoryRepository extends ServiceEntityRepository } /** - * Where a new category goes: after every one the reader already has. - * - * One past the largest position rather than a count, because removing a - * category leaves a gap and a count would then collide with the tab at the - * end of the strip. - */ - public function nextPositionFor(UserInterface $user): int - { - $largest = $this->createQueryBuilder('c') - ->select('MAX(c.position)') - ->where('c.usr = :user') - ->setParameter('user', $user) - ->getQuery() - ->getSingleScalarResult(); - - return null === $largest ? 0 : (int) $largest + 1; - } - - /** - * Every category the user made, in the order their tabs are drawn. + * Every category the user made, oldest first. Where their tabs go among the + * built-in ones is CategorySorting::$tabOrder, applied by InboxTabs. * * @return list */ public function findForUserOrdered(UserInterface $user): array { /** @var list */ - return $this->findBy(['usr' => $user], ['position' => 'ASC', 'id' => 'ASC']); + return $this->findBy(['usr' => $user], ['id' => 'ASC']); } } diff --git a/src/Repository/Mail/MessageThreadRepository.php b/src/Repository/Mail/MessageThreadRepository.php index 76136eaf..72109d4a 100644 --- a/src/Repository/Mail/MessageThreadRepository.php +++ b/src/Repository/Mail/MessageThreadRepository.php @@ -152,7 +152,7 @@ class MessageThreadRepository extends ServiceEntityRepository ->setMaxResults($perPage) ->distinct(); - $this->narrowToCategory($qb, $category); + $this->narrowToCategory($qb, $category, $this->switchedOffBy($user)); $this->excludeTrashed($qb); $this->narrowToUnread($qb, $unreadOnly); @@ -167,7 +167,11 @@ class MessageThreadRepository extends ServiceEntityRepository * count so the two cannot come to mean different things — a pager offering * a page the list does not have is what that looks like. */ - private function narrowToCategory(QueryBuilder $qb, MessageCategory|InboxTab|null $category): void + /** + * @param list $absorbed built-in values switched off — their + * conversations are listed under Primary + */ + private function narrowToCategory(QueryBuilder $qb, MessageCategory|InboxTab|null $category, array $absorbed = []): void { if (null === $category) { return; @@ -188,11 +192,41 @@ class MessageThreadRepository extends ServiceEntityRepository return; } + // Primary also holds whatever is in a category the reader switched + // off. The re-sort a switch queues moves those messages for good, but + // it takes minutes on a big mailbox, and a conversation somebody + // pinned there is not its to move: until then they would be in no tab + // at all. + if (MessageCategory::Primary === $tab->builtIn && [] !== $absorbed) { + $qb->andWhere('t.userCategory IS NULL') + ->andWhere('t.category IN (:categories)') + ->setParameter('categories', [MessageCategory::Primary->value, ...$absorbed]); + + return; + } + $qb->andWhere('t.userCategory IS NULL') ->andWhere('t.category = :category') ->setParameter('category', $tab->builtIn); } + /** + * The built-in categories this reader switched off, as stored values. + * + * @return list + */ + private function switchedOffBy(UserInterface $user): array + { + if (false === $user instanceof \App\Entity\User\User) { + return []; + } + + return array_map( + static fn (MessageCategory $category): string => $category->value, + $user->categorySorting->switchedOff(), + ); + } + /** * Which tab a grouped row belongs to: the reader's own when the thread is * filed in one, else the built-in category. @@ -202,17 +236,18 @@ class MessageThreadRepository extends ServiceEntityRepository * different tabs. The key is InboxTab's, which for a built-in is the value * it always was. */ - private function tabKeyOf(mixed $category, mixed $userCategoryId): string + /** + * @param list $absorbed switched-off built-ins, counted as Primary + */ + private function tabKeyOf(mixed $category, mixed $userCategoryId, array $absorbed = []): string { if (null !== $userCategoryId) { return InboxTab::CUSTOM_PREFIX . (int) $userCategoryId; } - if ($category instanceof MessageCategory) { - return $category->value; - } + $value = $category instanceof MessageCategory ? $category->value : (string) $category; - return (string) $category; + return true === in_array($value, $absorbed, true) ? MessageCategory::Primary->value : $value; } /** @@ -224,15 +259,16 @@ class MessageThreadRepository extends ServiceEntityRepository * last of them would report a fraction of the tab. * * @param iterable> $rows + * @param list $absorbed switched-off built-ins, counted as Primary * * @return array */ - private function foldByTab(iterable $rows, string $countField): array + private function foldByTab(iterable $rows, string $countField, array $absorbed = []): array { $counts = []; foreach ($rows as $row) { - $key = $this->tabKeyOf($row['category'], $row['userCategory'] ?? null); + $key = $this->tabKeyOf($row['category'], $row['userCategory'] ?? null, $absorbed); $counts[$key] = ($counts[$key] ?? 0) + (int) $row[$countField]; } @@ -260,7 +296,7 @@ class MessageThreadRepository extends ServiceEntityRepository ->setParameter('user', $user) ->setParameter('inbox', LabelRole::Inbox); - $this->narrowToCategory($qb, $category); + $this->narrowToCategory($qb, $category, $this->switchedOffBy($user)); // The same exclusion findForUnifiedInbox() applies, and it was missing // here. A thread keeps its Inbox label when it goes to the bin — the @@ -298,7 +334,7 @@ class MessageThreadRepository extends ServiceEntityRepository $this->excludeTrashed($qb); - return $this->foldByTab($qb->getQuery()->getResult(), 'threadCount'); + return $this->foldByTab($qb->getQuery()->getResult(), 'threadCount', $this->switchedOffBy($user)); } /** @@ -333,7 +369,7 @@ class MessageThreadRepository extends ServiceEntityRepository $this->excludeTrashed($qb); - return $this->foldByTab($qb->getQuery()->getResult(), 'threadCount'); + return $this->foldByTab($qb->getQuery()->getResult(), 'threadCount', $this->switchedOffBy($user)); } /** @@ -983,7 +1019,7 @@ class MessageThreadRepository extends ServiceEntityRepository $this->restrictToNew($qb, $now); $this->excludeTrashed($qb); - return $this->foldByTab($qb->getQuery()->getResult(), 'newCount'); + return $this->foldByTab($qb->getQuery()->getResult(), 'newCount', $this->switchedOffBy($user)); } /** @@ -1009,6 +1045,8 @@ class MessageThreadRepository extends ServiceEntityRepository \DateTimeImmutable $now, int $perCategory = 3, ): array { + $absorbed = $this->switchedOffBy($user); + $qb = $this->createQueryBuilder('t') ->select( 't.id AS threadId', @@ -1053,7 +1091,7 @@ class MessageThreadRepository extends ServiceEntityRepository } $threads[$id] = [ - 'category' => $this->tabKeyOf($row['category'], $row['userCategory'] ?? null), + 'category' => $this->tabKeyOf($row['category'], $row['userCategory'] ?? null, $absorbed), 'arrivedAt' => $row['arrivedAt'], 'name' => $name, 'at' => $at, diff --git a/src/Service/Backup/ConfigBackupUserRestorer.php b/src/Service/Backup/ConfigBackupUserRestorer.php index 77965259..07e9b03c 100644 --- a/src/Service/Backup/ConfigBackupUserRestorer.php +++ b/src/Service/Backup/ConfigBackupUserRestorer.php @@ -195,12 +195,43 @@ final readonly class ConfigBackupUserRestorer $this->retargetAccountCalendars($accounts, $calendars); $this->applyAliasOverrides($accounts, $this->rows($document, 'accounts')); $this->createRules($user, $this->rows($document, 'rules'), $accounts, $labels, $integrations, $categories); + $this->retargetTabOrder($user, $categories); $this->entityManager->flush(); return $user; } + /** + * The tab order names the person's own tabs by their category ids, which + * were rows in the source database. Each is rewritten to the row it became, + * and dropped when the file did not carry that category — left as it was, + * the number could name somebody else's tab on this installation, though + * InboxTabs would only ever show the reader their own. + * + * @param array $categories keyed by the id in the file + */ + private function retargetTabOrder(User $user, array $categories): void + { + $keys = []; + + foreach ($user->categorySorting->tabOrder as $key) { + if (false === \App\Domain\DTO\Mail\InboxTab::looksCustom($key, $match)) { + $keys[] = $key; + + continue; + } + + $category = $categories[(int) $match[1]] ?? null; + + if (null !== $category?->id) { + $keys[] = $category->tabKey(); + } + } + + $user->categorySorting->arrange($keys); + } + // ── Private: the user row ───────────────────────────────────────────────── /** @@ -569,7 +600,6 @@ final readonly class ConfigBackupUserRestorer $category = new InboxCategory(); $category->usr = $user; $category->name = $name; - $category->position = $this->number($row, 'position') ?? 0; $this->entityManager->persist($category); diff --git a/src/Service/Backup/ConfigBackupUsers.php b/src/Service/Backup/ConfigBackupUsers.php index 0cff9d1f..43614286 100644 --- a/src/Service/Backup/ConfigBackupUsers.php +++ b/src/Service/Backup/ConfigBackupUsers.php @@ -624,9 +624,8 @@ final readonly class ConfigBackupUsers foreach ($this->inboxCategories->findForUserOrdered($user) as $category) { $exported[] = [ - 'id' => $category->id, - 'name' => $category->name, - 'position' => $category->position, + 'id' => $category->id, + 'name' => $category->name, ]; } diff --git a/src/Service/Mail/InboxCategoryManager.php b/src/Service/Mail/InboxCategoryManager.php index 3f74f976..25b42e09 100644 --- a/src/Service/Mail/InboxCategoryManager.php +++ b/src/Service/Mail/InboxCategoryManager.php @@ -57,7 +57,6 @@ final readonly class InboxCategoryManager $category = new InboxCategory(); $category->usr = $owner; $category->name = $this->validName($owner, $name, null); - $category->position = $this->categories->nextPositionFor($owner); $this->em->persist($category); $this->em->flush(); diff --git a/src/Service/Mail/InboxTabs.php b/src/Service/Mail/InboxTabs.php index dd7820ae..eb58a7c8 100644 --- a/src/Service/Mail/InboxTabs.php +++ b/src/Service/Mail/InboxTabs.php @@ -6,6 +6,8 @@ namespace App\Service\Mail; use App\Domain\DTO\Mail\InboxTab; use App\Domain\Enum\Mail\MessageCategory; +use App\Entity\Embeddable\CategorySorting; +use App\Entity\User\User; use App\Repository\Mail\InboxCategoryRepository; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Contracts\Service\ResetInterface; @@ -29,6 +31,14 @@ use Symfony\Contracts\Service\ResetInterface; * (ThreadListQueryBudgetTest), and that single read is why the inbox's own * ceiling is one higher than the other lists'. * + * ORDER AND ON/OFF ARE APPLIED HERE TOO, so no surface draws the strip its + * own way. Primary first, always; then CategorySorting::$tabOrder; then any tab + * that list does not name — built-ins in their usual order, the reader's own + * oldest first. A built-in the reader switched off is not a tab at all: not in + * the strip, not a drop target, and its key resolves to nothing, exactly like a + * deleted category's. The settings page is the one place that lists it, through + * managedFor(). + * * Memoised for the request, and resettable for the reason NewMailMarkers is: a * worker serves many readers one after another, and a memo that outlived its * request would show the first reader's tabs to the second. @@ -59,16 +69,61 @@ final class InboxTabs implements ResetInterface } /** - * Every tab the reader could be shown: the built-in five, then their own. + * Every tab the reader could be shown, in their order, without the built-in + * ones they switched off. * * @return list */ public function allFor(UserInterface $user): array { - return [ - ...array_map(InboxTab::builtIn(...), MessageCategory::cases()), - ...$this->customFor($user), - ]; + $off = self::sorting($user)->switchedOff(); + + return array_values(array_filter( + $this->managedFor($user), + static fn (InboxTab $tab): bool => null === $tab->builtIn || false === in_array($tab->builtIn, $off, true), + )); + } + + /** + * Every tab there is for this reader, switched off or not, in their order — + * what the settings page lists, with a switch beside each built-in. + * + * @return list + */ + public function managedFor(UserInterface $user): array + { + $byKey = []; + + foreach (MessageCategory::cases() as $category) { + $byKey[$category->value] = InboxTab::builtIn($category); + } + + foreach ($this->customFor($user) as $tab) { + $byKey[$tab->value] = $tab; + } + + $ordered = [MessageCategory::Primary->value => $byKey[MessageCategory::Primary->value]]; + + foreach (self::sorting($user)->tabOrder as $key) { + if (true === isset($byKey[$key]) && false === isset($ordered[$key])) { + $ordered[$key] = $byKey[$key]; + } + } + + // What the stored order does not name, in the order it was built in + // above: built-ins first, then the reader's own oldest first. A tab + // made after the reader last arranged the strip lands at its end. + return array_values($ordered + $byKey); + } + + /** + * The built-in categories the reader switched off. Primary never is. + * + * @return list + */ + public function switchedOffFor(UserInterface $user): array + { + return self::sorting($user)->switchedOff(); } /** @@ -79,7 +134,9 @@ final class InboxTabs implements ResetInterface $builtIn = MessageCategory::tryFrom($key); if (null !== $builtIn) { - return InboxTab::builtIn($builtIn); + // Switched off is no tab, like a deleted category: a stale link + // opens Primary and a drop onto it is refused. + return true === self::sorting($user)->isSwitchedOff($builtIn) ? null : InboxTab::builtIn($builtIn); } if (false === InboxTab::looksCustom($key)) { @@ -95,6 +152,12 @@ final class InboxTabs implements ResetInterface return InboxTab::parse($key, $names); } + /** The reader's sorting choices; the shipped ones for anything not a User. */ + private static function sorting(UserInterface $user): CategorySorting + { + return $user instanceof User ? $user->categorySorting : new CategorySorting(); + } + /** * @return list */ diff --git a/src/Service/Mail/MessageCategorizer.php b/src/Service/Mail/MessageCategorizer.php index f4402e25..041acdf8 100644 --- a/src/Service/Mail/MessageCategorizer.php +++ b/src/Service/Mail/MessageCategorizer.php @@ -67,6 +67,7 @@ final class MessageCategorizer true === $sorting?->overrideProvider, true === $sorting?->ignoresAi(), true === $sorting?->assistantFirst(), + $sorting?->switchedOff() ?? [], )['category']; } @@ -132,6 +133,15 @@ final class MessageCategorizer * the message view offers both answers side by side, so the difference * is something to look at rather than something to discover later. * + * @param list $switchedOff + * Categories the reader switched off (CategorySorting::$disabled). + * Whatever the cascade would file under one of them — a header rule, + * Google's label, the assistant — is in Primary instead, with the + * reason `switched_off` and the category it would have been as the + * signal. Applied to the ANSWER rather than by skipping steps: "Forums + * is off" means a mailing list is ordinary mail, not that the next + * rule down gets to call it Promotions for its List-Unsubscribe. + * * @return array{category: MessageCategory, reason: string, signal: string|null} */ public function explain( @@ -140,6 +150,34 @@ final class MessageCategorizer bool $ignoreProviderLabels = false, bool $ignoreAi = false, bool $aiFirst = false, + array $switchedOff = [], + ): array { + $decided = $this->decide($message, $correspondentEmails, $ignoreProviderLabels, $ignoreAi, $aiFirst); + + if (true === in_array($decided['category'], $switchedOff, true) && MessageCategory::Primary !== $decided['category']) { + return [ + 'category' => MessageCategory::Primary, + 'reason' => 'switched_off', + 'signal' => $decided['category']->value, + ]; + } + + return $decided; + } + + /** + * The cascade itself, before the reader's switched-off tabs are applied. + * + * @param array $correspondentEmails + * + * @return array{category: MessageCategory, reason: string, signal: string|null} + */ + private function decide( + Message $message, + array $correspondentEmails, + bool $ignoreProviderLabels, + bool $ignoreAi, + bool $aiFirst, ): array { $gmailLabelIds = $message->gmailLabelIds; diff --git a/src/Twig/MessageCategoryExtension.php b/src/Twig/MessageCategoryExtension.php index 868b8d83..ebdc1594 100644 --- a/src/Twig/MessageCategoryExtension.php +++ b/src/Twig/MessageCategoryExtension.php @@ -156,6 +156,7 @@ final class MessageCategoryExtension extends AbstractExtension $sorting->overrideProvider, $sorting->ignoresAi(), $sorting->assistantFirst(), + $sorting->switchedOff(), ), // Gmail's own answer, on its own line, because it is one of the @@ -188,7 +189,8 @@ final class MessageCategoryExtension extends AbstractExtension } /** - * @param array $correspondents + * @param array $correspondents + * @param list $switchedOff * * @return array{category: string, reason: string, signal: string|null} */ @@ -198,6 +200,7 @@ final class MessageCategoryExtension extends AbstractExtension bool $ignoreProviderLabels, bool $ignoreAi, bool $aiFirst = false, + array $switchedOff = [], ): array { $explanation = $this->categorizer->explain( $message, @@ -205,6 +208,7 @@ final class MessageCategoryExtension extends AbstractExtension $ignoreProviderLabels, $ignoreAi, $aiFirst, + $switchedOff, ); return [ diff --git a/templates/mail/_message_details.html.twig b/templates/mail/_message_details.html.twig index 3b437f6f..15e0b12d 100644 --- a/templates/mail/_message_details.html.twig +++ b/templates/mail/_message_details.html.twig @@ -267,6 +267,10 @@ mailed-by and signed-by were silently missing from every IMAP message. — {{ 'message.details.categorised.pinned'|trans }} {%- elseif category.own %} — {{ 'message.details.categorised.own_filter'|trans }} + {%- elseif category.effective.reason == 'switched_off' %} + {#- The signal is the category it would have been, named + the way its tab was. #} + — {{ 'message.details.categorised.switched_off'|trans({'%category%': ('tabs.' ~ category.effective.signal)|trans}) }} {%- else %} — {{ ('message.details.categorised.' ~ category.effective.reason)|trans }} {%- if category.effective.signal %} diff --git a/tests/Entity/Mail/InboxCategoryTest.php b/tests/Entity/Mail/InboxCategoryTest.php index 22db0b91..ef1c2a6e 100644 --- a/tests/Entity/Mail/InboxCategoryTest.php +++ b/tests/Entity/Mail/InboxCategoryTest.php @@ -52,21 +52,21 @@ final class InboxCategoryTest extends KernelTestCase parent::tearDown(); } - public function testACategoryIsStoredForItsOwnerAndKeepsItsOrder(): void + public function testACategoryIsStoredForItsOwner(): void { $account = $this->seedAccount(); - $first = $this->category($account->usr, 'Shipping', 0); - $second = $this->category($account->usr, 'Receipts', 1); + $first = $this->category($account->usr, 'Shipping'); + $second = $this->category($account->usr, 'Receipts'); $this->em->clear(); $rows = $this->connection->fetchAllAssociative( - 'SELECT name, position FROM inbox_category WHERE usr_id = :usr ORDER BY position', + 'SELECT name FROM inbox_category WHERE usr_id = :usr ORDER BY id', ['usr' => $account->usr->id], ); - self::assertSame([['name' => 'Shipping', 'position' => 0], ['name' => 'Receipts', 'position' => 1]], $rows); + self::assertSame([['name' => 'Shipping'], ['name' => 'Receipts']], $rows); self::assertNotNull($first->id); self::assertNotNull($second->id); } @@ -76,7 +76,7 @@ final class InboxCategoryTest extends KernelTestCase // A user with no account: an account's own foreign key to its user is // not what this is about, and would refuse the delete before it began. $user = $this->seedUser(); - $this->category($user, 'Shipping', 0); + $this->category($user, 'Shipping'); $userId = (int) $user->id; $this->connection->executeStatement('DELETE FROM "user" WHERE id = :id', ['id' => $userId]); @@ -96,8 +96,8 @@ final class InboxCategoryTest extends KernelTestCase public function testRemovingACategoryUnfilesItsMailAndThreadsAndNothingElse(): void { $account = $this->seedAccount(); - $doomed = $this->category($account->usr, 'Shipping', 0); - $survivor = $this->category($account->usr, 'Receipts', 1); + $doomed = $this->category($account->usr, 'Shipping'); + $survivor = $this->category($account->usr, 'Receipts'); $inDoomed = $this->threadWithMessage($account, 'filed under the doomed category'); $inSurvivor = $this->threadWithMessage($account, 'filed under the surviving category'); @@ -137,12 +137,11 @@ final class InboxCategoryTest extends KernelTestCase // ── fixture ────────────────────────────────────────────────────────────── - private function category(User $user, string $name, int $position): InboxCategory + private function category(User $user, string $name): InboxCategory { - $category = new InboxCategory(); - $category->usr = $user; - $category->name = $name; - $category->position = $position; + $category = new InboxCategory(); + $category->usr = $user; + $category->name = $name; $this->em->persist($category); $this->em->flush(); diff --git a/tests/Repository/Mail/InboxTabQueriesTest.php b/tests/Repository/Mail/InboxTabQueriesTest.php index ae1c7ce4..532c3b9b 100644 --- a/tests/Repository/Mail/InboxTabQueriesTest.php +++ b/tests/Repository/Mail/InboxTabQueriesTest.php @@ -171,6 +171,37 @@ final class InboxTabQueriesTest extends KernelTestCase self::assertSame(['Promo Co'], $senders['promotions'] ?? null); } + /** + * Promotions switched off: its conversation is listed and counted under + * Primary — until the re-sort that the switch queues has moved it, and for + * good when somebody had pinned it there. The one filed in the reader's own + * tab stays where it is; switching a built-in off says nothing about that. + */ + public function testASwitchedOffCategoryIsListedAndCountedUnderPrimary(): void + { + $this->user->categorySorting->switchOff([MessageCategory::Promotions]); + + $primary = InboxTab::builtIn(MessageCategory::Primary); + + self::assertSame( + ['primary, read', 'promo, new'], + $this->subjects($this->repository->findForUnifiedInbox($this->user, $primary)), + ); + self::assertSame(2, $this->repository->countForUnifiedInbox($this->user, $primary)); + + $key = 'c' . $this->shipping->id; + + self::assertEquals( + [$key => 2, 'primary' => 2], + $this->sorted($this->repository->countByCategoryForUnifiedInbox($this->user)), + ); + self::assertEquals( + [$key => 1, 'primary' => 1], + $this->sorted($this->repository->countUnreadByCategoryForUnifiedInbox($this->user)), + ); + self::assertSame(['Promo Co'], $this->repository->newSendersByCategoryForUnifiedInbox($this->user, new DateTimeImmutable())['primary'] ?? null); + } + // ── fixture ────────────────────────────────────────────────────────────── /** diff --git a/tests/Service/Backup/ConfigBackupInboxCategoriesTest.php b/tests/Service/Backup/ConfigBackupInboxCategoriesTest.php index 331b7cae..3135cee6 100644 --- a/tests/Service/Backup/ConfigBackupInboxCategoriesTest.php +++ b/tests/Service/Backup/ConfigBackupInboxCategoriesTest.php @@ -104,6 +104,31 @@ final class ConfigBackupInboxCategoriesTest extends KernelTestCase ); } + /** + * The order of the strip and the switched-off tabs come back too, with the + * order's own tabs named by the ids the restore gave them. A key the file + * carries no category for is dropped. + */ + public function testTheTabOrderAndTheSwitchedOffTabsComeBackWithTheirNewIds(): void + { + $this->seedReader(); + + $document = $this->exporter->document(); + + $this->becomeADifferentInstallation(); + $this->importer->apply($document); + $this->entityManager->flush(); + $this->entityManager->clear(); + + $user = static::getContainer()->get(UserRepository::class)->findOneBy(['email' => self::EMAIL]); + self::assertInstanceOf(User::class, $user); + + [$shipping, $receipts] = static::getContainer()->get(InboxCategoryRepository::class)->findForUserOrdered($user); + + self::assertSame([$receipts->tabKey(), 'promotions', $shipping->tabKey()], $user->categorySorting->tabOrder); + self::assertSame(['forums'], $user->categorySorting->disabled); + } + /** * A reference to a category the file does not carry is dropped, not carried * as a number that now names somebody else's row or nothing at all. The @@ -162,9 +187,9 @@ final class ConfigBackupInboxCategoriesTest extends KernelTestCase $document = $this->exporter->document(); $document['users'][self::EMAIL]['inboxCategories'] = [ - ['id' => 1, 'name' => str_repeat('x', 60), 'position' => 0], - ['id' => 2, 'name' => ' ', 'position' => 1], - ['id' => 3, 'name' => 'Receipts', 'position' => 2], + ['id' => 1, 'name' => str_repeat('x', 60)], + ['id' => 2, 'name' => ' '], + ['id' => 3, 'name' => 'Receipts'], ]; $this->becomeADifferentInstallation(); @@ -199,20 +224,23 @@ final class ConfigBackupInboxCategoriesTest extends KernelTestCase $user->roles = ['ROLE_USER']; $this->entityManager->persist($user); - $shipping = new InboxCategory(); - $shipping->usr = $user; - $shipping->name = 'Shipping'; - $shipping->position = 0; + $shipping = new InboxCategory(); + $shipping->usr = $user; + $shipping->name = 'Shipping'; $this->entityManager->persist($shipping); - $receipts = new InboxCategory(); - $receipts->usr = $user; - $receipts->name = 'Receipts'; - $receipts->position = 1; + $receipts = new InboxCategory(); + $receipts->usr = $user; + $receipts->name = 'Receipts'; $this->entityManager->persist($receipts); $this->entityManager->flush(); + // Receipts before Promotions before Shipping, and a key for a category + // that is gone; Forums switched off. + $user->categorySorting->arrange([$receipts->tabKey(), 'promotions', $shipping->tabKey(), 'c999999']); + $user->categorySorting->switchOff([\App\Domain\Enum\Mail\MessageCategory::Forums]); + $rule = new MailRule(); $rule->usr = $user; $rule->name = 'Carriers'; diff --git a/tests/Service/Mail/InboxCategoryManagerTest.php b/tests/Service/Mail/InboxCategoryManagerTest.php index 5203a580..7f07ed59 100644 --- a/tests/Service/Mail/InboxCategoryManagerTest.php +++ b/tests/Service/Mail/InboxCategoryManagerTest.php @@ -67,15 +67,12 @@ final class InboxCategoryManagerTest extends KernelTestCase parent::tearDown(); } - public function testACategoryIsTrimmedAndAppendedAfterTheOnesThereAlready(): void + public function testACategoryIsTrimmed(): void { - $first = $this->manager->create($this->user, ' Shipping '); - $second = $this->manager->create($this->user, 'Receipts'); + $first = $this->manager->create($this->user, ' Shipping '); self::assertNotNull($first->id); self::assertSame('Shipping', $first->name); - self::assertSame(0, $first->position); - self::assertSame(1, $second->position, 'a new tab did not go to the end of the strip'); } /** diff --git a/tests/Service/Mail/InboxTabsTest.php b/tests/Service/Mail/InboxTabsTest.php index f90c15ba..f1e05370 100644 --- a/tests/Service/Mail/InboxTabsTest.php +++ b/tests/Service/Mail/InboxTabsTest.php @@ -74,7 +74,7 @@ final class InboxTabsTest extends TestCase self::assertNull($tabs->resolve($this->user('stranger'), 'c12'), 'resolved a category the asking reader does not own'); } - public function testAllForListsTheFiveThenTheReadersOwnInTheirOrder(): void + public function testWithNoOrderStoredTheFiveComeFirstThenTheReadersOwnOldestFirst(): void { $repository = $this->createStub(InboxCategoryRepository::class); $repository->method('findForUserOrdered')->willReturn([$this->category('Shipping', 12), $this->category('Receipts', 40)]); @@ -90,6 +90,47 @@ final class InboxTabsTest extends TestCase ); } + /** + * Primary first whatever the stored order says; then the reader's order, + * mixing built-ins and their own; then whatever the order does not name. + * A stale key — a category since deleted — is skipped. + */ + public function testTheReadersOrderPutsTheirTabsAmongTheBuiltInsAfterPrimary(): void + { + $repository = $this->createStub(InboxCategoryRepository::class); + $repository->method('findForUserOrdered')->willReturn([$this->category('Shipping', 12), $this->category('Receipts', 40)]); + + $reader = $this->user('reader'); + $reader->categorySorting->tabOrder = ['c40', 'updates', 'primary', 'c99', 'social']; + + self::assertSame( + ['primary', 'c40', 'updates', 'social', 'promotions', 'forums', 'c12'], + $this->keys((new InboxTabs($repository))->allFor($reader)), + ); + } + + /** + * A switched-off built-in is no tab: not listed, and its key resolves to + * nothing, as a deleted category's does. The settings page still lists it, + * in its place, so it can be switched back on. + */ + public function testASwitchedOffBuiltInIsNoTabButIsStillManaged(): void + { + $repository = $this->createStub(InboxCategoryRepository::class); + $repository->method('findForUserOrdered')->willReturn([]); + + $reader = $this->user('reader'); + $reader->categorySorting->switchOff([MessageCategory::Forums, MessageCategory::Primary]); + + $tabs = new InboxTabs($repository); + + self::assertNotContains('forums', $this->keys($tabs->allFor($reader))); + self::assertContains('primary', $this->keys($tabs->allFor($reader)), 'Primary cannot be switched off'); + self::assertNull($tabs->resolve($reader, 'forums')); + self::assertSame('updates', $tabs->resolve($reader, 'updates')?->value); + self::assertSame(['primary', 'social', 'promotions', 'updates', 'forums'], $this->keys($tabs->managedFor($reader))); + } + public function testAResetForgetsTheLastReader(): void { $repository = $this->createMock(InboxCategoryRepository::class); @@ -102,6 +143,16 @@ final class InboxTabsTest extends TestCase $tabs->customFor($this->user('reader')); } + /** + * @param list<\App\Domain\DTO\Mail\InboxTab> $tabs + * + * @return list + */ + private function keys(array $tabs): array + { + return array_map(static fn ($tab): string => $tab->value, $tabs); + } + private function user(string $email): User { $user = new User(); diff --git a/tests/Service/Mail/MessageCategorizerTest.php b/tests/Service/Mail/MessageCategorizerTest.php index e12f6f05..27606f46 100644 --- a/tests/Service/Mail/MessageCategorizerTest.php +++ b/tests/Service/Mail/MessageCategorizerTest.php @@ -427,6 +427,61 @@ final class MessageCategorizerTest extends TestCase ); } + /** + * A switched-off category takes no mail, whoever would have put it there — + * the header rules, Google's label or the assistant — and what it would + * have taken is in Primary. Not the next rule down: a mailing list carries + * List-Unsubscribe too, and "Forums is off" does not mean "lists are + * adverts". + * + * @return iterable + */ + public static function switchedOffDeciders(): iterable + { + $list = new Message(); + $list->headers = ['List-Post' => '', 'List-Unsubscribe' => '']; + $list->fromAddress = 'dev@lists.test'; + yield 'the header rules' => [$list, null]; + + $gmail = new Message(); + $gmail->fromAddress = 'dev@lists.test'; + $gmail->gmailLabelIds = ['INBOX', 'CATEGORY_FORUMS']; + yield "Google's label" => [$gmail, null]; + + $asked = new Message(); + $asked->fromAddress = 'someone@example.test'; + $asked->aiCategory = MessageCategory::Forums; + $assistant = new CategorySorting(); + $assistant->source = 'assistant'; + yield 'the assistant' => [$asked, $assistant]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('switchedOffDeciders')] + public function testASwitchedOffCategoryTakesNoMail(Message $message, ?CategorySorting $sorting): void + { + $sorting ??= new CategorySorting(); + + self::assertSame(MessageCategory::Forums, $this->categorizer->categorize($message, [], $sorting), 'the fixture must decide Forums while it is on'); + + $sorting->switchOff([MessageCategory::Forums]); + + self::assertSame(MessageCategory::Primary, $this->categorizer->categorize($message, [], $sorting)); + + $explained = $this->categorizer->explain($message, [], $sorting->overrideProvider, $sorting->ignoresAi(), $sorting->assistantFirst(), $sorting->switchedOff()); + self::assertSame('switched_off', $explained['reason']); + self::assertSame('forums', $explained['signal'], 'the panel names what it would have been'); + } + + public function testSwitchingOneCategoryOffLeavesTheOthersAlone(): void + { + $sorting = new CategorySorting(); + $sorting->switchOff([MessageCategory::Forums]); + + $newsletter = $this->message(['List-Unsubscribe' => ''], 'newsletter@shop.test'); + + self::assertSame(MessageCategory::Promotions, $this->categorizer->categorize($newsletter, [], $sorting)); + } + /** * No preference is the shipped behaviour, unchanged. * diff --git a/translations/messages.de.yaml b/translations/messages.de.yaml index dc87b5fa..2bff892a 100644 --- a/translations/messages.de.yaml +++ b/translations/messages.de.yaml @@ -2344,6 +2344,7 @@ message: pinned: 'du hast sie hierher verschoben' promotion: 'ein Massenmail-Header: ' social: 'die Absenderdomain ' + switched_off: "es wäre %category%, und das hast du ausgeschaltet" thread_differs: 'Diese Nachricht allein wäre %category% — eine Unterhaltung übernimmt die Kategorie ihrer neuesten Nachricht.' update: 'ein Hinweis auf automatischen Versand: ' category: Kategorie diff --git a/translations/messages.en.yaml b/translations/messages.en.yaml index 4f564a99..865951e3 100644 --- a/translations/messages.en.yaml +++ b/translations/messages.en.yaml @@ -2351,6 +2351,7 @@ message: pinned: 'you moved it here' promotion: 'a bulk-mail header: ' social: 'the sender domain ' + switched_off: "it would be %category%, which you switched off" # A conversation takes ONE category, from its newest message, so a # reply can carry the whole thread out of the tab this message # belongs in. Said outright, because without it the panel reads as diff --git a/translations/messages.en_PI.yaml b/translations/messages.en_PI.yaml index b41abec7..da63ea7e 100644 --- a/translations/messages.en_PI.yaml +++ b/translations/messages.en_PI.yaml @@ -2223,6 +2223,7 @@ message: pinned: 'ye hauled it here yerself' promotion: 'a bulk-cargo header: ' social: 'the sender port ' + switched_off: "it'd be %category%, an' ye scuttled that one" thread_differs: "This message alone would be %category% — a yarn takes th' category o' its newest message." update: 'a sign of an unmanned sender: ' category: rank diff --git a/translations/messages.fr.yaml b/translations/messages.fr.yaml index b911e6a2..59161f2d 100644 --- a/translations/messages.fr.yaml +++ b/translations/messages.fr.yaml @@ -2362,6 +2362,7 @@ message: pinned: 'vous l’avez déplacé ici' promotion: 'un en-tête de courrier de masse : ' social: 'le domaine de l’expéditeur ' + switched_off: "ce serait %category%, que vous avez désactivé" # A conversation takes ONE category, from its newest message, so a # reply can carry the whole thread out of the tab this message # belongs in. Said outright, because without it the panel reads as