[Enhancement] - Currency Preferences on Project Pages #354

Rcureton · 2018-10-15T14:20:13Z

What

User's that have selected and chosen currency can now view projects in their preferred currency.
If the user hasn't chosen a currency then the default US$ will be shown.
Created the UserCurrency class which displays the proper currency symbol also the project's pledge and goal amounts based on the fxRate.

See

…references

Rcureton · 2018-10-15T14:35:29Z

Currency Preferences on Project Pages

eoji

Nice work 💰 💰 💰

This PR is really big because a lot of stuff was reformatted but not changed. Let's revert those changes and then I had feedback about how we're getting the user's currency preference.

eoji · 2018-10-15T20:39:45Z

app/src/main/java/com/kickstarter/libs/UserCurrency.kt

+class UserCurrency(private val currentConfigType: CurrentConfigType) {
+
+    /**
+     * Returns a currency string appropriate to the user's locale and location relative to a project.


I am not sure if this is accurate to this class. Might be a case of copy pasta 🕵️‍♀️

app/src/main/java/com/kickstarter/libs/UserCurrency.kt

eoji · 2018-10-15T20:41:40Z

app/src/main/java/com/kickstarter/libs/UserCurrency.kt

+     * in $US as a default.
+     */
+    private fun userCurrencyOptions(value: Float, project: Project, symbol: String): KSCurrency.CurrencyOptions {
+        val fxRate = project.fx_rate()


what if this is null? Doing !! will still cause a crash

It shouldn't be null we get this value with every project that we pull in.

eoji · 2018-10-15T20:42:17Z

app/src/main/java/com/kickstarter/libs/UserCurrency.kt

+            CurrencyCode.SGD.rawValue() -> symbol = "S$ "
+            CurrencyCode.USD.rawValue() -> symbol = "$ "
+
+            else -> symbol = "$ "


Should we be returning US$ here?

eoji · 2018-10-15T20:43:28Z

app/src/main/java/com/kickstarter/models/User.java

@@ -19,6 +19,7 @@
  public abstract @Nullable Boolean artsCultureNewsletter();
  public abstract Avatar avatar();
  public abstract @Nullable Integer backedProjectsCount();
+  public abstract @Nullable String chosenCurrency();


I'm going to push back on this. Stay tuned for my followup in the viewmodel!

eoji · 2018-10-15T20:44:05Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

@@ -37,141 +43,243 @@
 public interface ProjectHolderViewModel {

  interface Inputs {
-    /** Call to configure view holder with a project and the config country. */
+    /**


Please revert all of these comments that got reformatted.

eoji · 2018-10-15T20:45:46Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

      this.ksCurrency = environment.ksCurrency();
+      this.userCurrency = environment.userCurrency();
+
+      this.client.fetchCurrentUser()


Here's the followup to my push back. We're updating the user using GraphQL, we should just query for their currencyPreference using GraphQL. The only info we want is the currency but we're pulling down the whole user object from REST. Then we won't need to update the user every time we look at a project.

eoji · 2018-10-15T20:46:12Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

@@ -350,131 +466,173 @@ public ViewModel(final @NonNull Environment environment) {
    public final Inputs inputs = this;
    public final Outputs outputs = this;

-    @Override public void configureWith(final @NonNull Pair<Project, String> projectAndCountry) {
+    @Override


Let's revert all of this reformatting.

eoji · 2018-10-15T20:47:08Z

app/src/test/java/com/kickstarter/UserCurrencyTest.kt

+        return UserCurrency(currentConfig)
+    }
+
+    fun testFormatCurrency_withUserInUS() {


I'm not sure what these tests are testing. Is it testing that the user prefers a currency or they're in the a locale? Let's rename it to what it's actually testing.

So these tests are testing where the user is and how the currency should be displayed based on the locale.

…ojectVH

eoji

Found some bugs 🐛but great progress on this. You're almost there!

eoji · 2018-10-17T14:54:43Z

app/src/main/java/com/kickstarter/libs/UserCurrency.kt

+        val config = this.currentConfigType.getConfig()
+
+        if (config.countryCode() == "US" && chosenCurrency == CurrencyCode.USD.rawValue()) {
+            symbol = "$ "


I think for US users looking at US projects, we'd just see "$1100" not "$ 1100". @nnekab what do you think?

@eoji correct -- remove the space between the currency symbol and amount/number. This is huge -- i'm really excited for this feature

eoji · 2018-10-17T14:55:55Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

@@ -37,7 +43,7 @@
 public interface ProjectHolderViewModel {

  interface Inputs {
-    /** Call to configure view holder with a project and the config country. */
+    /**Call to configure view holder with a project and the config country. */


These are still messed up 😭

eoji · 2018-10-17T15:09:13Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

+
+      this.apolloClient.userPrivacy()
+        .map(currency -> Objects.requireNonNull(currency.me()).chosenCurrency())
+        .compose(Transformers.neverError())


I think this is causing the funded amount to be blank when a user is logged out or doesn't have service.

I think adding tests for ProjectHolderViewModel will help you catch these edge cases!

eoji · 2018-10-17T15:10:16Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

@@ -414,7 +444,8 @@ public ViewModel(final @NonNull Environment environment) {
    @Override public @NonNull Observable<Boolean> projectDisclaimerTextViewIsGone() {
      return this.projectDisclaimerTextViewIsGone;
    }
-    @Override public @NonNull Observable<Integer> projectMetadataViewGroupBackgroundDrawableInt() {
+    @Override
+    public @NonNull Observable<Integer> projectMetadataViewGroupBackgroundDrawableInt() {


let's do this all on 1 line like
@Override public @NonNull Observable<Integer> projectMetadataViewGroupBackgroundDrawableInt() {

eoji

Have some more questions

eoji · 2018-10-18T17:27:14Z

app/src/main/java/com/kickstarter/libs/UserCurrency.kt

+        return if (config.countryCode() == "XX") {
+            KSCurrency.CurrencyOptions.builder()
+                    .country(project.country())
+                    .currencyCode("")


What is currencyCode used for and why do you pass "" for both the if and the else

So after some deep diving, we actually we were actually never using it because it would've shown in the projects currency. Now we actually capture it and map it to the showCurrencySymbol method where we map to the display strings we want to show the user. I just removed it. Great catch!

eoji · 2018-10-18T17:30:49Z

app/src/main/java/com/kickstarter/models/Project.java

@@ -33,9 +33,11 @@
  public abstract User creator();
  public abstract String currency(); // e.g.: USD
  public abstract String currencySymbol(); // e.g.: $
+  public abstract String current_currency(); // e.g.: User's Preferred currency USD


I think this should be currentCurrency

eoji · 2018-10-18T17:31:01Z

app/src/main/java/com/kickstarter/models/Project.java

  public abstract boolean currencyTrailingCode();
  public abstract @Nullable DateTime featuredAt();
  public abstract @Nullable List<User> friends();
+  public abstract Float fx_rate();


I think this one should be fxRate

eoji · 2018-10-18T17:36:47Z

app/src/test/kotlin/UserCurrencyTest.kt

+
+class UserCurrencyTest: TestCase() {
+
+    private fun createUserCurrency(countryCode: String): UserCurrency {


let's move private methods to the bottom of the class

eoji · 2018-10-18T17:51:20Z

app/src/test/kotlin/UserCurrencyTest.kt

+    fun testFormatCurrency_withUserInUS() {
+        val currency = createUserCurrency("US")
+        Assert.assertEquals("$100", currency.format(100.0f, ProjectFactory.project(), CurrencyCode.USD.rawValue()))
+        Assert.assertEquals("CA$100", currency.format(100.0f, ProjectFactory.caProject(), CurrencyCode.CAD.rawValue()))


I'm not understanding what these tests are testing. My understanding is if I prefer USD, if a project is in Canadian dollars, I'd see the conversion in USD.

For this project, I'd assume I would see US$100 because I prefer USD.

How does UserCurrency take in the user's currency preference?

So for these tests it tests the user's location and based upon the location and user's preferred currency it will show the proper currency symbol.

So for the test above the user is in the US and if the user's preferred currency is $US or they don't have a preference it still should show as $.

But this test is asserting that when we call currency.format(100.0f, ProjectFactory.caProject(), CurrencyCode.CAD.rawValue(), it will output "CA$100". Where does the user's location (US) come in? Shouldn't it be US$100?

The createCurrency method set's the config location then we use the currency object to format the currency. So if the user's location is US then currency.format(100.0f, ProjectFactory.caProject(), CurrencyCode.CAD.rawValue() should output CA$ 100 it's a Canadian project with my currency preference as Canadian Dollar but I'm in the US.

why does the user's location matter? 👀

Location matters because the display of the currency like $ or US$ depends on the location of the user.

ok but this test is called testFormatCurrency_withUserInUS so i'm assuming the user is always in the US. is the test name accurate?

Yes isn't it? It's testing the format of the currency with the user in the US 🧐

…rences

eoji

Thanks for pairing with me!

Here's a summary of the feedback I'd recommend:

Let's get rid of UserCurrency and add methods in KSCurrency that do what we want. We shouldn't be changing any of the exising KSCurrency methods. Something like KSCurrency.formatWithUserPreference() that takes in the symbol we prefer. Let's also be sure to update the Javadoc so it accurately reflects what the method is doing.
Let's rename currencySymbol to getSymbolForCurrency.
Let's handle the case of if the user is in an unlaunched country in our newly named method getSymbolForCurrency, then we can get rid of the if/else.
Let's add tests that handle the case of a user
-who is in the US and prefers a currency different from the project they're viewing and who prefers USD for a project not based in the US,
-who is in a launched country and 1. prefers a currency different from the project they're viewing 2. who prefers USD for a project based in the US 3. who prefers USD for a project not based in the US
-an unlaunched country and 1. prefers a currency different from the project they're viewing 2. who prefers USD for a project based in the US 3. who prefers USD for a project not based in the US

My last question:

I don't fully understand the rounding that should happen. If we convert a project's goal or pledged amount, should we be rounding up or down?

I think that's it. Lemme know if I missed anything.

…for user preference

eoji

Just tested this. I think we're almost there. I noticed you're not showing the converted amount for US projects when a user prefers a different currency. Any time there's a conversion, you need to show the amounts in the project's original currency. And the rewards need to have the "About X" conversion. Here's a US project that shows the funded amount without the conversion and the rewards don't show the conversion:

We also need to handle if the chosen currency already matches the project's currency:

The user shouldn't see the converted pledge amount and the rewards don't need to be converted.

Rcureton · 2018-10-22T19:03:17Z

The story says that the rewards should be converted to the user's preferred currency.

eoji · 2018-10-22T19:37:09Z

@nnekab Wanted to confirm if we prefer a currency that's different from the project's, we should see the reward in the original currency amount and the converted amount underneath.

Here's what it looks like now in the current version of the app. I'm viewing a project that's in Euros and I am defaulted to USD. The user sees the rewards in the project's currency with a conversion of the user's currency underneath

nnekab · 2018-10-22T19:53:01Z

@dannyalright @eoji this is how we handle on web -- i think its fine as shown in the image you shared. I get that it's little confusing because we convert the project goal and then show reward amount conversion underneath. but im fine with leaving similar to web

eoji · 2018-10-22T19:54:24Z

Will update on Trello 👍

…ct currency and user chosen currency match

Rcureton · 2018-10-23T16:19:38Z

I refactored the methods to show the conversion when the project currency and user currency differ.

eoji

Had some comments about documentation, tests, some redundancy and the blocking thing I found is fxRate can be null. After those things are wrapped up, we're G2G

app/src/main/java/com/kickstarter/libs/KSCurrency.java

eoji · 2018-10-24T17:07:26Z

app/src/main/java/com/kickstarter/libs/KSCurrency.java

+    return CurrencyOptions.builder()
+        .country(project.country())
+        .currencyCode("")
+        .currencySymbol(config.countryCode().equals("XX") ? "US$" : getSymbolForCurrency(symbol))


In the getSymbolForCurrency, looks like we're already checking if the country code is "XX"

app/src/main/java/com/kickstarter/models/Project.java

eoji · 2018-10-24T17:28:58Z

app/src/main/java/com/kickstarter/viewmodels/ProjectHolderViewModel.java

@@ -291,8 +291,8 @@ public ViewModel(final @NonNull Environment environment) {
        .filter(ObjectUtils::isNotNull)
        .map(NumberUtils::format);

-      this.usdConversionTextViewIsGone = this.projectAndCountry
-        .map(pc -> I18nUtils.isCountryUS(pc.second) && !I18nUtils.isCountryUS(pc.first.country()))
+      this.usdConversionTextViewIsGone = project


Let's rename this output since the name usdConversionTextViewIsGone is no longer accurate.

eoji · 2018-10-24T17:44:19Z

app/src/main/java/com/kickstarter/viewmodels/RewardViewModel.java

-      this.usdConversionTextViewIsGone = shouldDisplayUsdConversion
-        .map(BooleanUtils::negate)
-        .distinctUntilChanged();
+      this.usdConversionTextViewIsGone = this.projectAndReward


Let's rename this one also

eoji · 2018-10-24T17:52:39Z

app/src/test/java/com/kickstarter/KSCurrencyTest.java

 public class KSCurrencyTest extends TestCase {
  public void testFormatCurrency_withUserInUS() {
    final KSCurrency currency = createKSCurrency("US");
    assertEquals("$100", currency.format(100.0f, ProjectFactory.project()));
    assertEquals("$100 CAD", currency.format(100.0f, ProjectFactory.caProject()));
    assertEquals("£100", currency.format(100.0f, ProjectFactory.ukProject()));
+
+    assertEquals("$100", currency.formatWithUserPreference(100.0f, ProjectFactory.project(), CurrencyCode.USD.rawValue()));


I noticed all of the tests use formatWithUserPreference which rounds up. For project goals and pledged amounts, we should test rounding down.

eoji

👍Just 2 things left:

We should get rid of formatWithUserPreference(final float initialValue, final @NonNull Project project, final String symbol) since we only use it to test and not in the view models. We should use formatWithUserPreference(final float initialValue, final @NonNull Project project, final @NonNull RoundingMode roundingMode, final String symbol) in our tests.
That method is also missing documentation for roundingMode.

eoji · 2018-10-24T19:16:20Z

app/src/main/java/com/kickstarter/libs/KSCurrency.java

+
+    return CurrencyOptions.builder()
+        .country(project.country())
+        .currencyCode("")


Do we need this?

We need this because it's defined in the CurrencyOptions Model class. If we remove it, it won't build properly but also there's many other classes that use this KSCurrency class removing it altogether would affect the other classes.

I mean do we need it in this method?

Yes because we will get this error if it's removed:
Caused by: java.lang.IllegalStateException: Missing required properties: currencyCode

I think later we can actually remove it once we scope out how currency should be handled in those other classes.

eoji · 2018-10-24T19:25:01Z

app/src/main/java/com/kickstarter/libs/KSCurrency.java

+   * @param project The project to use to look up currency information.
+   * @param symbol The currency symbol that should be shown next to the pledge and goal amounts.
+   */
+  public String formatWithUserPreference(final float initialValue, final @NonNull Project project, final String symbol) {


The only place this method is used is in tests, so we're not actually testing what we're showing the user. I don't think we need it since we are only using
formatWithUserPreference(final float initialValue, final @NonNull Project project, final @NonNull RoundingMode roundingMode, final String symbol)

eoji

🚀

Rcureton added 16 commits October 8, 2018 14:33

updated alert message

c9dcaac

added style for alert dialog

d213013

added background to dialog

6c475fc

added error handling

42e2234

fixed styles

e3116b6

fixed project currency need to fix rewards currency

4fdeedf

added userCurrency class

98ba29f

fixed converted text method

8d47074

merged in changes from master

f16fd23

removed usused variable

c45c4e1

fixed format of KScurrency

04ce219

wrote tests for UserCurrency which sets the proper project currency p…

ef9a31e

…references

fixed checkstyle errors

b9c297c

fixed line spacing

3a0d92c

fixed commented out test and method documentation

f4f761c

fixed method description

68d59f9

Rcureton changed the title ~~[Enhancement] -~~ [Enhancement] - Currency Preferences on Project Pages Oct 15, 2018

Rcureton requested a review from eoji October 15, 2018 14:34

Rcureton added the feature ✨ label Oct 15, 2018

eoji reviewed Oct 15, 2018

View reviewed changes

Rcureton added 6 commits October 16, 2018 12:26

removed chosen currency from user model. implemented graph call in pr…

55de424

…ojectVH

added logic to determine location and symbol to show and fixed test

498f515

fixed method description

123cf05

Merge branch 'master' into rc/project-currency

7a6052e

added source directory for kotlin unit test

1ac7e31

removed repeat from graph call

23a1c64

eoji reviewed Oct 17, 2018

View reviewed changes

Rcureton added 2 commits October 17, 2018 16:12

added current currency field to project, removed graph call

08aee85

fixed spacing with method descriptions

9302f6f

Rcureton added 2 commits October 17, 2018 16:19

added description to current currency object

d4ba8c2

fixed formatting

baba0c6

eoji reviewed Oct 18, 2018

View reviewed changes

removed useless currency code reference and fixed project object refe…

3f64344

…rences

eoji reviewed Oct 22, 2018

View reviewed changes

Rcureton added 2 commits October 22, 2018 13:17

removed user currency, added methods to KSCurrency, also added tests …

e79688e

…for user preference

fixed code formatting

781b1d5

eoji reviewed Oct 22, 2018

View reviewed changes

Rcureton added 2 commits October 23, 2018 12:16

refactored methods to show conversion text view based on if the proje…

5a575c2

…ct currency and user chosen currency match

fixed checkstyle error

46afcaa

Rcureton added 5 commits October 23, 2018 15:38

refactored conversion test methods still fixing one

21b8b7b

still fixing one broken test

e87f7f6

fixed tests and rounding

8290138

removed line from test

c7828a2

renamed tests

7d6a16c

eoji reviewed Oct 24, 2018

View reviewed changes

Rcureton added 3 commits October 24, 2018 14:52

added documentation, fixed method in rewardVM, refactored variable names

4e888e2

renamed variables

6cb2159

fixed checkstyle error

d0f14f7

eoji reviewed Oct 24, 2018

View reviewed changes

added documentation for roundedMode and removed unused method

4af027a

eoji approved these changes Oct 24, 2018

View reviewed changes

Rcureton merged commit 673cf10 into master Oct 24, 2018

Rcureton deleted the rc/project-currency branch October 24, 2018 21:08


		class UserCurrencyTest: TestCase() {

		private fun createUserCurrency(countryCode: String): UserCurrency {

[Enhancement] - Currency Preferences on Project Pages #354

[Enhancement] - Currency Preferences on Project Pages #354

Conversation

Rcureton commented Oct 15, 2018 • edited

What

See

Rcureton commented Oct 15, 2018

eoji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rcureton Oct 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoji left a comment

Choose a reason for hiding this comment

eoji left a comment

Choose a reason for hiding this comment

Rcureton commented Oct 22, 2018

eoji commented Oct 22, 2018

nnekab commented Oct 22, 2018

eoji commented Oct 22, 2018

Rcureton commented Oct 23, 2018 • edited by eoji

eoji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eoji left a comment

Choose a reason for hiding this comment

Rcureton commented Oct 15, 2018 •

edited

Rcureton Oct 18, 2018 •

edited

Rcureton commented Oct 23, 2018 •

edited by eoji