- Create a Customer location (CL) and set a Company on it
- Configure Product A in FIFO / Perpetual
- Receive 2 Unit(s) of A in Stock
- Create an Internal Transfer
1 Unit(s) of Product A
Source location: Stock
Destination Location: CL
- Validate
The Inventory Valuation report incorrectly retrieves a quantity of 2 in
stock instead of 1.
The error is first due to a misconfiguration: the CL should not have a
Company set. However, we should not take into account any quantity in a
customer location, even if there is a Company set.
opw-1884675
opw-1884835
closesodoo/odoo#27145
Issue 1
-------
- Set a product with:
Inventory Valuation: Manual
Costing Method: Average Cost (or Standard Price)
- Receive 1 unit @ 10, set an owner
In the 'Inventory Valuation' report, the product is valued @ 10 while no
accounting entry was created.
The system simply multiplies the available quantity by the cost price,
without taking into account the ownership of the quants.
Issue 2
-------
- Set a product with:
Inventory Valuation: Automated
Costing Method: Average Cost (or Standard Price)
- Receive 1 unit @ 10, set an owner
In the 'Inventory Valuation' report, the product is valued @ 10 while no
accounting entry was created.
Once again, the system simply multiplies the available quantity by the
cost price, without taking into account the AML.
Solution
--------
A slight refactoring of the `_compute_stock_value` method is necessary
to make consistent the the FIFO computation and the Standard / Average
method:
- in case of manual valuation, the quantity on stock moves are taken
into account
- in case of perpetual valuation, the quantity on AMLs is taken into
account
The method `_compute_quantities_dict` needs a minor change so a `False`
value can be handled correctly.
The FIFO method is slightly changed:
- the quantity at date takes into account the owner
- in case of perpetual valuation, AMLs are used instead of stock moves
Bonus point: an error was discovered in `_create_account_move_line`
thanks to the test `test_change_cost_method_1`. The forced quantity
should follow the same sign logic than the product quantity.
opw-1849535
- Create a product where the associated product category has Costing
Method "Average Cost (AVCO)"
- Create a PO. Order 1 unit at $1000.
- Receive that unit. (System now has 1 unit at $1000).
- Create another PO. Order 3 units at $500 each.
- Receive only 1 unit and don’t create any back order.
System has 2 units @ $625, instead of $750.
The quantity computed should take into account the quantity actually
done.
opw-1843998
For the standard and average cost method, we use `qty_available` at date
and the `product.price.history` table.
For FIFO, the normal way would have been to get the `qty_available` at
date and value this quantity with the more recent IN moves at this date.
But there's two main obstacles preventing to use this solution:
- when editing a done move to change the quantity, the change is
reflected at the date, the `qty_available` at date will be incremented
- in the same usecase, the remaining_qty and remaining_value is
incremented in place, which mean that even if the layer was already
consumed, it's now appearing again at the bottom of the fifo layer
Fixing this issue by adding a stock move at the date of today instead of
editing in place in v11 and adding a proper "cost layer" model in master
was deemed too complex as well as unnecessary by our bosses
So we now say we continue to update in place, and when looking at the
quantity at date
- if periodic, we sum the value of the move since the beginning of times
- if perpetual, we sum the balance of the account move lines since the
beginning of times. Also, in this case, the quantity at date is
computed across account move lines too, via the "quantity" field on
aml.
This commit adds three non stored computed field:
- qty_at_date: will be computed across stock moves if fifo periodic and
across account move line if fifo perpetual to reflect the design
choice: if periodic, we *really* edit in the past ; if perpetual, no,
the change is done at today's date
- stock_fifo_real_time_aml_ids: the used aml to compute qty_at_date
- stock_fifo_manual_move_ids: the used move to compute qty_at_date
Properly set the quantity field on the account move lines when
exceptions happen (unlock and edit: only the difference is set as
quantity, negative stock and vacuum: no quantity should be set).
When applying a landed cost, no quantty should be set neither.
This is a preliminary work to bring the valuation at date wizard to v11.
Update the value on the moves when exceptions happen (unlock and edit,
negative stock).
This is a preliminary work to bring the valuation at date wizard to v11.
We also update the value on moves in the standard cost method to stay
consistent.
- `_run_fifo` now returns the used value in the negative stock case
- we simplify the `_fifo_vacuum` method so that the `remaining_value`
always represent the not yet compensated value
- we adapt the tests to reflect theses changes ; the value is not the
initial valued amount but is updated upon exceptions
If the vacuum finds that there is nothing to do regarding the accounting
entries, do not call `_account_entry_move`, else an accounting entry
will be created for the entire cost of the compensated move.
opw-1817372
A return should be valued at the standard price. As the `price_unit`
field is not set as copy=False and a return move is a copy of the
original move, the valuation was wrong.
opw 779803
Only looking on the stock move to determine if a move should create
accounting entries is not enough: it is possible that sublocations
implement some strategies that impact the valuation (example: a move
between stock and stock where the move lines move things to a scrapped
location).
We now look on the move line and if any of the move lines is IN, the move
is IN, if any of the move line is OUT, the move is OUT, if there's a
mix, the move is invalid.
We can imagine a scenario into which the main stock location (wh/stock)
is set as a view location to force people to put things in the
sublocations. Since the stock_account refactoring, only the location of
the stock move is considered to know if the locations should be valued
and, if so, the quantity to value. It is problematic since a view
location shouldn't be valued, but if its move lines put or take things
to or from a valued location to a non valued one, the valuation
mechanism should run.
We now introduce these specifications to handle this kind of usecase
correctly:
- the authority to get the price_unit is on the stock move
- the authority to now how much quantity should be valued is delegated
to the move lines.
The initial condition was too naive, it only checked the presence of
company_id on the concerned locations. This is problematic because
people need to create, for instance, multile inventory loss locations
once they have multiple companies, as the accounts set on the locations
are not property fields.
We also use the same condition to determine if the move is IN or OUT on
the move valuation mechanism and the accounting entries creation
mechanism.
- value move lines created when the move is done
- when reducing done out move, increase latest in move in order to not
lose the returned quantity in the fifo chain
The scheduler cron will now only call `run_scheduler`. We refactore this
method to extract `_run_scheduler_tasks` and place a call to
`_merge_quant` a the end.
We override `_run_scheduler_tasks` in stock_account to run the fifo
vacuum when the scheduler cron is executed.
This commit fixes:
- an expected singleton in vacuum, as `_get_in_domain` should be called
on the move we are looping on, not the whole recordset
- some entries were posted for non valuated stock moves
- the depends for the stock value where not correct and the value was
not recomputed when it should
- there was a mismatch between `get_price_unit` and `_get_price_unit`,
we rename all these method into the private one
- the valuation fields were copied on copied move. this proved to be
problmatic when creating extra move
- in the `_fifo_vacuum` method, when looking for candidates, filter on
available ones
We also refactore the way the valuation is implemented. Indeed, before
this commit, the valuation of a product was the sum of all the sum
fields of the valuated stock moves associated to the given product. This
proved to be the wrong approach when dealing with done stock move
edition and negative stock. Let's say you sold 20 items you do not have,
we'll post an entry with the estimated price and later, when we correct
this entry after receiving new products, updating the value was
complicated. We moved out from this approach by never editing the value
(it is the value at which the stock move was initially valuated) but
updating the remaining value: a negative stock move has a negative
remaining value, every time we compensate it we increase the value. Once
the remaining quantity is null, we wipe the remaining value. We adapted
the code according to this new principle, especially the code posting
the correction entry (that should compensate the negative remaiing
value).
A mechanism was not correctly implemented: increasing the quantity of a
done out move should re-run the fifo lookup. We extract the `_run_fifo`
method into a model method and add to it an optional quantity argument.
This allows to run the method from the write on a done stock move.
We also raise a blocking error if the valuation is 0, it means that the
user has not correctly set its master data.
We add a bunch of tests for all these usecases and complete some older
ones to check the total of account entries.
The approach we tried with the current implementation lead to a replay
of all the next moves since a move had to be corrected, and posting an
account entry with the valuation difference.
This proved to be slow and complicated, and moreover the resulting
account entry could be very difficult to understand and justify.
So we moved away from this strategy and implemented the following points:
For FIFO
* If a move goes into negative (sent more than availabale, we do not
counterpart directly at the next in move: we valued it at the last
fifo price and only when running `_fifo_vacuum` (which is called at
every openning of the valuation report), we use some units currently
in stock and post an account entry with the difference between the
fake value and the real one used.
* When editing a done move and increasing its quantity, the new quantity
is available now and will be used at the next move out. We do not try
to replay the history to make the new values go away directly.
* When editing a done move and decreasing its quantity, it acts as a
return to the supplier at the price unit of the move.
On average price:
* Don't replay anything, if we fall under the negative stock usecase,
value at the last average price.
We restore the previous code that set the average price on the standard
price and we slightly patch it to continue to update the average price
if quantity in stock is negative.
We also add a link between stock moves and account moves. A stat button
on the stock move's form redirect directly to them. It's therefore a
little easier to understand what happened.
We remove the separate fifo and average price fields that were added in
saas17, we keep the single standard price (that should be renamed
cost_price for consistency). In the same idea, remove the cumulated
value that was stored on the move. If we need it, just recompute it
(it's not a frequent operation). We remove last_done_move_id,
last_done_remaining_qty and last_done_qty that were added in saas17 to
help in the replay method, as we just got rid of it. We also remove the
code that compensate the negative stock directy at the next in.
What the fifo vacuum does is essentially the same thing as what an out
move do: find a candidate and take units from it. In this case, the
candidates are newer than its concerned stock move. We stored on the
concerned stock move the value at which we sent it (remaining_value) and
post the difference with the real value on a real product.
Do not inline the average price calculation in `action_done`, as there's
a computed field that handle correctly the negative stock (does not
trigger division by 0). Also, complete this compute method by also
looking on transit locations and implement two fallbacks if the quantity
in stock is null: return the latest cost, if not found return the
standard price.
Also, make sure that `remaining_qty` is only set on fifo moves.
rename `_get_candidates_out_move` to `_get_fifo_candidates_out_move`
and `_get_candidates_move` to `_get_fifo_candidates_in_move` as it's
more explicit that they're only used in FIFO costing.
Fix `_update_future_cumulated_value` to filter only on internal moves
and to remove the value added in the past (self is always an out move to
which we added some value) and rename into
`update_fifo_future_cumulated_value`.
In `action_done`, rename `qty_taken_for_candidate` to
`qty_taken_for_candidate` as it's the qty we take to compensate the
candidate and not a quantity we take on the candidate (in this case,
some old out moves that we could not valuate in time because the stock
was negative). We fix the write on the value and cumulated value to
decrease the qty_taken_for_candidate*move.price_unit, because an out
move has always a negative value and we actually substract the value of
`move` which is always an in move and has always a positive value.
We fix the purchase test where negative out moves were valued positively
and add `test_fifo_negative_1` that tests the sending of goods we do not
have and the receipt encoded afterwards.
We expose a serie of functions that return domains to find in and out
moves and, as opposed as before, the transit locations who have a
company id set (i.e. the inter warehouse transit) do not impact the
valuation of a company.
We rename the `not_move` argument of `_get_latest_cumulated_value` to
`exclude_move`, which is more explicit.
The new implementation of stock moves the cost of products from quants to
moves. So we have implemented the FIFO and average cost method based on
those moves instead of using quants.
- When average price is choosed, a field average_price is set on the
product.
- The tests have been completed with an example of how fifo work with
negative stock.
As the use of real price method is really rare, because of the rigor
required on traceability for such method, we removed this functionnality.
- The tests purchase/test/fifo_price.yml have been adapted to use the
FIFO costing method instead of real price.
The old report stock history doesn't exist anymore, and is replaced by
the move view that contains all the cost information.
The methods `get_price_unit` have been moved from the stock to
stock_account.
The methods `_set_default_price_moves` and
`set_default_price_unit_from_product` have been removed from stock.
Explaination of FIFO and average:
- In action_done for an out move in average and fifo, the price_unit and
stock_value are updated.
- For FIFO, it will use the qty_remaining on the in moves to check from
which in moves to take the stock values for the out.
- In case of negative stock in FIFO, it puts also qty_remaining on OUTs.
- In the case of average, it will take the cumulated value of the last
move and divide it by the quantity in stock.
This commit is pretty much a 'work in progress':
- Edit 'done' moves when the costing method is FIFO doesn't trigger the
replay the FIFO stack.
- The landed_costs feature doesn't work.
Nicolas Martinelli