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

Thanks for the review, Stefan.

- I'll look into the workflow issue. It's not immediately obvious to me why
it does not work.

- 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?

- I'm fine with the dependency on base_iban

- 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). 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.

Regarding the check for the presence of an account number, I'm fine with
that.

In the same line of thought, I also saw that Alexis put a feature to
preserve the history of exported files in the sepa module. That's also a
generic requirement that could live in account_payment_export.

-sbi

On Sat, Sep 7, 2013 at 3:40 PM, Stefan Rijnhart (Therp) <email address hidden>wrote:

> Review: Needs Fixing
>
> Hi Stéphane,
>
> thank you for your great work and apologies for the late review. My first
> impression is that the refactoring is very solid. Also, and importantly,
> upgrading any existing installation of banking-addons should be
> straightforward because of the dependency structure, as long as the new
> module is in the addons path.
>
> Of course, there are a few details to discuss:
>
> - Having account_banking_payment installed, rerouting signal done to state
> 'send' works when exporting the payment order, but when the reconciliation
> in account_banking attempts to finish the payment order workflow using the
> same signal, the payment order stays in the 'send' state. I'm sure there is
> a way to get this to work using the workflow engine, but depending on
> whether you are a fan of the workflow engine you could also consider
> creating a method on the payment order model that triggers 'sent' in
> account_banking_payment and 'done' in account_banking_payment_export.
>
> - As far as I know, there is no way to craft a migration script that
> guarantees to move an xml-id to a *newly* installed module that has no
> common dependency with the originating module. You should probably keep the
> old module name as part of the manual order type xml-id, and we'll move it
> back on the next OpenERP major release migration.
> - 'name' in the __openerp__.py manifest is the same for both payment
> modules, which makes it hard to distinguish in the interface
>
> - There is a missing dependency on base_iban, due to the bank type
> specified in the manual payment mode type. As depending on base_iban is a
> little Eurocentric, you could consider making the payment mode types
> editable in the interface (and its data 'noupdate') + let the SEPA export
> module depend on base_iban itself (or depend on base_iban now and we'll
> keep the latter in mind).
>
> - Are you very strongly against keeping our version line2bank in the
> export module? In the days of transitioning to SEPA, where multiple account
> numbers per partner are not uncommon, I think this is going to hurt a
> number of people. One option is to at least refactor it into a separate
> module. But I would prefer to have it in the export module for simplicity.
>
> Not for this review but on a related note, I want to implement a general
> check on the presence of an account number in each line. Would you agree to
> have me put this in the export module once it is merged? It is after all,
> essential for the export of any order to a bank. Alexis already resorted to
> putting one in the SEPA module itself, but it is useful for all export
> formats.
>
> --
>
> https://code.launchpad.net/~acsone-openerp/banking-addons/ba-70-payment-export-refactoring/+merge/179543
> You proposed
> lp:~acsone-openerp/banking-addons/ba-70-payment-export-refactoring for
> merging.
>