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

[IMP] account_payment_order: Set orders to done on payment reconciliation #860

Merged

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Oct 5, 2021

@luc-demeyer @pedrobaeza this is the close orders on reconciliation PR. But I don't really get what you mean in point 2 of #784 (comment) - isn't this in terms of reconciliation the same, just with a different account?

When all is ironed out here, this should be very simple to port to 13 and 14

@pedrobaeza
Copy link
Member

What I want to move is:

  • Instead of generating bank.payment.line, to generate account.payment.
  • On v14, it allows all the options of the transfer account (including specific account and journal).
  • Use the standard flow, appearing as blue lines in reconciliation and being able to have them in "In process of payment" state in the invoice.

@hbrunn
Copy link
Member Author

hbrunn commented Oct 5, 2021

your message is about 14, right?

@pedrobaeza
Copy link
Member

It can be done as well in v13, but yes, one missing thing in this version is to respect the define transfer account.

@pedrobaeza
Copy link
Member

But yes, for v12 I don't think it's possible.

@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 5, 2021
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Reviewing the PR, this can be complementary to what I'm talking about, so OK for me.

@hbrunn hbrunn force-pushed the 12.0-account_payment_order-done_on_reconcile branch from 03bd82a to f7253cf Compare October 5, 2021 17:10
@hbrunn hbrunn force-pushed the 12.0-account_payment_order-done_on_reconcile branch 3 times, most recently from 9b5adaf to 7b0940a Compare October 6, 2021 09:42
@luc-demeyer
Copy link
Contributor

I see the code is already in process of being merged, hence my feedback comes a bit late:
imho we should not put the state back to uploaded once the order is done.
If the accountant removes a reconciliation than he usually knows which actions he needs to perform to redo the reconciles but he will not expect a status change on a done payment order.
I suggest to drop the def remove_move_reconcile(self) logic.

@pedrobaeza
Copy link
Member

It's still time for comments, Luc, as no merge command has been launched. I think the reconcile cancel logic is OK, as long as after reconciling again, it turns again to "Done" status, which is what seems to happen.

@hbrunn
Copy link
Member Author

hbrunn commented Oct 6, 2021

yes, when you reconcile those move lines again the state goes back to done. @luc-demeyer would you be satisfied by a test that proves this?

@luc-demeyer
Copy link
Contributor

@hbrunn @pedrobaeza
Even though the order comes back to 'done' after redoing the reconcile I think it will not be appreciated by the accountant to reopen a 'done' payment order.
Consider it as a TODO list for the accountant for his payments. Once it's done, it means to him that either the payment has been successfully executed or alternatively that he may even never perform the payment (in which case the open item will usually be reconciled via a Refund or a Journal Entry in a Miscellanuous Journal).
For V13 we have added a button to set a payment order to 'done', cf. 752da9a, which I think is a good idea to force a payment order to 'done'. I mentionned the partial reconciles in #784 (comment). Imagine following use case: a payment order is uploaded by the accountant assistant to the web banking application, the accountant connects to the web banking application and changes the amount of one of the payments because of e.g. there is a dispute on a delivery before signing and hence executing the payment. Once the bank statement is downloaded with this reduced amount we'll have a partial reconcile for that particular supplier invoice and this one will remain in that state until the dispute is resolved (could be easily half year later). For the accountant the payment order is 'done'. After dispute resolution the open balance will be paid via a new Payment Order (and that one will be automatically set to done with the logic in this PR).
Hence I think it would be useful to also cherry-pick 752da9a for V12.

@hbrunn
Copy link
Member Author

hbrunn commented Oct 6, 2021

thanks for the point of view of the accountant. Is everyone on board with removing the unreconcile functionality and adding 752da9a here?

@pedrobaeza
Copy link
Member

In my country, it's not possible to edit a sent SEPA order, and if it's the case, as you have said: you have the manual done button to set it again as done. I'm against removing such part, as it's the way to control things. Invoices work this way as well, removing its paid state when unreconciling.

@EvvFoxy
Copy link

EvvFoxy commented Oct 7, 2021

I see the code is already in process of being merged, hence my feedback comes a bit late: imho we should not put the state back to uploaded once the order is done. If the accountant removes a reconciliation than he usually knows which actions he needs to perform to redo the reconciles but he will not expect a status change on a done payment order. I suggest to drop the def remove_move_reconcile(self) logic.

@luc-demeyer @pedrobaeza @hbrunn From a functional point of view, I agree with Luc Demeyer. Undoing a reconciliation should never reopen a payment order. A reconciliation can be undone for different reasons, but IMHO should never lead to reopening a payment order.

@hbrunn
Copy link
Member Author

hbrunn commented Oct 7, 2021

now two people for don't mess with the state on unreconcile and two against. @StefanRijnhart would you care to break the tie?

@StefanRijnhart
Copy link
Member

@hbrunn I believe that in the case that the transfer move of a payment order is unreconciled after it was first reconciled it does not mean that the payment order is not done. I can only see this happen as a temporary, administrative situation so I tend to go with Luc and Els. Pedro's argument about the analogy with reopened invoices rings true on the technical level yet at the same time it will be much more common to have multiple invoices with the same amount, even for the same customer, than it is to have payment orders of the same amount so it makes much more sense to have this behaviour for invoices, but maybe not for payment orders.

@pedrobaeza
Copy link
Member

Ok, you have one vote more for the no state modification on unreconciling after Stefan's argumentation.

@hbrunn hbrunn force-pushed the 12.0-account_payment_order-done_on_reconcile branch from 80be36b to 49e1aab Compare October 9, 2021 03:03
@hbrunn
Copy link
Member Author

hbrunn commented Oct 9, 2021

now without reopening on unreconciliation. Thanks everyone involved!

@hbrunn hbrunn force-pushed the 12.0-account_payment_order-done_on_reconcile branch from 49e1aab to e00f7bb Compare October 9, 2021 06:25
@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

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 7d42b85 into OCA:12.0 Oct 11, 2021
@OCA-git-bot
Copy link
Contributor

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

rvalyi added a commit to akretion/l10n-brazil that referenced this pull request Oct 12, 2021
pedrobaeza added a commit to OCA/l10n-spain that referenced this pull request Oct 13, 2021
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
pedrobaeza added a commit to OCA/l10n-spain that referenced this pull request Oct 13, 2021
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
pedrobaeza added a commit to OCA/l10n-spain that referenced this pull request Oct 13, 2021
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
mbcosta pushed a commit to akretion/l10n-brazil that referenced this pull request Feb 3, 2022
mbcosta pushed a commit to akretion/l10n-brazil that referenced this pull request Apr 1, 2022
angelgarciadelachica pushed a commit to sygel-technology/l10n-spain that referenced this pull request Apr 25, 2022
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
renatonlima pushed a commit to akretion/l10n-brazil that referenced this pull request May 25, 2022
marcelsavegnago pushed a commit to Escodoo/l10n-brazil that referenced this pull request Jul 25, 2022
mileo pushed a commit to kmee/l10n-brazil that referenced this pull request Oct 16, 2022
Reyes4711-S73 pushed a commit to Studio73/l10n-spain that referenced this pull request Feb 8, 2023
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
Reyes4711-S73 pushed a commit to Studio73/l10n-spain that referenced this pull request Jun 19, 2023
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
ygcarvalh pushed a commit to kmee/l10n-brazil that referenced this pull request Mar 22, 2024
manuelregidor pushed a commit to sygel-technology/l10n-spain that referenced this pull request May 6, 2024
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
pedrobaeza added a commit to OCA/l10n-spain that referenced this pull request Jun 27, 2024
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
antoniospneto pushed a commit to Engenere/l10n-brazil that referenced this pull request Jul 25, 2024
antoniospneto pushed a commit to Engenere/l10n-brazil that referenced this pull request Jul 25, 2024
antoniospneto pushed a commit to Engenere/l10n-brazil that referenced this pull request Jul 26, 2024
antoniospneto pushed a commit to Engenere/l10n-brazil that referenced this pull request Jul 29, 2024
Roodin pushed a commit to Comunitea/l10n-spain that referenced this pull request Oct 24, 2024
The merge of OCA/bank-payment#860 and later
current CI failing has revealed that this module is repeating tests that
has nothing to do with the feature it contains, so we lighten up for
testing what is needed.
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.

7 participants