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

fix(accounts): validate payment entry references with latest data. #31166

Merged
merged 15 commits into from
Jun 13, 2023

Conversation

dj12djdjs
Copy link
Collaborator

@dj12djdjs dj12djdjs commented May 27, 2022

Closes: #31138

Problem:

The system allows multiple draft payment entries to be made for a single invoice, which is fine, but when the first payment entry is submitted, the outstanding amount of the invoice in the second payment entry isn't accurate anymore. But the system still allows the user to submit the entry, resulting in negative outstanding amounts.

Solution:

During validation of allocated amount in the payment entry, update each row in references with latest information before comparing amounts. If the user tries to submit another payment entry but the corresponding invoice has already been fully paid, the system would throw an error to let the user know. If the invoice has been already partly paid, the user would be told the same and would be asked to use the 'Get Outstanding Invoice' button to get the latest outstanding amount.

Unrelated: I discovered fetching documents for advance payment via payment entry form was also broken. Fixed in 1ca20b9

@dj12djdjs
Copy link
Collaborator Author

I'm not sure if the tests are failing from this PR. I'll have a further look today.

@dj12djdjs
Copy link
Collaborator Author

I really don't know why those two tests keep failing. They don't fail on my local machine.

@codecov
Copy link

codecov bot commented Jun 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (develop@a3ea985). Click here to learn what that means.
The diff coverage is 89.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop   #31166   +/-   ##
==========================================
  Coverage           ?   64.11%           
==========================================
  Files              ?      811           
  Lines              ?    61870           
  Branches           ?        0           
==========================================
  Hits               ?    39669           
  Misses             ?    22201           
  Partials           ?        0           
Impacted Files Coverage Δ
...xt/accounts/doctype/payment_entry/payment_entry.py 84.01% <89.47%> (ø)

@dj12djdjs
Copy link
Collaborator Author

@deepeshgarg007 I've added another check incase the a reference get's partially allocated between two payment entries.
I'm using the outstanding_amount to check this.
https://github.com/dj12djdjs/erpnext/blob/1ef44c815743d6c97dd7efe270730a4782f170af/erpnext/accounts/doctype/payment_entry/payment_entry.py#L174-L175

I'm a confused why get_payment_entry makes the reference rows in this way. For example I create:
image

Output:
image

Shouldn't the outstanding be the same as the sales invoice outstanding (100) and the allocated be 50 ?

@stale
Copy link

stale bot commented Jun 29, 2022

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jun 29, 2022
@stale
Copy link

stale bot commented Jul 15, 2022

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jul 15, 2022
@stale stale bot closed this Jul 18, 2022
@dj12djdjs dj12djdjs reopened this Jul 18, 2022
@stale stale bot removed the inactive label Jul 18, 2022
@stale
Copy link

stale bot commented Aug 3, 2022

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Aug 3, 2022
@stale stale bot closed this Aug 6, 2022
@rtdany10
Copy link
Contributor

@dj12djdjs @deepeshgarg007

@rtdany10
Copy link
Contributor

rtdany10 commented Oct 1, 2022

@dj12djdjs @deepeshgarg007 any update on this?

@dj12djdjs
Copy link
Collaborator Author

@rtdany10 I think my solution to this isn't getting accepted. I'll think about it and try submitting a different solution.

@stale stale bot removed the inactive label Jun 12, 2023
@github-actions github-actions bot removed the accounts label Jun 12, 2023
deepeshgarg007
deepeshgarg007 previously approved these changes Jun 13, 2023
erpnext/accounts/doctype/payment_entry/payment_entry.py Outdated Show resolved Hide resolved
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
@anandbaburajan
Copy link
Contributor

Thanks @dj12djdjs!

@deepeshgarg007 deepeshgarg007 merged commit 20de27d into frappe:develop Jun 13, 2023
mergify bot pushed a commit that referenced this pull request Jun 13, 2023
…31166)

* test: payment entry over allocation.

* fix: validate allocated_amount against latest outstanding amount.

* fix: payment entry get outstanding documents for advance payments

* fix: only fetch latest outstanding_amount.

* fix: throw if reference is allocated

* test: throw error if a reference has been partially allocated after inital creation.

* chore: test name

* fix: remove unused part of test

* chore: linter

* chore: more user friendly error messages

* fix: only validate outstanding amount if partly paid and don't filter by cost center

* chore: minor refactor for doc.cost_center

Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>

---------

Co-authored-by: Anand Baburajan <anandbaburajan@gmail.com>
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
(cherry picked from commit 20de27d)
mergify bot pushed a commit that referenced this pull request Jun 13, 2023
…31166)

* test: payment entry over allocation.

* fix: validate allocated_amount against latest outstanding amount.

* fix: payment entry get outstanding documents for advance payments

* fix: only fetch latest outstanding_amount.

* fix: throw if reference is allocated

* test: throw error if a reference has been partially allocated after inital creation.

* chore: test name

* fix: remove unused part of test

* chore: linter

* chore: more user friendly error messages

* fix: only validate outstanding amount if partly paid and don't filter by cost center

* chore: minor refactor for doc.cost_center

Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>

---------

Co-authored-by: Anand Baburajan <anandbaburajan@gmail.com>
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
(cherry picked from commit 20de27d)

# Conflicts:
#	erpnext/accounts/doctype/payment_entry/test_payment_entry.py
deepeshgarg007 pushed a commit that referenced this pull request Jun 13, 2023
…31166)

