[HOLD issue #34615] Display waypoint error in confirm page and detail page #30621

dukenv0307 · 2023-10-31T06:35:45Z

Details

Display waypoint error in confirm page and detail page

Fixed Issues

$ #29783
PROPOSAL: #29783 (comment)

Tests

Create a distance request with the valid waypoints
Open the distance detail page
Go offline
Click on distance to go to edit page
Edit or add a new waypoint with an invalid address
Save the distance and go online
Verify that the error appears on the detail page

Verify that no errors appear in the JS console

Offline tests

QA Steps

Create a distance request with the valid waypoints
Open the distance detail page
Go offline
Click on distance to go to edit page
Edit or add a new waypoint with an invalid address
Save the distance and go online
Verify that the error appears on the detail page

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android-1.mov

Android: mWeb Chrome

chrome.mov

iOS: Native

ios.mov

iOS: mWeb Safari

safari.mov

MacOS: Chrome / Safari

web-1.mov

MacOS: Desktop

desktop-1.mov

melvin-bot · 2023-10-31T07:29:49Z

@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

neil-marcellini

Hey there 👋, I'm jumping in to give an early review.

src/components/ReportActionItem/MoneyRequestView.js

src/libs/ReportUtils.js

neil-marcellini · 2023-10-31T18:49:59Z

src/libs/ReportUtils.js

+ * @returns {Boolean}
+ */
+function hasDistanceTransactionRouteErrors(iouReportID) {
+    const distanceTransactions = getDistanceTransactions(iouReportID);


It would be more efficient to check if it's a distance transaction within the some callback, that way we don't have to loop through all transactions once we find one with a route error. Also I think we can just check if it has a route error since only distance transactions can.

This is the same way that we do in hasMissingSmartscanFields function so I think that's fine.

Let's update both functions to be more performant. I don't see why we should purposefully do more work than necessary.

We can just do

return _.some(allTransactions, (transaction) => TransactionUtils.hasRouteError(transaction));

src/libs/OptionsListUtils.js

neil-marcellini · 2023-10-31T18:52:28Z

src/languages/en.ts

@@ -592,6 +592,7 @@ export default {
            genericDeleteFailureMessage: 'Unexpected error deleting the money request, please try again later',
            genericEditFailureMessage: 'Unexpected error editing the money request, please try again later',
            genericSmartscanFailureMessage: 'Transaction is missing fields',
+            genericWaypointFailureMessage: 'Distance request has route error',


Did you get a copy check for this?

Thanks for asking for a copy check on the issue 🙂

neil-marcellini

Looking good thanks for the updates. I still want to see a more efficient check for the route error, also waiting on the copy check.

neil-marcellini

Thank you for the updates! There are still some things to patch up.

src/libs/ReportUtils.js

neil-marcellini · 2023-11-02T19:42:38Z

The copy check is done and it sounds like we can use an error message that already exists in our translation files.

dukenv0307 · 2023-11-03T03:34:57Z

Thanks for your feedback, I updated.

dukenv0307 · 2023-11-03T11:04:26Z

@neil-marcellini We don't have a translation like this. And genericWaypointFailureMessage is added for the error of IOU report which has many transactions. That is the reason I added a general message like Distance request has route error.

neil-marcellini

Your latest updates look good thank you. I tested and I don't see where we display genericWaypointFailureMessage. Do you know if we display it at all? If not we could use the error message from the route directly, in order to check if the report has any errors.

Instead of Please select a valid waypoint, the error message should be Please enter a valid address. That's very close to what was suggested for the copy and we already have a translation key for it.

Finally, I encountered a bug when editing a previously created distance request and adding an invalid address for a stop.

Screen.Recording.2023-11-03.at.11.36.42.AM.mov

dukenv0307 · 2023-11-06T05:20:52Z

Instead of Please select a valid waypoint, the error message should be Please enter a valid address. That's very close to what was suggested for the copy and we already have a translation key for it.

I updated.

Finally, I encountered a bug when editing a previously created distance request and adding an invalid address for a stop.

I also saw this bug. I think this came from AddressSearch or Google Autocomplete lib so I think it should be fixed as a separate issue.

neil-marcellini

We still need to update the error message where it's displayed to use the suggested copy text.

src/libs/OptionsListUtils.js

neil-marcellini · 2023-11-06T20:04:40Z

src/components/ReportActionItem/MoneyRequestView.js

@@ -211,6 +218,8 @@ function MoneyRequestView({report, betas, parentReport, policyCategories, should
                            shouldShowRightIcon={canEdit && !isSettled}
                            titleStyle={styles.flex1}
                            onPress={() => Navigation.navigate(ROUTES.EDIT_REQUEST.getRoute(report.reportID, CONST.EDIT_REQUEST_FIELD.DISTANCE))}
+                            brickRoadIndicator={hasErrors && hasRouteError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : ''}
+                            error={hasErrors && hasRouteError ? lodashValues(ErrorUtils.getLatestErrorField(transaction, 'route'))[0] : ''}


Here we should show Please enter a valid address

This display for special request so I think we should display correctly the route error that is returned from BE. We also display this in MoneyRequestConfirmList.

neil-marcellini · 2023-11-07T17:51:24Z

@dukenv0307 the text shown for the error simply needs to be changed.

Problem

The error message under the distance row is Please select a valid waypoint

Solution

The error message under the distance row should be Please enter a valid address

dukenv0307 · 2023-11-08T09:09:34Z

@neil-marcellini Updated.

neil-marcellini

Looks good but there are lint errors @dukenv0307

dukenv0307 · 2023-12-08T10:25:15Z

@situchan #30837 was merged. Please help to review again.

neil-marcellini

Looks good, pending C+ testing

situchan · 2023-12-14T23:17:59Z

will complete review by tomorrow

situchan · 2023-12-18T15:09:43Z

@dukenv0307 please merge main

dukenv0307 · 2023-12-19T04:59:45Z

@situchan Updated.

situchan · 2023-12-19T05:00:54Z

As logic was moved, can you please retest? One platform test is fine

dukenv0307 · 2023-12-19T05:59:24Z

@situchan What do you mean, I moved the logic from old file to the current confirm page now and tested locally.

situchan · 2023-12-19T06:00:20Z

@situchan What do you mean, I moved the logic from old file to the current confirm page now and tested locally.

yeah I meant that. Logic was moved from old file to new file

dukenv0307 · 2023-12-19T06:08:38Z

can you please retest?

@situchan I tested it locally.

situchan · 2023-12-19T13:12:52Z

@dukenv0307 After online, address is reverted back to valid ones and still shows error

Full repro video:

Screen.Recording.2023-12-19.at.7.04.56.PM.mov

situchan · 2023-12-19T13:14:23Z

Another bug: Edit distance was closed automatically and Distance field changes to Merchant (seems like report was switched to another one)

Screen.Recording.2023-12-19.at.7.10.01.PM.mov

dukenv0307 · 2023-12-20T08:00:41Z

After online, address is reverted back to valid ones and still shows error

@situchan This error is returned from BE since we update waypoint with invalid address.

dukenv0307 · 2023-12-20T08:04:39Z

Another bug: Edit distance was closed automatically and Distance field changes to Merchant (seems like report was switched to another one)

@situchan This is out of scope of this issue, because when we offline we generate another transaction thread reportID and with invalid address, when we go back from online this transaction is invalid and it's not stored from BE.

situchan · 2023-12-22T17:56:03Z

@neil-marcellini what do you think above issues?
I agree out of scope but should we proceed without holding for those fixes?

neil-marcellini · 2023-12-26T19:44:04Z

@dukenv0307 After online, address is reverted back to valid ones and still shows error
Full repro video:

Screen.Recording.2023-12-19.at.7.04.56.PM.mov

This is a bug, but it's related to the issue for handling offline geocoding errors in general. [DISTANCE] LOW: Handle errors with geocoding for offline distance requests. We had a pre-design in Slack a while ago about the desired UX, although it was focused on the creation flow. I think we should follow that plan here too, and we wouldn't get any benefit from merging this until it's implemented as we want it. I hope you're ok with working on this more @dukenv0307.

For this flow here's the desired behavior.

Go offline
View a distance request
Click the distance field
Edit 1 or more waypoints to be invalid
Click save
Expect dependent fields to be pending / TBD
Go online
Dependent fields are still TBD but not pending
The distance transaction chat shows in the LHN with an error indicator
There is a red dot indicator on the distance field
Click the distance field
Error messages are displayed under each invalid waypoint, instructing the user to edit them
When a waypoint is clicked the user can update the address and must use the suggestions which are now shown since they are online
When a new address is saved we clear any errors on the waypoint
Click to save the waypoint edits
Verify that an error message appears instructing the user to edit all invalid waypoints
Do that
Click to save the waypoint edits
Verify that all dependent fields update
Verify all error messages and indicators are cleared

neil-marcellini · 2023-12-26T19:51:25Z

@dukenv0307 @situchan Thanks for your hard work on this. I know we found some bugs that feel a bit out of scope, but I think we're better off fixing them all at once vs one at a time. That way it's more clear what the desired UX is. Let's use this as our tracking issue [DISTANCE] LOW: Handle errors with geocoding for offline distance requests. I know we'll need some backend changes too, and that issue is currently un-assigned. I hope to pick it up some time after vacation (after January 15).

We could add on to this PR and use it to implement the frontend changes, or just close this and start from scratch later. I would recommend the former however. You might be able to recruit another backend engineer in #wave5-free-submitters to help while I'm out.

stitesExpensify · 2024-01-09T00:12:56Z

@dukenv0307 @situchan do you know what backend changes @neil-marcellini is referring to? I'd be happy to take them on

dukenv0307 · 2024-02-20T03:58:18Z

I will re-work on this after money request flow is refactored completely. cc @neil-marcellini @situchan.

neil-marcellini · 2024-02-26T18:58:07Z

This has been handing around for a while, and the backend updates haven't been picked up yet, so going to close this. You can always reopen later.

dukenv0307 added 3 commits October 31, 2023 12:17

display waypoint error in confirm page and detail page

3c33874

get string error for distance request

3015ad8

display red dot in LHN if distance request has error

d1bc1d4

dukenv0307 marked this pull request as ready for review October 31, 2023 07:29

dukenv0307 requested a review from a team as a code owner October 31, 2023 07:29

melvin-bot bot removed the request for review from a team October 31, 2023 07:29

melvin-bot bot requested a review from aimane-chnaif October 31, 2023 07:29

neil-marcellini requested changes Oct 31, 2023

View reviewed changes

dukenv0307 added 2 commits November 1, 2023 10:33

Merge branch 'main' into fix/29783

afe1cef

rename function

bc084f7

neil-marcellini requested changes Nov 1, 2023

View reviewed changes

dukenv0307 added 2 commits November 2, 2023 11:43

Merge branch 'main' into fix/29783

38fed63

update funtion to be more performant

b0b2b57

neil-marcellini requested changes Nov 2, 2023

View reviewed changes

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

src/libs/ReportUtils.js Outdated Show resolved Hide resolved

dukenv0307 added 2 commits November 3, 2023 10:25

Merge branch 'main' into fix/29783

377f773

remove un-use function

51c2423

neil-marcellini requested changes Nov 3, 2023

View reviewed changes

dukenv0307 added 2 commits November 6, 2023 12:16

Merge branch 'main' into fix/29783

fa86169

fix translation

c361a34

neil-marcellini requested changes Nov 6, 2023

View reviewed changes

edit the error

5eea454

Merge branch 'main' into fix/29783

79d2392

neil-marcellini previously approved these changes Nov 8, 2023

View reviewed changes

dukenv0307 added 3 commits December 8, 2023 17:27

fix ts

0379ab3

fix ts

1784122

not check offline in submit function

6ed18c5

neil-marcellini changed the title ~~[HOLD 30837] Display waypoint error in confirm page and detail page~~ Display waypoint error in confirm page and detail page Dec 8, 2023

merge main

ca91b67

neil-marcellini previously approved these changes Dec 13, 2023

View reviewed changes

dukenv0307 added 2 commits December 19, 2023 11:39

Merge branch 'main' into fix/29783

015b990

merge main and copy logic to new confirm page

6adbf81

dukenv0307 dismissed neil-marcellini’s stale review via 6adbf81 December 19, 2023 04:59

neil-marcellini marked this pull request as draft December 27, 2023 17:45

dukenv0307 changed the title ~~Display waypoint error in confirm page and detail page~~ [HOLD issue #34615] Display waypoint error in confirm page and detail page Feb 26, 2024

neil-marcellini closed this Feb 26, 2024

[HOLD issue #34615] Display waypoint error in confirm page and detail page #30621

[HOLD issue #34615] Display waypoint error in confirm page and detail page #30621

Conversation

dukenv0307 commented Oct 31, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Oct 31, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Oct 31, 2023

Choose a reason for hiding this comment

dukenv0307 Nov 1, 2023

Choose a reason for hiding this comment

neil-marcellini Nov 1, 2023

Choose a reason for hiding this comment

neil-marcellini Oct 31, 2023

Choose a reason for hiding this comment

neil-marcellini Nov 1, 2023

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini commented Nov 2, 2023

dukenv0307 commented Nov 3, 2023

dukenv0307 commented Nov 3, 2023 • edited Loading

neil-marcellini left a comment

Choose a reason for hiding this comment

dukenv0307 commented Nov 6, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

neil-marcellini Nov 6, 2023

Choose a reason for hiding this comment

dukenv0307 Nov 7, 2023

Choose a reason for hiding this comment

neil-marcellini commented Nov 7, 2023

Problem

Solution

dukenv0307 commented Nov 8, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

dukenv0307 commented Dec 8, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

situchan commented Dec 14, 2023

situchan commented Dec 18, 2023

dukenv0307 commented Dec 19, 2023

situchan commented Dec 19, 2023

dukenv0307 commented Dec 19, 2023 • edited Loading

situchan commented Dec 19, 2023

dukenv0307 commented Dec 19, 2023

situchan commented Dec 19, 2023

situchan commented Dec 19, 2023 • edited Loading

dukenv0307 commented Dec 20, 2023

dukenv0307 commented Dec 20, 2023

situchan commented Dec 22, 2023 • edited Loading

neil-marcellini commented Dec 26, 2023

neil-marcellini commented Dec 26, 2023

stitesExpensify commented Jan 9, 2024

dukenv0307 commented Feb 20, 2024 • edited Loading

neil-marcellini commented Feb 26, 2024

dukenv0307 commented Oct 31, 2023 •

edited

Loading

dukenv0307 commented Nov 3, 2023 •

edited

Loading

dukenv0307 commented Dec 19, 2023 •

edited

Loading

situchan commented Dec 19, 2023 •

edited

Loading

situchan commented Dec 22, 2023 •

edited

Loading

dukenv0307 commented Feb 20, 2024 •

edited

Loading