Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix speed limit restriction #6436

Merged
merged 1 commit into from
Feb 2, 2024
Merged

fix speed limit restriction #6436

merged 1 commit into from
Feb 2, 2024

Conversation

kmer2016
Copy link
Contributor

@kmer2016 kmer2016 commented Jan 24, 2024 •
edited
Loading

Closes #6407

  • Editoast should return an error when the speed is less than 0
    • add validation when Deserialize SpeedSection
    • add check on infra endpoints (edition and loading)
  • Core should return an error when the speed is less than 0
  • migration to transform all 0 speed to null

@codecov Codecov
Copy link

codecov bot commented Jan 24, 2024 •
edited
Loading

Codecov Report

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

Comparison is base (2292d0d) 28.01% compared to head (802e1de) 28.03%.

Files Patch % Lines
front/src/reducers/editor/index.ts 0.00% 6 Missing ⚠️
...a/fr/sncf/osrd/reporting/exceptions/OSRDError.java 0.00% 3 Missing ⚠️
...ts/InfraSelector/InfraSelectorModalBodyEdition.tsx 0.00% 3 Missing ⚠️
.../osrd/infra/api/tracks/undirected/SpeedLimits.java 77.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev    #6436      +/-   ##
============================================
+ Coverage     28.01%   28.03%   +0.02%     
- Complexity     2167     2169       +2     
============================================
  Files          1034     1034              
  Lines        127681   127745      +64     
  Branches       2602     2603       +1     
============================================
+ Hits          35764    35813      +49     
- Misses        90403    90417      +14     
- Partials       1514     1515       +1
Flag Coverage Δ
core 78.53% <61.53%> (-0.02%) ⬇️
editoast 75.50% <100.00%> (+0.03%) ⬆️
front 8.61% <0.00%> (-0.01%) ⬇️
gateway 2.50% <ø> (ø)
railjson_generator 87.25% <ø> (ø)
tests 81.95% <ø> (ø)

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.

@kmer2016 kmer2016 force-pushed the cnh-fix-speed-limit-restriction branch 4 times, most recently from 1ba6994 to 1df9d83 Compare January 30, 2024 13:51
flomonster
flomonster reviewed Jan 31, 2024
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the editoast part. It seems like the initial approach of using validator::Validate will be heavy. My comments are aligned with this approach.

We can discuss implementing manually Deserialize instead of Validate.

@kmer2016 kmer2016 force-pushed the cnh-fix-speed-limit-restriction branch 2 times, most recently from 7c1f8c8 to 0239ca1 Compare February 1, 2024 14:31
@kmer2016 kmer2016 marked this pull request as ready for review February 1, 2024 14:34
@kmer2016 kmer2016 requested review from a team as code owners February 1, 2024 14:34
@kmer2016 kmer2016 requested a review from eckter February 1, 2024 14:34
flomonster
flomonster approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small fix and we're good to merge it!

@kmer2016 kmer2016 force-pushed the cnh-fix-speed-limit-restriction branch from 0239ca1 to f485eef Compare February 1, 2024 14:48
@kmer2016 kmer2016 mentioned this pull request Feb 1, 2024
Closed
4 tasks
Math-R
Math-R approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@Math-R Math-R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM =)

eckter
eckter approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@eckter eckter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kmer2016
core: check speedLimit greater than 0
802e1de 
editoast: check speedLimit greater than 0

editoast: migration speed_limit to null instead of 0

editoast: add speed_section validation on edit infra and load railjson endpoints

core: fix test

editoast: fix migration remove entries in speed_limit_by_tag where value <= 0

core: fix test

editoast: fix lint error

front: display error message when speed limit validation fail

editoast: fix lint

core: refacto. validation on RJSpeedSection

editoast: change validation strategy

editoast: clean
@kmer2016 kmer2016 force-pushed the cnh-fix-speed-limit-restriction branch from f485eef to 802e1de Compare February 2, 2024 09:11
@kmer2016 kmer2016 added this pull request to the merge queue Feb 2, 2024
Merged via the queue into dev with commit 33b4dc7 Feb 2, 2024
22 checks passed
@kmer2016 kmer2016 deleted the cnh-fix-speed-limit-restriction branch February 2, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eckter eckter eckter approved these changes

@flomonster flomonster flomonster approved these changes

@Math-R Math-R Math-R approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Speed limit edition and usage with no value fails
4 participants
@kmer2016 @eckter @flomonster @Math-R