Skip to content

DTSERWFOUR-461-Update decimal places for FX #253

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Mar 29, 2021
Merged

DTSERWFOUR-461-Update decimal places for FX #253

merged 9 commits into from
Mar 29, 2021

Conversation

@shanavas123
Copy link
Contributor

@shanavas123 shanavas123 commented Mar 17, 2021

No description provided.

@shanavas123 shanavas123 marked this pull request as ready for review March 17, 2021 13:23
@shanavas123 shanavas123 changed the base branch from development to S206 March 18, 2021 10:03
rbanikPP
rbanikPP approved these changes Mar 18, 2021
View reviewed changes
Copy link

@rbanikPP rbanikPP left a comment

Choose a reason for hiding this comment

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

Review comments are acted upon

vwagh-hw
vwagh-hw reviewed Mar 23, 2021
View reviewed changes
commonui/src/main/java/com/hyperwallet/android/ui/common/util/CurrencyParser.java Outdated
return null;
}

public static String getRateWithFourDecimal(String rate) {
Copy link
Collaborator

@vwagh-hw vwagh-hw Mar 23, 2021

Choose a reason for hiding this comment

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

  • number of decimal places should be a parameter, why hard code?
  • Method name should be generic enough, we could use this method to format any amount for any number of decimal places then instead of just Rate

Copy link
Contributor Author

@shanavas123 shanavas123 Mar 23, 2021

Choose a reason for hiding this comment

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

updated to generic method

commonui/src/main/java/com/hyperwallet/android/ui/common/util/CurrencyParser.java Outdated
public static String getRateWithFourDecimal(String rate) {
if (rate != null) {
StringBuilder builder = new StringBuilder();
String[] amount = rate.split("\\.");
Copy link
Collaborator

@vwagh-hw vwagh-hw Mar 23, 2021

Choose a reason for hiding this comment

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

why use string manipulation? Parsing to double isn't working?

Copy link
Contributor Author

@shanavas123 shanavas123 Mar 23, 2021

Choose a reason for hiding this comment

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

NumberFormat/Decimal formate rounding the value 1.0000 as 1 , i.e used string manipulation iOS also implemented the same.

Copy link
Collaborator

@vwagh-hw vwagh-hw Mar 25, 2021

Choose a reason for hiding this comment

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

please re-read my comment. Why can't we parse value to double and then truncate decimals ??
Screen Shot 2021-03-24 at 7 00 33 PM

Copy link
Contributor Author

@shanavas123 shanavas123 Mar 25, 2021

Choose a reason for hiding this comment

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

once we parse the value to double and truncate , its formatted and rounding the value eg: 1.000056 its return as 1. not as 1.0000

Copy link
Contributor Author

@shanavas123 shanavas123 Mar 26, 2021

Choose a reason for hiding this comment

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

Brandon confirm implement the round up for FX as a new ticket for IOS and android

vwagh-hw
vwagh-hw reviewed Mar 25, 2021
View reviewed changes
commonui/src/main/java/com/hyperwallet/android/ui/common/util/CurrencyParser.java
decimalLength = value.substring(value.indexOf(".")).length();
}
if (decimalLength > noOfDecimals) {
returnValue = value.substring(0, value.indexOf(".") + noOfDecimals + 1);
Copy link
Collaborator

@vwagh-hw vwagh-hw Mar 25, 2021

Choose a reason for hiding this comment

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

wrong implementation, when we are reducing number of decimal places, we can't just truncate the decimals, we either round up or round down values.
Please check in the grooming recording or ask Brandon, as far as I remember we round up

Copy link
Contributor Author

@shanavas123 shanavas123 Mar 25, 2021

Choose a reason for hiding this comment

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

Brandon, conformed only truncating the last two decimals not round up the value.

commonui/src/main/java/com/hyperwallet/android/ui/common/util/CurrencyParser.java Outdated
public static String getRateWithFourDecimal(String rate) {
if (rate != null) {
StringBuilder builder = new StringBuilder();
String[] amount = rate.split("\\.");
Copy link
Collaborator

@vwagh-hw vwagh-hw Mar 25, 2021

Choose a reason for hiding this comment

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

please re-read my comment. Why can't we parse value to double and then truncate decimals ??
Screen Shot 2021-03-24 at 7 00 33 PM

shahardra
shahardra approved these changes Mar 27, 2021
View reviewed changes
commonui/src/main/java/com/hyperwallet/android/ui/common/util/CurrencyParser.java Outdated
public static String getRateWithFourDecimal(String rate) {
StringBuilder builder = new StringBuilder();
String[] amount = rate.split("\\.");
if(amount.length == 2) {
Copy link

@shahardra shahardra Mar 24, 2021

Choose a reason for hiding this comment

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

95209a9#r599172273

vwagh-hw
vwagh-hw approved these changes Mar 29, 2021
View reviewed changes
Copy link
Collaborator

@vwagh-hw vwagh-hw left a comment

Choose a reason for hiding this comment

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

Approving for now, but as discussed the implementation using strings should change and be based on double with rounded values in next ticket

@vwagh-hw vwagh-hw merged commit c6be2be into S206 Mar 29, 2021
vwagh-hw pushed a commit that referenced this pull request Mar 30, 2021
@shanavas123 @SaiNarendran
S206 (#256)
c09a183 
* DTSERWFOUR-463-Allow Transaction Details text to be selected (#254)

* DTSERWFOUR-463-Allow Transaction Details text to be selected

* Address review comments

* Address review comments

* UIcase for Allow Transaction details text to be selected

Co-authored-by: Naren <najayaraman@paypal.com>

* DTSERWFOUR-461-Update decimal places for FX (#253)

* DTSERWFOUR-461-Update decimal places for FX

* address review comments

* address review comments

* update unit test.

* Address review comments

* Address review comments

* Address review comments

* Fix info overlap on smaller screens.

* Fix info overlap on smaller screens.

Co-authored-by: Naren <najayaraman@paypal.com>
@shahardra
Copy link

shahardra commented Apr 15, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vwagh-hw vwagh-hw vwagh-hw approved these changes

@SaiNarendran SaiNarendran Awaiting requested review from SaiNarendran

+3 more reviewers

@rbanikPP rbanikPP rbanikPP approved these changes

@shahardra shahardra shahardra approved these changes

@aparthibanpaypal aparthibanpaypal aparthibanpaypal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Ready To Merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@shanavas123 @shahardra @vwagh-hw @aparthibanpaypal @rbanikPP @SaiNarendran