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

refactor: don't book exchange gain/loss for sales/purchase orders #35061

Conversation

ruthra-kumar
Copy link
Member

@ruthra-kumar ruthra-kumar commented Apr 26, 2023

Don't book Exchange Gain/Loss while referencing Sales/Purchase Order

Sales/Purchase Orders don't have any accounting impact. So, Booking Gain/Loss for a Multi-Currency Order doesn't make sense.

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Apr 26, 2023
@ruthra-kumar ruthra-kumar force-pushed the refactor_pe_dont_book_gain_loss_for_sales_purchase_orders branch from a56de8e to f00bbe4 Compare April 26, 2023 05:29
@ruthra-kumar ruthra-kumar changed the title refactor: don't book exch gain/loss for sales/purchase orders refactor: don't book exchange gain/loss for sales/purchase orders Apr 26, 2023
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #35061 (ce4e18c) into develop (04902e1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #35061   +/-   ##
========================================
  Coverage    63.69%   63.70%           
========================================
  Files          813      813           
  Lines        60007    60030   +23     
========================================
+ Hits         38221    38240   +19     
- Misses       21786    21790    +4     
Impacted Files Coverage Δ
...xt/accounts/doctype/payment_entry/payment_entry.py 74.85% <100.00%> (+0.38%) ⬆️

... and 15 files with indirect coverage changes

@ruthra-kumar ruthra-kumar force-pushed the refactor_pe_dont_book_gain_loss_for_sales_purchase_orders branch from f00bbe4 to 5bd3d62 Compare April 26, 2023 08:29
Copy link
Member

@deepeshgarg007 deepeshgarg007 left a comment

Choose a reason for hiding this comment

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

See similar logic being used for the exchange rate in both places, maybe move this to a function and use that?

Also if we can add a simple unit test for this

@ruthra-kumar ruthra-kumar force-pushed the refactor_pe_dont_book_gain_loss_for_sales_purchase_orders branch from 5bd3d62 to ce4e18c Compare May 1, 2023 08:40
@ruthra-kumar ruthra-kumar marked this pull request as ready for review May 1, 2023 08:41
@ruthra-kumar ruthra-kumar merged commit 8f2302a into frappe:develop May 2, 2023
1 check passed
@ruthra-kumar ruthra-kumar added the backport version-14-hotfix backport to version 14 label May 2, 2023
ruthra-kumar added a commit that referenced this pull request May 2, 2023
…-35061

refactor: don't book exchange gain/loss for sales/purchase orders (backport #35061)
@ruthra-kumar ruthra-kumar deleted the refactor_pe_dont_book_gain_loss_for_sales_purchase_orders branch June 14, 2023 16:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 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.

None yet

2 participants