Hi Pieter, again, thank you for your fast and perceptive response! I agree with most of your remarks. Please find some additional comments below. On 27-04-11 12:30, Pieter J. Kersten (EduSense BV) wrote: > Generic: > I can see a lot of technical improvements for v6. However, there were > more changes than just the technical ones. For starters, v6 introduced > the voucher layer between account and account_payment. I'm missing > this in your proposal. Is this code used in production? If so, > what are the experiences? How does it coexist/cooperate with the > payment/voucher system? This code is used in a couple of small setups. We knowingly ignored account_voucher for this version in order to simplify verification of the operation of the migrated code. The process of payments (through payment orders) and bank reconciliation works fine without it, but a logical next step would be looking at integrating account_voucher. > account_banking/account_banking.py: @-1218,+1210 -1239,+1231: > Not sure if this is an improvement. I'll leave it for now. > We felt the error was a bit too harsh on the account number, given the poor performance of the online iban service for Dutch bank accounts. It is a matter of taste. We will be looking at integrating a paid service for iban/bic lookups. > account_banking/sepa/iban.py: > This change is not conform specs. Any sources I can check for this? > Will leave it untill more info arrives. This change was triggered by a real transaction. We looked to Wikipedia for confirmation (http://en.wikipedia.org/wiki/International_Bank_Account_Number lists German bbank as 18n, no check digit). Ecbs has the length listed as 18!n as well (http://www.ecbs.org/iban/germany-bank-account-number.html) > account_banking/wizard/bank_import/@@ -544,+545: > This is unneeded. You don't use it anywhere. Won't take it. > No, it is used in line 931, although I did not introduce it. > account_banking/wizard/bank_import/@@ -795,+798: > Can't see any value in this change. Just make sure that > isinstance(transaction.remote_account, (str, unicode)) is True > which must be done in the import parsers. In the ABNAmro filter the remote owner, but not the remote_account is retrieved in case of a payment terminal transaction. I would rather check here than having to initialize remote_account as an empty string. > account_banking/wizard/banktools/get_or_create_partner: > Ok, backporting the unique construct to v5. > Having some difficulties with the defensive field resolving code. > Is this really needed? Never faced any problems with it. Yes, if we take requiredness into account, some checks are superfluous. I would suggest to just keep user.company_id.partner_id.country and user.company_id.partner_id.country.id or False > account_banking/wizard/banktools/create_bank_account: > What is the purpose of these changes? > I can't see a semantic value for a catch-all 'UNKNOWN' bank. > Problem is, when the bank gets known, chances are it won't be > the same as "the other" accounts with UNKNOWN tied to it. > Better leave it. Indeed, might be a mistake on my part regarding requiredness Regards, Stefan. -- Therp - Maatwerk in open ontwikkeling Stefan Rijnhart - Ontwerp en implementatie mail: