Skip to content
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][ADD] stock_move_actual_date #1711

Open
wants to merge 4 commits into
base: 16.0
Choose a base branch
from

Conversation

AungKoKoLin1997
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 commented May 18, 2023

This module adds actual date in picking and scrap and pass the value to stock move, stock move line and accounting date of SVL's journal entry.

When a user makes the transfer for the products with "real time" inventory valuation, effective date in transfer couldn't be selected.
And Accounting date for stock valuation journal entry will be the same with effective date.
So, for the users who want to specify each transfer's actual accounting date needs to reset to draft manually and
change accounting date. This module intends to get rid of this inconvenience by adding actual date in transfer and scrap and then pass to journal entry.

@qrtl

# inventory adjustment
force_period_date = self._context.get("force_period_date")
if force_period_date:
self.write({"accounting_date": force_period_date})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AungKoKoLin1997 Never doing a write in a compute !

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch from 86a8263 to 825b3d1 Compare February 16, 2024 01:27
super(TestStockValuation, self).setUp()

def test_stock_picking_accounting_date(self):
self.product1.categ_id.property_cost_method = "fifo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.product1.categ_id.property_cost_method = "fifo"

I guess this assignment doesn't affect the test. Can we ensure that the valuation is real_time instead?

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch 3 times, most recently from 83171c6 to 2908226 Compare March 15, 2024 03:47
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AungKoKoLin1997 Can you please add another test to ensure that accounting date set in inventory adjustment is not interfered by this module?

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch 4 times, most recently from 1c84542 to 9332b0b Compare March 19, 2024 04:29
accounting_date = date.today() + timedelta(days=3)
inventory_quant.accounting_date = accounting_date
inventory_quant.with_context(
skip_exceeded_discrepancy=True
Copy link
Member

@yostashiro yostashiro Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be doing this as this context doesn't belong to a module in the dependency chain. It should ideally be handled by adjusting the stock_inventory_discrepancy module or by setting rogue modules.

store=True,
)

@api.depends("date", "accounting_date")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AungKoKoLin1997 Shouldn't it be 'picking_id.accounting_date' ???

def _compute_accounting_date(self):
# 'force_period_date' context is assigned when user sets accounting date in
# inventory adjustment
force_period_date = self._context.get("force_period_date")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like computed stored fields based on context values....

Comment on lines 17 to 28
# 'force_period_date' context is assigned when user sets accounting date in
# inventory adjustment
force_period_date = self._context.get("force_period_date")
if force_period_date:
for rec in self:
rec.accounting_date = force_period_date
else:
for rec in self:
if rec.picking_id.accounting_date:
rec.accounting_date = rec.picking_id.accounting_date
continue
rec.accounting_date = fields.Datetime.context_timestamp(self, rec.date)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the condition based on the context here, as @rousseldenis suggests.

Suggested change
# 'force_period_date' context is assigned when user sets accounting date in
# inventory adjustment
force_period_date = self._context.get("force_period_date")
if force_period_date:
for rec in self:
rec.accounting_date = force_period_date
else:
for rec in self:
if rec.picking_id.accounting_date:
rec.accounting_date = rec.picking_id.accounting_date
continue
rec.accounting_date = fields.Datetime.context_timestamp(self, rec.date)
for rec in self:
if rec.picking_id.accounting_date:
rec.accounting_date = rec.picking_id.accounting_date
continue
rec.accounting_date = fields.Datetime.context_timestamp(self, rec.date)

Comment on lines 40 to 41
am_vals = super(StockMove, self)._prepare_account_move_vals(
credit_account_id,
debit_account_id,
journal_id,
qty,
description,
svl_id,
cost,
)

This comment was marked as outdated.

Comment on lines 17 to 18
# 'force_period_date' context is assigned when user sets accounting date in
# inventory adjustment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, missed these lines.

Suggested change
# 'force_period_date' context is assigned when user sets accounting date in
# inventory adjustment

Comment on lines 20 to 30
for vals in vals_list:
if vals.get("actual_date"):
self = self.with_context(actual_date=vals["actual_date"])
return super().create(vals_list)
Copy link
Member

@yostashiro yostashiro Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit tedious but somethings like this?

Suggested change
for vals in vals_list:
if vals.get("actual_date"):
self = self.with_context(actual_date=vals["actual_date"])
return super().create(vals_list)
moves = self.env["stock.move"]
vals_list_with_actual_date = []
vals_list_without_actual_date = []
for vals in vals_list:
if vals.get("actual_date"):
vals_list_with_actual_date.append(vals)
else:
vals_list_without_actual_date.append(vals)
for vals in vals_list_with_actual_date:
move = super().with_context(actual_date=vals["actual_date"]).create(vals)
moves += move
if vals_list_without_actual_date:
moves += super().create(vals_list_without_actual_date)
return moves

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch from 4ec796a to 61c1128 Compare September 24, 2024 06:03
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AungKoKoLin1997 My suggestion should work as far as I've checked. Am I missing something?

