Add SparkPost email adapter - #146
Conversation
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
Greptile SummaryAdds a SparkPost email adapter and an end-to-end delivery test.
Confidence Score: 1/5This PR is not safe to merge because the adapter cannot successfully process a valid Email and its end-to-end test cannot construct the test message. The adapter calls a nonexistent sender method, mishandles normalized recipient records, drops additional To, CC, and BCC recipients, fails on non-JSON error bodies, and leaves successful delivery counts at zero; the accompanying test also omits mandatory constructor arguments. Files Needing Attention: src/Utopia/Messaging/Adapter/Email/SparkPost.php and tests/e2e/Email/SparkPostTest.php Important Files Changed
Prompt To Fix All With AIFix the following 7 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 7
src/Utopia/Messaging/Adapter/Email/SparkPost.php:56
**Undefined sender accessor crashes sends**
When any valid email is sent, this calls `Email::getFrom()`, but the message exposes only `getFromEmail()` and `getFromName()`, causing an undefined-method error before the SparkPost request is made.
### Issue 2 of 7
src/Utopia/Messaging/Adapter/Email/SparkPost.php:50
**Recipient records used as strings**
`getTo()[0]` is a normalized recipient array, not an address string. This places an object in SparkPost's `address.email` field, and the same value passed to `Response::addResult()` causes a type error because that method requires a string.
### Issue 3 of 7
src/Utopia/Messaging/Adapter/Email/SparkPost.php:50
**Additional recipients are silently dropped**
When an email has 2–1000 `to` recipients, the base adapter accepts it and invokes `process()` once, but this payload includes only `getTo()[0]`. Every remaining recipient is omitted from both delivery and response results.
### Issue 4 of 7
src/Utopia/Messaging/Adapter/Email/SparkPost.php:65-66
**Successful delivery count remains zero**
When SparkPost accepts a transmission, this branch adds a successful result but never calls `setDeliveredTo()`. The returned response therefore reports zero deliveries even though the email was sent, breaking delivery accounting and response assertions.
### Issue 5 of 7
tests/e2e/Email/SparkPostTest.php:15-19
**Required sender arguments are omitted**
Running this test constructs `Email` without its required `fromName` and `fromEmail` arguments, causing an argument-count error before the adapter is invoked.
### Issue 6 of 7
src/Utopia/Messaging/Adapter/Email/SparkPost.php:47-52
**CC and BCC recipients are omitted**
When an email contains CC or BCC recipients, the transmission body includes only the first `to` recipient and never adds either recipient group, causing those recipients to be silently excluded from delivery.
### Issue 7 of 7
src/Utopia/Messaging/Adapter/Email/SparkPost.php:68
**Non-JSON errors break response handling**
When SparkPost or an intermediary returns a plain-text or HTML error body, the request helper preserves `response` as a string, but this line accesses it as a nested array. Error handling then raises an offset error instead of returning the provider's failure response.
Reviews (1): Last reviewed commit: "feat: add SparkPost email adapter" | Re-trigger Greptile |
| ], | ||
| 'content' => [ | ||
| 'from' => [ | ||
| 'email' => $message->getFrom(), |
There was a problem hiding this comment.
Undefined sender accessor crashes sends
When any valid email is sent, this calls Email::getFrom(), but the message exposes only getFromEmail() and getFromName(), causing an undefined-method error before the SparkPost request is made.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Email/SparkPost.php
Line: 56
Comment:
**Undefined sender accessor crashes sends**
When any valid email is sent, this calls `Email::getFrom()`, but the message exposes only `getFromEmail()` and `getFromName()`, causing an undefined-method error before the SparkPost request is made.
How can I resolve this? If you propose a fix, please make it concise.| 'recipients' => [ | ||
| [ | ||
| 'address' => [ | ||
| 'email' => $message->getTo()[0], |
There was a problem hiding this comment.
Recipient records used as strings
getTo()[0] is a normalized recipient array, not an address string. This places an object in SparkPost's address.email field, and the same value passed to Response::addResult() causes a type error because that method requires a string.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Email/SparkPost.php
Line: 50
Comment:
**Recipient records used as strings**
`getTo()[0]` is a normalized recipient array, not an address string. This places an object in SparkPost's `address.email` field, and the same value passed to `Response::addResult()` causes a type error because that method requires a string.
How can I resolve this? If you propose a fix, please make it concise.| 'recipients' => [ | ||
| [ | ||
| 'address' => [ | ||
| 'email' => $message->getTo()[0], |
There was a problem hiding this comment.
Additional recipients are silently dropped
When an email has 2–1000 to recipients, the base adapter accepts it and invokes process() once, but this payload includes only getTo()[0]. Every remaining recipient is omitted from both delivery and response results.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Email/SparkPost.php
Line: 50
Comment:
**Additional recipients are silently dropped**
When an email has 2–1000 `to` recipients, the base adapter accepts it and invokes `process()` once, but this payload includes only `getTo()[0]`. Every remaining recipient is omitted from both delivery and response results.
How can I resolve this? If you propose a fix, please make it concise.| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | ||
| $response->addResult($message->getTo()[0]); |
There was a problem hiding this comment.
Successful delivery count remains zero
When SparkPost accepts a transmission, this branch adds a successful result but never calls setDeliveredTo(). The returned response therefore reports zero deliveries even though the email was sent, breaking delivery accounting and response assertions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Email/SparkPost.php
Line: 65-66
Comment:
**Successful delivery count remains zero**
When SparkPost accepts a transmission, this branch adds a successful result but never calls `setDeliveredTo()`. The returned response therefore reports zero deliveries even though the email was sent, breaking delivery accounting and response assertions.
How can I resolve this? If you propose a fix, please make it concise.| $message = new Email( | ||
| to: [\getenv('TEST_EMAIL')], | ||
| subject: 'Test Subject', | ||
| content: 'Test Content', | ||
| ); |
There was a problem hiding this comment.
Required sender arguments are omitted
Running this test constructs Email without its required fromName and fromEmail arguments, causing an argument-count error before the adapter is invoked.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Email/SparkPostTest.php
Line: 15-19
Comment:
**Required sender arguments are omitted**
Running this test constructs `Email` without its required `fromName` and `fromEmail` arguments, causing an argument-count error before the adapter is invoked.
How can I resolve this? If you propose a fix, please make it concise.| 'recipients' => [ | ||
| [ | ||
| 'address' => [ | ||
| 'email' => $message->getTo()[0], | ||
| ], | ||
| ], |
There was a problem hiding this comment.
CC and BCC recipients are omitted
When an email contains CC or BCC recipients, the transmission body includes only the first to recipient and never adds either recipient group, causing those recipients to be silently excluded from delivery.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Email/SparkPost.php
Line: 47-52
Comment:
**CC and BCC recipients are omitted**
When an email contains CC or BCC recipients, the transmission body includes only the first `to` recipient and never adds either recipient group, causing those recipients to be silently excluded from delivery.
How can I resolve this? If you propose a fix, please make it concise.| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | ||
| $response->addResult($message->getTo()[0]); | ||
| } else { | ||
| $error = $result['response']['errors'][0]['message'] ?? $result['error'] ?? 'Unknown error'; |
There was a problem hiding this comment.
Non-JSON errors break response handling
When SparkPost or an intermediary returns a plain-text or HTML error body, the request helper preserves response as a string, but this line accesses it as a nested array. Error handling then raises an offset error instead of returning the provider's failure response.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Email/SparkPost.php
Line: 68
Comment:
**Non-JSON errors break response handling**
When SparkPost or an intermediary returns a plain-text or HTML error body, the request helper preserves `response` as a string, but this line accesses it as a nested array. Error handling then raises an offset error instead of returning the provider's failure response.
How can I resolve this? If you propose a fix, please make it concise.
Rebased on latest main with code adaptation.
Adapted from original PR #47 (by ShivamChandra09):