[19.0][FIX] stock_account: migrate product.value#value to a unit cost - #5855
Open
cuongnmtm wants to merge 1 commit into
Open
[19.0][FIX] stock_account: migrate product.value#value to a unit cost#5855cuongnmtm wants to merge 1 commit into
cuongnmtm wants to merge 1 commit into
Conversation
19.0 replaced stock.valuation.layer with product.value, and pre-migration maps them with rename_models()/rename_tables(). That renames the table in place, so every row id and column value survives untouched -- but the two models give the `value` column different meanings: - 17.0 stock.valuation.layer#value is a *total* value delta, with the per-unit price in the separate unit_cost column. - 19.0 product.value#value is a *unit cost* for records not linked to a move. Those are what product.product#_get_last_product_value() reads to determine standard_price. So a move-less record keeps a total value where 19.0 reads a unit cost. Since a total value is the unit cost multiplied by the quantity of the layer, the resulting standard_price can be off by orders of magnitude, or negative where the layer was a decrease. Nothing fails loudly: the wrong cost simply propagates into COGS. Records linked to a move keep total-value semantics and are consumed correctly by stock_move_value(), so they are left alone; the new step runs last because it rewrites the values stock_move_value() reads. The unit cost is taken from unit_cost, which the rename leaves behind as an orphan column, wherever 17.0 populated it. For manual revaluations 17.0 wrote unit_cost NULL and quantity 0, so there it is derived the way 17.0 itself derived it in product.product._prepare_valuation_layer_field_values(): the running sum(value) / sum(quantity) over the product's layers up to that record's (date, id). Evaluating it per record reproduces the historical cost at that record's own date, which matters because _get_last_product_value() selects by date. Being an accumulation over the layers rather than a per-record field, it does not depend on the cost method, so it covers fifo -- where a revaluation is spread over the remaining_value of the layers still in stock and never lands in a per-product unit cost -- as well as average. The accumulation sums the 17.0 total values, so those are snapshot into a temporary table before the first pass starts overwriting them with unit costs. A record whose product had no quantity on hand at that point has no unit cost to derive; those keep their value and are logged rather than guessed at, since a wrong standard_price is silent and ends up in the accounts. Checked on an upgraded 17.0 database: every move-less record was resolved from one of the two sources, the derived values matched the costs 17.0 had recorded independently, and no move-linked or 19.0-native record was modified. The fifo path was exercised with layers generated through the revaluation wizard and a costing-method change on a fifo category.
hbrunn
requested changes
Aug 4, 2026
hbrunn
left a comment
Member
There was a problem hiding this comment.
why is this referring to 17.0 and not 18.0?
looking more closely at the code, I'm unsure if we should be renaming stock_valuation_layer to product_value at all, and not just pluck records from there that look like manual valuations. Any opinions @OCA/openupgrade-maintainers
In any case, some test data and tests will be helpful.
and this PR violates https://github.com/OCA/.github/blob/master/AI_POLICY.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
19.0 replaced
stock.valuation.layerwithproduct.value, andpre-migration.pymaps them withrename_models()/rename_tables(). That renames the table in place, so every row id and column value survives untouched — but the two models give thevaluecolumn different meanings:valueholdsstock.valuation.layerunit_costcolumnproduct.valueMove-less records are the ones
product.product._get_last_product_value()reads to determinestandard_price. After migration they hold a total value where 19.0 reads a unit cost. Since a total value is the unit cost multiplied by the layer quantity, the resultingstandard_pricecan be off by orders of magnitude, or negative where the layer was a decrease.Nothing fails loudly. The wrong cost simply propagates into COGS, which is how this surfaced — months after the upgrade, in the accounts rather than in a traceback.
Records linked to a move keep total-value semantics and are consumed correctly by the existing
stock_move_value(), so they must be left alone. The new step therefore runs last, because it rewrites the valuesstock_move_value()reads.The fix
Two sources, in order of directness:
unit_cost— the in-place rename leaves it behind as an orphan column, so it is exact wherever 17.0 populated it.unit_costNULL andquantity0, so there the unit cost is computed the way 17.0 itself computed it inproduct.product._prepare_valuation_layer_field_values(): the runningsum(value) / sum(quantity)over the product's layers up to that record's(date, id).Evaluating (2) per record reproduces the historical cost at each record's own date, which matters because
_get_last_product_value()selects by date. Because it is an accumulation over the layers rather than a per-record field, it does not depend on the cost method — so it also coversfifo, where a revaluation is spread over theremaining_valueof the layers still in stock and never lands in a per-product unit cost at all.The accumulation sums the 17.0 total values, so those are snapshot into a temporary table before the first pass starts overwriting them with unit costs.
A record whose product had no quantity on hand at that point has no unit cost to derive. Those keep their value and are logged rather than guessed at: a wrong
standard_priceis silent and ends up in the accounts, so reporting beats fabricating.Testing
Checked on a database upgraded from 17.0: every move-less record was resolved from one of the two sources, and the derived values matched the costs 17.0 had recorded independently in
unit_costwhere both were available. No move-linked or 19.0-native record was modified.The
fifopath was exercised separately, with layers generated through the revaluation wizard and through a costing-method change on afifocategory.Scope
Both upgrade paths reach
19.0.1.1with the source columns present —quantityandunit_costexist onstock.valuation.layerin 17.0 and, unchanged, in 18.0 (the model is only removed in 19.0, and18.0/.../stock_account/carries no valuation migration). Verification above was end-to-end on a 17.0 upgrade; the 18.0 path is covered by the identical schema but I have not run it, so a check against an 18.0 dataset would be welcome.