fix(accounts): validate payment entry references with latest data. (#31166)

* test: payment entry over allocation.

* fix: validate allocated_amount against latest outstanding amount.

* fix: payment entry get outstanding documents for advance payments

* fix: only fetch latest outstanding_amount.

* fix: throw if reference is allocated

* test: throw error if a reference has been partially allocated after inital creation.

* chore: test name

* fix: remove unused part of test

* chore: linter

* chore: more user friendly error messages

* fix: only validate outstanding amount if partly paid and don't filter by cost center

* chore: minor refactor for doc.cost_center

Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>

---------

Co-authored-by: Anand Baburajan <anandbaburajan@gmail.com>
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
(cherry picked from commit 20de27d)

Co-authored-by: Devin Slauenwhite <devin.slauenwhite@gmail.com>
anandbaburajan added a commit that referenced this pull request Jun 13, 2023
…ackport #31166) (#35674)

* fix(accounts): validate payment entry references with latest data. (#31166)

* test: payment entry over allocation.

* fix: validate allocated_amount against latest outstanding amount.

* fix: payment entry get outstanding documents for advance payments

* fix: only fetch latest outstanding_amount.

* fix: throw if reference is allocated

* test: throw error if a reference has been partially allocated after inital creation.

* chore: test name

* fix: remove unused part of test

* chore: linter

* chore: more user friendly error messages

* fix: only validate outstanding amount if partly paid and don't filter by cost center

* chore: minor refactor for doc.cost_center

Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>

---------

Co-authored-by: Anand Baburajan <anandbaburajan@gmail.com>
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
(cherry picked from commit 20de27d)

# Conflicts:
#	erpnext/accounts/doctype/payment_entry/test_payment_entry.py

* chore: resolve conflicts

* chore: resolve more conflicts

* chore: don't validate allocated amount in case of donation

---------

Co-authored-by: Devin Slauenwhite <devin.slauenwhite@gmail.com>
Co-authored-by: Deepesh Garg <deepeshgarg6@gmail.com>
Co-authored-by: Anand Baburajan <anandbaburajan@gmail.com>
frappe-pr-bot pushed a commit that referenced this pull request Jun 14, 2023
## [14.27.2](v14.27.1...v14.27.2) (2023-06-14)

### Bug Fixes

* `enqueue_after_commit` wherever it makes sense (backport [#35588](#35588)) ([#35590](#35590)) ([e505516](e505516))
* `TypeError` in Closing Stock Balance ([32e5bbb](32e5bbb))
* **accounts:** validate payment entry references with latest data. ([#31166](#31166)) ([4add1b4](4add1b4))
* added process loss in job card ([6a21d61](6a21d61))
* allow user to set rounding loss allowance for accounts balance ([cf14858](cf14858))
* attribute error on payment reconciliation tool ([25b3c77](25b3c77))
* based on status_update.py update opportunity status to converted… ([#35145](#35145)) ([dee8275](dee8275))
* calculate wdv depr schedule properly for existing assets [v14] ([#35613](#35613)) ([feb5d00](feb5d00))
* conflicts ([2060a00](2060a00))
* CSS not applied to product title ([#35630](#35630)) ([2cf871c](2cf871c))
* don't set default payment amount in case of invoice return ([#35645](#35645)) ([79483cc](79483cc))
* Lower deduction certificate not getting applied ([#35667](#35667)) ([6f59fa9](6f59fa9))
* Make difference entry button not working ([#35622](#35622)) ([043815e](043815e))
* make showing taxes as table in print configurable (backport [#35672](#35672)) ([#35678](#35678)) ([f39ae9d](f39ae9d))
* Payment against credit notes will be considered as payment against parent invoice in Accounts Receivable/Payable report ([#35642](#35642)) ([81ef2ba](81ef2ba))
* Project in item-wise sales register ([#35596](#35596)) ([7737b90](7737b90))
* Stock Reconciliation document update while reposting ([8b617fb](8b617fb))
* test case ([7af0380](7af0380))
* Update de.csv ([#35278](#35278)) ([2077f6e](2077f6e))
* Validation for delivery date in Sales Order ([#35597](#35597)) ([4a8ce22](4a8ce22))
frappe-pr-bot pushed a commit that referenced this pull request Jun 14, 2023
## [13.51.2](v13.51.1...v13.51.2) (2023-06-14)

### Bug Fixes

* **accounts:** validate payment entry references with latest data. (backport [#31166](#31166)) ([#35674](#35674)) ([4d4f218](4d4f218))
* Asset Depreciation Ledger Report - Add Total Row Checkbox Enabled ([3831c79](3831c79))
* calculate wdv depr schedule properly for existing assets [v13] ([#35615](#35615)) ([97f4af8](97f4af8))
* CSS not applied to product title (backport [#35582](#35582)) ([#35635](#35635)) ([1b69b37](1b69b37))
* don't set default payment amount in case of invoice return (backport [#35645](#35645)) ([#35648](#35648)) ([8e3636f](8e3636f))
* Lower deduction certificate not getting applied ([#35667](#35667)) ([c2bf8e3](c2bf8e3))
* make showing taxes as table in print configurable ([#35672](#35672)) ([4c2c037](4c2c037))
* Project in item-wise sales register ([#35596](#35596)) ([9d5b500](9d5b500))
* savepoint policy assignment submission, log errors & inform the user about failures ([#35507](#35507)) ([4a35ff0](4a35ff0))

### Performance Improvements

* refactor `get_all_nodes` in Org Chart ([986a90e](986a90e))
@ruchamahabal
Copy link
Member

ruchamahabal commented Jun 16, 2023

Payment Entry against Employee Advance (HR) stopped functioning after this PR:
Tests missing for Payment Entry against Employee Advance. Only Employee Advance -> Journal Entry flow gets tested in HR. Will add tests

image image

Fixed in: #35741

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Payment overallocation against invoice
5 participants