fix(general-ledger): add CPA support via Party Link cross-party GL filtering#54657
Draft
shubhdoshi21 wants to merge 3 commits intofrappe:developfrom
Draft
fix(general-ledger): add CPA support via Party Link cross-party GL filtering#54657shubhdoshi21 wants to merge 3 commits intofrappe:developfrom
shubhdoshi21 wants to merge 3 commits intofrappe:developfrom
Conversation
shubhdoshi21
changed the title
Contributor
📝 WalkthroughWalkthroughThe general ledger report's Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
erpnext/accounts/report/general_ledger/general_ledger.py (1)
350-380: Tighten the Party Link lookup and dedupe before emitting SQL.The query currently pulls every
Party Linkrow whoseprimary_partyorsecondary_partytext matches, then filters the role in Python and emits duplicates intoIN (...). It would be cleaner to constrain the role in SQL and accumulate parties in sets, which keeps the generated predicate smaller and avoids fetching irrelevant rows when names overlap across doctypes.Refactor direction
def get_linked_parties(party_list: list, party_type: str) -> dict: - parties_by_type = {party_type: list(party_list)} + parties_by_type = {party_type: set(party_list)} party_link_dt = frappe.qb.DocType("Party Link") links = ( frappe.qb.from_(party_link_dt) .select( party_link_dt.primary_role, party_link_dt.primary_party, party_link_dt.secondary_role, party_link_dt.secondary_party, ) .where( - (party_link_dt.secondary_party.isin(party_list)) | (party_link_dt.primary_party.isin(party_list)) + ( + (party_link_dt.secondary_role == party_type) + & (party_link_dt.secondary_party.isin(party_list)) + ) + | ( + (party_link_dt.primary_role == party_type) + & (party_link_dt.primary_party.isin(party_list)) + ) ) .run(as_dict=True) ) for link in links: if link.secondary_party in party_list and link.secondary_role == party_type: - parties_by_type.setdefault(link.primary_role, []).append(link.primary_party) + parties_by_type.setdefault(link.primary_role, set()).add(link.primary_party) elif link.primary_party in party_list and link.primary_role == party_type: - parties_by_type.setdefault(link.secondary_role, []).append(link.secondary_party) + parties_by_type.setdefault(link.secondary_role, set()).add(link.secondary_party) - return parties_by_type + return {ptype: sorted(parties) for ptype, parties in parties_by_type.items()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/report/general_ledger/general_ledger.py` around lines 350 - 380, The Party Link lookup in get_linked_parties currently fetches rows by party name then filters roles in Python and may emit duplicate parties; update get_linked_parties to include role constraints in the SQL WHERE (i.e., only select rows where primary_role = party_type when secondary_party is in party_list, and vice versa) to avoid irrelevant rows, and accumulate results into sets (e.g., use a set for parties_by_type entries) to deduplicate before returning; update build_common_party_condition to consume those sets (convert to list only for SQL emission) and ensure you still escape each party and the party_type when building the IN (...) predicates.
🤖 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/accounts/report/general_ledger/general_ledger.py`:
- Around line 280-283: The change removed the legacy "party only" fallback and
now always uses build_common_party_condition when filters.get("party") exists;
restore correct behavior by updating the branch in general_ledger.py so that if
filters.get("party") and NOT filters.get("party_type") you append the legacy
condition "party=%(party)s" to conditions, else call
build_common_party_condition(filters); alternatively, if you prefer to require a
type, enforce this in validate_party() by raising when party is supplied without
party_type and update callers accordingly—refer to validate_party(),
build_common_party_condition, filters["party"], filters["party_type"], and the
conditions list to locate and apply the fix.
---
Nitpick comments:
In `@erpnext/accounts/report/general_ledger/general_ledger.py`:
- Around line 350-380: The Party Link lookup in get_linked_parties currently
fetches rows by party name then filters roles in Python and may emit duplicate
parties; update get_linked_parties to include role constraints in the SQL WHERE
(i.e., only select rows where primary_role = party_type when secondary_party is
in party_list, and vice versa) to avoid irrelevant rows, and accumulate results
into sets (e.g., use a set for parties_by_type entries) to deduplicate before
returning; update build_common_party_condition to consume those sets (convert to
list only for SQL emission) and ensure you still escape each party and the
party_type when building the IN (...) predicates.
🪄 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: f06e4093-8874-42dd-9743-36de3837632f
📒 Files selected for processing (1)
erpnext/accounts/report/general_ledger/general_ledger.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #54657 +/- ##
===========================================
+ Coverage 79.74% 79.78% +0.03%
===========================================
Files 1160 1160
Lines 126362 126521 +159
===========================================
+ Hits 100768 100940 +172
+ Misses 25594 25581 -13
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ERPNext supports linking a Customer and Supplier as the same real-world entity via Party Link (Common Party Accounting). When such a link exists, filtering the General Ledger by the Customer should also include that entity's Supplier-side entries, and vice versa.
Before this change, the party filter used a plain
party IN (...)SQL condition. Filtering by Customer returned only the Customer's GL entries — the linked Supplier's GL entries were not included. Users had to run two separate reports and reconcile them manually.After this change, filtering the General Ledger by either party automatically includes GL entries posted under both the Customer and the linked Supplier.
Also adds a guard in
validate_party— passingpartywithoutparty_typepreviously silently returned empty data; it now throws a clear validation error.Test coverage (3 cases):
test_cpa_includes_linked_party_entries— filters the GL by each side of a Party Link independently; asserts that GL entries posted under both the Customer and the linked Supplier are included in both runs.test_party_filter_without_party_link— filters the GL by a Customer with no Party Link; asserts only that customer's GL entries are returned and no other party's entries bleed in.test_cpa_mixed_party_list_partial_links— filters the GL by two customers at once where only one has a Party Link; asserts the linked supplier's GL entries are included while the unlinked customer's filter brings in only its own GL entries.Why this is not a configurable filter on the report:
The Party Link relationship is an accounting truth, not a display preference. Showing only one side of a linked entity produces an incomplete and potentially misleading balance. An opt-out toggle would let accountants draw wrong conclusions from partial data without realising it. The feature is already opt-in at the data level — if no Party Link is configured, the report behaves exactly as before.