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 refactor 2 #535

Merged
merged 2 commits into from Jun 7, 2019
Merged

[πŸ”’] Currency refactor 2 #535

merged 2 commits into from Jun 7, 2019

Conversation

eoji
Copy link
Contributor

@eoji eoji commented Jun 6, 2019

What ❓

My fourth and hopefully final refactor of KSCurrency.

  • Added KSCurrency.currencyNeedsCode to determine if a country's currency requires the country code and KSCurrency.getCurrencySymbol to return currency symbols for a country. These are helper methods to do the spanned currencies. Currently only shown in the PledgeFragment.
  • By default, we round DOWN, so I created KSCurrency.format that only takes in a value and project.
  • If we're rounding DOWN, we floor the number, or else we just leave it as is.
  • There shouldn't be any platform dependent code in unit tests so KSCurrency never returns a SpannableString.
  • Updated KSCurrencyTest to show how rounding works and added tests for formatWithUserPreference with new precision option.
  • Added StringUtils.trim to remove trailing white space characters with tests.
  • Added ViewUtils.styleCurrency to return stylized currency.
  • Both examples in ShippingRuleFactory returned the same amount so there was no way to actually test that the number updated.
  • Updated *RewardViewModelTests to show how rounding works.

How to QA? πŸ€”

  • Currency conversions should match the web.
  • Only currencies with a country code should be spanned in PledgeFragment.

Story πŸ“–

This is a refactor that was necessary after #517 was merged without any tests for the currency code.

eoji added 2 commits June 4, 2019 18:28
…y requires the country code and KSCurrency.getCurrencySymbol to return currency symbols for a country.

By default, we round DOWN, so I created KSCurrency.format that only takes in a value and project.
If we're rounding DOWN, we floor the number, or else we just leave it as is.
There shouldn't be any platform dependent code in unit tests so KSCurrency never returns a SpannableString.
Updated KSCurrencyTest to show how rounding works and added tests for formatWithUserPreference with new precision option.
Added StringUtils.trim to remove trailing white space characters with tests.
Added ViewUtils.styleCurrency to return stylized currency.
Both examples in ShippingRuleFactory returned the same amount so there was no way to actually test that the number updated.
Updated RewardViewModelTests to show how rounding works.
Copy link
Contributor

@Rcureton Rcureton left a comment

Choose a reason for hiding this comment

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

Looks great! Especially for the second refactor amazing work Izzy πŸŽ‰

* Returns a string with no leading or trailing whitespace.
*/
public static String trim(final @NonNull String str) {
return str.replace('\u00A0', ' ').trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ‘πŸΎ

@@ -69,10 +69,10 @@ interface PledgeFragmentViewModel {
/** Emits a boolean determining if the continue button should be hidden. */
fun continueButtonIsGone(): Observable<Boolean>

/** Set the USD conversion. */
/** Emits a string representing the total pledge amount in the user's preferred currency. */
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ‘πŸΎ

@eoji eoji merged commit 54679ba into master Jun 7, 2019
@eoji eoji deleted the currency-refactor-2 branch June 7, 2019 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants