[HOLD] for #48630][$250] Distance - Unable to save distance when Start and Stop waypoints are valid address

izarutskaya commented 2 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: v9.0.27-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4908819 Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 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.
Go offline.
Click Distance.
Change both Start and Stop waypoints to invalid address ( use random strings).
Save it.
Go online.
Click on the header subtitle to return to main chat.
Click on the expense preview.
Click Distance.
Note that the waypoints are valid addresses (from Step 3).
Click Save.

Expected Result:

The distance will be saved because Start and Stop waypoints are valid.

Actual Result:

Nothing happens after clicking Save button when Start and Stop waypoints are valid.

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/bddd22db-5784-490e-8c3f-076b2da15e1a

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~013c32d2d6ff5c2253
Upwork Job ID: 1830768674764828148
Last Price Increase: 2024-10-22

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @kadiealexander (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.

Krishna2323 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-09-02 12:19:32 UTC.

Proposal 

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

Distance - Unable to save distance when Start and Stop waypoints are valid address

What is the root cause of that problem?

When the backend returns the invalid waypoint error, it is stored in the transaction object, and optimistically the waypoints are saved in the comment object.
But when we go back to parent report and come back to transaction thread, the transaction details are re-fetched and the invalid waypoints are removed from the comment object.
Now the waypoints are correct but the error message is still shown because it was merged in the original transaction. https://github.com/Expensify/App/blob/9051e001e43134636b3df101e5ab337f03cde85d/src/pages/iou/request/step/IOURequestStepDistance.tsx#L351-L363

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

When hasRouteError & isEditing is true, we can set the optimisticWaypoints value to transactionBackup/transaction?.modifiedWaypoints. This will make sure that we show the invalid waypoints when we have a route error.

https://github.com/Expensify/App/blob/9051e001e43134636b3df101e5ab337f03cde85d/src/pages/iou/request/step/IOURequestStepDistance.tsx#L82

const [optimisticWaypoints, setOptimisticWaypoints] = useState<WaypointCollection | null>(hasRouteError && isEditing ? transactionBackup?.modifiedWaypoints ?? null : null);

What alternative solutions did you explore? (Optional)

We can manually clear the errors when the MoneyRequestView component mounts for the first time and we have a route error.
We can use the same functions to clear the error like: https://github.com/Expensify/App/blob/7e0bf3dbd64546e3e1f47935bf367d57a23a60b1/src/components/ReportActionItem/MoneyRequestView.tsx#L486-L487
Or we can do that in IOURequestStepDistance. We can check if there is no error except the route error and then we can clear the errors.
Or we can clear the errors when the MoneyRequestView is unmounted and we have a route error.

Result

Krishna2323 commented 2 months ago

Proposal Updated

Added alternative

Krishna2323 commented 2 months ago

Proposal Updated

Updated alternative

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

nkdengineer commented 2 months ago

Proposal

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

Nothing happens after clicking Save button when Start and Stop waypoints are valid.

What is the root cause of that problem?

After we go to the expense report or transaction thread report again, the transaction data is returned from BE that will reset the waypoint data of the transaction but the transaction error isn't cleared. Then we cannot save the distance without changing it.

https://github.com/Expensify/App/blob/ec8a5d974d18dea142da2c27e4a8a27ce0a70794/src/pages/iou/request/step/IOURequestStepDistance.tsx#L100

https://github.com/Expensify/App/blob/ec8a5d974d18dea142da2c27e4a8a27ce0a70794/src/pages/iou/request/step/IOURequestStepDistance.tsx#L398-L400

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

In MoneyRequestView, we can create a useEffect that will clear the route error if hasRoute is true and the route transaction error still exists. That will ensure that the route error is only cleared when the route is re-fetched because we can have a case after the waypoint error appears user can offline and open the expense report/transaction thread report again (in this case, the waypoint isn't re-fetched and we don't want to clear the error). We can create a new util to only clear the route error or re-use Transaction.clearError function.

useEffect(() => {
    if (!transaction?.errorFields?.route || !hasRoute) {
        return;
    }

    Transaction.clearRouteError(transaction.transactionID);
}, [hasRoute, transaction?.errorFields?.route, transaction?.transactionID]);

What alternative solutions did you explore? (Optional)

NA

sobitneupane commented 2 months ago

@Krishna2323 @nkdengineer Looks like the issue is not reproducible any longer.

I cannot reproduce the issue.

nkdengineer commented 2 months ago

@sobitneupane I can still able to reproduce this bug on the latest main.

https://github.com/user-attachments/assets/dc4aef04-a034-4ca6-9f47-5a679437bacd

sobitneupane commented 1 month ago

Thanks @nkdengineer

I don't think we should remove the error. The invalid waypoint error will be shown in the transaction thread, and when the user opens the transaction, they won't find any error. We should display the error alongside the invalid addresses.

sobitneupane commented 1 month ago

Thanks for the proposal @Krishna2323

const hasRouteError = !!transaction?.errorFields?.route
const [optimisticWaypoints, setOptimisticWaypoints] = useState<WaypointCollection | null>(hasRouteError && isEditing ? transactionBackup?.modifiedWaypoints ?? null : null);

What are the different cases that can lead to a route error (apart from an invalid waypoint)? I can think of three: duplicate waypoints, empty waypoints, no route found

I’d like to gauge first whether those cases would be affected by the proposed change.

Krishna2323 commented 1 month ago

@sobitneupane, these are the other two errors.

https://github.com/Expensify/App/blob/9051e001e43134636b3df101e5ab337f03cde85d/src/pages/iou/request/step/IOURequestStepDistance.tsx#L351-L363

sobitneupane commented 1 month ago

What are the different cases that can lead to a route error (apart from an invalid waypoint)?

I can think of these two cases:

no route found
Route exceeds max distance allocation.

    const hasRouteError = !!transaction?.errorFields?.route;
    const isEditing = action === CONST.IOU.ACTION.EDIT; 
    const [optimisticWaypoints, setOptimisticWaypoints] = useState<WaypointCollection | null>(hasRouteError && isEditing ? transactionBackup?.modifiedWaypoints ?? null : null);

@Krishna2323 It does not solve the issue. I can still reproduce the issue.

https://github.com/user-attachments/assets/bedd7178-b133-4e53-a118-cb3f7cfde7af

nkdengineer commented 1 month ago

I don't think we should remove the error. The invalid waypoint error will be shown in the transaction thread, and when the user opens the transaction, they won't find any error. We should display the error alongside the invalid addresses.

@sobitneupane The problem here is after we re-open the expense report, BE will return the data that overrides the invalid waypoint.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

sobitneupane commented 1 month ago

The problem here is after we re-open the expense report, BE will return the data that overrides the invalid waypoint.

@nkdengineer I got it. If we don't show an error, the user might think their change was successful.

melvin-bot[bot] commented 1 month ago

@sobitneupane @kadiealexander 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!

melvin-bot[bot] commented 1 month ago

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

sobitneupane commented 1 month ago

Waiting for proposal.

melvin-bot[bot] commented 1 month ago

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

wildan-m commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-20 06:08:06 UTC.

Proposal

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

Distance - Unable to save distance when Start and Stop waypoints are valid addresses

What is the root cause of that problem?

Server send error and this check will be true:

https://github.com/Expensify/App/blob/7af7a3f85e040be3b862fe4a6dfeeeeccffed8aa/src/pages/iou/request/step/IOURequestStepDistance.tsx#L408-L411

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

We should re-validate the route on the initialMount, since the error remains, but waypoints has corrected from api failureData

In src/hooks/useFetchRoute.ts add new state isInitialMount with default to true

    const [isInitialMount, setIsInitialMount] = useState(true);

And change shouldFetchRoute logic

https://github.com/Expensify/App/blob/7af7a3f85e040be3b862fe4a6dfeeeeccffed8aa/src/hooks/useFetchRoute.ts#L23

To

   const shouldFetchRoute = (isDistanceRequest && (isRouteAbsentWithoutErrors || haveValidatedWaypointsChanged || isInitialMount) && !isLoadingRoute && Object.keys(validatedWaypoints).length > 1);

or only initial re-validate if it has route error

    const shouldFetchRoute = (isDistanceRequest && (isRouteAbsentWithoutErrors || haveValidatedWaypointsChanged || (isInitialMount && hasRouteError)) && !isLoadingRoute && Object.keys(validatedWaypoints).length > 1);

And we need to skip hasRouteError validation when submitWaypoints in offline mode, because we allow unvalidated waypoints while offline, without this, the save button will be blocked in offline mode: src/pages/iou/request/step/IOURequestStepDistance.tsx

    const submitWaypoints = useCallback(() => {
        // If there is any error or loading state, don't let user go to next page.
        if ((duplicateWaypointsError || atLeastTwoDifferentWaypointsError || (hasRouteError && !isOffline) || isLoadingRoute || (!isEditing && isLoading))) {

And remove transactionBackup?.pendingFields?.waypoints here so it can contain values for the case in this issue

https://github.com/Expensify/App/blob/dfe8e950cbbfd22adb32c622110a45e6c9b12dd0/src/pages/iou/request/step/IOURequestStepDistance.tsx#L83

We don't really need that check, related PR https://github.com/Expensify/App/pull/51519 still working properly without that.

Branch for this solution

What alternative solutions did you explore? (Optional)

Provide X button in waypoint editor, so user can manually clear the errors and able to save the editor

wildan-m commented 1 month ago

Proposal updated

or only initial re-validate if it has route error

wildan-m commented 1 month ago

Proposal Updated

handle issue when offline -> online, error shown -> offline

melvin-bot[bot] commented 1 month ago

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

sobitneupane commented 1 month ago

Thanks for the proposal @wildan-m Can you please explain the output after the change?

And we need to skip hasRouteError validation when submitWaypoints in offline mode: src/pages/iou/request/step/IOURequestStepDistance.tsx

Can you please add reasoning for this change?

In my understanding, the issue occurs because, instead of displaying the invalid addresses when opening the distance page, we are showing a previous valid route along with errors from the invalid route, which leads to the problem. I believe this should be the expected output.

wildan-m commented 1 month ago

Proposal updated

Add reasoning behind allowing save button with unvalidated waypoints in offline
Add alternative solution

In my understanding, the issue occurs because, instead of displaying the invalid addresses when opening the distance page, we are showing a previous valid route along with errors from the invalid route, which leads to the problem. I believe https://github.com/Expensify/App/issues/48401#issuecomment-2338167468 should be the expected output.

That's true, but when we call the API, if there is error the failedData section of the API will restore the value to its previous correct value.

There is no guarantee that the user will re-open that distance request, restoring it to previous correct value is better than keeping the wrong values.

Consider this scenario: If we keep the wrong values, then user clicking the X button to clear the error, which function will fix the wrong values? assume the user will not re-open the distance page and stay on its parent report.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

sobitneupane commented 1 month ago

@kadiealexander When submitting a distance expense(offline) with invalid waypoints (random strings for Start and Stop addresses), an error is shown and the invalid waypoints are optimistically saved. However, after coming back online and returning to the transaction thread shows the valid waypoints, as fetched from the API, instead of the invalid ones. The issue arises because the transaction details are re-fetched, which updates the waypoints to valid ones but retains the error message from the original invalid waypoints. This creates a mismatch where valid waypoints are displayed, but the error still reflects the previous invalid input. The question is: should we display the invalid waypoints along with the error message, or should we show the valid waypoints returned by the API and remove the error?

sobitneupane commented 1 month ago

Started discussion in slack: https://expensify.slack.com/archives/C01GTK53T8Q/p1727685264166239

melvin-bot[bot] commented 1 month ago

@sobitneupane @kadiealexander this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 month ago

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

kadiealexander commented 1 month ago

@sobitneupane in my opinion, we should show the valid waypoints returned by the API and remove the error.

melvin-bot[bot] commented 1 month ago

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

sobitneupane commented 1 month ago

Thanks for the input, @kadiealexander.

I will re-review the proposals, taking this comment into account as the expected output.

sobitneupane commented 4 weeks ago

Thanks for the proposal, @wildan-m

As suggested here, we should remove the error as well.

sobitneupane commented 4 weeks ago

Or we can do that in IOURequestStepDistance. We can check if there is no error except the route error and then we can clear the errors.

@Krishna2323 Can you please share more details on how you intend to implement the plan.

wildan-m commented 4 weeks ago

As suggested https://github.com/Expensify/App/issues/48401#issuecomment-2387658858, we should remove the error as well.

@sobitneupane my solution will remove the error once the server validated the waypoints.

Why the error restored when we navigate back? that's because we aren't updating the transactionBackup waypoints data when we are online so error persists in backup and will be restored when we navigate back from waypoint editor. This navigate back issue will be resolved after the PR in this issue merged https://github.com/Expensify/App/issues/48630

melvin-bot[bot] commented 4 weeks ago

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

sobitneupane commented 3 weeks ago

@wildan-m In that case we should wait for https://github.com/Expensify/App/issues/48630 to get resolved.

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

@sobitneupane, @kadiealexander Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 weeks ago

@sobitneupane, @kadiealexander 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

sobitneupane commented 2 weeks ago

Still waiting on https://github.com/Expensify/App/issues/48630

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 1 week ago

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

wildan-m commented 4 days ago

Proposal Updated

Merge branch with the latest main that include PR for this issue https://github.com/Expensify/App/issues/48630
Remove unnecessary change so the waypoints not undefined

@sobitneupane, I've tested it and the error message removed as expected.

Expensify / App