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

Add convert currency and tests #30

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Conversation

Edge7481
Copy link

@Edge7481 Edge7481 commented Apr 1, 2024

Pivotal Tracker Link

What this PR does:

This pull request implements
a static method in Currency_Conversion.rb that converts a Money object of a currency to a Money object of another currency, since as of right now there's no functionality to actually convert. It will be used to compute payments later

Include screenshots, videos, etc.

N/A

Who authored this PR?

How should this PR be tested?

  • Is there a deploy we can view?
  • What do the specs/features test?

I added 2 tests that tests the path that there is a valid currency in the table, and the path the there is no currency in the table.

  • Are there edge cases to watch out for?

Are there any complications to deploying this?

No

Checklist:

  • Has this been deployed to a staging environment or reviewed by a customer?
  • Tag someone for code review (either a coach / team member)
  • I have renamed the branch to match PivotTracker's suggested one (necessary for BlueJay)

@Edge7481 Edge7481 self-assigned this Apr 2, 2024
if conversion
Money.add_rate(from_currency, to_currency, conversion.rate)
amount.exchange_to(to_currency)
else
Copy link

Choose a reason for hiding this comment

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

Maybe consider returning -1 instead instead of returning an amount without conversion. Whenever you use this method convert_currency, you can check for the amount not being -1

Copy link

Choose a reason for hiding this comment

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

if you choose this path, remember to update the tests below

@Edge7481 Edge7481 force-pushed the 187348127-add-convert-currency branch from c3d0322 to 6923177 Compare April 5, 2024 00:11
@Edge7481
Copy link
Author

Edge7481 commented Apr 5, 2024

Updated the bad path to -1 now. Should work better for validation!

@Edge7481 Edge7481 merged commit e86d2f5 into main Apr 5, 2024
16 checks passed
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