From bcb1192ce06252fedffab04e0d01ebba1f0160f3 Mon Sep 17 00:00:00 2001 From: mani <15911342+Flouss@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:56:35 +0200 Subject: [PATCH] feat(mail): drag a conversation onto, and out of, the reader's own tab Dropping on one of the reader's own tabs files the conversation there and pins it, leaving the built-in category alone (JMAP clients are served that column). Dropping on a built-in tab from one clears the reader's tab, so a conversation is never in two. "Already there" is a statement about both columns. The route resolves the posted key against the dropping reader's own categories, so somebody else's category is refused like a made-up one and nothing moves. Co-Authored-By: Claude Sonnet 5.5 --- src/Controller/Mail/BulkStatusController.php | 22 ++- src/Service/Mail/ThreadStatusUpdater.php | 29 +++- .../thread/status/_category.stream.html.twig | 4 +- .../Controller/Mail/BulkCategoryDropTest.php | 151 ++++++++++++++++++ tests/Service/Mail/ThreadMoveToOwnTabTest.php | 129 +++++++++++++++ 5 files changed, 326 insertions(+), 9 deletions(-) create mode 100644 tests/Controller/Mail/BulkCategoryDropTest.php create mode 100644 tests/Service/Mail/ThreadMoveToOwnTabTest.php diff --git a/src/Controller/Mail/BulkStatusController.php b/src/Controller/Mail/BulkStatusController.php index 3d31eac0..d5ece411 100644 --- a/src/Controller/Mail/BulkStatusController.php +++ b/src/Controller/Mail/BulkStatusController.php @@ -8,13 +8,14 @@ use App\Controller\ChecksCsrf; use App\Controller\RendersTurboStreams; use App\Domain\Enum\Job\JobKind; use App\Domain\Enum\Mail\LabelRole; -use App\Domain\Enum\Mail\MessageCategory; use App\Entity\Job\BackgroundJob; use App\Entity\User\User; use App\Infrastructure\Messaging\Message\RunBulkStatusMessage; use App\Repository\Label\LabelRepository; +use App\Repository\Mail\InboxCategoryRepository; use App\Repository\Mail\MessageThreadRepository; use App\Security\Voter\OwnershipVoter; +use App\Service\Mail\InboxTabs; use App\Service\Mail\MailPlacement; use App\Service\Mail\MessagePurger; use App\Service\Mail\MoveToService; @@ -91,6 +92,8 @@ final class BulkStatusController extends AbstractController private readonly StatusUndoService $undo, private readonly MessagePurger $purger, private readonly MailPlacement $placement, + private readonly InboxTabs $inboxTabs, + private readonly InboxCategoryRepository $inboxCategories, ) { } @@ -211,7 +214,10 @@ final class BulkStatusController extends AbstractController $destination = null; if ('category' === $action) { - $category = MessageCategory::tryFrom((string) ($body['category'] ?? '')); + // Resolved against THIS reader's own tabs as well as the five, and + // a key that is somebody else's category is refused exactly like a + // made-up one: see InboxTabs::resolve(). + $category = $this->inboxTabs->resolve($user, (string) ($body['category'] ?? '')); // tryFrom and a refusal, where MailController falls back to Primary // on the same input. The difference is what the two are: `?tab=` is @@ -397,8 +403,18 @@ final class BulkStatusController extends AbstractController // strip counts — so this one is applied to the threads directly and // returns before the grouping it has no use for. if ('category' === $action) { + // The entity for one of the reader's own tabs, the enum for a + // built-in one: setCategory() writes whichever column it names. + $target = true === $category->isCustom() + ? $this->inboxCategories->find((int) $category->categoryId) + : $category->builtIn; + + if (null === $target) { + throw $this->createAccessDeniedException('Unknown inbox category.'); + } + foreach ($threads as $thread) { - $this->status->setCategory($thread, $category); + $this->status->setCategory($thread, $target); } return $this->renderTurboStream('thread/status/_category.stream.html.twig', [ diff --git a/src/Service/Mail/ThreadStatusUpdater.php b/src/Service/Mail/ThreadStatusUpdater.php index 39ad01c7..e2c2e2ae 100644 --- a/src/Service/Mail/ThreadStatusUpdater.php +++ b/src/Service/Mail/ThreadStatusUpdater.php @@ -10,6 +10,7 @@ use App\Infrastructure\Messaging\Message\ExtractEventsMessage; use App\Domain\DTO\Mail\RemoteFlagState; use App\Domain\Enum\Mail\LabelRole; use App\Domain\Enum\Mail\MessageCategory; +use App\Entity\Mail\InboxCategory; use App\Domain\Enum\Mail\MessageFlag; use App\Entity\Label\Label; use App\Entity\Mail\Account; @@ -370,13 +371,33 @@ final readonly class ThreadStatusUpdater * * @return bool whether anything changed */ - public function setCategory(MessageThread $thread, MessageCategory $category): bool + public function setCategory(MessageThread $thread, MessageCategory|InboxCategory $category): bool { - if ($category === $thread->category && null !== $thread->categoryPinnedAt) { - return false; + // TWO COLUMNS, AND A DROP WRITES THE RIGHT ONE. The reader's own tab is + // MessageThread::$userCategory and a built-in one is $category, and a + // conversation is in exactly one tab, so moving between the two kinds has + // to clear the other: a drop onto Primary from "Shipping" that left + // $userCategory set would leave it in Shipping, and a drop onto Shipping + // leaves the built-in column alone because that one is what JMAP serves. + // + // "Already there" is therefore a statement about BOTH columns. A + // conversation whose built-in category matches the drop but which is + // still in one of the reader's tabs is not already there. + if ($category instanceof InboxCategory) { + if ($category === $thread->userCategory && null !== $thread->categoryPinnedAt) { + return false; + } + + $thread->userCategory = $category; + } else { + if ($category === $thread->category && null === $thread->userCategory && null !== $thread->categoryPinnedAt) { + return false; + } + + $thread->category = $category; + $thread->userCategory = null; } - $thread->category = $category; $thread->categoryPinnedAt = new DateTimeImmutable(); // The thread's own row moved between two tabs; no message changed, so diff --git a/templates/thread/status/_category.stream.html.twig b/templates/thread/status/_category.stream.html.twig index 6ca0f896..66c376a4 100644 --- a/templates/thread/status/_category.stream.html.twig +++ b/templates/thread/status/_category.stream.html.twig @@ -13,7 +13,7 @@ Saying where it went is the whole of the feedback. Variables: count how many conversations were re-filed threads the conversations, so their rows can be removed - category the destination tab, for the toast + category the destination tab (an InboxTab), for the toast #} {% include 'thread/status/_bulk.stream.html.twig' with { count: count, @@ -21,4 +21,4 @@ Variables: leaves: true, } only %} -{% include '_partials/_toast.stream.html.twig' with { message: 'toast.categorised'|trans({ '%category%': ('tabs.' ~ category.value)|trans }) } only %} +{% include '_partials/_toast.stream.html.twig' with { message: 'toast.categorised'|trans({ '%category%': category.isCustom ? category.name : ('tabs.' ~ category.value)|trans }) } only %} diff --git a/tests/Controller/Mail/BulkCategoryDropTest.php b/tests/Controller/Mail/BulkCategoryDropTest.php new file mode 100644 index 00000000..5f676939 --- /dev/null +++ b/tests/Controller/Mail/BulkCategoryDropTest.php @@ -0,0 +1,151 @@ +client = static::createClient(); + $this->client->disableReboot(); + + $container = static::getContainer(); + $this->em = $container->get(EntityManagerInterface::class); + $this->connection = $container->get(Connection::class); + + $this->connection->beginTransaction(); + + $this->user = $this->seedUser(); + $this->account = $this->seedAccount(); + $this->inbox = $this->seedLabel('Inbox', LabelRole::Inbox); + + $this->client->loginUser($this->user); + } + + protected function tearDown(): void + { + if (true === isset($this->connection) && true === $this->connection->isTransactionActive()) { + $this->connection->rollBack(); + } + + parent::tearDown(); + } + + public function testADropOnTheReadersOwnTabFilesTheConversationThere(): void + { + $shipping = $this->category($this->user, 'Shipping'); + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + + $this->drop('c' . $shipping->id, [(int) $thread->id]); + + self::assertResponseIsSuccessful(); + + $filed = $this->reload($thread); + self::assertSame($shipping->id, $filed->userCategory?->id); + self::assertNotNull($filed->categoryPinnedAt); + self::assertSame(MessageCategory::Promotions, $filed->category); + } + + public function testADropOnABuiltInTabStillWorksAndTakesTheConversationOutOfTheReadersTab(): void + { + $shipping = $this->category($this->user, 'Shipping'); + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + $thread->userCategory = $shipping; + $this->em->flush(); + + $this->drop('primary', [(int) $thread->id]); + + self::assertResponseIsSuccessful(); + + $moved = $this->reload($thread); + self::assertNull($moved->userCategory); + self::assertSame(MessageCategory::Primary, $moved->category); + } + + public function testSomebodyElsesCategoryIsRefusedAndNothingMoves(): void + { + $theirs = $this->category($this->seedUser(), 'Theirs'); + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + + $this->drop('c' . $theirs->id, [(int) $thread->id]); + + self::assertResponseStatusCodeSame(403); + + $untouched = $this->reload($thread); + self::assertNull($untouched->userCategory, 'a conversation was filed under somebody else\'s category'); + self::assertNull($untouched->categoryPinnedAt); + } + + // ── fixture ────────────────────────────────────────────────────────────── + + /** + * The row as the database has it now. The request cleared the entity + * manager, so the object the test holds is detached and refreshing it + * would throw. + */ + private function reload(MessageThread $thread): MessageThread + { + $id = $thread->id; + + $this->em->clear(); + + $fresh = $this->em->find(MessageThread::class, $id); + self::assertNotNull($fresh); + + return $fresh; + } + + /** @param list $ids */ + private function drop(string $key, array $ids): void + { + $crawler = $this->client->request('GET', '/mail/inbox'); + $token = (string) $crawler->filter('meta[name="csrf-token"]')->attr('content'); + + $this->client->request( + 'POST', + '/status/bulk/category', + [], + [], + ['CONTENT_TYPE' => 'application/json', 'HTTP_X_CSRF_TOKEN' => $token], + (string) json_encode(['ids' => $ids, 'category' => $key]), + ); + } + + private function category(User $owner, string $name): InboxCategory + { + $category = new InboxCategory(); + $category->usr = $owner; + $category->name = $name; + $this->em->persist($category); + $this->em->flush(); + + return $category; + } +} diff --git a/tests/Service/Mail/ThreadMoveToOwnTabTest.php b/tests/Service/Mail/ThreadMoveToOwnTabTest.php new file mode 100644 index 00000000..904900e2 --- /dev/null +++ b/tests/Service/Mail/ThreadMoveToOwnTabTest.php @@ -0,0 +1,129 @@ +em = $container->get(EntityManagerInterface::class); + $this->connection = $container->get(Connection::class); + $this->status = $container->get(ThreadStatusUpdater::class); + + $this->connection->beginTransaction(); + + $this->user = $this->seedUser(); + $this->account = $this->seedAccount(); + $this->inbox = $this->seedLabel('Inbox', LabelRole::Inbox); + + $this->shipping = new InboxCategory(); + $this->shipping->usr = $this->user; + $this->shipping->name = 'Shipping'; + $this->em->persist($this->shipping); + $this->em->flush(); + } + + protected function tearDown(): void + { + if (true === $this->connection->isTransactionActive()) { + $this->connection->rollBack(); + } + + parent::tearDown(); + } + + public function testADropOnTheReadersOwnTabFilesAndPinsItAndLeavesTheBuiltInCategoryAlone(): void + { + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + + self::assertTrue($this->status->setCategory($thread, $this->shipping)); + + self::assertSame($this->shipping, $thread->userCategory); + self::assertNotNull($thread->categoryPinnedAt, 'the drop is not pinned, so the next message undoes it'); + self::assertSame( + MessageCategory::Promotions, + $thread->category, + 'a drop on the reader\'s own tab rewrote the built-in category JMAP serves', + ); + } + + public function testADropBackOntoABuiltInTabTakesItOutOfTheReadersTab(): void + { + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + $this->status->setCategory($thread, $this->shipping); + + self::assertTrue($this->status->setCategory($thread, MessageCategory::Primary)); + + self::assertNull($thread->userCategory, 'the conversation is in a built-in tab AND the reader\'s own'); + self::assertSame(MessageCategory::Primary, $thread->category); + self::assertNotNull($thread->categoryPinnedAt); + } + + /** + * Dropping where it already is changes nothing, and says so — for the reader's + * own tab as for the five. The companion is the pair above: the same call on + * a conversation that is NOT there yet does change it. + */ + public function testADropWhereTheConversationAlreadyIsIsNotAChange(): void + { + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + + self::assertTrue($this->status->setCategory($thread, $this->shipping)); + self::assertFalse($this->status->setCategory($thread, $this->shipping), 'a repeated drop reported a change'); + + self::assertTrue($this->status->setCategory($thread, MessageCategory::Primary)); + self::assertFalse($this->status->setCategory($thread, MessageCategory::Primary)); + } + + /** + * A built-in drop onto a conversation that is pinned to a built-in tab but + * ALSO has a tab of the reader's: the built-in column already matches, and + * "nothing to do" would leave it in the reader's tab. The case the old + * equality check would have got wrong. + */ + public function testABuiltInDropStillMovesAConversationWhoseBuiltInCategoryAlreadyMatches(): void + { + $thread = $this->thread('a notice', MessageCategory::Promotions, unread: 1); + $thread->userCategory = $this->shipping; + $thread->categoryPinnedAt = new \DateTimeImmutable(); + $this->em->flush(); + + self::assertTrue($this->status->setCategory($thread, MessageCategory::Promotions)); + + self::assertNull($thread->userCategory); + } +}