[$500] Distance - Duplicate waypoint error message is not dimissed after closing distance editor

[lanitochka17]

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.30-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:
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:

Precondition: App location access is disabled

1. Go to workspace chat
2. Create a distance request with A-B-A address
3. Navigate to request details page
4. Click Distance
5. Drag the waypoint so that it becomes A-A-B
6. Save the request
7. Exit the distance editor
8. Refresh the page
9. Click Distance

Expected Result:

The duplicate waypoint error message is dismissed

Actual Result:

The duplicate waypoint error message is not dismissed

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

Bug6352460_1706039016934.20240123_235006.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018929236a56deb238
• Upwork Job ID: 1749887372530053120
• Last Price Increase: 2024-01-30
• Automatic offers:
• alitoshmatov | Reviewer | 28142961
• tienifr | Contributor | 28142962

Issue Owner

Current Issue Owner: @JmillsExpensify

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (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 - @alitoshmatov (External)

[lanitochka17]

We think that this bug might be related to #wave6.
CC @greg-schroeder

[paultsimura]

Proposal

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

The "Duplicate waypoint" error remains between the "edit distance" page refreshes.

What is the root cause of that problem?

For the distance request, we use a hook that restores the transaction to the original one if it was not saved:

App/src/components/DistanceRequest/index.js

Lines 117 to 126 in 872eb17

	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
	}, []);

However, in this issue's scenario, we save the transaction (transactionWasSaved.current becomes true), but the changes are not persisted due to the route error. However, we still return early because the condition of transactionWasSaved.current is met, and therefore do not fully restore the transaction to its original state.

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

We should add an extra check – early return only if the transaction was saved without a route error:

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

What alternative solutions did you explore? (Optional)

[JordanLevy19]

Proposal

Please re-state the problem that we are trying to solve in this issue.
The error message regarding non-unique waypoints in a distance request feature persists even after a user refreshes the page. This indicates that the error state is not being reset as expected during a page reload.

What is the root cause of that problem?
The root cause of the error message persisting after a page refresh is perhaps due to the hasError state in the DistanceRequest component not being reset correctly. The state is initially set to false, but the code only explicitly resets it in a specific useEffect cleanup function tied to the transaction and transactionID dependencies. If this cleanup function doesn't trigger correctly on a page refresh - either because the component isn't unmounting/remounting properly, or the dependencies don't change as expected - then the hasError state remains true, causing the error message to persist. Additionally, the submitWaypoints function sets this error state under certain conditions, but without a corresponding reset mechanism that activates on page refresh, the error state is not cleared.

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

Ensure hasError Resets on Page Refresh:
Modify the useEffect for transaction and transactionID to explicitly reset hasError not only when the component unmounts but also when it remounts, which typically happens on a page refresh. This can involve adding a condition to check if waypoints or relevant conditions have been reinitialized and reset hasError accordingly.

Review and Update submitWaypoints Logic:
Implement a mechanism in submitWaypoints to reset hasError when the conditions that initially set it to true no longer apply. This could involve adding a state update at the start of this function to reset the error state, or incorporating additional checks to ensure the state aligns with the current conditions.

Improve Waypoints Update Logic:
In updateWaypoints, add logic to reassess the error state whenever waypoints are successfully updated. This ensures that if the error condition related to waypoints is resolved, hasError is reset to false.

What alternative solutions did you explore? (Optional)

[samilabud]

Proposal

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

Distance - waypoint error message is not dimissed after closing distance editor

What is the root cause of that problem?

We are using Onyx to store and persist errors inside the transaction object:

App/src/types/onyx/Transaction.ts

Line 60 in 0efabc2

errorFields?: OnyxCommon.ErrorFields<'route'>;

Because of this when we got an error we going to keep it at least we save the waypoints again.

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

We should clean the transaction errors when unmount the Distance request component inside the next line:

App/src/components/DistanceRequest/index.js

Line 117 in 0efabc2

return () => {

The code result should be:

.
.
.
  return () => {
            // If there are error cached in onyx
            if (hasRouteError) {
                TransactionEdit.cleanTransactionErrors(transaction);
            }
.
.
.

And the cleanTransactionErrors could be (inside of https://github.com/Expensify/App/blob/main/src/libs/actions/TransactionEdit.ts):

function cleanTransactionErrors(transaction: OnyxEntry<Transaction>) {
    if (!transaction) {
        return;
    }
    const newTransaction = {
        ...transaction,
        errorFields: {
            route: {},
        },
    };
    Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION_DRAFT}${transaction.transactionID}`, newTransaction);
}

Result:

Distance.request.error.test.mp4

[Krishna2323]

Proposal

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

Distance - Duplicate waypoint error message is not dimissed after closing distance editor

What is the root cause of that problem?

We save the waypoints even if we have duplicates & thats why its been stored in ONYX and it persists after refresh.

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

We should not save the waypoint if we have duplicates. We need to return from here:

App/src/components/DistanceRequest/index.js

Line 212 in 0efabc2

if (_.size(validatedWaypoints) < 2 || hasRouteError || isLoadingRoute || (isLoading && !isOffline)) {

To validate we can add an extra check like, (_.keys(waypoints).length > 2 && _.size(validatedWaypoints) !== _.keys(waypoints).length) to check if the all waypoints are valid.

We already have done it inside IOURequestStepDistance:

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

Lines 168 to 170 in 0efabc2

	if (_.size(validatedWaypoints) < 2 || (_.keys(waypoints).length > 2 && _.size(validatedWaypoints) !== _.keys(waypoints).length) || hasRouteError || isLoadingRoute || isLoading) {
	setHasError(true);
	return;

Result

[Krishna2323]

Proposal Updated

Added permalink for reference.

[tienifr]

Proposal

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

The duplicate waypoint error message is not dismissed

What is the root cause of that problem?

We're not validating duplicate waypoints when editing waypoints, so when we call the API to save it, it will return a back-end error which will not be cleared when we reload the page.

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

Make sure to validate duplicate waypoints and show the duplicate waypoints error properly when editing waypoints

1. Below here, add a condition to return an error if there's duplicate waypoints, we can check that the size of the validatedWaypoints is smaller than the size of waypoints because if there's duplicate waypoints, it will be removed in validatedWaypoints

if (_.size(validatedWaypoints) < _.keys(waypoints).length) {
    return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
}

2. In here, update so that it will show the DotIndicatorMessage (aka the error message UI) if there's error in getError()

{((hasError && !_.isEmpty(getError())) || hasRouteError) && (

3. In here, update so that it will set hasError to true if there's an error when getError, the rest remains as currently logic

if (!_.isEmpty(getError()) || isLoadingRoute || (isLoading && !isOffline)) {

We can consider making the error returned from getError() a variable, using useMemo, it will look cleaner and can minimize rerendering. We can do the same inside IOURequestStepDistance as well.

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

@JmillsExpensify, @alitoshmatov Eep! 4 days overdue now. Issues have feelings too...

[alitoshmatov]

Missed this one, reviewing today

[melvin-bot]

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

[alitoshmatov]

@paultsimura Thank you for your proposal. Even if transaction is restored when closing the page, error is still persisting.

[alitoshmatov]

@JordanLevy19 Don't get me wrong but your proposal looks like written by a chatbot, it is very vague and has no context awareness

[JordanLevy19]

Hey @alitoshmatov - not wrong at all - this was actually my first go at one of these bugs, and I have definitely learned to be more specific about the issues and to and identify as much as possible where the actual bugs reside in the code files and components!

Thank you for the feedback!

[alitoshmatov]

@Krishna2323 Thank you for your proposal, you are suggesting how to prevent duplicate waypoints from being submitted. I think your solution is not complete because there is no error handling there. You are just preventing user from submitting duplicate waypoints

[alitoshmatov]

@samilabud Thank you for your proposal. Your RCA is correct and your solution does solve this issue. But I am not sure if it is expected behavior to clean all transaction errors when distance page is closed

[alitoshmatov]

@tienifr Thank you for your proposal. Your RCA is correct. I do agree we should validate waypoints and show duplicate error before user submits the edit just like creating distance request.

[tienifr]

@tienifr Thank you for your proposal. Your RCA is correct. I do agree we should validate waypoints and show duplicate error before user submits the edit just like creating distance request.

@alitoshmatov Thanks! Do you think we can move forward with my proposal?

[kmbcook]

In #34441 we have proposed to use a draft transaction for editing waypoints, which would solve this issue.

[alitoshmatov]

@kmbcook Can you elaborate how that issue will solve ours?

[kmbcook]

@alitoshmatov sorry, I was wrong.

[alitoshmatov]

We can go with @tienifr 's proposal

C+ reviewed 🎀 👀 🎀

[melvin-bot]

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tgolen]

LGTM 👍

[melvin-bot]

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

@JmillsExpensify @tgolen @alitoshmatov @tienifr 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!

[tienifr]

@JmillsExpensify PR hit production. Melvin is malfunctioning. Please add HOLD for payment label.

[tienifr]

@JmillsExpensify PR hit production. Melvin is malfunctioning. Please add HOLD for payment label.

@JmillsExpensify Would you mind helping with payment here? Thanks

[alitoshmatov]

@JmillsExpensify Bump

[JmillsExpensify]

If you need something urgent please please reach out to me in Slack in our open source community or directly 1:1. Sorry for the delay!

[JmillsExpensify]

Payment summary:

• Contributor: @tienifr $500
• Contributor+: @alitoshmatov $500

Also @alitoshmatov I don't believe I've seen a BZ checklist. Can you please complete one? Thank you.

[JmillsExpensify]

All contributors have been paid out though please @alitoshmatov help with the BZ checklist before we can close this on out!

[JmillsExpensify]

@alitoshmatov bump on the BZ checklist.

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: feat: enable next button #29125
• 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/29125/files#r1546740052
• A discussion in #expensify-bugs 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: No need
• Determine if we should create a regression test for this bug. No need

cc: @JmillsExpensify

[JmillsExpensify]

Great thanks so much! Given no regression test is required, I'll close the issue.