Code review comment for lp:~acsone-openerp/banking-addons/ba-70-payment-export-refactoring

On 09/07/2013 05:57 PM, Stéphane Bidoul (Acsone) wrote:
> Thanks for the review, Stefan.
>
> - I'll look into the workflow issue. It's not immediately obvious to me why
> it does not work.

Hi Stéphane,

Sorry, problem between keyboard and chair here, as I was doing the
reconciliation of the payment order (admittedly fueled by my
unfamiliarity of using the same workflow signal for multiple
transitions). It works!

> - regarding the xml_id, if I keep "account_banking.manual_bank_transfer",
> it requires to have account_banking installed, which is a dependency we
> want to avoid. Or do I overlook something?

Yes, apparently. I thought we could pull this one off, as official
modules do use references to other modules, combined with the fact that
internally you can assign arbitrary 'module' labels in ir_model_data.
But when I test it, the installation process complains that
"account_banking.manual_bank_tranfer" refers to an uninstalled module.

I'm not sure what to do. If I upgrade an existing setup that contains a
payment mode with this type, the process breaks with 'null value in
column "type" violates not-null constraint'.

> - with line2bank, do you mean the payment_order_create class? I left it
> alone because it was pulling the payment_term filter which is unrelated to
> the pure payment export functionality (and also because I was not too sure
> of its purpose).

It is actually not line2bank itself, but making it work properly. The
fix is twofold. First, it is reading suitable bank types from the
payment mode type. You kept this bit. The other part is passing the
payment order mode to line2bank. As you can see here, OpenERP does not
do this:
http://bazaar.launchpad.net/~openerp/openobject-addons/7.0/view/head:/account_payment/wizard/account_payment_order.py#L72.

> Does it make sense to keep the
> payment_order_create.create_payment() method in account_payment_export and
> leave the rest in account_payment? Perhaps you can help me explaining it's
> purpose in the module manifest.

Account_banking_payment overwrites payment_order_create.create_payment()
but you don't have to do that if you agree to apply the following trick:
wrap around create_payment to extract the payment mode id and put it in
the context + call super(). Then also wrap around line2bank and extract
the id from the context again, if the payment mode id passed to
line2bank is None + call super() with it.

(the payment term filter is an optional way to use the payment term to
preselect the manner of payment through partner settings or at invoicing
time. This is particularly useful for debit orders because of the
required mandates per partner)

Cheers,
Stefan.