Code review comment for lp:~akretion-team/openobject-addons/trunk-payment-term-on-purchase

Hi Alexis,

it's a pleasure to see the openerp community contributing for a better product and new features, although here i somewhat have some reserves:

*first of all: i think you should better handle the cases where no payment_term are given on the PO. For example, the line 90 of your diff:
    'payment_term': orders[0].payment_term.id,
could be changed into:
    'payment_term': orders[0].payment_term and orders[0].payment_term.id or pay_term,

The same remark applies for line 41.

*Another point, more problematic but not under your responsibility: the field used as payment term for suppliers is the same as for customers. Obviously setting a payment term for the sales you'll make for a given partner doesn't assume any reciprocity when you will purchase to him. Those 2 different concepts should be depicted in different fields...

It was already there in the onchange of partner in supplier invoices for example, but i'd rather see merge proposals fixing this lack in the design rather that proposals using and going further in that bad idea.

*Last but not least: i'm not sure this enhancement should be integrated in the core. I'd see it more like an optional module available for those who need it. So i think you should bundle it as a module overwriting the existing behavior and publish it on apps.openerp.com (of course if you need to refactor some code during that process, we will gladly review/accept your merge proposals).

The reason why i have such a feeling is that i believe the added-value won't be that huge compared to the effort and complexity we will add.

So for these reasons, i'm rejecting this merge proposal. I hope i made it clear and that it won't slow you down for your next contributions!

Best Regards,
Quentin