Skip to content

Conversation

@aymenit2008
Copy link

@aymenit2008 aymenit2008 commented Jan 5, 2026

fix: add docstatus=1 filter to get_sales_invoice_item

  • Ensure only submitted sales invoice items are fetched
  • Prevent fetching cancelled/draft invoice items

Summary by CodeRabbit

  • Bug Fixes
    • Fixed Sales Invoice item lookup to only match submitted invoices, preventing unfinalized transactions from being incorrectly included in merge operations.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Jan 5, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Walkthrough

The change adds a filter requirement to the Sales Invoice item lookup function in the POS Invoice Merge Log module. The function get_sales_invoice_item now includes an additional condition that restricts matching to submitted invoices only by checking that SalesInvoice.docstatus == 1. The existing exception handling behavior remains unchanged, with the function continuing to return None when exceptions occur. No public APIs or exported entities were modified.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the main change: adding a docstatus condition to the get_sales_invoice_item function to filter for submitted invoices only.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bbcafe and dce6452.

📒 Files selected for processing (1)
  • erpnext/accounts/doctype/pos_invoice_merge_log/pos_invoice_merge_log.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.

Applied to files:

  • erpnext/accounts/doctype/pos_invoice_merge_log/pos_invoice_merge_log.py
🧬 Code graph analysis (1)
erpnext/accounts/doctype/pos_invoice_merge_log/pos_invoice_merge_log.py (2)
erpnext/accounts/doctype/sales_invoice/sales_invoice.py (1)
  • SalesInvoice (51-2011)
erpnext/selling/page/point_of_sale/pos_payment.js (1)
  • docstatus (599-599)
⏰ 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). (6)
  • GitHub Check: Patch Test
  • GitHub Check: Python Unit Tests (2)
  • GitHub Check: Python Unit Tests (1)
  • GitHub Check: Python Unit Tests (3)
  • GitHub Check: Python Unit Tests (4)
  • GitHub Check: Summary
🔇 Additional comments (1)
erpnext/accounts/doctype/pos_invoice_merge_log/pos_invoice_merge_log.py (1)

686-707: LGTM! Excellent defensive check for data integrity.

The addition of docstatus == 1 correctly ensures that only submitted Sales Invoices are considered when resolving the original Sales Invoice Item for returns. This prevents incorrect linkage to draft or cancelled invoices.

The logic is sound:

  • Draft invoices (docstatus=0) shouldn't be referenced as they're not finalized
  • Cancelled invoices (docstatus=2) shouldn't be referenced as they're invalidated
  • Only submitted invoices (docstatus=1) are valid for establishing return linkages

The sales_invoice_item field is properly designed to handle None values (typed as DF.Data | None), so when no matching submitted invoice is found, returning None is the correct behavior.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@stale
Copy link

stale bot commented Jan 24, 2026

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jan 24, 2026
@stale stale bot removed the inactive label Jan 28, 2026
@diptanilsaha diptanilsaha merged commit afc4c85 into frappe:version-15-hotfix Jan 28, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-tests This PR needs automated unit-tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants