[HOLD for payment 2023-11-22] [$500] Distance - Empty waypoint is not discarded but filled with Finish address in distance request

[lanitochka17]

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: 1.3.86-1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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 staging.new.expensify.com
2. Go to workspace chat > + > Request money > Distance
3. Enter a Start point
4. Leave Finish point empty
5. Click Add stop
6. Enter another distance and proceed with the request creation
7. Open the distance expense details page
8. Click on the Distance

Expected Result:

Since the waypoint in Step 4 is empty, it should be discarded automatically when the distance request is created

Actual Result:

The empty waypoint is now filled with the address in Finish point

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

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Bug6242005_1697648681894.20231018_192459__1_.mp4

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017934cb2ae94a72d9
• Upwork Job ID: 1716323415346421760
• Last Price Increase: 2023-10-23
• Automatic offers:
• cubuspl42 | Reviewer | 27557516
• paultsimura | Contributor | 27557519

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[paultsimura]

Proposal

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

When creating a Distance request with an empty stop in the middle, it is filled with the Finish address.

What is the root cause of that problem?

When submitting a Distance Request, we filter the valid waypoints that will be passed to the BE here:

App/src/libs/actions/IOU.js

Line 649 in 39d2a84

waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transaction.comment.waypoints, true)),

However, we do not remove invalid (or empty) waypoints from the IOU Transaction before building the optimistic data here:

App/src/libs/actions/IOU.js

Lines 622 to 637 in 39d2a84

	const {iouReport, chatReport, transaction, iouAction, createdChatReportActionID, createdIOUReportActionID, reportPreviewAction, onyxData} = getMoneyRequestInformation(
	report,
	participant,
	comment,
	amount,
	currency,
	created,
	merchant,
	userAccountID,
	currentUserEmail,
	optimisticReceipt,
	transactionID,
	category,
	tag,
	billable,
	);

We pass the existing transactionID, which is later fetched from Onyx when building an optimistic transaction:

App/src/libs/actions/IOU.js

Lines 509 to 520 in 3b86da6

	// If there is an existing transaction (which is the case for distance requests), then the data from the existing transaction
	// needs to be manually merged into the optimistic transaction. This is because buildOnyxDataForMoneyRequest() uses `Onyx.set()` for the transaction
	// data. This is a big can of worms to change it to `Onyx.merge()` as explored in https://expensify.slack.com/archives/C05DWUDHVK7/p1692139468252109.
	// I want to clean this up at some point, but it's possible this will live in the code for a while so I've created https://github.com/Expensify/App/issues/25417
	// to remind me to do this.
	const existingTransaction = existingTransactionID && TransactionUtils.getTransaction(existingTransactionID);
	if (existingTransaction) {
	optimisticTransaction = {
	...optimisticTransaction,
	...existingTransaction,
	};
	}

As a result, we build and push an optimistic transaction with the following waypoints into Onyx (middle waypoint is empty):

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": {
                "lat": 10.4958294,
                "lng": -66.884449,
                "address": "Av. Casanova, Caracas, Distrito Capital, Venezuela"
            },
            "waypoint1": {},
            "waypoint2": {
                "lat": 10.5050318,
                "lng": -66.84285249999999,
                "address": "Los Palos Grandes, Caracas 1060, Miranda, Venezuela"
            }
        }
    }
}

Then, the CreateDistanceRequest API response contains a merge transaction operation with the following data (contains only valid waypoints):

{
    "comment": {
        "waypoints": {
            "waypoint0": {
                "lat": 10.4958294,
                "lng": -66.884449,
                "address": "Av. Casanova, Caracas, Distrito Capital, Venezuela"
            },
            "waypoint1": {
                "lat": 10.5050318,
                "lng": -66.84285249999999,
                "address": "Los Palos Grandes, Caracas 1060, Miranda, Venezuela"
            }
        }
    }
}

We would expect here that the waypoints from the response would fully replace the waypoints in the optimistic transaction, but Onyx updates objects up to the last nested key, so since the response contains keys waypoints.waypoint0 and waypoints.waypoint1, they get updated. But the waypoints.waypoint2 remains untouched because it was absent in the API response.

As a result, we have a transaction with first 2 waypoints from API response, and third - the stale one from the optimistic transaction:

{
    "comment": {
        "waypoints": {
            "waypoint0": {
                "lat": 10.4958294,
                "lng": -66.884449,
                "address": "Av. Casanova, Caracas, Distrito Capital, Venezuela"
            },
            "waypoint1": {
                "lat": 10.5050318,
                "lng": -66.84285249999999,
                "address": "Los Palos Grandes, Caracas 1060, Miranda, Venezuela"
            },
            "waypoint2": {
                "lat": 10.5050318,
                "lng": -66.84285249999999,
                "address": "Los Palos Grandes, Caracas 1060, Miranda, Venezuela"
            }
        }
    }
}

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

We should clean up the existing transaction's waypoints in Onyx before building the optimistic data.

First, create a new function in actions/Transaction:

