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: payment request rounding in multi-currency and on status update #37123

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

blaggacao
Copy link
Collaborator

@blaggacao blaggacao commented Sep 17, 2023

Follow up on #36844

Context

  1. Make Sales Order:
    • in other than company currency
    • with a rounding adjustment amount
  2. Create and fulfill that foreign currency payment request
  3. Then the total amount in the payment entry's child table references will not be the rounded total but the grand total, despite the above Ref Doc having had a rounding adjustment amount

Proposed Solution

abandonded due to issue note in this comment

Rather rely on set_grand_total_and_outstanding_amount invoked by get_payment_entry which fetches
the right amounts when no party_amount is passed:

def set_grand_total_and_outstanding_amount(party_amount, dt, party_account_currency, doc):
	grand_total = outstanding_amount = 0
	if party_amount:
		grand_total = outstanding_amount = party_amount
	elif dt in ("Sales Invoice", "Purchase Invoice"):
		if party_account_currency == doc.company_currency:
			grand_total = doc.base_rounded_total or doc.base_grand_total
		else:
			grand_total = doc.rounded_total or doc.grand_total
		outstanding_amount = doc.outstanding_amount
	elif dt == "Dunning":
		grand_total = doc.grand_total
		outstanding_amount = doc.grand_total
	else:
		if party_account_currency == doc.company_currency:
			grand_total = flt(doc.get("base_rounded_total") or doc.get("base_grand_total"))
		else:
			grand_total = flt(doc.get("rounded_total") or doc.get("grand_total"))
		outstanding_amount = doc.get("outstanding_amount") or (grand_total - flt(doc.advance_paid))
	return grand_total, outstanding_amount

Similarily, rely on set_party_account and set_party_account_currency, respectively, instead of the removed portions of code.

  • Do the right thing and prefer base_rounded_total if exists according to other examples of fetching a refdoc total values throughout the code.

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Sep 17, 2023
@blaggacao blaggacao force-pushed the fix/payment-request-rounding branch from 096caee to 47feb5f Compare September 17, 2023 19:06
@blaggacao blaggacao marked this pull request as ready for review September 17, 2023 19:06
@blaggacao blaggacao marked this pull request as draft September 17, 2023 20:25
@ruthra-kumar ruthra-kumar self-assigned this Sep 18, 2023
@blaggacao

This comment was marked as resolved.

@blaggacao blaggacao force-pushed the fix/payment-request-rounding branch from 47feb5f to 6e1ad4c Compare September 18, 2023 18:29
@blaggacao blaggacao marked this pull request as ready for review September 18, 2023 18:31
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #37123 (6e1ad4c) into develop (5e21e7c) will increase coverage by 0.06%.
Report is 10 commits behind head on develop.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #37123      +/-   ##
===========================================
+ Coverage    66.56%   66.62%   +0.06%     
===========================================
  Files          793      793              
  Lines        62468    62501      +33     
===========================================
+ Hits         41582    41644      +62     
+ Misses       20886    20857      -29     
Files Changed Coverage
...ccounts/doctype/payment_request/payment_request.py 100.00%

@@ -249,7 +249,7 @@ def create_payment_entry(self, submit=True):
if (
party_account_currency == ref_doc.company_currency and party_account_currency != self.currency
):
party_amount = ref_doc.base_grand_total
party_amount = ref_doc.get("base_rounded_total") or ref_doc.get("base_grand_total")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this apply to the other branch of the condition as well?

(party_amount = self.get("rounded_total") or self.get("grand_total"))

Probably not the issue you're trying to solve, but good for consistency and in accordance with other parts of the code.

Copy link
Member

Choose a reason for hiding this comment

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

@barredterra 'Payment Request' doctype doesn't have rounded_total.

@ruthra-kumar
Copy link
Member

@blaggacao Tried some scenarios and change works fine. If you can add a test case for this, we can merge it.

@ruthra-kumar ruthra-kumar merged commit 3d99e57 into frappe:develop Oct 10, 2023
@ruthra-kumar ruthra-kumar added the backport version-14-hotfix backport to version 14 label Oct 10, 2023
ruthra-kumar added a commit that referenced this pull request Oct 10, 2023
…-37123

fix: payment request rounding in multi-currency and on status update (backport #37123)
@blaggacao blaggacao deleted the fix/payment-request-rounding branch October 10, 2023 20:37
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants