banking-addons-team team mailing list archive

["Stefan Rijnhart \(Therp\)" <stefan@xxxxxxxx>]

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
Your team Banking Addons Core Editors is subscribed to branch lp:banking-addons.