Hear mail read, unread, starred or unstarred in Outlook (#114)

Co-authored-by: Flouss <15911342+Flouss@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -8,6 +8,21 @@ The published image tags: `latest` follows the most recent release below,
 
 ## Unreleased
 
+**Mail read, unread, starred or unstarred in Outlook now reaches plMail.**
+An Outlook account follows each folder with a change feed, and the feed was begun asking for a
+message's id, folder and Message-ID only. Exchange reports a change in the properties a feed asks
+for, so a message whose only change was being read, unread, flagged or unflagged was not reported
+at all: mail handled in Outlook, on a phone or by a rule stayed as plMail had last seen it. (The
+other direction, plMail to Outlook, was not affected.) The feed now asks for both. A feed already
+running keeps what it was begun with, so after the upgrade every Outlook folder is listed once more
+and followed from a new feed; the account shows as importing until that pass is done, which costs
+its listing pages and fetches nothing it already has. Each message listed is also compared with the
+server's read state and star, so the few that drifted while nothing was listening move to match the
+server: a message plMail has as read that Outlook still has unread goes back to unread, and the
+other way round, so a badge can go up as well as down. **Migration:** clears the stored change feeds
+of Outlook accounts; nothing else changes in the schema.
+[#114](https://github.com/karatektus/pl_mail/pull/114), by @Flouss.
+
 **Saving an attachment to Paperless-ngx works.** The "Save to…" chooser listed your Paperless tags
 as folders to open, but a tag has nothing inside it, so clicking one only reloaded the same list and
 the dialog seemed to do nothing. A tag is now the choice itself: click it and the attachment is filed

migrations/Version20261014110000.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+namespace DoctrineMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Follow every Outlook folder again, with a delta that reports a read.
+ *
+ * A folder is followed by a delta link, and the link carries the $select the
+ * chain began with. Until now that left out isRead and flag, and Exchange
+ * does not send an entry for a message whose only change is one of those, so a
+ * message read or unread in Outlook, on a phone or by a rule was never heard
+ * of here. New chains now ask for both (GraphApiClient::DELTA_SELECT); the
+ * ones already stored cannot be widened, only replaced.
+ *
+ * Clearing the links is how: a folder without one is the import's, the next
+ * sync plans it, and the import lists the folder from the top with the new
+ * select and ends by storing a link that has it. What is already stored is
+ * recognised and not fetched again, so a mailbox that lost nothing costs its
+ * listing pages. Every listed message also has its read state and star
+ * compared with the server's, which settles the rows that drifted while
+ * nothing was listening.
+ *
+ * Until a folder's listing has finished the account shows as importing, which
+ * is what it is, and mail that arrives meanwhile is still picked up (see
+ * GraphApiSyncer::receivedSince()).
+ */
+final class Version20261014110000 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Hand every Outlook folder back to the import once, so it is followed by a delta that reports read state';
+    }
+
+    public function up(Schema $schema): void
+    {
+        $this->addSql(<<<'SQL'
+            UPDATE account
+            SET graph_delta_links = '{}'
+            WHERE auth_type = 'oauth2' AND oauth_provider = 'microsoft'
+            SQL);
+    }
+
+    public function down(Schema $schema): void
+    {
+        // Nothing to put back: the old links were the ones that could not hear
+        // a read, and a listing that has run again is no less complete.
+    }
+}

phpstan.dist.neon

@@ -45,6 +45,12 @@ parameters:
         - tests
         - importmap.php
 
+    # Read for its classes, not analysed: migrations are generated and nobody
+    # edits them by hand, but a test that runs one (tests/Migrations) names its
+    # class, and without this that is a class PHPStan has never heard of.
+    scanDirectories:
+        - migrations
+
     excludePaths:
         analyseAndScan:
             # Not PHP.
@@ -70,9 +76,9 @@ parameters:
             - playwright (?)
             - playwright-report (?)
             - test-results (?)
-            # Generated: 54 migrations nobody edits by hand, and a ~1000-line
-            # dump of the framework config tree that exists to be read.
-            - migrations
+            # Generated: a ~1000-line dump of the framework config tree that
+            # exists to be read. (migrations/ is not analysed either, by not
+            # being in `paths`; see scanDirectories.)
             - config/reference.php (?)
             # Twig extensions lifted from upstream rather than written here.
             # NOTE: only Vendor/ — the rest of src/Twig is application code

src/Service/Graph/GraphApiSyncer.php

@@ -572,12 +572,16 @@ final class GraphApiSyncer
 
             // Read state used to stop here. The comment above this block said
             // the only cheap thing that could have changed was location, and it
-            // was wrong: a Graph delta entry carries the whole message resource,
-            // isRead and flag included, so a message read in Outlook announced
-            // itself in this very payload and plMail dropped it. Mail read on a
+            // was wrong: a Graph delta entry carries the properties the chain
+            // was begun with, and with isRead and flag among them a message read
+            // in Outlook announced itself in this very payload. Mail read on a
             // phone stayed unread here, exactly as it did on IMAP, and for the
             // same reason — nothing ever re-read the state of a row it already
             // had.
+            //
+            // That holds only for a chain begun with them: GraphApiClient::
+            // DELTA_SELECT. One begun without is sent no entry at all for a
+            // message that was merely read, so this block cannot make up for it.
             $state = $this->flagStateOf($account, $graphId, $item);
 
             if (null !== $state) {

src/Service/Mail/GraphApiClient.php

@@ -114,8 +114,20 @@ final class GraphApiClient
      */
     private array $inviteSelectUnsupported = [];
 
-    /** Light projection used while planning — ids only, no bodies. */
-    private const string DELTA_SELECT = 'id,internetMessageId,parentFolderId';
+    /**
+     * Light projection a folder's delta is followed with — no bodies.
+     *
+     * isRead and flag are in it because a message delta reports a change only
+     * in the properties it was asked for. Without them Exchange sends no entry
+     * at all for a message that was merely read, unread, flagged or unflagged,
+     * so mail handled in Outlook or on a phone never reached plMail.
+     *
+     * The $select is sent once, on the call that begins the chain, and is the
+     * chain's from then on: the delta link carries it. Widening this therefore
+     * changes nothing for a link already stored — see the migration that
+     * hands every folder back to the import, and keep the two in step.
+     */
+    private const string DELTA_SELECT = 'id,internetMessageId,parentFolderId,isRead,flag';
 
     public function __construct(
         // The scoped client from framework.yaml, not the default one: it

tests/Migrations/GraphDeltaLinksResetMigrationTest.php

@@ -0,0 +1,173 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Migrations;
+
+use App\Domain\Enum\Account\AuthType;
+use App\Entity\Mail\Account;
+use App\Entity\User\User;
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\ORM\EntityManagerInterface;
+use DoctrineMigrations\Version20261014110000;
+use Psr\Log\NullLogger;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+
+/**
+ * That every Outlook folder is handed back to the import once, so it is
+ * followed again by a delta that can report a read.
+ *
+ * The links on an account were made with a $select that leaves out isRead, and
+ * that select belongs to the link for good: changing what new chains ask for
+ * does nothing for the ones already running. Exchange sends them no entry for
+ * a message that was only read or unread, so the only way to hear about those
+ * is a chain begun again.
+ *
+ * Run against rows, not against the SQL text: the failure to guard against is
+ * a statement that is valid, applies, and touches the wrong accounts.
+ */
+final class GraphDeltaLinksResetMigrationTest extends KernelTestCase
+{
+    private const array LINKS = [
+        'AAMkAD-inbox' => 'https://graph.microsoft.com/v1.0/me/mailFolders/x/messages/delta?$deltatoken=old-inbox',
+        'AAMkAD-sent'  => 'https://graph.microsoft.com/v1.0/me/mailFolders/y/messages/delta?$deltatoken=old-sent',
+    ];
+
+    private EntityManagerInterface $em;
+    private Connection $connection;
+    private User $user;
+
+    protected function setUp(): void
+    {
+        self::bootKernel();
+
+        $container        = self::getContainer();
+        $this->em         = $container->get(EntityManagerInterface::class);
+        $this->connection = $container->get(Connection::class);
+
+        $this->connection->beginTransaction();
+
+        $this->user = $this->seedUser();
+    }
+
+    protected function tearDown(): void
+    {
+        if (true === $this->connection->isTransactionActive()) {
+            $this->connection->rollBack();
+        }
+
+        parent::tearDown();
+    }
+
+    public function testAMicrosoftAccountFollowsNoFolderAfterwards(): void
+    {
+        $outlook = $this->seedAccount('microsoft', self::LINKS);
+
+        self::assertSame(self::LINKS, $this->linksOf($outlook), 'the fixture starts out following both folders');
+
+        $this->migrate();
+
+        self::assertSame([], $this->linksOf($outlook), 'with no link a folder is the import\'s, and is listed again from the top');
+    }
+
+    /**
+     * The companion to the test above, and the half a careless statement
+     * breaks: `UPDATE account SET graph_delta_links = '{}'` with no WHERE is
+     * just as good at emptying the first account, and also empties these.
+     */
+    public function testAccountsOfOtherProvidersAreLeftAlone(): void
+    {
+        $google = $this->seedAccount('google', self::LINKS);
+
+        $this->migrate();
+
+        self::assertSame(self::LINKS, $this->linksOf($google), 'a Gmail account does not use delta links, and the migration is not its business');
+    }
+
+    /**
+     * Idempotent in effect: a mailbox that is already being imported, or has no
+     * links to lose, comes out the same.
+     */
+    public function testAnAccountWithNoLinksStaysWithNone(): void
+    {
+        $outlook = $this->seedAccount('microsoft', []);
+
+        $this->migrate();
+
+        self::assertSame([], $this->linksOf($outlook));
+    }
+
+    // ── helpers ───────────────────────────────────────────────────────────
+
+    /**
+     * Applies the migration's own statements, in this test's transaction, so the
+     * database it started with is the database it leaves.
+     */
+    private function migrate(): void
+    {
+        // The migrations directory is read by Doctrine's own loader, not by
+        // Composer, so nothing has autoloaded the class by the time a test runs.
+        require_once dirname(__DIR__, 2) . '/migrations/Version20261014110000.php';
+
+        $migration = new Version20261014110000($this->connection, new NullLogger());
+        $migration->up(new Schema());
+
+        foreach ($migration->getSql() as $query) {
+            $this->connection->executeStatement($query->getStatement(), $query->getParameters(), $query->getTypes());
+        }
+
+        $this->em->clear();
+    }
+
+    /**
+     * @return array<string, string>
+     */
+    private function linksOf(Account $account): array
+    {
+        $json = $this->connection->fetchOne('SELECT graph_delta_links FROM account WHERE id = :id', ['id' => $account->id]);
+
+        return json_decode((string) $json, true, 512, JSON_THROW_ON_ERROR);
+    }
+
+    /**
+     * @param array<string, string> $links
+     */
+    private function seedAccount(string $provider, array $links): Account
+    {
+        $account                   = new Account();
+        $account->usr              = $this->user;
+        $account->name             = $provider . ' fixture';
+        $account->email            = $provider . '-' . uniqid('', true) . '@example.test';
+        $account->username         = uniqid($provider . '-', true);
+        $account->imapHost         = 'imap.example.test';
+        $account->imapPort         = 993;
+        $account->imapEncryption   = 'ssl';
+        $account->authType         = AuthType::OAuth2->value;
+        $account->oauthProvider    = $provider;
+        $account->oauthAccessToken = 'test-access-token';
+        $account->oauthTokenExpiry = new \DateTimeImmutable('+1 day');
+        $account->isActive         = true;
+        $account->graphDeltaLinks  = $links;
+
+        $this->em->persist($account);
+        $this->em->flush();
+
+        return $account;
+    }
+
+    private function seedUser(): User
+    {
+        $user            = new User();
+        $user->email     = 'delta-reset-' . uniqid('', true) . '@example.test';
+        $user->nameFirst = 'Delta';
+        $user->nameLast  = 'Reset';
+        $user->roles     = ['ROLE_USER'];
+        $user->password  = 'x';
+
+        $this->em->persist($user);
+        $this->em->flush();
+
+        return $user;
+    }
+}

tests/Service/Graph/GraphDeltaSelectTest.php

@@ -0,0 +1,117 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Service\Graph;
+
+use App\Domain\Enum\Account\AuthType;
+use App\Entity\Mail\Account;
+use App\Service\Mail\GraphApiClient;
+use DateTimeImmutable;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+use Symfony\Component\HttpClient\MockHttpClient;
+use Symfony\Component\HttpClient\Response\MockResponse;
+
+/**
+ * What a folder's delta is asked to carry.
+ *
+ * A message delta reports a change only in the properties it was asked for,
+ * and the $select it begins with is the chain's for good. Asked for
+ * `id,internetMessageId,parentFolderId`, Exchange did not merely leave isRead
+ * out of the entries it sent: it sent no entry at all for a message whose only
+ * change was being read or unread. Mail read in Outlook or on a phone never
+ * reached plMail, and GraphInboundReadStateTest passed throughout, because it
+ * puts `isRead` into the delta it hands the syncer — a delta the real server
+ * could not have produced.
+ *
+ * So this pins the request, which is the one place the fixture cannot lie.
+ */
+final class GraphDeltaSelectTest extends KernelTestCase
+{
+    private const string FOLDER = 'AAMkAD-inbox';
+
+    /** What the entries of a chain have to carry for a read or a star to be heard. */
+    private const array MUST_CARRY = ['isRead', 'flag'];
+
+    /** And what the syncer has always needed, so a rewrite of the list cannot drop it. */
+    private const array ALWAYS_CARRIED = ['id', 'internetMessageId', 'parentFolderId'];
+
+    public function testANewDeltaChainAsksForReadAndFlagState(): void
+    {
+        $select = $this->selectOf(fn (GraphApiClient $client, Account $account) => $client->deltaMessages($account, self::FOLDER, null));
+
+        foreach (self::MUST_CARRY as $property) {
+            self::assertContains($property, $select, sprintf('a delta that does not select %s is never told when it changes', $property));
+        }
+
+        foreach (self::ALWAYS_CARRIED as $property) {
+            self::assertContains($property, $select);
+        }
+    }
+
+    public function testAFoldersImportAsksForTheSameThing(): void
+    {
+        // The import's enumeration ends by handing the folder its delta link,
+        // so what it selects is what the folder is followed with afterwards.
+        $select = $this->selectOf(fn (GraphApiClient $client, Account $account) => $client->deltaMessagesPage($account, self::FOLDER, null, 50));
+
+        foreach (self::MUST_CARRY as $property) {
+            self::assertContains($property, $select, sprintf('a delta that does not select %s is never told when it changes', $property));
+        }
+
+        foreach (self::ALWAYS_CARRIED as $property) {
+            self::assertContains($property, $select);
+        }
+    }
+
+    // ── helpers ───────────────────────────────────────────────────────────
+
+    /**
+     * The properties named in the $select of the request the call makes.
+     *
+     * @param callable(GraphApiClient, Account): mixed $call
+     *
+     * @return list<string>
+     */
+    private function selectOf(callable $call): array
+    {
+        $urls = [];
+
+        $http = new MockHttpClient(function (string $method, string $url) use (&$urls): MockResponse {
+            $urls[] = $url;
+
+            return new MockResponse(
+                json_encode(['value' => [], '@odata.deltaLink' => 'https://graph.microsoft.com/v1.0/delta?$deltatoken=t'], JSON_THROW_ON_ERROR),
+                ['response_headers' => ['content-type' => 'application/json']],
+            );
+        });
+
+        $client = new GraphApiClient($http, self::getContainer()->get('App\Service\OAuth\OAuthTokenManager'));
+
+        $call($client, $this->account());
+
+        self::assertCount(1, $urls, 'one request, to begin the chain');
+
+        parse_str((string) parse_url($urls[0], PHP_URL_QUERY), $query);
+
+        self::assertArrayHasKey('$select', $query, 'the chain was begun without a $select, so it carries whatever Exchange defaults to');
+
+        return array_map('trim', explode(',', (string) $query['$select']));
+    }
+
+    /** Not stored: the token is current, so nothing asks the database. */
+    private function account(): Account
+    {
+        $account                   = new Account();
+        $account->name             = 'Graph fixture';
+        $account->email            = 'graph@example.test';
+        $account->username         = 'graph-select';
+        $account->authType         = AuthType::OAuth2->value;
+        $account->oauthProvider    = 'microsoft';
+        $account->oauthAccessToken = 'test-access-token';
+        $account->oauthTokenExpiry = new DateTimeImmutable('+1 day');
+        $account->isActive         = true;
+
+        return $account;
+    }
+}

tests/Service/Graph/GraphInboundReadStateTest.php

@@ -306,6 +306,57 @@ final class GraphInboundReadStateTest extends KernelTestCase
         );
     }
 
