fix: add tax_id update for Tax Withholding Entry#4110
fix: add tax_id update for Tax Withholding Entry#4110ljain112 wants to merge 6 commits intoresilient-tech:developfrom
Conversation
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| india_compliance/hooks.py | Registers get_tax_id_for_party as a regional override for India, routing TDS party ID lookups to the IC implementation that returns pan instead of tax_id. |
| india_compliance/income_tax_india/overrides/tax_withholding_category.py | Adds a one-line override of get_tax_id_for_party that returns the pan field for Supplier/Customer. No fallback for party types that don't carry a pan field (e.g., Employee), which would silently return None instead of the original tax_id. |
| india_compliance/income_tax_india/overrides/test_tax_withholding_category.py | New integration test file covering PAN-based TDS deduction, threshold aggregation across shared-PAN parties, and LDC application. Tests use generate_unique_pan() to avoid cross-run pollution, but existing_pans.add(pan) in that helper is dead code (no-op when element is already in the set). |
| india_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py | Migration patch that backfills tax_id with the party's pan for all non-migration Tax Withholding Entries in Indian companies. Logic is correct: INNER JOIN on party, filters on non-empty PAN, and excludes migration-created entries via created_by_migration == 0. |
| india_compliance/patches.txt | Appends the new v16 patch entry; trailing newline added, resolving a pre-existing POSIX convention issue. |
Reviews (7): Last reviewed commit: "fix: replace random PAN generation with ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
Show resolved
Hide resolved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an India regional override routing ERPNext's tax-withholding tax ID lookup to an India-specific implementation. Implements 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (1)
india_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py (1)
23-33: Consider chunking updates to reduce migration lock time on large sites.Line 23-Line 33 runs one broad UPDATE per party type. For high-volume
Tax Withholding Entrytables, chunking (e.g., by company) can reduce lock duration during migrate windows.Proposed refactor (company-wise chunks)
def execute(): @@ - update_tax_id("Supplier", indian_companies) - update_tax_id("Customer", indian_companies) + update_tax_id("Supplier", indian_companies) + update_tax_id("Customer", indian_companies) @@ def update_tax_id(party_type, companies): twe = frappe.qb.DocType("Tax Withholding Entry", alias="twe") party = frappe.qb.DocType(party_type, alias="party") - - ( - frappe.qb.update(twe) - .join(party) - .on(twe.party == party.name) - .set(twe.tax_id, party.pan) - .where(twe.party_type == party_type) - .where(twe.company.isin(companies)) - .where(party.pan.isnotnull()) - .where(party.pan != "") - .where((twe.tax_id.isnull()) | (twe.tax_id == "")) - .where(twe.created_by_migration == 0) - .run() - ) + for company in companies: + ( + frappe.qb.update(twe) + .join(party) + .on(twe.party == party.name) + .set(twe.tax_id, party.pan) + .where(twe.party_type == party_type) + .where(twe.company == company) + .where(party.pan.isnotnull()) + .where(party.pan != "") + .where((twe.tax_id.isnull()) | (twe.tax_id == "")) + .where(twe.created_by_migration == 0) + .run() + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e04f148-bd25-4f82-abb6-1dd58709fe4e
📒 Files selected for processing (5)
india_compliance/hooks.pyindia_compliance/income_tax_india/overrides/tax_withholding_category.pyindia_compliance/income_tax_india/overrides/test_tax_withholding_category.pyindia_compliance/patches.txtindia_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
Show resolved
Hide resolved
india_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py (1)
169-176: Assert exactTax Withholding Entrycount before checking row fields.The current assertion validates only the first row; if multiple rows are unexpectedly created, this can mask regressions.
Suggested test-hardening diff
twe_rows = frappe.get_all( "Tax Withholding Entry", filters={"parenttype": "Purchase Invoice", "parent": pi.name}, fields=["tax_id", "lower_deduction_certificate"], ) - self.assertTrue(twe_rows) + self.assertEqual(len(twe_rows), 1) self.assertEqual(twe_rows[0].tax_id, pan) self.assertEqual(twe_rows[0].lower_deduction_certificate, ldc_doc.name)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53d7a150-db05-4877-adda-20adcc158e3e
📒 Files selected for processing (2)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.pyindia_compliance/patches.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- india_compliance/patches.txt
india_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py
Outdated
Show resolved
Hide resolved
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py (1)
218-243: Make existing-category setup fully deterministic by resetting key flags.When a category already exists, calling this helper without explicit kwargs can retain stale fields from prior state. Reset
disable_transaction_thresholdexplicitly for deterministic fixtures.♻️ Suggested idempotency tweak
def create_tax_withholding_category(category_name, account_name, **kwargs): fiscal_year = get_fiscal_year(today(), company=COMPANY, as_dict=True) tax_withholding_rate = kwargs.pop("tax_withholding_rate", 10) single_threshold = kwargs.pop("single_threshold", 0) cumulative_threshold = kwargs.pop("cumulative_threshold", 0) + disable_transaction_threshold = kwargs.pop("disable_transaction_threshold", 0) @@ - doc.update(kwargs) + doc.update( + { + "disable_transaction_threshold": disable_transaction_threshold, + **kwargs, + } + ) doc.set("accounts", [account_row]) doc.set("rates", [rate_row]) doc.save()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c62d542-681e-467d-96f6-2c2a9a653732
📒 Files selected for processing (3)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.pyindia_compliance/patches.txtindia_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py
🚧 Files skipped from review as they are similar to previous changes (2)
- india_compliance/patches.txt
- india_compliance/patches/v16/update_tax_id_for_tax_withholding_entries.py
| twe_rows = frappe.get_all( | ||
| "Tax Withholding Entry", | ||
| filters={"parenttype": "Purchase Invoice", "parent": pi.name}, | ||
| fields=["tax_id", "lower_deduction_certificate"], | ||
| ) | ||
| self.assertTrue(twe_rows) | ||
| self.assertEqual(twe_rows[0].tax_id, pan) | ||
| self.assertEqual(twe_rows[0].lower_deduction_certificate, ldc_doc.name) |
There was a problem hiding this comment.
Avoid indexing twe_rows[0] without asserting row cardinality.
frappe.get_all ordering/cardinality assumptions can make this test brittle. Assert exactly one row (or iterate deterministically) before field checks.
✅ Suggested test hardening
twe_rows = frappe.get_all(
"Tax Withholding Entry",
filters={"parenttype": "Purchase Invoice", "parent": pi.name},
fields=["tax_id", "lower_deduction_certificate"],
)
self.assertTrue(twe_rows)
- self.assertEqual(twe_rows[0].tax_id, pan)
- self.assertEqual(twe_rows[0].lower_deduction_certificate, ldc_doc.name)
+ self.assertEqual(len(twe_rows), 1)
+ row = twe_rows[0]
+ self.assertEqual(row.tax_id, pan)
+ self.assertEqual(row.lower_deduction_certificate, ldc_doc.name)📝 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.
| twe_rows = frappe.get_all( | |
| "Tax Withholding Entry", | |
| filters={"parenttype": "Purchase Invoice", "parent": pi.name}, | |
| fields=["tax_id", "lower_deduction_certificate"], | |
| ) | |
| self.assertTrue(twe_rows) | |
| self.assertEqual(twe_rows[0].tax_id, pan) | |
| self.assertEqual(twe_rows[0].lower_deduction_certificate, ldc_doc.name) | |
| twe_rows = frappe.get_all( | |
| "Tax Withholding Entry", | |
| filters={"parenttype": "Purchase Invoice", "parent": pi.name}, | |
| fields=["tax_id", "lower_deduction_certificate"], | |
| ) | |
| self.assertTrue(twe_rows) | |
| self.assertEqual(len(twe_rows), 1) | |
| row = twe_rows[0] | |
| self.assertEqual(row.tax_id, pan) | |
| self.assertEqual(row.lower_deduction_certificate, ldc_doc.name) |
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
Outdated
Show resolved
Hide resolved
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py (1)
169-176:⚠️ Potential issue | 🟡 MinorAssert row cardinality before indexing
twe_rows[0].This assertion path is brittle if more than one
Tax Withholding Entryrow is returned.✅ Suggested hardening
twe_rows = frappe.get_all( "Tax Withholding Entry", filters={"parenttype": "Purchase Invoice", "parent": pi.name}, fields=["tax_id", "lower_deduction_certificate"], ) self.assertTrue(twe_rows) - self.assertEqual(twe_rows[0].tax_id, pan) - self.assertEqual(twe_rows[0].lower_deduction_certificate, ldc_doc.name) + self.assertEqual(len(twe_rows), 1) + row = twe_rows[0] + self.assertEqual(row.tax_id, pan) + self.assertEqual(row.lower_deduction_certificate, ldc_doc.name)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e307c6a6-fcc3-446a-87e6-a566dedf0bb9
📒 Files selected for processing (1)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
| import random | ||
| import string |
There was a problem hiding this comment.
Replace random-based PAN generation to avoid Ruff S311 CI failures.
generate_random_pan() uses random.choices/random.choice, which is flagged by Ruff S311 (as already reported). This can block lint even in tests.
✅ Suggested fix
-import random
+import secrets
import string
@@
def generate_random_pan():
return (
- "".join(random.choices(string.ascii_uppercase, k=5))
- + "".join(random.choices(string.digits, k=4))
- + random.choice(string.ascii_uppercase)
+ "".join(secrets.choice(string.ascii_uppercase) for _ in range(5))
+ + "".join(secrets.choice(string.digits) for _ in range(4))
+ + secrets.choice(string.ascii_uppercase)
)Also applies to: 199-204
| ( | ||
| frappe.qb.update(twe) | ||
| .join(party) | ||
| .on(twe.party == party.name) | ||
| .set(twe.tax_id, party.pan) | ||
| .where(twe.party_type == party_type) | ||
| .where(twe.company.isin(companies)) | ||
| .where(party.pan.isnotnull()) | ||
| .where(party.pan != "") | ||
| .where(twe.created_by_migration == 0) | ||
| .run() |
There was a problem hiding this comment.
frappe.qb.update().join() generates an INNER JOIN, so any Tax Withholding Entry whose linked party record has since been deleted will be excluded from the update and its tax_id will remain stale. For most real-world deployments this is acceptable (there's no PAN to look up for a deleted party), but it's worth a comment to make the intent clear to future maintainers.
| ( | |
| frappe.qb.update(twe) | |
| .join(party) | |
| .on(twe.party == party.name) | |
| .set(twe.tax_id, party.pan) | |
| .where(twe.party_type == party_type) | |
| .where(twe.company.isin(companies)) | |
| .where(party.pan.isnotnull()) | |
| .where(party.pan != "") | |
| .where(twe.created_by_migration == 0) | |
| .run() | |
| ( | |
| frappe.qb.update(twe) | |
| .join(party) # INNER JOIN: entries for deleted parties are intentionally skipped | |
| .on(twe.party == party.name) |
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py (1)
199-219:⚠️ Potential issue | 🟡 MinorReplace
randomwithsecretsto pass Ruff S311 lint check.While not a security concern in test code, Ruff S311 flags
random.choices/random.choiceand will fail CI. The past review already noted this.✅ Suggested fix
-import random +import secrets import string ... def generate_unique_pan(): existing_pans = frappe.get_all( "Supplier", pluck="pan", filters={"pan": ("is", "set")} ) existing_pans += frappe.get_all( "Customer", pluck="pan", filters={"pan": ("is", "set")} ) existing_pans = set(existing_pans) for _ in range(100): - letters = "".join(random.choices(string.ascii_uppercase, k=5)) - digits = "".join(random.choices(string.digits, k=4)) - suffix = random.choice(string.ascii_uppercase) + letters = "".join(secrets.choice(string.ascii_uppercase) for _ in range(5)) + digits = "".join(secrets.choice(string.digits) for _ in range(4)) + suffix = secrets.choice(string.ascii_uppercase) pan = f"{letters}{digits}{suffix}" if pan not in existing_pans: return pan existing_pans.add(pan) raise RuntimeError("Unable to generate unique PAN")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaf0db48-9af3-46d8-8dbd-75f04ce80591
📒 Files selected for processing (1)
india_compliance/income_tax_india/overrides/test_tax_withholding_category.py
| def get_tax_id_for_party(party_type, party): | ||
| return frappe.db.get_value(party_type, party, "pan") |
There was a problem hiding this comment.
pan field
frappe.db.get_value(party_type, party, "pan") silently returns None if the pan field doesn't exist on the requested DocType (e.g. "Employee" or "Company"). ERPNext's TDS framework can apply withholding tax to party types beyond Supplier and Customer, so if this override is ever invoked for such a type, tax_id will be set to None rather than falling back to the original tax_id field value.
Consider returning the original field for party types where pan is not applicable:
def get_tax_id_for_party(party_type, party):
if party_type in ("Supplier", "Customer"):
return frappe.db.get_value(party_type, party, "pan")
return frappe.db.get_value(party_type, party, "tax_id")
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
…withholding tests
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 medium |
🟢 Metrics 32 complexity
Metric Results Complexity 32
TIP This summary will be updated as you push new changes. Give us feedback
1 participant
Override Tax ID with PAN and update existing entries.
depends on: frappe/erpnext#53598