Closed izarutskaya closed 2 weeks ago
melvin-bot[bot]
commented
2 months ago 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
commented
2 months ago We think this issue might be related to the #collect project.
nyomanjyotisa
commented
2 months ago Edited by proposal-police: This proposal was edited at 2024-09-05 11:08:41 UTC.
Distance - Invalid waypoint stays on suggestion list
We don't filter out the invalid waypoint from recentWaypoints here
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')}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) {@johncschuster Eep! 4 days overdue now. Issues have feelings too...
melvin-bot[bot]
commented
2 months ago Job added to Upwork: https://www.upwork.com/jobs/~021833644939720857104
melvin-bot[bot]
commented
2 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox (External)
suneox
commented
2 months ago @nyomanjyotisa Thank you for your proposal, but we also need to prevent selecting invalid waypoints when the network is slow or offline.
nkdengineer
commented
2 months ago Invalid waypoints stay on the suggestion list
Here we only check the waypoint is empty or not to decide to save this waypoint to recent list
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));
}@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.
https://github.com/user-attachments/assets/23dd6229-13a4-475d-9403-948f6f2e0e11
trjExpensify
commented
1 month ago 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
commented
1 month ago 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
commented
1 month ago 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.
https://github.com/user-attachments/assets/b2295098-95e8-4022-84a8-1d6fcf519a14
trjExpensify
commented
1 month ago 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
commented
1 month ago 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[bot]
commented
1 month ago Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
nyomanjyotisa
commented
1 month ago 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
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/TransactionUtils/index.ts#L610-L612
suneox
commented
1 month ago 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.
melvin-bot[bot]
commented
1 month ago @johncschuster, @suneox, @neil-marcellini Whoops! This issue is 2 days overdue. Let's get this updated quick!
suneox
commented
1 month ago We’re still waiting for the internal team to choose a proposal.
neil-marcellini
commented
1 month ago Hi, sorry for the delay. I reviewed all proposals and I would like to suggest something different.
The root cause of the issue, in my opinion, is that the optimistically saved recent waypoints are not updated when we receive an error from the backend that the distance request failed to create, because the waypoints are invalid.
Idk if the error is that specific, but if a distance request fails to create then we should remove the waypoints it contains from the recent waypoints. A potential problem is if the recent waypoints already contain 'validAddressA' and the failed distance expense contains waypoints 'validAddressA', 'invalidAddressB'. We wouldn't want to remove 'validAddressA' from the recent waypoints, in that case. Therefore we'll also need mark new waypoints in the recent waypoints list as pending to be added, and on failure only remove those that are pending addition to the recent waypoints list.
Does that sounds good to you all? The first person to update or post a good proposal following this plan will be assigned.
nkdengineer
commented
1 month ago @neil-marcellini But we will save the waypoint to the recent list when we select the waypoint and we can have a case that user can enter an invalid waypoint then edit to another before creating the distance request.
neil-marcellini
commented
1 month ago Ok, interesting point. I think that flow would work like this.
The user enters an invalid waypoint, it's saved to the recent waypoint list optimistically and marked as pending add. Then, the user edits it to a valid waypoint which is also saved to recent waypoints as pending add. The request succeeds and an Onyx update with method 'set' returns the proper recent waypoints, clearing the invalid one.
In the same case where an invalid waypoint is edited to a valid one, but where there is still at least one invalid waypoint, the failure data set up in the response should reset the recent waypoints list to one where all waypoints are not pending addition.
I think that should all work. Any more questions?
melvin-bot[bot]
commented
1 month ago @johncschuster @suneox @neil-marcellini this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
johncschuster
commented
1 month ago 
wildan-m
commented
1 month ago Edited by proposal-police: This proposal was edited at 2024-10-01 22:08:35 UTC.
Invalid waypoints remain on the suggestion list when creating a distance request.
We are not revert the recent waypoints if something error when creating distance request.
We can revert recentWaypoints to the latest validated value if error occurs.
getRecentWaypoints() can be from Transaction action or as createDistanceRequest param
src/libs/actions/IOU.ts
createDistanceRequest(
.....
const recentServerValidatedWaypoints = getRecentWaypoints().filter(item => !item.pendingAction)
onyxData?.failureData?.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.NVP_RECENT_WAYPOINTS}`,
value: recentServerValidatedWaypoints,
} )
API.write(WRITE_COMMANDS.CREATE_DISTANCE_REQUEST, parameters, onyxData);
.....Flag new waypoint src/libs/actions/Transaction.ts
function saveWaypoint(transactionID: string, index: string, waypoint: RecentWaypoint | null, isDraft = false) {
..........
const recentWaypointAlreadyExists = recentWaypoints.find((recentWaypoint) => recentWaypoint?.address === waypoint?.address);
if (!recentWaypointAlreadyExists && waypoint !== null) {
const clonedWaypoints = lodashClone(recentWaypoints);
waypoint.pendingAction = CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD;
clonedWaypoints.unshift(waypoint);
Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, CONST.RECENT_WAYPOINTS_NUMBER));I think we don't need to clear pendingField, since the server response will be SET if success, but let me know if that's required
Branch for this solution
Identify recent validated waypoints using lat lng and name attributes
createDistanceRequest(
.......
const recentServerValidatedWaypoints = getRecentWaypoints().filter(item => item.lat !== 0 && item.lng !== 0 && !isEmpty(item.name));
onyxData?.failureData?.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.NVP_RECENT_WAYPOINTS}`,
value: recentServerValidatedWaypoints,
})
API.write(WRITE_COMMANDS.CREATE_DISTANCE_REQUEST, parameters, onyxData);
....
do the same while editing the money request / track expense
updateMoneyRequestDistance
.............
const recentServerValidatedWaypoints = getRecentWaypoints().filter(item => item.lat !== 0 && item.lng !== 0 && !isEmpty(item.name));
onyxData?.failureData?.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.NVP_RECENT_WAYPOINTS}`,
value: recentServerValidatedWaypoints,
})
API.write(WRITE_COMMANDS.UPDATE_MONEY_REQUEST_DISTANCE, params, onyxData);@wildan-m Thank you for your proposal. We also need to handle the case for TrackExpense.
I think we don't need to clear pendingField, since the server response will be SET if success, but let me know if that's required
@neil-marcellini Additionally, we need to clear pendingField on the BE side because the response data still includes the pendingAction field, to prevent cases where a valid waypoint gets cleared
wildan-m
commented
1 month ago @wildan-m Thank you for your proposal. We also need to handle the case for TrackExpense.
@suneox it seems track expense also call request money. do you think we need different treatment for track expense?
Additionally, we need to clear pendingField because the response data still includes the pendingAction field, to prevent cases where a valid waypoint gets cleared
I didn't notice that, seems it's more appropriate to clear that in the server side (?)
suneox
commented
1 month ago @suneox it seems track expense also call request money. do you think we need different treatment for track expense?
@wildan-m createDistanceRequest and trackExpense are different cases.
seems it's more appropriate to clear that in the server side
Yep, we’re still waiting for the internal team’s feedback on that point.
neil-marcellini
commented
1 month ago @neil-marcellini Additionally, we need to clear
pendingFieldon the BE side because the response data still includes thependingActionfield, to prevent cases where a valid waypoint gets cleared
Sorry, I'm not quite following. Would you please list the steps taken to encounter this problem and show the response that is returned? You can also link to a draft PR if that's needed to test this. Regardless, I think that the frontend should generally handle the pendingAction / pendingFields data. If there's some reason why that's not possible please let me know why.
wildan-m
commented
1 month ago @neil-marcellini successData is initialized before receiving the server response. The nvp_expensify_recentWaypoints latitude and longitude data are set to 0 before the response is received. If we generate the success data from the front end, it will overwrite the server response with lat: 0, lon: 0.
also the nvp_expensify_recentWaypoints doesn't have stable index key like reportActions, the local and returned data length can be different, we can't rely on array sequence index.
wildan-m
commented
1 month ago createDistanceRequest and trackExpense are different cases.
@suneox sure, but only the saveWaypoint function modifies NVP_RECENT_WAYPOINTS and it is called in IOURequestStepWaypoint. The track expense feature also utilizes IOURequestStepWaypoint but with different parameters.
suneox
commented
1 month ago Sorry, I'm not quite following. Would you please list the steps taken to encounter this problem and show the response that is returned? You can also link to a draft PR if that's needed to test this. Regardless, I think that the frontend should generally handle the pendingAction / pendingFields data. If there's some reason why that's not possible please let me know why.
I'll prepare a test branch to explain the details ASAP
suneox
commented
1 month ago @neil-marcellini Currently, the data type for NVP_RECENT_WAYPOINTS is an array, but the Onyx library only supports replacing the entire array. Therefore, after receiving new waypoints from the server, we cannot remove pendingAction in the successful request case by onxyData.successData
Example:
request data = [
{
"keyForList": "waypoint_1727255981380",
"address": "130 nguyen chi thanh",
"name": "",
"lat": 0,
"lng": 0,
"pendingAction": "add"
},
...
];
response data = [
{
"keyForList": "130 Đ. Nguyễn Chí Thanh_1727072486310",
"address": "130 Đường Nguyễn Chí Thanh, Phường 3, District 10, Ho Chi Minh City, Vietnam",
"name": "130 Đ. Nguyễn Chí Thanh",
"lat": 10.7608132,
"lng": 106.6722783,
"pendingAction": "add"
},
...
]In that case, we need BE support to remove pendingAction from the response. If we don’t remove pendingAction, any subsequent failed request may result in the removal of valid waypoints.
response data = [
{
"keyForList": "130 Đ. Nguyễn Chí Thanh_1727072486310",
"address": "130 Đường Nguyễn Chí Thanh, Phường 3, District 10, Ho Chi Minh City, Vietnam",
"name": "130 Đ. Nguyễn Chí Thanh",
"lat": 10.7608132,
"lng": 106.6722783,
- "pendingAction": "add"
},
...
]If this pattern isn’t commonly supported by the backend, we can proceed with the previous solution, which is to only prevent saving an invalid waypoint. WDYT?
suneox
commented
1 month ago We’re still waiting on the internal team to decide the solution prevent adding an invalid waypoint to the list, or the solution manage the pendingAction on the client side based on the request status (in this case we need backend support base on this comment)
neil-marcellini
commented
1 month ago Ah ok, I'm looking at the original bug report again and I'm realizing that I misinterpreted it originally. Sorry! I now see that no request is sent to the backend at all in the original bug report.
I don't think the provided steps result in a real bug. We have "invalid" waypoints in the recent waypoints list after coming back online, but I don't think that's really an issue. It doesn't cause a problem directly. The problem only occurs later if a user tries to create an expense with those invalid waypoints. When that request fails they are not cleared from the recent waypoints list. I will update the description and testing video accordingly.
Maybe in a perfect world we would queue some kind of action that validates waypoints saved while offline, that aren't used in any request, when going back online; but I don't think that's worth it the effort of building such a mechanism. Instead we can make sure optimistic recent waypoints are cleaned up when the next request is made.
Oh wait, I see that when you go back online and try to use these waypoints they won't be allowed, so the request will have to be made while offline. We don't really have a problem with invalid waypoints from offline being used while online. They can still be used while offline, but they aren't necessarily "invalid" at that point, we don't know whether they are valid or not because we are offline. As soon as any distance expense in successfully created, the invalid waypoints will be removed from Onyx.
Here's video proof:
https://github.com/user-attachments/assets/c709e195-6d4c-4999-a4ce-448b6add7242
So I think the only problem that remains is cleaning up these waypoints when the request fails. The case you mentioned of saving invalid waypoints, then switching to use valid ones is not a problem. We only have a problem when creating requests with invalid waypoints. Furthermore, I don't think we'll need to set any pendingAction data afterall.
Bug:
Pseudocode:
When creating the request onyxData {
In the optimistic data {
No changes needed, recent waypoints are already saved before this
}
In the success data {
No changes are needed. The backend will return an Onyx.set update with the proper recent waypoints
}
Failure data {
// The goal is to properly reset the recent waypoints
Create an Onyx.set update with the recent waypoints currently stored in Onyx
}
}Please let me know what you all think of this approach 🙇
wildan-m
commented
1 month ago 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)
@neil-marcellini Based on the latest expected behavior, we can proceed with the updated alternative proposal from @wildan-m.
neil-marcellini
commented
1 month ago 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[bot]
commented
1 month ago 📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
melvin-bot[bot]
commented
1 month ago 📣 @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
commented
1 month ago @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 https://github.com/Expensify/App/issues/48631#issuecomment-2367639225 and explained more here https://github.com/Expensify/App/issues/48631#issuecomment-2373623014
melvin-bot[bot]
commented
1 month ago @johncschuster @wildan-m @suneox @neil-marcellini this issue is now 4 weeks old, please consider:
Thanks!
wildan-m
commented
1 month ago @neil-marcellini bump https://github.com/Expensify/App/issues/48631#issuecomment-2390374379
wildan-m
commented
1 month ago @suneox The PR is ready https://github.com/Expensify/App/pull/50323. Thanks!
melvin-bot[bot]
commented
1 month ago Reviewing label has been removed, please complete the "BugZero Checklist".
melvin-bot[bot]
commented
1 month ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.47-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-10-17. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
1 month ago 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
commented
3 weeks ago johncschuster
commented
2 weeks ago
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: 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): testpayment935@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team
Action Performed:
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?
Screenshots/Videos
https://github.com/user-attachments/assets/c514b4fe-13e5-4dfa-8216-662571941710
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @johncschuster