Address review on #56090: get_bom_data groups components by item_code, so it
picks one representative BOM Item line for the (bom_no, is_phantom_item) pair.
Taking the first line by idx dropped the phantom flag when a non-phantom line
was listed before the phantom one, so explode_phantom_boms skipped the sub-BOM.
Keep one row per item_code (preserving the qty_per_unit total per component
rather than widening the GROUP BY), but make the representative phantom-
preferring: the first line, upgraded to the first phantom line if any exists.
A phantom sub-BOM is therefore never dropped due to line order, on either engine.
Adds test_phantom_explosion_when_phantom_line_is_not_first (phantom line at
idx 2) alongside the existing idx-1 case; both pass on MariaDB and Postgres and
the new one fails on the naive first-line representative.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a component is listed more than once in a BOM pointing at different
sub-BOMs (e.g. one phantom, one not), two queries grouped the duplicate
lines into a single row and aggregated bom_no and is_phantom_item with
*independent* Max(). That could pair the phantom flag of one line with the
bom_no of another, so the consumer recursed into the wrong sub-BOM:
- sub_assembly_queries._sub_assembly_rm_query keys on (item_code, bom_no)
and recurses on is_phantom_item. An incoherent pair sent raw-material
resolution down the wrong sub-assembly BOM.
- bom_stock_analysis.get_bom_data: explode_phantom_boms recurses into
bom_no only when is_phantom_item is set; an incoherent pair exploded a
non-phantom sub-BOM as if it were phantom (or vice-versa).
Fix:
- sub_assembly_queries: group also by (bom_no, is_phantom_item) so each
distinct sub-BOM is its own coherent row.
- bom_stock_analysis: drop the two independent Max()es and attach a single
representative line (lowest idx) per item_code before exploding.
This was previously undefined SQL (loose GROUP BY); the fix makes MariaDB
and Postgres agree on a deterministic, coherent pairing. Other Max()-wrapped
columns are functionally dependent on the grouped item and keep their value
on both engines.
Tests (fail on the old code, pass on both engines):
- test_phantom_explosion_picks_coherent_sub_bom: duplicate-component BOM
explodes the phantom sub-BOM, not the lexically-larger non-phantom one.
- test_sub_assembly_rm_query_keeps_bom_no_phantom_pair_coherent: the query
returns one coherent row per distinct sub-BOM with the right phantom flag.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
get_future_payments_from_journal_entry has no GROUP BY: the Sum() makes it an
implicit single-group aggregate, so every future-dated JE payment company-wide
collapses into ONE row keyed by an arbitrary (invoice, party). The Accounts
Receivable report then allocates the entire future sum against that one invoice and
shows zero future payment for every other invoice. This predates the postgres work
(MariaDB returned an arbitrary single row); the qb conversion only made the arbitrary
pick deterministic via Max().
Add an explicit GROUP BY (je.name, jea.reference_name, jea.party, jea.party_type,
je.posting_date, je.cheque_no) and drop the Max() wrappers, so each (JE, invoice,
party) is its own future-payment row -- matching the Payment Entry path and the
(invoice_no, party) keying the report's allocator already expects. Identical on
MariaDB and PostgreSQL.
Ships a JE-path future-payment test (one future JE paying two invoices -> each
invoice keeps its own future amount; fails on the old code with 0 != 50).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
stock_delivered_but_not_billed_gl_entries (C/15) is split into a thin loop
plus _sdbnb_booking_for_item (eligibility + valuation), _is_sdbnb_reversal
(the account predicate) and _append_sdbnb_gl_entries (the two GL rows) —
guard clauses replace the nested continues. get_gle_for_change_amount now
uses the shared _amount_in_account_currency helper. No C-rank functions
remain in the file. Behaviour unchanged; characterization tests and the
full Sales Invoice suite (133) green.
Three Query Reports embedded double-quoted string literals and an unquoted table
identifier that error on PostgreSQL. Portability-only, no behaviour change on either engine:
- material_requests_for_which_supplier_quotations_are_not_created,
requested_items_to_be_transferred: double-quoted string literals ("Stopped",
"Material Transfer") -> single quotes (double quotes are identifiers on postgres, not strings).
- items_to_be_requested: quote the `tabBin` table identifier so postgres doesn't lower-case it.
(received_items_to_be_billed was dropped: it is a Script Report, so its `query` field is dead
code and the fix never reaches the DB.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pin the two least-covered SalesInvoiceGLComposer methods before refactor:
- stock_delivered_but_not_billed_gl_entries: billing a perpetual Delivery
Note via Sales Invoice reverses the SDBNB account into COGS for an equal
amount.
- get_gle_for_change_amount: empty without change, mandatory-account error,
and the debit-to / change-account entry pair.
Extract two cross-cutting helpers used across the GL methods:
- _amount_in_account_currency: the repeated 'base if account is in company
currency else transaction amount' ternary (customer, tax, item, POS and
write-off entries).
- _return_aware_against_voucher: the return/self-outstanding against_voucher
rule duplicated in the customer and POS entries.
Pull the per-item income and discount rows into their own builders and
flatten make_item_gl_entries with a guard clause. Complexity drops:
make_customer C11->B7, make_pos C12->B7, make_item C14->B10,
make_discount C13->B10, make_tax->A3. No behaviour change; entry dicts and
amounts are identical (full Sales Invoice suite green).
stock_delivered_but_not_billed_gl_entries is left for a coverage-first pass
(it is the least-tested GL branch).
get_warehouse could never run: it filtered POS Profile on a non-existent
'user' column (users live in the applicable_for_users child table), and
embedded a Python bool (frappe.session["user"] == "") inside a query
builder predicate, which raises before reaching the database. It also
has no callers. Remove it and the now-unused msgprint import.
3-way merged onto develop (preserving the get_party_bank_account import move).
get_subscription_details passes order_by="" so get_all does not inject the
doctype default sort the raw query never had.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3-way merged onto develop, preserving develop's set_exchange_rate(ref_doc=doc) change.
One portable raw query is intentionally kept (as on the source branch).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The raw get_fiscal_years query had no ORDER BY (de-facto oldest-first); the
get_all port adds explicit order_by="name asc" so the Fiscal Year doctype
default (name DESC) does not reverse the report column order / cumulative values.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
