feat: Add SMSGateApp adapter #99
Conversation
|
""" WalkthroughA new SMS adapter for "SMS Gateway for Android™" was added, including its implementation, test coverage, and documentation update in the README. The adapter integrates with the SMS Gateway API, processes SMS sending, and handles responses. The README now lists this adapter as supported, and a test verifies its functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as SMSGateAppTest
participant Adapter as SMSGateApp
participant API as SMS Gateway API
Test->>Adapter: send(SMSMessage)
Adapter->>API: POST /send (with credentials and payload)
API-->>Adapter: HTTP 202 Accepted (delivery results)
Adapter-->>Test: Structured response with delivery results
Estimated code review effort2 (20–30 minutes) Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php (1)
77-82: Improve error message extraction.The error handling should be more robust when extracting error messages from the API response.
Consider improving error message handling:
} else { - $errorMessage = $result['response']['message'] ?? 'Unknown error'; + $errorMessage = 'Unknown error'; + if (isset($result['response'])) { + if (is_array($result['response']) && isset($result['response']['message'])) { + $errorMessage = $result['response']['message']; + } elseif (is_string($result['response'])) { + $errorMessage = $result['response']; + } + } + if (isset($result['error']) && !empty($result['error'])) { + $errorMessage = $result['error']; + } + foreach ($message->getTo() as $recipient) { $response->addResult($recipient, $errorMessage); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php(1 hunks)tests/Messaging/Adapter/SMS/SMSGateAppTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php (2)
src/Utopia/Messaging/Adapter.php (2)
Adapter(8-281)request(77-130)src/Utopia/Messaging/Response.php (4)
Response(5-76)addResult(56-63)setDeliveredTo(23-26)toArray(68-75)
🔇 Additional comments (7)
README.md (1)
109-109: LGTM! Documentation properly updated.The new SMS adapter entry follows the established format and correctly marks the adapter as supported with an appropriate link to the service.
tests/Messaging/Adapter/SMS/SMSGateAppTest.php (2)
26-29: LGTM! Message creation follows established patterns.The SMS message instantiation correctly uses named parameters and follows the expected structure for the messaging library.
32-36: LGTM! Response handling is appropriate.The test correctly calls the send method and uses the inherited assertResponse method for validation, maintaining consistency with other adapter tests.
src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php (4)
39-41: LGTM! Maximum message limit is appropriate.The 10 messages per request limit aligns with the service capabilities mentioned in the PR description.
49-54: LGTM! Request body structure is well-formed.The JSON payload structure correctly maps the SMS content and recipients according to the SMS Gateway API requirements.
56-64: LGTM! HTTP request setup is correct.The request configuration properly:
- Uses POST method for message sending
- Includes skipPhoneValidation parameter in URL
- Sets appropriate content type header
- Implements Basic authentication correctly
84-84: LGTM! Response handling follows framework patterns.The method correctly returns the response array using the Response object's toArray method, maintaining consistency with other adapters.
| if ($result['statusCode'] === 202) { | ||
| $success = 0; | ||
| foreach ($result['response']['recipients'] as $recipient) { | ||
| $response->addResult($recipient['phoneNumber'], $recipient['error'] ?? ''); | ||
|
|
||
| if ($recipient['state'] !== 'Failed') { | ||
| $success++; | ||
| } | ||
| } | ||
|
|
||
| $response->setDeliveredTo($success); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive programming for response structure.
The code assumes the response will have a recipients array without validation. This could cause fatal errors if the API response structure changes or is malformed.
Add validation for the response structure:
if ($result['statusCode'] === 202) {
+ if (!isset($result['response']['recipients']) || !is_array($result['response']['recipients'])) {
+ foreach ($message->getTo() as $recipient) {
+ $response->addResult($recipient, 'Invalid API response structure');
+ }
+ return $response->toArray();
+ }
+
$success = 0;
foreach ($result['response']['recipients'] as $recipient) {
+ if (!isset($recipient['phoneNumber'])) {
+ continue;
+ }
$response->addResult($recipient['phoneNumber'], $recipient['error'] ?? '');
- if ($recipient['state'] !== 'Failed') {
+ if (isset($recipient['state']) && $recipient['state'] !== 'Failed') {
$success++;
}
}📝 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.
| if ($result['statusCode'] === 202) { | |
| $success = 0; | |
| foreach ($result['response']['recipients'] as $recipient) { | |
| $response->addResult($recipient['phoneNumber'], $recipient['error'] ?? ''); | |
| if ($recipient['state'] !== 'Failed') { | |
| $success++; | |
| } | |
| } | |
| $response->setDeliveredTo($success); | |
| if ($result['statusCode'] === 202) { | |
| if (!isset($result['response']['recipients']) || !is_array($result['response']['recipients'])) { | |
| foreach ($message->getTo() as $recipient) { | |
| $response->addResult($recipient, 'Invalid API response structure'); | |
| } | |
| return $response->toArray(); | |
| } | |
| $success = 0; | |
| foreach ($result['response']['recipients'] as $recipient) { | |
| if (!isset($recipient['phoneNumber'])) { | |
| continue; | |
| } | |
| $response->addResult($recipient['phoneNumber'], $recipient['error'] ?? ''); | |
| if (isset($recipient['state']) && $recipient['state'] !== 'Failed') { | |
| $success++; | |
| } | |
| } | |
| $response->setDeliveredTo($success); |
🤖 Prompt for AI Agents
In src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php around lines 66 to 76, the
code assumes the 'recipients' array exists in the API response without
validation, which can cause errors if the response structure changes. Add
defensive checks to verify that 'response' and 'recipients' keys exist and that
'recipients' is an array before iterating over it. If these conditions are not
met, handle the situation gracefully, such as by logging an error or skipping
the iteration to prevent fatal errors.
capcom6
force-pushed
the
feat-n-a-smsgateapp-messaging-adapter
branch
from
11fa346 to
d795610
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.env.dev (1)
37-39: Re-order the new keys or update linter config to silence dotenv-linter warnings
dotenv-linterflags these three lines because, alphabetically, theSMSGATEAPP_*keys should appear before theTELNYX_*block (line 10).
If you want to keep the file linter-clean, move the new keys up accordingly or add a rule to ignore ordering for provider-grouped sections.Example diff to satisfy the linter:
- TELNYX_API_KEY= - TELNYX_PUBLIC_KEY= + SMSGATEAPP_USERNAME= + SMSGATEAPP_PASSWORD= + SMSGATEAPP_TO= + TELNYX_API_KEY= + TELNYX_PUBLIC_KEY=(Apply the same shift for
TELNYX_TO=and the rest of that block.)If grouping by provider is intentional, consider committing a
.dotenv-linter.tomlwithunordered_keys = trueto avoid repeated warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.dev(1 hunks)README.md(1 hunks)src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php(1 hunks)tests/Messaging/Adapter/SMS/SMSGateAppTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Messaging/Adapter/SMS/SMSGateAppTest.php
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.dev
[warning] 37-37: [UnorderedKey] The SMSGATEAPP_USERNAME key should go before the TELNYX_API_KEY key
[warning] 38-38: [UnorderedKey] The SMSGATEAPP_PASSWORD key should go before the SMSGATEAPP_USERNAME key
[warning] 39-39: [UnorderedKey] The SMSGATEAPP_TO key should go before the SMSGATEAPP_USERNAME key
1 participant
What does this PR do?
Adds a new SMS adapter for SMS Gateway for Android™ with the following features:
src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php]Test Plan
Tests have been added in [
tests/Messaging/Adapter/SMS/SMSGateAppTest.php].Manual verification steps:
Related PRs and Issues
None
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit