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

front: fix row deletion in timestop input table #9739

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Synar
Copy link
Contributor

@Synar Synar commented Nov 16, 2024

Partial fix of #8959
-> IV. Multi selected fields modification is bugged
-> V. Delete the value of a stopping time field behaves weirdly

Prevents deletion of empty fields from creating a via

@Synar Synar requested a review from a team as a code owner November 16, 2024 18:29
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2024

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

Codecov Report

Attention: Patch coverage is 35.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 37.78%. Comparing base (8a2b4c9) to head (f584878).
Report is 63 commits behind head on dev.

Files with missing lines Patch % Lines
front/src/modules/timesStops/helpers/utils.ts 41.17% 10 Missing ⚠️
front/src/modules/timesStops/TimesStopsInput.tsx 0.00% 3 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9739      +/-   ##
==========================================
- Coverage   37.85%   37.78%   -0.07%     
==========================================
  Files         990      994       +4     
  Lines       90921    91210     +289     
  Branches     1176     1178       +2     
==========================================
+ Hits        34415    34462      +47     
- Misses      56052    56294     +242     
  Partials      454      454              
Flag Coverage Δ
editoast 72.97% <ø> (-0.32%) ⬇️
front 20.09% <35.00%> (-0.02%) ⬇️
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:

@Synar Synar force-pushed the ali/fix-row-content-deletion-in-timestop-input-table branch from 4e131ce to 07c4387 Compare November 17, 2024 16:15
Copy link
Contributor

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

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

Working perfectly, thanks !

@emersion
Copy link
Member

Hm, I must admit I'm not a huge fan of this workaround. Could we instead fix the function which checks whether two rows are equal, to consider that null is equivalent to undefined?

@Synar
Copy link
Contributor Author

Synar commented Nov 18, 2024

Hm, I must admit I'm not a huge fan of this workaround. Could we instead fix the function which checks whether two rows are equal, to consider that null is equivalent to undefined?

Right now we're just using lodash isEqual. I suppose we could call a normalizing function setting null to undefined (or both to false for stopSignal) on both rows before comparing. Would you prefer that?

@emersion
Copy link
Member

emersion commented Nov 18, 2024

We could do something like:

const { stopFor, theoreticalMargin, ...rest } = row;
return isEqual(rest, ) && stopFor ==  && theoreticalMargin == ;

Normalizing sounds fine to me as well.

@emersion
Copy link
Member

Another upside of normalizing is that we can stick to === so we don't need to think about the weird rules of ==.

Copy link
Contributor

@kmer2016 kmer2016 left a comment

Choose a reason for hiding this comment

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

LGTM !

@Synar Synar force-pushed the ali/fix-row-content-deletion-in-timestop-input-table branch from 77481df to 4b788ca Compare November 20, 2024 16:28
@Synar Synar requested a review from emersion November 21, 2024 09:36
if (!newRowData.stopFor && op.fromRowIndex !== allWaypointsLength - 1) {
if (
!newRowData.stopFor &&
newRowData.onStopSignal &&
Copy link
Member

Choose a reason for hiding this comment

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

Nit: does this change really matter? If we reset the field to false when it's already false, it's a no-op so should be fine as well?

(In general, I prefer keeping the list of conditions in a if as small as possible.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does because we also set null/undefined to false, which means we need to add the row to the path to remember it.

We could also put it in the format function, but here I think it makes more sense to directly avoid doing a modification rather than do it then drop it after the comparison

Signed-off-by: Alice Khoudli <alice.khoudli@polytechnique.org>
@Synar Synar force-pushed the ali/fix-row-content-deletion-in-timestop-input-table branch from 4b788ca to f584878 Compare November 22, 2024 15:56
@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Nov 22, 2024
@Synar Synar requested a review from emersion November 22, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants