[4650][ADD] stock_valuation_fifo_lot #156

AungKoKoLin1997 · 2024-08-09T07:58:29Z

yostashiro

@AungKoKoLin1997 Please do a thorough review of the code, checking the consistency against the latest 16.0 standard methods especially for big ones such as _run_fifo().

We should also add tests. I feel it's too risky to go as it is.

yostashiro · 2024-08-10T08:15:31Z

stock_valuation_fifo_lot/views/stock_valuation_layer_views.xml

+            <field name="quantity" position="after">
+                <field name="remaining_qty" optional="show" />
+            </field>


IMO, showing remaining_qty and remaining_value fields in the list view in this module is a bit out of context and it should be done in another module (e.g. stock_valuation_layer_show_balance with its own context, or stock_view_adjust). Otherwise this version of the module will diverge from the OCA which makes it difficult to control manage the module.

yostashiro · 2024-08-10T08:35:54Z

stock_valuation_fifo_lot/models/product.py

+        """Hook function for other sort by"""
+        return all_candidates
+
+    def _get_all_candidates(self, company, sort_by=None):


@AungKoKoLin1997 Please follow up on your own comment OCA/stock-logistics-workflow#1527 (review), first by creating a new PR in the OCA repo perhaps (I think we've given it enough time).

yostashiro · 2024-08-10T08:36:40Z

stock_valuation_fifo_lot/models/product.py

+                if len(candidate.lot_ids) > 1:
+                    return min(candidate.lot_ids.mapped("create_date"))
+                elif candidate.lot_ids:
+                    return candidate.lot_ids[0].create_date


Suggested change

if len(candidate.lot_ids) > 1:

return min(candidate.lot_ids.mapped("create_date"))

elif candidate.lot_ids:

return candidate.lot_ids[0].create_date

if candidate.lot_ids:

return min(candidate.lot_ids.mapped("create_date"))

yostashiro · 2024-08-10T08:45:56Z

stock_valuation_fifo_lot/models/product.py

+        self.ensure_one()
+        move_id = self._context.get("used_in_move_id")
+        if self.tracking == "none" or not move_id:
+            vals = super()._run_fifo(quantity, company)


Suggested change

vals = super()._run_fifo(quantity, company)

return super()._run_fifo(quantity, company)

and then remove the else ladder to save some space and improve the readability.

yostashiro · 2024-08-10T08:54:13Z

stock_valuation_fifo_lot/models/product.py

+            if all_candidates:
+                new_standard_price = all_candidates[0].unit_cost
+            elif candidates:
+                new_standard_price = candidate.unit_cost


Should this have been the intention? Please double check.

Suggested change

new_standard_price = candidate.unit_cost

new_standard_price = candidates[0].unit_cost

yostashiro

I was also not sure if the reported issue in this PR OCA/stock-logistics-workflow#1350 (comment) has already been addressed.

yostashiro · 2024-08-10T09:55:57Z

stock_valuation_fifo_lot/models/stock_move.py

+    def _get_price_unit(self):
+        """
+        No PO, Get price unit from lot price
+        """
+        self.ensure_one()
+        price_unit = super()._get_price_unit()
+        if (
+            not self.purchase_line_id
+            and self.product_id.cost_method == "fifo"
+            and len(self.lot_ids) == 1
+        ):
+            candidates = (
+                self.env["stock.valuation.layer"]
+                .sudo()
+                .search(
+                    [
+                        ("product_id", "=", self.product_id.id),
+                        (
+                            "lot_ids",
+                            "in",
+                            self.lot_ids.ids,
+                        ),
+                        ("quantity", ">", 0),
+                        ("value", ">", 0),
+                        ("company_id", "=", self.company_id.id),
+                    ],
+                    limit=1,
+                )
+            )
+            if candidates:
+                price_unit = candidates[0].unit_cost
+        return price_unit


I don't feel comfortable with the current logic. I think this logic is primarily for increasing the quantity of the existing lot in an inventory adjustment, IIUC.

I think the correct approach may be just to follow the standard logic, or do the following (ideally based on the configuration):

Get remaining_value / remaining_qty from the candidate if remaining_qty is positive

Otherwise, follow the standard logic

@AungKoKoLin1997 What do you think?

I believe this is also for using lot price that is used when we create incoming or outgoing transfers.

yostashiro · 2024-08-10T10:35:44Z

stock_valuation_fifo_lot/models/stock_move.py

+    def _create_in_svl(self, forced_quantity=None):
+        """
+        1. Check stock move - Multiple lot on the stock move is not
+           allowed for incoming transfer
+        2. Change product standard price to first available lot price
+        """
+        layers = self.env["stock.valuation.layer"]
+        for move in self:
+            layer = super(StockMove, move)._create_in_svl(
+                forced_quantity=forced_quantity
+            )
+            # Calculate standard price (Sorted by lot created date)
+            if (
+                move.product_id.cost_method == "fifo"
+                and move.product_id.tracking != "none"
+            ):
+                all_candidates = move.product_id._get_all_candidates(
+                    move.company_id, sort_by="lot_create_date"
+                )
+                if all_candidates:
+                    move.product_id.sudo().with_company(
+                        move.company_id.id
+                    ).with_context(
+                        disable_auto_svl=True
+                    ).standard_price = all_candidates[
+                        0
+                    ].unit_cost
+            layers |= layer
+        return layers


@AungKoKoLin1997 I actually have no idea what this adjustment is for. Can you please explain what it is.

This adjustment is for using first lot product price when we receive this product.
Eg,

Receive product with lot number (11111) for price 100

Receive product with lot number (22222) for price 200

Now, the standard price of product is 100.
Then,

Deliver product with lot number(11111).

After that, the standard price of product is 200.
We create another receipt.

Receive product with lot number (11111) for price 300

We want to assign this price as standard cost because this lot is created first. So, the standard price is 300 with current design. The odoo standard behavior will keep 200 as standard price.

Receive product with lot number (11111) for price 300

Is the assumption here to change the price of the PO line for the first receipt of this lot from 100 (or 200) to 300, and then receive on the same stock move line as the first receipt by increasing the done quantity (sounds like a corner-case scenario to me)?

We want to assign this price as standard cost because this lot is created first. So, the standard price is 300 with current design. The odoo standard behavior will keep 200 as standard price.

I fail to see the point of doing this manipulation. What business goal does it try to address?

yostashiro

@AungKoKoLin1997 Can you please check my updates and add tests for more patterns including the corner-case ones.

Receive serials 001, 002 and 003 for an incoming move. Deliver 002, return 002 and deliver 002 again (it should work now).
Receive serials 001 at JPY100 and 002 at JPY200. Deliver 001 and 002. Return 002, and the returned cost should be JPY200.
Change (increase/decrease) qty_done value of a done move line (incoming and outgoing).
Test force_fifo_lot_id field.

AungKoKoLin1997 · 2024-09-20T08:07:26Z

Need to add test case for checking force_fifo_lot_id field. I have no idea at the moment for this.

yostashiro

Thanks Lin! Haven't reviewed tests yet.

yostashiro · 2024-09-20T08:39:12Z

stock_valuation_fifo_lot/models/stock_move.py

+                else:
+                    return move_line.value_remaining / move_line.qty_remaining


Suggested change

else:

return move_line.value_remaining / move_line.qty_remaining

return move_line.value_remaining / move_line.qty_remaining

yostashiro · 2024-09-20T08:42:51Z

stock_valuation_fifo_lot/models/stock_move.py

+        if hasattr(self, "purchase_line_id") and self.purchase_line_id:
+            return super()._get_price_unit()
+        if self.product_id.cost_method == "fifo" and len(self.lot_ids) == 1:
+            # Get the last consumed incoming move line.


Suggested change

# Get the last consumed incoming move line.

# Get the most recent incoming move line for the lot.

yostashiro · 2024-09-20T10:26:14Z

stock_valuation_fifo_lot/models/stock_move_line.py

+                layers = rec.move_id.stock_valuation_layer_ids
+                remaining_qty_layers = layers.filtered(lambda l: l.remaining_qty > 0)
+                if not remaining_qty_layers:
+                    rec.qty_remaining = 0
+                    rec.value_remaining = 0
+                    continue
+                rec.qty_remaining = rec.product_uom_id._compute_quantity(
+                    sum(remaining_qty_layers.mapped("remaining_qty")),
+                    rec.product_id.uom_id,
+                )
+                rec.value_remaining = (
+                    sum(remaining_qty_layers.mapped("remaining_value"))
+                    * sum(remaining_qty_layers.mapped("remaining_qty"))
+                    / rec.qty_remaining
+                )
+                continue


We should update the README as well to explain the design.

Suggested change

layers = rec.move_id.stock_valuation_layer_ids

remaining_qty_layers = layers.filtered(lambda l: l.remaining_qty > 0)

if not remaining_qty_layers:

rec.qty_remaining = 0

rec.value_remaining = 0

continue

rec.qty_remaining = rec.product_uom_id._compute_quantity(

sum(remaining_qty_layers.mapped("remaining_qty")),

rec.product_id.uom_id,

)

rec.value_remaining = (

sum(remaining_qty_layers.mapped("remaining_value"))

* sum(remaining_qty_layers.mapped("remaining_qty"))

/ rec.qty_remaining

)

continue

# Specifically for the case where qty_done of the outgoing stock move line with done state is reduced,

# which creates a positive stock valuation layer for the outgoing move.

