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.
