all: handle volt unit for electrification in data only #6229

bougue-pe · 2023-12-28T12:24:55Z

Front does not add 'V' as unit anymore (but some hard-coded front processing is not tackled and just migrated, see #6235 for a part of it).
The responsibility for unit is on the data side (rolling stock, catenaries, electrical profiles).

Hand-tested (cards, edit and curves on rolling-stock, legend and catenaries on map, traction on study's speed-space graph, simple stdcm use):

OK to load a dump, migrate and use (prints 1500V and colors)
OK to load a dump and use without migration, so with identical "wrong" values everywhere (front crashes on speed-space graph as expected for traction without 'V', otherwise prints 1500 black)

osrd part of https://github.com/osrd-project/osrd-dags/issues/135 (osrd-dags not migrated yet, both should be merged at the same time once done).

shenriotpro

OK for tests/

codecov · 2023-12-28T12:49:36Z

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (bf6da3d) 27.66% compared to head (cd790d9) 27.90%.
Report is 10 commits behind head on dev.

Files	Patch %	Lines
...ionResult/components/SpeedSpaceChart/sampleData.ts	0.00%	21 Missing ⚠️
front/src/common/Map/Layers/Electrifications.tsx	0.00%	15 Missing ⚠️
...rc/applications/editor/tools/rangeEdition/tools.ts	0.00%	4 Missing ⚠️
front/src/applications/editor/tools/switchProps.ts	0.00%	4 Missing ⚠️
...t/components/ChartHelpers/drawElectricalProfile.ts	0.00%	4 Missing ⚠️
...ront/src/applications/operationalStudies/consts.ts	86.36%	3 Missing ⚠️
.../ManageTrainSchedule/PowerRestrictionsSelector.tsx	0.00%	2 Missing ⚠️
editoast/src/converters/utils.rs	94.73%	1 Missing ⚠️
...src/applications/editor/components/EntitySumUp.tsx	0.00%	1 Missing ⚠️
...k/components/RollingStockCard/RollingStockCard.tsx	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #6229      +/-   ##
============================================
+ Coverage     27.66%   27.90%   +0.24%     
- Complexity     2136     2162      +26     
============================================
  Files           990     1000      +10     
  Lines        125902   126347     +445     
  Branches       2575     2578       +3     
============================================
+ Hits          34832    35259     +427     
- Misses        89580    89599      +19     
+ Partials       1490     1489       -1

Flag	Coverage Δ
core	`78.66% <ø> (-0.22%)`	⬇️
editoast	`75.79% <96.00%> (+0.20%)`	⬆️
front	`8.70% <28.75%> (+<0.01%)`	⬆️
gateway	`2.50% <ø> (ø)`
railjson_generator	`87.42% <ø> (ø)`
tests	`81.77% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Castavo

Works fine !

I ran the migration successfully, the run time with our current dump seems very acceptable.

I tested the cartography, RS editor and Electrification in the GEV, all seem to work fine after the migration.
I did not check before migration.

front/public/locales/fr/simulation.json

front/public/locales/fr/translation.json

clarani

LGTM, tested with the migration ✅
Tested the rs editor, the infra editor, the reference map, the rs selector and the gev

front/public/locales/en/simulation.json

front/public/locales/en/translation.json

editoast/migrations/2023-12-26-152659_add_electrification_unit/up.sql

Tguisnet

Lgtm

Erashin

Core part lgtm.

Front does not add 'V' as unit anymore (but some hard-coded front processing is not tackled and just migrated). The responsibility for unit is on the data side (rolling stock, catenaries, electrical profiles). Railjson generation from OSM is updated accordingly. Hand-tested (cards, edit and curves on rolling-stock, legend and catenaries on map, traction on study's speed-space graph): - OK to load a dump, migrate and use - OK to load a dump and use without migration (front crashes on speed-space graph as expected for traction)

Castavo

I checked with the last data dump, works fine !

I tested map, RS editor and simulation with electrical profiles.

bougue-pe requested review from a team as code owners December 28, 2023 12:24

bougue-pe requested a review from multun December 28, 2023 12:24

shenriotpro approved these changes Dec 28, 2023

View reviewed changes

bougue-pe requested a review from Khoyo December 28, 2023 14:25

bougue-pe mentioned this pull request Dec 29, 2023

front: handle electrification colors in a unified, stable and "agnostic" way #6235

Open

4 tasks

bougue-pe requested a review from Castavo January 3, 2024 09:57

Castavo approved these changes Jan 3, 2024

View reviewed changes

front/public/locales/fr/simulation.json Outdated Show resolved Hide resolved

front/public/locales/fr/translation.json Outdated Show resolved Hide resolved

clarani approved these changes Jan 4, 2024

View reviewed changes

front/public/locales/en/simulation.json Outdated Show resolved Hide resolved

front/public/locales/en/translation.json Outdated Show resolved Hide resolved

bougue-pe changed the title ~~all: handle 'V' unit for electrification in data only~~ all: handle volt unit for electrification in data only Jan 4, 2024

bougue-pe requested review from clarani and Castavo January 5, 2024 08:46

bougue-pe force-pushed the peb/all/v_for_electrification_in_data_only branch from 8adfa05 to fc8419e Compare January 8, 2024 06:35

bougue-pe requested a review from a team as a code owner January 8, 2024 06:35

Tguisnet requested changes Jan 8, 2024

View reviewed changes

editoast/migrations/2023-12-26-152659_add_electrification_unit/up.sql Outdated Show resolved Hide resolved

bougue-pe requested a review from Tguisnet January 11, 2024 09:39

Tguisnet approved these changes Jan 11, 2024

View reviewed changes

bougue-pe force-pushed the peb/all/v_for_electrification_in_data_only branch from 2b7b4cb to f1f0b89 Compare January 11, 2024 10:02

Erashin approved these changes Jan 11, 2024

View reviewed changes

flomonster removed the request for review from multun January 11, 2024 11:27

bougue-pe force-pushed the peb/all/v_for_electrification_in_data_only branch 3 times, most recently from cad1fdf to afc290e Compare January 12, 2024 09:00

bougue-pe force-pushed the peb/all/v_for_electrification_in_data_only branch from afc290e to cd790d9 Compare January 12, 2024 11:52

Castavo approved these changes Jan 12, 2024

View reviewed changes

bougue-pe added this pull request to the merge queue Jan 12, 2024

Merged via the queue into dev with commit 3374298 Jan 12, 2024
20 checks passed

bougue-pe deleted the peb/all/v_for_electrification_in_data_only branch January 12, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: handle volt unit for electrification in data only #6229

all: handle volt unit for electrification in data only #6229

bougue-pe commented Dec 28, 2023 •

edited

Loading

shenriotpro left a comment

codecov bot commented Dec 28, 2023 •

edited

Loading

Castavo left a comment

clarani left a comment

Tguisnet left a comment

Erashin left a comment

Castavo left a comment

all: handle volt unit for electrification in data only #6229

all: handle volt unit for electrification in data only #6229

Conversation

bougue-pe commented Dec 28, 2023 • edited Loading

shenriotpro left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 28, 2023 • edited Loading

Codecov Report

Castavo left a comment

Choose a reason for hiding this comment

clarani left a comment

Choose a reason for hiding this comment

Tguisnet left a comment

Choose a reason for hiding this comment

Erashin left a comment

Choose a reason for hiding this comment

Castavo left a comment

Choose a reason for hiding this comment

bougue-pe commented Dec 28, 2023 •

edited

Loading

codecov bot commented Dec 28, 2023 •

edited

Loading