Issue #10 (Overlapping Rides) Resolved along with Issues #6, #8, #12 #14
Conversation
| @@ -64,9 +64,12 @@ const AllUserBookings = () => { | |||
| let apiURL = `${process.env.NEXT_PUBLIC_BACKEND_URL}/bookings`; | |||
|
|
|||
| if (fromValue && toValue) { | |||
| if(endTime < startTime) { | |||
There was a problem hiding this comment.
it is possible to filter rides based on just one time field, so please put a not null check for endTime and startTime and only then show the warning.
There was a problem hiding this comment.
Open this form in a new tab
| @@ -222,13 +224,26 @@ export function NewBookingDialog({ fetchUserBookings, username, email }) { | |||
| setToggle("from"); | |||
| }; | |||
|
|
|||
| const findOverlappingRides = () => { | |||
| router.push({ | |||
There was a problem hiding this comment.
since you are using query strings to pass props, on clearing the filters in AllUserBookings, the query string is not cleared. Hence, on reloading the page or just switching tabs fills the filters again. (i.e. filters stay there forever, unless someone removes them from the query string)
|
|
|
Hi, Ideally, we would like to have one PR per every unrelated issue. Having one PR for multiple unrelated issues makes it quite a bit harder to review. Could you possibly try raising one per Issue? That way, we can land some changes while discussing/reviewing other changes. Since you already have changes but all in one branch, you can look at creating required branches from these commits itself if they are separated by concern . You can take a look at cherry picking which might help. However, if there is one commit for all four issues as it seems like, can you please try to separate out them instead? |
| @@ -79,8 +82,8 @@ const AllUserBookings = () => { | |||
| apiURL += `?from_loc=${fromValue}&to_loc=${toValue}`; | |||
| } | |||
| } else if (startTime || endTime) { | |||
| const isoStartTime = startTime?.toISOString(); | |||
| const isoEndTime = endTime?.toISOString(); | |||
| const isoStartTime = startTime ? new Date(startTime).toISOString() : null; | |||
There was a problem hiding this comment.
Is this necessary?
I think the optional chaining present here should automatically handle it?
| const isoStartTime = startTime?.toISOString(); | ||
| const isoEndTime = endTime?.toISOString(); | ||
| const isoStartTime = startTime ? new Date(startTime).toISOString() : null; | ||
| const isoEndTime = endTime ? new Date(endTime).toISOString() : null; |
There was a problem hiding this comment.
Are these dependency changes required for this issue? If not, please address them separately
I've resolved the overlapping rides issue #10 by checking for overlapping rides once the user fills the form for a new ride. If overlapping rides exist, then the user is asked if he wants to check those overlapping rides or continue booking the new ride. If he chooses to check overlapping rides, then he's redirected back to "All Rides" tab wherein the filters of his requirement are auto-applied. If he chooses to still continue booking, then a new ride is created once he clicks the Book button.
Note : This branch contains commits of previous PR as well wherein I resolved Issues #6 , #8 , #12 Hence, it has all of these issues resolved together.