[HOLD for payment 2024-10-17] [$250] Distance - Invalid waypoints aren't cleared when a request fails

[izarutskaya]

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

Action Performed:

1. Navigate to staging.new.expensify.com
2. Create a workspace
3. Go to workspace chat
4. Go offline
5. Go to submit distance expense
6. Enter an invalid waypoint and click save
7. Add another waypoint and continue to submit the expense
8. Go online
9. Verify the request fails
10. Green plus, submit an expense, distance
11. Go to enter a waypoint

Expected Result:

Invalid waypoints should be removed from suggestion list

Actual Result:

Invalid waypoints stay on the suggestion list

Workaround:

Submit a distance expense with valid waypoints, which will clear any invalid recent waypoints. Or. sign out and back in, since the invalid recent waypoints are only on the frontend.

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

Screen.Recording.2024-10-01.at.11.48.15.AM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021833644939720857104
• Upwork Job ID: 1833644939720857104
• Last Price Increase: 2024-09-10
• Automatic offers:
• suneox | Reviewer | 104234581
• wildan-m | Contributor | 104234583

Issue Owner

Current Issue Owner: @johncschuster

[melvin-bot]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #collect project.

[nyomanjyotisa]

Edited by proposal-police: This proposal was edited at 2024-09-05 11:08:41 UTC.

Proposal

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

Distance - Invalid waypoint stays on suggestion list

What is the root cause of that problem?

We don't filter out the invalid waypoint from recentWaypoints here

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

Filter out the invalid waypoint (the waypoint with lat and long equal to 0). Add the following filter to this selector

.filter((waypoint) => waypoint.lat !== 0 && waypoint.lng !== 0)

if we need to prevent selecting invalid waypoints when the network is offline we can change this and this to the following

        if (waypointValue !== '' && waypointAddress !== waypointValue) {
            ErrorUtils.addErrorMessage(errors, `waypoint${pageIndex}`, translate('distance.error.selectSuggestedAddress'));
        }

        hint={translate('distance.error.selectSuggestedAddress')}

What alternative solutions did you explore? (Optional)

Or we can just don't add the invalid waypoint to the recent waypoints here

if (!recentWaypointAlreadyExists && waypoint !== null && waypoint.lat !== 0 && waypoint.lng !== 0) {

[melvin-bot]

@johncschuster Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

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

[melvin-bot]

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

[suneox]

@nyomanjyotisa Thank you for your proposal, but we also need to prevent selecting invalid waypoints when the network is slow or offline.

[nkdengineer]

Proposal

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

Invalid waypoints stay on the suggestion list

What is the root cause of that problem?

Here we only check the waypoint is empty or not to decide to save this waypoint to recent list

App/src/libs/actions/Transaction.ts

Lines 127 to 130 in 1e8d80e

	if (!recentWaypointAlreadyExists && waypoint !== null) {
	const clonedWaypoints = lodashClone(recentWaypoints);
	clonedWaypoints.unshift(waypoint);
	Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, CONST.RECENT_WAYPOINTS_NUMBER));

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

We can use the waypointHasValidAddress function as we do here to check the waypoint is valid or not before saving it to recent list

if (!recentWaypointAlreadyExists && waypoint && TransactionUtils.waypointHasValidAddress(waypoint)) {
    const clonedWaypoints = lodashClone(recentWaypoints);
    clonedWaypoints.unshift(waypoint);
    Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, CONST.RECENT_WAYPOINTS_NUMBER));
}

App/src/libs/actions/Transaction.ts

Lines 127 to 130 in 1e8d80e

	if (!recentWaypointAlreadyExists && waypoint !== null) {
	const clonedWaypoints = lodashClone(recentWaypoints);
	clonedWaypoints.unshift(waypoint);
	Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, CONST.RECENT_WAYPOINTS_NUMBER));

What alternative solutions did you explore? (Optional)

[nyomanjyotisa]

Proposal updated

[suneox]

@johncschuster I’d like to confirm should we prevent selecting invalid waypoints in offline mode? (currently, we don’t validate waypoints in offline mode) to maintain consistency with the online flow.

RPReplay_Final1726056389-1.MP4

[trjExpensify]

but we also need to prevent selecting invalid waypoints when the network is slow or offline.

Why so? Don't we do that validation when you come back online? 🤔 CC: @neil-marcellini for vis on this issue.

[nkdengineer]

I think we should only prevent saving invalid waypoints to the recent list because if we prevent selecting invalid waypoint in offline, the user that doesn't have recent waypoints list can't create distance request in offline.

[suneox]

Why so? Don't we do that validation when you come back online?

@trjExpensify Currently, when submitting an expense with an invalid waypoint, the backend does not accept it. Therefore, I think we should also prevent selecting an invalid waypoint in offline mode to ensure consistency.

RPReplay_Final1726131672_2.mp4

[trjExpensify]

I think we should only prevent saving invalid waypoints to the recent list because if we prevent selecting invalid waypoint in offline, the user that doesn't have recent waypoints list can't create distance request in offline.

I agree. We shouldn't save invalid waypoints to recents, they're useless. But we shouldn't prevent selecting invalid waypoints, because we don't know if they are valid or not when you're offline, so then you couldn't create a distance request when offline.

[suneox]

But we shouldn't prevent selecting invalid waypoints, because we don't know if they are valid or not when you're offline, so then you couldn't create a distance request when offline.

Based on the expected result @nkdengineer proposal LGTM

Based on the expected result @nyomanjyotisa alternative proposal LGTM

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[nyomanjyotisa]

But we shouldn't prevent selecting invalid waypoints, because we don't know if they are valid or not when you're offline, so then you couldn't create a distance request when offline.

Based on the expected result @nkdengineer proposal LGTM

🎀 👀 🎀 C+ reviewed

I believe my alternative solution is works as the expected results

and the selected solution won't works since waypointHasValidAddress only checks for waypoint?.address and invalid waypoint has address data

App/src/libs/TransactionUtils/index.ts

Lines 610 to 612 in 00bc167

	function waypointHasValidAddress(waypoint: RecentWaypoint | Waypoint): boolean {
	return !!waypoint?.address?.trim();
	}

[suneox]

I’d like to clarify that the selected solution is to prevent saving an invalid waypoint, rather than filtering out the invalid waypoints in the list. The implementation details can be addressed during the code review process. The alternative solution proposed by @nyomanjyotisa is the same as @nkdengineer solution, which is to prevent saving an invalid waypoint. However, I selected @nkdengineer proposal because it was posted before @nyomanjyotisa notify an update.

But I just double-checked, and it turns out that the updated proposal (at 2024-09-11T03:14:11Z) was actually submitted before the new proposal (at 2024-09-11T03:14:32Z).

Sorry everyone, this was my mistake for not checking the detailed update times, so I’ve updated the selected proposal accordingly.

[wildan-m]

Proposal Updated

• Add new alternative solution based on this suggestion [HOLD for payment 2024-10-17] [$250] Distance - Invalid waypoints aren't cleared when a request fails #48631 (comment)

Instead of identify recentValidatedWaypoints using pending action, we can filter it using lat, lng and name

item => item.lat !== 0 && item.lng !== 0 && !isEmpty(item.name)

[suneox]

@neil-marcellini Based on the latest expected behavior, we can proceed with the updated alternative proposal from @wildan-m.

[neil-marcellini]

Ok sweet, I'm glad we've landed on a solution! I like @wildan-m's proposal too. I keep changing my mind, but I like the main solution with the pendingAction approach. Either way could work but that's slightly more clear, and I forgot in my last comment that we need a way to identify the current waypoints that have been saved by the server vs offline. Hiring 🚀

[melvin-bot]

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

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

[wildan-m]

@neil-marcellini Thanks for the assignment, please confirm that we'll go with adding pendingAction? If so, we'll need backend modification as mentioned here #48631 (comment) and explained more here #48631 (comment)

[melvin-bot]

@johncschuster @wildan-m @suneox @neil-marcellini this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[wildan-m]

@neil-marcellini bump #48631 (comment)

[wildan-m]

@suneox The PR is ready #50323. Thanks!

[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 9.0.47-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove invalid recent waypoint when creating / editing distance request #50323

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

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

• @wildan-m requires payment automatic offer (Contributor)
• @suneox requires payment automatic offer (Reviewer)

[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:

• [@suneox] The PR that introduced the bug has been identified. Link to the PR:
• [@suneox] 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:
• [@suneox] 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:
• [@suneox] Determine if we should create a regression test for this bug.
• [@suneox] 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.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[suneox]

Checklist

• [@suneox] The PR that introduced the bug has been identified. Link to the PR: N/A
• [@suneox] 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. There isn’t an offending PR in this case, as this is a new requirement intended to cover an edge case.​
• [@suneox] 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: N/A
• [@suneox] Determine if we should create a regression test for this bug: N/A
• [@suneox] 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. N/A

[johncschuster]

Payment Summary:

Contributor: @wildan-m paid $250 via Upwork
Contributor+: @suneox paid $250 via Upwork