diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a7f1ca2..fd34956d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/migrations/Version20261014110000.php b/migrations/Version20261014110000.php new file mode 100644 index 00000000..96765645 --- /dev/null +++ b/migrations/Version20261014110000.php @@ -0,0 +1,53 @@ +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. + } +} diff --git a/phpstan.dist.neon b/phpstan.dist.neon index 6cce8418..97b07c65 100644 --- a/phpstan.dist.neon +++ b/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 diff --git a/src/Service/Graph/GraphApiSyncer.php b/src/Service/Graph/GraphApiSyncer.php index 570f2d3c..fb22f191 100644 --- a/src/Service/Graph/GraphApiSyncer.php +++ b/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) { diff --git a/src/Service/Mail/GraphApiClient.php b/src/Service/Mail/GraphApiClient.php index a015452d..6aa198ad 100644 --- a/src/Service/Mail/GraphApiClient.php +++ b/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 diff --git a/tests/Migrations/GraphDeltaLinksResetMigrationTest.php b/tests/Migrations/GraphDeltaLinksResetMigrationTest.php new file mode 100644 index 00000000..9a2fb3bd --- /dev/null +++ b/tests/Migrations/GraphDeltaLinksResetMigrationTest.php @@ -0,0 +1,173 @@ + '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 + */ + 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 $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; + } +} diff --git a/tests/Service/Graph/GraphDeltaSelectTest.php b/tests/Service/Graph/GraphDeltaSelectTest.php new file mode 100644 index 00000000..e7d49077 --- /dev/null +++ b/tests/Service/Graph/GraphDeltaSelectTest.php @@ -0,0 +1,117 @@ +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 + */ + 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; + } +} diff --git a/tests/Service/Graph/GraphInboundReadStateTest.php b/tests/Service/Graph/GraphInboundReadStateTest.php index a9765856..3eee6cea 100644 --- a/tests/Service/Graph/GraphInboundReadStateTest.php +++ b/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 $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 $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;