[HOLD for payment 2024-10-14] [HOLD for payment 2024-10-11] [$250] Distance - Waypoints change to duplicate waypoints after drag and drop waypoints

[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: 9.0.44-3
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5040727
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Submit expense > Distance
3. Enter three waypoints A - B - C
4. Drag waypoint B to waypoint C and now it becomes A - C - B
5. Drag waypoint C to waypoint B

Expected Result:

The resulting waypoints will be A - B - C

Actual Result:

The resulting waypoints become A - C - C

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

Bug6623484_1727976649706.20241004_012620.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021841915588250417056
• Upwork Job ID: 1841915588250417056
• Last Price Increase: 2024-10-03
• Automatic offers:
• nkdengineer | Contributor | 104254910

Issue Owner

Current Issue Owner: @mananjadhav

[melvin-bot]

Triggered auto assignment to @Beamanator (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

We think that this bug might be related to #wave-collect - Release 1

[melvin-bot]

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

[melvin-bot]

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

[Beamanator]

This looks external b/c I'm seeing that the data sent to the backend (for GetRouteForDraft looks like it's incorrect / in the incorrect order), so it looooks like it's the front end's fault for sending the wrong data

[Beamanator]

cc @bernhardoj in case you have ideas here

[Beamanator]

After adding A, B, C, then moving C to B, I see 3 GetRouteForDraft calls which isn't great 🤔

• I don't see this on prod 🤔 so ya this is a pretty decent blocker

[nkdengineer]

Proposal

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

The resulting waypoints become A - C - C

What is the root cause of that problem?

The problem is here when we drag and drop waypoints, the emptyWaypointIndex is -1 but we still call removeWaypoint

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

Lines 389 to 392 in e8c4faf

	TransactionAction.removeWaypoint(transaction, emptyWaypointIndex.toString(), IOUUtils.shouldUseTransactionDraft(action)),
	TransactionAction.updateWaypoints(transactionID, newWaypoints, IOUUtils.shouldUseTransactionDraft(action)),
	]).then(() => {
	setOptimisticWaypoints(null);

Then here the removed is not empty because we splice(-1, 1) which will remove the last waypoint

App/src/libs/actions/Transaction.ts

Line 141 in e8c4faf

const removed = waypointValues.splice(index, 1);

So the GetRouteForDraft is often called because the waypoint is changed wrongly.

I noticed that the bug also happens in production but it's swift and only calls GetRouteForDraft one time maybe because these functions run very fast.

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

Lines 389 to 392 in e8c4faf

	TransactionAction.removeWaypoint(transaction, emptyWaypointIndex.toString(), IOUUtils.shouldUseTransactionDraft(action)),
	TransactionAction.updateWaypoints(transactionID, newWaypoints, IOUUtils.shouldUseTransactionDraft(action)),
	]).then(() => {
	setOptimisticWaypoints(null);

Screen.Recording.2024-10-04.at.02.59.59.mov

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

We should return early here if the currentIndex is -1 because that means there is no waypoint that needs to be removed

App/src/libs/actions/Transaction.ts

Line 134 in 3316136

function removeWaypoint(transaction: OnyxEntry<Transaction>, currentIndex: string, isDraft?: boolean): Promise<void | void[]> {

Or we can prevent calling removeWaypoint if emptyWaypointIndex is -1

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

Lines 389 to 392 in e8c4faf

	TransactionAction.removeWaypoint(transaction, emptyWaypointIndex.toString(), IOUUtils.shouldUseTransactionDraft(action)),
	TransactionAction.updateWaypoints(transactionID, newWaypoints, IOUUtils.shouldUseTransactionDraft(action)),
	]).then(() => {
	setOptimisticWaypoints(null);

What alternative solutions did you explore? (Optional)

[jasperhuangg]

@nkdengineer Thanks, can you get a fix up soon?

[melvin-bot]

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

[nkdengineer]

sure.

[jasperhuangg]

@Nodebrute changing that will introduce a previous regression that was solved, thanks for commenting though!

[Nodebrute]

@jasperhuangg Ah, my bad! I just realized I was testing on the wrong branch 😅

[nkdengineer]

@mananjadhav The PR is ready cc @jasperhuangg.

[Beamanator]

@nkdengineer thanks for the PR, so you didn't figure out what change caused the full regression eh? I still think at least the 3-calls of GetRouteForDraft per waypoint change was a regression b/c on prod i only saw that command called once per change

[jasperhuangg]

Ah hmm, it would be nice if we could also address the extra API calls in your PR @nkdengineer

[nkdengineer]

@jasperhuangg With my PR, that will also fix the extra API calls let see the result video.

Screen.Recording.2024-10-04.at.03.09.06.mov

[Beamanator]

Swapping @allroundexperts in to test, as discussed in slack here

[Beamanator]

PR was merged, being CP'd now

[Beamanator]

Looking good to me in staging!!

[melvin-bot]

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

• [CP Staging] prevent removing waypoint if it is unnecessary #50184

If no regressions arise, payment will be issued on 2024-10-11. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

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

• [CP Staging] prevent removing waypoint if it is unnecessary #50184

If no regressions arise, payment will be issued on 2024-10-14. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @nkdengineer requires payment automatic offer (Contributor)

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @OfstadC you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[OfstadC]

Payment Summary

• @allroundexperts - due $250 via manual New Expensify request
• @nkdengineer - paid $250 via Upwork

[JmillsExpensify]

$250 approved for @allroundexperts