banking-addons-team team mailing list archive

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

Review: Needs Fixing

Thanks for the changes already! Migration script looks good to me. With respect to this, there is a small detail to discuss. Some installations might not run the account_banking_payment_export refactoring. If they upgrade immediately to this version or later, the migration script will not run because you return when no version has been passed to the migration function. We can either remove this, which means that the migration function will run even on first installation, or we also put the same migration script in account_banking_payment. This might seem a bit retarded, but it ensures that it will only run on upgraded databases. It may run twice, but that will have no effect.

As for suggesting not to refactore into _prepare_method: I think it is a great improvement, but it is a bad idea to make changes in a piece of code that you are refactoring out to another method in the same merge.

Assignment of communication2: basically, the original code assigns the unstructured reference to communication2 and you need to change this around. When I take a really close look you have in fact changed the code in this respect but because the whole block has been moved I am having difficulty tracking the changes. One thing I can see is that the code still assigns values to communcation2 but it does not push this key to the dictionary. Would still prefer a proper diff on this piece without the refactoring.

And I still need you to do the simple refactoring in the clieop3 module, checking for the line's state when deciding to put in a structured or an unstructured reference.

Another detail: this module enlarges the size of 'communication2' from 64 to 128. Wouldn't it make sense to enlarge the size of 'communication' instead now, or do away with this entirely? I take it that you are going to concatenate communcation+communication2 in SEPA when you encounter an unstructured reference.

-- 
https://code.launchpad.net/~akretion-team/banking-addons/70-communication-communication2-clarify/+merge/212271
Your team Banking Addons Core Editors is subscribed to branch lp:banking-addons.