function cleanupWaypoints(transactionID: string): Promise<void> {
    const transaction = allTransactions[transactionID];
    const existingWaypoints = TransactionUtils.getWaypoints(transaction) ?? {};
    const validWaypoints = TransactionUtils.getValidWaypoints(existingWaypoints, true);

    const updatedWaypoints: WaypointCollection = lodashReduce(existingWaypoints, (result, value, key) => ({
        ...result,
        // Explicitly set the non-existing waypoints to null so that Onyx can remove them
        [key]: validWaypoints[key] ?? null,
    }), {});

    return Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
        comment: {
            waypoints: updatedWaypoints,
        },
    });
}

This function executes the same TransactionUtils.getValidWaypoints that we currently use in IOU.createDistanceRequest, and updates the transaction in Onyx so that the invalid waypoints are removed.

Second, clean up the existing transaction before calling the IOU.createDistanceRequest here (or alternatively, it can be done inside the IOU.createDistanceRequest:

    const createDistanceRequest = useCallback(
        (selectedParticipants, trimmedComment) => {
+            Transaction.cleanupWaypoints(props.iou.transactionID).then(() => {
                IOU.createDistanceRequest(...);
+            });
        },
        [props.report, props.iou.created, props.iou.transactionID, props.iou.category, props.iou.tag, props.iou.amount, props.iou.currency, props.iou.merchant, props.iou.billable],
    );

App/src/pages/iou/steps/MoneyRequestConfirmPage.js

Lines 190 to 207 in 39d2a84

	const createDistanceRequest = useCallback(
	(selectedParticipants, trimmedComment) => {
	IOU.createDistanceRequest(
	props.report,
	selectedParticipants[0],
	trimmedComment,
	props.iou.created,
	props.iou.transactionID,
	props.iou.category,
	props.iou.tag,
	props.iou.amount,
	props.iou.currency,
	props.iou.merchant,
	props.iou.billable,
	);
	},
	[props.report, props.iou.created, props.iou.transactionID, props.iou.category, props.iou.tag, props.iou.amount, props.iou.currency, props.iou.merchant, props.iou.billable],
	);

Third, remove the waypoints cleanup here, as it will already be done in the transaction:

App/src/libs/actions/IOU.js

Line 649 in 39d2a84

waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transaction.comment.waypoints, true)),

Result:

distance-empty-waypoint.mov

Update:

An important benefit of my solution compared to the one posted below (which was my alternative solution) is that it fixes not only an empty waypoint issue but also the issue when the same waypoint is added multiple times in a row:

Bug demo

Screen.Recording.2023-10-24.at.19.21.16.mp4

What alternative solutions did you explore? (Optional)

Alternatively, we can modify the UX behavior when adding waypoints: replace the first empty waypoint (if there is one) instead of adding a waypoint to the end and keeping an empty waypoint in the middle. However, this needs to be approved by the UX, since this behavior is different from the expected one (as described in the issue). I can provide code for this approach as well (changes in https://github.com/Expensify/App/blob/main/src/pages/iou/WaypointEditor.js), but did not include it here to keep the already long proposal a bit cleaner.

[jliexpensify]

This doesn't seem like an issue to me? If you don't enter a FINISH address but instead add a stop, it make sense that the generated request would have the stop as the finish address, since that's what we assume the destination would be?

cc @hayata-suenaga @neil-marcellini for your thoughts here

[paultsimura]

This doesn't seem like an issue to me? If you don't enter a FINISH address but instead add a stop, it make sense that the generated request would have the stop as the finish address, since that's what we assume the destination would be?

@jliexpensify The root cause I described above causes a worse scenario. Please see the recording:

trim.3B049C91-7C93-403C-AFB3-0EA9AF37B62B.MOV

[hayata-suenaga]

@jliexpensify the part marked in red below should not appear. so many regression from Wave 5 😢 Thank you for handling this 🙇

[jliexpensify]

Got it, thanks for confirming @hayata-suenaga - going to apply the External label and get this sorted.

[melvin-bot]

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

[melvin-bot]

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

[DylanDylann]

Proposal

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

Changing UX behavior, we decided to make the add button show up when there are 2+ valid waypoints.

What is the root cause of that problem?

Changing UX behavior, we decided to make the add button show up when there are 2+ valid waypoints.

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

1. We need to hide the add button if the start point and finish point aren't filled

    const numberOfFilledWaypoints = _.size(_.filter(waypoints, (waypoint) => !_.isEmpty(waypoint)));

and only display the add button if numberOfFilledWaypoints >=2

2. We should remove this function
   

   App/src/pages/iou/WaypointEditor.js

   Line 125 in 5b5d465

   const saveWaypoint = (waypoint) => {

This function is created to add a stop point to the middle of the start and finish point. With the new UX behavior, It becomes redundant.
We need to use Transaction.saveWaypoint instead of saveWaypoint(waypoint) in all places

App/src/pages/iou/WaypointEditor.js

Line 151 in 5b5d465

saveWaypoint(waypoint);

[jliexpensify]

@cubuspl42 we have a proposal above, any thoughts?

[paultsimura]

we have a proposal above

We have 2 of them, starting with this one: #29886 (comment)

[cubuspl42]

@paultsimura

I appreciate the detailed and deep root cause analysis! Not for the first time.

On the other hand, the proposal by @DylanDylann is less invasive, as it uses the already existing logic.

Is it possible that the local saveWaypoint function archives an equivalent effect to using your new cleanupWaypoints util?

I'm still trying to wrap my head around this.

[DylanDylann]

@cubuspl42 My solution is small change, I tested and It works well

[paultsimura]

@cubuspl42 the proposal from @DylanDylann was my alternative approach, but I did not include the actual code (my bad) to keep the proposal cleaner and it was different from the "Expected Behavior" described in the issue:

Alternatively, we can modify the UX behavior when adding waypoints: replace the first empty waypoint (if there is one) instead of adding a waypoint to the end and keeping an empty waypoint in the middle. However, this needs to be approved by the UX, since this behavior is different from the expected one (as described in the issue). I can provide code for this approach as well (changes in https://github.com/Expensify/App/blob/main/src/pages/iou/WaypointEditor.js), but did not include it here to keep the already long proposal a bit cleaner.

Please also check this part of my proposal, this is the main reason why I suggest going with the actual Onyx cleanup:

Update:
An important benefit of my solution compared to the one posted below (which was my alternative solution) is that it fixes not only an empty waypoint issue but also the issue when the same waypoint is added multiple times in a row:

But we can combine both the cleanup and the UX change.

The thing is, we are doing the waypoints cleanup here anyway:

App/src/libs/actions/IOU.js

Line 649 in 39d2a84

waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transaction.comment.waypoints, true)),

My solution just suggests moving this cleanup to the beginning of the CreateDistanceRequest flow.

[DylanDylann]

Don't agree this point:

the proposal from @DylanDylann was my alternative approach

Note that: @paultsimura edited his alternative solution after I posted proposal

[cubuspl42]

@DylanDylann Not the part we're discussing

[cubuspl42]

@paultsimura Your proposal is solid, but next time don't be afraid to go for the best / most reasonable solution. We quite often modify the app behavior (UX) slightly when solving issues, and it goes without UX approval. And, most importantly, the "Expected result" provided in the issue description should also be looked at with a healthy dose of scepticism. At times, it can be challenged and discussed.

[cubuspl42]

After giving it a thought, I'll suggest going with @DylanDylann, as they provided a simple change that solves the issue in practice while making an app behave reasonably.

C+ reviewed 🎀 👀 🎀

[paultsimura]

My PR's ready, I'm just waiting to be assigned to this issue.

[melvin-bot]

📣 @cubuspl42 🎉 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]

