[$500] Distance - Same location is allowed when it is entered in "Stop" #28477

kbecciv · 2023-09-29T13:08:33Z

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

Action Performed:

Open Request money
Change tab to distance
Click/tap on Add stop and enter location
Click/tap on Add stop and enter the same location again
Click/tap on Next

Expected Result:

The same location should not be allowed in the "Stop" field

Actual Result:

Adding the same location on stops is allowed if the values are entered in 'Stop' values not in Start and Finish

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.74.2
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
Notes/Photos/Videos: Any additional supporting documentation

2023-09-28.18-04-03.mp4

2023-09-27.17-09-08.mp4

mweb_stp.MP4

IOS_stp.MP4

Android_stp.mp4

Screen_Recording_20230929_090656_Chrome.mp4

Expensify/Expensify Issue URL:
Issue reported by: @jo-ui
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695823418913709

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01642f7f93e05ba24c
Upwork Job ID: 1707744185233846272
Last Price Increase: 2023-09-29

namhihi237 · 2023-09-29T13:08:34Z

Proposal

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

The Next button should disabled when adding the same stop and finish point without adding a different start point.

What is the root cause of that problem?

We are using getValidWaypoints function to get valid waypoints, thereby checking if the button is disabled or not

App/src/components/DistanceRequest.js

Line 112 in 38030f8

const validatedWaypoints = TransactionUtils.getValidWaypoints(waypoints);

App/src/components/DistanceRequest.js

Line 300 in 38030f8

isDisabled={_.size(validatedWaypoints) < 2 || hasRouteError || isLoadingRoute}

In the getValidWaypoints function we have logic to return valid waypoints:

App/src/libs/TransactionUtils.js

Lines 382 to 405 in 38030f8

    
               const validWaypoints = _.reduce( 
        
                   waypointValues, 
        
                   (acc, currentWaypoint, index) => { 
        
                       const previousWaypoint = waypointValues[lastWaypointIndex]; 
        
                       // Check if the waypoint has a valid address 
        
                       if (!waypointHasValidAddress(currentWaypoint)) { 
        
                           return acc; 
        
                       } 
        
                       // Check for adjacent waypoints with the same address 
        
                       if (previousWaypoint && currentWaypoint.address === previousWaypoint.address) { 
        
                           return acc; 
        
                       } 
        
                       const validatedWaypoints = {...acc, [`waypoint${reArrangeIndexes ? lastWaypointIndex + 1 : index}`]: currentWaypoint}; 
        
                       lastWaypointIndex += 1; 
        
                       return validatedWaypoints; 
        
                   }, 
        
                   {}, 
        
               ); 
        
               return validWaypoints; 
        
           }

When we leave start blank and have stop and finish the same. Then through iterations in reduce we have:

index = 0: lastWaypointIndex = -1 => previousWaypoint = null => go first if
index = 1: lastWaypointIndex = -1 => previousWaypoint = null => validatedWaypoints will be available
waypoint finish.
index = 2: lastWaypointIndex = 0 => previousWaypoint = {} This is not as expected previousWaypoint must finish waypoint.

So it results in the same finish and stop being added to the valid waypoint.

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

We should update logic to get correct previousWaypoint

let previousWaypoint = null;

Remove this line

App/src/libs/TransactionUtils.js

Line 416 in dfb3f9b

const previousWaypoint = waypointValues[lastWaypointIndex];

And assign new previousWaypoint

.....
previousWaypoint = currentWaypoint;
const validatedWaypoints = {...acc, [`waypoint${reArrangeIndexes ? lastWaypointIndex + 1 : index}`]: currentWaypoint};

What alternative solutions did you explore? (Optional)

N/A

melvin-bot · 2023-09-29T13:08:38Z

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

melvin-bot · 2023-09-29T13:08:39Z

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

melvin-bot · 2023-09-29T13:08:41Z

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 · 2023-09-29T13:08:44Z

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

rakshitjain13 · 2023-09-29T13:14:01Z

Proposal

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

Next Button Activated with Same Stop and Finish Point in Request Money

What is the root cause of that problem?

In
src/components/DistanceRequest.js
we check for validwaypoint and then decide wether we have to disable next button or not . The logic is to check the size of validatedWaypoints array which is shown below .

App/src/components/DistanceRequest.js

Line 300 in 71db96d

isDisabled={_.size(validatedWaypoints) < 2 || hasRouteError}

Now to generate validatedWaypoints the code is in file
src/libs/TransactionUtils.js

App/src/libs/TransactionUtils.js

Lines 357 to 390 in 71db96d

    
           function getValidWaypoints(waypoints, reArrangeIndexes = false) { 
        
               const sortedIndexes = _.map(_.keys(waypoints), (key) => getWaypointIndex(key)).sort(); 
        
               const waypointValues = _.map(sortedIndexes, (index) => waypoints[`waypoint${index}`]); 
        
               // Ensure the number of waypoints is between 2 and 25 
        
               if (waypointValues.length < 2 || waypointValues.length > 25) { 
        
                   return {}; 
        
               } 
        
               let lastWaypointIndex = -1; 
        
               const validWaypoints = _.reduce( 
        
                   waypointValues, 
        
                   (acc, currentWaypoint, index) => { 
        
                       const previousWaypoint = waypointValues[lastWaypointIndex]; 
        
                       // Check if the waypoint has a valid address 
        
                       if (!waypointHasValidAddress(currentWaypoint)) { 
        
                           return acc; 
        
                       } 
        
                       // Check for adjacent waypoints with the same address 
        
                       if (previousWaypoint && currentWaypoint.address === previousWaypoint.address) { 
        
                           return acc; 
        
                       } 
        
                       const validatedWaypoints = {...acc, [`waypoint${reArrangeIndexes ? lastWaypointIndex + 1 : index}`]: currentWaypoint}; 
        
                       lastWaypointIndex += 1; 
        
                       return validatedWaypoints; 
        
                   }, 
        
                   {}, 
        
               ); 
        
               return validWaypoints; 
        
           }

This function fails in a edge case where starting waypoints are invalid because here lastWaypointIndex is equal to -1 in starting which suggests that first waypoint will always be valid when waypoints size > 1 .

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

We have to have increment lastWaypointIndex till we find some valid waypoint and then it is good to go . Here is my solution

  let lastWaypointIndex = -1;
   let foundFirstValidWaypoint = false;

   const validWaypoints = _.reduce(
       waypointValues,
       (acc, currentWaypoint, index) => {
           const previousWaypoint = waypointValues[lastWaypointIndex];
          

           // Skip all starting invalid waypoints
           if (foundFirstValidWaypoint == false && !waypointHasValidAddress(currentWaypoint)) {
               lastWaypointIndex += 1;
               return acc;
           } else {
               foundFirstValidWaypoint = true;
           }

           // Check if the waypoint has a valid address
           if (!waypointHasValidAddress(currentWaypoint)) {
               return acc;
           }
           // console.log(lastWaypointIndex, index, currentWaypoint, previousWaypoint);
           // Check for adjacent waypoints with the same address
           if (previousWaypoint && currentWaypoint.address === previousWaypoint.address) {
               return acc;
           }

           const validatedWaypoints = {...acc, [`waypoint${reArrangeIndexes ? lastWaypointIndex + 1 : index}`]: currentWaypoint};

           lastWaypointIndex += 1;

           return validatedWaypoints;
       },
       {},
   );

Here I have introduced a new variable named as foundFirstValidWaypoint which is false in starting and once we find a validwaypoint we will set it to true till the time we will increment lastWaypointIndex in each iteration .

What alternative solutions did you explore? (Optional)

NA

tienifr · 2023-09-30T07:21:17Z

Proposal

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

Adding the same location on stops is allowed if the values are entered in 'Stop' values not in Start and Finish

What is the root cause of that problem?

The calculation of lastWaypointIndex here is not correct. It's incremented by 1 when we found a valid waypoint.

In the scenario:

Start: empty
Stop: California
Finish: California (same as Stop)

where Start is empty, lastWaypointIndex is still -1. When going to check the first Stop, it's a valid waypoint so now the lastWaypointIndex will be incremented to 0, this is not correct since index 0 is for the empty Start waypoint, while the waypoint that should be considered last is the current valid Stop waypoint. And when we go to the Finish waypoint and check for duplicate address, we should check it against the previous Stop waypoint (index 1) rather than the Start waypoint (index 0).

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

We can simply change this line to

lastWaypointIndex = index;

By this, when evaluating a waypoint, we always compare its address with the last valid waypoint before, which is the original intention.

What alternative solutions did you explore? (Optional)

We're currently allowing duplicate waypoints when creating a money request, as long as there're at least 2 valid waypoints. We can be stricter here by only allow the user to continue the flow if there's no duplicate waypoint in the waypoints list. If there's any duplicate adjacent waypoint, we can still draw the stops in the map, but the user must fix the duplicate waypoint before continuing.

tewodrosGirmaA · 2023-09-30T16:46:19Z

@abekkala I reported first here :https://expensify.slack.com/archives/C049HHMV9SM/p1695412007448899?thread_ts=1695412007.448899&cid=C049HHMV9SM could change my name , Thank you

DylanDylann · 2023-10-02T07:04:22Z

Proposal

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

There is no notification when selecting multiple Stop points with the same address

What is the root cause of that problem?

Let's see the logic to validate waypoints

App/src/libs/TransactionUtils.ts

Lines 424 to 444 in 732d48e

    
           let lastWaypointIndex = -1; 
        
           return waypointValues.reduce<WaypointCollection>((acc, currentWaypoint, index) => { 
        
               const previousWaypoint = waypointValues[lastWaypointIndex]; 
        
               // Check if the waypoint has a valid address 
        
               if (!waypointHasValidAddress(currentWaypoint)) { 
        
                   return acc; 
        
               } 
        
               // Check for adjacent waypoints with the same address 
        
               if (previousWaypoint && currentWaypoint?.address === previousWaypoint.address) { 
        
                   return acc; 
        
               } 
        
               const validatedWaypoints: WaypointCollection = {...acc, [`waypoint${reArrangeIndexes ? lastWaypointIndex + 1 : index}`]: currentWaypoint}; 
        
               lastWaypointIndex += 1; 
        
               return validatedWaypoints; 
        
           }, {});

The logic to update lastWaypointIndex in the end of function

App/src/libs/TransactionUtils.ts

Line 441 in 732d48e

lastWaypointIndex += 1;

Then if the condition here is true

App/src/libs/TransactionUtils.ts

Line 430 in 732d48e

if (!waypointHasValidAddress(currentWaypoint)) {

lastWaypointIndex will be not updated. It causes this bug

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

I suggest we should remove lastWaypointIndex and use index-1 instead

we also can move this line

App/src/libs/TransactionUtils.ts

Line 441 in 732d48e

lastWaypointIndex += 1;

before the condition

App/src/libs/TransactionUtils.ts

Line 430 in 732d48e

if (!waypointHasValidAddress(currentWaypoint)) {

What alternative solutions did you explore? (Optional)

We need to add a validation to check if the selected stop point is the previous stop point or not, if yes and there are more than 2, ignore the selected stop point like below:

 const isPreviousSelectedWaypoint = useCallback(
        (values) => {
            if (_.size(allWaypoints) > 0 && isEmpty(currentWaypoint)) {
                const lastWaypoint = lodashGet(allWaypoints, `waypoint${_.size(allWaypoints) - 2}`, {});
                return isEqual(lastWaypoint, values);
            } else if (!isEmpty(currentWaypoint)) {
                // Handle case edit waypoint
            }
            return false;
        },
        [allWaypoints],
    );

 const selectWaypoint = (values) => {
        if (!isPreviousSelectedWaypoint(values) || allWayPoints.length < 2) {
            saveWaypoint(waypoint);
        }
        Navigation.goBack(ROUTES.MONEY_REQUEST_DISTANCE_TAB.getRoute(iouType));
    };

Also rather than just ignoring the selected stop point, we can disable the user clicking on this selection, or display the error (notification) to indicate that this stop point was already selected before. The expected behavior can be discussed in the future

abekkala · 2023-10-02T16:18:12Z

@0xmiroslav can you please review the proposals that have come in? thanks!

neil-marcellini · 2023-10-05T21:36:05Z

We have this issue #27045 already which is going to handle this case so I'm going to close it.

rakshitjain13 · 2023-10-14T07:21:39Z

@neil-marcellini The above issue persists even after merging #27045. Can you please reopen this issue?

jo-ui · 2023-10-19T07:47:43Z

@neil-marcellini @0xmiroslav @abekkala this issue is still reproducible, can you reopen it

rakshitjain13 · 2023-10-19T07:52:43Z

@jo-ui #29895

0xmiros · 2023-10-19T07:55:25Z

#29895 is a dupe and we should reopen this one as it's older.

0xmiros · 2023-11-01T16:31:15Z

@jo-ui are you still able to reproduce this?

jo-ui · 2023-11-01T16:47:36Z

@0xmiroslav Yes it is still reproducible

0xmiros · 2023-11-03T08:43:16Z

@abekkala can we reopen this?

We have this issue #27045 already which is going to handle this case so I'm going to close it.

This case is not handled and bug still exists

tienifr · 2023-11-15T08:00:48Z

@abekkala gentle bump on #28477 (comment), thanks!

jo-ui · 2023-11-15T08:25:17Z

@abekkala @0xmiroslav the issue #29895 is dupe and closed because this issue is legit, can you reopen this issue and review please

rojiphil · 2023-11-21T12:17:38Z

Proposal

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

This proposal is brought from 29895 here.

We are able to create two consecutive duplicate waypoints when there are stops added to the waypoints. So, we are trying to solve the problem of avoiding duplicates here.

What is the root cause of that problem?

Currently, the logic supports consecutive duplicate waypoints when there are no stops added to the waypoints (i.e when only two waypoints are present). However, support for consecutive duplicate waypoints with additional stops in waypoints is not there.

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

In the logic of determining validatedWaypoints here, we already check if we have two consecutive duplicate waypoints and ignore such duplicates here. This means that the length of waypoints and validatedWaypoints will be different when there are duplicates. We can use this information to solve this problem.

First, let us ensure that on submitWaypoints here, we will prevent the user to go to the next page for two or more waypoints by comparing the lengths of waypoints and validatedWaypoints like this:

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

Now, since we already have hasError set, let us ensure that the rendering also happen by replacing our condition here like this:

         {((hasError && (_.size(validatedWaypoints) < 2) || (_.keys(waypoints).length > 2 && _.size(validatedWaypoints) !== _.keys(waypoints).length)) || hasRouteError) && (

Finally, we need to set the error message for duplicates here like this:

 if (_.keys(waypoints).length > 2 &&  _.size(validatedWaypoints) !== _.keys(waypoints).length) {
     return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
 }

0xmiros · 2023-11-21T12:22:15Z

@neil-marcellini can you please reopen this? You said #28477 (comment) but issue is still reproducible as #27045 didn't cover this.
cc: @abekkala

rushatgabhane · 2023-11-21T12:30:29Z

I think we should reopen the original issue here - #29895 (comment)

@rojiphil and I spent time on it

DylanDylann · 2023-11-21T13:07:01Z

Proposal

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

Users can go to the next page when entering two consecutive same endpoints

What is the root cause of that problem?

We don't have logic to prevent user from clicking next button when they enter two consecutive same endpoints

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

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 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))

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 29, 2023

melvin-bot bot changed the title ~~Distance - Same location is allowed when it is entered in "Stop"~~ [$500] Distance - Same location is allowed when it is entered in "Stop" Sep 29, 2023

melvin-bot bot assigned abekkala Sep 29, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 29, 2023

melvin-bot bot assigned 0xmiros Sep 29, 2023

melvin-bot bot added the Overdue label Oct 2, 2023

melvin-bot bot added Overdue and removed Overdue labels Oct 2, 2023

neil-marcellini closed this as not planned Won't fix, can't repro, duplicate, stale Oct 5, 2023

melvin-bot bot removed the Overdue label Oct 5, 2023

0xmiros mentioned this issue Oct 19, 2023

[HOLD for payment 2024-01-17] [$500] Distance - Able to request money by distance by adding a stop same as finish point without enterring start point From #29895

Closed

6 tasks

DylanDylann mentioned this issue Oct 26, 2023

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

Closed

6 tasks

paultsimura mentioned this issue Nov 21, 2023

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

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$500] Distance - Same location is allowed when it is entered in "Stop" #28477

[$500] Distance - Same location is allowed when it is entered in "Stop" #28477

kbecciv commented Sep 29, 2023 •

edited

Loading

namhihi237 commented Sep 29, 2023 •

edited

Loading

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

rakshitjain13 commented Sep 29, 2023 •

edited

Loading

tienifr commented Sep 30, 2023 •

edited

Loading

tewodrosGirmaA commented Sep 30, 2023

DylanDylann commented Oct 2, 2023 •

edited

Loading

abekkala commented Oct 2, 2023

neil-marcellini commented Oct 5, 2023

rakshitjain13 commented Oct 14, 2023 •

edited

Loading

jo-ui commented Oct 19, 2023

rakshitjain13 commented Oct 19, 2023

0xmiros commented Oct 19, 2023

0xmiros commented Nov 1, 2023

jo-ui commented Nov 1, 2023

0xmiros commented Nov 3, 2023

tienifr commented Nov 15, 2023

jo-ui commented Nov 15, 2023 •

edited

Loading

rojiphil commented Nov 21, 2023

0xmiros commented Nov 21, 2023

rushatgabhane commented Nov 21, 2023

DylanDylann commented Nov 21, 2023

[$500] Distance - Same location is allowed when it is entered in "Stop" #28477

[$500] Distance - Same location is allowed when it is entered in "Stop" #28477

Comments

kbecciv commented Sep 29, 2023 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

namhihi237 commented Sep 29, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

melvin-bot bot commented Sep 29, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 29, 2023

rakshitjain13 commented Sep 29, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

tienifr commented Sep 30, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

tewodrosGirmaA commented Sep 30, 2023

DylanDylann commented Oct 2, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

abekkala commented Oct 2, 2023

neil-marcellini commented Oct 5, 2023

rakshitjain13 commented Oct 14, 2023 • edited Loading

jo-ui commented Oct 19, 2023

rakshitjain13 commented Oct 19, 2023

0xmiros commented Oct 19, 2023

0xmiros commented Nov 1, 2023

jo-ui commented Nov 1, 2023

0xmiros commented Nov 3, 2023

tienifr commented Nov 15, 2023

jo-ui commented Nov 15, 2023 • edited Loading

rojiphil commented Nov 21, 2023

Proposal

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

What is the root cause of that problem?

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

0xmiros commented Nov 21, 2023

rushatgabhane commented Nov 21, 2023

DylanDylann commented Nov 21, 2023

Proposal

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

What is the root cause of that problem?

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

kbecciv commented Sep 29, 2023 •

edited

Loading

namhihi237 commented Sep 29, 2023 •

edited

Loading

rakshitjain13 commented Sep 29, 2023 •

edited

Loading

tienifr commented Sep 30, 2023 •

edited

Loading

DylanDylann commented Oct 2, 2023 •

edited

Loading

rakshitjain13 commented Oct 14, 2023 •

edited

Loading

jo-ui commented Nov 15, 2023 •

edited

Loading