-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
base: 16.0
Are you sure you want to change the base?
[16.0][ADD] stock_move_actual_date #1711
Conversation
26fa304
to
ebf73d2
Compare
ebf73d2
to
3379d16
Compare
3379d16
to
31121ad
Compare
31121ad
to
cba1f84
Compare
af39fdd
to
69cdb19
Compare
69cdb19
to
86a8263
Compare
# inventory adjustment | ||
force_period_date = self._context.get("force_period_date") | ||
if force_period_date: | ||
self.write({"accounting_date": force_period_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.
@AungKoKoLin1997 Never doing a write in a compute !
86a8263
to
825b3d1
Compare
super(TestStockValuation, self).setUp() | ||
|
||
def test_stock_picking_accounting_date(self): | ||
self.product1.categ_id.property_cost_method = "fifo" |
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.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?
83171c6
to
2908226
Compare
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 Can you please add another test to ensure that accounting date set in inventory adjustment is not interfered by this module?
1c84542
to
9332b0b
Compare
accounting_date = date.today() + timedelta(days=3) | ||
inventory_quant.accounting_date = accounting_date | ||
inventory_quant.with_context( | ||
skip_exceeded_discrepancy=True |
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 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") |
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 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") |
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 don't really like computed stored fields based on context values....
# '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) |
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.
Let's remove the condition based on the context here, as @rousseldenis suggests.
# '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) |
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.
This comment was marked as outdated.
Sorry, something went wrong.
# 'force_period_date' context is assigned when user sets accounting date in | ||
# inventory adjustment |
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.
Oops, missed these lines.
# 'force_period_date' context is assigned when user sets accounting date in | |
# inventory adjustment |
9be2cd9
to
4ec796a
Compare
for vals in vals_list: | ||
if vals.get("actual_date"): | ||
self = self.with_context(actual_date=vals["actual_date"]) | ||
return super().create(vals_list) |
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.
A bit tedious but somethings like this?
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 |
4ec796a
to
61c1128
Compare
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 My suggestion should work as far as I've checked. Am I missing something?
def _prepare_move_values(self): | ||
vals = super()._prepare_move_values() | ||
if self.actual_date: | ||
vals["actual_date"] = self.actual_date | ||
return vals | ||
|
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.
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 |
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.
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 |
@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) | ||
|
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.
@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) |
61c1128
to
5dd805e
Compare
@yostashiro You are right. It seems like I missed something. My apologies. |
5dd805e
to
257f1ba
Compare
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 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() |
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.
We should isolate our tests.
257f1ba
to
8c33664
Compare
cdda7e2
to
c046648
Compare
fb02ab9
to
b457c89
Compare
date(2024, 8, 11), | ||
) | ||
|
||
# Test inventory adjustment with actual 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.
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" |
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.
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.
adc1c9e
to
1c77025
Compare
_, _ = 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) |
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.
Why don't we just directly create a 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.
There was no reason. Your suggestion is more clear. I updated.
1c77025
to
88ad942
Compare
self.assertEqual( | ||
move.stock_valuation_layer_ids.account_move_id.date, date(2024, 9, 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.
move.stock_valuation_layer_ids
could involve adjustment layers and therefore is not desirable to be relied upon.
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.
88ad942
to
9f04df4
Compare
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.
Code review.
@AungKoKoLin1997 Please squash commits once it's good to do so.
stock_move_actual_date/hooks.py
Outdated
# Copyright 2024 Quartile (https://www.quartile.co) | ||
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). | ||
|
||
from openupgradelib import openupgrade |
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.
from openupgradelib import openupgrade | |
from odoo.tools.sql import column_exists |
We don't need openupgradelib?
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.
Thanks! It is valid.
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