Add webhook management methods #16
Conversation
WalkthroughThis pull request introduces webhook management capabilities to the Android SMS Gateway client library. The changes include adding new methods to the client classes for creating, retrieving, and deleting webhooks, along with supporting infrastructure such as a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIClient
participant WebhookService
Client->>APIClient: create_webhook(webhook)
APIClient->>WebhookService: POST /webhooks
WebhookService-->>APIClient: Return created webhook
APIClient-->>Client: Return webhook
Client->>APIClient: get_webhooks()
APIClient->>WebhookService: GET /webhooks
WebhookService-->>APIClient: Return list of webhooks
APIClient-->>Client: Return webhook list
Client->>APIClient: delete_webhook(webhook_id)
APIClient->>WebhookService: DELETE /webhooks/{id}
WebhookService-->>APIClient: Confirm deletion
APIClient-->>Client: Webhook deleted
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
pyproject.toml (1)
45-45: Document the encryption functionality.Since this PR introduces encryption capabilities:
- Consider adding documentation about the encryption features and their purpose
- Specify whether this is an optional or required feature for webhook functionality
tests/test_client.py (1)
53-57: Move test data to fixtures.Hardcoded test values are used across multiple tests. Consider moving them to fixtures for better maintainability.
@pytest.fixture def test_webhook(): return Webhook( id="webhook_123", url="https://example.com/webhook", event=WebhookEvent.SMS_RECEIVED, )android_sms_gateway/http.py (2)
82-89: Consider adding error response handling.While the implementation is correct, consider extracting the error handling into the
_process_responsemethod for consistency withgetandpostmethods.- self._session.delete(url, headers=headers).raise_for_status() + return self._process_response(self._session.delete(url, headers=headers))
144-151: Consider consistent response handling with get/post methods.For consistency with the
getandpostmethods, consider using method chaining.- response = await self._client.delete(url, headers=headers) - response.raise_for_status() + return ( + self._client.delete(url, headers=headers) + .raise_for_status() + )android_sms_gateway/ahttp.py (1)
148-156: Consider using async context manager for consistency.For consistency with the AiohttpAsyncHttpClient implementation, consider using an async context manager.
- response = await self._client.delete(url, headers=headers) - response.raise_for_status() + async with self._client.delete(url, headers=headers) as response: + response.raise_for_status()android_sms_gateway/client.py (1)
164-178: Consider adding existence check before deletion.Both sync and async delete_webhook methods might benefit from verifying the webhook exists before attempting deletion.
Consider adding a check:
def delete_webhook(self, _id: str) -> None: if self.http is None: raise ValueError("HTTP client not initialized") # Check if webhook exists webhooks = self.get_webhooks() if not any(webhook.id == _id for webhook in webhooks): raise ValueError(f"Webhook with id {_id} does not exist") self.http.delete(f"{self.base_url}/webhooks/{_id}", headers=self.headers)Also applies to: 273-286
README.md (1)
104-116: Consider using proper markdown headings.Instead of using emphasis for section headers, consider using proper markdown heading syntax.
-**Messages** +### Messages - `send(message: domain.Message) -> domain.MessageState`: Send a new SMS message. - `get_state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent message by its ID. -**Webhooks** +### Webhooks🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. -get_state(_i...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. -delete_webhook...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. -delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
106-106: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Makefile(1 hunks)README.md(1 hunks)android_sms_gateway/ahttp.py(5 hunks)android_sms_gateway/client.py(4 hunks)android_sms_gateway/domain.py(2 hunks)android_sms_gateway/enums.py(1 hunks)android_sms_gateway/http.py(5 hunks)pyproject.toml(1 hunks)tests/test_client.py(1 hunks)tests/test_domain.py(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. - get_state(_i...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. - delete_webhook...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. - delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
README.md
106-106: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
Makefile (1)
9-9: Verify the relationship between encryption and webhook management.The PR description mentions webhook management capabilities, but this change adds encryption dependencies. Could you clarify if encryption is required for webhook functionality? If not, consider submitting these changes in a separate PR focused on encryption features.
Let's check if there are any webhook-related changes that might require encryption:
android_sms_gateway/enums.py (1)
12-30: Well-structured webhook event enumeration!The WebhookEvent enum is well-designed with:
- Clear category:action pattern in values
- Comprehensive coverage of SMS states
- Helpful docstrings for each event
android_sms_gateway/http.py (1)
16-28: LGTM! Well-documented protocol definition.The abstract delete method is clearly defined with comprehensive docstrings.
android_sms_gateway/ahttp.py (2)
16-30: LGTM! Well-structured async protocol definition.The async delete method is properly defined with comprehensive docstrings.
89-96: LGTM! Proper async context manager usage.The implementation correctly uses async context managers for resource handling.
android_sms_gateway/client.py (1)
126-141: LGTM! Well-implemented webhook retrieval.The get_webhooks implementation is clean and properly handles the response transformation.
| return cls( | ||
| id=payload["id"], | ||
| url=payload["url"], | ||
| event=WebhookEvent(payload["event"]), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make id field handling more robust in from_dict.
The method assumes 'id' is always present in the payload, but it's defined as Optional in the class.
return cls(
- id=payload["id"],
+ id=payload.get("id"),
url=payload["url"],
event=WebhookEvent(payload["event"]),
)📝 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.
| return cls( | |
| id=payload["id"], | |
| url=payload["url"], | |
| event=WebhookEvent(payload["event"]), | |
| ) | |
| return cls( | |
| id=payload.get("id"), | |
| url=payload["url"], | |
| event=WebhookEvent(payload["event"]), | |
| ) |
| def test_webhook_create(self, client: APIClient): | ||
| """ | ||
| Tests that a webhook can be successfully created using the client. | ||
| It creates a webhook, and then asserts that the created webhook matches the | ||
| expected values. | ||
| :param client: An instance of `APIClient`. | ||
| """ | ||
| item = Webhook( | ||
| id="webhook_123", | ||
| url="https://example.com/webhook", | ||
| event=WebhookEvent.SMS_RECEIVED, | ||
| ) | ||
|
|
||
| created = client.create_webhook(item) | ||
|
|
||
| assert created.id == "webhook_123" | ||
| assert created.url == "https://example.com/webhook" | ||
| assert created.event == WebhookEvent.SMS_RECEIVED | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case testing for webhook creation.
The test only covers the happy path. Consider adding tests for:
- Invalid URLs
- Invalid event types
- Server errors
def test_webhook_create_invalid_url(self, client: APIClient):
"""Tests that creating a webhook with an invalid URL raises an error."""
with pytest.raises(ValueError):
client.create_webhook(Webhook(
url="not_a_url",
event=WebhookEvent.SMS_RECEIVED
))| def test_webhook_get(self, client: APIClient): | ||
| """ | ||
| Tests that the `get_webhooks` method retrieves a non-empty list of webhooks | ||
| and that it contains a webhook with the expected ID, URL, and event type. | ||
| :param client: An instance of `APIClient`. | ||
| """ | ||
|
|
||
| webhooks = client.get_webhooks() | ||
|
|
||
| assert len(webhooks) > 0 | ||
|
|
||
| assert any( | ||
| [ | ||
| webhook.id == "webhook_123" | ||
| and webhook.url == "https://example.com/webhook" | ||
| and webhook.event == WebhookEvent.SMS_RECEIVED | ||
| for webhook in webhooks | ||
| ] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make tests independent of each other.
The get and delete tests depend on the state created by the create test. This makes the tests brittle and harder to maintain.
Consider:
- Using setup/teardown to ensure a clean state for each test
- Creating required webhooks within each test
- Cleaning up created webhooks after each test
Also applies to: 86-100
capcom6
changed the title
capcom6
force-pushed
the
webhooks/add-webhooks-support
branch
from
0924289 to
75e58e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_client.py (1)
28-34: Consider validating environment variables before client initialization.The fixture could benefit from explicit validation of environment variables before client initialization to provide clearer error messages.
with RequestsHttpClient() as h, APIClient( - os.environ.get("API_LOGIN") or "test", - os.environ.get("API_PASSWORD") or "test", - base_url=os.environ.get("API_BASE_URL") or DEFAULT_URL, + login=os.environ.get("API_LOGIN") or "test", + password=os.environ.get("API_PASSWORD") or "test", + base_url=os.environ.get("API_BASE_URL") or DEFAULT_URL, http=h, ) as c:tests/test_domain.py (1)
85-101: Add edge cases to webhook serialization tests.Consider adding tests for:
- Empty URL
- Malformed URL
- Invalid event type
def test_webhook_from_dict_empty_url(): """Test that empty URL raises ValueError.""" with pytest.raises(ValueError): Webhook.from_dict({"url": "", "event": "sms:received"})android_sms_gateway/client.py (1)
127-142: Consider pagination for webhook listing.The
get_webhooksmethod might need to handle pagination if the number of webhooks grows large.Also applies to: 237-252
README.md (1)
111-115: Add usage examples for webhook methods.Consider adding code examples demonstrating how to use the webhook management methods.
# Example webhook creation webhook = domain.Webhook( url="https://example.com/webhook", event=WebhookEvent.SMS_RECEIVED ) client.create_webhook(webhook)🧰 Tools
🪛 LanguageTool
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. -delete_webhook...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. -delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
111-111: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)android_sms_gateway/ahttp.py(5 hunks)android_sms_gateway/client.py(4 hunks)android_sms_gateway/domain.py(2 hunks)android_sms_gateway/http.py(5 hunks)tests/test_client.py(1 hunks)tests/test_domain.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- android_sms_gateway/domain.py
- android_sms_gateway/ahttp.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. - get_state(_i...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. - delete_webhook...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. - delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
README.md
106-106: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (4)
tests/test_client.py (2)
47-67: Add error case testing for webhook creation.The test only covers the happy path. Consider adding tests for error cases.
#!/bin/bash # Check for existing error case tests rg -l "test_webhook.*invalid|test_webhook.*error" tests/
83-118: Make tests independent of each other.The tests are tightly coupled to the state created by previous tests. This makes them brittle and harder to maintain.
#!/bin/bash # Check for test cleanup or setup methods rg "def setup|def teardown|@pytest.fixture" tests/test_client.pyandroid_sms_gateway/http.py (1)
16-28: Well-implemented HTTP client changes.The delete method implementation is consistent across clients with proper error handling and status checking.
Also applies to: 82-89, 144-151
android_sms_gateway/client.py (1)
144-163: Add URL validation to webhook creation.Consider validating the webhook URL format before sending it to the server.
capcom6
force-pushed
the
webhooks/add-webhooks-support
branch
from
75e58e6 to
8dbbd71
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
android_sms_gateway/domain.py (1)
79-93: Add error handling for required fields.The method could raise KeyError if required fields are missing. Consider adding explicit error handling:
@classmethod def from_dict(cls, payload: t.Dict[str, t.Any]) -> "Webhook": + required_fields = ['url', 'event'] + missing_fields = [field for field in required_fields if field not in payload] + if missing_fields: + raise ValueError(f"Missing required fields: {', '.join(missing_fields)}") + return cls( id=payload.get("id"), url=payload["url"], event=WebhookEvent(payload["event"]), )README.md (1)
106-115: Improve section formatting in documentation.Consider these formatting improvements:
- Use heading syntax (
###) instead of bold text for "Messages" and "Webhooks" sections- Remove trailing colons after method signatures
-**Messages** +### Messages -**Webhooks** +### Webhooks🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. -get_state(_i...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. -delete_webhook...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. -delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
106-106: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
android_sms_gateway/http.py (1)
82-89: Consider reducing code duplication in HTTP clients.The session/client initialization check is repeated across all methods. Consider extracting it to a decorator or helper method:
def require_session(func): def wrapper(self, *args, **kwargs): if self._session is None: # or self._client for HttpxHttpClient raise ValueError("Session not initialized") return func(self, *args, **kwargs) return wrapper class RequestsHttpClient(HttpClient): @require_session def delete(self, url: str, *, headers: t.Optional[t.Dict[str, str]] = None) -> None: self._session.delete(url, headers=headers).raise_for_status()Also applies to: 144-151
android_sms_gateway/ahttp.py (2)
66-71: Standardize error messages across methods.While the error handling is robust, consider standardizing the error messages for consistency:
- "Session not initialized" (get)
- "Session not initialized" (post)
- "Session not initialized" (delete)
Consider extracting this to a class constant:
class AiohttpAsyncHttpClient(AsyncHttpClient): + _SESSION_NOT_INITIALIZED_ERROR = "Session not initialized" + def __init__(self, session: t.Optional[aiohttp.ClientSession] = None) -> None: self._session = session async def get(self, url: str, *, headers: t.Optional[t.Dict[str, str]] = None) -> dict: if self._session is None: - raise ValueError("Session not initialized") + raise ValueError(self._SESSION_NOT_INITIALIZED_ERROR)Also applies to: 80-87, 89-96
148-156: Align response handling with get/post methods.While the delete implementation is correct, consider aligning the response handling style with get/post methods for consistency:
async def delete( self, url: str, *, headers: t.Optional[t.Dict[str, str]] = None ) -> None: if self._client is None: raise ValueError("Client not initialized") response = await self._client.delete(url, headers=headers) - response.raise_for_status() + response.raise_for_status().json() # Note: result discarded as per return type
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(3 hunks)android_sms_gateway/ahttp.py(5 hunks)android_sms_gateway/client.py(4 hunks)android_sms_gateway/domain.py(2 hunks)android_sms_gateway/http.py(5 hunks)tests/test_client.py(1 hunks)tests/test_domain.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_domain.py
- android_sms_gateway/client.py
- tests/test_client.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. - get_state(_i...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. - delete_webhook...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. - delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~129-~129: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2565 characters long)
Context: ... Contributing Contributions are welcome! Please submit a pull request or create ...
(EN_EXCESSIVE_EXCLAMATION)
🪛 Markdownlint (0.37.0)
README.md
106-106: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (6)
android_sms_gateway/domain.py (2)
68-77: Well-structured domain model with good documentation!The
Webhookdataclass follows good practices:
- Immutable design using
@dataclasses.dataclass(frozen=True)- Clear documentation for each field
- Consistent with other domain models in the codebase
Note: The previous review suggestion about URL validation is still applicable.
95-105: LGTM! Clean and correct implementation.The method correctly handles all fields and properly serializes the enum value.
android_sms_gateway/http.py (1)
16-28: Well-documented abstract method with clear contract.The delete method is properly defined with comprehensive docstring and consistent signature.
android_sms_gateway/ahttp.py (3)
16-30: Well-documented protocol extension!The new
deleteabstract method is properly typed and follows REST conventions with a void return type. The comprehensive docstring will help implementers understand the contract.
57-59: Improved session cleanup logic!The null check before session cleanup prevents potential issues during context manager exit.
66-67: Verify error handling coverage.The error handling for uninitialized clients is consistent across methods. Let's verify there are no missed cases:
Also applies to: 80-81, 92-93, 127-128, 141-142, 151-152
✅ Verification successful
Error handling verification complete
The error handling for uninitialized clients is consistently implemented across both synchronous (http.py) and asynchronous (ahttp.py) implementations, with proper checks before all session and client operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent error handling across HTTP client implementations # Look for raise ValueError patterns in HTTP client methods echo "Checking error handling patterns:" rg --type py 'raise ValueError\("(?:Session|Client) not initialized"\)' -A 1 -B 1Length of output: 2395
capcom6
force-pushed
the
webhooks/add-webhooks-support
branch
from
8dbbd71 to
cad7130
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/test_domain.py (1)
85-101: 🛠️ Refactor suggestionAdd error case tests for webhook validation.
The current tests only cover the happy path. Consider adding tests for:
- Invalid URL formats
- Missing required fields
- Invalid event types
def test_webhook_from_dict_invalid_url(): """Tests that creating a webhook with an invalid URL raises an error.""" with pytest.raises(ValueError): Webhook.from_dict({ "url": "not_a_url", "event": "sms:received" }) def test_webhook_from_dict_missing_fields(): """Tests that creating a webhook with missing fields raises an error.""" with pytest.raises(KeyError): Webhook.from_dict({"url": "https://example.com"}) # missing eventandroid_sms_gateway/client.py (1)
144-163: 🛠️ Refactor suggestionAdd URL validation for webhook creation.
Consider adding URL validation before sending the request to ensure the URL is properly formatted and uses HTTPS.
from urllib.parse import urlparse def _validate_webhook_url(self, url: str) -> None: """Validates the webhook URL format and scheme.""" parsed = urlparse(url) if not parsed.scheme or not parsed.netloc: raise ValueError("Invalid URL format") if parsed.scheme != "https": raise ValueError("URL must use HTTPS scheme")
🧹 Nitpick comments (3)
tests/test_domain.py (1)
103-137: Add edge case tests for webhook serialization.The asdict tests could be expanded to cover more scenarios:
- Empty URL validation
- Special characters in URLs
- All possible event types
android_sms_gateway/http.py (1)
82-89: Consider adding response status code validation.Both implementations could benefit from explicit status code validation for DELETE operations, especially for 404 (Not Found) and 403 (Forbidden) responses.
def _validate_delete_response(self, response): """Validates the response from a DELETE request.""" if response.status_code == 404: raise ValueError("Webhook not found") if response.status_code == 403: raise ValueError("Permission denied") response.raise_for_status()Also applies to: 144-151
android_sms_gateway/client.py (1)
165-179: Add retry logic for webhook deletion.Consider implementing retry logic for delete operations to handle temporary network issues.
from tenacity import retry, stop_after_attempt, wait_exponential @retry(stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=4, max=10)) def delete_webhook(self, _id: str) -> None: """Deletes a webhook with retry logic.""" if self.http is None: raise ValueError("HTTP client not initialized") self.http.delete(f"{self.base_url}/webhooks/{_id}", headers=self.headers)Also applies to: 275-288
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(2 hunks)android_sms_gateway/ahttp.py(5 hunks)android_sms_gateway/client.py(4 hunks)android_sms_gateway/domain.py(2 hunks)android_sms_gateway/http.py(5 hunks)tests/test_client.py(1 hunks)tests/test_domain.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- android_sms_gateway/domain.py
- tests/test_client.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. - get_state(_i...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. - delete_webhook...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. - delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~129-~129: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2565 characters long)
Context: ... Contributing Contributions are welcome! Please submit a pull request or create ...
(EN_EXCESSIVE_EXCLAMATION)
🪛 Markdownlint (0.37.0)
README.md
106-106: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (4)
README.md (1)
104-115: LGTM! Clear and well-organized documentation.The webhook management methods are well documented with clear method signatures and descriptions.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. -get_state(_i...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. -delete_webhook...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. -delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
106-106: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
android_sms_gateway/http.py (1)
16-28: LGTM! Well-documented abstract method.The delete method is properly defined with clear documentation and type hints.
android_sms_gateway/ahttp.py (2)
16-30: LGTM! Well-documented async abstract method.The async delete method is properly defined with clear documentation and type hints.
89-96: Consider adding response status code validation for async clients.Both async implementations could benefit from explicit status code validation for DELETE operations.
Also applies to: 148-156
capcom6
force-pushed
the
webhooks/add-webhooks-support
branch
from
cad7130 to
f78072c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
android_sms_gateway/domain.py (1)
72-77: 🛠️ Refactor suggestionAdd URL validation to prevent runtime errors.
The url field should be validated to ensure it's a valid HTTPS URL before being sent to the server.
Apply this diff to add URL validation:
+from urllib.parse import urlparse + @dataclasses.dataclass(frozen=True) class Webhook: """A webhook configuration.""" id: t.Optional[str] """The unique identifier of the webhook.""" url: str """The URL the webhook will be sent to.""" event: WebhookEvent """The type of event the webhook is triggered for.""" + + def __post_init__(self): + parsed = urlparse(self.url) + if not all([parsed.scheme == 'https', parsed.netloc]): + raise ValueError("URL must be a valid HTTPS URL")
🧹 Nitpick comments (2)
android_sms_gateway/domain.py (1)
89-93: Make field handling more consistent in from_dict.The method uses get() for 'id' but not for other fields, which could lead to KeyError if fields are missing.
Apply this diff for consistent error handling:
return cls( id=payload.get("id"), - url=payload["url"], - event=WebhookEvent(payload["event"]), + url=payload.get("url") or "", + event=WebhookEvent(payload.get("event", "")) )README.md (1)
106-115: Fix markdown formatting issues.The following formatting issues were detected:
- Use proper heading syntax (##) instead of bold text for "Messages" and "Webhooks" sections
- Remove loose punctuation marks after code blocks
Apply these changes:
-**Messages** +## Messages -**Webhooks** +## Webhooks🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. -get_state(_i...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. -delete_webhook...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. -delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint (0.37.0)
106-106: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(2 hunks)android_sms_gateway/ahttp.py(5 hunks)android_sms_gateway/client.py(4 hunks)android_sms_gateway/domain.py(2 hunks)android_sms_gateway/http.py(5 hunks)tests/test_client.py(1 hunks)tests/test_domain.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_domain.py
- tests/test_client.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ... domain.Message) -> domain.MessageState: Send a new SMS message. - get_state(_i...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~109-~109: Loose punctuation mark.
Context: ..._state(_id: str) -> domain.MessageState`: Retrieve the state of a previously sent...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~113-~113: Loose punctuation mark.
Context: ...get_webhooks() -> list[domain.Webhook]: Retrieve a list of all webhooks registe...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~114-~114: Loose punctuation mark.
Context: ...hook: domain.Webhook) -> domain.Webhook: Create a new webhook. - delete_webhook...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~115-~115: Loose punctuation mark.
Context: ...ew webhook. - delete_webhook(_id: str): Delete a webhook by its ID. ## HTTP Cl...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~129-~129: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2565 characters long)
Context: ... Contributing Contributions are welcome! Please submit a pull request or create ...
(EN_EXCESSIVE_EXCLAMATION)
🪛 Markdownlint (0.37.0)
README.md
106-106: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
111-111: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (10)
android_sms_gateway/http.py (3)
16-28: LGTM! Well-documented protocol extension.The new
deletemethod is well-documented with clear parameter descriptions and return type.
82-89: LGTM! Proper error handling implementation.The implementation correctly:
- Checks for uninitialized session
- Uses raise_for_status() for error handling
144-151: LGTM! Consistent implementation across clients.The HttpxHttpClient implementation maintains consistency with RequestsHttpClient.
android_sms_gateway/ahttp.py (3)
16-30: LGTM! Well-defined async protocol extension.The async delete method maintains consistency with the sync version while following async patterns.
89-96: LGTM! Proper async resource management.The implementation correctly uses async context manager for proper resource cleanup.
148-156: LGTM! Consistent async implementation.The HttpxAsyncHttpClient implementation maintains consistency with AiohttpAsyncHttpClient.
android_sms_gateway/client.py (4)
85-85: LGTM! Improved HTTP client lifecycle management.The changes properly handle HTTP client initialization and cleanup.
Also applies to: 89-89, 91-91, 96-100
127-142: LGTM! Well-implemented webhook retrieval.The get_webhooks implementation is consistent between sync and async versions with proper error handling.
Also applies to: 237-252
144-163: Add URL validation to complement existing type checks.The Webhook class already has several validation mechanisms through type hints, immutable dataclass, and enum validation. However, consider adding URL format validation to ensure the
urlfield contains a valid URL before sending it to the server.Example validations to add:
- URL format validation (e.g., using
urllib.parse.urlparse)- Empty string check for the URL field
- HTTPS scheme requirement (if applicable)
Also applies to: 254-273
165-179: LGTM! Clean webhook deletion implementation.The delete_webhook implementation is consistent between sync and async versions with proper error handling.
Also applies to: 275-288
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependencies
pycryptodome).Testing