front: nge saving node's positions

[sim51]

Related to https://github.com/osrd-project/osrd-confidential/issues/597

What is done :

• [X] Editoast API
• [X] Sync between OSRD/NGE
• [X] Using a layout algo for node's position
• [X] On load, deduplicate nodes
• [x] Creating new nodes in NGE
• [x] testing import from viriato, basic & opendata
• [x] check the trainrun modification

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 96.15070% with 47 lines in your changes missing coverage. Please review.

Project coverage is 80.15%. Comparing base (29b4005) to head (84d327f). Report is 93 commits behind head on dev.

Files with missing lines	Patch %	Lines
editoast/src/views/scenario/macro_nodes.rs	92.05%	27 Missing :warning:
front/src/common/api/generatedEditoastApi.ts	60.97%	16 Missing :warning:
editoast/src/models/tags.rs	76.92%	3 Missing :warning:
editoast/src/views/pagination.rs	75.00%	1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #9105      +/-   ##
==========================================
+ Coverage   80.10%   80.15%   +0.04%     
==========================================
  Files        1054     1057       +3     
  Lines      105648   106171     +523     
  Branches      759      759              
==========================================
+ Hits        84631    85100     +469     
- Misses      20976    21029      +53     
- Partials       41       42       +1     

Flag	Coverage Δ
editoast	74.43% <91.90%> (+0.17%)	:arrow_up:
front	89.29% <98.09%> (+<0.01%)	:arrow_up:
gateway	2.18% <ø> (ø)
osrdyne	3.28% <ø> (ø)
railjson_generator	87.50% <ø> (ø)
tests	87.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[leovalais]

Also is it OK to link Viriato.json and basic.json publicly on GitHub?

[sim51]

@leovalais can you check again ? Normally I did all the requested changes. Thanks!

[leovalais]

Tested the feature, a few questions:

• The coordinates of the nodes seem to always be integers, yet the table columns are floats (unlike what's stated in the design ticket). The only time I could get a float to be stored was when making a non-placement modification (name, labels, ...) of a node that was placed algorithmically. Even then the value was something like 324.99xxxxxx. Could the coordinates be rounded so that we only have to deal with integers on editoast side?

• The deletion of a train schedule can lead to persisted "ghost nodes" in the DB. These should probably be removed at the same time.

1. Create two schedules A and B
2. Open NGE
3. Move all 4 nodes around to persist them.
4. Go back to the Micro tab
5. Delete schedule A
6. Reopen NGE
7. The nodes of A aren't shown anymore, but still exist in the DB, making them impossible to remove from the UI

This also has a side-effect: if you re-create the circulation, any metadata from the old nodes (tags, name) becomes automatically attached to the nodes of the new schedule.

• From the Micro tab, how do you create a schedule so that the nodes representing it in NGE use either op_id:X or trigram:X? I only could create nodes with uic:X and track_offset:X (but that's probably a skill issue on my end tbh).

[sim51]

Good point for the deletion !

I did a fix, which is not perfect : when you open the macro tab , a call is done to retrieve all the saved nodes. If a saved node is not part of a train schedule, I delete it.

Like you said, best will be to enhance the removeTrains function of the hook useScenarioData. When a train is remove and one of if its node is not used anymore, we check if it is saved , and if so we delete it.

Said like this it's easy, but the hard point is that we handle the fusion of nodes. A node with an item_path_key "trigram:XXX" and an other one with "uic:XXXX" can be the same, but we only store the one with the "uic". If the deleted TS implies to delete the "trigram:XXX", I need to know that in fact It's "UIC:XXXX" that should be deleted ... Moreover, to know if a node is useless, I need to rebuild the NGE graph (which takes times and do some API calls).

For your others points :

• I change the coordinates types from float to integer on DB, editoast, ...
• to create a node with a trigram, you can just create a new node on NGE
• to create a node with a op_id, I did a basic import

[flomonster]

* From the `Micro` tab, how do you create a schedule so that the nodes representing it in NGE use either `op_id:X` or `trigram:X`? I only could create nodes with `uic:X` and `track_offset:X` (but that's probably a skill issue on my end tbh).

You can't fromt the interface, but you can import train schedules from json files with "invalid" op_id or trigram.

[flomonster]

* The deletion of a train schedule can lead to persisted "ghost nodes" in the DB. These should probably be removed at the same time.

1. Create two schedules A and B
2. Open NGE
3. Move all 4 nodes around to persist them.
4. Go back to the `Micro` tab
5. Delete schedule A
6. Reopen NGE
7. The nodes of A aren't shown anymore, but still exist in the DB, making them impossible to remove from the UI

This also has a side-effect: if you re-create the circulation, any metadata from the old nodes (tags, name) becomes automatically attached to the nodes of the new schedule.

I don't know for sure what behaviour we should expect here. In my mind it was not such a big deal to keep "ghosty" nodes. Maybe we should ask to @louisgreiner and @maelysLeratRosso.

[leovalais]

In my mind it was not such a big deal to keep "ghosty" nodes.

If that requires a mountain of work probably not. But we should at least try to keep the DB as clean as we can.

Said like this it's easy, but the hard point is that we handle the fusion of nodes.

It's probably not worth it if the automatic cleanup when opening the Macro tab is good enough. I feel like it's fine to have some edge cases where the nodes aren't deleted, but we should try to delete most of them when it's not too much work.

• to create a node with a trigram, you can just create a new node on NGE
• to create a node with a op_id, I did a basic import You can't fromt the interface, but you can import train schedules from json files with "invalid" op_id or trigram.

Thanks for the clarifications, I'll try that when I'll test the PR again.

---

@flomonster: https://github.com/OpenRailAssociation/osrd/pull/9105#discussion_r1813057760

[maelysLeratRosso]

Sorry for the delay, I missed the message on Friday...

The idea was to keep the NGE board just as the user left it, so if he doesn't explicitely remove a node, we should keep it (see AC in https://github.com/osrd-project/osrd-confidential/issues/597). So if a node is present in NGE, it is persisted and not removed until the user decides to do so. In Viriato for example, users first create their nodes and then add trains on it. The nodes are the "framework" of the macro board and are handled by the user only. For now, we create the nodes depending on the trains represented, but it might change later. So it is important that we don't remove nodes automatically.

@flomonster @leovalais

[flomonster]

If I get it right with @leovalais and @maelysLeratRosso comments.

• The only case where we delete a node is when the node is explicitely deleted by the user.

This behaviour is okay for me. It makes me ask a new question, should we still show persisted nodes in NGE even there not part of a train schedule. If yes we can still do it in another PR.

[sim51]

@clarani & @emersion I just pushed a commit that fix your feedbacks.

There is one comment I don't get it : This is missing the secondary_code. I don't know what you mean by that. The secondary_code is used to build the trigram. Probably I missed something, but then I need your help to fix it.

[emersion]

There is one comment I don't get it : This is missing the secondary_code. I don't know what you mean by that. The secondary_code is used to build the trigram. Probably I missed something, but then I need your help to fix it.

See the definition of PathItemLocation: there is a ch/secondary_code in the case of a trigram and in the case of a UIC. The code here only handles secondary_code for trigrams, and ignores them for UICs.

[flomonster]

I'm wondering if we really should build on top of the /project/id/study/id/... API though. If I recall correctly, this URL scheme was chosen because we wanted to replace the IDs with slugs. But that was almost two years ago and we're not planning to implement this feature anytime soon as far as I know. And even if we had it, it's not great in terms of API design to not be able to access an object by its ID directly. (We're also likely to boil down to the resource ID for the permission checks anyway, so we might as well have the endpoint and redirect from the slug chain.)

I suggest getting rid of the prefixes and simply access these endpoints at /macro_node and /macro_node/{id}. This would simplify the implementation as well. Wdyt? (ping @flomonster)

It's true that we've been talking about it for a long time. In my opinion, the most important thing is to be consistent in our API. So I suggest leaving it as it is for this PR and making a refacto ticket that will give more people a chance to give their opinion on the matter.

[emersion]

I've refactored your code to keep both sides separate here: https://github.com/OpenRailAssociation/osrd/compare/emr/nge-save-node

I've tried to keep this as close as possible to your code, only moving code around but otherwise not changing the logic.

[sim51]

@emersion & @clarani

I review the way how I stored the nodes in the state and their relative indices. So now a node has a ngeId & dbId, and methods have the corresponding suffix (ex: getNodeByNgeId)

Normally this refacto solved all your last comments, except two which are not critical, the index of labels and the rename of a field in the database.

BTW, I found & fixed a bug when you want to delete a project or a study.