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_account_move_reset_to_draft: New module #1813

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

victoralmau
Copy link
Member

New module

Please @pedrobaeza and @carlosdauden can you review it?

@Tecnativa TT51201

@yostashiro
Copy link
Member

Looks like we should converge the ideas. OCA/account-financial-tools#1946 :)
CC: @rousseldenis

@pedrobaeza
Copy link
Member

This one seems more complete, handling all the SVLs adjustments.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Oct 9, 2024
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.

Why "multi" in the module name?

@pedrobaeza
Copy link
Member

Because you can reset to draft "multiple" times. The standard only allows one.

@yostashiro
Copy link
Member

yostashiro commented Oct 9, 2024

The standard only allows one.

I don't see it this way. You can reset a bill to draft multiple times with the standard as long as there is no adjustment SVL, no?

@pedrobaeza
Copy link
Member

Yes, correct, but we are referring if you change the price multiple times. That's difficult to be reflected in the technical name, but if you have a nicer name, we are all ears.

@yostashiro
Copy link
Member

My preference is stock_account_move_force_reset_to_draft but it's not a strong one. I was just curious.

@victoralmau victoralmau force-pushed the 16.0-add-stock_account_move_multi_reset_to_draft branch from 5fc94b3 to 5a0b36c Compare October 9, 2024 15:49
Copy link
Contributor

@carlosdauden carlosdauden left a comment

Choose a reason for hiding this comment

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

Code LGTM

@victoralmau victoralmau force-pushed the 16.0-add-stock_account_move_multi_reset_to_draft branch from 5a0b36c to 393a244 Compare October 10, 2024 06:52
@rousseldenis
Copy link
Contributor

Looks like we should converge the ideas. OCA/account-financial-tools#1946 :) CC: @rousseldenis

Yes, I think this one is better in terms of reliability and avoid to enforce additional protections on prices changes or ...

@rousseldenis
Copy link
Contributor

Yes, correct, but we are referring if you change the price multiple times. That's difficult to be reflected in the technical name, but if you have a nicer name, we are all ears.

I'm also in favor of not making 'multi' in the name. As, if you do it once, you can dot it more. That's the same principle for all existing modules that reset objects to draft state. But you can add it to USAGE which is a good approach IMHO.

@pedrobaeza
Copy link
Member

Then which name?

@victoralmau victoralmau force-pushed the 16.0-add-stock_account_move_multi_reset_to_draft branch from 393a244 to ddbfdb6 Compare October 10, 2024 07:38
@rousseldenis
Copy link
Contributor

stock_account_move_force_reset_to_draft

Preference for stock_account_move_reset_to_draft which means what it does.

@pedrobaeza
Copy link
Member

OK, let's rename it, but first please do the review for performing all the possible changes at the same time.

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.

Can you please check my suggestion?

Comment on lines 15 to 27
res = super().button_draft()
for item in self.sudo().filtered(
lambda x: x.is_inbound
and any(line.stock_valuation_layer_ids for line in x.line_ids)
):
for line in item.line_ids.filtered("stock_valuation_layer_ids"):
svls = line.stock_valuation_layer_ids
value = sum(svls.mapped("value"))
if not float_is_zero(
value, precision_rounding=line.currency_id.rounding
):
svls[0].copy({"value": -value})
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.

There are certain cases where we shouldn't be allowing the reset-to-draft operation, and a few considerations should be added.

Suggested change
res = super().button_draft()
for item in self.sudo().filtered(
lambda x: x.is_inbound
and any(line.stock_valuation_layer_ids for line in x.line_ids)
):
for line in item.line_ids.filtered("stock_valuation_layer_ids"):
svls = line.stock_valuation_layer_ids
value = sum(svls.mapped("value"))
if not float_is_zero(
value, precision_rounding=line.currency_id.rounding
):
svls[0].copy({"value": -value})
return res
for move in self.sudo():
if not move.is_inbound:
continue
for line in move.line_ids.filtered("stock_valuation_layer_ids"):
origin_svls = line.stock_valuation_layer_ids.stock_valuation_layer_id
if len(origin_svls.stock_valuation_layer_ids.account_move_line_id.filtered(lambda x: x.parent_state == "posted")) > 1:
raise UserError(_("Invenotory valuation records are intertwined for %(line_name)s.", line_name=line.display_name))
for origin_svl in origin_svls:
if origin_svl.quantity != origin_svl.remaining_qty:
raise UserError(_("The inventory has already been (partially) consumed for %(line_name)s.", line_name=line.display_name))
svls = origin_svl.stock_valuation_layer_ids
value = sum(svls.mapped("value"))
if not float_is_zero(
value, precision_rounding=line.currency_id.rounding
):
origin_svl.remaining_value -= value
revert_svl = svls[0].copy({"value": -value})
revert_svl._validate_accounting_entries()
product = line.product_id.with_company(move.company_id.id)
if product.cost_method == "average":
product.sudo().with_context(disable_auto_svl=True).write(
{"standard_price": product.value_svl / product.quantity_svl}
)
return super().button_draft()

The suggested code has survived my functional test, although I may have missed some corner cases.

I did the following steps with FIFO and AVCO products and checked the valuation at each step.

  • PO for a product: 2 pcs at EUR10
  • Receive 1 pc and create a backorder
  • Receive 1 pc
  • Create a bill for 2 pcs at EUR12 and post
  • Reset the bill to draft
  • Change the bill content to 1 pc at EUR15 and post
  • Create another bill for 1 pc at EUR8 and post
  • Reset the first bill to draft -> User error to prevent valuation inconsistencies
  • Delivery 1 pc

Copy link
Member Author

Choose a reason for hiding this comment

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

I am trying to add this change and creating a test with the use case you describe but there are several considerations:

  • I failed to reproduce the error in the step “Reset the first bill to draft -> User error to prevent valuation inconsistencies”.
  • I think that the _validate_accounting_entries() method should not be called.
  • I think changing the standard_price is something this module should not do.

In general I think it is adding too much logic to something that should not have it, you should be able to change to draft as many times as you want (at least that's my opinion).

Copy link
Member

Choose a reason for hiding this comment

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

@victoralmau Thanks for checking.

  • I failed to reproduce the error in the step “Reset the first bill to draft -> User error to prevent valuation inconsistencies”.

I think the error should be reproducible if you follow the steps exactly as described.

  • I think that the _validate_accounting_entries() method should not be called.

The inventory / interim receipt account balance will be incorrect if you don't do this.

  • I think changing the standard_price is something this module should not do.

Why not? The value of the outgoing shipment will be incorrect without this, if the shipment is processed before the next update of standard_price (repost of the bill or a receipt of the product).

In general I think it is adding too much logic to something that should not have it, you should be able to change to draft as many times as you want (at least that's my opinion).

It can't be that simple, unfortunately, and that's why there has been no proper "fix" by Odoo until today after this comment odoo/odoo#118687 (comment). Inventory valuation and account balances will go wrong with the current code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added all the changes you indicate, but in the test this step Reset the first bill to draft -> User error to prevent valuation inconsistencies does not work.

@victoralmau victoralmau force-pushed the 16.0-add-stock_account_move_multi_reset_to_draft branch from ddbfdb6 to 9c1760c Compare October 14, 2024 06:53
@victoralmau victoralmau changed the title [16.0][ADD] stock_account_move_multi_reset_to_draft: New module [16.0][ADD] stock_account_move_reset_to_draft: New module Oct 14, 2024
@rousseldenis
Copy link
Contributor

@victoralmau @pedrobaeza Could you check @yostashiro comments ?

@victoralmau
Copy link
Member Author

Renamed to stock_account_move_reset_to_draft and applied the changes suggested in #1813 (comment) (personally I think it is adding logic that this module should not have).

@yostashiro
Copy link
Member

personally I think it is adding logic that this module should not have

@victoralmau Thanks for the update but I'm not sure where your comment is coming from. If an implementation risks compromising data integrity/consistency, we must do our best to prevent that. No question about it, IMO.

@gdgellatly
Copy link

@pedrobaeza @victoralmau I added some comments here OCA/account-financial-tools#1947 (comment) about an alternative approach which is strictly speaking less accurate, but more tolerant of intervening transactions (e.g. valuation already fixed, sold though etc) and avoids editing svl's. I make no claim that it is better or worse, just food for thought.

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

Successfully merging this pull request may close these issues.

7 participants