[HOLD on #39144] [$500] [MEDIUM] Distance - Blank fields when editing distance and returning online with distance editor open

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.24-7
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Go to workspace chat.
2. Create a distance request.
3. Navigate to distance request details page.
4. Go offline.
5. Click Distance and edit distance.
6. Click Distance.
7. Go online with distance editor open.

Expected Result:

The distance editor will not close and the details update accordingly.

Actual Result:

After returning online, the distance editor closes automatically and all the fields become blank.
On web, mweb and desktop app, the fields remain blank until the page is revisited.
On Android and iOS app, the details are blank for a while but still receive update on its own.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6340096_1705061991913.20240105_112608.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e1e98db2d0a90542
• Upwork Job ID: 1745795410999205888
• Last Price Increase: 2024-01-26

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

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

[bernhardoj]

Proposal

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

The money request view shows empty request data when updating the distance while offline and then open the edit distance page again.

What is the root cause of that problem?

The issue has 2 root causes. The first one lies in this logic.

App/src/pages/EditRequestDistancePage.js

Lines 57 to 65 in 176fb00

	useEffect(() => {
	hasWaypointError.current = Boolean(lodashGet(transaction, 'errorFields.route') || lodashGet(transaction, 'errorFields.waypoints'));
	// When the loading goes from true to false, then we know the transaction has just been
	// saved to the server. Check for errors. If there are no errors, then the modal can be closed.
	if (prevIsLoading && !transaction.isLoading && !hasWaypointError.current) {
	Navigation.dismissModal(report.reportID);
	}
	}, [transaction, prevIsLoading, report]);

The above logic is trying to close the modal/page once the edit distance request completes by checking the prev and current transaction.isLoading data which is set to true optimistically and false when success/fail.

However, if we reopen the edit distance page while the transaction.isLoading is still true, the prevIsLoading data will be true too and when transaction.isLoading becomes false, the above logic will fire thus the edit distance page closes.

For the second issue, it will happen when the page is dismissed/closed. That is because in DistanceRequest component, on component unmount, we will restore the transaction backup if we cancel the edit.

App/src/components/DistanceRequest/index.js

Lines 113 to 125 in 176fb00

	// On mount, create the backup transaction.
	TransactionEdit.createBackupTransaction(transaction);
	return () => {
	// If the user cancels out of the modal without without saving changes, then the original transaction
	// needs to be restored from the backup so that all changes are removed.
	if (transactionWasSaved.current) {
	return;
	}
	TransactionEdit.restoreOriginalTransactionFromBackup(transaction.transactionID);
	};
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

The way it works is, that when the component is mounted, the backup is created by copying the transaction from TRANSACTION to TRANSACTION_DRAFT. Any waypoint changes are directed to TRANSACTION. The problem is, that when the edit API request is successful, we clear the backup.

App/src/libs/actions/IOU.js

Lines 1068 to 1075 in 176fb00

	if (_.has(transactionChanges, 'waypoints')) {
	// Delete the draft transaction when editing waypoints when the server responds successfully and there are no errors
	successData.push({
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`,
	value: null,
	});
	}

When we close the edit distance page (canceling it), it will try to restore the backup, which is null.

App/src/libs/actions/TransactionEdit.ts

Lines 29 to 40 in 176fb00

	function restoreOriginalTransactionFromBackup(transactionID: string) {
	const connectionID = Onyx.connect({
	key: `${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transactionID}`,
	callback: (backupTransaction) => {
	Onyx.disconnect(connectionID);
	// Use set to completely overwrite the original transaction
	Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, backupTransaction);
	removeBackupTransaction(transactionID);
	},
	});
	}

So, we see the empty money request data.

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

For the first issue, we need to make sure the dismiss logic is fired/executed only if we press the save button. We can use a ref for this and set it to true anytime the save button is pressed. This way, the loading state from a previous request won't affect the new edit distance page.

For the second issue, we should check whether the backup contains data or not, if not, return early.
(or we can reverse the situation by saving any edit to TRANSACTION_DRAFT and only save it to TRANSACTION when confirming)

What alternative solutions did you explore? (Optional)

For the first issue, we can rely on the local loading state as shown below:

// 1. init the state
const [isLoading, setIsLoading] = useState(false)
const prevIsLoading = usePrevious(isLoading);

// 2. set the loading to true when pressing the button
setIsLoading(true)

// 3. set the loading to false when transaction.loading becomes false
useEffect(() => if (!transaction.isLoading) setIsLoading(false), [transaction.isLoading])

// 4. replace the dismiss logic with the new loading state
if (prevIsLoading && !isLoading

or the simplest one, disable the edit distance while transaction.isLoading is true.

For the second issue, one flaw in saving the backup to TRANSACTION_DRAFT is that it gets cleared when the edit API request is successful, so if that happens while we are on the edit distance page, the backup is gone and we never be able to restore it. This backup thing is only used for editing distance and is only cleared when the edit distance is successful in successData, so instead of clearing it when successful, we can clear (TransactionEdit.removeBackupTransaction) it immediately when the edit distance page is closed by saving it or without any edit.

App/src/components/DistanceRequest/index.js

Lines 116 to 122 in f6a2e78

	return () => {
	// If the user cancels out of the modal without without saving changes, then the original transaction
	// needs to be restored from the backup so that all changes are removed.
	if (transactionWasSaved.current) {
	return;
	}
	TransactionEdit.restoreOriginalTransactionFromBackup(transaction.transactionID);

// we want to clear the backup without restoring it if the changes is saved or there are no any changes/edit
if (transactionWasSaved.current || !transactionWasEdited.current) {
    TransactionEdit.removeBackupTransaction(transaction.transactionID)
    return;
}

transactionWasEdited.current can be set to true when the waypoints are updated inside a useEffect.

[melvin-bot]

@stephanieelliott, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[stephanieelliott]

Hey @situchan can you review this proposal when you get a chance?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[situchan]

reviewing proposal

[kmbcook]

Proposal

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

Blank fields when editing distance and returning online with distance editor open.

What is the root cause of that problem?

The component pages/EditRequestDistancePage sends components/DistanceRequest the id of the transaction, which is a problem since components/DistanceRequest actually modifies that transaction before the user presses Save, which ideally should never happen.

There is a completely separate cause for the problem of the distance editor closing automatically which might be resolved in a separate issue.

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

Only allow changes to be made to the transaction after the user presses Save. Change pages/EditRequestDistancePage so that it does not send the id of the current transaction to components/DistanceRequest. All that DistanceRequest actually needs are the current waypoints, and all that it actually returns to EditRequestDistancePage are the new waypoints, as parameters to the onSubmit function when the user presses Save. So to solve the problem, essentially only send waypoints to DistanceRequest, which can be done by creating a temporary transaction which only contains the current waypoints.

[situchan]

@kmbcook thanks for the proposal. Your RCA doesn't make sense to me. Please check Expected and Actual Result in OP.

[kmbcook]

@situchan I'll do my best. The expected result is that the field are not blank, and the actual result is that they are blank. My proposal addresses that problem, not the problem of the distance editor closing automatically. The blank fields occur because components/DistanceRequest sets the transaction to null. The reason it sets the transaction to null is because it is trying to undo any changes it may possibly have made, using a backup of the original transaction, which in this case doesn't work because the backup has been erased. In this case there is no longer a backup transaction. Yes, there is no backup transaction, but is a backup transaction necessary? No, not if DistanceRequest never changes the original transaction. When the user presses Save, DistanceRequest returns new waypoints to pages/EditRequestDistancePage, and EditRequestDistancePage uses those waypoints to modify the transaction. All that EditRequestDistancePage needs from DistanceRequest is new waypoints, and only if the user presses Save. There is no need for DistanceRequest to modify the transaction. DistanceRequest should not be allowed to make changes to the original transaction. Right now it is designed to make changes, and then undo those changes if the user does not press save. It is in the process of undoing, in this case, that the original transaction is accidentally set to null. A cause of the fields becoming blank is that DistanceRequest is permitted to make any changes at all to the original transaction. Why would we want to change the original transaction before the user presses Save? Doing so invites bugs, and does not provide any value.

[situchan]

@bernhardoj thanks for the proposal. The root cause is correct. Do you mind sharing test branch to verify your solution?

[bernhardoj]

@situchan So, I planned to use my main solution to fix the blank field issue (by using TRANSACTION_DRAFT when editing so we don't need backup) and my alternative solution for the auto-close distance page issue. Currently, we use EditRequestDistancePage which uses DistanceRequest component to do the distance edit, but I just remembered that we have a new component (from the money request refactor) called IOURequestStepDistance that already uses TRANSACTION_DRAFT when updating the waypoint and we have an ongoing issue that will replace EditRequestDistancePage with IOURequestStepDistance.

It should solve the blank field issue, but not the auto-close issue. To avoid double work, I think we can hold for that cleanup issue and come back here once done.

For the auto-close issue, here is the diff:

diff

diff --git a/src/pages/EditRequestDistancePage.js b/src/pages/EditRequestDistancePage.js
index f3ea76a339..bb1cdc27bc 100644
--- a/src/pages/EditRequestDistancePage.js
+++ b/src/pages/EditRequestDistancePage.js
@@ -1,6 +1,6 @@
 import lodashGet from 'lodash/get';
 import PropTypes from 'prop-types';
-import React, {useEffect, useRef} from 'react';
+import React, {useEffect, useState, useRef} from 'react';
 import {withOnyx} from 'react-native-onyx';
 import _ from 'underscore';
 import DistanceRequest from '@components/DistanceRequest';
@@ -52,17 +52,25 @@ function EditRequestDistancePage({report, route, transaction, transactionBackup}
     const {isOffline} = useNetwork();
     const {translate} = useLocalize();
     const hasWaypointError = useRef(false);
-    const prevIsLoading = usePrevious(transaction.isLoading);
+    const [isLoading, setIsLoading] = useState(false);
+    const prevIsLoading = usePrevious(isLoading);
+
+    useEffect(() => {
+        if (transaction.isLoading) {
+            return;
+        }
+        setIsLoading(false);
+    }, [transaction.isLoading]);
 
     useEffect(() => {
         hasWaypointError.current = Boolean(lodashGet(transaction, 'errorFields.route') || lodashGet(transaction, 'errorFields.waypoints'));
 
         // When the loading goes from true to false, then we know the transaction has just been
         // saved to the server. Check for errors. If there are no errors, then the modal can be closed.
-        if (prevIsLoading && !transaction.isLoading && !hasWaypointError.current) {
+        if (prevIsLoading && !isLoading && !hasWaypointError.current) {
             Navigation.dismissModal(report.reportID);
         }
-    }, [transaction, prevIsLoading, report]);
+    }, [transaction, prevIsLoading, isLoading, report]);
 
     /**
      * Save the changes to the original transaction object
@@ -79,6 +87,7 @@ function EditRequestDistancePage({report, route, transaction, transactionBackup}
             return;
         }
 
+        setIsLoading(true);
         IOU.updateMoneyRequestDistance(transaction.transactionID, report.reportID, waypoints);
 
         // If the client is offline, then the modal can be closed as well (because there are no errors or other feedback to show them

[situchan]

Still holding

[stephanieelliott]

Still held on both #34610 and #34613.

[stephanieelliott]

Still held on both #34610 and #34613.

[stephanieelliott]

Still on hold for #34610 (which is also still on hold)

[stephanieelliott]

Still on hold, #34610 also on hold

[stephanieelliott]

Still on hold for #34610 (which is also still on hold)

[stephanieelliott]

Still held

[bernhardoj]

The PR is merged but looks like there are a couple of regressions, let's keep holding until it's stable.

[bernhardoj]

So, I just retested and both issues don't happen anymore. The blank field bug is fixed because we removed the backup logic (but I see they are reintroducing it here) and the auto close bug is gone because we removed the loading logic when editing the distance. I'm not sure though whether it's intentional to remove the loading when editing the request.

Previously, when you edit a distance, the save button will show a loading indicator and the page will close only when the request is successful.

Now, pressing the Save button will immediately close the page.

[stephanieelliott]

Yeah maybe #37850 changed the loading behavior? Either way I think that is the intended behavior based on OFFLINE_UX.md. I also can't repro so it does seem like this is fixed, I suppose we can close?

[bernhardoj]

Yeah maybe #37850 changed the loading behavior?

Oh, it's not that one. We are holding on to this PR and that PR removes the behavior where users will see a loading when updating a distance routes/waypoints while online. I don't see any discussion from them about removing that behavior, so I'm confused whether it's expected or not.

If it's expected, then we can skip the auto-close issue. If not, then we need to reintroduce it and fix the auto-close page issue. Can you confirm this one, please?

I suppose we can close?

I think we should hold longer. The reason we hold for #35302 PR is because they are removing a logic that causes this GH bug, but I see they are reintroducing that again because that causes another bug. So, if that's merged, then we may need to proceed with my initial solution.

[stephanieelliott]

Now, pressing the Save button will immediately close the page.

This is the intended behavior, so it seems like we are good on this piece!

I think we should hold longer. The reason we hold for #35302 PR is because they are removing a logic that causes this GH bug, but I see they are reintroducing that again because that causes another bug. So, if that's merged, then we may need to proceed with my initial solution.

Ok, so are you saying that we should now hold on PR #39144, then try testing again to see if it causes us to get the blank fields again?

[bernhardoj]

Correct.

[stephanieelliott]

Ok great makes sense, let's do that!

[stephanieelliott]

#39144 is undergoing QA, we'll take this off hold soon once that hits staging

[stephanieelliott]

#39144 was merged, just retested and the behavior is no longer happening -- that is, when coming back online the fields have updated and are not blank