layers = rec.move_id.stock_valuation_layer_ids.filtered(lambda l: l.remaining_qty > 0)

if not layers:

continue

rec.qty_remaining = rec.product_id.uom_id._compute_quantity(

sum(layers.mapped("remaining_qty")), rec.product_uom_id

)

rec.value_remaining = (

sum(layers.mapped("remaining_value"))

* sum(layers.mapped("remaining_qty"))

/ rec.qty_remaining

)

continue

We still need this code for the case of there is no remaining_qty in SVL.
Eg,

Receive 10

Deliver 10

Reduce deliver qty_done to 8
At the moment, the remaining qty of outgoing move line is 2.
Then,

Deliver 2
In this case, we need to clear existing remaining_qty.

if not remaining_qty_layers: rec.qty_remaining = 0 rec.value_remaining = 0 continue

AungKoKoLin1997 · 2024-09-21T05:39:56Z

stock_valuation_fifo_lot/models/product.py

@@ -85,7 +85,7 @@ def _run_fifo(self, quantity, company):
        move_lines = correction_move_line or fifo_move._get_out_move_lines()
        for ml in move_lines:
            fifo_lot = ml.force_fifo_lot_id or ml.lot_id
-            ml_qty = fifo_move.product_uom._compute_quantity(ml.qty_done, self.uom_id)
+            ml_qty = ml.product_uom_id._compute_quantity(ml.qty_done, self.uom_id)


I did this to solve the issue of below case.

Receive 1 dozen (12 units in SVL)

Deliver 1 dozen
With previous code, it will just deduct 1 unit from remaining qty of receiving SVL.

Isn't this the issue you mentioned about the Odoo core? If that is the case, we shouldn't be handling it in this module.

Anyway, I've changed the strategy to use the UoM of the product (not the move UoM) for all the added quantity columns in stock.move.line (for ease of balance comparison between stock valuation layers and stock move lines).

The issue I mentioned was not decreasing qty_done after product is delivered.
This is for just receive and deliver with same uom that is not default uom of product.

yostashiro · 2024-09-22T01:08:27Z

@AungKoKoLin1997 Currently qty_remaining and value_remaining are not updated for incoming receipts where you manually assign the lot number, and I haven't figured out how to fix it. It seems the layers are not found when the following compute is triggered. Would be great if you can fix this.

    @api.depends(
        "lot_id",
        "qty_base",
        "qty_consumed",
        "move_id.stock_valuation_layer_ids",
        "move_id.stock_valuation_layer_ids.remaining_value",
    )
    def _compute_remaining_value(self):
        for rec in self:
            if not rec.product_id._is_fifo() or not rec.lot_id:
                continue
            rec.qty_remaining = rec.qty_base - rec.qty_consumed
            layers = rec.move_id.stock_valuation_layer_ids.filtered(
                lambda x: x.lot_ids in [rec.lot_id]
            )
            remaining_qty = sum(layers.mapped("remaining_qty"))
            if not remaining_qty:
                rec.qty_remaining = 0
                rec.value_remaining = 0
                continue
            rec.value_remaining = (
                sum(layers.mapped("remaining_value"))
                * rec.qty_remaining
                / remaining_qty
            )

kanda999

Code and Functional check

kanda999 · 2024-09-24T10:33:50Z

stock_valuation_fifo_lot/models/stock_move.py

+            # To prevent unknown creation of negative inventory.
+            if (
+                product.cost_method == "fifo"
+                and product.tracking != "none"
+                and layer.remaining_qty < 0
+            ):
+                raise UserError(
+                    _("Negative inventory is not allowed for product %s.")
+                    % product.display_name
+                )


I don't think we need to depend on stock_no_negative because of this part.

jcdrubay · 2024-10-08T07:46:16Z

You might want to reference somewhere that in version 18 of Odoo this is becoming native (from my understanding):

odoo/odoo@2d933b8

AungKoKoLin1997 added 2 commits August 9, 2024 07:56

[ADD] stock_valuation_fifo_lot

fe8efb7

[IMP] stock_valuation_fifo_lot: adjust view

86aef2f

AungKoKoLin1997 requested review from yostashiro and kanda999 August 9, 2024 07:59

yostashiro requested changes Aug 10, 2024

View reviewed changes

yostashiro reviewed Aug 10, 2024

View reviewed changes

AungKoKoLin1997 added 2 commits September 13, 2024 09:56

reflect oca changes

d4945b9

relect oca changes

84a2277

AungKoKoLin1997 changed the title ~~[4650][ADD] stock_valuation_fifo_lot~~ [4650][ADD] stock_valuation_fifo_lot, stock_valuation_fifo_lot_mrp_landed_cost Sep 13, 2024

[RMV] stock_valuation_fifo_lot_mrp_landed_cost

3f0c591

