[$500] Distance - Inconsistency in error message when more than two same adddress is added

[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.85-0

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. Launch New Expensify app
2. Go to + > Request money > Distance
3. Enter the same address for both Start and Finish point
4. Tap Next
5. Note the error message is 'Please remove duplicate waypoints'
6. Tap Add stop and add the same address and save it
7. Tap Next

Expected Result:

The error message should still be 'Please remove duplicate waypoints'

Actual Result:

The error message now shows 'Please enter at least two waypoints'

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

Bug6240647_1697554305957.Screen_Recording_20231017_193302_New_Expensify__1_.1.mp4

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018cedadf58c08d896
• Upwork Job ID: 1714334639555403776
• Last Price Increase: 2023-10-17

Issue Owner

Current Issue Owner: @GabiHExpensify

[melvin-bot]

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

[melvin-bot]

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

[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 - @ntdiary (External)

[tgolen]

@BhuvaneshPatil this is related to the changes in #29125. Can you work on correcting this, please?

[BhuvaneshPatil]

Sure thing @tgolen.
What should be the error message here?

[kuttyhub]

Proposal

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

Inconsistent error message while adding duplicate waypoints.

What is the root cause of that problem?

The root cause for this problem is the logic used in the onError function that returns the error message, which checks duplication only the waypoints have no additional points init _.keys(waypoints).length === 2 .
which leads to hit to the final if conditions and returns the Please enter at least two waypoints.

App/src/components/DistanceRequest/index.js

Lines 148 to 162 in b494186

	const getError = () => {
	// Get route error if available else show the invalid number of waypoints error.
	if (hasRouteError) {
	return ErrorUtils.getLatestErrorField(transaction, 'route');
	}
	// Initially, both waypoints will be null, and if we give fallback value as empty string that will result in true condition, that's why different default values.
	if (_.keys(waypoints).length === 2 && lodashGet(waypoints, 'waypoint0.address', 'address1') === lodashGet(waypoints, 'waypoint1.address', 'address2')) {
	return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
	}
	if (_.size(validatedWaypoints) < 2) {
	return {0: translate('iou.error.emptyWaypointsErrorMessage')};
	}
	};

also, I noticed the error handling is not done properly and it only checks if it has any two valid points. That introduces bugs like allowing duplicate waypoints if they have any two valid points.

App/src/components/DistanceRequest/index.js

Lines 226 to 233 in b494186

	{/* Show error message if there is route error or there are less than 2 routes and user has tried submitting, */}
	{((hasError && _.size(validatedWaypoints) < 2) || hasRouteError) && (
	<DotIndicatorMessage
	style={[styles.mh4, styles.mv3]}
	messages={getError()}
	type="error"
	/>
	)}

App/src/components/DistanceRequest/index.js

Lines 185 to 192 in b494186

	const submitWaypoints = useCallback(() => {
	// If there is any error or loading state, don't let user go to next page.
	if (_.size(validatedWaypoints) < 2 || hasRouteError || isLoadingRoute || isLoading) {
	setHasError(true);
	return;
	}
	onSubmit(waypoints);
	}, [onSubmit, setHasError, hasRouteError, isLoadingRoute, isLoading, validatedWaypoints, waypoints]);

check the recordings for the bug.
https://github.com/Expensify/App/assets/71143839/37328a72-1da2-452f-937e-76dc2794db9d

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

to fix the error handling properly we can introduce the memo variable hasInValidWayPoints which tracks differences in original waypoints and validatedWaypoints which has the list of unique and valid waypoints.

const validatedWaypoints = useMemo(() => TransactionUtils.getValidWaypoints(waypoints), [waypoints]);

const hasInValidWayPoints = useMemo(
  () => _.size(validatedWaypoints) < _.keys(waypoints).length ,
  [waypoints, validatedWaypoints]
)

Then update the OnError functions logic:

 const getError = () => {
        // Get route error if available else show the invalid number of waypoints error.
        if (hasRouteError) {
            return ErrorUtils.getLatestErrorField(transaction, 'route');
        }
        const startAddress = lodashGet(waypoints, 'waypoint0.address', {})
        const finalAddress = lodashGet(waypoints, 'waypoint1.address', {})
        // check start and final waypoints is empty
        if (_.keys(waypoints).length === 2 && (_.isEmpty(startAddress) || _.isEmpty(finalAddress))) {
            return { 0: translate('iou.error.emptyWaypointsErrorMessage') };
        }

        // Checking Duplicates in waypoints from start to end
        if (hasInValidWayPoints) {
            return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
        }
    };

Then update the logic on submitWaypoints to prevent submission and set hasError to true.

 const submitWaypoints = useCallback(() => {
       // If there is any error or loading state, don't let user go to next page.
       if ( hasInValidWayPoints || hasRouteError || isLoadingRoute || isLoading) {
           setHasError(true);
           return;
       }
       onSubmit(waypoints);
   }, [onSubmit, setHasError, hasRouteError, isLoadingRoute, isLoading, waypoints, hasInValidWayPoints]);

Then update the logic on showing error in view.

  {/* Show error message if there is route error or there are less than 2 routes and use

r has tried submitting, */}
  {((hasError && hasInValidWayPoints) || hasRouteError) && (
      <DotIndicatorMessage
          style={[styles.mh4, styles.mv3]}
          messages={getError()}
          type="error"
      />
  )}

by using the hasInValidWayPoints value now we can properly update hasError state and it prevents submitting. demo here,
https://github.com/Expensify/App/assets/71143839/c5afa8d2-d60e-41cb-a029-9f4725056e06

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

📣 @kuttyhub! 📣
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>

[kuttyhub]

Contributor details
Your Expensify account email: nishanthsundaraj@outlook.com
Upwork Profile Link: https://www.upwork.com/freelancers/~018770a46267a23e10

[melvin-bot]

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

[dukenv0307]

Proposal

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

Inconsistency in error message when more than two same adddress is added

What is the root cause of that problem?

We only display the duplicate error when having two waypoints in the same

App/src/components/DistanceRequest/index.js

Lines 154 to 156 in 19ef1ca

	if (_.keys(waypoints).length === 2 && lodashGet(waypoints, 'waypoint0.address', 'address1') === lodashGet(waypoints, 'waypoint1.address', 'address2')) {
	return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
	}

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

We should check if the validatedWaypoints is less than the non-empty waypoints which means we have some duplicate waypoints, and we will display the duplicate error.

To do this we should:

1. Update getValidWaypoints function to make this work as expected.

let lastWaypointIndex = -1;
let waypointIndex = -1;

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

lastWaypointIndex = index;
waypointIndex += 1;

App/src/libs/TransactionUtils.ts

Lines 430 to 431 in 19ef1ca

	if (!waypointHasValidAddress(currentWaypoint)) {
	return acc;

2. In DistanceRequest, we can create a variable that is the number of none-empty waypoints from waypoints

const numberOfNoneEmptyWaypoints = _.values(waypoints).filter((value) => !_.isEmpty(value) && TransactionUtils.waypointHasValidAddress(value)).length;
const isDuplicatedWaypoints = numberOfNoneEmptyWaypoints > _.size(validatedWaypoints) && _.size(validatedWaypoints) === 1

And then change the condition here to

if (isDuplicatedWaypoints) {
    return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
}

App/src/components/DistanceRequest/index.js

Lines 154 to 156 in 19ef1ca

	if (_.keys(waypoints).length === 2 && lodashGet(waypoints, 'waypoint0.address', 'address1') === lodashGet(waypoints, 'waypoint1.address', 'address2')) {
	return {0: translate('iou.error.duplicateWaypointsErrorMessage')};
	}

What alternative solutions did you explore? (Optional)

NA

Result

Screencast.from.18-10-2023.11.35.53.webm

[tgolen]

I'm going to take this issue over from @puneetlath since I was the one working on this feature. @kuttyhub I like your proposal because I think it does a good job to clean up the error handling to be more consistent and easier to follow and maintain (especially the relationship between waypoints and validWaypoints).

In the future, it would be best to write your proposal with little to no code posted (so no one is tempted to review the actual code, which is what the PR process is for).

@ntdiary Are you OK with this proposal?

[ntdiary]

@ntdiary Are you OK with #29803 (comment)?

@tgolen, yeah, it looks good to me. : )

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[kuttyhub]

Thanks @tgolen for the advice, I will keep it my mind for future proposals. I have one doubt regards the sub bug I found which allows submission when you have any two valid waypoints even though its contains duplicates. Is that expected behaviour?

[dukenv0307]

@ntdiary @tgolen @kuttyhub 's proposal doesn't work for all cases. Let's see the video:

Screencast.from.18-10-2023.22.41.41.webm

The reason is hasInValidWayPoints is true, and now we have three waypoints.

We should get the none-empty waypoints to compare with the validatedWaypoints to make this works exactly.

Actually, validatedWaypoints is updated incorrectly because we are missing the increase lastWaypointIndex in a case , see my video for more details.

Screencast.from.18-10-2023.22.45.36.webm

My proposal can fix both cases above can you please check again.

[tgolen]

I have one doubt regards the sub bug I found which allows submission when you have any two valid waypoints even though its contains duplicates. Is that expected behaviour?

I believe this is expected and intentional. The reason is that the server will automatically remove any duplicate waypoints and simply ignore them, hopefully making it a little easier UX.

@dukenv0307 Thank you for showing the bug with @kuttyhub's proposal, and I'd like to give them a chance to address that bug. I've looked at your proposal again, and I still am not as fond of it because it feels to me like it solves this problem with the fewest absolute number of changes, but doesn't actually improve the code overall and instead leaves it in a state where the validation/submission is confusing and difficult to follow and maintain.

To be clear, the intention of the errors is the following.

1. Duplicate waypoints - This should only happen when the waypoints don't take anyone anywhere. If there are more than two waypoints and there are two adjacent addresses that are the same, the server will ignore the duplicate (makes it easy for the user).

• Invalid: A > A
• Invalid: A > A > A
• Valid: A > B > A
• Valid: A > B > B > A (the second B is ignored by the server)

2. Please enter two waypoints - This should only happen when there are less than two valid waypoints.

• Invalid: empty > empty
• Invalid: empty > A
• Valid: A > empty > B
• Valid: A > empty > B > A
• Should trigger the duplicate waypoint error: A > empty > A

It was kind of difficult to write this up, so I would actually welcome any proposals to simplify this even further. Maybe we should only have a single error case? Would that make it less confusing? Let's maybe consider some out-of-the-box solutions that would refactor this a bit.

[dukenv0307]

but doesn't actually improve the code overall and instead leaves it in a state where the validation/submission is confusing and difficult to follow and maintain.

We can create a variable hasDuplicateWayPoint to replace this condition (numberOfNoneEmptyWaypoints > _.size(validatedWaypoints) that can make the code easy to follow.

[tgolen]

What about a variable more like routeGoesSomewhere? 😃

[dukenv0307]

@tgolen In the duplicate case above A > A > B is valid or not?

[ntdiary]

Let's nix the word "waypoint", that's not really a word that people use colloquially.

Could you update this message to, "Please enter a different starting and ending address"?

Alternatively, we could write, "The start and finish addresses are the same. Please update one of them."

@GabiHExpensify, eh, precisely speaking, there is not necessarily a starting or finishing. Currently, we think that as long as there are at least two different addresses, the input is valid, which is more convenient for users to use. 😂
e.g.

[tgolen]

Yeah, sorry @GabiHExpensify! We had a hard time trying to figure out all the cases for different error conditions and we are really trying to simplify all possible cases with a single message. How about this?

Please enter at least two different addresses.

[GabiHExpensify]

That looks great, thanks @tgolen! Are we good to go on the Spanish translation, or does that still need to be double-checked?

[dukenv0307]

@GabiHExpensify Yes, please help to check the Spanish translation.

[GabiHExpensify]

Here's Spanish:

Por favor introduce al menos dos direcciones diferentes.

[tgolen]

Thanks for the translations, Gabi! This should be merged soon.

[rakshitjain13]

@dukenv0307 But I see getValidWaypoints has bug even now when start is empty but it is not creating any problem because of a recent change that add stop button will show only after start and end are there . But I think it is more of a workaround . For more details regarding the bug I am saying : #29895 (comment) cc: @tgolen .

[melvin-bot]

@tgolen, @ntdiary, @GabiHExpensify, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[ntdiary]

Not overdue, the related PR has been deployed

[melvin-bot]

@tgolen, @ntdiary, @GabiHExpensify, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[dukenv0307]

@tgolen @GabiHExpensify The PR was deployed in production one week ago. Is this issue ready for payment?

[ntdiary]

@tgolen @GabiHExpensify The PR was deployed in production one week ago. Is this issue ready for payment?

It seems that the bot stopped working earlier, causing this issue not to be changed to "Awaiting payment". 😂

cc @tgolen

[tgolen]

@GabiHExpensify Can you help with the payments on this one? It is past the regression period, but looks like the automation didn't work for some reason.

[tgolen]

bump @GabiHExpensify

[GabiHExpensify]

ah sorry i missed your comment last week @tgolen -- i don't know anything about the payments, i'm just on this GH for the copy. should there be someone from the contributor team who handles that?

[tgolen]

Oops, sorry about that! I'll get someone from the bugzero team.

[melvin-bot]

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

[tgolen]

@sonialiap Could you help us settle up some payments here? I think this is what is needed:

• @dukenv0307 wrote the PR so they need paid as a contributor
• @ntdiary reviewed and tested the PR so they need paid as a C+

[tgolen]

bump @sonialiap

[tgolen]

bump @sonialiap

[sonialiap]

@dukenv0307 contributor $500 - offer sent - paid ✔️
@ntdiary reviewer $500 - offer sent - paid ✔️

[ntdiary]

@dukenv0307 contributor $500 - offer sent @ntdiary reviewer $500 - offer sent

@sonialiap, thank you! Have accepted. :)

[sonialiap]

Everyone has been paid ✔️
Seems like we're good to close