Comment on lines 11 to 16
def _prepare_move_values(self):
vals = super()._prepare_move_values()
if self.actual_date:
vals["actual_date"] = self.actual_date
return vals

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _prepare_move_values(self):
vals = super()._prepare_move_values()
if self.actual_date:
vals["actual_date"] = self.actual_date
return vals

if not account_moves:
continue
account_moves._update_accounting_date()
return res
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return res
return res
def do_scrap(self):
for scrap in self:
scrap = scrap.with_context(actual_date=scrap.actual_date)
super(StockScrap, scrap).do_scrap()
return True

Comment on lines 18 to 31
@api.model_create_multi
def create(self, vals_list):
# Pass the 'actual_date' as context to use in _compute_actual_date() when
# the stock move is created for the scrap.
# Since there is no 'scrap_ids' value at the time of creation of the move,
# the scrap and move are linked only after the move is done as per the design
# of the 'do_scrap()' method.
for vals in vals_list:
if vals.get("actual_date") and vals.get("scrapped"):
# Expect only one stock move in vals_list if it is from scrap.
self = self.with_context(actual_date=vals["actual_date"])
return super(StockMove, self).create([vals])
return super().create(vals_list)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@api.model_create_multi
def create(self, vals_list):
# Pass the 'actual_date' as context to use in _compute_actual_date() when
# the stock move is created for the scrap.
# Since there is no 'scrap_ids' value at the time of creation of the move,
# the scrap and move are linked only after the move is done as per the design
# of the 'do_scrap()' method.
for vals in vals_list:
if vals.get("actual_date") and vals.get("scrapped"):
# Expect only one stock move in vals_list if it is from scrap.
self = self.with_context(actual_date=vals["actual_date"])
return super(StockMove, self).create([vals])
return super().create(vals_list)

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch from 61c1128 to 5dd805e Compare September 24, 2024 15:08
@AungKoKoLin1997
Copy link
Contributor Author

My suggestion should work as far as I've checked. Am I missing something?

@yostashiro You are right. It seems like I missed something. My apologies.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch from 5dd805e to 257f1ba Compare September 24, 2024 15:16
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AungKoKoLin1997 Can you please increase the test coverage.

I'm also thinking that the name stock_move_actual_date may be more appropriate. What do you think?


class TestStockValuation(TestStockValuation):
def setUp(self):
super(TestStockValuation, self).setUp()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should isolate our tests.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch from 257f1ba to 8c33664 Compare September 25, 2024 02:22
@AungKoKoLin1997 AungKoKoLin1997 changed the title [16.0][ADD] stock_picking_actual_date [16.0][ADD] stock_move_actual_date Sep 25, 2024
@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch 2 times, most recently from cdda7e2 to c046648 Compare September 25, 2024 04:20
date(2024, 8, 11),
)

# Test inventory adjustment with actual date
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this into a dedicated test method to make it easier to follow.

)
cls.supplier_location = cls.env.ref("stock.stock_location_suppliers")
cls.stock_location = cls.env.ref("stock.stock_location_stock")
cls.env.user.tz = "Asia/Tokyo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity of the test scenario, it would be better to put this inside test_stock_move_without_actual_date_from_picking_or_scrap() which is the only test this setting matters.

Comment on lines 108 to 116
_, _ = self.create_picking()
inventory_quant = self.env["stock.quant"].search(
[
("location_id", "=", self.stock_location.id),
("product_id", "=", self.product_1.id),
]
)
inventory_quant.inventory_quantity = 20.0
inventory_quant.accounting_date = date(2024, 7, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just directly create a quant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no reason. Your suggestion is more clear. I updated.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-stock_picking_accounting_date branch from 1c77025 to 88ad942 Compare September 26, 2024 09:02
Comment on lines 87 to 89
self.assertEqual(
move.stock_valuation_layer_ids.account_move_id.date, date(2024, 9, 1)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move.stock_valuation_layer_ids could involve adjustment layers and therefore is not desirable to be relied upon.

Suggested change
self.assertEqual(
move.stock_valuation_layer_ids.account_move_id.date, date(2024, 9, 1)
)
self.assertEqual(move.account_move_ids.date, date(2024, 9, 1))

Please apply the same to the rest.

Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review.

@AungKoKoLin1997 Please squash commits once it's good to do so.

# Copyright 2024 Quartile (https://www.quartile.co)
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

from openupgradelib import openupgrade
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from openupgradelib import openupgrade
from odoo.tools.sql import column_exists

We don't need openupgradelib?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It is valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants