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

Move electrical profile set id from timetable to scenario #8167

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

Castavo
Copy link
Contributor

@Castavo Castavo commented Jul 24, 2024

Warning

For editoast please review only the first commit

The timetable model gets a ton of changes in editoast because the ModelV2 derive does not handle struct with only primary keys.
Indeed, the derive creates a TimetableV2Changeset struct, with no fields, and a struct with no field can not derive AsChangeset and Insertable from diesel.
With @leovalais we checked the following solutions:

  • implementing the diesel traits manually on TimetableV2Changeset: impossible due to the way they're made
  • update the derive so in the case there are only primary keys in the struct, we do not rely on a TimetableV2Changeset struct: this would complexify the derive a lot for a single and simple use, we deemed it overkill.
  • updating the derive so that you could pick which parts of the CRUD you wish to be automatically derived: most desirable but very havy in resources, should come at some point
  • implementing the ModelV2 traits manually: the solution we selected.

I removed the timetable listing endpoint because it was unused, because the List trait required the struct to be a Model (which it isn't anymore), and because a list of only ids did not seem very exploitable...

@Castavo Castavo requested review from Wadjetz and flomonster July 24, 2024 14:09
@Castavo Castavo force-pushed the bpt/editoast/move_electrical_profile_set_id branch from e5d181e to 4cc1899 Compare July 24, 2024 15:10
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.

Great PR. Not tested yet (waiting to be ready for review)

Copy link
Contributor

@younesschrifi younesschrifi 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 editoast part :)

@Castavo Castavo force-pushed the bpt/editoast/move_electrical_profile_set_id branch from 4cc1899 to 937fd4d Compare July 25, 2024 16:40
@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 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 (9f8d246) to head (fe27f60).
Report is 2 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    #8167       +/-   ##
=============================================
+ Coverage     27.25%   87.49%   +60.23%     
=============================================
  Files          1310       31     -1279     
  Lines        157453     1535   -155918     
  Branches       3259        0     -3259     
=============================================
- Hits          42915     1343    -41572     
+ Misses       112557      192   -112365     
+ Partials       1981        0     -1981     
Flag Coverage Δ
core ?
editoast ?
front ?
gateway ?
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.

Copy link
Member

@Wadjetz Wadjetz left a comment

Choose a reason for hiding this comment

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

LGTM

@Castavo Castavo force-pushed the bpt/editoast/move_electrical_profile_set_id branch from 91fb501 to 2a34c19 Compare July 26, 2024 10:19
@Castavo Castavo marked this pull request as ready for review July 27, 2024 12:53
@Castavo Castavo requested review from a team as code owners July 27, 2024 12:53
@Castavo Castavo requested review from bloussou and shenriotpro and removed request for bloussou July 27, 2024 12:53
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, (tested + reviewed)

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, not tested. An issue tracking the problem with ModelV2 is opened here: #8111

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.

OK for tests/

@Castavo Castavo self-assigned this Jul 29, 2024
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Thank you for this nice PR, left some comments (mostly for syntax and front good practices)

Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, one issue left

@Castavo Castavo changed the title Move electrical profile set id from scenario to timetable Move electrical profile set id from timetable to scenario Jul 31, 2024
@emersion
Copy link
Member

emersion commented Jul 31, 2024

Note to self: we're now passing the electrical set ID as query param in requests because a single timetable might be used from multiple scenarii.

@Castavo Castavo requested a review from SharglutDev July 31, 2024 10:05
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Apart from these minor comments, looks good!

@Castavo Castavo force-pushed the bpt/editoast/move_electrical_profile_set_id branch 2 times, most recently from 82b21d9 to b9e0ffa Compare July 31, 2024 10:34
@Castavo Castavo enabled auto-merge July 31, 2024 10:35
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm and tested, great job :)

@Castavo Castavo force-pushed the bpt/editoast/move_electrical_profile_set_id branch from b9e0ffa to 6ff702b Compare July 31, 2024 14:21
@Castavo Castavo force-pushed the bpt/editoast/move_electrical_profile_set_id branch from 6ff702b to fe27f60 Compare July 31, 2024 16:04
@Castavo Castavo added this pull request to the merge queue Jul 31, 2024
Merged via the queue into dev with commit 94c5c9f Jul 31, 2024
16 of 20 checks passed
@Castavo Castavo deleted the bpt/editoast/move_electrical_profile_set_id branch July 31, 2024 16:18
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.

10 participants