Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[14.0][ADD] account_payment_order_grouped_output: Generate grouped entries from payment orders + [FIX] account_payment_order: Remove leftover options #1015

Merged

Conversation

pedrobaeza
Copy link
Member

@pedrobaeza pedrobaeza commented Jan 5, 2023

[FIX] account_payment_order: Remove leftover options

They are ignored since #979, and there can't be equivalent.

[ADD] account_payment_order_grouped_output: Generate grouped entries from payment orders

This module adds an option to generate extra grouped moves for the payment orders since the refactoring done to use native Odoo payments.

This serves for easing the reconciliation on bank statements of large payment orders, handling them as one or several journal entries according payment date.

@Tecnativa

@sbidoul
Copy link
Member

sbidoul commented Jan 5, 2023

The loss of grouping moves per payment date is a problem. When you have large amount for payments or direct debits, you can ask the bank to send one statement per batch per day so you don't need to reconcile individual payments, and instead reconcile all payments of the day at once.

@sbidoul
Copy link
Member

sbidoul commented Jan 5, 2023

Is it possible to generate an account.move by date that reconciles all corresponding moves generated from the account.payment. That will still generate more account moves than before (that we can't avoid) but at least only one per date will stay open to ease reconciliation.

They are ignored since OCA#979, and there can't be equivalent.
@pedrobaeza pedrobaeza force-pushed the 14.0-account_payment_order-leftovers branch from e1d066d to d0f8530 Compare January 5, 2023 14:52
@pedrobaeza
Copy link
Member Author

Why not using https://github.com/OCA/account-reconcile/tree/14.0/account_reconcile_payment_order instead?

Anyway, you can do such improvement if you consider it, but I see several problems with that approach:

  • You get "anonymous" balances.
  • The already mentioned extra entries.
  • The bank not always group the same way the transactions. For example, I always do my debit order with all of them at fixed date, but the bank randomly splits the incomes into 2 or 3 transactions.

@pedrobaeza
Copy link
Member Author

Ah, and another thing: reintroducing this one will again put a lot of code needed for doing the stuff, making again the module complex. If you still want this option, I would go for a new module on top of this one, but I beg you to reconsider other options. I think account_reconcile_payment_order can be improved to match things by payment date instead and this will make the trick.

@sbidoul
Copy link
Member

sbidoul commented Jan 5, 2023

Do you have experience using account_reconcile_payment_order to reconcile 10 000 moves?

@pedrobaeza
Copy link
Member Author

I can't say for such amount certainly. I think it would be more a limitation from the reconciliation widget (trying to paint all the lines by JS), than other problem.

@sbidoul
Copy link
Member

sbidoul commented Jan 5, 2023

Actually, it can't work for performance reasons, because people reconcile bank statements interactively, while they are happy to generate payment orders with background jobs.

No, IMO this feature must stay in the base module.

I was kind of hoping you'd do it, Pedro, because frankly, as much as I appreciate your effort and I agree with the overall change, you merged a bit quickly while people were still reviewing.

@pedrobaeza
Copy link
Member Author

pedrobaeza commented Jan 5, 2023

I will reintroduce the option, but in a separate module.

@sbidoul
Copy link
Member

sbidoul commented Jan 5, 2023

Thanks you very much!

I think having it in a separate module only makes it harder for the users to discover the feature, though.

The bank not always group the same way the transactions. For example, I always do my debit order with all of them at fixed date, but the bank randomly splits the incomes into 2 or 3 transactions.

In my experience for large volumes, bank let you choose how statements work wrt payment orders. Typically one move per date in the statement, with exceptions in separate payment return messages.

@pedrobaeza
Copy link
Member Author

In my experience for large volumes, bank let you choose how statements work wrt payment orders. Typically one move per date in the statement, with exceptions in separate payment return messages.

That's you that are a privileged ones having proper banks. Here in Spain, there's an incredible technical debt. They haven't even implemented the CAMT bank statement...

…from payment orders

This module adds an option to generate extra grouped moves for the payment
orders since the refactoring done to use native Odoo payments.

This serves for easing the reconciliation on bank statements of large payment
orders, handling them as one or several journal entries according payment date.
@pedrobaeza pedrobaeza changed the title [14.0][FIX] account_payment_order: Remove leftover options [14.0][ADD] account_payment_order_grouped_output: Generate grouped entries from payment orders + [FIX] account_payment_order: Remove leftover options Jan 6, 2023
@pedrobaeza
Copy link
Member Author

I have already included the module account_payment_order_grouped_output for creating the grouped entries. Please check.

Copy link

@qgroulard qgroulard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (code and functional review)

@pedrobaeza
Copy link
Member Author

@sbidoul do you see correct this approach then to merge it?

@sbidoul
Copy link
Member

sbidoul commented Jan 24, 2023

I'll look into it later this week.

Copy link

@omar7r omar7r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Functional test. It's slowest that before but the final result, unique move to ease the reconciliation works.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member Author

Let's merge it and we can amend anything later. Yes, it's a bit slower as there's double move + reconcilation, but the idea on v16 is to avoid this grouped move and handle it in the reconciliation module, plus handle in_payment state, that I'm on it.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-1015-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 596b519 into OCA:14.0 Feb 8, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 4bbf854. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 14.0-account_payment_order-leftovers branch March 4, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants