[HOLD for payment 2025-01-24] [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving #52413

lanitochka17 · 2024-11-12T17:02:17Z

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: 9.0.60-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to workspace chat
Submit a distance expense
Go to transaction thread
Click Distance
Add another waypoint
Refresh the page while distance editor RHP is open (close and reopen the app on Android and iOS)
Click outside the RHP to dismiss it
Click Distance

Expected Result:

The waypoint added in Step 6 will not appear because the new distance is not saved

Actual Result:

The waypoint added in Step 6 appears even when the new distance is not saved

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6662755_1731429445776.20241113_003143.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021857103747653162057
Upwork Job ID: 1857103747653162057
Last Price Increase: 2024-12-19
Automatic offers:

rayane-djouah | Reviewer | 105406722
FitseTLT | Contributor | 105406724

Issue Owner

Current Issue Owner: @joekaufmanexpensify

melvin-bot · 2024-11-12T17:02:27Z

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

FitseTLT · 2024-11-12T18:13:58Z

Edited by proposal-police: This proposal was edited at 2024-11-21 19:03:45 UTC.

Proposal

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

Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving

What is the root cause of that problem?

We already have a code that is run on unmount to restore transaction here

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

Line 230 in 8a83e2b

    
           TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));

But it is never called for the case of a reloading the page
so when it is opened after refresh we re-load the changed transaction to the backup so if we dismiss it will have the new waypoints

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

Line 221 in 8a83e2b

TransactionEdit.createBackupTransaction(transaction);

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

We should update to only load backup transaction if the the transactionBackup is empty
we can use Onyx.connect and run the code inside the callback in createBackupTransaction
by updating createBackupTransaction

function createBackupTransaction(transaction: OnyxEntry<Transaction>, isDraft: boolean) {
    if (!transaction) {
        return;
    }

    // In Strict Mode, the backup logic useEffect is triggered twice on mount. The restore logic is delayed because we need to connect to the onyx first,
    // so it's possible that the restore logic is executed after creating the backup for the 2nd time which will completely clear the backup.
    // To avoid that, we need to cancel the pending connection.
    Onyx.disconnect(connection);
    const newTransaction = {
        ...transaction,
    };
    const conn = Onyx.connect({
        key: `${ONYXKEYS.COLLECTION.TRANSACTION_BACKUP}${transaction.transactionID}`,
        callback: (transactionBackup) => {
            Onyx.disconnect(conn);
            if (transactionBackup) {
                Onyx.set(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`, transactionBackup);
                return;
            }
            // Use set so that it will always fully overwrite any backup transaction that could have existed before
            Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION_BACKUP}${transaction.transactionID}`, newTransaction);
        },
    });
}

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

Line 221 in 64eaf2f

TransactionEdit.createBackupTransaction(transaction);

        TransactionEdit.createBackupTransaction(transaction, IOUUtils.shouldUseTransactionDraft(action));

Note that this will fix the bug not only on page reloadrefresh but also for app closing and reopening 👍

Or optionally
Or optionally

we need the setTimeout b/c by the time the effect is run useOnyx will not populate transactionBackup but

 setTimeout(() => {
            if (!transactionBackup) {
                // On mount, create the backup transaction.
                TransactionEdit.createBackupTransaction(transaction);
            }
        }, 0);

huult · 2024-11-13T06:02:05Z

Edited by proposal-police: This proposal was edited at 2024-11-13 06:06:56 UTC.

Proposal

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

Waypoint is saved when distance editor RHP is refreshed and dismissed without saving

What is the root cause of that problem?

When the page is reloaded, we can't execute this code to restore the original transaction from the backup.

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

Lines 222 to 236 in 3897516

    
           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) { 
        
                   TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1'); 
        
                   return; 
        
               } 
        
               TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action)); 
        
               // If the user opens IOURequestStepDistance in offline mode and then goes online, re-open the report to fill in missing fields from the transaction backup 
        
               if (!transaction?.reportID) { 
        
                   return; 
        
               } 
        
               Report.openReport(transaction?.reportID); 
        
           };

After the reload is complete, create a backup transaction with data that has not been restored to the original transaction. This causes the issue where we still see the waypoint edited but not saved

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

Lines 214 to 220 in 3897516

    
           useEffect(() => { 
        
               if (isCreatingNewRequest) { 
        
                   return () => {}; 
        
               } 
        
               // On mount, create the backup transaction. 
        
               TransactionEdit.createBackupTransaction(transaction);

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

To resolve this issue, we need to check if the page is reloaded, then we don't need to create a new backup with the edited data. By the following steps:

1 Create a new Onyx to indicate whether the page is reloaded or not

// src/ONYXKEYS.ts#L459
+      EDIT_DISTANCE_PAGE_RELOAD: 'editDistancePageReload_',

2 Listen for the event before the reload

// src/pages/iou/request/step/IOURequestStepDistance.tsx#L214
+    useEffect(() => {
+        window.addEventListener('beforeunload', () => {
+            Onyx.set(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`, true);
+        });

+        return () => {
+            window.removeEventListener('beforeunload', () => {
+                Onyx.set(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`, false);
+            });

+            Onyx.set(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`, false);
+        };
+    }, [transactionID]);

3 Add loading to wait value of edit distance reload

// src/pages/iou/request/step/IOURequestStepDistance.tsx#L75
+    const [isEditDistancePageRefreshing, isEditDistancePageResult] = useOnyx(`${ONYXKEYS.EDIT_DISTANCE_PAGE_RELOAD}${transactionID}`);
// src/pages/iou/request/step/IOURequestStepDistance.tsx#L530

+    if (isLoadingOnyxValue(isEditDistancePageResult)) {
+        return <FullScreenLoadingIndicator />;
+    }

4 Add condition to check create new back up or not.

// src/pages/iou/request/step/IOURequestStepDistance.tsx#L215
 useEffect(() => {
        if (isCreatingNewRequest) {
            return () => {};
        }

        // On mount, create the backup transaction.
-        TransactionEdit.createBackupTransaction(transaction);
+        InteractionManager.runAfterInteractions(() => {
+            requestAnimationFrame(() => {
+                // On mount, create the backup transaction.
+                if (isTransactionPageRefreshing === true || isTransactionPageRefreshing === undefined) {
+                    if (transactionWasSaved.current) {
+                        TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
+                        return;
+                    }
+                    TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));

+                    if (!transaction?.reportID) {
+                        return;
+                    }
+                    Report.openReport(transaction?.reportID);
+                    return;
+                }

+                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) {
                TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
                return;
            }
            TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));

            // If the user opens IOURequestStepDistance in offline mode and then goes online, re-open the report to fill in missing fields from the transaction backup
            if (!transaction?.reportID) {
                return;
            }
            Report.openReport(transaction?.reportID);
        };
        // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
    }, []);

POC

Screen.Recording.2024-11-13.at.12.57.58.mp4

Test Branch

What alternative solutions did you explore? (Optional)

joekaufmanexpensify · 2024-11-14T16:50:00Z

Reproduced.

melvin-bot · 2024-11-14T16:50:13Z

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

melvin-bot · 2024-11-14T16:50:18Z

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

melvin-bot · 2024-11-18T09:41:09Z

@joekaufmanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

rayane-d · 2024-11-18T12:15:44Z

Reviewing

rayane-d · 2024-11-20T22:12:13Z

we need the setTimeout b/c by the time the effect is run useOnyx will not populate transactionBackup but we can also optionally use Onyx.connect and run the code inside the callback in createBackupTransaction

@FitseTLT, could you please share a code example that demonstrates how to fix the bug by modifying the createBackupTransaction implementation?

FitseTLT · 2024-11-20T22:13:46Z

we need the setTimeout b/c by the time the effect is run useOnyx will not populate transactionBackup but we can also optionally use Onyx.connect and run the code inside the callback in createBackupTransaction

@FitseTLT, could you please share a code example that demonstrates how to fix the bug by modifying the createBackupTransaction implementation?

Will provide tomorrow 👍

huult · 2024-11-21T02:25:27Z

@rayane-djouah What do you think about my proposal above?

melvin-bot · 2024-11-21T16:00:07Z

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

FitseTLT · 2024-11-21T19:04:30Z

@rayane-djouah U can check it now. Updated

rayane-d · 2024-11-25T19:39:21Z

@joekaufmanexpensify could you please clarify the expected behavior? When we open a distance request, edit the itinerary on the Distance Editor RHP page, and then refresh the page without saving, should the changes be discarded (similar to dismissing the Distance Editor RHP)? Or should the changes persist as a draft until we either dismiss the page or click "Save"?

joekaufmanexpensify · 2024-11-25T21:06:01Z

Left thoughts here. I would expect us not to save the behavior if you refresh the page without saving.

rayane-d · 2024-11-25T21:15:57Z

@FitseTLT @huult Could you please update your proposals based on this?

FitseTLT · 2024-11-25T21:25:06Z

Updated according to the expectation above.

huult · 2024-11-26T08:12:22Z

@FitseTLT @huult Could you please update your proposals based on this?

I will update it today.

melvin-bot · 2024-11-26T09:01:12Z

@joekaufmanexpensify @rayane-djouah this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

joekaufmanexpensify · 2025-01-06T15:07:05Z

Moving to weekly given PRs are in review.

joekaufmanexpensify · 2025-01-13T17:00:12Z

PRs still in review

melvin-bot · 2025-01-17T13:33:42Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2025-01-17T13:33:48Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.86-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Fix - Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving #54475

If no regressions arise, payment will be issued on 2025-01-24. 🎊

For reference, here are some details about the assignees on this issue:

@FitseTLT requires payment automatic offer (Contributor)
@rayane-djouah requires payment automatic offer (Reviewer)

melvin-bot · 2025-01-17T13:33:49Z

@rayane-djouah @joekaufmanexpensify @rayane-djouah The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

joekaufmanexpensify · 2025-01-20T17:51:43Z

@rayane-djouah could you handle checklist here? TY!

joekaufmanexpensify · 2025-01-22T16:35:25Z

Asked here

rayane-d · 2025-01-22T17:32:12Z

BugZero Checklist:

Classify the bug:

Bug classification

Source of bug:

1a. Result of the original design (eg. a case wasn't considered)
1b. Mistake during implementation
1c. Backend bug
1z. Other:

Where bug was reported:

2a. Reported on production
2b. Reported on staging (deploy blocker)
2c. Reported on a PR
2z. Other:

Who reported the bug:

The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

Link to comment: https://github.com/Expensify/App/pull/26141/files#r1925694325
If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

Link to discussion: N/A
If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
[@joekaufmanexpensify] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

Link to issue: https://github.com/Expensify/Expensify/issues/464352

Regression Test Proposal


1. Go to workspace chat
2. Submit a distance expense
3. Go to transaction thread
4. Click Distance
5. Add another waypoint
6. Refresh the page while distance editor RHP is open (close and reopen the app on Android and iOS)
7. Verify that the waypoint is restored to the original value and the unsaved waypoint change (the waypoint added in step (5) is reverted

Do we agree 👍 or 👎

joekaufmanexpensify · 2025-01-24T18:43:47Z

Added regression test issue above. All set to issue payment!

joekaufmanexpensify · 2025-01-24T18:44:17Z

We need to pay:

@FitseTLT - $250 for PR via Upwork.
@rayane-djouah - $250 for PR via Upwork.

joekaufmanexpensify · 2025-01-24T18:45:52Z

@FitseTLT $250 sent and contract ended!

joekaufmanexpensify · 2025-01-24T18:46:01Z

@rayane-djouah $250 sent and contract ended!

joekaufmanexpensify · 2025-01-24T18:46:09Z

Upwork job closed.

joekaufmanexpensify · 2025-01-24T18:46:14Z

All set, thanks everyone!

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 12, 2024

melvin-bot bot assigned joekaufmanexpensify Nov 12, 2024

joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Nov 14, 2024

melvin-bot bot changed the title ~~Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving~~ [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving Nov 14, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 14, 2024

joekaufmanexpensify added this to [#whatsnext] #expense Nov 14, 2024

melvin-bot bot assigned rayane-d Nov 14, 2024

joekaufmanexpensify moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 14, 2024

melvin-bot bot added the Overdue label Nov 18, 2024

melvin-bot bot removed the Overdue label Nov 18, 2024

melvin-bot bot added the Overdue label Jan 6, 2025

joekaufmanexpensify added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 Overdue labels Jan 6, 2025

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 17, 2025

melvin-bot bot changed the title ~~[$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving~~ [HOLD for payment 2025-01-24] [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving Jan 17, 2025

melvin-bot bot removed the Reviewing Has a PR in review label Jan 17, 2025

garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Jan 21, 2025

rayane-d mentioned this issue Jan 22, 2025

Allow Editing Distance Requests #26141

Merged

57 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 24, 2025

joekaufmanexpensify closed this as completed Jan 24, 2025

github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2025-01-24] [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving #52413

[HOLD for payment 2025-01-24] [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving #52413

lanitochka17 commented Nov 12, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Nov 12, 2024

FitseTLT commented Nov 12, 2024 •

edited

Loading

huult commented Nov 13, 2024 •

edited

Loading

joekaufmanexpensify commented Nov 14, 2024

melvin-bot bot commented Nov 14, 2024

melvin-bot bot commented Nov 14, 2024

melvin-bot bot commented Nov 18, 2024

rayane-d commented Nov 18, 2024

rayane-d commented Nov 20, 2024

FitseTLT commented Nov 20, 2024

huult commented Nov 21, 2024

melvin-bot bot commented Nov 21, 2024

FitseTLT commented Nov 21, 2024

rayane-d commented Nov 25, 2024

joekaufmanexpensify commented Nov 25, 2024

rayane-d commented Nov 25, 2024

FitseTLT commented Nov 25, 2024

huult commented Nov 26, 2024

melvin-bot bot commented Nov 26, 2024

joekaufmanexpensify commented Jan 6, 2025

joekaufmanexpensify commented Jan 13, 2025

melvin-bot bot commented Jan 17, 2025

melvin-bot bot commented Jan 17, 2025

melvin-bot bot commented Jan 17, 2025

joekaufmanexpensify commented Jan 20, 2025

joekaufmanexpensify commented Jan 22, 2025

rayane-d commented Jan 22, 2025 •

edited by joekaufmanexpensify

Loading

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

[HOLD for payment 2025-01-24] [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving #52413

[HOLD for payment 2025-01-24] [$250] Distance- Waypoint is saved when distance editor RHP is refreshed and dismissed without saving #52413

Comments

lanitochka17 commented Nov 12, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Nov 12, 2024

FitseTLT commented Nov 12, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

huult commented Nov 13, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Test Branch

What alternative solutions did you explore? (Optional)

joekaufmanexpensify commented Nov 14, 2024

melvin-bot bot commented Nov 14, 2024

melvin-bot bot commented Nov 14, 2024

melvin-bot bot commented Nov 18, 2024

rayane-d commented Nov 18, 2024

rayane-d commented Nov 20, 2024

FitseTLT commented Nov 20, 2024

huult commented Nov 21, 2024

melvin-bot bot commented Nov 21, 2024

FitseTLT commented Nov 21, 2024

rayane-d commented Nov 25, 2024

joekaufmanexpensify commented Nov 25, 2024

rayane-d commented Nov 25, 2024

FitseTLT commented Nov 25, 2024

huult commented Nov 26, 2024

melvin-bot bot commented Nov 26, 2024

joekaufmanexpensify commented Jan 6, 2025

joekaufmanexpensify commented Jan 13, 2025

melvin-bot bot commented Jan 17, 2025

melvin-bot bot commented Jan 17, 2025

melvin-bot bot commented Jan 17, 2025

joekaufmanexpensify commented Jan 20, 2025

joekaufmanexpensify commented Jan 22, 2025

rayane-d commented Jan 22, 2025 • edited by joekaufmanexpensify Loading

BugZero Checklist:

Regression Test Proposal

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

joekaufmanexpensify commented Jan 24, 2025

lanitochka17 commented Nov 12, 2024 •

edited by melvin-bot bot

Loading

FitseTLT commented Nov 12, 2024 •

edited

Loading

huult commented Nov 13, 2024 •

edited

Loading

rayane-d commented Jan 22, 2025 •

edited by joekaufmanexpensify

Loading