+    /**
+     * The half of a folder's second life that the migration relies on. Every
+     * folder's link is cleared once (Version20261014110000), so the folder is
+     * the import's and is listed from the top; the listing carries each
+     * message's isRead, and a message it already has is settled by it, not
+     * skipped as "known". Then the folder is followed again, by the link the
+     * listing ends with.
+     *
+     * Both reads are asserted — the message read here and unread there, and
+     * the other way round — because a listing that only ever agreed with one
+     * of them would pass either alone.
+     */
+    public function testAFolderListedAgainSettlesTheReadStateOfWhatItAlreadyHas(): void
+    {
+        $readHere   = $this->seedMessage(seen: true);
+        $unreadHere = $this->seedMessage(seen: false, graphId: 'AAkALgAAother');
+
+        $page = new MockResponse(json_encode([
+            'value' => [
+                ['id' => self::GRAPH_ID, 'parentFolderId' => self::INBOX_FOLDER, 'isRead' => false],
+                ['id' => 'AAkALgAAother', 'parentFolderId' => self::INBOX_FOLDER, 'isRead' => true],
+            ],
+            '@odata.deltaLink' => 'https://graph.microsoft.com/v1.0/me/mailFolders/x/messages/delta?$deltatoken=renewed',
+        ], JSON_THROW_ON_ERROR), ['response_headers' => ['content-type' => 'application/json']]);
+
+        // What the migration leaves: no link, and the folder planned for the import.
+        $this->account->graphDeltaLinks = [];
+        $this->account->graphImport     = [['folder' => self::INBOX_FOLDER, 'since' => time(), 'next' => null]];
+        $this->em->flush();
+
+        $this->syncerAnswering($page)->importPage($this->account);
+
+        $this->em->clear();
+
+        $readHere   = $this->em->find(Message::class, $readHere->id);
+        $unreadHere = $this->em->find(Message::class, $unreadHere->id);
+        $account    = $this->em->find(Account::class, $this->account->id);
+
+        self::assertNull($readHere->seenAt, 'read here, unread on the server: the server\'s word is taken');
+        self::assertNotNull($unreadHere->seenAt, 'unread here, read on the server: the server\'s word is taken');
+        self::assertSame(
+            'https://graph.microsoft.com/v1.0/me/mailFolders/x/messages/delta?$deltatoken=renewed',
+            $account->graphDeltaLinks[self::INBOX_FOLDER] ?? null,
+            'the listing ends by handing the folder the link it is followed with from now on',
+        );
+
+        // The re-listing changed rows the open list is showing, and it runs
+        // through importPage(), not sync(): that path has to tell the list too.
+        $this->assertListToldToRefresh();
+    }
+
     // ── helpers ───────────────────────────────────────────────────────────
 
     /**
@@ -316,6 +367,25 @@ final class GraphInboundReadStateTest extends KernelTestCase
      * @param array<string,mixed> $extra
      */
     private function syncDelta(array $extra): void
