Decimal number

[Muhmad-hamdi]

Please confirm the following:

• I've checked the current issues for duplicate issues.
• I've requested a single (only one) feature/change in this issue. It complies with the One Request Per GitHub Issue (ORPGI) rule.
• My issue is well-defined and describes how it should be implemented from UI/UX perspective.

What do you want to be added or improved?

As a user I want to make it optional to show decimal number or not.
I don't like numbers like this 2500.00

Why do you need it?

How do you imagine it?

No response

[ivywallet]

Thank you @Muhmad-hamdi for raising Issue #3120! 🚀
What's next? Read our Contribution Guidelines 📚.

Tagging @ILIYANGERMANOV for review & approval 👀

[ILIYANGERMANOV]

Add a setting whether to show the ".00" decimal numbers or not. Decimal numbers should be should by default unless explicitly disabled. @Muhmad-hamdi please add more info - for example do you want to show "1233.XX"?

[Muhmad-hamdi]

yes i want to add a setting whether to show the ".00" decimal numbers or not

…

On Thu, Apr 11, 2024 at 9:32 AM Iliyan Germanov ***@***.***> wrote: Add a setting whether to show the ".00" decimal numbers or not. Decimal numbers should be should by default unless explicitly disabled. @Muhmad-hamdi <https://github.com/Muhmad-hamdi> please add more info - for example do you want to show "1233.XX"? — Reply to this email directly, view it on GitHub <#3120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZT3DHXMIFQWEMIJCG5AN43Y4Y4CJAVCNFSM6AAAAABGBWL5D2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBZGEYDCNZWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sandeepjak2007]

It should reflect in all values right?

[Muhmad-hamdi]

Yes

…

On Sun, Apr 14, 2024, 10:47 AM Karanam Sandeep ***@***.***> wrote: It should reflect in all values right? — Reply to this email directly, view it on GitHub <#3120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZT3DHVWJPUKPATZT3IVNWTY5I7DRAVCNFSM6AAAAABGBWL5D2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJTHE3DSOJTGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ILIYANGERMANOV]

@sandeepjak2007 make sure that the current behavior isn't broken in any way and that performance isn't affected. If that's not possible we ca skip implementing this setting. Also check the IvyFeatures class which should help for implementing the setting. There should also be a Features screen

[sandeepjak2007]

Once will make changes and will check once then only try to push the changes

[shamim-emon]

I'm on it

[ivywallet]

Thank you for your interest @shamim-emon! 🎉
Issue #3120 is assigned to you. You can work on it! ✅

If you don't want to work on it now, please un-assign yourself so other contributors can take it.

Also, make sure to read our Contribution Guidelines.

[shamim-emon]

@ILIYANGERMANOV
I'm thinking, when the feature is enabled :

1. I'd make the changes in data layer.
2. I'd apply flooring/Ceiling. eg: 2500.43 become 2500, 100.50 becomes 101.
   Please let me know if that's alright or if you have any suggestion to add.

[ILIYANGERMANOV]

@ILIYANGERMANOV I'm thinking, when the feature is enabled :

1. I'd make the changes in data layer.
2. I'd apply flooring/Ceiling. eg: 2500.43 become 2500, 100.50 becomes 101.
   Please let me know if that's alright or if you have any suggestion to add.

No, no. This change must be done in :shared:ui. We need a centralized FormatMoneyUseCase that does the proper Double -> String formatting based on user's settings

[ILIYANGERMANOV]

The data layer must always return the true values. This is an UI feature that applies different formatting. Also centralizing all formatting in one usecase will be awesome. See how I did it for formatting date and time

[shamim-emon]

@ILIYANGERMANOV I'm thinking, when the feature is enabled :

1. I'd make the changes in data layer.
2. I'd apply flooring/Ceiling. eg: 2500.43 become 2500, 100.50 becomes 101.
   Please let me know if that's alright or if you have any suggestion to add.

No, no. This change must be done in :shared:ui. We need a centralized FormatMoneyUseCase that does the proper Double -> String formatting based on user's settings

Ok

[shamim-emon]

@ILIYANGERMANOV want to share my current approach with this.

1. Creating a UseCase FormatMoneyUseCase under :shared:domain (using domain package instead of ui as other useCases are under domain package).
2. FormatMoneyUseCase will have 1 method that will take double value and return Int(if decimal place value is <.50 we will floor it otherwise ceil it eg: 2500.43 become 2500, 100.50 becomes 101)
3. Write UnitTest for 2 to confirm expected behaviour
4. Add a boolean IvyFeatures which by default will be false.
5. When concerned IvyFeatures is set to true, in viewModels we will perform this conversion (Double to Int) for every double value using this useCase and otherwise will return default double value(Please note that I'm not planning to perform the Int to String conversion within the usecase, planning to do it in viewModels as we do for default value.

Let me know your, comment/suggestion and/or let me know if I got anything wrong.
Since this is going to be a wide-spread change, discussing the idea before hand.

[ILIYANGERMANOV]

@shamim-emon

1. Creating a UseCase FormatMoneyUseCase under :shared:domain (using domain package instead of ui as other useCases are under domain package).

We have use-cases both in :shared:ui and :shared:domain depending on their responsibility. FormatMoneyUseCase is purely an UI layer thing and must be placed in :shared:ui.

2. FormatMoneyUseCase will have 1 method that will take double value and return Int(if decimal place value is <.50 we will floor it otherwise ceil it eg: 2500.43 become 2500, 100.50 becomes 101)

No no, formatting is an action of transforming domain data (Double, Instant etc) into String. It must have one method:

fun format(value: Value): String

5. When concerned IvyFeatures is set to true, in viewModels we will perform this conversion (Double to Int) for every double value using this useCase and otherwise will return default double value(Please note that I'm not planning to perform the Int to String conversion within the usecase, planning to do it in viewModels as we do for default value.

Now that we have the FormatMoneyUseCase as a class in the DI graph, we can inject the IvyFeatures into it.

class FormatMoneyUseCase @Inject constructor(
  private val feature: Features
) {
//...
}

Once we have the FormatMoneyUseCase we can merge the PR only with its creation. Migration of existing screens to it will happen in follow-up PRs. Good idea for syncing the approach! 💯

[ILIYANGERMANOV]

@shamim-emon and there is no rounding or Int calls anywhere, instead use DecimalFormat.

• `DecimalFormat("###,###") equivalent to rounding

Read the decimal format docs or even better ask ChatGPT for appropriate DecimalFormat for India. The decimal format will take care of the rounding

[shamim-emon]

@shamim-emon

1. Creating a UseCase FormatMoneyUseCase under :shared:domain (using domain package instead of ui as other useCases are under domain package).

We have use-cases both in :shared:ui and :shared:domain depending on their responsibility. FormatMoneyUseCase is purely an UI layer thing and must be placed in :shared:ui.

2. FormatMoneyUseCase will have 1 method that will take double value and return Int(if decimal place value is <.50 we will floor it otherwise ceil it eg: 2500.43 become 2500, 100.50 becomes 101)

No no, formatting is an action of transforming domain data (Double, Instant etc) into String. It must have one method:

fun format(value: Value): String

5. When concerned IvyFeatures is set to true, in viewModels we will perform this conversion (Double to Int) for every double value using this useCase and otherwise will return default double value(Please note that I'm not planning to perform the Int to String conversion within the usecase, planning to do it in viewModels as we do for default value.

Now that we have the FormatMoneyUseCase as a class in the DI graph, we can inject the IvyFeatures into it.

class FormatMoneyUseCase @Inject constructor(
  private val feature: Features
) {
//...
}

Once we have the FormatMoneyUseCase we can merge the PR only with its creation. Migration of existing screens to it will happen in follow-up PRs. Good idea for syncing the approach! 💯

@ILIYANGERMANOV
Awesome, just 2 more qeries:

1. fun format(value: Value): String the computation is insignificant, but would still prefer to make it suspending function & perform the computation in background thread. Is that alright?
2. I'm gonna create the useCase in marked package(just wanna double confirm)
   

[ILIYANGERMANOV]

@shamim-emon

1. fun format(value: Value): String the computation is insignificant, but would still prefer to make it suspending function & perform the computation in background thread. Is that alright?

Yes, sounds good. Make it suspend operation.

2. I'm gonna create the useCase in marked package(just wanna double confirm)

Looks good, too!

[shamim-emon]

@ILIYANGERMANOV I'm currently working on the task.
Facing issue with Tests. Seems like we can't use mockk in tests. Please have a look at the below sc.
Please note that I'm facing same error in any existing unit tests containing any mockked item.

I haven't found any solution yet on this.
Is there any specific configuration to run Unit tests locally containing mockked items that I'm missing? or is it a bug?

[ILIYANGERMANOV]

@ILIYANGERMANOV I'm currently working on the task. Facing issue with Tests. Seems like we can't use mockk in tests. Please have a look at the below sc. Please note that I'm facing same error in any existing unit tests containing any mockked item.

I haven't found any solution yet on this. Is there any specific configuration to run Unit tests locally containing mockked items that I'm missing? or is it a bug?

@shamim-emon raise a PR, you've probably setup something incorrectly. Mockk should work. Also, try running the entire test file from Android Studio instead of an individual test

[shamim-emon]

@ILIYANGERMANOV I have tried running entire test file and same issue.
here is the draft PR #3568