-
-
Notifications
You must be signed in to change notification settings - Fork 654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[16.0][MIG] stock_valuation_fifo_lot: Migration to 16.0 #1676
base: 16.0
Are you sure you want to change the base?
Conversation
a3ece53
to
3c26a62
Compare
Test failed from |
b60bb4e
to
71c8a67
Compare
@matt454357 |
I started a review, and will post some suggestions. |
@matt454357 After you review and the PR is merged, you can create a new PR to follow up on your idea. What do you think? |
71c8a67
to
07a5f63
Compare
@matt454357 I will not add this enhancement in the current PR because it is not related to migration, and there is no use case with the current design. So, could you please handle this when you work on revaluation? |
I think it's a good idea to finish this migration PR, and then do a follow up PR. And, I'm happy to do the follow up PR. However, there a couple of problems with allowing multiple lots/serials per SVL. Maybe this initial migration should prevent revaluation of tracked products? Maybe also raise an error when |
The believe PR #1677 supports the idea of enforcing single lot/serial per SVL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just commenting on this PR, not suggesting that we make these changes immediately.
return_move = return_picking.move_ids | ||
return_move.move_line_ids.qty_done = 5 | ||
return_picking.button_validate() | ||
self.assertEqual(abs(return_move.stock_valuation_layer_ids.value), 1500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertEqual(abs(return_move.stock_valuation_layer_ids.value), 1500) | |
self.assertEqual(abs(return_move.stock_valuation_layer_ids.value), 1500) | |
def test_customer_partial_return(self): | |
"""Verify partial customer return of delivery with multiple lots of the | |
same product""" | |
receipt_picking_1 = self.create_picking( | |
self.supplier_location, self.stock_location, self.picking_type_in | |
) | |
move = self.create_stock_move(receipt_picking_1, self.product, 100) | |
self.create_stock_move_line(move, "11111") | |
receipt_picking_1.action_confirm() | |
receipt_picking_1.action_assign() | |
receipt_picking_1._action_done() | |
receipt_picking_2 = self.create_picking( | |
self.supplier_location, self.stock_location, self.picking_type_in | |
) | |
move = self.create_stock_move(receipt_picking_2, self.product, 200) | |
self.create_stock_move_line(move, "22222") | |
receipt_picking_2.action_confirm() | |
receipt_picking_2.action_assign() | |
receipt_picking_2._action_done() | |
delivery_picking_2 = self.create_picking( | |
self.stock_location, self.customer_location, self.picking_type_out | |
) | |
move = self.create_stock_move(delivery_picking_2, self.product) | |
move_line = self.create_stock_move_line(move) | |
lot_id = self.env["stock.lot"].search( | |
[("name", "=", "11111"), ("product_id", "=", self.product.id)] | |
) | |
move_line.write({"lot_id": lot_id}) | |
move_line = self.create_stock_move_line(move) | |
lot_id = self.env["stock.lot"].search( | |
[("name", "=", "22222"), ("product_id", "=", self.product.id)] | |
) | |
move_line.write({"lot_id": lot_id}) | |
delivery_picking_2.action_confirm() | |
delivery_picking_2.action_assign() | |
delivery_picking_2._action_done() | |
return_picking_wizard_form = Form( | |
self.env["stock.return.picking"].with_context( | |
active_ids=delivery_picking_2.ids, | |
active_id=delivery_picking_2.id, | |
active_model="stock.picking", | |
) | |
) | |
return_picking_wizard = return_picking_wizard_form.save() | |
return_picking_wizard.product_return_moves.write({"quantity": 5}) | |
return_picking_wizard_action = return_picking_wizard.create_returns() | |
return_picking = self.env["stock.picking"].browse( | |
return_picking_wizard_action["res_id"] | |
) | |
return_move = return_picking.move_ids | |
return_move.move_line_ids.lot_id = lot_id | |
return_move.move_line_ids.qty_done = 5 | |
return_picking.button_validate() | |
self.assertEqual(abs(return_move.stock_valuation_layer_ids.value), 1000) | |
def test_revaluation_layer(self): | |
"""Verify revaluation layers are included in unit price calculation""" | |
receipt_picking_1 = self.create_picking( | |
self.supplier_location, self.stock_location, self.picking_type_in | |
) | |
move = self.create_stock_move(receipt_picking_1, self.product, 1000) | |
self.create_stock_move_line(move, "11111") | |
receipt_picking_1.action_confirm() | |
receipt_picking_1.action_assign() | |
receipt_picking_1._action_done() | |
receipt_picking_2 = self.create_picking( | |
self.supplier_location, self.stock_location, self.picking_type_in | |
) | |
move = self.create_stock_move(receipt_picking_2, self.product, 2000) | |
self.create_stock_move_line(move, "22222") | |
receipt_picking_2.action_confirm() | |
receipt_picking_2.action_assign() | |
receipt_picking_2._action_done() | |
wiz = ( | |
self.env["stock.valuation.layer.revaluation"] | |
.sudo() | |
.create( | |
{ | |
"company_id": self.company.id, | |
"product_id": self.product.id, | |
"added_value": -500.0, | |
"account_id": self.cost_account.id, | |
"reason": "test serial revaluation", | |
} | |
) | |
) | |
wiz.action_validate_revaluation() | |
delivery_picking = self.create_picking( | |
self.stock_location, self.customer_location, self.picking_type_out | |
) | |
move = self.create_stock_move(delivery_picking, self.product) | |
move_line = self.create_stock_move_line(move) | |
lot_id = self.env["stock.lot"].search( | |
[("name", "=", "11111"), ("product_id", "=", self.product.id)] | |
) | |
move_line.write({"lot_id": lot_id}) | |
delivery_picking.action_confirm() | |
delivery_picking.action_assign() | |
delivery_picking._action_done() | |
self.assertEqual(abs(move.stock_valuation_layer_ids.value), 4500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These suggested tests fail because we are not enforcing single lot/serial per SVL.
"license": "AGPL-3", | ||
"author": "Ecosoft, Odoo Community Association (OCA)", | ||
"website": "https://github.com/OCA/stock-logistics-workflow", | ||
"depends": ["stock_account", "stock_no_negative", "mrp_landed_costs"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should depend on mrp_landed_costs
. Could that be handled in a separate module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move to a separate module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I will handle it.
if not self.company_id.use_lot_get_price_unit_fifo: | ||
return super()._get_price_unit() | ||
if ( | ||
not self.purchase_line_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on purchase
module, which I guess is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added hasattr(self, "purchase_line_id")
in the conditon. So, there is no issue I guess.
if ( | ||
not self.purchase_line_id | ||
and self.product_id.cost_method == "fifo" | ||
and len(self.lot_ids) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we raise an error when len(self.lot_ids) > 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any concerns or inconveniences if we use Odoo's standard behavior for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we raise an error when
len(self.lot_ids) > 1
?
I think we could make it configurable if the community requires it. I don't have a strong need for that at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, single lot per SVL would not work when migrating/installing with existing data. So, I don't think we can do that.
Thanks for the pointer and your interest in following up. What do you think of my idea of adding an option to choose a single or multiple lot/serial per SVL? As I mentioned, I don't want to make this adjustment in the migration PR because it would make the module bigger, and the community might be less interested in reviewing it. So, my idea is to get this PR merged quickly and then follow up with the changes. |
Yes, keep this PR small. Regarding the option to choose between multiple or single lot per SVL: I'm not sure why someone would want multiple, but it sounds like you have a use case in mind. So, I suppose it's fine, as long as we raise errors for the issues described above. |
07a5f63
to
2a2dfb9
Compare
@TheerayutEncoder |
@matt454357 Please let me handle this part and include it in this PR because we urgently need this module, and I don't know when this PR will be merged. |
@AungKoKoLin1997 any contribution you make is very much appreciated, Do it the way you feel is best, and I will work with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
fbff13d
to
a7a187c
Compare
"in", | ||
self.lot_ids.ids, | ||
), | ||
("remaining_qty", ">", 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we misunderstood the usage of this logic. I now think this doesn't work. The original logic isn't optimal either. I'm thinking of a different implementation...
if ( | ||
not self.purchase_line_id | ||
and self.product_id.cost_method == "fifo" | ||
and len(self.lot_ids) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we raise an error when
len(self.lot_ids) > 1
?
I think we could make it configurable if the community requires it. I don't have a strong need for that at the moment.
/ocabot migration stock_valuation_fifo_lot |
We will also need odoo/odoo#179447, in order to do revaluation (later). |
c9c987d
to
bdc4591
Compare
forced_quantity=forced_quantity | ||
) | ||
layers |= layer | ||
# Calculate standard price (sorted by lot created date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is in the wrong position.
# Calculate standard price (sorted by lot created date) |
continue | ||
for ml in layer.stock_move_id.move_line_ids: | ||
ml.qty_base = ml.qty_done | ||
# Change product standard price to the first available lot price. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Change product standard price to the first available lot price. | |
# Update product standard price to the first available lot price (by sorting by lot create date). |
continue | ||
product = product.with_company(move.company_id.id) | ||
product = product.with_context(disable_auto_svl=True) | ||
product.sudo().standard_price = candidate.unit_cost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
product.sudo().standard_price = candidate.unit_cost | |
# `candidate.unit_cost` is not totally accurate in 16.0 because of https://github.com/odoo/odoo/issues/171464 | |
product.sudo().standard_price = candidate.remaining_value / candidate.remaining_qty |
# Change product standard price to the first available lot price. | ||
product = product.with_context(sort_by="lot_create_date") | ||
candidate = product._get_fifo_candidates(move.company_id)[:1] | ||
if not candidate: | ||
continue | ||
product = product.with_company(move.company_id.id) | ||
product = product.with_context(disable_auto_svl=True) | ||
product.sudo().standard_price = candidate.unit_cost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not clear about the business rationale why we need this part.
@ps-tubtim @Saran440 Could you please explain what business needs you had to have this logic? In my view, product standard price doesn't mean much in FIFO (it doesn't affect the transaction except for the case where inventory is created with a new lot with no purchase order) and this logic can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yostashiro https://github.com/OCA/stock-logistics-workflow/blob/15.0/stock_valuation_fifo_lot/models/stock_move.py
As I check the same file in version 15, I don't see that part of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kittiu Sorry, we made it hard to track. It's this part: https://github.com/OCA/stock-logistics-workflow/blob/a87e986/stock_valuation_fifo_lot/models/stock_move.py#L47-L62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember we have a case where it will be incorrect without this fix. I can't remember the case. @newtratip do you remember the case or any where issue is open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yostashiro
As far as I remember, this part will affect the case where you return items to lot in the warehouse.
Example
- Receive goods (WH/IN) with lot001 10 unit, price/unit = $10 => Standard price = $10 (Price from lot001)
- Receive goods (WH/IN) with lot002 5 unit, price/unit = $15 => Standard price = $10 (Price from lot001)
- Issue goods (WH/OUT) 12 unit => Standard price = $15 (Price from lot002)
- Return goods (WH/RET) lot001 3 unit => Standard price = $10 (Price from lot001)
This part of code will check when incoming goods to warehouse, the system will pull price from first lot always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@newtratip Thanks for the explanation. I understand your logic, but I don't fully understand what happens if we don't update the standard price in this scenario. I assume that we don't care about the standard price for FIFO products. Could you please explain the problem if we don't update the standard price in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kittiu @newtratip I removed this part as I don’t see any business need for it, and I don't think we need to be concerned about the standard price for FIFO products. Please let me know if I’m missing something regarding the business or operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AungKoKoLin1997 I believe the standard price is used in at least 2 places, even for FIFO products:
- Sales order line cost and margin
- Inventory adjustment / new quant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matt454357 Thanks for the pointer. I think this part is useful in cases where the first lot of a product is delivered in full and then some quantity is returned. In this situation, the standard price is updated to match the price of the first lot.
So, I think using the standard price of the first lot for the Sales Order line cost doesn't have an impact. Am I misunderstanding?
For inventory adjustments or new quants, adjustments for existing inventory will use the lot price of that inventory with the current design. When creating a new quant, which means a new lot, the standard price of the first or any other lot isn’t necessarily relevant, as I wouldn’t expect it to use the same standard price. So, user should check the standard price before they create new quant.
"Stock valuation after return should be 1500.0", | ||
) | ||
|
||
def test_receive_deliver_return_lot(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite get the point of this test. Does it prove anything when all lots have the same cost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was for the case like we receive serials 001
, 002
and 003
for an incoming move.Then, Deliver 002
, return 002
and deliver 002
again. In this case, we want to check the remaining_qty
was taken from return move's SVL for latest delivery instead of first incoming move's SVL because there remaining qty of first incoming move's SVL is only for both 001
and 002
serials now.
) | ||
self.assertEqual(move_in_2.stock_valuation_layer_ids.remaining_qty, 0.0) | ||
|
||
def test_cost_tracking_by_lot(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be combined with the first test? It looks redundant...
bbc4fc4
to
77bcfce
Compare
4d1f25d
to
05f3f3c
Compare
- Adapt Odoo's standard method _get_fifo_candidates. - Update _get_price_unit logic and make it configurable to also use Odoo's standard behavior.
This commit fixes the incorrect stock_valuation_layer values for deliveries when the product was received before this module was installed. The issue occurs because the old stock_valuation_layer records do not have lot_ids values, which leads to incorrect valuation during the delivery process. Issue link: OCA#1350 (comment)
- Avoid completely overriding _run_fifo and use the standard behavior. - Update the product's standard price to the first available lot price. - Improve _get_price_unit to retrieve the value from the most recent incoming move line for the lot for the No PO (e.g. customer returns) stock moves. - Add quantity and value-related fields in stock_move_line to cover all cases including multiple lots exist in an SVL record, and it is unclear which lot's remaining quantity is being delivered(Eg. Receive serials 001, 002 and 003 for an incoming move. Deliver 002, return 002 and deliver 002 again.) - Add force_fifo_lot_id in stock_move_line to handle cases where a delivery needs to be made for an existing lot created before the installation of this module (e.g., lots 001 and 002 were received, lot 002 was delivered before the module's installation, and lot 001 is delivered after the module is installed). - Add a post_init_hook to update the lot_ids in SVL for existing records and to update the field values in existing stock_move_line records. - Remove stock_no_negative from dependency and check negative inventory is created when SVL is created for delivery.
05f3f3c
to
6ca1b5a
Compare
Superseding of #1527
In this PR, I address the following points:
_run_fifo()
and use the standard behavior._get_price_unit()
to retrieve the value from the most recent incoming move line for the lot for the No PO (e.g. customer returns) stock moves.stock_move_line
to cover all cases including multiple lots exist in an SVL record, and it is unclear which lot's remaining quantity is being delivered(Eg. Receive serials001
,002
and003
for an incoming move. Deliver002
, return002
and deliver002
again.)force_fifo_lot_id
instock_move_line
to handle cases where a delivery needs to be made for an existing lot created before the installation of this module (e.g., lots001
and002
were received, lot002
was delivered before the module's installation, and lot001
is delivered after the module is installed).post_init_hook
to update thelot_ids
in SVL for existing records and to update the field values in existing stock_move_line records.Depends on odoo/odoo#180245
@qrtl QT4650