AungKoKoLin1997 changed the title ~~[4650][ADD] stock_valuation_fifo_lot, stock_valuation_fifo_lot_mrp_landed_cost~~ [4650][ADD] stock_valuation_fifo_lot Sep 13, 2024

kanda999 mentioned this pull request Sep 13, 2024

[4650][ADD] stock_valuation_fifo_lot #158

Closed

awesome improvements

9d5d4d2

yostashiro force-pushed the 16.0-add-stock_valuation_fifo_lot branch from 9348d86 to 9d5d4d2 Compare September 14, 2024 16:03

add a means of resolving conflicts, update readme

fa44d0d

yostashiro reviewed Sep 16, 2024

View reviewed changes

AungKoKoLin1997 force-pushed the 16.0-add-stock_valuation_fifo_lot branch 2 times, most recently from 3199861 to 6542fca Compare September 20, 2024 08:04

AungKoKoLin1997 requested a review from yostashiro September 20, 2024 08:53

adj, add tests

647c3c0

AungKoKoLin1997 force-pushed the 16.0-add-stock_valuation_fifo_lot branch from 6542fca to 647c3c0 Compare September 20, 2024 08:57

yostashiro requested changes Sep 20, 2024

View reviewed changes

adj

88e3356

AungKoKoLin1997 commented Sep 21, 2024

View reviewed changes

yostashiro force-pushed the 16.0-add-stock_valuation_fifo_lot branch from 139e6d9 to 526210c Compare September 21, 2024 10:27

add qty_base to sml, refactor code

afbb94d

yostashiro force-pushed the 16.0-add-stock_valuation_fifo_lot branch from 526210c to afbb94d Compare September 21, 2024 12:24

fixup!

adaaf0b

yostashiro force-pushed the 16.0-add-stock_valuation_fifo_lot branch from fee9979 to adaaf0b Compare September 21, 2024 16:00

fixup!

c227feb

fix

21c8a7e

AungKoKoLin1997 requested a review from yostashiro September 22, 2024 03:35

yostashiro and others added 3 commits September 22, 2024 12:20

fix post_init_hook, prevent negative valuation

d6f910c

remove _is_fifo()

94d03a6

upd tests

7f11c13

kanda999 approved these changes Sep 24, 2024

View reviewed changes

kanda999 reviewed Sep 24, 2024

View reviewed changes

remove dependency

4140a04

AungKoKoLin1997 force-pushed the 16.0-add-stock_valuation_fifo_lot branch from 6491a3a to 4140a04 Compare September 25, 2024 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4650][ADD] stock_valuation_fifo_lot #156

[4650][ADD] stock_valuation_fifo_lot #156

AungKoKoLin1997 commented Aug 9, 2024

yostashiro left a comment

yostashiro Aug 10, 2024

yostashiro Aug 10, 2024

yostashiro Aug 10, 2024

yostashiro Aug 10, 2024

yostashiro Aug 10, 2024

yostashiro left a comment

yostashiro Aug 10, 2024

AungKoKoLin1997 Aug 13, 2024

yostashiro Aug 10, 2024

AungKoKoLin1997 Aug 13, 2024 •

edited

Loading

yostashiro Aug 13, 2024

yostashiro left a comment

AungKoKoLin1997 commented Sep 20, 2024 •

edited

Loading

yostashiro left a comment

yostashiro Sep 20, 2024

yostashiro Sep 20, 2024

yostashiro Sep 20, 2024

AungKoKoLin1997 Sep 21, 2024

AungKoKoLin1997 Sep 21, 2024

yostashiro Sep 22, 2024 •

edited

Loading

AungKoKoLin1997 Sep 22, 2024

yostashiro commented Sep 22, 2024

kanda999 left a comment

kanda999 Sep 24, 2024

jcdrubay commented Oct 8, 2024

	vals = super()._run_fifo(quantity, company)
	return super()._run_fifo(quantity, company)

	new_standard_price = candidate.unit_cost
	new_standard_price = candidates[0].unit_cost

		else:
		return move_line.value_remaining / move_line.qty_remaining

	# Get the last consumed incoming move line.
	# Get the most recent incoming move line for the lot.

[4650][ADD] stock_valuation_fifo_lot #156

Are you sure you want to change the base?

[4650][ADD] stock_valuation_fifo_lot #156

Conversation

AungKoKoLin1997 commented Aug 9, 2024

yostashiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AungKoKoLin1997 Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro left a comment

Choose a reason for hiding this comment

AungKoKoLin1997 commented Sep 20, 2024 • edited Loading

yostashiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yostashiro commented Sep 22, 2024

kanda999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcdrubay commented Oct 8, 2024

AungKoKoLin1997 Aug 13, 2024 •

edited

Loading

AungKoKoLin1997 commented Sep 20, 2024 •

edited

Loading

yostashiro Sep 22, 2024 •

edited

Loading