editoast: work-schedules: add endpoints for easier edition and viewing #9785

eckter · 2024-11-19T14:50:59Z

Would fix 2/3 of https://github.com/osrd-project/osrd-confidential/issues/838, the rest is changing the etl for batched imports.

I've added a few more endpoints than just what was in the ticket, to have a complete workflow:

POST /work_schedules/group: create a new group (name can be empty and autofilled (the unique name thing is just painful))
GET /work_schedules/group: lists all the group ids
PUT /work_schedules/group/42: insets a work schedule into an existing group
GET /work_schedules/group/42: lists all the work schedules in the group
DELETE /workçschedules/group/42: deletes the group and its work schedules

What I have not added but could have been nice:

More details in GET /work_schedules/group (name, creation date, ...)
GET /work_schedules to list them all
Any kind of endpoint dealing with individual work schedules

codecov-commenter · 2024-11-19T14:53:34Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.40426% with 39 lines in your changes missing coverage. Please review.

Project coverage is 37.89%. Comparing base (535d81d) to head (b27ef13).
Report is 33 commits behind head on dev.

Files with missing lines	Patch %	Lines
editoast/src/views/work_schedules.rs	84.26%	31 Missing ⚠️
front/src/common/api/generatedEditoastApi.ts	80.64%	6 Missing ⚠️
editoast/src/views/operational_studies.rs	66.66%	2 Missing ⚠️

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

Additional details and impacted files

@@           Coverage Diff            @@
##              dev    #9785    +/-   ##
========================================
  Coverage   37.88%   37.89%            
========================================
  Files         992      994     +2     
  Lines       90965    91403   +438     
  Branches     1176     1176            
========================================
+ Hits        34463    34635   +172     
- Misses      56048    56314   +266     
  Partials      454      454

Flag	Coverage Δ
editoast	`73.03% <83.82%> (-0.33%)`	⬇️
front	`20.13% <80.64%> (+0.01%)`	⬆️
gateway	`2.18% <ø> (ø)`
osrdyne	`3.28% <ø> (ø)`
railjson_generator	`87.49% <ø> (ø)`
tests	`86.74% <ø> (ø)`

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.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

front/public/locales/fr/errors.json

Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>

woshilapin

Nice work, just some comments about if we should do more transactional operations or not.

woshilapin · 2024-11-20T16:11:36Z

editoast/src/views/work_schedules.rs

@@ -26,19 +10,56 @@ use crate::views::path::projection::PathProjection;
 use crate::views::AuthenticationExt;
 use crate::views::AuthorizationError;
 use crate::AppState;
+use axum::extract::State;
+use axum::extract::{Json, Path};


nitpick: Can you unmerge the use, it's preferable for the diff (but hard to enforce in the formatter without being a pain for developers)

Fixed in f937a33

woshilapin · 2024-11-21T08:40:56Z

editoast/src/views/work_schedules.rs

+    let work_schedule_ids = WorkSchedule::list(conn, selection_setting)
+        .await?
+        .into_iter()
+        .map(|work_schedule| work_schedule.id);
+
+    // Delete them
+    use crate::models::DeleteBatch;
+    let conn = &mut db_pool.get().await?;
+    WorkSchedule::delete_batch(conn, work_schedule_ids).await?;
+
+    // Delete the group itself
+    WorkScheduleGroup::delete(&work_schedule_group, conn).await?;


Should we run all of this into a transaction (or at least the delete_batch and delete)? I wonder how much of a problem it is if remove all of the work schedule and miss the removal of the work schedule group? @eckter Any opinion? Feel free to resolve if you think that is not a problem.

I didn't even know transactions were a thing in this context 😄

I don't think it would be an issue, but it would still generally make sense to run a transaction. I'll do the change.

Fixed in f937a33

Please let me know if I did the transaction wrong, I did the same thing as in other places

If I am not mistaken in this comment the delete_batch and the transaction might not be needed. What do you think @woshilapin ?

woshilapin · 2024-11-21T09:18:26Z

editoast/src/views/work_schedules.rs

+    WorkScheduleGroup::retrieve_or_fail(conn, group_id, || {
+        WorkScheduleError::WorkScheduleGroupNotFound { id: group_id }
+    })
+    .await?;
+
+    // Create work schedules
+    let work_schedules_changesets = work_schedules
+        .into_iter()
+        .map(|work_schedule| work_schedule.into_work_schedule_changeset(group_id))
+        .collect::<Vec<_>>();
+    let work_schedules = WorkSchedule::create_batch(conn, work_schedules_changesets).await?;


Same here, maybe we need a transaction, because I'm not sure what happens if the work schedule group is deleted in between?

Fixed in f937a33

It would have created "orphaned" work schedules that couldn't be used, queried, or deleted

… viewing

leovalais

LGTM, a few non-blocking comments :)

editoast/src/views/work_schedules.rs

Sh099078

Great PR ! I added a few comments but mostly nitpicks

editoast/src/views/work_schedules.rs

SharglutDev

Lgtm for front part (not tested)

front/public/locales/en/errors.json

…ion and viewing

leovalais

LGTM, thanks for the changes!

