Handle email move errors gracefully with admin notification #159
Conversation
Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
…eated instantiation Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
…pagate Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
|
@copilot : Fix tests. |
Co-authored-by: HNygard <168380+HNygard@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances error handling in the email moving flow so that individual IMAP move failures are handled gracefully, with admin notifications, while allowing processing to continue up to a bounded number of errors.
Changes:
- Extend
ThreadEmailMoverto track move errors, send admin notifications (viaAdminNotificationService) on each failure, and stop processing once a configurable maximum error count (5) is reached. - Update the move logic to log errors, lazily initialize
AdminNotificationService, and request folder status updates only on successful moves. - Add and adjust unit tests in
ThreadEmailMoverTestto cover single-error continuation, max-error stopping behavior, and mixed success/error scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| organizer/src/class/ThreadEmailMover.php | Adds AdminNotificationService dependency, introduces MAX_ERRORS, wraps moveEmail in error-handling that logs, notifies admins on each failure, continues processing, and throws once the maximum error count is reached, while still updating ImapFolderStatus on successful moves. |
| organizer/src/tests/ThreadEmailMoverTest.php | Adjusts the existing move-error test to expect graceful completion instead of bubbling the exception, and adds new tests for continuing after a single error, stopping after 5 errors, and handling mixed successes and failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function testProcessMailboxStopsAfterMaxErrors() { | ||
| // Create 7 mock emails, all will fail to move | ||
| $mockEmails = []; | ||
| for ($i = 1; $i <= 7; $i++) { | ||
| $mockEmail = $this->createMock(\Imap\ImapEmail::class); | ||
| $mockEmail->uid = $i; | ||
| // Only first 5 emails will have their methods called | ||
| if ($i <= 5) { | ||
| $mockEmail->expects($this->once()) | ||
| ->method('getEmailAddresses') | ||
| ->willReturn(["test{$i}@example.com"]); | ||
| } | ||
| $mockEmails[] = $mockEmail; | ||
| } | ||
|
|
||
| $this->mockEmailProcessor->expects($this->once()) | ||
| ->method('getEmails') | ||
| ->with('INBOX') | ||
| ->willReturn($mockEmails); | ||
|
|
||
| // Only 5 emails will be processed before stopping | ||
| $this->mockConnection->expects($this->exactly(5)) | ||
| ->method('getRawEmail') | ||
| ->willReturn('Raw email content'); | ||
|
|
||
| // All move attempts will fail, but only 5 will be attempted | ||
| $this->mockFolderManager->expects($this->exactly(5)) | ||
| ->method('moveEmail') | ||
| ->willThrowException(new Exception('Move failed')); | ||
|
|
||
| $emailToFolder = []; | ||
| for ($i = 1; $i <= 7; $i++) { | ||
| $emailToFolder["test{$i}@example.com"] = "INBOX.Test - Thread {$i}"; | ||
| } | ||
|
|
||
| // Expect exception to be thrown after reaching max errors | ||
| $this->expectException(Exception::class); | ||
| $this->expectExceptionMessage('Maximum error count (5) reached, stopping email processing'); | ||
|
|
||
| $this->threadEmailMover->processMailbox('INBOX', $emailToFolder); |
There was a problem hiding this comment.
testProcessMailboxStopsAfterMaxErrors assumes that processMailbox() will "complete" and return an array even after hitting the maximum error threshold, but the implementation now throws an Exception once $errorCount >= self::MAX_ERRORS (see ThreadEmailMover::processMailbox catch block). With 7 emails and all 5 first moves failing, this test will encounter an uncaught exception at the 5th error and never reach the assertions at lines 722–724. Either the test should be updated to expectException (verifying that processing stops after 5 errors via the thrown exception and mock call counts) or the implementation should be changed to break/return instead of throwing so the behavior matches the current test name and assertions.
Email Move Error Handling - Implementation Complete ✅
ThreadEmailMover::processMailbox()to catch and handle move errorsSummary
Successfully implemented error handling for email move operations in the Offpost system. The solution ensures that:
The changes are minimal and focused on the specific issue while maintaining backward compatibility.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.