feat: add Vonage multi-channel Messages adapter (#8139)#110
feat: add Vonage multi-channel Messages adapter (#8139)#110deepshekhardas wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
deepshekhardas
force-pushed
the
feat/8139-vonage-messages-adapter
branch
from
ba22934 to
d623ac7
Compare
|
Rebased onto latest main. Could you please review? 🙏 |
Greptile SummaryThis PR introduces a
Confidence Score: 2/5Not safe to merge — every adapter crashes at runtime before sending a message, and the Viber channel name is wrong at the API level. The trait constructor silently skips initialising $sendCounter, so any call to send() will throw a typed-property-not-initialised fatal. The SMS adapter's bare use VonageTrait; resolves to a non-existent class in its namespace, causing a class-not-found error at instantiation. The Viber adapter sends an invalid channel value that the Vonage API rejects. The test suite cannot execute any assertions: it calls an undefined method, passes an array to json_decode, and uses a non-functional RSA key. src/Utopia/Messaging/Adapter/VonageTrait.php, src/Utopia/Messaging/Adapter/SMS/VonageMessages.php, src/Utopia/Messaging/Adapter/Viber/Vonage.php, and tests/Messaging/Adapter/SMS/VonageMessagesTest.php Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/Utopia/Messaging/Adapter/Viber/Vonage.php:30
**Wrong Viber channel value — all Viber sends will fail**
The Vonage Messages API requires `"viber_service"` as the channel value for Viber messages, not `"viber"`. Sending `channel: "viber"` will result in a 422 Unprocessable Entity from Vonage and no message will be delivered. This is confirmed by the [Vonage API reference](https://developer.vonage.com/en/messages/code-snippets/viber/send-text) and the [OpenAPI spec](https://raw.githubusercontent.com/api-evangelist/vonage/refs/heads/main/openapi/vonage-openapi.yml).
```suggestion
return $this->processMessage($message, 'viber_service');
```
Reviews (3): Last reviewed commit: "feat: add Vonage multi-channel Messages ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| // This assumes the mock server returns a success response | ||
| // In a real environment, we'd check the request-catcher data | ||
| $this->assertNotEmpty($result); | ||
| } | ||
|
|
||
| public function testSendWhatsApp(): void | ||
| { | ||
| $sender = new VonageWhatsApp($this->applicationId, $this->privateKey); |
There was a problem hiding this comment.
setEndpoint() is undefined on these adapters. None of VonageMessages, VonageWhatsApp, VonageViber, or VonageMMS define a setEndpoint() method — it only exists on Mock. Every test method in this class will crash with "Call to undefined method" before reaching any assertions. The same issue occurs in all four test methods (testSendSMS, testSendWhatsApp, testSendViber, testSendMMS). Either add setEndpoint() / getEndpoint() to VonageTrait, or use an environment variable / constructor parameter to override the base URL.
| public function testSendWhatsApp(): void | ||
| { | ||
| $sender = new VonageWhatsApp($this->applicationId, $this->privateKey); |
There was a problem hiding this comment.
json_decode receives an array, not a string. send() returns array, so \json_decode($response, true) is called with an array argument. In PHP 8 this causes a TypeError; in earlier versions it silently returns null, making assertNotEmpty($result) fail. The same pattern is repeated in all four test methods. Either call send() directly and use assertResponse() (already defined in Base), or cast to JSON before decoding.
deepshekhardas
force-pushed
the
feat/8139-vonage-messages-adapter
branch
from
d623ac7 to
4448c62
Compare
| public function __construct( | ||
| private string $applicationId, | ||
| private string $privateKey, | ||
| private ?string $from = null | ||
| ) { | ||
| } |
There was a problem hiding this comment.
Adapter::__construct() but never calls it, so the base class private $sendCounter property stays uninitialized. A normal new VonageMessages(...)->send($message) reaches Adapter::recordResponse() after process() returns, then reads $this->sendCounter and fails with Typed property Utopia\Messaging\Adapter::$sendCounter must not be accessed before initialization unless callers manually call setTelemetry() first.
Artifacts
Repro: focused PHP harness for VonageMessages send without initialized telemetry
- Contains supporting evidence from the run (text/x-php; charset=utf-8).
Stack trace captured during the T-Rex run
- Keeps the raw stack trace available without making the summary code-heavy.
| headers: [ | ||
| 'Content-Type: application/json', | ||
| 'Authorization: Bearer ' . $jwt, | ||
| 'Accept: application/json', | ||
| ], |
There was a problem hiding this comment.
Adapter::request() expects headers as a list of complete header strings and passes the same array to CURLOPT_HTTPHEADER. This associative array can send malformed headers to cURL, so the Vonage request may miss a valid Authorization or Content-Type header.
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/8139-vonage-messages-adapter
branch
from
4448c62 to
43b104a
Compare
| * @throws \Exception | ||
| */ | ||
| protected function process(SMSMessage $message): array | ||
| { |
There was a problem hiding this comment.
The Vonage Messages API requires "viber_service" as the channel value for Viber messages, not "viber". Sending channel: "viber" will result in a 422 Unprocessable Entity from Vonage and no message will be delivered. This is confirmed by the Vonage API reference and the OpenAPI spec.
| { | |
| return $this->processMessage($message, 'viber_service'); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/Viber/Vonage.php
Line: 30
Comment:
**Wrong Viber channel value — all Viber sends will fail**
The Vonage Messages API requires `"viber_service"` as the channel value for Viber messages, not `"viber"`. Sending `channel: "viber"` will result in a 422 Unprocessable Entity from Vonage and no message will be delivered. This is confirmed by the [Vonage API reference](https://developer.vonage.com/en/messages/code-snippets/viber/send-text) and the [OpenAPI spec](https://raw.githubusercontent.com/api-evangelist/vonage/refs/heads/main/openapi/vonage-openapi.yml).
```suggestion
return $this->processMessage($message, 'viber_service');
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Adds a Vonage multi-channel Messages adapter supporting SMS, WhatsApp, Viber, and MMS.
Changes
Testing
Closes #8139