editoast: front: adapt nge node path item key - #18105
Conversation
github-actions
Bot
added
area:front
5030db5 to
9be44aa
Compare
DucNg
left a comment
DucNg
left a comment
There was a problem hiding this comment.
This fixes import/export feature in NGE
| ...indexNode, | ||
| ...castNgeNode(node, netzgrafikDto.labels), | ||
| trigram: node.betriebspunktName, | ||
| trigram: domesticReference, |
There was a problem hiding this comment.
Same as #18035 (comment): this creates a desync between NGE's current state and the database state.
There was a problem hiding this comment.
We have to overwrite the user input. This is not ideal but is required to have an import/export working as expected.
I can update the NGE state directly so we don't have to refresh.
There was a problem hiding this comment.
We should never overwrite the NGE state in response to an NGE change. Overwriting user input is also bad practice.
There was a problem hiding this comment.
Again, I agree, it's not ideal. I'm not defending this behaviour. I'm trying to fix the import/export since @maelysLeratRosso considers the current issue not acceptable for a release.
How can we fix the import/export without overriding the trigram's node (which is what we currently do)?
We could adapt the NGE export:
- Adding country code information
- We need to be compatible with the original format
- This should be done in our fork, I guess 🤷
Do you think it's a reasonable solution?
There was a problem hiding this comment.
We already do it for secondary code actually
There was a problem hiding this comment.
I think it is better like this until we have a propre way to search among infrastructure OPs from NGE ?
| DELETE FROM macro_node WHERE path_item_key LIKE 'domestic:FR-%'; | ||
| UPDATE macro_node SET path_item_key = 'domestic:FR-' || SUBSTRING(path_item_key, 9) WHERE path_item_key LIKE 'trigram:%'; | ||
| DELETE FROM macro_node WHERE path_item_key LIKE 'domestic:%:FR'; | ||
| UPDATE macro_node SET path_item_key = 'domestic:' || SUBSTRING(path_item_key, 9) || '#FR', trigram = trigram || '#FR' WHERE path_item_key LIKE 'trigram:%'; |
There was a problem hiding this comment.
Should we update the column trigram as well when path_item_key is not a trigram ?
There was a problem hiding this comment.
It will be done by NGE at the opening of the scenario.
There was a problem hiding this comment.
This is false: we don't overwrite the trigram field at NGE load time. (This PR overwrites it on node update only, see the thread above.)
8538864 to
5442af8
Compare
The idea is to add the country code at the end of the path_item_key. In addition we adapt nodes trigram so the import/export works again. Signed-off-by: Florian Amsallem <florian.amsallem@gmail.com>
5442af8 to
9831fe0
Compare
| test('Verify NGE train data', async ({ ngePage }) => { | ||
| await test.step('Verify nodes displayed on NGE graph', async () => { | ||
| await ngePage.expectNodes(['SWS/BV', 'MWS/BV', 'MES/BV']); | ||
| await ngePage.expectNodes(['SWS/BV#…', 'MWS/BV#…', 'MES/BV#…']); |
There was a problem hiding this comment.
The test still fails here: it expects MWS/BV… instead of MWS/BV#…
Shouldn’t the country code be displayed instead of ...?
There was a problem hiding this comment.
Fixed in 7cd6922
Displaying the country code instead of "…" requires changing the ellipsis behavior inside NGE so it needs to be done in a distinct PR.
There was a problem hiding this comment.
I think the ellipsis is ok for now. We didn't really want to display the country code at first, we just have to put it in the trigram field. So it's good that we don't see it too much :)
7 participants
The idea is to add the country code at the end of the path_item_key. In addition, we adapt nodes trigram so the import/export works again.
Note: I adapted an existing migration; it's normal since we are adapting a fix. It's not a big deal for people who already ran this migration.