Skip to content

Test PR from ForkPilot - Add test_common_module.py #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alokit-bot wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Test PR from ForkPilot - Add test_common_module.py #6

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions erpnext/test_common_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Common Module - Present in Main Repository
# This is a shared module that should be present in all forks

def validate_email(email):
"""Basic email validation"""
if '@' not in email or '.' not in email:
return False, "Invalid email format"
return True, "Valid email"
Comment on lines +4 to +8
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.

Suggested change
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
Comment on lines +10 to +12
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -100

Repository: aerele/erpnext

Length of output: 8795

🏁 Script executed:

# Read the actual file to see the implementation
sed -n '1,20p' ./erpnext/test_common_module.py

Repository: 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 -50

Repository: 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 -20

Repository: 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 -60

Repository: 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.