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

Fix simulation_summary being called too often #8004

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

anisometropie
Copy link
Contributor

@anisometropie anisometropie commented Jul 10, 2024

  • removing train_schedulev2 tag from postV2TrainScheduleProjectPath and making the endpoint a query (codegen made it a mutation by defaut as it is a POST)
  • keeping train_schedulev2 postV2TrainScheduleSimulationSummary => the endpoint should but called again if one train is updated (more refinement to come Moving a train in space-time chart trigger simulation summary and projection unnecessarily #8010 ). It should not if a train is created/removed, but we can handle that manually with trainResultToFetch logic

closes #7989

@anisometropie anisometropie requested review from a team as code owners July 10, 2024 14:54
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

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

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes missing coverage. Please review.

Project coverage is 28.06%. Comparing base (a9a3ddc) to head (766b2fa).
Report is 4 commits behind head on dev.

Files Patch % Lines
front/src/config/openapi-editoast-config.ts 0.00% 5 Missing ⚠️
...nt/src/applications/stdcm/hooks/useStdcmResults.ts 0.00% 2 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff            @@
##                dev    #8004   +/-   ##
=========================================
  Coverage     28.05%   28.06%           
  Complexity     2081     2081           
=========================================
  Files          1289     1289           
  Lines        157727   157708   -19     
  Branches       3114     3116    +2     
=========================================
+ Hits          44254    44257    +3     
+ Misses       111598   111576   -22     
  Partials       1875     1875           
Flag Coverage Δ
core 75.11% <ø> (-0.09%) ⬇️
editoast 70.76% <ø> (-0.03%) ⬇️
front 9.90% <22.22%> (+<0.01%) ⬆️
gateway 2.34% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 73.18% <ø> (+0.24%) ⬆️

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.

@emersion
Copy link
Member

Instead of changing the OpenAPI definition side, we can override the auto-generated providesTags/invalidatesTags here: https://github.com/OpenRailAssociation/osrd/blob/dev/front/src/common/api/osrdEditoastApi.ts

@flomonster flomonster changed the title front: fix /simulation_summary being called too often Fix simulation_summary being called too often Jul 11, 2024
@anisometropie anisometropie force-pushed the vcs/unnecessary-call-to-simulation-sumary branch 2 times, most recently from 4c902b0 to 4dd33f7 Compare July 11, 2024 13:14
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

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.

We should maybe connect the trainResultToFetch state to the hook containing the summary endpoint to be sure that when we add a train, we fetch only the summary/getTrainSchedule of the new train and not the others that didn't changed.
Same when deleting a train.

@anisometropie anisometropie force-pushed the vcs/unnecessary-call-to-simulation-sumary branch 2 times, most recently from 089249c to 9c80ff5 Compare July 17, 2024 22:14
@SharglutDev
Copy link
Contributor

SharglutDev commented Jul 18, 2024

Thank you for the changes. Regarding my last comment, we have fresh news.

A high priority issue has just came up. Most of the logic of timetable, simulation result will be changed (including trainResultToFetch) so don't bother to make these changes.

You just have the build to fix and we can approve this PR I think :)

…ainScheduleProjectPath a query.

Codegen makes all the POSTs mutations by default. This one is only a POST to have a body, but is not creating/modifying anything in the DB.
No reason to be considered a mutation on the frontend.

As a query it doesn't have an invalidatesTags array anymore and prevents the unnecessary calls to simulation_summary.
@anisometropie anisometropie force-pushed the vcs/unnecessary-call-to-simulation-sumary branch from 9c80ff5 to 766b2fa Compare July 18, 2024 09:48
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, thank you !

@anisometropie anisometropie added this pull request to the merge queue Jul 18, 2024
@anisometropie anisometropie removed this pull request from the merge queue due to a manual request Jul 18, 2024
@anisometropie anisometropie added this pull request to the merge queue Jul 18, 2024
Merged via the queue into dev with commit 608d35b Jul 18, 2024
20 checks passed
@anisometropie anisometropie deleted the vcs/unnecessary-call-to-simulation-sumary branch July 18, 2024 10:47
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.

Simulation summary endpoint is called unnecessarily
5 participants