+    {
+        $syncer = $this->syncerAnswering($this->deltaPage($extra));
+
+        // Folders that are already being followed. One without a delta link is
+        // the import's and would not be asked for changes at all — see
+        // GraphImportTest for that half.
+        $this->account->graphDeltaLinks = array_fill_keys([self::INBOX_FOLDER], 'https://graph.microsoft.com/v1.0/me/mailFolders/x/messages/delta?$deltatoken=old');
+
+        $syncer->sync($this->account, [self::INBOX_FOLDER]);
+
+        $this->em->flush();
+    }
+
+    /**
+     * What a folder's delta, or its listing, says about the message.
+     *
+     * @param array<string,mixed> $extra
+     */
+    private function deltaPage(array $extra): MockResponse
     {
         $delta = [
             'value' => [array_merge([
@@ -325,13 +395,18 @@ final class GraphInboundReadStateTest extends KernelTestCase
             '@odata.deltaLink' => 'https://graph.microsoft.com/v1.0/me/mailFolders/x/messages/delta?$deltatoken=new',
         ];
 
+        return new MockResponse(json_encode($delta, JSON_THROW_ON_ERROR), [
+            'response_headers' => ['content-type' => 'application/json'],
+        ]);
+    }
+
+    private function syncerAnswering(MockResponse $response): GraphApiSyncer
+    {
         $container = self::getContainer();
 
-        $syncer = new GraphApiSyncer(
+        return new GraphApiSyncer(
             new GraphApiClient(
-                new MockHttpClient([new MockResponse(json_encode($delta, JSON_THROW_ON_ERROR), [
-                    'response_headers' => ['content-type' => 'application/json'],
-                ])]),
+                new MockHttpClient([$response]),
                 $container->get('App\Service\OAuth\OAuthTokenManager'),
             ),
             $container->get('App\Service\Graph\GraphFolderResolver'),
@@ -347,18 +422,9 @@ final class GraphInboundReadStateTest extends KernelTestCase
             $container->get('App\Service\Label\ThreadLabelSynchronizer'),
             new \App\Service\Mail\SyncNotifier($this->hub, new \Psr\Log\NullLogger()),
         );
-
-        // Folders that are already being followed. One without a delta link is
-        // the import's and would not be asked for changes at all — see
-        // GraphImportTest for that half.
-        $this->account->graphDeltaLinks = array_fill_keys([self::INBOX_FOLDER], 'https://graph.microsoft.com/v1.0/me/mailFolders/x/messages/delta?$deltatoken=old');
-
-        $syncer->sync($this->account, [self::INBOX_FOLDER]);
-
-        $this->em->flush();
     }
 
-    private function seedMessage(bool $seen, bool $flagged = false): Message
+    private function seedMessage(bool $seen, bool $flagged = false, string $graphId = self::GRAPH_ID): Message
     {
         $thread                    = new MessageThread();
         $thread->account           = $this->account;
@@ -373,7 +439,7 @@ final class GraphInboundReadStateTest extends KernelTestCase
         $message->thread         = $thread;
         $message->subject        = 'Read elsewhere';
         $message->fromAddress    = 'sender@example.test';
-        $message->graphId        = self::GRAPH_ID;
+        $message->graphId        = $graphId;
         $message->receivedAt     = new DateTimeImmutable('-1 hour');
         $message->sentAt         = $message->receivedAt;
         $message->hasAttachments = false;