Handle tax for split requests by MonilBhavsar · Pull Request #40240 · Expensify/App

MonilBhavsar · 2024-04-15T14:14:30Z

Details

Pass taxCode and taxAmount to Split API commands.
Tracks tax both online and offline optimistically for split expense with policy expense chat

Fixed Issues

$ #39690
PROPOSAL:

Tests

Same as QA steps

Verify that no errors appear in the JS console

Offline tests

Same as QA steps

QA Steps

Prerequisite: Tax is enabled on workspace

Go to FAB > Split > Select a workspace chat
Ensure tax rate and tax amount are selected
Update currency, expense amount and tax rate and ensure tax rate and tax amount are updated accordingly. Note the taxAmount at this point
Split the expense
Go to money request with workspace and ensure tax rate is selected as it is and tax amount is divided by half

Now we repeat by splitting with individuals

Go to FAB > Split > Select a workspace chat
Ensure tax rate and tax amount are not displayed
Go to individual money requests and ensure tax rate and tax amount are no displayed
Repeat for offline

Testing split scans

Go to FAB and select Split Expense and select Scan
Add a receipt
While in scanning state, update tax rate and ensure tax rate and tax amount are updated in draft transaction accordingly and no API call is being made
While in scanning state, update tax amount and ensure tax amount are updated in draft transaction accordingly and no API call is being made
While in scanning state, update expense amount and currency and ensure tax rate and tax amount are updated in draft transaction accordingly and no API call is being made

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

See MacOS: Chrome / Safari

Android: mWeb Chrome

See MacOS: Chrome / Safari

iOS: Native

See MacOS: Chrome / Safari

iOS: mWeb Safari

See MacOS: Chrome / Safari

MacOS: Chrome / Safari

https://drive.google.com/file/d/1C6T9vQ1VwujJJKlCbNNo8cZ_udQZmc3W/view?usp=sharing

MacOS: Desktop

See MacOS: Chrome / Safari

…nto monil-splitBillTaxUpdate

dukenv0307 · 2024-05-16T03:31:13Z

@MonilBhavsar I found the following bug

Go to FAB > Split
Enter small value
Select a workspace chat
Click Next
Observe that tax rate and tax amount are shown
Go back to amount page
Enter large value
Select the same workspace as before
Click Next
Observe that tax rate and tax amount are not shown

web-resize.mp4

dukenv0307 · 2024-05-16T03:31:57Z

It worked well when I enter the large value at the beginning

MonilBhavsar · 2024-05-16T06:41:44Z

Good catch! I have reproduced. Looking into it

…mation page

MonilBhavsar · 2024-05-16T11:46:52Z

Typescript checks are broken on main and being fixed here #42256

MonilBhavsar · 2024-05-16T11:55:41Z

src/pages/iou/request/step/IOURequestStepParticipants.tsx

-    const selectedReportID = useRef<string>(reportID);
+
+    // We need to set selectedReportID if user has navigated back from confirmation page and navigates to confirmation page with already selected participant
+    const selectedReportID = useRef<string>(participants?.length === 1 ? participants[0]?.reportID ?? reportID : reportID);


Bug was

User enters amount

Selects participants and clicks "Next"

Goes to confirmation page and see's Category, Tags and Tax fields

User goes back to 1 and edit's amount(doesn't matter)

User without selecting a participant clicks "Next" because participant was already selected(This is root cause)

Goes to confirmation page and doesn't see Category, Tags and Tax fields because reportID and policyID is not properly set from 5 above.

Let me know if you think this is not the correct way and should use separate hook or may be something else. Open to other solutions.

MonilBhavsar · 2024-05-17T09:43:16Z

TypeScript checks are passing now

dukenv0307 · 2024-05-20T03:46:37Z

@MonilBhavsar

Bug:

Go to FAB > Split
Enter any value
Select a workspace chat
Click Next
Observe that tax rate and tax amount are shown
Go back to amount page
Enter other value
Select the same workspace as before
Click Next
Observe that the tax amount is not updated

web-resize.mp4

MonilBhavsar · 2024-05-21T14:25:04Z

src/components/MoneyRequestConfirmationList.tsx

-        if (transaction?.taxAmount && previousTransactionAmount === transaction?.amount && previousTransactionCurrency === transaction?.currency) {
-            return IOU.setMoneyRequestTaxAmount(transactionID, transaction?.taxAmount ?? 0, true);
-        }


I mostly think this was pre optimization. When the user updates transactionAmount, previousTransactionAmount was not being changed, and we we were not setting updated taxAmount in Onyx

No, this introduced a bug where taxAmount is not being updated in draft transaction. looking...

This is fixed now with latest commit - c671c7a

MonilBhavsar · 2024-05-21T18:50:37Z

src/pages/iou/request/step/IOURequestStepAmount.tsx

            return `${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${transactionID}`;
        },
    },
-    draftTransaction: {


withFullTransactionOrNotFound() returns draftTransaction or transaction depending upon condition. So I think we can remove this

MonilBhavsar · 2024-05-21T18:56:39Z

@dukenv0307 I have fixed the bug and also noticed bugs with Split scan flow. So also fixed that. Please take a look and let me know what do you think. Thank you! 🙌

MonilBhavsar · 2024-05-23T14:34:50Z

Thanks for reporting bugs 😄
I've fixed them. Let me know what do you think!

dukenv0307 · 2024-05-24T04:16:15Z

code looks good and tests well

melvin-bot · 2024-05-24T04:16:54Z

@jasperhuangg 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]

OSBotify · 2024-05-24T20:10:24Z

✋ 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 · 2024-05-27T14:24:35Z

🚀 Deployed to staging by https://github.com/jasperhuangg in version: 1.4.76-0 🚀

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

blimpich · 2024-05-27T17:52:45Z