📣 @paultsimura 🎉 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 📖

[paultsimura]

Thanks. The PR is ready: #31017

[jliexpensify]

Hi @cubuspl42 - let me know what partial compensation you'd like to offer and I'll write it into my Payment Summary. Thanks!

[cubuspl42]

@jliexpensify I was thinking about $250 for @DylanDylann. It can be taken from my share.

[jliexpensify]

Thanks! Assuming there are no regressions/bonuses, here's what the Payment Summary would look like:

• Contributor: @paultsimura $500
• Contributor: @DylanDylann $250 - comment
• C+: @cubuspl42 $250

Upwork job

[jo-ui]

@jliexpensify this issue is dupe of #28477 this has been told many times but nothing has been done to close this issue and open the #28477

[paultsimura]

@jo-ui this is definitely not a dupe of #28477. In fact, during the investigation of this issue, a separate bug was found, but we decided not to fix it because it would be fixed in #28477

This has been told many times

I can't find any mention of this issue in #28477 – could you please point us to where it was mentioned as a dupe?

[jo-ui]

@paultsimura sorry for the confusion, I wrote the comment in the wrong issue sorry again

[melvin-bot]

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

[melvin-bot]

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

• fix: Distance – show "Add stop" button only when start & finish are filled #31017

If no regressions arise, payment will be issued on 2023-11-22. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @cubuspl42 requires payment offer (Reviewer)
• @paultsimura requires payment offer (Contributor)
• @DylanDylann requires payment

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@cubuspl42] The PR that introduced the bug has been identified. Link to the PR:
• [@cubuspl42] 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:
• [@cubuspl42] 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:
• [@cubuspl42] Determine if we should create a regression test for this bug.
• [@cubuspl42] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[cubuspl42]

• The PR that introduced the bug has been identified. Link to the PR:
  • It seems that the bug was present in the relevant components since they were created
• 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:
  • N/A
• 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 for additional discussion
• Determine if we should create a regression test for this bug.
  • Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • Go to "+" (FAB) -> "Request money" -> "Distance" tab
    • Verify that the "Add stop" button is not shown
  • Fill the "Start" waypoint
    • Verify that the "Add stop" button is still not shown
  • Fill the "Finish" waypoint
    • Verify that the "Add stop" button is shown now
  • Press "Add stop" and add a waypoint
    • Verify that the new waypoint is added at the end of a route and is now marked as "Finish"
  • Remove two waypoints, so only one is filled
    • Verify that the "Add stop" button is not shown anymore

[jliexpensify]

There's some issues with Upworks atm, going to try paying later today

[jliexpensify]

Everyone is now paid and job is closed!