front: heavily refactor graou open data import #14517
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Synar
force-pushed
the
ali/fix-refactor-graou-import
branch
from
b3ac4b4 to
de39021
Compare
Synar
force-pushed
the
ali/fix-refactor-graou-import
branch
from
de39021 to
48472c8
Compare
Contributor
|
Can we wait for this PR to be merged before reviewing this one ? |
This is a step in centralizing all graou parsing logic, in order to improve code maintainability and enable us to streamline the import process. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
This is a step in centralizing all graou parsing logic, in order to improve code maintainability and enable us to streamline the import process. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
This is a step in centralizing all graou parsing logic, in order to improve code maintainability and enable us to streamline the import process. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
This is a step in centralizing all graou parsing logic, in order to improve code maintainability and enable us to streamline the import process. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
This is a step in centralizing all graou parsing logic, in order to improve code maintainability and enable us to streamline the import process. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
The only difference between trainList (used for Graou) and trainJson (used for all other sources) import was that trainList contained train schedules that were only partially parsed, as we performed some processing of Graou trains before filling the list and some after. There is however no good reason not to fully parse Graou trains before filling the list, and fuse trainList into trainJson. This lets us both: - simplify the properties and logic of ImportTimetableItemTrainsList - streamline the process of parsing Graou trains and avoid exposing intermediate results Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Transforming the uic by filling the checksum is a parsing/formatting operation, which naturally belongs with other parsing/formatting operations rather than in the api call. Furthermore, it makes sense to do it right before converting the GraouTrainSchedule string uic to a TrainSchedule number uic. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
This reduce the number of functions exposed by parseGraouTrains, making the parsing more streamlined and self contained. The final goal would be to only expose generateTrainSchedulesPayloads as far as parsing/formatting is concerned. Additionally drops an unnecessary loop. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Conmputing stopFor duration at the same time as arrival and departure times makes sense, and enables us to remove a large loop and function call while streamlining the import process. Also use Duration.subtractDate and .toISOString instead of manual computations for standardization. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Contribute to centralizing all graou parsing logic. Also simplify the rs matching logic, drop a custom type and avoid type casting. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Finish adding doc strings to functions that were lacking them. Also move populateUicChecksum next to other internal parsing functions. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
While previous commits attempted to centralize parsing logic, this commit goal is to centralize validation logic, which is mostly done in the api call. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Wrapping everything in a single try/catch/finally makes the function more robust (parsing failures can occur) and streamlined. The "if (filteredTrains && !isEmpty(filteredTrains))" condition was unnecessary and was dropped. Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Signed-off-by: Alice K. <alice.khoudli@gmail.com>
- move all the validation logic inside graouApi - be more resilient to errors by filtering invalid schedules and steps instead of throwing, as graou api erros are frequent - add toast warning for filtered schedules - fix the translation strings for the filtered steps toast warning Signed-off-by: Alice K. <alice.khoudli@gmail.com>
Synar
force-pushed
the
ali/fix-refactor-graou-import
branch
from
48472c8 to
ccbf4e7
Compare
Contributor
Author
I was on vacation, but now I suppose the question is moot haha. |
Contributor
|
To keep everyone updated, we will soon lose access to graou api so the feature shall be removed from osrd. @Synar will soon add a commit to remove everything concerning it from the codebase. |
Member
|
Why will we loose access? |
Member
@nicolaswurtz Informed me that the service will drop (migrate) soon. In any case, we will no longer have access to the endpoint. And we should think about a more standard way of retrieving open source data (e.g. GTFS). |
github-project-automation
bot
moved this from Awaiting merge
to Awaiting Validation
in Board PI 18
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Graou train schedules parsing logic was spread amongst 5 different files, with functions called at various points of the importing process, without any unifying convention.
In particular, this meant we had to carry a TrainLists parameter on top of the TrainFromJson one in ImportTimetableItemList just for the purpose of storing half-formatted graou train schedules.
Instead :
I'm not 100% sure about where the validation logic should live, and how strict we want to be with it. For example, should we really reject all train schedules because one uic was missing?
Also close #10159 by querying all chs for each step, as is done in many other places in the front. The fixup commit at the end is an alternative version of the code (which I favor, but does require a new type). Both commits can be extracted to another pr if you wish.
For reviewing, I suggest taking a look at the overall new code flow in ImportTimetableItemConfig as well as parseGraouTrains, and then go commit by commit. I tried to be as atomic as possible (perhaps too much?). Each commit compiles and runs.
Next step could be to write a couple unit tests, at least for the overall Graou import.
Edit : added a last commit that improve and standardize validation, see the commit text for details. It can be moved to another pr if you wish.