fix: unify TBD-related logic for Distance requests #34135

paultsimura · 2024-01-09T13:04:04Z

Details

Change places where "TBD" was used to use "Route pending..." instead;
Unify some pending-route-related logic to minimize potential discrepancies among different screens of the same transaction.
Update the Merchant to "Route pending..." when editing waypoints

Fixed Issues

$ #33362
PROPOSAL: #33362 (comment)

Large Slack discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1705425683928599

Tests

Same as QA

Offline tests

Same as QA

QA Steps

Go offline
Click FAB -> Distance
Proceed to the confirmation page

Verify the amount is blank
Verify the distance shows "Route pending..."

Complete the distance request
Navigate (if not already) to the chat that contains the recently created Distance request

Verify that the amount is displayed as Route pending...
Verify that in the LHN, the preview shows Route pending...
Verify the total Expense shows $0

Go Online (and wait a little)

Verify that the amount and distance are updated with the data from the BE

Navigate to this Distance request
Go offline again
Click "Distance"
Add a waypoint and click "Save"

Verify that the amount is blank
Verify that the distance field says Route pending
Verify the report title says Route pending

Click on the receipt thumbnail

Verify that the distance on the e-Receipt shows Route pending and the amount is not shown

Without going back online, modify the amount manually (available only if you're the admin or manager of the workspace)

Verify the amount field shows the manual amount
Verify the distance still shows "Route pending..."

Go to the parent Expense report

Verify the money request preview shows the manual amount and "Route pending..." in the subtitle
Verify in LHN, the report preview shows the manually requested amount.

PR Author Checklist

Screenshots/Videos

Android: Native

android-compressed.mp4

Android: mWeb Chrome

chrome-compressed.mp4

iOS: Native

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-09.at.15.02.49-compressed.mp4

iOS: mWeb Safari

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-09.at.14.54.34-compressed.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-01-20.at.20.28.56-compressed.mp4

MacOS: Desktop

Screen.Recording.2024-01-09.at.15.05.45-compressed.mp4

melvin-bot · 2024-01-09T14:17:49Z

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

# Conflicts: # src/components/ReportActionItem/MoneyRequestPreview.js

src/libs/ReportUtils.ts

src/libs/TransactionUtils.ts

src/components/ReportActionItem/ReportPreview.js

src/libs/TransactionUtils.ts

# Conflicts: # src/components/ReportActionItem/ReportPreview.js # src/libs/ReportUtils.ts

neil-marcellini · 2024-01-19T18:14:16Z

@paultsimura I'm going to convert this to a draft so we can tell when it's ready.

# Conflicts: # src/components/ReportActionItem/MoneyRequestPreview.js

paultsimura · 2024-01-20T20:27:31Z

src/libs/TransactionUtils.ts

@@ -266,8 +266,8 @@ function getDescription(transaction: OnyxEntry<Transaction>): string {
 /**
 * Return the amount field from the transaction, return the modifiedAmount if present.
 */
-function getAmount(transaction: OnyxEntry<Transaction>, isFromExpenseReport: boolean): number {
-    // IOU requests cannot have negative values but they can be stored as negative values, let's return absolute value
+function getAmount(transaction: OnyxEntry<Transaction>, isFromExpenseReport = false): number {


There are a bunch of places where this second parameter is not provided. So I've set it to default false to avoid "Wrong params count" errors.

amyevans · 2024-02-06T14:41:50Z

@paultsimura In case you didn't see, there's more conflicts 🥲

# Conflicts: # src/libs/ReportUtils.ts # src/libs/actions/IOU.ts

paultsimura · 2024-02-06T15:04:26Z

There's more conflicts 🥲

Thank you for the ping, resolved again ✔️

src/components/ReportActionItem/ReportPreview.tsx

src/components/ReportActionItem/MoneyRequestPreview.tsx

tgolen · 2024-02-06T16:05:53Z

src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

-    const isDistanceRequestWithoutRoute = isDistanceRequest && !hasRoute;
-    const formattedAmount = isDistanceRequestWithoutRoute
-        ? translate('common.tbd')
+    const isDistanceRequestWithPendingRoute = isDistanceRequest && (!hasRoute || !rate);


Why isn't this using isFetchingWaypointsFromServer()?

The isFetchingWaypointsFromServer depends on this variable, not vice versa:

App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

Lines 477 to 479 in e63553c

if (isDistanceRequestWithPendingRoute) {

IOU.setMoneyRequestPendingFields(transaction.transactionID, {waypoints: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD});

}

I feel like that's kind of an anti pattern. I think we should only call actions based directly on user action, not based on data changing. Do you agree Tim?

But we already do call these actions in multiple places when landing on different steps of the IOU creation. Since in all these places we update the draft transaction, it looks more like a way to transfer the state between the IOU creation steps, and not an actual data modification.

Ah, I think you're both right. @paultsimura you're correct that this is more akin to storing the form state between steps. @neil-marcellini I think you might be onto something about this looking like an anti-pattern.

What do you think about this?

Instead of calling IOU.setMoneyRequestPendingFields(transaction.transactionID, {waypoints: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}); on this page, set that data on the previous page when the user submits the form. I think it could be part of the data that gets set here:

App/src/pages/iou/request/step/IOURequestStepDistance.js

Line 159 in e83c24a

Transaction.updateWaypoints(transactionID, newWaypoints, true).then(() => {

The issue here is the following scenario:

The user completes the first steps up until the confirmation step while being offline. Then, on the confirmation step, they come back online. Here, the map should get fetched, and we no longer should have the waypoints pending (even though when we landed on this page, the map was still pending).

Oh, that darn offline flow gets confusing! Thanks for pointing that out. It's probably OK then, but I would ask that you also put that context into a code comment where it happens.

Got you, thanks for taking your time on this. Updated ✔️

Then, on the confirmation step, they come back online. Here, the map should get fetched, and we no longer should have the waypoints pending

Thank you for explaining. I think that flow makes sense, but how does that relate to this condition? If we are offline and set the waypoints pending when they are updated (as Tim suggested), then when the user is navigated to this page the pending fields will show correctly. Once they go back online I would think that the GetRoute response should clear the pending fields for the waypoints (and or the route), and then all data on the confirmation page would no longer show as pending. Does that make sense?

I'm going to leave this as a NAB. We could fix it in a follow up if needed.

Probably yes, but I'm not sure where to attach the workspace rate being still loaded in this case. That being said, I would really appreciate it if we could merge this PR as-is if it's an NAB. It's my longest one so far 🙂

src/components/MoneyRequestConfirmationList.js

src/components/DistanceEReceipt.js

neil-marcellini

I think it's pretty much good to go, but Tim did point out something a bit weird so I'll wait to approve until we get that settled.

neil-marcellini · 2024-02-06T19:06:27Z

src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

-    const isDistanceRequestWithoutRoute = isDistanceRequest && !hasRoute;
-    const formattedAmount = isDistanceRequestWithoutRoute
-        ? translate('common.tbd')
+    const isDistanceRequestWithPendingRoute = isDistanceRequest && (!hasRoute || !rate);


I feel like that's kind of an anti pattern. I think we should only call actions based directly on user action, not based on data changing. Do you agree Tim?

# Conflicts: # src/libs/ReportUtils.ts

paultsimura · 2024-02-06T23:21:04Z

@tgolen @neil-marcellini please re-review before we have new conflicts 🙂 🙏

amyevans · 2024-02-07T17:23:21Z

Can you merge main to clear up the failing reassure performance tests?

paultsimura · 2024-02-07T17:52:07Z

@amyevans done ✔️

OSBotify · 2024-02-07T21:45:39Z

✋ 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-02-08T18:43:25Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.4.39-0 🚀

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

OSBotify · 2024-02-12T14:01:04Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.39-8 🚀

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

paultsimura added 2 commits January 9, 2024 11:50

Use transaction.isLoading to identify pending Distance requests

f4aff81

Merge branch 'main' into fix/33362-is-loading-tbd

152b569

paultsimura changed the title ~~Fix/33362 is loading tbd~~ fix: unify TBD-related logic for Distance requests Jan 9, 2024

Use transaction.isLoading to identify pending Distance requests

b30a5e7

paultsimura marked this pull request as ready for review January 9, 2024 14:17

paultsimura requested a review from a team as a code owner January 9, 2024 14:17

melvin-bot bot requested review from parasharrajat and removed request for a team January 9, 2024 14:17

paultsimura mentioned this pull request Jan 9, 2024

[HOLD for payment 2024-02-19] [$500] Distance - Receipt shows original amount and distance instead of TBD when editing distance offline #33362

Closed

6 tasks

Merge branch 'main' into fix/33362-is-loading-tbd

f68e06e

# Conflicts: # src/components/ReportActionItem/MoneyRequestPreview.js

parasharrajat suggested changes Jan 16, 2024

View reviewed changes

src/libs/ReportUtils.ts Outdated Show resolved Hide resolved

src/libs/TransactionUtils.ts Outdated Show resolved Hide resolved

src/components/ReportActionItem/ReportPreview.js Outdated Show resolved Hide resolved

tgolen requested changes Jan 16, 2024

View reviewed changes

src/libs/TransactionUtils.ts Outdated Show resolved Hide resolved

src/libs/TransactionUtils.ts Outdated Show resolved Hide resolved

neil-marcellini self-requested a review January 16, 2024 19:46

paultsimura added 5 commits January 16, 2024 22:44

Merge branch 'main' into fix/33362-is-loading-tbd

27e9415

Rename isLoadingDistanceRequest -> isDistanceBeingCalculated

46dec59

Merge branch 'main' into fix/33362-is-loading-tbd

c05c602

# Conflicts: # src/components/ReportActionItem/ReportPreview.js # src/libs/ReportUtils.ts

Use "Route pending" instead of "TBD"

b87642a

Fix the pending route logic in Distance e-Receipt

5a24138

neil-marcellini marked this pull request as draft January 19, 2024 18:14

paultsimura added 3 commits January 20, 2024 14:12

Show the "Pending route..." as merchant for requests with manual amount

dd94a01

Merge branch 'main' into fix/33362-is-loading-tbd

cda5367

# Conflicts: # src/components/ReportActionItem/MoneyRequestPreview.js

Update hasPendingRoute logic

373c670

paultsimura requested review from tgolen and parasharrajat January 20, 2024 16:33

paultsimura marked this pull request as ready for review January 20, 2024 20:12

Update "Route pending" display based on amount

634b5fa

paultsimura commented Jan 20, 2024

View reviewed changes

Revert some changes

f2a7a06

Merge branch 'main' into fix/33362-is-loading-tbd

d22cfe5

# Conflicts: # src/libs/ReportUtils.ts # src/libs/actions/IOU.ts

paultsimura requested a review from neil-marcellini February 6, 2024 15:02

amyevans previously approved these changes Feb 6, 2024

View reviewed changes

tgolen requested changes Feb 6, 2024

View reviewed changes

Remove hasOnlyPendingDistanceRequests vars

a020a23

paultsimura dismissed amyevans’s stale review via a020a23 February 6, 2024 17:08

paultsimura requested a review from tgolen February 6, 2024 17:14

Merge branch 'main' into fix/33362-is-loading-tbd

1c849c3

neil-marcellini reviewed Feb 6, 2024

View reviewed changes

paultsimura added 3 commits February 6, 2024 23:13

Explain the setMoneyRequestPendingFields call

534b2c1

Merge branch 'main' into fix/33362-is-loading-tbd

82bb06e

# Conflicts: # src/libs/ReportUtils.ts

Merge branch 'main' into fix/33362-is-loading-tbd

749a395

paultsimura requested a review from neil-marcellini February 6, 2024 23:25

Merge branch 'main' into fix/33362-is-loading-tbd

8d493e2

tgolen approved these changes Feb 7, 2024

View reviewed changes

Merge branch 'main' into fix/33362-is-loading-tbd

2e4c40f

amyevans approved these changes Feb 7, 2024

View reviewed changes

neil-marcellini approved these changes Feb 7, 2024

View reviewed changes

neil-marcellini merged commit 60c7d92 into Expensify:main Feb 7, 2024
19 checks passed

github-actions bot mentioned this pull request Feb 8, 2024

Deploy Checklist: New Expensify 2024-02-08 #36168

Closed

77 tasks

kbecciv mentioned this pull request Feb 8, 2024

Web - IOU - Wrong values for Amount and Total fields in Offline #36184

Closed

6 tasks

paultsimura mentioned this pull request Feb 20, 2024

[$500] Distance - Total amount in report is not reset to 0.00 when changing distance request offline #36892

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: unify TBD-related logic for Distance requests #34135

fix: unify TBD-related logic for Distance requests #34135

paultsimura commented Jan 9, 2024 •

edited

Loading

melvin-bot bot commented Jan 9, 2024

neil-marcellini commented Jan 19, 2024

paultsimura Jan 20, 2024

amyevans commented Feb 6, 2024

paultsimura commented Feb 6, 2024

tgolen Feb 6, 2024

paultsimura Feb 6, 2024

neil-marcellini Feb 6, 2024

paultsimura Feb 6, 2024

tgolen Feb 6, 2024

paultsimura Feb 6, 2024

tgolen Feb 6, 2024 •

edited

Loading

paultsimura Feb 6, 2024

neil-marcellini Feb 7, 2024

paultsimura Feb 7, 2024

neil-marcellini left a comment

neil-marcellini Feb 6, 2024

paultsimura commented Feb 6, 2024

amyevans commented Feb 7, 2024

paultsimura commented Feb 7, 2024

OSBotify commented Feb 7, 2024

OSBotify commented Feb 8, 2024

OSBotify commented Feb 12, 2024

	if (isDistanceRequestWithPendingRoute) {
	IOU.setMoneyRequestPendingFields(transaction.transactionID, {waypoints: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD});
	}

fix: unify TBD-related logic for Distance requests #34135

fix: unify TBD-related logic for Distance requests #34135

Conversation

paultsimura commented Jan 9, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 9, 2024

neil-marcellini commented Jan 19, 2024

Choose a reason for hiding this comment

amyevans commented Feb 6, 2024

paultsimura commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paultsimura commented Feb 6, 2024

amyevans commented Feb 7, 2024

paultsimura commented Feb 7, 2024

OSBotify commented Feb 7, 2024

OSBotify commented Feb 8, 2024

OSBotify commented Feb 12, 2024

paultsimura commented Jan 9, 2024 •

edited

Loading

tgolen Feb 6, 2024 •

edited

Loading