Greptile flagged that the sales-side zero-qty-return fix had no dedicated
test proving the behavior - the existing suite happened to pass, but
nothing specifically asserted that an all-zero return is rejected while
a normal negative-qty return still succeeds.
Adds two tests covering the doctypes that rely entirely on this check
(no other guard covers them for a non-stock-effect return):
- Delivery Note return with qty 0 -> rejected
- Sales Invoice return with qty 0 (no update_stock) -> rejected
POS Invoice is not covered separately here since it always runs with
update_stock=1, which is already guarded by the pre-existing
validate_zero_qty_for_return_invoices_with_stock check regardless of
this fix.
validate_returned_items() set items_returned=True whenever a row matched
a valid item from the original document, even if its qty was 0. This let
a Sales Invoice, Delivery Note, or POS Invoice return be submitted with
every line at qty=0 - a no-op document with no stock or financial effect
that still consumed a document number and linked back to the original
transaction.
Scoped to the Sales side only: items_returned now flips to True for
Sales Invoice/Delivery Note/POS Invoice only when qty (or received_qty)
is actually negative, so an all-zero sales return correctly hits the
existing "At least one item should be entered with negative quantity"
check. Purchase Invoice, Purchase Receipt, and Subcontracting Receipt
are unchanged.
* fix(controllers): source trend report labels from the master
item_name, customer_name, territory and supplier_name are stored on each
transaction and editable, so they are not functionally dependent on the grouped
key and historical documents can hold different values for the same item,
customer or supplier. Aggregating them with Max() is a text sort, and MariaDB
folds case while PostgreSQL orders by byte value, so the two engines can label
the same row differently.
Read each from its master instead. Those values ARE dependent on the grouped
key, so they can be grouped without splitting rows and agree on both engines by
construction rather than by an assumption about the data. Supplier needed no new
join -- the Supplier master was already joined as t3 for supplier_group.
A Quotation's party_name is a dynamic link to either a Customer or a Lead, so
neither master can be joined without dropping the other; there the values come
from correlated subqueries over both, keyed only on the grouped party_name.
Row counts and every numeric total are unchanged. What changes is that a
renamed record now shows its current name rather than whichever historical
snapshot happened to sort highest.
* test(selling): assert which label the trends report returns
The existing tests assert the customer stays one row but never which territory
or name comes back, so a divergence between engines passes unnoticed. Asserts
both equal the Customer master's values while an order stores a different
territory.
* fix(controllers): resolve a Quotation's party label through quotation_to
party_name is a dynamic link, so looking it up in Customer and Lead alone was
wrong twice over: a Quotation raised against a Prospect or a CRM Deal got a
blank label, and when a Lead shared its name with a Customer the Customer-first
lookup returned the wrong record's name and territory.
Resolve through the quotation_to discriminator instead, mirroring
Quotation.set_customer_name -- Customer, Lead (company_name falling back to
lead_name), Prospect, and CRM Deal. The CRM Deal branch is emitted only when its
table exists, since it ships with the CRM app.
quotation_to joins the GROUP BY as well: two parties of different types can
share a name, and merging them into one row was never right.
* style(controllers): name the quotation CASE branches
semgrep's string-concat-in-list flags adjacent string literals inside a list,
since that shape is usually a missing comma rather than deliberate. Bind each
branch to a name first so the concatenation is unambiguous.
* fix(accounts): key the payment ledger CTEs on account, not Max(account)
QueryPaymentLedger builds two CTEs -- voucher amount and outstanding -- and
joins them on account among other columns. Both sides selected Max(account)
while grouping without it, so the join key was an aggregate over two different
row sets. A voucher posting ledger entries against two party accounts could
have the two sides pick different accounts, the join miss, and the outstanding
come back NULL. Max() over text is a sort, so which account wins is also
collation-dependent, and the engines sort text differently.
Group both CTEs by account instead. That makes the join key a real column and
scopes each Sum() to a single account -- so amount_in_account_currency is no
longer summed across accounts that may not share a currency. Row shape only
changes for a voucher that genuinely spans two party accounts for one party,
where today's single row is already an arbitrary pick over mixed currencies.
cost_center and remarks stay descriptive but genuinely vary per entry, and were
aggregated independently, so they could be stitched together from different
entries into a row that was never posted. They now come off one real entry,
picked by Min(name) -- Payment Ledger Entry declares no autoname rule, so
frappe names it by hash, and those are lower-case, which keeps the pick free of
the collation divergence.
* test(accounts): cover payment ledger metadata coherence
A Journal Entry posting two receivable lines for one customer with different
cost centers and remarks. Whatever row the ledger returns, its cost center and
remarks must be a pair that was actually posted. Guards the fixture itself, so
it cannot pass by posting only one distinct pair.
* test(accounts): cover the account-keyed payment ledger aggregation
The coherence test posts both party lines to one account, so it exercises the
representative-row metadata but not the account-keyed grouping or the CTE join.
Adds a Journal Entry posting to two receivable accounts for one customer and
asserts each account comes back as its own row, with its own amount and a
non-null outstanding.
Set the number format on the session user rather than on System Settings: the
code reads the user default, which shadows the global one, so these tests never
exercised the path they were written for. Restoring it in a finally also keeps
a failed assertion from leaving the whole suite in another locale.
Add a table test over every format in NUMBER_FORMAT_MAP, covering the grouped
values and the three formats parse_float used to read as 0, and restore the
formula-based coverage for non-numeric readings.
parse_float and is_valid_number each re-derived the number grammar, so the
validator accepted strings flt() cannot parse: str.isdigit() lets superscripts
through and lstrip("+-") lets repeated signs through, both then silently scored
as 0. One parse_reading() returning None when float() refuses the value makes
acceptance and conversion true by construction.
The grammar was also wrong for several formats. Where the group separator is
not a dot, a dot-decimal reading such as 1.15 parsed correctly before and is
accepted again. #,### and #.### report no decimal separator at all, which
rejected every fractional reading outright and, for #.###, reread a stored
1.500 as 1500.0; they now fall back to a dot and give up the grouping that
would collide with it.
Only readings that change are checked, so an inspection entered by a user in
one locale stays saveable and submittable by a user in another, and manual
inspection rows keep the free text they were never parsed for.
NumberFormat replaces get_number_format_info, which frappe drops in v16.
covers the reported case, a 1,15 reading in the space grouped "# ###,##"
format, which was read as 115 and rejected. also covers the dot grouped
comma format, and asserts that a reading written with the wrong separator,
or one that is not a number at all, is now rejected with an error rather
than read as a different value.
a numeric reading of "random text" was read as 0 and pulled the mean from
0.6 down to 0.4, which the test then asserted as accepted. such a reading
is now rejected outright, and the test is about formula evaluation, so drop
the row. its assertions are unchanged.
readings are Data fields, so they are parsed server side. parse_float only
swapped the separators for "#.###,##", so in the space grouped "# ###,##"
(polish) a reading of 1,15 was read as 115, fell outside the acceptance
range and silently rejected the inspection. strip whatever the group
separator is and normalise whatever the decimal separator is instead.
it also read the global number format, while the desk formats numbers with
the user's own. a user whose locale differs from the site therefore typed
readings in a format the server did not parse them with. read the user
default, which falls back to the global one.
a reading that is not a valid number in that format is now rejected with an
error instead of being read as a different number.
* fix(accounts): take POS summary warehouse and cost centre from one item line
Both describe an item line, not the invoice, and an invoice can carry several.
They were aggregated independently per invoice, so the report could show a
warehouse from one line beside a cost centre from another -- a pair that was
never posted.
The warehouse then becomes an outer grouping key, so the pick is not merely a
label: it decides how rows are partitioned across owner/date and therefore what
each row totals. Max() over text is a sort, and MariaDB folds case while
PostgreSQL orders by byte value, so the two engines can partition differently.
Take both off one real line instead, and the mode of payment off one real
payment line for the same reason. Sales Invoice Item is hash-named and Sales
Invoice Payment declares no autoname rule, so frappe hash-names it too -- which
keeps Min(name) free of the collation divergence that sorting text has.
* test(accounts): cover POS summary warehouse/cost-centre coherence
The existing tests post a single item line, so they cannot see this. Adds an
invoice with two lines whose warehouse and cost centre are deliberately
crossed: the higher warehouse sits on the line with the lower cost centre, so
an independently aggregated pair belongs to neither line.
* fix(accounts): pick the POS summary representative by idx, not by hash
Min(name) selected whichever child row happened to have the lowest hash, which
is arbitrary and turns on something unrelated to the data. Min(idx) selects the
first line the user actually entered: an integer, so the pick is free of
collation, and it is meaningful rather than incidental.
The join moves to (parent, idx), which is unique per parent.
The existing query tests assert only how many rows come back, so an ordering
divergence between engines passes unnoticed. Adds a case-adversarial pair: a
lead whose name starts with the search term in upper case, and one containing
it in lower case later on. The first must rank ahead of the second.
Reapplies #56330, which was reverted by #56389 with no recorded reason and has
been absent since 23 June.
The search filter uses .like(), which frappe renders as ILIKE on PostgreSQL, so
a candidate matches regardless of case. The ranking used a bare Locate(), which
frappe renders as strpos() -- case-sensitive there. A candidate can therefore
pass the filter, score no match in the ranking, fall back to 99999 and sort
last, while MariaDB's case-insensitive LOCATE ranks it first.
Same query, different order on the two engines, and a different result page
once page_len cuts between them.
Lower() both operands, matching the item, project, user and pick list handlers
in this same file, which were already correct.
Three helpers each managed their own dictionary on frappe.local, duplicating
cache lifecycle and key handling. @request_cache does the same thing centrally
and is cleared with the request, so the copies cannot drift apart.
Behaviour is unchanged: the decorator keys on the call arguments, which are the
same tuple each hand-rolled key was built from.
A BOM listing one item on two lines, with descriptions and source warehouses
that differ. The second line's description sorts above the first on either
engine, so an aggregated value would win; the row must instead carry the first
line's description together with that same line's warehouse.
Max() over a text column is a sort, and the engines sort text differently:
MariaDB's utf8mb4 collations fold case, the CI PostgreSQL orders by byte
value. MAX('abc','ABD') is 'ABD' on MariaDB and 'abc' on PostgreSQL --
confirmed on CI in the probe attached to #56241.
The parity effort wrapped many descriptive columns in Max() on the reasoning
that it returns the value MySQL picked arbitrarily. Where the column is
functionally dependent on the group key that holds and the wrap is a genuine
no-op. Where it genuinely varies -- description, item_name, uom and their
warehouses all describe a LINE, not the item -- it does not: MySQL picked a
row, not a maximum, and the sort now diverges between engines. Aggregating
each column separately can also pair one line's description with another's
warehouse, or a uom with the wrong conversion factor.
Take those columns from a single real line instead, the first by idx.
Only groups built from more than one line need it. Each query now also selects
Count(<line>.name).distinct(), and the representative pass returns immediately
when no group has more than one line -- in that case Max() of a single value
is already exact and collation cannot apply. A BOM with no repeated item
therefore issues no extra query at all, which matters because the explosion
and sub-assembly resolution recurse per sub-BOM. Genuine repeats are memoised
per request.
Sites covered: BOM explosion and sub-item queries, sub-assembly raw materials,
get_bom_items_as_dict, BOM Stock Analysis (both queries), Requested Items to
Order and Receive, Pending SO Items for Purchase Request, and Job Card
secondary items.
* fix(stock): take disassembly source columns from one posted line
get_items_from_manufacture_stock_entry collapses a work order's Manufacture
entries to one row per item and wrapped fifteen Stock Entry Detail columns in
independent Max() to satisfy Postgres' strict GROUP BY. Those columns describe
a line, not an item, and three sets have to stay together:
uom only means something beside its conversion_factor
batch_no and serial_no only beside their warehouse
is_finished_item decides whether the row is the output or an input
Aggregated separately they can be drawn from different lines. Two Manufacture
entries consuming the same item in Nos and in Box return ("Nos", 5) -- a pair
that was never posted, and one that does not describe the summed quantity.
Keep the sums (and the qty-weighted basic_rate) in the aggregate, and read the
descriptive columns off a single real line: the earliest by Stock Entry
creation then idx. That is what MariaDB returned in practice, it is
deterministic, and it is identical on both engines. Same representative-row
shape already used by BOM Stock Analysis and the sub-assembly queries.
* test(manufacturing): cover disassembly source-row coherence
Two Manufacture entries consume the same raw material in different UOMs, so
the max uom and the max conversion factor come from different lines. Asserts
the returned pair is one that was actually posted. Fails on the previous
per-column Max() with ('Nos', 5.0) not found in {('Nos', 1.0), ('Box', 5.0)}.
* fix(stock): aggregate disassembly quantities in stock UOM
* fix(manufacturing): stop BOM Stock Analysis inflating both its sums
get_bom_data left-joined Bin on item_code alone and then summed over the
result. Bin holds one row per warehouse and BOM Item one row per line, so the
join is a cross product and each SUM counts the other side's rows:
Sum(qty_consumed_per_unit) x (number of warehouses holding the item)
Sum(bin.actual_qty) x (number of BOM lines carrying the item)
A component on two BOM lines, stocked in two warehouses, reported a per-unit
requirement of 10 instead of 5 and available stock of 20 instead of 10 --
wrong on both engines, and wrong in the single-line case too as soon as the
item sits in more than one warehouse.
Aggregate Bin to one row per item_code before joining, so neither sum can see
the other's duplicates. The warehouse filter moves into that subquery; it
previously sat in the outer WHERE against a left-joined column, which
silently made the join inner, so the join is now made inner explicitly when a
warehouse is given to keep items with no bin there excluded as before.
* test(manufacturing): cover the BOM Stock Analysis bin-join cross product
Component on two BOM lines, stocked in two warehouses: the join yields four
rows, so both sums are doubled. Asserts qty_per_unit is the sum of the lines'
own per-unit quantities and actual_qty the real total across warehouses.
Fails on the previous single-query form with 10.0 != 5.0.
* fix(manufacturing): compute BOM item amount per line
get_bom_items_as_dict groups BOM lines by item_code, so a BOM listing the
same item on more than one line collapses to a single row. The amount column
multiplied the summed quantity by a single line's rate:
Sum(stock_qty / bom.quantity) * Max(rate) * qty
That is neither line's amount and not their total. The Max() was added to
satisfy Postgres' strict GROUP BY on the assumption that rate is constant per
item, but rate is editable per line.
Fold the rate into the sum so every line contributes its own:
Sum(stock_qty / bom.quantity * rate) * qty
Identical for the common single-line item, correct for duplicates, and valid
on both engines. Same class as the fix applied to budget_controller's
requested amount.
* test(manufacturing): cover BOM item amount across duplicate lines
A BOM listing the same item twice, once in the stock UOM and once in a UOM
with a conversion factor, gives the two lines different rates (rate is the
valuation rate scaled by the conversion factor). The two lines collapse into
one row in get_bom_items_as_dict, so amount must be the sum of each line's
own qty x rate.
Guards the fixture with an assertion that the two rates actually differ,
so the test cannot pass vacuously. Fails on the previous
Sum(stock_qty) * Max(rate) expression.
* fix(manufacturing): use matching UOM quantity for BOM amount
Max()/Min() over a text column is a sort, and the engines sort text
differently: MariaDB's utf8mb4 collations fold case, PostgreSQL as CI runs it
orders by byte value. MAX('abc','ABD') is 'ABD' on MariaDB and 'abc' on
PostgreSQL, confirmed on CI in the probe attached to #56241.
That makes a Max() over a text column which varies in case within its group a
live parity gap, rather than the arbitrary-pick preservation the wrap is
usually justified as. Where the column is functionally dependent on the group
key it stays a genuine no-op and collation cannot matter, so the rule is
scoped to non-FD columns to keep it a high-precision signal.
Recorded as a fifth second-order trap in the guide and in the Greptile
instructions, including the trap that a local macOS PostgreSQL agrees with
MariaDB here and reports a false all-clear.
GITHUB_REF is already the fully qualified ref for both branch and tag events,
so reconstructing refs/heads/$GITHUB_REF_NAME and refs/tags/$GITHUB_REF_NAME
just risks the two drifting apart. Keep the type check, since it still decides
whether the develop fallback applies, and take the ref verbatim.
The probe used --heads with a bare name, so it could not describe a tag push
and would have fallen back to develop for one. Resolve a fully qualified ref
from the event instead: the PR base or pushed branch under refs/heads, a tag
under refs/tags, and fail loudly on an unrecognised ref type.
Only branch refs are eligible for the develop fallback. A tag that is absent
from frappe is a real error, not a stacked-PR base, so it still fails.
The previous `||` treated every fetch failure as a missing branch, so a
transient network or auth error on a base that does exist in frappe would
silently substitute develop and report Patch Test results against the wrong
revision.
Probe with `ls-remote --exit-code` instead: exit 2 means no matching ref, so
fall back; any other non-zero status is a real failure and is re-raised.
The Patch Test fetches the frappe repo using this erpnext PR's base branch
name. For an ordinary PR that is develop, which exists in frappe/frappe. For a
stacked PR the base is an erpnext feature branch with no counterpart there, so
the fetch fails and the step exits 128 before any patch runs:
fatal: couldn't find remote ref pg-audit/bom-amount-per-line
This affects every stacked PR. It has been latent rather than absent: earlier
stacks passed only because their Patch Test ran while they still targeted
develop, before being retargeted onto the layer below.
Fall back to develop when the base ref does not resolve. Ordinary PRs and
version-branch PRs are unaffected -- their base exists in frappe, so the first
fetch succeeds and the fallback never runs.
