When the in-memory running rate is zero, the fallback went through
get_incoming_rate, whose previous-SLE lookup matches the same
posting_datetime and can land on a sibling line of the voucher being
replayed. Replace it with get_previous_sle_of_current_voucher excluding
the current voucher, keeping the get_valuation_rate chain when no
previous entry exists. get_incoming_rate is no longer used in this
module.
Reposting a return that removes most of the stock across several lines
of the same item must keep every line at the running average and produce
identical results on a second repost. Before the fix the first repost
already drifted, seeding each line from a sibling row of the same
voucher.
During repost, a return line with recalculate_rate resolved its moving
average rate through get_incoming_rate -> get_previous_sle, which matches
posting_datetime <= and orders by creation desc. For a multi-line return
of the same item, every line shares one posting_datetime, so the query
landed on a sibling line of the same voucher whose stored valuation_rate
was still the previous repost run's output, not the rate before the
voucher.
Each repost run therefore re-seeded the voucher from its own prior
output. The error gain per run is (qty returned at the stale rate) /
(qty remaining after the return), so whenever a return removes most of
the stock the loop diverges instead of converging, alternating sign and
growing until stock_value overflows decimal(21,9) and the repost dies
with 'Out of range value for column stock_value'.
Use the in-memory running valuation rate that update_entries_after
already tracks for the warehouse at this point in the repost. It is the
authoritative pre-entry state, is immune to sibling rows, and makes the
repost idempotent. The database lookup is kept only as a fallback for a
zero in-memory rate, preserving the existing zero-rate fallback chain.
* fix: use current batch avg rate for outward returns of batchwise valuation batches
* fix: honor zero batch average and avoid duplicate batch classification query
Keep validate_warehouses() and add validate_over_delivery_receipt_allowance();
validate_serial_and_batch_no_settings() does not exist on this branch.
Keep this branch's field labels and add only the non_negative flag the
backported commit introduced.
Drop test_over_order_allowance_against_material_request and
test_blanket_order_over_order_aggregated_across_rows: both are develop-only
context the cherry-pick swallowed into the conflicts, and Buying Settings has
no over_order_allowance field here.
Use frappe.tests.utils.change_settings instead of ERPNextTestSuite, which this
branch does not have.
Revert the valuation_method literal to the three options this branch offers -
Standard Cost rode along from a regenerated develop type block.
* fix(subscription): don't reactivate a cancelled subscription
Backport of #57774 to version-15-hotfix.
set_subscription_status() unconditionally set status to Active once
there was no outstanding invoice, with no check for whether the
subscription had been intentionally cancelled. process()'s
cancel_at_period_end check compared posting_date against
getdate(self.end_date), and getdate(None) returns today, so an empty
end_date was silently treated as "cancel now" on every reprocess.
version-15-hotfix does not yet have the Payment Entry -> invoice ->
refresh_subscription_status() hook chain that #57761 reports (that
was added later), and the scheduler already excludes Cancelled
subscriptions, so the exact automatic repro in the issue does not
apply here. The underlying status logic is defective the same way
though, and reachable directly (e.g. any caller invoking the
whitelisted process()/set_subscription_status() on a cancelled
subscription), so fixing it here too rather than leaving the same
class of bug in place.
Related to #57761
* fix(test): avoid tripping the unrelated period-end cancel clause
The test backdated start_date to 2018-01-01, so by the time process()
ran, current_invoice_end was long past. That made the *other* half of
the cancel_at_period_end OR-condition (period end reached) true on
its own, so process() tried to cancel an already-cancelled
subscription and hit cancel_subscription()'s "already cancelled"
guard - unrelated to the empty end_date bug being tested.
Use nowdate() as start_date so current_invoice_end sits safely in the
future, isolating the assertion to the empty end_date guard. Also
drop the fragile intermediate "Unpaid" status assertion, which
wasn't part of what this test verifies.
* fix(subscription): don't let period rollover defeat cancel_at_period_end
process() can advance current_invoice_end to the next period (via
update_subscription_period(), when generating the current period's
invoice) before the cancel_at_period_end check further down runs. For
a subscription with no end_date, that check now compared posting_date
against the already-rolled-forward current_invoice_end, which is
always in the future, so cancel_at_period_end was silently never
honored.
Snapshot current_invoice_end before any rollover and use that in the
check instead, so it still targets the period that just ended.
Fixes the P1 flagged by Greptile review on this PR.
---------
Co-authored-by: test <test@test.com>
* fix: incorrect batch-wise valuation rate for entries with same posting datetime
The tie-breaker in get_batch_no_ledgers compared the bundle's creation
against the SLE's creation. These are different timelines - a bundle can
be created (drafted) much before its SLE (created at submission). For
entries sharing a posting datetime (backdated / amended vouchers), this
mis-ordered the entries against the ledger's replay order (SLE creation),
causing double counting or omission of batch qty / value and runaway
outgoing rates that no repost could heal.
Now the tie is broken using the creation of the bundle's own SLE (same
timeline on both sides). When the valuation runs through the bundle
before its SLE exists, the entry is by definition last in its timestamp
group, so all same-timestamp entries already in the ledger precede it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test: batch-wise valuation ordering for same posting datetime entries
Covers both tie-breaking branches of get_batch_no_ledgers:
- submission (pre-insertion) branch: same-timestamp inward at a different
rate plus a multi-row outward voucher (same item and warehouse), at
submission and after a backdated repost
- existing-SLE branch: a bundle created after its sibling's SLE, the
ordering must follow the SLE creation and not the bundle creation
Both tests fail with the previous parent.creation < sle.creation
tie-breaker and pass with the fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The test disabled the shared `Test Size` Item Attribute. On version-15
`FrappeTestCase` rolls back once per class instead of once per test, so the
flag stayed visible for the rest of `TestItem` and broke the seven tests that
build a variant from that attribute.
Build a dedicated attribute and template instead. Nothing the test writes is
reachable from another test, on either branch, so no cleanup is needed.
Assert that a variant saves after its attribute is disabled when the edit
leaves the attribute rows alone, and that changing an attribute value still
throws.
(cherry picked from commit 8d5326196e)
Disabling an Item Attribute writes `disabled = 1` into every Item Variant
Attribute row, including the rows on the template. `validate_variant` runs
on every save and walks the whole attribute table, so any later save of an
existing variant re-checked its untouched rows against the now-disabled
template row and threw. `update_variants` hit the same wall, which made a
single template save fail once an attribute was disabled.
The flag exists to keep an attribute out of new variants, not to freeze the
variants that already use it. item.js only reads it to drop the attribute
from the variant creation dialog.
Skip rows that are unchanged since the last save. New and edited rows are
still checked, so a disabled attribute cannot be added to an existing
variant, and the same guard covers the sibling checks for attributes and
values that the template no longer offers.
(cherry picked from commit 25cd793617)
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.
(cherry picked from commit 732c884633)
# Conflicts:
# erpnext/controllers/tests/test_sales_and_purchase_return.py
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.
(cherry picked from commit a3e9d13da3)
# Conflicts:
# erpnext/controllers/sales_and_purchase_return.py
