-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add CI/CH/UIC codes next to station names #6180
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #6180 +/- ##
============================================
+ Coverage 26.66% 26.67% +0.01%
Complexity 2139 2139
============================================
Files 931 931
Lines 123397 123444 +47
Branches 2682 2682
============================================
+ Hits 32901 32927 +26
- Misses 88898 88919 +21
Partials 1598 1598
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f1874d4
to
5ef96c5
Compare
I understand this is the easier way to implement it, but ch codes aren't something international. Couldn't we use an enum of common location specifiers ? I believe the UIC has a list of CH equivalents |
9a38335
to
a464b17
Compare
@multun didn't we agree on implementing this that way for now and think about a more international-friendly model later ? We were already displaying those info in the search results, this PR is just supposed to spread this infos. I agree that it is not the target but this is a quick improvement. |
a464b17
to
7c6c5fb
Compare
I don't think we did agree: this change starts to hardcode assumptions about CH codes, which is pretty much documented nowhere on the internet. Can't we delay this a little bit, and take the time to make an internationally acceptable list of place types? I'm pretty sure I've seen one in UIC documentation. The plan would then be as follows:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments (some stylistic, some about factorizing code). But nothing blocking from merging.
front/src/modules/trainschedule/components/DriverTrainSchedule/DriverTrainScheduleStop.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/trainschedule/components/ManageTrainSchedule/Itinerary/DisplayVias.tsx
Outdated
Show resolved
Hide resolved
e9822c0
to
f695fdd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (peer reviewed), one small fix I've missed:
f695fdd
to
2e458fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
This plan should be kept in mind for later, but once again this task was to spread those info quickly to acess them in front end. We know it's not the target. It's a quick unperfect improvement. |
sure. I'm worried I'll be one more thing on a list of hacks we don't process as fast as we create. |
The purpose of this PR is to add context to the OPs. For the moment, we haven't generalized the trigram and CH code system. But this can be done later. During a side discussion, we agreed to carry out this task. However, it's important to ensure that the services work properly when the SNCF extension is not filled in. |
with german infra, CH and CI are null, everything works fine |
I created this issue to keep track of the leftover work. Feel free to add some details to it ! |
front/src/modules/trainschedule/components/DriverTrainSchedule/DriverTrainScheduleStop.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments :)
front/src/modules/trainschedule/components/ManageTrainSchedule/Itinerary/DisplayVias.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/trainschedule/components/ManageTrainSchedule/Itinerary/ModalSuggeredVias.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/trainschedule/components/ManageTrainSchedule/Itinerary/ModalSuggeredVias.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/trainschedule/components/ManageTrainSchedule/Itinerary/ModalSuggeredVias.tsx
Outdated
Show resolved
Hide resolved
bfa2f12
to
419861e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅ I didn't test but I believe the previous reviewers did it :)
editoast: pathfinding: add CI/CH codes to pathfinding response - add uic and ch to PathfindingResponse and PathWaypoint struct - get them from waypoint in Pathfinding impl - add ch to ResultStops struct, add_stops_additional_information fn and train_with_simulation_output_fixture_set fn - update openapi.yaml front: pathfinding: display CI/CH codes - create formatUicToCi in utils/strings.ts - add CI/CH codes in ModalSugerredVias, StationCard and DisplayVias - add CH code in AllowancesModalOP and DriverTrainScheduleStop - add elipsis to vias name in modal - replace suggered by suggested tests: - add ch and uic to steps in test_pathfinding.py
419861e
to
c7e60ec
Compare
editoast:
front: