feat: add Telnyx SMS adapter (#7811)#108
Conversation
WalkthroughThe Telnyx SMS adapter's constructor parameter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Messaging/Adapter/SMS/TelnyxTest.php`:
- Around line 16-22: The test fails because Telnyx is instantiated with too few
arguments and doesn't guard missing env vars; update the test in TelnyxTest to
first read and validate getenv('TELNYX_API_KEY'), getenv('TELNYX_TO') and
getenv('TELNYX_FROM') and call markTestSkipped (or return) if any are empty,
then construct the Telnyx sender passing the required from argument (e.g.,
include the from value when calling new Telnyx(...)) and create the SMS using
the validated env values before calling send.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea64e157-ee53-4fa5-baaf-f297eaae6463
📒 Files selected for processing (2)
src/Utopia/Messaging/Adapter/SMS/Telnyx.phptests/Messaging/Adapter/SMS/TelnyxTest.php
| $sender = new Telnyx(\getenv('TELNYX_API_KEY')); | ||
|
|
||
| // $message = new SMS( | ||
| // to: ['+18034041123'], | ||
| // content: 'Test Content', | ||
| // from: '+15005550006' | ||
| // ); | ||
| $message = new SMS( | ||
| to: [\getenv('TELNYX_TO')], | ||
| content: 'Test Content', | ||
| from: \getenv('TELNYX_FROM') | ||
| ); |
There was a problem hiding this comment.
Pass the required Telnyx from argument and guard missing env vars.
This test currently instantiates Telnyx with too few arguments, which will fail before the send call runs. It also should skip cleanly when Telnyx env vars are not configured.
Proposed patch
public function testSendSMS(): void
{
- $sender = new Telnyx(\getenv('TELNYX_API_KEY'));
+ $apiKey = \getenv('TELNYX_API_KEY') ?: null;
+ $from = \getenv('TELNYX_FROM') ?: null;
+ $to = \getenv('TELNYX_TO') ?: null;
+
+ if (!$apiKey || !$from || !$to) {
+ $this->markTestSkipped('TELNYX_API_KEY, TELNYX_FROM, and TELNYX_TO are required for this integration test.');
+ }
+
+ $sender = new Telnyx($apiKey, $from);
$message = new SMS(
- to: [\getenv('TELNYX_TO')],
+ to: [$to],
content: 'Test Content',
- from: \getenv('TELNYX_FROM')
+ from: $from
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $sender = new Telnyx(\getenv('TELNYX_API_KEY')); | |
| // $message = new SMS( | |
| // to: ['+18034041123'], | |
| // content: 'Test Content', | |
| // from: '+15005550006' | |
| // ); | |
| $message = new SMS( | |
| to: [\getenv('TELNYX_TO')], | |
| content: 'Test Content', | |
| from: \getenv('TELNYX_FROM') | |
| ); | |
| $apiKey = \getenv('TELNYX_API_KEY') ?: null; | |
| $from = \getenv('TELNYX_FROM') ?: null; | |
| $to = \getenv('TELNYX_TO') ?: null; | |
| if (!$apiKey || !$from || !$to) { | |
| $this->markTestSkipped('TELNYX_API_KEY, TELNYX_FROM, and TELNYX_TO are required for this integration test.'); | |
| } | |
| $sender = new Telnyx($apiKey, $from); | |
| $message = new SMS( | |
| to: [$to], | |
| content: 'Test Content', | |
| from: $from | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Messaging/Adapter/SMS/TelnyxTest.php` around lines 16 - 22, The test
fails because Telnyx is instantiated with too few arguments and doesn't guard
missing env vars; update the test in TelnyxTest to first read and validate
getenv('TELNYX_API_KEY'), getenv('TELNYX_TO') and getenv('TELNYX_FROM') and call
markTestSkipped (or return) if any are empty, then construct the Telnyx sender
passing the required from argument (e.g., include the from value when calling
new Telnyx(...)) and create the SMS using the validated env values before
calling send.
deepshekhardas
force-pushed
the
feat/7811-telnyx-adapter
branch
from
e416217 to
4621f3b
Compare
|
Rebased onto latest main. Could you please review? 🙏 |
Greptile SummaryThis PR refactors the Telnyx SMS adapter to send one API request per recipient (enabling per-recipient delivery tracking and error reporting) and makes
Confidence Score: 2/5The test file will crash immediately with a fatal PHP error due to a missing required constructor argument, meaning the new test coverage provides no validation until fixed. The constructor signature change in Telnyx.php is correct, but the test was not updated to match — it still calls new Telnyx with one argument while the constructor now requires two. This blocking mistake prevents the test from running at all. tests/Messaging/Adapter/SMS/TelnyxTest.php needs the $from argument added to the Telnyx constructor call before this can be merged. Important Files Changed
Reviews (1): Last reviewed commit: "feat: add Telnyx SMS adapter (#7811)" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $sender = new Telnyx(\getenv('TELNYX_API_KEY')); | ||
|
|
||
| // $message = new SMS( | ||
| // to: ['+18034041123'], | ||
| // content: 'Test Content', | ||
| // from: '+15005550006' | ||
| // ); | ||
| $message = new SMS( | ||
| to: [\getenv('TELNYX_TO')], | ||
| content: 'Test Content', | ||
| from: \getenv('TELNYX_FROM') | ||
| ); |
There was a problem hiding this comment.
Telnyx constructor now requires two arguments ($apiKey and $from), but only one is passed here. This will throw a PHP fatal error — "Too few arguments to function Telnyx::__construct(), 1 passed and exactly 2 expected" — making the entire test suite fail immediately. The TELNYX_FROM env var is already used for the SMS message, but needs to be passed to the adapter constructor as well.
| $sender = new Telnyx(\getenv('TELNYX_API_KEY')); | |
| // $message = new SMS( | |
| // to: ['+18034041123'], | |
| // content: 'Test Content', | |
| // from: '+15005550006' | |
| // ); | |
| $message = new SMS( | |
| to: [\getenv('TELNYX_TO')], | |
| content: 'Test Content', | |
| from: \getenv('TELNYX_FROM') | |
| ); | |
| $sender = new Telnyx(\getenv('TELNYX_API_KEY'), \getenv('TELNYX_FROM')); | |
| $message = new SMS( | |
| to: [\getenv('TELNYX_TO')], | |
| content: 'Test Content', | |
| ); |
| foreach ($message->getTo() as $to) { | ||
| $result = $this->request( | ||
| method: 'POST', | ||
| url: 'https://api.telnyx.com/v2/messages', | ||
| headers: [ | ||
| 'Content-Type: application/json', | ||
| "Authorization: Bearer {$this->apiKey}", | ||
| ], | ||
| body: [ | ||
| 'from' => $this->from, | ||
| 'to' => $to, | ||
| 'text' => $message->getContent(), | ||
| ] | ||
| ); | ||
|
|
||
| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | ||
| $response->setDeliveredTo(\count($message->getTo())); | ||
| foreach ($message->getTo() as $to) { | ||
| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | ||
| $response->incrementDeliveredTo(); | ||
| $response->addResult($to); | ||
| } | ||
| } else { | ||
| foreach ($message->getTo() as $to) { | ||
| $response->addResult($to, 'Unknown error.'); | ||
| } else { | ||
| $response->addResult($to, $result['response']['errors'][0]['detail'] ?? 'Unknown error'); | ||
| } | ||
| } |
There was a problem hiding this comment.
getMaxMessagesPerRequest() returns 1, the parent send() method enforces that process() is always called with exactly one recipient and throws an exception otherwise. The foreach loop will therefore always iterate exactly once, making it misleading and inconsistent with the Twilio adapter (which uses $message->getTo()[0] directly). Accessing the single recipient directly is clearer and aligns with how other single-recipient adapters in this codebase are implemented.
| foreach ($message->getTo() as $to) { | |
| $result = $this->request( | |
| method: 'POST', | |
| url: 'https://api.telnyx.com/v2/messages', | |
| headers: [ | |
| 'Content-Type: application/json', | |
| "Authorization: Bearer {$this->apiKey}", | |
| ], | |
| body: [ | |
| 'from' => $this->from, | |
| 'to' => $to, | |
| 'text' => $message->getContent(), | |
| ] | |
| ); | |
| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | |
| $response->setDeliveredTo(\count($message->getTo())); | |
| foreach ($message->getTo() as $to) { | |
| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | |
| $response->incrementDeliveredTo(); | |
| $response->addResult($to); | |
| } | |
| } else { | |
| foreach ($message->getTo() as $to) { | |
| $response->addResult($to, 'Unknown error.'); | |
| } else { | |
| $response->addResult($to, $result['response']['errors'][0]['detail'] ?? 'Unknown error'); | |
| } | |
| } | |
| $to = $message->getTo()[0]; | |
| $result = $this->request( | |
| method: 'POST', | |
| url: 'https://api.telnyx.com/v2/messages', | |
| headers: [ | |
| 'Content-Type: application/json', | |
| "Authorization: Bearer {$this->apiKey}", | |
| ], | |
| body: [ | |
| 'from' => $this->from, | |
| 'to' => $to, | |
| 'text' => $message->getContent(), | |
| ] | |
| ); | |
| if ($result['statusCode'] >= 200 && $result['statusCode'] < 300) { | |
| $response->setDeliveredTo(1); | |
| $response->addResult($to); | |
| } else { | |
| $response->addResult($to, $result['response']['errors'][0]['detail'] ?? 'Unknown error'); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Updates the Telnyx SMS adapter.
Changes
Testing
Closes #7811