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

Currency Conversion #26

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Currency Conversion #26

wants to merge 12 commits into from

Conversation

asfktz
Copy link
Contributor

@asfktz asfktz commented Feb 19, 2018

Related to #22

Credentials Setup

  • Additional dialog added for setting up an app ID for openexchangerates.org

Helpers added:

Rates helper

Responsible for fetching historical rates from openexchangerates.org.
It will cache each request to reduce the risk of reaching the free limit of 1000 requests per month

Currency helper

  • handle Formatting (100, USD) -> $100
  • Converts One currency to another by historical rate (as I write this, I think it might be better fit in the rates helper)

Notes

  • The original amount is added as a memo for each converted transaction in the CSV
  • Currently, a transaction will be converted if its originalCurrency is not ILS

External modules added

  • lodash
  • currency-formatter
  • money
  • oxr

@eshaham
Copy link
Owner

eshaham commented Feb 20, 2018

@asfktz before I begin going over this PR, I have a few important asks and comments:

  1. could you add a proper description?
  2. could you try to cleanup unrelated code refactoring? it makes it very hard to follow with all the changes, and some of them seem unrelated to the changes needed for this PR.
  3. I believe we can't move forward until we implement Add a charged currency property to transactions israeli-bank-scrapers#80. Your assumption that a transaction in a foreign currency should always be converted isn't valid. For most people, myself included, such transactions get converted automatically by the credit card company, and chargedAmount already holds the actual amount in ILS - so no conversion is needed. We will need to convert only if the new introduced chargedCurrency holds a foreign currency.

@asfktz
Copy link
Contributor Author

asfktz commented Feb 20, 2018

  1. could you add a proper description?

Done!

  1. Could you try to clean up unrelated code refactoring? it makes it very hard to follow with all the changes, and some of them seem unrelated to the changes needed for this PR.

Got rid of the non-relevant stuff.
Please excuse me while I'm learning the art of making small & focused PRs (:

  1. I believe we can't move forward until we implement Add a charged currency property to transactions israeli-bank-scrapers#80. Your assumption that a transaction in a foreign currency should always be converted isn't valid. For most people, myself included, such transactions get converted automatically by the credit card company, and chargedAmount already holds the actual amount in ILS - so no conversion is needed. We will need to convert only if the new introduced chargedCurrency holds a foreign currency.

Hmm.. yeah you're right, haven't taken that into account. sounds right 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants