[$500] Distance-In receipt, finish address shown is different from the address entered

[izarutskaya]

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.4.1-4
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:

1. Launch app
2. Tap on a workspace
3. Tap plus near compose----Request money
4. Tap Distance
5. Enter Duplicate(same)start and finish waypoints
6. Tap add stop
7. Enter a different address
8. Tap add stop again
9. Add same "add stop" address which is entered in step 7
10. Tap next
11. Tap distance amount
12. Tap distance request created to open distance request details page
13. Tap receipt
14. Note the waypoints displayed in receipt page

Expected Result:

The waypoints entered while creating distance request must be displayed in receipt.

Actual Result:

In receipt page, the finish address shown is different from the address entered while creating distance request.

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

Bug6285167_1700553234943.duplicate.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019b70146b213c00f1
• Upwork Job ID: 1726919846086856704
• Last Price Increase: 2023-11-21

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[DylanDylann]

Proposal

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

Distance-In receipt, finish address shown is different from the address entered

What is the root cause of that problem?

In this case, the optimistic data waypoints will be: A->A->B->B
The data is returned from BE will be: A->B

When we merge data from BE to ONYX we are using MERGE method, so that the result will be: A->B->B->B (Because BE only return 2 waypoints so the App only update 2 first waypoints to ONYX and retain 2 last ones)

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

IDEA: We need to clean the waypoints in optimistic data, If the user enters A->A->B->B, we only should update A->B to ONYX

Detail: Currently We don't have logic to prevent users from clicking the next button when they enter two consecutive same endpoints

Note that in getValidWaypoints function we had logic to remove a waypoint if 2 consecutive endpoints is same
In here

App/src/components/DistanceRequest/index.js

Line 180 in bf2d545

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

we should check if _.size(validatedWaypoints) < _.size(waypoints) we will prevent user from submitting

Also need to update here

App/src/components/DistanceRequest/index.js

Line 220 in bf2d545

{((hasError && _.size(validatedWaypoints) < 2) || hasRouteError) && (

In this function

App/src/components/DistanceRequest/index.js

Line 147 in bf2d545

const getError = () => {

we also add a message like "please remove duplicated consecutive waypoints" if _.size(validatedWaypoints) < _.size(waypoints)

We also prevent user going to next page by deeplink when they enter 2 consecutive same waypoints

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 98 in bf2d545

const isInvalidDistanceRequest = !isDistanceRequest || isInvalidWaypoint;

In here we also need to add condition like we did in DistanceRequest Page (_.size(validatedWaypoints) < _.size(waypoints))

What alternative solutions did you explore? (Optional)

When receiving data waypoint from BE, we need to update totally (using SET instead of MERGE)

[paultsimura]

This issue should have been fixed in #28477
cc: @abekkala @0xmiroslav

[rojiphil]

I also think this is dupe of #29895 or #28477.
cc: @strepanier03 @rushatgabhane
Can we agree on where we are solving this?

[rushatgabhane]

I think #29895 should be reopened because that is where we first approved @rojiphil's proposal.

But it was closed incorrectly

[tobyjsullivan]

Proposal

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

The order of waypoints is not preserved correctly when a money request is created with manual route including duplicate consecutive waypoints.

A simplified reproduction case can include just three waypoints:

1. San Fransisco, CA
2. San Fransisco, CA
3. San Diego, CA

The resulting request receipt will show different waypoints:

1. San Fransisco, CA
2. San Diego, CA
3. San Diego, CA

What is the root cause of that problem?

The list of waypoints differ between the optimistic transaction data and the saved transaction data. These waypoints are saved in an object instead of an array which causes the merge process to mangle the route when the final collection has fewer keys than the original.

Specifically, for my example above, the original waypoint includes three waypoints of which two are duplicated:

{
    "waypoint0": { "lat": 37.7749295, "lng": -122.4194155, "address": "San Francisco, CA, USA", "name": "San Francisco" },
    "waypoint1": { "lat": 37.7749295, "lng": -122.4194155, "address": "San Francisco, CA, USA", "name": "San Francisco" },
    "waypoint2": { "lat": 32.715738, "lng": -117.1610838, "address": "San Diego, CA, USA", "name": "San Diego" }
}

However, the waypoints are deduplicated before saving via the CreateDistanceRequest API command.

{
    "waypoint0": { "lat":37.7749295, "lng":-122.4194155, "address":"San Francisco, CA, USA", "name":"San Francisco" },
    "waypoint1": { "lat":32.715738, "lng":-117.1610838, "address":"San Diego, CA, USA", "name":"San Diego" }
}

Finally, these values are merged without deleting the extraneous waypoint property, resulting in the last waypoint being duplicated.

{
    "waypoint0": { "lat": 37.7749295, "lng": -122.4194155, "address": "San Francisco, CA, USA", "name": "San Francisco" },
    "waypoint1": { "lat": 32.715738, "lng": -117.1610838, "address": "San Diego, CA, USA", "name": "San Diego" },
    "waypoint2": { "lat": 32.715738, "lng": -117.1610838, "address": "San Diego, CA, USA", "name": "San Diego" }
}

It looks like this incorrect merging behaviour has been in place since #26591 which initially introduced waypoint correction.

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

The following multi-part proposal is designed to remove complexity from the overloaded TransactionUtils.getValidWaypoints(). This is accomplished by decomposing that helper function, fixing waypoints earlier in the workflow, and blocking users from creating requests with duplicate waypoints in the first place.

Part 1: Refactor waypoint utils into granular helpers

The existing implementation of TransactionUtils.getValidWaypoints(waypoints) is used in two places which each need and do slightly different things. This causes it to be overloaded.

• The createDistanceRequest() action needs a clean set of waypoints, excluding blank waypoints.
• MoneyRequestParticipantsPage counts the number of non-empty, non-repeated waypoints to ensure there are at least two.

These use cases can be served by removing getValidWaypoints(waypoints) and replacing it with the following set of simplified utils:

• TransactionUtils.removeEmptyWaypoints(waypoints) => WaypointCollection
• TransactionUtils.removeRepeatedWaypoints(waypoints) => WaypointCollection
• TransactionUtils.countDistinctWaypoints(waypoints) => number

Note that the first two functions are composable and would be used to implement the third.

Part 2: Stop removing repeated waypoints

Removing repeated waypoints from a report seems like an unintended side effect of #26591 reusing TransactionUtils.getValidWaypoints(). There's no indication that silently removing duplicates was intended. By contrast, an error message is displayed to the user when they are editing an existing report. The UX during report creation should be made consistent (this is covered in Part 4).

Instead, we can replace the call in this line to use the new TransactionUtils.removeEmptyWaypoints(waypoints) mentioned in Part 1.

Part 3: Remove empty waypoints earlier in the workflow

The workflow to create a distance-based request includes multiple steps:

• Step 1: Tap "Request Money"
• Step 2: Tap the "Distance" tab (conditional)
• Step 3: Add a waypoint (repeat for each waypoint)
• Step 4: Tap "Next" to navigate to the participant selection screen.
• Step 5: Tap on the Participant (e.g., Workspace name). Navigates to the confirmation screen.
• Step 6: Tap "Request $xx.xx" to create the request.

The waypoints are currently cleaned at Step 6; however, this is later in the workflow than is ideal. Instead, empty waypoints should be removed in Step 4, when the "Next" button is first tapped. This can be accomplished by expanding the callback
in NewDistanceRequestPage to remove empty waypoints and update the transaction.

Note: this is where we would also fix the initial waypoint merging issue. This can be fixed by setting unused waypoint keys to null.

Part 4: Present an error message to users when duplicate waypoints exist (Optional)

An error message is presented to users when editing a money request. This same error message should be displayed when creating a request.

What alternative solutions did you explore? (Optional)

• Fix only the waypoint merging. However, this did not resolve the fundamental complexity in TransactionUtils.getValidWaypoints() and was unsatisfying.
• Refactor TransactionUtils.getValidWaypoints() to remove overloading and complexity but continue to silently remove duplicate waypoints when creating the final transaction (the very last step of the workflow). However, I do not like that this keeps duplicate and empty waypoints in the transaction for longer than necessary. Instead, it seems invalid waypoints should be detected and cleaned as early as possible.
• Silently remove invalid waypoints earlier in the workflow. This is inconsistent with the behavior when editing an existing request. As such, I recommend displaying the same error message at the same step to make the workflows consistent.
• Stop removing duplicate waypoints altogether. Instead, they can be left in place and only discounted when checking a route has two or more steps in MoneyRequestParticipantsPage. In fact, I personally prefer this alternative approach as I don't see a technical reason that duplicate waypoints need to be removed at all. However, based on the current behaviour, I can only assume there is a product requirement for doing so.

[melvin-bot]

📣 @tobyjsullivan! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[tobyjsullivan]

Contributor details
Your Expensify account email: expensify.dev@computomatic.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0176782daf3ef21895

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[conorpendergrast]

Closing in favour of #29895, thanks for the context!