Add missing 'device_id' attr in Webhook class#35
Add missing 'device_id' attr in Webhook class#35vk496 wants to merge 4 commits intoandroid-sms-gateway:masterfrom
Conversation
WalkthroughAdded a new optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@android_sms_gateway/domain.py`:
- Around line 218-223: The deserializer is using the snake_case key "device_id"
but API payloads use camelCase; update the factory so the Device/Webhook model
constructor uses payload.get("deviceId") for the device id (keep id as
payload.get("id"), url as payload["url"], and event as
WebhookEvent(payload["event"]))—locate the classmethod that builds the cls
instance (the code creating id=..., device_id=..., url=..., event=...) and
replace the payload lookup for device_id with payload.get("deviceId").
- Around line 231-236: The returned dict uses snake_case for the device key;
change the key "device_id" to camelCase "deviceId" to match the API and the
conversion used by Message.asdict(); update the object method (the
dict-returning method in domain.py that references self.device_id and
self.event.value) to emit "deviceId": self.device_id (or use the existing
snake_to_camel conversion used by Message.asdict()) so keys are consistent with
the API.
|
Thank you for the PR. Please correct the field name as suggested by CodeRabbit so it matches the API documentation. |
|
Do you prefer I update the tests or make None as the default parameter for device_id? |
|
Yes, please add default value. The new version must be backward compatible. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android_sms_gateway/domain.py (1)
195-206: Field ordering violates dataclass rules—will raiseTypeErrorat import.The
device_idfield has a default value but is followed byurlandeventwhich have no defaults. Withoutkw_only=Truein the decorator (unlikeMessageat line 15), Python will raise:TypeError: non-default argument 'url' follows default argumentMove
device_idafter all required fields.🐛 Proposed fix
`@dataclasses.dataclass`(frozen=True) class Webhook: """A webhook configuration.""" id: t.Optional[str] """The unique identifier of the webhook.""" - device_id: t.Optional[str] = None - """The unique identifier of the device the webhook is associated with.""" url: str """The URL the webhook will be sent to.""" event: WebhookEvent """The type of event the webhook is triggered for.""" + device_id: t.Optional[str] = None + """The unique identifier of the device the webhook is associated with."""
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@android_sms_gateway/domain.py`:
- Around line 231-236: Webhook.asdict() currently always emits "deviceId" even
when self.device_id is None; change Webhook.asdict() to follow the
Message.asdict() pattern and only add the "deviceId" key when self.device_id is
not None (i.e., construct the dict with id, url, event and then conditionally
set "deviceId": self.device_id if self.device_id is truthy/ not None) so the
returned mapping omits deviceId when unset.
| return { | ||
| "id": self.id, | ||
| "deviceId": self.device_id, | ||
| "url": self.url, | ||
| "event": self.event.value, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the domain.py file and examine its structure
find . -name "domain.py" -path "*/android_sms_gateway/*" -type fRepository: android-sms-gateway/client-py
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Look at the Webhook class and Message class asdict implementations
rg -A 15 "class Webhook" --type py
rg -A 15 "class Message" --type pyRepository: android-sms-gateway/client-py
Length of output: 4939
🏁 Script executed:
#!/bin/bash
# Search for Message.asdict() to see how it handles optional fields
rg -B 2 -A 10 "def asdict" --type pyRepository: android-sms-gateway/client-py
Length of output: 3877
🏁 Script executed:
#!/bin/bash
# Get the complete Message.asdict() implementation
rg -B 5 -A 40 "class Message:" --type py | head -80Repository: android-sms-gateway/client-py
Length of output: 3181
🏁 Script executed:
#!/bin/bash
# Search for device_id within Message.asdict() context
rg -A 50 "def asdict.*Message" --type pyRepository: android-sms-gateway/client-py
Length of output: 55
🏁 Script executed:
#!/bin/bash
# More targeted search for Message's asdict and how it handles device_id
sed -n '/^class Message:/,/^class [A-Z]/p' android_sms_gateway/domain.py | grep -A 50 "def asdict"Repository: android-sms-gateway/client-py
Length of output: 866
🏁 Script executed:
#!/bin/bash
# Search for webhook-related tests or API calls
rg -i "webhook" --type py -B 2 -A 5 | grep -i -A 5 "create\|update\|deviceid"Repository: android-sms-gateway/client-py
Length of output: 4480
🏁 Script executed:
#!/bin/bash
# Check test files for webhook creation patterns
fd -e "test_*.py" -o -e "*_test.py" | head -20Repository: android-sms-gateway/client-py
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Look for any documented issues or comments about deviceId
rg "deviceId" --type py -B 2 -A 2Repository: android-sms-gateway/client-py
Length of output: 1638
🏁 Script executed:
#!/bin/bash
# Get the full webhook create test to see what's actually sent
rg -A 20 "def test_webhook_create\(" tests/test_client.pyRepository: android-sms-gateway/client-py
Length of output: 753
🏁 Script executed:
#!/bin/bash
# Look for webhook asdict tests
rg -B 5 -A 15 "webhook.*asdict\|asdict.*webhook" --type pyRepository: android-sms-gateway/client-py
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that explicitly test Webhook.asdict()
rg "Webhook.*asdict" --type py -A 5 -B 5Repository: android-sms-gateway/client-py
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check how webhook.asdict() is actually used in the client
rg -B 3 -A 3 "webhook.*asdict\(\)" --type pyRepository: android-sms-gateway/client-py
Length of output: 1868
🏁 Script executed:
#!/bin/bash
# Look at the create_webhook implementation in detail
rg -A 20 "def create_webhook" android_sms_gateway/client.pyRepository: android-sms-gateway/client-py
Length of output: 1183
🏁 Script executed:
#!/bin/bash
# Check what http methods are being called with webhook data
rg "self.http.*webhook" -B 2 -A 2 --type pyRepository: android-sms-gateway/client-py
Length of output: 781
🏁 Script executed:
#!/bin/bash
# Get the complete webhook asdict test
rg -A 30 "def test_webhook_asdict" tests/test_domain.pyRepository: android-sms-gateway/client-py
Length of output: 929
🏁 Script executed:
#!/bin/bash
# Get the rest of the test to see the full expected_dict when device_id is None
rg -A 50 "def test_webhook_asdict" tests/test_domain.py | tail -20Repository: android-sms-gateway/client-py
Length of output: 575
Webhook.asdict() should omit deviceId key when not set.
The current implementation always includes "deviceId": None in the returned dictionary, but the test test_webhook_asdict() expects the key to be absent when device_id is unset. This inconsistency can cause API validation errors and unintended side effects. The codebase already follows this pattern in Message.asdict(), which only includes non-None fields. Update Webhook.asdict() to conditionally include deviceId only when it's set:
Suggested fix
def asdict(self) -> t.Dict[str, t.Any]:
"""Returns a dictionary representation of the webhook.
Returns:
A dictionary containing the webhook's data.
"""
result = {
"id": self.id,
"url": self.url,
"event": self.event.value,
}
if self.device_id is not None:
result["deviceId"] = self.device_id
return result🤖 Prompt for AI Agents
In `@android_sms_gateway/domain.py` around lines 231 - 236, Webhook.asdict()
currently always emits "deviceId" even when self.device_id is None; change
Webhook.asdict() to follow the Message.asdict() pattern and only add the
"deviceId" key when self.device_id is not None (i.e., construct the dict with
id, url, event and then conditionally set "deviceId": self.device_id if
self.device_id is truthy/ not None) so the returned mapping omits deviceId when
unset.
|
Please fix the code so that it passes the tests and merge all commits into one. |
2 participants
It's kinda important one, since it allows to manipulate webhooks from other devicesand not only the credentials related one
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.