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

editoast, schemas: add 'freight_compatible' to the rolling stock model #8856

Closed
wants to merge 4 commits into from

Conversation

woshilapin
Copy link
Contributor

@woshilapin woshilapin commented Sep 12, 2024

Classify rolling stock between those that can be used as fret, and those who don't (or those we don't know). A new field freight_compatible is added to the rolling stock schema (both python osrd_schemas and editoast), then also to the rolling stock model (DB), and finally in all API exposed struct like LightRollingStock and RollingStockForm.

Since there is still a lot of rolling stock for which we don't know any information, the new field is nullable.

⚠️ First thing to review: what do you think of the naming freight_compatible. If you don't like it, have other ideas, don't spend time reviewing the rest of the PR, let's discuss that first.

I also added a little icon for freight compatible train in the rolling stock editor.

image

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2024

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.49%. Comparing base (68e0513) to head (78b1852).
Report is 6 commits behind head on dev.

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

Additional details and impacted files
@@              Coverage Diff              @@
##                dev    #8856       +/-   ##
=============================================
+ Coverage     37.13%   87.49%   +50.35%     
=============================================
  Files          1260       31     -1229     
  Lines        115076     1535   -113541     
  Branches       3230        0     -3230     
=============================================
- Hits          42736     1343    -41393     
+ Misses        70407      192    -70215     
+ Partials       1933        0     -1933     
Flag Coverage Δ
core ?
editoast ?
front ?
gateway ?
osrdyne ?
railjson_generator 87.49% <ø> (ø)
tests ?

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.

@woshilapin woshilapin force-pushed the wsl/feat/editoast/multiple-units branch 4 times, most recently from a3132aa to 314c132 Compare September 18, 2024 14:13
@woshilapin woshilapin force-pushed the wsl/feat/editoast/multiple-units branch from 314c132 to 011e52e Compare September 19, 2024 10:05
@woshilapin woshilapin marked this pull request as ready for review September 19, 2024 12:53
@woshilapin woshilapin requested review from a team as code owners September 19, 2024 12:53
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.

LGTM. I'm fine with freight_compatible. About its nullability: do we plan to make the field non-nullable eventually or is there a benefit to encode "we're not sure" in the application?

@woshilapin woshilapin force-pushed the wsl/feat/editoast/multiple-units branch from 011e52e to 2450cf9 Compare September 19, 2024 14:14
@woshilapin woshilapin requested a review from a team as a code owner September 19, 2024 14:14
@woshilapin woshilapin changed the title editoast, schemas: add 'train_type' to the rolling stock model editoast, schemas: add 'freight_compatible' to the rolling stock model Sep 19, 2024
@woshilapin woshilapin force-pushed the wsl/feat/editoast/multiple-units branch from e79759d to 5c3faac Compare September 19, 2024 14:37
@flomonster
Copy link
Contributor

I would have preferred an enum field like transport_type.

With these possible values:

@woshilapin
Copy link
Contributor Author

woshilapin commented Sep 19, 2024

I would have preferred an enum field like transport_type.

With these possible values:

* `freight`

* `passenger`

* `construction` for stuff like [tamping machines](https://en.wikipedia.org/wiki/Tamping_machine)

* ...

@flomonster I see, but I don't think that is ok, because it's not an XOR. A locomotive can be both freight and passenger compatible. A better modeling would be some sort of list of tags... but that might mean another DB table and maybe a bit more design. What do you think?

Copy link
Contributor

@shenriotpro shenriotpro left a comment

Choose a reason for hiding this comment

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

Thanks

@flomonster
Copy link
Contributor

I see, but I don't think that is ok, because it's not an XOR. A locomotive can be both freight and passenger compatible. A better modeling would be some sort of list of tags... but that might mean another DB table and maybe a bit more design. What do you think?

I see, why do we need this information attached to the rolling stock? Is it only for the icon?

@woshilapin
Copy link
Contributor Author

woshilapin commented Sep 19, 2024

I see, why do we need this information attached to the rolling stock? Is it only for the icon?

@flomonster No, we're going to use it to filter what is presented in the STDCM form, see #8921 for the following PR. The icon was just a quick win to display this information somewhere.

@flomonster
Copy link
Contributor

I see, why do we need this information attached to the rolling stock? Is it only for the icon?

No, we're going to use it to filter what is presented in the STDCM form, see #8921 for the following PR. The icon was just a quick win to display this information somewhere.

I think the best way to deal with this in the long term is to have groups of rolling stock (necessary for permissions). Then we could assign a group of rolling stock to the STDCM search environment.

It feels weird to add a flaky field and limit usage of STDCM depending of this field.

Note

Since we want this feature quickly and we can't wait for rolling stock groups, should we add an hotfix in the frontend? It seems easier to revert than changing our model.

@woshilapin
Copy link
Contributor Author

LGTM. I'm fine with freight_compatible. About its nullability: do we plan to make the field non-nullable eventually or is there a benefit to encode "we're not sure" in the application?

@leovalais I actually went back and forth on this idea, first thinking, if we don't know, we just don't know, it's not necessary false. But then I realize that on STDCM form, we're going to have to take a decision about "do we display rolling stocks that are null or not?" and the answer is not clear. So I made the change to simplify the code in 78b1852.

Copy link
Contributor

@clarani clarani 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 the front part ✅

@woshilapin
Copy link
Contributor Author

Finally closing this PR. After discussion with multiple actors (@clarani @flomonster @axrolld), we realized that the vision is not yet clear and that we can achieve our short-term objective differently: we can have much less technical debt by encoding what we need directly in front without modifying our internal backend model.

@woshilapin woshilapin closed this Sep 23, 2024
@woshilapin woshilapin deleted the wsl/feat/editoast/multiple-units branch October 8, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants