Skip to content

Fix tracking rate#7639

Merged
deetergp merged 2 commits into
Expensify:mainfrom
marktoman:fix-tracking-rate
Feb 11, 2022
Merged

Fix tracking rate#7639
deetergp merged 2 commits into
Expensify:mainfrom
marktoman:fix-tracking-rate

Conversation

@marktoman

@marktoman marktoman commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

Details

Fix Rate formatting

Fixed Issues

$ #6213

Tests and QA Steps

Action 1

Steps
  1. In the Rate field, type any number that has less than 2 decimal places
  2. Wait more than 3 seconds
Result

The number is formatted to 2 decimal places

Action 2

Steps
  1. In the Rate field, type any number that has more than or equal to 2 decimal places
  2. Wait more than 3 seconds
Result

The number is preserved

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

Screenshot_1644428887

Desktop

iOS

Android

@marktoman marktoman requested a review from a team as a code owner February 9, 2022 00:37
@MelvinBot MelvinBot requested review from deetergp and removed request for a team February 9, 2022 00:37
@deetergp

deetergp commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

@marktoman Just a reminder that I can't review this till you've added PR details and tested on all platforms.

@marktoman

Copy link
Copy Markdown
Contributor Author

@deetergp Updated

@deetergp

Copy link
Copy Markdown
Contributor

Thanks @marktoman.

Now I realize what's going on. You are saving the rate as 0.585 which makes perfect sense. But in order to normalize across several different kinds of currencies, we save our amounts in cents only, so we need to be saving the amount of fifty eight and a half cents as 58.5 but displaying it as 0.585 (zero dollars and fifty eight cents).

To illustrate, this:
Screen Shot 2022-02-09 at 5 01 04 PM

Functionally ends up making expenses that pay out $0.0059 per km
Screen Shot 2022-02-09 at 5 02 16 PM

Which would very likely end up upsetting anyone that tries to get reimbursed for their mileage. Does that make cents sense?

@deetergp deetergp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to be storing these as as cents rather than dollars and cents.

@deetergp deetergp assigned deetergp and marktoman and unassigned deetergp Feb 10, 2022

@deetergp deetergp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the latest changes, I am not able to type anything with a leading decimal into the form. We specifically need to be able to add 0.585 since that is the 2022 default rate for the IRS.

@marktoman

marktoman commented Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

@deetergp Do you experience different behavior than what the video shows? Otherwise, can you elaborate?

Rate.mp4

@deetergp

Copy link
Copy Markdown
Contributor

@marktoman What I am experiencing is the UI simply won't let me type 0.anything
2022-02-10_15-39-37

@marktoman

Copy link
Copy Markdown
Contributor Author

@deetergp Strange, after running

git clone https://github.com/marktoman/fix-tracking-rate fix-tracking-rate
cd fix-tracking-rate
npm install && npm run web

, I tested both desktop and mobile and it behaves like my video.

Can you see errors on the console?

@marktoman

Copy link
Copy Markdown
Contributor Author

With the latest changes, I am not able to type anything with a leading decimal into the form. We specifically need to be able to add 0.585 since that is the 2022 default rate for the IRS.

@deetergp Is this unique to the latest commit? I have tested the first commit from scratch — no difference on my end.

@deetergp

Copy link
Copy Markdown
Contributor

@marktoman I'm not sure what went wrong — probably PEBCAK — but I dumped my copy of your branch, checked it out again, re-ran npm -i and now it works as intended, even after logging out and back in again. I also verified that it looks right in OldDot! (cc @trjExpensify)

Screen Shot 2022-02-11 at 11 42 00 AM

@deetergp deetergp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good and tests well

@deetergp deetergp merged commit e75be3f into Expensify:main Feb 11, 2022
@OSBotify

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @deetergp in version: 1.1.39-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @timszot in version: 1.1.39-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants