fix: add duplicate GSTIN and PAN message checks in party and address#3821
fix: add duplicate GSTIN and PAN message checks in party and address#3821vorasmit merged 13 commits intoresilient-tech:developfrom
Conversation
📝 WalkthroughWalkthroughAdds client-side calls to check for duplicate GSTIN/PAN after local validation and implements a server-side API that searches Parties and Addresses (with permission checks), returning an HTML alert linking to any matching records. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser as Client (party.js / quick_entry.js)
participant Server as Backend (gst_india.utils)
participant DB as Database
User->>Browser: Enter or edit GSTIN/PAN
Browser->>Browser: local validate_gstin / validate_pan
Browser->>Server: frappe.call -> check_duplicate_gstin/check_duplicate_pan(args)
Server->>Server: validate party_type and permissions
Server->>DB: Query Parties (by PAN/GSTIN)
alt Address-linked duplicates
Server->>DB: Query Addresses -> linked Party(s)
end
DB-->>Server: return matches (if any)
alt duplicates found
Server->>Server: build HTML links (mark via-address if applicable)
Server-->>Browser: return alert HTML
Browser->>User: render orange alert with links
else no duplicates
Server-->>Browser: return no-duplicates
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🧹 Nitpick comments (1)
india_compliance/gst_india/client_scripts/party.js (1)
83-111: Add a small guard aroundfrappe.boot.gst_party_typesto avoid runtime errorsThe Address‑aware logic for resolving
party_type,party, andaddress_nameis well thought out and matches the server API. To be defensive, consider guarding theincludescall in casefrappe.boot.gst_party_typesis missing or not an array:- if (doc.doctype === "Address") { - if (!doc.links || doc.links.length !== 1) return; - - const link = doc.links[0]; - if (!frappe.boot.gst_party_types.includes(link.link_doctype)) return; + if (doc.doctype === "Address") { + if (!doc.links || doc.links.length !== 1) return; + + const link = doc.links[0]; + if (!Array.isArray(frappe.boot.gst_party_types) || + !frappe.boot.gst_party_types.includes(link.link_doctype)) { + return; + }This avoids a
Cannot read properties of undefinederror if boot data changes while keeping the current behavior otherwise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
india_compliance/gst_india/client_scripts/party.js(3 hunks)india_compliance/gst_india/utils/__init__.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-26T03:16:30.230Z
Learnt from: vorasmit
Repo: resilient-tech/india-compliance PR: 3344
File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0
Timestamp: 2025-05-26T03:16:30.230Z
Learning: In GST account-wise summary reports, the filtering logic `doc.company_gstin != IfNull(doc[counterparty_gstin_field], "")` is intentionally designed to exclude internal transfers (where company GSTIN equals party GSTIN) while including transactions with unregistered parties (empty GSTIN). Internal transfers are excluded as they are only for internal reporting purposes like stock transfers between branches.
Applied to files:
india_compliance/gst_india/client_scripts/party.js
📚 Learning: 2025-11-21T08:53:26.329Z
Learnt from: ljain112
Repo: resilient-tech/india-compliance PR: 3807
File: india_compliance/gst_india/overrides/transaction.py:142-157
Timestamp: 2025-11-21T08:53:26.329Z
Learning: In the india_compliance/gst_india/overrides/transaction.py file, the `_item_wise_tax_details` field used in `validate_item_wise_tax_detail()` is populated from ERPNext (not from within the India Compliance codebase) before validation runs, as part of cross-repository changes.
Applied to files:
india_compliance/gst_india/client_scripts/party.js
🧬 Code graph analysis (1)
india_compliance/gst_india/client_scripts/party.js (3)
india_compliance/gst_india/utils/__init__.py (2)
check_duplicate_gstin(1177-1180)check_duplicate_pan(1137-1140)india_compliance/gst_india/client_scripts/address.js (2)
doc(33-33)frappe(37-41)india_compliance/gst_india/overrides/party.py (1)
validate_pan(61-78)
🪛 GitHub Actions: Linters
india_compliance/gst_india/utils/__init__.py
[error] 1245-1245: Blocking Semgrep rule fired: frappe-translation-variable-only. Avoid translating variables with _("{}").format(msg). Use proper translation handling. Rule: frappe-semgrep-rules.rules.frappe-translation-variable-only.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (4)
india_compliance/gst_india/utils/__init__.py (1)
1136-1174: PAN duplicate check helper looks correct and side‑effect onlyPermission check, normalization (
upper().strip()), and the duplicate lookup/message construction all look good and match the requirement of warning (not blocking) on PAN reuse across the same party type.india_compliance/gst_india/client_scripts/party.js (3)
48-80: Wiring duplicate GSTIN check intovalidate_gstinlooks correctCalling
check_duplicate_gstin(frm.doc)right afterindia_compliance.validate_gstin(gstin)ensures the server sees a syntactically valid GSTIN, and because the server also normalizes (upper().strip()), the slight timing difference withfrm.doc.gstin = gstindoes not affect correctness.
114-128: PAN duplicate check hook is placed appropriatelyInvoking
check_duplicate_pan(frm.doc)immediately afterindia_compliance.validate_pan(pan)ensures only normalized, valid PAN values trigger the server‑side duplicate warning, which matches the intended behavior.
130-135: PAN duplicate check helper is a clean, minimal wrapperThe
check_duplicate_panhelper correctly forwardspan,party_type, andpartyto the new server method without adding extra client‑side branching, keeping responsibilities cleanly separated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
india_compliance/gst_india/utils/__init__.py (2)
1136-1172: PAN duplicate check logic looks sound; consider small polish for reuse/testingThe PAN normalization, exclusion of the current party, and HTML message construction all look correct and aligned with how
_check_duplicate_gstinworks. To make this a bit easier to reuse and test in isolation, you might consider (optionally) returning theexisting_parties/ rendered HTML list from_check_duplicate_panin addition to side‑effecting viamsgprint, so callers (or tests) can assert behavior without scraping UI. Not a blocker.
1174-1252: GSTIN duplicate detection is correct and fixes prior i18n issue; only minor nitsThe combined duplicate logic across Party and Address looks correct: you normalize GSTIN, exclude the current party (and optionally the current address), avoid generating a
NOT IN ()when there are no direct matches, and now build the “via Address …” text using a constant translation string with placeholders, which should satisfy the Semgrepfrappe-translation-variable-onlyrule and keep the fragment translatable. Two minor cleanups you might consider:
- Rename the loop variable in
for party in existing_parties:toparty_name(or similar) to avoid shadowing thepartyparameter.- Update the
_check_duplicate_gstindocstring to mention that it also checks Address-linked GSTINs, not just party records.These are purely cosmetic; the current implementation is functionally and i18n-wise in good shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
india_compliance/gst_india/utils/__init__.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3821 +/- ##
===========================================
- Coverage 69.51% 69.35% -0.16%
===========================================
Files 182 182
Lines 17965 18028 +63
===========================================
+ Hits 12489 12504 +15
- Misses 5476 5524 +48
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ljain112
changed the title
|
@ljain112 Also add this validation in |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
india_compliance/public/js/quick_entry.js (1)
500-523: Consider passingaddress_namefor Address doctype for consistency withparty.js.The
party.jsversion ofcheck_duplicate_gstinpassesaddress_namewhen the doctype is Address (see lines 100-101 in party.js). While for quick entry the address doesn't exist yet, passingnullexplicitly would maintain consistency with the server API signature and the other client implementation.🔎 Suggested fix
if (!party_type) return; if (!frappe.boot.gst_party_types.includes(party_type)) return; frappe.call({ method: "india_compliance.gst_india.utils.check_duplicate_gstin", args: { gstin: gstin, party_type: party_type, party: party_name, + address_name: null, }, }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
india_compliance/gst_india/client_scripts/party.jsindia_compliance/gst_india/utils/__init__.pyindia_compliance/public/js/quick_entry.js
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-21T08:53:26.329Z
Learnt from: ljain112
Repo: resilient-tech/india-compliance PR: 3807
File: india_compliance/gst_india/overrides/transaction.py:142-157
Timestamp: 2025-11-21T08:53:26.329Z
Learning: In the india_compliance/gst_india/overrides/transaction.py file, the `_item_wise_tax_details` field used in `validate_item_wise_tax_detail()` is populated from ERPNext (not from within the India Compliance codebase) before validation runs, as part of cross-repository changes.
Applied to files:
india_compliance/public/js/quick_entry.jsindia_compliance/gst_india/utils/__init__.pyindia_compliance/gst_india/client_scripts/party.js
📚 Learning: 2025-11-11T12:44:44.690Z
Learnt from: ljain112
Repo: resilient-tech/india-compliance PR: 3784
File: india_compliance/gst_india/client_scripts/company.js:25-28
Timestamp: 2025-11-11T12:44:44.690Z
Learning: In india_compliance, when using erpnext.company.set_custom_query(frm, [field_name, filters]) in Company doctype client scripts, the function automatically adds company and is_group filters. The filters object parameter should only contain additional filters beyond these defaults (e.g., root_type). For default_gst_expense_account, no root_type filter is needed, so an empty object {} is passed.
Applied to files:
india_compliance/public/js/quick_entry.js
📚 Learning: 2025-09-04T13:11:55.495Z
Learnt from: karm1000
Repo: resilient-tech/india-compliance PR: 3636
File: india_compliance/gst_india/utils/e_invoice.py:0-0
Timestamp: 2025-09-04T13:11:55.495Z
Learning: For e-invoice API error codes 3028 and 3029, the response will always contain a GSTIN that can be extracted from the error message using GSTIN_FORMAT regex.
Applied to files:
india_compliance/gst_india/utils/__init__.py
📚 Learning: 2025-10-01T10:54:11.096Z
Learnt from: karm1000
Repo: resilient-tech/india-compliance PR: 3709
File: india_compliance/gst_india/utils/exporter.py:174-174
Timestamp: 2025-10-01T10:54:11.096Z
Learning: In india_compliance/gst_india/doctype/gstr_1/gstr_1_export.py, FIELD_TRANSFORMATIONS dictionary contains lambdas that are only called internally by DataProcessor.apply_transformations() with a single argument. These are separate from header transforms used with ExcelExporter.insert_data() which accept two arguments (value, row).
Applied to files:
india_compliance/gst_india/utils/__init__.py
📚 Learning: 2025-05-26T03:16:30.230Z
Learnt from: vorasmit
Repo: resilient-tech/india-compliance PR: 3344
File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0
Timestamp: 2025-05-26T03:16:30.230Z
Learning: In GST account-wise summary reports, the filtering logic `doc.company_gstin != IfNull(doc[counterparty_gstin_field], "")` is intentionally designed to exclude internal transfers (where company GSTIN equals party GSTIN) while including transactions with unregistered parties (empty GSTIN). Internal transfers are excluded as they are only for internal reporting purposes like stock transfers between branches.
Applied to files:
india_compliance/gst_india/client_scripts/party.js
🧬 Code graph analysis (2)
india_compliance/public/js/quick_entry.js (1)
india_compliance/gst_india/utils/__init__.py (1)
check_duplicate_gstin(1193-1217)
india_compliance/gst_india/client_scripts/party.js (2)
india_compliance/gst_india/utils/__init__.py (2)
check_duplicate_gstin(1193-1217)check_duplicate_pan(1157-1174)india_compliance/gst_india/client_scripts/address.js (2)
doc(33-33)frappe(37-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (13)
india_compliance/public/js/quick_entry.js (2)
100-101: LGTM!The duplicate GSTIN check is correctly invoked before proceeding with autofill or category guessing. This ensures users are alerted about duplicates early in the form filling process.
174-187: LGTM!Contact fields layout adjustment looks correct. No functional changes to the field definitions.
india_compliance/gst_india/utils/__init__.py (7)
36-36: LGTM!Import of
GST_PARTY_TYPESis correctly added to support party type validation.
1137-1145: LGTM!The alert function correctly formats the message with duplicate links and uses an appropriate orange indicator to warn users without blocking their workflow.
1148-1153: LGTM!The validation correctly gates duplicate checks to only GST party types and enforces permission checks before proceeding.
1156-1174: LGTM!The
check_duplicate_panfunction is well-structured with proper input validation, normalization, and thepartyparameter is correctly optional to handle new documents.
1177-1189: LGTM!Simple and effective duplicate PAN query. Correctly excludes the current party when editing.
1192-1217: LGTM!The function correctly handles both direct party GSTIN matches and address-linked GSTIN matches, with proper translation handling for the "via Address" message.
1220-1275: LGTM!The UNION query efficiently combines direct party matches and address-linked matches in a single database call. The deduplication logic correctly prioritizes direct GSTIN matches over address-linked ones when a party appears in both result sets.
india_compliance/gst_india/client_scripts/party.js (4)
63-63: LGTM!Duplicate GSTIN check is correctly invoked after validation and before refreshing the field.
83-110: LGTM!The function correctly handles the Address doctype by using the linked party information and safely returns early when multiple parties are linked (avoiding ambiguity about which party to check against). The
__islocalcheck properly handles new documents by passingnullfor party/address_name.
121-121: LGTM!Duplicate PAN check is correctly placed after validation.
128-143: LGTM!The
check_duplicate_panfunction mirrors the pattern ofcheck_duplicate_gstinappropriately. PAN checks don't need Address-specific handling since PAN is typically a party-level field.
|
LGTM |
vorasmit
force-pushed
the
feat-duplicate-pan-gstin
branch
from
8fd8340 to
6266563
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@india_compliance/gst_india/utils/__init__.py`:
- Around line 1165-1174: The code building duplicate_links assumes each row from
_get_duplicate_pan_party() has keys "via_address" and "address", causing
KeyError; update the loop that iterates existing_parties to safely access
row.get("via_address") and row.get("address") (or normalize rows returned by
_get_duplicate_pan_party()) before calling get_link_to_form, e.g., check if
row.get("via_address") is truthy and only then call get_link_to_form("Address",
row.get("address")), otherwise fall back to using party_link; ensure
duplicate_links.append still uses the constructed link_msg.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@india_compliance/gst_india/utils/__init__.py`:
- Around line 1165-1173: existing_parties from _get_duplicate_gstin_party() can
be plain dicts, so using attribute access like row.name / row.via_address /
row.address will raise AttributeError; update the loop that builds link_msg
(referencing existing_parties, get_link_to_form, party_type) to either normalize
each row to a frappe._dict (e.g., row = frappe._dict(row)) before use or switch
to dict-style access (row["name"], row.get("via_address"), row.get("address"))
when building party_link and address_link so duplicate-GSTIN cases do not crash.
| for row in existing_parties: | ||
| party_link = get_link_to_form(party_type, row.name) | ||
| if row.via_address: | ||
| address_link = get_link_to_form("Address", row.address) | ||
| link_msg = _("{0} (via Address {1})").format(party_link, address_link) | ||
|
|
||
| else: | ||
| link_msg = party_link | ||
|
|
There was a problem hiding this comment.
Fix dict attribute access to avoid duplicate-GSTIN crash.
_get_duplicate_gstin_party() returns plain dicts, so row.name / row.via_address / row.address will raise AttributeError when duplicates exist. Use dict access (or normalize to frappe._dict) before formatting the message.
🛠️ Proposed fix
- for row in existing_parties:
- party_link = get_link_to_form(party_type, row.name)
- if row.via_address:
- address_link = get_link_to_form("Address", row.address)
+ for row in existing_parties:
+ party_name = row.get("name")
+ if not party_name:
+ continue
+ party_link = get_link_to_form(party_type, party_name)
+ if row.get("via_address"):
+ address_link = get_link_to_form("Address", row.get("address"))
link_msg = _("{0} (via Address {1})").format(party_link, address_link)
else:
link_msg = party_link🤖 Prompt for AI Agents
In `@india_compliance/gst_india/utils/__init__.py` around lines 1165 - 1173,
existing_parties from _get_duplicate_gstin_party() can be plain dicts, so using
attribute access like row.name / row.via_address / row.address will raise
AttributeError; update the loop that builds link_msg (referencing
existing_parties, get_link_to_form, party_type) to either normalize each row to
a frappe._dict (e.g., row = frappe._dict(row)) before use or switch to
dict-style access (row["name"], row.get("via_address"), row.get("address")) when
building party_link and address_link so duplicate-GSTIN cases do not crash.
…tfix/pr-3821 fix: add duplicate GSTIN and PAN message checks in party and address (backport #3821)
|
@Mergifyio backport version-16-hotfix |
✅ Backports have been createdDetails
Cherry-pick of 5c96a45 has failed: Cherry-pick of e2ee569 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
…tfix/pr-3821 fix: add duplicate GSTIN and PAN message checks in party and address (backport #3821)
3 participants
closes: #3781
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.