[Hold for Payment] [$500] Distance - For selected Invalid address, error displayed but showing previously selected waypoint

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

Pre-condition: In offline, create and save 2 vague locations (eg-WT)for making distance request
1.Go to https://staging.new.expensify.com/
2.Tap employee WS
3.Tap plus--request money-- distance
4.Enter start and finish point with any valid address
5.Tap stop point and enter invalid address
6.Note incorrect error
(Issues with map provider, try again)shown
7.Tap that waypoint and change to valid address
8. Again tap and select same address
9.Now select any invalid address

Expected Result:

When Invalid address is selected, the selected address and correct error message should be displayed.

Actual Result:

When Invalid address is selected, error is displayed but showing previously selected waypoint.

Incorrect error message - ( Issues with map provider, try again) is shown

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

Bug6262139_1699012200332.error.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0116f74087ff38a9d5
• Upwork Job ID: 1720412223318081536
• Last Price Increase: 2023-11-10
• Automatic offers:
• DylanDylann | Contributor | 27799860

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @NicMendonca (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 - @allroundexperts (External)

[tienifr]

Proposal

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

1. When Invalid address is selected, error is displayed but showing previously selected waypoint.

2. Incorrect error message - ( Issues with map provider, try again) is shown

What is the root cause of that problem?

1. In here, the values.name is undefined, so when we're trying to merge the values in Onyx, it will not override the existing value, so although the waypoint address is updated, its name is still of the previous selected waypoint

2. The error message comes from the back-end

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

1. When saving the waypoint, if any fields of the waypoint is null/undefined here, we need to fallback to null so it'll be reset in Onyx.
2. if we want to fix it, we need to fix in back-end

What alternative solutions did you explore? (Optional)

An alternative for 1 is to fix from the source so that it will return the field as null if it doesn't exist in the selected search result, rather than undefined (in AddressSearch's onPress). But I feel it's better to put right before we save to Onyx, since resetting a value by using null is an implementation specific to Onyx.

[FitseTLT]

Proposal

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

If we input an invalid address on a address field which previously had a valid address the name will still display the previous inserted valid address

What is the root cause of that problem?

When we select an invalid address Transaction.saveWaypoint is called here with waypoint.name value as undefined because it is not a valid address

App/src/pages/iou/WaypointEditor.js

Lines 163 to 172 in c0657fa

	const selectWaypoint = (values) => {
	const waypoint = {
	lat: values.lat,
	lng: values.lng,
	address: values.address,
	name: values.name,
	};
	Transaction.saveWaypoint(transactionID, waypointIndex, waypoint, isEditingWaypoint);

So in Transaction.saveWaypoint here below onyx merge is called with waypoint.name as undefined but that is understood by onyx that name prop is not to be merged or changed

App/src/libs/actions/Transaction.ts

Lines 59 to 73 in c0657fa

	function saveWaypoint(transactionID: string, index: string, waypoint: RecentWaypoint | null, isEditingWaypoint = false) {
	Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
	pendingFields: {
	comment: isEditingWaypoint ? CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE : CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
	},
	comment: {
	waypoints: {
	[`waypoint${index}`]: waypoint,
	},
	},
	// Empty out errors when we're saving a new waypoint as this indicates the user is updating their input
	errorFields: {
	route: null,
	},

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

Anything regarding the error display message should be fixed on backend.
But to fix the bug regarding address name we need to change

App/src/pages/iou/WaypointEditor.js

Lines 163 to 172 in c0657fa

	const selectWaypoint = (values) => {
	const waypoint = {
	lat: values.lat,
	lng: values.lng,
	address: values.address,
	name: values.name,
	};
	Transaction.saveWaypoint(transactionID, waypointIndex, waypoint, isEditingWaypoint);

to

 const selectWaypoint = (values) => {
        const waypoint = {
            lat: values.lat,
            lng: values.lng,
            address: values.address,
            name: values.name || null,
        };
        Transaction.saveWaypoint(transactionID, waypointIndex, waypoint, isEditingWaypoint);

so that in invalid addresses case the name field will be reset in onyx merge 👍
POC:

2023-11-03.16-26-58.mp4

What alternative solutions did you explore? (Optional)

[DylanDylann]

Proposal

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

We have 2 bugs here:
Bug 1. Select a waypoint (name field exists) then select another waypoint (name field doesn't exist) --> the old name still be displayed
Bug 2. Select a waypoint (name field exists) then go offline and update waypoint with any name --> the old name still be displayed

What is the root cause of that problem?

We don't remove old name value
Bug 1: In here

App/src/pages/iou/WaypointEditor.js

Lines 164 to 169 in aa30ef2

	const selectWaypoint = (values) => {
	const waypoint = {
	lat: values.lat,
	lng: values.lng,
	address: values.address,
	name: values.name,

Because values.name is undefined, when using MERGE method ONYX won't overwrite old waypoint.name

Bug 2: In here

App/src/pages/iou/WaypointEditor.js

Lines 146 to 150 in aa30ef2

	const waypoint = {
	lat: null,
	lng: null,
	address: waypointValue,
	};

We don't reset values.name

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

Bug 1: In here

App/src/pages/iou/WaypointEditor.js

Lines 164 to 169 in aa30ef2

	const selectWaypoint = (values) => {
	const waypoint = {
	lat: values.lat,
	lng: values.lng,
	address: values.address,
	name: values.name,

We should update like this name: values.name || null

Bug 2: In here

App/src/pages/iou/WaypointEditor.js

Lines 146 to 150 in aa30ef2

	const waypoint = {
	lat: null,
	lng: null,
	address: waypointValue,
	};

We should add name: waypointValue or name: null

What alternative solutions did you explore? (Optional)

We can consider combine old data and update data and then using SET method instead of MERGE method

[melvin-bot]

@NicMendonca, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

[NicMendonca]

@allroundexperts proposals for you! ^

[melvin-bot]

@NicMendonca, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@NicMendonca, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[allroundexperts]

Thanks for the proposals everyone. I think we should go with @DylanDylann's proposal since it covers extra cases which everyone else missed.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[DylanDylann]

@Beamanator Could you help to review my proposal? Thanks so much

[melvin-bot]

@Beamanator, @NicMendonca, @allroundexperts 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[Beamanator]

I'm officially back from vacayyyy! reviewing now

[melvin-bot]

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($500)

[melvin-bot]

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

[melvin-bot]

@Beamanator @NicMendonca @allroundexperts @DylanDylann this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[Beamanator]

@DylanDylann when do you think we can get a PR here?

[DylanDylann]

The PR will be ready in tomorrow

[DylanDylann]

@allroundexperts The PR is ready for reviewing

[melvin-bot]

This issue has not been updated in over 15 days. @Beamanator, @NicMendonca, @allroundexperts, @DylanDylann eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[DylanDylann]

@NicMendonca The PR is merged and deployed to production, could you help to move forward with this issue?

[DylanDylann]

@NicMendonca Bump again

[NicMendonca]

omg, so sorry, I don't know why this didn't bump to a daily

[NicMendonca]

BZ payment summary:

Contributor: @DylanDylann - $500 - paid via Upwork
C+: @allroundexperts - $500 - paid via Expensify

[NicMendonca]

@DylanDylann you've been paid!

@allroundexperts don't forget to request payment via Expensify!

[JmillsExpensify]

$500 approved for @allroundexperts based on summary above.