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.03k stars 2.54k forks source link

Add tax fields for distance requests #42342

Closed narefyev91 closed 4 days ago

narefyev91 commented 2 weeks ago

Details

This PR adds Support for tax tracking when offline. (continue work from https://github.com/Expensify/App/pull/41379)

Fixed Issues

$ https://github.com/Expensify/App/issues/31673 PROPOSAL:

Tests

Expectation: Both the tax amount and tax rate fields cannot be modified - no right icon and the tax row is not interactive. Edit Expense for Distance request Go to the transaction thread and previously submitted distance expense. Expectation: Both the tax amount and tax rate fields cannot be modified - no right icon and the tax row is not interactive.

  1. Create some distance rates and taxes inside your workspace
  2. Start a Submit expense (money request) flow - manual one
  3. Switch tax - and verify that tax amount will be changed
  4. Move to Distance tab
  5. Change distance rate - and verify that tax amount is also changed

Offline tests

Expectation: Both the tax amount and tax rate fields cannot be modified - no right icon and the tax row is not interactive. Edit Expense for Distance request Go to the transaction thread and previously submitted distance expense. Expectation: Both the tax amount and tax rate fields cannot be modified - no right icon and the tax row is not interactive.

  1. Create some distance rates and taxes inside your workspace
  2. Start a Submit expense (money request) flow - manual one
  3. Switch tax - and verify that tax amount will be changed
  4. Move to Distance tab
  5. Change distance rate - and verify that tax amount is also changed

QA Steps

Expectation: Both the tax amount and tax rate fields cannot be modified - no right icon and the tax row is not interactive. Edit Expense for Distance request Go to the transaction thread and previously submitted distance expense. Expectation: Both the tax amount and tax rate fields cannot be modified - no right icon and the tax row is not interactive.

  1. Create some distance rates and taxes inside your workspace
  2. Start a Submit expense (money request) flow - manual one
  3. Switch tax - and verify that tax amount will be changed
  4. Move to Distance tab
  5. Change distance rate - and verify that tax amount is also changed

PR Author Checklist

Screenshots/Videos

Android: Native https://github.com/Expensify/App/assets/28352309/f1f98fbf-51d2-471a-919b-8975a97eb1f4
Android: mWeb Chrome https://github.com/Expensify/App/assets/28352309/cc07f485-f52b-48f9-8ed3-2eda4c540227
iOS: Native https://github.com/Expensify/App/assets/28352309/06ee3c5e-48c2-4b91-b657-ffc731e5b355
iOS: mWeb Safari https://github.com/Expensify/App/assets/28352309/30d941ab-7b5b-4fbe-8219-40940538bf66
MacOS: Chrome / Safari https://github.com/Expensify/App/assets/28352309/eba3f22f-ecce-425d-8eb7-d0f7b7bccbae
MacOS: Desktop https://github.com/Expensify/App/assets/28352309/e7b1337c-8bd4-4faa-aac9-2d24de6ae201
melvin-bot[bot] commented 2 weeks ago

@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

narefyev91 commented 2 weeks ago

@MonilBhavsar could not make Eto's PR ready to review. Probably we can move here. Now we support fields visibility based on defined conditions, also new calculation from FE applied if user goes offline and distance rate is changed. I think we can start reviewing code and check if everything is good. I will prepare test cases and update videos in parallel.

MonilBhavsar commented 2 weeks ago

Thanks @narefyev91! In QA and Tests, can we include the step to update the distance rate and ensure tax amount is updated accordingly?

ishpaul777 commented 2 weeks ago

Reviewer Checklist

Screenshots/Videos

Android: Native https://github.com/Expensify/App/assets/104348397/48a147b4-2c64-42f5-9bb8-c96c79af695d
Android: mWeb Chrome https://github.com/Expensify/App/assets/104348397/beeab463-49a0-4ffd-9ef7-9d3059057be4
iOS: Native https://github.com/Expensify/App/assets/104348397/ce1b1906-c73a-409b-924c-c655029c74c0
iOS: mWeb Safari https://github.com/Expensify/App/assets/104348397/840a8d42-1926-45c2-a93d-c03f14c664de
MacOS: Chrome / Safari https://github.com/Expensify/App/assets/104348397/cbedc118-c8d4-4a75-ac1d-42009cf22531
MacOS: Desktop https://github.com/Expensify/App/assets/104348397/7f15131d-08f0-4dcb-9022-19a4c86e7da5
ishpaul777 commented 1 week ago

On my list for today πŸ˜„

ishpaul777 commented 1 week ago

currently i am enabling policy.customUnits.attributes.taxEnabled by executing Onyx in console is there a better way to enable it currently in App ?

MonilBhavsar commented 1 week ago

You can do it from oldDot app. Go to Workspaces > Select a workspace > Expenses > Distance

ishpaul777 commented 1 week ago

Got it! Thanks @MonilBhavsar

Screenshot 2024-05-20 at 4 40 53β€―PM
MonilBhavsar commented 1 week ago

Thanks for the updates! Some more comments. Let's update this too and re test πŸš€

narefyev91 commented 1 week ago

Thanks for the updates! Some more comments. Let's update this too and re test πŸš€

done!

ishpaul777 commented 1 week ago

It works great, i just noticed one thing that feels confusing UX, If tax is 0% we still show the edit amount and we really can't change it throws error

https://github.com/Expensify/App/assets/104348397/3a8c0b8b-e570-4b94-9773-9059d1cb4770

MonilBhavsar commented 1 week ago

That's a feature πŸ˜„ As far as we're displaying error, it's fine UX, I think

MonilBhavsar commented 1 week ago

Not merging because of freeze

trjExpensify commented 1 week ago

@MonilBhavsar what about spinning up a feature branch for these remaining XeroCon commit /app issues? Then we can merge into that once review and approved, to keep other issues moving and testing the changes in aggregate. We're doing that for the Xero integration and it's working great!

MonilBhavsar commented 1 week ago

Sounds good! I'll do that. I was thinking to merge this next week after things stabilizes little.

OSBotify commented 4 days ago

:hand: 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 commented 3 days ago

πŸš€ Deployed to staging by https://github.com/MonilBhavsar in version: 1.4.77-0 πŸš€

platform result
πŸ€– android πŸ€– success βœ…
πŸ–₯ desktop πŸ–₯ success βœ…
🍎 iOS 🍎 success βœ…
πŸ•Έ web πŸ•Έ success βœ…
OSBotify commented 2 days ago

πŸš€ Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 πŸš€

platform result
πŸ€– android πŸ€– failure ❌
πŸ–₯ desktop πŸ–₯ success βœ…
🍎 iOS 🍎 success βœ…
πŸ•Έ web πŸ•Έ success βœ…
OSBotify commented 2 days ago

πŸš€ Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 πŸš€

platform result
πŸ€– android πŸ€– success βœ…
πŸ–₯ desktop πŸ–₯ success βœ…
🍎 iOS 🍎 success βœ…
πŸ•Έ web πŸ•Έ success βœ