[12.0] [account_asset_management] Updating the profile doesn't impact the asset

OCA / account-financial-tools

Odoo Accountant Financial Tools and Utils

GNU Affero General Public License v3.0

312 stars 756 forks source link

[12.0] [account_asset_management] Updating the profile doesn't impact the asset #910

Closed ivantodorovich closed 1 year ago

ivantodorovich commented 4 years ago

1) Configure an asset profile with accounts, analytic account, etc.. 2) Configure an asset and use that profile 3) Change the profile's accounts / analytic / etc..

Expectation: New asset depreciation moves should use the new account / analytic account.

Result: New asset depreciation moves continue to use the account set on the asset, because it wasn't changed.

The asset will continue to use the previous account / analytic account. Changes on the profile will not impact the asset until the onchange('profile_id') is triggered. https://github.com/OCA/account-financial-tools/blob/12.0/account_asset_management/models/account_asset.py#L283

It's confusing and counter-intuitive.

IMO this should be changed to make it work similarly as how products and product.categories work. Meaning, instead of using onchange, compute the values when needed like this: account = asset.account_id or asset.profile_id.account_id

If agreed this could be targeted for 13.0

pedrobaeza commented 4 years ago

That's not how profiles/templates work in any part of Odoo: they act as initial master data when applying that profile/template, but no later sync is done if you change later that profile/template. That would mean a lot of things to take into account and have unexpected side effects.

I don't think this should be modified, and if you do so, do it on a separate module.

luc-demeyer commented 4 years ago

I fully agree with Pedro's point of view.

ivantodorovich commented 4 years ago

Well.. the model I have in mind is product.template and product.category. The account used on the move lines will be the one on the template, if set.. and default to the one on the category.

Accountants are used to that behaviour and -intuitively- they expect the same for assets.

I know in the enterprise module it's working with this template/asset behaviour, but that doesn't mean it's the best approach

pedrobaeza commented 4 years ago

It's not the same, Iván: the product template is directly linked to variants because of inheritance by delegation, and in fact the fields are in template, not in variant, but you see them as related fields. In this case, they are 2 separate objects and they shouldn't be linked that way.

Examples of this pattern (template factory can be called) that you have found in every places of Odoo/OCA:

You take a customer that has a payment term, and put it in an SO. Payment term is changed in that moment, but you can change it in that specific SO by any other.
You put a product, and price and taxes are copied in the SO line, but then you can change them.
On OCA contract module, you choose a contract template for starting with, but then you modify at your taste.
...

This pattern is different from what you want, and both have their pros and cons (I avoid expanding this more for now), but that's why I say that this shouldn't be changed in the module, as it's the expected pattern, and if you want it to do it, you can provide an extra module that performs the sync, but take care with the mentioned side effects (one of the worse cons of the mirror patern).

ivantodorovich commented 4 years ago

I'm not talking about the product.template/product.product inheritance. I thought that was obvious.

I'm talking about how accounts are configured in products and product.category.

Code speaks by itself, so: https://github.com/odoo/odoo/blob/12.0/addons/account/models/product.py#L39

That's the pattern accountants are used to. They do it all the time for products, and it's understandable to expect it in assets aswell.

ivantodorovich commented 4 years ago

I guess the main conceptual difference is that for you the asset.profile is a template, while for me is more like a category.

pedrobaeza commented 4 years ago

Sorry for the confusion. Yeah, that part can be extensible to leave without filling certain fields, and if so, rely on profile's value instead.

@luc-demeyer what do you think?

luc-demeyer commented 4 years ago

I think that this thread proves that the module needs better documentation.

github-actions[bot] commented 1 year ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.