editoast/src/views/work_schedules.rs

…ion and viewing Update editoast/src/views/work_schedules.rs Co-authored-by: Léo Valais <leo.valais97@gmail.com>

eckter force-pushed the ech/work-schedule-endpoints branch 5 times, most recently from 7525cf6 to 7cb0332 Compare November 19, 2024 15:44

github-actions bot added the area:editoast Work on Editoast Service label Nov 19, 2024

eckter force-pushed the ech/work-schedule-endpoints branch 6 times, most recently from 8f1bb19 to 5f2eca1 Compare November 20, 2024 14:28

eckter marked this pull request as ready for review November 20, 2024 14:47

eckter requested review from a team as code owners November 20, 2024 14:47

eckter requested review from emersion and woshilapin November 20, 2024 14:48

SharglutDev reviewed Nov 21, 2024

View reviewed changes

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

eckter force-pushed the ech/work-schedule-endpoints branch from 5f2eca1 to 53adda9 Compare November 21, 2024 08:52

editoast: work-schedules: add endpoints for easier edition and viewing

2c360cb

Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>

eckter force-pushed the ech/work-schedule-endpoints branch from 53adda9 to 2c360cb Compare November 21, 2024 08:55

woshilapin approved these changes Nov 21, 2024

View reviewed changes

fixup! editoast: work-schedules: add endpoints for easier edition and…

f937a33

… viewing

leovalais reviewed Nov 21, 2024

View reviewed changes

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

Sh099078 reviewed Nov 21, 2024

View reviewed changes

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Show resolved Hide resolved

SharglutDev approved these changes Nov 21, 2024

View reviewed changes

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

github-actions bot added the area:front Work on Standard OSRD Interface modules label Nov 22, 2024

eckter added 2 commits November 22, 2024 10:24

fixup! fixup! editoast: work-schedules: add endpoints for easier edit…

a1bd117

…ion and viewing

fixup! fixup! editoast: work-schedules: add endpoints for easier edit…

64d4756

…ion and viewing

fixup! fixup! editoast: work-schedules: add endpoints for easier edit…

61426f6

…ion and viewing

eckter force-pushed the ech/work-schedule-endpoints branch from 62011da to 61426f6 Compare November 22, 2024 09:43

fixup! fixup! editoast: work-schedules: add endpoints for easier edit…

dbeed01

…ion and viewing

eckter requested review from leovalais and woshilapin November 22, 2024 10:24

leovalais approved these changes Nov 22, 2024

View reviewed changes

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

editoast/src/views/work_schedules.rs Outdated Show resolved Hide resolved

eckter force-pushed the ech/work-schedule-endpoints branch from 2f59abc to fadd260 Compare November 22, 2024 12:30

fixup! fixup! editoast: work-schedules: add endpoints for easier edit…

b27ef13

…ion and viewing Update editoast/src/views/work_schedules.rs Co-authored-by: Léo Valais <leo.valais97@gmail.com>

eckter force-pushed the ech/work-schedule-endpoints branch from fadd260 to b27ef13 Compare November 22, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

editoast: work-schedules: add endpoints for easier edition and viewing #9785

editoast: work-schedules: add endpoints for easier edition and viewing #9785

eckter commented Nov 19, 2024 •

edited

Loading

codecov-commenter commented Nov 19, 2024 •

edited

Loading

woshilapin left a comment

woshilapin Nov 20, 2024

eckter Nov 21, 2024

woshilapin Nov 21, 2024

eckter Nov 21, 2024

eckter Nov 21, 2024 •

edited

Loading

Sh099078 Nov 22, 2024

woshilapin Nov 21, 2024

eckter Nov 21, 2024

leovalais left a comment

Sh099078 left a comment

SharglutDev left a comment •

edited

Loading

leovalais left a comment

editoast: work-schedules: add endpoints for easier edition and viewing #9785

Are you sure you want to change the base?

editoast: work-schedules: add endpoints for easier edition and viewing #9785

Conversation

eckter commented Nov 19, 2024 • edited Loading

codecov-commenter commented Nov 19, 2024 • edited Loading

Codecov Report

woshilapin left a comment

Choose a reason for hiding this comment

woshilapin Nov 20, 2024

Choose a reason for hiding this comment

eckter Nov 21, 2024

Choose a reason for hiding this comment

woshilapin Nov 21, 2024

Choose a reason for hiding this comment

eckter Nov 21, 2024

Choose a reason for hiding this comment

eckter Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Sh099078 Nov 22, 2024

Choose a reason for hiding this comment

woshilapin Nov 21, 2024

Choose a reason for hiding this comment

eckter Nov 21, 2024

Choose a reason for hiding this comment

leovalais left a comment

Choose a reason for hiding this comment

Sh099078 left a comment

Choose a reason for hiding this comment

SharglutDev left a comment • edited Loading

Choose a reason for hiding this comment

leovalais left a comment

Choose a reason for hiding this comment

eckter commented Nov 19, 2024 •

edited

Loading

codecov-commenter commented Nov 19, 2024 •

edited

Loading

eckter Nov 21, 2024 •

edited

Loading

SharglutDev left a comment •

edited

Loading