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.

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

Add rolling stocks categories #10701

Merged
merged 2 commits into from
Feb 14, 2025
Merged

Conversation

hamz2a
Copy link
Contributor

@hamz2a hamz2a commented Feb 6, 2025

Part of #10574

Editoast:

  1. A new migration file was added to create a rolling_stock_category enum and update the rolling_stock table with the new fields primary_category and other_categories.
  2. New models (rolling_stock_category and rolling_stock_categories) were added to the editoast_models crate, as the goal is to migrate all models to this crate for better organization.
  3. The RollingStockModel struct was updated to include primary_category and other_categories.
  4. New test cases were added to check if primary_category and other_categories are correctly set when creating rolling stock.
  5. openapi.yaml was updated to reflect the new primary_category and other_categories fields in RollingStock.

Front:

  1. generatedEditoastApi.ts was updated to include the new RollingStockCategory and RollingStockCategories types.

osrd_schemas

  1. New enum RollingStockCategory was added
  2. RollingStock class was updated to include primary_category and other_categories.
  3. Update the test files

core

  1. updating rolling stock railjson version

@hamz2a hamz2a requested a review from a team as a code owner February 6, 2025 09:53
@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:editoast Work on Editoast Service labels Feb 6, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 68.75000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 81.93%. Comparing base (90dc639) to head (d4347b6).
Report is 25 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/editoast_models/src/tables.rs 55.00% 9 Missing ⚠️
...chemas/src/rolling_stock/rolling_stock_category.rs 25.00% 6 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10701      +/-   ##
==========================================
- Coverage   81.95%   81.93%   -0.03%     
==========================================
  Files        1083     1084       +1     
  Lines      107626   107212     -414     
  Branches      741      737       -4     
==========================================
- Hits        88203    87839     -364     
+ Misses      19382    19332      -50     
  Partials       41       41              
Flag Coverage Δ
editoast 74.37% <68.75%> (-0.03%) ⬇️
front 89.49% <ø> (+0.02%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.58% <ø> (+0.07%) ⬆️
tests 87.91% <ø> (ø)

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.

@hamz2a
Copy link
Contributor Author

hamz2a commented Feb 6, 2025

Hi @flomonster, I removed the generate_missing_sql_type_definitions = false configuration to allow Diesel to generate sql types automatically. Is that okay with you? Was there any particular reason for setting it to false?

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch 2 times, most recently from 3ce02b5 to dc40bee Compare February 6, 2025 10:01
@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from dc40bee to 611ace2 Compare February 6, 2025 10:16
@hamz2a hamz2a requested a review from a team as a code owner February 6, 2025 10:16
@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch 3 times, most recently from 159aa07 to ae4dbf2 Compare February 6, 2025 10:56
@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from ae4dbf2 to 87502e8 Compare February 6, 2025 12:19
@hamz2a hamz2a self-assigned this Feb 6, 2025
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

I'm not sure that I understand why we need to nest into Option? Apart from that, nice PR.

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from 87502e8 to 35f70ec Compare February 6, 2025 14:12
@hamz2a hamz2a requested a review from woshilapin February 6, 2025 14:13
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

Thanks for adding things in editoast_models, less things to do in the future!

Shouldn't the -Forms be updated as well with the categories?

Note

The commit and the PR title use "categorie" instead of "category/ies"

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from 35f70ec to 2f0caec Compare February 6, 2025 14:56
@hamz2a hamz2a requested a review from SharglutDev February 6, 2025 15:01
@hamz2a hamz2a changed the title editoast: add categorie to RollingStockModel editoast: add categories to RollingStockModel Feb 6, 2025
@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from 56cb9ea to fa838cd Compare February 6, 2025 15:04
@hamz2a
Copy link
Contributor Author

hamz2a commented Feb 6, 2025

The commit and the PR title use "categorie" instead of "category/ies"

Thanks for noticing that! I’ve fixed it now.

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from fa838cd to 6c47779 Compare February 6, 2025 15:13
@hamz2a
Copy link
Contributor Author

hamz2a commented Feb 6, 2025

Shouldn't the -Forms be updated as well with the categories?

As mentioned in this issue, this PR addresses Task 1, which involves adding two new columns to the model and updating the model in schemas. Task 2, which involves adding these columns to the endpoints, will be handled in a separate PR.

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from 400f053 to b06df50 Compare February 10, 2025 13:19
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 for core

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.

Thanks for your PR. There are a few things I'd like to clarify before I approve.

@flomonster flomonster changed the title editoast: add categories to RollingStockModel Add rolling stocks categories Feb 11, 2025
@hamz2a hamz2a requested a review from flomonster February 12, 2025 10:11
@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch 4 times, most recently from e76a492 to adbc99d Compare February 13, 2025 12:26
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 still do not agree with the unknown variant. If the other maintainers agree with this, then I'll yield.

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from adbc99d to 519b22f Compare February 13, 2025 15:57
@hamz2a
Copy link
Contributor Author

hamz2a commented Feb 13, 2025

I still do not agree with the unknown variant. If the other maintainers agree with this, then I'll yield.

@leovalais @woshilapin what do you think?
#10701 (comment)

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from 519b22f to b5b89c7 Compare February 14, 2025 09:50
@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch 4 times, most recently from 46ad103 to 0941476 Compare February 14, 2025 13:59
@hamz2a
Copy link
Contributor Author

hamz2a commented Feb 14, 2025

As discussed with @flomonster, @leovalais and @woshilapin: we will use SCREAMING_SNAKE_CASE format, remove the unknown, and set a default category chosen randomly.

@hamz2a hamz2a force-pushed the hai/editoast-add-categories-to-rolling-stock branch from 0941476 to d4347b6 Compare February 14, 2025 15:31
@hamz2a hamz2a requested a review from flomonster February 14, 2025 15:37
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.

LGTM

@hamz2a hamz2a added this pull request to the merge queue Feb 14, 2025
Merged via the queue into dev with commit cdfeff0 Feb 14, 2025
27 checks passed
@hamz2a hamz2a deleted the hai/editoast-add-categories-to-rolling-stock branch February 14, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core Work on Core Service area:editoast Work on Editoast Service area:front Work on Standard OSRD Interface modules area:integration-tests Work on Integration test, by nature related to different services area:railjson Work on Proposed Unified Rail Assets Data Exchange Format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants