[HOLD for payment 2024-03-29] [MEDIUM] Distance - Unable to save manually entered address after selecting address from list

[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: 1.4.46-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/4355973
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:

Precondition:

• There are a few addresses in recent destination list

1. Go to staging.new.expensify.com
2. Go offline
3. Go to workspace chat
4. Open distance request page
5. Click Start
6. Select an address from the recent list and save it
7. Click Start again
8. Manually enter an address
9. Save it

Expected Result:

The new address is saved

Actual Result:

The new address is not saved

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

Bug6398757_1709324381934.bandicam_2024-03-02_01-20-57-341.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011597298ea5827798
• Upwork Job ID: 1763727574947143680
• Last Price Increase: 2024-03-02

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #wave5
CC @dylanexpensify

[lanitochka17]

@greg-schroeder FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[gijoe0295]

Proposal

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

Distance - Unable to save manually entered address after selecting address from list

What is the root cause of that problem?

We used waypoint${index} as inputID:

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

Line 210 in f92b845

inputID={`waypoint${pageIndex}`}

but we retrieved using values.name:

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

Line 128 in f92b845

name: values.name,

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

Change values.name to

values[`waypoint${pageIndex}`]

or name: waypointValue. I think in offline mode, the input address is also its name.

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

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

[melvin-bot]

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

[abzokhattab]

Proposal

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

Distance - Unable to save manually entered address after selecting address from list

What is the root cause of that problem?

When saving a defined waypoint for the first time, we save an object contains the address and the name to Onyx using the saveWaypoint function. This function merges the old transaction with the new object. The problem arises when we enter an unsaved address while offline because unsaved addresses have the address name as undefined, which causes the old name to remain and not be overwritten in the new selection

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

When selecting a waypoint in offline mode, we need to assign an empty string as the fallback value, to ensure that Onyx overrides the old name when it merges with the new object.

so we need to change this:

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

Line 128 in f92b845

name: values.name,

to

                name: values.name ?? '',

and do the same here
Also we can do the same with the lat, long and the address to make sure that the new object overrides the old one

[bernhardoj]

Proposal

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

Can't save an address while offline if there is already a previously selected address while online.

What is the root cause of that problem?

The distance start and stop list shows either the waypoint name or address.

App/src/components/DistanceRequest/DistanceRequestRenderItem.tsx

Line 54 in af68510

const title = waypoint.name || waypoint.address;

When we first select a suggested waypoint while online, we save both of the waypoint name and address.

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

Lines 143 to 151 in af68510

	const selectWaypoint = (values: Waypoint) => {
	const waypoint = {
	lat: values.lat,
	lng: values.lng,
	address: values.address,
	name: values.name,
	};
	Transaction.saveWaypoint(transactionID, pageIndex, waypoint, action === CONST.IOU.ACTION.CREATE);

While offline, we also try to save both waypoint name and address.

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

Lines 116 to 131 in af68510

	const submit = (values: FormOnyxValues<'waypointForm'>) => {
	const waypointValue = values[`waypoint${pageIndex}`] ?? '';
	// Allows letting you set a waypoint to an empty value
	if (waypointValue === '') {
	Transaction.removeWaypoint(transaction, pageIndex, true);
	}
	// While the user is offline, the auto-complete address search will not work
	// Therefore, we're going to save the waypoint as just the address, and the lat/long will be filled in on the backend
	if (isOffline && waypointValue) {
	const waypoint = {
	address: waypointValue,
	name: values.name,
	};
	saveWaypoint(waypoint);
	}

However, the waypoint name is always undefined because values is an object that contains the form data. The form data of the waypoint has only one input, which is the waypoint address. So, the previous waypoint address is overwritten, but not with the name.

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

While offline, we will never be able to get the address name, so to overwrite the previous waypoint name, we can just set null to the name.

name: null,

this was previously fixed in #33692

[5695mp]

● Use State to Manage Address:

➢Define a state variable to store the address, let's call it address.
Use the useState hook to manage the address state.

➢Update Address when Manually Entered:
Update the address state when the user manually enters an address.

➢Select Address from List:
When the user selects an address from the list, update the address state with the selected address.

➢Save the Address:
Provide a button or trigger to save the address.
When the save button is pressed, save the address state to your database or storage.

[melvin-bot]

📣 @5695mp! 📣
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>

[Tony-MK]

Proposal

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

Distance - Unable to save manually entered address after selecting an address from the list

What is the root cause of that problem?

In the saveWaypoint function, the onyx only merges the waypoint.address while the previous data of the waypoint persists.

App/src/libs/actions/Transaction.ts

Lines 64 to 67 in af68510

	Onyx.merge(`${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
	comment: {
	waypoints: {
	[`waypoint${index}`]: waypoint,

Therefore, when the user submits the new waypoint, lng, lat, and address will not be nullified but be reused.

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

Lines 123 to 130 in f92b845

	// While the user is offline, the auto-complete address search will not work
	// Therefore, we're going to save the waypoint as just the address, and the lat/long will be filled in on the backend
	if (isOffline && waypointValue) {
	const waypoint = {
	address: waypointValue,
	name: values.name,
	};
	saveWaypoint(waypoint);

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

In the saveWaypoint function, check if lat, lng and name are present in the waypoint to nullify from the transaction's onyx waypoints.

Create a condition after this line as illustrated.

[`waypoint${index}`]: {
  address: waypoint?.address ?? '',
  name: waypoint?.name ?? '',
  lat: waypoint?.lng ?? null,
  lng: waypoint?.lng ?? null,
}

[parasharrajat]

@abzokhattab's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[Tony-MK]

Hi @parasharrajat, I believe we need remove lat and lng because the waypoint will then get added to recentWaypointAlreadyExists if the condition illustrated below is not satistied.

App/src/libs/actions/Transaction.ts

Lines 90 to 94 in 22cb01c

	// You can save offline waypoints without verifying the address (we will geocode it on the backend)
	// We're going to prevent saving those addresses in the recent waypoints though since they could be invalid addresses
	// However, in the backend once we verify the address, we will save the waypoint in the recent waypoints NVP
	if (!lodashHas(waypoint, 'lat') || !lodashHas(waypoint, 'lng')) {
	return;

Therefore, if the address is invalid and the GPS coordiantes are present then it will lead to a regression.
Where the invalid address will be suggested as a recent waypoint when editing the request.

[parasharrajat]

How come the invalid address has GPS coordinates? Does this happen currently in the code?

[abzokhattab]

I see that the invalid address is already not returned as a recent address when online or offline

Screen.Recording.2024-03-04.at.8.21.40.PM.mov

[Tony-MK]

If the address is invalid, the waypoint will still be valid because the GPS coordinates from the previous valid waypoint are sent as a part of the waypoint in the updateMoneyRequestDistance request parameters.

Since the backend expects the lat and lng to be valid and can not verify the address offline, we have to remove the lat and lng for the backend to instead consider the waypoint as new and unverified.

Notice that the GPS coordinates from the merged modifiedWaypoints are identical to the initial waypoints

1) Initial waypoints before editing waypoint0 offline

2) Merged modifiedWaypoints after going online

However, removing lat and lng will prevent an invalid address from being considered a valid waypoint.

[amyevans]

@abzokhattab what's a reasonable ETA to expect a PR?

[abzokhattab]

Should be ready today… thanks amy

[abzokhattab]

Hi team, i just have a small issue with the Upwork offer, the offer you sent cannot be accepted, I get this error:

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

• Fix: Unable to save manually entered address after selecting address while offline #38026

If no regressions arise, payment will be issued on 2024-03-29. 🎊

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

• @parasharrajat requires payment through NewDot Manual Requests
• @abzokhattab requires payment (Needs manual offer from BZ)

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

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

[greg-schroeder]

Payment pending, not overdue

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: [TS migration] Migrate 'IOUWaypoint' page to TypeScript #33244
• [@parasharrajat] 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: https://github.com/Expensify/App/pull/33244/files#r1537264009
• [@parasharrajat] 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: Not needed
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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.

Regression Test Steps

Precondition:

• There are a few addresses in the recent destination list

1. Go to staging.new.expensify.com
2. Go offline
3. Go to workspace chat
4. Open distance request page
5. Click Start
6. Select an address from the recent list and save it
7. Click Start again
8. Manually enter an address
9. Save it.
10. Manually entered address should replace the previous saved waypoint.

Do you agree 👍 or 👎 ?

[greg-schroeder]

Processing

[greg-schroeder]

Offer sent @abzokhattab

[greg-schroeder]

@abzokhattab - C - $500
@parasharrajat - C+ - $500 (you can make a ND manual request)

[abzokhattab]

Thank you ... just accepted the offer

[greg-schroeder]

filed regression test, paying, closing

[parasharrajat]

Payment requested as per #37640 (comment)

[JmillsExpensify]

$500 approved for @parasharrajat