feat: Billing for static ip#6292
Conversation
phot0n
requested review from
Aradhya-Tripathi,
adityahase,
balamurali27,
ssiyad and
tanmoysrt
as code owners
|
| Filename | Overview |
|---|---|
| press/press/doctype/static_ip_log/static_ip_log.py | Core billing logic for Static IP; decorator uses wrong attribute names (cloud_provider vs provider, static_ip vs ip/is_static_ip) causing log entries to never be created and OCI support to be silently missing |
| press/press/doctype/server/server.py | Adds @create_static_ip_log_if_applicable decorator to get_static_ip; decorator ordering with @frappe.whitelist() may cause TypeErrors if Frappe passes kwargs |
| press/press/doctype/virtual_machine/virtual_machine.py | Adds Database Server to the list of doctypes that sync is_static_ip from VirtualMachine — straightforward and correct |
| press/press/doctype/database_server/database_server.json | Adds is_static_ip Check field and get_static_ip action button to Database Server doctype — schema change looks correct |
| press/press/doctype/static_ip_log/static_ip_log.json | New Static IP Log doctype definition with server, server_type, static_ip, and status fields; looks correct |
| press/press/doctype/static_ip_log/test_static_ip_log.py | Empty test class — no test cases implemented for the new billing feature |
Sequence Diagram
sequenceDiagram
participant Admin
participant Server/DBServer
participant Decorator as create_static_ip_log_if_applicable
participant VirtualMachine
participant StaticIPLog
Admin->>Server/DBServer: get_static_ip() [whitelisted]
Server/DBServer->>Decorator: wrapper(self)
Decorator->>Decorator: static_ip = self.get("static_ip") [wrong field, returns None]
Decorator->>Server/DBServer: fn(self) → get_aws_static_ip() / get_oci_static_ip()
Server/DBServer->>VirtualMachine: vm_doc.sync()
VirtualMachine->>Server/DBServer: update_servers() sets is_static_ip, ip
Decorator->>Decorator: check self.cloud_provider == "AWS EC2" [wrong attr, never True]
Note over Decorator,StaticIPLog: Log never created due to wrong attribute names
Decorator-->>Admin: return result
Prompt To Fix All With AI
This is a comment left during a code review.
Path: press/press/doctype/static_ip_log/static_ip_log.py
Line: 45-50
Comment:
**`cloud_provider` and `static_ip` attributes don't exist on Server/DatabaseServer**
The decorator accesses `self.cloud_provider` but the actual field on both `Server` and `Database Server` is `self.provider`. Since `self.cloud_provider` will raise `AttributeError` (or silently evaluate to falsy via `getattr`), the condition on line 45 will never match `"AWS EC2"` and **no log entry will ever be created**.
Additionally, `self.static_ip` and `self.get("static_ip")` reference a non-existent field. The IP address is stored in `self.ip`, and the flag field is `is_static_ip` (a `Check`). The log will always be created with an empty `static_ip` value and incorrect status even if the provider check were fixed.
```python
# line 42 – should capture IP before the call
static_ip = self.get("ip")
result = fn(self)
# line 45 – use self.provider, not self.cloud_provider
if self.provider == "AWS EC2":
create_static_ip_log(
server=self.name,
server_type=self.doctype,
static_ip=self.ip or static_ip,
status="Attached" if self.is_static_ip else "Detached",
)
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: press/press/doctype/static_ip_log/static_ip_log.py
Line: 40-55
Comment:
**OCI static IP assignment silently not logged**
The function call at line 43 (`result = fn(self)`) can raise an exception (e.g. if AWS `allocate_address` fails or `frappe.throw` is called for unsupported providers). The decorator only logs for `"AWS EC2"`; `get_static_ip` also calls `get_oci_static_ip` which successfully assigns a reserved IP and syncs the VM, yet the decorator produces no billing log for OCI. Static IP billing for OCI is silently missing.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: press/press/doctype/static_ip_log/static_ip_log.py
Line: 23-25
Comment:
**Validation blocks future server types without graceful handling**
The `validate` hard-codes the allowed `server_type` values to `"Server"` and `"Database Server"`. If the billing feature is ever extended to `"Proxy Server"` or `"NAT Server"` (both of which already have `is_static_ip` synced in `virtual_machine.py`), records created through the decorator will fail validation. Consider removing the guard (the `server_type` field already has a `Link` to DocType for referential integrity) or using a configurable allowlist.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: press/press/doctype/server/server.py
Line: 2562-2563
Comment:
**Decorator order may break `@frappe.whitelist()` calls with kwargs**
The decorator stack is:
```python
@frappe.whitelist()
@create_static_ip_log_if_applicable
def get_static_ip(self): ...
```
The inner `wrapper(self)` only accepts `self`, but Frappe may pass additional keyword arguments to whitelisted methods. If any caller passes extra arguments the call will fail with a `TypeError`. The wrapper should accept `*args, **kwargs` and forward them to `fn`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(server): add log to track static ip..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self.cloud_provider == "AWS EC2": | ||
| create_static_ip_log( | ||
| server=self.name, | ||
| server_type=self.doctype, | ||
| static_ip=self.static_ip or static_ip, | ||
| status="Attached" if self.static_ip else "Detached", |
There was a problem hiding this comment.
cloud_provider and static_ip attributes don't exist on Server/DatabaseServer
The decorator accesses self.cloud_provider but the actual field on both Server and Database Server is self.provider. Since self.cloud_provider will raise AttributeError (or silently evaluate to falsy via getattr), the condition on line 45 will never match "AWS EC2" and no log entry will ever be created.
Additionally, self.static_ip and self.get("static_ip") reference a non-existent field. The IP address is stored in self.ip, and the flag field is is_static_ip (a Check). The log will always be created with an empty static_ip value and incorrect status even if the provider check were fixed.
# line 42 – should capture IP before the call
static_ip = self.get("ip")
result = fn(self)
# line 45 – use self.provider, not self.cloud_provider
if self.provider == "AWS EC2":
create_static_ip_log(
server=self.name,
server_type=self.doctype,
static_ip=self.ip or static_ip,
status="Attached" if self.is_static_ip else "Detached",
)Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/static_ip_log/static_ip_log.py
Line: 45-50
Comment:
**`cloud_provider` and `static_ip` attributes don't exist on Server/DatabaseServer**
The decorator accesses `self.cloud_provider` but the actual field on both `Server` and `Database Server` is `self.provider`. Since `self.cloud_provider` will raise `AttributeError` (or silently evaluate to falsy via `getattr`), the condition on line 45 will never match `"AWS EC2"` and **no log entry will ever be created**.
Additionally, `self.static_ip` and `self.get("static_ip")` reference a non-existent field. The IP address is stored in `self.ip`, and the flag field is `is_static_ip` (a `Check`). The log will always be created with an empty `static_ip` value and incorrect status even if the provider check were fixed.
```python
# line 42 – should capture IP before the call
static_ip = self.get("ip")
result = fn(self)
# line 45 – use self.provider, not self.cloud_provider
if self.provider == "AWS EC2":
create_static_ip_log(
server=self.name,
server_type=self.doctype,
static_ip=self.ip or static_ip,
status="Attached" if self.is_static_ip else "Detached",
)
```
How can I resolve this? If you propose a fix, please make it concise.| def create_static_ip_log_if_applicable(fn): | ||
| def wrapper(self): | ||
| static_ip = self.get("static_ip") | ||
| result = fn(self) | ||
|
|
||
| if self.cloud_provider == "AWS EC2": | ||
| create_static_ip_log( | ||
| server=self.name, | ||
| server_type=self.doctype, | ||
| static_ip=self.static_ip or static_ip, | ||
| status="Attached" if self.static_ip else "Detached", | ||
| ) | ||
|
|
||
| return result | ||
|
|
||
| return wrapper |
There was a problem hiding this comment.
The function call at line 43 (result = fn(self)) can raise an exception (e.g. if AWS allocate_address fails or frappe.throw is called for unsupported providers). The decorator only logs for "AWS EC2"; get_static_ip also calls get_oci_static_ip which successfully assigns a reserved IP and syncs the VM, yet the decorator produces no billing log for OCI. Static IP billing for OCI is silently missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/static_ip_log/static_ip_log.py
Line: 40-55
Comment:
**OCI static IP assignment silently not logged**
The function call at line 43 (`result = fn(self)`) can raise an exception (e.g. if AWS `allocate_address` fails or `frappe.throw` is called for unsupported providers). The decorator only logs for `"AWS EC2"`; `get_static_ip` also calls `get_oci_static_ip` which successfully assigns a reserved IP and syncs the VM, yet the decorator produces no billing log for OCI. Static IP billing for OCI is silently missing.
How can I resolve this? If you propose a fix, please make it concise.| def validate(self): | ||
| if self.server_type not in ("Server", "Database Server"): | ||
| frappe.throw("Server Type must be either 'Server' or 'Database Server'") |
There was a problem hiding this comment.
The validate hard-codes the allowed server_type values to "Server" and "Database Server". If the billing feature is ever extended to "Proxy Server" or "NAT Server" (both of which already have is_static_ip synced in virtual_machine.py), records created through the decorator will fail validation. Consider removing the guard (the server_type field already has a Link to DocType for referential integrity) or using a configurable allowlist.
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/static_ip_log/static_ip_log.py
Line: 23-25
Comment:
**Validation blocks future server types without graceful handling**
The `validate` hard-codes the allowed `server_type` values to `"Server"` and `"Database Server"`. If the billing feature is ever extended to `"Proxy Server"` or `"NAT Server"` (both of which already have `is_static_ip` synced in `virtual_machine.py`), records created through the decorator will fail validation. Consider removing the guard (the `server_type` field already has a `Link` to DocType for referential integrity) or using a configurable allowlist.
How can I resolve this? If you propose a fix, please make it concise.| @frappe.whitelist() | ||
| @create_static_ip_log_if_applicable |
There was a problem hiding this comment.
@frappe.whitelist() calls with kwargs
The decorator stack is:
@frappe.whitelist()
@create_static_ip_log_if_applicable
def get_static_ip(self): ...The inner wrapper(self) only accepts self, but Frappe may pass additional keyword arguments to whitelisted methods. If any caller passes extra arguments the call will fail with a TypeError. The wrapper should accept *args, **kwargs and forward them to fn.
Prompt To Fix With AI
This is a comment left during a code review.
Path: press/press/doctype/server/server.py
Line: 2562-2563
Comment:
**Decorator order may break `@frappe.whitelist()` calls with kwargs**
The decorator stack is:
```python
@frappe.whitelist()
@create_static_ip_log_if_applicable
def get_static_ip(self): ...
```
The inner `wrapper(self)` only accepts `self`, but Frappe may pass additional keyword arguments to whitelisted methods. If any caller passes extra arguments the call will fail with a `TypeError`. The wrapper should accept `*args, **kwargs` and forward them to `fn`.
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (68.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6292 +/- ##
===========================================
+ Coverage 56.33% 56.41% +0.08%
===========================================
Files 936 938 +2
Lines 77710 77935 +225
Branches 525 525
===========================================
+ Hits 43779 43970 +191
- Misses 33903 33937 +34
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* also, add Static IP Plan at relevant places
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Co-authored-by: Shadrak Gurupnor <30501401+shadrak98@users.noreply.github.com>
2 participants
Cases: