Code review comment for lp:~akretion-team/banking-addons/70-fully-handle-payment-types

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Thanks for your response, Alexis. I see you honoured most of my requests. As for purchase do_merge, it's OK to leave it for another proposal.

I am still not convinced of using the payment type, instead of the payment mode. I think it is safe to say that the payment type is an arbitrary technical detail of the way that payment orders are exported for each bank account, but then again so is the actual bank account. However, the first option leads to a big hassle when a bank changes the payment type that it supports (which we see now in the transfer to SEPA and we will see in the future with SEPA updates). The latter option is transparent in that case: you just change this technical detail in the payment mode.

You mention a valid example of course, of a company exporting to one of a couple of bank accounts depending on their liquidity situation. But this is not an argument for implementing this feature on the wrong level. Instead, this is supported transparently in the following manner: link the same payment mode on every partner that you want to include in payments from this pool of bank accounts. This particular payment mode is of course tied to a specific bank account itself, but if the user wants to change the bank account, they can simply change the payment mode on the payment order after selecting the payables, switching to the bank account that they want to use this time.

On the contrary, my use case of a company that wants to pay some suppliers from account A and some suppliers from account B is not supported at all in the current implementation. It would be when you implement it on the level of the payment mode.

It looks to me as if implementing this on the level of the payment mode leads to greater flexibility and no real problems when the payment type should change. So I'm still asking you to reconsider.

review: Needs Fixing

« Back to merge proposal