I think this caused a few minor deploy blockers in the most recent deploy checklist.

bondydaa · 2024-05-27T20:54:53Z

I reverted this PR locally to see if it would fix #42655 and it appears to. so I think this might have caused a few blockers.

bondydaa · 2024-05-27T21:00:40Z

created a revert PR here #42670

discussion is on going in this thread https://expensify.slack.com/archives/C07J32337/p1716843389797979

bondydaa · 2024-05-27T21:44:08Z

we did revert this PR and cp'd it to staging

Julesssss · 2024-05-28T09:30:50Z

Just adding a linked bug reported here. I'm going to remove the blocking label though.

OSBotify · 2024-05-29T09:17:13Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.76-7 🚀

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

Pass taxCode and taxAmount params for split bill request

c611187

MonilBhavsar self-assigned this Apr 15, 2024

Merge branch 'main' into monil-splitBillTaxUpdate

c7ec5f2

MonilBhavsar mentioned this pull request Apr 23, 2024

[$250] [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [Track Tax] Support tax tracking for split requests with workspace #39690

Closed

6 tasks

MonilBhavsar added 5 commits May 13, 2024 15:13

Merge branch 'main' into monil-splitBillTaxUpdate

8cb3a49

Merge branch 'monil-splitBillTaxUpdate' of github.com:Expensify/App i…

bb6d480

…nto monil-splitBillTaxUpdate

Merge branch 'main' into monil-splitBillTaxUpdate

98e0e40

Split amount based on new logic and update optimistic transaction

477b381

Fix lint

ec48476

MonilBhavsar marked this pull request as ready for review May 15, 2024 14:04

MonilBhavsar requested a review from a team as a code owner May 15, 2024 14:04

MonilBhavsar requested review from dukenv0307 and removed request for a team May 15, 2024 14:12

MonilBhavsar added 2 commits May 16, 2024 16:48

Merge branch 'main' into monil-splitBillTaxUpdate

bd20eed

Fix bug when selectedReportID and hence policy is undefined on confir…

de50156

…mation page

MonilBhavsar commented May 16, 2024

View reviewed changes

Merge branch 'main' into monil-splitBillTaxUpdate

c3db0c8

Fix taxAmount not being calculated

c667482

MonilBhavsar commented May 21, 2024

View reviewed changes

MonilBhavsar added 2 commits May 21, 2024 21:02

Handle editing split bill case

cdfb3ad

Handle scan split requests optimistically

78bf98a

MonilBhavsar commented May 21, 2024

View reviewed changes

Fix lint

776856c

MonilBhavsar added 4 commits May 23, 2024 14:27

Merge branch 'main' into monil-splitBillTaxUpdate

c7d8b42

Fix taxAmount is negative optimistically

787c520

Fix lint and typecheck

d136b4a

Fix bug when updating taxAmount and refactor

c671c7a

MonilBhavsar changed the title ~~Handle tax for split bill requests~~ Handle tax for split requests May 23, 2024

dukenv0307 approved these changes May 24, 2024

View reviewed changes

melvin-bot bot requested a review from jasperhuangg May 24, 2024 04:16

jasperhuangg approved these changes May 24, 2024

View reviewed changes

jasperhuangg merged commit c5c1120 into main May 24, 2024

jasperhuangg deleted the monil-splitBillTaxUpdate branch May 24, 2024 20:02

github-actions bot mentioned this pull request May 27, 2024

Deploy Checklist: New Expensify 2024-05-27 #42636

Closed

69 tasks

m-natarajan mentioned this pull request May 27, 2024

Tax - Split scan - Selected tax rate is duplicated in Tax rate list #42642

Closed

6 tasks

cretadn22 mentioned this pull request May 27, 2024

Revert "Handle tax for split requests" #42666

Closed

bondydaa mentioned this pull request May 27, 2024

Expense - Unable to select a different currency in transaction thread #42655

Closed

6 tasks

bondydaa mentioned this pull request May 27, 2024

Revert "Handle tax for split requests" #42670

Merged

izarutskaya mentioned this pull request May 28, 2024

Tax - Tax amount is not updated when changing Tax rate #42682

Closed

6 tasks

Conversation

MonilBhavsar commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

dukenv0307 commented May 16, 2024

Uh oh!

dukenv0307 commented May 16, 2024

Uh oh!

MonilBhavsar commented May 16, 2024

Uh oh!

MonilBhavsar commented May 16, 2024

Uh oh!

MonilBhavsar May 16, 2024

Choose a reason for hiding this comment

Uh oh!

MonilBhavsar commented May 17, 2024

Uh oh!

dukenv0307 commented May 20, 2024

Uh oh!

MonilBhavsar May 21, 2024

Choose a reason for hiding this comment

Uh oh!

MonilBhavsar May 21, 2024

Choose a reason for hiding this comment

Uh oh!

MonilBhavsar May 23, 2024

Choose a reason for hiding this comment

Uh oh!

MonilBhavsar May 21, 2024

Choose a reason for hiding this comment

Uh oh!

MonilBhavsar commented May 21, 2024

Uh oh!

MonilBhavsar commented May 23, 2024

Uh oh!

dukenv0307 commented May 24, 2024

Uh oh!

melvin-bot bot commented May 24, 2024

Uh oh!

OSBotify commented May 24, 2024

Uh oh!

OSBotify commented May 27, 2024

Uh oh!

blimpich commented May 27, 2024

Uh oh!

bondydaa commented May 27, 2024

Uh oh!

bondydaa commented May 27, 2024

Uh oh!

bondydaa commented May 27, 2024

Uh oh!

Julesssss commented May 28, 2024

Uh oh!

OSBotify commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MonilBhavsar commented Apr 15, 2024 •

edited

Loading