Test PR from ForkPilot - Add test_common_module.py#6
Test PR from ForkPilot - Add test_common_module.py#6alokit-bot wants to merge 1 commit intoaerele:developfrom
Conversation
This is a test PR created by automated testing to verify GitHub integration. Files changed: 1
📝 WalkthroughWalkthroughA new utility module Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Warning |
![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
🧹 Nitpick comments (1)
erpnext/test_common_module.py (1)
1-3: Clarify that this is a simplified helper module, not production tax/email logic.Lines 1-2 currently read like a shared production contract. Consider explicitly marking this as test/simple utility to reduce accidental reuse in business flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/test_common_module.py` around lines 1 - 3, The top-of-file comment in erpnext/test_common_module.py reads like a shared production contract; change the header to explicitly state this is a simplified test/helper module (e.g., "Test helper / simplified utility — not production tax/email logic") so maintainers won't reuse it in business flows, and add a brief line indicating intended scope (tests/mocks only) and any limitations or warnings about not using functions from this module in production code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@erpnext/test_common_module.py`:
- Around line 4-8: The validate_email function implements a weak check and can
raise on non-string inputs; replace its logic to call
frappe.utils.validate_email_address (import it) and return the same (bool,
message) tuple: call validate_email_address(email) to perform validation,
catch/handle non-string or raised exceptions and return (False, "Invalid email
format") for invalid cases and (True, "Valid email") for valid ones, ensuring
the function name validate_email remains and behavior aligns with ERPNext
patterns.
- Around line 10-12: The calculate_tax function currently returns a raw float
and lacks validation; update calculate_tax to validate that amount is
non-negative (raise ValueError for negatives), convert and round the result
using ERPNext's flt() with an appropriate precision parameter (e.g., precision
from currency or a passed-in precision arg), and ensure inputs are coerced via
flt() before the multiplication; reference calculate_tax and use flt(...) for
the final returned value to avoid floating-point rounding issues.
---
Nitpick comments:
In `@erpnext/test_common_module.py`:
- Around line 1-3: The top-of-file comment in erpnext/test_common_module.py
reads like a shared production contract; change the header to explicitly state
this is a simplified test/helper module (e.g., "Test helper / simplified utility
— not production tax/email logic") so maintainers won't reuse it in business
flows, and add a brief line indicating intended scope (tests/mocks only) and any
limitations or warnings about not using functions from this module in production
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a094c7b0-0f84-432b-9dd4-c3acca503303
📒 Files selected for processing (1)
erpnext/test_common_module.py
| def validate_email(email): | ||
| """Basic email validation""" | ||
| if '@' not in email or '.' not in email: | ||
| return False, "Invalid email format" | ||
| return True, "Valid email" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify repository-standard email validation usage.
rg -n --type=py -C2 '\bvalidate_email_address\s*\('
rg -n --type=py -C2 'def\s+validate_email\('Repository: aerele/erpnext
Length of output: 2506
Use the framework email validator; current implementation is too weak and inconsistent with codebase patterns.
The validation at line 6 accepts invalid addresses (e.g., "test@.com") and fails on non-string input with a TypeError. Align with the established ERPNext pattern using validate_email_address from frappe.utils.
Proposed fix
+from frappe.utils import validate_email_address
+
def validate_email(email):
- """Basic email validation"""
- if '@' not in email or '.' not in email:
- return False, "Invalid email format"
- return True, "Valid email"
+ """Email validation using framework utility"""
+ if not isinstance(email, str):
+ return False, "Invalid email format"
+ try:
+ validate_email_address(email, throw=True)
+ except Exception:
+ return False, "Invalid email format"
+ return True, "Valid email"📝 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.
| def validate_email(email): | |
| """Basic email validation""" | |
| if '@' not in email or '.' not in email: | |
| return False, "Invalid email format" | |
| return True, "Valid email" | |
| from frappe.utils import validate_email_address | |
| def validate_email(email): | |
| """Email validation using framework utility""" | |
| if not isinstance(email, str): | |
| return False, "Invalid email format" | |
| try: | |
| validate_email_address(email, throw=True) | |
| except Exception: | |
| return False, "Invalid email format" | |
| return True, "Valid email" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/test_common_module.py` around lines 4 - 8, The validate_email
function implements a weak check and can raise on non-string inputs; replace its
logic to call frappe.utils.validate_email_address (import it) and return the
same (bool, message) tuple: call validate_email_address(email) to perform
validation, catch/handle non-string or raised exceptions and return (False,
"Invalid email format") for invalid cases and (True, "Valid email") for valid
ones, ensuring the function name validate_email remains and behavior aligns with
ERPNext patterns.
| def calculate_tax(amount, tax_rate=18): | ||
| """Standard tax calculation""" | ||
| return (amount * tax_rate) / 100 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how monetary precision is handled in existing tax logic.
rg -n --type=py -C2 '\b(flt|precision|Decimal)\b'
rg -n --type=py -C2 '\bcalculate_tax\s*\('Repository: aerele/erpnext
Length of output: 50372
🏁 Script executed:
# Check if test_common_module.py exists
find . -name "test_common_module.py" -type f
# Search for tax calculation functions in the codebase
rg -n "def calculate_tax" --type=py
# Check common patterns for tax calculations in ERPNext
rg -n "tax.*\*.*100|rate.*\*.*100" --type=py -C 2 | head -100Repository: aerele/erpnext
Length of output: 8795
🏁 Script executed:
# Read the actual file to see the implementation
sed -n '1,20p' ./erpnext/test_common_module.pyRepository: aerele/erpnext
Length of output: 459
🏁 Script executed:
# Find the flt function definition
rg -n "^def flt" --type=py
# Look for flt import location
rg -n "from.*flt|import.*flt" erpnext/test_common_module.py
# Check if there are existing tax calculations that use Decimal
rg -n "from decimal import|Decimal" --type=py | head -20
# Look at how tax calculations are done in ERPNext (focus on precision)
rg -n "tax.*flt|flt.*tax" --type=py -C 2 | head -50Repository: aerele/erpnext
Length of output: 4798
🏁 Script executed:
# Search for where flt is defined and what it does
rg "from frappe.utils import.*flt" erpnext/ --type=py | head -5
# Check if any tax calculation validates negative values
rg -n "tax.*<.*0|amount.*<.*0|negative" erpnext/controllers/ --type=py -C 2 | head -40
# Search for Decimal usage in financial calculations
rg -n "from decimal import|import decimal" erpnext/ --type=py | grep -v test | head -20Repository: aerele/erpnext
Length of output: 3952
🏁 Script executed:
# Check if test_common_module is imported or used anywhere
rg "from.*test_common_module|import.*test_common_module" --type=py
# Check what flt actually does - look for its definition in frappe context
rg "def flt" --type=py
# Check actual usage patterns of flt for tax calculations in real code
rg -B2 -A2 "tax_amount.*flt|flt.*tax_amount" --type=py | head -60Repository: aerele/erpnext
Length of output: 4947
🏁 Script executed:
# Check the directory structure and what test_common_module is
head -1 ./erpnext/test_common_module.py
ls -la ./erpnext/test_common_module.py
file ./erpnext/test_common_module.py
# Search to see if it's referenced in __init__ or setup files
rg "test_common" --type=py
# Look at the region implementations more closely to see the pattern
cat -n erpnext/regional/italy/utils.py | sed -n '20,35p'Repository: aerele/erpnext
Length of output: 1184
Use ERPNext's standard flt() function with precision handling instead of float arithmetic.
Line 12 returns a raw float without precision handling, which can introduce rounding errors in currency calculations. Also add input validation for negative values.
The codebase standard for tax calculations (e.g., erpnext/regional/italy/utils.py line 29) uses flt((amount * tax_rate) / 100, precision) to handle both type conversion and rounding with proper decimal places.
Suggested fix
+from frappe.utils import flt
+
def calculate_tax(amount, tax_rate=18):
"""Standard tax calculation"""
- return (amount * tax_rate) / 100
+ if amount < 0 or tax_rate < 0:
+ raise ValueError("amount and tax_rate must be non-negative")
+ return flt((amount * tax_rate) / 100, 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@erpnext/test_common_module.py` around lines 10 - 12, The calculate_tax
function currently returns a raw float and lacks validation; update
calculate_tax to validate that amount is non-negative (raise ValueError for
negatives), convert and round the result using ERPNext's flt() with an
appropriate precision parameter (e.g., precision from currency or a passed-in
precision arg), and ensure inputs are coerced via flt() before the
multiplication; reference calculate_tax and use flt(...) for the final returned
value to avoid floating-point rounding issues.
1 participant
This is a test PR created by automated testing to verify GitHub integration.