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

8.0 improve selection of invoices to pay, solves #93 #110

Merged
merged 6 commits into from
Apr 8, 2015

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Jan 22, 2015

This PR has already been extensively discussed in acsone#5.

It provides:

  • an improvement of the criteria to select invoices to pay
  • a button on payment orders with transfer moves to show the partially reconciled transfer moves in one click, therefore providing a path to reconcile and writeoff such partial payments; this is particularly useful in the presence of cash discount and fixes Problem adding partially reconciled moves to a payment order #93

@@ -45,9 +46,33 @@ def default_get(self, field_list):
def extend_payment_order_domain(self, payment_order, domain):
if payment_order.payment_order_type == 'payment':
domain += [('account_id.type', 'in', ('payable', 'receivable')),
('amount_to_pay', '>', 0)]
('credit', '>', 0)]
Copy link
Member

Choose a reason for hiding this comment

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

This is the main reason why we can't add refunds. We can redo this making:

...
'|',
'&',
('credit', '>', '0'),
('type', '=', 'in_invoice'),
'&',
('credit', '<', '0'),
('type', '=', 'in_refund')]

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate the scenario that causes problems for you? In my understanding credit or debit can never be negative in Odoo.

So we have the following:

  • supplier invoice: credit > 0 on payable account
  • supplier refund: not something to be paid on a credit transfer order
  • customer invoice: not something to be paid on a credit transfer order
  • customer refund: credit > 0 on receivable account

@alexis-via
Copy link
Contributor

@sbidoul I tested your PR and it works well.

But I have a question: I see you added a button "Partial reconciles counter" on payment order to see the number of lines that you need to reconcile manually. Don't you think that we should be able to do it automatically ? I mean, if I have the module account_banking_payment_transfer:

  1. I create a supplier invoice amount 100 for agrolait
  2. I create a supplier refund amount 20 for agrolait
  3. I partially reconcile the invoice and refund (reconcile mark A1)
  4. I create a payment order and I select the account move line of the invoice of agrolait: it creates a payment line of 80 => your PR does that very well.
  5. when I valiade the payment order, an account move will be generated from agrolait's supplier account to the transfer account for an amount of 80. We should be able to delete the reconcile mark A1 and re-create a new reconcile mark A2 with the same lines as A1 plus the line of the transfer move: if all the lines of the reconcile mark A2 are balanced, A2 will be a "full reconcile" mark (it is the case in this scenario) ; otherwise, A2 will be a partial reconcile mark (like A1), and we will be able to select the invoice again in another payment order to pay the residual.

I think it is possible to develop that. And I think it's what accountants would expect. I can help you implement that if you think it's possible.

@sbidoul
Copy link
Member Author

sbidoul commented Jan 25, 2015

Hi Alexis,

Yes the scenario you describe in 5) should be automatic. Actually, I thought it was the case already. So if it's not we should develop it (in another PR I suppose).

But the scenario that led us to create that button is the following (see also #101):

  • accountant selects invoice to pay
  • accountant modifies the amount to pay on the payment line because he can benefit from a discount for early payment (actually, we are preparting a set of modules that help doing that)
  • the system does the transfer move and a partial reconcile with the invoice
  • the residual amount must be put on an income account: the new button introduced in this PR is there to facilitate that by giving one click access to the partially reconciled transfer moves so the user can do the write-off easily and make sure the invoice will not be proposed again for payment

@adrienpeiffer
Copy link
Contributor

@alexis-via changes in b5e571f allow automatic reconciliation with the transfer move even if move line linked on the payment line is already partially reconciled

@matthieuchoplin
Copy link

Hi,
thanks for your work.
I would like to add some tests in the same idea of this PR: #136 according to the test scenario described by @alexis-via above.
I cannot checkout this PR at the moment (because of write access). Do you know an other way to do it?
I think that with automated tests, it will be easier to decide to merge a PRs.
What do you think?

@sbidoul
Copy link
Member Author

sbidoul commented Mar 2, 2015

@matthieuchoplin thanks a lot for proposing to add test. This is most welcome.

Since the test you propose is specific to this PR, I suggest you do it as a PR to branch acsone:8.0-bank-payment-fix93-sbi. I'll review it and merge it into this PR.

@sbidoul sbidoul force-pushed the 8.0-bank-payment-fix93-sbi branch from 7e1fc25 to b3c7061 Compare March 4, 2015 10:15
@sbidoul
Copy link
Member Author

sbidoul commented Mar 4, 2015

I committed a more sophisticated filter on receivables to filter out partially reconciled customer credit notes.

And rebased to get the latest pep8 fixes.

@matthieuchoplin
Copy link

Test have been proposed for the use case stated in #93 here acsone#6

sbidoul and others added 5 commits March 6, 2015 18:19
This fixes OCA#93.

- filter invoices that are already in draft and open payment orders.
- use residual amount to care for partial reconciliations and align with official 8.0
…nciled then write new values on this partial reconcile
@sbidoul sbidoul force-pushed the 8.0-bank-payment-fix93-sbi branch from 830b27d to 0e02b7e Compare March 6, 2015 17:21
@sbidoul
Copy link
Member Author

sbidoul commented Mar 6, 2015

The tests from @matthieuchoplin are merged and I rebased.

@sbidoul
Copy link
Member Author

sbidoul commented Mar 9, 2015

@pedrobaeza @alexis-via your comments have been cared for. This is working fine in production at several customers. Looking forward to your reviews.

@matthieuchoplin
Copy link

I would be happy to have this one merged and carry on the improvements in other PRs.
@pedrobaeza : do you still have concern regarding the selection of customer credit notes?
If so, I am not sure about the use case to test, could you please write it here?
Thanks

@pedrobaeza
Copy link
Member

Well, for now it can be merged by my part, because I need to go deeper to solve my problem, but there's not related to this PR.

👍

Let's see if @alexis-via also agrees about merging.

@matthieuchoplin
Copy link

Hi,
FYI, I am also using this code in production.
@alexis-via: do you agree for merging this branch into OCA:8.0?

@alexis-via
Copy link
Contributor

yes, let's go 👍

pedrobaeza added a commit that referenced this pull request Apr 8, 2015
8.0 improve selection of invoices to pay, solves #93
@pedrobaeza pedrobaeza merged commit 723d7e8 into OCA:8.0 Apr 8, 2015
@sbidoul sbidoul deleted the 8.0-bank-payment-fix93-sbi branch April 13, 2015 14:06
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