fix(sales invoice): toggle Get Items From button based on is_return and POS view#52594
fix(sales invoice): toggle Get Items From button based on is_return and POS view#52594Navin-S-R wants to merge 4 commits intofrappe:developfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #52594 +/- ##
===========================================
- Coverage 79.39% 79.38% -0.01%
===========================================
Files 1169 1169
Lines 123580 123601 +21
===========================================
+ Hits 98115 98123 +8
- Misses 25465 25478 +13
🚀 New features to boost your workflow:
|
|
@CodeRabbit review this |
|
✅ Actions performedReview triggered.
|
|
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 Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@erpnext/accounts/doctype/sales_invoice/sales_invoice.js`:
- Around line 257-272: The toggle_get_items() logic removes the "Delivery Note"
picker on reload when this.frm.doc.is_return is true; to fix, change the else
branch so that when this.frm.doc.is_return is truthy you still invoke
this.frm.cscript.delivery_note_btn() (while still removing Sales
Order/Quotation/Timesheet), otherwise remove all "Get Items From" buttons;
update toggle_get_items() to call delivery_note_btn() in the else-path for
return invoices so the DN picker is present on load.
🧹 Nitpick comments (2)
erpnext/controllers/queries.py (2)
13-13:Coalesceis imported but unused.The new query builder code in
get_delivery_notes_to_be_billeddoesn't useCoalesce. Remove it to keep imports clean.Proposed fix
-from frappe.query_builder.functions import Coalesce, Concat, Locate, Sum +from frappe.query_builder.functions import Concat, Locate, Sum
399-401: Minor:docstatusfilter is applied twice — once in the base query (line 385) and again via dynamic filters.Not a bug, but worth noting. The base query already filters
docstatus == 1, and the JS also passesdocstatus: 1in filters. Consider excluding already-handled keys from the dynamic loop to avoid redundancy, or leave as-is since it's harmless.
ravibharathi656
requested review from
mihir-kandoi,
rohitwaghchaure and
ruthra-kumar
as code owners
Navin-S-R
force-pushed
the
fix-sales-invoice-get-items
branch
from
462aeb8 to
9cc5d14
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.js (1)
258-258:cint()applied to the comparison result rather than the operand — non-standard usage.
cint(this.frm.doc.docstatus == 0)wraps the boolean outcome, not the value. It works (cint(true) === 1) but diverges from the conventional pattern everywhere else in the codebase.♻️ Idiomatic alternative
-if (cint(this.frm.doc.docstatus == 0) && this.frm.page.current_view_name !== "pos") { +if (this.frm.doc.docstatus === 0 && this.frm.page.current_view_name !== "pos") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/sales_invoice/sales_invoice.js` at line 258, The condition incorrectly applies cint to the boolean result (cint(this.frm.doc.docstatus == 0)); change it to apply cint to the operand instead (e.g., cint(this.frm.doc.docstatus) == 0) or drop cint and compare the numeric/docstatus value directly so the expression follows the project's idiomatic pattern in sales_invoice.js; update the condition used in the relevant handler where this.frm.doc.docstatus is checked and ensure the logical AND with this.frm.page.current_view_name !== "pos" is preserved.
🤖 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/doctype/sales_invoice/sales_invoice.js`:
- Around line 257-273: toggle_get_items is causing duplicate "Delivery Note"
buttons and leaving DN behind when skipping the add branch; to fix, at the start
of toggle_get_items() call this.frm.clear_custom_buttons() (or the form API to
clear the "Get Items From" group) so existing buttons are removed before adding
new ones, and in the else branch also remove the "Delivery Note" button by
calling this.frm.remove_custom_button("Delivery Note", "Get Items From"); update
the caller path (is_return -> toggle_get_items) only if it relies on external
clearing, but primarily ensure toggle_get_items itself clears buttons before
calling this.frm.cscript.delivery_note_btn().
In `@erpnext/controllers/queries.py`:
- Line 13: Remove the unused import Coalesce from the import line that brings in
Coalesce, Concat, Locate, Sum; locate the import statement referencing Coalesce
(used with Concat/Locate/Sum) and delete only Coalesce so the module imports
only Concat, Locate, Sum, ensuring no other references to Coalesce remain (e.g.,
in get_delivery_notes_to_be_billed) before committing.
- Around line 373-379: The subquery original_dn incorrectly filters out
originals with per_returned == 100, causing return DNs that reference
fully-returned originals to be excluded; update the where clause in the
original_dn QueryBuilder (the DeliveryNote subquery) to only check that the
original is submitted and not a return by removing the per_returned != 100
condition so it uses (DeliveryNote.docstatus == 1) & (DeliveryNote.is_return ==
0) only, leaving the main return_against.isin(original_dn) logic unchanged.
- Line 404: The return currently uses query.run(as_dict=True) which is
inconsistent with other whitelisted search functions (employee_query,
lead_query, get_project_name, get_account_list, get_filtered_child_rows) that
return tuples; change this call to return tuple form (use
query.run(as_dict=False) or query.run(as_list=True)) so the function matches the
file-wide convention of returning tuples instead of dicts/ frappe._dict,
updating any callers if needed to expect tuple semantics; locate the statement
by the return line containing query.run(as_dict=True) in
erpnext/controllers/queries.py and replace it accordingly.
---
Nitpick comments:
In `@erpnext/accounts/doctype/sales_invoice/sales_invoice.js`:
- Line 258: The condition incorrectly applies cint to the boolean result
(cint(this.frm.doc.docstatus == 0)); change it to apply cint to the operand
instead (e.g., cint(this.frm.doc.docstatus) == 0) or drop cint and compare the
numeric/docstatus value directly so the expression follows the project's
idiomatic pattern in sales_invoice.js; update the condition used in the relevant
handler where this.frm.doc.docstatus is checked and ensure the logical AND with
this.frm.page.current_view_name !== "pos" is preserved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/controllers/queries.py
Navin-S-R
force-pushed
the
fix-sales-invoice-get-items
branch
from
9cc5d14 to
1bf1bc5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
erpnext/controllers/queries.py (3)
418-418:query.run(as_dict=True)is inconsistent with every other search function in this file.All other whitelisted search functions return tuples;
get_filtered_child_rows(line 998, immediately below) explicitly usesquery.run(as_dict=False). Callers of this function (and the Frappe link-field picker framework) expect the standard tuple format.♻️ Proposed fix
- return query.run(as_dict=True) + return query.run(as_dict=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/controllers/queries.py` at line 418, The function currently returns query.run(as_dict=True) which is inconsistent with other whitelisted search helpers and breaks callers that expect tuple rows; change the call in this function to query.run(as_dict=False) so it returns tuples (matching get_filtered_child_rows and the Frappe link-field picker framework) and update any inline comment if present to reflect the tuple return shape.
387-393:per_returned != 100inoriginal_dnbreaks the fix for full-return scenarios.When an original DN has been fully returned (
per_returned = 100), it is excluded fromoriginal_dn. The return DN that references it then fails thereturn_against.isin(original_dn)check and is never surfaced — exactly the scenario this PR intends to fix. The subquery only needs to verify a valid, non-return, submitted original exists; itsper_returnedstate is irrelevant.🐛 Proposed fix
original_dn = ( frappe.qb.from_(DeliveryNote) .select(DeliveryNote.name) .where( - (DeliveryNote.docstatus == 1) & (DeliveryNote.is_return == 0) & (DeliveryNote.per_returned != 100) + (DeliveryNote.docstatus == 1) & (DeliveryNote.is_return == 0) ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/controllers/queries.py` around lines 387 - 393, The subquery original_dn (built from DeliveryNote) should not filter out records with DeliveryNote.per_returned == 100 because fully-returned originals must still be considered when checking return DN linkage; remove the per_returned != 100 predicate from the where clause in the original_dn query so original_dn only checks (DeliveryNote.docstatus == 1) & (DeliveryNote.is_return == 0), ensuring the return_against.isin(original_dn) test can match returns that reference fully-returned originals.
13-13:Coalesceis imported but never used.No call to
Coalesceexists anywhere in the updatedget_delivery_notes_to_be_billedor elsewhere in the file.🗑️ Proposed fix
-from frappe.query_builder.functions import Coalesce, Concat, Locate, Sum +from frappe.query_builder.functions import Concat, Locate, Sum🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/controllers/queries.py` at line 13, Remove the unused Coalesce import from the import statement in erpnext/controllers/queries.py: update the from frappe.query_builder.functions import ... line to drop Coalesce (it is not referenced in get_delivery_notes_to_be_billed or elsewhere), leaving only the actually used symbols such as Concat, Locate, Sum to avoid dead imports.
🧹 Nitpick comments (1)
erpnext/controllers/queries.py (1)
380-382:filterstype annotation is missing| Noneand a default value.Every other comparable search function in the file (
employee_query,lead_query,item_query,bom,get_project_name) declaresfilters: dict | None = None. The function body already handlesNonewithif filters and isinstance(filters, dict):, so the signature should reflect that.♻️ Proposed fix
def get_delivery_notes_to_be_billed( - doctype: str, txt: str, searchfield: str, start: int, page_len: int, filters: dict + doctype: str, txt: str, searchfield: str, start: int, page_len: int, filters: dict | None = None ) -> list:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/controllers/queries.py` around lines 380 - 382, Update the function signature of get_delivery_notes_to_be_billed to accept an optional filters argument by changing the parameter from filters: dict to filters: dict | None = None (matching other queries like employee_query/lead_query/item_query), since the body already checks for None with if filters and isinstance(filters, dict):; no other logic changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@erpnext/controllers/queries.py`:
- Line 418: The function currently returns query.run(as_dict=True) which is
inconsistent with other whitelisted search helpers and breaks callers that
expect tuple rows; change the call in this function to query.run(as_dict=False)
so it returns tuples (matching get_filtered_child_rows and the Frappe link-field
picker framework) and update any inline comment if present to reflect the tuple
return shape.
- Around line 387-393: The subquery original_dn (built from DeliveryNote) should
not filter out records with DeliveryNote.per_returned == 100 because
fully-returned originals must still be considered when checking return DN
linkage; remove the per_returned != 100 predicate from the where clause in the
original_dn query so original_dn only checks (DeliveryNote.docstatus == 1) &
(DeliveryNote.is_return == 0), ensuring the return_against.isin(original_dn)
test can match returns that reference fully-returned originals.
- Line 13: Remove the unused Coalesce import from the import statement in
erpnext/controllers/queries.py: update the from frappe.query_builder.functions
import ... line to drop Coalesce (it is not referenced in
get_delivery_notes_to_be_billed or elsewhere), leaving only the actually used
symbols such as Concat, Locate, Sum to avoid dead imports.
---
Nitpick comments:
In `@erpnext/controllers/queries.py`:
- Around line 380-382: Update the function signature of
get_delivery_notes_to_be_billed to accept an optional filters argument by
changing the parameter from filters: dict to filters: dict | None = None
(matching other queries like employee_query/lead_query/item_query), since the
body already checks for None with if filters and isinstance(filters, dict):; no
other logic changes required.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/controllers/queries.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/doctype/sales_invoice/sales_invoice.js
Navin-S-R
force-pushed
the
fix-sales-invoice-get-items
branch
from
1bf1bc5 to
b9a54ca
Compare
Navin-S-R
force-pushed
the
fix-sales-invoice-get-items
branch
from
b9a54ca to
0697b24
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
erpnext/accounts/doctype/sales_invoice/sales_invoice.js (1)
257-273: Duplicate "Delivery Note" button whenis_returnis toggled at runtime.
delivery_note_btn()on line 267 adds a DN button without removing the previous one. Each call totoggle_get_items()(from bothrefresh()andis_return()) adds another DN button on top of existing ones.🐛 Proposed fix — remove before re-adding
} + this.frm.remove_custom_button("Delivery Note", "Get Items From"); this.frm.cscript.delivery_note_btn();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/sales_invoice/sales_invoice.js` around lines 257 - 273, The toggle_get_items logic causes duplicate "Delivery Note" buttons because delivery_note_btn() adds a button unconditionally on each call; update toggle_get_items (or delivery_note_btn) to remove any existing "Delivery Note" button before re-adding by calling this.frm.remove_custom_button("Delivery Note", "Get Items From") prior to invoking this.frm.cscript.delivery_note_btn(), ensuring the button is removed in both the is_return==false path and the else path so toggling is_return or refresh doesn't create duplicates; reference functions: toggle_get_items, delivery_note_btn, remove_custom_button, is_return.
🤖 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/doctype/sales_invoice/sales_invoice.js`:
- Around line 257-266: toggle_get_items currently removes the "Timesheet" custom
button when is_return is true but only re-adds Sales Order and Quotation when
is_return becomes false, so restore the Timesheet button in the
!this.frm.doc.is_return branch by calling the same helper that adds it (e.g.
this.frm.cscript.timesheet_btn()) or, if a dedicated helper doesn't exist,
invoke the routine that registers all three "Get Items From" buttons (ensure you
reference toggle_get_items, this.frm.cscript.sales_order_btn,
this.frm.cscript.quotation_btn and the remove_custom_button call) so the
Timesheet button is re-added immediately (alternatively trigger a short form
refresh via this.frm.refresh() if adding a button helper is not available).
In `@erpnext/controllers/queries.py`:
- Around line 380-382: The function get_delivery_notes_to_be_billed currently
declares as_dict without a default, causing a TypeError when called via
`@frappe.validate_and_sanitize_search_inputs` (which supplies only the six
standard args); update the function signature
get_delivery_notes_to_be_billed(..., as_dict: bool = False) to give as_dict a
default (False) so calls from the Delivery Note picker succeed, and verify any
internal uses of as_dict still behave correctly with the False default.
---
Duplicate comments:
In `@erpnext/accounts/doctype/sales_invoice/sales_invoice.js`:
- Around line 257-273: The toggle_get_items logic causes duplicate "Delivery
Note" buttons because delivery_note_btn() adds a button unconditionally on each
call; update toggle_get_items (or delivery_note_btn) to remove any existing
"Delivery Note" button before re-adding by calling
this.frm.remove_custom_button("Delivery Note", "Get Items From") prior to
invoking this.frm.cscript.delivery_note_btn(), ensuring the button is removed in
both the is_return==false path and the else path so toggling is_return or
refresh doesn't create duplicates; reference functions: toggle_get_items,
delivery_note_btn, remove_custom_button, is_return.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/accounts/doctype/sales_invoice/sales_invoice.jserpnext/controllers/queries.py
Navin-S-R
force-pushed
the
fix-sales-invoice-get-items
branch
2 times, most recently
from
0aee651 to
9bd89f7
Compare
2 participants
Issue: Return Delivery Notes are not listed on "Get Items From" (Delivery Note) when creating a Sales Invoice with is_return enabled
Ref: 59697
Steps to Reproduce
update stockunchecked.Is Returnchecked.Before:
Screen.Recording.2026-02-09.at.18.29.33.mov
After:
Screen.Recording.2026-02-09.at.18.33.32.mov
Backport Needed v15