Code review comment for lp:~akretion-team/banking-addons/70-fully-handle-payment-types

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Thanks. The general architecture looks very neat.

Can I ask you why you chose the level of payment mode type to register on partners and invoices instead of the payment mode level? The payment mode type level does not cover the case where you have several payment modes of the same SEPA mode type but on different bank accounts. It also makes transitions between various SEPA modes difficult (for instance, when the bank switches from supporting version 03 to 04).

Other remarks:

- IMHO the sale/sale_stock/purchase modules should be set to autoinstall (=installed automatically when dependencies are fulfilled).
- Filtering seems very strict. Maybe allow for a transition by means of a checkbox on the payment mode (type) to indicate if you want to filter invoices by this exact payment mode or also select for invoices with no payment mode set yet. What do you think?
- Purchase module: maybe override do_merge and apply any payment mode found in the original purchase orders to the resulting merged order? If you merge purchase orders with this code, you don't get any payment mode in the result.
- I would have prefered module names with 'payment_mode' (like account_payment_mode_sale). Would you consider renaming them?
- You really should apply the reverse payment mode setting from the partner for refunds. With regards to that, the fields had better be called something like credit_payment_mode and debit_payment_mode instead of supplier_payment_mode and customer_payment_mode. Maybe combine this with a corresponding filter on 'debit' or 'credit' modes.
- What is the use of partner_bank_receivable? It does not seem related at all related to the rest of the changes, or to payment functionality in general.
- Moot point if you honour my first question, but I think it would be nice if the payment type tree view could show inactive items by default, by passing {'active_test': False} in its act_window's context.

review: Needs Fixing

« Back to merge proposal