Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[$250] Distance rate-Decimals are not supported for this currency yet entering dot is allowed #49336

Open IuliiaHerets opened 3 days ago

IuliiaHerets commented 3 days ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.36-1 Reproducible in staging?: Y Reproducible in production?: Y Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to profile icon --workspaces --workspace
  3. Change workspace currency to ALL
  4. Tap more features -- distance rate
  5. Tap on rate
  6. Enter decimal number with dot eg : 6.4
  7. Tap save
  8. Tap rate again

Expected Result:

Decimals are bit supported for this currency so user must not be allowed to enter dot.

Actual Result:

Decimals are not supported for this currency yet entering dot is displayed in distance rate page.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/77633941-6a95-4b64-8e52-d5d1b4db0737

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836536263898090209
  • Upwork Job ID: 1836536263898090209
  • Last Price Increase: 2024-09-18
Issue OwnerCurrent Issue Owner: @DylanDylann
melvin-bot[bot] commented 3 days ago

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 3 days ago

@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

BhuvaneshPatil commented 3 days ago

Edited by proposal-police: This proposal was edited at 2024-09-17 11:35:39 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Distance rate-Decimals are not supported for this currency yet entering dot is allowed

What is the root cause of that problem?

We are adding extra decimals prop here https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx#L93

This causes allowing one decimal to any currency.

What changes do you think we should make in order to solve the problem?

We should remove that prop or set to different value based on the decimals supported for the currency

Note : We need to check with original contributor as to why this was set to 1, based on that we can change the value.

Same can be done in this component PolicyDistanceRateEditPage We can always find and update pages where we are using distance rate add/update functionality

What alternative solutions did you explore? (Optional)

Krishna2323 commented 3 days ago

Edited by proposal-police: This proposal was edited at 2024-09-17 11:38:52 UTC.

Proposal


Please re-state the problem that we are trying to solve in this issue.

Distance rate-Decimals are not supported for this currency yet entering dot is allowed

What is the root cause of that problem?

We aren't checking if the currency supports decimals or not before passing extraDecimals={1}. https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx#L93

What changes do you think we should make in order to solve the problem?


What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 day ago

Job added to Upwork: https://www.upwork.com/jobs/~021836536263898090209

melvin-bot[bot] commented 1 day ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @DylanDylann (External)

alexpensify commented 1 day ago

@DylanDylann when you get a chance, can you review the proposals to address this Albanian currency issue? Also, I'm not sure if there are other currencies that are affected by this issue like Japanese Yen. Thanks!

daledah commented 11 hours ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Decimals are not supported for this currency yet entering dot is allowed

What is the root cause of that problem?

We always display one extra decimal

https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/pages/workspace/distanceRates/CreateDistanceRatePage.tsx#L93

If currency is ALL the default decimal is 0 and the extra decimal is 1 --> we will display one decimal

If currency is USD the default decimal is 2 and the extra decimal is 1 --> we will display three decimal

What changes do you think we should make in order to solve the problem?

I don't see any reason why we need to add extra decimal. Let's remove this prop

Also see the same behavior on the submit expense page, we only use default decimal and don't add any extra decimal

daledah commented 11 hours ago

@DylanDylann Please confirm the expect before going to proposals

How many decimals should be displayed?

DylanDylann commented 7 hours ago

@daledah Interesting point. I will review this issue today

DylanDylann commented 7 hours ago

I lean toward removing the extra decimal in the distance rate page as mentioned by @daledah

Anyway, I think we still need to confirm the expected first because the extra decimal is added a long time ago for the rate feature

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 7 hours ago

Triggered auto assignment to @lakchote, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

DylanDylann commented 7 hours ago

@Expensify/design @lakchote The number of decimals on the submit expense page and distance rate page is inconsistent. Is there any reason why we need to add one more extra decimal on the distance rate page? If not, should we remove extraDecimal to keep it consistent?

  1. In the Submit Expense Page, we use the default number of decimal places for each currency (USD: 2, ALL: 0)

https://github.com/user-attachments/assets/083d78d1-35c3-43f8-b74b-520de39d17f9

  1. In the Add distance rate, we have one more decimal compared to the default (USD: 3, ALL: 1)
Screenshot 2024-09-20 at 14 46 01
shawnborton commented 7 hours ago

I think that is on purpose, since a lot of government distance rates use three decimal places. cc @trjExpensify to fact check me there.

DylanDylann commented 6 hours ago

@shawnborton For the VND, ALL currency (there are no decimal in the submit expense page), Should we allow one decimal in the distance rate page?

BhuvaneshPatil commented 6 hours ago

@DylanDylann I have mentioned about removing that prop in my proposal. Any reason for selecting the other proposal

DylanDylann commented 6 hours ago

I lean toward removing the extra decimal in the distance rate page as mentioned by @daledah

@daledah gave a good explanation of why we should remove that prop that makes me lean toward this approach

@BhuvaneshPatil Let's discuss the expectation first, I will come back to proposals later. Thanks

shawnborton commented 5 hours ago

What do you mean by the VND?

DylanDylann commented 4 hours ago

@shawnborton Ahhh VND is a currency, ALL also be a currency

For the VND currency and ALL currency (there are no decimal in the submit expense page), Should we allow one decimal in the distance rate page?

https://github.com/user-attachments/assets/30c82278-e269-4dc6-93d8-94d86a2449c9

shawnborton commented 2 hours ago

Hmm honestly I have no idea. I kind of think we can just always allow three decimal places no matter what the currency is to keep things simple? But would love someone more familiar with distance rates to fact check me, cc @twisterdotcom

Krishna2323 commented 2 hours ago

Proposal Updated

twisterdotcom commented 55 minutes ago

I think we should just allow three everywhere. That's what we do on oldDot.