[IMP][8.0] pos_margin add margin_percent field and improve installation

[legalsylvain]

This PR :

• add a new field margin_percent on pos order; (with a new decimal precision for this value)
• display margin and margin percent on pos order tree and form view;
• make installation faster, if data are present, using SQL request on pre_init_hook, instead of classical ORM;

Note:
This PR fixes purchase_price initialization because before this PR, purchase_price is based on the current purchase_price. (now is based on product_price_history value)

[legalsylvain]

FYI, time reduction from 6 days -> ~2 hours for a DB with over 2 Millions of pos order line.

[pedrobaeza]

What I don't like of these hooks is that there's different compute algorithm that on compute method. Isn't there any way to make faster the compute method using read_group or similar things? I will check for our module account_invoice_margin

[legalsylvain]

hi @pedrobaeza , thanks for your review.

I'm not sure to understand "different compute algorithm".
Is it because SQL algorithm is based on history, and not the ORM one ? If yes, it could better to use history in the ORM method too.

Otherwise, I'd like to understand better your last comment.

[pedrobaeza]

Yeah, that's it. We have 2 different pieces of code for making the same computation. We should remove the hook and make faster the compute method. I would like to avoid SQL querys anyway, and that's why I told you about read_group ORM method that is also efficient.

[legalsylvain]

Yeah, that's it. We have 2 different pieces of code for making the same computation. We should remove the hook and make faster the compute method

I'm not agree with that point. For me it's like OpenUpgrade SQL script.

• We add a new feature and we have a _compute function to compute new values.
• In the meantime, we write an SQL Script to manage anteriority.

I'm ok that SQL scripts quantity should be limited, but in that case, it's interesting. We are talking about populating a huge table. (And furthermore the data "standard_price" is a property, so I guess that access is slower).

[pedrobaeza]

@legalsylvain the problem is in the maintenance: you have to maintain 2 codes for the same thing. Imagine there's an error on one of them and not the other, and you are not aware of as some of your data has been populated on install and the rest by normal computation. It would be very difficult to debug and locate that.

Another example: you change the computation on one place, but not the other, and you have mixed results.

That's why I say to unify them on compute method. Sometimes that means to reduce a bit the performance, but we can get a balanced formula. We can even use SQL in the compute method if we don't find another way to get enough performance, but please, don't duplicate code (DRY principle).

[legalsylvain]

Thanks for giving your point of view, I understand better, but i'm still not agree with you.

Well basically, I'm agree that duplicating algorithm generates more maintenance effort (for the good reasons you gave), and so should be limited. In the other hand, it is sometimes necessary. That's why a lot of existing OCA modules are containing exactly the same hooks as this one, to manage anteriority.

non exhausting list :

• stock-logistics-warehouse / stock_valuation_account_manual_adjustment (explanation here)
• stock-logistics-warehouse / stock_removal_location_by_priority
• stock-logistics-workflow / stock_ownership_availability_rules
• hr / hr_recruitment_partner
• ...

Let ask to other OCA members, what they're thinking about that point.

Regards.

[sebastienbeau]

After talking with @legalsylvain, We think that the solution is to set the purchase_price like a normal field (fill by an onchange) https://github.com/grap/pos/blob/e5b36985fddf3621b8282ec1b6270fd500a0a38b/pos_margin/models/pos_order_line.py#L19

Indeed this field do an historisation of the purchase price. Having a computed field will return a different value depending of the moment of computation. Which is dangerous because we can loss the data if the field is recomputed for some unknow reason later.

So basically the idea is to set purchase_price as a normal field and fill the historic with an SQL request.

[legalsylvain]

Hi @sebastienbeau,

thanks a lot for your answer. I so changed purchase_price type from computed / store True to no computed, with onchange feature. It fixes the problem you mention about the different value, depending of the moment of the computation.
In that way the SQL script to compute purchase_price for anteriority is now not a duplicate of an ORM function.

I propose to let the trivial SQL script that compute margin and margin_percent fields, because :

• they improve really installation time on huge databases
• they have trivial algorithm and are stable (should not have to change)

I hope it will be acceptable.

kind regards.

[pedrobaeza]

This can be performed without subquery (the same as in OpenUpgrade) and without the performance penalty it means. Do you figure out how to do it?

[legalsylvain]

Not really, could you help me. I'm really not very good for update request.
Regards.

[legalsylvain]

Hi @pedrobaeza. Could we consider to merge this PR as it, and search some optimization. The subquery is not generating a big performance penalty. Otherwise, could you point me how to do ?
regards.

[legalsylvain]

ping @pedrobaeza