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

feat: currency exchange settings #27318

Merged
merged 27 commits into from
Jan 11, 2022

Conversation

rtdany10
Copy link
Contributor

@rtdany10 rtdany10 commented Sep 2, 2021

Currency Exchange Settings will enable users to use exchange rate provider of their choice.
CES Change API

Closes #27317
#no-docs (temp)

@coveralls
Copy link

coveralls commented Sep 2, 2021

Coverage Status

Coverage decreased (-0.01%) to 51.499% when pulling f460229 on rtdany10:currency-exchange-settings into ceaa804 on frappe:develop.

@rtdany10 rtdany10 marked this pull request as ready for review September 4, 2021 13:58
Copy link
Member

@ankush ankush left a comment

Choose a reason for hiding this comment

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

@marination @deepeshgarg007 @nextchamp-saqib @barredterra

Your comments? From UX perspective most users won't understand what endpoint/parameters etc even means. It might be a better idea to have a list of providers from which users can choose the one they want.

@marination
Copy link
Collaborator

@ankush I agree with your solution but have a different reasoning.

@rtdany10
My concern is error handling. Most APIs have their quirks and will need special error handling. Making something as generic (the API field) for currency exchange rates doesn't sound like the best idea stability wise. Setup may go through, but what about after that ? Things like unsupported currencies and the service being down need to be handled separately.

I would also suggest giving a list of services, that we support, to choose from so that we know that these are well handled and compatible with ERPNext. We can incrementally build on these services as and when there is demand for them.

@rtdany10
Copy link
Contributor Author

rtdany10 commented Sep 14, 2021

Things like unsupported currencies and the service being down need to be handled separately.

I believe that the function to get exchange rate from the provider does handle this. It returns 0.00 and logs an error.

But again, as pointed out, having some select-able pre-configured options will make it easier for the end user, like in the case of Email Accounts where certain service providers can be selected without configuring Email Domain.

I can add some service providers; would be nice if someone shares a preferred list of service providers. @barredterra and other end users.

@ankush @marination

@stale stale bot added inactive and removed inactive labels Sep 29, 2021
@rtdany10 rtdany10 marked this pull request as draft October 5, 2021 05:40
@stale stale bot added the inactive label Oct 20, 2021
@frappe frappe deleted a comment from stale bot Oct 20, 2021
@stale stale bot removed the inactive label Oct 20, 2021
@frappe frappe deleted a comment from stale bot Oct 20, 2021
@barredterra
Copy link
Collaborator

@rtdany10 let's go with exchangerate.host and frankfurter.app for now. The official API of the European Central Bank would also be interesting: https://sdw-wsrest.ecb.europa.eu/help/

@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #27318 (93d784d) into develop (9d36e30) will increase coverage by 9.07%.
The diff coverage is 59.52%.

@@             Coverage Diff             @@
##           develop   #27318      +/-   ##
===========================================
+ Coverage    48.84%   57.92%   +9.07%     
===========================================
  Files         1107     1110       +3     
  Lines        67910    67991      +81     
===========================================
+ Hits         33174    39386    +6212     
+ Misses       34736    28605    -6131     
Impacted Files Coverage Δ
erpnext/setup/install.py 0.00% <0.00%> (ø)
...cy_exchange_settings/currency_exchange_settings.py 62.26% <62.26%> (ø)
...ings_details/currency_exchange_settings_details.py 100.00% <100.00%> (ø)
...ttings_result/currency_exchange_settings_result.py 100.00% <100.00%> (ø)
erpnext/setup/utils.py 89.69% <100.00%> (+2.05%) ⬆️
...eorder_level/itemwise_recommended_reorder_level.py 92.45% <0.00%> (-1.89%) ⬇️
...ctype/accounting_dimension/accounting_dimension.py 64.34% <0.00%> (-1.56%) ⬇️
...e/period_closing_voucher/period_closing_voucher.py 88.23% <0.00%> (-1.48%) ⬇️
...ype/account/chart_of_accounts/chart_of_accounts.py 77.39% <0.00%> (-0.69%) ⬇️
erpnext/stock/report/stock_ageing/stock_ageing.py 93.06% <0.00%> (ø)
... and 211 more

@rtdany10
Copy link
Contributor Author

rtdany10 commented Nov 3, 2021

CES-1.mp4

I hope this is fine. Will make the fields read only.

@rtdany10 rtdany10 marked this pull request as ready for review November 4, 2021 05:49
@rtdany10 rtdany10 requested a review from ankush November 4, 2021 05:52
@stale
Copy link

stale bot commented Jan 2, 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 Jan 2, 2022
@stale stale bot closed this Jan 5, 2022
@barredterra barredterra reopened this Jan 5, 2022
@rtdany10 rtdany10 requested a review from marination as a code owner January 10, 2022 12:02
@deepeshgarg007 deepeshgarg007 self-assigned this Jan 10, 2022
@deepeshgarg007 deepeshgarg007 force-pushed the currency-exchange-settings branch from c357459 to 93d784d Compare January 10, 2022 15:59
@deepeshgarg007
Copy link
Member

@Mergifyio backport version-13-hotfix

@deepeshgarg007 deepeshgarg007 merged commit 962dd5e into frappe:develop Jan 11, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 11, 2022

backport version-13-hotfix

❌ No backport have been created

  • Backport to branch version-13-hotfix failed
    Pull request with merge commit are not supported

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable currency exchange API
6 participants