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 by making
Confidence Score: 3/5The test will not pass as written, and the adapter removes a fallback that callers in this codebase rely on. The test incorrectly routes TELNYX_FROM to the SMS message object (where it is ignored) rather than the Telnyx constructor, and the adapter now silently discards $message->getFrom() in a way that contradicts every other SMS adapter in the repo. Both issues need to be corrected before the PR is merge-ready. Both files need attention: TelnyxTest.php has the wrong constructor call and misrouted env var, and Telnyx.php drops the $message->getFrom() fallback without a clear justification given the established pattern. Important Files Changed
Reviews (3): 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!
deepshekhardas
force-pushed
the
feat/7811-telnyx-adapter
branch
from
4621f3b to
150212f
Compare
| "Authorization: Bearer {$this->apiKey}", | ||
| ], | ||
| body: [ | ||
| 'from' => $this->from, |
There was a problem hiding this comment.
The constructor now documents $from as either a Telnyx phone number or profile ID, but this request always sends that value in the from field. Telnyx uses a separate messaging_profile_id field for profile IDs, so a caller that passes a profile UUID as allowed by this adapter will send an invalid payload and the SMS will fail.
| ); | ||
|
|
||
| // $result = $sender->send($message); | ||
| $result = $sender->send($message); |
There was a problem hiding this comment.
This test now performs a real Telnyx send in the default PHPUnit suite. In normal CI or local runs without Telnyx credentials, test numbers, and network access, this path will fail or attempt to send a real SMS. The previous test was skipped for this reason, so this should stay gated on available credentials or use a mocked/request-catcher flow.
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!
deepshekhardas
force-pushed
the
feat/7811-telnyx-adapter
branch
from
150212f to
d215ba9
Compare
1 participant
Updates the Telnyx SMS adapter.
Changes
Testing
Closes #7811