Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: logic for waypoint validation #26591

Merged
merged 9 commits into from
Sep 4, 2023
+47 −26
Merged

fix: logic for waypoint validation #26591

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
109726e
fix: logic for waypoint validation
allroundexperts Sep 2, 2023
f7d7796
fix: lint issues
allroundexperts Sep 2, 2023
9b1e693
fix: add proper typings for waypoint validation
allroundexperts Sep 3, 2023
4bc3a23
fix: re-arrange indexes
allroundexperts Sep 3, 2023
2d34d98
lint errors
allroundexperts Sep 3, 2023
d6de602
fix: simplify the logic for waypoint validation
allroundexperts Sep 3, 2023
ac08f33
fix: disable button when valid waypoints are less than 2
allroundexperts Sep 3, 2023
4304851
merge with main
allroundexperts Sep 3, 2023
a8dda33
tiny refactors for waypoint logic
allroundexperts Sep 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/components/DistanceRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ function DistanceRequest({iou, iouType, report, transaction, mapboxAccessToken})
const previousWaypoints = usePrevious(waypoints);
const haveWaypointsChanged = !_.isEqual(previousWaypoints, waypoints);
const doesRouteExist = lodashHas(transaction, 'routes.route0.geometry.coordinates');
const shouldFetchRoute = (!doesRouteExist || haveWaypointsChanged) && !isLoadingRoute && TransactionUtils.validateWaypoints(waypoints);

const validatedWaypoints = TransactionUtils.getValidWaypoints(waypoints);
const shouldFetchRoute = (!doesRouteExist || haveWaypointsChanged) && !isLoadingRoute && _.keys(validatedWaypoints).length > 1;
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
const waypointMarkers = useMemo(
() =>
_.filter(
Expand Down Expand Up @@ -149,14 +149,13 @@ function DistanceRequest({iou, iouType, report, transaction, mapboxAccessToken})
const visibleAreaEnd = lodashGet(event, 'nativeEvent.contentOffset.y', 0) + scrollContainerHeight;
setShouldShowGradient(visibleAreaEnd < scrollContentHeight);
};

useEffect(() => {
if (isOffline || !shouldFetchRoute) {
return;
}

Transaction.getRoute(iou.transactionID, waypoints);
}, [shouldFetchRoute, iou.transactionID, waypoints, isOffline]);
Transaction.getRoute(iou.transactionID, validatedWaypoints);
}, [shouldFetchRoute, iou.transactionID, validatedWaypoints, isOffline]);

useEffect(updateGradientVisibility, [scrollContainerHeight, scrollContentHeight]);

Expand Down
49 changes: 27 additions & 22 deletions src/libs/TransactionUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,34 +268,39 @@ function getAllReportTransactions(reportID) {
}

/**
* Verifies that the provided waypoints are valid
* Filters the waypoints which are valid and returns those
* @param {Object} waypoints
* @returns {Boolean}
* @param {Boolean} reArrangeIndexes
* @returns {Object} validated waypoints
*/
function validateWaypoints(waypoints) {
function getValidWaypoints(waypoints, reArrangeIndexes = false) {
const waypointValues = _.values(waypoints);

// Ensure the number of waypoints is between 2 and 25
if (waypointValues.length < 2 || waypointValues.length > 25) {
return false;
}

for (let i = 0; i < waypointValues.length; i++) {
const currentWaypoint = waypointValues[i];
const previousWaypoint = waypointValues[i - 1];

// Check if the waypoint has a valid address
if (!currentWaypoint || !currentWaypoint.address || typeof currentWaypoint.address !== 'string' || currentWaypoint.address.trim() === '') {
return false;
}

// Check for adjacent waypoints with the same address
if (previousWaypoint && currentWaypoint.address === previousWaypoint.address) {
return false;
}
return {};
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
}

return true;
const validWaypoints = _.reduce(
waypointValues,
(acc, currentWaypoint, index) => {
const previousWaypoint = waypointValues[index - 1];
// Check if the waypoint has a valid address
if (!currentWaypoint || !currentWaypoint.address || typeof currentWaypoint.address !== 'string' || currentWaypoint.address.trim() === '') {
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
return acc;
}

// Check for adjacent waypoints with the same address
if (previousWaypoint && currentWaypoint.address === previousWaypoint.address) {
return acc;
}

const lastIndex = Math.max(..._.map(_.keys(acc), (key) => parseInt(key.replace('waypoint', ''), 10)));
return {...acc, [`waypoint${reArrangeIndexes && lastIndex > -1 ? lastIndex + 1 : index}`]: currentWaypoint};
},
{},
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can refactor this code to be more readable.
How about changing to array first using _.filter and then back to object?
This won't require calculating key names in complex way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so will not preserve the keys. We want to not preserve the keys only when creating the distance request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so will not preserve the keys. We want to not preserve the keys only when creating the distance request.

I also commented here. But did you find any issue without preserving keys on Request money page before creating request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work. Ref:

Screen.Recording.2023-09-03.at.4.39.48.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the root cause? I thought waypoints is used to display routes, not validatedWaypoints

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The route is calculated via the validatedWaypoints. The response from the getRoute request gets stored into onyx. Since the response has the same keys as the waypoints supplied in the request body, therefore, the above error occurs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App/src/components/DistanceRequest.js

Line 174 in 388ddc8

{_.map(waypoints, (waypoint, key) => {

Copy link
Contributor Author

@allroundexperts allroundexperts Sep 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This waypoints come from the transaction object here which gets updated when a call to the API is made for getting the route here with validated waypoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I have simplified the logic for calculating the new indexes!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, confirmed. I suspected GetRoute api would also require rearranged keys but no need

response


return validWaypoints;
}

/*
Expand Down Expand Up @@ -325,7 +330,7 @@ export {
getAllReportTransactions,
hasReceipt,
isReceiptBeingScanned,
validateWaypoints,
getValidWaypoints,
isDistanceRequest,
hasMissingSmartscanFields,
};
2 changes: 1 addition & 1 deletion src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ function createDistanceRequest(report, participant, comment, created, transactio
createdChatReportActionID,
createdIOUReportActionID,
reportPreviewReportActionID: reportPreviewAction.reportActionID,
waypoints: JSON.stringify(transaction.comment.waypoints),
waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transaction.comment.waypoints, true)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think 2nd param is needed.
This will also be used in GetRoute api. And doesn't affect displaying routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this logic to reorganize indices to be consecutive (i.e. we need to pass true)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I meant to reorganize always without needing to pass 2nd param but later I realized it should be conditional

created,
},
onyxData,
Expand Down