[8.0][revolution] Add support for grouping the payments, for deducing a refund from an invoice and remove storno

[alexis-via]

Here are some explanation about this important PR. This PR adds support for grouping the payments, which is a feature that I always needed and wanted to add in OpenERP, and this PR makes it possible at last !

Example1: I want to pay 2 invoices of the same supplier via SEPA credit transfer on 2015-09-30. With the current code, it will generate one SEPA Credit Transfer file with 2 payments inside. So, for the bank, it will be considered as 2 different payments ; if the bank charges per payment (most banks do that, at least in France), the cost is x2 !

Example2 (which is the scenario that motivate this PR): I have an agreement with my customers to invoice for each delivery and do one "direct debit" per month for all the invoices of the month. So I need to generate a direct debit file that has 1 payment corresponding to N invoices.

I have thought about many different ways of implementing this:

a) do the "grouping" inside the code that generate the bank file.
Advantage: no changes in "account_banking_payment_transfer", no change in the datamodel.
Disadvantage: when the user looks at the payment.line on the payment.order, it doesn't reflect the content of the bank file. The user could read the bank file itself to check the grouping, but the bank files are not easy to read !

b) have grouped "payment.line" (1 payment.line for N invoices)
Advantage : no changes in the code that generate the bank files
Disadvantage : I can't do a link from 1 payment.line to N account.move.line because the M2O field"move_line_id" goes from payment.line to account.move.line (I would need it the other way around !). That would also break account_banking_payment_transfer.

c) have 2 O2M fields on payment.order:

• 1 O2M field pointing to payment.line (current object)
• 1 O2M field pointing to bank.payment.line (new object). One bank.payment.line would group several payment.lines (and we would have a O2M field from bank.payment.line to payment.line, to keep track of the grouping)
  The bank file would be generated from the object bank.payment.line.
  Avantage : it's powerful, we can control the grouping and the user can check the content of the bank file by looking at the bank payment lines. No changes required in the module account_banking_payment_transfer.
  Disadvantages : it's an extension of the current datamodel, with the introduction of a new object.

So this PR implements option "c" !

The very good point of this PR is that there are almost no changes in the SEPA modules that generate the bank file ; the only change is that instead of looping on the payment lines, we loop on the bank payment lines.

This PR changes the dependency of the module account_banking_mandate to add a dependency on account_banking_payment_export. I don't know if it's a problem or not (I consider that "account_banking_payment_export" is the "base" module of this OCA project).

[pedrobaeza]

Thanks for explaining your rationale. I have copied-pasted on the first comment to have it as reference. I will check it deeply in the coming days, because I would go for approach a), but the disadvantage you mention is of course there. Anyway, the feature is needed, so it's very welcome.

I'll let you know my impressions.

[alexis-via]

@pedrobaeza Two more thing to mention about the advantages of option "c" vs option "a":

• with option "c", the code of the module that generate the payment files is almost unchanged. You just need to replace "line_ids" by "bank_line_ids" ! For example, see this PR on l10n-france https://github.com/OCA/l10n-france/pull/36/files that adapts the code of a bank file which is specific to France to this PR.
• with option C, I can develop a report based on payment.order which is able to show all the bank payment lines and, for each bank payment lines, the related payment lines (via the O2M field payment_line_ids) and therefore the related invoices. This is needed in my scenario, because we want to send to the customer a report that shows the list of invoices related to the monthly payment, so that he can understand how the debit amount was computed.

[alexis-via]

I am now working on adding the following feature in this PR: on a direct debit order, you should be able to select both customer invoices AND customer refunds, in order to debit the customer of the difference between his invoices and his refunds...

[alexis-via]

I have finished the work I wanted to carry out on this PR.

I didn't want to make so many changes initially, but adding support for grouping allowed easy addition of support for deducing refunds from invoices in payment... and all this code re-write motivated the removal of storno ! :)

So I'm waiting for your reviews now. I have a customer going live with this code on October 1st.

[pedrobaeza]

@alexis-via, please rebase the branch in order to make the review

[MOSAMAD]

👍

[brother-bernard]

👍

[pedrobaeza]

Please rebase this, @alexis-via, so that I can review it.

[alexis-via]

@pedrobaeza OK, I'll try to rebase, even if I know that it won't be easy. I'm using this branch in production for my 2 biggest customers on v8, so I can't must not break it and I need to be very careful. I'm very busy on "intrastat v3" this week, so it will probably wait at least one week.

[pedrobaeza]

Thanks. If you prefer, I can make the rebase and superseed your branch.

[alexis-via]

@pedrobaeza I can do it myself, I just need some time to do it properly... and my top priority at the moment is on intrastat.

[pedrobaeza]

I know your skills are enough for doing, but as you are busy, we need to hurry this up, that's why I told you about superseeding this 😉

[pedrobaeza]

Hi, @alexis-via, I really need you make any move: or release this to allow me to continue in a new branch, or rebase and check conflicts so that I can review it and maybe make a PR against your branch with improvements.

[alexis-via]

@pedrobaeza Yes ; I'll do it before Friday for sure.

[pedrobaeza]

Thanks!

[alexis-via]

I rebased it, but I haven't tested the result yet.

[alexis-via]

Hey, I removed the module "account_banking" in this "revolution" PR ; I was so happy to do that !

[pedrobaeza]

I'm checking this PR during these days. You should ask @StefanRijnhart or @hbrunn if they still use it.

[fclementic2c]

👍

[alexis-via]

@pedrobaeza Are you OK with this PR ?
On my side, all my customers on v8 are using it in production for SEPA credit transfer, SEPA direct debit and LCR (a French direct debit method in l10n-france)

[alexis-via]

Will 2016 be the year of the revolution ? :)

[sandrafig]

I really need this PR, so thank you so much @alexis-via for your work!

I've tried it on runbot and I have one question:

On Example 1 on your first comment, you say this PR will group the invoices of the same supplier, so for the payment order it will be considered as if was just one transaction, so the bank will charge you once instead of twice. Is that correct? Because I haven't been able to group the invoices in the payment order.

Thank you

Edit: OK, my bad, the Due date was different and that was why it wasn't grouping them.
👍

[alexis-via]

@pedrobaeza @sbidoul I think you are the only ones that:

1. have the "super-powers" on bank-payment to make a merge
2. know the code base of bank-payment and the stakes of this kind of changes in the code base
3. have the legitimacy to merge this PR (other contributors with super-power rights on this project will probably never dare merge this "revolution" PR)
   So either we merge this now, or we never merge it... it's up to you to decide. But I think there is no advantage in postponing the decision.

What I can say is that all must customers that use accounting on v8 are using this branch in production and are very satisfied.

I can also say that I can't contribute any more on bank-payment/8.0 if this branch is not merged as all my users are on this branch and it's too far away from the bank-payment/8.0 branch to make changes that work on this branch and on bank-payment/8.0.

Also we should merge this PR before we start working on porting bank-payment to v9, because it will make our life much simpler thanks to all the code that has been removed !

[pedrobaeza]

Hi, @alexis-via, I'm in my honeymoon and I have been delaying this a lot, but I promise you that I'll review it this week. But then I'll ask you to review then one PR about the usability in exchange 😉

[sbidoul]

Hi Alexis,

I'm fully in line with what you are proposing here and I'm ok with it from a code review perspective. I just have not found the time to test it. That last part will be done before end of January. cc/ @adrienpeiffer

[alexis-via]

@pedrobaeza OK, no problem !

[cdubuit]

👍 using this PR in production

[adrienpeiffer]

Why have you removed this check ?

[alexis-via]

The help pop-up of entry_posted says: 'Check this box to automatically post entries of this journal. Note that legally, some entries may be automatically posted when the source document is validated (Invoices), whatever the status of this field.'

When you validate an invoice or a bank statement in Odoo, the entry is automatically posted, even when entry_posted=False. I feel that it should be the same for the transfer move. It would be difficult to explain to accountants that this option is ignored for invoices and bank statements, and it is taken into account for the transfer move of a payment.

For me, the entry_posted option is taken into account for moves that are manually created by the user.

[pedrobaeza]

I agree in this point.

[sbidoul]

@alexis-via please preserve the existing behaviour. We have customers who depend on it, in combination with account_default_draft_move and account_move_batch_validate.

[pedrobaeza]

I don't mind to have it or not, although the reasoning of @alexis-via seems correct. But making this in a stable version can cause breaking some things, so I think we can keep it. I'm about merging this one when the question about account_banking removal gets answered, so I can change it while merging (I need also to rebase for allowing to merge). Do you agree?

[alexis-via]

@sbidoul For me, this is not so important. If it's a problem for your customers that I removed the check on move.journal_id.entry_posted, it's not a big deal, I can put it back (although I still think that it's a strange way of interpreting the option "entry_posted" on the journal.

[sbidoul]

Actually, what I find strange is that Odoo does not honor the flag for all journals. Anyway, in 9.0 it's gone for good, so that will end that debate 😄

[alexis-via]

ok :)

[pedrobaeza]

@alexis-via, why removing the account_banking module? Isn't that being used by people like @hbrunn or @StefanRijnhart?

[StefanRijnhart]

No, account_banking is not in use anymore for 8.0 so its removal is fine, but you should do it in a separate PR IMHO.

[pedrobaeza]

Yeah, but I don't want to force again @alexis-via to change the PR, because it has been a long journey. We can make 2 things: remove the removal while I merge and propose it later, or accept it in this PR as an exception. What do you think?

[StefanRijnhart]

Oh, I was thinking that it would help the review to get the removal out of the way, because the diff is now too long for Github to display. If we quickly remove it separately, I think the diff on Github will adapt automatically.

[pedrobaeza]

Are you going to review this PR? If so, I can propose another PR cleaning up account_banking module removal.

[StefanRijnhart]

@pedrobaeza I'll decide once I can see the diff if I review this one or not ;-) Account banking now to be removed in #248.

[pedrobaeza]

@alexis-via, I have made #250 for giving the last steps for merging this. Please give your bless. I close this one.