Fix due date orders#24
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds payment schedule metadata and propagation of computed due/discount dates, makes barcode scanning warehouse-aware with virtual "last_scanned_warehouse" fields and UI, enables selective child-item mapping via JSON args.filtered_children, migrates workstation operating components to a table model with migration, refactors many raw SQL queries to Query Builder, and adjusts dunning/payment flows and related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as transaction.js
participant Controller as AccountsController
participant DB
User->>UI: change payment term / edit due or discount date
UI->>UI: clear dependent fields (due_date_based_on, credit_days/months, discount_validity_*)
UI->>Controller: fetch payment term details (term, posting_date/bill_date)
Controller->>DB: load term/template and schedules
Controller->>Controller: compute due_date via get_due_date(...), discount_date via get_discount_date(...)
Controller-->>UI: return term_details including computed dates and metadata
UI->>UI: update payment_schedule rows and refresh UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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)
erpnext/stock/doctype/delivery_note/test_delivery_note.py (1)
1318-1319: Prefer a single decorator style throughout the fileEverywhere else the test suite uses the short alias
@change_settings, but here the fully-qualified@IntegrationTestCase.change_settingsform is introduced.
Both are equivalent, yet mixing styles makes the codebase a little noisier.-@IntegrationTestCase.change_settings("Accounts Settings", {"automatically_fetch_payment_terms": 1}) +@change_settings("Accounts Settings", {"automatically_fetch_payment_terms": 1})Sticking to the already-imported alias keeps the file uniform and a tad easier to scan.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
erpnext/accounts/doctype/payment_schedule/payment_schedule.json(3 hunks)erpnext/accounts/doctype/payment_schedule/payment_schedule.py(1 hunks)erpnext/accounts/doctype/payment_terms_template_detail/payment_terms_template_detail.json(2 hunks)erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py(1 hunks)erpnext/buying/doctype/purchase_order/test_purchase_order.py(3 hunks)erpnext/controllers/accounts_controller.py(4 hunks)erpnext/public/js/controllers/transaction.js(3 hunks)erpnext/selling/doctype/sales_order/test_sales_order.py(3 hunks)erpnext/stock/doctype/delivery_note/test_delivery_note.py(1 hunks)erpnext/stock/doctype/purchase_receipt/test_purchase_receipt.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
erpnext/controllers/accounts_controller.py (1)
erpnext/accounts/party.py (1)
get_due_date(625-646)
⏰ 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). (1)
- GitHub Check: Patch Test
🔇 Additional comments (22)
erpnext/accounts/doctype/payment_terms_template_detail/payment_terms_template_detail.json (2)
138-144: Discount validity no longer mandatory when a discount exists – deliberate or oversight?By dropping the former
mandatory_depends_onconstraint, the system now allows a discount to be defined without any validity period.
If invoices still rely ondiscount_validityfor early-payment logic, this can silently introduce perpetual discounts.Server-side validation (e.g., in
validate()) that raises ifdiscount> 0 anddiscount_validityis empty would keep data quality intact.
160-160: Check framework compatibility forrow_format: "Dynamic"The
row_formatflag is only recognised from Frappe v15 onward. Sites on v13/v14 will ignore it or, worse, error when loading the grid.Verify the minimum supported Frappe version in this project and, if needed, wrap this change in a version-guarded patch/migration.
erpnext/stock/doctype/purchase_receipt/test_purchase_receipt.py (1)
1204-1204: Excellent refactoring to use settings decorator!This change improves test isolation by using the
@IntegrationTestCase.change_settingsdecorator instead of manual setup/teardown of theautomatically_fetch_payment_termssetting. The decorator ensures proper cleanup and makes the test intent clearer, aligning well with the PR's payment terms automation objectives.erpnext/accounts/doctype/purchase_invoice/test_purchase_invoice.py (1)
2150-2150: LGTM! Excellent refactoring to use settings decorator.This change improves test organization by using a declarative decorator approach instead of manual setting enable/disable calls. This ensures proper cleanup and is consistent with the broader standardization effort across test modules.
erpnext/accounts/doctype/payment_schedule/payment_schedule.py (4)
20-21: LGTM! Clean addition of credit duration fields.The
credit_daysandcredit_monthsfields are properly typed and have descriptive names that clearly indicate their purpose for credit term calculations.
26-26: LGTM! Appropriate addition of discount validity duration field.The
discount_validityfield is properly typed as an integer and complements the discount validity calculation functionality.
27-32: LGTM! Well-designed literal field for discount validity calculation methods.The
discount_validity_based_onfield provides comprehensive and clearly named options for different discount validity calculation approaches. The literal values are descriptive and cover the expected use cases.
35-40: LGTM! Critical field addition that addresses the main PR objective.The
due_date_based_onfield directly addresses the PR's core issue by providing flexible options for due date calculation methods. This enables proper recalculation of due dates based on invoice date rather than incorrectly copying from PO/SO schedules, which was the main problem described in the PR objectives.erpnext/selling/doctype/sales_order/test_sales_order.py (3)
14-14: LGTM: Import addition supports new due date calculation logic.The addition of
get_due_dateimport is necessary for the enhanced payment schedule comparison and follows proper import conventions.
1681-1681: LGTM: Cleaner test configuration using decorator pattern.The use of
@IntegrationTestCase.change_settingsdecorator is a better approach than manual setting manipulation, ensuring proper cleanup and consistency across test files.
2571-2576: LGTM: Implements dynamic due date calculation as per PR objectives.The enhanced logic correctly:
- Determines posting_date from available date fields (bill_date, posting_date, transaction_date)
- Conditionally recalculates due_date using
get_due_datewhendue_date_based_onis set- Uses computed due_date for comparison instead of stored values
This aligns perfectly with the PR goal to fix due date calculations by recalculating based on payment terms rather than copying stale dates from orders.
erpnext/public/js/controllers/transaction.js (3)
1106-1112: LGTM! Well-implemented discount date handler.The new
discount_datemethod correctly clears related discount validity fields when the discount date is manually changed, maintaining consistency with the payment terms auto-calculation logic.
1114-1122: LGTM! Proper handling of due date changes in child tables.The enhanced
due_datemethod correctly identifies child table changes and clears the related auto-calculated fields (due_date_based_on,credit_days,credit_months) when the due date is manually modified in payment schedule rows.
2648-2678: LGTM! Efficient payment term handling with proper field management.The enhanced
payment_termmethod correctly:
- Clears all relevant date condition fields before fetching new payment term details
- Uses efficient direct property assignment instead of multiple
frappe.model.set_valuecalls- Properly refreshes the payment schedule field to update the UI
The direct assignment approach is appropriate here since these are read-only fields populated from the backend.
erpnext/accounts/doctype/payment_schedule/payment_schedule.json (2)
13-18: LGTM! Well-structured due date calculation fields.The new due date related fields are properly configured:
due_date_based_onprovides clear options for date calculation methodscredit_daysandcredit_monthshave appropriate conditional visibility based on the selected calculation method- All fields are correctly marked as read-only and fetch from the linked Payment Term
- Field validation with
non_negativeconstraint is appropriateAlso applies to: 181-207
22-25: LGTM! Consistent discount validity field implementation.The new discount validity fields follow the same well-structured pattern as the due date fields:
discount_validity_based_onproperly depends on the presence of a discountdiscount_validityhas appropriate conditional visibility- Both fields correctly fetch from the linked Payment Term and are read-only
- Field ordering logically groups discount-related fields together
Also applies to: 208-226
erpnext/buying/doctype/purchase_order/test_purchase_order.py (3)
543-543: LGTM: Decorator correctly enables automatic payment terms fetching.The decorator appropriately sets up the test environment for validating payment terms copying from Purchase Order to Purchase Invoice.
715-715: LGTM: Decorator correctly disables automatic payment terms fetching.The decorator appropriately configures the test to validate the negative scenario where payment terms should not be automatically copied when the setting is disabled.
909-909: LGTM: Decorator correctly enables automatic payment terms fetching.The decorator properly sets up the test environment to validate that payment terms are automatically fetched when creating purchase invoices with purchase order references.
erpnext/controllers/accounts_controller.py (3)
2597-2597: LGTM!The posting date extraction logic correctly prioritizes
bill_dateoverposting_dateovertransaction_date, which aligns with the PR objective of using the current document's date for due date calculations.
2486-2490: LGTM!The new payment term fields are correctly populated into the
term_detailsdictionary, following the same pattern as other field assignments in this function. This provides the necessary metadata for due date and discount date calculations.
3505-3506: LGTM!The function signature extension correctly adds an optional
default_dateparameter while maintaining backward compatibility. This allows for more flexible due date calculations when a specific starting date is needed.
…ports feat: show party name in reports
…y-expiry-date-report fix: batch expiry date, serial no warranty and AMC expiry date in traceability report
…adjustments-in-report refactor: consider asset value adjustments in report
fix: sync translations from crowdin
…e-property fix(pick list): update warehouse property on refresh
fix: improve queries with query builder and input sanitization
…r-fieldname fix: fetch fieldname in accounting dimension filter
fix: handle mode of payment filter
fix(quotation): update currency on duplicate
…cate fix: prevent duplicate reverse Journal Entry
…-operating-component refactor: workstation operating component
fix(stock): don't override t_warehouse if no rules found
…t-company fix: company issue in setup wizard
style: format controllers with prettier
There was a problem hiding this comment.
Actionable comments posted: 32
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
erpnext/stock/doctype/inventory_dimension/test_inventory_dimension.py (1)
713-716: Typo and mismatched key: “expene_account” should be “expense_account” (breaks downstream usage).
prepare_data_for_internal_transfer()returnsexpene_account, but call sites expectdata.expense_account. With frappe._dict, the missing attr silently becomes None, which can mask issues and change accounting side-effects in tests.Fix the variable and returned key:
- expene_account = frappe.db.get_value( + expense_account = frappe.db.get_value( "Company", company, "stock_adjustment_account" ) or frappe.db.get_value("Account", {"company": company, "account_type": "Expense Account"}, "name") @@ return frappe._dict( { "from_warehouse": warehouse, "to_warehouse": to_warehouse, "customer": customer, "supplier": supplier, "company": company, "cost_center": cost_center, - "expene_account": expene_account, + "expense_account": expense_account, "store_warehouse": frappe.db.get_value( "Warehouse", {"name": ("like", "Store%"), "company": company}, "name" ), } )Also applies to: 717-729
erpnext/accounts/doctype/accounting_dimension_filter/accounting_dimension_filter.py (1)
91-99: Add fallback for missingfieldnameinget_dimension_filter_mapPre-existing filters may have a
NULLfieldname, causing entries to be keyed by(None, account)and disabling those filters. Derive a safe key from the dimension label whenf.fieldnameis falsy:• File: erpnext/accounts/doctype/accounting_dimension_filter/accounting_dimension_filter.py
Method: get_dimension_filter_map (around line 91)- for f in filters: - build_map( - dimension_filter_map, - f.fieldname, - f.applicable_on_account, - f.dimension_value, - f.allow_or_restrict, - f.is_mandatory, - ) + for f in filters: + # Fallback to scrub(dimension) if fieldname wasn’t backfilled + dimension_key = f.fieldname or frappe.scrub(f.accounting_dimension) + build_map( + dimension_filter_map, + dimension_key, + f.applicable_on_account, + f.dimension_value, + f.allow_or_restrict, + f.is_mandatory, + )erpnext/stock/doctype/pick_list/pick_list.py (1)
1296-1319: Bug: kwargs reassignment breaks select_item filteringcreate_dn_from_so defines select_item based on kwargs.filtered_children, then reassigns kwargs to a new dict. Because Python closures capture by reference, select_item sees the overwritten dict and filtered_children is lost, so no filtering occurs.
Apply this diff to preserve original kwargs for filtering and pass an augmented copy to the SO→DN mapper:
-def create_dn_from_so(pick_list, sales_order_list, delivery_note=None, kwargs=None): +def create_dn_from_so(pick_list, sales_order_list, delivery_note=None, kwargs=None): if not sales_order_list: return delivery_note - def select_item(d): - filtered_items = kwargs.get("filtered_children", []) + kwargs = kwargs or {} + + def select_item(d): + filtered_items = kwargs.get("filtered_children", []) child_filter = d.name in filtered_items if filtered_items else True return child_filter item_table_mapper = { "doctype": "Delivery Note Item", "field_map": { "rate": "rate", "name": "so_detail", "parent": "against_sales_order", }, "condition": lambda doc: abs(doc.delivered_qty) < abs(doc.qty) and doc.delivered_by_supplier != 1 and select_item(doc), } - kwargs = {"skip_item_mapping": True, "ignore_pricing_rule": pick_list.ignore_pricing_rule} + delivery_note_kwargs = { + **kwargs, + "skip_item_mapping": True, + "ignore_pricing_rule": pick_list.ignore_pricing_rule, + } delivery_note = create_delivery_note_from_sales_order( - next(iter(sales_order_list)), delivery_note, kwargs=kwargs + next(iter(sales_order_list)), delivery_note, kwargs=delivery_note_kwargs )erpnext/public/js/controllers/taxes_and_totals.js (2)
212-215: Fix incorrect use ofthisinside $.each callback (breaks validation calls).Inside initialize_taxes(),
thisrefers to the currenttaxrow (jQuery’s $.each), not the TaxesAndTotals instance. This causesthis.discount_amount_appliedto be undefined and passesundefinedinstead offrmto validate_inclusive_tax. Use the capturedme.Apply this diff:
- if (!this.discount_amount_applied) { - erpnext.accounts.taxes.validate_taxes_and_charges(tax.doctype, tax.name); - erpnext.accounts.taxes.validate_inclusive_tax(tax, this.frm); - } + if (!me.discount_amount_applied) { + erpnext.accounts.taxes.validate_taxes_and_charges(tax.doctype, tax.name); + erpnext.accounts.taxes.validate_inclusive_tax(tax, me.frm); + }
997-1009: Sametotal_amount_to_payre-declaration issue here.Mirror the fix in set_total_amount_to_default_mop().
Apply this diff:
if (this.frm.doc.party_account_currency == this.frm.doc.currency) { - var total_amount_to_pay = flt( + let total_amount_to_pay = flt( grand_total - this.frm.doc.total_advance - this.frm.doc.write_off_amount, precision("grand_total") ); } else { - var total_amount_to_pay = flt( + let total_amount_to_pay = flt( flt(base_grand_total, precision("base_grand_total")) - this.frm.doc.total_advance - this.frm.doc.base_write_off_amount, precision("base_grand_total") ); }erpnext/accounts/doctype/pos_invoice/pos_invoice.py (1)
921-955: Critical: Using child row docstatus breaks reserved-qty computation. Filter by parent docstatus instead.Child tables typically aren’t submitted independently; their docstatus is 0. This condition will zero-out the SUM and misreport availability. Filter POS Invoice docstatus instead.
Apply:
reserved_qty = ( frappe.qb.from_(p_inv) .from_(p_item) .select(Sum(p_item[qty_column]).as_("stock_qty")) .where( (p_inv.name == p_item.parent) & (IfNull(p_inv.consolidated_invoice, "") == "") - & (p_item.docstatus == 1) + & (p_inv.docstatus == 1) & (p_item.item_code == item_code) & (p_item.warehouse == warehouse) ) ).run(as_dict=True)
🧹 Nitpick comments (80)
erpnext/selling/doctype/product_bundle/product_bundle.py (1)
92-100: Harden the qty check (type safety) and add a clearer error title; consider a more explicit method nameTwo minor improvements:
- Use flt() to guard against None/strings and avoid type issues when comparing to 0.
- Provide a title for the thrown error for better UX and consistency with other validations.
Optional naming nit: “non_zero” suggests only != 0, but you’re enforcing strictly positive. Consider renaming to validate_child_items_qty_positive.
Apply this diff within this block to harden the check and improve the error:
def validate_child_items_qty_non_zero(self): for item in self.items: - if item.qty <= 0: - frappe.throw( - _( - "Row #{0}: Quantity cannot be a non-positive number. Please increase the quantity or remove the Item {1}" - ).format(item.idx, frappe.bold(item.item_code)) - ) + if flt(item.qty) <= 0: + frappe.throw( + _( + "Row #{0}: Quantity must be greater than 0. Increase the quantity or remove {1}." + ).format(item.idx, frappe.bold(item.item_code)), + title=_("Invalid Quantity"), + )Additionally, update the import to support flt (outside this hunk):
from frappe.utils import get_link_to_form, fltOptional rename for clarity (apply both diffs if you choose to rename):
- self.validate_child_items_qty_non_zero() + self.validate_child_items_qty_positive()-def validate_child_items_qty_non_zero(self): +def validate_child_items_qty_positive(self):Also consider adding unit tests covering:
- qty == 0 → throws
- qty < 0 → throws
- qty as string/numeric string → correctly validated
- qty > 0 (including fractional) → passes
erpnext/stock/doctype/inventory_dimension/test_inventory_dimension.py (1)
231-233: No-op call to load_from_db; either call it or remove the line.
doc.load_from_dbwithout parentheses is a function reference, not an invocation. If reload is desired, call it; otherwise, drop the line.Apply one of the following:
- doc.load_from_db + doc.load_from_db()or
- doc.load_from_dberpnext/accounts/report/item_wise_sales_register/item_wise_sales_register.py (1)
358-366: Switching to an IN-subquery for MOP filtering fixes row-duplication; simplify with DISTINCT (optional).Good call moving MOP filtering into a subquery to avoid join-induced row multiplication. You can simplify the subquery by using DISTINCT instead of GROUP BY on parent for clarity and (typically) equivalent plans.
Apply this diff:
- subquery = ( - frappe.qb.from_(sip) - .select(sip.parent) - .where(sip.mode_of_payment == filters.get("mode_of_payment")) - .groupby(sip.parent) - ) + subquery = ( + frappe.qb.from_(sip) + .select(sip.parent) + .distinct() + .where(sip.mode_of_payment == filters.get("mode_of_payment")) + ) query = query.where(si.name.isin(subquery))If you notice performance issues on large datasets, consider EXISTS as an alternative predicate (only if supported/ergonomic with our QB), and ensure there’s an index aiding the predicate on Sales Invoice Payment (at least on parent; mode_of_payment is a nice-to-have).
erpnext/patches/v16_0/set_invoice_type_in_pos_settings.py (1)
4-6: Guard against missing DocType to make the patch more defensiveOn atypical sites or partial app installs, the DocType might be absent; this guard makes the patch no-op instead of erroring.
Apply this diff:
def execute(): - if not frappe.db.get_single_value("POS Settings", "invoice_type"): - frappe.db.set_single_value("POS Settings", "invoice_type", "POS Invoice") + # Skip if the DocType is not available on this site + if not frappe.db.exists("DocType", "POS Settings"): + return + + current = frappe.db.get_single_value("POS Settings", "invoice_type") + if not current: + frappe.db.set_single_value("POS Settings", "invoice_type", "POS Invoice")erpnext/accounts/report/gross_profit/gross_profit.py (1)
255-258: Drop the unused Supplier master-name lookupThis report is customer-only. Checking Buying Settings’ supp_master_name here is misleading and only used to over-constrain the condition. Keep it focused on Selling Settings.
- supplier_master_name = frappe.db.get_single_value("Buying Settings", "supp_master_name") - customer_master_name = frappe.db.get_single_value("Selling Settings", "cust_master_name") + customer_master_name = frappe.db.get_single_value("Selling Settings", "cust_master_name")erpnext/accounts/report/general_ledger/general_ledger.py (1)
350-362: Optimize get_party_name_map to fetch only required partiesFetching all Customers, Suppliers, and Employees can be heavy on large datasets. Consider limiting to parties present in gl_entries to reduce memory and query time.
Sketch of a targeted approach (requires minor wiring in get_gl_entries):
# Inside get_gl_entries, after fetching gl_entries: from collections import defaultdict parties_by_type = defaultdict(set) for e in gl_entries: if e.party_type and e.party: parties_by_type[e.party_type].add(e.party) party_name_map = get_party_name_map(parties_by_type) # Adjust signature to accept parties_by_type: def get_party_name_map(parties_by_type): party_map = {} if parties_by_type.get("Customer"): customers = frappe.get_all( "Customer", fields=["name", "customer_name"], filters={"name": ["in", list(parties_by_type["Customer"])]}, ) party_map["Customer"] = {c.name: c.customer_name for c in customers} if parties_by_type.get("Supplier"): suppliers = frappe.get_all( "Supplier", fields=["name", "supplier_name"], filters={"name": ["in", list(parties_by_type["Supplier"])]}, ) party_map["Supplier"] = {s.name: s.supplier_name for s in suppliers} if parties_by_type.get("Employee"): employees = frappe.get_all( "Employee", fields=["name", "employee_name"], filters={"name": ["in", list(parties_by_type["Employee"])]}, ) party_map["Employee"] = {e.name: e.employee_name for e in employees} return party_mapThis avoids full-table scans and scales better.
erpnext/stock/doctype/putaway_rule/putaway_rule.py (1)
134-138: Simplify the ternary for readabilityThe same logic can be expressed more readably with a single or-chain. Also, a short comment helps future readers understand the precedence.
Apply this diff:
- warehouse = ( - (source_warehouse or item.get("warehouse")) - if not item.get("t_warehouse") - else item.get("t_warehouse") - ) + # Prefer scanned/target warehouse if present; otherwise fall back to source or legacy item.warehouse + warehouse = item.get("t_warehouse") or source_warehouse or item.get("warehouse")erpnext/assets/doctype/asset/asset.py (1)
1111-1114: Avoid unused variable in get_depreciation_accounts destructuringfixed_asset_account is captured but never used in this function. Prefer discarding it to avoid noise and potential linter warnings.
- ( - fixed_asset_account, - accumulated_depreciation_account, - depreciation_expense_account, - ) = get_depreciation_accounts(asset.asset_category, asset.company) + ( + _, + accumulated_depreciation_account, + depreciation_expense_account, + ) = get_depreciation_accounts(asset.asset_category, asset.company)erpnext/accounts/doctype/loyalty_program/loyalty_program.py (2)
8-8: Import Coalesce to ensure SUMs never return NoneFollow-up to the query change below: guard aggregated values against NULLs at the DB level.
-from frappe.query_builder.functions import Sum +from frappe.query_builder.functions import Sum, Coalesce
59-81: Optional: remove redundant GROUP BY for a simpler and slightly faster querySince you filter by a single customer, GROUP BY adds no value and can be removed (as in the diff above).
erpnext/setup/doctype/item_group/item_group.py (1)
4-4: Remove unused import
copyis no longer used after the change. Clean it up.-import copyerpnext/setup/doctype/brand/brand.py (1)
5-5: Remove unused import
copyis unused; please drop it.-import copyerpnext/stock/doctype/item/item.py (3)
1280-1283: Consistent sanitization; makepopsafeThe switch to
as_dict(no_private_properties=True)here mirrors the other doctypes—good. Consider a safe pop to avoid KeyError ifnameis ever excluded by future changes.- row = d.as_dict(no_private_properties=True) - row.pop("name") + row = d.as_dict(no_private_properties=True) + row.pop("name", None)
4-4: Remove unused import
copyis not used in this module anymore.-import copy
1274-1278: Optional: also sanitize the top-levelItemdict for consistencyIf the intent is to avoid leaking private/internal fields broadly, consider sanitizing the top-level
Itemdict as well. This may be a behavioral change; verify consumers don’t rely on default/private fields fromget_item_defaults.- out = item.as_dict() + out = item.as_dict(no_private_properties=True)Please verify:
- No downstream callers rely on default/private fields (e.g.,
owner,creation, etc.) from this function.- Integration tests covering API responses still pass.
erpnext/accounts/doctype/accounting_dimension_filter/accounting_dimension_filter.json (1)
95-100: Harden the hidden field against accidental edits and copiesSince fieldname is derived and used internally, mark it read-only and no-copy to avoid accidental mutation via API or during copy/duplicate flows.
{ "fieldname": "fieldname", "fieldtype": "Data", "hidden": 1, - "label": "Fieldname" + "label": "Fieldname", + "read_only": 1, + "no_copy": 1 }erpnext/accounts/doctype/accounting_dimension_filter/accounting_dimension_filter.py (3)
22-22: Type hint for accounting_dimension looks offaccounting_dimension: DF.Literal[None] suggests the field only accepts None, which is incorrect for a required Select. If the generator can’t enumerate options, consider a broader type such as DF.Data | None or DF.Select | None. At minimum, avoid Literal[None] to prevent misleading type checks.
Proposed hint (non-blocking, manual adjustment outside the auto-generated block if your generator can’t be updated):
- accounting_dimension: DF.Literal[None] + accounting_dimension: DF.Data | None
39-42: Guard validate() against missing AD entries and avoid redundant writesLogic is fine; small robustness improvement: compute once, fallback to scrub, and only assign if changed. This reduces unnecessary dirtying and defends if accounting_dimension is ever empty (even though it’s required).
- self.fieldname = frappe.db.get_value( - "Accounting Dimension", {"document_type": self.accounting_dimension}, "fieldname" - ) or frappe.scrub(self.accounting_dimension) # scrub to handle default accounting dimension + if self.accounting_dimension: + value = frappe.db.get_value( + "Accounting Dimension", + {"document_type": self.accounting_dimension}, + "fieldname", + ) or frappe.scrub(self.accounting_dimension) + if self.fieldname != value: + self.fieldname = value
5-7: Remove unused import scrubscrub is imported but not used (you’re calling frappe.scrub instead). Clean up to appease linters.
-from frappe import _, scrub +from frappe import _erpnext/patches/v15_0/update_fieldname_in_accounting_dimension_filter.py (1)
28-36: Optional: drop default_accounting_dimension() after adopting LEFT JOINLEFT JOIN + fallback makes this redundant. If you prefer simplicity, remove the pre-update and the function.
-def execute(): - default_accounting_dimension() +def execute(): ADF = DocType("Accounting Dimension Filter") AD = DocType("Accounting Dimension") @@ -def default_accounting_dimension(): - ADF = DocType("Accounting Dimension Filter") - for dim in ("Cost Center", "Project"): - ( - frappe.qb.update(ADF) - .set(ADF.fieldname, frappe.scrub(dim)) - .where(ADF.accounting_dimension == dim) - .run() - ) +def default_accounting_dimension(): + # No-op after LEFT JOIN change; retained only if you want explicit defaults. + passerpnext/manufacturing/doctype/bom_creator/bom_creator.py (1)
496-501: Guardparent_row_noassignment & early-return before looping items
- Move the
if name == doc.namecheck (currently at lines 496–501) to immediately before thefor name in doc.itemsloop so you short-circuit without scanning the entire list.- Since
parent_row_nois defined as a Data field inbom_creator_item.json, passingNonewill be stored as null. In theadd_sub_assemblyfunction (in the samebom_creator.py), only include or setparent_row_nowhen it’s truthy. For example:# before: always writing None- child = {
- "item_code": code,
- "parent_row_no": parent_row_no,
- …
- }
- child = {
- "item_code": code,
- …
- }
- if parent_row_no:
- child["parent_row_no"] = parent_row_no
This ensures empty parent references aren’t serialized as null in the child table. </blockquote></details> <details> <summary>erpnext/manufacturing/doctype/workstation_cost/workstation_cost.js (1)</summary><blockquote> `1-8`: **Replace commented scaffold with a clear intent marker** This file currently contains only commented-out scaffold. Either remove the dead code or add a concise “intentionally blank” note to avoid confusion in future diffs. Apply this diff to keep the header and add an explicit note: ```diff // Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and contributors // For license information, please see license.txt -// frappe.ui.form.on("Workstation Cost", { -// refresh(frm) { -// -// }, -// }); +// Intentionally left blank. Client-side handlers will be added when UI interactions are implemented.erpnext/manufacturing/doctype/workstation_cost/test_workstation_cost.py (1)
1-21: Add a minimal meta-level test to validate the new DocTypeSince this is a new DocType, a lightweight meta test helps catch regressions (field renames, child-table status). Suggest asserting it’s a child table and that expected fields exist.
Apply this diff:
# Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and Contributors # See license.txt -# import frappe +import frappe from frappe.tests import IntegrationTestCase # On IntegrationTestCase, the doctype test records and all # link-field test record dependencies are recursively loaded # Use these module variables to add/remove to/from that list EXTRA_TEST_RECORD_DEPENDENCIES = [] # eg. ["User"] IGNORE_TEST_RECORD_DEPENDENCIES = [] # eg. ["User"] class IntegrationTestWorkstationCost(IntegrationTestCase): """ Integration tests for WorkstationCost. Use this class for testing interactions between multiple components. """ - pass + def test_meta_defines_expected_fields(self): + meta = frappe.get_meta("Workstation Cost") + self.assertTrue(meta.istable, "Workstation Cost should be a child table") + fieldnames = {df.fieldname for df in meta.fields} + self.assertIn("operating_component", fieldnames) + self.assertIn("operating_cost", fieldnames)erpnext/manufacturing/doctype/workstation_operating_component_account/test_workstation_operating_component_account.py (1)
1-21: Add a minimal meta-load test to validate DocType presenceA simple smoke test ensures the DocType metadata loads (useful when refactoring or renaming).
Apply this diff:
# Copyright (c) 2025, Frappe Technologies Pvt. Ltd. and Contributors # See license.txt -# import frappe +import frappe from frappe.tests import IntegrationTestCase # On IntegrationTestCase, the doctype test records and all # link-field test record dependencies are recursively loaded # Use these module variables to add/remove to/from that list EXTRA_TEST_RECORD_DEPENDENCIES = [] # eg. ["User"] IGNORE_TEST_RECORD_DEPENDENCIES = [] # eg. ["User"] class IntegrationTestWorkstationOperatingComponentAccount(IntegrationTestCase): """ Integration tests for WorkstationOperatingComponentAccount. Use this class for testing interactions between multiple components. """ - pass + def test_meta_loads(self): + # Validate that the DocType exists and metadata is retrievable + meta = frappe.get_meta("Workstation Operating Component Account") + self.assertIsNotNone(meta)erpnext/manufacturing/doctype/workstation_cost/workstation_cost.json (2)
14-19: Currency field should declare currency optionsWithout options, Currency fields may render with defaults inconsistent with company currency. Consider binding to company currency (if available on the parent) or the generic currency field.
Apply one of these, depending on parent context:
Option A: If parent has a company field
{ "fieldname": "operating_cost", "fieldtype": "Currency", "in_list_view": 1, "label": "Operating Cost", + "options": "Company:company:default_currency", "reqd": 1 },Option B: If a generic currency field is available in parent
{ "fieldname": "operating_cost", "fieldtype": "Currency", "in_list_view": 1, "label": "Operating Cost", + "options": "currency", "reqd": 1 },
30-31: Disable web page search indexing for child tableChild tables are not meant to be discoverable via website search. Turn off indexing.
- "index_web_pages_for_search": 1, + "index_web_pages_for_search": 0,erpnext/manufacturing/doctype/workstation_operating_component_account/workstation_operating_component_account.json (3)
22-27: Make Expense Account required (or validate server-side) and constrain to expense leaf accountsLeaving the expense account optional can yield incomplete mappings; also, users can currently select non-expense or group accounts.
Minimal JSON change:
{ "fieldname": "expense_account", "fieldtype": "Link", "in_list_view": 1, "label": "Expense Account", - "options": "Account" + "options": "Account", + "reqd": 1 }Additionally, add a link filter via client script to enforce:
- is_group = 0
- root_type = "Expense"
- company = row.company
Example (workstation_operating_component_account.js):
frappe.ui.form.on('Workstation Operating Component', { setup(frm) { frm.set_query('expense_account', 'accounts', function(doc, cdt, cdn) { const row = locals[cdt][cdn]; return { filters: { company: row.company, is_group: 0, root_type: 'Expense', }, }; }); }, });If you prefer server-side enforcement, add a validate hook to reject invalid accounts.
30-31: Disable web search indexing for child tableSame rationale as other child tables: turn off indexing.
- "index_web_pages_for_search": 1, + "index_web_pages_for_search": 0,
12-20: Ensure uniqueness per company row under a componentPrevent duplicate company rows under the same parent to avoid ambiguous account resolution. Implement a validate in Workstation Operating Component to enforce one account row per company.
I can draft the validate() hook to enforce uniqueness if you confirm the parent Doctype file path you want to place it in.
erpnext/manufacturing/doctype/bom_item/bom_item.json (1)
1-329: Add tests to assert propagation of is_sub_assembly_itemRecommend adding tests for:
- BOM explosion includes is_sub_assembly_item on relevant rows.
- Toggling do_not_explode and sub-assembly behavior doesn’t regress.
I can scaffold pytest cases for BOM explosion asserting the flag propagation; say the word.
erpnext/manufacturing/doctype/workstation/test_workstation.py (1)
4-4: Drop unused translation import once _() usages are removed.After switching to literals, this import is no longer needed.
-from frappe import _erpnext/manufacturing/doctype/bom/bom.js (1)
92-109: SFG toggle handler is straightforward and correct.Updating read_only for finished_good, finished_good_qty, bom_no on the Operations grid is appropriate.
If these fields are already controlled via depends_on in JSON, consider consolidating logic to a single mechanism (either JSON depends_on for hidden/mandatory/read-only or JS) to reduce drift.
erpnext/manufacturing/doctype/workstation_operating_component/test_workstation_operating_component.py (3)
4-6: Drop commented import and keep imports minimal.The commented-out
import frappecan be removed to keep the test module tidy; re-add it when needed.
7-12: Scaffold variables are fine but can be omitted until used.The EXTRA/IGNORE dependency hooks are OK to keep, but consider adding them only when a test needs them to reduce churn.
14-20: Add at least one smoke test to assert doc creation/fixtures.A basic test ensures the Doctype wiring works and prevents regressions.
Apply this diff to replace the pass with a minimal smoke test:
class IntegrationTestWorkstationOperatingComponent(IntegrationTestCase): """ Integration tests for WorkstationOperatingComponent. Use this class for testing interactions between multiple components. """ - - pass + def test_can_load_component_doctype(self): + # sanity: ensure doctype exists and can be listed + names = frappe.get_all("Workstation Operating Component", pluck="name") + self.assertIsInstance(names, list)erpnext/manufacturing/doctype/production_plan/production_plan.py (4)
1004-1004: Rename sentinel to reflect intent (tracks processing, not BOM setting).
track_semi_finished_goodshere is used as a “processed-any-bom” flag. The name is misleading and makes the condition at Line 1040 harder to read.Apply this diff to improve readability:
- track_semi_finished_goods = True + any_bom_processed = False
1015-1022: Avoid spamming the user with repeated warnings.This message will print once per PO item whose BOM has Track Semi Finished Goods enabled. If many rows match, users get spammed.
Aggregate and print a single message summarizing all skipped rows/BOMs, or deduplicate per BOM. If you want, I can provide a patch that collects and prints once.
1025-1025: Flip the sentinel with clearer semantics.This line signals that at least one BOM got processed. With the proposed rename, set the flag to True.
Apply this diff:
- track_semi_finished_goods = False + any_bom_processed = True
1040-1044: Condition reads clearer when keyed off “processed-any-bom”.The note should show only when something was processed, nothing was added, and skipping is enabled.
Apply this diff:
- if ( - not track_semi_finished_goods - and not sub_assembly_items_store - and self.skip_available_sub_assembly_item - ): + if (any_bom_processed and not sub_assembly_items_store and self.skip_available_sub_assembly_item):erpnext/manufacturing/doctype/workstation_type/workstation_type.py (1)
30-41: Use a set for duplicate detection; simpler and faster.Current list-based accumulation is O(n^2) worst-case and more verbose.
Apply this diff:
def validate_duplicate_operating_component(self): - components = [] - for row in self.workstation_costs: - if row.operating_component not in components: - components.append(row.operating_component) - else: - frappe.throw( - _("Duplicate Operating Component {0} found in Operating Components").format( - bold(row.operating_component) - ) - ) + seen = set() + for row in self.workstation_costs: + comp = row.operating_component + if comp in seen: + frappe.throw( + _("Duplicate Operating Component {0} found in Operating Components").format(bold(comp)) + ) + seen.add(comp)erpnext/patches/v16_0/make_workstation_operating_components.py (1)
5-12: Guard against missing default accounts.If a Company has neither default operating cost nor expense account, you’ll insert child rows with a null
expense_account, which may violate mandatory constraints.Apply this diff to make account assignment safe:
def get_operating_cost_account(company): company_details = frappe.db.get_value( "Company", company, ["default_operating_cost_account", "default_expense_account"], as_dict=True ) - return company_details.get("default_operating_cost_account") or company_details.get( - "default_expense_account" - ) + return company_details.get("default_operating_cost_account") or company_details.get("default_expense_account")And below (Lines 31-36):
- for company in companies: - operating_cost_account = get_operating_cost_account(company) - - doc.append("accounts", {"company": company, "expense_account": operating_cost_account}) + for company in companies: + operating_cost_account = get_operating_cost_account(company) + if operating_cost_account: + doc.append("accounts", {"company": company, "expense_account": operating_cost_account}) + # else: skip; avoid inserting invalid rowsPlease confirm whether
Workstation Operating Component Account.expense_accountis mandatory; if it is, the guard is essential.erpnext/manufacturing/doctype/workstation_operating_component/workstation_operating_component.json (1)
27-31: Nit: Pluralize the table field label.The "accounts" field is a Table; consider pluralizing the label for clarity.
Apply this diff:
- "label": "Component Expense Account", + "label": "Component Expense Accounts",erpnext/manufacturing/doctype/bom_operation/bom_operation.json (1)
197-206: UI gating for semi-finished flow is coherent; consider making key fields mandatory when visible.Finished Good, BOM No, and Qty to Produce are conditionally shown when track_semi_finished_goods is enabled. It may be beneficial to make some of them mandatory when visible to prevent incomplete operation setups.
Apply this diff to add mandatory_depends_on:
{ "columns": 3, "depends_on": "eval:parent.track_semi_finished_goods === 1", "fieldname": "finished_good", "fieldtype": "Link", "in_list_view": 1, "label": "FG / Semi FG Item", "options": "Item", + "mandatory_depends_on": "eval:parent.track_semi_finished_goods === 1" }, { "columns": 2, "depends_on": "eval:parent.track_semi_finished_goods === 1", "fieldname": "bom_no", "fieldtype": "Link", "in_list_view": 1, "label": "BOM No", "options": "BOM", + "mandatory_depends_on": "eval:parent.track_semi_finished_goods === 1" }, { "columns": 2, "default": "1", "depends_on": "eval:parent.track_semi_finished_goods === 1", "fieldname": "finished_good_qty", "fieldtype": "Float", "in_list_view": 1, "label": "Qty to Produce", + "mandatory_depends_on": "eval:parent.track_semi_finished_goods === 1" }Also applies to: 205-211, 214-221, 223-230, 275-279
erpnext/manufacturing/doctype/workstation/workstation.py (1)
69-83: Prevent false duplicate detection for empty components.If operating_component is missing on multiple rows, current logic flags duplicates. Skip empty/null values.
Apply this diff:
def validate_duplicate_operating_component(self): components = [] for row in self.workstation_costs: - if row.operating_component not in components: + if not row.operating_component: + continue + if row.operating_component not in components: components.append(row.operating_component) else: frappe.throw( _("Duplicate Operating Component {0} found in Operating Components").format( bold(row.operating_component) ) )erpnext/accounts/doctype/pos_invoice/pos_invoice.js (2)
59-67: Harden get_value path and coerce to booleanGuard on r.message instead of r.exc (which may not be set) and coerce the flag to a boolean.
Apply this diff:
- if (this.frm.doc.pos_profile) { - frappe.db - .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop") - .then((r) => { - if (!r.exc) { - this.frm.set_default_payment = r.message.set_grand_total_to_default_mop; - } - }); - } + if (this.frm.doc.pos_profile) { + frappe.db + .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop") + .then((r) => { + if (r && r.message) { + this.frm.set_default_payment = !!r.message.set_grand_total_to_default_mop; + } + }); + }
134-134: Use fallback and boolean coercion for set_default_paymentDepending on backend changes, the payload may expose set_default_payment or the legacy set_grand_total_to_default_mop. Use a nullish-coalescing fallback and coerce to boolean.
Apply this diff:
- this.frm.set_default_payment = r.message.set_default_payment; + this.frm.set_default_payment = !!(r.message.set_default_payment ?? r.message.set_grand_total_to_default_mop);Also confirm that downstream consumers read this frm-level property (set_default_payment) to affect payment behavior; otherwise, this will be a no-op.
erpnext/stock/doctype/purchase_receipt/purchase_receipt.json (1)
1291-1296: Mark virtual field as read-only to match display-only intentSince last_scanned_warehouse is virtual and display-only, mark it read_only to prevent user edits that won’t persist.
Apply this diff:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1 }erpnext/selling/doctype/sales_order/sales_order.json (1)
1686-1691: Make the virtual field read-only for clarityAs with other DocTypes, the Last Scanned Warehouse virtual field should be read-only to reflect its display-only purpose.
Apply this diff:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1 }erpnext/stock/doctype/delivery_note/delivery_note.json (1)
1415-1421: Mark the virtual field as read-only and hide from printSince last_scanned_warehouse is UI-only (is_virtual = 1), make it read-only and hidden on print to avoid accidental edits and noisy printouts.
Apply this diff:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1, + "print_hide": 1 }erpnext/stock/doctype/delivery_note/delivery_note.py (1)
887-891: Clarify semantics for empty selection listsCurrently, if args.filtered_children is present but empty, all items will pass the condition (treats empty as “select all”). If the intent of providing filtered_children is to explicitly control inclusion, consider treating an empty list as “select none”, and only include all when the key is absent.
- def select_item(d): - filtered_items = args.get("filtered_children", []) - child_filter = d.name in filtered_items if filtered_items else True - return child_filter + def select_item(d): + filtered_items = args.get("filtered_children", None) + # Include all if the key is absent; if present, respect the explicit selection (even if empty). + return True if filtered_items is None else d.name in filtered_itemserpnext/stock/doctype/stock_reconciliation/stock_reconciliation.json (1)
183-189: Make the virtual field read-only and hidden on printSame rationale as other doctypes: it’s UI-only and should not be editable or printed.
Apply this diff:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1, + "print_hide": 1 }erpnext/selling/doctype/quotation/quotation.json (1)
1103-1110: Make virtual field read-only to avoid UX confusionSince last_scanned_warehouse is virtual (non-persistent), setting it read-only prevents user edits that won’t be saved.
Apply:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1 }erpnext/accounts/doctype/purchase_invoice/purchase_invoice.json (1)
1658-1664: Make virtual field read-only for clarityMarking the virtual “Last Scanned Warehouse” as read-only avoids editable-but-unsaved UX.
Apply:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1 },erpnext/accounts/doctype/sales_invoice/sales_invoice.json (1)
2225-2231: Mark virtual ‘Last Scanned Warehouse’ as read-onlyConsistent with other doctypes, make the virtual field read-only.
Apply:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1 }erpnext/accounts/doctype/pos_invoice/pos_invoice.json (1)
1599-1605: Make the virtual field read-only (and hidden in print) to avoid accidental editsThis is a computed, non-persistent display. Marking it read-only (and print-hidden) prevents confusion and unintended edits/printing.
Apply this diff:
{ "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1, + "print_hide": 1 }erpnext/stock/doctype/purchase_receipt/purchase_receipt.py (1)
1329-1333: Harden JSON args parsingGuard against invalid JSON to avoid tracebacks when args is a malformed string.
Apply this diff:
@frappe.whitelist() def make_purchase_invoice(source_name, target_doc=None, args=None): if args is None: args = {} if isinstance(args, str): - args = json.loads(args) + try: + args = json.loads(args) + } except Exception: + args = {}erpnext/public/js/controllers/buying.js (1)
485-486: Simplify condition for readabilityThe “> 0” comparison adds noise without changing semantics. Prefer a clear boolean.
Apply this diff:
- item.type_of_transaction = !doc.is_return > 0 ? "Inward" : "Outward"; + item.type_of_transaction = !doc.is_return ? "Inward" : "Outward";erpnext/buying/doctype/purchase_order/purchase_order.json (1)
44-47: Virtual “Last Scanned Warehouse”: add read_only and hide from printThe field is virtual status text; mark it read-only and excluded from print to prevent accidental edits and leakage into print formats. Placement near scan_barcode is sensible; moving set_from_warehouse after items_col_break is fine.
Apply this diff:
"last_scanned_warehouse", ... { "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1, + "print_hide": 1 }Also applies to: 1312-1318
erpnext/stock/doctype/material_request/material_request.json (1)
23-26: Virtual “Last Scanned Warehouse”: mark read-only and hide from print; field reordering LGTMAdding the non-persistent field near scan_barcode aligns with the intended UX. Suggest making it read-only and not printable.
Apply this diff:
"last_scanned_warehouse", "column_break5", "set_from_warehouse", ... { "depends_on": "eval: doc.last_scanned_warehouse", "fieldname": "last_scanned_warehouse", "fieldtype": "Data", "is_virtual": 1, - "label": "Last Scanned Warehouse" + "label": "Last Scanned Warehouse", + "read_only": 1, + "print_hide": 1 }Also applies to: 359-365
erpnext/accounts/doctype/dunning/dunning.py (1)
167-227: Dunning state updater: deduplicate fetched names and consider robustness of outstanding checkOverall approach—toggling Dunning status based on invoice/payment-schedule outstanding—is sound and removes docstatus coupling. Two improvements:
- Avoid duplicate loads/saves when the join returns the same Dunning multiple times.
- Consider OR semantics to be conservative when determining “Unresolved” to avoid prematurely resolving if either the invoice or the specific overdue payment still shows outstanding due to timing/order of updates.
Apply this diff to deduplicate loaded dunnings:
- dunnings = [frappe.get_doc("Dunning", dunning.name) for dunning in dunnings] + dunning_names = {d.name for d in dunnings} + dunnings = [frappe.get_doc("Dunning", name) for name in dunning_names]Optionally, make the outstanding check conservative:
- has_outstanding = invoice_outstanding > 0 and ps_outstanding > 0 + # conservative: if either shows outstanding, keep unresolved + has_outstanding = (invoice_outstanding > 0) or (ps_outstanding > 0)If you keep the stricter AND, please ensure Overdue Payment rows always have a valid payment_schedule and that schedule.outstanding is updated synchronously with invoice.outstanding to prevent false “Resolved” states.
erpnext/patches/v14_0/single_to_multi_dunning.py (1)
50-52: OK to bypass link validation during data patch; consider hardening save contextSetting
dunning.flags.ignore_links = Trueimmediately beforesave()is reasonable in a patch to avoid link validation noise on migrated docs. Two small hardening tweaks you may consider:
- Save with explicit permission bypass to avoid patch failures in restrictive environments:
dunning.save(ignore_permissions=True).- Add a short comment explaining why link validation is intentionally bypassed here (will help future maintainers).
- dunning.flags.ignore_links = True - dunning.save() + # Skip link validations for migration-save; permissions bypass is intentional in patches. + dunning.flags.ignore_links = True + dunning.save(ignore_permissions=True)erpnext/selling/doctype/sales_order/sales_order.py (1)
996-1000: Parse kwargs when passed as JSON (consistency with other APIs)
kwargsmay arrive as a JSON string from client calls. You already parseargssimilarly elsewhere; mirror that here to avoid subtle runtime issues.if not kwargs: kwargs = { "for_reserved_stock": frappe.flags.args and frappe.flags.args.for_reserved_stock, "skip_item_mapping": frappe.flags.args and frappe.flags.args.skip_item_mapping, } - kwargs = frappe._dict(kwargs) + # Support both dict and JSON string inputs for kwargs. + if isinstance(kwargs, str): + kwargs = json.loads(kwargs) + kwargs = frappe._dict(kwargs)erpnext/public/js/controllers/accounts.js (1)
26-35: Avoid redeclaration of account_type (lint error)Redeclaring
var account_typein each branch triggers noRedeclare. Define once and assign per-branch.- frm.set_query("account_head", "taxes", function (doc) { - if (frm.cscript.tax_table == "Sales Taxes and Charges") { - var account_type = ["Tax", "Chargeable", "Expense Account"]; - } else { - var account_type = [ + frm.set_query("account_head", "taxes", function (doc) { + let account_type; + if (frm.cscript.tax_table == "Sales Taxes and Charges") { + account_type = ["Tax", "Chargeable", "Expense Account"]; + } else { + account_type = [ "Tax", "Chargeable", "Income Account", "Expenses Included In Valuation", ];erpnext/public/js/controllers/taxes_and_totals.js (1)
1052-1058: Don’t use Array.prototype.find for side-effects; use forEach.find() is meant to locate one element and stops early; here we mutate all rows. Use forEach for clarity and correctness.
Apply this diff:
- this.frm.doc.payments.find((payment) => { - if (payment.default) { - payment.amount = total_amount_to_pay; - } else { - payment.amount = 0; - } - }); + this.frm.doc.payments.forEach((payment) => { + payment.amount = payment.default ? total_amount_to_pay : 0; + });erpnext/public/scss/erpnext.scss (1)
622-628: LGTM; ensure parent container is positioned for correct absolute layout.The button relies on absolute positioning. Confirm the field wrapper has position: relative; otherwise placement may be off in some forms.
erpnext/stock/doctype/stock_reconciliation/stock_reconciliation.js (1)
10-11: Optional: detach scanner listeners on form teardown.If BarcodeScanner attaches DOM listeners, consider disposing it on form unload to prevent leaks when navigating between forms frequently.
I can draft a small helper to support
.dispose()on the scanner and hook it into the form’s on_close if the utility supports it.erpnext/stock/doctype/stock_entry/stock_entry.json (1)
695-702: Make Last Scanned Warehouse read-only and normalize depends_on expression.Since this is a virtual, system-populated field, it should be read-only. Also, normalize the depends_on expression for consistency.
Apply this diff:
- { - "depends_on": "eval: doc.last_scanned_warehouse", - "fieldname": "last_scanned_warehouse", - "fieldtype": "Data", - "is_virtual": 1, - "label": "Last Scanned Warehouse" - } + { + "depends_on": "eval:doc.last_scanned_warehouse", + "fieldname": "last_scanned_warehouse", + "fieldtype": "Data", + "is_virtual": 1, + "read_only": 1, + "label": "Last Scanned Warehouse" + }erpnext/stock/doctype/stock_entry/test_stock_entry.py (1)
2086-2149: Avoid pre-submit valuation assertions to reduce flakiness; keep the post-submit checks.Distribution of additional_costs to valuation_rate may run on validate/submit depending on hooks. The pre-submit assertion on valuation_rate can intermittently fail.
Suggested tweak:
- self.assertEqual(se.items[0].basic_rate, 100) - self.assertEqual(se.items[0].valuation_rate, 125) + self.assertEqual(se.items[0].basic_rate, 100)Optional: use assertAlmostEqual for the final incoming_rate assertion to be resilient to precision:
- self.assertEqual(incoming_rate, 125.0) + self.assertAlmostEqual(incoming_rate, 125.0, places=4)erpnext/accounts/utils.py (1)
1943-1944: Verify availability and usage of update_linked_dunnings.The inline import and usage look fine and help avoid circular imports, but please confirm:
- erpnext.accounts.doctype.dunning.dunning exports update_linked_dunnings(doc, previous_outstanding_amount),
- No unintended import cycle in production,
- The function tolerates receiving a LazyDocument.
If needed, guard the call to fail-safe in environments without Dunning enabled.
Example guard:
- from erpnext.accounts.doctype.dunning.dunning import update_linked_dunnings + try: + from erpnext.accounts.doctype.dunning.dunning import update_linked_dunnings + except Exception: + update_linked_dunnings = None ... - update_linked_dunnings(ref_doc, previous_outstanding_amount) + if update_linked_dunnings: + update_linked_dunnings(ref_doc, previous_outstanding_amount)Also applies to: 1974-1989
erpnext/buying/doctype/request_for_quotation/request_for_quotation.py (2)
586-604: Robustness and readability: use an explicit join, handle None filters before building the qb query
- Using inner_join makes intent clearer than cross join + where.
- Guard against filters being None for defensive safety.
Apply:
- rfq = frappe.qb.DocType("Request for Quotation") - rfq_supplier = frappe.qb.DocType("Request for Quotation Supplier") - - query = ( - frappe.qb.from_(rfq) - .from_(rfq_supplier) - .select(rfq.name) - .distinct() - .select(rfq.transaction_date, rfq.company) - .where( - (rfq.name == rfq_supplier.parent) - & (rfq_supplier.supplier == filters.get("supplier")) - & (rfq.docstatus == 1) - & (rfq.company == filters.get("company")) - ) - .orderby(rfq.transaction_date, order=Order.asc) - .limit(page_len) - .offset(start) - ) + rfq = frappe.qb.DocType("Request for Quotation") + rfq_supplier = frappe.qb.DocType("Request for Quotation Supplier") + + filters = filters or {} + + query = ( + frappe.qb.from_(rfq) + .inner_join(rfq_supplier) + .on(rfq.name == rfq_supplier.parent) + .select(rfq.name) + .distinct() + .select(rfq.transaction_date, rfq.company) + .where( + (rfq_supplier.supplier == filters.get("supplier")) + & (rfq.docstatus == 1) + & (rfq.company == filters.get("company")) + ) + .orderby(rfq.transaction_date, order=Order.asc) + .limit(page_len) + .offset(start) + )
606-608: Nit: LIKE pattern can use single % delimitersDouble % isn’t harmful, but conventional and clearer to use one on each side.
- query = query.where(rfq.name.like(f"%%{txt}%%")) + query = query.where(rfq.name.like(f"%{txt}%"))erpnext/accounts/doctype/purchase_invoice/purchase_invoice.py (2)
2083-2088: Expanded API for make_purchase_receipt: args parsing looks fineAccepting args (dict or JSON string) provides flexibility for selective mapping. Consider documenting expected shape in the docstring for discoverability.
2126-2127: Guard against None in condition with robust numeric conversionIf received_qty or qty can ever be None, abs(None) will raise. Using flt guards against this gracefully (consistent with update_item).
- "condition": lambda doc: abs(doc.received_qty) < abs(doc.qty) and select_item(doc), + "condition": lambda doc: flt(doc.received_qty) < flt(doc.qty) and select_item(doc),erpnext/accounts/doctype/pos_invoice/pos_invoice.py (2)
414-416: Nit: Improve error message wording.Minor copy-edit for clarity and consistency.
Apply:
- _("Row #{}: Stock quantity not enough for Item Code: {} under warehouse {}.").format( - d.idx, item_code, warehouse - ), + _("Row #{}: Insufficient stock for Item Code {} in warehouse {}.").format( + d.idx, item_code, warehouse + ),
879-883: Guard against division by zero when computing bundle availability.If a bundle component row (unexpectedly) has qty == 0, this will raise; add a safe guard.
- max_available_bundles = item_bin_qty / item.qty + max_available_bundles = (item_bin_qty / item.qty) if item.qty else 0erpnext/buying/doctype/purchase_order/purchase_order.py (2)
751-756: Harden args parsing against non-dict inputs.If a caller passes a JSON array/string accidentally, json.loads will return a list and args.get will crash. Normalize to a dict.
def make_purchase_receipt(source_name, target_doc=None, args=None): if args is None: args = {} if isinstance(args, str): - args = json.loads(args) + args = json.loads(args) or {} + if not isinstance(args, dict): + args = {}If you prefer, use frappe.utils.parse_json which already handles strings safely.
830-835: Mirror args normalization in get_mapped_purchase_invoice.Same robustness improvement as above.
def get_mapped_purchase_invoice(source_name, target_doc=None, ignore_permissions=False, args=None): if args is None: args = {} if isinstance(args, str): - args = json.loads(args) + args = json.loads(args) or {} + if not isinstance(args, dict): + args = {}erpnext/controllers/accounts_controller.py (1)
3420-3422: Guard against None posting_date when adjusting due_date.If posting_date is None, getdate(None) can misbehave. Add a simple guard.
- if getdate(term_details.due_date) < getdate(posting_date): - term_details.due_date = posting_date + if posting_date and term_details.due_date and getdate(term_details.due_date) < getdate(posting_date): + term_details.due_date = posting_dateerpnext/public/js/utils/barcode_scanner.js (1)
55-66: Clarify error message when scanning a warehouse on unsupported forms.Current logic shows “Cannot find Item with this Barcode” even when a Warehouse barcode is scanned on a doctype that lacks last_scanned_warehouse. Consider a clearer message and/or guiding users.
Example tweak:
- If data.warehouse && !this.has_last_scanned_warehouse: show “Warehouse scanning is not supported on this form.”
erpnext/accounts/report/asset_depreciations_and_balances/asset_depreciations_and_balances.py (1)
299-317: Avoid StopIteration when no per-asset depreciation row is found.Using next(...) without a default can raise StopIteration if assets is empty or the asset is absent. Provide a default empty dict to keep flt(...) downstream safe.
- row.update(next(asset for asset in assets if asset["asset"] == asset_detail.get("name", ""))) + asset_row = next( + (asset for asset in assets if asset["asset"] == asset_detail.get("name", "")), + {}, + ) + row.update(asset_row)Additionally, consider applying the same pattern to the category path where you do row.update(next(...)) for asset_categories to make both code paths robust (outside this hunk).
| existing_reverse = frappe.db.exists("Journal Entry", {"reversal_of": source_name, "docstatus": 1}) | ||
| if existing_reverse: | ||
| frappe.throw( | ||
| _("A Reverse Journal Entry {0} already exists for this Journal Entry.").format( | ||
| get_link_to_form("Journal Entry", existing_reverse) | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent duplicate reverse entries under concurrency by validating on submit
The pre-check blocking a second submitted reversal is a solid improvement. However, there’s still a race window: two users can generate draft reversals concurrently and both proceed to submit, causing duplicate reversals. Add a server-side check on submit to guarantee uniqueness.
Suggested additions outside this hunk (in JournalEntry.before_submit or validate):
# In JournalEntry.before_submit (or validate, but submit is stricter)
def before_submit(self):
# existing logic...
# ...
if self.reversal_of:
existing = frappe.db.exists(
"Journal Entry",
{
"reversal_of": self.reversal_of,
"docstatus": 1,
"name": ["!=", self.name],
},
)
if existing:
frappe.throw(
_("A submitted Reverse Journal Entry {0} already exists for {1}.").format(
get_link_to_form("Journal Entry", existing),
get_link_to_form("Journal Entry", self.reversal_of),
)
)Optional refinement:
- If you want to also avoid multiple drafts, block when a non-cancelled reversal already exists (docstatus in [0,1]) or return the existing draft instead of creating a new one.
Tests: add one that creates a reversal, submits it, then attempts to create another reversal and asserts a throw.
I can add the before_submit guard and wire up a test case in the JE test suite. Want me to draft the patch?
| LoyaltyPointEntry = frappe.qb.DocType("Loyalty Point Entry") | ||
|
|
||
| query = ( | ||
| frappe.qb.from_(LoyaltyPointEntry) | ||
| .select( | ||
| Sum(LoyaltyPointEntry.loyalty_points).as_("loyalty_points"), | ||
| Sum(LoyaltyPointEntry.purchase_amount).as_("total_spent"), | ||
| ) | ||
| .where( | ||
| (LoyaltyPointEntry.customer == customer) | ||
| & (LoyaltyPointEntry.loyalty_program == loyalty_program) | ||
| & (LoyaltyPointEntry.posting_date <= expiry_date) | ||
| ) | ||
| .groupby(LoyaltyPointEntry.customer) | ||
| ) | ||
|
|
||
| if company: | ||
| condition = " and company=%s " % frappe.db.escape(company) | ||
| query = query.where(LoyaltyPointEntry.company == company) | ||
|
|
||
| if not include_expired_entry: | ||
| condition += " and expiry_date>='%s' " % expiry_date | ||
|
|
||
| loyalty_point_details = frappe.db.sql( | ||
| f"""select sum(loyalty_points) as loyalty_points, | ||
| sum(purchase_amount) as total_spent from `tabLoyalty Point Entry` | ||
| where customer=%s and loyalty_program=%s and posting_date <= %s | ||
| {condition} | ||
| group by customer""", | ||
| (customer, loyalty_program, expiry_date), | ||
| as_dict=1, | ||
| ) | ||
| query = query.where(LoyaltyPointEntry.expiry_date >= expiry_date) | ||
|
|
||
| loyalty_point_details = query.run(as_dict=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exclude non-expiring points bug, potential None sums, and missing docstatus filter
- Entries with no expiry_date are currently excluded when include_expired_entry is False. Those should count as valid (non-expiring) points.
- SUM() can return NULLs, which can bubble into lp_details.total_spent and break arithmetic later (e.g., addition in Lines 106–115). Coalesce at the DB level to ensure 0s.
- Consider filtering only submitted entries (docstatus = 1) to avoid drafts/cancelled data skewing balances.
- The GROUP BY is redundant since you already filter by a specific customer.
Apply this diff to the query block:
- LoyaltyPointEntry = frappe.qb.DocType("Loyalty Point Entry")
-
- query = (
- frappe.qb.from_(LoyaltyPointEntry)
- .select(
- Sum(LoyaltyPointEntry.loyalty_points).as_("loyalty_points"),
- Sum(LoyaltyPointEntry.purchase_amount).as_("total_spent"),
- )
- .where(
- (LoyaltyPointEntry.customer == customer)
- & (LoyaltyPointEntry.loyalty_program == loyalty_program)
- & (LoyaltyPointEntry.posting_date <= expiry_date)
- )
- .groupby(LoyaltyPointEntry.customer)
- )
+ LoyaltyPointEntry = frappe.qb.DocType("Loyalty Point Entry")
+
+ query = (
+ frappe.qb.from_(LoyaltyPointEntry)
+ .select(
+ Coalesce(Sum(LoyaltyPointEntry.loyalty_points), 0).as_("loyalty_points"),
+ Coalesce(Sum(LoyaltyPointEntry.purchase_amount), 0).as_("total_spent"),
+ )
+ .where(
+ (LoyaltyPointEntry.customer == customer)
+ & (LoyaltyPointEntry.loyalty_program == loyalty_program)
+ & (LoyaltyPointEntry.posting_date <= expiry_date)
+ & (LoyaltyPointEntry.docstatus == 1)
+ )
+ )
@@
- if company:
- query = query.where(LoyaltyPointEntry.company == company)
+ if company:
+ query = query.where(LoyaltyPointEntry.company == company)
@@
- if not include_expired_entry:
- query = query.where(LoyaltyPointEntry.expiry_date >= expiry_date)
+ if not include_expired_entry:
+ # include non-expiring entries too
+ query = query.where(
+ (LoyaltyPointEntry.expiry_date >= expiry_date)
+ | (LoyaltyPointEntry.expiry_date.isnull())
+ )
@@
- loyalty_point_details = query.run(as_dict=True)
+ loyalty_point_details = query.run(as_dict=True)Notes:
- If docstatus is not applicable for this DocType, drop that predicate. Otherwise it prevents draft/cancelled entries from affecting totals.
📝 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.
| LoyaltyPointEntry = frappe.qb.DocType("Loyalty Point Entry") | |
| query = ( | |
| frappe.qb.from_(LoyaltyPointEntry) | |
| .select( | |
| Sum(LoyaltyPointEntry.loyalty_points).as_("loyalty_points"), | |
| Sum(LoyaltyPointEntry.purchase_amount).as_("total_spent"), | |
| ) | |
| .where( | |
| (LoyaltyPointEntry.customer == customer) | |
| & (LoyaltyPointEntry.loyalty_program == loyalty_program) | |
| & (LoyaltyPointEntry.posting_date <= expiry_date) | |
| ) | |
| .groupby(LoyaltyPointEntry.customer) | |
| ) | |
| if company: | |
| condition = " and company=%s " % frappe.db.escape(company) | |
| query = query.where(LoyaltyPointEntry.company == company) | |
| if not include_expired_entry: | |
| condition += " and expiry_date>='%s' " % expiry_date | |
| loyalty_point_details = frappe.db.sql( | |
| f"""select sum(loyalty_points) as loyalty_points, | |
| sum(purchase_amount) as total_spent from `tabLoyalty Point Entry` | |
| where customer=%s and loyalty_program=%s and posting_date <= %s | |
| {condition} | |
| group by customer""", | |
| (customer, loyalty_program, expiry_date), | |
| as_dict=1, | |
| ) | |
| query = query.where(LoyaltyPointEntry.expiry_date >= expiry_date) | |
| loyalty_point_details = query.run(as_dict=True) | |
| LoyaltyPointEntry = frappe.qb.DocType("Loyalty Point Entry") | |
| query = ( | |
| frappe.qb.from_(LoyaltyPointEntry) | |
| .select( | |
| Coalesce(Sum(LoyaltyPointEntry.loyalty_points), 0).as_("loyalty_points"), | |
| Coalesce(Sum(LoyaltyPointEntry.purchase_amount), 0).as_("total_spent"), | |
| ) | |
| .where( | |
| (LoyaltyPointEntry.customer == customer) | |
| & (LoyaltyPointEntry.loyalty_program == loyalty_program) | |
| & (LoyaltyPointEntry.posting_date <= expiry_date) | |
| & (LoyaltyPointEntry.docstatus == 1) | |
| ) | |
| ) | |
| if company: | |
| query = query.where(LoyaltyPointEntry.company == company) | |
| if not include_expired_entry: | |
| # include non-expiring entries too | |
| query = query.where( | |
| (LoyaltyPointEntry.expiry_date >= expiry_date) | |
| | (LoyaltyPointEntry.expiry_date.isnull()) | |
| ) | |
| loyalty_point_details = query.run(as_dict=True) |
| frappe.db | ||
| .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop") | ||
| .then((r) => { | ||
| if (!r.exc) { | ||
| me.frm.set_default_payment = r.message.set_grand_total_to_default_mop; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Guard against missing POS Profile and null r.message to avoid runtime errors
If pos_profile is unset or get_value returns no record, r.message will be undefined and r.message.set_grand_total_to_default_mop access will throw. Add a guard and bail early when pos_profile is not available.
Apply this diff:
- frappe.db
- .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop")
- .then((r) => {
- if (!r.exc) {
- me.frm.set_default_payment = r.message.set_grand_total_to_default_mop;
- }
- });
+ if (this.frm.doc.pos_profile) {
+ frappe.db
+ .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop")
+ .then((r) => {
+ if (!r.exc && r.message) {
+ me.frm.set_default_payment = r.message.set_grand_total_to_default_mop;
+ }
+ });
+ }📝 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.
| frappe.db | |
| .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop") | |
| .then((r) => { | |
| if (!r.exc) { | |
| me.frm.set_default_payment = r.message.set_grand_total_to_default_mop; | |
| } | |
| }); | |
| if (this.frm.doc.pos_profile) { | |
| frappe.db | |
| .get_value("POS Profile", this.frm.doc.pos_profile, "set_grand_total_to_default_mop") | |
| .then((r) => { | |
| if (!r.exc && r.message) { | |
| me.frm.set_default_payment = r.message.set_grand_total_to_default_mop; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In erpnext/accounts/doctype/sales_invoice/sales_invoice.js around lines 61 to
67, the code calls frappe.db.get_value with this.frm.doc.pos_profile but does
not guard for a missing pos_profile or a missing r.message; accessing
r.message.set_grand_total_to_default_mop can throw. Add an early return if
this.frm.doc.pos_profile is falsy before calling get_value, and after the
promise resolves check that r and r.message are defined before reading
set_grand_total_to_default_mop; only assign me.frm.set_default_payment when the
property exists (otherwise leave it unchanged or set a safe default).
| def get_asset_value_adjustment_map_by_category(filters): | ||
| asset_value_adjustments = frappe.db.sql( | ||
| """ | ||
| SELECT | ||
| a.asset_category AS asset_category, | ||
| IFNULL( | ||
| SUM( | ||
| CASE | ||
| WHEN gle.posting_date < %(from_date)s | ||
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s) | ||
| THEN gle.debit - gle.credit | ||
| ELSE 0 | ||
| END | ||
| ), | ||
| 0) AS value_adjustment_before_from_date, | ||
| IFNULL( | ||
| SUM( | ||
| CASE | ||
| WHEN gle.posting_date <= %(to_date)s | ||
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(to_date)s) | ||
| THEN gle.debit - gle.credit | ||
| ELSE 0 | ||
| END | ||
| ), | ||
| 0) AS value_adjustment_till_to_date | ||
|
|
||
| FROM `tabGL Entry` gle | ||
| JOIN `tabAsset` a ON gle.against_voucher = a.name | ||
| JOIN `tabAsset Category Account` aca | ||
| ON aca.parent = a.asset_category | ||
| AND aca.company_name = %(company)s | ||
| WHERE gle.is_cancelled = 0 | ||
| AND a.docstatus = 1 | ||
| AND a.company = %(company)s | ||
| AND a.purchase_date <= %(to_date)s | ||
| AND gle.account = aca.fixed_asset_account | ||
| GROUP BY a.asset_category | ||
| """, | ||
| {"from_date": filters.from_date, "to_date": filters.to_date, "company": filters.company}, | ||
| as_dict=1, | ||
| ) | ||
|
|
||
| category_value_adjustment_map = {} | ||
|
|
||
| for r in asset_value_adjustments: | ||
| category_value_adjustment_map[r["asset_category"]] = { | ||
| "adjustment_before_from_date": flt(r.get("value_adjustment_before_from_date", 0)), | ||
| "adjustment_till_to_date": flt(r.get("value_adjustment_till_to_date", 0)), | ||
| } | ||
|
|
||
| return category_value_adjustment_map | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix disposal-date guard and add finance_book gating for value adjustments.
Two issues in the SQL:
- value_adjustment_till_to_date excludes all adjustments for assets disposed before to_date, even those posted before disposal. This understates adjustment_during_period for disposed assets.
- finance_book filtering is missing, while other queries in this report honor it. This can produce inconsistent results when a finance_book filter is applied.
Proposed fix:
- Bound “till” by disposal_date when present: include entries up to LEAST(to_date, disposal_date).
- Honor finance_book when provided and gate assets similarly to other queries.
Apply this diff:
def get_asset_value_adjustment_map_by_category(filters):
- asset_value_adjustments = frappe.db.sql(
- """
+ finance_book_filter = ""
+ exists_condition = ""
+ if filters.get("finance_book"):
+ finance_book_filter = " and ifnull(gle.finance_book, '')=%(finance_book)s"
+ exists_condition = (
+ " and exists (select 1 from `tabAsset Depreciation Schedule` ads "
+ "where ads.asset = a.name and ads.finance_book = %(finance_book)s)"
+ )
+
+ asset_value_adjustments = frappe.db.sql(
+ f"""
SELECT
a.asset_category AS asset_category,
IFNULL(
SUM(
CASE
WHEN gle.posting_date < %(from_date)s
AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s)
THEN gle.debit - gle.credit
ELSE 0
END
),
0) AS value_adjustment_before_from_date,
IFNULL(
SUM(
CASE
- WHEN gle.posting_date <= %(to_date)s
- AND (a.disposal_date IS NULL OR a.disposal_date >= %(to_date)s)
+ WHEN gle.posting_date <= %(to_date)s
+ AND (a.disposal_date IS NULL OR gle.posting_date <= a.disposal_date)
THEN gle.debit - gle.credit
ELSE 0
END
),
0) AS value_adjustment_till_to_date
FROM `tabGL Entry` gle
JOIN `tabAsset` a ON gle.against_voucher = a.name
JOIN `tabAsset Category Account` aca
ON aca.parent = a.asset_category
AND aca.company_name = %(company)s
WHERE gle.is_cancelled = 0
AND a.docstatus = 1
AND a.company = %(company)s
AND a.purchase_date <= %(to_date)s
AND gle.account = aca.fixed_asset_account
- GROUP BY a.asset_category
- """,
- {"from_date": filters.from_date, "to_date": filters.to_date, "company": filters.company},
+ {exists_condition}
+ {finance_book_filter}
+ GROUP BY a.asset_category
+ """,
+ {
+ "from_date": filters.from_date,
+ "to_date": filters.to_date,
+ "company": filters.company,
+ "finance_book": filters.get("finance_book"),
+ },
as_dict=1,
)📝 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.
| def get_asset_value_adjustment_map_by_category(filters): | |
| asset_value_adjustments = frappe.db.sql( | |
| """ | |
| SELECT | |
| a.asset_category AS asset_category, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date < %(from_date)s | |
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_before_from_date, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date <= %(to_date)s | |
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(to_date)s) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_till_to_date | |
| FROM `tabGL Entry` gle | |
| JOIN `tabAsset` a ON gle.against_voucher = a.name | |
| JOIN `tabAsset Category Account` aca | |
| ON aca.parent = a.asset_category | |
| AND aca.company_name = %(company)s | |
| WHERE gle.is_cancelled = 0 | |
| AND a.docstatus = 1 | |
| AND a.company = %(company)s | |
| AND a.purchase_date <= %(to_date)s | |
| AND gle.account = aca.fixed_asset_account | |
| GROUP BY a.asset_category | |
| """, | |
| {"from_date": filters.from_date, "to_date": filters.to_date, "company": filters.company}, | |
| as_dict=1, | |
| ) | |
| category_value_adjustment_map = {} | |
| for r in asset_value_adjustments: | |
| category_value_adjustment_map[r["asset_category"]] = { | |
| "adjustment_before_from_date": flt(r.get("value_adjustment_before_from_date", 0)), | |
| "adjustment_till_to_date": flt(r.get("value_adjustment_till_to_date", 0)), | |
| } | |
| return category_value_adjustment_map | |
| def get_asset_value_adjustment_map_by_category(filters): | |
| # Gate by finance_book if provided | |
| finance_book_filter = "" | |
| exists_condition = "" | |
| if filters.get("finance_book"): | |
| finance_book_filter = " and ifnull(gle.finance_book, '')=%(finance_book)s" | |
| exists_condition = ( | |
| " and exists (select 1 from `tabAsset Depreciation Schedule` ads " | |
| "where ads.asset = a.name and ads.finance_book = %(finance_book)s)" | |
| ) | |
| asset_value_adjustments = frappe.db.sql( | |
| f""" | |
| SELECT | |
| a.asset_category AS asset_category, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date < %(from_date)s | |
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_before_from_date, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date <= %(to_date)s | |
| AND (a.disposal_date IS NULL OR gle.posting_date <= a.disposal_date) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_till_to_date | |
| FROM `tabGL Entry` gle | |
| JOIN `tabAsset` a ON gle.against_voucher = a.name | |
| JOIN `tabAsset Category Account` aca | |
| ON aca.parent = a.asset_category | |
| AND aca.company_name = %(company)s | |
| WHERE gle.is_cancelled = 0 | |
| AND a.docstatus = 1 | |
| AND a.company = %(company)s | |
| AND a.purchase_date <= %(to_date)s | |
| AND gle.account = aca.fixed_asset_account | |
| {exists_condition} | |
| {finance_book_filter} | |
| GROUP BY a.asset_category | |
| """, | |
| { | |
| "from_date": filters.from_date, | |
| "to_date": filters.to_date, | |
| "company": filters.company, | |
| "finance_book": filters.get("finance_book"), | |
| }, | |
| as_dict=1, | |
| ) | |
| category_value_adjustment_map = {} | |
| for r in asset_value_adjustments: | |
| category_value_adjustment_map[r["asset_category"]] = { | |
| "adjustment_before_from_date": flt(r.get("value_adjustment_before_from_date", 0)), | |
| "adjustment_till_to_date": flt(r.get("value_adjustment_till_to_date", 0)), | |
| } | |
| return category_value_adjustment_map |
| def get_asset_value_adjustment_map(filters): | ||
| asset_with_value_adjustments = frappe.db.sql( | ||
| """ | ||
| SELECT | ||
| a.name AS asset, | ||
| IFNULL( | ||
| SUM( | ||
| CASE | ||
| WHEN gle.posting_date < %(from_date)s | ||
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s) | ||
| THEN gle.debit - gle.credit | ||
| ELSE 0 | ||
| END | ||
| ), | ||
| 0) AS value_adjustment_before_from_date, | ||
| IFNULL( | ||
| SUM( | ||
| CASE | ||
| WHEN gle.posting_date <= %(to_date)s | ||
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(to_date)s) | ||
| THEN gle.debit - gle.credit | ||
| ELSE 0 | ||
| END | ||
| ), | ||
| 0) AS value_adjustment_till_to_date | ||
|
|
||
| FROM `tabGL Entry` gle | ||
| JOIN `tabAsset` a ON gle.against_voucher = a.name | ||
| JOIN `tabAsset Category Account` aca | ||
| ON aca.parent = a.asset_category | ||
| AND aca.company_name = %(company)s | ||
| WHERE gle.is_cancelled = 0 | ||
| AND a.docstatus = 1 | ||
| AND a.company = %(company)s | ||
| AND a.purchase_date <= %(to_date)s | ||
| AND gle.account = aca.fixed_asset_account | ||
| GROUP BY a.name | ||
| """, | ||
| {"from_date": filters.from_date, "to_date": filters.to_date, "company": filters.company}, | ||
| as_dict=1, | ||
| ) | ||
|
|
||
| asset_value_adjustment_map = {} | ||
|
|
||
| for r in asset_with_value_adjustments: | ||
| asset_value_adjustment_map[r["asset"]] = { | ||
| "adjustment_before_from_date": flt(r.get("value_adjustment_before_from_date", 0)), | ||
| "adjustment_till_to_date": flt(r.get("value_adjustment_till_to_date", 0)), | ||
| } | ||
|
|
||
| return asset_value_adjustment_map | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the disposal-date and finance_book fixes for per-asset adjustments.
Same concerns as the category-level function:
- Include adjustments up to disposal_date (if any) rather than excluding disposed assets from the “till” bucket.
- Honor finance_book consistently.
Apply this diff:
def get_asset_value_adjustment_map(filters):
- asset_with_value_adjustments = frappe.db.sql(
- """
+ finance_book_filter = ""
+ exists_condition = ""
+ if filters.get("finance_book"):
+ finance_book_filter = " and ifnull(gle.finance_book, '')=%(finance_book)s"
+ exists_condition = (
+ " and exists (select 1 from `tabAsset Depreciation Schedule` ads "
+ "where ads.asset = a.name and ads.finance_book = %(finance_book)s)"
+ )
+
+ asset_with_value_adjustments = frappe.db.sql(
+ f"""
SELECT
a.name AS asset,
IFNULL(
SUM(
CASE
WHEN gle.posting_date < %(from_date)s
AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s)
THEN gle.debit - gle.credit
ELSE 0
END
),
0) AS value_adjustment_before_from_date,
IFNULL(
SUM(
CASE
- WHEN gle.posting_date <= %(to_date)s
- AND (a.disposal_date IS NULL OR a.disposal_date >= %(to_date)s)
+ WHEN gle.posting_date <= %(to_date)s
+ AND (a.disposal_date IS NULL OR gle.posting_date <= a.disposal_date)
THEN gle.debit - gle.credit
ELSE 0
END
),
0) AS value_adjustment_till_to_date
FROM `tabGL Entry` gle
JOIN `tabAsset` a ON gle.against_voucher = a.name
JOIN `tabAsset Category Account` aca
ON aca.parent = a.asset_category
AND aca.company_name = %(company)s
WHERE gle.is_cancelled = 0
AND a.docstatus = 1
AND a.company = %(company)s
AND a.purchase_date <= %(to_date)s
AND gle.account = aca.fixed_asset_account
- GROUP BY a.name
- """,
- {"from_date": filters.from_date, "to_date": filters.to_date, "company": filters.company},
+ {exists_condition}
+ {finance_book_filter}
+ GROUP BY a.name
+ """,
+ {
+ "from_date": filters.from_date,
+ "to_date": filters.to_date,
+ "company": filters.company,
+ "finance_book": filters.get("finance_book"),
+ },
as_dict=1,
)📝 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.
| def get_asset_value_adjustment_map(filters): | |
| asset_with_value_adjustments = frappe.db.sql( | |
| """ | |
| SELECT | |
| a.name AS asset, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date < %(from_date)s | |
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_before_from_date, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date <= %(to_date)s | |
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(to_date)s) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_till_to_date | |
| FROM `tabGL Entry` gle | |
| JOIN `tabAsset` a ON gle.against_voucher = a.name | |
| JOIN `tabAsset Category Account` aca | |
| ON aca.parent = a.asset_category | |
| AND aca.company_name = %(company)s | |
| WHERE gle.is_cancelled = 0 | |
| AND a.docstatus = 1 | |
| AND a.company = %(company)s | |
| AND a.purchase_date <= %(to_date)s | |
| AND gle.account = aca.fixed_asset_account | |
| GROUP BY a.name | |
| """, | |
| {"from_date": filters.from_date, "to_date": filters.to_date, "company": filters.company}, | |
| as_dict=1, | |
| ) | |
| asset_value_adjustment_map = {} | |
| for r in asset_with_value_adjustments: | |
| asset_value_adjustment_map[r["asset"]] = { | |
| "adjustment_before_from_date": flt(r.get("value_adjustment_before_from_date", 0)), | |
| "adjustment_till_to_date": flt(r.get("value_adjustment_till_to_date", 0)), | |
| } | |
| return asset_value_adjustment_map | |
| def get_asset_value_adjustment_map(filters): | |
| finance_book_filter = "" | |
| exists_condition = "" | |
| if filters.get("finance_book"): | |
| finance_book_filter = " and ifnull(gle.finance_book, '')=%(finance_book)s" | |
| exists_condition = ( | |
| " and exists (select 1 from `tabAsset Depreciation Schedule` ads " | |
| "where ads.asset = a.name and ads.finance_book = %(finance_book)s)" | |
| ) | |
| asset_with_value_adjustments = frappe.db.sql( | |
| f""" | |
| SELECT | |
| a.name AS asset, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date < %(from_date)s | |
| AND (a.disposal_date IS NULL OR a.disposal_date >= %(from_date)s) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_before_from_date, | |
| IFNULL( | |
| SUM( | |
| CASE | |
| WHEN gle.posting_date <= %(to_date)s | |
| AND (a.disposal_date IS NULL OR gle.posting_date <= a.disposal_date) | |
| THEN gle.debit - gle.credit | |
| ELSE 0 | |
| END | |
| ), | |
| 0) AS value_adjustment_till_to_date | |
| FROM `tabGL Entry` gle | |
| JOIN `tabAsset` a ON gle.against_voucher = a.name | |
| JOIN `tabAsset Category Account` aca | |
| ON aca.parent = a.asset_category | |
| AND aca.company_name = %(company)s | |
| WHERE gle.is_cancelled = 0 | |
| AND a.docstatus = 1 | |
| AND a.company = %(company)s | |
| AND a.purchase_date <= %(to_date)s | |
| AND gle.account = aca.fixed_asset_account | |
| {exists_condition} | |
| {finance_book_filter} | |
| GROUP BY a.name | |
| """, | |
| { | |
| "from_date": filters.from_date, | |
| "to_date": filters.to_date, | |
| "company": filters.company, | |
| "finance_book": filters.get("finance_book"), | |
| }, | |
| as_dict=1, | |
| ) | |
| asset_value_adjustment_map = {} | |
| for r in asset_with_value_adjustments: | |
| asset_value_adjustment_map[r["asset"]] = { | |
| "adjustment_before_from_date": flt(r.get("value_adjustment_before_from_date", 0)), | |
| "adjustment_till_to_date": flt(r.get("value_adjustment_till_to_date", 0)), | |
| } | |
| return asset_value_adjustment_map |
🤖 Prompt for AI Agents
In
erpnext/accounts/report/asset_depreciations_and_balances/asset_depreciations_and_balances.py
around lines 510 to 561, the per-asset SQL currently excludes disposed assets
from the "till" bucket and ignores the finance_book filter; update the query to
honor finance_book and to include value adjustments up to an asset's
disposal_date by (1) joining tabAsset Finance Book (alias afb) ON afb.parent =
a.name AND afb.finance_book = %(finance_book)s and adding %(finance_book)s to
the query params, and (2) change the CASE conditions so that the "till"
aggregation requires gle.posting_date <= %(to_date)s AND gle.posting_date <=
COALESCE(a.disposal_date, %(to_date)s) (similarly ensure the "before"
aggregation only counts entries with gle.posting_date < %(from_date)s AND
gle.posting_date <= COALESCE(a.disposal_date, %(from_date)s)); return the same
map structure but now filtered by finance_book and respecting disposal_date when
computing the two buckets.
| row = d.as_dict(no_private_properties=True) | ||
| row.pop("name") | ||
| return row |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good sanitization shift; make pop safe and confirm framework support for the flag
Switching to as_dict(no_private_properties=True) is a sensible improvement. Two follow-ups:
- Use a safe pop to avoid surprises if
nameisn’t present. - Verify the installed Frappe version supports the
no_private_propertieskwarg to prevent a runtime TypeError.
Apply this diff for the safe pop:
- row = d.as_dict(no_private_properties=True)
- row.pop("name")
+ row = d.as_dict(no_private_properties=True)
+ row.pop("name", None)To confirm framework support for the flag:
🌐 Web query:
Does the Frappe Document.as_dict method support the `no_private_properties` argument (and from which version)?
💡 Result:
Short answer: No — there is no documented / repo-backed argument named no_private_properties on the core frappe.model.Document.as_dict. It is not part of the public API in the docs, and a code search of the Frappe repo does not show that name.
Details and sources
- The official Document API/docs show as_dict used without that argument (doc.as_dict()) and do not document a no_private_properties parameter. (docs.frappe.io)
- In the Frappe codebase some DocTypes override as_dict (for example Email Queue uses as_dict(include_recipients=False)), which shows custom kwargs are possible on per-doctype implementations — but that is not the same as a core Document.as_dict argument named no_private_properties. (github.com)
If you want, I can:
- search a particular Frappe tag/branch (for example v13, v14, v15 or develop) and show the exact as_dict signature / implementation and any PR that introduced a parameter, or
- give a short snippet you can use to remove private keys (keys starting with "_") from doc.as_dict() output.
Which would you prefer?
Citations:
- 1: https://docs.frappe.io/framework/v13/user/en/basics/doctypes/controllers?utm_source=chatgpt.com
- 2: https://github.com/frappe/frappe/issues/26669?utm_source=chatgpt.com
Remove private properties manually and use safe pop
The no_private_properties argument isn’t supported by the core Document.as_dict. Instead:
- Call
as_dict()without extra kwargs. - Strip out keys beginning with
_yourself. - Use
pop("name", None)to avoid KeyError if"name"is missing.
Apply this diff:
- row = d.as_dict(no_private_properties=True)
- row.pop("name")
+ # get full dict, then remove private fields
+ row = d.as_dict()
+ row = {k: v for k, v in row.items() if not k.startswith("_")}
+ row.pop("name", None)📝 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.
| row = d.as_dict(no_private_properties=True) | |
| row.pop("name") | |
| return row | |
| - row = d.as_dict(no_private_properties=True) | |
| # get full dict, then remove private fields | |
| row = d.as_dict() | |
| row = {k: v for k, v in row.items() if not k.startswith("_")} | |
| row.pop("name", None) | |
| return row |
🤖 Prompt for AI Agents
In erpnext/setup/doctype/item_group/item_group.py around lines 93 to 95, the
code calls d.as_dict(no_private_properties=True) and then row.pop("name") which
is invalid because Document.as_dict doesn't accept that kwarg and pop may raise
KeyError; change to call d.as_dict() with no extra kwargs, remove any keys that
start with "_" from the returned dict (e.g., iterate over list(row.keys()) and
delete keys beginning with "_"), and use row.pop("name", None) to safely remove
the name key without raising an error.
| customer = create_internal_customer( | ||
| "_Test Internal Customer 3", | ||
| "_Test Internal Customer 2", | ||
| company, | ||
| company, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Change to use “_Test Internal Customer 2” looks good; aligns with common fixtures.
This should reduce fixture divergence and improve test idempotency where “2” is the established internal customer in other tests.
Run this quick scan to ensure no remaining references to “_Test Internal Customer 3” linger elsewhere:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 -F '_Test Internal Customer 3' || trueLength of output: 2054
Inconsistent Internal Customer Fixture References Detected
The quick scan reveals lingering uses of “_Test Internal Customer 3” in the sales invoice tests. To maintain consistency and idempotency, please update all of these to “_Test Internal Customer 2”:
• erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py:78
• …/test_sales_invoice.py:2743
• …/test_sales_invoice.py:4948
• …/test_sales_invoice.py:4974
Once updated, rerun your scan to confirm no “_Test Internal Customer 3” references remain.
🤖 Prompt for AI Agents
In erpnext/stock/doctype/inventory_dimension/test_inventory_dimension.py around
lines 669 to 673, the internal customer fixture is created as "_Test Internal
Customer 2" but other sales invoice tests still reference "_Test Internal
Customer 3"; update all lingering references in the listed files
(erpnext/accounts/doctype/sales_invoice/test_sales_invoice.py at lines near 78,
2743, 4948, 4974) to "_Test Internal Customer 2" so fixtures are consistent and
idempotent, then run the test/scan suite to verify no "_Test Internal Customer
3" occurrences remain.
| mr = frappe.qb.DocType("Material Request") | ||
| mr_item = frappe.qb.DocType("Material Request Item") | ||
|
|
||
| query = ( | ||
| frappe.qb.from_(mr) | ||
| .from_(mr_item) | ||
| .select(mr.name) | ||
| .distinct() | ||
| .select(mr.transaction_date, mr.company) | ||
| .where( | ||
| (mr.name == mr_item.parent) | ||
| & (mr_item.item_code.isin(supplier_items)) | ||
| & (mr.material_request_type == "Purchase") | ||
| & (mr.per_ordered < 99.99) | ||
| & (mr.docstatus == 1) | ||
| & (mr.status != "Stopped") | ||
| & (mr.company == filters.get("company")) | ||
| ) | ||
| .orderby(mr_item.item_code, order=Order.asc) | ||
| .limit(cint(page_len)) | ||
| .offset(cint(start)) | ||
| ) | ||
|
|
||
| if txt: | ||
| query = query.where(mr.name.like(f"%%{txt}%%")) | ||
|
|
||
| if filters.get("transaction_date"): | ||
| date = filters.get("transaction_date")[1] | ||
| query = query.where(mr.transaction_date[date[0] : date[1]]) | ||
|
|
||
| material_requests = query.run(as_dict=True) | ||
|
|
||
| return material_requests |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix incorrect date-range filter and prefer an explicit JOIN
Two issues:
- The date filter uses slicing syntax
mr.transaction_date[date[0] : date[1]], which is not a valid Frappe QB predicate. Usebetween(from_date, to_date). - Prefer an explicit
JOINover a cross-join + where filter for clarity and query planning.
- mr = frappe.qb.DocType("Material Request")
- mr_item = frappe.qb.DocType("Material Request Item")
+ mr = frappe.qb.DocType("Material Request")
+ mr_item = frappe.qb.DocType("Material Request Item")
- query = (
- frappe.qb.from_(mr)
- .from_(mr_item)
+ query = (
+ frappe.qb.from_(mr)
+ .join(mr_item)
+ .on(mr.name == mr_item.parent)
.select(mr.name)
.distinct()
.select(mr.transaction_date, mr.company)
- .where(
- (mr.name == mr_item.parent)
- & (mr_item.item_code.isin(supplier_items))
+ .where(
+ (mr_item.item_code.isin(supplier_items))
& (mr.material_request_type == "Purchase")
& (mr.per_ordered < 99.99)
& (mr.docstatus == 1)
& (mr.status != "Stopped")
& (mr.company == filters.get("company"))
)
.orderby(mr_item.item_code, order=Order.asc)
.limit(cint(page_len))
.offset(cint(start))
)
if txt:
query = query.where(mr.name.like(f"%%{txt}%%"))
- if filters.get("transaction_date"):
- date = filters.get("transaction_date")[1]
- query = query.where(mr.transaction_date[date[0] : date[1]])
+ if filters.get("transaction_date"):
+ # Expecting filters["transaction_date"] like ["between", [from_date, to_date]]
+ _, date_range = filters.get("transaction_date")
+ from_date, to_date = date_range
+ query = query.where(mr.transaction_date.between(from_date, to_date))
material_requests = query.run(as_dict=True)To ensure no other instances use the invalid slice syntax on QB fields, you can scan the codebase:
🏁 Script executed:
#!/bin/bash
rg -nP --type=py -C2 '\.\w+\[[^\]]+:[^\]]+\]' | sed -n '1,200p'Length of output: 8533
Enforce valid date‐range filtering and use explicit JOINs in all QB queries
The Python slice syntax (e.g. field[from:to]) is not a supported Frappe QB predicate and must be replaced with the .between(start, end) method. Likewise, prefer .join(…) over cross-joins for better readability and query planning. Besides the Material Request snippet, the scan uncovered other occurrences:
• erpnext/stock/doctype/material_request/material_request.py (Lines 634–666)
– use
diff - query = ( - frappe.qb.from_(mr) - .from_(mr_item) + query = ( + frappe.qb.from_(mr) + .join(mr_item).on(mr.name == mr_item.parent) … - if filters.get("transaction_date"): - date = filters.get("transaction_date")[1] - query = query.where(mr.transaction_date[date[0] : date[1]]) + if filters.get("transaction_date"): + _, (from_date, to_date) = filters["transaction_date"] + query = query.where(mr.transaction_date.between(from_date, to_date))
• erpnext/stock/report/stock_ledger/stock_ledger.py (Line 396)
Replace
- .where(... & (sle.posting_datetime[from_date:to_date]))
+ .where(... & sle.posting_datetime.between(from_date, to_date))• erpnext/selling/report/payment_terms_status_for_sales_order/payment_terms_status_for_sales_order.py (Line 212)
Replace
- & (so.transaction_date[conditions.start_date : conditions.end_date])
+ & so.transaction_date.between(conditions.start_date, conditions.end_date)• erpnext/controllers/budget_controller.py (Lines 215, 246, 279)
Replace each
- po.transaction_date[self.fy_start_date : self.fy_end_date]
+ po.transaction_date.between(self.fy_start_date, self.fy_end_date)(and similarly for mr.transaction_date and gl.posting_date)
• erpnext/accounts/doctype/tax_withholding_category/tax_withholding_category.py (Lines 505, 674)
Replace
- ple.posting_date[from_date:to_date]
+ ple.posting_date.between(from_date, to_date)• erpnext/accounts/report/invalid_ledger_entries/invalid_ledger_entries.py (Lines 113, 128)
Replace
- gle.posting_date[filters.from_date : filters.to_date]
+ gle.posting_date.between(filters.from_date, filters.to_date)(and the identical slice on ple.posting_date)
Please apply these refactors throughout the codebase to ensure all QB date‐range filters and joins are valid and explicit.
📝 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.
| mr = frappe.qb.DocType("Material Request") | |
| mr_item = frappe.qb.DocType("Material Request Item") | |
| query = ( | |
| frappe.qb.from_(mr) | |
| .from_(mr_item) | |
| .select(mr.name) | |
| .distinct() | |
| .select(mr.transaction_date, mr.company) | |
| .where( | |
| (mr.name == mr_item.parent) | |
| & (mr_item.item_code.isin(supplier_items)) | |
| & (mr.material_request_type == "Purchase") | |
| & (mr.per_ordered < 99.99) | |
| & (mr.docstatus == 1) | |
| & (mr.status != "Stopped") | |
| & (mr.company == filters.get("company")) | |
| ) | |
| .orderby(mr_item.item_code, order=Order.asc) | |
| .limit(cint(page_len)) | |
| .offset(cint(start)) | |
| ) | |
| if txt: | |
| query = query.where(mr.name.like(f"%%{txt}%%")) | |
| if filters.get("transaction_date"): | |
| date = filters.get("transaction_date")[1] | |
| query = query.where(mr.transaction_date[date[0] : date[1]]) | |
| material_requests = query.run(as_dict=True) | |
| return material_requests | |
| mr = frappe.qb.DocType("Material Request") | |
| mr_item = frappe.qb.DocType("Material Request Item") | |
| query = ( | |
| frappe.qb.from_(mr) | |
| .join(mr_item).on(mr.name == mr_item.parent) | |
| .select(mr.name) | |
| .distinct() | |
| .select(mr.transaction_date, mr.company) | |
| .where( | |
| (mr.name == mr_item.parent) | |
| & (mr_item.item_code.isin(supplier_items)) | |
| & (mr.material_request_type == "Purchase") | |
| & (mr.per_ordered < 99.99) | |
| & (mr.docstatus == 1) | |
| & (mr.status != "Stopped") | |
| & (mr.company == filters.get("company")) | |
| ) | |
| .orderby(mr_item.item_code, order=Order.asc) | |
| .limit(cint(page_len)) | |
| .offset(cint(start)) | |
| ) | |
| if txt: | |
| query = query.where(mr.name.like(f"%%{txt}%%")) | |
| if filters.get("transaction_date"): | |
| _, (from_date, to_date) = filters["transaction_date"] | |
| query = query.where(mr.transaction_date.between(from_date, to_date)) | |
| material_requests = query.run(as_dict=True) | |
| return material_requests |
| "filter": lambda d: ( | ||
| get_pending_qty(d)[0] <= 0 if not doc.get("is_return") else get_pending_qty(d)[0] > 0 | ||
| ) | ||
| and select_item(d), | ||
| }, |
There was a problem hiding this comment.
Bug: pending-qty filter is inverted; normal flow should include items with pending_qty > 0
For non-return PRs, items with pending_qty > 0 should be mapped. The current <= 0 condition likely excludes the intended items. For returns, include negative pending_qty.
Apply this diff:
- "filter": lambda d: (
- get_pending_qty(d)[0] <= 0 if not doc.get("is_return") else get_pending_qty(d)[0] > 0
- )
- and select_item(d),
+ "filter": lambda d: select_item(d)
+ and (
+ get_pending_qty(d)[0] > 0
+ if not doc.get("is_return")
+ else get_pending_qty(d)[0] < 0
+ ),📝 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.
| "filter": lambda d: ( | |
| get_pending_qty(d)[0] <= 0 if not doc.get("is_return") else get_pending_qty(d)[0] > 0 | |
| ) | |
| and select_item(d), | |
| }, | |
| "filter": lambda d: select_item(d) | |
| and ( | |
| get_pending_qty(d)[0] > 0 | |
| if not doc.get("is_return") | |
| else get_pending_qty(d)[0] < 0 | |
| ), |
🤖 Prompt for AI Agents
In erpnext/stock/doctype/purchase_receipt/purchase_receipt.py around lines 1422
to 1426, the lambda filter for pending_qty is inverted: for non-return PRs it
currently uses <= 0 and for returns it uses > 0. Change the non-return branch to
require get_pending_qty(d)[0] > 0 and change the return branch to require
get_pending_qty(d)[0] < 0, preserving the trailing "and select_item(d)" so that
normal PRs include items with positive pending_qty and returns include negative
pending_qty.
| this.barcode_scanner = new erpnext.utils.BarcodeScanner({ | ||
| frm: this.frm, | ||
| warehouse_field: (doc) => { | ||
| return doc.purpose === "Material Transfer" ? "t_warehouse" : "s_warehouse"; | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix warehouse selection for Material Receipt scans (and guard against duplicate scanner init).
Routing all non-Transfer scans to s_warehouse will break Material Receipt (and other inbound) flows where only t_warehouse is relevant. Also, setup can be re-run in some edge navigations; guard scanner re-instantiation.
Apply this diff:
- this.barcode_scanner = new erpnext.utils.BarcodeScanner({
- frm: this.frm,
- warehouse_field: (doc) => {
- return doc.purpose === "Material Transfer" ? "t_warehouse" : "s_warehouse";
- },
- });
+ if (!this.barcode_scanner) {
+ this.barcode_scanner = new erpnext.utils.BarcodeScanner({
+ frm: this.frm,
+ warehouse_field: (doc) => {
+ // Inbound purposes should use target warehouse; outbound use source
+ const inboundPurposes = [
+ "Material Receipt",
+ "Manufacture",
+ "Repack",
+ "Send to Subcontractor",
+ "Disassemble",
+ ];
+ if (doc.purpose === "Material Transfer") return "t_warehouse";
+ return inboundPurposes.includes(doc.purpose) ? "t_warehouse" : "s_warehouse";
+ },
+ });
+ }📝 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.
| this.barcode_scanner = new erpnext.utils.BarcodeScanner({ | |
| frm: this.frm, | |
| warehouse_field: (doc) => { | |
| return doc.purpose === "Material Transfer" ? "t_warehouse" : "s_warehouse"; | |
| }, | |
| }); | |
| // Initialize barcode scanner once, with correct warehouse logic for inbound vs outbound | |
| if (!this.barcode_scanner) { | |
| this.barcode_scanner = new erpnext.utils.BarcodeScanner({ | |
| frm: this.frm, | |
| warehouse_field: (doc) => { | |
| // Inbound purposes should use target warehouse; outbound use source | |
| const inboundPurposes = [ | |
| "Material Receipt", | |
| "Manufacture", | |
| "Repack", | |
| "Send to Subcontractor", | |
| "Disassemble", | |
| ]; | |
| if (doc.purpose === "Material Transfer") return "t_warehouse"; | |
| return inboundPurposes.includes(doc.purpose) ? "t_warehouse" : "s_warehouse"; | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In erpnext/stock/doctype/stock_entry/stock_entry.js around lines 1006-1012, the
barcode scanner is always routing non-Transfer scans to s_warehouse and is
re-instantiated on repeated setup; change the warehouse_field logic to return
t_warehouse for inbound purposes (e.g., "Material Receipt") as well as for
"Material Transfer", otherwise use s_warehouse, and add a guard so you do not
recreate this.barcode_scanner if it already exists (e.g., if
this.barcode_scanner is truthy, skip creation or reuse the existing instance).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
erpnext/controllers/accounts_controller.py (2)
2586-2606: Ensure due_date alignment to invoice posting_date and set base_outstandingTwo gaps in the auto-fetch path:
- Computed due_date isn’t aligned to the invoice posting_date (as done elsewhere), which can yield a due date earlier than the invoice’s posting date when bill_date < posting_date.
- base_outstanding isn’t initialized; elsewhere it’s set to base_payment_amount for consistency.
Apply this diff to address both:
@@ - if schedule.due_date_based_on: - payment_schedule["due_date"] = get_due_date(schedule, posting_date) + if schedule.due_date_based_on: + payment_schedule["due_date"] = get_due_date(schedule, posting_date) + # Align computed due_date to invoice posting_date if it falls earlier + if self.get("posting_date") and getdate(payment_schedule["due_date"]) < getdate(self.get("posting_date")): + payment_schedule["due_date"] = self.posting_date @@ - payment_schedule["outstanding"] = payment_schedule["payment_amount"] + payment_schedule["outstanding"] = payment_schedule["payment_amount"] + payment_schedule["base_outstanding"] = payment_schedule["base_payment_amount"]
3429-3431: Guard againstNonewhen aligningdue_datetoposting_date
The comparison inerpnext/controllers/accounts_controller.py:3429can fail or misbehave if eitherposting_dateorterm_details.due_dateisNone(e.g. when callers supply onlybill_date). Add a simple guard to skip the check unless both dates are present:• File: erpnext/controllers/accounts_controller.py
Lines: 3429–3431- if getdate(term_details.due_date) < getdate(posting_date): - term_details.due_date = posting_date + if posting_date and term_details.due_date and getdate(term_details.due_date) < getdate(posting_date): + term_details.due_date = posting_date
♻️ Duplicate comments (1)
erpnext/controllers/accounts_controller.py (1)
2588-2591: Confirmed: fixed thedue_date_based_onfield assignment bugThe field now correctly copies
schedule.due_date_based_on(instead ofschedule.credit_days) as flagged earlier. Thanks for addressing.
🧹 Nitpick comments (2)
erpnext/controllers/accounts_controller.py (2)
3399-3416: Avoid duplicate assignment of payment_term; centralize fields to copyYou already set payment_term (Line 3398) and again include it in fields_to_copy. It’s harmless but redundant. Drop it from fields_to_copy for clarity.
fields_to_copy = [ "description", "invoice_portion", - "payment_term", "discount_type", "discount", "mode_of_payment", "due_date_based_on", "credit_days", "credit_months", "discount_validity_based_on", "discount_validity", ]
3435-3445: Safer fallback when due_date_based_on is unset or unrecognizedMinor hardening: default to the base date when due_date_based_on is unset/unknown so callers always get a usable date (instead of None) unless explicitly overridden by default_date.
-def get_due_date(term, posting_date=None, bill_date=None, default_date=None): - due_date = default_date - date = bill_date or posting_date +def get_due_date(term, posting_date=None, bill_date=None, default_date=None): + date = bill_date or posting_date + due_date = default_date or date if term.due_date_based_on == "Day(s) after invoice date": due_date = add_days(date, term.credit_days) elif term.due_date_based_on == "Day(s) after the end of the invoice month": due_date = add_days(get_last_day(date), term.credit_days) elif term.due_date_based_on == "Month(s) after the end of the invoice month": due_date = get_last_day(add_months(date, term.credit_months)) return due_date
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
erpnext/controllers/accounts_controller.py(4 hunks)erpnext/stock/doctype/delivery_note/test_delivery_note.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/stock/doctype/delivery_note/test_delivery_note.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
erpnext/controllers/accounts_controller.py (1)
erpnext/accounts/party.py (1)
get_due_date(625-646)
🪛 GitHub Actions: Linters
erpnext/controllers/accounts_controller.py
[warning] 1-1: Semgrep timeout error while running rule frappe-semgrep-rules.rules.frappe-monkey-patching-not-allowed. Current version has 159 findings.
⏰ 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). (1)
- GitHub Check: Patch Test
🔇 Additional comments (1)
erpnext/controllers/accounts_controller.py (1)
2573-2573: LGTM: Correct basis date selection for term computationsUsing bill_date → posting_date → transaction_date as the base is aligned with the PR intent and prevents stale due dates from orders.
…unt date handling
ljain112
force-pushed
the
fix-due-date-orders
branch
from
ba5c85d to
cac3dbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
erpnext/controllers/accounts_controller.py (2)
2586-2606: Set base_outstanding when auto-fetching terms to avoid inconsistent outstanding values.When
automatically_fetch_payment_termsis enabled and allocation is based on terms (the else-branch inset_payment_schedulere-fetches and does not normalize fields),base_outstandingmay remain unset on the appended rows. Set it alongsidebase_payment_amounthere.Apply this diff:
payment_schedule["payment_amount"] = flt( grand_total * flt(payment_schedule["invoice_portion"]) / 100, schedule.precision("payment_amount"), ) payment_schedule["base_payment_amount"] = flt( base_grand_total * flt(payment_schedule["invoice_portion"]) / 100, schedule.precision("base_payment_amount"), ) + payment_schedule["base_outstanding"] = payment_schedule["base_payment_amount"] payment_schedule["outstanding"] = payment_schedule["payment_amount"]
3422-3431: Guard against unintended due-date bump when only bill_date is provided.Line 3429 compares
due_dateagainstposting_dateunconditionally. Ifposting_dateis None,getdate(posting_date)resolves to today, which can incorrectly advance due dates computed frombill_date.Add a guard so this alignment only happens when
posting_dateis actually provided.Proposed adjustment (outside the changed lines) for clarity:
# Only align when posting_date is provided if posting_date and term_details.due_date and getdate(term_details.due_date) < getdate(posting_date): term_details.due_date = posting_date
♻️ Duplicate comments (1)
erpnext/controllers/accounts_controller.py (1)
2588-2588: Good fix: Correctly copyingdue_date_based_on.Assignment now uses
schedule.due_date_based_on(wascredit_daysearlier). This aligns with the PR objective and unblocks correct due-date computation.
🧹 Nitpick comments (3)
erpnext/controllers/accounts_controller.py (3)
2573-2573: Use a neutral variable name for the computed reference date.This local variable can be a bill date, posting date, or transaction date. Naming it
reference_datewould better reflect its role and reduce confusion when passed into due/discount-date calculators.
3400-3416: Minor duplication:payment_termis assigned twice.It’s set in the
else:branch above and also included infields_to_copy. Keeping just one source reduces redundancy. Either remove theelse:assignment or droppayment_termfromfields_to_copy(but not both).
3435-3444: get_due_date: Provide a safe fallback when rules are missing.If
term.due_date_based_onis empty/unrecognized, the function currently returnsdefault_date(often None). Fallback to the computeddateto avoid returning None and silently breaking downstream validations.Apply this diff:
def get_due_date(term, posting_date=None, bill_date=None, default_date=None): due_date = default_date date = bill_date or posting_date if term.due_date_based_on == "Day(s) after invoice date": due_date = add_days(date, term.credit_days) elif term.due_date_based_on == "Day(s) after the end of the invoice month": due_date = add_days(get_last_day(date), term.credit_days) elif term.due_date_based_on == "Month(s) after the end of the invoice month": due_date = get_last_day(add_months(date, term.credit_months)) + if not due_date: + due_date = date return due_date
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
erpnext/controllers/accounts_controller.py(4 hunks)erpnext/stock/doctype/delivery_note/test_delivery_note.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/stock/doctype/delivery_note/test_delivery_note.py
⏰ 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). (1)
- GitHub Check: Patch Test
🔇 Additional comments (1)
erpnext/controllers/accounts_controller.py (1)
3400-3416: LGTM: Propagating term metadata to payment schedule is correct.Copying
due_date_based_on,credit_days,credit_months,discount_validity_based_on, anddiscount_validitymakes the invoice self-descriptive and supports recomputation on mapping.
…cy handling in transaction callback
ljain112
force-pushed
the
fix-due-date-orders
branch
from
76768cf to
938819e
Compare
18 participants
The due date on the payment schedule should be dynamically derived based on the payment terms (credit days) on the PO and Bill date or the posting date of the invoice. Currently, it's just copying whatever due date is on the PO/SO schedule. For blanket orders, the PO / SO date could be sometime in past (say 1st Jan), and the due date could be 31st Jan (30 days). However, when the invoice is submitted on 1st Mar, the system should calculate the due date as 31st Mar (1st + 30 days) instead of updating the due date as 30th Jan (doing the latter marks the invoice as overdue on submit).
New behaviour:
Storing fields at the invoice level because they are also present in the Payment terms template.
fields will be read only
If the due_date or discount_date is updated after auto-setting, fields will be set as empty.
On mapping, if automatically_fetch_payment_terms is enabled, then due_date and discount_date will be recalculated.
no-docs
Further enhancement that can be done:
Summary by CodeRabbit
New Features
Bug Fixes
Tests