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.

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: rides history page #867

Merged
merged 4 commits into from
Aug 30, 2024
Merged

fix: rides history page #867

merged 4 commits into from
Aug 30, 2024

Conversation

TomRytt
Copy link
Collaborator

@TomRytt TomRytt commented Aug 27, 2024

Description

Fixes issue #856 by adding the required "arrival_time_from" and "arrival_time_to" fields to the gtfs_ride_stops api call.
Changed the previous api call which utilized the gtfsAPI to an axios call directly to the server with the required data since the gtfsAPI whitelisted the required fields mentioned above which caused the api call to fail.

Still requires futher optimizations since the request is very slow and tests need to be written in future issues.

@NoamGaash

image
image

@TomRytt TomRytt requested a review from NoamGaash as a code owner August 27, 2024 16:02
Copy link
Contributor

github-actions bot commented Aug 27, 2024

@NoamGaash NoamGaash changed the title fix- #854 History page error by replacing GTFS_API with direct axios call fix: rides history page Aug 27, 2024
Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

looks amazing 👏
would you like some help with the linter?

Comment on lines 130 to 131
const maxEndTime = Math.max(...rides.map((ride) => Number(ride.endTime)))
const minStartTime = Math.min(...rides.map((ride) => Number(ride.startTime)))
Copy link
Member

Choose a reason for hiding this comment

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

just an idea - sometimes the actual ride happen before / after this range. maybe we should consider something like -

Suggested change
const maxEndTime = Math.max(...rides.map((ride) => Number(ride.endTime)))
const minStartTime = Math.min(...rides.map((ride) => Number(ride.startTime)))
const timeMargin = 1000 * 60 * 60 * 1.5 // 1.5 hours
const maxEndTime = Math.max(...rides.map((ride) => Number(ride.endTime))) + timeMargin
const minStartTime = Math.min(...rides.map((ride) => Number(ride.startTime))) + timeMargin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the quick response. I fixed the linter issues, added the correct type and added error & empty data error "handling" (can be improved) and submitted before seeing your replies. Would you like me to incorporate your suggestion and create another commit?

Copy link
Member

Choose a reason for hiding this comment

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

It's up to you, it was just a suggestion.
Thanks for fixing the lint issues!


return stopHits.sort((hit1, hit2) => +hit1.arrivalTime! - +hit2.arrivalTime!)
} catch (error) {
console.error(`Error fetching stop hits:`, error)
Copy link
Member

Choose a reason for hiding this comment

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

nice. I guess we can use some error boundary component to render a meaning full error message to the user

Comment on lines 130 to 131
const maxEndTime = Math.max(...rides.map((ride) => Number(ride.endTime)))
const minStartTime = Math.min(...rides.map((ride) => Number(ride.startTime)))
Copy link
Member

Choose a reason for hiding this comment

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

It's up to you, it was just a suggestion.
Thanks for fixing the lint issues!

@TomRytt TomRytt merged commit 2c9611e into hasadna:main Aug 30, 2024
13 checks passed
@NoamGaash
Copy link
Member

Thank you!
@all-contributors please add @TomRytt as code contributor

Copy link
Contributor

@NoamGaash

I've put up a pull request to add @TomRytt! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants