[16.0] [ADD] sale_commission_margin: Decuct commission from margin

[luisDIXMIT]

This addon allows commissions to be deducted from the margin.

Because precomputed fields must depend on other precomputed fields, some computed fields in the commission module have been established as precomputed.

Issue #153

[OCA-git-bot]

Hi @pedrobaeza,
some modules you are maintaining are being modified, check this out!

[pedrobaeza]

I don't think these changes are correct.

[etobella]

This changes are necessary in order to avoid warnings. The problem comes from the fact that margin fields are pre-computed, then it needs pre-computed fields.

However, after checking Odoo documentation, we can use pre-compute to fill this fields, as they are generated in batch (they are lines). This is why we thought it was ok.

https://www.odoo.com/documentation/16.0/developer/reference/backend/orm.html?highlight=precompute

If you have a reason that we cannot see (it might be possible), we would be happy if you can enlighten us.

[pedrobaeza]

Then change this attribute in the module sale_commission_margin, not here. I prefer to not change it due to things like Precomputing a field can be counterproductive if the records of the given model are not created in batch

[etobella]

I understand your concerns, but the following sentences states:

Following the remark above, precomputed fields can be interesting on the lines of a one2many, which are usually created in batch by the ORM itself, provided that they are created by writing on the record that contains them

Usually, invoice_lines and sale order lines are generated with [(0, 0, {}), ...]. That means that they shouldn't be affected by the issue that you stated.

I prefer to do it directly on the base model in order to avoid future errors or incompatibilities (if we add fields that are not pre-computed, warnings will be raised if this module is installed). However, if there are not other options, we can do it as you suggest.

Also, it is recommended to change the priority of the view on sale.order in order to show the commission and then the margin. Should we do that directly on this new module or can we do it on sale_commission?

[pedrobaeza]

Uhm, I'm still a bit reluctant. We don't know how can it impact. If precompute mode was so good, then it would be enabled by default. I still think that should be in the new module.

About the view priority switch, it can also be done on the new module. That way, you identify the whys of that changes.

[etobella]

For that reason I am suggesting to use a new version. If we are going to block this kind of changes do we need to block all refactorings (even on migrations) too?

Also, by checking the Odoo documentation we showed that the solution makes sense. I don't understand why there are so many complains if we have provided answers with the issue and we have proposed a solution that minimizes effects on the current versions. Keeping it in the module might have other undesired effects as failing integrations with other commission modules and warnings. This is the reason why we are asking for the integration on the main module (we can accept to do it on a non released module).

Just to end, it is not "my" module. It is an OCA module that is fixing a behavior that is not working as expected when we install commission and a core community odoo module. It was documented 7 years ago and no one has addressed it since today

[pedrobaeza]

This sentence in the ORM documentation is the key: Precomputing a field can be counterproductive if the records of the given model are not created in batch. Activating precompute is not needed unless you install sale_margin and this glue module. That's why I say that flag should go into the glue module, not on commission or sale_commission one.

I don't see either why it's a problem to put it in the glue module and why you want it to put on the base module if it's not going to have any benefit, but on contrary, it can be counterproductive as stated in the documentation.

[etobella]

And the next sentence states that it makes sense in O2M fields like sale order lines, purchase order lines or account move lines:

Following the remark above, precomputed fields can be interesting on the lines of a one2many, which are usually created in batch by the ORM itself, provided that they are created by writing on the record that contains them.

It is not counterproductive IMO. Right now, commission and sale_margin do not work properly between them. We are just fixing this behavior.

The other option is to write down that we need to exclude sale_margin when using commission, as it makes no sense

[pedrobaeza]

But you are needing the same this glue module for deducting the amount, so even if you put precompute=True in the base module, you will still need the glue module. That's why I think it should go all together, the change to precompute=True, the view change, and the business logic.

[etobella]

And then we are leaving a big hole for possible future problems.

I could understand your position when we are struggling with my own modules or OCA modules, but we are talking about an integration with a core community module of Odoo.

This are small changes that could be done on the commission module and we both know it.

[etobella]

@OCA/crm-sales-marketing-maintainers